Skip to content

fix: reconcile absent ASCII Box claims - #1874

Open
shunkakinoki wants to merge 1 commit into
openclaw:mainfrom
shunkakinoki:codex/ascii-absent-claim-reconcile
Open

fix: reconcile absent ASCII Box claims#1874
shunkakinoki wants to merge 1 commit into
openclaw:mainfrom
shunkakinoki:codex/ascii-absent-claim-reconcile

Conversation

@shunkakinoki

Copy link
Copy Markdown
Contributor

Summary

Fix #1730 by reconciling a stale ASCII Box claim when both the exact native lookup reports a recognized 404 and complete box list --all inventory omits the claimed Box ID.

The release fence now checks native identity before consulting a retained deletion operation. Proven absence removes only the exact unchanged local claim; it does not read a stale operation, repeat SSH teardown, or issue another stop/delete. Observable Boxes still require valid operation evidence, while failed or partial inventory, cancellation, matching IDs, and replacement identities retain the claim.

Documentation and regression coverage encode the new provider boundary. No changelog entry is included, per contributor policy.

Verification

  • go test ./internal/providers/asciibox
  • go test -race ./internal/providers/asciibox
  • go vet ./...
  • go build -trimpath -o /private/tmp/crabbox-ascii-absence-proof ./cmd/crabbox
  • git diff --check

The full go test ./... run completed the changed provider package, then stalled in the pre-existing internal/cli harness with no live test process; it was interrupted after 156 seconds. The scoped race suite, repository-wide vet, and production build all completed successfully.

Live provider proof

Built candidate d9e3847ed84f6e3bc51e3862d0bbb205bcf172e0 was run against the retained task-owned claim from the linked issue's earlier cleanup proof. The provider had already deleted its exact Box, while Crabbox still held a stale claim and blocked operation reference.

released lease=<redacted> box=<redacted>
  • Candidate stop completed successfully in 13.0 seconds.
  • The exact local claim is absent afterward.
  • Direct native box --json info <exact-id> still returns status: 404.
  • No Box was allocated and no repeated native deletion was issued.

This exercises the repaired release path against real ASCII provider state and confirms no provider or local-claim residue for the exact lease.

@clawsweeper

clawsweeper Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

🦞👀
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 clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Sep 5, 2026
@clawsweeper

clawsweeper Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Codex review: blocked before merge. Reviewed September 5, 2026, 12:36 AM ET / 04:36 UTC.

ClawSweeper review

What this changes

Allow ASCII Box cleanup to remove an unchanged local ownership claim after an exact not-found lookup and complete inventory absence, without checking a retained deletion operation.

Regression provenance

Possible regression — suspected (reviewed change). No predecessor PR is attributed.

Merge readiness

Blocked before merge - 6 items remain

The recovery problem remains on main, but this patch changes a shipped cleanup contract and introduces two blocking claim-finalization defects.

Priority: P2
Reviewed head: d9e3847ed84f6e3bc51e3862d0bbb205bcf172e0
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) Relevant live proof and focused coverage support the contribution, but premature finalization and false durable completion evidence block landing.
Proof confidence 🐚 platinum hermit (4/6) Sufficient (terminal): The captured body reports this exact candidate's stop path against real ASCII state and an existing retained claim, with released output, claim removal in 13 seconds, continued native 404, and no repeated deletion. This proves the changed reconciliation behavior, but does not establish that the hidden native deletion operation completed.
Patch quality 🦐 gold shrimp (3/6) 2 actionable review findings remain.

Verification

Check Result Evidence
Real behavior Verified Sufficient (terminal): The captured body reports this exact candidate's stop path against real ASCII state and an existing retained claim, with released output, claim removal in 13 seconds, continued native 404, and no repeated deletion. This proves the changed reconciliation behavior, but does not establish that the hidden native deletion operation completed.
Evidence reviewed 10 items Verified introduced change: The original head has the pinned main commit as its recorded parent. The test merge has the exact main/head parents and the same resulting tree as the head, so the reviewed behavior survives the three-way merge.
Main still requires completion evidence: Current main checks the bound deletion operation before Box lookup and rejects absence without a completed-deletion witness; the proposed reconciliation is not already implemented.
Released compatibility contract: The supplied latest release, v0.49.1, contains the same completion requirement. Existing retained claims therefore change behavior when processed by this candidate.
Findings 2 actionable findings [P1] Retain hidden pending operations until deletion completes
[P2] Separate absence reconciliation from native-completion records
Security None None.

How this fits together

The ASCII Box adapter translates Crabbox cleanup requests into native CLI operations and tracks unfinished cleanup in durable local ownership claims. Its release fence validates the resource and claim before authorizing remote cleanup or local claim removal.

flowchart TD
A[Stop or automatic cleanup] --> B[Validate and lock ownership claim]
B --> C[Exact Box lookup]
C -->|Observable| D[Validate identity and deletion operation]
C -->|Not found| E[Check complete inventory]
D --> F[Retain claim or complete native deletion]
E -->|ID absent| G[Remove unchanged local claim]
E -->|Uncertain or present| F
Loading

Decision needed

Question Recommendation
Should ordinary stop retain its native-completion requirement, with claim-only forgetting treated as a separate recovery workflow? Preserve completion-based release: Keep pending-operation tracking intact and narrow recovery work to trustworthy completion evidence and visible cleanup progress.

Why: The requested shortcut changes the shipped meaning of successful release; source and live reconciliation output cannot establish maintainer acceptance of abandoning unfinished deletion tracking.

Before merge

  • Retain hidden pending operations until deletion completes (P1) - After an accepted deletion times out or becomes blocked, ASCII can hide the Box from both normal lookup and inventory; client.waitForDeletion explicitly documents this behavior. Moving the operation check under successful GetBox and removing the completion guard makes a retry return success and delete the claim, including its only durable operation reference, while deletion is unfinished. The changed hidden-pending tests now expect that failure mode. Keep the recorded operation authoritative even when the Box is hidden, rather than treating absence as completion.
  • Separate absence reconciliation from native-completion records (P2) - The new absence-only success path reaches this existing callback with nativeCompleted == false and releaseErr == nil, so it writes ascii_box_deletion_completed without observing native completion. The shared transaction durably writes these labels before unlinking the claim and retains or restores them on removal/sync failure; a crash in between also leaves the false witness. Distinguish reconciliation from completed native deletion and preserve that distinction through interrupted finalization.
  • Resolve merge risk (P1) - Upgrading can cause previously retained pending or blocked cleanup claims to disappear merely because the provider hides the Box, losing the durable operation reference before cleanup is confirmed.
  • Resolve merge risk (P1) - Interrupted local finalization can preserve a completed-deletion witness that was never backed by native completion.
  • Complete next step (P2) - Resolve the cleanup-contract decision and repair both claim-finalization defects before merge.
  • Resolve maintainer decision - Resolve the maintainer decision shown above before merge.

Findings

  • [P1] Retain hidden pending operations until deletion completes — internal/providers/asciibox/ownership.go:351-354
  • [P2] Separate absence reconciliation from native-completion records — internal/providers/asciibox/ownership.go:221-224
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test delta Production +21/-21; tests +70/-28 Production size is unchanged, while the test growth primarily encodes the new cleanup semantics.

Root-cause cluster

Relationship: fixed_by_candidate
Canonical: #1730
Summary: This PR is a candidate for the linked issue's stale-claim reconciliation component; its progress-reporting component remains outside the patch.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge-risk options

Maintainer options:

  1. Preserve the released cleanup contract (recommended)
    Retain completion checks for hidden pending operations and prevent absence-only outcomes from creating native-completion records.
  2. Pause for a separate recovery design
    Separate explicit local claim forgetting from ordinary resource release before introducing the new behavior.

Technical review

Best possible solution:

Keep ordinary release completion-based, preserve unfinished cleanup records, and consider independent claim-forgetting only as an explicitly approved recovery workflow.

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

Yes, by source inspection of the candidate: a retained pending operation plus exact 404 and empty complete inventory reaches claim removal, as the new fixtures demonstrate. No tests or provider commands were executed during this read-only review.

Is this the best way to solve the issue?

No. Inventory absence does not distinguish completed deletion from a hidden pending operation, and the existing finalizer incorrectly promotes the new success outcome into durable completion evidence.

Full review comments:

  • [P1] Retain hidden pending operations until deletion completes — internal/providers/asciibox/ownership.go:351-354
    After an accepted deletion times out or becomes blocked, ASCII can hide the Box from both normal lookup and inventory; client.waitForDeletion explicitly documents this behavior. Moving the operation check under successful GetBox and removing the completion guard makes a retry return success and delete the claim, including its only durable operation reference, while deletion is unfinished. The changed hidden-pending tests now expect that failure mode. Keep the recorded operation authoritative even when the Box is hidden, rather than treating absence as completion.
    Confidence: 0.98
  • [P2] Separate absence reconciliation from native-completion records — internal/providers/asciibox/ownership.go:221-224
    The new absence-only success path reaches this existing callback with nativeCompleted == false and releaseErr == nil, so it writes ascii_box_deletion_completed without observing native completion. The shared transaction durably writes these labels before unlinking the claim and retains or restores them on removal/sync failure; a crash in between also leaves the false witness. Distinguish reconciliation from completed native deletion and preserve that distinction through interrupted finalization.
    Confidence: 0.99

Overall correctness: patch is incorrect
Overall confidence: 0.97

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against a6021b3633b8.

Labels

Label changes:

  • add P2: This is a bounded recovery change for one provider, with merge-blocking cleanup defects but no demonstrated widespread outage.
  • add merge-risk: 🚨 compatibility: Existing retained claims can be discarded under weaker completion conditions than those shipped in v0.49.1.
  • add proof: sufficient: Contributor real behavior proof is sufficient. The captured body reports this exact candidate's stop path against real ASCII state and an existing retained claim, with released output, claim removal in 13 seconds, continued native 404, and no repeated deletion. This proves the changed reconciliation behavior, but does not establish that the hidden native deletion operation completed.
  • add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🐚 platinum hermit and patch quality is 🦐 gold shrimp.
  • add status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (terminal): The captured body reports this exact candidate's stop path against real ASCII state and an existing retained claim, with released output, claim removal in 13 seconds, continued native 404, and no repeated deletion. This proves the changed reconciliation behavior, but does not establish that the hidden native deletion operation completed.

Label justifications:

  • P2: This is a bounded recovery change for one provider, with merge-blocking cleanup defects but no demonstrated widespread outage.
  • merge-risk: 🚨 compatibility: Existing retained claims can be discarded under weaker completion conditions than those shipped in v0.49.1.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🐚 platinum hermit and patch quality is 🦐 gold shrimp.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (terminal): The captured body reports this exact candidate's stop path against real ASCII state and an existing retained claim, with released output, claim removal in 13 seconds, continued native 404, and no repeated deletion. This proves the changed reconciliation behavior, but does not establish that the hidden native deletion operation completed.
  • proof: sufficient: Contributor real behavior proof is sufficient. The captured body reports this exact candidate's stop path against real ASCII state and an existing retained claim, with released output, claim removal in 13 seconds, continued native 404, and no repeated deletion. This proves the changed reconciliation behavior, but does not establish that the hidden native deletion operation completed.

Evidence

What I checked:

  • Verified introduced change: The original head has the pinned main commit as its recorded parent. The test merge has the exact main/head parents and the same resulting tree as the head, so the reviewed behavior survives the three-way merge. (d9e3847ed84f)
  • Main still requires completion evidence: Current main checks the bound deletion operation before Box lookup and rejects absence without a completed-deletion witness; the proposed reconciliation is not already implemented. (internal/providers/asciibox/ownership.go:320, a6021b3633b8)
  • Released compatibility contract: The supplied latest release, v0.49.1, contains the same completion requirement. Existing retained claims therefore change behavior when processed by this candidate. (internal/providers/asciibox/ownership.go:351, 775ebb1160d6)
  • Native visibility is not deletion completion: The production native-CLI client explicitly records that accepted deletion hides normal Box reads and polls the exact operation until completed. This establishes the relevant ASCII CLI contract used by the patch. (internal/providers/asciibox/client.go:436, d9e3847ed84f)
  • Hidden pending operation loses its claim: The new test starts from a durably retained pending operation, forbids reading that operation, and expects claim removal after absent Box reads. Another changed test replaces the former hidden-pending retention expectation with successful removal. (internal/providers/asciibox/ownership_test.go:653, d9e3847ed84f)
  • Absence success fabricates a completion witness: The new absence-only success reaches the existing release callback with nativeCompleted false, but releaseErr nil still adds the completed-native-operation witness. (internal/providers/asciibox/ownership.go:221, d9e3847ed84f)

Likely related people:

  • shunkakinoki: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • Peter Steinberger: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

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

  • Resolve the cleanup-contract decision and retain tracking for hidden pending or blocked operations.
  • Separate reconciliation outcomes from native-completion witnesses and cover interrupted claim finalization.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ASCII Box release hides bounded cleanup progress for stale claims

1 participant