feat(api): bulk apply reconciles Teammate, Schedule and Webhook documents - #1675

Review Loop · Human review needed

Revision: 18c091f2efcf686a7fc5c39d04a44dba7166288d · 0 fix round(s) · generation 2 · Run: 0fb3bee4-3442-4fd2-9235-43b30028147d

Run deadline exhausted; work remains incomplete

• medium · needs_human 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.
  Finding: 0bfeb7943fa6d1bd1f5b97a17e9c0c153199f2b87f540cafe1f6907b76a144f8
  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

• high · needs_human 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 secret property 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.
  Finding: 3e2bff5d2e0c897c98a52e3b35f5764c88da84e4a81e9778f5e6fead441a4d8d
  Evidence: The call shape is renderApplyResults in the base CLI, git show 539a300:cli/internal/cmd/apply.go lines 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) does if renderApplyResults(results) { os.Exit(1) }. formatResultErrors returns the literal string "apply failed" when errs is empty, and an unchanged row carries errors: 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 :unchanged whenever 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 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 action as 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: true field 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

• medium · needs_human update_teammate/4 writes the same column, with the same blank-trimming and the same "only if it actually moved" rule, as the existing rename_teammate/4: both set the teammate conversation's title and both record metadata %{"fields" => ["name"]}. They disagree only on the action string. A teammate renamed through PATCH /api/team/:agent_id records team.renamed; the identical rename applied from a Teammate document records team.updated. ADR 0013 calls action "the trail's only groupable column" and notes Fountain.Analytics mirrors it through, so an operator or dashboard filtering on team.renamed silently misses every apply-driven rename — the exact door-dependent coverage gap CLAUDE.md's audit section says the #540 campaign existed to remove. The new team.updated action is justified for the environment/vault bindings, which rename_teammate/4 does not cover; it is only the overlapping name field that is now recorded two ways, along with two copies of the write-and-detect-a-real-change logic.
  Finding: 6dfc40754c14bd8b432b7825384414c2cc2e6ff468f3c1bf2a69ee608c537a1e
  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: @bindings maps "name" -> :title, binding_changes/2 applies the same blank_to_nil and the same reject-if-equal, record(user_id, "team.updated", updated, opts, %{"fields" => fields})). Both then call broadcast_changed(user_id). Fountain.Manifest is the only non-test caller of update_teammate/4 (apps/fountain/lib/fountain/manifest.ex:321), and apps/fountain/test/fountain/manifest_test.exs exercises a rename-only apply that lands on team.updated.
  Suggested remediation: A maintainer decision, because either direction moves a recorded action string. Option A: have update_teammate/4 emit team.renamed when changes is exactly %{title: _} and team.updated otherwise, so the existing action keeps its meaning and no route's trail changes — smallest change, but two action strings for one function. Option B: make rename_teammate/4 delegate to update_teammate(user_id, agent_id, %{"name" => name}, opts) so there is one write path — cleaner, but it retires team.renamed from PUT/PATCH /api/team/:agent_id and from team_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 update audit_guardrail_test.exs, team_test.exs and team_controller_test.exs accordingly.
  Disposition: Reviewer requests a human decision

• low · needs_human The PR adds unclaimed/3 for Teammate documents and explains exactly why: two documents that describe one record make every pass rewrite the other's, so no apply is ever unchanged. A Schedule is keyed by name under its teammate and has the same property, but no equivalent guard. Two Schedule documents with the same metadata.name and the same teammate — 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 reports created/updated rather than unchanged and writes two team.schedule.updated audit rows, which is precisely the audit noise this PR set out to remove in its ### Fixed entry. 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.
  Finding: ae8e3540572f841e4006a972be0879bf703c8f547bf3c0589db77d1719e21c6b
  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 in apply_schedule/4 at apps/fountain/lib/fountain/manifest.ex:328-341. reconcile_schedule/5 (manifest.ex:343-357) finds the row with Schedules.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 a claimed map to every callback, but apply_schedule/4 ignores it and reconcile_schedule/5 returns nil as 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 that reconcile/3 claims by, so two same-named schedules under different teammates are legitimate and must keep working. Option A (matches the Teammate precedent): have apply_schedule/4 take the claimed argument reconcile/3 already 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 needs reconcile/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

• low · needs_human 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 at Fountain.Environments.update_environment/3 as the canonical copy, and Fountain.Team.Schedules.update_schedule/3 already carried a fifth. Three of the four are structured as result = Repo.update(...) then an if/else that returns result or audited(result, ...), and the fourth (webhooks) as a bare if inside a case, 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.
  Finding: 2f23c82e473f28ea47a614cb5caf69b0d8d80414a34eaa549d60f53f0ff94b55
  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 rule Fountain.Environments.update_environment/3 follows (#1636)". apps/fountain/lib/fountain/team/schedules.ex:122 has the pre-existing if changeset.changes != %{} form.
  Suggested remediation: Add one helper next to the audit plumbing — e.g. Fountain.Audit.changed?/1 or an audited_if_changed(result, changeset, action, opts) shape each context's private audited/3 can 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 in Fountain.Audit or stays as a per-context idiom, given each context's audited/3 is private and resource-typed.
  Disposition: Reviewer requests a human decision

• low · needs_human Showing a webhook signing secret once at creation is the established design, and fountain webhooks create prints it the same way, so this is consistent rather than novel. What is new is the surface: fountain apply is the one command the docs explicitly recommend wiring into an unattended pipeline ("Useful for CI: keep fountain.yml in source control, run fountain apply -f fountain.yml from your deploy pipeline"), so the first apply of a manifest containing a Webhook writes 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 create is 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.
  Finding: 53eed83bad49d5dc335c02facd8e469d8f93e10721d2f087dc32084566f84a42
  Evidence: cli/internal/cmd/apply.go:164-171 prints r.Secret to stdout inside renderApplyResults, unconditionally and with no TTY check. apps/fountain/priv/help/manifest.md:145 ("Useful for CI: keep fountain.yml in source control, run fountain apply -f fountain.yml from 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 interactive webhooks create path 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 Webhook paragraph 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 at fountain webhooks rotate-secret for 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-secrets flag that CI must opt into. Option (a) is the smallest and needs no code change.
  Disposition: Reviewer requests a human decision

Usage: 101034 reported tokens. Approval does not merge the PR.

Maintainers can post new top-level PR comments: /review-loop resolve 0fb3bee4-3442-4fd2-9235-43b30028147d FINDING_ID reason or /review-loop reject-fix 0fb3bee4-3442-4fd2-9235-43b30028147d FINDING_ID reason. Use a unique finding ID prefix of at least 12 characters. Decisions do not dismiss human replies. After recording decisions, start a fresh review with /review-loop retry 0fb3bee4-3442-4fd2-9235-43b30028147d. Edited comments do not execute.