fix: retry compaction-busy turns and close shell fs jail gaps#228
Conversation
CompactionBusyError from preflight (async post-turn compaction holding the 300s-TTL lease past busyTimeoutMs on large sessions) propagated through runTransition's catch-all and killed the chat with a terminal "response failed: from assistant_streaming: compaction already in progress". - CompactionBusyError now extends TransientError: runTransition re-throws and the durable turn-step queue retries once the lease releases, the same proven path the session-lease contention already uses. - preflight no longer conflates compact_now statuses: only 'ok' maps to 'compacted' (reload messages); 'empty' or an unknown/drifted status logs a warning and proceeds without claiming the context shrank. - Document the orphaned `turn-step` queue config at TURN_STEP_QUEUE: steps go to the engine `default` queue; switching is a deliberate follow-up. - Tests: regression for the transient re-throw (no terminal state::set), direct instanceof contract pin, and the two new status branches.
…e errors S215/S210 jail errors never named the configured host_root, so jailed agents could not self-correct: the x7tshdba session burned turns guessing paths (a relative "." probe died with S210) before falling back to shell::exec ls. - validate_path resolves relative paths against the canonical host_root when jailed; the canonical starts_with check still bounds the joined path, so `..` cannot escape. Empty paths get an explicit S210. - S215 messages (validate_path and the write parents:true branch) name the canonical jail root so a caller can recover in one step. - lexical_operand: rm/chmod/mv/sed need lexical (non-canonicalized) operands for unlink/perm/rename semantics, but rebuilt them from the raw request string — with relative inputs now accepted, the validated path (host_root/<rel>) and the operated-on path (<cwd>/<rel>) diverged into a jail escape. The helper anchors relative inputs to the same root validate_path validated, closing the divergence (found in review by two independent models). - Tests: relative resolution under the jail, dotdot escape, empty path (jailed and unjailed), S215 names the root (validate_path and parents:true write), and cwd!=host_root regressions for rm/mv/chmod/sed.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
skill-check — worker0 verified, 14 skipped (no docs/).
Four for four. Nicely done. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR makes two independent changes: first, it reclassifies ChangesTurn-Step Transient Error Handling
Filesystem Relative Path Support
Sequence Diagram(s)sequenceDiagram
participant runPreflight
participant compact_now
participant caller
runPreflight->>compact_now: request status
alt status is 'ok'
compact_now-->>runPreflight: status: 'ok'
runPreflight-->>caller: return 'ok'
else status is 'compacted'
compact_now-->>runPreflight: status: 'compacted'
runPreflight-->>caller: return 'compacted'
else status is 'overflow'
compact_now-->>runPreflight: status: 'overflow'
runPreflight-->>runPreflight: throw ContextOverflowError
else status is 'busy'
compact_now-->>runPreflight: status: 'busy'
runPreflight-->>runPreflight: throw CompactionBusyError
else status is unknown
compact_now-->>runPreflight: status: 'empty' or other
runPreflight-->>runPreflight: log warning
runPreflight-->>caller: return 'ok'
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Summary
Fixes two failure modes found by investigating session
x7tshdba(chat died withresponse failed: from assistant_streaming: compaction already in progress), plus a critical jail-escape regression caught in pre-landing review of the fix itself.harness — compaction busy is transient, not terminal
CompactionBusyErrornow extendsTransientError. The async post-turn compaction of a large session can hold thecompactionlease (300s TTL) pastbusyTimeoutMs(30s);compact_nowthen returnsbusy, and the resulting throw used to hitrunTransition's catch-all and route the session to terminalfailed. It now re-throws so the durable turn-step queue retries the step once the lease releases — the same proven path session-lease contention already uses (run-transition.ts:94).runPreflightno longer conflatescompact_nowstatuses: only'ok'maps to'compacted'(caller reloads messages);'empty'/unknown statuses log a warning and proceed without claiming the context shrank.turn-stepqueue config atTURN_STEP_QUEUE(store.ts): steps go to the enginedefaultqueue whileengine.config.yamlconfigures aturn-stepFIFO (session grouping, max_retries 5) that nothing uses. Switching is a deliberate follow-up — it changes scheduling semantics for every step.shell — fs jail errors are recoverable, and the jail actually holds
validate_pathresolves relative paths against the canonicalhost_rootwhen jailed (agents commonly probe with./bare names; previously S210). The canonicalstarts_withcheck still bounds the joined path, so..cannot escape. Empty paths get an explicit S210.rm/chmod/mv/sedneed lexical (non-canonicalized) operands for unlink/perm/rename semantics and rebuilt them from the raw request string. With relative inputs accepted, the validated path (host_root/<rel>) and the operated-on path (<cwd>/<rel>) diverged — a destructive escape. Newlexical_operandhelper anchors relative inputs to the same rootvalidate_pathvalidated.Test plan
state: failed;instanceof TransientErrorcontract pinned; both new preflight status branches covered)cargo test --libgreen — 151/151 (+10 new: relative resolution under jail, dotdot escape, empty path jailed/unjailed, S215 names the root, cwd≠host_root regressions for rm/mv/chmod/sed)cargo fmt --all --checkclean; Biome clean on changed TSfs::ls/rmprobe with relative pathsFollow-ups (flagged, intentionally out of scope)
turn-stepFIFO queue (per-session ordering + bounded retries) — semantics change for every step, needs its own validationSummary by CodeRabbit
Release Notes
Bug Fixes
Improvements