[freeqaz]

Problem

Git expiry never converged after a detach intent that never took effect. subtractExpiredOpenShellGit returned pending whenever the Git provider was still attached and a request ID was recorded without a confirmed epoch. Every later pass did the same, so the worker rechecked REPOSITORY_WITHDRAWAL_UNCONFIRMED forever. Three ways to get there:

• a crash between beginDetach and the send;
• a send that failed with UNAVAILABLE or DEADLINE_EXCEEDED;
• a pass aborted after the intent.

The "intent without effect" conformance case pinned that loop.

Decision (provisional; a maintainer can overrule it)

Resend the same request. When the provider is still attached at the recorded Sandbox version, the next pass runs the usual pre-send checks and then repeats the identical DetachSandboxProvider request: same request ID, same expectedResourceVersion. If the Sandbox version moved since the intent, the runtime is retired without a send.

Why this is safe, from OpenShell 0.1.5 at the pinned revision (bdb956f2):

• DetachSandboxProvider runs under durable request-ID mutation replay (crates/openshell-server/src/grpc/mutation_replay.rs; proto/openshell.proto; docs/sdk/api-errors.mdx).
• The claim is keyed by caller identity, method, workspace and request ID, and holds a fingerprint of the full request, the version precondition included.
• How the gateway answers a repeated request:
  • Completed: it replays the original result. Nothing is applied twice.
  • Admitted, outcome unknown: FAILED_PRECONDITION (REQUEST_OUTCOME_UNCERTAIN), which retires the runtime.
  • Different payload under the same ID: FAILED_PRECONDITION, which also retires the runtime.
  • Never seen (the first send never arrived): it runs fresh under the same version precondition, the same request the first send would have been, after the same fresh check of the Sandbox ID, launch hash and version.
• So a resend can't double-apply, and it is exactly as safe as the first send.

The alternative was to retire the runtime after a bounded number of passes. It would also converge, but it would end the conversation in the common case: a send that simply never reached the gateway.

Change

• CredentialDeliveryReductions.desire() also returns the recorded expected_version. The column is already written by begin(), and migration 0062 requires it whenever there is a request ID. The route store's expire() returns it as detach.resourceVersion.
• subtractExpiredOpenShellGit, while the provider is still attached:
  • A confirmed epoch, or a recorded request whose version differs from the Sandbox's current version, retires the runtime (attachment-after-send-intent).
  • Otherwise it runs the same before evidence and double inspection as a first send, then sends the recorded request again (repeated-conditional-detach). A first send still records a new intent first.
  • A repeated send that the gateway answers with FAILED_PRECONDITION (it holds the earlier request without a result, which may still be applying) reads the Sandbox once more. If the provider is now absent, the existing OBSERVE path settles it; otherwise the runtime retires.
• A detach that took effect is still only observed after a lost reply, never sent again.
• Comments and docs (flow and reference) describe the new rule.

Verification

• tests/conformance/openshell-git-runtime.test.mjs, "Git expiry observes the original model...":
  • The fake gateway now models request-ID claims, as the pinned server does. It replays a completed request for the same payload, refuses an admitted request without a result (or with another payload) with FAILED_PRECONDITION, and runs an unseen request under its version precondition. On a mismatch, the claim stays unresolved and the gateway answers ABORTED.
  • Every send must carry the recorded ID and the recorded version. Mismatches are collected rather than asserted inside the call, because an assertion there would read as a transport failure.
  • New and changed cases (each fails on main):
    • "intent without effect": pending, then withdrawn. Two sends, one admitted.
    • "outcome uncertain at gateway": pending, then retire-runtime.
    • "first send still applying": pending, then withdrawn. The repeat is held as uncertain while the first send lands, and the provider is then observed absent.
    • "Sandbox moved after intent": pending, then retire-runtime, with no second send.
    • "Sandbox moved before send": retire-runtime (ABORTED).
  • Mutants killed: a new request ID on the repeat, no version check, the old pending-forever rule, ABORTED taken as pending, and an uncertain repeat retiring without a fresh read.
  • All other cases are unchanged. A detach that took effect is never sent again.
• tests/integration/postgres-openshell-git-routes.test.ts: expire() returns the recorded resourceVersion beside the request ID.
• Local, Node 24:
  • Conformance file: 43/43 passed. Merged with the companion route-bookkeeping fix for finding 2218, the file passes 45/45.
  • Real PostgreSQL 18.6: postgres-openshell-git-routes (8/8), postgres-openshell-standing-git-transfer (its lost-reply journey still sends one detach) and postgres-openshell-standing-route: all passed (standing-git-transfer 1/1 on the final head; standing-route 6/6 on the first commit, before the uncertain-repeat change).
  • tsc --build (repo + tests), ESLint and Prettier on the touched files: clean.
  • The flow doc stays under its word limit (2,492 words).
• Pinned: tests/conformance/openshell-git-runtime.test.mjs passed 20/20 runs at 2 CPUs each, on the final head.
• Internal review (security-adjacent, high effort): approved with nits. It spot-checked the replay semantics at the pinned revision and the client's error-code mapping. Its should-fix on fake-gateway fidelity, and its nits on settling an uncertain repeat, the docs wording and the comment, are applied.

Risks

• A resend that reaches a gateway with a reset replay database, after the 24-hour replay window, or under a different caller identity, runs as a fresh conditional detach. That is the same request as the first send, after the same fresh checks.
• A same-named replacement Sandbox at the same resource version is the one case the version precondition alone does not cover. The send has the same exposure, and the inspection just before each send checks the original Sandbox ID.
• Each resend that the gateway refuses as uncertain repeats only a replay lookup, not a new admission.
• The repeat carries no stored authority. The gateway authorizes each call against the worker's own gateway identity, exactly as for the first send. If that identity has lost Workspace access, the call is refused (PERMISSION_DENIED) before any mutation, and the runtime retires. The mutation itself can only remove Git access.
• Not proven here: a run of the real client against a pinned gateway, recovering from an unapplied send. The fake gateway in the conformance test models the pinned server's replay rules, which were read from source. A deployed proof still needs a test install.
• If the gateway's per-caller admission limit is full, a repeat that was never admitted stays pending until slots free up. The first send had the same exposure; this change adds no new bound for that case.

[clawsweeper]

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

[clawsweeper]

Codex review: needs real behavior proof before merge.

What this changes

This PR retries an unapplied OpenShell Git detach using its recorded request ID and Sandbox resource version.

Example: A detach records Sandbox version 13, but its first RPC never reaches the gateway.

• Before: Every recovery pass returns pending while Git remains attached.
• After: Recovery repeats the recorded request at version 13 and verifies removal; a changed version requires runtime retirement.

Review scores

Measure	Result	What it means
Overall readiness	🧂 unranked krab (1/6)	The focused repair has no identified correctness defect, but the prior authority-chain proof requirement remains unmet on the unchanged head.
Proof confidence	🧂 unranked krab (1/6)	Real behavior proof is necessary before merge. See Before merge.
Patch quality	🦐 gold shrimp (3/6)	No actionable review findings were identified.

Product

Kind: Bug fix · Worth it: Yes · Fix scope: Complete
User problem: Repository expiry can leave cleanup pending forever after a detach intent whose RPC never took effect.
Reason: The source-backed recovery defect warrants a focused repair, and maintainer authorship establishes direction without another product approval.

Merge readiness

⛔ Blocked before merge - 1 item remains

Keep this PR open: it addresses a current-main recovery loop, and inspection found no introduced correctness defect.

Priority: P2
Reviewed head: 09907ba00fa697a21a32f86dc9d241c7bdf94d7f

Before merge

• Add real behavior proof - Authority-chain proof required: at head 09907ba, the new recovery cases substitute the gateway method, and the author explicitly reports no real-client recovery run. Supply redacted real-client and pinned-gateway request/response traces showing an unapplied send recovering under the same request ID and version, plus rejection before mutation when the caller loses Workspace access between inspection and the repeated RPC. Include the final Sandbox attachment state and redact credentials, IPs, and non-public endpoints; then request re-review. After adding proof, update the PR body, then request a new review with @clawsweeper re-review. The PR author or someone with repository write access can request it.

Findings

None.

Tests

• Missing end-to-end proof: The added recovery scenarios replace the gateway method; real-client recovery and revoked-caller rejection at the pinned gateway remain unproved.

Agent review details

How this fits together

The controller’s repository-expiry workflow reads durable OCC delivery state, invokes the OpenShell gateway, and returns withdrawn, pending, or retire-runtime after checking the original Sandbox and retained model execution.

flowchart TD
  A[Repository expiry worker] --> B[OCC delivery state]
  B --> C[Git expiry reconciler]
  C --> D[OpenShell gateway client]
  D --> E[Gateway authorization and replay]
  E --> F[Sandbox provider attachment]
  F --> C

Loading

Technical review

Best possible solution:

Retain the focused retry in the existing expiry owner, preserving exact request identity, conditional mutation, and observed removal.

Do we have a high-confidence way to reproduce the issue?

Current-main source establishes the loop: an attached provider plus a recorded request without an epoch returns pending on every pass; no target code was executed during this review.

Is this the best way to solve the issue?

The repair uses the existing lifecycle and storage owners, preserves the gateway’s request fingerprint, and retires rather than resending when the recorded precondition no longer holds.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 38ecfca18762.

Provenance checked

• Git expiry retry and durable reduction progress changes intended behavior with a stated reason (3115679: Credential Proxy Implementation #2487 preserved admitted ownership and used one conditional send followed by observation to avoid uncertain duplicate effects.)

Testing

Proof path: in-process harness.

Security

None.

Evidence

What I checked:

• Current-main recovery loop: Current main returns pending whenever a recorded detach request has no confirmed epoch and Git remains attached, without another send or a retirement decision. (apps/controller/src/drivers/egress-proxy/openshell-expired-git.ts:296, 38ecfca18762)
• Introduced retry preserves request identity: The introduced hunks retain the request ID, require the recorded Sandbox version to match, repeat the existing identity checks, and observe removal after an uncertain repeated RPC. (apps/controller/src/drivers/egress-proxy/openshell-expired-git.ts:311, 09907ba00fa6)
• Existing storage format: The patch only reads expected_version already written by begin(); the existing database constraint pairs request_id with expected_version, so no stored-data format or migration changes. (packages/occ/src/state/credential-delivery-reductions.ts:115, 09907ba00fa6)
• Proof scope and re-review: The full PR body explicitly says real-client recovery against the pinned gateway is unproven; the author’s subsequent comment only requests re-review. The supplied body identity is 85f110479fbd23c99e9ccf28d46b0b15841078074433388099a665733ace0418. (09907ba00fa6)
• Feature provenance and ownership: The expiry implementation and reduction store were added by commit 3115679 in Credential Proxy Implementation #2487; subsequent Git lifecycle refactoring also traces to Free Wortley. Some follow-history and blame operations hit unavailable older blobs, limiting deeper attribution. (apps/controller/src/drivers/egress-proxy/openshell-expired-git.ts:296, 31156790238d)
• Required dependency contract: The changed production client invokes DetachSandboxProvider, and the PR’s safety argument expressly depends on this pinned implementation. Its mutation runner authorizes original and effective requests before replay lookup, fingerprints the payload, replays completed results, and rejects unresolved admissions. (crates/openshell-server/src/grpc/mutation_replay.rs:179, bdb956f2cd65)

Labels

Label changes:

No label changes.

Label justifications:

• P2: Repairs a bounded repository-expiry recovery failure; Git issuance is already fenced while cleanup remains pending.
• merge-risk: 🚨 security-boundary: Recovery repeats a gateway mutation from persisted intent and depends on current Workspace authorization and durable replay semantics.
• rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦐 gold shrimp.
• status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

• Add real-client final-effect proof that authorized recovery converges and revoked Workspace access prevents the repeated mutation.

Rating scale

6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior.

Workflow

ClawSweeper edits this one comment on every review. Comment @clawsweeper re-review for a fresh review only. Repair and merge execution lanes are retired.

History

Review history (1 earlier review cycle)

• reviewed 2026-10-23T04:16:35.237Z sha 09907ba :: needs real behavior proof before merge. :: none

Reviewed October 23, 2026, 12:28 AM ET / 04:28 UTC (Revision 2).

[freeqaz]

@clawsweeper re-review

[clawsweeper]

🦞👀
Exact review queued.

Re-review progress:

• State: Complete
• Detail: The durable review result and its route handoff completed.
• Run: https://github.com/openclaw/clawsweeper/actions/runs/39900675101
• Updated: 2026-10-23T04:30:43.435Z