Repository navigation
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
github-actions
Bot
added
size: L
![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: 80d1bede59
ℹ️ 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".
| // Verify + claim BEFORE any turn transition. | ||
| let verified = port | ||
| .verify_and_claim(&scope, &actor, run_id, &gate_ref, &claim) | ||
| .await | ||
| .map_err(map_attested_continuation_rejection)?; |
There was a problem hiding this comment.
When proof_json contains a valid proof for the authoritative hash but the separate attested_approved_tx_hash is malformed or different, the production verify_and_claim path verifies the hash embedded in proof_json and irreversibly claims the grant/advances the signing ledger before this top-level value is validated by AttestationClaimRef and the resume port. The later resume then fails and leaves the run BlockedAttested, while a corrected retry is rejected because the one-shot grant and ledger row were already consumed. Validate and bind every resume input before claiming, or make the claim and turn transition recoverable/atomic.
AGENTS.md reference: AGENTS.md:L189-L190
Useful? React with 👍 / 👎.
| R::Unavailable | R::BackendUnavailable => { | ||
| ProductSurfaceError::internal_from("attested-signing continuation is unavailable") |
There was a problem hiding this comment.
When signing or broadcast infrastructure fails, the composition adapter maps the failure to Unavailable or BackendUnavailable, but this branch calls internal_from, which always produces Internal/HTTP 500 with retryable: false. A transient RPC or backend outage therefore loses the service-unavailable and retry semantics documented for the continuation path, preventing callers from distinguishing a recoverable outage from an internal invariant failure; map these cases to the intended sanitized 503 response with the appropriate retry flag.
Useful? React with 👍 / 👎.
🔎 Review · PR #6822
2 actionable findings →The attested ingress is fail-closed for invalid proofs and ownership mismatches, but the new orchestration cannot recover from broadcast-tail failures after transitioning the turn, and it misclassifies continuation outages as non-retryable internal errors. Automatic · PR opened · attempt 1 of 3 · completed in 1m 55s Run details
|
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Review complete · PR #6822
The attested ingress is fail-closed for invalid proofs and ownership mismatches, but the new orchestration cannot recover from broadcast-tail failures after transitioning the turn, and it misclassifies continuation outages as non-retryable internal errors.
Findings
- 🔴 High · Broadcast failure leaves the resolved turn with no retry path —
crates/ironclaw_product/src/reborn_services.rs:5968-5971
Details are attached to the relevant diff. - 🟠 Medium · Recoverable continuation outages are returned as non-retryable 500s —
crates/ironclaw_product/src/reborn_services.rs:6687-6690
Details are attached to the relevant diff.
Validation and technical details
- Reviewed the complete trusted comparison
refs/ironloop/base(12384b4) torefs/ironloop/head(80d1bed): five changed files, 444 additions and 3 deletions. - Inspected crate-local AGENTS.md/CLAUDE.md guidance, request parsing and proof-size validation, run-state routing, ownership checks, continuation verification/claim/broadcast behavior, composition wiring, turn-store attested resume transitions, and all changed tests.
git diff --check refs/ironloop/base..refs/ironloop/headcompleted without whitespace errors.- Focused Rust tests could not be executed because
cargois not installed in the review environment (cargo: command not found). - Base:
attested-g8-ledger-product - Head:
attested-g9-gate-ingressat80d1bed - Run:
68c8f1d4-a174-4c7c-ac29-6ccb06acd2dd
| // Signed and claimed; now broadcast. No re-verification, no re-claim. | ||
| port.broadcast_resolved(&scope, run_id, &gate_ref, verified) | ||
| .await | ||
| .map_err(map_attested_continuation_rejection)?; |
There was a problem hiding this comment.
🔴 High · Broadcast failure leaves the resolved turn with no retry path
verify_and_claim consumes the one-shot grant, then resume_turn durably advances the run to AttestedResolved, and only afterward does this call broadcast. If broadcasting returns an RPC/backend error, the opaque verified handle is dropped and the request returns an error even though the gate is already resolved. Retrying the endpoint cannot recover: routing rejects the now non-blocked/terminal state before reaching the continuation, and re-running verification would encounter the already-claimed grant anyway. This can strand an approved signing operation without broadcast or intent projection. Persist a recoverable continuation/outbox before transitioning, or make transition plus continuation handoff durable so a worker can retry the broadcast tail.
| match rejection { | ||
| R::Unavailable | R::BackendUnavailable => { | ||
| ProductSurfaceError::internal_from("attested-signing continuation is unavailable") | ||
| } |
There was a problem hiding this comment.
🟠 Medium · Recoverable continuation outages are returned as non-retryable 500s
Both Unavailable (documented as a post-verification broadcast-tail failure) and BackendUnavailable are mapped through ProductSurfaceError::internal_from, which produces Internal, HTTP 500, and retryable: false. This contradicts the rejection contract and prevents clients/operators from distinguishing a recoverable service outage from an invariant failure. Map recoverable backend/broadcast unavailability to a sanitized Unavailable 503 with the appropriate retryability; keep any genuinely terminal category separate.
github-actions
Bot
added
size: XL
12384b4 to
c046764
Compare
2669c9a to
0653c77
Compare
c046764 to
09e9bba
Compare
051f029 to
d9cbd0f
Compare
Review-response summary (g9 — gate resolve on capability dispatch)Force-pushed several times, so inline anchors have moved. Consolidated response to the 4 findings here, plus a summary of what this branch grew — it changed the most of any PR in the stack. Five commits added since review
The authorization bypass (HIGH)
Now fail-closed: the intercept is gone, so Two pre-existing defects surfaced while testing this, both real and both fixed:
The 4 findings on this PR — genuinely openNone are fixed in this push:
Process noteI force-pushed g4–g8 once without compiling them, and a test build was red as a result (an upstream signature change broke a mock). Caught at the stack tip. All nine branches have since been compiled individually with Also: the Verification: clippy clean at |
8bc62a5 to
a814f8f
Compare
251945c to
8c00165
Compare
Re-expresses the attested gate/resolve ingress for current main. The old stack routed it through a WebUI-inbound facade that main deleted outright -- webui_inbound has zero references there -- and replaced with capability/command dispatch. RESOLVE_GATE_COMMAND already exists, so an attested resolve is a GATE RESOLUTION CARRYING A CLAIM, not a new command. It therefore inherits the existing authorization, audit, and safety pipeline rather than standing up a second dispatch surface. Wire shape: ProductResolveGateRequest gains three optional attested fields, parsed into a ProductGateResolution::Attested variant. All three are required together -- a partial proof is a validation error, never a silently-defaulted claim carried on to the device path. The payload is opaque and passes through uninterpreted; a test pins that an arbitrary JSON shape is not reshaped here. The cross-user IDOR contract is the load-bearing part. An attested resolution routes to the attested resolver EVEN WHEN THE RUN CANNOT BE READ -- which is exactly what a cross-user request looks like. That resolver runs its own owner-scoped verify-and-claim and fails closed indistinguishably from a missing gate. Falling through to the generic route would instead reject the resolution SHAPE with a 400, confirming to the caller that the request was attested-shaped at all. Gate-ref shape cannot divert it either. Mutation-verified: removing the early route lets two tests fail. Ordering is the other contract: verify + claim the one-shot grant BEFORE the turn transitions, resume only on success, then broadcast. A rejected proof leaves the turn BlockedAttested with no state-machine mutation, so the gate stays cleanly retryable and an unverified proof never advances a signing turn. The approval and auth resolvers, and the generic fallback, all refuse an attested resolution rather than resuming it -- the generic path has only one-shot resume_turn and no verification. Every rejection collapses to one of two shapes on the way out. A caller must not be able to distinguish a missing binding from a forged proof from an already-claimed grant; that distinction is an oracle for which gates exist and which have been driven. Backend unavailability is the one genuinely-our-fault case and is reported as such. ProductGateResolution and ProductInboundCommand drop their Eq derives: the proof payload is serde_json::Value, which is PartialEq but not Eq. The port is wired in composition only when the runtime composed both an attested signing graph and an intent store; absent either, an attested resolution is refused.
…ntime
Every piece of the attested-signing path shipped and was unit-tested, but
nothing ever constructed it. build_backend_production -- the sole runtime
construction site, serving both production and local-storage
production-shaped deployments -- hardcoded attested_signing: None and
intent_store: None, and no AttestedRaiseHook was handed to the host runtime.
The gaps compounded across four seams, so the feature was inert end to end:
1. product_surface registers the attested continuation only when BOTH
attested_signing() and intent_store() are Some, so the resolve-gate
ingress had nothing to dispatch to and refused every resolution.
2. production_turn_state_store received attested_resume_port: None, so the
turn store never admitted an attested resume.
3. With no raise hook, request_signature could never raise a gate.
No gate could be raised, and no gate could be resolved.
Wire all four: build the composition and intent minting, share one binding
store between the composition and the resume port (two stores would let a
gate be resumed that the driver cannot verify), and register the raise hook.
The stores are in-memory, which is fail-closed rather than merely
non-durable: grants, claims, and bindings share a lifetime, so a restart
drops a pending gate entirely instead of leaving a resolvable grant whose
claim record was lost. Swapping in the durable backends from
attested_durable additionally requires erasing RebornRuntime.attested_signing
behind a trait -- it is typed to the in-memory monomorphization today --
which is its own change.
The behaviour of each piece was already covered; what no existing suite could
see was whether anything built them. The new test asserts exactly that, and
drives the raise hook through the real capability path rather than an
accessor. Its discriminating assertion is mutation-verified: dropping
with_attested_raise_hook fails it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…kends
RebornRuntime.attested_signing was typed to the CONCRETE
InMemoryAttestedComposition, so the durable PostgreSQL and libSQL
compositions -- fully assembled and tested since the durable-stores group --
could not be stored in it at all. Not a wiring oversight: a type error. The
runtime could only ever hold one backend shape, and it was the in-memory one.
Introduce two narrow dyn-safe seams:
* SignerContinuationDriver (attested_runtime) -- exactly the three calls the
resume path makes: assert_binding_owner, verify_and_sign,
broadcast_signed_continuation. Kept minimal deliberately; widening it
would let callers reach past the ownership assertion into verify/
broadcast.
* AttestedComposition (composition) -- bindings() + driver(). Raise-path
methods stay OFF it: the raise hook holds the concrete composition, so
exposing register_attested_gate here would hand every holder of the
erased graph the ability to seal grants.
RebornAttestedContinuation now holds the erased driver, so the resume path
cannot observe which persistence backend it is running on.
This is the blocker removal only -- no behaviour change, and the production
path still composes the in-memory graph. Selecting the durable backends
additionally needs DurableBackend threaded into build_backend_production
(it is consumed earlier, on RootFilesystemBundle) plus RPC-endpoint config,
which is a separate change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The erasure made durable compositions storable; this makes one get chosen.
build_backend_production now receives the deployment's database handle and
selects the durable graph when THREE things hold together: a database handle,
the attested-broadcast feature, and at least one configured chain RPC
endpoint. All three are required because a durable graph without a
broadcaster would persist grants for signatures it can never submit --
durable bookkeeping around a dead end. Falling back never weakens a check; it
narrows durability, and a restart then drops pending gates outright rather
than leaving them half-resolvable.
Three supporting changes fell out of doing this honestly:
* assemble() now takes the binding store TYPED rather than as an erased
handle, and derives both the async store view and the sync resume-port
view from that one value. Two parameters could be handed different
stores, and a resume port that admits a gate the driver cannot verify is
exactly the divergence this path must not have. The durable module
already closed this gap for its own assembly; assemble() was the way
around it, and a test was in fact taking that route.
* The resume port is now built FROM the composition, so it reads the same
bindings the driver verifies against on every backend.
* present_env() reads through the shared runtime-env seam instead of
std::env::var, so attested config honours the same override mechanism as
the rest of composition -- and so selection is testable without set_var
(this crate forbids unsafe).
AttestedComposition gains broadcasts(), which is the documented guard made
observable: the in-memory fallback wires the non-submitting broadcaster, and
"signed but silently never broadcast" is the one failure this path cannot
otherwise detect from outside. The three selection tests pin the
discriminator in both directions.
Custodial keys stay process-scoped even on the durable path, which is
deliberate: custodial mainnet signing is already refused without a KMS
backend, and the durable path exists for the external-wallet ceremonies,
which hold no key here. Durable custodial custody means wiring KMS.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
invoke_capability intercepted request_signature and called the composition
raise hook directly, returning BEFORE invoke_json ran the kernel authorize()
fold. That skipped trust classification, capability grants, runtime policy,
credential pre-flight, persistent approval, and the sealed Authorized
witness. Demonstrated: this exact call with an EMPTY grant set reached the
hook, so an ungranted caller -- prompt injection included -- could mint a
human-facing approval prompt for a transaction of its choosing. The human
approval boundary still stood, but a signing capability must not depend on it
as its only gate.
It was a rebase artifact, not a design decision: authorization used to run
before invoke_capability and upstream moved it inside the fold. An external
security review rated it HIGH and rejected the two cheap fixes -- partially
reimplementing authorization in production.rs would create a second,
divergent authority path, and authorize() cannot serve as a pre-check because
it starts run state, prepares obligations, and mints a single-use witness.
Fail-closed for now: the intercept is gone, so request_signature flows
through the fold like every other capability and reaches only the
deliberately-refusing first-party handler. Raising is unavailable rather than
unauthorized. The correct fix is to raise as an AUTHORIZED DISPATCH RESULT --
invoke the hook from the dispatcher, which already receives the sealed
Authorized, and carry the gate back through a neutral typed deferred channel
that DefaultHostRuntime maps to AttestedSigningRequired, the same shape
ApprovalRequired and AuthRequired already use. The kernel stays free of
attested/chain/crypto types.
Two pre-existing defects surfaced while testing this, both real:
* builtin.request_signature referenced an input schema
(schemas/builtin/request_signature.input.v1.json) that was never added,
so granting the capability failed surface construction outright -- it
could never have been used. Added, mirroring RequestSignatureParams and
deliberately leaving `decoded` free-form so a chain-free layer does not
grow a second source of truth about transaction shape.
* The builtin package expectations omitted the capability entirely and
defaulted its permission to Allow; it declares Ask.
Tests now pin the boundary: an ungranted caller must be refused with
Authorization, and a wired hook must NOT be reached from invoke_capability
even for a granted caller. Both carry the note that they flip when the
authorized-dispatch rework lands.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The test set IRONCLAW_DISABLE_OS_KEYCHAIN via raw std::env::set_var, which trips the hermetic-env guard (process env is global, and the mutation is UB on 1.82+). It also was not needed: the justification comment claimed the custodial keystore build reaches for the OS keychain, but SecretsCrypto's generate() only draws random bytes. Verified by running the test with the variable unset in the environment -- it passes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
a814f8f to
6487113
Compare
8c00165 to
7f1e445
Compare
This branch has not been deployed
1 participant
What
The attested gate/resolve ingress, deferred from #6769 and now re-expressed for current
main. Stacks on #6818.The old stack routed this through a WebUI-inbound facade that main deleted outright —
webui_inboundhas zero references there — and replaced with capability/command dispatch. SinceRESOLVE_GATE_COMMANDalready exists, an attested resolve is a gate resolution carrying a claim, not a new command. It therefore inherits the existing authorization, audit, and safety pipeline instead of standing up a second dispatch surface.The cross-user IDOR contract — the load-bearing part
An attested resolution routes to the attested resolver even when the run cannot be read — which is exactly what a cross-user request looks like. That resolver runs its own owner-scoped verify-and-claim and fails closed indistinguishably from a missing gate.
Falling through to the generic route would instead reject the resolution shape with a 400 — confirming to the caller that the request was attested-shaped at all. Gate-ref shape cannot divert it either.
Mutation-verified: removing the early route makes two tests fail.
Ordering is the other contract
Verify + claim the one-shot grant before the turn transitions; resume only on success; then broadcast. A rejected proof leaves the turn
BlockedAttestedwith no state-machine mutation, so the gate stays cleanly retryable and an unverified proof never advances a signing turn.The approval resolver, the auth resolver, and the generic fallback all refuse an attested resolution rather than resuming it — the generic path has only one-shot
resume_turnand no verification, so resuming there would be the exact failure this design prevents.Sanitized rejections
Every rejection collapses to one of two shapes on the way out. A caller must not be able to distinguish a missing binding from a forged proof from an already-claimed grant — that distinction is an oracle for which gates exist and which have been driven. Backend unavailability is the one genuinely-our-fault case and is reported as such.
Wire shape
ProductResolveGateRequestgains three optional attested fields, parsed into aProductGateResolution::Attestedvariant. All three are required together — a partial proof is a validation error, never a silently-defaulted claim carried on to the device path. The payload is opaque and passes through uninterpreted; a test pins that an arbitrary JSON shape is not reshaped here.ProductGateResolutionandProductInboundCommanddrop theirEqderives: the proof payload isserde_json::Value, which isPartialEqbut notEq.The port is wired in composition only when the runtime composed both an attested signing graph and an intent store; absent either, an attested resolution is refused.
Verification
8 new tests (4 wire-layer, 4 routing/IDOR), full
ironclaw_productsuite,ironclaw_architecture100%, whole workspace compiles including tests,fmt,clippy --testszero warnings,check_no_panics.py,check-hermetic-env.sh— all clean against currentmain.