Repository navigation
Conversation
Summary: - validate exact workspace, runtime envelope, profile, and session bindings - attach through the retained ACP owner and expose transcript/unavailable modes - document the owner seam and add exact-path/no-replacement regression tests Rationale: - IDE and terminal debug consumers need one harness-neutral binding boundary - owner loss must not silently launch a competing ACP session Tests: - cargo fmt --all - cargo check-system-duckdb - cargo clippy with system DuckDB and all targets - cargo test-system-duckdb
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98d8e9427a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .runtime_envelope | ||
| .as_ref() | ||
| .ok_or(AttachmentError::BindingMismatch( | ||
| "runtime envelope is missing", | ||
| ))?; |
There was a problem hiding this comment.
When the issue is a parent/multi-checkout execution, the production backend creates no TerminalRuntimeEnvelope because the parent workspace has no checkout generation, and persists the binding in run.parent_runtime_envelope instead. This unconditional access therefore rejects every valid parent ACP session as runtime envelope is missing, preventing parent issues from using the new debug attachment core. Validate either the terminal or parent envelope with the appropriate identity checks.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 198d74f: added parent runtime-envelope validation and current parent execution-root verification.
| Ok(owner) => { | ||
| let snapshot = owner | ||
| .inspect() | ||
| .await | ||
| .map_err(|error| AttachmentError::BindingMismatch(host_error_detail(error)))?; |
There was a problem hiding this comment.
When stale or copied conversation metadata names another live issue's owner_id with the same generation, SessionHost::lookup returns that issue's handle because it filters only by owner ID and generation. The inspected snapshot is not compared with the validated workspace/profile/run/session target, so the resolver marks the wrong owner as live and exposes its events and control operations, including attachment and retirement. Compare the returned snapshot's complete binding with the target before returning the handle.
AGENTS.md reference: AGENTS.md:L32-L35
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 198d74f: compare the live owner snapshot against the complete persisted ACP identity before exposing it.
| validate_binding( | ||
| &issue, | ||
| &run, | ||
| &conversation, | ||
| state.identity.clone(), |
There was a problem hiding this comment.
When the issue, run, and conversation manifests are mutually consistent but stale after a checkout generation, target commit, policy, or instruction change, this validation accepts them without checking the actual managed checkout. The existing native debug path calls WorkspaceManager::verify_runtime_envelope_for_retry, which checks the handle generation and current checkout provenance; this resolver should perform the equivalent verification before exposing an owner, otherwise it can attach to a session bound to obsolete workspace contents.
AGENTS.md reference: AGENTS.md:L32-L35
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 198d74f: revalidate terminal checkouts and parent execution roots against the current runtime envelope.
| if let Some(requested) = request.profile_id.as_deref() | ||
| && requested != state.identity.profile_id |
There was a problem hiding this comment.
When profile_id is omitted, as the request documentation permits, the resolver trusts the conversation state's profile and never checks run.acp_route or even that state.harness is acp. A stale conversation and envelope can therefore remain self-consistent while the current run selected another profile, causing attachment to the prior profile and credential/tool scope instead of rejecting the mismatch. Require the persisted route's harness and profile to match the ACP state before using it.
AGENTS.md reference: AGENTS.md:L165-L169
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 198d74f: require the persisted ACP route and profile to match the conversation state.
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | ||
| pub enum AttachmentMode { | ||
| Live, | ||
| TranscriptInspection, | ||
| Unavailable, |
There was a problem hiding this comment.
After owner loss, the debugging specification requires consumers to distinguish capability-gated restoration and fresh-context reset from transcript inspection, but this enum cannot represent either state and inspect_recorded_session collapses every record with a session ID to TranscriptOnly. A persisted session whose initialization advertises load/resume is therefore reported as transcript-only, while callers lose the actionable restoration status needed to recover it safely. Preserve the negotiated capability result and expose distinct restoration/reset modes without starting a replacement session.
AGENTS.md reference: AGENTS.md:L11-L19
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 198d74f: preserve and expose capability-gated restoration and fresh-context reset modes separately from transcript inspection.
| let snapshot = owner | ||
| .inspect() | ||
| .await | ||
| .map_err(|error| AttachmentError::BindingMismatch(host_error_detail(error)))?; |
There was a problem hiding this comment.
If the retained owner exits after lookup returns its handle but before inspect completes—for example when the idle-retention timer fires or the process terminates—inspect returns HostError::Unavailable, which is converted here into a binding mismatch. This normal owner-loss race should follow the same recorded-session fallback as an unavailable lookup so callers receive TranscriptInspection or Unavailable rather than a false metadata-corruption error.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 198d74f: treat owner-inspection loss as the recorded-session fallback path.
Summary: - Validate terminal and parent runtime envelopes against current workspace state. - Check persisted ACP route/profile and complete live-owner identity. - Preserve restoration, reset, transcript, and unavailable attachment modes. - Document the expanded attachment validation and regression coverage. Rationale: - Prevent stale or cross-issue owners from being exposed through debug attach. - Keep parent sessions attachable without launching competing runtimes. - Preserve capability-gated recovery outcomes after owner loss. Tests: - cargo test-system-duckdb --lib attachment::tests -- --nocapture - cargo test-system-duckdb --test acp_session_host -- --nocapture - cargo test-system-duckdb --test debug -- --nocapture - cargo clippy --no-default-features --features duckdb-prebuilt --all-targets -- -D warnings - cargo test-system-duckdb (1480 passed on rerun)
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 198d74fbb3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| session_id, | ||
| envelope, | ||
| )?; | ||
| manager.verify_runtime_envelope(workspace, envelope).await?; |
There was a problem hiding this comment.
When attaching after the active ACP agent has modified its checkout, this calls strict verify_runtime_envelope, which invokes Git verification with worker changes disallowed; consequently a normal dirty worktree is rejected before its retained owner can be attached. The existing native debug path deliberately uses verify_runtime_envelope_for_retry, which still validates the generation and envelope while allowing those expected edits, and the ACP resolver needs the same behavior.
Useful? React with 👍 / 👎.
| let snapshot = super::inspect_recorded_session(manager, workspace) | ||
| .await | ||
| .map_err(|error| AttachmentError::BindingMismatch(host_error_detail(error)))?; |
There was a problem hiding this comment.
If the scheduler advances the workspace to another run between the initial manifest validation and an unavailable owner lookup, inspect_recorded_session reloads the newer conversation manifest and this function returns that snapshot alongside the target built from the older run. Because the reloaded snapshot is never compared with target, callers can receive restoration or transcript status for a different profile, run, attempt, generation, owner, or native session; validate the fallback snapshot against the previously verified identity before returning it.
Useful? React with 👍 / 👎.
| if route.harness_kind != "acp" | ||
| || route.harness_profile.as_deref() != Some(state.identity.profile_id.as_str()) |
There was a problem hiding this comment.
When a terminal conversation contains stale or inconsistent ACP metadata and its owner is unavailable, validate_route verifies only the run route and profile, so a state.harness value other than acp reaches the recorded-session fallback and is returned as a valid ACP attachment target. Fresh evidence after the prior fix is that this revised condition still omits state.harness, while only the parent and live-owner paths perform that check; require the persisted state harness to match the ACP route before recovery data is exposed.
AGENTS.md reference: AGENTS.md:L165-L169
Useful? React with 👍 / 👎.
| match snapshot.state.recovery { | ||
| AcpRecovery::RestoredLoad | AcpRecovery::RestoredResume => { | ||
| AttachmentMode::CapabilityGatedRestoration { | ||
| recovery: snapshot.state.recovery, | ||
| } | ||
| } | ||
| AcpRecovery::Fresh => AttachmentMode::FreshContextReset, |
There was a problem hiding this comment.
When the owner is lost during a prompt, the persisted state can retain RestoredLoad or RestoredResume while its status is Submitted/Uncertain or its process is still recorded as running, but this match advertises CapabilityGatedRestoration solely from the recovery enum. A consumer following that actionable mode could attempt recovery while the prior submission or process may still be active; classify restoration/reset as unavailable or uncertain until the status and process prove quiescence.
AGENTS.md reference: AGENTS.md:L165-L169
Useful? React with 👍 / 👎.
| pub target: AttachmentTarget, | ||
| pub mode: AttachmentMode, | ||
| pub snapshot: SessionSnapshot, | ||
| pub owner: Option<SessionHandle>, |
There was a problem hiding this comment.
For every live resolution, this public field exposes the full SessionHandle, whose public methods include prompt, operation, and SessionControl::Retire; those commands require only the generation and do not require an orchestrator-owned writer lease. A terminal or IDE consumer can therefore start work or retire the retained owner immediately after attachment, bypassing the scheduler hold and transfer protocol that is supposed to authorize execution; return an observation-only facade and expose mutation only after orchestrator control is granted.
AGENTS.md reference: AGENTS.md:L23-L27
Useful? React with 👍 / 👎.
| || conversation.conversation_id != session_id.unwrap_or_default() | ||
| || envelope.conversation_binding.as_deref() != session_id | ||
| || envelope.acp_session.as_ref() != Some(&identity) |
There was a problem hiding this comment.
When the terminal conversation manifest contains a stale or partially updated runtime_envelope, this validation ignores it and accepts the run envelope as long as the nested ACP state happens to match. The parent path explicitly requires its conversation envelope to equal the run envelope, and the existing native debug verifier likewise requires both copies to agree; add the equivalent terminal comparison so inconsistent persisted metadata is rejected rather than attaching through only one side of the durable binding.
Useful? React with 👍 / 👎.
| } else if state.session_id.is_some() | ||
| && !matches!( | ||
| state.recovery, | ||
| AcpRecovery::RestoredLoad | AcpRecovery::RestoredResume | ||
| ) | ||
| { | ||
| state.recovery = AcpRecovery::TranscriptOnly; |
There was a problem hiding this comment.
When a persistence-capable session is created normally for the first time, its recovery value remains Fresh even though the stored initialization advertises load or resume; after that owner exits, this branch overwrites it with TranscriptOnly because it preserves restoration only when the previous owner was itself restored. The fresh evidence after the prior claimed fix is that the new condition never inspects the persisted initialization capabilities, so the common first owner loss still cannot expose capability-gated restoration.
Useful? React with 👍 / 👎.
| Debug attachment core regression tests also cover parent/store/nested-path | ||
| rejection, exact workspace binding, terminal and parent envelope verification, | ||
| persisted route/profile mismatches, complete owner identity comparison, the | ||
| no-runtime-binding failure path, owner-loss races, and the distinct | ||
| restoration/reset/transcript/unavailable modes. The host control-plane tests |
There was a problem hiding this comment.
This paragraph says the core tests cover envelope verification, route/profile mismatches, owner identity, owner-loss races, and every attachment mode, but the entire new attachment module contains only two tests: ordinary parent/nested-path rejection and the missing-run path. No other test invokes resolve_debug_attachment, so the claimed coverage does not exist and the substantive binding/state-machine behavior is left unverified; add the listed regressions or correct the evidence rather than documenting them as present.
AGENTS.md reference: AGENTS.md:L421-L427
Useful? React with 👍 / 👎.
1 participant
Summary
Add the harness-neutral attachment core for host-owned ACP debug sessions.
Changes
attempt, opaque session identity, and current terminal or parent envelope.
exposing an attachment.
SessionHandleowner seam onlyafter comparing the complete owner identity.
inspection, and unavailable context without starting a replacement session.
Evidence
Test output
Verification
The pre-change debug suite passed its existing OpenHands and Codex cases but
had no shared ACP attachment resolver. The resolver now rejects parent/store
paths, stale route/profile and envelope bindings, cross-owner snapshots, and
owner-loss races while preserving the existing native debug routes.
Checklist
cargo test-system-duckdb)cargo fmt --all)cargo clippywith system DuckDB and all targets)AGENTS.mdupdated (if repo knowledge changed)Type of change
Breaking changes
None. Existing OpenHands and Codex debug routes remain unchanged.
Related issues
Additional notes
The repository's
clippy-system-duckdbalias currently forwards its baked-in--all-targets -- -D warningsincorrectly under this Cargo toolchain.Validation used the equivalent direct command with the same system DuckDB
environment.