Repository navigation
Hybrid Signpost Generation (Cycle 0028) - #15
Conversation
Replace fully-generated signposts with hybrid model using <!-- generate:NAME --> / <!-- /generate --> markers.
5 tests for replaceGeneratedSections: marker replacement, multiple markers, no markers, unknown generator, whitespace preservation. Module does not exist yet.
Ship sync now finds <!-- generate:NAME --> / <!-- /generate --> marker pairs in signpost files and replaces their content using a registry of named generators. Hand-authored prose outside markers is preserved. Generators: - cli-commands: from CLI_TOPICS + usage() in cli-args.ts - mcp-tools: from MCP_TOOLS array in mcp.ts - signpost-inventory: static table of all signposts Converted signposts: - docs/CLI.md: hand-written intro + generated commands + hand-written config - docs/MCP.md: hand-written intro + generated tools + hand-written errors - docs/GUIDE.md: hand-written advice + generated signpost inventory Removed old generateCliReference/generateMcpReference from index.ts (replaced by generate.ts module). 127 tests pass (122 prior + 5 new generate tests).
Hill met. Hybrid signpost generation with generate markers. CLI.md, MCP.md, GUIDE.md all use markers. 127 tests pass. No drift.
Summary by CodeRabbit
WalkthroughThis PR implements a hybrid signpost generation system replacing full-file regeneration with selective marker-based updates. Documentation files now contain Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Rationale: Heterogeneous changes spanning documentation, new abstraction layer with regex-based parsing, and significant refactoring of core logic. The Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/generate.ts`:
- Around line 38-44: The exit codes table in generate.ts is hardcoded and can
drift from the canonical definitions; update generate.ts to derive the table
from the single source of truth (e.g., import the exit code constants or map
from errors.ts or cli-args.ts) instead of using literal lines.push entries, and
programmatically iterate over that exported constants/map to build the Markdown
rows so changes to the constants (including those used by method drift)
automatically update the generated table.
- Line 7: The MARKER_PATTERN regex in src/generate.ts mandates a literal "\n"
after the opening marker so it fails on CRLF files; update the MARKER_PATTERN
constant to allow an optional carriage return before the newline (replace the
literal "\n" with a CR-optional newline pattern such as "\r?\n") while
preserving the existing [\s\S]*? and the /gu flags so markers match both LF and
CRLF line endings.
- Around line 18-19: The template assembly in generate.ts assumes the string
returned by calling generator() ends with a newline, which is fragile; modify
the code around the generated variable (the result of generator()) so you
normalize it before embedding—e.g., compute generated = generator(); if it does
not endWith('\n') append a '\n'—then return the template string using that
normalized generated value so the closing marker always appears on its own line.
In `@tests/generate.test.ts`:
- Around line 82-105: Add three edge-case unit tests in tests/generate.test.ts
that exercise replaceGeneratedSections: (1) "malformed markers" — pass input
missing the closing <!-- /generate --> and assert that replaceGeneratedSections
throws a descriptive error (so malformed marker detection is covered); (2)
"empty generator output" — register a generator like 'x': () => '' and assert
the result preserves markers and produces no extra blank lines between markers
(exact string equality using toBe()); (3) "generator throws" — register a
generator that throws and assert that replaceGeneratedSections propagates that
error (use expect(...).toThrow()). Use the existing test structure and the same
join('\n') inputs/expectations so they run alongside the current "preserves
exact whitespace around markers" test.
- Around line 70-80: The test for unknown generator should assert that the
entire input (including the generate markers) is unchanged: update the assertion
in the test that calls replaceGeneratedSections to use an exact equality check
(expect(result).toBe(input)) instead of toContain, so the markers and content
are fully verified; relocate this change in the test case named "leaves marker
intact when no generator matches" that constructs input and calls
replaceGeneratedSections.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: faead2bc-195e-420b-a85a-1a41ab09cf97
📒 Files selected for processing (10)
docs/BEARING.mddocs/CLI.mddocs/GUIDE.mddocs/MCP.mddocs/design/0028-hybrid-signpost-generation/hybrid-signpost-generation.mddocs/method/retro/0028-hybrid-signpost-generation/hybrid-signpost-generation.mddocs/method/retro/0028-hybrid-signpost-generation/witness/verification.mdsrc/generate.tssrc/index.tstests/generate.test.ts
| export type GeneratorFn = () => string; | ||
| export type GeneratorRegistry = Record<string, GeneratorFn>; | ||
|
|
||
| const MARKER_PATTERN = /<!-- generate:(\S+) -->\n[\s\S]*?<!-- \/generate -->/gu; |
There was a problem hiding this comment.
Regex silently fails on CRLF line endings.
The pattern mandates a literal \n after the opening marker. Files with Windows-style \r\n line endings will never match, causing silent no-op behavior. This is a landmine waiting for a contributor on Windows or a file that passes through a tool that normalizes to CRLF.
🛠️ Proposed fix to handle both LF and CRLF
-const MARKER_PATTERN = /<!-- generate:(\S+) -->\n[\s\S]*?<!-- \/generate -->/gu;
+const MARKER_PATTERN = /<!-- generate:(\S+) -->\r?\n[\s\S]*?<!-- \/generate -->/gu;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const MARKER_PATTERN = /<!-- generate:(\S+) -->\n[\s\S]*?<!-- \/generate -->/gu; | |
| const MARKER_PATTERN = /<!-- generate:(\S+) -->\r?\n[\s\S]*?<!-- \/generate -->/gu; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/generate.ts` at line 7, The MARKER_PATTERN regex in src/generate.ts
mandates a literal "\n" after the opening marker so it fails on CRLF files;
update the MARKER_PATTERN constant to allow an optional carriage return before
the newline (replace the literal "\n" with a CR-optional newline pattern such as
"\r?\n") while preserving the existing [\s\S]*? and the /gu flags so markers
match both LF and CRLF line endings.
| const generated = generator(); | ||
| return `<!-- generate:${name} -->\n${generated}<!-- /generate -->`; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Fragile contract: relies on generators to include trailing newline.
The output format assumes generated ends with \n, otherwise the closing marker smashes onto the last content line:
<!-- generate:foo -->
some content<!-- /generate -->
All three current generators happen to comply, but this is implicit. A future generator omitting the trailing newline will produce malformed output with no warning.
🛡️ Defensive normalization
const generated = generator();
- return `<!-- generate:${name} -->\n${generated}<!-- /generate -->`;
+ const normalized = generated.endsWith('\n') ? generated : `${generated}\n`;
+ return `<!-- generate:${name} -->\n${normalized}<!-- /generate -->`;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const generated = generator(); | |
| return `<!-- generate:${name} -->\n${generated}<!-- /generate -->`; | |
| const generated = generator(); | |
| const normalized = generated.endsWith('\n') ? generated : `${generated}\n`; | |
| return `<!-- generate:${name} -->\n${normalized}<!-- /generate -->`; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/generate.ts` around lines 18 - 19, The template assembly in generate.ts
assumes the string returned by calling generator() ends with a newline, which is
fragile; modify the code around the generated variable (the result of
generator()) so you normalize it before embedding—e.g., compute generated =
generator(); if it does not endWith('\n') append a '\n'—then return the template
string using that normalized generated value so the closing marker always
appears on its own line.
| lines.push('### Exit Codes'); | ||
| lines.push(''); | ||
| lines.push('| Code | Meaning |'); | ||
| lines.push('|------|---------|'); | ||
| lines.push('| 0 | Success |'); | ||
| lines.push('| 1 | Error (bad arguments, missing workspace, failed operation) |'); | ||
| lines.push('| 2 | Drift found (from `method drift`) |'); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Exit codes table is hardcoded - potential drift risk.
Exit code semantics are duplicated here rather than derived from a single source of truth. If method drift changes its exit code or new codes are added, this table will silently drift.
Consider whether exit codes should be co-located with their definitions (e.g., constants in errors.ts or cli-args.ts) and referenced here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/generate.ts` around lines 38 - 44, The exit codes table in generate.ts is
hardcoded and can drift from the canonical definitions; update generate.ts to
derive the table from the single source of truth (e.g., import the exit code
constants or map from errors.ts or cli-args.ts) instead of using literal
lines.push entries, and programmatically iterate over that exported
constants/map to build the Markdown rows so changes to the constants (including
those used by method drift) automatically update the generated table.
| it('leaves marker intact when no generator matches', () => { | ||
| const input = [ | ||
| '<!-- generate:unknown -->', | ||
| 'existing content', | ||
| '<!-- /generate -->', | ||
| ].join('\n'); | ||
|
|
||
| const result = replaceGeneratedSections(input, {}); | ||
|
|
||
| expect(result).toContain('existing content'); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Weak assertion for unknown generator test.
The test only checks toContain('existing content') which doesn't verify the markers themselves are preserved intact. Consider strengthening to toBe(input) for exact preservation verification.
🔧 Proposed fix
it('leaves marker intact when no generator matches', () => {
const input = [
'<!-- generate:unknown -->',
'existing content',
'<!-- /generate -->',
].join('\n');
const result = replaceGeneratedSections(input, {});
- expect(result).toContain('existing content');
+ expect(result).toBe(input);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('leaves marker intact when no generator matches', () => { | |
| const input = [ | |
| '<!-- generate:unknown -->', | |
| 'existing content', | |
| '<!-- /generate -->', | |
| ].join('\n'); | |
| const result = replaceGeneratedSections(input, {}); | |
| expect(result).toContain('existing content'); | |
| }); | |
| it('leaves marker intact when no generator matches', () => { | |
| const input = [ | |
| '<!-- generate:unknown -->', | |
| 'existing content', | |
| '<!-- /generate -->', | |
| ].join('\n'); | |
| const result = replaceGeneratedSections(input, {}); | |
| expect(result).toBe(input); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/generate.test.ts` around lines 70 - 80, The test for unknown generator
should assert that the entire input (including the generate markers) is
unchanged: update the assertion in the test that calls replaceGeneratedSections
to use an exact equality check (expect(result).toBe(input)) instead of
toContain, so the markers and content are fully verified; relocate this change
in the test case named "leaves marker intact when no generator matches" that
constructs input and calls replaceGeneratedSections.
| it('preserves exact whitespace around markers', () => { | ||
| const input = [ | ||
| 'before', | ||
| '', | ||
| '<!-- generate:x -->', | ||
| 'old', | ||
| '<!-- /generate -->', | ||
| '', | ||
| 'after', | ||
| ].join('\n'); | ||
|
|
||
| const generators = { 'x': () => 'new\n' }; | ||
| const result = replaceGeneratedSections(input, generators); | ||
|
|
||
| expect(result).toBe([ | ||
| 'before', | ||
| '', | ||
| '<!-- generate:x -->', | ||
| 'new', | ||
| '<!-- /generate -->', | ||
| '', | ||
| 'after', | ||
| ].join('\n')); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding edge case tests.
The whitespace test is solid with exact toBe() matching. However, the suite lacks coverage for:
- Malformed markers (missing
<!-- /generate -->) - Empty generator output
- Generator that throws
These aren't blockers but would harden the implementation against regressions.
Want me to generate additional edge case tests?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/generate.test.ts` around lines 82 - 105, Add three edge-case unit tests
in tests/generate.test.ts that exercise replaceGeneratedSections: (1) "malformed
markers" — pass input missing the closing <!-- /generate --> and assert that
replaceGeneratedSections throws a descriptive error (so malformed marker
detection is covered); (2) "empty generator output" — register a generator like
'x': () => '' and assert the result preserves markers and produces no extra
blank lines between markers (exact string equality using toBe()); (3) "generator
throws" — register a generator that throws and assert that
replaceGeneratedSections propagates that error (use expect(...).toThrow()). Use
the existing test structure and the same join('\n') inputs/expectations so they
run alongside the current "preserves exact whitespace around markers" test.
1 participant
Summary
Replaces fully-generated signposts with a hybrid model. Hand-authored prose stays. Sections marked with
<!-- generate:NAME -->/<!-- /generate -->get replaced on ship sync.Generators:
cli-commands— fromCLI_TOPICS+usage()mcp-tools— fromMCP_TOOLSarraysignpost-inventory— static signpost tableConverted files:
docs/CLI.md— intro (hand) + commands (generated) + config (hand)docs/MCP.md— intro (hand) + tools (generated) + errors (hand)docs/GUIDE.md— advice (hand) + signpost inventory (generated)Test plan
method sync shippopulates all markers