fix: make the env guard catch dead knobs, not just unreachable ones - #656
Merged
Merged
Conversation
The guard checked one direction — variables the backend reads that no compose file passes — and was blind to the inverse: a variable compose passes that no Spring config reads. That one looks configurable in .env and does nothing. Which is exactly what I shipped. #645 wired GEO_TIMEOUT_SECONDS into both compose files because application.yml referenced it, without checking whether anything consumed it. Nothing did — GeoIpService uses hardcoded timeout constants — so the knob was inert from the moment it was added, and CI passed. #653 removed the dead property and the compose lines with it. A check written after this failure mode bit twice should not have been able to introduce a third instance of it. Now reports both directions separately, with the fix for each. The allowance for variables consumed outside Spring is an explicit two-entry list (APP_MEMORY_MB, TZ) rather than prefix matching. A pattern like LLM_* or MINIO_* would be less maintenance but would hide a genuinely dead knob in the same family — the precise thing this half of the check exists to find. Verified in both directions: injecting a knob nothing reads fails the check, and removing SURICATA_ENABLED from compose fails it too. Passes clean on main. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe environment checker now validates Spring-to-Compose and Compose-to-Spring coverage. It adds documented passthrough exceptions for infrastructure-consumed variables and reports each failure type separately. ChangesEnvironment passthrough validation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/check_env_passthrough.py`:
- Around line 110-115: Replace the filtered diagnostic append loops that
populate unreachable and dead in the environment passthrough analysis with
list.extend comprehensions over the same filtered tuples. Preserve the existing
ordering and filtering conditions while eliminating the PERF401 findings.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: afeb08ff-f96e-49a2-bf0c-54301f353842
📒 Files selected for processing (1)
scripts/check_env_passthrough.py
PERF401 from review on #656. Both filtered append loops become extend over a generator, which also gave each branch a one-line comment saying what it detects — the two halves of this check are easy to confuse when skimming. Adopted the intent rather than the suggested diff: the proposal rewrote the second loop as dead.extend([(stack, var)]) still inside the for, which is a single-element extend per iteration and strictly worse than the append it replaces. Note there is no Ruff config or Python lint in CI, so this is a readability change rather than a gate. Behaviour verified unchanged in both directions — injected dead knob still fails, removed SURICATA_ENABLED still fails, clean tree still passes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
The gap
The guard added in #645 checked one direction only — variables the backend reads that no compose file passes. It was blind to the inverse: a variable compose passes that no Spring config reads. That knob looks configurable in
.envand silently does nothing.Which is what I shipped in that same PR. #645 wired
GEO_TIMEOUT_SECONDSinto both compose files becauseapplication.ymlreferenced it — without checking whether anything consumed it. Nothing did;GeoIpServiceuses hardcoded timeout constants. The knob was inert from the moment it was added, and CI passed green.#653 caught it independently and removed both the dead property and the compose lines.
A check written because this failure mode had already bitten twice (#628, #641) should not have been able to introduce a third instance of it.
The fix
Reports both directions separately, each with its own remedy:
One deliberate choice
The allowance for variables consumed outside Spring is an explicit two-entry list —
APP_MEMORY_MB(read by the entrypoint to size the heap) andTZ(consumed by the OS) — not prefix matching.I first wrote it as a regex covering
LLM_*,MINIO_*,DATABASE_*and friends. That is less maintenance, and it would hide a genuinely dead knob in any of those families — the precise failure this half of the check exists to find. Enumerating the real set showed it is only two entries, so the maintenance argument was hollow.Verification
SURICATA_ENABLEDremoved from compose🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation