Skip to content

feat(cli): add sentinel review --list, the store-backed review queue - #16

Draft
ChelseaKR wants to merge 1 commit into
mainfrom
feat-review-list-queue
Draft

feat(cli): add sentinel review --list, the store-backed review queue#16
ChelseaKR wants to merge 1 commit into
mainfrom
feat-review-list-queue

Conversation

@ChelseaKR

Copy link
Copy Markdown
Owner

The gap

sentinel verify --list already answers "what still needs me" for source
verification with no network and no writes, straight from the registry. The
change-review side had no equivalent: a reviewer's only way to learn which
change ids were still unreviewed was to catch watch's output as it scrolled
by, or read the review-queue GitHub issue before it closed. Coming back later
meant either re-running watch against live government servers just to
relist, or opening the SQLite store by hand.

The fix

sentinel review --list [--jurisdiction TX] queries the local snapshot store
for everything still unreviewed (drift and possibly_removed escalations
alike) and prints it in the exact wording watch already uses — no fetch, no
prompt, no write. The print logic is shared via a new _print_pending_change
helper so watch's live output and review --list's later re-read can never
drift apart on wording.

Guardrail note

Adding --list meant change_id becomes optional and
--reviewer/--significance/--status can no longer be required=True in
argparse (a --list-only invocation must not trip them). This is the same
shape verify already uses for --verifier (default="", checked before
the store is touched), not a new pattern.

The actual safety property — CLAUDE.md's four-layer "never auto-classify"
guardrail, layer 4 being "the CLI requires --reviewer" — is unchanged:
_cmd_review still refuses to touch the store without a name, before
reviewed_by (layer 2) or the SQL CHECK (layer 3) would ever see the call.
Only the exit code for that refusal moved from an argparse usage error (2) to
the "refused review" bucket the module's own exit-code table already names
for code 1 — this is no longer an argparse-internal edge case, it is the
command's own refusal.

Updated the one test that pinned the old mechanism
(test_the_cli_cannot_review_without_a_reviewer) into two:

  • test_the_cli_review_command_needs_a_reviewer_before_the_parser_will_even_run_it
    confirms --significance/--status are still parse-time
    choices=-checked (exit 2).
  • test_the_cli_refuses_to_review_without_a_reviewer — end-to-end over the
    real main() path, proving a reviewer-less review is still refused and
    nothing is ever written to the store.

Tests (+4)

  • the queue prints and writes nothing
  • --jurisdiction filters it
  • a possibly_removed escalation appears in it exactly as watch would have shown it
  • missing arguments are refused with a message naming --list as the way out

Gates

make verify — 7/7 green, 414 passed, 93.51% coverage.

🤖 Generated with Claude Code

The gap: `verify --list` already answers "what still needs me" for source
verification with no network and no writes, straight from the registry. The
change-review side had no equivalent — a reviewer's only way to learn which
change ids were still unreviewed was to catch `watch`'s output as it scrolled
by, or read the review-queue GitHub issue before it closed. Coming back later
meant either re-running `watch` against live government servers just to
relist, or opening the SQLite store by hand.

`sentinel review --list [--jurisdiction TX]` fixes that: it queries the local
snapshot store for everything still `unreviewed` (drift and
`possibly_removed` escalations alike) and prints it in the exact wording
`watch` already uses — no fetch, no prompt, no write. The print logic is
shared via a new `_print_pending_change` helper so the two call sites can
never drift apart on wording.

Guardrail note: adding `--list` meant `change_id` becomes optional and
`--reviewer`/`--significance`/`--status` can no longer be `required=True` in
argparse (a `--list`-only invocation must not trip them). This is the same
shape `verify` already uses for `--verifier` (`default=""`, checked before
the store is touched) rather than a new pattern. The actual safety property —
CLAUDE.md's four-layer "never auto-classify" guardrail, layer 4 being "the
CLI requires --reviewer" — is unchanged: `_cmd_review` still refuses to touch
the store without a name, before `reviewed_by` (layer 2) or the SQL CHECK
(layer 3) would ever see the call. Only the exit code for that refusal moved
from an argparse usage error (2) to the "refused review" bucket the module's
own exit-code table already names for code 1 — no longer an argparse-internal
edge case. Updated the one test that pinned the old mechanism
(`test_the_cli_cannot_review_without_a_reviewer`) into two: one confirming
`--significance`/`--status` are still parse-time `choices=`-checked, and one
end-to-end test over the real `main()` path proving a reviewer-less review is
still refused and nothing is ever written to the store.

Tests (+4): the queue prints and writes nothing; `--jurisdiction` filters it;
a `possibly_removed` escalation appears in it exactly as `watch` would have
shown it; missing arguments are refused with a message naming `--list` as the
way out.

`make verify` — 7/7 green, 414 passed, 93.51% coverage.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant