fix(runtime): repair sessions and allow staged media through symlinked dirs#100026
fix(runtime): repair sessions and allow staged media through symlinked dirs#100026steipete wants to merge 3 commits into
Conversation
|
Codex review: needs changes before merge. Reviewed July 4, 2026, 4:23 PM ET / 20:23 UTC. Summary PR surface: Source +62, Tests +23. Total +85 across 9 files. Reproducibility: yes. source-reproducible. The linked issue and PR body give the exact macOS Vitest command for the original failures, and the remaining cleanup defects follow from the PR helper overriding the existing realpath containment path. Review metrics: 1 noteworthy metric.
Stored data model Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Copy recommended automerge instructionNext step before merge
Security Review findings
Review detailsBest possible solution: Preserve the macOS test-gate fix only after restoring session cleanup containment for existing symlinks and dot segments, or move to a repaired fixture-layer replacement if maintainers want no production runtime changes here. Do we have a high-confidence way to reproduce the issue? Yes, source-reproducible. The linked issue and PR body give the exact macOS Vitest command for the original failures, and the remaining cleanup defects follow from the PR helper overriding the existing realpath containment path. Is this the best way to solve the issue? No. The macOS test fixes are useful, but the runtime cleanup helper is not the best mergeable shape until it preserves existing-file symlink containment and rejects dot-segment stored paths. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 7a0188cbd20d. Label changesLabel justifications:
Evidence reviewedPR surface: Source +62, Tests +23. Total +85 across 9 files. View PR surface stats
Security concerns:
Acceptance criteria:
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
Review history (5 earlier review cycles)
|
clawsweeper
Bot
added
proof: sufficient
2a1ae2c to
976d13d
Compare
![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: 976d13da25
ℹ️ 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 (parentReal !== realSessionsDir) { | ||
| return undefined; | ||
| } | ||
| return path.join(parentReal, path.basename(trimmed)); |
There was a problem hiding this comment.
When sessionFile names a symlink in the sessions directory, this parent-only realpath check accepts it and returns the symlink path without resolving the file target. That bypasses the containment behavior in resolveSessionFilePath, so sessions cleanup --fix-missing can read/keep an entry based on a transcript outside the agent sessions directory instead of falling back to the canonical transcript or pruning the stale row. This only needs a corrupt or user-created symlinked sessionFile; realpath the candidate when it exists and only use the parent-realpath path for genuinely missing files.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
rating: 🧂 unranked krab
…lize macOS test pins
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a77ef11dc3
ℹ️ 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 (parentReal !== realSessionsDir) { | ||
| return undefined; | ||
| } | ||
| return path.join(parentReal, path.basename(trimmed)); |
There was a problem hiding this comment.
When a corrupted absolute sessionFile is exactly under the sessions dir but names . or .. (for example <sessions>/..), this join normalizes the basename and returns the sessions directory or its parent. transcriptExistsWithMessages() then treats an existing directory as a valid transcript, so sessions cleanup --fix-missing keeps the stale row instead of falling back to the canonical transcript path and pruning it as before. Reject ./.. basenames before returning this path.
Useful? React with 👍 / 👎.
|
Superseded by #100069, landed as bbb7442. Both failures this PR targets are fixed there at the fixture layer instead of per-test patches: the shared temp-root helpers ( The Verification on macOS: the issue's repro ( |
steipete
changed the title
|
Rescoped: the original test-gate stabilization here was superseded by #100069 (nice fixture-root canonicalization — better boundary than my per-assertion edits) and #100053. What remains are the two production fail-closed bugs on symlinked dirs that the same investigation uncovered, now with explicit-symlink regression tests that run meaningfully on Linux CI as well. Verified both directions: tests fail on main without the fixes, pass with them. |
… gate Concurrent scripts/pr gate runs across .worktrees queued on the shared heavy-check lock mid-test: the queued run's children hit the 10-minute lock timeout while its unlocked build stage piled CPU load onto the holder's vitest shards, which then stalled past the 120s no-output watchdog and were SIGTERMed with zero real test failures (observed landing PRs #99935/#100026 on a loaded maintainer Mac). - scripts/pr-gates-lock.mjs holds the shared heavy-check lock for the whole local gate block; gate stages inherit the existing *_LOCK_HELD child contract, so concurrent gate runs now queue as whole units before their first command. - OPENCLAW_PR_GATES_REMOTE=testbox runs the full-suite pnpm test gate on a Blacksmith Testbox via scripts/crabbox-wrapper.mjs (same delegation as check:changed). The tbx_ lease id and Actions run URL from the crabbox --timing-json report are recorded in .local/gates.env (REMOTE_GATES_*) and .local/prep.md. Local remains the default; pnpm build/check stay local. Formatting verified with the primary checkout's oxfmt (hook bypassed: linked worktree has no hydrated node_modules).
… gate Concurrent scripts/pr gate runs across .worktrees queued on the shared heavy-check lock mid-test: the queued run's children hit the 10-minute lock timeout while its unlocked build stage piled CPU load onto the holder's vitest shards, which then stalled past the 120s no-output watchdog and were SIGTERMed with zero real test failures (observed landing PRs #99935/#100026 on a loaded maintainer Mac). - scripts/pr-gates-lock.mjs holds the shared heavy-check lock for the whole local gate block; gate stages inherit the existing *_LOCK_HELD child contract, so concurrent gate runs now queue as whole units before their first command. - OPENCLAW_PR_GATES_REMOTE=testbox runs the full-suite pnpm test gate on a Blacksmith Testbox via scripts/crabbox-wrapper.mjs (same delegation as check:changed). The tbx_ lease id and Actions run URL from the crabbox --timing-json report are recorded in .local/gates.env (REMOTE_GATES_*) and .local/prep.md. Local remains the default; pnpm build/check stay local. Formatting verified with the primary checkout's oxfmt (hook bypassed: linked worktree has no hydrated node_modules).
… gate Concurrent scripts/pr gate runs across .worktrees queued on the shared heavy-check lock mid-test: the queued run's children hit the 10-minute lock timeout while its unlocked build stage piled CPU load onto the holder's vitest shards, which then stalled past the 120s no-output watchdog and were SIGTERMed with zero real test failures (observed landing PRs #99935/#100026 on a loaded maintainer Mac). - scripts/pr-gates-lock.mjs holds the shared heavy-check lock for the whole local gate block; gate stages inherit the existing *_LOCK_HELD child contract, so concurrent gate runs now queue as whole units before their first command. - OPENCLAW_PR_GATES_REMOTE=testbox runs the full-suite pnpm test gate on a Blacksmith Testbox via scripts/crabbox-wrapper.mjs (same delegation as check:changed). The tbx_ lease id and Actions run URL from the crabbox --timing-json report are recorded in .local/gates.env (REMOTE_GATES_*) and .local/prep.md. Local remains the default; pnpm build/check stay local. Formatting verified with the primary checkout's oxfmt (hook bypassed: linked worktree has no hydrated node_modules).
… gate Concurrent scripts/pr gate runs across .worktrees queued on the shared heavy-check lock mid-test: the queued run's children hit the 10-minute lock timeout while its unlocked build stage piled CPU load onto the holder's vitest shards, which then stalled past the 120s no-output watchdog and were SIGTERMed with zero real test failures (observed landing PRs #99935/#100026 on a loaded maintainer Mac). - scripts/pr-gates-lock.mjs holds the shared heavy-check lock for the whole local gate block; gate stages inherit the existing *_LOCK_HELD child contract, so concurrent gate runs now queue as whole units before their first command. - OPENCLAW_PR_GATES_REMOTE=testbox runs the full-suite pnpm test gate on a Blacksmith Testbox via scripts/crabbox-wrapper.mjs (same delegation as check:changed). The tbx_ lease id and Actions run URL from the crabbox --timing-json report are recorded in .local/gates.env (REMOTE_GATES_*) and .local/prep.md. Local remains the default; pnpm build/check stay local. Formatting verified with the primary checkout's oxfmt (hook bypassed: linked worktree has no hydrated node_modules).
… gate (#100242) * feat(scripts): serialize pr prepare gates and add remote testbox test gate Concurrent scripts/pr gate runs across .worktrees queued on the shared heavy-check lock mid-test: the queued run's children hit the 10-minute lock timeout while its unlocked build stage piled CPU load onto the holder's vitest shards, which then stalled past the 120s no-output watchdog and were SIGTERMed with zero real test failures (observed landing PRs #99935/#100026 on a loaded maintainer Mac). - scripts/pr-gates-lock.mjs holds the shared heavy-check lock for the whole local gate block; gate stages inherit the existing *_LOCK_HELD child contract, so concurrent gate runs now queue as whole units before their first command. - OPENCLAW_PR_GATES_REMOTE=testbox runs the full-suite pnpm test gate on a Blacksmith Testbox via scripts/crabbox-wrapper.mjs (same delegation as check:changed). The tbx_ lease id and Actions run URL from the crabbox --timing-json report are recorded in .local/gates.env (REMOTE_GATES_*) and .local/prep.md. Local remains the default; pnpm build/check stay local. Formatting verified with the primary checkout's oxfmt (hook bypassed: linked worktree has no hydrated node_modules). * fix(scripts): refresh the gate stamp when lease-retry gates rerun for a rebased head The lease-retry path reran build/check/test for the rebased prep head but left .local/gates.env describing the pre-push head, so prep.md/prep.env attributed stale evidence (including the new remote testbox lease id) to the pushed commit. Extract write_gates_env_stamp as the single stamp writer, rewrite the stamp from the retry path for all modes, and re-source gates.env in prepare_push after the push settles. Found by autoreview (codex/gpt-5.5); formatting verified with the primary checkout's oxfmt (hook bypassed: linked worktree has no node_modules). * fix(scripts): harden remote PR gate evidence * fix(scripts): serialize complete gate setup * fix(scripts): clear stale docs gate proof
… gate (openclaw#100242) * feat(scripts): serialize pr prepare gates and add remote testbox test gate Concurrent scripts/pr gate runs across .worktrees queued on the shared heavy-check lock mid-test: the queued run's children hit the 10-minute lock timeout while its unlocked build stage piled CPU load onto the holder's vitest shards, which then stalled past the 120s no-output watchdog and were SIGTERMed with zero real test failures (observed landing PRs openclaw#99935/openclaw#100026 on a loaded maintainer Mac). - scripts/pr-gates-lock.mjs holds the shared heavy-check lock for the whole local gate block; gate stages inherit the existing *_LOCK_HELD child contract, so concurrent gate runs now queue as whole units before their first command. - OPENCLAW_PR_GATES_REMOTE=testbox runs the full-suite pnpm test gate on a Blacksmith Testbox via scripts/crabbox-wrapper.mjs (same delegation as check:changed). The tbx_ lease id and Actions run URL from the crabbox --timing-json report are recorded in .local/gates.env (REMOTE_GATES_*) and .local/prep.md. Local remains the default; pnpm build/check stay local. Formatting verified with the primary checkout's oxfmt (hook bypassed: linked worktree has no hydrated node_modules). * fix(scripts): refresh the gate stamp when lease-retry gates rerun for a rebased head The lease-retry path reran build/check/test for the rebased prep head but left .local/gates.env describing the pre-push head, so prep.md/prep.env attributed stale evidence (including the new remote testbox lease id) to the pushed commit. Extract write_gates_env_stamp as the single stamp writer, rewrite the stamp from the retry path for all modes, and re-source gates.env in prepare_push after the push settles. Found by autoreview (codex/gpt-5.5); formatting verified with the primary checkout's oxfmt (hook bypassed: linked worktree has no node_modules). * fix(scripts): harden remote PR gate evidence * fix(scripts): serialize complete gate setup * fix(scripts): clear stale docs gate proof
1 participant
What Problem This Solves
Two runtime fail-closed bugs when OpenClaw state or media roots are reached through symlinked directories (macOS
/var -> /private/vartmp paths, or any user-level symlinked state/media dir):sessions cleanupsilently skipped both stale-generated-metadata repair and missing-transcript pruning: the resolver realpaths the sessions dir, a storedsessionFileaddressed through the symlink failed containment, and the canonical-transcript fallback masked the stale row forever.Related to #100025 (the test-gate side of that issue was fixed by #100069; these are the two production bugs uncovered by the same investigation).
Why This Change Was Made
src/config/sessions/cleanup-service.ts: cleanup now resolves a stored absolutesessionFiledirectly when its realpathed parent equals the realpathed sessions dir, instead of letting the resolver fall back to the canonical transcript. Agents-shaped fallbacks and every existing path pin are untouched.src/media/local-media-access.ts: missing candidates realpath their parent dir and rejoin the basename before the containment check. The invariant (path must be inside a resolved allowed root) is unchanged; only the previously fail-closed symlink form is corrected.Both come with cross-platform regression tests that build an explicit
fs.symlinkdir, so they exercise the fixed branches on Linux CI too — not only on macOS.User Impact
Sessions cleanup repairs/prunes correctly for stores on symlinked paths; outbound media staged via host-read callbacks under symlinked allowed roots now sends instead of erroring. No behavior change for non-symlinked setups.
Evidence
mainwithout the fixes and pass with them (verified both directions):store.pruning.integration.test.ts"repairs stale generated metadata when the store is reached through a symlinked dir" — repaired 0 -> 1local-media-access.test.ts"allows missing files under a root reached through a symlinked dir" — path-not-allowed -> allowedpnpm tsgoclean.