ci: lint Markdown JS/TS code samples#66
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a Markdown JS/TS code-sample linter CLI with tests, wires it into CI and local pre-push gates as Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant FS as Filesystem
participant CI as CI Workflow
participant Lint as Markdown Linter (CLI)
participant TS as TypeScript Parser
Dev->>FS: push repo changes (linter + docs)
CI->>FS: checkout repository
CI->>Lint: run `npm run lint:md:code`
Lint->>FS: collect Markdown files
Lint->>TS: parse/extract fenced JS/TS blocks & request diagnostics
TS-->>Lint: return diagnostics
Lint-->>CI: report results (exit code)
CI->>CI: if lint passed -> continue to security audit gate
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 unit tests (beta)
📝 Coding Plan
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 |
Release Preflight
If you tag this commit as |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
docs/ADR-001-Folds.md (1)
122-128: Keep the View API example linted.This block is valid JS/TS syntax, so changing it to
textdrops the new Markdown sample-lint coverage for what still reads like an executable example. Consider keeping this fence asts(orjavascript) and reservingtextfor the arrow-notation pseudocode block below.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/ADR-001-Folds.md` around lines 122 - 128, The fenced code block showing usage of graph.view and View methods (view.traverse, view.query, view.renderAscii) should be marked as executable TS/JS so linters treat it as code: change the fence language from "text" to "ts" or "javascript" for the block containing const view = graph.view({ fold: { mode: "shallow", maxDepth: 1 } }); await view.traverse(...); await view.query(...); await view.renderAscii(...); and keep the later arrow-notation pseudocode block as "text".scripts/lint-markdown-code-samples.js (1)
136-139: Accessing TypeScript internalparseDiagnosticsproperty.The
parseDiagnosticsproperty is not part of TypeScript's public API surface — it's an internal property on theSourceFileobject. While this is a common pattern for accessing parse-only diagnostics without a full program, be aware it could break with future TypeScript major versions.The fallback to
|| []is a good defensive measure. Consider adding a brief comment noting the internal API usage for future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/lint-markdown-code-samples.js` around lines 136 - 139, The code currently reads parse-only diagnostics from an internal TypeScript SourceFile property (parseDiagnostics) when building diagnostics for the diagnostics variable; add a concise inline comment above this expression mentioning that parseDiagnostics is an internal/non-public TS API and may change across TypeScript major versions, keep the existing defensive fallback (|| []) and note why we're using this internal access so future maintainers understand the risk and can revisit if TS evolves; reference the parseDiagnostics property and the sourceFile/diagnostics usage in your comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/ADR-001-Folds.md`:
- Around line 122-128: The fenced code block showing usage of graph.view and
View methods (view.traverse, view.query, view.renderAscii) should be marked as
executable TS/JS so linters treat it as code: change the fence language from
"text" to "ts" or "javascript" for the block containing const view =
graph.view({ fold: { mode: "shallow", maxDepth: 1 } }); await
view.traverse(...); await view.query(...); await view.renderAscii(...); and keep
the later arrow-notation pseudocode block as "text".
In `@scripts/lint-markdown-code-samples.js`:
- Around line 136-139: The code currently reads parse-only diagnostics from an
internal TypeScript SourceFile property (parseDiagnostics) when building
diagnostics for the diagnostics variable; add a concise inline comment above
this expression mentioning that parseDiagnostics is an internal/non-public TS
API and may change across TypeScript major versions, keep the existing defensive
fallback (|| []) and note why we're using this internal access so future
maintainers understand the risk and can revisit if TS evolves; reference the
parseDiagnostics property and the sourceFile/diagnostics usage in your comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 160df22b-251c-4e60-b44a-03b55b35c753
📒 Files selected for processing (12)
.github/workflows/ci.ymlCHANGELOG.mdROADMAP.mddocs/ADR-001-Folds.mddocs/GUIDE.mddocs/ROADMAP/COMPLETED.mddocs/design/MIGRATION_PROBLEM.mddocs/design/joinreducer-split.mddocs/design/warpgraph-decomposition.mdpackage.jsonscripts/lint-markdown-code-samples.jstest/unit/scripts/lint-markdown-code-samples.test.js
|
@coderabbitai review please 🥕 |
|
✅ Addressed in commits
|
|
✅ Actions performedReview triggered.
|
Release Preflight
If you tag this commit as |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docs/ADR-001-Folds.md (1)
122-128: Add a clarifying note that the View API methods are proposed and not yet implemented.The code sample mixes implemented APIs (
query(),match(),run()) with unimplemented ones (graph.view(),view.traverse(),view.renderAscii()). While the document status is marked "Proposed (Deferred)" in the header and section 3 is titled "API Surface (Proposed)", the code block itself lacks direct indication that these methods don't currently exist.Consider adding a brief comment before the code block:
#### 3.1 View API +<!-- Note: The following APIs are proposed and not yet implemented --> ```ts const view = graph.view({ fold: { mode: 'shallow', maxDepth: 1 } });Alternatively, change the language tag from
tstotextto avoid implying these are currently valid TypeScript APIs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/ADR-001-Folds.md` around lines 122 - 128, The code sample mixes proposed/unimplemented APIs (graph.view, view.traverse, view.renderAscii) with implemented ones (query, match, run); update the documentation by adding a brief inline note above the snippet stating that graph.view, view.traverse, and view.renderAscii are proposed and not yet implemented, or alternatively change the code block language tag from "ts" to "text" so it doesn't imply valid TypeScript usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/lint-markdown-code-samples.js`:
- Around line 57-73: The fence detection only matches fences at column 1; update
the regex used when computing fenceMatch and the closePattern to allow up to
three optional leading spaces so indented fenced blocks are recognized.
Concretely, change the opening match (used to set activeFence with marker,
markerLength, language via parseFenceLanguage) to permit up to three leading
spaces before the backtick/tilde group, and build closePattern to also accept up
to three leading spaces before the marker repeat (preserving use of
activeFence.marker and activeFence.markerLength). Ensure the captured marker
(fenceMatch[1][0]) and language parsing still work with the adjusted regex.
- Around line 129-135: Replace the hardcoded ts.ScriptTarget.Latest when
creating sourceFile with the repository's configured target by loading the repo
tsconfig (tsconfig.base.json) compilerOptions.target, converting that target
string/number into the corresponding ts.ScriptTarget, and passing that value to
ts.createSourceFile; update the code around sourceFile / ts.createSourceFile
(which currently uses sample.language and scriptKind) to first read and parse
tsconfig.base.json (or use
ts.parseJsonConfigFileContent/ts.convertCompilerOptionsFromJson) to obtain
compilerOptions.target and map it to the ts.ScriptTarget enum before calling
ts.createSourceFile.
---
Nitpick comments:
In `@docs/ADR-001-Folds.md`:
- Around line 122-128: The code sample mixes proposed/unimplemented APIs
(graph.view, view.traverse, view.renderAscii) with implemented ones (query,
match, run); update the documentation by adding a brief inline note above the
snippet stating that graph.view, view.traverse, and view.renderAscii are
proposed and not yet implemented, or alternatively change the code block
language tag from "ts" to "text" so it doesn't imply valid TypeScript usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ddeb8668-2ceb-4aed-a340-e9434894fc96
📒 Files selected for processing (3)
docs/ADR-001-Folds.mdscripts/lint-markdown-code-samples.jstest/unit/scripts/lint-markdown-code-samples.test.js
|
@coderabbitai review please 🥕 |
|
✅ Addressed the failing
|
|
✅ Actions performedReview triggered.
|
Release Preflight
If you tag this commit as |
|
✅ Addressed the remaining CodeRabbit review comments in commits
|
Release Preflight
If you tag this commit as |
Release Preflight
If you tag this commit as |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/hooks/pre-push (1)
21-21:⚠️ Potential issue | 🟡 MinorMinor inconsistency in quick mode message.
Line 21 mentions "Gate 6 (unit tests)" but unit tests are now Gate 8 per lines 64-68. The message should reference Gate 8.
🐛 Proposed fix
- echo "WARP_QUICK_PUSH: quick mode active — Gate 6 (unit tests) will be skipped" + echo "WARP_QUICK_PUSH: quick mode active — Gate 8 (unit tests) will be skipped"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/hooks/pre-push` at line 21, The quick-mode status message in the pre-push hook is outdated: update the echo string that prints "WARP_QUICK_PUSH: quick mode active — Gate 6 (unit tests) will be skipped" so it references Gate 8 instead; locate the echo invocation in scripts/hooks/pre-push (the line emitting WARP_QUICK_PUSH) and change "Gate 6" to "Gate 8" so the message matches the actual gate numbering used later.
🧹 Nitpick comments (1)
scripts/lint-markdown-code-samples.js (1)
175-197: Consider adding symlink cycle protection.The recursive traversal follows into directories without checking for symlinks, which could cause an infinite loop if the repo contains circular symlinks. This is an edge case, but since this script runs in pre-push hooks blocking developer workflow, a defensive guard would be prudent.
♻️ Optional fix using a visited set
export function collectMarkdownFiles(startPath = root) { + const visited = new Set(); + return collectMarkdownFilesRecursive(resolve(startPath), visited); +} + +function collectMarkdownFilesRecursive(resolved, visited) { - const resolved = resolve(startPath); + const realPath = realpathSync(resolved); + if (visited.has(realPath)) { + return []; + } + visited.add(realPath); const stats = statSync(resolved);This requires importing
realpathSyncfromnode:fs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/lint-markdown-code-samples.js` around lines 175 - 197, The recursive collector collectMarkdownFiles currently follows directories without guarding against symlink cycles; add a visited-paths set (use realpathSync to get canonical paths) passed through recursion to skip directories whose realpath is already seen, and check Dirent.isSymbolicLink() or compare realpaths before recursing from the loop that calls collectMarkdownFiles(join(resolved, entry.name)), ensuring you import realpathSync and thread the visited set through initial and recursive calls so cycles are avoided.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/ADR-001-Folds.md`:
- Around line 122-129: The disclaimer currently calls out only graph.view(),
view.traverse(), and view.renderAscii() as not implemented; update the note to
state that the entire proposed API sketch is not implemented yet so readers
don't assume any part exists—explicitly mention graph.view, view.traverse,
view.renderAscii, and view.query (and any other sketched methods) in the
reworded sentence to make the scope clear.
In `@docs/ROADMAP/COMPLETED.md`:
- Line 399: The completed entry for B87 currently includes a self-expiring note
"pending merge" which will become inaccurate once the PR lands; update the B87
line in COMPLETED.md to use a stable status or remove the branch-state wording —
e.g., either omit "pending merge" entirely or replace it with a neutral phrase
like "awaiting merge" or "merged after review" depending on truth today; refer
to the B87 entry, branch name feature/b87-markdown-code-lint, and artifacts
scripts/lint-markdown-code-samples.js / npm run lint:md:code to ensure the
description remains correct and not time-sensitive.
---
Outside diff comments:
In `@scripts/hooks/pre-push`:
- Line 21: The quick-mode status message in the pre-push hook is outdated:
update the echo string that prints "WARP_QUICK_PUSH: quick mode active — Gate 6
(unit tests) will be skipped" so it references Gate 8 instead; locate the echo
invocation in scripts/hooks/pre-push (the line emitting WARP_QUICK_PUSH) and
change "Gate 6" to "Gate 8" so the message matches the actual gate numbering
used later.
---
Nitpick comments:
In `@scripts/lint-markdown-code-samples.js`:
- Around line 175-197: The recursive collector collectMarkdownFiles currently
follows directories without guarding against symlink cycles; add a visited-paths
set (use realpathSync to get canonical paths) passed through recursion to skip
directories whose realpath is already seen, and check Dirent.isSymbolicLink() or
compare realpaths before recursing from the loop that calls
collectMarkdownFiles(join(resolved, entry.name)), ensuring you import
realpathSync and thread the visited set through initial and recursive calls so
cycles are avoided.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9ac58aa5-beef-4c58-bc22-8ad2323ee25b
📒 Files selected for processing (9)
CHANGELOG.mdROADMAP.mddocker-compose.test.ymldocker/Dockerfile.denodocs/ADR-001-Folds.mddocs/ROADMAP/COMPLETED.mdscripts/hooks/pre-pushscripts/lint-markdown-code-samples.jstest/unit/scripts/lint-markdown-code-samples.test.js
✅ Files skipped from review due to trivial changes (1)
- docker-compose.test.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- test/unit/scripts/lint-markdown-code-samples.test.js
|
✅ Addressed in commits
@coderabbitai Thanks! One note on the symlink-cycle suggestion: this looks like a mix-up in Node's |
|
@coderabbitai review please 🥕 |
|
Thanks for the detailed explanation on the symlink behavior — that makes sense. Since ✏️ Learnings added
✅ Actions performedReview triggered.
|
|
Kicking off the review pass now! 🥕 ✏️ Learnings added
🧠 Learnings used✅ Actions performedReview triggered.
|
Release Preflight
If you tag this commit as |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/hooks/pre-push (1)
54-60: Gate numbering diverges from CI workflow.The pre-push hook gate numbers don't align with
.github/workflows/ci.yml:
Check Pre-push CI ESLint Gate 1 Gate 4 TypeScript Gate 2 Gate 1 IRONCLAD policy Gate 3 Gate 2 Consumer type surface Gate 4 Gate 3 Declaration surface Gate 5 Gate 5 Markdown lint Gate 6 Gate 6 Markdown code-sample Gate 7 Gate 7 When CI fails "Gate 1" (TypeScript), developers might incorrectly reference local "Gate 1" (ESLint). Consider aligning the numbering for consistent debugging experience.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/hooks/pre-push` around lines 54 - 60, The gate numbers in the pre-push hook messages are misaligned with CI; update the wait error messages to match the CI gate mapping by changing the reported gate numbers for the PID variables so they correspond to .github/workflows/ci.yml (set $TC_PID to "Gate 1", $POLICY_PID to "Gate 2", $CONSUMER_PID to "Gate 3", $LINT_PID to "Gate 4", keep $SURFACE_PID as "Gate 5", $MD_PID as "Gate 6", and $MD_CODE_PID as "Gate 7"); locate the wait lines referencing $LINT_PID, $TC_PID, $POLICY_PID, $CONSUMER_PID, $SURFACE_PID, $MD_PID, and $MD_CODE_PID and only change the human-readable "Gate N" numbers in the echo strings to the CI-consistent values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/hooks/pre-push`:
- Line 5: Update the inaccurate header comment in the pre-push hook that
currently reads "Eight gates in parallel, then unit tests." to correctly state
"Seven gates in parallel, then unit tests." so it accurately reflects that Gates
1–7 run in parallel and Gate 8 is the sequential unit tests; edit the comment
string in the scripts/hooks/pre-push file accordingly.
---
Nitpick comments:
In `@scripts/hooks/pre-push`:
- Around line 54-60: The gate numbers in the pre-push hook messages are
misaligned with CI; update the wait error messages to match the CI gate mapping
by changing the reported gate numbers for the PID variables so they correspond
to .github/workflows/ci.yml (set $TC_PID to "Gate 1", $POLICY_PID to "Gate 2",
$CONSUMER_PID to "Gate 3", $LINT_PID to "Gate 4", keep $SURFACE_PID as "Gate 5",
$MD_PID as "Gate 6", and $MD_CODE_PID as "Gate 7"); locate the wait lines
referencing $LINT_PID, $TC_PID, $POLICY_PID, $CONSUMER_PID, $SURFACE_PID,
$MD_PID, and $MD_CODE_PID and only change the human-readable "Gate N" numbers in
the echo strings to the CI-consistent values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3f332ca8-7445-45db-98c8-6abf90da1e39
📒 Files selected for processing (4)
CHANGELOG.mddocs/ADR-001-Folds.mddocs/ROADMAP/COMPLETED.mdscripts/hooks/pre-push
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/ADR-001-Folds.md
|
@coderabbitai review please 🥕 |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
Release Preflight
If you tag this commit as |
1 participant
Summary
scripts/lint-markdown-code-samples.jsto extract fencedjs/tsblocks from Markdown and syntax-check them with line-accurate diagnosticsnpm run lint:md:codeinto the CI fast firewall after markdownlinttextwhile restoring valid executable examples to code-fence coverageTest plan
npm run lint:mdnpm run lint:md:codenpx vitest run test/unit/scripts/lint-markdown-code-samples.test.js282test files,4981tests)ADR checks
Summary by CodeRabbit
New Features
Tests
Documentation
Chores