Explain file discovery ranking and clarify live gate#70
Conversation
📝 WalkthroughWalkthroughThis PR adds structured match metadata to file discovery results and makes integration test timeouts configurable. ChangesCodexFS File Discovery Match Metadata
Configurable Integration Test Timeouts
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50d17e0dbc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if pathMatch.hasBoundaryMatch { | ||
| reasons.append(.init(kind: .pathBoundaryMatch, value: 8)) | ||
| } |
There was a problem hiding this comment.
rankingReasons is documented as carrying per-signal score contributions, but pathBoundaryMatch is emitted as a fixed +8 whenever any boundary match occurs, while the boundary bonus is already folded into subsequenceScore (and may be applied multiple times for one match). For queries that hit several boundaries (for example acronyms across path segments), the exposed reason values become inconsistent with the actual score and can mislead UIs that explain or total contributions from rankingReasons.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
README.md (1)
50-50: ⚡ Quick winSplit this usage paragraph into shorter bullets for readability.
The content is valuable, but this single line is hard to scan and feels maintainer-dense for first-time users.
As per coding guidelines, "Keep the README product-facing and approachable for package users and their agents".Proposed doc-only rewrite
-Use `CodexAppServer.fs` when a sandboxed client needs filesystem metadata, directory listings, file bytes, file discovery, fuzzy file lookup, or file-change watches through the Codex app-server instead of reading local disk directly. File-discovery hits include match kind, matched character ranges, and ranking reasons for picker highlighting and result explanations. `CodexWorkspace` carries app-server-owned workspace permission selections, active permission-profile provenance, and runtime filesystem/network permission facts for started threads and turns. Use `CodexAppServer.config` for effective config reads, and `CodexAppServer.extensions` for app, skill, plugin, and collaboration-mode inventory. +Use `CodexAppServer.fs` when a sandboxed client needs filesystem metadata, directory listings, file bytes, discovery, fuzzy lookup, or file-change watches without reading local disk directly. + +Discovery hits include match kind, matched character ranges, and ranking reasons so picker UIs can highlight and explain ordering. + +Use: +- `CodexWorkspace` for workspace permission selections, permission-profile provenance, and runtime filesystem/network permission facts. +- `CodexAppServer.config` for effective config reads. +- `CodexAppServer.extensions` for app, skill, plugin, and collaboration-mode inventory.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` at line 50, Split the long single-sentence paragraph into a short bulleted list for readability: create separate bullets for (1) CodexAppServer.fs responsibilities (filesystem metadata, directory listings, file bytes, file discovery, fuzzy file lookup, file-change watches via the app-server), and note that file-discovery hits include match kind, matched character ranges, and ranking reasons for picker highlighting; (2) CodexWorkspace purpose (app-server-owned workspace permission selections, active permission-profile provenance, and runtime filesystem/network permission facts for started threads and turns); (3) configuration access via CodexAppServer.config for effective config reads; and (4) inventory via CodexAppServer.extensions for app, skill, plugin, and collaboration-mode listings — keep each bullet short and user-facing.ROADMAP.md (1)
1226-1228: 💤 Low valueMinor wording inconsistency in completed backlog item.
The word "soon" is leftover from when this was a TODO. Now that it's marked complete
[x], it reads awkwardly as "Add richer ... search metadata soon".Consider removing "soon" for clarity:
-- [x] Add richer `CodexFS.FileDiscoveryHit` search metadata soon, including +- [x] Add richer `CodexFS.FileDiscoveryHit` search metadata, including🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ROADMAP.md` around lines 1226 - 1228, The completed checklist item "Add richer `CodexFS.FileDiscoveryHit` search metadata soon, including match kind, matched ranges, or ranking reason once UI highlighting needs an explicit public model." reads awkwardly; remove "soon" and update tense to match the checked state—e.g., change to "Added richer `CodexFS.FileDiscoveryHit` search metadata, including match kind, matched ranges, or ranking reason" or simply remove "soon" to read "Add richer `CodexFS.FileDiscoveryHit` search metadata, including...".
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Tests/SwiftASBTests/Public/CodexAppServerLiveIntegrationTestSupport.swift`:
- Around line 2211-2219: The liveTimeoutSeconds(default:) function currently
accepts positive Double values from the environment but doesn't exclude
infinity; update the guard in liveTimeoutSeconds to require seconds.isFinite (in
addition to seconds > 0) so that infinite values from
ProcessInfo.processInfo.environment["SWIFTASB_LIVE_CODEX_TIMEOUT_SECONDS"] are
rejected and the defaultSeconds returned, preventing overflow when the value is
later converted to nanoseconds (see withTimeout(seconds:operation:) and the
UInt64(seconds * 1_000_000_000) conversion).
---
Nitpick comments:
In `@README.md`:
- Line 50: Split the long single-sentence paragraph into a short bulleted list
for readability: create separate bullets for (1) CodexAppServer.fs
responsibilities (filesystem metadata, directory listings, file bytes, file
discovery, fuzzy file lookup, file-change watches via the app-server), and note
that file-discovery hits include match kind, matched character ranges, and
ranking reasons for picker highlighting; (2) CodexWorkspace purpose
(app-server-owned workspace permission selections, active permission-profile
provenance, and runtime filesystem/network permission facts for started threads
and turns); (3) configuration access via CodexAppServer.config for effective
config reads; and (4) inventory via CodexAppServer.extensions for app, skill,
plugin, and collaboration-mode listings — keep each bullet short and
user-facing.
In `@ROADMAP.md`:
- Around line 1226-1228: The completed checklist item "Add richer
`CodexFS.FileDiscoveryHit` search metadata soon, including match kind, matched
ranges, or ranking reason once UI highlighting needs an explicit public model."
reads awkwardly; remove "soon" and update tense to match the checked state—e.g.,
change to "Added richer `CodexFS.FileDiscoveryHit` search metadata, including
match kind, matched ranges, or ranking reason" or simply remove "soon" to read
"Add richer `CodexFS.FileDiscoveryHit` search metadata, including...".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ba5d4341-558b-49b6-9a48-92c9dc7b1ecd
📒 Files selected for processing (10)
CONTRIBUTING.mdREADME.mdROADMAP.mdSources/SwiftASB/Public/CodexFS.swiftSources/SwiftASB/SwiftASB.docc/CodexFS.mdTests/SwiftASBTests/Public/CodexAppServerFileSystemTests.swiftTests/SwiftASBTests/Public/CodexAppServerLiveIntegrationTestSupport.swiftTests/SwiftASBTests/Public/CodexAppServerLiveIntegrationTests.swiftscripts/run-live-codex-integration-tests.shscripts/run-live-codex-release-gate.sh
| func liveTimeoutSeconds(default defaultSeconds: Double) -> Double { | ||
| guard let rawValue = ProcessInfo.processInfo.environment["SWIFTASB_LIVE_CODEX_TIMEOUT_SECONDS"], | ||
| let seconds = Double(rawValue), | ||
| seconds > 0 | ||
| else { | ||
| return defaultSeconds | ||
| } | ||
| return seconds | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C2 'SWIFTASB_LIVE_CODEX_TIMEOUT_SECONDS|Double\\(rawValue\\)|seconds > 0|UInt64\\(seconds \\* 1_000_000_000\\)' --type=swiftRepository: gaelic-ghost/SwiftASB
Length of output: 893
🏁 Script executed:
rg -n 'liveTimeoutSeconds' Tests/SwiftASBTests/Public/CodexAppServerLiveIntegrationTestSupport.swift -A 10Repository: gaelic-ghost/SwiftASB
Length of output: 2250
🏁 Script executed:
rg -n '1_000_000_000|nanosecond|timeout' Tests/SwiftASBTests/Public/CodexAppServerLiveIntegrationTestSupport.swift --type=swiftRepository: gaelic-ghost/SwiftASB
Length of output: 1433
🏁 Script executed:
sed -n '2195,2210p' Tests/SwiftASBTests/Public/CodexAppServerLiveIntegrationTestSupport.swiftRepository: gaelic-ghost/SwiftASB
Length of output: 520
🏁 Script executed:
python3 << 'EOF'
import sys
# Test if UInt64(Double.infinity * multiplier) crashes
try:
result = int(float('inf') * 1_000_000_000)
print(f"Result: {result}")
except Exception as e:
print(f"Error: {type(e).__name__}: {e}")
# Also test conversion directly
try:
result = int(float('inf'))
print(f"Direct inf to int: {result}")
except Exception as e:
print(f"Error on direct conversion: {type(e).__name__}: {e}")
EOFRepository: gaelic-ghost/SwiftASB
Length of output: 212
Add isFinite check before returning timeout value to prevent infinity values from crashing in nanosecond conversion.
The liveTimeoutSeconds() function accepts any positive Double from environment parsing, including infinity (if set to "inf"). This value is later passed to withTimeout(seconds:operation:) at line 1718/1734/1755/1773, which converts it to nanoseconds via UInt64(seconds * 1_000_000_000) at line 2201. Converting infinity to UInt64 crashes with an overflow error.
The seconds > 0 check alone is insufficient because infinity > 0 evaluates to true. Add seconds.isFinite to the guard condition.
Proposed fix
func liveTimeoutSeconds(default defaultSeconds: Double) -> Double {
guard let rawValue = ProcessInfo.processInfo.environment["SWIFTASB_LIVE_CODEX_TIMEOUT_SECONDS"],
let seconds = Double(rawValue),
+ seconds.isFinite,
seconds > 0
else {
return defaultSeconds
}
return seconds
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func liveTimeoutSeconds(default defaultSeconds: Double) -> Double { | |
| guard let rawValue = ProcessInfo.processInfo.environment["SWIFTASB_LIVE_CODEX_TIMEOUT_SECONDS"], | |
| let seconds = Double(rawValue), | |
| seconds > 0 | |
| else { | |
| return defaultSeconds | |
| } | |
| return seconds | |
| } | |
| func liveTimeoutSeconds(default defaultSeconds: Double) -> Double { | |
| guard let rawValue = ProcessInfo.processInfo.environment["SWIFTASB_LIVE_CODEX_TIMEOUT_SECONDS"], | |
| let seconds = Double(rawValue), | |
| seconds.isFinite, | |
| seconds > 0 | |
| else { | |
| return defaultSeconds | |
| } | |
| return seconds | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Tests/SwiftASBTests/Public/CodexAppServerLiveIntegrationTestSupport.swift`
around lines 2211 - 2219, The liveTimeoutSeconds(default:) function currently
accepts positive Double values from the environment but doesn't exclude
infinity; update the guard in liveTimeoutSeconds to require seconds.isFinite (in
addition to seconds > 0) so that infinite values from
ProcessInfo.processInfo.environment["SWIFTASB_LIVE_CODEX_TIMEOUT_SECONDS"] are
rejected and the defaultSeconds returned, preventing overflow when the value is
later converted to nanoseconds (see withTimeout(seconds:operation:) and the
UInt64(seconds * 1_000_000_000) conversion).
1 participant
Summary
Verification
Summary by CodeRabbit
Release Notes
New Features
Documentation
Chores