fix: address PR #84 alignment follow-ups (manifest reference, env validation, Netlify context preservation) - #97
Conversation
…lowlist The header comment claimed the manifest must be kept aligned with career-navigator/scripts/sync-infisical-vault.ts PARITY_KEYS, but that repo/export never existed. This repo already has its own scripts/sync-infisical-vault.ts, whose ALLOWLIST overlaps significantly with P0_KEYS/P1_KEYS here (BASE44_AUTH_JSON, BEEHIIV_API_KEY, SUPABASE_SERVICE_ROLE_KEY, NOTION_API_KEY, SENDGRID_API_KEY, SEC06_INTERNAL_TOKEN, SEC06_SCHEDULER_SECRET), so point the comment at that verifiable, local source of truth instead of an inferred external path. Co-authored-by: palmtr3man <palmtr3man@users.noreply.github.com>
INFISICAL_TOKEN, NETLIFY_AUTH_TOKEN, and NETLIFY_SITE_ID were only checked for truthiness, so a whitespace-only value (e.g. a misconfigured GitHub Actions secret) would pass the guard and fail later with a confusing API error. Add a requireEnv() helper that trims and rejects missing/empty/whitespace-only values with a clear error, and export it + guard the script's top-level execution so it's testable without running the full drift check. Adds scripts/test-drift-check.ts and a test:drift-check npm script, following the existing test-<script>.ts convention in this repo. Co-authored-by: palmtr3man <palmtr3man@users.noreply.github.com>
upsertNetlifySecret read only existing.values[0].context and sent a single-value payload. Since Netlify's env-var PATCH/POST bodies replace the entire values array, this silently deleted any Preview or Branch deploy overrides a variable already had, keeping only the first context (or clobbering everything with 'all' when there was no existing entry). Extract buildNetlifyValuesPayload() to build the values array from every existing context entry (preserving context_parameter for branch-specific overrides) and apply the new value to each one, instead of dropping all but the first. Also guard the script's top-level main() invocation so the module is importable for tests. Adds scripts/test-sync-drift-keys.ts (mocks fetch; no live Netlify/ Infisical calls) and a test:sync-drift-keys npm script. Co-authored-by: palmtr3man <palmtr3man@users.noreply.github.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Essentials Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe drift scripts now validate required environment values, avoid side effects when imported, preserve Netlify context metadata during synchronization, and include isolated unit-test commands. ChangesDrift validation and synchronization
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 2 potential issues.
2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| "Content-Type": "application/json", | ||
| }; | ||
| const valuePayload = { values: [{ context, value }] }; | ||
| const valuePayload = { values }; |
There was a problem hiding this comment.
🟡 Existing Netlify variables cannot update
When a key exists, valuePayload uses PUT's array schema for a PATCH endpoint that accepts one context value. Netlify rejects the update, stopping synchronization.
Prompt for agents
The Netlify endpoint at /api/v1/accounts/{accountId}/env/{key} uses PATCH to set one contextual value with a body shaped as { context, context_parameter?, value }. Replacing all values uses PUT with the full environment-variable body. upsertNetlifySecret currently sends { values: [...] } through PATCH for both existing keys and create-conflict fallback, so real updates fail. Choose the appropriate API operation, preserve all required variable metadata if using PUT, and add a mock that validates the request method and exact body against Netlify's API contract. Also account for the create-conflict path, where the current values were not loaded.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const infisicalToken = requireEnv("INFISICAL_TOKEN"); | ||
| const netlifyAuthToken = requireEnv("NETLIFY_AUTH_TOKEN"); | ||
| const netlifySiteId = requireEnv("NETLIFY_SITE_ID"); |
There was a problem hiding this comment.
🟡 Automation bypasses credential validation
The requireEnv checks guard an unused entry point; the configured drift command executes another script. Automation still accepts whitespace credentials and fails later.
Prompt for agents
The GitHub drift-check workflow runs npm run drift-check, and package.json maps that command to scripts/sync-drift-keys.ts rather than scripts/drift-check.ts. Consequently, requireEnv does not validate the automated path. Decide which script is the intended drift-check implementation, wire the npm script and workflow to it, or share the validation with sync-drift-keys.ts. Ensure every credential actually consumed by the selected script is trimmed and rejected when empty or whitespace-only, including supported fallback variable names.
Was this helpful? React with 👍 or 👎 to provide feedback.
Greptile SummaryThe PR improves required-environment validation, corrects the parity-manifest reference, and adds helpers intended to preserve Netlify deployment contexts during secret updates.
Confidence Score: 4/5The PR appears safe to merge, with a non-blocking test gap around validating context preservation through the real Netlify lookup path. The implementation changes do not establish a current blocking failure, but the new regression test can pass without exercising the production-filtered lookup that determines which deployment contexts are available to preserve. Files Needing Attention: scripts/test-sync-drift-keys.ts, scripts/sync-drift-keys.ts
|
| Filename | Overview |
|---|---|
| scripts/drift-check.ts | Adds testable environment validation that trims accepted values and clearly rejects missing or whitespace-only credentials. |
| scripts/sync-drift-keys.ts | Adds context-aware Netlify payload construction and import-safe execution, but the operational lookup-to-update behavior is not covered by the new regression test. |
| scripts/test-sync-drift-keys.ts | Covers helper payload construction and mocked request bodies but supplies contexts directly, bypassing the production-filtered lookup central to the preservation behavior. |
| scripts/test-drift-check.ts | Directly covers valid, missing, empty, and whitespace-only environment-variable handling. |
| scripts/parity-manifest.ts | Updates only the source-of-truth documentation reference. |
| package.json | Exposes the two new focused test commands. |
Prompt To Fix All With AI
### Issue 1
scripts/test-sync-drift-keys.ts:76-81
**Test Bypasses Context Lookup**
The regression test injects production, preview, and branch values directly into `upsertNetlifySecret`, while the real repair path first fetches variables with `context_name=production`. It therefore remains green without validating that the operational lookup supplies every context the update is intended to preserve.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "fix: preserve all Netlify deploy context..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| { context: "production", value: "old-prod" }, | ||
| { context: "deploy-preview", value: "old-preview" }, | ||
| { context: "branch-deploy", context_parameter: "staging", value: "old-branch" }, | ||
| ], | ||
| }) | ||
| ); |
There was a problem hiding this comment.
The regression test injects production, preview, and branch values directly into upsertNetlifySecret, while the real repair path first fetches variables with context_name=production. It therefore remains green without validating that the operational lookup supplies every context the update is intended to preserve.
Knowledge Base Used:
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/test-sync-drift-keys.ts
Line: 76-81
Comment:
**Test Bypasses Context Lookup**
The regression test injects production, preview, and branch values directly into `upsertNetlifySecret`, while the real repair path first fetches variables with `context_name=production`. It therefore remains green without validating that the operational lookup supplies every context the update is intended to preserve.
**Knowledge Base Used:**
- [Data, security, and platform operations](https://app.greptile.com/palmtree-studios/-/custom-context/knowledge-base/palmtr3man/thispagedoesnotexist12345/-/docs/data-security-platform.md)
- [Deployment security operations](https://app.greptile.com/palmtree-studios/-/custom-context/knowledge-base/palmtr3man/thispagedoesnotexist12345/-/docs/deployment-security-operations.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
2 participants
Summary
Addresses three alignment items flagged during review of #84 (
fix/drift-check-infisical-root-path).scripts/parity-manifest.ts— The header comment claimed the manifest must stay aligned withcareer-navigator/scripts/sync-infisical-vault.ts PARITY_KEYS, but that repo/export never existed. This repo already has its ownscripts/sync-infisical-vault.ts, whoseALLOWLISToverlaps significantly withP0_KEYS/P1_KEYShere (BASE44_AUTH_JSON,BEEHIIV_API_KEY,SUPABASE_SERVICE_ROLE_KEY,NOTION_API_KEY,SENDGRID_API_KEY,SEC06_INTERNAL_TOKEN,SEC06_SCHEDULER_SECRET). Repointed the comment at that verifiable, local source of truth instead of the stale/nonexistent external one.scripts/drift-check.ts—INFISICAL_TOKEN,NETLIFY_AUTH_TOKEN, andNETLIFY_SITE_IDwere only checked for truthiness, so a whitespace-only value (e.g. a misconfigured GitHub Actions secret) would pass the guard and fail later with a confusing API error instead of a clear one. Added arequireEnv()helper that trims and rejects missing/empty/whitespace-only values, and exported it (with the script's top-level execution guarded) so it's unit-testable.scripts/sync-drift-keys.ts—upsertNetlifySecretread onlyexisting.values[0].contextand sent a single-value payload. Since Netlify's env-var PATCH/POST bodies replace the entirevaluesarray, this silently deleted any Preview or Branch-deploy overrides a variable already had, keeping only the first context. ExtractedbuildNetlifyValuesPayload()to preserve every existing context entry (includingcontext_parameterfor branch-specific overrides) and apply the new value to each one, rather than clobbering the rest.Tests
scripts/test-drift-check.ts(npm run test:drift-check) coveringrequireEnv: trims valid values, and rejects missing/empty/whitespace-only.scripts/test-sync-drift-keys.ts(npm run test:sync-drift-keys, mocksfetch, no live network calls) coveringbuildNetlifyValuesPayloadandupsertNetlifySecret: preserves Production/Preview/Branch contexts (withcontext_parameter) on update, and still creates a singleall-context value for brand-new keys.test-<script>.tsconvention used elsewhere inscripts/.Verification
npx tsc --noEmit— passesnpm run test:drift-check— passesnpm run test:sync-drift-keys— passesnpm run build— passesscripts/drift-check.tsandscripts/sync-drift-keys.tswith missing/whitespace-only env vars to confirm they still fail closed with the new clearer error messages.Made with Cursor
Summary by CodeRabbit
Bug Fixes
Tests
Documentation