fix(procedures): make a bad query loud, and bound the scout's toolset - #23
Conversation
query-records.sh had two silent-wrong-answer shapes. Both exited 0 with plausible output, so a caller could not tell a broken query from a genuine miss — the worst failure mode for a discovery tool, and the same one the existing `exit 3` scan-error path was added to prevent. 1. Repeating a value-taking flag silently kept the last occurrence. `--keyword recall --keyword gitflow` returned gitflow-only results at full speed with no warning. Now exit 2, with the working idiom in the message (several terms belong in ONE --keyword string — they union). 2. A --keyword whose tokens all fell under the 3-char floor produced an empty token set, empty stdout and exit 0 — indistinguishable from "searched, found nothing", when no search had run at all. Hits real terms: ci, pr, gh, db. Now exit 2 naming the floor and the dropped input. The token floor is now the named MIN_TOKEN_LEN rather than an inlined 3, and the exit-code contract is documented in the header so a caller can tell "found nothing" (0) from "never searched" (2) from "scan broke" (3). procedure-scout carried no `tools:` key at all, so a subagent briefed read-only inherited the full toolset — the mechanism behind three logged incidents of read-only forks writing to GitHub. Allowlist Bash/Read/Grep/Glob and state plainly in Invariants that Bash still reaches gh/git/rm, so read-only remains a rule the agent keeps rather than one the harness enforces. Tests: 6 added. 3 fail on the parent commit and pass here (repeat-flag x2, short-token); 3 are regression guards that must pass both sides (--full stays idempotent, a genuine miss stays exit 0 + empty, one-string multi-term still unions). Suite 145 -> 151, 0 failures. Closes #21 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 37 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe PR validates repeated query flags, distinguishes invalid keyword input from genuine misses, adds regression tests, and updates the procedure scout’s tool allowlist and read-only rule. ChangesQuery validation
Scout tool policy
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/procedures/scripts/query-records.sh`:
- Around line 110-139: Add a literal “# PLUGIN ADAPTATION: <why>” comment
immediately before each affected divergence: the repeated-flag validation in
plugins/procedures/scripts/query-records.sh lines 110-139, the all-short-token
error path in plugins/procedures/scripts/query-records.sh lines 204-213, and the
regression-test block in plugins/procedures/hooks/tests/query-records.bats lines
575-615. Explain the plugin-specific reason for each adaptation without changing
the behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5fb43316-dbb0-46dc-8a47-36f920ad64fb
📒 Files selected for processing (3)
plugins/procedures/agents/procedure-scout.mdplugins/procedures/hooks/tests/query-records.batsplugins/procedures/scripts/query-records.sh
…d work-reviewer
Review found the first pass shipped a worse bug than the one it fixed.
`--keyword pr` could never work. The 3-char token floor was applied in three
places — the query tokenizer, record-match.awk:159 and record-rarity.awk:75 —
so `pr` (135 keyword slots in the live corpus), `ci` (29) and `gh` (16) were
unmatchable from both sides. `pr-review` tokenizes to {pr, review} and the
`pr` half was dropped off the record too. The previous commit turned that
silent empty into a hard exit 2, which made a real gap louder while leaving
the lookup just as impossible: the scout could not find PR procedures at all.
The floor is now a `min_tok` parameter threaded through all three sites. The
PULL path passes 2; every other caller defaults to 3, so the PUSH router is
untouched. This is the same split the file already makes with `gate=0` — an
explicit --keyword is a term the caller chose, not prompt noise to filter.
record-rarity and record-match must be given the same value or df/idf is
computed over a different token set than the one scored; both now are.
Marked the three diverging sections `# PLUGIN ADAPTATION` per the repo's
vendoring rule (CodeRabbit, PR #23).
work-reviewer had no `tools:` key, so an agent whose Boundaries say "never
re-run the work, read the diff, or verify independently" could do all three.
Measured: 1.2 Bash calls per invocation — boundary violations. Allowlist
Read/Grep/Glob and withhold Bash, closing the re-run vector in the harness
rather than in prose.
Tests: 3 added (2-char match, 2-char inside a hyphenated keyword, 1-char
still dropped); the short-token error test retargeted to a 1-char input. All
4 fail on the parent commit. Suite 151 -> 154, 0 failures.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Addressed in f77e3b2. PLUGIN ADAPTATION markers — added at all three sites CodeRabbit named: the repeated-flag validation, the all-short-token error path, and the regression-test block. Two more went in with this push ( A worse bug found while re-reviewing. The first pass made The floor is now a work-reviewer now carries |
"No other optimizations?" — the full survey, with verdictsEverything considered, and why it is or is not in this PR. Rejections are as load-bearing as the changes.
The honest summary: the two biggest levers (#22, #303) are both outside this PR's reach, and four candidates were rejected on evidence rather than shipped. What is here is the correctness fix plus one unmeasured cost change. One caveat on this PR's own |
✅ Post-merge verification: the
|
2 participants
What
Two silent-wrong-answer defects in
query-records.sh, plus atools:allowlist onprocedure-scout.Closes #21.
Why
This came out of pricing
/how-do-iand/am-i-doneagainst local agent transcripts. The headline cost finding is not in this PR — it is the tier leak in #22 (~66% of the priced cost of/how-do-i, no flow change). What is here is the correctness half: the scout's query tool could return a confidently wrong answer, fast, with exit 0.For a discovery tool that is the worst failure shape — the same reasoning behind the existing
exit 3scan-error path, which already says "NOT 'no matches'" out loud.Changes
scripts/query-records.sh+scripts/lib/record-match.awk+scripts/lib/record-rarity.awk--keyword prcould never work. The 3-char token floor was applied in three places (query tokenizer,record-match.awk:159,record-rarity.awk:75), so it dropped short tokens from the query and from every record's keywords.proccupies 135 keyword slots in the live corpus,ci29,gh16;pr-reviewtokenizes to{pr, review}and lost itsprhalf too. The scout could not find PR procedures. The floor is now amin_tokparameter threaded through all three sites: the PULL path passes 2, every other caller defaults to 3, so the PUSH router is unchanged. Same split the file already makes withgate=0— an explicit--keywordis a term the caller chose, not prompt noise.record-rarityandrecord-matchmust receive the same value or df/idf is computed over a different token set than the one scored.--keyword a --keyword breturned b-only results at full speed, no warning. Now exit 2, with the working idiom in the message — several terms belong in ONE--keywordstring, where they tokenize into a ranked OR union. Applied to all seven value-taking flags;--fullis exempt (idempotent, takes no value).--keywordwhose tokens all fell under the 3-char floor produced an empty token set, empty stdout, exit 0 — indistinguishable from "searched, found nothing" when no search had run. Hits real terms:ci,pr,gh,db. Now exit 2, naming the floor and the rejected input.MIN_TOKEN_LENrather than an inlined3, and the exit-code contract is documented in the header:0= ran, empty means genuine miss ·2= usage error, never searched ·3= scan broke.agents/work-reviewer.md— addedtools: Read, Grep, Glob. Its Boundaries say "Never re-run the work, read the diff, or verify independently", and it was measured doing 1.2 Bash calls per invocation — boundary violations. WithholdingBashcloses the re-run vector in the harness rather than in prose.agents/procedure-scout.md— addedtools: Bash, Read, Grep, Glob. There was notools:key at all, so a subagent briefed read-only inherited the full toolset. That is the mechanism behind three logged incidents of read-only forks writing to GitHub.Allowlist checked against the agent's own steps: every command in Steps 2–4 (
query-records.sh, thegrep -rnkeyword fallback, theawk 'FNR==1{...}'batch read, thegrep -icE/grep -iE | tailsweep) runs under Bash.Read/Grep/Globare read-only spares. Nothing in the documented flow needs a tool outside the list, so this cannot silently degrade a lookup.⚠ This narrows the surface; it does not close the hole.
Bashstill reachesgh,gitandrm. The Invariants section now says so explicitly, so nobody reads the allowlist as enforcement. A real closure needs Bash-command-level filtering, which is out of scope here.Human verification
proven — 6 tests added, and the suite is not vacuous. 3 fail on the parent commit and pass here; 3 are regression guards that must pass on both sides.
Against
origin/main(4cb4071):On this branch, all 6 pass. Full suite, measured in two separate worktrees:
proven — behaviour, run against a throwaway fixture corpus (
QUERY_RECORDS_ROOT) so the transcript carries no real record content:missing — the
procedure-scout.mdchange has no automated coverage. Nothing underhooks/testsasserts an agent file's frontmatter, so the 151-pass figure speaks only to the script half. The allowlist-vs-steps check above is a paper review, not a test. I have also not observed a scout dispatched under the allowlist —tools:is applied by the harness at dispatch, and nothing in this repo exercises it.missing — the repo-local reviewer agents (
principles-reviewer,hygiene-reviewer,security-reviewer,test-reviewer) were not run; this session is under a standing instruction not to dispatch subagents unprompted. CONTRIBUTING asks for all four on a script change, so this is a known policy gap on this PR, not a waiver.What I dropped, and why
The original plan also collapsed the scout's
Readfan-out intoquery-records.sh --full. Dropped — the premise was stale.The transcripts I measured end
2026-08-07T10:58:46Z; PR #9 merged at11:45:59Z. So the 3.8 Reads/invocation I measured is entirely pre-#9 behaviour, and #9 already batches those into oneawk 'FNR==1{...}'call. Worse, Step 3 now deliberately says not to use--fullon a broad survey, for a reason that still holds — dumping before selecting wastes context on records a gloss would have excluded. Overriding a rule that had just landed, using numbers taken before it landed, would have been a regression dressed as an optimisation.Everything measured about the scout's cost predates #9 and should be re-measured before anyone acts on it.
Vendoring note
query-records.shis declared vendored from the codex repo, but the copies have already diverged substantially — the upstream copy lacks--limit,--full,--rel-ratioand--k-floor.procedure-scout.mdhas no upstream copy any more.These changes are bug fixes, not hosting adaptations, so they are not marked
# PLUGIN ADAPTATION— that marker is for divergence required by the plugin's hosting. Whether they should also land upstream is the open question in #19; this PR does not decide it.Review round 2 (f77e3b2)
CodeRabbit's
PLUGIN ADAPTATIONfinding was correct and is addressed at all five diverging sites. I had argued in this body that these were bug fixes and therefore exempt — wrong; the rule is about behavioral divergence from upstream, which they are regardless of motive.Re-reviewing also surfaced that the first pass shipped a worse bug than it fixed (the
prcase above). Turning a silent empty into a hardexit 2made a real gap louder while leaving the lookup equally impossible. That is now fixed properly; 4 tests cover it and all 4 fail on the parent commit.On the
am-i-donefork cost — what this PR can and cannot reach/am-i-doneis 69% fork-boot overhead (~30,145 cache-write tokens per invocation, for 2.8 turns of work). Measured composition of that boot context:work-reviewer.md+am-i-done/SKILL.mdCLAUDE.mdimport chain + memory indextools:Only 6.1% of it is plugin-owned. The
tools:allowlists here attack the schema share; the import-chain share is host-side and filed separately (orchard-codex#303). No change confined to this repo can move the 69% materially, and claiming otherwise would be the same overreach as theprfix.