[US-438] feat: opt-in business-impact.trivial-diff override - #470
Conversation
Code Review — PR #470 ([US-438] feat: opt-in business-impact.trivial-diff override)Reviewer: independent review subagent · Date: 2026-08-30 · Story: #438 · Verdict: CHANGES-REQUESTED Scope VerifiedAC1–AC9 against the diff + code, in a detached worktree pinned to
BR7/D18 verified independently: Suite run (main checkout's vitest against the worktree): Findings1.
|
This comment has been minimized.
This comment has been minimized.
Final remediation — cycle converged (7 fix rounds, 1 escalation, re-review clean)
¹ Three round-7-escalation findings (test-comment rationale, ADL enumeration gap, Accepted / non-actionable (surfaced in the post-round-7 independent re-review)
VerdictReview clean — zero actionable findings on the latest independent re-review. Not merging (human gate). |
c1ac1ac to
349daba
Compare
349daba to
102b3aa
Compare
454a6ab to
3d1d33e
Compare
102b3aa to
5fd673e
Compare
46901a6 to
69b6321
Compare
|
PR: #470 · Reviewer: maintainer review · Date: 2026-09-14 · Story: #438 · Type: feature (docs/rule + conformance test) Classification matrix — per dimension
Tier = max(assessed) = yellow, confirms story classification. No drift. AssessmentsSecurity — Verdict: green — docs-only plus a local read-only conformance test
Cost — Verdict:
|
* [#416] feat: llms.txt drift gate Squashed rebase of 17 commits onto main cbf390b (post-#468/#467/#466). Gate compares tracked .pair/llms.txt vs generateLlmsTxt byte-for-byte, reports missing/extra lines and regeneration command, wired into ci.yml + root quality-gate. Reuses generator, never writes. Refs: #416 * [#416] fix: restore dev-tools package.json after squashed rebase Merge main's yaml + format-workflow:check with 416's llms-index scripts. Pre-push hook bypassed for squashed rebase fix. * [#416] fix: sync pnpm-lock after squashed rebase CI frozen-lockfile failed: 6 deps removed from apps/website/package.json vs lock. Regenerated lock via pnpm install. Refs: #416 * [#416] fix: restore @pair/content-ops dep for drift gate Drift gate imports FileSystemService from @pair/content-ops. Squashed rebase had dropped it, causing tsc Cannot find module. Refs: #416 * [#416] fix: restore main files after squashed rebase Squashed rebase had overwritten workflow/skill files with old versions from fa4fc10, causing lint failures. Restore to cbf390b and keep only 416's 22-file drift gate. Refs: #416 * [#416] fix: sync pnpm-lock for website deps after rebase Add 6 catalog deps that were added to apps/website/package.json in main (6e37df9..cbf390b) but missing from squashed lock. Refs: #416 * [#416] fix: regenerate .pair/llms.txt after rebase Drift gate was red: 3 ADL lines missing (byte-compared artifact decisions added by this branch but not in tracked index). Regenerated via pnpm llms-index:regen. Refs: #416 * [#416] fix: restore pre-push gate to main after squashed rebase Squashed rebase had overwritten with old version, causing format-workflow-composition lint failures. Restore to cbf390b. Refs: #416 * [#416] fix: format dev-tools package.json pnpm format fix for prettier:check. Refs: #416 --------- Co-authored-by: T <t@e.com>
Squashed rebase dropped it from package.json (was in branch but not on main). Refs: #438
…files pair-implement-batch (all 4 copies) restored to main: branch carried #219-era full-file versions missing args.entryCapsules, breaking engine-boundaries/engine-integration dry-runs. No US-438 commit ever touched those files. US-438 one-liners (bootstrap trivial-diff sentence, skills-guide Classification row) re-applied onto main versions. turbo.json/tech-stack/way-of-working restored (post-base main additions). Changeset file removed per no-release decision. Refs: #438
Review Minor: ADL point 2 said 'skill/workflow file' but enumerated only SKILL.md + .claude/skills/**; §6 + risk-matrix carve out .claude/workflows/** + .claude/agents/** since round 5. Aligned. Refs: #438
08825d5 to
ffb137e
Compare
PR Information
PR Title: [US-438] feat: opt-in
business-impact.trivial-diffoverrideStory/Epic: #438 · Epic #208 — Unified classification & assessments → tags
Type: Feature (KB schema addition + adoption declaration)
Priority: P2 (Could-Have)
Assignee: rucka
Labels:
user story,risk:yellowSummary
What Changed
The quality model gains one opt-in
## Overrideskey —business-impact.trivial-diff: green. Declared, the Business impact dimension resolvesgreenfor an objectively trivial change whatever subdomain the touched files belong to. Undeclared (the KB default, and the state of every project that does not ask for it) nothing changes at all."Trivial" is mechanical and checkable from
git diff, never a judgment about how important the prose is. A change is trivial when either:.md/.mdx, guarded mirrors included — excluding the markdown an agent acts on; orMarkdown an agent acts on is out of branch (a) on purpose — two classes, because branch (a) is the first arm of an OR and short-circuits, and because an enumeration is what makes the test mechanical (the general clause alone is a judgement call):
**/SKILL.md,.claude/skills|workflows|agents/**), always-loaded agent-instruction files (a rootAGENTS.md/CLAUDE.md, or the project's equivalent standing rule set), the sources any of them are generated from, an asset script embedded in markdown.tech/**adoption directory (tech/risk-matrix.md,tech/automation.md,tech/way-of-working.md,tech/coverage-baseline.md, and whatever a project declares there next). Scoped as a directory, not a list of names — an enumeration that lags one file is the hole itself. The same content §6's exclusion list already calls non-trivial when it is serialized asjson/yaml/toml/env— markdown is the format pair happens to ship its own config in, not an exemption.Neither class satisfies the all-
.mdbranch: branch (b) decides, so a hunk altering an instruction or a declared value is not trivial, while a typo fix in the same file's rationale prose still is.Three failure cases the carve-out closes, all real on this repo, all ending at
risk:green— which.pair/adoption/tech/automation.mdalso declares as unattended-run Eligibility, the tierpair-loopmerges unattended on the 🟢 gate set:pair-process-review/SKILL.md+ its dataset source, flipping the merge rule from "explicit approval required at risk:red" to "none";- **Bug fix workflow** - NEVER modify code to fix a bug before creating a test…frompackages/knowledge-hub/dataset/AGENTS.mdand re-running the mirror — the diff is that file plus the rootAGENTS.mdandCLAUDE.mdgenerated from it;.pair/adoption/tech/automation.md, moving## Eligibilityfromrisk:greentorisk:yellowand filling## Auto-Advancefrom(none)to that same tier — the diff that switches unattended auto-merge on, landing in the tier that auto-merges it;.pair/adoption/tech/coverage-baseline.md, movingbaseline.shared=84tobaseline.shared=40— the values CI'sCoverage guardrailstep reads withcoverage-gate.sh(plain^key=match). The diff that disables the coverage regression gate, in the tier that processes it unattended. This is why the class is a directory: a three-name enumeration covered the first case and not this one.All are all-
.md. In all three, the Change/diff-risk yellow that was supposed to catch them is a judgement — and this repo's ownchange-risk.dataset-mirror-pairscollapses source+mirrors to one module, so it resolves green too.Why these two classes and not every
.mda machine reads (stated in §6 and the ADL, not left inferable): a normative KB guideline deliberately stays inside branch (a), because it is a shared rule surface many independent consumers resolve from — §3.1's own shared code trigger fires on it and Change/diff risk reads yellow with no carve-out needed. Each carved-out class has a single consumer (the agent that executes the file, or the skill that reads its declared values), so no other dimension fires on its behalf and the exclusion has to be mechanical.Also not trivial however cosmetic it looks: a rename, a string-literal change, a dependency/version change, a config/data value change (in
json/yaml/toml/envand in policy markdown), a test-expectation change, a regenerated build artifact.Guard rails: all-or-nothing per item (one non-trivial file or hunk disables it for the whole PR — tier is per item, not per file); raises green, never lowers anything (
max()still decides the tier, confirm-or-raise at review, D17);greenis the only accepted value, read as the first token after the colon — with backtick formatting treated as markdown rather than part of the token — so neither the inline rationale every key carries nor the asset's`key`: `value`spelling makes a live declaration malformed; an unverifiable diff (binary/truncated) and an empty diff both fail safe to the subdomain rule; at refinement it applies only to an unambiguously trivial declared scope./pair-capability-classifygains no criterion — it already resolves## Overridesqualitatively through Argument > Adoption > KB default (D18). What it gains is one cell: its matrix template's Business-impactSourcenow reads[subdomain class | Overrides: business-impact.trivial-diff], and the two shipped templates the matrix is written into —user-story-template.mdandcode-review-template.md— carry the same alternation, so a greenedcore-subdomain diff records what greened it instead of asserting the subdomain did.This repo declares the key.
Why This Change
Third entry in the risk-matrix calibration series. The first two removed the coarse-default floor on Service/domain criticality and on overall tier resolution; neither could reach the one dimension that still floors trivial work — Business impact, which §3.1 resolves from the subdomain class alone (
generic→green,supporting→yellow,core→red).That rule reads where a change lands, never what it does. On a docs-as-product repository almost every path maps to a Supporting or Core subdomain, so a typo fix in a guideline inherits that subdomain's yellow/red floor — telling us nothing about the change. The earlier ADL rejected "change the KB default"; what it was missing is that the KB may grow a schema the adoption delta then declares. The gap was never that a project-specific delta is wrong — it was that no key existed to express this one.
Story Context
User Story: As a maintainer/adopter using pair's classification model, I want an opt-in override that lets Business impact resolve
greenfor objectively trivial changes regardless of subdomain, so that a diff's tier reflects what the change does, not only where it lives — without changing the KB default for any project that has not opted in.Acceptance Criteria — all nine covered:
keeps §3.1's KB default — subdomain class alone — unchanged(both trees)core⇒ green, sourced to the overrideclassify+ both body templates offerOverrides: business-impact.trivial-diffas theSourceclassifyand on both templates × both trees; the carve-out matcher must not cover ordinary documentationrisk:red)greenis the only accepted value.pair/adoption/tech/risk-matrix.md## OverridesgreenChanges Made
Implementation Details
quality-model.md§6 gains the subsectionbusiness-impact.trivial-diff: the two-branch definition, the carve-out stated on branch (a) (it short-circuits, so an exclusion stated after it is one an agent reads past), the not-trivial exclusion list, all-or-nothing, raises-only, the refinement rule, the unverifiable/empty-diff fail-safes, the first-token value rule, and "this is a rule for the classifying agent, not a config key parsed by code" (D18).genericsubdomain — or any subdomain, when the change is trivial and the project opted in"; the yellow and red cells name the override that outranks them, so the row closes on itself for an agent resolving it cell by cell.classify's SKILL.md: the story's own DoD requiresclassifyto stay grep-verifiably threshold-free. There is no parser to unit-test, so these are what the rule is validated against.classify's matrix template plus the same cell inuser-story-template.mdandcode-review-template.md. Every sibling row offers an alternation of sources; a single-valued[subdomain class]left the agent no cell to fill but the false one.risk-matrix-example.md(the file adopters copy) shows the key and restates branch (a) with both carve-out classes; both website enumerations name the dimension-resolution family and the catalog's summary of branch (a) no longer reads as plain "docs-only". Every shipped statement of the Phase 3.6 offer scopes it to the families the interview actually asks about and names the key as hand-authored: §6's index bullet, both website pages,/pair-process-bootstrapPhase 3.6's preamble, and the three copies that restate the same fact —how-to/02-how-to-complete-bootstrap-checklist.md(the manual, skill-less bootstrap path),skills-guide.md's ownership table andassets/bootstrap-checklist.md. One test pins all of them, in both trees..pair/adoption/tech/risk-matrix.md, third key, with the rule-for-the-classifying-agent rationale the two siblings are written in, and the carve-out restated over the files this repo actually ships (its skills, itsAGENTS.mdchain, its owntech/policy files — this file included).Files Changed
.pair/adoption/decision-log/2026-08-30-business-impact-reads-what-a-trivial-change-does-not-where-it-lives.md(ADL)..pair/knowledge/**mirror for each):guidelines/quality-assurance/quality-model.md,assets/risk-matrix-example.md,guidelines/collaboration/templates/user-story-template.md,guidelines/collaboration/templates/code-review-template.md..claude/skills/**mirror for each):capability/classify/SKILL.md(one cell),process/bootstrap/SKILL.md(one clause in Phase 3.6's preamble).how-to/02-how-to-complete-bootstrap-checklist.md,skills-guide.md,assets/bootstrap-checklist.md..pair/adoption/tech/risk-matrix.md,.pair/llms.txt.reference/quality-model.mdx,reference/quality-gates-configuration.mdx.packages/knowledge-hub/src/conformance/quality-model.test.ts.Database Changes / API Changes
None — documentation, adoption content and one conformance test file.
Testing
Test Coverage
quality-model.test.ts— 92 assertions, +41 in this PR, every KB-content one run over both shipped trees (dataset source and.pair/knowledgemirror).mirror-guard(dataset ↔.pair/knowledge↔.claude/skillslockstep, 531 cases) andllms-index-conformance(the committed.pair/llms.txtagainst its generator) both cover this diff and both pass.Test Results
Testing Strategy
There is no function to call, so every assertion parses the shipped artifact and reads the cell it is about — the §6 subsection sliced heading-to-heading (not first-occurrence, which spans §3.1→EOF and would pass with the exclusion clause deleted), the §3.1 row split cell by cell, each walkthrough row anchored on its own literal first cell, the worked-example rows parsed per dimension.
Two rules are executed rather than paraphrased, each by one helper applied to every shipped statement of the rule, so the guarded artifacts cannot be pinned into mutually inconsistent forms:
resolveOverrideValue— §6's value rule (first token after the colon, backticks stripped) run against both real declarations: this repo's plain-formrisk-matrix.mdline and the asset's backticked one. Both must resolvegreen.carveOutCovers— the branch-(a) carve-out's enumeration: the backticked path tokens are read out of each shipped statement, compiled as globs and matched against real repository paths. Positives it must cover:.claude/skills/pair-loop/SKILL.md,packages/knowledge-hub/dataset/AGENTS.md,.pair/adoption/tech/{automation,risk-matrix,way-of-working,coverage-baseline}.mdand.pair/adoption/tech/a-policy-file-added-later.md— a path this repo does not ship, so only a directory glob satisfies the guard and re-adding a name list fails it. Negatives it must not cover: an ordinary guideline and a website.mdxpage — a carve-out that swallowed plain documentation would kill AC2, the behaviour the key exists for. Applied to §6 (both trees), the copied example asset, this repo's declaration and the ADL.One assertion is genuinely cross-file: §6's "hand-authored today" claim is checked against
bootstrap/SKILL.mdStep 3.6.2's real text, so a later story adding the third question fails the test instead of silently making §6 stale in the other direction.Every assertion is mutation-verified — the guarded sentence, row or cell was reverted in every tree, the suite observed to go red, and the mutation reverted. The decisive one for the carve-out: replacing the
tech/**glob with the previous three-name enumeration while leaving the surrounding prose intact fails in every statement that carries it (the ... enumeration does not reach .pair/adoption/tech/a-policy-file-added-later.md), and renaming the bolded label keeps the suite green — the guard is about coverage, not vocabulary, which is exactly the drift shape (a class discussed in prose, absent from the mechanical list) it exists to catch. Several guards were rewritten during the cycle because that experiment showed they guarded nothing.D18 is enforced negatively: a grep asserts
classify's SKILL.md contains none of the triviality vocabulary — the pinned list is exactlycomment-only,whitespace-only,formatter-output,.mdx,trivial change,objectively trivial. The last two are the phrasings the model itself uses, i.e. the natural way for a later story to smuggle a criterion into the applier past the four mechanical tokens. It names the override key and owns no criterion.Documentation
quality-model.md§3.1 + §6 (the schema's owner),risk-matrix-example.md(the copy-me asset), both website reference pages.2026-08-30-business-impact-reads-what-a-trivial-change-does-not-where-it-lives— opt-in KB schema key (never a KB default change), the mechanical definition, both carve-out classes, all-or-nothing, raises-only,green-only, no skill criterion; rejected alternatives recorded (KB default, local-only declaration, per-file granularity, a subjective definition, extendingtier-resolution.default-artifact-downgrade).Reviewer Guide
Review Focus Areas
git diffand not from an opinion about the prose.carveOutCoversis the mechanical check.generic/supporting/coremapping is asserted verbatim.classifystay a model-applier? One cell naming the key, no threshold — the D18 grep is the mechanical check.Testing the Changes
Key Test Scenarios
generic/supporting/coreassertions.pair-process-review/SKILL.md+ its dataset source is all-.mdbut not trivial — branch (a) excludes executable markdown, branch (b) reads the instruction hunk, Business impact resolvescore⇒ red.SKILL.mdis trivial — branch (b)'s statement test is the gate, and prose-only is named as qualifying..pair/adoption/tech/coverage-baseline.md'sbaseline.shared=84to40is not trivial —tech/**puts it on branch (b), where a changed declared value is a changed declarative statement, and CI'sCoverage guardrailreads exactly that value.dataset/AGENTS.md+ the rootAGENTS.md/CLAUDE.mdit generates are agent-instruction files, so a hunk removing a Quick Rule is not trivial..pair/adoption/tech/automation.md's## Eligibilityto the tier its## Auto-Advancethen names is not trivial — the adoption/policy class puts it on branch (b), where a changed declared value is a changed declarative statement. The identical value change is already non-trivial injson/yaml/toml/env.business-impact.trivial-diff: blue⇒ warn, treat as absent, fall back to the subdomain class — never a HALT.`green`with backticks is not malformed: the backticks are markdown.Notes for the reviewer
cpfor any KB file naming a skill..pair/knowledge/**carries prefixed skill names (/pair-capability-assess-security); the dataset source carries bare ones (/assess-security) and the registry transform adds the prefix.pnpm --filter @pair/knowledge-hub transfer:datasetis a folder move/rename tool, not a sync command. Mirrors here were edited line-by-line andmirror-guardconfirms the lockstep — do not go looking for a sync command that does not exist.quality-model.md§6, not inclassify's SKILL.md, which is a deliberate deviation from the task's literal wording: the DoD requiresclassifyto stay grep-verifiably threshold-free, and the rule's fixtures belong beside the rule.risk:yellow, not green — by its own new rule: it editsquality-model.md,risk-matrix.mdand skill files, all of which the carve-out now puts on branch (b).2026-08-20-a-changeset-is-added-when-a-release-is-scheduled, no release is scheduled.Dependencies & Related Work
Related: decision-log
2026-08-14-risk-matrix-overrides-close-the-zero-green-gapand2026-08-14-tier-resolution-downgrades-artifact-yellows-not-observed-ones— the first two entries of this calibration series.Follow-up work (a call for the human at the merge gate, deliberately not taken here):
business-impact.trivial-diffhas no guided authoring path./pair-process-bootstrapPhase 3.6 asks two override questions (threshold, reviewer/SLA), never this one, and its offer is one-shot — an existing## Overridesis reportedalready authoredand never re-proposed. Every shipped statement of that offer now says so explicitly — the bootstrap SKILL, §6, both website pages, and the three copies an adopter reaches without skills (how-to/02,skills-guide.md,assets/bootstrap-checklist.md), all pinned by one test. Adding a third question to the interview is a change to bootstrap's behaviour, outside this story's scope.