review: tier the 27 paths that were falling to the router's default - #322
Conversation
…ng to the router's default The consumer-config checker (#317) reported that 27 of this repo's 430 tracked files matched no `tier=` rule and silently took the router's default `low`. Most were harmless, but one group was not. `workflows/autofix/lib/*.ts` was `low`. That code decides what the autofix workflow does and it PUSHES COMMITS to PRs, so it has at least the blast radius of `workflows/review/lib/**` sitting next to it at `high`. It was low purely because no rule named it. Also now `high`: `.github/aw/actions-lock.json` (the SHA pins for every third-party action our compiled workflows run), `.github-staging/**` (a workflow staged for a human `git mv` into .github/workflows/ is CI the moment it moves, and is reviewed here or nowhere), and `workflows/*/package{,-lock}.json` (review.md's pre-agent step runs `npm ci` against the released lockfile inside a consumer's CI, so a dependency added here executes in every consuming repo). Medium for config that shapes releases, review routing, or what the reviewer can see, without being executable: .changeset/config.json, .github/REVIEWERS (the only source of the router's team ownership), .gitattributes (which files the reviewer skips as generated), root package.json, and tsconfig.json. Low, explicitly rather than by default, for the dev-only remainder: lint/format/test wiring, type shims, .github/NOTIFIED, and eval output. Checker now reports 0 errors and 0 warnings over all 430 files; high goes 100 -> 109. Each tier is spot-checked with --explain rather than inferred, including that autofix's own tests still fall to low via `**/*.test.ts`.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| # The autofix workflow's enforcement layer. It PUSHES COMMITS to PRs in consuming | ||
| # repos, so it has at least the blast radius of the reviewer lib beside it; it was | ||
| # falling to the default `low` purely because no rule named it. | ||
| workflows/autofix/lib/** tier=high |
There was a problem hiding this comment.
suggestion (non-blocking): This enumerates the 27 symptoms but leaves the fail-open root cause: unmatched paths still default to low. The PR's own diagnosis is 'it was low purely because no rule named it', and router.ts still hard-codes config.defaultTier ?? "low" (router.ts:530) with the only safety net being a checker warning the description says was already easy to miss. Since later rules win, a ** tier=medium rule placed FIRST would be overridden by every explicit rule below it and make unnamed files fail toward medium instead of low — though it would mute the 'next unrated file is a signal' property, so the alternative is promoting the checker's unmatched-file warning to an error.
A sketch, not a committable replacement:
Either add a leading `** tier=medium` catch-all (last-one-wins means all existing rules still override it), or make check-consumer-config treat unmatched files as an error rather than a warning — one of the two closes the recurrence path this PR documents.
There was a problem hiding this comment.
Agreed on the diagnosis, declining both remedies here, and I went and read the checker rather than arguing from the description.
The ** tier=medium catch-all is worse than muting the signal; it deletes the detector. check-consumer-config.ts computes the unmatched set as !riskRules.some((rule) => matchesGlob(file.path, rule.pattern)). A leading ** rule matches every path by construction, so unmatched is permanently empty and the warning that produced this PR can never fire again. The warning was easy to miss, which is your point; a check that is structurally incapable of firing is not the fix for one that is easy to miss. It would also silently move every genuinely-low unrated file to medium, and tier drives review budget, so that is a repo-wide spend increase to guard a case the checker already names.
The warning-to-error promotion is the right idea and is already an invocation choice, not a code change. The checker takes --strict, documented as "errors exit 1; warnings exit 0 unless --strict". So there is no code to write; what is missing is a CI job that runs it that way. That job cannot be added in this PR because the checker is not on main yet: it lives in #317, still open. Adding it here would either duplicate the file or depend on an unmerged branch. It belongs in #317 or a follow-up once #317 lands, and I would rather it be --strict in CI than a catch-all in the config, because it fails loudly at the moment of introduction instead of quietly absorbing the file.
One thing this PR does owe you, though, and it is a real disclosure. Once #317 lands, the fix for the blocking finding above (.github-staging/**/*.md tier=high) will itself trip a warning: it is the only pattern in the file matching no tracked file today, so it lands in the checker's deadPatterns. That is anticipated by the checker's own comment ("Deliberately dead patterns exist (a rule written ahead of the directory it guards), hence a warning rather than an error"), and it is exactly the tradeoff this thread is about: a prospective rule cannot be verified by a sweep over current files. So the honest verification number is 0 errors and 1 expected dead-pattern warning, not 0/0. I updated the description to say that instead of leaving the earlier 0/0 claim standing.
…d align the prose with ROUTING Two review findings on #322. `.github-staging/** tier=high` was silently defeated for `.md` files. Tier resolution is last-match-wins, and the broad `**/*.md tier=trivial` rule sits below it, so a staged agentic workflow's `.md` source (the frontmatter carrying permissions, secrets and network, plus the prompt) routed as trivial while only the compiled `.lock.yml` got `high`. That is backwards, and it defeats the rule's whole reason for existing. `.github/workflows/*.md tier=high` already exists to re-raise workflow sources after the docs rule; this adds the same re-raise for the staging pen. Verified with the repo's own parseRoutingConfig + matchesGlob: `.github-staging/workflows/review.md` went trivial -> high, and `.github-staging/**/*.md` matches at depth 1 as well as deeper. The sweep over all 430 tracked files is byte-identical before and after (111 high, or 109 after the generated-file exemption), confirming the rule is purely prospective: nothing under `.github-staging/` is a `.md` today. risk-classification.md (model-facing prose, injected into the same reviewer) had drifted from ROUTING and this PR would have codified the split. Aligned: the autofix lib, the staging pen, `workflows/*/package{,-lock}.json` and the gh-aw action lockfile move to High; `config/`, `types/`, `.eslintrc.js` and `pnpm-workspace.yaml` drop to Low, where ROUTING already puts them and where their actual contents belong (vitest setup and ambient `.d.ts` shims); `tsconfig.json` stays Medium on its own since it decides typecheck coverage; `.changeset/config.json`, `.github/REVIEWERS`, `.gitattributes` and the agent-steering prose dirs are named at Medium. Left alone deliberately: `workflows/review/eval/*.ts` is Medium in the prose and `low` in ROUTING. That divergence predates this PR and resolving it is a real tier decision about eval review depth, not a consistency edit.
Review Guidancegithub-actions (2 files)
Common patterns2 files: Parallel config + documentation update — the same paths ( - (no rule for workflows/autofix/lib/**)
+ workflows/autofix/lib/** tier=high
---
- (no high-risk entry for autofix lib)
+ - **The autofix workflow's enforcement layer**: `workflows/autofix/lib/**`. It decides what autofix does, and autofix PUSHES COMMITS to consumer PRs... |
There was a problem hiding this comment.
3 of 4 prior review threads resolved; 1 still unaddressed as of ff66dcb:
1 non-blocking thread still open
- suggestion (non-blocking)
.github/aw/review/ROUTING:40: This enumerates the 27 symptoms but leaves the fail-open root cause: unmatched paths still default tolow. The PR's ow...
| # defeated for exactly the file that carries the frontmatter -- permissions, | ||
| # secrets, network -- and the prompt. Nothing in the pen is docs; it holds files | ||
| # awaiting a `git mv` into .github/workflows/. | ||
| .github-staging/**/*.md tier=high |
There was a problem hiding this comment.
thought (non-blocking): The .github-staging/**/*.md re-raise patches one instance of a structural ordering hazard the file keeps. **/*.md tier=trivial (line 56) still comes after every directory-high rule, so each new high directory must remember its own re-raise; workflows/*/*.md tier=high (line 58) is single-star and does not reach lib/ subdirectories. I checked: neither lib dir contains a .md today, so this is latent, but the PR itself is evidence the trap fires in practice.
A sketch, not a committable replacement:
Consider restructuring so broad demotions (`**/*.md`, `**/*.test.ts`) sit before the specific directory escalations — with last-wins, escalation-late/demotion-early makes high directories win by construction and retires the re-raise pattern (audit the few paths whose tier would shift, e.g. `actions/**/*.md`).
2 participants
The consumer-config checker from #317 reported that 27 of this repo's 430 tracked files matched no
tier=rule and silently took the router's defaultlow. Most were harmless. One group was not.The one that matters
workflows/autofix/lib/*.tswaslow. That code decides what the autofix workflow does, and autofix pushes commits to PRs.workflows/review/lib/**sits right next to it athigh. There is no reading on which the autofix enforcement layer is lower-risk than the reviewer's; it waslowpurely because no rule named it.Also raised to
high.github/aw/actions-lock.json.github-staging/**git mvinto.github/workflows/is CI the moment it moves. It is reviewed here or nowhere.workflows/*/package.json,workflows/*/package-lock.jsonreview.md's pre-agent step runsnpm ciagainst the released lockfile inside a consumer's CI, so a dependency added here executes in every consuming repo.Medium
Config that shapes releases, review routing, or what the reviewer itself can see, without being executable:
.changeset/config.json— shapes what gets versioned and tagged. Medium rather than high because the publish code inutils/and its version-sync test are the actual gate..github/REVIEWERS— the router's only source of team ownership, so a wrong line silently routes reviews to the wrong team, or to nobody..gitattributes— decides which files the reviewer treats as generated and therefore skips entirely..cursor/rules/**— same reasoning as.claude/skills/**one directory over: Cursor rules steer an agent working in this repo, so they are executable instructions the broad docs rule would otherwise drop to trivial.package.json,tsconfig.json.Low, explicitly
Lint/format/test wiring, type shims,
.github/NOTIFIED, and eval output. These were already effectivelylow; naming them means the next unrated file is a signal rather than noise.Review fixes
Both review findings are addressed in ff66dcb.
.github-staging/** tier=highwas silently defeated for.mdfiles (blocking). Tier resolution is last-match-wins and the broad**/*.md tier=trivialrule sits below it, so a staged agentic workflow's.mdsource — the frontmatter carrying permissions, secrets and network, plus the prompt — routed astrivial, while only the compiled.lock.ymlgothigh. Backwards, and it defeats the rule's only reason for existing..github/workflows/*.md tier=highalready exists to re-raise workflow sources after the docs rule; this adds the same re-raise for the staging pen:risk-classification.mdhad drifted fromROUTING, and this PR would have codified the split. The two files are injected into the same reviewer, so shipping a disagreement in a PR whose whole purpose is making tiers deliberate would be self-defeating. Aligned in both directions: the autofix lib, the staging pen,workflows/*/package{,-lock}.jsonand the gh-aw action lockfile move to High;config/,types/,.eslintrc.jsandpnpm-workspace.yamldrop to Low, whereROUTINGalready puts them and where their actual contents belong (config/is onlytests/setup.ts,types/only ambient.d.tsshims);tsconfig.jsonsplits out and stays Medium on its own footing, since it decides which shipped codepnpm typecheckcovers at all.Two remedies I declined, with reasoning in-thread: a leading
** tier=mediumcatch-all would not merely mute the unrated-file signal, it would delete the detector (the checker computes its unmatched set as!riskRules.some(...), which a**rule satisfies for every path), and promoting the checker's warning to an error is already just its documented--strictflag, which needs a CI job in #317 rather than a config change here.One divergence left in place deliberately:
workflows/review/eval/*.tsis Medium in the prose andlowinROUTING. That predates this PR in both files, and closing it means deciding what review depth the eval suite deserves, which is a tier judgment rather than a consistency edit.Verification
check-consumer-config.ts --files-fromreported 0 errors, 0 warnings over all 430 files at the first commit (was 1 warning naming the 27).highgoes 100 → 109.Corrected after review: the honest number is now 0 errors and 1 expected warning. The
.github-staging/**/*.mdfix is the only pattern in the file matching no tracked file today, so once #317 lands it will surface in the checker'sdeadPatterns— anticipated by that code's own comment ("Deliberately dead patterns exist (a rule written ahead of the directory it guards), hence a warning rather than an error"). This is inherent to a prospective rule:.github-staging/holds one tracked file today and it is a.yml, so no sweep over current files can exercise the.mdpath. Verified directly that the fix regresses nothing: the tier sweep over all 430 tracked files is byte-identical before and after it (111 high, 109 after the generated-file exemption for the two.lock.yml).Each tier is checked with
--explainrather than inferred, including the ordering case that matters — autofix's own tests must still land atlow:No recompile:
ROUTINGis read at run time by the router, not embedded at compile time. No changeset: this changes only.github/aw/review/, this repo's own reviewer config, which ships in no package and is excluded fromcheck-for-changeset(precedent: #277 changedROUTINGalone with no changeset).One thing worth arguing with
workflows/*/package-lock.jsonathighputs a lockfile in the highest tier. Tier drives review depth and budget rather than whether the file is read line-by-line, and a dependency addition that lands in every consumer's CI seems worth the depth — but if you'd rather it weremedium, that's a defensible read.Follow-up, not this PR
tsconfig.json'sincludelistsactions/**,utils/**,types/**,config/**andvitest.config.ts, sopnpm typecheckdoes not coverworkflows/**at all — nothing typechecks the reviewer lib today. I hit that in #320, where a rename invalidated a call site and only the tests caught it. The review noted that a risk tier is the wrong layer to fix this with, which is correct;tsconfig.json's Medium tier stands on its own reasoning above, and the coverage gap wants its own PR.