fix: guarantee the picked working directory reaches every shell/coder call - #388
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
skill-check — worker0 verified, 30 skipped (no docs/).
Four for four. Nicely done. |
|
Warning Review limit reached
Next review available in: 37 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?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 reviews. How do review 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 refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a ChangesHarness workspace scope stamping
Estimated code review effort: 3 (Moderate) | ~30 minutes DirectoryPicker unified validation
Estimated code review effort: 1 (Trivial) | ~5 minutes Sequence Diagram(s)sequenceDiagram
participant TurnLoop
participant WorkspaceInject
participant PreTriggerHooks
participant TargetCall
TurnLoop->>WorkspaceInject: inject(function_id, call.arguments, working_dir())
WorkspaceInject-->>TurnLoop: staged_args
TurnLoop->>PreTriggerHooks: run_pre_trigger(staged_args)
PreTriggerHooks-->>TurnLoop: eff_args
TurnLoop->>WorkspaceInject: inject(eff_args, working_dir())
WorkspaceInject-->>TurnLoop: scoped_args
TurnLoop->>TargetCall: invoke(scoped_args)
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
harness/src/types/turn.rs (1)
88-101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider centralizing the stamp call to reduce duplication and future drift.
The
working_dir()accessor is solid. Every call site in this cohort (turn_loop.rs×2,function_trigger.rs×2,deferred.rs×1) repeats the samecrate::workspace_inject::inject(&function_id, args, record.options.working_dir())pattern. A thin wrapper here (e.g.TurnOptions::stamp_workspace(&self, function_id, arguments) -> Value) would remove that duplication and reduce the chance of call sites drifting apart (see the cross-file comment onfunction_trigger.rsdocumenting one such drift in behavior description).♻️ Suggested helper
impl TurnOptions { pub fn working_dir(&self) -> Option<&str> { self.metadata .as_ref() .and_then(|m| m.get("working_dir")) .and_then(Value::as_str) } + + /// Stamp this turn's workspace scope onto a scoped `shell::*` / `coder::*` + /// call's arguments. Thin wrapper over `workspace_inject::inject` so every + /// call site derives the same `working_dir` the same way. + pub fn stamp_workspace(&self, function_id: &str, arguments: Value) -> Value { + crate::workspace_inject::inject(function_id, arguments, self.working_dir()) + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@harness/src/types/turn.rs` around lines 88 - 101, Centralize the repeated workspace-stamping logic around TurnOptions::working_dir() by adding a thin helper on TurnOptions, such as a stamp_workspace method that wraps workspace_inject::inject for a function_id and arguments. Then replace the duplicated crate::workspace_inject::inject(&function_id, args, record.options.working_dir()) calls in turn_loop.rs, function_trigger.rs, and deferred.rs with the new helper so the behavior stays consistent across all scoped shell/coder paths.
🤖 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.
Nitpick comments:
In `@harness/src/types/turn.rs`:
- Around line 88-101: Centralize the repeated workspace-stamping logic around
TurnOptions::working_dir() by adding a thin helper on TurnOptions, such as a
stamp_workspace method that wraps workspace_inject::inject for a function_id and
arguments. Then replace the duplicated
crate::workspace_inject::inject(&function_id, args,
record.options.working_dir()) calls in turn_loop.rs, function_trigger.rs, and
deferred.rs with the new helper so the behavior stays consistent across all
scoped shell/coder paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 54568fd1-4cdf-4804-87f4-d771f24f4cc9
📒 Files selected for processing (6)
console/web/src/components/chat/DirectoryPicker.tsxharness/src/deferred.rsharness/src/functions/function_trigger.rsharness/src/subagent.rsharness/src/turn_loop.rsharness/src/types/turn.rs
…n path The console-picked working directory is guaranteed reachable through the per-call base_dir stamp, but three invocation paths skipped it: - the deferred approval-release path invoked the recovered transcript arguments un-stamped, so an approved shell/coder call lost the session scope and a model-supplied base_dir survived un-stripped - sub-agent turns were seeded with no metadata, so children could never reach the parent session's directory - harness::function::trigger invoked its target without the stamp pre_trigger hooks now receive the stamped arguments (an approver reviews the base_dir the call actually runs under), with an idempotent re-stamp after the chain so a hook rewrite can never widen the scope. TurnOptions::working_dir() is the shared accessor for all stamp sites; children inherit only the parent's working_dir, never its per-turn tracing metadata.
… worker Browsed "use this folder" selections bypassed shell::workspace::validate. Every selection path — pasted, remembered, or browsed — now round-trips through validate, so a stale listing can't select a vanished directory and the worker-echoed canonical path is what gets stored.
ytallo
force-pushed
the
fix/workspace-scope-guarantee
branch
from
July 2, 2026 10:38
f68690b to
16cb669
Compare
Extract the working_dir metadata key into a single WORKING_DIR_KEY constant (mirroring workspace_inject::BASE_DIR_FIELD) used by both TurnOptions::working_dir (read) and subagent::inherit_workspace (write), so a rename cannot silently desync the two and drop child scope. Also complete the working_dir() doc enumeration to include function::trigger, the third stamping path.
andersonleal
added a commit
that referenced
this pull request
Jul 3, 2026
Upstream #388 added a test TurnRecord literal that predates this branch's display_parent_session_id / spawned_by_subscription_id / reactive_depth fields; the rebase merged clean but test compilation broke.
andersonleal
added a commit
that referenced
this pull request
Jul 3, 2026
Upstream #388 added a test TurnRecord literal that predates this branch's display_parent_session_id / spawned_by_subscription_id / reactive_depth fields; the rebase merged clean but test compilation broke.
andersonleal
added a commit
that referenced
this pull request
Jul 3, 2026
…-in, wire hardening, and spawn console view (#401) * feat(harness): reactive trigger bridge (harness::react) with join fan-in and lifecycle hardening Ports the engine's trigger/notify primitives into a harness-native reactive sub-agent bridge and hardens the full registration/fire/teardown lifecycle against gaps found in live testing. - harness::react: sub-agent spec fires on engine triggers (turn events, state, cron, stream); join fan-in with an expect array, fire-once accumulator, and rearm for standing watchers. - Interceptor pass-through (subscribe.rs): agent-issued engine::register_trigger calls get owner + subscription id stamped server-side into the react metadata, closing several trust gaps in the raw registration path. - Idempotent registration (dedup by canonical request key) and a durable owner sweep on session::deleted, replacing two pipelines that could double-register the same reaction. - Startup reconcile: GC react bindings whose owner session is gone and notify bindings unknown to the local registry; never GC on doubt. - Loop breakers: self-edge drop, reactive-depth cap, per-subscription fire-rate limit. - Join results deliver into the registering (owner) session by default instead of a detached, unread child session; parent nesting falls back from the event's session through the owner stamp to resolve_root. - Registration advisories for turn-event filters naming a nonexistent session, and for a join key wired to the same event source as a sibling key. - Policy aid: narrowed sub-agents are told their allowed/denied function surface directly in the system prompt instead of discovering it via a denied functions::list call. - Heal dangling function_calls left by interrupted/compacted turns before the next generate step. - Docs: tech spec, skill, and all prompt variants updated for the react/join doctrine. * feat(console): dedicated chat view for harness::spawn Replace the raw-JSON fallback card with an instrument-panel view: policy chips (model/mode/turns/thinking/output/allow/deny), the task rendered as markdown, and the child's result as markdown, highlighted JSON, or the direct-call child ids. Guard errors and failed children route through the existing SandboxErrorView; the approval gate gets a policy-first preview. Session ids link to the child conversation via the sidebar's select when the console knows the session. Includes Zod parsers for the spawn wire schema (excerpt-tolerant), fixtures for all six card states, a gated-spawn playground scenario, and parser tests locking envelope unwrapping and error-before-success dispatch. * fix(harness): seed react-bridge TurnRecord fields in subagent test Upstream #388 added a test TurnRecord literal that predates this branch's display_parent_session_id / spawned_by_subscription_id / reactive_depth fields; the rebase merged clean but test compilation broke. * feat(harness): prompt doctrine — name every spawned child session Every harness::spawn must pass session_id: a short readable job slug plus a few random characters (fetch-headlines-b4k9), replacing the opaque engine-minted UUIDs in the console tree. Never the parent session id as a prefix; the random suffix carries the run-uniqueness guarantee instead (a reused id silently resumes the old session). Scoped to direct spawn calls only — in a react trigger's metadata a fixed session_id funnels every firing into one session and re-aims join delivery. Fan-in doctrine updated to the same naming across all five prompt variants. * fix(providers): keep displaced tool results adjacent to their call A notification or steering user entry injected while a call window is open (a parked harness::spawn holds one open for minutes) lands between function_call and function_result in the durable transcript. Every wire mapper only repaired MISSING results (orphan placeholder) — a DISPLACED result survived to the wire as assistant(tool_use) / user(text) / user(tool_result), which Anthropic 400s ('tool_use ids were found without tool_result blocks immediately after') and OpenAI/xAI/Responses reject as a user row between tool_calls and its tool rows. The durable transcript replays the shape on every retry, permanently wedging the turn. Fix: shared llm_router::types::messages::reorder_displaced_results runs first in all four providers' to_wire_messages — each FunctionResult moves directly after the assistant that emitted its call, order preserved, orphan results untouched. Wedged sessions self-heal: the transcript itself was never illegal, only the wire projection. Repro test written first and failed with the exact live shape; regression tests in all four providers plus unit tests on the shared helper. * fix(harness): rotate mid-generation user arrivals past the interrupted reply A user entry appended while a step is generating (or assembling — the compaction/hook window) lands before that step's assistant entry in the durable log. The steering check then re-generates, but the assembled context ENDS with a call-less assistant message — a prefill request newer Anthropic models reject ('This model does not support assistant message prefill. The conversation must end with a user message.'), wedging the turn on every retry. Older models silently accepted prefill, hiding this path. Fix: rotate_mid_generation_users presents arrivals after the previous step's watermark AFTER the reply they interrupted — semantically exact, the model answered without seeing them. Two invariants hardened by adversarial review: - the new watermark is assigned only after router.chat returns; the pre-generate put_turn persists the OLD one, so a redelivered step keeps its rotation window instead of re-issuing the rejected shape forever - rotation runs on the FINAL assembled values, never on the candidate: compaction persists tail_start_entry_id as a log-order cursor indexed from the candidate, and rotating first would silently drop the rotated message from every future window Also: has_user_after_watermark now loads include_custom=true, matching the list the watermark comes from (a watermark landing on a custom entry silently disabled the steering check). * style: cargo fmt (harness, provider-openai, provider-xai) * fix(harness): Display for DispatchError + fmt (rebase fallout) Main's fs-scope refactor (#397) introduced the bare DispatchError struct; the react-bridge reconcile pass logs it with %e, which needs Display — an error type should carry one anyway. * docs(harness): revert harness.md spec changes Restore tech-specs/2026-06-agentic/harness.md to main's version — the react-bridge spec additions come out of this PR. * fix(harness): address CodeRabbit review on #401 - react: include the join (id, key) in the fallback fire-gate hash — state-based join predecessors share the whole downstream spec except their key, so a wide join shared one 10-fires/min budget and tripped the breaker spuriously - react: retry the join accumulator delete (3 attempts) — a failed delete left fire=1 behind, permanently wedging a rearmed join's fire-once guard; persistent failure on a rearmed join now logs at error level with the recovery path - spawn: strip spawned_by_subscription_id / reactive_depth on the model-reachable dispatch path — react-internal bookkeeping a model could spoof to defeat the self-edge breaker and depth cap - skills: align SKILL.md fan-in naming with the prompt doctrine (slug + random suffix, never the originating session id as a prefix) - tests: multi-owner displaced-result reorder case; round-trip the react-bridge TurnRecord fields with real values
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.
Summary
When a working directory is picked in the chat console, it must be reachable from every
shell::*/coder::*call the session makes. The mechanism for that guarantee already exists — the harness stamps the turn'sworking_dironto each scoped call asbase_dir, and the shell worker adopts an operator-pickedbase_diras an effective jail root per call — but three invocation paths skipped the stamp, silently breaking the guarantee:deferred.rsexecute): a hook-held call, once approved, was invoked with the transcript-original arguments and no stamp — the approved shell/coder call ran outside the session scope, and a model-suppliedbase_dirrecovered from the transcript survived un-stripped (a scope-widening hole).subagent.rs): child turns were seeded withmetadata: None, so the picked directory was unreachable from every child (and grandchild) turn. Children now inherit exactly the parent'sworking_dir— never its per-turn tracing metadata.harness::function::trigger(function_trigger.rs): the direct-invoke entry point never stamped, with the same un-scoped/escape consequences. With no turn record, a caller-suppliedbase_diron a scoped call is now stripped.Two related hardenings ride along:
pre_triggerhooks now receive the stamped arguments, so an approval gate reviews thebase_dirthe call will actually run under (previously it saw the call without its scope, or with a bogus model-supplied one). The stamp is re-applied after the hook chain, so a hook rewrite can never widen or drop the scope.shell::workspace::validatelike pasted/remembered picks — every selection path is validated against the live worker and stores the worker-echoed canonical path.TurnOptions::working_dir()is the new shared accessor; the turn loop, deferred release,function::trigger, and the system-prompt working-dir aid all read the same source.Known gaps left out (deliberate contracts, follow-up decisions)
working_diruntil the next turn (turn options are frozen by design).base_dirby design (it is a host path).coder::infohas nobase_dirfield and describes the unscoped jail to a scoped model.Test plan
cargo testinharness/— 119 passed, including new unit tests forTurnOptions::working_dir()and sub-agentworking_dirinheritance (only-inherit-the-scope, no-parent, unscoped-parent cases)cargo clippy --all-targetsandcargo fmt --checkcleanconsole/web:tsc -b --noEmit,vitest run(803 tests, 52 files),biome checkon the changed file — all cleanshell::execand a sub-agent spawn, confirm both operate in the picked directorySummary by CodeRabbit
New Features
Bug Fixes