feat(groom): path input scopes an audit to one directory, without resetting the cadence clock (BE-4757) - #83
feat(groom): path input scopes an audit to one directory, without resetting the cadence clock (BE-4757)#83mattmillerai wants to merge 4 commits into
path input scopes an audit to one directory, without resetting the cadence clock (BE-4757)#83Conversation
…esetting the cadence clock (BE-4757)
Adds `path` (string, default '') to groom.yml so a run — typically a manual
dispatch, but any caller may pin one — audits ONE directory instead of the whole
repo. Empty reproduces today's behavior, so all existing callers are unaffected.
Enforced, not merely instructed (scope_label/scope_desc were prompt prose only):
syntactic validation in the cheap gate job, filesystem containment against the
checkout in the finder job, a concrete in-scope file list handed to the finder,
and a post-filter that drops findings whose evidence lies entirely outside the
scope with the dropped count logged. The checkout stays FULL on purpose.
Critically, a scoped run must not stamp "done" over the whole-repo audit it never
performed: workflow_dispatch bypasses the interval gate, so a scoped run reaches
the finder. The finder job is renamed `Audit — finder (scoped)` and interval.py
excludes that marker when deriving the last real groom, so the next scheduled
whole-repo tick stays DUE. The dedup signature is likewise decoupled from `path`
(verifier.md's new `{{SIG_SCOPE}}` is wired to the caller's RAW scope_label), so
a scoped run and a whole-repo run suppress each other's duplicates.
mattmillerai
added
cursor-review
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 16 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 10 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 3 |
| 🟡 Medium | 6 |
| 🟢 Low | 1 |
Panel: 8/8 reviewers contributed findings.
…accuracy (BE-4757) Addresses the cursor-review panel on #83. - interval.py: the cadence clock is now PER SCOPE. The finder job marker carries the path (`(scoped: services/api)`), and a tick counts only a prior run of its own scope. A scope-blind `(scoped)` exclusion left a permanently path-scoped caller — a configuration the `path` input explicitly advertises — with no countable run in history, failing open on every tick and re-billing the audit daily regardless of GROOM_INTERVAL_DAYS. - interval.py: a job matched only by the bare `audit_find` id form carries no rendered display name, and therefore no scope marker, so it can no longer be attributed to a whole-repo run. Unknown scope now falls through to fail-open rather than letting a scoped run suppress the next full sweep. - scope.py: `normalize_site` collapses traversal and rejects a site that climbs out of the repo root, so `services/api/../../common/x` can no longer pass the lexical prefix test — the `..` hole `validate_path` already closes on the input side, closed on the side the agent controls. - scope.py: `scope_note` now states the ANY-in-scope rule the filter actually enforces. Demanding EVERY site be in scope told the agents to suppress the cross-boundary findings the full checkout exists to enable. - scope.py: the filter FAILS when the finder emitted no `findings` array. The workflow's only check is `jq '.findings | length'`, which scores a missing field as 0 — indistinguishable from a clean directory, so a structurally broken finder went green on nothing. - scope.py: `derive_scope` exposes `sig_scope`, the normalized (never path-derived) dedup-signature scope, wired to verifier.md's {{SIG_SCOPE}}. A blank `scope_label` previously reached the brief empty and yielded `repo::slug`. - scope.py: tracked filenames with control characters are dropped rather than inlined into the finder prompt, where a newline could forge file-list entries. - groom.yml: a scoped run with zero tracked files now fails loudly instead of buying a billed audit that can only report "clean" (an all-gitignored directory passes containment). - scope.py/groom.yml: drop the private consumer repo/directory tuple from this public repo, per AGENTS.md. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Review resolution pass — all 10 cursor-review panel findings triaged in Two were behavior bugs, not hardening:
Correctness/robustness: site Brief accuracy: Also: dropped the private consumer repo/directory tuple from this public repo, per AGENTS.md. Thanks for catching that one. Deferred (1): the verifier output is never re-filtered against the scope, so a DOWNGRADE that narrows a kept cross-boundary finding to purely out-of-scope evidence can still be filed. Real, but not a filter call in the right place — the verifier schema has no 140 groom tests pass; |
mattmillerai
added
cursor-review
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
🤖 The reviews loop filed Linear follow-up ticket(s) for review thread(s) deferred as out of scope for this PR:
|
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 9 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 1 |
| 🟡 Medium | 5 |
| 🟢 Low | 3 |
Panel: 8/8 reviewers contributed findings.
…g holes (BE-4757)
Cursor-review panel follow-ups on the `path` input, in severity order.
- The scope filter ran only on the FINDER's output, but a `DOWNGRADE` verdict
explicitly reshapes a finding — so a cross-boundary candidate that legitimately
passed that filter could be narrowed onto its OUT-of-scope half and filed under
a directory it no longer belongs to. `verifier.md` now emits `sites`, and
`scope.py verify` re-applies the same ANY-in-scope rule to what the verifier
confirmed. A finding with no LOCATABLE site is KEPT there (the opposite of the
finder-side rule): `sites` is advisory on that schema, so dropping on it would
discard every survivor and read as an honest "nothing survived verification".
- `canonicalize_signature` forces the dedup key's scope component back to the
literal the brief handed out, so signature scope-independence no longer rests
on an untrusted model obeying `{{SIG_SCOPE}}`. Left alone when `scope_label`
itself contains a colon — component boundaries are ambiguous there, and
corrupting a working dedup key is worse than the double-filing it guards.
- `normalize_site` no longer relativizes an absolute path that is NOT under the
clone; it returns "" (unlocatable). Stripping the leading slash silently
reinterpreted `/services/api/x.go` as repo-relative, so an out-of-tree site
could satisfy a matching scope.
- `resolve_within` rejects a SYMLINKED scope even when it points inside the
repo. Verified against git: `git ls-files -- <link>` returns the link entry
alone, so groom.yml's non-empty guard passed while the finder audited nothing
and reported clean. The error names the fix (pass the real directory).
- `list_files` reads bytes and decodes with surrogateescape: one non-UTF-8
tracked filename previously raised UnicodeDecodeError and killed the scoped
audit before the finder ran. `printable_path` drops surrogates too, and the
omitted count is now WARNED rather than silently shortening the list.
- `run_audited` compares the `(scoped: <path>)` marker case-SENSITIVELY, so
`services/api` and `services/API` keep separate cadence clocks instead of one
silently suppressing the other's due tick.
- Every caller-controlled value reaches argparse as `--flag=value`: a directory
named `-foo` is valid per the charset and the bare form reads it as an option.
mattmillerai
added
cursor-review
|
🤖 The reviews loop filed Linear follow-up ticket(s) for review thread(s) deferred as out of scope for this PR:
|
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 7 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 3 |
| 🟢 Low | 4 |
Panel: 8/8 reviewers contributed findings.
…e hardening (BE-4757)
Round-3 review findings on the `path` scoping PR. Five fixes, one deferral.
* SUBMODULE SCOPE READ AS CLEAN. `git ls-files -- <path>` on a submodule
returns the mode-160000 gitlink as a single entry, so `list_files` was
non-empty, groom.yml's empty-list guard passed, and the finder audited a
directory a default checkout never populates — reporting it clean. The
listing is now taken with `-s` and gitlinks are skipped (and warned about),
so a submodule-only scope falls through to the empty-list FAILURE instead.
Verified the premise: groom.yml never passes `submodules:` to
actions/checkout, so a submodule's files are genuinely unauditable here; the
error names the working path (groom it from its own repository).
* UNICODE LINE SEPARATORS FORGED PROMPT LINES. `printable_path` rejected ASCII
controls but not U+0085/U+2028/U+2029, which Git permits in filenames and
many consumers render as a line break — so a planted name could still forge
`- {f}` bullets in the finder's authoritative file list.
* FILE LIST CAPPED BY COUNT, NOT SIZE. 1500 deeply-nested near-PATH_MAX names
cleared `_MAX_LISTED_FILES` and still serialized to megabytes, overflowing
the finder's context. Now capped by bytes as well, always listing at least
one name so a single pathological path cannot empty the block.
* A VERDICT TYPO WAS SILENTLY DISCARDED. groom.yml's malformed-finding check
required `verdict` to be a string but not one of CONFIRM/DOWNGRADE/REJECT,
so `CONFIRMED` passed validation and was then dropped by the KEEP filter —
a real confirmed finding lost with the run green, the exact silent discard
that fail-loud step exists to prevent.
* AN OVER-LONG PATH KILLED THE CADENCE CLOCK. The path is embedded in the
finder job name as the `(scoped: <path>)` marker `interval.py` matches;
GitHub truncates a long job name, so no prior run is ever recognised and a
permanently scoped caller re-bills its audit every tick. `validate_path` now
caps the normalized path at 160 characters.
Not fixed, deliberately: `site_in_scope` stays purely lexical (replied on the
thread), and counting a PRE-AGENT finder failure as a spent audit is deferred
to a follow-up — it lives in `_AUDITED_CONCLUSIONS`, predates path scoping,
and changing it alters cadence for every groom run, whole-repo included.
Tests: submodule gitlink, the three line separators, the byte cap and its
always-list-one floor, and the path-length cap (including that it is measured
after normalization). 165 -> 170 green.
|
🤖 The reviews loop filed Linear follow-up ticket(s) for review thread(s) deferred as out of scope for this PR:
|
2 participants
ELI-5
Groom currently audits your whole repo or nothing. This adds a
pathinput so you can say "just look atservices/apithis time" — and, importantly, makes that actually true rather than a polite request in a prompt. It also makes sure a small scoped run doesn't trick groom into thinking it already did the big whole-repo sweep this week.What changed
New input
path(type: string,required: false,default: ''). Empty is today's whole-repo behavior; all 11 live callers pass nothing and are unaffected. Exposed as a normal input, not gated ongithub.event_name, so a monorepo caller can pin a permanently scoped run the way the studio fleet already groomsComfy-Org/cloud's two subtrees as independent units.It constrains, it does not instruct.
scope_label/scope_descare prompt substitutions — settingscope_desc: "only services/api"today asks and enforces nothing. Layered enforcement in new.github/groom/scope.py:gatejob, before any billed agent: absolute paths,..components, empty components, backslashes, control chars and anything outside a conservative[A-Za-z0-9._-]per-component charset are rejected. A dotted directory name (services/my..svc) is legitimate and is accepted — only a..component is dangerous.os.path.realpath(the Pythoncd && pwd -P) before the prefix compare, so a symlinked component can't escape and a trailing separator can't report a false escape. Also proves the directory exists, so a typo'd dispatch fails loudly instead of auditing an empty file list and reporting "clean".git ls-files), truncated at 1500 with the truncation announced.::warning::. Runs on the finder output because that is wheresiteslive — which also avoids paying the verifier to re-check findings that would be dropped anyway.The checkout stays full on purpose: a refactor in
services/apilegitimately referencescommon/, and a sparse checkout would blind the finder to the context that makes a finding correct.The cadence clock — the headline requirement.
workflow_dispatchbypasses the interval gate by design, so a scoped run does reach the finder. Without a fix, a manualpath: services/apirun would become the repo's "last real groom" and suppress the next scheduled whole-repo tick for a fullGROOM_INTERVAL_DAYS— a partial audit stamping "done" over the full one.I picked option (a), distinguishing the job name, over (b)
run-name. Reason:run-nameis a property of the caller workflow file, not the called reusable — a run created byci-groom.ymltakesci-groom.yml'srun-name, and the reusable's is ignored. Option (b) would therefore require editing all 11 callers to teach each one aboutpath, which directly contradicts "existing callers unaffected". Option (a) is implementable entirely inside the reusable. The coupling the ticket flags is real, so I made it explicit and pinned on both sides:groom.ymlrenames the finder jobAudit — finder (scoped)whenpathis set,interval.pygrew_SCOPED_JOB_MARKERwith a comment on each side saying "keep in sync", andScopedRunDoesNotResetCadenceintest_interval.pyasserts it end-to-end through the realevaluate(), with a control case proving an unscoped run in the same fixture does suppress the tick (so the assertion can't pass because the gate is broken).Dedup stays content-derived.
verifier.md's signature template moved from{{REPO_BASENAME}}:{{SCOPE_LABEL}}:<slug>to{{REPO_BASENAME}}:{{SIG_SCOPE}}:<slug>, andSIG_SCOPEis wired to the caller's rawscope_label, never the path-derived one. So a scoped run and a whole-repo run produce the same signature for the same defect and suppress each other — and, just as important, already-filed issues (whose markers carrywhole-repo) keep deduping, which a straight "drop the scope component" rewrite would have broken by re-filing every open finding once.Filed issues name their scope. When
pathis set andscope_label/scope_descare still at their documented defaults, they're derived (services/api, "theservices/apidirectory"). An explicit caller value always wins.Tests
python3 -m unittest discover -s .github/groom/tests -p 'test_*.py'— 125 tests, green (97 before). Newtest_scope.pymirrorsscan-path-scoping-test.sh's structure and names the same four invariants in the order they'd hurt.actionlintclean on both touched workflows..github/workflows/groom.ymladded totest-groom-scripts.yml's path filter, sincetest_scope.pynow asserts against the workflow's ownSIG_SCOPEwiring.Judgment calls
services/api/a.goandcommon/x.gois kept. Requiring every site in scope would suppress exactly the cross-cutting findings the full checkout was deliberately kept to enable. What gets dropped is a finding whose evidence lies entirely outside the scope (including one with no locatable sites, which can't be attributed). Called out because the acceptance criterion is compatible with either reading.scope.py.scope_label/scope_descnow have whitespace runs collapsed before reaching$GITHUB_OUTPUT, where an embedded newline would truncate the value and inject a bogus output key. YAML>-inputs already fold to one line, so this is byte-identical for every real caller.pr_size_limitand human review remain the backstops on patch breadth.(scoped)is deliberate. A caller whose own job name contained(scoped)would make a real whole-repo run stop counting — i.e. groom runs more often. Same fail-open bias as every other branch ininterval.py; never the silent under-run.Unmet criterion
AC 7's
docs/callers/groom.mdrow is NOT in this PR — that guide does not exist onmain; it lands in #80 (docs/caller-setup-guides-and-public-hygiene, still open). The ticket sanctions either "rebase on it or add the row when merging", and I chose not to stack: stacking would block this behind a docs PR for one table row. Documentation here instead goes to the three places that do exist onmain— thepathinput's own description (which covers the cadence interaction that AC 7 wants in the footguns section), the caller-pattern block ingroom.yml's header, thegroom.ymlrow inREADME.md, andAGENTS.md's layout section. Whichever of #80 and this PR merges second should add thepathrow + the cadence footgun todocs/callers/groom.md.Not applicable: negative-claim falsification
This change adds a capability rather than denying one. The only deny paths it introduces (
UnsafePathErroron/etc,../../etc, a non-existent directory) reject malformed input, not a product capability a user could reach another way — and each is paired with a positive test proving the legitimate neighbouring form (services/agent/internal/api,services/my..svc) is accepted, which is the anti-over-rejection half.