fix(config): keep the generated header on its own line, and parse the spec once - #871
Conversation
… spec once Two from the review of #864, which merged before I had read them. The header says which spec a file came from, in a *line* comment, and the name is a file name — or whatever a caller hands to `source_of_spec`. A newline in it ended that comment and everything after it read as code, in a file the adopter did not write. Through `one_line`, like every other name that lands in a comment. And the watch list and the registry were each parsing the whole spec, so every build parsed it twice. Once now, with the includes printed before the registry is built — so a spec whose settings are refused still watches the file its author is about to go and edit. `generate_to` returns what it watched. Printing to cargo is not something a test in this process can see, so the list it prints and the list it returns are now one value rather than two assembled the same way: a watch list nothing checks is one that can quietly lose a file.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe config-build generator now parses specs once, returns watched paths from ChangesConfig generation
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: ⚪ Minimal · up to The PR keeps generated headers safe and ensures specification includes remain watched while avoiding duplicate parsing; no actionable merge-blocking risk remains beyond normal checks and review. Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
Greptile SummaryThe PR sanitizes spec names embedded in generated line comments and refactors registry generation to parse each spec once while returning the same source list emitted to Cargo.
Confidence Score: 5/5The PR appears safe to merge with no actionable correctness or security issues identified. The single-parse path preserves source ordering and watch behavior, all repository callers remain type-correct, and the header normalization blocks newline escape from the generated Rust comment. Important Files Changed
Reviews (1): Last reviewed commit: "fix(config): keep the generated header o..." | Re-trigger Greptile |
Instruction countsNothing was compared, and so nothing was gated. No series appears on both sides: either the base has no measurements recorded, or the two were measured on different runner classes, which are deliberately not comparable — counts shift between machine types by more than a real regression does. New, nothing to compare against: Only instruction counts gate. Wall clock is shown for context — on identical hardware it moves 4-20% run to run. Measured by tak — instruction-counted CLI benchmarks, stored in this repository's git notes. Shadow comparisonParsing
|
1 participant
Two findings from the review of #864, which merged before I had read them — my sweep was baselining on
the head commit, and these arrived nine minutes earlier.
The generated header could end its own comment. The first line says which spec the file came
from, in a line comment, and the name is a file name — or whatever a caller hands to
source_of_spec. A newline in it ended the comment and everything after it read as code, in a filethe adopter did not write. It goes through
one_linenow, like every other name that lands in acomment (the
prop::doc comments already did).The spec was parsed twice on every build.
generate_toaskedsource()for the registry andwatched()for the files to watch, and each of them parsed the whole spec. Once now — and theincludes are printed before the registry is built, so a spec whose settings are refused still
watches the file its author is about to go and edit.
The related half of that finding — reporting include paths whose own read or parse fails — I am
leaving. It needs usage-lib to report paths out through the error path, and it buys nothing: a build
script that fails produces no output, so cargo re-runs it on the next build regardless of what it
watched. The case that matters is a successful build followed by an edit, which is covered.
generate_tonow returns what it watched. Printing to cargo is not something a test in thisprocess can see, so the list it prints and the list it returns are one value rather than two
assembled the same way — a watch list nothing checks is one that can quietly lose a file. The test
asserts the returned list against
watched(), and dropping the includes from it fails.Two mutations, two dead tests: the header name interpolated raw, and the includes dropped from the
watch list.
AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5; version: unavailable.
Note
Low Risk
Build-time code generation and cargo rerun hints only; no runtime CLI or auth behavior.
Overview
Hardens generated registry headers so a spec name (or arbitrary
source_of_speclabel) cannot break out of the line comment via embedded newlines — the name now goes throughone_line, same as other comment-bound strings.Build pipeline changes in
generate_to: the spec is parsed once (parse) instead of separately insource()andwatched();cargo::rerun-if-changedfor includes is emitted before registry generation so invalid specs still watch the files the author will edit.generate_toreturnsVec<PathBuf>of watched paths (the same list it prints), so tests can assert watch behavior without relying on cargo output.Reviewed by Cursor Bugbot for commit ce1d0e0. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Bug Fixes
Improvements