Repository navigation
[3381 by Claude Opus 5.5] Make 4 test files robust on the self-hosted nightly runner, without weakening what they verify. Check current upstream main first. - #3383
Conversation
![propr-dev[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
…ouldn't do the real-root check the issue asks for. This container has no Docker, sudo or user namespaces, so that run still needs a disposable root container. Upstream `main` was the same commit as this branch, so nothing there already covered it. What was wrong and the fix `agentDefaultConfigPaths`: the test sets `HOME=/root`. At ba1e272 (the nightly's commit), the task args mounted `/root/.claude.json` from that HOME, which only exists on the root host. Current main writes that mount differently (`--mount type=bind,source=...`), so the old check (no argument starts with `/root/.claude`) passes again but no longer catches anything. The test now sets `CLAUDE_JSON_PATH` to a temp `.claude.json`. It checks that this fixture is the mounted file and that no argument contains `/root/.claude` anywhere. `agentToolPolicyRuntime`: the generated MCP config starts the local adapter with the image's `/usr/local/bin/node`. The test already redirected the adapter script to the repo copy, but not that Node path. If `/usr/local/bin/node` is missing on the runner (my guess, since the nightly uses setup-node), the adapter never starts. When I pointed that path at a missing binary, I got exactly the nightly failure: the same 18-tool list and both failing tests. The test now uses the test's own Node (`process.execPath`), and the CLI's XDG config/cache dirs sit inside the temp HOME. `followupHandoffReviewBlockers` (F23): as root, `chmod 000` can't produce EACCES. The test now injects EACCES, EPERM and EIO into the access check, so it runs under every uid. The real-`chmod` check is a separate test, skipped only when `process.getuid?.() === 0`, with that reason in the skip message. To prove it still catches problems, I temporarily made the code treat EACCES as "missing": the injected test failed even with the uid faked to 0. `agentRunAdmission` (180 s timeout): the cause was a hang, not CPU. Two problems: - The file never exited. Importing the Docker executor opens the database connection, and nothing closed it. GitHub-hosted runners don't have `redis-server`, so those tests are skipped there and the file finishes in about 1 s. The self-hosted host has Redis, so they run and the file hung until the 180 s limit. It now closes the connection in `after()`. - The 12-process case failed intermittently (2 of 3 full runs here). The worker fixture exited 500 ms after finishing, sometimes before its Redis slot was released. It now waits until Redis no longer holds a slot for its containers (up to 30 s), and fails if the slot is never released. Verification - Normal user: all 4 files pass, with a locally built `redis-server` so the 25 Redis tests actually run. `agentRunAdmission` takes 70–83 s, including under a 2-CPU limit to mimic the runner's quota. - Closest to root I could get: all 4 pass with `process.getuid()` faked to 0 (the F23 skip shows its reason) and a fake populated agent home: `.claude.json` with an extra MCP server, `.claude/settings.json`, `.credentials.json`, `.codex/config.toml`. The host-style variables (`CLAUDE_JSON_PATH`, `CLAUDE_CONFIG_DIR`, `CODEX_HOME`, `XDG_CONFIG_HOME`, `HOST_CLAUDE_DIR`) were set as well. - Lint: `npm run lint -- --max-warnings 0` passes. Root `test/` isn't covered by ESLint or the typecheck gate, so I ran `tsc` on the 5 changed files: they have the same 4 type errors as before, and none are new. A possible product issue, left unchanged When the backend runs in a container and `CLAUDE_JSON_PATH` isn't set, the code falls back to `.claude.json` in the backend's own home. That's the same "uses the backend HOME" pattern the config-path code refuses for the credentials folder. The code comment says this is intended for dev/compose setups and it was only just reworked, so I didn't change it. It may be worth deciding separately whether it should also refuse. Changed files: the 4 test files plus `test/fixtures/agent-admission/worker.ts`. There's nothing visual, so no previews.
|
/review |
|
✅ AI Code Review Complete requested by @integry Posted 1 review: |
🔍 AI Code Review — codex:gpt-6-astraOverall EvaluationReady to merge within the visible scope. The changes improve host isolation and cleanup without visibly weakening the existing assertions. Current head checks report 18 passed and no failures. ✅ Credential isolation — The temporary Claude JSON fixture explicitly verifies the selected bind source and strengthens the assertion against ✅ Permission coverage — Injected EACCES, EPERM and EIO failures preserve error-propagation coverage under root, while the real permission test remains enabled for non-root users. ✅ Cleanup synchronization — The worker waits for its own started containers’ admission records to disappear instead of relying on a fixed 500 ms delay. This is a static review of the supplied code; no commands were run. The issue description is explicitly truncated, so acceptance criteria beyond the visible text could not be assessed. Merge blockersNo merge blockers. SuggestionsThese are optional follow-ups and are not sent to S1: 🟢 Record target-runner validationA disposable root-environment run with synthetic agent configuration and the pinned CLIs would strengthen confidence in the intended nightly-runner fix. Record which runtime tests actually executed, since CLI version checks can skip them. This is supplemental validation, not a demonstrated defect in the changed code; the supplied passing checks do not establish those specific execution conditions. ScoreThe changes are focused, preserve substantive assertions, and have passing current-head checks. Confidence is limited by static-only review and the unavailable remainder of the issue specification. Score: 8/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
|
Validated on the target condition: the merged result runs as root in One more root-only failure is left in It's the same cause as the F23 case you already fixed: root bypasses the chmod-based 'inaccessible' fixture. Apply the same approach (an injected access error, or a uid-0-only skip with an explicit reason that still enforces it elsewhere), and check the whole file for any other chmod/EACCES fixtures. Rerun the file as root and as a normal user. |
|
✅ Applied the requested follow-up changes in commit e06873b Summary of ChangesI've fixed the last root-only failure in What changed ( I also detach the worktree's branch before cleanup, which makes the test stricter than before. With no worktree registered on the branch, reading the access error as "missing" would wrongly settle the handoff. With the old chmod fixture, git itself also failed, so the test passed whether or not that bug was there. Rest of the file: the only other chmod fixture is "F23: a real permission-denied worktree path rejects…". It already skips under uid 0 with an explicit reason, and the injected Test results:
Only the test file changed. There's nothing visual, so no previews. 🤖 Implementation Details
Undo Changes • View Task Execution 💡 ProPR Slash Commands
🤖 Generated by ProPR |
…offReviewBlockers.test.ts`. I couldn't run it as real root, because this container has no Docker, sudo or user namespaces. I could only fake uid 0, so your `node:24-bookworm` root run is still the one that confirms it. I've fixed the last root-only failure in `test/followupHandoffReviewBlockers.test.ts`. I couldn't run it as real root, because this container has no Docker, sudo or user namespaces. I could only fake uid 0, so your `node:24-bookworm` root run is still the one that confirms it. **What changed** (`test/followupHandoffReviewBlockers.test.ts:1085`): "F23: recovery cleanup keeps the handoff when the worktree path is inaccessible" no longer uses `chmod 0o000`. Instead, `fs.promises.access` throws an injected `EACCES` for any path under the fixture directory, and passes everything else through. This is the same approach as the earlier F23 git-ops test. The test now runs for every uid, with no skip. I also detach the worktree's branch before cleanup, which makes the test stricter than before. With no worktree registered on the branch, reading the access error as "missing" would wrongly settle the handoff. With the old chmod fixture, git itself also failed, so the test passed whether or not that bug was there. **Rest of the file:** the only other chmod fixture is "F23: a real permission-denied worktree path rejects…". It already skips under uid 0 with an explicit reason, and the injected `EACCES`/`EPERM`/`EIO` test above it covers the same rule for root. Nothing else in the file uses chmod or relies on permissions. **Test results:** - **Normal user (uid 1000):** 64 of 64 pass. - **`process.getuid()` faked to 0:** 63 pass and 1 skipped (the real-chmod test, which shows its reason). The rewritten test doesn't depend on file permissions, so being root can't change its result. - **Negative check:** I temporarily changed `src/jobs/followupHandoffGitOps.ts` to treat `EACCES` as "missing". The rewritten test then failed with exactly your root output (`+ 'settled'` / `- 'cleanup_pending'`). I restored the file afterwards, so production code is unchanged. Only the test file changed. There's nothing visual, so no previews. PR: #3383 Comment by: @integry (ID: 6501900004) Model: Claude Opus 5.5
|
/review |
|
✅ AI Code Review Complete requested by @integry Posted 1 review: |
🔍 AI Code Review — codex:gpt-6-astraOverall EvaluationThe PR makes the tests less dependent on host credentials, executable locations, permissions, and cleanup timing without changing production code. The supplied changes appear merge-ready within scope, conditional on pending checks completing successfully. ✅ UID-independent error coverage — Injected access errors exercise the real handoff logic under root. Detaching the worktree ensures that incorrectly treating an access error as absence would fail the recovery assertion. ✅ Explicit fixture isolation — The Claude seed uses a temporary file, and runtime tests set temporary XDG directories while retaining tool-selection and credential-leak assertions. ✅ Observable cleanup completion — The admission worker waits for its started containers’ Redis reservations to disappear instead of relying on a fixed delay. This was a static review; no commands were run. The supplied issue description is truncated, so requirements beyond its visible text could not be assessed. Merge blockersNo merge blockers. SuggestionsThese are optional follow-ups and are not sent to S2: 🟢 Record final UID validationAttach results for the updated ScoreThe changes preserve meaningful assertions and address the visible host-dependent failures. No code blocker was found; five current-head checks remain pending, and final root/non-root rerun results are not supplied. Score: 8/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
AI Implementation Summary
Closes #3381
Branch:
3381/claude-opus-5-5-make-4-test-files-robust--20261029-1224-8k2Commits: ✅ Changes committed (9de0aba)
AI Processing Completed
Execution Details:
Summary:
I've fixed all 4 test files without changing production code, but I couldn't do the real-root check the issue asks for. This container has no Docker, sudo or user namespaces, so that run still needs a disposable root container. Upstream
mainwas the same commit as this branch, so nothing there already covered it.What was wrong and the fix
agentDefaultConfigPaths: the test setsHOME=/root. At ba1e272 (the nightly's commit), the task args mounted/root/.claude.jsonfrom that HOME, which only exists on the root host. Current main writes that mount differently (--mount type=bind,source=...), so the old check (no argument starts with/root/.claude) passes again but no longer catches anything. The test now setsCLAUDE_JSON_PATHto a temp.claude.json. It checks that this fixture is the mounted file and that no argument contains/root/.claudeanywhere.agentToolPolicyRuntime: the generated MCP config starts the local adapter with the image's/usr/local/bin/node. The test already redirected the adapter script to the repo copy, but not that Node path. If/usr/local/bin/nodeis missing on the runner (my guess, since the nightly uses setup-node), the adapter never starts. When I pointed that path at a missing binary, I got exactly the nightly failure: the same 18-tool list and both failing tests. The test now uses the test's own Node (process.execPath), and the CLI's XDG config/cache dirs sit inside the temp HOME.followupHandoffReviewBlockers(F23): as root,chmod 000can't produce EACCES. The test now injects EACCES, EPERM and EIO into the access check, so it runs under every uid. The real-chmodcheck is a separate test, skipped only whenprocess.getuid?.() === 0, with that reason in the skip message. To prove it still catches problems, I temporarily made the code treat EACCES as "missing": the injected test failed even with the uid faked to 0.agentRunAdmission(180 s timeout): the cause was a hang, not CPU. Two problems:redis-server, so those tests are skipped there and the file finishes in about 1 s. The self-hosted host has Redis, so they run and the file hung until the 180 s limit. It now closes the connection inafter().Verification
redis-serverso the 25 Redis tests actually run.agentRunAdmissiontakes 70–83 s, including under a 2-CPU limit to mimic the runner's quota.process.getuid()faked to 0 (the F23 skip shows its reason) and a fake populated agent home:.claude.jsonwith an extra MCP server,.claude/settings.json,.credentials.json,.codex/config.toml. The host-style variables (CLAUDE_JSON_PATH,CLAUDE_CONFIG_DIR,CODEX_HOME,XDG_CONFIG_HOME,HOST_CLAUDE_DIR) were set as well.npm run lint -- --max-warnings 0passes. Roottest/isn't covered by ESLint or the typecheck gate, so I rantscon the 5 changed files: they have the same 4 type errors as before, and none are new.A possible product issue, left unchanged
When the backend runs in a container and
CLAUDE_JSON_PATHisn't set, the code falls back to.claude.jsonin the backend's own home. That's the same "uses the backend HOME" pattern the config-path code refuses for the credentials folder. The code comment says this is intended for dev/compose setups and it was only just reworked, so I didn't change it. It may be worth deciding separately whether it should also refuse.Changed files: the 4 test files plus
test/fixtures/agent-admission/worker.ts. There's nothing visual, so no previews.Detailed Logs:
1ad0fb67-46c0-4ecd-a0fe-201f6c3a3bbfLog files stored at:
/usr/src/app/logs/executions/issue-3381-2026-10-29T13-10-18-700Z-conversation.json/usr/src/app/logs/executions/issue-3381-2026-10-29T13-10-18-700Z-output.txtLatest Conversation Messages
This PR was created automatically by ProPR after processing issue #3381.
💡 Need changes?
Comment on this PR to request refinements — the AI agent monitors comments and will update the implementation based on your feedback. Keep iterating until you're satisfied!