REVISE PR #435: the store and HTTP halves are sound, but the CLI caller discards the new revoked signal so the defect survives - #445
Conversation
…revoke
Store layer (collections.py) captures DELETE rowcount and sets
col["revoked"] = removed > 0. HTTP handler pops revoked from the inner
collection and returns it at the envelope top level. CLI caller reads
revoked, prints a distinguishable noop message to stderr, and returns 1
when no grant matched.
docs/collections.md endpoint table updated to reflect {"collection": ...,
"revoked": ...} response shape. HTTP tests assert revoked is True on a
matching grantee, False on a non-matching grantee, and absent from the
inner collection object in both cases. Store tests cover both outcomes.
New CLI test verifies noop revoke returns rc=1 with 'no grant matched'
on stderr while the grant survives.
Verified: tests/test_collections_cli.py 6 passed, tests/test_collections_store.py
+test_collections_http.py 42 passed.
Docs-Reviewed: collections endpoint change is documented in docs/collections.md; A2A handlers in http_server.py are unaffected
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe revoke flow now reports whether a grant was removed. The HTTP response exposes this status, and the CLI reports no-match revocations with a non-zero exit code and stderr output. Store, HTTP, and CLI tests cover both matching and non-matching grants. ChangesGrant revoke result
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The change reports whether a grant was actually revoked and makes the CLI return a distinct failure status for no-op revocations; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 6 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| self._send_json(404, {"error": str(exc)}) | ||
| return | ||
| self._send_json(200, {"collection": col}) | ||
| revoked = col.pop("revoked") |
There was a problem hiding this comment.
WARNING: Asymmetric default on col.pop("revoked") is fragile
The CLI uses col.pop("revoked", False) (safe), but this HTTP handler uses bare col.pop("revoked") — a KeyError will be raised if any future caller (test mock, alternate store, a wrapper that strips the field, or a regression that drops the assignment in CollectionStore.revoke) returns a col dict without the key. The two consumers should agree; mirror the CLI's defensive default so a single bad call site cannot 500 the whole endpoint.
| revoked = col.pop("revoked") | |
| revoked = col.pop("revoked", False) |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| revoked = col.pop("revoked", False) | ||
| if not revoked: | ||
| print(f"noop: no grant matched for {args.agent}", file=sys.stderr) | ||
| return 1 |
There was a problem hiding this comment.
SUGGESTION: Document or unify exit codes for collections revoke
This branch returns 1 for "no grant matched", while the surrounding except CollectionNotFoundError / except ValueError blocks return 2 for errors. A user reading _collections_revoke cannot tell from the code that 1 specifically means "noop / nothing to do" and 2 means "real error" — and the conventions used by the rest of the CLI are not stated anywhere. Either add a short comment clarifying the rc=1 vs rc=2 split, or unify with the existing 2 so callers don't need to distinguish. Tests pin rc == 1, so any unification must update them too.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (8 files)
Fix these issues in Kilo Cloud Reviewed by minimax-m3:free · Input: 30.8K · Output: 2.9K · Cached: 210.1K |
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
|
Reviewed, clean. Branch is fresh off master as the card asked (merge-base is The decisive question was whether the revoked signal survives to the CLI surface, since that is where the parent died. Driven end to end against a real store, exit status captured per arm: With master's Caller enumeration rather than a grep for the symbol: Seven mutations by line number across the store, HTTP and CLI branches separately, failed and error counts read from the summary in the same command, baseline 48 passed: all seven killed, zero survivors, zero collection errors. That includes the pop-to-get mutant, so the assertion that Suite on the merged tree is 1809 passed / 10 skipped / 7 errors against the 1804 / 10 / 7 baseline on One thing to consider folding in before this merges, since it is a one-line change in a file this PR already touches. Also worth knowing for merge ordering: this and #443 can both land, and neither duplicates the other. Their subjects are disjoint, and the only conflict is Leaving this for your merge decision. |
1 participant
CARD TITLE (intent, not commit subject): REVISE PR #435: the store and HTTP halves are sound, but the CLI caller discards the new revoked signal so the defect survives
Autonomous build of board card tsk-d6giky.
Store layer (collections.py) captures DELETE rowcount and sets
col["revoked"] = removed > 0. HTTP handler pops revoked from the inner
collection and returns it at the envelope top level. CLI caller reads
revoked, prints a distinguishable noop message to stderr, and returns 1
when no grant matched.
docs/collections.md endpoint table updated to reflect {"collection": ...,
"revoked": ...} response shape. HTTP tests assert revoked is True on a
matching grantee, False on a non-matching grantee, and absent from the
inner collection object in both cases. Store tests cover both outcomes.
New CLI test verifies noop revoke returns rc=1 with 'no grant matched'
on stderr while the grant survives.
Verified: tests/test_collections_cli.py 6 passed, tests/test_collections_store.py
+test_collections_http.py 42 passed.
Docs-Reviewed: collections endpoint change is documented in docs/collections.md; A2A handlers in http_server.py are unaffected
Files:
docs/collections.md | 2 +-
taosmd/cli.py | 4 +++
taosmd/collections.py | 7 +++--
taosmd/http_server.py | 3 ++-
tests/test_collections_cli.py | 13 ++++++++++
tests/test_collections_http.py | 39 ++++++++++++++++++++++++++++
tests/test_collections_store.py | 26 +++++++++++++++++++
8 files changed, 98 insertions(+), 4 deletions(-)
Summary by CodeRabbit
Bug Fixes
revokedstatus while preserving collection details.Documentation
revokedresponse field.Tests