Repository navigation
chore(workflows): migrate release/labeling bash logic to Node scripts - #456
Merged
Merged
Conversation
Contributor
|
Warning Review limit reached
More reviews will be available in 52 minutes and 45 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (15)
✨ Finishing Touches🧪 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2284245254
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| env: | ||
| GITHUB_EVENT_NAME: ${{ github.event_name }} | ||
| GITHUB_ACTOR: ${{ github.actor }} | ||
| run: node scripts/workflows/release/trigger-telemetry.cjs |
There was a problem hiding this comment.
In the trigger-telemetry job I inspected, there is no actions/checkout step before this run, so a fresh GitHub Actions runner will not have scripts/workflows/release/trigger-telemetry.cjs in the workspace. The previous inline bash did not need repository files, but this migration makes the first job fail with Cannot find module, which prevents the downstream lint and release jobs from running.
Useful? React with 👍 / 👎.
Comment on lines
+27
to
+29
| return execGit( | ||
| `git log ${rangeStart}..develop --first-parent --pretty=format:"- %h %s"`, | ||
| ); |
There was a problem hiding this comment.
When a dry-run release is dispatched with a notes_from value, that input is interpolated directly into an execSync shell command here. Values such as v1.0.0; <command> are therefore executed by the shell in the release job context, whereas the old bash path quoted the range; use execFileSync/spawnSync with argument arrays or otherwise validate the ref before invoking git.
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request transitions custom workflow logic from Bash to Node.js scripts, introducing a workflow validator with guardrails to enforce the new policy, along with several release-related Node scripts, a shared runtime helper, and updated documentation. The review feedback highlights several critical improvements: addressing a potential command injection vulnerability in build-notes-preview.cjs by replacing execSync with execFileSync, ensuring temporary directories in tests are properly cleaned up, broadening the workflow validator's regex to catch all if statements, and hardening the shared runtime helper to support multiline outputs and preserve error stack traces for easier debugging.
Comment on lines
+8
to
+39
| const { execSync } = require("child_process"); | ||
| const { readEnv, log, runMain } = require("../shared/runtime.cjs"); | ||
|
|
||
| function execGit(command, allowError = false) { | ||
| try { | ||
| return execSync(command, { | ||
| encoding: "utf8", | ||
| stdio: ["ignore", "pipe", "pipe"], | ||
| }).trim(); | ||
| } catch (error) { | ||
| if (allowError) { | ||
| return ""; | ||
| } | ||
| throw new Error(`Git command failed: ${command}\n${error.message}`); | ||
| } | ||
| } | ||
|
|
||
| function buildGitLogContent(rangeStart) { | ||
| if (rangeStart) { | ||
| return execGit( | ||
| `git log ${rangeStart}..develop --first-parent --pretty=format:"- %h %s"`, | ||
| ); | ||
| } | ||
|
|
||
| return execGit('git log develop --first-parent --pretty=format:"- %h %s"'); | ||
| } | ||
|
|
||
| async function main() { | ||
| let rangeStart = readEnv("INPUT_NOTES_FROM", { defaultValue: "" }).trim(); | ||
| if (!rangeStart) { | ||
| rangeStart = execGit("git describe --tags --abbrev=0", true); | ||
| } |
Contributor
There was a problem hiding this comment.
Using execSync with a shell-interpolated string containing rangeStart (which is sourced from the user-controlled INPUT_NOTES_FROM environment variable) introduces a Command Injection vulnerability. To eliminate this risk, use execFileSync to execute git directly with an array of arguments, bypassing the shell entirely.
const { execFileSync } = require("child_process");
const { readEnv, log, runMain } = require("../shared/runtime.cjs");
function execGit(args, allowError = false) {
try {
return execFileSync("git", args, {
encoding: "utf8",
stdio: ["ignore", "pipe", "pipe"],
}).trim();
} catch (error) {
if (allowError) {
return "";
}
throw new Error("Git command failed: git " + args.join(" ") + "\n" + error.message);
}
}
function buildGitLogContent(rangeStart) {
if (rangeStart) {
return execGit([
"log",
rangeStart + "..develop",
"--first-parent",
"--pretty=format:- %h %s",
]);
}
return execGit([
"log",
"develop",
"--first-parent",
"--pretty=format:- %h %s",
]);
}
async function main() {
let rangeStart = readEnv("INPUT_NOTES_FROM", { defaultValue: "" }).trim();
if (!rangeStart) {
rangeStart = execGit(["describe", "--tags", "--abbrev=0"], true);
}
Comment on lines
+21
to
+47
| test("trigger-telemetry writes expected GITHUB_OUTPUT and telemetry payload", () => { | ||
| const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), "wf-telemetry-")); | ||
| const outputPath = path.join(tempDir, "github_output.txt"); | ||
|
|
||
| execFileSync(process.execPath, [runtimeScript], { | ||
| cwd: tempDir, | ||
| env: { | ||
| ...process.env, | ||
| GITHUB_OUTPUT: outputPath, | ||
| GITHUB_EVENT_NAME: "workflow_dispatch", | ||
| GITHUB_ACTOR: "ash", | ||
| }, | ||
| encoding: "utf8", | ||
| }); | ||
|
|
||
| const outputContent = fs.readFileSync(outputPath, "utf8"); | ||
| const telemetry = JSON.parse( | ||
| fs.readFileSync(path.join(tempDir, "trigger-telemetry.json"), "utf8"), | ||
| ); | ||
|
|
||
| expect(outputContent).toContain("unauthorized_attempts=0"); | ||
| expect(telemetry).toEqual({ | ||
| event: "workflow_dispatch", | ||
| actor: "ash", | ||
| unauthorized_attempts: 0, | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
The test creates a temporary directory using fs.mkdtempSync but never cleans it up. This leaves leftover files and directories on the host machine or CI runner. Wrap the test logic in a try...finally block to ensure the temporary directory is recursively deleted using fs.rmSync after the test completes. (Note: The same cleanup pattern should be applied to the other tests in this file that use fs.mkdtempSync.)
test("trigger-telemetry writes expected GITHUB_OUTPUT and telemetry payload", () => {
const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), "wf-telemetry-"));
try {
const outputPath = path.join(tempDir, "github_output.txt");
execFileSync(process.execPath, [runtimeScript], {
cwd: tempDir,
env: {
...process.env,
GITHUB_OUTPUT: outputPath,
GITHUB_EVENT_NAME: "workflow_dispatch",
GITHUB_ACTOR: "ash",
},
encoding: "utf8",
});
const outputContent = fs.readFileSync(outputPath, "utf8");
const telemetry = JSON.parse(
fs.readFileSync(path.join(tempDir, "trigger-telemetry.json"), "utf8"),
);
expect(outputContent).toContain("unauthorized_attempts=0");
expect(telemetry).toEqual({
event: "workflow_dispatch",
actor: "ash",
unauthorized_attempts: 0,
});
} finally {
fs.rmSync(tempDir, { recursive: true, force: true });
}
});
Comment on lines
+406
to
+409
| const hasControlFlow = | ||
| /(^|\n)\s*(if\s+\[|if\s+\[\[|case\s+|for\s+|while\s+|until\s+|select\s+)/m.test( | ||
| runScript, | ||
| ); |
Contributor
There was a problem hiding this comment.
The regex if\s+\[|if\s+\[\[ only matches if statements that use brackets (e.g., if [ ... or if [[ ...). However, bash if statements can execute any command directly without brackets (e.g., if git diff-index ...; then). Simplify the if match to if\s+ to catch all if control flows.
Suggested change
| const hasControlFlow = | |
| /(^|\n)\s*(if\s+\[|if\s+\[\[|case\s+|for\s+|while\s+|until\s+|select\s+)/m.test( | |
| runScript, | |
| ); | |
| const hasControlFlow = | |
| /(^|\n)\s*(if\s+|case\s+|for\s+|while\s+|until\s+|select\s+)/m.test( | |
| runScript, | |
| ); |
Comment on lines
+42
to
+48
| function writeGithubOutput(key, value, outputPath = process.env.GITHUB_OUTPUT) { | ||
| if (!outputPath) { | ||
| throw new Error("GITHUB_OUTPUT is not set"); | ||
| } | ||
|
|
||
| fs.appendFileSync(outputPath, `${key}=${value}\n`, "utf8"); | ||
| } |
Contributor
There was a problem hiding this comment.
writeGithubOutput writes key-value pairs directly as key=value. If value contains newlines, GitHub Actions will fail to parse it correctly or throw an error. Implement the standard GitHub Actions multiline output syntax using a random delimiter when the value contains newlines.
function writeGithubOutput(key, value, outputPath = process.env.GITHUB_OUTPUT) {
if (!outputPath) {
throw new Error("GITHUB_OUTPUT is not set");
}
const strValue = String(value);
if (strValue.includes("\n")) {
const delimiter = "EOF_" + Math.random().toString(36).substring(2);
fs.appendFileSync(outputPath, key + "<<" + delimiter + "\n" + strValue + "\n" + delimiter + "\n", "utf8");
} else {
fs.appendFileSync(outputPath, key + "=" + strValue + "\n", "utf8");
}
}
Comment on lines
+64
to
+71
| async function runMain(mainFn) { | ||
| try { | ||
| await mainFn(); | ||
| } catch (error) { | ||
| console.error(error.message); | ||
| process.exit(1); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
runMain only logs error.message when an exception is caught. This hides the stack trace, making it very difficult to debug unexpected runtime errors (like TypeError or ReferenceError) in CI workflows. Log error.stack or the full error object to provide complete debugging context.
async function runMain(mainFn) {
try {
await mainFn();
} catch (error) {
console.error(error.stack || error);
process.exit(1);
}
}
github-actions
Bot
added
migrate:area:ci
github-actions
Bot
added
the
migrate:meta:needs-changelog
Contributor
🔍 Reviewer Summary for PR #456CI Status: ✅ Recommendations
|
Contributor
🔍 Reviewer Summary for PR #456CI Status: ✅ Recommendations
|
Contributor
🔍 Reviewer Summary for PR #456CI Status: ✅ Recommendations
|
16 tasks
This was referenced Aug 18, 2026
6 tasks done
eleshar
added a commit
that referenced
this pull request
Sep 26, 2026
Each finding was checked against the code before changing anything. Two were inconsistencies I had introduced or missed. The link rules still promised a bare `PR-NNN` in the business-rule table and in research.md after the previous commit narrowed the field description to `#123`, and the claim that a full markdown URL makes a reference machine-checkable was itself wrong: neither /#(\d+)/ nor /issues\/#(\d+)/ matches a /pull/456 path, so such a link satisfies only the non-empty-target check. The rule, the field description, the research summary and both error examples now agree that only `#123` and `issues/#123` are resolved. Separately, the branch-prefix bypass survived in research.md's pseudocode and in three places in the requirements checklist; the pseudocode now mirrors changelog-unified.yml (bot author, docs-only diff, or an explicit meta:no-changelog label) and the branch name is never consulted. The recovery mutex had no recovery path: a process exiting while holding it would leave every later contender unable to enter the protocol, so the abandoned lock could never be removed. It now requires an OS-backed lock and the portable stat-and-compare fallback is not used where no such primitive exists. Three quickstart checks could pass without proving anything. The missing-file test used /nonexistent/file.md, which the plan rejects as an absolute path before any read, so it never exercised missing-file handling; CHANGELOG.missing.md does, and returns exit 1 with "not found". The documentation greps were chained with &&, letting an early failure be masked by a later success; they now collect failures and exit nonzero, verified by removing one needle and confirming exit 1. The workflow scenario accepted a precomputed CHANGELOG_CHANGED flag without exercising any documented skip condition; it now classifies each of bot author, docs-only, mixed diff, meta:no-changelog and empty diff, verified across eight cases. Also: the skill name must match its containing directory per the Agent Skills specification; the changelog entry references #3500 rather than the linked issue #3465; and FEEDBACK_RESPONSE.md no longer implies rejected feedback was deferred.
eleshar
added a commit
that referenced
this pull request
Sep 26, 2026
…ation specs (#3500) * docs(specs): land CI remediation, changelog agent and agent consolidation specs (#3465) Specs only, so the team can start per-agent spec work from develop: - 017-ci-failure-remediation from audit/governance-audit-implementation, renumbered from 015 (015 is pr-agent-consolidation, 016 is taken); the quoted original request keeps its 015. - 016-changelog-agent-quality and the 014-agents-restructure-consolidate tasks.md and registry-schema.json updates from #3434, unchanged. - CATALOG.md rows for 014-017. * docs(specs): sync 014 and 016 with the agreed #3434 head (b29c26b) #3500 had copied an older revision. Picks up the #3470 decisions: the REST changelog_path/changelog_content table, merged-only PR links, the changelog-unified.yml workflow name and the corrected checklist. * docs(specs): fix five consistency findings in specs 016 and 017 CodeRabbit full review of #3500, tracked in #3519: - 016 checklist: "no implementation details" was both failed and passed, and the summary said all items pass. Recorded it as an accepted exception. - 016 check-links: an unmerged link fails only with --strict, matching the default and the JSON example; the exit-code table now says so. - 016 quickstart: the closing frontmatter delimiter must be a whole line (rejects ---oops; accepts CRLF and end of file). - 016 locking: stale-lock recovery is an atomic compare-and-remove (rename to a per-contender tombstone, verify inode and token, restore with link() if a live lock was moved) plus a token fence before every write. A path-based unlink after re-stat could delete a new owner's lock. - 017 checklist: only User Story 1 was verified; Stories 2-4 are marked incomplete, as the spec's resolution note says. The 016 changes are applied identically to #3434. * docs(specs): address the second CodeRabbit review on #3500 - 014 registry schema: generatedSkill uses the documented model fields (location, agentskills_compliant, compliance_violations, used_by). - 014 tasks: US4 (T058-T069) unchecked; none of its deliverables exist. - 016 CLI contract: document the validator's real JSON error payloads and its full --help output. - 016 data model: lifecycle branches after validation; sample character_count is 86. - 016 quickstart: required-field check uses jq -e with type checks; the documentation check exits non-zero when a file is missing. - 016 spec: validation gate lives in changelog-unified.yml. - 017 checklist: replace the stale /speckit-plan next step. - Catalog: move resolved 017 to a Resolved section. Validator schema enforcement: #3522. Other 014 tasks: #3523. * docs(specs): address the third CodeRabbit review on #3500 - 014 schema: version pattern is full SemVer 2.0.0 (pre-release, build). - 016 data model: ValidationResult records skill_id with skill_version. - 016 quickstart: assert the four planned skill directories; sample output uses their real names. - 016 plan: list the shipped changelog-unified.yml workflow. - 016 spec: Scenario 4 invokes skills through the npm CLI (Phase 1). - 017 data model: terminal state includes GOVERNANCE WORKFLOW. - 017 plan: name the real branch, audit/governance-audit-implementation (#3367); 017-ci-failure-remediation never existed. - Catalog: next-number steps no longer say 013/014. * docs(specs): use the computed number in the catalogue's create-directory step * docs(specs): address the fourth CodeRabbit review on #3500 - 014 schema: the three registry variants exclude each other's defining fields (agents, schema, category), so oneOf matches exactly one. Tested with ajv: each valid variant passes; a category registry carrying the consolidated schema URL is rejected. - 014 tasks: uncheck T033, T034, T036, T038-T040, T042, T044-T047 and T050-T057; their deliverables are absent (the agent template has 4 of its 7 components; the skills/ category folders do not exist). See #3523. - 016 data model: release_date is typed date | "Unreleased"; both entry lifecycles go VALID -> READY_FOR_RELEASE -> MERGED. * docs(specs): close review contract gaps * docs: align the 016 error object and scope the merged-PR check (#3500) Two consistency defects in the changelog-agent quality spec. The error shape was spelled three different ways across the spec set: data-model.md defined error_code, expected_format and actual_value, spec.md used error_type for the same field, and research.md's example used type, actual, expected and a second current_value. data-model.md also called the entity ValidationError in one relationship and ErrorObject in the other two. data-model.md's field table is the contract, so ErrorObject with error_code, expected_format, actual_value, suggestion and severity is now the single shape, and the two examples and the spec prose are aligned to it, including the severity and suggestion fields the examples were missing. The validation rules required every pr_issues reference to be a merged pull request, but pr_issues also carries issue references and an issue has no merged state, so a valid issue reference could never satisfy the rule. The rule and the constraint row now apply the merged check to pull-request references only and state that issue references must merely exist. * docs: finish aligning the 016 error object and sync with develop (#3500) CodeRabbit's two findings on this pull request were addressed in 23cf746, but the error-object contract was only partly aligned: spec.md still summarised the Error Object with `location` and `fix_suggestion` and omitted entry_id, severity and field, and FR-005 described the report in prose that an implementer could read as licence to reintroduce error_type and fix_suggestion. Point both at the canonical field set in data-model.md, so every definition, requirement and example in the specification now names the same fields. The merged-state finding needed no further work: data-model.md already scopes the merged requirement to pull-request references and exempts issues, which have no merged state. Also merge origin/develop, keeping both additive CHANGELOG entries. * docs: correct three 016 spec contradictions against the shipped gate (#3500) A Qodo review of the head found three spec statements that the shipped code contradicts. Each was checked against the code that actually runs. The bypass mechanism was described as automatic for chore/ and deps/ branches. changelog-unified.yml has no such bypass: it skips Dependabot and docs-bot authors, docs-only diffs (every changed file under docs/** or ending in .md), and the meta:no-changelog label, which it refuses for high-impact change types. A chore/ branch with a code diff therefore still needs a changelog entry or the label, so FR-009, Q1, research and the decision table now state the shipped behaviour and say explicitly that branch prefix is not a bypass. The spec required labels meta:has-changelog, meta:needs-changelog-fix and meta:changelog-exempt. None exist; the canonical set has only meta:needs-changelog and meta:no-changelog. Because labels.yml is a locked file this fixes the spec rather than adding labels: passing validation clears meta:needs-changelog, and failing keeps it applied. The label families, the User Story 4 acceptance criteria and the quickstart commands now use the two real names. pr_issues accepted "#123" or "PR-456", but the engine matches only /#(\d+)/ and /issues\/#(\d+)/, so a PR-456 reference matches neither and is counted as no link at all instead of a valid one. The data model now records #123 as the only machine-validated form, keeps PR-456 as a human-readable convention, and says that a full markdown URL is what makes it checkable. * docs: fix four more 016/017 contradictions found by review (#3500) A forced full review of the head surfaced four more statements that the shipped code contradicts; each was checked against the code that runs. spec.md's acceptance scenario promised the tool accepts "#123 or PR-456", but the engine matches only /#(\d+)/, so a PR-456 entry is still reported missing. The scenario now states #123 as the machine-validated format and says a bare PR-456 is not recognised. FR-007 said only "labels from the canonical set with prefix meta:" and never named one, which is how the spec ended up requiring three labels that do not exist. It now names the two that do, meta:needs-changelog and meta:no-changelog, and forbids any other changelog label. The 017 quickstart told readers to run node .github/validation/changelog/validator.js, which does not exist; the entry point is bin/validate.js and it needs --changelog-path. The corrected command was executed and runs. The 017 comment template shipped PR #3367's measurements as if they were reusable: 29 failures, 48/54 entries, HIGH confidence. A template that carries frozen numbers invites a reviewer to act on a stale count, so the figures are now {{placeholders}} under a warning that every number must be recomputed. * fix: close the registry schema gap and the last 017 quickstart/template claims (#3500) A full review of the head found the 014 registry schema accepted invalid skill metadata. No object schema set additionalProperties: false, so every undeclared or misspelled field validated: a skill whose compliance_violations was typed as complianceViolations, or whose agentskills_compliant was misspelled, passed cleanly and would have produced a wrong compliance report. Reproduced all five cases before fixing. Added additionalProperties: false to the eight object schemas and confirmed three well-formed registry shapes still validate while the misspelled and junk-key cases are now rejected. The 017 quickstart also runs two npm scripts that do not exist: validate:mermaid and validate:agent-spec are absent from package.json, so those steps fail immediately. Each is now marked as not yet defined, and the summary block distinguishes the defined scripts from the missing ones. The 017 comment template still carried PR #3367's numbers in its body -- 48/54 entries, 88.9%, 6/54 compliant, "within 48 hours", Q4 2026 -- below the warning added earlier. A warning does not stop anyone copying a stale count, so all 15 figures are now {{placeholders}}. * docs: add AI feedback response for #3500 (#3500) The ai-feedback-validation workflow reported that the pull request had no issue link, so this records the review feedback with a status per item as that workflow requires. Linkage: the pull request now closes #3465, which is the rebase of #3434 onto develop and the resolution of the spec 014/016 conflicts -- exactly what this pull request delivers. #3464 and #3434 stay as "Relates to" because neither is completed here. Tracked: eleven items are marked addressed, each with the commit that fixed it. Two are deferred to existing open issues rather than dropped: the repeat prose findings in specs 016 and 017 go to #3519, and the stricter 014 registry schema not being enforced at runtime goes to #3522, since the validator does not read the loaded schema and that is a code change outside a documentation-only pull request. Verified by running scripts/validation/ai-feedback-helpers.cjs against this file and the updated pull request body: passed, with no invalid status markers and no deferred item lacking an issue reference. * fix: placeholder the 017 comment template conclusions, not just its figures (#3500) Qodo was right that the template was only half fixed. The measurements became placeholders, but the body still asserted the conclusions: "Audit governance implementation is clean", "Classification confidence: HIGH", "Ready for merge", a fixed six-category count, an approval section stating all conditions were met, and instructions written for PR #3367 specifically. A template that asserts those verdicts will tell a reviewer that a different pull request is approved when it is not, and a stale verdict is harder to notice than a stale number because it reads as an assessment rather than a measurement. The verdicts, confidence level, merge readiness, approval section and category count are now placeholders as well, taking the body to 25, and the warning states that conclusions must be derived from evidence for the pull request under review. Only the historical "Usage Context" line still names PR #3367, which is correct: it documents where the example came from and is outside the reusable body. * docs: record the rejected claim that validate:changelog is missing (#3500) Qodo reported at High severity that the 017 quickstart runs a root script the package does not define, so the documented command fails before validation runs. It is defined, at package.json line 103, and running it from the root exits 0 with real output. The rejection is recorded in FEEDBACK_RESPONSE.md with that evidence so the decision is auditable rather than invisible. * docs: address the nine open CodeRabbit findings on #3500 Each finding was checked against the code before changing anything. Two were inconsistencies I had introduced or missed. The link rules still promised a bare `PR-NNN` in the business-rule table and in research.md after the previous commit narrowed the field description to `#123`, and the claim that a full markdown URL makes a reference machine-checkable was itself wrong: neither /#(\d+)/ nor /issues\/#(\d+)/ matches a /pull/456 path, so such a link satisfies only the non-empty-target check. The rule, the field description, the research summary and both error examples now agree that only `#123` and `issues/#123` are resolved. Separately, the branch-prefix bypass survived in research.md's pseudocode and in three places in the requirements checklist; the pseudocode now mirrors changelog-unified.yml (bot author, docs-only diff, or an explicit meta:no-changelog label) and the branch name is never consulted. The recovery mutex had no recovery path: a process exiting while holding it would leave every later contender unable to enter the protocol, so the abandoned lock could never be removed. It now requires an OS-backed lock and the portable stat-and-compare fallback is not used where no such primitive exists. Three quickstart checks could pass without proving anything. The missing-file test used /nonexistent/file.md, which the plan rejects as an absolute path before any read, so it never exercised missing-file handling; CHANGELOG.missing.md does, and returns exit 1 with "not found". The documentation greps were chained with &&, letting an early failure be masked by a later success; they now collect failures and exit nonzero, verified by removing one needle and confirming exit 1. The workflow scenario accepted a precomputed CHANGELOG_CHANGED flag without exercising any documented skip condition; it now classifies each of bot author, docs-only, mixed diff, meta:no-changelog and empty diff, verified across eight cases. Also: the skill name must match its containing directory per the Agent Skills specification; the changelog entry references #3500 rather than the linked issue #3465; and FEEDBACK_RESPONSE.md no longer implies rejected feedback was deferred. * docs: address the ten findings from the CodeRabbit CLI review (#3500) Ran `coderabbit review --agent --base develop` as the review comments suggest. It reviewed 19 files and returned ten findings, nine major. Each was checked against the code before changing anything; all ten held. Two of my own earlier fixes were wrong. The recovery-mutex rule I added previously listed an `O_CREAT | O_EXCL` lock file as an OS-released primitive. It is not: the file survives process exit, so using it would cause exactly the deadlock the rule exists to prevent. The rule now names only `flock`/`fcntl` and Windows named mutexes or kernel semaphores, and requires the operation to be refused outright where no such primitive exists. The example in the entry model also used `PR-3373`, which the link rules do not resolve. The classification guidance contradicted itself across two files. data-model.md called a differing error between branches "likely infrastructure"; the quickstart matrix called it audit-related; the quickstart flow called it ambiguous. All three now read ambiguous, requiring per-failure comparison, which is the only defensible reading. FR-008 and FR-009 overlapped on a changelog-only pull request: the shipped docs-only test accepts any file ending in `.md`, and `CHANGELOG.md` ends in `.md`, so the one pull request that most needs its entries validated is exempt. FR-009 keeps the shipped behaviour and now says so explicitly and names the gap, rather than leaving two requirements in silent conflict. The 017 quickstart compared against the specification branch while attributing the failures to PR #3367, whose branch is `audit/governance-audit-implementation`; it now checks out that branch. It also used the root `validate:changelog` script to establish a 6/54 entry baseline, but that script runs the repository-wide safety audit, which reports no per-entry results; the feature-016 engine is used instead. The undefined `validate:mermaid` and `validate:agent-spec` commands are no longer presented as evidence producers: each is guarded and records UNVERIFIED when absent, and pipefail stops `tee` masking a missing script. The reusable comment template still declared classifications in its section headings and inline "pre-exists on develop" notes, so replacing the numeric placeholders could not have corrected them. Those are placeholders now, 6 headings and 6 inline claims. The 017 plan presented six categories as classified evidence while spec.md records that only User Story 1 was verified and the failures no longer occur; the plan now carries a banner saying it is a historical record. The 016 workflow scenario ended its failure branch with a successful echo, so a blocked pull request still exited 0; it now exits nonzero. * docs: record the CodeRabbit CLI review passes on #3500 Ran the review the comments suggest, following the documented agent workflow at docs.coderabbit.ai/cli/overview: coderabbit review --agent --base develop, then a second pass as step 4 recommends. Pass 1 returned 10 findings across 19 files and all 10 were addressed. Pass 2 returned 10 more. The loop stops after two passes, as that guidance requires. The remaining findings concern the prose of specifications 016 and 017, which describe a system that has not been built, so there is no implementation to verify a claim against and each pass raises further hypotheticals. Recorded rather than chased. Two are called out as independently actionable: the CHANGELOG.md docs-only bypass in FR-009, which needs a change to changelog-unified.yml rather than to this documentation, and the generated performance results fixture. * docs: route the CodeRabbit pass-2 findings to their existing trackers (#3500) The pass-2 section claimed the remaining findings had no tracker and that the loop simply stopped. That was wrong: #3519 already exists to hold spec 016 and 017 design findings from this very pull request, and it already listed most of them. Mapped the ten findings against the open trackers. Five were duplicates of existing #3519 items, re-raised against the corrected prose: inconclusive classifications, category verdicts in the comment template, the wrong comparison branch, losing the file type when linting, and posting the comment without re-verification. The generated performance fixture is already covered by #3498 and PR #3499. Four were genuinely new and are now appended to #3519 as checklist items, taking it from 13 to 17: excluding CHANGELOG.md from the docs-only bypass, limiting automatic labelling to non-exemption labels, running one validator on both branches, and testing the shipped workflow rather than a local simulation. * docs: fix the MD022 failure the specification lint caught (#3500) The rewritten pass-2 paragraph began a line with '#3519 and #3470 ...', which markdownlint parses as an ATX heading, so MD022 fired and Specification Validation failed. Reflowed so no line starts with a hash, and turned the bare issue references into links now that they name the trackers the findings were routed to. Also corrected the stale lead-in, which still said two findings were acted on here: they are checklist items in #3519 and #3498. Verified with markdownlint over the exact 18-file set the workflow lints. An earlier local run covered only three files, which is why this reached CI. * fix: address the four open Qodo findings on #3500 (#3500) Three of the four were defects I had introduced, and one of those I had previously "fixed" in the wrong direction. The baseline comparison read a file nothing writes: the develop step wrote /tmp/develop-changelog-results.json while the count and the cross-branch diff read .txt. The grep pattern could not have matched JSON either. Both branches now run the same engine with --output text and write matching filenames, so the count and the diff operate on the file that exists. This also completes the "run the same entry validator on both branches" item in #3519. I had marked validate:mermaid and validate:agent-spec as UNVERIFIED because those exact script names are absent. That was the wrong response: the repository exposes validate:mermaid-syntax and validate:agents, both of which run. The guards recorded a false gap where a working validator already existed, so all three now call the real scripts, with pipefail preserving a nonzero exit -- which for validate:agents is a genuine finding rather than a missing command. The registry schema still accepted malformed skill metadata. generatedSkill constrained id, category and type to minLength 1 only, so Bad_ID, a category of "Not A Category!" and even ../etc/passwd all validated. id and category now require the same ^[a-z0-9-]+$ slug that legacySkill already uses and type takes the same action/query/transform/utility enum; a valid document still validates. The comment template's fourth category asserted a tentative ENVIRONMENTAL verdict and that the pull request was not blocked, while the template's own rules forbid posting a category that is still unclassified. The verdict and both of its consequences are placeholders now, and the rules state the constraint explicitly: an unclassified category is omitted, or the whole comment is withheld. * docs: record the Qodo pass on the current head (#3500) Qodo resolved its four threads on the push rather than leaving them to close individually, so the reasoning is recorded here instead. Three of the four were defects this pull request had introduced, and one of those had been fixed in the wrong direction: validate:mermaid and validate:agent-spec are genuinely absent, but the repository does expose validate:mermaid-syntax and validate:agents, so wrapping the missing names in UNVERIFIED guards recorded a false gap rather than correcting the call. Noted that the schema tightening has no runtime effect until #3522 lands via PR #3550, and that what the inconclusive-classification standard should be stays with the spec owner in #3519. --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
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.
See commit 2284245 for full change-set including workflow migration and included project archival updates.