fix: resolve secret env aliases by layer priority - #34
Conversation
Aliases were compared for equality before layer precedence was applied, so a stale value in the persisted env store hard-failed the run instead of losing to a rotated value in the layered secrets files. Rotating a token that is declared under two aliases (GH_TOKEN/GITHUB_TOKEN) wedged every project whose persisted store still held the old value. The highest-priority layer now wins outright. Only aliases resolved from the same layer must agree, since that is the one case the user has to fix; the error names the layer. Cross-layer drift is warned about and heals on the next run, because the winning value is injected into every alias and written back to the persisted store.
EclipseSourceAI
left a comment
EclipseSourceAI
left a comment
There was a problem hiding this comment.
Note
Autonomous AI review.
This review was done by an AI agent and therefore may contain mistakes. Feel free to ignore any comment you disagree with. Noting why helps, since replies are read and taken into account in follow-up reviews.
Resolving all AI comments does not lead to an automatic approval. A maintainer still needs to review and sign off on the overall architecture and design.
To get an updated review after pushing changes, a maintainer may re-request a review from this account.
Submitted via review-guard-mcp
Fixes a real wedge: resolveEnvAliasValue compared alias values for equality before applying layer precedence, so a stale persisted GH_TOKEN hard-failed the run instead of losing to a rotated GITHUB_TOKEN in the layered secrets files. The fix picks the highest-priority alias first and only errors when a same-priority alias disagrees; cross-layer drift warns and heals, since SecretValues carries the winner for every alias into the persisted store (and Persist defaults on).
Approach and scope look right, one function plus tests plus a doc bullet. make test and make lint are both green here.
Main thing to look at: the three layered secrets files are merged into one map by auth.ResolveLayeredSecrets, so they all share priority: 2, and alias drift between global.env and the project file still hard-fails, the same rotation breakage one level down. Rest of the comments are the docs bullet claiming a stronger rule than the code enforces, layer names in the messages not matching what users see in the docs, and where the new tests live.
| } | ||
| if value, ok := layeredSecrets[envVar]; ok && value != "" { | ||
| values = append(values, aliasValue{envVar: envVar, value: value, source: "secrets", priority: 2, found: true}) | ||
| values = append(values, aliasValue{envVar: envVar, value: value, source: "secrets", priority: 2}) |
There was a problem hiding this comment.
auth.ResolveLayeredSecrets flattens global.env, global/<tool>.env and projects/<hash>/<tool>.env into one map (link), so all three collapse into priority: 2 here. GH_TOKEN left in global.env plus a rotated GITHUB_TOKEN in the project file is then a same-layer conflict and still hard-fails, which is the same rotation breakage one level down. Worth a follow-up, since the file order already has a documented precedence.
| continue | ||
| } | ||
| if value.priority == chosen.priority { | ||
| return "", "", false, fmt.Errorf("secret %q has conflicting values across env aliases (%s vs %s, both set in the %s layer)", secret.ID, chosen.envVar, value.envVar, chosen.source) |
There was a problem hiding this comment.
The check only enforces agreement inside the winning layer, not inside every layer: with three or more aliases, two conflicting values sitting in a non-winning layer only warn. That is fine as a rule (the persisted layer must be allowed to drift so it can heal), but the docs bullet in this PR states the stronger version.
| if value.priority == chosen.priority { | ||
| return "", "", false, fmt.Errorf("secret %q has conflicting values across env aliases (%s vs %s, both set in the %s layer)", secret.ID, chosen.envVar, value.envVar, chosen.source) | ||
| } | ||
| logx.Warnf("Secret %s: using %s from the %s layer; %s in the %s layer holds a different value and is ignored.", secret.ID, chosen.envVar, chosen.source, value.envVar, value.source) |
There was a problem hiding this comment.
"the secrets layer" and "the persisted layer" are internal source ids, not names users see anywhere. docs/auth.md calls them layered secrets files (three of them) and the host-side env store (link). Same for the error above, which asks the user to fix a conflict without saying which file to open.
| an API key). | ||
| - Env aliases name one value. Precedence: host env, layered secrets files, | ||
| persisted store; the winner is injected into *every* alias, so a stale | ||
| persisted value heals after one run. Aliases set in the *same* layer with |
There was a problem hiding this comment.
This is stronger than the code: same-layer disagreement is only fatal in the layer that wins, elsewhere it warns. Also this bullet documents behavior in the middle of a list where every other bullet documents a credentials.sources.<id> key, and the user-visible half (which error, which warning) fits better under ## Secrets in docs/auth.md where the layer precedence is already spelled out (link).
| // A stale persisted alias must lose to the layered secrets files rather than | ||
| // wedge the run, which is the drift that broke token rotation. Every alias then | ||
| // carries the winning value, so the persisted store heals. | ||
| func TestInjectDeclaredSecretsPrefersLayeredSecretsOverStalePersistedAlias(t *testing.T) { |
There was a problem hiding this comment.
Both new tests go through injectDeclaredSecrets, but the resolution tests live in active_secrets_test.go (link). A table test straight on resolveEnvAliasValue would cover env-beats-secrets, secrets-beats-persisted and the non-winning-layer cases for almost nothing. The integration test here is still worth keeping since it is the one that proves every alias gets the winner.
2 participants
What it does
Aliases were compared for equality before layer precedence was applied, so a stale value in the persisted env store hard-failed the run instead of losing to a rotated value in the layered secrets files. Rotating a token that is declared under two aliases (GH_TOKEN/GITHUB_TOKEN) wedged every project whose persisted store still held the old value.
The highest-priority layer now wins outright. Only aliases resolved from the same layer must agree, since that is the one case the user has to fix; the error names the layer. Cross-layer drift is warned about and heals on the next run, because the winning value is injected into every alias and written back to the persisted store.
How to test
Change your GH_TOKEN/GITHUB_TOKEN to a temporary value (if it was set beforehand) and see that the build succeeds, beforehand it would fail with an error.
Follow-ups
Breaking changes
Review checklist