Repository navigation
Make empty web test discovery version-stable - #8946
lawrencecchen wants to merge 11 commits into
Conversation
📝 WalkthroughWalkthroughCodex hook handling now uses shared schemas and content-addressed script names, stricter launch sanitization, stdin-draining no-op commands, and expanded regression coverage. Web tests now run through a shared isolated runner with recursive discovery and CI guards. ChangesCodex hook lifecycle
Shared web test runner
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CMUXCLI
participant CodexHookScriptName
participant Codex
participant AgentLaunchSanitizer
CMUXCLI->>CodexHookScriptName: derive content-addressed filename
CodexHookScriptName-->>CMUXCLI: return canonical script name
CMUXCLI->>Codex: emit schema-based hook arguments
Codex->>AgentLaunchSanitizer: provide launch arguments
AgentLaunchSanitizer-->>Codex: remove recognized cmux hook block
Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (22 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
Greptile SummaryThis PR centralizes Codex hook schema and script identity handling while introducing a shared, isolated web-test runner.
Confidence Score: 4/5The PR is safe to merge with non-blocking issues in web-test discovery parity and generated-hook ownership recognition. The web guard does not exactly mirror Bun's supported test names, and Codex hook cleanup treats a syntactically valid generated-script filename as ownership proof without validating its content. Files Needing Attention: web/scripts/run-tests.sh; Packages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/CodexHookScriptName.swift Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
A["bun run test"] --> B["scripts/run-tests.sh"]
B --> C{"Explicit arguments?"}
C -->|Yes| D["bun test --isolate arguments"]
C -->|No| E["Pre-scan default test files"]
E -->|Found| F["bun test --isolate"]
E -->|None| G["Stable cmux diagnostic and exit 1"]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| *) continue ;; | ||
| esac | ||
| case "${file##*/}" in | ||
| *".test"* | *"_test_"* | *".spec"* | *"_spec_"*) return 0 ;; |
There was a problem hiding this comment.
The basename substrings do not match Bun's default filename rules: *_test_* misses a valid name such as example_test.ts, while *.test* accepts a helper such as example.test-helper.ts. This can reject a valid default suite or delegate an actually empty suite back to Bun's version-dependent behavior.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
Superseded by the concurrently updated #8891 at |
![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: cb19b85071
ℹ️ 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 ! PATH="$fixture_dir/bin:/usr/bin:/bin" \ | ||
| CMUX_WEB_TEST_RUNNER_ARGS_LOG="$args_log" \ | ||
| /bin/bash "$fixture_runner"; then |
There was a problem hiding this comment.
This invokes the new runner with no arguments, but the fixture contains only web/scripts/run-tests.sh and the fake bun executable. Consequently, default_test_file_exists exits before the fake Bun is called; running bash tests/test_ci_self_hosted_guard.sh produces cmux web test runner found no test files followed by FAIL: shared web test runner default discovery should execute. Since .github/workflows/ci.yml runs this script in workflow-guard-tests, that required job now fails on every run; create a correctly named fixture test before invoking the runner.
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: 6
🤖 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 `@CLI/CMUXCLI`+AgentHookDefinitions.swift:
- Around line 437-444: Update isCmuxOwnedCodexHookScriptCommand to obtain the
Codex hooks directory path without calling codexHookScriptsDirectory(), since
that helper creates the directory as a side effect. Reuse or introduce a
non-creating path resolver for the ownership check, preserving the existing
filename and standardized-path comparisons.
In `@cmuxTests/CmuxConfigActionSaverTests.swift`:
- Around line 497-506: Strengthen the assertions around the generated Codex hook
arguments so they independently verify that hook stripping still removes the
stop-event hook, rather than relying only on the hardcoded
“cmux-codex-hook-stop.sh” name. Use schema-derived data or another
schema-independent invariant, while preserving the existing assertions for the
current stop subcommand.
In
`@Packages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/AgentLaunchSanitizerCodexLaunch.swift`:
- Around line 255-283: Update cmuxCodexHookScriptFilename to reject commands
containing token separators or arguments before returning a hook filename;
validate that normalizedCommand is exactly one complete, recognizable hook
script path under .cmux/hooks, including the expected hook-script
naming/extension, while preserving valid paths with interior component spaces.
In
`@Packages/macOS/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/CodexHookInjectionStrippingTests.swift`:
- Around line 578-584: The schema-derived fixture in
Packages/macOS/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/CodexHookInjectionStrippingTests.swift#L578-L584
must become an independent literal list for codexWrapperHookEvents, matching the
pinned legacySavedLayoutHookEvents and legacyAliasHookEvents fixtures so schema
changes require explicit recognized entries. In
cmuxTests/CmuxConfigActionSaverTests.swift#L497-L506, retain the schema-derived
fixture but update the relevant assertions to check for “cmux-codex-hook” and
“--enable hooks” instead of the hardcoded stop filename, preventing vacuous
passes.
In `@tests/test_ci_self_hosted_guard.sh`:
- Around line 869-900: Update check_web_test_runner_behavior to create a minimal
valid test fixture under fixture_dir before invoking fixture_runner, ensuring
the runner passes its discovery guard and delegates to the Bun stub. Keep the
existing args_log assertion and cleanup behavior unchanged.
In `@web/scripts/run-tests.sh`:
- Around line 7-20: Update default_test_file_exists to mirror Bun’s discovery
rules exactly: restrict matching to Bun-supported test filename patterns, avoid
partial matches such as foo.test-helper.ts, and prune every ignored node_modules
and hidden directory during find traversal, including nested paths. Preserve the
function’s existing return behavior by returning success only when a
Bun-discoverable test file is found.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 18f14d07-ea5b-4a12-b350-b94228db8b6a
📒 Files selected for processing (16)
.github/workflows/ci.ymlCLI/CMUXCLI+AgentHookDefinitions.swiftCLI/CMUXCLI+CodexFireAndForgetHooks.swiftPackages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/AgentLaunchSanitizerCodexLaunch.swiftPackages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/CodexHookInjectionEvent.swiftPackages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/CodexHookInjectionSchema.swiftPackages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/CodexHookScriptName.swiftPackages/macOS/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/CodexHookInjectionStrippingTests.swiftPackages/macOS/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/CodexHookScriptNameTests.swiftcmuxTests/CLICodexHookTimeoutRegressionTestSupport.swiftcmuxTests/CLICodexHookTimeoutRegressionTests.swiftcmuxTests/CmuxConfigActionSaverTests.swifttests/test_ci_self_hosted_guard.shweb/package.jsonweb/scripts/run-tests.shweb/tests/web-test-runner-isolation.test.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 6
🤖 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 `@CLI/CMUXCLI`+AgentHookDefinitions.swift:
- Around line 437-444: Update isCmuxOwnedCodexHookScriptCommand to obtain the
Codex hooks directory path without calling codexHookScriptsDirectory(), since
that helper creates the directory as a side effect. Reuse or introduce a
non-creating path resolver for the ownership check, preserving the existing
filename and standardized-path comparisons.
In `@cmuxTests/CmuxConfigActionSaverTests.swift`:
- Around line 497-506: Strengthen the assertions around the generated Codex hook
arguments so they independently verify that hook stripping still removes the
stop-event hook, rather than relying only on the hardcoded
“cmux-codex-hook-stop.sh” name. Use schema-derived data or another
schema-independent invariant, while preserving the existing assertions for the
current stop subcommand.
In
`@Packages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/AgentLaunchSanitizerCodexLaunch.swift`:
- Around line 255-283: Update cmuxCodexHookScriptFilename to reject commands
containing token separators or arguments before returning a hook filename;
validate that normalizedCommand is exactly one complete, recognizable hook
script path under .cmux/hooks, including the expected hook-script
naming/extension, while preserving valid paths with interior component spaces.
In
`@Packages/macOS/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/CodexHookInjectionStrippingTests.swift`:
- Around line 578-584: The schema-derived fixture in
Packages/macOS/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/CodexHookInjectionStrippingTests.swift#L578-L584
must become an independent literal list for codexWrapperHookEvents, matching the
pinned legacySavedLayoutHookEvents and legacyAliasHookEvents fixtures so schema
changes require explicit recognized entries. In
cmuxTests/CmuxConfigActionSaverTests.swift#L497-L506, retain the schema-derived
fixture but update the relevant assertions to check for “cmux-codex-hook” and
“--enable hooks” instead of the hardcoded stop filename, preventing vacuous
passes.
In `@tests/test_ci_self_hosted_guard.sh`:
- Around line 869-900: Update check_web_test_runner_behavior to create a minimal
valid test fixture under fixture_dir before invoking fixture_runner, ensuring
the runner passes its discovery guard and delegates to the Bun stub. Keep the
existing args_log assertion and cleanup behavior unchanged.
In `@web/scripts/run-tests.sh`:
- Around line 7-20: Update default_test_file_exists to mirror Bun’s discovery
rules exactly: restrict matching to Bun-supported test filename patterns, avoid
partial matches such as foo.test-helper.ts, and prune every ignored node_modules
and hidden directory during find traversal, including nested paths. Preserve the
function’s existing return behavior by returning success only when a
Bun-discoverable test file is found.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 18f14d07-ea5b-4a12-b350-b94228db8b6a
📒 Files selected for processing (16)
.github/workflows/ci.ymlCLI/CMUXCLI+AgentHookDefinitions.swiftCLI/CMUXCLI+CodexFireAndForgetHooks.swiftPackages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/AgentLaunchSanitizerCodexLaunch.swiftPackages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/CodexHookInjectionEvent.swiftPackages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/CodexHookInjectionSchema.swiftPackages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/CodexHookScriptName.swiftPackages/macOS/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/CodexHookInjectionStrippingTests.swiftPackages/macOS/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/CodexHookScriptNameTests.swiftcmuxTests/CLICodexHookTimeoutRegressionTestSupport.swiftcmuxTests/CLICodexHookTimeoutRegressionTests.swiftcmuxTests/CmuxConfigActionSaverTests.swifttests/test_ci_self_hosted_guard.shweb/package.jsonweb/scripts/run-tests.shweb/tests/web-test-runner-isolation.test.ts
🛑 Comments failed to post (6)
CLI/CMUXCLI+AgentHookDefinitions.swift (1)
437-444: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Ownership predicate creates
~/.cmux/hooksas a side effect.
codexHookScriptsDirectory()callscreateDirectory(withIntermediateDirectories:), so this read-only check writes to the filesystem — including during uninstall/prune scans where the directory may have just been removed. Resolve the path without creating it here.🤖 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 `@CLI/CMUXCLI`+AgentHookDefinitions.swift around lines 437 - 444, Update isCmuxOwnedCodexHookScriptCommand to obtain the Codex hooks directory path without calling codexHookScriptsDirectory(), since that helper creates the directory as a side effect. Reuse or introduce a non-creating path resolver for the ownership check, preserving the existing filename and standardized-path comparisons.cmuxTests/CmuxConfigActionSaverTests.swift (1)
497-506: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assertions can go vacuous if the schema's
stopsubcommand is renamed.The fixture is now schema-derived, but Lines 361 and 377 still assert against the hardcoded
"cmux-codex-hook-stop.sh". IfCodexHookInjectionSchema.currentrenames that subcommand, the substring is never generated and both assertions pass while verifying nothing. Add a schema-independent assertion so the test keeps failing when stripping breaks.💚 Suggested strengthening
- `#expect`(nativeCommand?.contains("cmux-codex-hook-stop.sh") == false) + `#expect`(nativeCommand?.contains("cmux-codex-hook") == false) + `#expect`(nativeCommand?.contains("--enable hooks") == false)🤖 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 `@cmuxTests/CmuxConfigActionSaverTests.swift` around lines 497 - 506, Strengthen the assertions around the generated Codex hook arguments so they independently verify that hook stripping still removes the stop-event hook, rather than relying only on the hardcoded “cmux-codex-hook-stop.sh” name. Use schema-derived data or another schema-independent invariant, while preserving the existing assertions for the current stop subcommand.Packages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/AgentLaunchSanitizerCodexLaunch.swift (1)
255-283: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Show the full helper and any other callers that trust its result. ast-grep outline Packages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/AgentLaunchSanitizerCodexLaunch.swift --items all rg -nP --type=swift -C3 'cmuxCodexHookScriptFilename' PackagesRepository: manaflow-ai/cmux
Length of output: 3329
🏁 Script executed:
#!/bin/bash set -euo pipefail cat -n Packages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/AgentLaunchSanitizerCodexLaunch.swift | sed -n '1,290p' echo '--- behavioral probe ---' python3 - <<'PY' # Minimal semantic probe for the path/component checks in AgentLaunchSanitizerCodexLaunch.swift. import posixpath codex_hook_shell_metacharacters = set(";&|<>`$'\"\\ \t\n") def cmux_codex_hook_script_filename(normalized_command: str): if not normalized_command.startswith("/"): return None if any(ord(c) <= 32 for c in normalized_command): # control chars; Space 32 is handled below return None if any(c in codex_hook_shell_metacharacters for c in normalized_command): return None # Swift URL(fileURL...).standardizedFileURL does not shell-expand; use posixpath normalization here. # URL normalization for embedded URL arguments/token separators would not produce these path components. components = posixpath.normpath(normalized_command).split("/") # Strip empty first due leading slash; URL pathComponents has root as empty, last pathComponent as final component. # The Swift guard compares URL path == normalizedCommand but split pathComponents can include root component, so align. normalized_normalized = normalized_command.rstrip("/") or "/" split_path = normalized_normalized.split("/") # The check below is analogous to URL.pathComponents.allSatisfy trim: no boundary whitespace in path components, # preserving interior whitespace. for p in split_path: if p != p.strip(): return None # Suffix .cmux hooks is on directoryComponents, which excludes lastPathComponent directory_components = posixpath.normpath(normalized_command).rsplit("/",1)[0].split("/") if "/" in normalized_normalized.lstrip("/") else [] # Leading root is first empty component because normpath('/') is '/' # Actually: for '/usr/bin/foo', components include '', 'usr', 'bin', 'foo'. components = posixpath.normpath(normalized_command).split("/") directory_components = components[:-1] if len(directory_components) < 2: return None if directory_components[-2:] != ['.cmux', 'hooks']: return None return components[-1] for command in [ "/usr/bin/env sh relative/.cmux/hooks/cmux-codex-hook-<id>-stop.sh", "/usr/local/bin/cmux-agent .cmux/hooks/cmux-codex-hook-<id>-stop.sh", "/home/user/.cmux/hooks/cmux-codex-hook-<id>-stop.sh", ]: print(command, cmux_codex_hook_script_filename(command)) PYRepository: manaflow-ai/cmux
Length of output: 13616
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '--- tests mentioning codex hooks / injection / sanitization ---' rg -nP --type-test --glob '*Codex*|*Launch*|*Sanitizer*' -C3 'hooks|codex|cmuxCodex|CodexHook|replay|preserve|stripCmux' . || true echo '--- test files containing hook command samples ---' rg -n --type-test -C3 "cmux-codex-hook|\\.cmux/hooks|hooks codex|\\[\\{hooks" . || true echo '--- all references to hook command filename/schema ---' rg -n --type=swift -C3 'CodexHookScriptName|codexWrapperInjectedHookSubcommandAliases|cmuxCodexHookScriptFilename|recognized' Packages || trueRepository: manaflow-ai/cmux
Length of output: 50372
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' # Read-only semantic probe matching the exact Swift guard: # 1. first metacharacters guard includes space/tabs/control; # 2. URL(fileURLWithPath: path, isDirectory: false).standardizedFileURL preserves path components with spaces; # 3. URL path equality preserves spacing; # 4. pathComponents trim only boundary whitespace. # Use the set of shell metacharacters from codexHookShellMetacharacters, excluding the metacharacters it already rejects, # then test interior space in a path component as the review describes. import urllib.parse, os codex_hook_shell_metacharacters = set(";&|<>`$'\"\\\\ \t\n") def cmux_codex_hook_script_filename(normalized_command: str): if not normalized_command.startswith("/"): return None if any(ord(c) <= 32 for c in normalized_command): return None if any(c in codex_hook_shell_metacharacters for c in normalized_command): return None url_path = urllib.parse.unquote(urllib.parse.urlparse(normalized_command).path) if url_path != normalized_command: return None parts = urllib.parse.urlparse(normalized_command).path.split("/") parts2 = urllib.parse.urlparse(normalized_command).path.replace("//", "/").lstrip("/").split("/") normalized_normalized = normalized_command.rstrip("/") or "/" parts = normalized_normalized.split("/") if any(p != p.strip() for p in parts): return None directory_components = parts[:-1] if len(directory_components) < 2: return None if directory_components[-2:] != [".cmux", "hooks"]: return None return parts[-1] cases = [ "/usr/local/bin/cmux-agent .cmux/hooks/cmux-codex-hook-a-stop.sh", "/usr/local/bin/cmux-agent .cmux/hooks/cmux-codex-hook-a-stop.sh", "/usr/local/bin/cmux-agent .cmux/hooks/cmux-codex-hook-a-stop.sh", "/home/user/My App/.cmux/hooks/cmux-codex-hook-a-stop.sh", "/.cmux/hooks/cmux-codex-hook-a-stop.sh", ] for command in cases: print(command, cmux_codex_hook_script_filename(command)) PY echo '--- relevant CodexHookInjectionSchema.swift ---' cat -n Packages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/CodexHookInjectionSchema.swift | sed -n '1,120p'Repository: manaflow-ai/cmux
Length of output: 3311
Reject multi-token Codex hook commands before stripping hook arguments.
cmuxCodexHookScriptFilenameonly trims boundary whitespace per path component, so commands like/usr/local/bin/cmux-agent .cmux/hooks/cmux-codex-hook-a-stop.shsatisfy the path check and can remove the hook as cmux-injected config. Require the command to be the complete single-file path of a recognizable hook script, or explicitly reject non-trailing-command tokens before stripping.🤖 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 `@Packages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/AgentLaunchSanitizerCodexLaunch.swift` around lines 255 - 283, Update cmuxCodexHookScriptFilename to reject commands containing token separators or arguments before returning a hook filename; validate that normalizedCommand is exactly one complete, recognizable hook script path under .cmux/hooks, including the expected hook-script naming/extension, while preserving valid paths with interior component spaces.Packages/macOS/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/CodexHookInjectionStrippingTests.swift (1)
578-584: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Schema-derived hook fixtures remove the independent oracle from the stripping tests. Both suites replaced pinned literal argv with fixtures generated from
CodexHookInjectionSchema.current, so a change to the production schema updates the expectation and the input together — the tests can no longer detect a shape change that breaks stripping of argv captured by an older build.
Packages/macOS/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/CodexHookInjectionStrippingTests.swift#L578-L584: pin the current event list literally, aslegacySavedLayoutHookEventsandlegacyAliasHookEventsalready are, so a schema change forces the previous shape to be added torecognized.cmuxTests/CmuxConfigActionSaverTests.swift#L497-L506: keep the schema-derived fixture but make the Line 361/377 assertions schema-independent (assert on"cmux-codex-hook"/"--enable hooks"rather than the hardcodedstopfilename) so they cannot pass vacuously.📍 Affects 2 files
Packages/macOS/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/CodexHookInjectionStrippingTests.swift#L578-L584(this comment)cmuxTests/CmuxConfigActionSaverTests.swift#L497-L506🤖 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 `@Packages/macOS/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/CodexHookInjectionStrippingTests.swift` around lines 578 - 584, The schema-derived fixture in Packages/macOS/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/CodexHookInjectionStrippingTests.swift#L578-L584 must become an independent literal list for codexWrapperHookEvents, matching the pinned legacySavedLayoutHookEvents and legacyAliasHookEvents fixtures so schema changes require explicit recognized entries. In cmuxTests/CmuxConfigActionSaverTests.swift#L497-L506, retain the schema-derived fixture but update the relevant assertions to check for “cmux-codex-hook” and “--enable hooks” instead of the hardcoded stop filename, preventing vacuous passes.tests/test_ci_self_hosted_guard.sh (1)
869-900: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Create a fixture test before asserting Bun delegation.
The fixture has no test files, so the runner exits at its new empty-discovery guard and never invokes the Bun stub. This check now fails unconditionally.
Proposed fix
- mkdir -p "$fixture_dir/web/scripts" "$fixture_dir/bin" + mkdir -p "$fixture_dir/web/scripts" "$fixture_dir/web/tests" "$fixture_dir/bin" cp "$ROOT_DIR/web/scripts/run-tests.sh" "$fixture_runner" + : > "$fixture_dir/web/tests/default.test.ts"📝 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.check_web_test_runner_behavior() { local fixture_dir fixture_runner args_log expected_args fixture_dir="$(mktemp -d)" fixture_runner="$fixture_dir/web/scripts/run-tests.sh" args_log="$fixture_dir/bun-args.log" mkdir -p "$fixture_dir/web/scripts" "$fixture_dir/web/tests" "$fixture_dir/bin" cp "$ROOT_DIR/web/scripts/run-tests.sh" "$fixture_runner" : > "$fixture_dir/web/tests/default.test.ts" cat > "$fixture_dir/bin/bun" <<'EOF' #!/usr/bin/env bash set -euo pipefail printf '%s\n' "$@" > "$CMUX_WEB_TEST_RUNNER_ARGS_LOG" EOF chmod +x "$fixture_dir/bin/bun" if ! PATH="$fixture_dir/bin:/usr/bin:/bin" \ CMUX_WEB_TEST_RUNNER_ARGS_LOG="$args_log" \ /bin/bash "$fixture_runner"; then rm -rf "$fixture_dir" echo "FAIL: shared web test runner default discovery should execute" exit 1 fi expected_args=$'test\n--isolate' if [[ "$(cat "$args_log")" != "$expected_args" ]]; then echo "FAIL: shared web test runner must preserve Bun's default discovery with isolation" cat "$args_log" rm -rf "$fixture_dir" exit 1 fi rm -rf "$fixture_dir" }🤖 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/test_ci_self_hosted_guard.sh` around lines 869 - 900, Update check_web_test_runner_behavior to create a minimal valid test fixture under fixture_dir before invoking fixture_runner, ensuring the runner passes its discovery guard and delegates to the Bun stub. Keep the existing args_log assertion and cleanup behavior unchanged.web/scripts/run-tests.sh (1)
7-20: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
For the Bun version pinned by this repository, what exact defaultbun testfilename patterns and directory exclusions apply during recursive discovery?💡 Result:
By default, bun test recursively searches the project directory for files that match any of the following filename patterns [1][2]: *.test.{js|jsx|ts|tsx|mjs|cjs|mts|cts} *_test.{js|jsx|ts|tsx|mjs|cjs|mts|cts} *.spec.{js|jsx|ts|tsx|mjs|cjs|mts|cts} *_spec.{js|jsx|ts|tsx|mjs|cjs|mts|cts} During recursive discovery, bun test automatically ignores the following by default [1][2]: node_modules directories Hidden directories (those starting with a period
.) Files that do not have JavaScript-like extensions (as determined by available loaders) You can further customize which files are excluded from discovery by using the pathIgnorePatterns option in your bunfig.toml file, which supports glob patterns [3][4]. Additionally, bun test filters passed as positional arguments are used to restrict the set of discovered tests; these filters are treated as substring matches rather than glob patterns [1][2].Citations:
- 1: https://bun.com/docs/test/discovery
- 2: https://bun.sh/docs/test/discovery
- 3: https://bun.com/docs/test/configuration
- 4: https://bun.sh/docs/test/configuration
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '--- repo files ---\n' git ls-files | rg '(^|/)web/scripts/run-tests\.sh$|(^|/)bun\.lockb$|(^|/)package\.json$|(^|/)bunfig\.toml$' || true printf '\n--- run-tests.sh ---\n' if [ -f web/scripts/run-tests.sh ]; then cat -n web/scripts/run-tests.sh fi printf '\n--- bun version files ---\n' for f in package.json bun.lockb bunfig.toml; do if [ -f "$f" ]; then echo "### $f" sed -n '1,160p' "$f" fi doneRepository: manaflow-ai/cmux
Length of output: 245
🏁 Script executed:
#!/bin/bash set -euo pipefail # Deterministic behavioral probe: compare the shell preflight pattern to Bun’s documented # default filename patterns and exclusions over a representative set of paths. python3 - <<'PY' from pathlib import PurePosixPath extensions = {'.js','.jsx','.ts','.tsx','.mjs','.cjs','.mts','.cts'} allowed_extension = lambda p: PurePosixPath(p).suffix in extensions def shell_preflight(path): base = PurePosixPath(path).name if not allowed_extension(path): return False segments = path.strip('/').split('/') # current implementation prunes visible ./node_modules and .next only after # traversing, so a deep hidden or nested node_modules is still visited and tested. for seg in segments: if seg in {'node_modules','.next'}: # it still would have reached hidden/nested node_modules files before pruning, # so include them for this preflight probe. pass if seg.startswith('.') and seg not in {'.','..' }: pass # filename matcher from the snippet: patterns = ('*.test', '_test_', '*.spec', '_spec_') return any(base.endswith(nfx) or base.startswith(nfx) for (p,nfx) in patterns) def bun_discovery(path): base = PurePosixPath(path).name if not allowed_extension(path): return False for seg in PurePosixPath(path).parts: if seg.startswith('.'): return False if seg == 'node_modules': return False # exact suffix-only patterns (including all documented loader-like extensions): suffixes = ( '.test.js','.test.jsx','.test.ts','.test.tsx','.test.mjs','.test.cjs','.test.mts','.test.cts', '.spec.js','.spec.jsx','.spec.ts','.spec.tsx','.spec.mjs','. spec.cjs','.spec.mts','..spec.cts', '_test.js','_test.jsx','_test.ts','_test.tsx','_test.mjs','_test.cjs','_test.mts','_test.cts', '_spec.js','_spec.jsx','_spec.ts','_spec.tsx','_spec.mjs','_spec.cjs','_spec.mts','_spec.cts' ) return base.endswith(suffixes) samples = [ './foo.test-helper.ts', './__tests__/foo.test.ts', './.hidden/foo.test.ts', './foo.test.ts', './foo_test.ts', './foo.test_helper.ts', './hidden_module/foo.test.ts', './node_modules/foo.test.ts', './lib/node_modules/foo.test.ts', './foo.helper.testHelper.ts', ] for p in samples: print(p, "shell=", shell_preflight(p), "bun=", bun_discovery(p)) mism = [(p, shell_preflight(p), bun_discovery(p)) for p in samples if shell_preflight(p) != bun_discovery(p)] print("\nmismatches:", mism) PYRepository: manaflow-ai/cmux
Length of output: 365
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "--- repo files ---" git ls-files | grep -E '(^|/)web/scripts/run-tests\.sh$|(^|/)bun\.lockb$|(^|/)package\.json$|(^|/)bunfig\.toml$' || true echo echo "--- run-tests.sh ---" if [ -f web/scripts/run-tests.sh ]; then nl -ba web/scripts/run-tests.sh fi echo echo "--- bun version files ---" for f in package.json bun.lockb bunfig.toml; do if [ -f "$f" ]; then echo "### $f" sed -n '1,160p' "$f" fi doneRepository: manaflow-ai/cmux
Length of output: 497
🏁 Script executed:
#!/bin/bash set -euo pipefail # Behavioral probe: compare the shell preflight matcher/exclusion to Bun's documented test discovery rules. python3 - <<'PY' from pathlib import PurePosixPath extensions = {'.js','jsx'.replace('j','j'),'.ts','.tsx','.mjs','.cjs','.mts','.cts'} extensions = {'.js','.jsx','.ts','.tsx','.mjs','.cjs','.mts','.cts'} def shell_path_pruned(path): # Original find runs from current directory and first searches for -path './node_modules' and -path ' prunes = [] for i, seg in enumerate(PurePosixPath(path).parts): # path "./node_modules" exactly prunes top-level node_modules; nested like lib/node_modules is not caught by "-path ./node_modules". # Hidden directory pruning is not in the original find, so paths under . are not pruned. pass return prunes def shell_preflight(path): p = PurePosixPath(path) if p.suffix not in extensions: return False base = p.name # Matches anything containing .test / _test_ / .spec / _spec_ in the filename. return any(x in base for x in ['.test','_test_','.spec','_spec_']) def bun_discovery(path): p = PurePosixPath(path) suffix = p.suffix if not (suffix.startswith('.') and suffix[1:] in {'js','jsx','ts','tsx','mjs','cjs','mts','cts'}): return False segments = PurePosixPath(path).parts for seg in segments: if seg.startswith('.') or seg == 'node_modules': return False base = p.name exact = [ ['.test.js','.test.jsx','.test.ts','.test.tsx','.test.mjs','.test.cjs','.test.mts','.test.cts'], ['_test.js','_test.jsx','_test.ts','_test.tsx','_test.mjs','_test.cjs','_test.mts','_test.cts'], ['.spec.js','.spec.jsx','.spec.ts','.spec.tsx','.spec.mjs','.spec.cjs','.spec.mts','.spec.cts'], ['_spec.js','_spec.jsx','_spec.ts','_spec.tsx','_spec.mjs','_spec.cjs','_spec.mts','_spec.cts'], ] return any(base == suffix_for + ext for suffix_for in ['test','spec'] for ext in suffixes_for_ext) samples = [ './foo.test-helper.ts', './__tests__/foo.test.ts', './.hidden/foo.test.ts', './foo.test.ts', './foo_test.ts', './foo.test_helper.ts', './lib/node_modules/foo.test.ts', './node_modules/foo.test.ts', './foo.helper.testHelper.ts', ] def shell_file_seen(path): # Original find scans everything unless path is exactly "./node_modules" or "./.next"; hidden files are not pruned. return True for p in samples: print(f"{p} shell_found={shell_file_seen(p)} shell_preflight={shell_preflight(p)} bun_discovery={bun_discovery(p)}") PYRepository: manaflow-ai/cmux
Length of output: 362
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '--- files ---' git ls-files | grep -E '(^|/)web/scripts/run-tests\.sh$|(^|/)web/bunfig\.toml$|(^|/)web/package\.json$|(^|/)package\.json$|(^|/)bun\.lockb$' || true echo echo '--- web/scripts/run-tests.sh ---' sed -n '1,120p' web/scripts/run-tests.sh echo echo '--- web/bunfig.toml ---' if [ -f web/bunfig.toml ]; then sed -n '1,160p' web/bunfig.toml; fi echo echo '--- bun config refs ---' grep -RInE '(bun|bunfig|test|node_modules|exclude|ignore)' package.json web/package.json web/bunfig.toml 2>/dev/null || trueRepository: manaflow-ai/cmux
Length of output: 3323
🌐 Web query:
Bun test discovery hidden directories node_modules default patterns exact * .test .spec _test _spec extensions💡 Result:
Bun's test runner automatically discovers and executes test files by recursively searching the project directory for files that match specific naming patterns [1][2]. Default Discovery Patterns: Bun identifies test files by looking for the following suffixes, supporting common JavaScript and TypeScript extensions (.js,.jsx,.ts,.tsx,.mjs,.cjs,.mts,.cts) [1][3]: - .test. - _test. - .spec. - _spec. Default Exclusions: To optimize performance and prevent execution of unintended files, Bun automatically ignores [1]: - node_modules directories - Hidden directories (any directory starting with a period
.) - Files lacking recognized JavaScript-like extensions Customizing Discovery: You can further control test discovery using the following methods: - Path Ignore Patterns: You can exclude specific directories or files (such as vendor folders or fixtures) from discovery by configuring pathIgnorePatterns in your bunfig.toml file [4][5]. This is highly efficient as Bun prunes these directories during the initial scan [4]. - Root Directory: By default, Bun searches from the current working directory, but you can specify a different starting point using the root configuration in bunfig.toml [1][5]. - Filtering: While not for discovery, you can limit execution by passing positional arguments to the bun test command. Any file with a path matching the provided string filter will be executed [1][2]. Note that these are substring matches, not glob patterns [1][3].Citations:
- 1: https://bun.com/docs/test/discovery
- 2: https://bun.com/docs/test
- 3: https://bun.sh/docs/test
- 4: https://bun.sh/docs/test/configuration
- 5: https://bun.com/docs/test/configuration
Mirror Bun’s default discovery rules exactly.
This preflight passes files Bun will not run and scans paths Bun ignores. It accepts partial names like
foo.test-helper.tsand traverses hidden and nestednode_modulesdirectories, so it can report “tests found” when Bun would discover none.Proposed fix
- case "${file##*/}" in - *".test"* | *"_test_"* | *".spec"* | *"_spec_"*) return 0 ;; + case "${file##*/}" in + *.test.js | *.test.jsx | *.test.ts | *.test.tsx | \ + *_test.js | *_test.jsx | *_test.ts | *_test.tsx | \ + *.spec.js | *.spec.jsx | *.spec.ts | *.spec.tsx | \ + *_spec.js | *_spec.jsx | *_spec.ts | *_spec.tsx) return 0 ;; esac done < <( find . \ - \( -path "./node_modules" -o -path "./.next" \) -prune \ + \( -type d \( -name node_modules -o -path '*/.*' \) -prune \) \ -o -type f -print0 )📝 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.default_test_file_exists() { while IFS= read -r -d '' file; do case "$file" in *.js | *.jsx | *.ts | *.tsx | *.mjs | *.cjs | *.mts | *.cts) ;; *) continue ;; esac case "${file##*/}" in *.test.js | *.test.jsx | *.test.ts | *.test.tsx | \ *_test.js | *_test.jsx | *_test.ts | *_test.tsx | \ *.spec.js | *.spec.jsx | *.spec.ts | *.spec.tsx | \ *_spec.js | *_spec.jsx | *_spec.ts | *_spec.tsx) return 0 ;; esac done < <( find . \ \( -type d \( -name node_modules -o -path '*/.*' \) -prune \) \ -o -type f -print0 )🤖 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 `@web/scripts/run-tests.sh` around lines 7 - 20, Update default_test_file_exists to mirror Bun’s discovery rules exactly: restrict matching to Bun-supported test filename patterns, avoid partial matches such as foo.test-helper.ts, and prune every ignored node_modules and hidden directory during find traversal, including nested paths. Preserve the function’s existing return behavior by returning success only when a Bun-discoverable test file is found.
1 participant
Summary
990bf083e61d4704625d05502728d6f91343a52dWhy
Bun 1.2.14 exits successfully when default discovery finds no tests, while Bun 1.3.x exits nonzero. The shared runner must not delegate this repository invariant to version-dependent CLI behavior.
Review finding: #8941 (comment)
Verification
98e18193a7fails the focused empty-discovery assertionbun test tests/web-test-runner-isolation.test.ts -t "default discovery"(2 passed)bash -n web/scripts/run-tests.shgit diff --checkSummary by CodeRabbit
Bug Fixes
Tests