fix: repair drift-check workflow after #75#76
Conversation
Add root package.json and compare Infisical staging /tuj keys against Netlify production env via API instead of the GHA runner process.env. Co-authored-by: Cursor <cursoragent@cursor.com>
✅ Deploy Preview for thispagedoesnotexist12345-app ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Want AI-powered code review for this PR? Get instant analysis, interactive visualizations, and actionable insights. |
📝 WalkthroughWalkthroughRewrites ChangesInfisical↔Netlify Drift Check Rewrite
Sequence Diagram(s)sequenceDiagram
participant CI as CI Runner
participant Script as drift-check.ts
participant Infisical as Infisical SDK
participant Netlify as Netlify REST API
CI->>Script: npm run drift-check
Script->>Infisical: fetchInfisicalKeys(token, project, env, path)
Infisical-->>Script: secret keys[]
Script->>Netlify: fetchNetlifyKeys(NETLIFY_AUTH_TOKEN, NETLIFY_SITE_ID)
Netlify-->>Script: env var keys[]
Script->>Script: build expected = P0+P1+Infisical keys
Script->>Script: reportDrift(strictness, missingOnNetlify, missingInInfisical)
Script-->>CI: exit 1 (fail) or console.warn
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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 (1)
scripts/parity-manifest.ts (1)
1-4: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftRemove the second source of truth for parity keys.
Line 3 still leaves
career-navigator/scripts/sync-infisical-vault.ts PARITY_KEYSas a parallel manifest owner. If that list drifts from this file, the sync job and the drift check will enforce different key sets and produce false drift. Please have both scripts import the same exported manifest instead of relying on manual alignment.🤖 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 `@scripts/parity-manifest.ts` around lines 1 - 4, The parity key list still has two owners, which can drift between the manifest and the sync job. Update scripts/parity-manifest.ts to be the single exported source of truth and change career-navigator/scripts/sync-infisical-vault.ts to import and use that shared manifest instead of maintaining PARITY_KEYS separately. Make sure the drift check and sync logic both reference the same symbols so any future key changes only happen in one place.
🤖 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 `@package.json`:
- Line 11: The dependency version for `@types/node` is out of sync with the
Node.js 20 runtime used by the drift-check workflow. Update the `@types/node`
entry in package.json to a Node 20-compatible major, or change the Node version
in .github/workflows/drift-check.yml to match the type package. Keep the runtime
and typings aligned so symbols like the package.json dependency and the
drift-check workflow stay on the same major version.
In `@scripts/drift-check.ts`:
- Around line 114-124: The drift check currently flattens P0 and P1 into a
single manifest set, which loses tier-specific handling in drift reporting.
Update the logic in drift-check.ts around manifestKeys/expectedOnNetlify and
reportDrift() to preserve separate P0 and P1 collections, compute drift for each
tier independently, and only allow P1 items to downgrade to warnings when
strictness is warn while keeping P0 as enforced.
- Around line 19-28: Both upstream awaits in the drift check can hang
indefinitely, so add explicit timeouts around the Infisical and Netlify requests
instead of relying on the GitHub Actions job timeout. Update fetchInfisicalKeys
and the Netlify fetch logic used by Promise.all() to go through a shared timeout
helper that fails fast with a clear error message, and apply the same pattern to
the other referenced call sites so the workflow exits promptly when either API
stalls.
---
Nitpick comments:
In `@scripts/parity-manifest.ts`:
- Around line 1-4: The parity key list still has two owners, which can drift
between the manifest and the sync job. Update scripts/parity-manifest.ts to be
the single exported source of truth and change
career-navigator/scripts/sync-infisical-vault.ts to import and use that shared
manifest instead of maintaining PARITY_KEYS separately. Make sure the drift
check and sync logic both reference the same symbols so any future key changes
only happen in one place.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0da1f2a5-caae-4dfc-9dd9-1d0801bdc689
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
.github/workflows/drift-check.yml.gitignorepackage.jsonscripts/drift-check.tsscripts/parity-manifest.tstsconfig.json
| }, | ||
| "devDependencies": { | ||
| "@infisical/sdk": "^5.0.2", | ||
| "@types/node": "^26.0.1", |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "package.json `@types/node`:"
jq -r '.devDependencies["`@types/node`"]' package.json
echo
echo "workflow node-version:"
sed -n '17,21p' .github/workflows/drift-check.ymlRepository: palmtr3man/thispagedoesnotexist12345
Length of output: 362
🌐 Web query:
@types/node versioning follows Node.js major release types package documentation Node 20 vs Node 26 APIs
💡 Result:
The @types/node package versioning is designed to align its major version with the major version of Node.js [1][2]. To ensure type definitions accurately match your environment, you should use the @types/node major version that corresponds to your Node.js major version (e.g., use @types/node@20.x for Node.js 20 and @types/node@26.x for Node.js 26) [2]. Please note the following regarding the package: - Versioning Policy: While the major version generally aligns with Node.js, the package does not strictly adhere to SemVer in the traditional sense regarding breaking changes [3]. Updates are frequent, and breaking changes can occur within any release [3]. - Node 20 vs. Node 26 APIs: - Node 26 (Current) includes significant updates, such as the Temporal API enabled by default [4][5][6] and an upgrade to V8 14.6 [4][6]. - It removes several long-deprecated APIs that were present in Node 20, such as http.Server.prototype.writeHeader (which should be replaced with writeHead) and various legacy stream* modules [6]. - The @types/node 26.x package includes type definitions for these new APIs and removals, ensuring your TypeScript project reflects the Node 26 environment [7][6]. For projects supporting multiple Node.js versions, developers often use scripts to dynamically align the @types/node version with the active Node.js version in their development environment [2].
Citations:
- 1: https://stackoverflow.com/questions/42035263/relationship-between-the-version-of-node-js-and-the-version-of-types-node
- 2: [@types/node] How to automatically install the correct version for the current Node.js environment DefinitelyTyped/DefinitelyTyped#69418
- 3: Releases of [@types/node] are too often DefinitelyTyped/DefinitelyTyped#68434
- 4: https://nodejs.org/en/blog/release/v26.0.0
- 5: 2026-05-05, Version 26.0.0 (Current) nodejs/node#62526
- 6: https://maximov.by/nodejs-26-complete-guide.html
- 7: https://nodejs.org/docs/v26.0.0/api/
🌐 Web query:
DefinitelyTyped @types/node latest major version Node 20 Node 26 compatibility
💡 Result:
Latest major version of DefinitelyTyped @types/node (as of 2026-06-27): 26.0.0 [1]. Compatibility with Node.js 20 and Node.js 26: - The DefinitelyTyped project maintains major branches/releases such that the @types/node major version should match the environment’s Node.js major version [2]. - Therefore, for Node.js 20 you should use @types/node 20.x (not the latest 26.x), and for Node.js 26 you should use @types/node 26.x [2]. Notes specific to the Node 26 @types/node major line: - The DefinitelyTyped PR updating to “node: v26” indicates that v26 will require TypeScript 5.6+ (it mentions removal of TS <=5.5 iterator helper compatibility definitions) [3]. Answer summary: - Use @types/node@20 for Node 20, and @types/node@26 for Node 26; the latest major currently is 26 [1][2].
Citations:
- 1: https://www.npmjs.com/package/@types/node
- 2: [@types/node] How to automatically install the correct version for the current Node.js environment DefinitelyTyped/DefinitelyTyped#69418
- 3: node: v26 DefinitelyTyped/DefinitelyTyped#75025
Align @types/node with the Node 20 runtime. package.json pins @types/node to ^26.0.1, but .github/workflows/drift-check.yml runs Node.js 20. Keep the type package on the same major as the runtime, or bump the workflow/runtime in lockstep.
🤖 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 `@package.json` at line 11, The dependency version for `@types/node` is out of
sync with the Node.js 20 runtime used by the drift-check workflow. Update the
`@types/node` entry in package.json to a Node 20-compatible major, or change the
Node version in .github/workflows/drift-check.yml to match the type package.
Keep the runtime and typings aligned so symbols like the package.json dependency
and the drift-check workflow stay on the same major version.
| async function fetchInfisicalKeys(token: string): Promise<Set<string>> { | ||
| const client = new InfisicalSDK({ siteUrl: INFISICAL_SITE_URL }); | ||
| await client.auth().accessToken(token); | ||
|
|
||
| const response = await client.secrets().listSecrets({ | ||
| projectId: INFISICAL_PROJECT_ID, | ||
| environment: INFISICAL_ENV, | ||
| secretPath: INFISICAL_PATH, | ||
| viewSecretValue: false, | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add explicit timeouts around both upstream calls.
Both Infisical and Netlify requests are awaited without a deadline. If either API hangs, Promise.all() stalls the workflow until the GitHub Actions job timeout. Wrap both calls in a timeout helper so the step fails fast with a clear error.
Also applies to: 44-49, 109-112
🤖 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 `@scripts/drift-check.ts` around lines 19 - 28, Both upstream awaits in the
drift check can hang indefinitely, so add explicit timeouts around the Infisical
and Netlify requests instead of relying on the GitHub Actions job timeout.
Update fetchInfisicalKeys and the Netlify fetch logic used by Promise.all() to
go through a shared timeout helper that fails fast with a clear error message,
and apply the same pattern to the other referenced call sites so the workflow
exits promptly when either API stalls.
| const manifestKeys = [...new Set([...P0_KEYS, ...P1_KEYS])]; | ||
| const expectedOnNetlify = new Set([...infisicalKeys, ...manifestKeys]); | ||
|
|
||
| const missingOnNetlify = [...expectedOnNetlify].filter( | ||
| (key) => !isKeyPresent(key, netlifyKeys) | ||
| ); | ||
| const missingInInfisical = manifestKeys.filter( | ||
| (key) => !isKeyPresent(key, infisicalKeys) | ||
| ); | ||
|
|
||
| reportDrift(strictness, missingOnNetlify, missingInInfisical); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep P0 and P1 separate in the drift calculation.
manifestKeys = [...new Set([...P0_KEYS, ...P1_KEYS])] throws away the tiering defined in scripts/parity-manifest.ts, so reportDrift() can only apply one global policy. That makes a P0 miss degrade to a warning whenever STRICTNESS=warn, which contradicts the manifest contract. Compute and report P0/P1 drift separately, then only downgrade P1 on warn runs.
🤖 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 `@scripts/drift-check.ts` around lines 114 - 124, The drift check currently
flattens P0 and P1 into a single manifest set, which loses tier-specific
handling in drift reporting. Update the logic in drift-check.ts around
manifestKeys/expectedOnNetlify and reportDrift() to preserve separate P0 and P1
collections, compute drift for each tier independently, and only allow P1 items
to downgrade to warnings when strictness is warn while keeping P0 as enforced.
1 participant
Summary
package.json/ lockfile sonpm cisucceeds in CIscripts/drift-check.tsto compare Infisical staging/tujkeys against Netlify production env vars via API (the merged feat: add Netlify Guardrails drift check #75 script comparedprocess.envon the GHA runner, which only hasINFISICAL_TOKEN)scripts/parity-manifest.tswith P0/P1 keys and alias groups aligned to SEC-05 paritynpm run drift-checkand requireNETLIFY_AUTH_TOKEN+NETLIFY_SITE_IDPrerequisites (repo settings)
Before CI passes, add in
palmtr3man/thispagedoesnotexist12345:INFISICAL_TOKEN— read access to Infisicalstagingenv, path/tujNETLIFY_AUTH_TOKEN— Netlify personal access token with env readNETLIFY_SITE_ID— site UUID forthispagedoesnotexist12345-appTest plan
Netlify Guardrails: Drift Checkworkflow passes onmainworkflow_dispatchand confirm drift output lists any real gapsMade with Cursor
Summary by CodeRabbit
New Features
Bug Fixes
Chores