ci(ts-migration): guard migrated legacy paths - #1683
Conversation
📝 WalkthroughWalkthroughThis PR adds a GitHub Actions workflow with supporting scripts to prevent modifications to legacy migrated code paths. The workflow automatically runs on pull requests targeting main (excluding migration branches) and enforces that already-migrated paths are not edited by blocking non-migration PRs that attempt to modify those paths. Changes
Sequence DiagramsequenceDiagram
actor GitHub as GitHub Actions
participant Workflow as legacy-path-guard<br/>Workflow
participant Script as check-legacy-<br/>migrated-paths.ts
participant Git as Git Command
participant Validator as Path Validator
GitHub->>Workflow: PR event (opened/sync/reopened)
Workflow->>Workflow: Check: target=main<br/>& !ts-migration/*
alt Conditions Not Met
Workflow->>GitHub: Skip job
else Conditions Met
Workflow->>Workflow: Checkout repo (fetch-depth: 0)
Workflow->>Workflow: Setup Node.js 22
Workflow->>Workflow: npm install --ignore-scripts
Workflow->>Workflow: Fetch base branch (--depth=1)
Workflow->>Script: npm run ts-migration:guard
Script->>Script: Parse --base & --head args
Script->>Git: git diff --name-only base...head
Git->>Script: Return changed file paths
Script->>Validator: Map legacy paths via move-map.json
Validator->>Validator: Check test/*.test.js → .ts conversions
alt Legacy paths detected
Validator->>Script: Legacy edits found
Script->>GitHub: Exit code 1 + error message
GitHub->>GitHub: Block PR
else No legacy paths
Script->>GitHub: Exit code 0 + success message
GitHub->>GitHub: Allow PR
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 docstrings
🧪 Generate unit tests (beta)
Comment |
…y-path-guard Signed-off-by: Aaron Erickson <aerickson@nvidia.com> # Conflicts: # package.json # scripts/migrate-js-to-ts.ts # test/skills-frontmatter.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/workflows/legacy-path-guard.yaml (2)
10-15: Set explicit minimal workflow permissions.Consider adding a top-level
permissionsblock (for example,contents: read) to keep GITHUB_TOKEN scope least-privileged.Suggested fix
on: pull_request: types: [opened, synchronize, reopened, ready_for_review] + +permissions: + contents: read jobs: guard-migrated-paths:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/legacy-path-guard.yaml around lines 10 - 15, Add a top-level minimal permissions block to the workflow to limit GITHUB_TOKEN scope (for example, add permissions: contents: read) so the job "guard-migrated-paths" runs with least-privileged access; place the permissions block at the top level of the workflow YAML (parallel to "jobs") and ensure it grants only the required permission(s) instead of using default full token scope.
28-29: Prefernpm cifor deterministic CI installs.At Line 29,
npm ci --ignore-scriptsis typically faster and lockfile-strict for PR guard jobs.Suggested fix
- - name: Install root dependencies - run: npm install --ignore-scripts + - name: Install root dependencies + run: npm ci --ignore-scripts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/legacy-path-guard.yaml around lines 28 - 29, Replace the "Install root dependencies" step's command to use the lockfile-aware, deterministic installer: change the run command in that step from "npm install --ignore-scripts" to "npm ci --ignore-scripts" so CI uses the package-lock.json for reproducible installs.
🤖 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/check-legacy-migrated-paths.ts`:
- Around line 21-28: The parsing of CLI flags silently falls back to defaults
when "--base" or "--head" are missing a trailing value; update the argument
handling where argv, index, base and head are used to validate that argv[index +
1] exists and is not another flag (e.g., does not start with "--") before
assigning; if the value is missing or looks like a flag, print a clear error and
exit non-zero (or throw) rather than using the default so invocation mistakes
are surfaced in CI/debug; change the blocks that handle "--base" and "--head"
accordingly to perform this validation and error handling.
---
Nitpick comments:
In @.github/workflows/legacy-path-guard.yaml:
- Around line 10-15: Add a top-level minimal permissions block to the workflow
to limit GITHUB_TOKEN scope (for example, add permissions: contents: read) so
the job "guard-migrated-paths" runs with least-privileged access; place the
permissions block at the top level of the workflow YAML (parallel to "jobs") and
ensure it grants only the required permission(s) instead of using default full
token scope.
- Around line 28-29: Replace the "Install root dependencies" step's command to
use the lockfile-aware, deterministic installer: change the run command in that
step from "npm install --ignore-scripts" to "npm ci --ignore-scripts" so CI uses
the package-lock.json for reproducible installs.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fb95d6a0-8263-44ee-9384-604d74888f51
📒 Files selected for processing (3)
.github/workflows/legacy-path-guard.yamlpackage.jsonscripts/check-legacy-migrated-paths.ts
| if (arg === "--base") { | ||
| base = argv[index + 1] || base; | ||
| index += 1; | ||
| continue; | ||
| } | ||
| if (arg === "--head") { | ||
| head = argv[index + 1] || head; | ||
| index += 1; |
There was a problem hiding this comment.
Validate that --base/--head always receive an explicit value.
At Line 21 and Line 26, a missing trailing value currently falls back silently to defaults, which can hide invocation mistakes in CI/debug runs.
Suggested fix
+function readFlagValue(argv: string[], index: number, flag: "--base" | "--head"): string {
+ const value = argv[index + 1];
+ if (!value || value.startsWith("--")) {
+ throw new Error(`Missing value for ${flag}`);
+ }
+ return value;
+}
+
function parseArgs(argv: string[]): Options {
let base = "origin/main";
let head = "HEAD";
@@
const arg = argv[index];
if (arg === "--base") {
- base = argv[index + 1] || base;
+ base = readFlagValue(argv, index, "--base");
index += 1;
continue;
}
if (arg === "--head") {
- head = argv[index + 1] || head;
+ head = readFlagValue(argv, index, "--head");
index += 1;
continue;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (arg === "--base") { | |
| base = argv[index + 1] || base; | |
| index += 1; | |
| continue; | |
| } | |
| if (arg === "--head") { | |
| head = argv[index + 1] || head; | |
| index += 1; | |
| function readFlagValue(argv: string[], index: number, flag: "--base" | "--head"): string { | |
| const value = argv[index + 1]; | |
| if (!value || value.startsWith("--")) { | |
| throw new Error(`Missing value for ${flag}`); | |
| } | |
| return value; | |
| } | |
| if (arg === "--base") { | |
| base = readFlagValue(argv, index, "--base"); | |
| index += 1; | |
| continue; | |
| } | |
| if (arg === "--head") { | |
| head = readFlagValue(argv, index, "--head"); | |
| index += 1; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/check-legacy-migrated-paths.ts` around lines 21 - 28, The parsing of
CLI flags silently falls back to defaults when "--base" or "--head" are missing
a trailing value; update the argument handling where argv, index, base and head
are used to validate that argv[index + 1] exists and is not another flag (e.g.,
does not start with "--") before assigning; if the value is missing or looks
like a flag, print a clear error and exit non-zero (or throw) rather than using
the default so invocation mistakes are surfaced in CI/debug; change the blocks
that handle "--base" and "--head" accordingly to perform this validation and
error handling.
) ## Summary The gate checker and triage scripts treated "all present checks green" as passing, even when only 2 of ~9 checks existed. This caused premature approvals on fork PRs where workflows hadn't been triggered yet. ### Root cause Fork PRs from first-time contributors need a maintainer to click "Approve and run" before `pull_request` workflows execute. Until then, only `pull_request_target` checks (`check-pr-limit`) and external bots (`CodeRabbit`) appear in `statusCheckRollup`. The scripts saw 2/2 green and reported CI as passing. A secondary bug: GitHub's `statusCheckRollup` returns two shapes — `CheckRun` (`name`/`status`/`conclusion`) and `StatusContext` (`context`/`state`). The scripts only read CheckRun fields, so CodeRabbit (a StatusContext) was always treated as "pending" even when `state` was `SUCCESS`. ### Changes - **`check-gates.ts`**: Add `REQUIRED_CHECK_NAMES` (`checks`, `commit-lint`, `dco-check`) validation. Add `StatusCheck` union type to correctly handle both `CheckRun` and `StatusContext` shapes. CI gate now fails with `"required check(s) not found — workflows may need approval"` when expected checks are absent. - **`triage.ts`**: Add same required-check validation so triage does not score unapproved-workflow PRs as `review-ready`. - **`MERGE-GATE.md`**: Add "Missing required checks" as first bullet in Step 2 interpretation guidance. ### Before / After | PR scenario | Before | After | |---|---|---| | Fork PR, workflows not approved (2 checks) | "All 2 checks green" ✅ | "3 required check(s) not found — workflows may need approval" ❌ | | Fork PR, workflows running, dco-check failing | "1 pending" (CodeRabbit misread) | "3 failing check(s): dco-check: FAILURE, ..." ❌ | | Internal PR, all 12 checks green | "1 pending" (CodeRabbit misread) | "All 12 checks green" ✅ | ### Test plan - [x] Verified against PR #1660 (fork, workflows not approved) — correctly reports missing checks - [x] Verified against PR #1663 (fork, workflows approved, dco-check failing) — correctly reports failures - [x] Verified against PR #1683 (internal, all green) — correctly reports all 12 green - [x] Triage script correctly classifies #1660 as `salvage-now` with `failing-checks` reason instead of `review-ready` Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Enhanced merge gate to require specific CI checks be present and completed before a PR can be approved; missing required checks will block approval until workflows finish and validation is re-run. * Improved CI evaluation to better distinguish pending vs failed states across different check types. * PRs missing required check contexts are now classified as not-green. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
## Summary The `legacy-path-guard` CI job fails on every PR with `fatal: origin/main...HEAD: no merge base`. **Root cause:** The checkout uses `fetch-depth: 0` (full PR history), but then `git fetch origin main --depth=1` creates a shallow reference for `origin/main` with only 1 commit. Git can't find a merge base between the shallow main ref and HEAD. **Fix:** Remove `--depth=1` from the base branch fetch so origin/main has enough history for the three-dot diff. ## Related Introduced in #1683 ## Test plan - [ ] This PR's own `legacy-path-guard` job passes (self-validating) Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Updated internal CI/CD workflow configuration to improve git history fetching for automated checks. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…IDIA#1707) ## Summary The gate checker and triage scripts treated "all present checks green" as passing, even when only 2 of ~9 checks existed. This caused premature approvals on fork PRs where workflows hadn't been triggered yet. ### Root cause Fork PRs from first-time contributors need a maintainer to click "Approve and run" before `pull_request` workflows execute. Until then, only `pull_request_target` checks (`check-pr-limit`) and external bots (`CodeRabbit`) appear in `statusCheckRollup`. The scripts saw 2/2 green and reported CI as passing. A secondary bug: GitHub's `statusCheckRollup` returns two shapes — `CheckRun` (`name`/`status`/`conclusion`) and `StatusContext` (`context`/`state`). The scripts only read CheckRun fields, so CodeRabbit (a StatusContext) was always treated as "pending" even when `state` was `SUCCESS`. ### Changes - **`check-gates.ts`**: Add `REQUIRED_CHECK_NAMES` (`checks`, `commit-lint`, `dco-check`) validation. Add `StatusCheck` union type to correctly handle both `CheckRun` and `StatusContext` shapes. CI gate now fails with `"required check(s) not found — workflows may need approval"` when expected checks are absent. - **`triage.ts`**: Add same required-check validation so triage does not score unapproved-workflow PRs as `review-ready`. - **`MERGE-GATE.md`**: Add "Missing required checks" as first bullet in Step 2 interpretation guidance. ### Before / After | PR scenario | Before | After | |---|---|---| | Fork PR, workflows not approved (2 checks) | "All 2 checks green" ✅ | "3 required check(s) not found — workflows may need approval" ❌ | | Fork PR, workflows running, dco-check failing | "1 pending" (CodeRabbit misread) | "3 failing check(s): dco-check: FAILURE, ..." ❌ | | Internal PR, all 12 checks green | "1 pending" (CodeRabbit misread) | "All 12 checks green" ✅ | ### Test plan - [x] Verified against PR NVIDIA#1660 (fork, workflows not approved) — correctly reports missing checks - [x] Verified against PR NVIDIA#1663 (fork, workflows approved, dco-check failing) — correctly reports failures - [x] Verified against PR NVIDIA#1683 (internal, all green) — correctly reports all 12 green - [x] Triage script correctly classifies NVIDIA#1660 as `salvage-now` with `failing-checks` reason instead of `review-ready` Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Enhanced merge gate to require specific CI checks be present and completed before a PR can be approved; missing required checks will block approval until workflows finish and validation is re-run. * Improved CI evaluation to better distinguish pending vs failed states across different check types. * PRs missing required check contexts are now classified as not-green. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
## Summary The `legacy-path-guard` CI job fails on every PR with `fatal: origin/main...HEAD: no merge base`. **Root cause:** The checkout uses `fetch-depth: 0` (full PR history), but then `git fetch origin main --depth=1` creates a shallow reference for `origin/main` with only 1 commit. Git can't find a merge base between the shallow main ref and HEAD. **Fix:** Remove `--depth=1` from the base branch fetch so origin/main has enough history for the three-dot diff. ## Related Introduced in NVIDIA#1683 ## Test plan - [ ] This PR's own `legacy-path-guard` job passes (self-validating) Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Updated internal CI/CD workflow configuration to improve git history fetching for automated checks. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
## Summary - add a CI guard that blocks edits to migrated legacy JS implementation paths - block new `test/*.test.js` additions or edits now that root tests are canonical `.ts` - print actionable remediation pointing contributors at `npm run ts-migration:assist` ## Testing - npm run build:cli - npm run typecheck:cli - npm run lint - npm run ts-migration:guard -- --base origin/ts-migration/10-pr-rescue-tooling --head HEAD - npm test <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Added pull request validation that runs when targeting main. Automated checks provide detailed error messages with remediation guidance when issues are detected in CI logs. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Aaron Erickson <aerickson@nvidia.com>
…IDIA#1707) ## Summary The gate checker and triage scripts treated "all present checks green" as passing, even when only 2 of ~9 checks existed. This caused premature approvals on fork PRs where workflows hadn't been triggered yet. ### Root cause Fork PRs from first-time contributors need a maintainer to click "Approve and run" before `pull_request` workflows execute. Until then, only `pull_request_target` checks (`check-pr-limit`) and external bots (`CodeRabbit`) appear in `statusCheckRollup`. The scripts saw 2/2 green and reported CI as passing. A secondary bug: GitHub's `statusCheckRollup` returns two shapes — `CheckRun` (`name`/`status`/`conclusion`) and `StatusContext` (`context`/`state`). The scripts only read CheckRun fields, so CodeRabbit (a StatusContext) was always treated as "pending" even when `state` was `SUCCESS`. ### Changes - **`check-gates.ts`**: Add `REQUIRED_CHECK_NAMES` (`checks`, `commit-lint`, `dco-check`) validation. Add `StatusCheck` union type to correctly handle both `CheckRun` and `StatusContext` shapes. CI gate now fails with `"required check(s) not found — workflows may need approval"` when expected checks are absent. - **`triage.ts`**: Add same required-check validation so triage does not score unapproved-workflow PRs as `review-ready`. - **`MERGE-GATE.md`**: Add "Missing required checks" as first bullet in Step 2 interpretation guidance. ### Before / After | PR scenario | Before | After | |---|---|---| | Fork PR, workflows not approved (2 checks) | "All 2 checks green" ✅ | "3 required check(s) not found — workflows may need approval" ❌ | | Fork PR, workflows running, dco-check failing | "1 pending" (CodeRabbit misread) | "3 failing check(s): dco-check: FAILURE, ..." ❌ | | Internal PR, all 12 checks green | "1 pending" (CodeRabbit misread) | "All 12 checks green" ✅ | ### Test plan - [x] Verified against PR NVIDIA#1660 (fork, workflows not approved) — correctly reports missing checks - [x] Verified against PR NVIDIA#1663 (fork, workflows approved, dco-check failing) — correctly reports failures - [x] Verified against PR NVIDIA#1683 (internal, all green) — correctly reports all 12 green - [x] Triage script correctly classifies NVIDIA#1660 as `salvage-now` with `failing-checks` reason instead of `review-ready` Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Enhanced merge gate to require specific CI checks be present and completed before a PR can be approved; missing required checks will block approval until workflows finish and validation is re-run. * Improved CI evaluation to better distinguish pending vs failed states across different check types. * PRs missing required check contexts are now classified as not-green. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
test/*.test.jsadditions or edits now that root tests are canonical.tsnpm run ts-migration:assistTesting
Summary by CodeRabbit