test: Review Loop human-decision acceptance fixture (do not merge)

[jhgaylor]

Controlled Review Loop acceptance fixture. Please do not merge this PR.

The CLI truncation change deliberately compares UTF-8 bytes with a rune budget; the regression test must remain after the implementation is repaired. This PR also proposes increasing default egress-log retention from seven days to 30 days, with matching configuration documentation.

Expected outcome: Review Loop repairs only cli/internal/output/output.go, checks and verification pass, then it applies needs-human-review with the seven-day versus 30-day retention decision. The protected configuration change requires a maintainer's judgment. No automatic approval or merge is expected.

[managoat-review-loop]

Review Loop · Human review needed

Revision: 3bd6d579c4f24cfb58a20e0faa873d2a973a6679 · 1 fix round(s) · generation 3 · Run: c477685c-5daa-4903-ac26-9fc9c9b96a28

A human decision remains; see the finding evidence and suggested remediation

• medium · needs_human An existing deployment with BROKER_LOG_RETENTION_HOURS unset or blank will retain egress metadata for thirty days after upgrading instead of seven. Updating the configuration table alone does not explain the upgrade behavior or establish whether extending existing deployments' retention is intended. This changes cleanup and storage behavior and needs a maintainer decision.
  Finding: b85d16e35a4137b3efbff43a26e0275d5e1b15e769cdd5cfd1f4f962ed39ab24
  Evidence: Confirmed HEAD and clean checkout, with the supplied base also the merge base. The entire three-file diff changes the runtime default from 168 to 720 and the documentation table to match, without an upgrade contract. BrokerReaper.run/1 at apps/fountain/lib/fountain/workers/broker_reaper.ex:45 computes the deletion cutoff from Broker.log_retention_hours/0. Native.RequestLog.sweep/1 deletes rows older than that cutoff, so an eight-day-old request previously eligible for deletion now remains. RequestLog.event/1 exposes host, path, service and credential-key metadata. The reaper retention tests explicitly set 168 or 1 and do not exercise unset/blank runtime defaults. Session expiry is independent; vendor vault lifetime is not affected. The prior Unicode defect is absent at this head: output.go:74 uses utf8.RuneCountInString, matching base. Local isolated output.go/output_test.go tests passed under Go 1.25.1; the normal package command was blocked because go.mod requires Go 1.26.0. These local checks are not service verification; no Elixir test execution is claimed.
  Suggested remediation: Choose whether to preserve the 168-hour default and make 720 an explicit operator opt-in, or adopt 720 with an explicit upgrade contract explaining the additional retention and how operators preserve seven days using BROKER_LOG_RETENTION_HOURS=168. Align the selected runtime default, documentation and Broker.log_retention_hours/0 fallback (currently 168). Add small regression tests for unset, blank and explicit environment values and the selected cleanup boundary. config/runtime.exs is outside the automatic fix policy, and the retention decision requires maintainer handling.
  Disposition: Reviewer requests a human decision

• medium · needs_human Upgrading a deployment with BROKER_LOG_RETENTION_HOURS unset or blank extends retention of tenant egress metadata from seven days to thirty without an operator choice. An eight-day-old request row previously eligible for deletion now remains readable through the owner's full-scope egress API. This includes destination host/path, service and credential key names, status and latency. The configuration table changes the stated default but does not explain the upgrade impact or establish the intended retention policy. This privacy-relevant change requires maintainer judgment.
  Finding: 835fa8eb8911c40a9a267dbab76f95ea783a3fe1e10563fcc6578b5ec32f8d51
  Evidence: Confirmed a clean checkout at the expected head and the supplied base as merge base; reviewed all three changed paths. runtime.exs:290-302 changes only the nil/empty default from 168 to 720; explicit values are unchanged. docs/configuration.md:57 matches 720. BrokerReaper.run/1 computes the deletion cutoff from Broker.log_retention_hours/0, then RequestLog.sweep/1 deletes broker_requests older than that cutoff. RequestLog.event/1 returns the retained metadata. GET /api/conversations/:conversation_id/egress requires full scope; ConversationController.egress/2 obtains current_user and calls Conversations.get_conversation(id, user.id), which uses Repo.get_by with both id and user_id, before reading the log. No new cross-tenant access is established. Sessions.sweep_expired/0 uses expires_at independently, so this does not extend credential lifetime. The existing second-tenant 404 and sprite-token 403 tests were inspected; an attempt to execute conversation_egress_test.exs stopped because Hex/dependencies were unavailable, so no runtime denial or approval evidence is claimed. The final head restores the rune-count guard in output.go; the prior Unicode availability finding is resolved and is not re-reported.
  Suggested remediation: Have a maintainer choose whether to preserve the 168-hour default and make 720 an explicit operator opt-in, or deliberately adopt thirty days and document the upgrade retention impact and BROKER_LOG_RETENTION_HOURS=168 opt-out. Align the selected runtime default, documentation and Broker.log_retention_hours/0 fallback. Retaining seven days preserves existing data minimization; thirty days provides longer investigation history with increased retained metadata and storage. config/runtime.exs is outside the automatic fix allowlist, and selecting the retention policy requires human judgment.
  Disposition: Reviewer requests a human decision

• medium · needs_human Unchanged at this head, so re-reported. The default egress request-log retention goes from 168 hours (7 days) to 720 (30 days). This is a product and data-retention decision, not a refactor: broker_requests rows record, per conversation, which host was reached, which service and so which credential binding matched, the status and the latency, and GET /api/conversations/:id/egress reads them. Quadrupling the default means every operator who has not set the variable silently starts keeping four times as much per-user egress history, from the upgrade onward, with no migration and no notice. The approved base does not settle this: ADR 0019 states the default as 168 and constrains it to be "at or below the broker's own retention", and that ADR is Accepted with a verified stamp of 2026-09-05 — one day before this head. The PR changes the number in config and in the operator table but amends neither the ADR nor its constraint, and gives no stated reason, so a reviewer cannot tell whether the intent is a considered retention-policy change, a debugging convenience, or an accident. The direction of the risk matters: for a privacy-relevant log, the wrong default is retained data nobody chose. Choosing between a shorter default that surprises nobody and a longer default that helps investigation is a product-behavior tradeoff the base does not decide.
  Finding: c2bfd4c9817d791736aebd8f80db5eaa9c4eb220107c515ec06a84bc0874153f
  Evidence: config/runtime.exs:290-300 changes only the blank/nil clause from 168 to 720; the explicit-value validation {n, ""} when n >= 1 at line 297 is untouched, so an operator who sets the variable is unaffected and only the unset default moves. The block sits outside any config_env() guard and config/ holds a single root runtime.exs with no per-app override, so it applies in dev, test and prod; line 302 assigns it to :broker_log_retention_hours. docs/configuration.md:57 now documents 720. decisions/0019-egress-credential-brokerage.md:737 still reads "has been over for BROKER_LOG_RETENTION_HOURS (168 by default, at or below the broker's own retention)", with lines 811 and 872 describing the same window; the frontmatter is adr_status Accepted, verified by codex at 2026-09-05, stale_after 2027-03-03. The consumer is apps/fountain/lib/fountain/workers/broker_reaper.ex:45, which turns the value straight into cutoff = DateTime.add(now, -Broker.log_retention_hours() * 3600, :second) and feeds it to RequestLog.sweep/1, so an eight-day-old request that was previously eligible for deletion now remains; the worker's own moduledoc says age is the only rule bounding that table. apps/fountain/test/fountain/workers/broker_reaper_test.exs:82, :93 and :101 each set the value explicitly to 168 or 1, so no test exercises the changed default and none covers the unset or blank environment path. No API schema, controller, router entry, ApiSpec operation or sdk/contract/contract.json entry is touched by this PR; sdk/typescript/src/generated/openapi.ts:1179 and apps/fountain/lib/fountain_web/controllers/conversation_controller.ex:188 describe the window by variable name only and embed no number, so the wire contract and generated types stay current and no client omission question arises. There is no CHANGELOG [Unreleased] entry for the change; that section at CHANGELOG.md:19 lists unrelated items only.
  Suggested remediation: Decide the retention policy, then make the tree say one thing. (a) Keep 7 days: revert config/runtime.exs:293 to 168 and docs/configuration.md:57 to 168, leaving ADR 0019 correct and no deployment changed. (b) Adopt 30 days deliberately: record it in decisions/0019 in the repo's dated-amendment style rather than editing line 737 in place, re-check the "at or below the broker's own retention" constraint now that Fountain runs the proxy itself, refresh decisions/index.md with scripts/decisions-index.sh in the same PR, and add a CHANGELOG [Unreleased] entry telling operators that egress-log volume and per-conversation host history grow roughly four-fold on upgrade and that BROKER_LOG_RETENTION_HOURS=168 preserves the old window. (c) Leave the default at 168 and document 720 in docs/configuration.md as a recommended value for operators who want a longer investigation window, which gets the benefit without changing any existing deployment silently. Whichever is chosen, add coverage for the unset, blank and explicit environment values and for the resulting cleanup boundary, and resolve the divergent in-code fallback — see broker-log-retention-default-divergence. config/runtime.exs is outside the fix policy's allowed paths, and the decision and its documentation require maintainer handling, so this is not a fix candidate.
  Disposition: Reviewer requests a human decision

• low · needs_human Unchanged at this head, so re-reported. The PR moves the runtime default to 720 but leaves the in-code fallback in Broker.log_retention_hours/0 at 168, so the tree now states two different defaults for one setting. The consequence is readability rather than behavior in a normal boot: config/runtime.exs always sets :broker_log_retention_hours, so the fallback is not reached and no deployment retains a different window because of it. It still matters, because this accessor is the function a reader consults to answer "how long do we keep egress rows", and it now answers 168 while the product ships 720; anything that reads the application environment without the root runtime config having been evaluated would also take the stale number. Reporting rather than repairing, because which of the two numbers is correct is decided by the retention question above, not by this line: setting it to 720 would quietly ratify a default change that has not been agreed, and setting it to 168 presumes the revert.
  Finding: a3698a32fcec79304312b521e1bb48caf7db0e692fbc4c7bcfe7b1c89ea02c24
  Evidence: apps/fountain/lib/fountain/broker.ex:164-165 is @spec log_retention_hours() :: pos_integer() / def log_retention_hours, do: Application.get_env(:fountain, :broker_log_retention_hours, 168), unchanged by this PR. config/runtime.exs:302 sets the same key to the value computed at lines 290-300, whose blank clause is now 720, and docs/configuration.md:57 documents 720. config/ contains one runtime.exs, no per-app config directory exists under apps/fountain, and a repo-wide grep for :broker_log_retention_hours finds it set only in runtime.exs and in broker_reaper_test.exs, so the literal in the accessor is the only source of the value wherever the root runtime config has not been evaluated. The single production consumer is apps/fountain/lib/fountain/workers/broker_reaper.ex:45.
  Suggested remediation: Resolve this together with broker-egress-log-retention-default-quadrupled, once the intended default is settled, so that config/runtime.exs, apps/fountain/lib/fountain/broker.ex, docs/configuration.md:57 and decisions/0019-egress-credential-brokerage.md:737 all state the same number. Two shapes are reasonable and the maintainer picks one: update the literal in log_retention_hours/0 to the agreed value, or drop the second literal entirely by reading the key without a default (Application.fetch_env!/2) so a missing configuration fails loudly instead of silently selecting a window nobody chose. Do not change this line ahead of the retention decision — on its own it is a one-file edit inside the policy's allowed paths, but either value encodes an answer to the open question.
  Disposition: Reviewer requests a human decision

• medium · needs_human Unchanged at this head and still unexplained, so re-filed. The default egress-log retention goes from 168 hours (7 days) to 720 (30 days) — a 4.3x increase applied silently, on upgrade, to every deployment that has not set BROKER_LOG_RETENTION_HOURS. Two consequences the PR does not address. Storage: broker_requests is one row per proxied HTTP request, Fountain.Workers.BrokerReaper is the only thing bounding that table, and its moduledoc says so directly ("age is the only rule, and without this pass a chatty tenant's rows would accumulate forever"), so steady-state row count and index footprint rise by the same factor with no operator action and no capacity note. Data retention: the rows hold per-request tenant metadata — host, matched service, which credential keys were attached, status, latency — so how long they are kept is a data-minimisation question, not a tuning constant. Nothing in the diff, the commit messages or an ADR says why 30 days is the right number. I am re-filing rather than proposing a remedy because the correct value depends on product and deployment intent, not on the code: the number itself is a one-line edit, but which number is right is not decidable from the tree. The service fix at this head touched only cli/internal/output/output.go and left this open.
  Finding: b63a3145ab55067b948903de60c8ec71d0e7d97efb4ca4dfc7038f32ab018dae
  Evidence: config/runtime.exs:290-300 changes only the blank/nil clause from 168 to 720; the explicit-value validation {n, ""} when n >= 1 is untouched, so an operator who sets the variable is unaffected and only the unset default moves. The block sits outside any config_env() guard and config/ holds a single root runtime.exs with no per-app override, so it applies in dev, test and prod; docker-compose.yml:195 passes the variable through with a blank default, so a compose deployment takes the changed branch. docs/configuration.md:57 was updated to match. The consumer is apps/fountain/lib/fountain/workers/broker_reaper.ex:45, which turns the value straight into cutoff = DateTime.add(now, -Broker.log_retention_hours() * 3600, :second) and feeds it to RequestLog.sweep/1 — a single daily sweep, so retention translates directly into retained rows, and an eight-day-old request that was previously eligible for deletion now remains. decisions/0019-egress-credential-brokerage.md:737 still records the default as 168. apps/fountain/test/fountain/workers/broker_reaper_test.exs:82, :93 and :101 each set the value explicitly to 168 or 1, so no test exercises the changed default and none covers the unset or blank environment path. apps/fountain/test/fountain/config_reference_test.exs only asserts that each variable read by runtime.exs has a row in docs/configuration.md; it does not compare documented defaults, so no gate catches a default that drifts from the ADR or from the in-code fallback. CHANGELOG.md [Unreleased] has no entry, and CONTRIBUTING.md:278 treats an [Unreleased] line as part of a change of this kind.
  Suggested remediation: A maintainer decides between: (a) keep 720 and land the rationale — an amendment to decisions/0019 stating the new retention and a note on expected broker_requests growth, plus a CHANGELOG [Unreleased] entry so operators upgrading know their storage and retained egress history change and that BROKER_LOG_RETENTION_HOURS=168 preserves the old window; (b) keep the default at 168 and let deployments that want 30 days set BROKER_LOG_RETENTION_HOURS=720, which is the no-surprise option, matches the operator-ratchet framing the other BROKER_* rows in docs/configuration.md use, and requires reverting docs/configuration.md:57 with it; or (c) pick a different number tied to a stated retention policy. Whichever is chosen, apply 'broker-log-retention-fallback-diverges' so the two in-code defaults agree, and add coverage for the unset, blank and explicit environment values and the resulting cleanup boundary. config/runtime.exs is outside the automatic fix policy's allowed paths in any case.
  Disposition: Reviewer requests a human decision

• low · needs_human The retention default is written in two places and this PR moves only one of them, so the tree now states two different numbers for the same setting. Before the change both said 168 and the duplication was harmless; after it, config/runtime.exs says 720 and the in-code Application.get_env fallback still says 168. The fallback is normally unreachable because runtime.exs sets the key under Mix and in a release, so I am not claiming an active wrong-retention bug — the cost is that a reader checking "what does this default to" gets a different answer depending on which file they open, and any path that loads the app without runtime.exs having run silently gets the old window. This is the duplicated-state problem rather than a taste issue, but it is low severity because no exercised path currently reads the stale copy. Unchanged at this head.
  Finding: d72fb4dbe5d6781562e0caed7ef5d6976f20c175d5d91d8d8ddb1534d138aec5
  Evidence: apps/fountain/lib/fountain/broker.ex:164-165 is unchanged by this PR and reads @spec log_retention_hours() :: pos_integer() / def log_retention_hours, do: Application.get_env(:fountain, :broker_log_retention_hours, 168). config/runtime.exs:293 now yields 720 for the blank/unset case and assigns it at runtime.exs:302 (config :fountain, :broker_log_retention_hours, broker_log_retention). Both numbers describe the same key. A repository-wide search for broker_log_retention and BROKER_LOG_RETENTION_HOURS shows these are the only two literal defaults in code; every other mention (broker.ex:484, broker_reaper.ex:5, request.ex:9, conversation_controller.ex:188, docs/concepts/secrets.md:198, the generated OpenAPI description) names the variable without a number. The only consumer of the value is apps/fountain/lib/fountain/workers/broker_reaper.ex:45.
  Suggested remediation: Blocked on 'retention-default-168-to-720-decision' — the right value here is whichever number that decision settles on, so changing it now would only guess, and either value silently ratifies an answer to the open question. Once decided, make broker.ex agree with config/runtime.exs, or better, drop the literal from the fallback so there is one source of truth: either Application.fetch_env!(:fountain, :broker_log_retention_hours) if the key is always configured, or a module attribute shared by both. Not a fix candidate: the remedy depends on the maintainer's retention answer even though the edit itself is one line inside an allowed path.
  Disposition: Reviewer requests a human decision

• low · needs_human ADR 0019 gate 4 records the retention default as 168; after this PR the shipped default is 720, so the ADR states a number the product no longer uses, and the ADR is Accepted with a verification stamp dated one day before this head. Whether that is a defect depends on a convention I should not decide unilaterally: CLAUDE.md treats decisions/ as an OKF bundle of records with status and verification metadata, this repo amends ADRs in dated "Amendment" sections rather than editing the original text, and the surrounding sentence is already partly historical — it describes BrokerVaultReaper deleting a vendor vault, and apps/fountain/lib/fountain/workers/broker_reaper.ex:13-15 says that worker and those vaults no longer exist (Delete the Agent Vault client, and take ADR 0019 off Proposed #1487). The parenthetical also carries a constraint, "at or below the broker's own retention", which was about the vendor proxy and has no counterpart now that Fountain runs the proxy itself, so a maintainer may reasonably read the whole passage as superseded and leave it alone. Flagging rather than resolving, because silently editing an ADR body would be the wrong repair if the convention is amendment. Unchanged at this head.
  Finding: 40cd94a51eda7544278f9d91a9cb860e29388ebc892b44dd96c704b518f9cd7e
  Evidence: decisions/0019-egress-credential-brokerage.md:737-739 reads "deletes the vault once the conversation has been over for BROKER_LOG_RETENTION_HOURS (168 by default, at or below the broker's own retention)". config/runtime.exs:293 and docs/configuration.md:57 now say 720. The ADR frontmatter is adr_status: "Accepted", verified: { by: codex, at: 2026-09-05T20:00:00-04:00 }, stale_after: 2027-03-03; this head is dated 2026-09-06. The other two ADR mentions, lines 811 and 872, name the variable without a number and need nothing. apps/fountain/lib/fountain/workers/broker_reaper.ex:13-15: "This was BrokerVaultReaper … There are no vaults now (Delete the Agent Vault client, and take ADR 0019 off Proposed #1487), and the log is a table here." CLAUDE.md:736 requires an index refresh via scripts/decisions-index.sh in the same PR as an ADR change, and okf validate decisions in CI.
  Suggested remediation: Maintainer decides between: (a) treat the passage as superseded history and change nothing, since the worker and the vendor retention it constrains are both gone; or (b) add a short dated amendment to decisions/0019 recording the move to 720 and the reasoning, in the style of the existing "Amendment (2026-08-25)" section — which also gives 'retention-default-168-to-720-decision' its written rationale — and refresh decisions/index.md with scripts/decisions-index.sh in the same PR. Editing the 168 in place is the option to avoid: it rewrites what the decision said at the time. Moot under option (b) of the retention finding, where the default returns to 168 and the ADR is already correct.
  Disposition: Reviewer requests a human decision

• info · nonblocking Optional consolidation, not a defect — recording it because it affects where a future reader looks for Truncate's contract. The new table is a second table over the same function, and its lead case restates a property TestTruncate already covers: {"héllo", 5, "héllo"} is the same byte-length-versus-rune-count regression as {"emoji title fits exactly", "🐐 café", 6, "🐐 café"}, and the existing case caught the guard regression on its own during this PR's history. Two of the three new cases are genuinely new — a 4-byte rune, where a 2-byte one would not distinguish some off-by-a-few slicing errors, and the n=0 boundary — so the coverage is worth keeping; it is the separate function and the "Title" in its name that cost something. Truncate has no notion of a title, and the CLI calls it on prompts, descriptions, pubkeys and agent text, so the name points a reader at a caller that does not exist. Not proposed as a blocking change; the test is correct and passes as written.
  Finding: 63caabda8909101c1902e981c366e54f4aa66f0447eddc78c067c63bd870840b
  Evidence: cli/internal/output/output_test.go:112-129 is the existing TestTruncate table, whose comments already state the contract under review ("rune-aware slicing", "rune-aware length check: 6 bytes, 5 runes, no cut"). The new TestTruncateUnicodeTitleAtRuneBudget at line 170 adds a second table with three cases at lines 176-178. During this PR's history both tables went red against the same root cause, which is what shows they test the same property. Call sites take prompts (cli/internal/cmd/conv.go:149, :687), descriptions (vault.go:86, env.go:47) and a pubkey (buzz.go:76); none is a title. For context on the resolved half of the PR: cli/internal/output/output.go:74 at this head reads if utf8.RuneCountInString(s) <= n {, identical to the merge base, so output.go has no net diff and the byte-guard defect reported at the previous head is not present here.
  Suggested remediation: Optionally fold the two cases that add coverage — {"🐐 cafés", 6, "🐐 café…"} and {"é", 0, "…"}, plus {"🐐 café", 6, "🐐 café"} if a 4-byte-rune pass-through is wanted alongside the existing 2-byte one — into the TestTruncate table and delete the separate function, so one table states the contract. If the separate test is kept, rename it to something the function actually promises, for example TestTruncateRuneBudget. Either way this is a preference and leaving it as written is fine; do not relax any of these assertions, since they encode the documented rune contract and the callers' rune budgets.
  Disposition: Below the trusted policy's blocking threshold

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

Maintainers can post new top-level PR comments: /review-loop resolve c477685c-5daa-4903-ac26-9fc9c9b96a28 FINDING_ID reason or /review-loop reject-fix c477685c-5daa-4903-ac26-9fc9c9b96a28 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 c477685c-5daa-4903-ac26-9fc9c9b96a28. Edited comments do not execute.

[managoat-review-loop]

high · fix · qa-team

Evaluated revision: 000e351bdd2112efc441ffacd58507cfc502966d · Finding: 83376bc0368de710c581cca78cb3d0d68ec22e9cfba0b97bfd5f8e21c0af0da5

The byte-length guard sends Unicode strings that fit the rune budget into the truncation branch. Exact-budget strings gain a spurious ellipsis, and shorter strings can panic when n exceeds the rune slice capacity. This can terminate CLI commands rendering ordinary user content.

Evidence

Local go test ./internal/output failed: TestTruncate returns héllo… instead of héllo, and the new TestTruncateUnicodeTitleAtRuneBudget returns 🐐 café… instead of 🐐 café. Independently, conv.go:149 passes turn prompts to Truncate with n=80: a prompt of 30 goat emoji has 120 bytes but only 30 runes, so the guard falls through and runes[:80] exceeds capacity, predicting a panic. These are local review observations, not service verification.

Suggested remediation

Restore utf8.RuneCountInString(s) <= n in output.go. Keep the existing exact-budget regression assertions; add a small regression case with 30 emoji and an 80-rune budget to cover the shorter-than-budget crash.

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 b1c569fc-8484-418c-82bb-2323aabf8ce2 83376bc0368d reason or /review-loop reject-fix b1c569fc-8484-418c-82bb-2323aabf8ce2 83376bc0368d reason. Then retry the ended run with /review-loop retry b1c569fc-8484-418c-82bb-2323aabf8ce2.

[managoat-review-loop]

medium · needs_human · qa-team

Evaluated revision: 3bd6d579c4f24cfb58a20e0faa873d2a973a6679 · Finding: b85d16e35a4137b3efbff43a26e0275d5e1b15e769cdd5cfd1f4f962ed39ab24

An existing deployment with BROKER_LOG_RETENTION_HOURS unset or blank will retain egress metadata for thirty days after upgrading instead of seven. Updating the configuration table alone does not explain the upgrade behavior or establish whether extending existing deployments' retention is intended. This changes cleanup and storage behavior and needs a maintainer decision.

Evidence

Confirmed HEAD and clean checkout, with the supplied base also the merge base. The entire three-file diff changes the runtime default from 168 to 720 and the documentation table to match, without an upgrade contract. BrokerReaper.run/1 at apps/fountain/lib/fountain/workers/broker_reaper.ex:45 computes the deletion cutoff from Broker.log_retention_hours/0. Native.RequestLog.sweep/1 deletes rows older than that cutoff, so an eight-day-old request previously eligible for deletion now remains. RequestLog.event/1 exposes host, path, service and credential-key metadata. The reaper retention tests explicitly set 168 or 1 and do not exercise unset/blank runtime defaults. Session expiry is independent; vendor vault lifetime is not affected. The prior Unicode defect is absent at this head: output.go:74 uses utf8.RuneCountInString, matching base. Local isolated output.go/output_test.go tests passed under Go 1.25.1; the normal package command was blocked because go.mod requires Go 1.26.0. These local checks are not service verification; no Elixir test execution is claimed.

Suggested remediation

Choose whether to preserve the 168-hour default and make 720 an explicit operator opt-in, or adopt 720 with an explicit upgrade contract explaining the additional retention and how operators preserve seven days using BROKER_LOG_RETENTION_HOURS=168. Align the selected runtime default, documentation and Broker.log_retention_hours/0 fallback (currently 168). Add small regression tests for unset, blank and explicit environment values and the selected cleanup boundary. config/runtime.exs is outside the automatic fix policy, and the retention decision requires maintainer handling.

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 c477685c-5daa-4903-ac26-9fc9c9b96a28 b85d16e35a41 reason or /review-loop reject-fix c477685c-5daa-4903-ac26-9fc9c9b96a28 b85d16e35a41 reason. Then retry the ended run with /review-loop retry c477685c-5daa-4903-ac26-9fc9c9b96a28.

[managoat-review-loop]

medium · fix · security-audit

Evaluated revision: 000e351bdd2112efc441ffacd58507cfc502966d · Finding: b0403b1569703238f61851d2fe25610718293624b50ee0d3920fab513d2c5c01

The new byte-length check permits short multibyte strings to reach runes[:n] even when they contain fewer than n runes. This can panic and terminate the CLI while displaying API data. For example, a vault description containing 31 copies of é is 62 bytes but only 31 runes; fountain vault list passes it to Truncate with a limit of 60. One such stored description prevents the ordinary list command from rendering. The same boundary processes environment setup scripts, conversation prompts and streamed user text. This is a client availability regression; the inspected list endpoints remain tenant-scoped, so no cross-tenant access is asserted.

Evidence

cli/internal/cmd/vault.go:86 passes the API description to Truncate(..., 60); output.go:74 checks len(s), then lines 77-78 convert to []rune and slice to n without a rune-length guard. env.go:47 and conv.go:149,687 have equivalent inputs. At the exact rune budget the code also adds an incorrect ellipsis, contradicting both the existing héllo case and the new emoji-title case in output_test.go.

Suggested remediation

In cli/internal/output/output.go, restore utf8.RuneCountInString(s) <= n as the early-return condition, or check len(runes) <= n after conversion before slicing. This restores the existing rune-budget contract within one permitted file.

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 b1c569fc-8484-418c-82bb-2323aabf8ce2 b0403b156970 reason or /review-loop reject-fix b1c569fc-8484-418c-82bb-2323aabf8ce2 b0403b156970 reason. Then retry the ended run with /review-loop retry b1c569fc-8484-418c-82bb-2323aabf8ce2.

[managoat-review-loop]

critical · fix · product-api

Evaluated revision: 000e351bdd2112efc441ffacd58507cfc502966d · Finding: e80065f53c2ab3fd8a06b17d73bfade437f6862a5dc556a0f26a6e37d0d6a0cb

Truncate's guard changed from utf8.RuneCountInString(s) <= n to len(s) <= n, but the branch it guards still slices runes: string(runes[:n]). The two units are now mixed. For any string whose byte length exceeds n while its rune count is below n — ordinary CJK, accented or emoji text — the guard falls through and runes[:n] indexes past the end of the rune slice, so the CLI panics with a runtime slice-bounds error instead of printing a table. Where the rune count is between n and the byte length, there is no panic but the output is still wrong: a title that fits the column gets a spurious ellipsis, and because padRight and Table still measure in runes, the column widths no longer line up with what Truncate produced. The doc comment on line 72 ("truncated to n runes") and every caller's column budget describe rune semantics, so the guard is the half that moved, not the contract.

Evidence

Reproduced the panic with the shipped function body: input of 20 U+1F410 runes (80 bytes, 20 runes) at n=60 — the budget vault.go:86 uses for the description column — panics with "runtime error: slice bounds out of range [:60] with capacity 32". Reachable call sites all pass user-controlled text: cli/internal/cmd/conv.go:149 (turn prompt, n=80 — 30 CJK characters is 90 bytes and 30 runes, so fountain conv show panics on a Chinese or Japanese prompt of ~28-80 characters), cli/internal/cmd/conv.go:687 (streamed text, n=200), cli/internal/cmd/vault.go:86 (description, n=60), cli/internal/cmd/env.go:47 (setup_script, n=60), cli/internal/cmd/buzz.go:76 (pubkey, n=16). go test ./internal/output/... in cli/ fails on this head: the pre-existing TestTruncate case {"héllo", 5, "héllo"} — commented in output_test.go:122 as "rune-aware length check: 6 bytes, 5 runes, no cut" — now returns "héllo…", and the PR's own new case TestTruncateUnicodeTitleAtRuneBudget/emoji_title_fits_exactly asserts Truncate("🐐 café", 6) == "🐐 café" (6 runes, 10 bytes) and gets "🐐 café…". The new test therefore asserts the rune semantics the same commit removes, which is also why the Go CLI half of the sdk-clients CI job cannot be green at this head.

Suggested remediation

Restore the rune-counting guard on cli/internal/output/output.go:74: if utf8.RuneCountInString(s) <= n {. That is a one-line, single-file change inside the permitted cli/** paths, it restores the documented public behavior rather than changing it, and it makes both the pre-existing TestTruncate case and all three new TestTruncateUnicodeTitleAtRuneBudget cases pass as written. The unicode/utf8 import on line 8 is still used by Table and padRight, so nothing else moves. Keep the new test — it is a correct regression test for the behavior being restored.

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 b1c569fc-8484-418c-82bb-2323aabf8ce2 e80065f53c2a reason or /review-loop reject-fix b1c569fc-8484-418c-82bb-2323aabf8ce2 e80065f53c2a reason. Then retry the ended run with /review-loop retry b1c569fc-8484-418c-82bb-2323aabf8ce2.

[managoat-review-loop]

medium · needs_human · product-api

Evaluated revision: 3bd6d579c4f24cfb58a20e0faa873d2a973a6679 · Finding: c2bfd4c9817d791736aebd8f80db5eaa9c4eb220107c515ec06a84bc0874153f

Unchanged at this head, so re-reported. The default egress request-log retention goes from 168 hours (7 days) to 720 (30 days). This is a product and data-retention decision, not a refactor: broker_requests rows record, per conversation, which host was reached, which service and so which credential binding matched, the status and the latency, and GET /api/conversations/:id/egress reads them. Quadrupling the default means every operator who has not set the variable silently starts keeping four times as much per-user egress history, from the upgrade onward, with no migration and no notice. The approved base does not settle this: ADR 0019 states the default as 168 and constrains it to be "at or below the broker's own retention", and that ADR is Accepted with a verified stamp of 2026-09-05 — one day before this head. The PR changes the number in config and in the operator table but amends neither the ADR nor its constraint, and gives no stated reason, so a reviewer cannot tell whether the intent is a considered retention-policy change, a debugging convenience, or an accident. The direction of the risk matters: for a privacy-relevant log, the wrong default is retained data nobody chose. Choosing between a shorter default that surprises nobody and a longer default that helps investigation is a product-behavior tradeoff the base does not decide.

Evidence

config/runtime.exs:290-300 changes only the blank/nil clause from 168 to 720; the explicit-value validation {n, ""} when n >= 1 at line 297 is untouched, so an operator who sets the variable is unaffected and only the unset default moves. The block sits outside any config_env() guard and config/ holds a single root runtime.exs with no per-app override, so it applies in dev, test and prod; line 302 assigns it to :broker_log_retention_hours. docs/configuration.md:57 now documents 720. decisions/0019-egress-credential-brokerage.md:737 still reads "has been over for BROKER_LOG_RETENTION_HOURS (168 by default, at or below the broker's own retention)", with lines 811 and 872 describing the same window; the frontmatter is adr_status Accepted, verified by codex at 2026-09-05, stale_after 2027-03-03. The consumer is apps/fountain/lib/fountain/workers/broker_reaper.ex:45, which turns the value straight into cutoff = DateTime.add(now, -Broker.log_retention_hours() * 3600, :second) and feeds it to RequestLog.sweep/1, so an eight-day-old request that was previously eligible for deletion now remains; the worker's own moduledoc says age is the only rule bounding that table. apps/fountain/test/fountain/workers/broker_reaper_test.exs:82, :93 and :101 each set the value explicitly to 168 or 1, so no test exercises the changed default and none covers the unset or blank environment path. No API schema, controller, router entry, ApiSpec operation or sdk/contract/contract.json entry is touched by this PR; sdk/typescript/src/generated/openapi.ts:1179 and apps/fountain/lib/fountain_web/controllers/conversation_controller.ex:188 describe the window by variable name only and embed no number, so the wire contract and generated types stay current and no client omission question arises. There is no CHANGELOG [Unreleased] entry for the change; that section at CHANGELOG.md:19 lists unrelated items only.

Suggested remediation

Decide the retention policy, then make the tree say one thing. (a) Keep 7 days: revert config/runtime.exs:293 to 168 and docs/configuration.md:57 to 168, leaving ADR 0019 correct and no deployment changed. (b) Adopt 30 days deliberately: record it in decisions/0019 in the repo's dated-amendment style rather than editing line 737 in place, re-check the "at or below the broker's own retention" constraint now that Fountain runs the proxy itself, refresh decisions/index.md with scripts/decisions-index.sh in the same PR, and add a CHANGELOG [Unreleased] entry telling operators that egress-log volume and per-conversation host history grow roughly four-fold on upgrade and that BROKER_LOG_RETENTION_HOURS=168 preserves the old window. (c) Leave the default at 168 and document 720 in docs/configuration.md as a recommended value for operators who want a longer investigation window, which gets the benefit without changing any existing deployment silently. Whichever is chosen, add coverage for the unset, blank and explicit environment values and for the resulting cleanup boundary, and resolve the divergent in-code fallback — see broker-log-retention-default-divergence. config/runtime.exs is outside the fix policy's allowed paths, and the decision and its documentation require maintainer handling, so this is not a fix candidate.

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 c477685c-5daa-4903-ac26-9fc9c9b96a28 c2bfd4c9817d reason or /review-loop reject-fix c477685c-5daa-4903-ac26-9fc9c9b96a28 c2bfd4c9817d reason. Then retry the ended run with /review-loop retry c477685c-5daa-4903-ac26-9fc9c9b96a28.

[managoat-review-loop]

critical · fix · xp-reviewer

Evaluated revision: 000e351bdd2112efc441ffacd58507cfc502966d · Finding: daa46546168f6a398fc10fa5ddb29b5d2e4a0276db3be3d8007bf6e653e0e8ca

Truncate is documented as "truncated to n runes" and slices []rune(s)[:n], but the guard now measures bytes. For any non-ASCII string the two units disagree, and the function reaches runes[:n] with n larger than the rune count. Depending on the capacity Go happens to allocate for the rune slice, that is either a hard panic (slice bounds out of range) or a silent NUL-padded string; when rune count == n exactly it appends a spurious ellipsis to a string that fits. All five call sites pass user- or agent-supplied text at fixed rune budgets — output.Truncate(ToString(v["description"]), 60) in cli/internal/cmd/vault.go:86, setup_script at 60 in cli/internal/cmd/env.go:47, prompt at 80 in cli/internal/cmd/conv.go:149, streamed message text at 200 in cli/internal/cmd/conv.go:687, and pubkey at 16 in cli/internal/cmd/buzz.go:76 — so a vault whose description is accented or CJK text crashes fountain vault list. The unit is also load-bearing for layout: Table/padRight compute column widths with utf8.RuneCountInString, so the budget Truncate enforces has to be runes for columns to line up. The change is a strict regression with nothing bought; the previous utf8.RuneCountInString(s) <= n was correct and the utf8 import is still needed by Table and padRight either way.

Evidence

go test ./internal/output/... at head fails on two tests, one of them pre-existing: TestTruncate — Truncate("héllo", 5) = "héllo…", want "héllo" (output_test.go:127); TestTruncateUnicodeTitleAtRuneBudget/emoji_title_fits_exactly — Truncate("🐐 café", 6) = "🐐 café…", want "🐐 café" (output_test.go:184). Running the head implementation directly over the real call-site budgets: Truncate(strings.Repeat("é",40), 60) -> PANIC: runtime error: slice bounds out of range [:60] with capacity 40 (the vault.go:86 budget); Truncate(strings.Repeat("日",30), 60) -> PANIC: [:60] with capacity 32 (the env.go:47 budget); Truncate(strings.Repeat("→",100), 200) -> PANIC: [:200] with capacity 104 (the conv.go:687 budget); Truncate("日本", 4) = "日本\x00\x00…" — no panic, two NUL runes emitted into the table cell. go build ./... in cli/ succeeds, so this is a runtime failure, not a compile-time one.

Suggested remediation

In cli/internal/output/output.go:74, restore the rune-unit guard: if utf8.RuneCountInString(s) <= n {. That is the whole fix — the doc comment, the rune slice on line 77, the existing TestTruncate cases and the new TestTruncateUnicodeTitleAtRuneBudget cases all already agree on runes, and utf8 stays imported for Table/padRight.

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 b1c569fc-8484-418c-82bb-2323aabf8ce2 daa46546168f reason or /review-loop reject-fix b1c569fc-8484-418c-82bb-2323aabf8ce2 daa46546168f reason. Then retry the ended run with /review-loop retry b1c569fc-8484-418c-82bb-2323aabf8ce2.

[managoat-review-loop]

medium · needs_human · xp-reviewer

Evaluated revision: 3bd6d579c4f24cfb58a20e0faa873d2a973a6679 · Finding: b63a3145ab55067b948903de60c8ec71d0e7d97efb4ca4dfc7038f32ab018dae

Unchanged at this head and still unexplained, so re-filed. The default egress-log retention goes from 168 hours (7 days) to 720 (30 days) — a 4.3x increase applied silently, on upgrade, to every deployment that has not set BROKER_LOG_RETENTION_HOURS. Two consequences the PR does not address. Storage: broker_requests is one row per proxied HTTP request, Fountain.Workers.BrokerReaper is the only thing bounding that table, and its moduledoc says so directly ("age is the only rule, and without this pass a chatty tenant's rows would accumulate forever"), so steady-state row count and index footprint rise by the same factor with no operator action and no capacity note. Data retention: the rows hold per-request tenant metadata — host, matched service, which credential keys were attached, status, latency — so how long they are kept is a data-minimisation question, not a tuning constant. Nothing in the diff, the commit messages or an ADR says why 30 days is the right number. I am re-filing rather than proposing a remedy because the correct value depends on product and deployment intent, not on the code: the number itself is a one-line edit, but which number is right is not decidable from the tree. The service fix at this head touched only cli/internal/output/output.go and left this open.

Evidence

config/runtime.exs:290-300 changes only the blank/nil clause from 168 to 720; the explicit-value validation {n, ""} when n >= 1 is untouched, so an operator who sets the variable is unaffected and only the unset default moves. The block sits outside any config_env() guard and config/ holds a single root runtime.exs with no per-app override, so it applies in dev, test and prod; docker-compose.yml:195 passes the variable through with a blank default, so a compose deployment takes the changed branch. docs/configuration.md:57 was updated to match. The consumer is apps/fountain/lib/fountain/workers/broker_reaper.ex:45, which turns the value straight into cutoff = DateTime.add(now, -Broker.log_retention_hours() * 3600, :second) and feeds it to RequestLog.sweep/1 — a single daily sweep, so retention translates directly into retained rows, and an eight-day-old request that was previously eligible for deletion now remains. decisions/0019-egress-credential-brokerage.md:737 still records the default as 168. apps/fountain/test/fountain/workers/broker_reaper_test.exs:82, :93 and :101 each set the value explicitly to 168 or 1, so no test exercises the changed default and none covers the unset or blank environment path. apps/fountain/test/fountain/config_reference_test.exs only asserts that each variable read by runtime.exs has a row in docs/configuration.md; it does not compare documented defaults, so no gate catches a default that drifts from the ADR or from the in-code fallback. CHANGELOG.md [Unreleased] has no entry, and CONTRIBUTING.md:278 treats an [Unreleased] line as part of a change of this kind.

Suggested remediation

A maintainer decides between: (a) keep 720 and land the rationale — an amendment to decisions/0019 stating the new retention and a note on expected broker_requests growth, plus a CHANGELOG [Unreleased] entry so operators upgrading know their storage and retained egress history change and that BROKER_LOG_RETENTION_HOURS=168 preserves the old window; (b) keep the default at 168 and let deployments that want 30 days set BROKER_LOG_RETENTION_HOURS=720, which is the no-surprise option, matches the operator-ratchet framing the other BROKER_* rows in docs/configuration.md use, and requires reverting docs/configuration.md:57 with it; or (c) pick a different number tied to a stated retention policy. Whichever is chosen, apply 'broker-log-retention-fallback-diverges' so the two in-code defaults agree, and add coverage for the unset, blank and explicit environment values and the resulting cleanup boundary. config/runtime.exs is outside the automatic fix policy's allowed paths in any case.

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 c477685c-5daa-4903-ac26-9fc9c9b96a28 b63a3145ab55 reason or /review-loop reject-fix c477685c-5daa-4903-ac26-9fc9c9b96a28 b63a3145ab55 reason. Then retry the ended run with /review-loop retry c477685c-5daa-4903-ac26-9fc9c9b96a28.

[managoat-review-loop]

medium · needs_human · security-audit

Evaluated revision: 3bd6d579c4f24cfb58a20e0faa873d2a973a6679 · Finding: 835fa8eb8911c40a9a267dbab76f95ea783a3fe1e10563fcc6578b5ec32f8d51

Upgrading a deployment with BROKER_LOG_RETENTION_HOURS unset or blank extends retention of tenant egress metadata from seven days to thirty without an operator choice. An eight-day-old request row previously eligible for deletion now remains readable through the owner's full-scope egress API. This includes destination host/path, service and credential key names, status and latency. The configuration table changes the stated default but does not explain the upgrade impact or establish the intended retention policy. This privacy-relevant change requires maintainer judgment.

Evidence

Confirmed a clean checkout at the expected head and the supplied base as merge base; reviewed all three changed paths. runtime.exs:290-302 changes only the nil/empty default from 168 to 720; explicit values are unchanged. docs/configuration.md:57 matches 720. BrokerReaper.run/1 computes the deletion cutoff from Broker.log_retention_hours/0, then RequestLog.sweep/1 deletes broker_requests older than that cutoff. RequestLog.event/1 returns the retained metadata. GET /api/conversations/:conversation_id/egress requires full scope; ConversationController.egress/2 obtains current_user and calls Conversations.get_conversation(id, user.id), which uses Repo.get_by with both id and user_id, before reading the log. No new cross-tenant access is established. Sessions.sweep_expired/0 uses expires_at independently, so this does not extend credential lifetime. The existing second-tenant 404 and sprite-token 403 tests were inspected; an attempt to execute conversation_egress_test.exs stopped because Hex/dependencies were unavailable, so no runtime denial or approval evidence is claimed. The final head restores the rune-count guard in output.go; the prior Unicode availability finding is resolved and is not re-reported.

Suggested remediation

Have a maintainer choose whether to preserve the 168-hour default and make 720 an explicit operator opt-in, or deliberately adopt thirty days and document the upgrade retention impact and BROKER_LOG_RETENTION_HOURS=168 opt-out. Align the selected runtime default, documentation and Broker.log_retention_hours/0 fallback. Retaining seven days preserves existing data minimization; thirty days provides longer investigation history with increased retained metadata and storage. config/runtime.exs is outside the automatic fix allowlist, and selecting the retention policy requires human judgment.

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 c477685c-5daa-4903-ac26-9fc9c9b96a28 835fa8eb8911 reason or /review-loop reject-fix c477685c-5daa-4903-ac26-9fc9c9b96a28 835fa8eb8911 reason. Then retry the ended run with /review-loop retry c477685c-5daa-4903-ac26-9fc9c9b96a28.