Hold already-allowed mutations for confirmation

[novelKR]

Summary

• Add approval_create, approval_resolve, and operation_resume as a workflow pause for already-allowed mutations, not a privilege grant or isolation boundary. The same MCP caller can grant.
• Workspace approvals: off|confirm (default off) returns APPROVAL_REQUIRED before begin()/spawn. Resume re-checks allow(), records consumed only with a terminal result, recovers patches from the operations ledger, and does not respawn an interrupted exec (APPROVAL_AMBIGUOUS).
• Retrying the same logical apply/exec reuses the active hold (fingerprint). Terminal rows scrub V4A/argv snapshots in CODESPACE_OPERATIONS_DB. Bilingual docs and LIVE_TOOLS contract tests stay aligned; ClientClaims.approved still grants nothing.

Test plan

• cargo test -p codespace-server --test approvals
• cargo test -p codespace-store --lib
• cargo test -p codespace-server --test policy_contract --test process --test protocol_compat --test stdio_contract --test http_contract
• Confirm workspace: apply_patch returns APPROVAL_REQUIRED without writing; grant then resume applies once and operation_status can read the patch ledger
• Lost APPROVAL_REQUIRED then the same apply/exec retry returns the same approval_id; concurrent resume runs once
• Restart resuming patch without a ledger executes once; with a ledger recovers; exec is ambiguous and does not spawn
• Deny then resume fails; terminal rows scrub params_json; same MCP client can grant; default off still applies immediately

[novelKR]

Detailed review

The overall direction is strong. This PR keeps approval state separate from the patch operation ledger, re-checks policy on resume, snapshots the exact arguments being approved, and preserves the existing operation_status recovery path for patches. The tests also cover the basic grant/deny/off/confirm behavior well, and CI is green.

That said, I think there are several issues that should be addressed before this is treated as a durable host-confirmation / suspend-resume contract.

Blocking correctness / durability concerns

1. consumed is written before execution, but the result is persisted afterward.
   A crash after the CAS to consumed but before execution, during execution, or before finish_resume() leaves a permanent consumed + result_json=NULL row. A later resume cannot distinguish "never executed" from "executed but result persistence was lost"; it only returns APPROVAL_CONFLICT. This is especially problematic because the public contract says a repeated resume returns the stored result or a conflict, while the conflict currently loses the execution uncertainty.

   I would introduce an intermediate durable state such as resuming / executing with a resume-attempt identifier and explicit recovery semantics. consumed should mean "terminal result durably recorded", not merely "one caller won the claim".

2. The result-persistence error is explicitly ignored.
   Even without a process crash, a DB failure after the mutation has run can return the execution result to the caller while leaving the approval unrecoverable. That breaks the replay guarantee. The result write needs to be part of the observable resume contract, with a defined ambiguous/failure outcome when it cannot be persisted.

3. Automatic confirmation holds are not idempotent.
   Every retry of an apply_patch / exec_command request under approvals=confirm creates a new approval row. If the APPROVAL_REQUIRED response is lost and the client retries, multiple independent holds are created for the same logical request. Patch may be partially protected when an operation_key is present, but exec has no equivalent and two granted holds can spawn the command twice.

   Please add an approval-level idempotency/recovery key (or an equivalent active-hold dedupe rule) and cover transport retry with tests.

Security / contract concerns

4. This is not currently an enforceable "host confirmation" boundary.
   approval_resolve is exposed in the same LIVE_TOOLS set to the same MCP principal that receives APPROVAL_REQUIRED. As the docs correctly note, that caller can self-grant. If the intended property is a human/host security boundary, the resolver needs a distinct authenticated/authorized channel or scope. If the intended v1 property is only a workflow pause primitive, I suggest changing the naming and tool guidance so callers do not infer a security guarantee that is not enforced.

5. Approval snapshots materially change data-at-rest behavior.
   params_json contains the full ApplyPatchParams / ExecCommandParams, including the complete V4A patch or command arguments. The patch operation ledger intentionally avoided storing patch text; this table now persists it in the same operations DB. That is reasonable for deferred execution, but it should be explicit in the security/storage documentation and should have a terminal retention policy. After deny or after a consumed result is durably stored, the original arguments can usually be scrubbed or replaced with a digest/metadata.

Suggested state invariant

A useful invariant would be:

pending -> granted -> resuming -> consumed

with denied terminal, and:

• consumed always has a persisted terminal result;
• resuming explicitly means execution may be in flight or ambiguous;
• a retry can either replay a terminal result or perform a documented reconciliation path;
• patch resume can durably link to operation_id / operation_key for operation_status recovery;
• exec resume should return an explicit ambiguous state if durable process recovery remains out of scope.

Tests I would add

• crash/restart after claim but before execute;
• crash/restart after execution but before resume-result persistence;
• injected finish_resume DB failure;
• lost APPROVAL_REQUIRED response followed by retry (patch and exec);
• two concurrent resume callers;
• restart with pending, granted, resuming, and terminal rows;
• terminal snapshot scrubbing / retention behavior;
• authority test showing whether the model principal can or cannot call approval_resolve.

The separation of approval, operation, and process identities is a good foundation. Fixing the durability boundary and making the authority/storage semantics explicit would make this a much stronger primitive for a thin agent loop.

[novelKR]

Blocking: consumed is being used as both “resume claimed” and “resume completed”.

This CAS prevents two callers from executing concurrently, which is good, but it creates an unrecoverable crash window: if the server dies after this update and before execution (or after execution but before result_json is written), the row is permanently consumed with no result. A retry then reports only APPROVAL_CONFLICT, so the caller cannot tell whether the mutation never ran or ran with a lost result.

Please introduce an intermediate durable state such as resuming / executing (ideally with an attempt id and, for patches, a durable link to the resulting operation) and reserve consumed for “terminal outcome persisted”. Add restart tests around both sides of the execution boundary.

[novelKR]

Blocking: do not discard the resume-result persistence error.

If the mutation succeeds and this write fails, the caller can receive a success while the approval remains consumed without a replayable result. That violates the documented “repeat resume returns the stored result” behavior and makes recovery ambiguous.

The persistence step needs defined public semantics. At minimum, propagate/encode an explicit ambiguous resume outcome; preferably make the state machine ensure that a terminal/consumed state is only visible once the outcome is durably stored.

[novelKR]

Blocking: automatic holds need idempotency across transport retries.

Every call creates a fresh approval row. If the APPROVAL_REQUIRED response is lost, retrying the same request creates another independent hold. With exec_command, granting/resuming both holds can spawn the same command twice; there is no operation_key equivalent to protect it.

Please add an approval-level idempotency key/fingerprint contract (or reuse an existing active hold for the same logical request) and add a test that drops the first hold response, retries, then proves only one logical resume can execute.

[novelKR]

Design/security boundary: this tool is exposed to the same MCP caller that receives the hold, so the caller can self-grant. The docs acknowledge this, but the API and PR title repeatedly describe the feature as “host confirmation”.

If host/human confirmation is meant to be an enforced safety boundary, approval_resolve should be gated behind a distinct authenticated principal/channel/scope and should not be callable by the model-facing tool surface. If v1 intentionally provides only a workflow pause primitive, please rename/reframe the contract so integrators do not treat it as a security guarantee.

[novelKR]

Storage/security: this persists the full serialized arguments, including the complete V4A patch text (and exec argv), in CODESPACE_OPERATIONS_DB.

That is materially different from the patch operation ledger, which intentionally stores observed hashes rather than patch text. The snapshot is necessary while a hold is resumable, but please document this data-at-rest change explicitly and define terminal retention. After deny, or after a consumed result is durably recorded, consider scrubbing params_json (or replacing it with a digest + minimal metadata) so source/secret-bearing patch content is not retained indefinitely.