config: guard review.fields key list against silent drift - #6092
Merged
Conversation
.loopover.yml.example had TWO top-level review: keys 546 lines apart: a "live" one holding auto_review + ~16 display/behavior toggles, and a second, fully-commented-out one 546 lines later holding everything else -- including footer/note/fields, the keys that actually control what renders in the PR comment. Most of the second block's keys (inline_comments, changed_files_summary, effort_score, impact_map, culture_profile, selftune, memory, finding_categories, auto_review) were themselves already documented in the first block, hand-duplicated rather than shared. Merged into one contiguous review: section: genuinely-unique keys (exclude_paths, path_filters, tone, profile, security_focus, instructions, path_instructions, linkedIssueSatisfaction, pre_merge_checks, enrichment, ai_model, visual, footer, note, fields) moved up; duplicate keys dropped from the second copy. Fixed the section-3 divider (it labeled itself "settings:" while preceding review:) and added a section-4 divider for the actual settings: block. config/examples/loopover.full.yml resynced to match byte-for-byte (enforced by test/unit/config-templates.test.ts). Closes #6071
The review.fields key list (linkedIssue | relatedWork | reviewLoad | validationEvidence | openPrQueue | contributorContext | gateResult | improvementSignal) is hand-copied in 5 documentation places plus one test fixture, each in a different textual format (YAML comment vs. prose vs. inline comment) -- too varied for a single generated line to regenerate safely, so this guards drift instead: a new test asserts every REVIEW_FIELD_KEYS entry appears in each doc copy. CONTRIBUTING.md was already stale (missing improvementSignal) -- fixed here, and the new guard would have caught it. test/unit/signals-coverage.test.ts's hardcoded KEYS array (the other previously-unguarded copy) now derives from REVIEW_FIELD_KEYS directly instead of duplicating it a second time. Closes #6070
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6092 +/- ##
==========================================
- Coverage 95.23% 95.23% -0.01%
==========================================
Files 595 595
Lines 47015 47013 -2
Branches 15015 15015
==========================================
- Hits 44773 44771 -2
Misses 1493 1493
Partials 749 749
Flags with carried forward coverage won't be shown. Click here to find out more. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #6070.
Stacked on #6088 (the config-merge PR) since this touches the same
.loopover.yml.example/loopover.full.ymlregion, just to keep line-shifted diffs clean — this PR's own diff doesn't otherwise depend on that one's content.Summary
The
review.fieldskey list is hand-copied in 7 places. I found a scope change from the issue's original proposal worth flagging explicitly: a single generated line (à lagen-cf-typegen.mjs) can't cleanly regenerate all 7 copies, because they're not the same text in different files — they're 4 genuinely different formats of the same information (a YAML block comment in the two.yml.examplefiles, a single-line inline comment in.loopover.yml/the bundled TS fallback, and an English sentence inCONTRIBUTING.md). Auto-generating into hand-written prose risked producing something that reads worse than what a maintainer would write, for marginal benefit over a guard.So this PR guards drift instead of eliminating the copies:
config-templates.test.ts): for each of the 5 documentation-bearing files (.loopover.yml,.loopover.yml.example,config/examples/loopover.full.yml,src/config/loopover-repo-focus-manifest.ts,CONTRIBUTING.md), assert every entry in the realREVIEW_FIELD_KEYSconstant appears somewhere in that file. A key added to the source without updating one of these copies now fails CI instead of rotting silently.CONTRIBUTING.mdwas already stale (missingimprovementSignal) — fixed as part of this PR, and the new test would have caught it before merge.test/unit/signals-coverage.test.ts's hardcodedKEYSarray (the other genuinely-unguarded copy) now derives fromREVIEW_FIELD_KEYSdirectly (filtered to excludeimprovementSignal, which that test's scenario never triggers) instead of duplicating the list a second time — this one copy is eliminated outright, not just guarded.Test plan
npx vitest run test/unit/config-templates.test.ts test/unit/signals-coverage.test.ts— 76/76 pass, including 5 new drift-guard cases (one per documentation file)npx tsc --noEmitcleannpm run docs:drift-check,npm run manifest:drift-check— passgit diff --checkclean