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:
📝 WalkthroughWalkthroughAdds three GitHub Actions workflows for CI, advisory, and governance; a Markdown docs link checker and tests; expands the PR template and CONTRIBUTING guidance for testing/compliance; updates ESLint ignores and adds a docs-check npm script; and wires conditional CI gating based on detected change classifications. Changes
Sequence Diagram(s)sequenceDiagram
participant PR as Pull Request / Push
participant GHA as GitHub Actions
participant Detect as Detect Changes Job
participant Jobs as Conditional Jobs
participant Gate as required-pr Gate
participant Status as PR Status
PR->>GHA: Trigger workflow (pull_request / push / schedule)
GHA->>Detect: Run detect changes job
Detect->>Detect: Classify diffs → docs_changed, code_changed, workflow_changed
Detect->>Jobs: Emit flags
par Conditional execution
Jobs->>Jobs: If code_changed → lint, typecheck, build, unit tests
Jobs->>Jobs: If docs_changed → docs-sanity (docs:check)
Jobs->>Jobs: If workflow_changed → actionlint
end
Jobs->>Gate: Report statuses
Gate->>Gate: Aggregate required checks
Gate->>Status: Set PR pass/fail status
Status->>PR: Display gating result
sequenceDiagram
participant PR as PR Event
participant PG as PR Governance Job
participant GH as GitHub API
participant Risk as Risk Analyzer
participant Valid as PR Body Validator
participant Label as Label Manager
participant Summary as Job Summary
PR->>PG: Trigger on pull_request_target
PG->>GH: Fetch changed files and PR body
GH->>Risk: Provide file list
Risk->>Risk: Match risky patterns (auth, request, storage, etc.)
Risk->>PG: Set live_verification_required flag
PG->>Valid: Parse PR body for required sections and checkboxes
Valid->>Valid: Validate compliance and live verification consistency
alt live_verification_required != label state
PG->>Label: Create/update `needs-live-verification` label
Label->>GH: Attach/remove label as needed
end
Valid->>Summary: Build findings summary
Summary->>GH: Write job summary
PG->>PG: Fail job if validation failures exist
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 11 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/ci/docs-check.js">
<violation number="1" location="scripts/ci/docs-check.js:56">
P2: Non-markdown file arguments can crash the docs checker because they are passed to `walkMarkdownFiles` as if they were directories.</violation>
</file>
<file name=".github/workflows/ci.yml">
<violation number="1" location=".github/workflows/ci.yml:69">
P2: Docs change detection is too narrow for `test/` and can skip `docs-sanity` for markdown files like `test/AGENTS.md`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Pull request overview
Adds a required CI workflow with change detection and governance checks, plus an advisory workflow lane and a docs-link checker, and updates docs/templates to reflect the new CI model.
Changes:
- Introduces required
CIworkflow with change classification, required check aggregation, and workflow linting - Adds
PR Governance(template/compliance enforcement) andPR Advisory(coverage/compat/audit) workflows - Adds
docs:checkscript to validate local markdown links and workflow badge targets; updates contributor/testing docs and PR template
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/ci/docs-check.js | New Node script to validate Markdown local links and GitHub Actions workflow badge/link targets |
| package.json | Adds docs:check script entry |
| eslint.config.js | Ignores coverage/** in ESLint |
| docs/development/TESTING.md | Documents required vs advisory CI lanes; updates “See Also” links |
| docs/development/CONFIG_FIELDS.md | Updates cross-reference link to testing/CI guidance |
| README.md | Adds CI workflow badge/link |
| CONTRIBUTING.md | Updates contributor checklist to include new validation commands and live verification expectations |
| .github/workflows/pr-governance.yml | Adds PR template/compliance/live-verification enforcement via pull_request_target |
| .github/workflows/pr-advisory.yml | Adds advisory lanes for coverage, compat matrix, and dependency audit with change detection |
| .github/workflows/ci.yml | Adds required CI with smart change detection and required check aggregation |
| .github/pull_request_template.md | Expands testing checklist and adds docs impact + live verification checkboxes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
.github/workflows/ci.yml (1)
208-224: Consider addingnpm cifor future-proofing.The
docs-sanityjob skipsnpm cibecausedocs-check.jscurrently only uses Node.js built-ins. This works now but could break if the script ever adds npm dependencies.If you want to keep the job fast, the current approach is acceptable. Otherwise, consider adding the install step for consistency with other jobs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 208 - 224, The docs-sanity job currently runs "npm run docs:check" without installing dependencies, which may break if docs-check.js later requires packages; add an install step (run "npm ci") in the docs-sanity job before the "Verify markdown links and CI badge targets" step so that the environment matches other jobs and ensures reproducible installs for the "npm run docs:check" command.test/docs-check.test.ts (1)
3-11: Test covers the critical parentheses edge case.The test validates the specific hardening mentioned in the PR commits for markdown parsing. Consider expanding coverage in a follow-up to include additional edge cases:
- Code block exclusion (links inside fenced blocks should be ignored)
- Angle-bracket targets:
[text](<url with spaces>)- Links with title text:
[text](url "title")- Image links:
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/docs-check.test.ts` around lines 3 - 11, The test using extractMarkdownLinks currently validates parentheses-inside-link-targets but should be extended to cover additional edge cases: add assertions in the "keeps balanced parentheses inside markdown link targets" test (or a new test in the same suite) that verify links are ignored in fenced code blocks, that angle-bracket targets like [text](<url with spaces>) are parsed correctly, that links with title text like [text](url "title") return the url without the title, and that image links like  are excluded or handled as expected by extractMarkdownLinks; update the test cases to include examples for each of these scenarios referencing the extractMarkdownLinks import and the existing test structure so the parser hardening is fully covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/pr-governance.yml:
- Around line 61-65: The workflow currently only uses complianceChecked to gate
failure; update the pr-governance job to enforce all compliance checkboxes by
evaluating noLiveRequiredChecked, manualLiveChecked, and maintainerLiveChecked
alongside complianceChecked (and any additional checkbox patterns introduced in
the PR template) and fail when any of them is false. Locate the regex-derived
booleans (complianceChecked, noLiveRequiredChecked, manualLiveChecked,
maintainerLiveChecked) and change the condition that decides success/failure to
require all of them (e.g., logical AND of these variables or explicit checks),
ensuring the job reports failure if any of the required assertions is unchecked.
- Around line 64-65: The maintainer verification is being read from the PR body
via the regex that sets maintainerLiveChecked (and similarly later at lines
115-121), which the PR author can edit; change the logic to use a
maintainer-controlled signal instead (for example check for a repo label like
"maintainer-verified" or a GitHub review/approval from a user in the MAINTAINERS
list) rather than the PR body text. Update the code that currently tests body
with /- \[x\] Maintainer live verification completed\./i.test(body) to instead
query the PR's labels or reviews and set maintainerLiveChecked based on the
presence of that label or an authenticated maintainer approval; apply the same
change to the duplicate check at the later location so both places rely on the
trusted signal.
- Around line 41-50: The risky-path detector only checks file.filename, so
renames that move files out of risky directories bypass detection; update the
logic that builds changedFiles/riskyPaths to consider both file.filename and
file.previous_filename (when present) from the pulls.listFiles response: for
each file object (referenced by changedFiles and the riskyMatchers array),
collect both names into the list you filter, then evaluate
riskyMatchers.some(...) against both filename and previous_filename to ensure
moves out of lib/auth/, lib/request/, lib/storage (and index.ts) still trigger
liveVerificationRequired.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 208-224: The docs-sanity job currently runs "npm run docs:check"
without installing dependencies, which may break if docs-check.js later requires
packages; add an install step (run "npm ci") in the docs-sanity job before the
"Verify markdown links and CI badge targets" step so that the environment
matches other jobs and ensures reproducible installs for the "npm run
docs:check" command.
In `@test/docs-check.test.ts`:
- Around line 3-11: The test using extractMarkdownLinks currently validates
parentheses-inside-link-targets but should be extended to cover additional edge
cases: add assertions in the "keeps balanced parentheses inside markdown link
targets" test (or a new test in the same suite) that verify links are ignored in
fenced code blocks, that angle-bracket targets like [text](<url with spaces>)
are parsed correctly, that links with title text like [text](url "title") return
the url without the title, and that image links like  are
excluded or handled as expected by extractMarkdownLinks; update the test cases
to include examples for each of these scenarios referencing the
extractMarkdownLinks import and the existing test structure so the parser
hardening is fully covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ea8eaccb-7c36-4cfb-b504-59a9e8e4282b
📒 Files selected for processing (12)
.github/pull_request_template.md.github/workflows/ci.yml.github/workflows/pr-advisory.yml.github/workflows/pr-governance.ymlCONTRIBUTING.mdREADME.mddocs/development/CONFIG_FIELDS.mddocs/development/TESTING.mdeslint.config.jspackage.jsonscripts/ci/docs-check.jstest/docs-check.test.ts
There was a problem hiding this comment.
2 issues found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/ci/docs-check.js">
<violation number="1" location="scripts/ci/docs-check.js:23">
P2: `normalizeLinkTarget` can leave `<...>` wrappers on valid link destinations that include a title, causing false "Missing local target" failures.</violation>
</file>
<file name=".github/workflows/pr-advisory.yml">
<violation number="1" location=".github/workflows/pr-advisory.yml:71">
P3: Docs-only detection misses non-whitelisted root `*.md` files, so markdown-only PRs (e.g. `AGENTS.md`) still run expensive advisory jobs.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
.github/workflows/pr-governance.yml (3)
64-68:⚠️ Potential issue | 🟠 MajorTwo compliance assertions from the PR template are never enforced.
.github/pull_request_template.mdadds the official-auth and tests/docs checkboxes, but this job only parses and fails on the first compliance line. A PR can leave those unchecked and still get a greenpr-governanceresult.Also applies to: 114-116
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/pr-governance.yml around lines 64 - 68, The PR governance job only checks complianceChecked and the live-related flags; add regex tests for the two missing checkboxes from the template (official-auth and tests/docs) and include them in the overall failure condition so the job fails when either is unchecked. Concretely, add constants like officialAuthChecked = /- \[x\] Official auth required and documented\./i.test(body) and testsDocsChecked = /- \[x\] Tests\/docs updated\/added as needed\./i.test(body) (use the exact checkbox text from the PR template), then update the pass/fail logic that currently uses complianceChecked, noLiveRequiredChecked, manualLiveChecked, and maintainerLiveChecked to also require officialAuthChecked and testsDocsChecked; ensure the job logs which checkbox is missing and exits non‑zero when any required box is not checked.
68-68:⚠️ Potential issue | 🟠 MajorUse a maintainer-owned signal for maintainer verification.
maintainerLiveCheckedcomes from the PR body, so the author can tick it themselves and satisfy the required gate without any maintainer action. Gate this on something the PR author cannot edit, like a label or maintainer review.Also applies to: 118-123
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/pr-governance.yml at line 68, The current maintainerLiveChecked boolean is derived from the PR body (the regex on body), which the author can edit; replace that check with an authority-bound signal such as a maintainer-owned label or an official maintainer review. Specifically, update the logic that sets maintainerLiveChecked to instead inspect context.payload.pull_request.labels for a maintainer verification label (e.g., "maintainer-verified") or check reviews for an APPROVED review from a user in your MAINTAINERS list (or a review with a "maintainer" team membership), and apply the same change to the duplicate logic referenced around lines 118-123 so both places rely on the immutable maintainer-owned signal rather than the PR body.
44-54:⚠️ Potential issue | 🟠 MajorInclude renamed-from paths in the risky matcher.
A PR can move
lib/auth/...,lib/request/..., orlib/storage...out of a risky path and clearliveVerificationRequired, because this only inspectsfile.filename. Match against bothfile.filenameandfile.previous_filenamefrompulls.listFiles.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/pr-governance.yml around lines 44 - 54, The risky-path check only tests file.filename, allowing renames to bypass detection; update the logic that builds riskyPaths (which uses changedFiles, riskyMatchers, riskyPaths, and liveVerificationRequired) to test both file.filename and file.previous_filename from the files list returned by pulls.listFiles (falling back to undefined safely), i.e., when filtering run riskyMatchers.some(...) against both names so renames still count as risky and liveVerificationRequired is set appropriately.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/pr-governance.yml:
- Around line 72-84: The ensureLabel() function needs to tolerate a race when
two PRs both get a 404 and try to create the same repo-scoped label; update the
error handling in ensureLabel() (around github.rest.issues.getLabel and
github.rest.issues.createLabel) so that when createLabel throws a 422 (label
already exists) you swallow/ignore that specific error and continue, but still
rethrow any other errors (i.e., catch errors from createLabel, if error.status
=== 422 || error.message includes 'already_exists' return/continue, otherwise
throw).
In `@scripts/ci/docs-check.js`:
- Around line 239-242: The current check resolves rawPath to resolvedPath and
then tests existence, but doesn't prevent paths that escape the repository;
modify the logic in the code that computes resolvedPath (using path.resolve) to
first compute rel = path.relative(ROOT, resolvedPath) and reject any target
where rel starts with '..' or is an absolute escape (e.g., rel === '' and
resolvedPath outside ROOT), returning an error like "Target escapes repository
root" before calling exists(resolvedPath); keep the exists(resolvedPath) call
only after the boundary check so functions like exists and symbols resolvedPath,
rawPath, ROOT, path.relative are used as described.
---
Duplicate comments:
In @.github/workflows/pr-governance.yml:
- Around line 64-68: The PR governance job only checks complianceChecked and the
live-related flags; add regex tests for the two missing checkboxes from the
template (official-auth and tests/docs) and include them in the overall failure
condition so the job fails when either is unchecked. Concretely, add constants
like officialAuthChecked = /- \[x\] Official auth required and
documented\./i.test(body) and testsDocsChecked = /- \[x\] Tests\/docs
updated\/added as needed\./i.test(body) (use the exact checkbox text from the PR
template), then update the pass/fail logic that currently uses
complianceChecked, noLiveRequiredChecked, manualLiveChecked, and
maintainerLiveChecked to also require officialAuthChecked and testsDocsChecked;
ensure the job logs which checkbox is missing and exits non‑zero when any
required box is not checked.
- Line 68: The current maintainerLiveChecked boolean is derived from the PR body
(the regex on body), which the author can edit; replace that check with an
authority-bound signal such as a maintainer-owned label or an official
maintainer review. Specifically, update the logic that sets
maintainerLiveChecked to instead inspect context.payload.pull_request.labels for
a maintainer verification label (e.g., "maintainer-verified") or check reviews
for an APPROVED review from a user in your MAINTAINERS list (or a review with a
"maintainer" team membership), and apply the same change to the duplicate logic
referenced around lines 118-123 so both places rely on the immutable
maintainer-owned signal rather than the PR body.
- Around line 44-54: The risky-path check only tests file.filename, allowing
renames to bypass detection; update the logic that builds riskyPaths (which uses
changedFiles, riskyMatchers, riskyPaths, and liveVerificationRequired) to test
both file.filename and file.previous_filename from the files list returned by
pulls.listFiles (falling back to undefined safely), i.e., when filtering run
riskyMatchers.some(...) against both names so renames still count as risky and
liveVerificationRequired is set appropriately.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 50699a6c-2fd2-4ca8-b783-97f3a616faf8
📒 Files selected for processing (5)
.github/workflows/ci.yml.github/workflows/pr-advisory.yml.github/workflows/pr-governance.ymlscripts/ci/docs-check.jstest/docs-check.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/ci.yml
- test/docs-check.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/ci/docs-check.js`:
- Around line 215-235: getWorkflowPathFromUrl currently treats any
github.com/actions/workflows URL as a local workflow; change it to only return a
workflow filename when the URL path includes the current repository owner/name.
Update getWorkflowPathFromUrl to parse pathname with a regex like
/^\/([^/]+)\/([^/]+)\/actions\/workflows\/([^/]+)(?:\/badge\.svg)?$/ to capture
owner, repo, and workflowFile, then compare owner/repo against the current
repository (use process.env.GITHUB_REPOSITORY if available or a repository
constant used by this script) and only return the workflowFile when they match;
keep validateLink behavior but rely on the updated getWorkflowPathFromUrl to
avoid flagging external repo badges as missing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 450bb52a-7294-40f3-9496-d2dbb8d86875
📒 Files selected for processing (6)
.github/pull_request_template.md.github/workflows/pr-advisory.yml.github/workflows/pr-governance.ymlCONTRIBUTING.mdscripts/ci/docs-check.jstest/docs-check.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/pull_request_template.md
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/ci/docs-check.js">
<violation number="1" location="scripts/ci/docs-check.js:225">
P2: Normalize owner/repo casing before comparing, otherwise same-repo workflow URLs with different case bypass workflow validation.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/pr-governance.yml (1)
142-144: Consider specifying which sections are missing in the error message.The current message doesn't tell contributors which specific sections they need to add, making it harder to fix.
📝 Suggested improvement
+ const sectionNames = ["Summary", "Testing", "Docs Impact", "Compliance Confirmation", "Notes"]; + const missingSectionNames = missingSections.map((pattern, i) => sectionNames[i] || pattern.toString()); if (missingSections.length > 0) { - failures.push("PR description is missing one or more required template sections."); + failures.push(`PR description is missing required sections: ${missingSectionNames.join(", ")}.`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/pr-governance.yml around lines 142 - 144, When pushing the failure into failures, include the actual missing section names from missingSections so contributors know what to add; replace the static message in the block that checks if (missingSections.length > 0) (the failures.push call) with a message that appends or interpolates missingSections.join(', ') (or formatted list) into the error string so it reads something like "PR description is missing required template sections: X, Y, Z."
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/pr-governance.yml:
- Around line 142-144: When pushing the failure into failures, include the
actual missing section names from missingSections so contributors know what to
add; replace the static message in the block that checks if
(missingSections.length > 0) (the failures.push call) with a message that
appends or interpolates missingSections.join(', ') (or formatted list) into the
error string so it reads something like "PR description is missing required
template sections: X, Y, Z."
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a43f4a85-4e40-4f46-98d9-fdcf13b61173
📒 Files selected for processing (5)
.github/workflows/ci.yml.github/workflows/pr-advisory.yml.github/workflows/pr-governance.ymlscripts/ci/docs-check.jstest/docs-check.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- scripts/ci/docs-check.js
- .github/workflows/pr-advisory.yml
- .github/workflows/ci.yml
- test/docs-check.test.ts
|
@coderabbitai review |
@ndycode I have started the AI code review. It will take a few minutes to complete. |
|
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
@ndycode I have started the AI code review. It will take a few minutes to complete. |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/pr-governance.yml">
<violation number="1" location=".github/workflows/pr-governance.yml:41">
P1: The markdown stripping is incomplete, so governance checkboxes can still be spoofed inside valid code syntax (e.g., `~~~` fenced blocks or multi-backtick code spans).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
2 issues found across 12 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="docs/development/TESTING.md">
<violation number="1" location="docs/development/TESTING.md:42">
P3: Update the advisory-matrix description to include the Windows Node 18 lane so the testing guide matches the actual workflow.</violation>
</file>
<file name=".github/workflows/pr-governance.yml">
<violation number="1" location=".github/workflows/pr-governance.yml:40">
P1: Compliance checkbox validation can be bypassed via HTML comments because comment content is not stripped before regex checks.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
Addressed the remaining governance/docs review items in
Reset the PR body back to the human-maintained summary for current head @coderabbitai review |
@ndycode I have started the AI code review. It will take a few minutes to complete. |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
2 issues found across 12 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/pr-governance.yml">
<violation number="1" location=".github/workflows/pr-governance.yml:20">
P2: The concurrency key is too specific: including `github.event.action` prevents stale governance runs for the same PR from being canceled.</violation>
</file>
<file name="scripts/ci/docs-check.js">
<violation number="1" location="scripts/ci/docs-check.js:270">
P2: `extractMarkdownLinks` misses shortcut reference links (`[label]`), so those links are not validated and broken docs links can slip through.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
Addressed the shortcut-reference docs-check gap in
Refreshed the PR body for current head @greptile @cubic review |
@ndycode I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
2 issues found across 12 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/ci/docs-check.js">
<violation number="1" location="scripts/ci/docs-check.js:251">
P1: The shortcut-reference regex is too permissive and matches inline/full-reference links, which can cause false broken-link failures in docs-check.</violation>
</file>
<file name=".github/workflows/pr-governance.yml">
<violation number="1" location=".github/workflows/pr-governance.yml:20">
P2: Concurrency is partitioned by event action, so governance runs for the same PR can execute in parallel and race on label updates/state checks.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
Addressed the remaining governance/docs-check review items in
Re-ran the full local validation set on the committed head:
Reset the PR body back to the human-maintained summary for current head @coderabbitai review |
|
✅ Actions performedReview triggered.
|
|
Addressed the remaining fresh governance review item in
Re-ran the local validation set on the committed head:
Reset the PR body back to the human-maintained summary for current head @coderabbitai review |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 245-247: The CI job invoking the docs-check script uses "npm run
docs:check", but docs-check.js contains an isDirectRun guard that checks
process.argv[1] against __filename so main() never runs when launched via npm;
update the workflow step that currently runs npm run docs:check to invoke the
script directly with Node (e.g., run node ./scripts/ci/docs-check.js) so the
isDirectRun check passes and main() executes, ensuring the markdown link/CI
badge validation actually runs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bad10035-39c7-467e-8c41-32849c519255
📒 Files selected for processing (6)
.github/workflows/ci.yml.github/workflows/pr-advisory.yml.github/workflows/pr-governance.ymldocs/development/TESTING.mdscripts/ci/docs-check.jstest/docs-check.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/docs-check.test.ts
Summary
ci.ymlworkflow with smart change detection,required-pr, and workflow lintingTesting
npm run docs:checknpm run lintnpm run typechecknpm run buildnpm testDocs Impact
Compliance Confirmation
maintainer-live-verifiedlabel after independent live verification.Notes
npm run test:coverageandnpm run audit:ciremain advisory because the current baseline still fails the global coverage threshold and reports the existinghonoaudit issue. This PR also touches.github/pull_request_template.md, so theexemptlabel is needed for the existingpr-qualityworkflow.Summary by CodeRabbit
Release Notes
Documentation
Tests
Chores
note: greptile review for oc-chatgpt-multi-auth. cite files like
lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.Greptile Summary
this PR introduces a complete CI/PR automation layer: a required
ci.ymlworkflow with smart rename-aware change detection and a hardenedrequired-praggregate gate, advisorypr-advisory.ymllanes for coverage, compat-matrix, and dependency audit, apr-governance.ymlworkflow that validates the PR template and manages live-verification labels viapull_request_target, adocs-check.jslink validator using only node built-ins, and accompanying docs/template updates.changesanddetect-dependency-changeboth havetimeout-minutes: 10,required-prfails fast whenCHANGES_RESULT != "success", HTML comments are stripped before link extraction, andscripts/ci/changes now triggerdocs_changed=truepr-governance.ymlcorrectly avoids checking out PR head ref underpull_request_target, uses live label reads viagithub.paginate, and serialises runs (cancel-in-progress: false) to prevent label race conditions — no windows filesystem or token-safety paths are touched by this PRcoverage,compat-matrix,dependency-audit) lack an equivalentDETECT_RESULTguard: ifdetect-dependency-changefails or is cancelled, all three advisory jobs are silently skipped with no step summary written; since these are unrequired this does not block merge, but silent skips are hard to debuggetSectionContentin the governance script builds aRegExpfrom an unescaped heading string — benign with today's hardcoded callers but a latent injection risk if the function is reusedtest/docs-check.test.tsretry cleanup is intentionally broad (retries all errors, not just Windows transient ones), which could leave orphanedtmp/docs-check-*dirs on Windows runners if the retry budget is exhausted by a non-transient error; low severity given runner teardownConfidence Score: 4/5
CHANGES_RESULTcheck, timeouts) and governance workflow correctness (pull_request_targetwithout head checkout, live label reads, serialised concurrency) are all solid. the remaining issues are advisory-lane visibility and a latent regex-injection in a helper function — neither blocks the required CI path or introduces a token/filesystem safety risk on windowspr-advisory.yml(silent skip on detect failure) andpr-governance.yml(unescaped regex ingetSectionContent)Important Files Changed
required-praggregate check; previous thread issues (missing timeout, CHANGES_RESULT guard) are resolveddetect-dependency-changehas a timeout but downstream jobs lack a result guard — a failed detection job silently skips all advisory lanes with no summary outputpull_request_targetwithissues: writeto manage labels and validate PR template; correctly avoids checking out PR head ref;getSectionContentbuilds regex with unescaped heading string — benign today but latent if reusedtypecheck,docs:check, Docs Impact section, and live-verification checklist items aligned with the newpr-governanceworkflow requirementsrequired-prandpr-governanceas the two branch-protection gatesdocs:checkandaudit:ciscripts; no auth, request-routing, or storage paths changed; dependency versions unchangedSequence Diagram
sequenceDiagram participant PR as Pull Request participant CH as changes job participant CJ as code jobs<br/>(lint/typecheck/build/unit-linux/unit-windows) participant DS as docs-sanity participant AL as actionlint participant RP as required-pr gate participant PG as pr-governance<br/>(pull_request_target) participant AD as advisory lanes<br/>(coverage/compat/audit) PR->>CH: trigger on pull_request CH->>CH: classify changed files<br/>(code / docs / workflow) CH-->>CJ: code_changed=true CH-->>DS: docs_changed=true CH-->>AL: workflow_changed=true CJ-->>RP: result DS-->>RP: result AL-->>RP: result CH-->>RP: CHANGES_RESULT (fail-fast if not success) RP->>RP: evaluate gates<br/>exit 1 on any failure Note over RP: branch-protection required check PR->>AD: trigger (advisory, not required) AD->>AD: detect-dependency-change AD-->>AD: coverage / compat-matrix / dependency-audit<br/>(continue-on-error: true) PR->>PG: pull_request_target<br/>(opened/edited/labeled/sync) PG->>PG: fetch live labels via paginate PG->>PG: strip code blocks + HTML comments PG->>PG: validate template sections + checkboxes PG->>PG: detect risky paths (auth/request/storage) PG-->>PR: add/remove needs-live-verification label PG-->>PR: fail if template incomplete Note over PG: branch-protection required checkComments Outside Diff (4)
scripts/ci/docs-check.js, line 244-248 (link)Tilde-fenced code blocks not stripped before link extraction
extractMarkdownLinksonly strips triple-backtick fences but not~~~(tilde) fences, which are valid CommonMark code blocks. any link inside a tilde-fenced block is still extracted and validated:this would cause a false failure in
docs-sanityfor anyone using tilde fences. thestripMarkdownCodeinpr-governance.ymlcorrectly handles both fence characters — the same fix is needed here.|~~~)[\s\S]*?(?:```|~~~)/g, "\n")
.replace(/
[^\n]+`/g, "`code`");.github/workflows/pr-governance.yml, line 292-296 (link)core.summary.write()failure silences governance failuresawait core.summary.addRaw(...).write()is awaited before thecore.setFailedcall. if the GitHub step-summary API is transiently unavailable,write()throws, the exception propagates, andcore.setFailedis never reached. the caller sees a generic network error rather than the compliance failures accumulated infailures[], making governance silent exactly when it should be loudest.wrapping the summary write in a try/catch with a fallback
console.logpreserves governance signal even when the summary endpoint is down:scripts/ci/docs-check.js, line 243-247 (link)non-greedy fence pattern can prematurely close code blocks
the pattern
(?:\{3}|{3})[\s\S]*?(?:`{3}|{3})` is non-greedy and doesn't track fence-char identity. if a fenced code block's body contains a run of exactly 3 backticks (common in contributing/readme docs that demonstrate markdown syntax), the non-greedy match closes early at that interior sequence and everything after it — still inside the real block — is exposed to link extraction. a broken-link reference that appears after the interior triple-backtick will be extracted and reported as a false failure.example that triggers it:
the regex closes the outer block at the inner
```, so[guide](./missing.md)is extracted and validated.pr-governance.yml'sstripMarkdownCodealready handles this correctly with a line-by-line state machine that tracks fence char and length. the same approach should be used here.no test case in
docs-check.test.tscovers code blocks whose bodies contain 3 backtick/tilde sequences..github/workflows/pr-advisory.yml, line 104-108 (link)No failure-propagation guard for
detect-dependency-changeunlike
required-princi.yml(which now correctly fails fast whenCHANGES_RESULT != "success"), the advisory downstream jobs (coverage,compat-matrix,dependency-audit) only gate on the outputs, not on the job result. ifdetect-dependency-changeis cancelled or errors out, all three advisory jobs are silently skipped with no indication in the run summary.since these lanes are explicitly advisory this won't break the required gate, but it does mean a hung or crashed
detect-dependency-changejob produces a deceptively clean advisory run. consider adding aneeds.detect-dependency-change.result == 'success'guard to each advisoryif:condition, or a dedicated advisory summary step, so transient runner failures are visible.Prompt To Fix All With AI
Last reviewed commit: d4974b5