Repository navigation
feat(audit): record circuit-breaker resets and execution terminates in the audit log (#3180) - #4035
AndriiPasternak31 wants to merge 13 commits into
Conversation
POST /api/agents/{agent_name}/circuit-breaker/reset, the most-run operator
write, left no audit row. It now writes one CONFIGURATION row,
`circuit_breaker_reset`, with actor_user=current_user, after both breaker
resets have returned. The key fields come from the principal through
platform_audit_service.log (#2389), so a reset made with a key names the key.
The row does not depend on DISPATCH_BREAKER_ENABLED or the per-agent toggle,
because the transport breaker trips with both off.
Tests run through the real resolver and the real route on a real SQLite
store: a session and a user-scoped key each write one row with the right
actor and key id; refused calls (operator session, operator key, ops key,
admin-owned agent key, no credential) and a failed reset write none; with
the breaker feature off, the reset still writes its row.
Part of #3180
POST /api/agents/{name}/executions/{execution_id}/terminate left no audit
row. It now writes one EXECUTION row, `execution_terminate`, with
actor_user=current_user, for the three outcomes in which work was actually
ended: cancelled while queued, cancelled while parked in the backend
agent-call queue, and terminated in the container. An `already_finished`
answer from the agent ended nothing (#679) and writes none; every error arm
raises before the row. The key fields come from the principal through
platform_audit_service.log (#2389). The termination service is unchanged:
the row is written at the operator route, which is the caller that has a
principal.
Tests drive the real route through the real resolver: a session and a
user-scoped key each write one row (queued cancel and container-proxy
terminate; parked cancel by session); refused calls (outsider session, ops
key, sibling agent key, no credential) and failed calls (agent 500 / 404 /
unreachable / timeout, container stopped or missing) write none and leave
the execution row as it was.
Part of #3180
Adds `execution_terminate` (EXECUTION) and `circuit_breaker_reset` (CONFIGURATION) to the §20.1 Event Categories, each with when the row is written. No other docs change. Part of #3180
Prepends the entry for tests/unit/test_3180_operational_write_audit.py. Part of #3180
A terminate of a turn still parked in the backend agent-call queue answers `cancelled_while_parked` on every call until its dispatcher resumes, but only the call that wins the CANCELLED CAS ended anything. Keying the row on the response status therefore wrote a second row, crediting whoever sent the repeat. terminate_execution now takes an optional `on_ended` hook and awaits it once, from inside the arm that knows: the queued cancel that won its CAS, the parked cancel that won its CAS, or the container proxy when the agent answered `terminated`. The operator route passes a hook that writes the `execution_terminate` row as the caller. Responses are unchanged, and the public-link and Workspace callers pass no hook, so their behaviour is unchanged too. Regression: two terminates of one parked turn, by a session and then by a user-scoped key, both answer 200 `cancelled_while_parked` and write exactly one row, the session's. Red before this change (2 rows), green after. Part of #3180
…e CAS The proxy arm wrote the execution_terminate row whenever the agent answered `terminated`. The agent answers that again for a turn it accepted but has not started (its pending entry is kept), so a repeat terminate by a second credential wrote a second row crediting itself, the defect the parked arm was already fixed for. The row now follows the CANCELLED CAS, like every other terminal side effect (execution.md). Accepted cost: killing a process whose row is already terminal writes no row. Tests: a repeated container terminate writes one row (the session's); a terminate of an already-FAILED row writes none; a queued cancel that loses its CAS to the backlog drain's real claim writes only the container arm's row (the queued gate had no test). Drops a process note from a test docstring.
…open reset_circuit and reset_dispatch log and return when Redis refuses the DELETE, so the route answers 200 `closed` with both breakers still open (pre-existing). The row is written once both reset calls return, so it records the request, not a confirmed reset; the comment and the §20.1 line said "after both breakers are reset". No behaviour change. Tests: the real fail-open shape (Redis refuses the delete: 200, breakers unchanged, one row); the raising-reset test is kept and described as what it is, an ordering guard (a row moved above the resets goes red).
…ency (#3180) test_an_authorization_refusal_records_the_client overrode main.get_current_user. The unit conftest evicts `dependencies` after every test, so when a test file has imported a router before `main` is first imported (this slice's audit suite imports routers.agents and routers.chat at module level), `main` binds a newer copy of get_current_user than the one the POST /api/agents route was built with. The override was then never consulted and the request answered 401 instead of 403: red whenever the audit suite ran first on the same xdist worker, green alone. Take the dependency from the route's own globals instead, through the route_globals helper the file already uses for the sign-in tests. The assertion is unchanged. The sibling slice carries the identical change, so whichever merges second merges it as a no-op.
|
merge-train 2026-10-27 — validated READY at
Also: add |
dolho
left a comment
dolho
left a comment
There was a problem hiding this comment.
Review via /review (pre-landing structural review).
/review Report
Branch: PR #4035 head 4220a0515 → dev (merge-base f00cca137)
Files Changed: 7 (+802/-11). That is 705 lines of new test, 85 of source, 13 in the registry and 4 in docs.
Scope: CLEAN. The PR asks for two operator writes to leave an audit_log row, and the diff adds exactly that. The test_ent873 change is a test-isolation fix the PR body explains.
Plan Completion: 2 done (reset row at routers/agents.py:1150, terminate row at routers/chat.py:1033 through the on_ended hook). Out of scope and listed under "Not delivered": the public-link terminate, the Workspace terminate and the monitoring-check breaker reset.
Merge conflict: GitHub reports CONFLICTING against today's dev. git merge-tree shows the only conflicting file is tests/registry.json, the usual case where both sides prepend an entry. Keep both entries when you re-merge. This review was done against the merge-base.
Execution coverage (Step 2.5)
| changed symbol / test file | executed by | live consumer | verdict |
|---|---|---|---|
reset audit row (routers/agents.py:1150) |
TestCircuitBreakerReset (real routes and resolver, rows read back from SQLite) |
POST /api/agents/{agent_name}/circuit-breaker/reset |
✅ executed |
_audit_terminate + on_ended= (routers/chat.py:1033-1054) |
TestExecutionTerminate (queued, parked and container arms, plus repeat, lost-CAS and already-terminal cases) |
operator terminate route | ✅ executed |
on_ended in _cancel_queued_if_queued / _cancel_inflight_if_parked / _proxy_terminate_and_finalize |
same suite | terminate_execution ← routers/chat.py:1049. The other callers, public.py:1009 and client_portal/service.py:4497, pass no hook |
✅ executed |
test_ent873_audit_client_ip.py override key |
the test itself | route_globals helper that already exists in that file |
✅ executed |
| source-text grep over the changed tests | no hits | — | ✅ |
Fix mutation: I reverted all three source files to the merge-base in a scratch worktree and ran the new suite. 12 failed, 22 passed. The red tests are the positive cases: session and key reset, queued, parked and container terminate, the repeat cases, and the breaker-off reset. The 22 that stayed green are the "writes no row" cases, which is expected because nothing in that state writes a row. At head the suite plus test_ent873 is 41 passed (-p no:randomly).
Critical Findings (block merge)
None.
Informational Findings (review required)
[I1] Earlier finding, still open: actor_ip is NULL on both new rows (Confidence: 8/10). This was raised in the 2026-10-27 merge-train comment by vybe, item 1, and is not yet addressed: the head is still 4220a0515. Both handlers still lack request: Request. reset_circuit_breaker_endpoint(agent_name: AuthorizedAgentByName, current_user: User = Depends(require_admin)) and terminate_agent_execution(execution_id, task_execution_id, name, current_user) pass no actor_ip= to platform_audit_service.log. The other lifecycle writes in agents.py do pass it (:640/:866/:904/:995, actor_ip=request.client.host if request.client else None). Since ent#873's whole point is that audit rows carry the real client address, two new rows without it are a visible gap. Fix: add request: Request to both handlers, pass actor_ip=... to log, and add one assertion per route.
[I2] Earlier finding, not reproduced: "the ExecutionDetail Stop door never writes a row" (Confidence that it is not a bug: 8/10). Item 2 of the same comment says the frontend's body-sent task_execution_id arrives as None, so the CAS never runs. The route covers this case. routers/chat.py:1019-1020 reads
if not task_execution_id:
task_execution_id = execution_idand ExecutionDetail.vue:748-749 posts to /executions/${executionId.value}/terminate, the same id it puts in the ignored body field. The container arm therefore gets a non-empty task_execution_id, runs the CANCELLED CAS and fires on_ended when the CAS wins. No change is needed for the audit row. The body-vs-query mismatch is harmless dead weight in the frontend.
Clean Categories
- SQL / data safety: no new SQL. The rows go through the existing
platform_audit_service.log. - Concurrency: the terminate row is gated on each arm's CANCELLED CAS result (
cancel_won), not on the agent's answer. That covers the repeat case and the concurrent-terminate case, and the tests exercise the repeat case and the lost-CAS race against the backlog drain. The reset row is written after both resets (agents.py:1141-1143). - Auth boundaries: no gate changes. Reset stays
require_adminplusAuthorizedAgentByNameon{agent_name}, which is the correct pairing. Terminate staysget_authorized_agenton{name}. The refusal tests cover operator,ops, agent and outsider principals. - Credential exposure: the details hold only ids and states. Key id and name come from the principal (#2389), never the key value.
- Enterprise disclosure: the
security.mdaddition names two OSS actions and nothing enterprise. - Error handling:
logswallows its own write errors by design, so the hook cannot turn a successful terminate into a 500. The PR states this under "Unverified". - Enum completeness: both actions use existing
AuditEventTypemembers (CONFIGURATION, EXECUTION), andsecurity.md§20.1 lists them. - Side effects: public-link and Workspace callers pass no hook, so their behaviour is unchanged. The
on_ended=Nonedefault is kept at every layer.
Summary
- Critical: 0, none found.
- Informational: 2. I1 is an earlier finding that is still open and worth the two one-liners before merge. I2 is an earlier finding that does not reproduce.
- Scope: clean. Resolve the
tests/registry.jsonconflict withdev(keep both entries). The closing keyword stays with whichever of #4024 / #4035 merges second, as the PR body plans.
|
Resolve by merging |
|
Resolve by running |
Brings the branch onto origin/dev b900357. One conflict, tests/registry.json, where both sides prepend entries: both kept (594 entries, no duplicate file, every dev entry equal to dev's). This slice's diff against dev is unchanged: 7 files, +802/-11. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HWna79vVmnezQdhpaGRKbw
|
On item 2 of the 2026-10-28 comment above (the ExecutionDetail Stop door writing no row): checked, and it doesn't reproduce. On the closing keyword: #4024 carries |
Brings the branch onto origin/dev d200a70. The only conflict was tests/registry.json, where both sides prepend an entry; both are kept (599 entries, each equal to its source side). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HWna79vVmnezQdhpaGRKbw
…ws (#3180) Both new audit rows wrote actor_ip as NULL. platform_audit_service.log stores actor_ip only from its kwarg, and neither handler passed one, while the agent lifecycle writes in routers/agents.py do. - circuit-breaker reset takes `request: Request` and passes `actor_ip=request.client.host if request.client else None`, the same expression as the create/delete/start/stop rows. - terminate takes `request: Request = None` (audit context only, the repo's existing spelling for that role) so the direct callers in test_679 / test_1332 / test_ent875 keep calling it without one; the on_ended hook passes the same address. request.client.host is the canonical client address resolved by ClientAddressMiddleware (ent#873), so behind the proxy the row names the client, not the proxy. The two row helpers in test_3180_operational_write_audit.py now assert actor_ip equals the TestClient peer. Before this change 12 of 34 cases fail (4 reset, 8 terminate) with `assert None == 'testclient'`; after, 34 pass. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HWna79vVmnezQdhpaGRKbw
|
On item 1 of the 2026-10-28 merge-train comment (
|
Part of #3180
The sibling slice (#4024, the bounded
ops_writemachine credential) carries the closing keyword. If the sibling is about to merge while this PR is still open, the closing keyword moves to this PR by a gated body edit, so the issue never closes with these two writes unattributed.What changes
Two operator writes left no row in the append-only
audit_log. Each now writes exactly one, attributed to the authenticated principal, and only when the call succeeds:POST /api/agents/{agent_name}/circuit-breaker/resetcircuit_breaker_reset, detailsprior_transport_state/prior_dispatch_stateDISPATCH_BREAKER_ENABLEDor the per-agent toggle (the transport breaker trips with both off). Both resets are fail-open (with Redis refusing they log and return), so the row records the request, not a confirmed resetPOST /api/agents/{name}/executions/{execution_id}/terminateexecution_terminate, detailsexecution_id/task_execution_id/statusterminatedagain because it accepted but has not started it), a container kill of a turn whose row was already terminal, analready_finishedanswer (#679) and every error arm write noneplatform_audit_service.logalready derivesmcp_key_id/mcp_key_name/mcp_scopefrom the principal (#2389), so a reset or terminate made with an API key names the key. Both rows record the caller's address inactor_ip(request.client.host, the client address ent#873 resolves behind the proxy), the same way the agent create, delete, start and stop rows do. No gate, scope, route, schema or response changes.chat_execution_service.terminate_executiongains an optionalon_endedhook, awaited once and only from inside the arm that knows this call ended work: after that arm's CANCELLED CAS was won and after the activity closes. The operator route passes a hook that writes the row as the caller; the public-link and Workspace callers pass none, so their behaviour is unchanged. Docs: the two actions are added torequirements/security.md§20.1 Event Categories.One existing test is made order-independent:
tests/unit/test_ent873_audit_client_ip.py, fromdev. Its agent-refusal case overrodemain.get_current_user. The unit conftest evictsdependenciesafter each test, so once this PR's suite has importedrouters.agentsbeforemain,mainholds a newer copy than the one the route was built with. The override was then never consulted, and the call answered 401 instead of 403. That made the case red whenever this PR's suite ran first on the same worker; it was green alone. It now takes the dependency from the route's own globals, and its assertion is unchanged. #4024 carries the identical change, so whichever merges second merges it as a no-op.Independent of the sibling slice: nothing here imports or depends on it, and it is useful today (a person's or a
userkey's reset and terminate leave no row ondev).Journey Impact: none: adds two audit_log rows on existing operator routes; no response, gate, route or user-visible flow changes
Validation:
tests/unit/test_3180_operational_write_audit.py(new, 34 tests): realget_current_userwith a JWT or a key row minted in the real schema, the real routes, a real SQLite store, rows read back fromaudit_log. Faked only at the boundaries a unit run cannot reach: Redis (one fakeredis behind every breaker, token and in-flight lookup), the container lookup, the HTTP call into the container, the capacity slot release. A session and auser-scoped key each write one row with the right actor, email, caller address, key id and key name; refused calls (operator session and key,opskey, admin-owned agent key, outsider session, sibling agent key, no credential) and failed calls (a raising reset; agent 500 / 404 / unreachable / timeout; container stopped or missing) write none and leave the breaker or execution unchanged. With Redis refusing the reset (the real fail-open shape) the route answers 200, the breakers stay open and one row records the request. Terminate covers queued cancel, parked cancel and container-proxy terminate; a repeated parked cancel and a repeated container terminate (a session, then auserkey; both answer 200) each write exactly one row, the session's; a container terminate of a turn already FAILED writes none; a queued cancel that loses its CAS to the backlog drain writes only the row of the arm that ended the turn. Other shape: breaker feature off (DISPATCH_BREAKER_ENABLEDfalse, per-agent breaker disabled, transport breaker open) still writes the reset row. Pinned values differ from production defaults (flags on, breakers open, distinctive principals, fixed execution date).Result at d5f15f1: 34 passed. With the 32 existing terminate/reset unit files (
test_679_*,test_1332,test_1491,test_2106,test_2433_*,test_2795,test_3406,test_3588,test_ent155,test_ent279_*,test_terminate_async_executor,test_timeout_termination) under pytest-randomly seeds 1/2/3: 383 passed, 2 skipped each.Guards, unedited, at d5f15f1:
test_2323_machine_identities,test_293_admin_gate_rejects_agent_keys, everytest_2996_*file (the route census and its runtime sibling included),test_1310_auth_wiring,test_3102_host_header_fences: 399 passed. MCPtool-visibility.test.ts(theOPERATOR_SCOPESpin) and the whole MCP suite are untouched by this PR (no TypeScript change) and green in the batch below.test_2338_journey_catalog(registry consumer): 51 passed.Brought onto dev:
origin/devf00cca1 was merged into the branch, cleanly and as a merge rather than a rebase. Dev's enterprise submodule pointer bump (chore(enterprise): bump the submodule pointer to main (982735f -> bacec40) #3995) had made this branch's unchanged pointer read as a change. This PR's own diff against dev is the same after the merge as before it, apart from line offsets. On the head 4220a05, 64 files ran under-n 4 --dist loadfile -p randomlyon seeds 12345 and 67890: this PR's suite, the terminate/reset neighbours, the pinned guards, and dev's new sign-in and client-address files. Both seeds: 1,505 passed, 3 skipped.tests/lint_sys_modules.py: no new violations. Run alone after this PR's suite in one process, the ent#873 file is 41 / 41 green; it was 1 red before the fix.Batch: both slices of feat: a bounded, attributable machine credential for operational WRITES that survives enforced 2FA (write tier for the ops key scope) #3180 were merged with
--no-ffonto one integration branch offorigin/devf00cca1, with feat(auth): ops_write — a bounded machine credential for four operational writes (#3180) #4024 at 7ab23ea and this PR at 4220a05. The only conflict wastests/registry.json; both entries were kept (583 entries, each equal to its source side). The full backend unit tier (tests/unit -n 4 --dist loadfile -p randomly) ran on seeds 12345, 67890 and 99999: 29,934 passed and 5 failed on each. All 5 reds reproduce on pristineorigin/devf00cca1 with the same seed, so none was introduced:test_ent679_payments_pin_paritycases: local venv package versions vs the image pins;test_2850_skill_allowed_tools_forms::test_warning_truncates_the_offending_value: a log-line length that embeds the xdist tmp path.The three feat: a bounded, attributable machine credential for operational WRITES that survives enforced 2FA (write tier for the ops key scope) #3180 suites (38 / 30 / 34) and every pinned guard counted green in each seed's junit.
tests/lint_sys_modules.py: no new violations. Frontendnpm run test:unit: 415 files / 6,471 tests passed, the raw-colour and loading-gate ratchets included. MCP server: 1,002 passed,tsc --noEmitclean. This supersedes the batch onorigin/devf754dfb, which had the same 5 pre-existing reds and nothing else.gitleaks over the branch's commits: no leaks.
2026-10-29,
actor_ip(merge-train item 1, dolho's I1): both rows wroteactor_ipNULL. Fixed in 5f95b4f: the reset handler takesrequest: Request, the terminate handler takesrequest: Request = None(audit context only, so the direct callers intest_679_*,test_1332andtest_ent875_exec_control_routesneed not pass one), and both passactor_ip=request.client.hostwhen a client is present. Both row helpers in the suite now assert it. Against 4220a05 that is 12 of 34 red (4 reset, 8 terminate,assert None == 'testclient'); at 5f95b4f, 34 / 34. The branch also mergesorigin/devb900357 (553f554) and d200a70 (5832a72); each time onlytests/registry.jsonconflicted and both entries were kept (599 entries, each equal to its source side). The PR's diff against dev is still 7 files. At 5f95b4f, 63 files under-n 4 --dist loadfile -p randomly --randomly-seed=12345: this PR's suite, the terminate/reset neighbours, ent873/875/926, the pinned guards and route censuses, and the audit-log files: 1,434 passed, 4 skipped.tests/lint_sys_modules.py: no new violations;tests/lint_root_test_placement.py: OK. The full unit tier was not re-run locally at 5f95b4f; CI runs it.Mutation:
Run at d5f15f1 in a scratch copy through the real routes, each mutant applied alone and the files restored byte-identical after each. Baseline 34/34 green.
actor_user=None→ 4/34; reset row gated onDISPATCH_BREAKER_ENABLED(the other shape) → 1/34; reset row moved above the two resets → 1/34 (the ordering guard).on_endedhook → 8/34; terminateactor_user=None→ 8/34; terminate row written before the attempt → 16/34.cancel_wongate dropped → 1/34; container arm gated on the agent's answer instead of the CAS → 2/34.require_adminopened toget_current_user→ 3/34; terminate's agent-access dependency removed → 4/34 (the refusal tests are not vacuous).Fresh review: an independent reviewer (Claude Opus 5.5, extra-high effort, briefed with the slice's goal and the bar only) returned REVISE with one blocking finding, two should-fix and two nits. Every one is closed. The blocking one was that the container arm credited a repeat terminate to whoever sent it, because it trusted the agent's
terminatedanswer. It is fixed in b07118b, so the row now follows the CANCELLED CAS. The untested queued CAS gate and a leftover docstring note are also fixed in b07118b. The reset row's overstated wording is fixed in d5f15f1, which adds the real Redis-refusal test. The pre-existing false 200 on reset is out of an audit slice's scope and has a follow-up. One nit was declined with a reason: an earlier commit message, carried in the squash, says the termination service is unchanged, and rewriting it would re-hash the reviewed commits; the later commits in the same squash state the change.Unverified:
TEST_POSTGRES_URLunset); the PostgreSQL arm ofdb_backenddid not run.platform_audit_service.logswallows its own write errors by design (services/platform_audit_service.py:211-218); no test injects a write failure.chat_execution_service.pywere not checked. feat(access): hold an agent's turn to what its person may do, and declare every agent route's tier (ent#926) #3778 against today's dev already conflicts intests/registry.jsonon its own; this PR adds no conflict to that.terminatedby the agent; that only the CAS winner writes the row was shown for the repeat case, and the concurrent race was reasoned from the agent code, not executed.chat_execution_service.py).Not delivered
POST /api/monitoring/agents/{name}/check. Follow-up to be filed.closedwhen Redis refused the reset, because both resets are fail-open (pre-existing; the new row says it records the request). Follow-up to be filed.user-scoped admin key. When the sibling slice lands, four of them (circuit-breaker reset, agent start, agent stop, execution terminate) can move to its bounded key; these still need a person's session or auser-scoped admin key: fleet restart; retention acknowledge; schedule edit and toggle; schedule trigger; agent resources, capacity and timeout; every platform setting; subscriptions; user roles; the login whitelist; capability grants and skill managers; agent MCP-key regeneration; credential injection; creating and deleting agents; skill-library sources; chat and task dispatch; system-agent restart (and start/stop of the system agent); start/stop of ephemeral agents; emergency stop, fleet stop, schedule pause/resume; monitoring enable/disable; notification acknowledge/dismiss; health-check trigger. The toolkit'supdateandsubscriptionflows therefore keep a user-scoped key.Collisions
tests/registry.jsonboth slices prepend an entry, and whichever merges second keeps both. Intests/unit/test_ent873_audit_client_ip.pyboth carry the identical fix, which merges as a no-op. Both handlers andsecurity.mdmerge cleanly with it. Re-checked at 5f95b4f: against feat(auth): ops_write — a bounded machine credential for four operational writes (#3180) #4024's next head (45bd068, local, on dev b900357)git merge-treeconflicts only intests/registry.json. feat(auth): ops_write — a bounded machine credential for four operational writes (#3180) #4024's published head 7ab23ea predates dev's ent#875 merge and conflicts with today's dev itself independencies.py,routers/agents.pyandrouters/chat.py, with or without this PR.chat_execution_service.py. Its code merges cleanly with this PR's head (re-checked at feat(access): hold an agent's turn to what its person may do, and declare every agent route's tier (ent#926) #3778 e7460a0 against 5f95b4f). Its only conflict,tests/registry.json, is with today's dev and exists without this PR. Re-merge and re-run if it lands first.