fix: Stale pointer bug in env generation for SOPS variables

[ainsleyclark]

Summary

Fixed a bug where SOPS environment variables were appearing as empty strings in generated .env files. The issue was caused by capturing a pointer to a range-loop variable before SOPS resolution occurred, resulting in stale data being used.

Key Changes

• generate.go: Changed from storing a pointer to the app (targetApp = &app) to storing the index (targetAppIdx = i) during the app lookup loop. This ensures that after SOPS resolution modifies appDef.Apps[i].Env, we read from the updated slice element rather than a stale copy.
• generate.go: Updated all references to use appDef.Apps[targetAppIdx] directly instead of the targetApp pointer, ensuring resolved SOPS values are visible.
• generate_test.go: Added comprehensive regression test TestGenerateDefaultSOPSResolution with two sub-tests:
  • Demonstrates the bug: a range-loop copy captures nil SOPS values before resolution
  • Validates the fix: reading from the slice element after resolution yields the correct resolved values

Implementation Details

The root cause was that Go range loops create copies of values, so targetApp = &app captured a pointer to a temporary copy. When ResolveForEnvironment later modified the actual slice element appDef.Apps[i].Env, those changes were invisible to code using the stale pointer. By storing the index instead and accessing appDef.Apps[targetAppIdx] after resolution, we ensure we're always reading from the authoritative slice element.

https://claude.ai/code/session_01Q9rtBre5uvt5TuyubMF8Lx

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[claude]

Review summary

• Overall score: 8/10
• Critical issues: 0
• Warnings: 1
• Suggestions: 2
• Recommendation: ⚠️ Approve with comments

Correct, minimal fix for a classic Go range-loop pointer bug. The production code change is clean and well-reasoned. One warning about a dead-code block in the new test; two minor suggestions on test structure.

---

Critical issues 🔴

None

---

Warnings 🟡

Dead code in TestGenerateDefaultSOPSResolution / "Stale copy" sub-test (generate_test.go:88–106)

The apps slice declared at the top of the sub-test is populated and immediately mutated to simulate post-resolution state, but it is never referenced again. The test then creates a separate origApps slice to actually demonstrate the stale-pointer behaviour, making the first declaration unused.

Detail

// These nine lines are never read after line 106:
apps := []appdef.App{
    {Name: "cms", Env: appdef.Environment{...}},
}
apps[0].Env.Production["SECRET_KEY"] = appdef.EnvValue{...}

// Test then starts over with origApps:
origApps := []appdef.App{...}

The dead block should be removed. If the intent was to show the contrast (resolved vs. unresolved), that contrast is already provided by the two separate sub-tests, so this setup adds noise rather than clarity.

---

Suggestions 🟢

1. Consider asserting the key exists before checking its value (generate_test.go:134)

assert.Nil(t, staleVars["SECRET_KEY"].Value, ...)

If SECRET_KEY is absent from the map, Go returns the zero value, so Value is nil and the assertion passes trivially. A require.Contains (or checking _, ok := staleVars["SECRET_KEY"]) before this line would make the intent explicit and guard against a silent no-op.

2. The regression test simulates the resolution step manually rather than exercising Generate() end-to-end

This is a reasonable trade-off for a unit test, and the comment at the top of the function explains the reasoning well. If an integration-level test harness already exists (e.g. one with a mock SOPSClient), it would be worth adding a case there too so the test catches regressions that involve the full call chain. Not blocking; just worth noting for future test coverage.

[codecov]

Codecov Report

❌ Patch coverage is 0% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.26%. Comparing base (7f6b060) to head (460e113).
⚠️ Report is 531 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/cmd/env/generate.go	0.00%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #448      +/-   ##
==========================================
+ Coverage   64.59%   70.26%   +5.67%     
==========================================
  Files         154      187      +33     
  Lines        6064     7439    +1375     
==========================================
+ Hits         3917     5227    +1310     
+ Misses       2064     2012      -52     
- Partials       83      200     +117     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.