feat(api): bulk apply reconciles Teammate, Schedule and Webhook documents - #1675
feat(api): bulk apply reconciles Teammate, Schedule and Webhook documents#1675lex00 wants to merge 8 commits into
Conversation
lex00
added
enhancement
Review Loop · Human review neededRevision: Run deadline exhausted; work remains incomplete
and env ! my-project: apply failed on stderr and exit status 1, against a manifest that applied cleanly and a server that wrote nothing wrong. The Swift SDK is unaffected (sdk/swift/.../Account.swift declares
Usage: 101034 reported tokens. Approval does not merge the PR. Maintainers can post new top-level PR comments: |
![managoat-review-loop[bot]](https://avatars.githubusercontent.com/u/321987611?s=60&v=4){kind=small}
| # Only once the new binding is the committed one: a machine torn down | ||
| # against a write that then failed would be rebuilt for nothing. | ||
| # Ownership: established above, by the scoped get_teammate. | ||
| _ = Conversations._unsafe_retire_orphaned_homes(orphans, "teammate_rebound", opts) |
There was a problem hiding this comment.
high · fix · qa-team
Evaluated revision: 7afa4ef65fd0eecd94c4409e999b2ced1a43fdd3 · Finding: c31f87205fcabf9da98d7b78ab377317422e29a215701b34a82cc69c2da8954b
Rebinding one teammate leaves its sandbox_id pointing to the retired shared home. If another live conversation shares that home, the next wake moves both conversations onto whichever environment/vault the waking conversation names. Thus waking the teammate first moves the other conversation onto the new binding; waking the other conversation first moves the teammate back onto the old binding. The database environment_id/vault_id fields remain different from the machine actually used. This can expose the wrong environment files and vault material to a conversation.
Evidence
write_bindings/5 updates only the teammate conversation and calls reset_sandbox through _unsafe_retire_orphaned_homes. reset_sandbox preserves all conversations and their sandbox_id. Team.send_message/5 resumes these live conversations through ConversationServer.send_prompt. Conversations.create_fresh_sandbox_and_start/4 builds the replacement from the waking conversation environment/vault and then unconditionally calls move_cotenants/3 (conversations.ex:3299). That function updates every other live cotenant sandbox_id without comparing bindings (3355-3382). The new retirement test checks only that the old sandbox terminates, with no second conversation or subsequent wake.
Suggested remediation
In apps/fountain/lib/fountain/conversations.ex, restrict replacement cotenant migration to conversations whose effective environment and vault match the replacement sandbox identity; leave conversations with a different identity for their own wake. Add a regression scenario with two idle conversations sharing a home, rebind one, and wake them in both orders, asserting each uses its declared identity.
Disposition
Blocking finding requires remediation
Human replies stay open until addressed. Resolving a conversation alone does not approve the PR.
Maintainers: post a new top-level PR comment /review-loop resolve e1ee3a9d-7d18-43de-833f-2446d4273f43 c31f87205fca reason or /review-loop reject-fix e1ee3a9d-7d18-43de-833f-2446d4273f43 c31f87205fca reason. Then retry the ended run with /review-loop retry e1ee3a9d-7d18-43de-833f-2446d4273f43.
| do: {:ok, conv, :unchanged} | ||
|
|
||
| defp write_bindings(user_id, conv, changes, orphans, opts) do | ||
| case Conversations.update_conversation(conv, changes) do |
There was a problem hiding this comment.
medium · needs_human · qa-team
Evaluated revision: 7afa4ef65fd0eecd94c4409e999b2ced1a43fdd3 · Finding: 0bfeb7943fa6d1bd1f5b97a17e9c0c153199f2b87f540cafe1f6907b76a144f8
When the same agent already has a live persistent home for the requested destination environment/vault, applying the teammate binding reports updated and retires its current home, but subsequent messages cannot wake the teammate. The wake path tries to insert another home for the destination identity instead of attaching to the existing one, and the unique home constraint rejects it. Repeating the manifest reports unchanged and does not recover the teammate.
Evidence
An agent can have homes for different environment/vault pairs (_unsafe_find_home/4). update_teammate changes the conversation binding but retains sandbox_id. After retirement, Team.send_message uses wake_conversation for the still-live conversation. create_fresh_sandbox_and_start/4 inserts a persistent sandbox with the new environment/vault (conversations.ex:3257-3271), without looking up an existing destination home or recovering a home uniqueness conflict. Sandbox.changeset/2 enforces sandboxes_home_identity_index. The added tests never create a destination home and never send a message after rebinding.
Suggested remediation
Define and implement destination-home handling before accepting a rebind: either attach to the existing home using the normal ownership, readiness and quota checks, or refuse the rebind before mutating anything when that destination is occupied. Reuse preserves the persistent-home contract; refusal is simpler but restricts valid bindings. This needs maintainer choice and coordinated lifecycle regression coverage beyond a narrow automatic fix. Test rebinding with both source and destination homes already present, then sending a message.
Disposition
Reviewer requests a human decision
Human replies stay open until addressed. Resolving a conversation alone does not approve the PR.
Maintainers: post a new top-level PR comment /review-loop resolve e1ee3a9d-7d18-43de-833f-2446d4273f43 0bfeb7943fa6 reason or /review-loop reject-fix e1ee3a9d-7d18-43de-833f-2446d4273f43 0bfeb7943fa6 reason. Then retry the ended run with /review-loop retry e1ee3a9d-7d18-43de-833f-2446d4273f43.
There was a problem hiding this comment.
high · fix · security-audit
Evaluated revision: 7afa4ef65fd0eecd94c4409e999b2ced1a43fdd3 · Finding: fe815dae40a8ea57431a08dd7b7ab09415076e9e624ef2f9805abec05f9e560a
The newly supported Webhook kind lets a sprite- or principal-scoped token perform webhook management that the dedicated webhook API reserves for full-scoped keys. Untrusted sandbox code, including code induced by prompt injection, can POST a Webhook document to /api/apply with an attacker-controlled HTTPS URL and event_types ["*"]. This creates an account-wide subscription and returns its signing secret. Subsequent conversation and agent IDs, parent IDs, turn IDs, lifecycle states, timestamps and durations leave the tenant through that endpoint. Delivery persists after the originating sandbox token expires or is revoked; the endpoint has no dependency on that credential. The payload does not include transcript or environment secret values.
Evidence
router.ex routes /api/apply through accepts_json + api (lines 485-486, 530), whereas /api/webhooks explicitly uses require_full_scope (lines 292-293). TenantAPIAuth assigns the authenticated key but does not enforce full scope. ApplyController.create passes only user.id and audit attribution into Manifest; apply_webhook and reconcile_webhook call Webhooks.create_endpoint/update_endpoint without checking the credential. ApiKey.may_manage_keys?/1 permits only full; sprite and principal are excluded. Webhooks.conversation_and_endpoints joins all active endpoints by conversation user_id, and WebhookDelivery.perform checks endpoint existence/status only, not the originating key. Static trace establishes the bypass; attempted targeted tests could not start because Hex/dependencies are absent. Runtime denied-path and second-tenant checks therefore remain unexecuted.
Suggested remediation
In apply_controller.ex, before calling Manifest.apply_manifest, enforce the existing RequireFullScope policy when the submitted resources include a Webhook document, and stop on a halted connection. Keep manifests containing only the existing ordinary resource kinds accessible under their existing policy. This restores the dedicated webhook API boundary within one permitted file. Verify sprite and principal requests are denied before any writes, full-scoped creation succeeds, and a second tenant cannot update the first tenant's endpoint even when using the same URL.
Disposition
Blocking finding requires remediation
Human replies stay open until addressed. Resolving a conversation alone does not approve the PR.
Maintainers: post a new top-level PR comment /review-loop resolve e1ee3a9d-7d18-43de-833f-2446d4273f43 fe815dae40a8 reason or /review-loop reject-fix e1ee3a9d-7d18-43de-833f-2446d4273f43 fe815dae40a8 reason. Then retry the ended run with /review-loop retry e1ee3a9d-7d18-43de-833f-2446d4273f43.
| action: %Schema{type: :string, enum: ["created", "updated", "error"]}, | ||
| action: %Schema{ | ||
| type: :string, | ||
| enum: ["created", "updated", "unchanged", "error"], |
There was a problem hiding this comment.
high · needs_human · product-api
Evaluated revision: 7afa4ef65fd0eecd94c4409e999b2ced1a43fdd3 · Finding: 3e2bff5d2e0c897c98a52e3b35f5764c88da84e4a81e9778f5e6fead441a4d8d
POST /api/apply now returns action="unchanged" for any row whose record already matched the document, and by design that is the common case: an idempotent re-apply returns "unchanged" for every row (apps/fountain/test/fountain_web/controllers/apply_controller_test.exs asserts List.duplicate("unchanged", 6), and docs/cli.md now says "A second apply of an unchanged manifest reports unchanged for every row"). The only shipped client that branches on this value is the Go CLI, and every already-released fountain binary has no "unchanged" case. Adding a value to a response enum is a distinct contract from adding an optional field: the new
secretproperty is ignored safely by old decoders, but a widened enum is not. The contract and conformance suites cannot catch this — CONTRIBUTING.md says both "compare a schema with another schema", and the regenerated sdk/contract/contract.json plus sdk/typescript/src/generated/openapi.ts agree with the server precisely because they were regenerated from it. Nothing in this PR (CHANGELOG.md, docs/cli.md, docs/api.md, apps/fountain/priv/help/manifest.md) tells an operator that the released CLI must be upgraded before the server ships.
Evidence
The call shape is
renderApplyResultsin the base CLI,git show 539a300:cli/internal/cmd/apply.golines 145-163:switch r.Action {
case "created": fmt.Printf("%s + %s\n", label, r.Name)
case "updated": fmt.Printf("%s ~ %s\n", label, r.Name)
default:
anyFailed = true
warnf("%s ! %s: %s", label, r.Name, formatResultErrors(r.Errors))
}and
runApply(cli/internal/cmd/apply.go:68-70) doesif renderApplyResults(results) { os.Exit(1) }.formatResultErrorsreturns the literal string "apply failed" whenerrsis empty, and an unchanged row carrieserrors: nil(Fountain.Manifest.result/6, apps/fountain/lib/fountain/manifest.ex:562). Server side,verdict/2(apps/fountain/lib/fountain/manifest.ex:549-555) returns:unchangedwhenever the schema fields minus timestamps compare equal, and ApplyJSON renders it verbatim (apps/fountain/lib/fountain_web/controllers/apply_json.ex:12). So an operator on a pre-#1636 binary running the pipeline that docs/cli.md and apps/fountain/priv/help/manifest.md both recommend ("keep fountain.yml in source control, run fountain apply -f fountain.yml from your deploy pipeline") gets, on the second and every later run:env ! my-project: apply failed
vault ! alice: apply failed
agent ! researcher: apply failedon stderr and exit status 1, against a manifest that applied cleanly and a server that wrote nothing wrong. The Swift SDK is unaffected (sdk/swift/.../Account.swift declares
actionas a plain String), and the TypeScript change is types only, which is why the CLI is the whole blast radius.
Suggested remediation
This is a product decision the approved base does not settle, so it needs the maintainer rather than an automatic fix. Three alternatives: (1) ship the widened enum and state the incompatibility explicitly — a CHANGELOG note under a Changed/Breaking heading and a line in docs/cli.md saying a fountain binary older than this release reports every unchanged row as a failure and exits nonzero — and sequence the CLI release ahead of the server deploy; (2) keep the wire value at "updated" unless the caller opts in, e.g. an
unchanged: truefield on ApplyRequest or an explicit client-capability header, so old binaries keep the behavior they were built against and the new CLI gets the=output; (3) narrow the blast radius by having the server emit "unchanged" only for the three new kinds, which no released CLI can send. Whichever is chosen, say it in the release notes: the two schema-to-schema gates will stay green either way.
Disposition
Reviewer requests a human decision
Human replies stay open until addressed. Resolving a conversation alone does not approve the PR.
Maintainers: post a new top-level PR comment /review-loop resolve e1ee3a9d-7d18-43de-833f-2446d4273f43 3e2bff5d2e0c reason or /review-loop reject-fix e1ee3a9d-7d18-43de-833f-2446d4273f43 3e2bff5d2e0c reason. Then retry the ended run with /review-loop retry e1ee3a9d-7d18-43de-833f-2446d4273f43.
| Exception.format(:error, error, __STACKTRACE__) | ||
| ) | ||
|
|
||
| {result(kind, str(name), :error, %{"base" => [Exception.message(error)]}, []), nil} |
There was a problem hiding this comment.
medium · fix · xp-reviewer
Evaluated revision: 7afa4ef65fd0eecd94c4409e999b2ced1a43fdd3 · Finding: d77a775fcd08c26ef45f784146924c0e50b8681a6e0dcbf1d3c73c1a7d9641be
guarded/3already logs the full exception and stacktrace, which is the useful record. It then also putsException.message(error)(rescue branch) andinspect(reason)(catch branch) verbatim into the result row'serrors, andFountainWeb.ApplyJSON.result/1serializeserrorsstraight into the 200 response, whichfountain applythen prints. That turns any unexpected crash anywhere in a pass into client-visible internal detail of unbounded size. It also weakens an invariant this very module states and tests:apply_manifest/3's docs say "Secret values are never echoed back", andapply_controller_test.exsasserts it — but the Environment and Vault passes handle plaintext secret values insideguarded/3, and Elixir's own messages forMatchError,CaseClauseError,BadMapErrorandArgumentErrorembed the offending value. The guard itself is well motivated (a crash after environments/vaults/agents are committed would otherwise hand the caller a 500 and no results); only the choice to forward the text is the problem.
Evidence
apps/fountain/lib/fountain/manifest.ex:151-166 builds the error map from
Exception.message(error)/inspect(reason)immediately afterLogger.error(...)has already captured the same information. apps/fountain/lib/fountain_web/controllers/apply_json.ex:15 renderserrors: result.errorsunmodified. apps/fountain/lib/fountain/manifest.ex:81 (Secret values are never echoed backin the moduledoc, retained by this PR) and apps/fountain/test/fountain_web/controllers/apply_controller_test.exs:"never echoes secret values back" state the invariant, which only holds on the non-raising paths. apps/fountain/test/fountain/manifest_test.exs asserts the leaked text is public:assert errors == %{"base" => ["the sandbox provider fell over"]}.
Suggested remediation
In
guarded/3keep the twoLogger.errorcalls exactly as they are and return a fixed, non-reflective message in the result row instead of the raised text — e.g.%{"base" => ["apply failed unexpectedly; see the server log"]}for both branches. Update the one manifest_test assertion that pins the raised string to assert the fixed message instead. Single file plus its test; the documented behaviour (the row fails, the request still returns 200 with the other rows) is unchanged.
Disposition
Blocking finding requires remediation
Human replies stay open until addressed. Resolving a conversation alone does not approve the PR.
Maintainers: post a new top-level PR comment /review-loop resolve e1ee3a9d-7d18-43de-833f-2446d4273f43 d77a775fcd08 reason or /review-loop reject-fix e1ee3a9d-7d18-43de-833f-2446d4273f43 d77a775fcd08 reason. Then retry the ended run with /review-loop retry e1ee3a9d-7d18-43de-833f-2446d4273f43.
| case Conversations.update_conversation(conv, changes) do | ||
| {:ok, updated} -> | ||
| fields = for {key, field} <- @bindings, Map.has_key?(changes, field), do: key | ||
| record(user_id, "team.updated", updated, opts, %{"fields" => fields}) |
There was a problem hiding this comment.
medium · needs_human · xp-reviewer
Evaluated revision: 7afa4ef65fd0eecd94c4409e999b2ced1a43fdd3 · Finding: 6dfc40754c14bd8b432b7825384414c2cc2e6ff468f3c1bf2a69ee608c537a1e
update_teammate/4writes the same column, with the same blank-trimming and the same "only if it actually moved" rule, as the existingrename_teammate/4: both set the teammate conversation'stitleand both record metadata%{"fields" => ["name"]}. They disagree only on the action string. A teammate renamed throughPATCH /api/team/:agent_idrecordsteam.renamed; the identical rename applied from aTeammatedocument recordsteam.updated. ADR 0013 callsaction"the trail's only groupable column" and notesFountain.Analyticsmirrors it through, so an operator or dashboard filtering onteam.renamedsilently misses every apply-driven rename — the exact door-dependent coverage gap CLAUDE.md's audit section says the #540 campaign existed to remove. The newteam.updatedaction is justified for the environment/vault bindings, whichrename_teammate/4does not cover; it is only the overlappingnamefield that is now recorded two ways, along with two copies of the write-and-detect-a-real-change logic.
Evidence
apps/fountain/lib/fountain/team.ex:422-440 (
rename_teammate/4:blank_to_nil,Conversations.update_conversation(conv, %{"title" => title}),if updated.title != conv.title,record(user_id, "team.renamed", updated, opts, %{"fields" => ["name"]})) versus apps/fountain/lib/fountain/team.ex:504-596 (update_teammate/4:@bindingsmaps"name" -> :title,binding_changes/2applies the sameblank_to_niland the same reject-if-equal,record(user_id, "team.updated", updated, opts, %{"fields" => fields})). Both then callbroadcast_changed(user_id).Fountain.Manifestis the only non-test caller ofupdate_teammate/4(apps/fountain/lib/fountain/manifest.ex:321), and apps/fountain/test/fountain/manifest_test.exs exercises a rename-only apply that lands onteam.updated.
Suggested remediation
A maintainer decision, because either direction moves a recorded action string. Option A: have
update_teammate/4emitteam.renamedwhenchangesis exactly%{title: _}andteam.updatedotherwise, so the existing action keeps its meaning and no route's trail changes — smallest change, but two action strings for one function. Option B: makerename_teammate/4delegate toupdate_teammate(user_id, agent_id, %{"name" => name}, opts)so there is one write path — cleaner, but it retiresteam.renamedfromPUT/PATCH /api/team/:agent_idand fromteam_controller.ex:95's documented behaviour, which is a visible change to the audit trail and to analytics grouping. Option C: accept the split and document it. Whichever is chosen, fold the duplicated title-write into one place and updateaudit_guardrail_test.exs,team_test.exsandteam_controller_test.exsaccordingly.
Disposition
Reviewer requests a human decision
Human replies stay open until addressed. Resolving a conversation alone does not approve the PR.
Maintainers: post a new top-level PR comment /review-loop resolve e1ee3a9d-7d18-43de-833f-2446d4273f43 6dfc40754c14 reason or /review-loop reject-fix e1ee3a9d-7d18-43de-833f-2446d4273f43 6dfc40754c14 reason. Then retry the ended run with /review-loop retry e1ee3a9d-7d18-43de-833f-2446d4273f43.
| {:ok, vault_id} <- resolve_ref("vault", user_id, spec["vault"], refs.vaults) do | ||
| attrs = %{"name" => name, "environment_id" => env_id, "vault_id" => vault_id} | ||
| reconcile_teammate(user_id, name, agent_id, attrs, opts) | ||
| else |
There was a problem hiding this comment.
low · needs_human · xp-reviewer
Evaluated revision: 7afa4ef65fd0eecd94c4409e999b2ced1a43fdd3 · Finding: ae8e3540572f841e4006a972be0879bf703c8f547bf3c0589db77d1719e21c6b
The PR adds
unclaimed/3for Teammate documents and explains exactly why: two documents that describe one record make every pass rewrite the other's, so no apply is everunchanged. A Schedule is keyed by name under its teammate and has the same property, but no equivalent guard. Two Schedule documents with the samemetadata.nameand the sameteammate— a plausible copy-paste in a hand-written manifest — resolve to the same row: the first document creates or updates it, the second immediately updates it back, and the same flip-flop repeats forever. Every CI apply then reportscreated/updatedrather thanunchangedand writes twoteam.schedule.updatedaudit rows, which is precisely the audit noise this PR set out to remove in its### Fixedentry. Nothing is corrupted and the row converges to whichever document is last, so the cost is a permanently noisy trail and an apply that never reports a clean state.
Evidence
apps/fountain/lib/fountain/manifest.ex:283-296 (
unclaimed/3, with the comment "without this the second renames the first's conversation on every pass and the manifest is never idempotent") has no counterpart inapply_schedule/4at apps/fountain/lib/fountain/manifest.ex:328-341.reconcile_schedule/5(manifest.ex:343-357) finds the row withSchedules.list_schedules(user_id, agent_id) |> Enum.find(&(&1.name == name)), so both documents select the same schedule.reconcile/3(manifest.ex:136-144) does pass aclaimedmap to every callback, butapply_schedule/4ignores it andreconcile_schedule/5returnsnilas the id, so nothing is ever claimed for this kind. apps/fountain/test/fountain/manifest_test.exs covers the duplicate-Teammate case ("a manifest with a duplicate Teammate is still idempotent for the rest") and has no duplicate-Schedule case.
Suggested remediation
A maintainer decision on the intended semantics, and the reason it is not a one-liner: a Schedule's real key is the pair
(teammate, name), not the document name thatreconcile/3claims by, so two same-named schedules under different teammates are legitimate and must keep working. Option A (matches the Teammate precedent): haveapply_schedule/4take theclaimedargumentreconcile/3already hands it, key it on{agent_id, name}rather than the document name, and refuse the second document with%{"name" => ["is already used by another Schedule document for this teammate"]}; this needsreconcile/3's accumulator to carry that pair, which fits inside manifest.ex. Option B: declare last-write-wins for schedules, leave the code as is, and say so in the Teammate/Schedule section of apps/fountain/priv/help/manifest.md so a reader is not led by the Teammate rule to expect a refusal. Either way, add a duplicate-Schedule case beside the existing duplicate-Teammate test.
Disposition
Reviewer requests a human decision
Human replies stay open until addressed. Resolving a conversation alone does not approve the PR.
Maintainers: post a new top-level PR comment /review-loop resolve e1ee3a9d-7d18-43de-833f-2446d4273f43 ae8e3540572f reason or /review-loop reject-fix e1ee3a9d-7d18-43de-833f-2446d4273f43 ae8e3540572f reason. Then retry the ended run with /review-loop retry e1ee3a9d-7d18-43de-833f-2446d4273f43.
| changeset | ||
| |> Repo.update() | ||
| |> audited("environment.updated", merge_metadata(opts, Audit.changed_fields(changeset))) | ||
| result = Repo.update(changeset) |
There was a problem hiding this comment.
low · needs_human · xp-reviewer
Evaluated revision: 7afa4ef65fd0eecd94c4409e999b2ced1a43fdd3 · Finding: 2f23c82e473f28ea47a614cb5caf69b0d8d80414a34eaa549d60f53f0ff94b55
The behaviour change is right and matches CLAUDE.md's "Only record what happened ... a no-op sync records nothing", and the CHANGELOG documents it. The shape is the concern:
if changeset.changes == %{}is now written out four times in four contexts, each with a comment pointing atFountain.Environments.update_environment/3as the canonical copy, andFountain.Team.Schedules.update_schedule/3already carried a fifth. Three of the four are structured asresult = Repo.update(...)then an if/else that returnsresultoraudited(result, ...), and the fourth (webhooks) as a bareifinside acase, so the same rule reads differently in each place. The next context to gain an update function has nothing to call and will copy a fifth variant, and there is no single place a reader can check which contexts observe the rule.
Evidence
apps/fountain/lib/fountain/environments.ex:135-152, apps/fountain/lib/fountain/vaults.ex:109-120, apps/fountain/lib/fountain/agents.ex:186-193 and apps/fountain/lib/fountain/webhooks.ex:123-135 all guard on
changeset.changes == %{}(or!= %{}); three of them carry the comment "A save that moves nothing records nothing, the same ruleFountain.Environments.update_environment/3follows (#1636)". apps/fountain/lib/fountain/team/schedules.ex:122 has the pre-existingif changeset.changes != %{}form.
Suggested remediation
Add one helper next to the audit plumbing — e.g.
Fountain.Audit.changed?/1or anaudited_if_changed(result, changeset, action, opts)shape each context's privateaudited/3can wrap — and have the five call sites use it, so the rule and its comment live in one place. This spans five files and so exceeds the one-file fix budget; it is also worth a maintainer's call whether the helper belongs inFountain.Auditor stays as a per-context idiom, given each context'saudited/3is private and resource-typed.
Disposition
Reviewer requests a human decision
Human replies stay open until addressed. Resolving a conversation alone does not approve the PR.
Maintainers: post a new top-level PR comment /review-loop resolve e1ee3a9d-7d18-43de-833f-2446d4273f43 2f23c82e473f reason or /review-loop reject-fix e1ee3a9d-7d18-43de-833f-2446d4273f43 2f23c82e473f reason. Then retry the ended run with /review-loop retry e1ee3a9d-7d18-43de-833f-2446d4273f43.
| // A webhook endpoint's signing secret comes back on the apply that | ||
| // created it and never again, so print it where the reader is looking. | ||
| if r.Secret != "" { | ||
| fmt.Printf(" signing secret %s %s\n", r.Name, r.Secret) |
There was a problem hiding this comment.
low · needs_human · xp-reviewer
Evaluated revision: 7afa4ef65fd0eecd94c4409e999b2ced1a43fdd3 · Finding: 53eed83bad49d5dc335c02facd8e469d8f93e10721d2f087dc32084566f84a42
Showing a webhook signing secret once at creation is the established design, and
fountain webhooks createprints it the same way, so this is consistent rather than novel. What is new is the surface:fountain applyis the one command the docs explicitly recommend wiring into an unattended pipeline ("Useful for CI: keepfountain.ymlin source control, runfountain apply -f fountain.ymlfrom your deploy pipeline"), so the first apply of a manifest containing aWebhookwrites a live HMAC signing secret into the build log, where it is retained by the CI provider and readable by anyone with log access.fountain webhooks createis an interactive command, so it never had this property. The secret is not otherwise recoverable, so simply suppressing it is not an option; this is a trade-off worth stating rather than a defect in the code as written.
Evidence
cli/internal/cmd/apply.go:164-171 prints
r.Secretto stdout insiderenderApplyResults, unconditionally and with no TTY check. apps/fountain/priv/help/manifest.md:145 ("Useful for CI: keepfountain.ymlin source control, runfountain apply -f fountain.ymlfrom your deploy pipeline") and docs/cli.md's apply section both present apply as the pipeline command. cli/internal/cmd/webhooks.go:155-177 and :294-306 show the interactivewebhooks createpath this mirrors.
Suggested remediation
A product call on how an unattended apply should hand over a one-time credential. Options: (a) leave as is and add one line to the
Webhookparagraph of docs/cli.md and apps/fountain/priv/help/manifest.md warning that a create in CI puts the secret in the build log, and pointing atfountain webhooks rotate-secretfor recovery; (b) print the secret only when stdout is a TTY and otherwise print a pointer telling the operator to rotate the secret interactively; (c) add an explicit--show-secretsflag that CI must opt into. Option (a) is the smallest and needs no code change.
Disposition
Reviewer requests a human decision
Human replies stay open until addressed. Resolving a conversation alone does not approve the PR.
Maintainers: post a new top-level PR comment /review-loop resolve e1ee3a9d-7d18-43de-833f-2446d4273f43 53eed83bad49 reason or /review-loop reject-fix e1ee3a9d-7d18-43de-833f-2446d4273f43 53eed83bad49 reason. Then retry the ended run with /review-loop retry e1ee3a9d-7d18-43de-833f-2446d4273f43.
…ents (#1636) One manifest now declares a whole estate. `POST /api/apply` takes three more kinds after the existing three, reconciled in a fixed order: Environment, Vault, Agent, Teammate, Schedule, Webhook. A spec names another document by name and the server resolves it, against the manifest first and then against the tenant's own records, the way an Agent's `environment` already did. * Teammate: keyed by the document name, which is what the teammate is called; `agent`, `environment` and `vault` resolve by name. Adding one opens the teammate's conversation through `Fountain.Team.add_teammate/4`. Re-applying moves the name and the environment and vault bindings through a new `Fountain.Team.update_teammate/4`, which gates both ids on the agent's allowlists and records `team.updated` only when something moved. It provisions no second computer. * Schedule: keyed by the document name under its `teammate`, with the `TeamScheduleCreateRequest` fields. It goes through `Fountain.Team.Schedules`, so an invalid cron fails with the create route's own changeset error. * Webhook: keyed by `spec.url`, with the `WebhookEndpointCreateRequest` fields. `Fountain.Webhooks.create_endpoint/3` hands the signing secret back on the apply that mints it; the result row carries it once and an update never does. Result rows gain a fourth verdict, `unchanged`, for every kind: a record that already matched the document was not written to, so a second apply of the same file says so instead of claiming an update. Inline `spec.secrets` are re-encrypted on each apply and keep reporting `upserted`, because the stored ciphertext cannot be compared with the plaintext given. Apply stays additive. A document dropped from the manifest leaves its record in place; there is no prune, and none is added here. The CLI groups and orders the six kinds, prints `=` for `unchanged`, and prints a new endpoint's signing secret once. A server too old for `/api/apply` cannot reconcile the three new kinds, so the fallback path reports them and exits nonzero rather than dropping them silently. Signed-off-by: lex00 <121451605+lex00@users.noreply.github.com>
…mate moves its computer (#1636) Review of the bulk-apply branch found five defects. All five are here. An `unchanged` row still left an `*.updated` audit event. `Repo.update` on an empty changeset skips the SQL, but the four contexts audited the `{:ok, _}` either way, so an idempotent re-apply wrote six rows saying a record nobody had touched was updated, each naming an empty changed-field list. That is what CLAUDE.md's "only record what happened" forbids, and what the OpenAPI description and the manual already claimed did not happen. `Environments`, `Vaults`, `Agents` and `Webhooks` now gate the recording on a non-empty changeset, the way `Team.Schedules.update_schedule/3` already did. `Team.update_teammate/4` rebound a live conversation with no regard for the machine underneath it. A home is keyed on (user, agent, environment, vault), so moving either id leaves the running computer holding the old environment's secrets while the row names the new one, and orphans the home: the next launch looks under the new key, builds a fresh machine, and the old one stays `ready` holding a concurrency slot. This is the hazard #1084 made `Agents.update_agent/3` refuse, and it is refused the same way now, with `:sandbox_mid_turn` before anything is written and `reset_sandbox/2` on the orphan after. An ephemeral computer is a conversation's own and is left alone; a name-only change touches nothing. Two Teammate documents naming one agent described one record, so each pass renamed the other's conversation and the manifest was never idempotent. The second and any later document now fail with their own error, and so does a second document reusing a name, which a Schedule could not have resolved. Nothing isolated an exception. The Teammate pass reaches Horde, the sandbox quota lock, the credit gate and the sandbox provider, and a raise there abandoned a call that had already committed the environments, vaults and agents, leaving the caller a 500 and no rows at all, against a moduledoc promising best effort per resource. Each document's reconcile is now wrapped, and a raise or an exit becomes that row's error with a logged stacktrace. The audit assertions covered only the create path. They now cover the update path too, for `team.updated`, `team.schedule.updated` and `webhook_endpoint.updated`, with the request's actor and IP. Those webhook actions keep the `webhook_endpoint` prefix the webhook routes have always written rather than the `webhook` one the issue named, because renaming a live audit vocabulary breaks every trail already recorded; the deviation is stated in the manual. Also, from the same review: a Teammate document is read as a whole declaration, so an absent `environment` or `vault` clears that binding, which now says so in all three places a reader looks; a Schedule naming a teammate two of them answer to fails rather than binding to whichever the roster listed last; the known context refusals get sentences instead of `inspect/1` output; result-row error keys are strings whichever branch produced them; and `update_teammate/4` takes the teammate map a caller already holds so bulk apply lists the roster once per document rather than twice. Signed-off-by: lex00 <121451605+lex00@users.noreply.github.com>
sobelow flagged the interpolated atom in binding_allowed/4; the two refusals are now literal atoms passed by the caller. Signed-off-by: lex00 <121451605+lex00@users.noreply.github.com>
… invoking RequireFullScope for any manifest containing a Webhook before calling Manifest.apply_manifest. Return the halted connection immediately on refusal, preventing writes to all resources in a denied mixed manifest. Manifests without Webhooks retain their existing scope policy; full-scoped requests retain the existing tenant-scoped reconciliation. The trusted max_files=1 policy limits this proposal to the webhook finding. Findings c31f87205fcabf9da98d7b78ab377317422e29a215701b34a82cc69c2da8954b (cotenant identity) and d77a775fcd08c26ef45f784146924c0e50b8681a6e0dcbf1d3c73c1a7d9641be (exception disclosure) remain unresolved and need separate proposals. No test execution is claimed. <!-- review-loop-effect:e1ee3a9d-7d18-43de-833f-2446d4273f43:7afa4ef65fd0eecd94c4409e999b2ced1a43fdd3:publish --> Signed-off-by: managoat-review-loop[bot] <325469270+managoat-review-loop[bot]@users.noreply.github.com>
The review loop found that a Webhook document let a sandbox-scoped token create an endpoint through apply, which POST /api/webhooks refuses. Its fix is the commit before this one; these are the tests, including that a refused mixed manifest writes none of its other resources. Signed-off-by: lex00 <121451605+lex00@users.noreply.github.com>
…at is taken (#1636) Two findings from the review of #1675, both real. A rebinding retired the teammate's home but left every conversation on it still naming that row, co-tenants included. The next wake re-provisioned from whichever conversation woke first and `move_cotenants/3` then repointed the others at it without looking at what they declared, so waking the teammate first pulled its co-tenant onto the new environment and vault, and waking the co-tenant first pulled the teammate back onto the old ones. Either way a conversation ran on a machine built from another conversation's binding, holding that binding's environment files and vault material, while its own row said something else. Co-tenants only diverge because rebinding makes them diverge — attaching to a machine requires the same identity — so this is the rebinding's bill to pay. `_unsafe_list_cotenants_with_identity/2` now reads each co-tenant's effective pair, the conversation's own override falling back to its agent's environment, which is what a sandbox row carries and what `_unsafe_find_home/4` keys on. Only the ones that match the replacement follow onto it. One that named something else keeps pointing at the retired row, which its own wake reads as `:create_new` and builds from its own pair. Every co-tenant is still told the disk is gone, because for every one of them it is; the ones left behind are told why they did not follow. The second finding is an occupied destination. There is one live home per (user, agent, environment, vault), and the wake path builds a home rather than attaching to one, so writing a binding whose identity already has a home left a teammate that could not wake at all: the insert hit `sandboxes_home_identity_index`, and re-applying the same manifest reported `unchanged` and did not recover it. `update_teammate/4` now refuses that before anything is written, with `:destination_home_occupied`, which `Fountain.Manifest` maps to a per-row sentence naming the fix. Refusing is deliberately the conservative half of the choice the reviewer put to a maintainer. Merging the teammate onto the machine that is already there is the other half, and it needs the readiness, runtime-shape and quota checks `attach_conversation/3` makes and this function does not. Refusing cannot strand a teammate; attaching wrongly can. Both halves of the decision are stated in `docs/api.md`, `docs/cli.md` and the manifest help topic, so whoever wants the merge later can see what was decided and why. The retirement test asserted only that the old row terminated. It now has a co-tenant and a subsequent wake, in both orders, at the `Conversations` level and end to end through `Team.update_teammate/4`, plus the occupied-destination refusal at both levels. The three co-tenant cases were checked against the old `move_cotenants/3` and fail there. Also formats the line the review-loop commit left over the limit, which `mix format --check-formatted` was failing on. Signed-off-by: lex00 <121451605+lex00@users.noreply.github.com>
…#1636) guarded/3 logged the exception and then also put its message into the result row, which ApplyJSON serialises into the 200 body. The environment and vault passes hold plaintext secrets inside that rescue, and Elixir's MatchError, CaseClauseError, BadMapError and ArgumentError messages embed the value they choked on, so the module's own promise that secret values are never echoed back held only on the paths that did not raise. Signed-off-by: lex00 <121451605+lex00@users.noreply.github.com>
0268a67 to
18c091f
Compare
|
All three findings are addressed in the commits just pushed, and the branch has been rebased onto current main. On the blocking co-tenant identity finding: On the finding that asked for a decision, an occupied destination home: it refuses. On the exception disclosure finding, which was mine to cause: the rescue now puts a fixed sentence in the result row and nothing else. The logs are unchanged. The reviewer was right that this was more than noise, because the environment and vault passes hold plaintext secrets inside that rescue and Elixir's own The webhook scope commit is kept as pushed, with tests: a sandbox-scoped token is refused on a manifest containing a |
managoat-review-loop
Bot
added
the
needs-human-review
|
Split into a stack of eight PRs, per the rule against large PRs. This branch stays open as the reference — its body and review threads are the record of why each decision was made — and closes when #1806 merges. Each PR is based on the one above it; review and merge top down:
Nothing was dropped. Two deliberate changes from this branch:
Verified on the tip, not just per PR: 6 doctests and 4,761 tests with 0 failures; 🤖 Generated with Claude Code |
2 participants
POST /api/applyreconciles a manifest ofEnvironment,VaultandAgentdocuments, and everything else a declared estate needs (team membership, schedules, webhook endpoints) had to be driven by hand through their own routes afterwards. This PR extends the manifest vocabulary so one apply reconciles the whole estate.Teammate,ScheduleandWebhookkinds, reconciled after the existing three in that order. A Teammate resolvesagent,environmentandvaultby name against the manifest first, then the tenant; a Schedule is keyed by(teammate, name); a Webhook is keyed byurland returns its signing secret only on the apply that creates it.unchangedverdict for all six kinds (Bulk apply reportsupdatedfor a document that changed nothing, so a client cannot tell a no-op from a write #1680), so a second identical apply reports every row unchanged. The environment, vault, agent and webhook contexts now record no audit event on a no-op update, which is what the audit rules require and what schedules already did.Team.update_teammate/4to move a teammate's environment and vault binding. It refuses while a turn is running on the teammate's computer and retires the home the old identity named, mirroringAgents.update_agent/3.Prune is left out; apply stays additive. The audit actions keep their existing names (
team.*,team.schedule.*,webhook_endpoint.*) rather than thewebhook.*the issue wrote.Closes #1636
Closes #1680
Validation:
mix compile --warnings-as-errors,mix format --check-formatted,mix credo --strict, dialyzer, sobelow anddeps.unlock --unusedare clean. The prod release assembles.go test ./...andgo vet ./...incli/; the SDK contract check, the TypeScript and Python verifiers and the conformance lint pass.scripts/docs-style.pyis clean.swift testdoes not compile on this machine's toolchain on main either.