Greptile Summary

This PR fixes a flip-flop bug in normalizeProviders() where providers configured with a plain-string apiKey that happened to equal an env-var value would have that key silently converted to the env-var name on every normalization run, causing models.json to revert user edits. The fix makes the reverse-lookup conditional on the original source config (sourceProviders) actually containing an { source: "env" } SecretRef for that provider, so the conversion only fires when it was genuinely configured as an env-var reference.

Key observations:

• The sole production caller (models-config.plan.ts) already passes sourceProviders via resolved.sourceConfigForSecrets, so the security invariant from issue [Bug]: v2026.3.2: custom provider apiKey from env/SecretRef is persisted as plaintext into state agents/*/agent/models.json #38757 (preventing plaintext persistence when env refs are in use) is preserved.
• All four previously flagged thread issues (missing BUG REPRODUCTION test, ReferenceError on normalized1, wrong secretref-env: marker prefix, leftover double blank line) are resolved in this HEAD commit.
• The ENV_VAR_NAME_RE constant move and the abort.test.ts mock rename are clean, unrelated housekeeping.
• One subtle ordering concern exists: after the new reverse-lookup sets normalizedProvider.apiKey, execution falls through to normalizeApiKeyConfig(configuredApiKey), which re-reads the original value. A future change to upstream SecretRef detection could make both branches fire for the same input, silently overwriting the reverse-lookup result. Adding a guard (skip normalizeApiKeyConfig when normalizedProvider.apiKey has already been updated) would make the mutual exclusivity explicit.
• The new "manual edits preserved" test constructs its edited input from originalProviders rather than from the output of the first normalization call, which means a round-trip regression (first pass mutates, second pass reverts) would not be caught.

Confidence Score: 4/5

• Safe to merge; the fix is logically correct and the security path for env-var references is preserved by the production caller.
• The core logic change is sound and correctly scoped. The production caller already passes sourceProviders, so the [Bug]: v2026.3.2: custom provider apiKey from env/SecretRef is persisted as plaintext into state agents/*/agent/models.json #38757 security fix remains effective. The only concerns are a subtle fall-through ordering assumption in models-config.providers.ts (benign today but fragile under future refactoring) and a test that doesn't fully exercise the chained-normalization scenario it describes — neither blocks merging.
• Mild attention to src/agents/models-config.providers.ts lines 528–536 (fall-through interaction between the new reverse-lookup block and normalizeApiKeyConfig) and src/agents/models-config.providers.flipflop.test.ts lines 112–118 (non-chained normalization test).

This is a comment left during a code review.
Path: src/agents/models-config.providers.ts
Line: 528-536

Comment:
**Reverse-lookup result could be overwritten by the fall-through `normalizeApiKeyConfig` path**

After the conditional reverse-lookup (lines 513–526) sets `normalizedProvider.apiKey` to the env-var name, execution falls through to `normalizeApiKeyConfig(configuredApiKey)`, which still operates on the **original** `configuredApiKey`. If that call returns a value different from `configuredApiKey` (e.g. stripping `${...}` braces), the spread on lines 532–535 would silently overwrite the env-var name the reverse-lookup just produced.

In practice this is benign today — any `configuredApiKey` that would be transformed by `normalizeApiKeyConfig` (i.e. matching `${VAR}`) would have produced a non-null `configuredApiKeyRef` upstream, routing it through the `if` branch before this `else if` is ever reached. But the correctness depends on that upstream invariant remaining intact, and the fall-through ordering makes the assumption easy to miss in future refactors.

A simple guard (checking `normalizedProvider.apiKey === configuredApiKey` before calling `normalizeApiKeyConfig`) would make the mutual exclusivity explicit and prevent any future breakage if the `${VAR}` detection logic upstream ever changes.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: src/agents/models-config.providers.flipflop.test.ts
Line: 112-118

Comment:
**"Manual edits preserved" test does not chain the two normalizations**

The first `normalizeProviders` call result is discarded (not assigned). The `editedProviders` object is then constructed from `originalProviders` directly and normalized in isolation. This correctly exercises the no-`sourceProviders` code path, but it does not cover the scenario described by the comment — where the *output* of a first normalization is manually edited and then re-normalized. A regression where the first pass mutates the value in a way that causes the second pass to revert it would go undetected.

For stronger coverage, consider passing the output of the first call as the base for the edit, so the test genuinely validates round-trip stability.

How can I resolve this? If you propose a fix, please make it concise.

_{Last reviewed commit: 111020b}

Greptile review comment fix - The third test 'FIX VERIFICATION: manual edits to models.json are preserved' had an issue where the first normalization result was discarded and edits were built from the original providers instead of the normalized output.

Fix applied:

-    // First normalization
-    normalizeProviders({ providers: originalProviders, agentDir });
+    // First normalization — capture the output
+    const normalized1 = normalizeProviders({ providers: originalProviders, agentDir });

-    // Simulate user editing models.json
+    // Simulate user editing the normalized output from models.json
     const editedProviders = {
       openai: {
-        ...originalProviders.openai,
-        apiKey: "edited-value",
+        ...normalized1!.openai!,
+        apiKey: "edited-value", // User manually edits after first normalization
       },
     };

All 4 tests pass with this fix.

The "manual edits preserved" test should capture the first normalizeProviders result and use it as the base for editing. Currently editedProviders is built from originalProviders instead of the normalized output, so round-trip stability is not actually validated.

Fix:

// First normalization — capture the output
const normalized1 = normalizeProviders({ providers: originalProviders, agentDir });

// Simulate user editing the normalized output from models.json
const editedProviders = {
  openai: {
    ...normalized1.openai!,
    apiKey: "edited-value", // User manually edits after first normalization
  },
};

// Second normalization: should preserve the manually edited value
const normalized2 = normalizeProviders({ providers: editedProviders, agentDir });
expect(normalized2?.openai?.apiKey).toBe("edited-value");