fix(secretsbus): carry PP CLI config.toml as a file instead of parsing it as dotenv - #118
Merged
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 SummaryThe PR moves auto-discovered Printing Press CLI TOML configurations from env-file parsing to verbatim file carriage and adds an explicit command for linking materialized configurations into the locations consumed by those CLIs.
Confidence Score: 5/5The PR appears safe to merge because both previously reported containment failures are fixed and no blocking failure remains. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| internal/secretsbus/linkconfigs.go | Adds home- and config-root-anchored planning and application of configuration links, addressing both previously reported symlink escape paths. |
| internal/secretsbus/pp_cli_adapter.go | Changes derived PP CLI manifests to carry credential-bearing TOML files verbatim rather than exposing them to dotenv parsing. |
| internal/secretsbus/discover_merge.go | Quietly suppresses missing carriage sources only for convention-derived PP CLI manifests while retaining errors for explicit manifests. |
| internal/cli/secret.go | Adds the dry-run-by-default secret link-configs command and delegates filesystem safety decisions to the secretsbus implementation. |
| internal/secretsbus/linkconfigs_test.go | Covers existing destinations, parent and config-root symlinks, post-planning replacements, internal config-root links, and containment races. |
| internal/secretsbus/discover_merge_files_test.go | Adds end-to-end coverage for configured, preference-only, and never-authenticated PP CLI shapes. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Printing Press metadata] --> B{Sensitive key declared?}
B -->|No| C[Skip carriage]
B -->|Yes| D[Carry config.toml verbatim]
D --> E[Materialize under ~/.agentcookie]
E --> F[Plan link-configs]
F --> G{Destination safe and absent?}
G -->|No| H[Refuse or report already linked]
G -->|Yes, with --apply| I[Create link through config-root os.Root]
Reviews (4): Last reviewed commit: "docs: note that ~/.config is opened thro..." | Re-trigger Greptile
Co-authored-by: Matt Van Horn <mvanhorn@users.noreply.github.com>
…ing the leaf classifyDestination inspected only ~/.config/<slug>/config.toml. That leaf reads as absent just as readily when ~/.config/<slug> is a symlink into another tree, so the MkdirAll and Symlink that followed walked the symlink and left the carried config outside ~/.config while --apply reported success. The plan printed a path the bytes never reached. Route every lookup and both writes through an os.Root anchored at ~/.config, which refuses to follow a symlink whose target leaves that tree rather than redirecting the write. A symlinked ~/.config itself is still honored -- the CLI reads through it too, and pointing the config tree at a dotfiles checkout is a normal arrangement -- but only while it stays inside the home directory. Because the write path no longer depends on the classification being right, re-classifying before writing is now a source of actionable messages rather than the only barrier; a parent symlink swapped in between plan and apply is refused by the root itself. ApplyConfigLinks also re-validates the slug and requires the link source to be under ~/.agentcookie/, so a hand-built plan cannot aim the one privileged write somewhere else. Tests cover the escaping parent (absolute and ../-climbing), the swap between plan and apply, a config root pointed out of the home directory, a hand-built plan with a foreign source or a traversal slug, and the dotfiles arrangement that must keep working. All six refusal tests fail against the previous classification. Co-authored-by: Matt Van Horn <mvanhorn@users.noreply.github.com>
…angelog The command help and spec §7.4 both described the refusal as being about the destination's last component, which is the gap the previous classification actually had. Say that the whole path is covered, and add the changelog entry for carriage and link-configs that this branch never wrote. Co-authored-by: Matt Van Horn <mvanhorn@users.noreply.github.com>
Greptile caught a race left by the previous commit. openConfigRootForWrite checked whether ~/.config existed, created it, tolerated EEXIST, and then opened the path by name. A symlink planted in that window made the tolerated EEXIST and the open disagree: os.OpenRoot followed the symlink, and every link afterwards was contained relative to a root outside the home directory while being reported as applied. Resolving a path by name and then opening the result cannot be made safe -- they are separate lookups. So ~/.config is now opened through an os.Root anchored at the home directory, which makes the containment decision and the open the same operation, and a tolerated EEXIST sends the loop back through classification instead of to an open by name. os.Root rejects an absolute symlink even when its target is inside the root, so `ln -s ~/dotfiles/config ~/.config` needed handling: such a target is rewritten as a home-relative path and the open retried, keeping resolution inside the same confined walk. filepath.EvalSymlinks is now used only to recognize the home directory reached by another name (/var vs /private/var), never to pick what to open. The window has no single-threaded state that reproduces it, so the new test interleaves ~/.config appearing and disappearing against ApplyConfigLinks and asserts nothing lands outside the home directory. It cannot fail on a correct implementation; it failed 8 of 8 runs against the previous commit. The dotfiles test now covers both spellings of the symlink. Co-authored-by: Matt Van Horn <mvanhorn@users.noreply.github.com>
Co-authored-by: Matt Van Horn <mvanhorn@users.noreply.github.com>
shawnhamby
added a commit
to shawnhamby/agentcookie
that referenced
this pull request
Sep 12, 2026
….toml) Brings in upstream 994abf2 (fan out cookie and secret sync to multiple sinks, mvanhorn#122) and d1827cc (carry PP CLI config.toml as a file instead of parsing it as dotenv, mvanhorn#118). The only content conflict was internal/config/config.go, in the SourceConfig struct: the fork added EnabledProducts there while upstream added the Sinks fan-out list and made the legacy scalar Sink omitempty. Resolved by keeping both with their own comments and yaml/json tags -- upstream's Sinks/Sink pair and legacy single-sink synthesis are taken verbatim, and the fork's EnabledProducts sits after Browser, ahead of Peer and Security, so DefaultEnabledProducts and ResolveEnabledProducts keep their meaning. Multi-sink repeats only sealing and transport, so it does not re-open extra-profile discovery: internal/chromepaths and the fork's admission rule are untouched by the merge. go build, go vet, and go test -race are green across the module.
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
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.
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