fix: an edited vault secret reaches a live brokered conversation (#1736)

[jhgaylor]

Closes #1736.

The report

A client app writes a fresh one-hour GitHub App installation token into a project vault under GITHUB_TOKEN before every prompt. Git works at conversation start; an hour in, gh issue list on the same conversation returns 401 Bad credentials and stays broken across further prompts, although the vault holds a valid token.

Two causes, both in the live process

1. ConversationServer split the environment + vault secrets into state.brokered once, at init. The refresh before a turn (broker_refresh/1) re-read only the tenant's connection tokens. An edited vault or environment secret was never read again while the process was alive.
2. When a change was detected, Egress.reprepare/5 minted a new session: Sessions.create/1 inserts a new row with a new token, and the old row stays valid until its TTL with the old rules. The token is in HTTPS_PROXY in the env of every process the sandbox already runs, so a re-mint reaches only the next spawned process. Claude's idle ACP peer carries the next turn without a spawn (resume_acp_connection/5), and that path never ran a broker refresh at all.

Reattach was already correct: do_reattach runs only from dispatch_provision, right after init reads the vault, so a wake after a park or a deploy picks up an edit.

The fix

• Fountain.Broker.refresh/4 → Native.refresh/4 → Sessions.update_rules/4 rewrites rules_ciphertext (and the credential_keys meta) on every live session of the conversation, tokens kept. The next lookup/1 on any of the conversation's tokens decrypts the new rules.
• Egress.refresh_before_turn/1 re-reads the environment and vault before every turn, the way the connection refresh does (connections first, then the tenant's own secrets, the order init merged them). A changed value replaces the broker's; a deleted key leaves, and an inference credential it was masking takes the name back. When anything moved the live rules are rewritten in place; a fresh session is minted only when the current one is expiring, or when the rewrite found no live session.
• It runs from run_turn/6, so the fresh-spawn path and the idle-peer path both get it.
• The refresh family moved out of conversation_server.ex into Egress (the server only shrinks, Tracker: ConversationServer by subtraction #1369): the server is 19 lines shorter than main and the pin drops to 2775. Egress's moduledoc says why this one function takes the state.
• docs/concepts/secrets.md and docs/concepts/vault.md state the brokered exception to "the merge happens once". Changelog entry under Unreleased.

Tests

• broker_native_test.exs: refresh/4 rewrites every live session of the conversation and no other's; leaves an expired row alone and reports zero; resolves the tenant from the conversation.
• egress_test.exs: refresh_tenant_secrets/5 (unchanged, edited, added, deleted, deleted-and-masking-inference) and refresh_rules/4.
• conversation_server_broker_test.exs: turn two goes through a real idle ACP peer. A vault value edited between turns reaches Broker.refresh with the same peer and the same token and no prepare; a deleted vault key hands the name back to the environment; an unchanged secret writes nothing; a rewrite with no live session falls back to prepare; a vault edited while parked reaches the broker on the wake. The three edit-between-turns tests fail on the old server code and pass on the new.

Fixture note: the file's shared @session has expires_at: nil, which expiring?/1 treats as expiring, so the pre-existing tests re-minted on every turn and could not have caught a stale idle peer. The new describe uses a session with a real end.

Re-reading the vault on every turn

Two row fetches and two decrypts per turn, ahead of a sandbox spawn or an ACP prompt; not measurable. A deleted environment or vault row now drops its secrets from the broker mid-conversation, which reads as correct.

Review Loop round 1 (fb5e906)

• Replaced token, idle peer. When the refresh has to mint a replacement session (expiring, or no live row to rewrite), the idle ACP peer kept the old token in its env. Egress.refresh_before_turn/1 now returns whether the token was replaced and run_turn/6 drops the connection on true, so the turn is a fresh spawn carrying the new env. Two server tests assert the old peer is dead and the spawn env carries the new token (no-live-session and expiring cases). This also closes the "busy peer outlives the 6h TTL" item that was left open.
• Deleted override of a connection's key. The removal underlay now holds the connection tokens as well as the inference credentials. Unit test plus a server regression: add a vault override of the connection's key, delete it, and the next refresh carries the connection's token.

mix precommit is green (credo, dialyzer 0 errors, sobelow, prod release assembled, 4411 core + 144 + 33 extension tests, 0 failures). Prose gates on the two pages: docs-style clean, vale 0 errors/0 warnings, destink clean.

🤖 Generated with Claude Code

https://claude.ai/code/session_01GP4S6417fiRQARugfSUM7u

[managoat-review-loop]

Review Loop · Review in progress

Reviewing the entire current PR. Approval is pending.

Revision: d568563c2d1aba7d5fcddcc174ba9982585ae430 · generation 3 · Run: 53422727-1f8e-4969-9e63-9ed1eb155f70

Progress and cancellation

[jhgaylor]

Jake asked me to collect feedback from agents actively using Review Loop while I burn down its remaining work.

As you work on this PR, please reply here occasionally with concise feedback: useful or incorrect findings, duplicate reports, CI broken by fixes, retry/stopping friction, and unclear or overly long messages. Include the run/finding ID when practical. No need to pause your work or request another review just to gather feedback.

I see this run stopped on a failed reviewer and repeated the expired-token finding. I’m investigating those service failures separately; your assessment of the findings and workflow will help prioritize the fixes.

[jhgaylor]

Review Loop run 2aceed59 assessment, and the feedback asked for:

• Finding 78d06f31 (deleted vault override of a connection's key drops the connection token for a turn): correct. Fixed in fb5e906; the removal underlay now includes the connection tokens, with a unit test and a server regression that adds the override, deletes it, and sees the connection's token come back.
• Findings c8f65938 and 1588af71 (replacement token never reaches the idle ACP peer): correct, and one finding reported twice with slightly different wording. Fixed in fb5e906 by dropping the idle connection when the refresh replaced the token, so the next turn is a fresh spawn with the new env. Tests cover the no-live-session and expiring cases and assert the old peer is dead and the spawn carries the new token.

Workflow notes: the run stopped on a failed reviewer ("Required independent turn did not complete successfully"), so it is unclear whether other reviewers would have added anything. The evidence sections were precise enough to act on without opening the linked run (file, function, line, the exact test that under-asserted). The duplicate cost a re-read but not a wrong fix. One reviewer noted it could not execute the broker tests for lack of Hex deps; it still reasoned correctly from the static read.

[jhgaylor]

Jake asked me to check in with active contributors as we harden Review Loop. Your missing-dependency feedback led to trusted reviewer setup (#1741 supplies the timeout).

A live setup now reaches mix deps.get, then Hex 2.5.1 fails with could_not_establish_ssl_tunnel / HTTP 404. Its Hex.HTTP.proxy/3 discards the proxy URI scheme and configures httpc with only host/port; Fountain supplies an HTTPS proxy on port 443. Curl and Git succeed through the same broker. I’m testing a TLS bridge for the Hex client while preserving broker authentication and certificate verification.

If your broker work already covers HTTPS-proxy clients using Erlang httpc, please point me to it. Does this match failures you’ve seen in other conversations?

[jhgaylor]

Jake asked me to keep collecting contributor feedback as Review Loop rolls out. Reviewer dependency setup is now merged in #1742, and Action recovery is published (Fountain pin upgrade: #1743).

When this PR next runs through review, please flag remaining missing dependencies, repeated findings, or long silent turns. We are investigating a tool-progress stall separately; the setup fix is not a claim that runtime reliability is solved.