feat(scripts): add consumer-side handoff queue front-matter validator - #2444
Conversation
Implements scripts/check-queue-front-matter.sh per #1647: validates required keys, documented status/priority sets, id/filename stem match, and prints file-count vs parseable-front-matter reconciliation so a status grep cannot report a false all-clear. Decision: land in scripts/ now (not wait for work-items adapter). Closes #1647. Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ca740b5a4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Claude finished @kyle-sexton's task in 1m 18s —— View job Security Review
Scope: This PR adds two new shell scripts only — No security issues found. Specifically checked and ruled out:
One minor, non-blocking observation (not flagging as a finding since it requires an attacker who already has write access to the queue directory, at which point they have much larger attack surface): the script follows symlinks when globbing |
|
Claude finished @kyle-sexton's task in 6m 25s —— View job Review complete
This repo has a Note: Posted 3 inline findings:
Everything else in the script read cleanly: the required-key/status/priority/id checks avoid the classic I reviewed by reading the code; this session's Bash tool wouldn't let me actually execute the scripts (blocked at the permission layer), so the findings above are static-analysis-based, not test-run-confirmed — worth a quick manual check of finding #1 before merge if there's any doubt. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| fm_value() { | ||
| awk -v key="$2" ' | ||
| $1 == key ":" { | ||
| sub(/^[^:]*:[[:space:]]*/, "") | ||
| exit | ||
| } | ||
| ' <<<"$1" | ||
| } |
There was a problem hiding this comment.
Important — trailing whitespace on a value causes false-positive violations.
sub(/^[^:]*:[[:space:]]*/, "") only strips the leading key: portion; it leaves any trailing whitespace on the line untouched (e.g. a stray trailing space, or a \r if the item file happens to be CRLF-terminated).
That trailing whitespace survives into every comparison that isn't itself whitespace-tolerant:
status '$status'enum check at line 82 —status: unclaimed(trailing space) fails^(unclaimed|...)$and is wrongly reported as an invalid status.priorityenum check at line 89 — same failure mode.idvs filename-stem check at line 95 —id: 20260812-samplewould be reported as a mismatch against stem20260812-sampleeven though they're the same id.
The "missing required key" check at line 75 happens to be immune (it strips all whitespace before testing emptiness), which is likely why this wasn't caught by hand-testing.
A one-line fix in fm_value (e.g. also trimming trailing [[:space:]]*$ in the sub, or piping through something that trims both ends) would remove this whole class of false positive — which matters here specifically because the script's stated purpose is to avoid false readings of queue state.
| #!/usr/bin/env bash | ||
| # Unit tests for check-queue-front-matter.sh. | ||
| set -uo pipefail | ||
|
|
||
| SELF_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | ||
| SCRIPT="$SELF_DIR/check-queue-front-matter.sh" |
There was a problem hiding this comment.
Important — this test suite doesn't appear to run anywhere.
scripts/run-plugin-tests.sh only discovers plugins/**/*.test.sh and .claude/hooks/*.test.sh (mapfile -t tests < <(find plugins .claude/hooks -type f -name '*.test.sh' ...)), so it never picks up scripts/*.test.sh. Every other scripts/check-*.sh in this repo compensates by getting an explicit run: bash scripts/<name>.test.sh step added to .github/workflows/ci.yml (see e.g. check-silent-skips.test.sh, check-plugin-manifest-presence.test.sh, check-orphaned-fixtures.test.sh). I couldn't find check-queue-front-matter anywhere in .github/workflows/, so this 107-line test file has no CI step invoking it.
The PR's decision brief says the checker itself is intentionally "not wired to CI" because the queue lives outside the repo — but that reasoning doesn't extend to the unit tests: they're fully self-contained (each test does its own mktemp -d and doesn't touch anything outside the repo), just like the sibling tests that do run in CI. As written, a future regression in check-queue-front-matter.sh's parsing logic wouldn't be caught automatically.
| if run_check "$q" >/dev/null 2>&1; then | ||
| ok "README.md ignored" | ||
| else | ||
| fail "README should be ignored: valid sole item should pass" | ||
| fi | ||
|
|
||
| printf '\n%d passed, %d failed\n' "$PASS" "$FAIL" | ||
| [[ "$FAIL" -eq 0 ]] |
There was a problem hiding this comment.
Suggestion — coverage gaps relative to the checker's own logic.
The five cases here (valid item, wholesale-missing front matter, bad status, id/stem mismatch, README skip) all just assert pass/fail via exit status. A few things the checker does aren't exercised by any case:
- No test for an individual missing required key (
title/created/producer) when front matter otherwise parses — only "front matter entirely absent" is covered. - No test for an out-of-set
priorityvalue (the optional-key branch atcheck-queue-front-matter.sh:87-92). - No test asserting the
Reconciliation: %d item file(s), %d with parseable front matterline/count-gap violation (check-queue-front-matter.sh:100-106) — this is the PR's headline fix for the "grep '^status:'false all-clear" problem (handoff-queue: consumer-side front-matter validator (a status grep reports a false all-clear on a malformed item) #1647), but nothing here verifies the counts are actually reported correctly for a mixed queue (some valid items + some with no front matter at all).
None of these are required for the PR to be correct, but since this suite won't run in CI yet (see the other comment on this file) and the checker's core value proposition is exactly the reconciliation-count behavior, it'd be worth locking that down with a test before it can silently regress.
|
Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
2 participants
Summary
Adds
scripts/check-queue-front-matter.sh— a repo-agnostic consumer validator for file-based markdown handoff queues outside the repo.Behavior
id,title,status,created,producer)statusmust be inunclaimed|claimed|in-progress|blocked|doneprioritychecked when presentidmust match the filename stemgrep '^status:'cannot false-clear malformed itemsDecision brief (#1647)
RECOMMENDED: Build now in
scripts/rather than waiting for thework-itemscustom adapter — live false-all-clear today, cheap script, logic can migrate later.Not wired to CI (queue is out-of-repo). Invoke at claim time.
Closes #1647.
Related
N/A