feat(bounty): compensation pattern for create/claim/settle [BLOCKED ON #240]#241
Closed
windoliver wants to merge 1 commit intomainfrom
Closed
feat(bounty): compensation pattern for create/claim/settle [BLOCKED ON #240]#241windoliver wants to merge 1 commit intomainfrom
windoliver wants to merge 1 commit intomainfrom
Conversation
…ED ON #240) **DO NOT MERGE** — this is a draft PR tracking in-progress work on bounty operation compensation. Three HIGH/CRITICAL correctness bugs have been flagged by adversarial review across three independent rounds; see #240 for the findings, design options, and recommended next steps. ## What this adds - `createBountyOperation`: try/catch that voids the credit reservation when `bountyStore.createBounty()` throws, so escrow is returned on creation failure. - `claimBountyOperation`: pre-flight check that only `open` bounties can be claimed, plus try/catch that releases the claim record when `bountyStore.claimBounty()` throws so other agents aren't blocked until the lease expires. - `settleBountyOperation`: reordered to persist `completed → settled` state transitions BEFORE calling `creditsService.capture()`, with a comment explaining the intent (make the failure recoverable by retry). - Test coverage for each compensation path using mock stores. - Extract `MISSING_BOUNTY_STORE` constant for the repeated error message. ## Known bugs (see #240 for details) 1. **[CRITICAL]** Settlement now commits terminal `settled` state before `capture()` runs. A capture failure leaves the bounty paid according to the state machine but with no funds actually transferred, and there is no reconciler in the codebase to repair it. src/core/operations/bounty.ts:319-333 2. **[HIGH]** The `createBounty` catch voids the reservation on any error, including post-commit failures from the NexusBountyStore two-step write (bounty doc first, then status index). A late failure leaks an "open" bounty with no escrow backing it. src/core/operations/bounty.ts:156-176 3. **[HIGH]** Same shape on `claimBounty`: the compensation releases the claim even when the bounty state transition actually persisted, leaving the bounty stuck in `claimed` pointing at a released claim. src/core/operations/bounty.ts:258-269 ## Design options for the fix See #240 — three paths, roughly: - **(a)** Full saga: `pending_settlement` state + reconciler + CAS confirmation on post-commit failures. ~200-500 lines, 1-2 days. - **(b)** CAS confirmation only: re-read before compensating so we don't rollback committed state. Doesn't fix the settle-before-pay race. ~100-200 lines. - **(c)** Revert the compensation refactor, keep only the cosmetic changes (constant extract + pre-flight claim check). Relies on reservation/lease expiry for recovery. ~50-100 lines. The issue recommends starting with (c) to stop the bleed, then doing (a) as a proper saga-log project with its own RFC. ## Why open this as a draft PR now This code existed in an uncommitted worktree alongside the `fix(tui,mcp,acpx)` work in PR #239. Rather than leave it in a limbo state where it can be accidentally restaged on the next push, this branch makes it visible, reviewable, and explicitly linked to the design issue it depends on. Do not merge until #240 has a resolved design and the code matches.
Owner
Author
|
Closing — consolidating all bounty saga work into issue #240, which is now the single tracking issue. The compensation refactor here had three HIGH/CRITICAL correctness bugs that codex flagged across three adversarial-review rounds (see #240 for full analysis), and the fix direction is still under design. Keeping the code around as a draft PR added ambiguity — it looked actionable when it wasn't. If someone picks up option (c) from #240 (the revert + cosmetic keeper), they should cut a fresh branch from `main` and reference #240 directly rather than resurrecting this one. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds compensation try/catch logic around `createBountyOperation`, `claimBountyOperation`, and `settleBountyOperation` so failed store writes can be rolled back cleanly. Also extracts a shared error message constant and adds a pre-flight `open` status check before claiming.
Why it's blocked
Three independent codex adversarial-review rounds all flagged the same correctness bugs in this compensation logic. Details + design options are in #240.
The compensation code is correct for pre-persistence failures but wrong for post-persistence failures, and the NexusBountyStore layer doesn't tell the caller which mode a given throw came from.
Design options (from #240)
Recommendation: start with (c) to stop the bleed, then do (a) as a proper RFC-backed saga-log project.
Why this PR exists
This code was sitting uncommitted in the `ancient-nibbling-anchor` worktree alongside the TUI/MCP session-scoping work in #239. Rather than leave it in limbo where it could be accidentally restaged, this PR makes it visible, reviewable, and explicitly linked to the design issue it depends on.
Current test state
`bun test src/core/operations/bounty.test.ts` → 55 pass, 0 fail. The tests cover the happy path and basic error injection; the codex findings are latent partial-failure scenarios that the current test harness doesn't reproduce (they need a way to mock `NexusBountyStore.transitionBounty` to throw after the bounty document CAS-write but before the status-index write, which isn't currently possible with the in-memory store used here).
Checklist before merging