fix(secretsbus): carry PP CLI config.toml as a file instead of parsing it as dotenv - #118
fix(secretsbus): carry PP CLI config.toml as a file instead of parsing it as dotenv#118mvanhorn wants to merge 6 commits into
Conversation
…in-place The canonical PP CLI auth location is TOML, but [secrets.file] is an env-shaped slot read by the strict dotenv parser, so every discovered CLI failed to sync: configured ones on a parse error, unconfigured ones as a missing file. Derive a [[files]] carriage item instead, gated on the manifest declaring at least one sensitive key so preference-only configs (espn's [favorites]) are not swept in.
A derived manifest asserts a conventional path, so an absent config.toml just means the CLI was never authenticated. Add ErrCarrySourceMissing so LoadPayloadWithDiscovery can skip that case quietly for derived manifests while a hand-written manifest, whose author chose the path, still errors. Sources that exist but cannot be carried keep erroring either way.
Carried configs materialize under ~/.agentcookie/, but PP CLIs read ~/.config/<cli>/config.toml and only 2 of 59 installed binaries honor XDG_CONFIG_HOME or <API>_CONFIG_DIR. Rather than widen the bus's write authority, linking is an explicit opt-in step: read-only planning, dry run by default, refuses to replace a real config or write through a symlink pointing outside ~/.agentcookie/.
Canonical credential-bearing scaffold, espn-style preference-only config (including a nested table dotenv could never express), and an installed- but-never-authenticated CLI, discovered together. Also pins that v1 still wins per-key over a carried key.
Section 7 mapped the PP adapter to [secrets.file] while section 5.4 already said a TOML config.toml cannot ride as KEY=VALUE. Correct the mapping, add 7.4 explaining the sensitivity gate and why consumption is a separate step, and mark the audit's non-env-shaped-artifact finding as partly addressed.
Derived PP CLI manifests no longer set [secrets.file], so ReadInPlacePath is empty for them. Two consumers assumed otherwise: - secret revoke printed a copy-pasteable silencing manifest containing path = "", which is invalid. The block is unnecessary; drop it. - discover fell back to labelling any empty path '(legacy bus dir)', which is wrong for a carriage-based manifest. Show the carried source instead.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
Greptile SummaryThis PR changes auto-discovered Printing Press CLI configuration syncing from dotenv parsing to verbatim file carriage and adds an explicit command for linking materialized configs into their runtime locations.
Confidence Score: 4/5The config-carriage correction is sound, but the new link command should validate destination parent components before merging because apply mode can write through a symlinked configuration directory. The new privileged filesystem operation checks only the destination leaf, so a symlinked parent can redirect link creation outside the location the command claims to have validated. Files Needing Attention: internal/secretsbus/linkconfigs.go, internal/secretsbus/linkconfigs_test.go
|
| Filename | Overview |
|---|---|
| internal/secretsbus/pp_cli_adapter.go | Correctly replaces env-shaped parsing with gated, non-optional verbatim carriage for sensitive PP CLI configurations. |
| internal/secretsbus/discover_merge.go | Suppresses only the typed missing-source error for convention-derived manifests while retaining other carriage errors. |
| internal/secretsbus/filecarriage.go | Introduces a typed missing-source sentinel so callers can distinguish normal absence from other read failures. |
| internal/secretsbus/linkconfigs.go | Adds explicit config-link planning and application, but its leaf-only destination check permits writes through symlinked parent directories. |
| internal/cli/secret.go | Exposes the link workflow with dry-run default and explicit apply mode; apply errors are reported per entry. |
| internal/secretsbus/linkconfigs_test.go | Covers leaf symlinks, clobber prevention, and happy paths, but omits symlinked destination-parent behavior. |
| internal/secretsbus/discover_merge_files_test.go | Adds end-to-end coverage for configured, preference-only, and unauthenticated PP CLI shapes and verifies byte-preserving carriage. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[PP metadata discovered] --> B{Sensitive key declared?}
B -- No --> C[No carriage item]
B -- Yes --> D[Carry config.toml verbatim]
D --> E[Materialize under ~/.agentcookie/slug]
E --> F[secret link-configs plan]
F --> G{Destination classified safe?}
G -- No --> H[Refuse]
G -- Yes and apply --> I[Create link under ~/.config/slug]
J[Symlinked destination parent] -. currently not inspected .-> I
Reviews (1): Last reviewed commit: "fix(cli): repair two displays that assum..." | Re-trigger Greptile
| if err := os.MkdirAll(filepath.Dir(e.Destination), 0o700); err != nil { | ||
| errs = append(errs, fmt.Errorf("%s: create config dir: %w", e.Slug, err)) | ||
| continue | ||
| } | ||
| if err := os.Symlink(e.Materialized, e.Destination); err != nil { |
There was a problem hiding this comment.
Symlinked parents redirect config links
When ~/.config/<slug> or one of its ancestors is a symlink and the destination leaf is absent, the leaf-only Lstat classifies the path as safe before MkdirAll and Symlink follow that parent, causing --apply to create config.toml in an external directory while reporting success.
Problem
Every auto-discovered Printing Press CLI failed to sync its secrets.
agentcookie source --onceprinted 40secrets-buserrors in two classes:read ~/.config/<cli>/config.toml: line 1: whitespace around '=' is not allowed, ormissing '=' (expected KEY=VALUE)where the file opens with a[table]header. These CLIs had real credentials on disk that silently never reached the sink.read-in-place file missingfor ~26 CLIs that were simply never authenticated. A normal state, reported as an error on every push.Root cause
DeriveManifestFromPPpointed[secrets.file]at~/.config/<cli_name>/config.toml. But[secrets.file]is env-shaped by contract and is read by the strictKEY=VALUEparser, while PP CLIs write actual TOML there. The spec already knew this — §5.4 says "a TOMLconfig.tomlcannot ride as a singleKEY=VALUEvalue" — but §7 mapped the PP adapter to the env-shaped slot anyway. The adapter took the canonical path from the PP audit and did not carry across its format finding.Fix
Derive a
[[files]]carriage item instead — machinery the repo already ships for exactly this case, whose doc example is literally a pp-cliconfig.toml. Bytes are carried verbatim, so nested tables and comments survive; nothing is parsed.config.tomlas a file, gated on the manifest declaring at least one sensitive key so preference-only configs (espn's[favorites]) are not swept inErrCarrySourceMissinglets never-configured CLIs skip quietly, while hand-written manifests (whose author chose the path) still erroragentcookie secret link-configsbridges carried configs into~/.config/Why a link step instead of an env pointer
Carried files materialize under
~/.agentcookie/, but PP CLIs read~/.config/<slug>/config.toml. I measured the installed fleet:XDG_CONFIG_HOME/<API>_CONFIG_DIROnly 2 of 59 installed binaries can follow an env pointer. Writing directly into
~/.config/would reach all 59 but would break the containment invariantvalidateMaterializeTargetexists to enforce — the sink's defense against a manifest naming an arbitrary write path. So linking is a separate explicit step: read-only planning, dry run by default, refuses to replace an existing config or write through a symlink pointing outside~/.agentcookie/.Verification
Measured on a real machine, not just fixtures:
secrets-buserror lines: 40 -> 0[table]headers and comments intactgo build,go vet,gofmtclean;secretsbus115 passingFour tests fail on this branch (
TestInstacartAdapter_IsInstalled_*,TestCheckDaemonBinaryPath) — all four fail identically onmain, verified by checkout.Known limitations
auth_env_varsand noauth_env_var_specs, so they are excluded despite holding access tokens. Not a regression (they did not sync before either), but the fix does not reach them. The gate cannot be loosened, because espn declares nothing either and metadata alone cannot separate "has no secrets" from "did not say." The fix belongs in those CLIs'.printing-press.json.[sync.keys]would. The audit's per-filelocal-onlymarker is the real answer and is still open.cookies.json,browser-session-proof.json) are still uncarried.Review note
Two regressions from this change were caught and fixed before this PR:
secret revokeemitted an invalidpath = ""in its copy-pasteable silencing manifest, anddiscovermislabelled carriage-based manifests as(legacy bus dir). Both stemmed from consumers assuming a non-emptyReadInPlacePath.Plan:
docs/plans/2026-08-13-2206-fix-pp-cli-toml-secrets-carriage-plan.md🤖 Generated with Claude Code
https://claude.ai/code/session_015AGvauBWJUf3EyrHSotKdL