Conversation
📝 WalkthroughSeverity: Low-risk infrastructure change. This PR mirrors three release-blocker checks ( Test Coverage: A new Vitest test suite ( Workflow Changes:
Noted Parity Gap: The PR acknowledges that Validation Status: Lint, typecheck, and build checks passed; focused unit test execution confirmed the workflow-parity tests run successfully. Full test suite validation remains incomplete per PR checklist. Walkthroughworkflow configuration adds permissions, concurrency controls, and two new ci jobs ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes notes:
🚥 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 docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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 |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/ci-workflows.test.ts`:
- Around line 39-47: The test currently uses substring checks on strings ci and
prCi (from readWorkflow) which can falsely pass; update the test "keeps push CI
using the same stale-run cancellation as PR CI" to parse the workflow YAML into
an object (e.g., via readWorkflow returning parsed YAML or by passing the string
into a YAML parser) and assert the top-level concurrency block exists and
contains cancel-in-progress: true for both prCi and ci (check prCi.concurrency
and ci.concurrency and their cancel-in-progress property) instead of using
expect(...).toContain on raw strings so the assertion is scoped to the
workflow-level concurrency block.
- Around line 24-37: The test currently only checks that each command in
requiredCommands appears somewhere in ci and prCi strings; instead parse the
workflow YAML (using the existing readWorkflow result stored in ci and prCi) and
assert that there exists a single job whose steps (or run commands) contain all
three requiredCommands together. Locate and update the test block (the it("keeps
push CI aligned with the PR release harness checks", ...) that uses variables
ci, prCi and requiredCommands) to load the YAML for each workflow, iterate jobs,
and assert at least one job per workflow has all requiredCommands present in its
step run texts.
- Around line 7-21: The extractJobBlock function's boundary regex only matches
lowercase-hyphen IDs and Unix newlines, causing failures for valid job ids
(e.g., foo_bar) and CRLF files; update the nextMatch regex in extractJobBlock to
accept CRLF or LF and a broader job-id charset (e.g., /\r?\n
[A-Za-z0-9_-]+:\r?\n/) so it detects lines like " JOB_ID:" across platforms and
with underscores, and keep using the existing start/nextMatch math to compute
end and return the slice.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 67ecee1c-de05-4dbf-905b-0f67587ab142
📒 Files selected for processing (2)
.github/workflows/ci.ymltest/ci-workflows.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/ci-workflows.test.ts
🔇 Additional comments (4)
.github/workflows/ci.yml (3)
7-13: good hardening at workflow scope.top-level
permissions: contents: readand stale-run cancellation are correctly set for push ci. citation:lib/.github/workflows/ci.yml:7-13.
78-103: release harness job is correctly wired to release blockers.this job runs the exact three commands needed for push-side parity and uses pinned node 20.x setup consistently. citation:
lib/.github/workflows/ci.yml:78-103.
104-123: windows coverage for script typecheck is correctly added.the dedicated windows job is scoped and minimal, and it executes
npm run typecheck:scriptsas intended. citation:lib/.github/workflows/ci.yml:104-123.test/ci-workflows.test.ts (1)
49-59: windows parity intent is solid here.good use of per-job extraction before asserting
runs-onandtypecheck:scripts; this avoids the prior cross-job false positive class. citation:test/ci-workflows.test.ts:49-59.
| function readWorkflow(name: string): string { | ||
| return readFileSync(join(projectRoot, ".github", "workflows", name), "utf-8"); | ||
| } | ||
|
|
||
| function extractJobBlock(workflow: string, jobName: string): string { | ||
| const start = workflow.indexOf(` ${jobName}:`); | ||
| if (start === -1) { | ||
| throw new Error(`Missing workflow job: ${jobName}`); | ||
| } | ||
| const nextMatch = workflow | ||
| .slice(start + 1) | ||
| .match(/\n [a-z0-9][a-z0-9-]*:\n/); | ||
| const end = nextMatch ? start + 1 + nextMatch.index + 1 : workflow.length; | ||
| return workflow.slice(start, end); | ||
| } |
There was a problem hiding this comment.
make job-block extraction deterministic across windows and valid job ids.
line [18] only matches lowercase-hyphen ids and \n, so a valid job id like foo_bar or crlf input can break block boundaries and create false positives. citation: test/ci-workflows.test.ts:11-20.
proposed fix
function readWorkflow(name: string): string {
- return readFileSync(join(projectRoot, ".github", "workflows", name), "utf-8");
+ return readFileSync(join(projectRoot, ".github", "workflows", name), "utf-8").replace(/\r\n/g, "\n");
}
function extractJobBlock(workflow: string, jobName: string): string {
const start = workflow.indexOf(` ${jobName}:`);
if (start === -1) {
throw new Error(`Missing workflow job: ${jobName}`);
}
const nextMatch = workflow
.slice(start + 1)
- .match(/\n [a-z0-9][a-z0-9-]*:\n/);
+ .match(/\n [A-Za-z0-9_][A-Za-z0-9_-]*:\n/);
const end = nextMatch ? start + 1 + nextMatch.index + 1 : workflow.length;
return workflow.slice(start, end);
}as per coding guidelines, test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/ci-workflows.test.ts` around lines 7 - 21, The extractJobBlock
function's boundary regex only matches lowercase-hyphen IDs and Unix newlines,
causing failures for valid job ids (e.g., foo_bar) and CRLF files; update the
nextMatch regex in extractJobBlock to accept CRLF or LF and a broader job-id
charset (e.g., /\r?\n [A-Za-z0-9_-]+:\r?\n/) so it detects lines like "
JOB_ID:" across platforms and with underscores, and keep using the existing
start/nextMatch math to compute end and return the slice.
| it("keeps push CI aligned with the PR release harness checks", () => { | ||
| const ci = readWorkflow("ci.yml"); | ||
| const prCi = readWorkflow("pr-ci.yml"); | ||
| const requiredCommands = [ | ||
| "npm run typecheck:scripts", | ||
| "npm run pack:check", | ||
| "npm run vendor:verify", | ||
| ]; | ||
|
|
||
| for (const command of requiredCommands) { | ||
| expect(prCi).toContain(command); | ||
| expect(ci).toContain(command); | ||
| } | ||
| }); |
There was a problem hiding this comment.
assert release-blocker commands are co-located in the intended job block.
current checks only prove each command exists somewhere in each workflow, not that one release-harness-style job contains all three. this can miss parity regressions. citation: test/ci-workflows.test.ts:24-37.
proposed fix
it("keeps push CI aligned with the PR release harness checks", () => {
const ci = readWorkflow("ci.yml");
const prCi = readWorkflow("pr-ci.yml");
+ const ciReleaseHarness = extractJobBlock(ci, "release-harness");
+ const prValidate = extractJobBlock(prCi, "validate");
const requiredCommands = [
"npm run typecheck:scripts",
"npm run pack:check",
"npm run vendor:verify",
];
for (const command of requiredCommands) {
- expect(prCi).toContain(command);
- expect(ci).toContain(command);
+ expect(ciReleaseHarness).toContain(command);
+ expect(prValidate).toContain(command);
}
});as per coding guidelines, test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/ci-workflows.test.ts` around lines 24 - 37, The test currently only
checks that each command in requiredCommands appears somewhere in ci and prCi
strings; instead parse the workflow YAML (using the existing readWorkflow result
stored in ci and prCi) and assert that there exists a single job whose steps (or
run commands) contain all three requiredCommands together. Locate and update the
test block (the it("keeps push CI aligned with the PR release harness checks",
...) that uses variables ci, prCi and requiredCommands) to load the YAML for
each workflow, iterate jobs, and assert at least one job per workflow has all
requiredCommands present in its step run texts.
| it("keeps push CI using the same stale-run cancellation as PR CI", () => { | ||
| const ci = readWorkflow("ci.yml"); | ||
| const prCi = readWorkflow("pr-ci.yml"); | ||
|
|
||
| expect(prCi).toContain("concurrency:"); | ||
| expect(prCi).toContain("cancel-in-progress: true"); | ||
| expect(ci).toContain("concurrency:"); | ||
| expect(ci).toContain("cancel-in-progress: true"); | ||
| }); |
There was a problem hiding this comment.
scope the concurrency assertion to the top-level block.
line [43]-line [46] use global substring checks, so the test can pass even if cancel-in-progress: true is moved out of the workflow-level concurrency section. citation: test/ci-workflows.test.ts:39-47.
as per coding guidelines, test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/ci-workflows.test.ts` around lines 39 - 47, The test currently uses
substring checks on strings ci and prCi (from readWorkflow) which can falsely
pass; update the test "keeps push CI using the same stale-run cancellation as PR
CI" to parse the workflow YAML into an object (e.g., via readWorkflow returning
parsed YAML or by passing the string into a YAML parser) and assert the
top-level concurrency block exists and contains cancel-in-progress: true for
both prCi and ci (check prCi.concurrency and ci.concurrency and their
cancel-in-progress property) instead of using expect(...).toContain on raw
strings so the assertion is scoped to the workflow-level concurrency block.
mirror release harness checks on push CI
1 participant
Summary
What Changed
release-harnessjob toci.ymlfortypecheck:scripts,pack:check, andvendor:verifytest/ci-workflows.test.tsso future workflow edits have to keep those commands in bothci.ymlandpr-ci.ymlValidation
npm run lintnpm run typechecknpm testnpm test -- test/documentation.test.tsnpm run buildnpx vitest run test/ci-workflows.test.tsDocs and Governance Checklist
docs/getting-started.mdupdated (if onboarding flow changed)docs/features.mdupdated (if capability surface changed)docs/reference/*pages updated (if commands/settings/paths changed)docs/upgrade.mdupdated (if migration behavior changed)SECURITY.mdandCONTRIBUTING.mdreviewed for alignmentRisk and Rollback
5c9efedAdditional Notes
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 mirrors the three release-blocker checks (
typecheck:scripts,pack:check,vendor:verify) frompr-ci.ymlinto push CI by adding a dedicatedrelease-harnessjob and ascripts-windowsjob toci.yml, plus a test file that enforces parity between the two workflow files going forward.ci.ymlgainspermissions: contents: read, aconcurrency: cancel-in-progress: trueblock (scoped toci-${{ github.ref }}so rapid pushes to main cancel stale runs),release-harness(ubuntu, node 20), andscripts-windows(windows-latest, node 20) — all mirroring existingpr-ci.ymljobstest/ci-workflows.test.tsadds three vitest cases: command parity, concurrency-block parity, and windows-job parity; uses a localextractJobBlockhelper to slice job boundaries by 2-space-indented yaml keysextractJobBlockregex[a-z0-9][a-z0-9-]*only matches lowercase job names — addingA-Zor theiflag prevents silent over-extraction if a camelCase job is ever addedtoContainrather than scoped job-block assertions, so a command string surviving in a disabled or conditional job would still satisfy the testreadFileSynchere is read-only and path-joined vianode:path, so noEBUSY/EPERMexposurenpm testwas left unchecked in the pr checklist; the focusednpx vitest run test/ci-workflows.test.tsrun covers the new file but the full suite hasn't been confirmed green on this branchConfidence Score: 5/5
safe to merge — changes are additive CI jobs and a test file with no production code impact
all findings are P2: a lowercase-only regex in a test helper and a whole-file containment assertion that could be scoped more tightly. neither affects current correctness — all three commands are present in the correct jobs and the tests will pass as-is.
test/ci-workflows.test.ts — the
extractJobBlockregex and containment-scope issue are worth tightening before this test file accrues further parity checksImportant Files Changed
permissions: contents: readandconcurrency: cancel-in-progress: true, plusrelease-harnessandscripts-windowsjobs that mirror the three release-blocker checks already inpr-ci.yml; no logic issues foundextractJobBlockregex only handles lowercase job names (P2), and the first test uses whole-filetoContainrather than scoped job-block assertions, making it easier to accidentally satisfy with orphaned command stringsFlowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD push["push to main"] --> wf["ci.yml workflow"] wf --> test["test (Node 20.x / 22.x matrix)\ntypecheck + coverage + build"] wf --> lint["lint\nnpm run lint"] wf --> rh["release-harness (NEW)\ntypecheck:scripts\npack:check\nvendor:verify"] wf --> sw["scripts-windows (NEW)\ntypecheck:scripts\n(windows-latest)"] wf --> cc["codex-compat\nsmoke tests"] style rh fill:#d4edda,stroke:#28a745 style sw fill:#d4edda,stroke:#28a745Prompt To Fix All With AI
Reviews (3): Last reviewed commit: "ci: tighten push workflow parity" | Re-trigger Greptile