fix(ci): enforce trusted createRequire ratchet - #7057
Conversation
Co-authored-by: Tinson Lai <tinsonl@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
apurvvkumaria
added
area: ci
Co-authored-by: Tinson Lai <tinsonl@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
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 a base-trusted createRequire allowlist ratchet. It parses checker allowlists, inventories TypeScript usage, compares current and base revisions, wires verification into CI, and adds focused unit and workflow contract tests. ChangesCreateRequire allowlist ratchet
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CI as ci-static-checks action
participant Runtime as create-require-ratchet.mts
participant Verifier as verifyTrustedCreateRequireRatchet
participant Git as createGitRunner
CI->>CI: Install verifier dependencies
CI->>Runtime: Run ratchet script
Runtime->>Verifier: Verify repository
Verifier->>Git: Resolve trusted base
Git-->>Verifier: Return base checker
Verifier-->>Runtime: Return failure or success
Runtime-->>CI: Set exit status
Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage remains at 96%, unchanged from the TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most impacted files.
Updated |
PR Review Advisor — InformationalAdvisor assessment: Informational / high confidence Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: None This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Co-authored-by: Tinson Lai <tinsonl@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/actions/ci-static-checks/create-require-ratchet-core.mts (2)
145-178: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueBase-checker lookup discards git failure detail.
When
git showfails for a reason other than "path missing at that revision" (auth, network, corrupt object, etc.),requireSingleBaseChecker(Lines 168-171) silently treats it as a non-match and, if both candidate paths fail, only ever reportsfound 0, hiding the real cause from CI logs.🩹 Proposed fix to surface stderr on unexpected failures
const matches = ALLOWLIST_PATHS.flatMap((relativePath) => { const result = runGit(["show", `${revision}:${relativePath}`]); - return result.status === 0 ? [{ path: relativePath, source: result.stdout }] : []; + if (result.status === 0) return [{ path: relativePath, source: result.stdout }]; + if (!/^fatal: path .* does not exist/.test(result.stderr)) { + throw new Error(`git show ${revision}:${relativePath} failed: ${result.stderr}`); + } + return []; });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/actions/ci-static-checks/create-require-ratchet-core.mts around lines 145 - 178, Update requireSingleBaseChecker so runGit failures are treated as missing paths only when git explicitly indicates the candidate path does not exist at the revision; for other nonzero statuses, surface the command’s stderr in an error instead of adding no match. Preserve the existing exactly-one-match validation and return behavior for valid paths.
60-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffAvoid relying on
parseDiagnostics
parseDiagnosticsis internal and not part of the publicts.SourceFileAPI. Use a supported path likets.createProgram(...).getSyntacticDiagnostics()instead so this check doesn't depend on compiler internals.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/actions/ci-static-checks/create-require-ratchet-core.mts around lines 60 - 82, Update parseSourceFile to stop casting sourceFile to access the internal parseDiagnostics property. Use the public TypeScript API by creating a program and retrieving syntactic diagnostics with getSyntacticDiagnostics(), preserving the existing diagnostic formatting and error behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/actions/ci-static-checks/create-require-ratchet-core.mts:
- Around line 290-328: Update collectCreateRequireInventory so testSupport also
scans test/**/*.test.ts files, either by adding a dedicated test bucket to
CreateRequireInventory or by including them in the existing testSupport
collection. Ensure root-level test files such as test/package-contract/*.test.ts
are included in the createRequire ratchet without removing coverage for non-test
support files.
---
Nitpick comments:
In @.github/actions/ci-static-checks/create-require-ratchet-core.mts:
- Around line 145-178: Update requireSingleBaseChecker so runGit failures are
treated as missing paths only when git explicitly indicates the candidate path
does not exist at the revision; for other nonzero statuses, surface the
command’s stderr in an error instead of adding no match. Preserve the existing
exactly-one-match validation and return behavior for valid paths.
- Around line 60-82: Update parseSourceFile to stop casting sourceFile to access
the internal parseDiagnostics property. Use the public TypeScript API by
creating a program and retrieving syntactic diagnostics with
getSyntacticDiagnostics(), preserving the existing diagnostic formatting and
error behavior.
🪄 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: Enterprise
Run ID: 0fbcd278-6318-447a-b405-6249061c4959
⛔ Files ignored due to path filters (1)
.github/actions/ci-static-checks/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
.github/actions/ci-static-checks/action.yaml.github/actions/ci-static-checks/create-require-ratchet-core.mts.github/actions/ci-static-checks/create-require-ratchet.mts.github/actions/ci-static-checks/package.jsontest/create-require-ratchet.test.tstest/pr-workflow-contract.test.ts
|
One trusted-scanner bypass blocks this revision. nodeModule[`create${"Require"}`](import.meta.url)This expression contains no Please add conservative static folding for template heads/spans whose expressions already resolve to static strings, and add focused plus spawned end-to-end regressions proving the interpolated form is inventoried. Do not broaden this into flagging every unresolved element access: The current DCO/signature, ordinary CI, CodeRabbit thread, and no-E2E-selection evidence are otherwise clean. The failed secondary advisor and its separate event-provenance checker mismatch still need policy reconciliation, but the concrete code blocker above is independently reproducible. |
Co-authored-by: Tinson Lai <tinsonl@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Pushed signed follow-up The fix conservatively folds Local verification:
GitHub reports the commit signature as Verified ( Separately, the maintainer advisor provenance checker currently accepts only |
Co-authored-by: Tinson Lai <tinsonl@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Blocking exact head
Please rerun the exact-head trusted scanner, ordinary CI, advisor, and protected E2E gate after both are resolved. |
Co-authored-by: Tinson Lai <tinsonl@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Co-authored-by: Tinson Lai <tinsonl@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Co-authored-by: Tinson Lai <tinsonl@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Read the checker and TypeScript inventory from the exact pull-request head while comparing against the verified merge base. Fail closed on ambiguous or unreadable Git objects and escape untrusted paths in diagnostics. Co-authored-by: Tinson Lai <tinsonl@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Keep the fail-closed Git object coverage within the enforced test conditional budget. Co-authored-by: Tinson Lai <tinsonl@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Run the trusted entrypoint across a real TypeScript-to-MTS checker migration. Co-authored-by: Tinson Lai <tinsonl@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Approved on the refreshed exact head after the deterministic gate passed: 51 current checks green, mergeable/current base, no unresolved major/critical findings, DCO present, and all 12 commits verified. The ratchet validates exact event SHAs and a unique merge base, reads PR files as data without executing them, rejects non-regular entries, and uses the base-trusted locked TypeScript parser before candidate installation. Focused ratchet/workflow contracts passed 47 tests; the nine-category security review is clean.
6 participants
Summary
Run the createRequire allowlist ratchet from the immutable base-trusted static-check action. Pull requests now compare their reviewed allowlists with the validated base commit even from a shallow checkout, rejecting additions while permitting removals.
Related Issue
Closes #7056
Changes
.tschecker and the pending.mtsform, validate literal allowlists, and compare them with the pull request base revision.Type of Change
Quality Gates
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpx vitest run --project integration test/create-require-ratchet.test.ts test/pr-workflow-contract.test.ts(2 files, 23 tests passed)npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — hosted CI pendingnpm run docsbuilds without warnings (doc changes only)Signed-off-by: Apurv Kumaria akumaria@nvidia.com
Summary by CodeRabbit
New Features
createRequireusage against trusted allowlists from the base revision.Bug Fixes
Tests