Skip to content

reconcile [o]verride writes ${env:…}/${secret:…} literally into the live destination #239

Description

@spxrogers

Found while planning #229. Out of scope there — that issue is a refactor of the drift walk, and this is upstream of it, in how reconcile constructs its plan.

This is not a secret leak. It is the inverse: the destination gets the template text rather than the resolved value. Nothing sensitive is written that was not already in the canonical source. The damage is a broken destination config, not a disclosure.

What happens

reconcile builds its plan from the templated canonical:

// internal/cli/reconcile.go:156
plan, err := render.Plan(secrets.ForRender(c), reg, agents, sc, projectRoot, s, userHome)

secrets.ForRender is a bare wrapper — it performs no substitution:

// internal/secrets/resolved.go
func ForRender(c source.Canonical) Resolved { return Resolved{c: c} }

So every op.Content in that plan still carries ${env:MY_TOKEN} / ${secret:github.token} verbatim. Choosing [o]verride on a drifted item then hands those ops straight to the adapter's Apply:

// internal/cli/reconcile.go:437-440
rw := render.NewWriter(s, home, userHome, sc, projectRoot, name)
if err := a.Apply(ops, rw); err != nil { … }

The destination — e.g. a live ~/.claude.json — receives the literal ${env:MY_TOKEN}. A subsequent agentsync apply rewrites it correctly, so the corruption is transient in the happy path, but until then the agent reads an unresolvable placeholder as its credential, and RecordOpsState records state for the templated content.

Why the other surfaces do not have this

apply plans from the resolved model:

surface plan input
apply (apply.go:143, :212) resolvedsecrets.SubstituteCanonical
status (status.go:183-188) ForRender, then SubstituteCanonical
explain (explain.go:239-241) ForRender, then SubstituteCanonical
diff (diff.go:102) ForRender only — but diff never writes, and it has a fail-closed UnresolvedSecretRefs gate
reconcile (reconcile.go:156) ForRender only — and it writes

reconcile is the only surface that both plans from templated content and writes to a destination. Its canMask check at :130 computes UnresolvedSecretRefs but uses it for masking display output, not for gating the write.

Why it is not a five-line fix

Switching :156 to SubstituteCanonical makes the override write correct values, but it also puts resolved cleartext into the items reconcile displays and diffs — which is exactly what the masking path at :130 exists to prevent, and that path is currently allowed to degrade (canMask == false) rather than fail closed. Getting this right means deciding:

  • whether the override write and the display/diff should plan from different models (resolved for the write, templated for display), and
  • whether reconcile should refuse outright when UnresolvedSecretRefs is non-empty and an override is queued, the way diff refuses.

Both interact with the secret-handling invariants in CLAUDE.md and deserve their own review rather than a drive-by change inside a refactor.

Suggested test

A reconcile --auto-override run against a canonical carrying a ${env:…} reference, asserting the destination ends up with the resolved value and that state records the same. There is no test covering the override write path's content today.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions