fix(secret-reference): keep literal reference when secret does not exist

[claude]

Requested by Daniel Hougaard · Slack thread

Before / After

Before: A secret reference like ${SOME_SECRET} pointing to a secret that does not exist resolved to an empty string (""). This was a regression introduced by #6844 ("check for empty value on reference"), which removed the if (referencedSecretValue) guard. It broke customer production services that validate their environment variables are non-empty.

After:

• A reference to a secret that does not exist keeps its literal ${REF} text in the output, untouched.
• A reference to a secret that exists but has an empty value ("") still resolves to "" — the behavior feat(secret-reference): check for empty value on reference #6844 intended is preserved.

How

The two cases above were previously indistinguishable: fetchSecret returned { value: "", tags: [] } for both "secret not found" and "found with empty value".

• fetchSecret now returns an exists: boolean flag on every return path (folder missing, key absent, DB error => exists: false; found in map / cache hit => exists: true), distinguishing the two cases.
• The substitution in recursivelyExpandSecret is now gated on exists rather than on value truthiness. When the referenced secret does not exist, the literal ${REF} is left in place.

A focused e2e test was added asserting that hello ${NON_EXISTENT_SECRET} resolves to the literal hello ${NON_EXISTENT_SECRET}, alongside the existing hello ${EMPTY} => "hello " test which continues to pass.

Note: the v1 legacy expansion path in secret-fns.ts still has the old unconditional substitution and could receive the same treatment as a follow-up.

Relates to / partially reverts the conflation introduced by #6844.

🤖 Generated with Claude Code

https://claude.ai/code/session_01CaforYBUXs5dkUfUBwSKS5

---

Generated by Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1e3a28c71f

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

Do not mark negative cache hits as existing

When a missing reference is fetched below, its { exists: false } result is written into secretCache; on the next lookup this branch sees that truthy cached object and overwrites it to exists: true. A value such as ${MISSING} ${MISSING} (or two secrets expanded by the same factory that both reference MISSING) will therefore remove the second/future occurrence(s) as an empty string instead of preserving the literal, which is exactly the case this fix is meant to protect.

Useful? React with 👍 / 👎.

[greptile-apps]

Greptile Summary

This PR fixes a regression where referencing a non-existent secret in a secret value would silently resolve to "" instead of preserving the literal ${REF} text. It adds an exists flag to fetchSecret and gates the substitution on that flag rather than value truthiness.

• fetchSecret now returns exists: boolean on every code path, distinguishing "secret not found" from "secret found with empty value", and recursivelyExpandSecret skips the replaceAll call when exists is false.
• A new e2e test asserts the literal is preserved for a single non-existent reference, while the existing empty-value test continues to verify \"\" resolution is unaffected.

Confidence Score: 3/5

The fix works for the single-reference case but leaves a cache corruption path that turns literal-preservation off for any second use of the same non-existent key within the same expansion batch.

After fetchSecret returns { exists: false } for a missing key, recursivelyExpandSecret writes that result object into secretCache (lines 198-199 and 219-220). Because every JS object is truthy, the next call to fetchSecret for the same key finds that cache entry and returns { ...cachedSecret, exists: true }, causing the substitution to fire and silently replace the literal with "". This affects both duplicate references within one value and any second secret in the same batch that references the same missing key — a realistic scenario when the factory is reused across all secrets in a getSecrets call.

backend/src/services/secret-v2-bridge/secret-reference-fns.ts — the manual cache-write block for non-existent secrets needs to be guarded by referredValue.exists.

Important Files Changed

Filename	Overview
backend/src/services/secret-v2-bridge/secret-reference-fns.ts	Adds exists flag to fetchSecret and gates substitution on it — correct intent, but writing non-existent secret stubs into secretCache causes the second reference to the same missing key to incorrectly resolve to "".
backend/e2e-test/routes/v3/secret-reference.spec.ts	Adds a focused e2e test for the non-existent secret case; the test only covers a single reference per value and does not catch the duplicate-reference or cross-secret cache bug.

Comments Outside Diff (1)

1. backend/src/services/secret-v2-bridge/secret-reference-fns.ts, line 197-199 (link)

   Cache poisoned for non-existent secrets

   recursivelyExpandSecret writes the fetchSecret result (including exists: false) into secretCache at lines 199 and 220. When a second reference to the same non-existent secret is later looked up — either in the same value ("${MISSING} and ${MISSING}") or in a different secret expanded by the same factory instance (which shares secretCache) — fetchSecret finds a truthy object ({ value: "", tags: [], exists: false }) for that key and returns { ...cachedSecret, exists: true }, incorrectly reporting the secret as existing and substituting "". The fix: only cache the secret if it exists, e.g. if (referredValue.exists) { secretCache[cacheKey][secretKey] = referredValue; }. The same pattern applies at lines 219-220 for cross-environment references.

_{Reviews (1): Last reviewed commit: "fix(secret-reference): keep literal ${re..." | Re-trigger Greptile}

[varonix0]

@claude, did you address this?

Cache poisoned for non-existent secrets

recursivelyExpandSecret writes the fetchSecret result (including exists: false) into secretCache at lines 199 and 220. When a second reference to the same non-existent secret is later looked up — either in the same value ("${MISSING} and ${MISSING}") or in a different secret expanded by the same factory instance (which shares secretCache) — fetchSecret finds a truthy object ({ value: "", tags: [], exists: false }) for that key and returns { ...cachedSecret, exists: true }, incorrectly reporting the secret as existing and substituting "". The fix: only cache the secret if it exists, e.g. if (referredValue.exists) { secretCache[cacheKey][secretKey] = referredValue; }. The same pattern applies at lines 219-220 for cross-environment references.

[claude]

Yes — fixed in 538d279.

The root cause was as you described: recursivelyExpandSecret writes each fetchSecret result (including exists: false) back into secretCache, and the cache-hit branch then hardcoded exists: true on the truthy cached object, so a repeated ${MISSING} (or the same missing key reused across a batch) flipped to "".

Rather than skip caching for missing secrets, I stored exists in the cache and made the cache-hit branch return the cached flag instead of hardcoding true. That fixes the same scenarios while still letting missing-key lookups hit the cache (no re-fetch on repeats). Added an e2e test for ${MISSING} ${MISSING} resolving to the literal ${MISSING} ${MISSING}. Type-check and lint are green.