feat(store,api): hard-delete a feature alongside cancel — the tombstone edge (v0.23.0) - #55
Merged
Merged
Conversation
…ne edge (v0.23.0) Follow-up to #47: that shipped cancel-with-audit (close + `cancelled` lane); this adds the hard-delete sibling the issue's "and/or" also called for, so the operator chooses per-case instead of the maintainer baking one in. • store.delete_feature(fid, reason) → `br delete --reason "deleted: …"` — a tombstone in the JSONL (recoverable), run THROUGH the board so board ↔ JSONL stay in step (vs the raw `br` reach-around the issue warned desyncs the board). Refuses via `br`'s non-zero exit when the feature has dependents (deleting would orphan them) — cancel or re-point first. Returns the pre-delete snapshot for the API echo. • DELETE /features/{fid} (the route the issue proposed; reason optional, body or none). cancel stays the recommended default (visible, reopenable audit lane); delete is for a feature that should leave no trace. Verified `br delete` tombstones + the dependent guard against real `br` 0.1.23; unit-tested both methods + the route. Ref #47 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
👀 Quinn is reviewing — verdict (PASS / WARN / FAIL) + findings to follow. |
There was a problem hiding this comment.
QA Audit — PR #55 | feat(store,api): hard-delete a feature alongside cancel — the tombstone edge (v0.23.0)
VERDICT: WARN (non-blocking — CI is still queued; upgrade to PASS once terminal-green)
CI Status
- test: queued
Diff Review
- Adds
store.delete_feature(fid, reason)mirroringcancel_feature— captures pre-delete snapshot via_require, tombstones viabr delete --reason, returns the snapshot as API echo. Correctly captures before the operation (unlike cancel which re-fetches after) since the tombstoned feature would be unfetchable post-delete. - New
DELETE /features/{fid}route following the identical_guard+Body(default={})pattern as the cancel route — clean and consistent. - Version bumps (0.22.0 → 0.23.0) in both
pyproject.tomlandprotoagent.plugin.yamlare in sync. - 193 tests passing;
ruff check+ruff format --checkclean per PR description.
Observations
- LOW:
clawpatch_reviewnot available for this repo (not in project registry) — structural review skipped. Manual diff review shows no cross-file hazards given the small, pattern-following change. - LOW:
test_delete_route_calls_delete_featureasserts 200 + store call but doesn't inspect the response body for the pre-delete snapshot echo. The store-level test covers this — non-blocking. - No unresolved CodeRabbit threads.
— Quinn, QA Engineer
|
Submitted COMMENT review on #55. Summary: Clean, pattern-following change — |
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
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.
Follow-up to #47
#47 shipped cancel-with-audit (
br close -r+ acancelledlane). The issue's proposed direction was an and/or — cancel and/or a hard delete — with an open question for the maintainer about which the intent was. Rather than bake one in, this adds the hard-delete sibling so the operator chooses per-case.What
store.delete_feature(fid, reason)→br delete --reason "deleted: …"— a tombstone in the JSONL (recoverable, not a history nuke), run through the board so board ↔ JSONL stay in step (vs the rawbrreach-around the issue warned desyncs the board). Refuses (BoardError, viabr's non-zero exit) when the feature has dependents — deleting would orphan them; cancel or re-point first. Returns the pre-delete snapshot as the API echo.DELETE /features/{fid}— the route the issue proposed;reasonoptional (body or none).cancel vs delete
cancelledlane; full audit trail.Verification
br0.1.23:br delete --reasontombstones (removed frombr list), and a feature with a dependent is refused ("Use --force to orphan… No changes made") → surfaces as a 400, not a silent orphan.delete_feature(tombstone + reason, unknown-id raises) andDELETE …/features/{fid}(with + without a body).ruff check+ruff format --checkclean.Ref #47
🤖 Generated with Claude Code