feat(credentials): runtime secret-scrub seam + license-blind vault MCP/UI (ent#279 PR-2)

[AndriiPasternak31]

Refs trinity-enterprise#279

What this is — PR-2, the public OSS half

The public-stream half of trinity-enterprise#279: a generic OSS runtime secret-scrub seam plus its execution-terminal chokepoint wiring, the license-blind MCP proxy tools, a gated Settings panel, and docs. The private producer that stages a fetched secret before delivering it merged separately in the enterprise submodule (trinity-enterprise#410, on enterprise main as 90f2f2c).

The seam (src/backend/services/runtime_secret_scrub.py) is mechanism-only and behaviour-neutral for OSS: a producer calls stage_secret(value) (fail-CLOSED — an unstageable value raises StagingUnavailable so delivery is refused), and the persistence chokepoints call get_staged_values() + scrub_text/scrub_obj to strip every staged value out of the durable output (fail-OPEN) before the write. This catches arbitrary prefix-less values (a customer DB password) that the pattern-based utils/credential_sanitizer cannot. With nothing staged, every scrub is a no-op → no schema change, no migration, no flag.

Scope (rebased onto dev @ dd910564, 2026-09-02)

• Seam: services/runtime_secret_scrub.py
• Chokepoint wirings: task_execution_service (apply_result both branches — SUCCESS response/execution_log/raw_response and FAILED error + the bug: error_during_execution runs store zero telemetry (no execution_log/session_id/cost) and the JSONL is reaped in 1-7h — failure class is undiagnosable #1853 salvaged transcript & tool_calls summary — plus _write_terminal_and_gate), chat_execution_service (_finalize_chat_success incl. the idempotency snapshot · _parse_agent_http_error scrubbed before its ERROR log and before the 500-char truncation · _finalize_budget_exhausted), pull_coordination_service, proactive_message_service (TOP — before delivery and persist), channel_history, routers/voice
• MCP: tools/credential_vault.ts (list/fetch, license-blind, operator-scope) + client.ts/server.ts/tools/index.ts + tool-visibility.test.ts
• Vue: gated CredentialVaultPanel.vue + Settings tab entry (adminOnly, requires: 'credential_vault'); loading states gated on loading && no-data-yet per the bug: background polls re-flash loaded content and reset UI state (design-system p13/p14) #1927 ratchet (design-system p13/p14)
• Docs: requirements credentials.md §3.7 · architecture services-catalog entry · new feature-flows/runtime-secret-scrub.md + both index rows
• CI: seam file added to enterprise-docs-guard.yml (SEAM_FILES + both paths:)
• Tests: test_ent279_secret_scrub.py (seam + chokepoints + escaping-evasion + cross-worker) · test_ent279_scrub_parity.py (drift guard, test_1804 idiom; now allowlists dev's bug: watchdog fails admitted-but-undispatched executions as "completed on agent but status not reported", releasing their slots and masking the real terminal #2433 _cancel_inflight_if_parked, whose only error write is a static literal) · test_ent279_d26_combined_tree.py (proves by interception that the enterprise vault's lazy stage_secret import and the OSS chokepoints bind the same module object; skips on OSS-only clones)

Status — all former blockers closed; READY

1. Gitlink bump DONE, then superseded: dev itself now carries the gitlink at 90f2f2c, so the branch's own bump commit was dropped as previously-applied during the rebase. git ls-files -s src/backend/enterprise on this branch → 90f2f2c. (Reminder for anyone touching this: the submodule is update = none / ignore = all, so gitlink moves are invisible to a plain git status.)
2. Rebase DONE: onto dd910564 (2026-09-02). Two conflicts, both in chat_execution_service.py, both additive (dev's bug: watchdog fails admitted-but-undispatched executions as "completed on agent but status not reported", releasing their slots and masking the real terminal #2433 cancelled flag and quote-style change vs. this branch's scrub insertions) — resolved keeping both sides.
3. bug: error_during_execution runs store zero telemetry (no execution_log/session_id/cost) and the JSONL is reaped in 1-7h — failure class is undiagnosable #1853 FAILED-path scrub gap DONE (82b83340): the FAILED branch now identity-scrubs envelope.execution_log before the salvage serialization, mirroring SUCCESS — the salvaged transcript and its derived bug: schedule_executions.tool_calls stores a verbatim copy of execution_log — breaks the tool-call metric and survives log retention #1741 tool_calls summary no longer rely on the pattern pass alone.
4. D26 combined-tree test DONE (74531123): the hand-run composition check is now a committed guard.
5. CI build red (bug: background polls re-flash loaded content and reset UI state (design-system p13/p14) #1927 loading-gate ratchet) DONE (aa15aa93): both bare gates in CredentialVaultPanel.vue now gate on loading && !items.length.

Validation (re-run on this exact tree, 2026-09-02)

• Backend: pytest unit/ -k 'ent279 or ent435 or 2052' → 161 passed, 2 skipped (skips environmental: sibling redis :6390)
• Conflict-area suites: pytest unit/ -k '2433 or 1853' → 152 passed, 1 skipped
• Frontend: vitest tests/unit/loadingGateRatchet.spec.js → 6 passed
• Not re-run since the rebase: the full /validate-pr sweep and /verify-local. CI on this push is the current signal.

From the original pre-rebase /validate-pr — all green at that time: security scans clean; enterprise-docs-guard grep = 0 hits; packaging fine (services/ is COPY'd wholesale, no new os.getenv); commits conventional; docs + both feature-flow index rows present. (The Vue panel has no unit test — repo has no component-mount harness.)

Notes for the reviewer / merger

• Cross-tracker Refs trinity-enterprise#279: same-repo status auto-promotion will not fire, and GitHub will not auto-close the private issue. Set status-in-dev and close trinity-enterprise#279 by hand at the release cut.
• This PR is release-floor bar 6 of trinity-enterprise#441; dogfooding on the main instance is trinity-enterprise#452 and starts after merge.

🤖 Generated with Claude Code

[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.

[github-actions]

⚠️ Live-instance suite skipped — merge conflict against dev.

Resolve by merging dev locally and pushing the result; the next nightly re-tests.

[dolho]

/review Report

Scope: CLEAN on substance — the diff is the ent#279 PR-2 surface exactly as described (seam + six chokepoint wirings + license-blind MCP tools + gated panel + docs/CI/tests). One scope note below on formatter churn.

I read the full seam, every chokepoint wiring, the MCP tool/visibility/audit surface, and the panel; test files and docs skimmed.

Critical findings

None. The specific traps I went hunting for all check out:

• Failure asymmetry is real, not just claimed: stage_secret raises StagingUnavailable on any store/encrypt error (fail-closed), get_staged_values returns [] + one throttled ERROR (fail-open) — and the docstring's argument for why open-scrub never widens exposure (no NEW secret delivered while staging is down) holds.
• Scrub placement is right at all six chokepoints: before sanitize_*, before json.dumps, before truncation (scrub_text(_staged, e.response.text)[:500] — the straddling-fragment case is explicitly handled), before the CAS write, and before the idempotency response_snapshot that replays to duplicate-key callers for 24h. The FAILED branch scrubs envelope.error at the top, so the #1578 completion event, the activity close, and the ent#265 channel report all inherit it.
• fetch_credential plaintext cannot leak via the MCP audit wrapper: withAudit logs duration/error/target_id and returns the result without capturing it (audit.ts:172-176); the tools never throw (both wrap in degradeList/failFetch), so no error path carries a value either.
• Tool visibility is an allowlist that fails closed: operator scopes only, connector/anonymous/portal_delegate/future-scope all hidden, pinned by the new tests — the exact #848/#2323 deny-check lesson, applied correctly to a tool whose whole job is handing out secrets.
• REDACTION_PLACEHOLDER exists at utils/credential_sanitizer.py:112; the seam reuses it rather than minting a second marker.
• Panel: values are write-only (type="password", never echoed, "shown once"), no v-html, raw-axios-in-component matches the existing panel precedent, and the loading gates satisfy the #1927 ratchet as claimed.

Informational

[I1] Formatter churn dominates the two hottest dispatch files (Confidence: 9/10)
A large fraction of the task_execution_service.py (+242/−82) and chat_execution_service.py (+537/−211) hunks are pure black-style re-wraps of untouched code (import splitting, comment realignment, 2**attempt, ternary re-flow). RoE #2 says no cosmetic formatting of unrelated code — and practically, these are the files every in-flight execution-path branch touches (the #2314 decomposition included), so each pure-format hunk is a future conflict for zero semantic content. If a re-push happens anyway, consider dropping the format-only hunks; if not, the merger should just know the real diff is ~⅓ the stated size.

[I2] The proactive-message wiring deliberately breaks the seam's own contract — say so in the seam (Confidence: 8/10)
The seam docstring promises "the live tool result stays whatever the producer delivered; only the persisted copy is scrubbed", but proactive_message_service.send_message scrubs before channel delivery (D19, argued in-code and I think correctly — a proactive message is an exfil channel, not a tool result). The two texts currently contradict each other; one sentence in the seam docstring naming the delivery-scrub exception keeps the next reader from "fixing" either side.

[I3] The scrub tax is fleet-global once anything is staged (Confidence: 7/10)
The store is deliberately global, so one vault-active agent makes every terminal fleet-wide pay HGETALL + N AES decrypts + N×3 str.replace passes over multi-MB transcripts, on the event loop. Bounded (dedup by value-hash, 24h member TTL, warn 200 / cap 1000) and str.replace is C-fast, so this is fine at realistic set sizes — but worth one sentence in the feature-flow's operational notes so a future "why did terminals get slower" investigation starts in the right place.

[I4] The <8-char floor is a fail-open edge on the fail-closed side (Confidence: 6/10)
stage_secret silently skips values under _MIN_VALUE_LEN (WARNING only) and the caller proceeds to deliver — a 6-char vault value is delivered and never identity-scrubbed. The floor itself is right (scrubbing short strings shreds transcripts, and it matches the sanitizer's own ≥8 rule). The clean closure is on the private side: have the vault write path refuse to store values under the floor, so the unstaged-delivery state is unreachable. Worth a cross-check against trinity-enterprise#410 — if it already does, ignore.

Summary

• Critical: none found.
• Informational: 4 — I2 is a one-sentence docstring fix worth doing; I1 is for the merger's awareness; I3/I4 are notes.
• backend-unit-test is still pending as I post this — note that dev has a time-bomb test failing after 10:00 UTC today (test_2467_turn_integrity fleet-list arm, hardcoded timestamp vs the hours=24 window; fix in #2488). If this PR's run reds on exactly that test, it is not this PR.

Approving — the seam's design is careful, the chokepoint discipline (scrub-before-sanitize/truncate/CAS/snapshot, both applier branches) is exactly right, and the guards (visibility allowlist, D26 combined-tree, parity suite) pin the properties that matter.

🤖 Generated with Claude Code

https://claude.ai/code/session_01NLfHNPtB5UCMk4LonZiJux

[obasilakis]

/validate-pr: no critical findings — leaving this as a comment rather than a second approval, since @dolho has already reviewed head aa15aa93. One item below should gate the merge, though.

Blocking-ish: the green CI predates the base this will merge into

The tested merge commit is dd910564 + aa15aa93 (run created 12:12Z). dev has since gained 12 commits, including bf6fcf11 — the #2398 sanitizer ReDoS hotfix, +92/−6 to src/backend/utils/credential_sanitizer.py. That is the exact module this seam imports (REDACTION_PLACEHOLDER) and composes with at every chokepoint (identity scrub, then pattern sanitize).

The imported constant is unchanged (credential_sanitizer.py:112), so the coupling looks stable — but the ent#279 suites have never executed against the new sanitizer. Please re-run CI on current dev before merging.

(For the record: the PR is not stale in effect. Opened 2026-08-17 but rebased today; 12 commits behind dev with zero file overlap against dev-only changes; mergeable: MERGEABLE, mergeStateStatus: CLEAN.)

Security battery — all clean

• Steps 4.1–4.9: no key/token hits, only test-fixture emails (u@x.io), no public IPs, no .env or credential files, no infra change, no new top-level src/backend/*.py (the seam lives under services/, copied wholesale), no new os.getenv. The sole 4.5 hit is a test literal in test_ent279_secret_scrub.py. gitleaks passes.
• Fail-open scrub is deliberate design, not a defect (runtime_secret_scrub.py:26-34,176-189), and the asymmetry holds: stage_secret fails closed, and enterprise _stage_or_fail_closed (service.py:398) turns any staging failure into a 503 refuse-delivery — so no new secret ships during an outage. The residual is honestly documented (flow doc, lines 97-98): a turn already in flight when Redis drops persists its transcript un-identity-scrubbed, with the pattern pass still running, for a window bounded by the turn timeout. A fail-closed scrub would destroy terminal CAS writes; this is the right trade.
• No secret value reaches any log, response or error body — verified end to end. StagingUnavailable carries type(e).__name__ only, never str(e) (:168-170); short-value WARNING logs the floor and agent only (:130-135); the fail-open ERROR logs the redis exception only (:188); the decrypt WARNING (:200) logs a ValueError from credential_encryption.decrypt, whose messages carry only JSON-parse/version/algorithm/InvalidTag text — no plaintext, no ciphertext; _parse_agent_http_error scrubs before the 500-char slice and before its ERROR log (f47f73c); MCP withAudit never captures the result; the Vue value fields are write-only.
• Invariant #13 verified against the pinned submodule 90f2f2c (equal to dev's gitlink, so no bump is owed): backend router.py:51 prefix /api/enterprise/credential-vault plus GET /available (:200) and POST /fetch (:205) match client.ts:2828/2836 exactly. The agent-server surface is N/A — agents reach the vault via MCP.
• @dolho's open I4 is closed: enterprise VALUE_MIN_CHARS = 8 (service.py:40) is enforced at the write path with a 422, exactly matching the seam's _MIN_VALUE_LEN = 8, so the silent-skip branch is unreachable for vault values by construction.

Warnings

• Formatter churn in the two hottest dispatch files (RoE #2). chat_execution_service.py: 741 changed lines, 19 of them (2.6%) mentioning scrub/staged. task_execution_service.py: 297 / 21 (7%). Roughly 95% is cosmetic re-wrap, which is real conflict surface for in-flight branches — #2314 and #2489 both touch task_execution_service.py.
• dev is now at v0.9.5-rc1, so merging post-RC lands this in the next cut, not 0.9.5-rc1 — worth noting against the claim of clearing release-floor bar 6 of ent#441.
• The body uses "Refs", not a closing keyword — and it's cross-tracker anyway, so automation can't fire. trinity-enterprise#279 is already status-in-dev but will need a manual close at the release cut.

Suggestions

• If a re-push happens anyway, drop the format-only hunks from the two dispatch files — it also shrinks the rebase surface for #2314.
• Add one seam-docstring sentence naming the proactive_message_service delivery-scrub exception (@dolho's I2); the docstring's "only the persisted copy is scrubbed" currently contradicts that wiring.
• Note the fleet-global scrub cost in the flow doc's operational notes (@dolho's I3), so a future "terminals got slower" hunt starts in the right place.

---

Findings produced by /validate-pr (Claude Code).

[vybe]

Rebased onto current dev — CI re-run request honoured

Merged current dev (f2843ee0). This addresses the earlier review note that the green CI predated the base this would merge into; the run on this head includes the #2398 sanitizer hotfix and the five PRs merged since (#2498, #2489, #2485, #2496, #2500).

The conflict, and how it was resolved

#2489 (issue #2314) decomposed execute_task into named phases in the same module this branch touches. The two changes turned out to be semantically disjoint:

• Every ent#279 scrub site lives in _write_terminal_and_gate and apply_result. #2489 left both byte-identical.
• Every other difference this branch carried in that file was black reformatting with zero semantic content (verified function-by-function against the merge base).

So the resolution takes dev's file whole — keeping the decomposed execute_task, the nine extracted helpers, and the services/execution_envelope re-export — and re-applies only the scrub import plus the four scrub blocks. None of the reformatting was carried over, which is why the diff against dev is now ~20 lines in that file instead of ~1,300.

One real cross-PR gap, caught by your own guard

test_every_free_text_writer_scrubs failed on the merged tree. #2489 moved three static admission-refusal writes — capacity-full, dispatch-breaker, ephemeral-exhausted — out of execute_task into the new _admission_gate, which the ent#279 allowlist did not yet name. The writes themselves are unchanged and still run before any agent call, so no agent-authored text and no staged secret can exist on that path.

_admission_gate is therefore allowlisted on exactly the grounds the execute_task entry already recorded, and that entry is narrowed to the single backend-shutdown write it still holds. Neither entry is stale.

Worth noting the guard did its job here: this gap exists only in the merged result, and neither PR is wrong on its own.

Verification

1864 passed, 9 skipped   # all 90 unit-test files touching
                         # task_execution_service / execute_task / apply_result
49 passed, 2 skipped     # the three ent#279 suites

@AndriiPasternak31 please sanity-check the scrub placement survived the move intact — the four blocks are unchanged in content and sit at the same points relative to the sanitize/CAS ordering your comments describe.

[vybe]

Merged current dev again (36e6e1a3) — post-#2513

Pushed c596d15e: a merge of dev after #2509 (main reconcile), #2479 (zod 4), #2500, #2480 and — the one that matters here — #2513 (feat(pull): let scheduled work reach the durable queue), which is the only PR since f2843ee0 that touches task_execution_service.py.

Overlap with #2513

Git merged it clean (no conflict markers). Verified by hand on the merged file:

• The import block now carries both from services.pull_pilot import note_unreachable_pull_trigger, pull_owns_dispatch (feat(pull): let scheduled work reach the durable queue (#2391) #2513) and from services.runtime_secret_scrub import get_staged_values, scrub_obj, scrub_text (this PR).
• All four scrub blocks are intact and in place: _write_terminal_and_gate (error), and apply_result SUCCESS (response / execution_log / raw_response) + FAILED (error, execution_log).
• feat(pull): let scheduled work reach the durable queue (#2391) #2513's build_pull_queue_payload lands above _write_terminal_and_gate and it widens _admission_gate — the writer your test_every_free_text_writer_scrubs allowlist already names after the refactor(execution): decompose execute_task into named phases (#2314) #2489 move. The pull QUEUED handoff returns before any free-text write, so the allowlist is unchanged.

Verification on the merged tree

3634 passed, 16 skipped, 1 failed   # 175 unit files touching task_execution_service /
                                    # execute_task / apply_result / runtime_secret_scrub /
                                    # ent279 / pull_pilot / capacity / backlog / chat_execution /
                                    # proactive_message / channel_history / voice / scheduler

The single failure — test_1310_auth_consolidation.py::test_schedules_create_access_before_existence_404[sqlite] — is order-dependent: the file passes 46/46 in isolation on this tree and on a dev-equivalent tree, and it is an auth-consolidation test neither this PR nor #2513 goes near. Noting it rather than chasing it here; if the CI matrix reds on exactly that test it is not this PR.

CI on c596d15e is running; merging once the pytest matrix + e2e land.

[vybe]

/validate-pr — APPROVE

Building on @dolho's /review (approved, no critical findings) and @obasilakis's /validate-pr (security battery clean, one gating item: re-run CI on current dev). That item is now closed twice over — the branch has been brought onto dev at 36e6e1a3 (post-#2513, the only change since that touches task_execution_service.py), and the full matrix on c596d15e is green: 6/6 pytest seeds, regression diff, e2e (path-triggered, ran and passed without the ui label), frontend build, CodeQL, gitleaks, schema-parity, prod-image-smoke, non-root.

Local verification on the merged tree: 3634 passed / 16 skipped across the 175 unit files adjacent to the seam and the dispatch path; the one failure (test_1310_auth_consolidation…[sqlite]) is order-dependent and passes 46/46 in isolation on this tree and on dev.

Remaining notes, none blocking:

• Cross-tracker Refs trinity-enterprise#279 — no closing keyword, so the automation cannot promote it; ent#279 is already status-in-dev and needs the manual close at the release cut.
• The new feature flow (runtime-secret-scrub.md) uses the Problem/Goal/seam/chokepoints structure rather than the template's Overview/User Story/Entry Points/Frontend/Backend sections. Reasonable for a backend-only seam with no UI entry point; Error Handling / Testing / Related Flows are present.
• @dolho's I2 (seam docstring vs. the proactive-message delivery-scrub exception) and I3 (fleet-global scrub cost in the flow's operational notes) are still open as doc follow-ups; I4 was closed by the enterprise write-path floor.

Squash-merging.