fix(settings): one validated, audited write path for retention windows (ent#297)

[dolho]

What

The retention windows that drive irreversible deletion now have exactly one API write path, and it validates and audits.

	before	after
PUT /api/settings/{key}	bare db.set_setting, no checks	422, points at /ops/config
PUT /api/settings/ops/config	Dict[str, str] written straight through, no audit	type/range-validated, all-or-nothing, audit-logged

Why

ent#297 (private tracker) is the root-cause issue for a class that has been point-fixed five times: an agent-scoped MCP key resolves to its owner carrying the owner's role, so on a default admin-owned install every agent's injected TRINITY_MCP_API_KEY satisfied every admin gate — 114 of them.

The root fix is #1890, which puts reject_agent_principal inside require_admin and assert_admin (ent#297 AC 1/2/3/6). This PR is the other half — AC #4 and #5, the retention windows the attack actually reached.

What this does NOT do

Stating this plainly because the asymmetry is counter-intuitive and easy to oversell:

• Garbage always failed safe. Every reader coerces max(int(raw), 0) in a try/except returning 0, and 0 means sweep disabled — so "abc" widened retention to forever.
• A small valid integer is the catastrophic input. {"execution_row_retention_days": "1"} is well-typed, in range, and is exactly the issue's PoC. It is deliberately still accepted, because no range check can separate it from an operator who genuinely wants a one-day window.

So validation is not the control that stops the attack — the admin gate (#1890) and the #1644 blast-radius guard are. What it buys is a loud boundary failure instead of a silent coercion to a value nobody chose (#1525), and the removal of the second, entirely unvalidated path.

The audit half is its own small fix: /ops/config and /ops/reset logged nothing, while the generic PUT /{key} immediately above them does. The one route that could shrink a retention window was the one route that left no trace of having done it — ent#297 lists the audit surface in its blast radius.

Deliberately not clamped to the community floor

0 stays valid on every window (documented "disable this sweep"), and an explicit 3 is not rewritten to 5. The #1039 floor reaches installs by seeding fresh ones plus an enterprise entitlement clamp — never an OSS hard limit (#1638). Silently rewriting an admin's explicit choice is the same class of invisible mutation #1638 was about.

Verification

34 unit tests — structural and behavioural through the real handlers, with an explicit dataclass admin principal rather than a MagicMock (whose truthy .agent_name reads as an agent key — the trap recorded in #1816 and re-hit in ent#293).

HTTP-level via TestClient against the real router:

generic PUT retention window -> 422    (the PoC call, blocked)
generic PUT normal key       -> 200    (no collateral damage)
ops/config garbage           -> 422
ops/config negative          -> 422
ops/config valid             -> 200
ops/config ssh toggle (UI)   -> 200    (the only live UI caller of this route)

I checked the callers before blocking anything: the Settings UI writes retention only through the enterprise PUT /api/enterprise/retention/config, and the sole UI use of /ops/config is the ssh_access_enabled toggle. Neither regresses.

Two parity guards included, because this repo's recurring failure is a value set defined in one place and consumed in another that drifts: a new RETENTION_OPS_KEYS entry or a new ops setting without a validation spec fails the build.

352 passed across the settings / retention / ops / #1638 / #1644 / #1709 selection.

Related to abilityai/trinity-enterprise#297

[dolho]

/review Report — #1890 + #1893 (the ent#297 pair)

Branches: fix/293-admin-gate-rejects-agent-keys → dev (MB 04d11cf2) · fix/297-retention-settings-hardening → dev (MB 39f29c64)
Files Changed: 8 (+448/−15) · 5 (+559/−9)
Scope: CLEAN both
Plan Completion (ent#297 ACs): 6 done / 1 partial / 0 not done / 1 changed / 0 unverifiable

Self-review, so weighted toward finding problems — my self-review of #1885 earlier this week missed that I'd reintroduced the exact duplication that issue forbade. Four findings, all fixed.

---

Critical

[C1] Auth boundary: a THIRD admin-gate spelling left open (Confidence: 10/10) — FIXED bf23a2d7

File: src/backend/routers/agents.py:808

@router.post("/{agent_name}/circuit-breaker/reset")
async def reset_circuit_breaker_endpoint(
    agent_name: AuthorizedAgentByName,
    current_user: User = Depends(require_role("admin")),   # <- not require_admin

and the reason it stayed open (dependencies.py:590):

def _require_role(current_user: User = Depends(get_current_user)) -> User:
    _reject_connector_principal(current_user)      # connector, yes
    # reject_agent_principal — absent

Issue. #1890 closes require_admin and assert_admin. There are three spellings of an admin gate; require_role("admin") is the third, and after the other two were closed it was the only one left accepting an admin-owned agent's own key. Its single call site is POST /api/agents/{name}/circuit-breaker/reset — a route ent#297 names in its own blast-radius list ("circuit-breaker resets"). An agent that can re-close its own dispatch breaker on demand defeats the control built to contain it.

The naive fix is wrong, which is why this needed checking rather than patching: adding reject_agent_principal to require_role would break require_role("creator") on POST /api/agents, and agent-spawned agent creation is a supported feature (ent#69 Part 2 — spawned_by_agent provenance, auto-granted parent→child edge, enforce_agent_spawn_scope). It would kill ghost spawning.

Fix applied: swap the one call site to require_admin — equivalent for the admin case (ROLE_HIERARCHY = [..., "admin"], so >= admin is == admin) and it rejects agent principals. require_role's docstring now records why it stays permissive, so nobody "fixes" it later and breaks spawning.

Pinned by an AST scan over routers/. AST, not a regex — the comment on the fixed call site necessarily contains the offending string, and my first version flagged its own documentation. That is the second time this session I wrote a source-scanning guard that matched its own explanatory text; it is now the reason _calls_in_body exists in this file. Meta-tested by planting a violation in monitoring.py and confirming it fires.

---

Informational

[I1] #1893 invented a bound instead of adopting the existing one (Confidence: 9/10) — FIXED 825afbfd

config.py had _DAYS_MAX = 36500 (100y). The enterprise retention module validates the same windows at le=3650:

execution_row_retention_days: Optional[int] = Field(None, ge=0, le=_MAX_DAYS)   # _MAX_DAYS = 3650

Two validated write paths, two different contracts for one value: an admin could store a window via the OSS route that the managed panel then refuses to edit — surfaced by its own GET, rejected by its own PUT. Aligned to 3650 with a test pinning it, since the enterprise constant can't be imported (private submodule; OSS must build without it). This is the duplicated-policy-constant drift class from learnings.md, committed inside a fix whose theme is "one path".

[I2] Two false claims in #1893's own prose (Confidence: 10/10) — FIXED 825afbfd

• "the seven retention windows" — there are eight. agent_reminders_retention_days (feat: agent self-reminders — agent-callable one-shot deferred self-trigger ("remind me to do X later") #1296) joined RETENTION_OPS_KEYS and my prose didn't. Stated in three code comments, the commit message, and the PR body, while my own test asserted == 8.
• "this is the ONLY write path" — false. PUT /api/enterprise/retention/config is a second one. It was already typed and range-validated, so no security claim rested on this, but the sentence was wrong.

[I3] #1890's gate docstrings undercounted the class (Confidence: 8/10) — FIXED bf23a2d7

Both said "three consecutive incidents". ent#297 traced five (ops-agent#232, #1644, #1816, ent#236, ent#293), against 18 bolt-ons and 114 admin-gated call sites. In a security docstring the count is the argument for why the gate moved, so an undercount weakens the reasoning a future reader inherits.

[I4] Enterprise panel can't set one of the eight windows (Confidence: 8/10) — NOT FIXED, enterprise-side

RetentionConfigUpdate covers 7 of 8; agent_reminders_retention_days (#1296) is absent. So the managed retention panel cannot set it, and an entitled operator has no UI for that window. Pre-existing, in the private repo, out of scope for these PRs — flagging for a follow-up.

---

Clean (verified, cited)

• AC fix: add missing logging_config.py to backend Dockerfile #4 blocklist is not a bypass. DELETE /api/settings/{key} is untouched, but deleting a retention row reverts to OPS_SETTINGS_DEFAULTS, which are the wide values (30/90/7/180/30/90/90/90 — read at runtime, not assumed). Deleting fails safe; it is not a way around the 422.
• No legitimate caller broken. Checked before blocking: the Settings UI writes retention only through the enterprise endpoint (Settings.vue:2524), and the sole UI use of /ops/config is the ssh_access_enabled toggle (Settings.vue:3799), which the bool validator accepts. Driven end-to-end via TestClient: the PoC call 422s, a normal key still 200s, the SSH toggle still 200s.
• All-or-nothing is real, not aspirational — validation runs in its own loop before any db.set_setting, and a behavioural test asserts the good key in a mixed request never lands.
• assert_admin/require_admin parity — both reject agent + connector, both keep human admins and trinity-system (agent_name is set only for scope == "agent").
• Enum/value completeness — OPS_SETTINGS_VALIDATION has parity tests against both RETENTION_OPS_KEYS and OPS_SETTINGS_DEFAULTS, so a new window or ops setting can't ship unvalidated.
• Credential exposure — the new audit details carries ops config values; none of the 16 OPS_SETTINGS_DEFAULTS keys is a secret (thresholds, day counts, one bool), and answering "who shortened retention, to what, when" is the point.
• Migrations / SQL / concurrency — no schema, no SQL, no shared mutable state in either diff.

Summary

• Critical: 1 — found and fixed (bf23a2d7)
• Informational: 4 — three fixed, one (I4) is enterprise-side and reported
• Scope: clean

The one that mattered was C1, and it is worth naming why it survived the first pass: I audited the gates I changed rather than enumerating every way an admin gate can be spelled. Same shape as the #1871 lesson in learnings.md — a guard for a "more than one call site" class has to start by enumerating the API surface, not the call name you just fixed.

[github-actions]

⚠️ Nightly unit-suite check skipped — merge conflict against dev.

Resolve by running git merge dev locally and pushing the result. The next nightly run will re-test once the conflict is gone.

[vybe]

/validate-pr — approved.

All green including the pytest matrix (which the four required checks on dev don't cover, so worth naming). Security scans clean; no new os.getenv, no schema change, no BaseModel under routers/ (Invariant #14). 22 test defs (34 with parametrize) named for the issue, driven through the real handlers with a dataclass admin principal rather than a MagicMock — the #1816/ent#293 trap. except HTTPException: raise correctly added so the 422 isn't swallowed into a 500. The parity guards against RETENTION_OPS_KEYS/OPS_SETTINGS_DEFAULTS drift are the right shape.

One discrepancy, not a blocker. The PR body says "/ops/config and /ops/reset logged nothing", and the architecture.md prose this PR adds says "neither this route nor /ops/reset logged anything before" — but only /ops/config got the audit call. reset_ops_settings still deletes ops-setting rows, including ssh_access_enabled, with no audit entry. The retention claim holds (reset correctly skips RETENTION_OPS_KEYS); the audit claim as written doesn't. Filing a follow-up rather than holding the PR — the retention half is the part ent#297 needed.

Also: _DAYS_MAX = 3650 is aligned to the enterprise _MAX_DAYS by value plus a comment because the constant can't cross the submodule seam. That's the exact drift class this PR names as the repo's recurring failure — the parity assertion can only live on the enterprise side.

Cross-tracker ref, so the automation can't fire: setting status-in-dev on ent#297 manually after merge.