test(security): drop vacuous runtime smoke test from secret guard (#3786 follow-up)#3819
Merged
Merged
Conversation
follow-up) Greptile flagged on the now-merged PR #3786 review that the 4th test in the secret-guard suite was passing vacuously: it imported runtime-config.ts and asserted `getRuntimeConfigSnapshot().secrets` contained no platform-only secret, but in node:test `import.meta.env` is undefined, so `readEnvSecret()` returns '' for every key regardless of process.env or requiredSecrets contents. The snapshot is always empty and the test always passes — even if a contributor adds WORLDMONITOR_API_KEY to a requiredSecrets array (the exact regression test #1 catches via source-grep). This commit was originally made on the #3786 branch but missed the merge window. Re-applying against new main as a follow-up PR. The 3 remaining static-analysis tests catch every realistic regression path. The honest runtime check is a bundle-grep at deploy time: npm run build && grep -r "WORLDMONITOR_API_KEY" dist/ Pattern documented in ~/.claude/skills/test-ci-gotchas/reference/vacuous-test-when-runtime-injected-context-absent.md (extracted this session).
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Greptile SummaryDrops the vacuous 4th test from the
Confidence Score: 5/5Safe to merge — removes a test that provided false confidence, and the three remaining tests still cover every meaningful static invariant in CI. The change correctly removes a test that was always passing regardless of code state, so deleting it cannot regress CI coverage. The three source-level static checks remain intact, and the inline comment accurately describes both the failure mode of the old test and the deploy-time alternative. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["browser-bundle-secret-guard tests"] --> T1["Test #1\nrequiredSecrets source scan\n(static regex on runtime-config.ts)"]
A --> T2["Test #2\nvite.config.ts define: block scan\n(static regex)"]
A --> T3["Test #3\nvite.config.ts envPrefix check\n(parses string / array literal)"]
A --> REMOVED["❌ Test #4 (REMOVED)\nruntime-config snapshot check\n(always passed vacuously —\nimport.meta.env undefined in node:test)"]
REMOVED -.->|"replaced by"| MANUAL["📋 Manual / deploy-time check\nnpm run build\ngrep -r WORLDMONITOR_API_KEY dist/"]
T1 & T2 & T3 -->|"CI guards"| CI["CI passes\nPlatform secrets not in browser bundle"]
style REMOVED fill:#f99,stroke:#c00
style MANUAL fill:#ffe,stroke:#990
style CI fill:#9f9,stroke:#090
Reviews (1): Last reviewed commit: "test(security): drop vacuous runtime smo..." | Re-trigger Greptile |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #3786 (merged at 16:43Z) addressing the P2 review comment from Greptile that I caught after the merge window closed.
Summary
The 4th test in the secret-guard suite was passing vacuously:
```ts
it('runtime-config snapshot does not contain a platform-only secret at module load', async () => {
const mod = await import('../src/services/runtime-config.ts');
const snapshot = mod.getRuntimeConfigSnapshot();
for (const secret of PLATFORM_ONLY_SECRETS) {
assert.equal(snapshot.secrets[secret], undefined, '…');
}
});
```
In `node:test`, `import.meta.env` is undefined → `readEnvSecret()` returns `''` for every key regardless of `process.env` or `requiredSecrets` contents → the snapshot is always empty → the assertion always passes, even if a contributor adds `WORLDMONITOR_API_KEY` to a `requiredSecrets` array (the exact regression test #1 catches via source-grep).
This provided false confidence, not real defense-in-depth.
Change
Drop the test. Replace with an inline note explaining (a) why it was removed and (b) where the honest runtime check belongs:
```bash
npm run build
grep -r "WORLDMONITOR_API_KEY" dist/ # must return zero hits
```
That's a deploy-time / manual check, not a unit-test guard. Tests #1–#3 (requiredSecrets scan, define-block scan, envPrefix array-form scan) remain as the load-bearing CI guards.
Test plan
The dropped-test pattern is now documented in `~/.claude/skills/test-ci-gotchas/reference/vacuous-test-when-runtime-injected-context-absent.md` (extracted this session) so the same trap doesn't get re-introduced.
Background: this commit was originally part of the #3786 review-response work but missed the merge window when you merged #3786 before my push completed. Greptile's P2 comment lives on the merged PR; replies posted there acknowledging this PR.