Skip to content

feat(automation): three AI lines per tool action behind one approval enforcement point (#18430, #17463) - #18453

Open
mrveiss wants to merge 6 commits into
mainfrom
issue-18430-ai-lines
Open

mrveiss wants to merge 6 commits into
mainfrom
issue-18430-ai-lines

Conversation

@mrveiss

@mrveiss mrveiss commented Oct 12, 2026 •

Copy link
Copy Markdown
Owner

Thinking Path

Owner rule 2026-10-12 (#18430): for every action of every tool, AI has one of three lines (1 autonomous then report, 2 AI drafts and a person sends, 3 AI never touches). An unset line counts as line 2, and agents never change a line. #17463 found three approval policies of which one was enforced. They are one enforcement point, so one branch.

The design question was where the decision lives. Everything that used to decide on its own became an input that can only tighten: the approval baseline (#18294), the work item's declared categories, and the role policy needs_approval. The point is services/ai_line_enforcement.py; the chat seam and the agent terminal both call it, and a guard test fails if a second decision path appears.

What Changed

Single point. enforce_line (line 1 runs and reports, line 2 files a pending company-scoped approval and does not run, line 3 refuses). It fails closed: an unreadable line store refuses, an unreadable baseline gates every category, an approval that cannot be recorded still does not run.

Wired paths.

  • Chat tool seam: enforce_ai_line replaces the work-item hold (enforce_work_item_approval is retired, subsumed) and runs from _seam_guard_message (extracted from _dispatch_tool_call, same order, to stay inside the function-length limit).
  • Plan-time interrupt of the chat graph: _tool_call_needs_approval now asks the same resolver (call_needs_approval). A person's yes sets LLMIterationContext.person_approved (its own field, so a request cannot forge it); it satisfies line 2 and never line 3.
  • Agent terminal: services/agent_terminal/ai_line_gate.py, called from _check_auto_approval_or_queue. Line 2 uses the terminal's existing command queue (see gaps).

Store and API. llc_ai_lines (migration 098, single head), llc/services/ai_lines.py (one writer, record_approved_line, called only by the post-approval handler), GET /api/llc/ai-lines/{company_id}, POST /api/llc/ai-lines/{company_id}/changes (interactive person only, files an approval, changes nothing). ai_line_change_action applies an approved change only if the decider is an owner or admin of that company. api.ts regenerated with the pinned openapi-typescript (163 added lines, no other change).

#17463. needs_approval() and the ApprovalHandler wrapper now have their production caller (terminal gate). minimal guard profile is documented DEV-ONLY, and a guard fails if AgentLoop gains a production caller.

Found on the way, fixed here. POST /api/llc/approvals/{id}/decide raised "a transaction is already begun" for every real decision, because load_authorized begins the session's transaction and the handler then called session.begin() (its tests mock the session). A company-scoped approval therefore could not be decided at all. It now commits the open transaction, and it also runs the post-approval actions (it never did). Generic /approval-gates only decides unscoped approvals, so it cannot stand in.

Ratchets. Re-pinned chunker-private-hook-scan and store-fact-chokepoint floors mid-window (this PR adds 17 tracked files); lowered the size ceilings for graph.py, tool_handler.py, agent_terminal/service.py (all shrank).

Acceptance criteria

#17463 Evidence
role auto_approve_* honoured on the live path, test with auto_approve_safe=False services/agent_terminal/ai_line_gate.py:30, ai_line_terminal_18430_test.py::test_a_role_with_auto_approve_safe_false_does_not_auto_approve_a_safe_command
ApprovalHandler.needs_approval() has a production caller ai_line_gate.py (role_requires_approval=), pinned by ai_line_single_policy_18430_test.py::test_the_role_policy_is_consulted_by_the_terminal_and_only_there
minimal cannot disable approval on a reachable path, or dev-only plus a guard agent_loop/guard_profile.py:32 (DEV-ONLY), ..._test.py::test_the_agent_loop_has_no_production_caller_...
the planes enumerated in one place DECISION_FUNCTIONS in repo_tests/ai_line_single_policy_18430_test.py:56, and the module docstring of services/ai_line_enforcement.py
#18430 Evidence
line 2 creates an Approvals item and does nothing; line 3 refused at the execution layer ai_line_enforcement.py:225 (enforce_line); ai_line_enforcement_test.py::test_line_two_..., ::test_line_three_...; seam: test_ai_line_seam_18430.py
unset lines behave as line 2 autobot_shared/ai_lines.py:45; ::test_an_unset_mutating_action_behaves_as_line_two...
changing a line creates an approval request llc/api/ai_lines.py:102; test_ai_lines_api_18430.py::test_a_proposal_files_an_approval_and_changes_nothing
one enforcement point shared with the approval policies the guard test above
every tool action, process and meta skill carries a line; strictest-of for processes NOT delivered (tool actions only)
four-move setup flow, Turn on disabled with reason, "Send a test" NOT delivered (frontend session)

Verification

  • New tests: 11 pure (autobot_shared/ai_lines_test.py), 21 enforcement incl. real store and approvals, 12 API, 7 terminal, 7 chat seam, 10 guard definitions (13 collected), plus 1 graph-context test, all passing.
  • Round 9a re-run: 4376 passed (services, chat_workflow, tests/unit/chat_workflow, llc API, autobot_shared); ratchets, guards, router auth, reach_declarations and reach_scope: 686 passed. The wider run also showed 4 failures in api/voice_integration_test.py (live TTS worker returns 401, unrelated to enforcement) and flaky http_client_test / openai_provider_test / test_org_role_authority, all passing on re-run or independent of this change.
  • Earlier suites run green: tests/unit/chat_workflow + chat_workflow (1033 passed), services/agent_terminal (86), llc/tests + approval service/proposer/orphan chain + tests/migrations (3184 passed, 1 order-dependent failure test_org_role_authority::test_every_registered_adapter_has_a_decision, which passes alone: 29 passed), router auth coverage, symbol_fork_ratchet_17312, python_file_size_ratchet, ceiling_record_parity_17872, guard_reach_meta, reach_scope_claim_17844 alone (10 passed).
  • alembic heads: 20261012_098 (head). Branch is 0 behind main. Pre-push ran without an override.
  • Guard population: declare(... min_fraction=0.45), REACH.examined and REACH.completed both used, so it cannot pass on an empty scan.

Review round 9a (fixes in this PR)

Finding Fix Evidence
CRITICAL: one "yes" approved every later line-2 action; the interrupt showed only the first call The interrupt lists every call needing a person; request_approval stores a decision bound to the HMAC fingerprints of exactly those calls (chat_workflow/approval_binding.py); the seam lets a call through line 2 only if its fingerprint is bound. The decision is reset on every plan (_generate_response_planned), every no-approval batch and every turn (initialize_session). The client's resume payload contributes only approved/reason, never the bound set. test_approval_binding_18430.py: a second batch after a yes is held again, a call not shown is not approved, a new turn starts clean, forged payload keys do nothing
HIGH 1: company read from the client's context verified_company_id is overlaid from the authenticated org_id (apply_auth_role(context, role, org); the client's value is stripped). A missing or malformed company REFUSES any mutating action. The terminal uses the session's JWT org. test_ai_line_seam_18430.py::test_a_missing_or_malformed_verified_company_refuses_a_mutating_action, ::test_the_requests_own_company_claim_never_scopes_a_decision
HIGH 2: unknown tool ran as read-only is_mutating: unknown means mutating; only catalogue-known reads and the two conversation tools are free. The browser and extract read tools the seam dispatches were added to TOOL_EFFECTS (READ only) and the *_index variants aliased. ai_lines_test.py, test_ai_line_seam_18430.py::test_an_unknown_tool_is_held_when_unset_and_a_known_read_runs
HIGH 3: realtime and voice bypassed the point Both routes funnel through RealtimeMCPBridge.call_tool, which now calls enforce_line with the authenticated org. A2A: a2a/task_executor.py only calls orchestrator.process_request, and the orchestrator package has no tool, terminal or chat-dispatch references (grep), so A2A executes no tool itself and is not wired. The single-policy guard now closes the set of entry points. realtime_mcp_bridge_ai_line_18430_test.py; ai_line_single_policy_18430_test.py::test_exactly_the_known_entry_points_enforce_a_line
HIGH 4: decide ran any registered action run_post_approval_actions(..., only=); the LLC decide route passes only ai_line.set_line; any other action leaves a durable comment and is not executed. test_ai_lines_api_18430.py::test_the_decide_route_does_not_execute_any_other_registered_action
Owner decision: four eyes execute_line_change refuses the requester deciding their own line change when the company has two or more owners/admins; a single owner/admin may self-approve and the audit entry carries self_approval: true. A line-2 action may be approved by its requester, bound to the shown calls. three four-eyes tests in test_ai_lines_api_18430.py
LOW: fingerprint HMAC-SHA256 of the RAW call, keyed from the app signing secret (random per process when none is configured), never hardcoded. ai_line_enforcement_test.py::test_the_fingerprint_is_an_hmac_of_the_raw_call_and_is_not_the_plain_hash

To stay under the size ratchets the two chat SSE generators moved from api/chat.py to api/chat_streaming.py unchanged apart from the new auth_org_id argument.

Superseded by 9b below: a user with no company now has a personal scope instead of a refusal.

Review round 9b (fixes in this PR)

Personal scope (owner decision, #18430). A user with no company is no longer refused; they have a personal scope.

  • Same store: llc_ai_lines gained scope_kind (company | user); the id column holds the company's id or the user's id. Unique on (scope_kind, id, action_key). Migration 098 edited in place (not yet merged), number unchanged.
  • Same decision: resolve_scope(company, user) feeds the same resolve_line and enforce_line on the chat seam, the agent terminal and realtime/voice. Unset mutating actions are line 2, bound to the shown calls so the user approves their own action; known reads run; the baseline floor still applies.
  • A company that is present but malformed is still REFUSED and is never replaced by the user's scope. An absent company is the personal case.
  • Personal line changes: GET /api/llc/ai-lines/me, POST /api/llc/ai-lines/me/changes. Only that user can approve one (anyone else's approval is recorded but the line does not move); it is always audit-flagged self_approval. The signed-in user's id comes from the session, never the URL or the body.
  • The chat overlay carries verified_company_id and verified_user_id from the authenticated principal; the request's own company_id/user_id are stripped. The terminal resolves its verified owner's user id.

_handle_llc_tool scoped by the verified company only (and verified_user_id as the actor), and a session with none is refused before dispatch_llc_tool. Test: test_llc_tool_scope_18430.py.

Fingerprint key. shown_call_fingerprint is HMAC-SHA256 keyed from jwt_secret or secret_key. When neither is set the key falls back to a random per-process key, so approvals bound to a fingerprint must be given again after a restart (the safe direction: a pending approval can never be replayed against a different process).

Requested test Evidence
no-company user: mutating unset gives line 2, self-approvable ai_line_enforcement_test.py::test_a_user_with_no_company_gets_line_two_for_an_unset_mutating_action_and_can_approve_it; seam: test_ai_line_seam_18430.py::test_a_user_with_no_company_gets_line_two_and_can_approve_their_own_shown_call
a read runs ::test_a_user_with_no_company_runs_a_known_read
a forged company is still refused ::test_a_forged_company_is_still_refused_even_for_a_signed_in_user
personal change self-approvable, flagged, nobody else test_ai_lines_api_18430.py::test_the_person_can_approve_their_own_personal_change_and_it_is_flagged, ::test_nobody_else_can_approve_a_personal_change
LLC tool uses the verified company, refuses without one test_llc_tool_scope_18430.py

Review round 9c (fixes in this PR)

Finding Fix Evidence
HIGH: four-eyes bypass. POST /llc/approvals took any payload and any requester, so admin A could create an ai_line.set_line approval naming B as requester and decide it as a non-self ai_line.set_line is registered server_only; BOTH create routes (LLC POST /llc/approvals, generic /approval-gates) refuse it with 403. Only propose_line_change and /me/changes create one, with the requester from the verified session. A missing or unparseable requester, or a missing verified decider, now REFUSES the decision instead of reading as "not self". test_ai_lines_api_18430.py: test_a_set_line_approval_cannot_be_created_through_the_llc_route_..., ..._generic_route, test_a_decision_on_an_approval_with_no_parseable_requester_is_refused, test_the_handler_refuses_without_a_verified_decider_or_requester; guard test_the_line_change_cannot_be_forged_...
MED: decided_by was parsed as a UUID before a username lookup, so a username spelling another user's undashed UUID could decide that user's personal lines The handler never parses a username. Each decision route records the VERIFIED decider id for that decision (set_verified_decider: ctx.user_id on the LLC route, the principal's id on the generic route) and the handler compares ids only. The username lookup is gone. test_a_username_that_spells_another_users_id_does_not_decide_that_users_personal_lines

Not covered here: other registered post-approval actions are still creatable through the generic route (they predate this PR); only ai_line.set_line is marked server-only. The A2A gap is tracked on #16950.

Voice upload from .m4a (#18477, joined because this PR already edits api/voice.py)

POST /api/voice/voices/create forwarded the upload unchanged and the worker (libsndfile) cannot read AAC/m4a, so a phone voice memo ended as a 500.

  • media/audio/reference_wav.py::to_reference_wav: a WAV (judged by its RIFF/WAVE magic bytes) passes through byte-for-byte; anything else is converted to PCM mono WAV by the EXISTING bounded FFmpegService (get_ffmpeg_service().extract_audio, extension allowlist, pinned demuxer, duration cap AUTOBOT_VOICE_REFERENCE_MAX_SECONDS, 24 kHz mono). No second converter. read_capped_audio still runs first.
  • Undecodable audio is 422 voice.unsupported_audio; an extension outside the allowlist is 415 with the same code; a missing ffmpeg is 503 voice.converter_unavailable. None is a 500.
  • Worker (tts-worker.py.j2 create_voice): a libsndfile decode failure returns 422 with the reason; everything else is unchanged. The client maps a worker 422 to the same 4xx.
  • Tests (media/audio/reference_wav_test.py, 15): an m4a reaches the worker as WAV, a WAV is untouched, garbage gets the 4xx and never reaches the worker, the size cap still applies first, the worker template branch, the client's 422 mapping, and two tests that run the real FFmpegService (a generated AAC sample becomes mono WAV; garbage is 422; skipped without ffmpeg).

Resubmit rewrote what was reviewed (HIGH; Refs #17349, not closed here)

POST /approval-gates/{id}/resubmit wrote body.context verbatim and accepted anyone, so it routed around the create-route refusal: create something harmless, request revision, resubmit with ai_line.set_line, decide it. The same hole let a requester swap the target of any registered action between a reviewer's look and approval.

  • context_change_violation (services/approval_execution.py): a resubmit may not change context.action, and for an approval naming a registered action it may not change ANY context key except the free-text keys that action declares as ignored (free_text_keys, none by default; ai_line.set_line declares none). An approval naming no action keeps a freely editable context.
  • A changed action goes through _reject_unregistered_action first, so an unregistered or server-only action gets the same status and detail as on create (400 / 403); an allowed-but-changed one is 403 context.<key> cannot change on resubmit.
  • Only the original requester may resubmit: the caller's verified id or username must equal requested_by_agent (or its part after the last colon, the form server proposals use). A request with no recorded requester can be resubmitted by nobody.
  • Tests (llc/tests/test_approval_resubmit_17349.py): harmless approval resubmitted as set_line gives 403 and no transition; a real personal set_line proposal with a changed line, action_key or scope_id gives 403 and the context is untouched, while a description-only resubmit works; a non-requester gets 403; no recorded requester gives 403; an unregistered action gives 400 with the create-route detail; a registered destructive action cannot have its collection swapped; an ordinary approval still resubmits with a new context. The single-policy guard fails if the route stops calling _guard_resubmit.
#17349 AC Evidence
resubmit rejects an unregistered context.action through the create path, same status and detail test_an_unregistered_action_is_refused_on_resubmit_as_on_create
a test asserts the exact refusal and no state transition status codes asserted exactly; test_a_harmless_approval_cannot_be_resubmitted_as_a_set_line_proposal asserts it stays revision_requested
an authorship check governs who may resubmit test_only_the_requester_can_resubmit, ..._no_recorded_requester_...
a registered destructive action cannot change its target on resubmit test_a_registered_destructive_action_cannot_have_its_target_swapped

Known gaps

Closes #17463
Closes #18477
Refs #17349
Refs #18430

Single-issue rationale: #17463 and #18477 are fully delivered (#18477 rides here because this PR already edits api/voice.py); #18430 is the umbrella and stays open for the frontend half, process-level lines and the setup flow.

Model Used

Claude Sonnet 5.5 (claude-sonnet-5-5)

🤖 Generated with Claude Code

@github-actions

Copy link
Copy Markdown
Contributor

Notice: 29 open PRs — past the runaway threshold (25)

This is not a request to defer this PR. The actual limit is the open-PR cap (AUTOBOT_OPEN_PR_CAP, default 40), enforced at pre-push against NEW branches only -- pushing more commits to this PR's own branch is never blocked by it.

This notice only means the count is high enough to be worth a glance for a runaway — something opening PRs in a loop, or a merge pipeline that has stalled so nothing is draining.

Currently open:

If the queue is draining normally, ignore this. Otherwise:

  1. Check whether CI is dispatching at all — see the ci-dispatch-watchdog status on these PRs
  2. Merge the ones whose CI has finished and review has passed: gh pr merge <number> --squash --delete-branch
  3. Look for a loop opening near-identical PRs

Warn-only runaway detector — .github/workflows/pr-queue-gate.yml. It never blocks a merge.

@coderabbitai

coderabbitai Bot commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough

Walkthrough

The pull request adds three AI action lines, company-scoped line settings, approval-based line changes, and shared enforcement for chat tools and agent-terminal commands. It also adds tests and repository checks for the new policy and its enforcement paths.

Changes

AI-line approval policy

Layer / File(s) Summary
Define AI-line resolution
autobot_shared/ai_lines.py, autobot_shared/tool_catalogue.py, autobot_shared/ai_lines_test.py
Defines autonomous, draft-then-send and never lines, with action-key handling, resolution rules and approval-category matching. Adds tests for parsing, defaults, fallbacks and policy floors.
Store and approve line changes
autobot-backend/llc/models/*, autobot-backend/llc/services/ai_lines.py, autobot-backend/llc/api/*, autobot-backend/services/ai_line_change_*, autobot-backend/services/ai_line_view.py, autobot-backend/migrations/versions/20261012_098_llc_ai_lines.py, autobot-backend/services/approval_gate_service.py, autobot-backend/llc/tests/test_ai_lines_api_18430.py, autobot-backend/api/approval_gates.py
Adds per-company line storage and endpoints to read settings or propose changes. An approved proposal records the line and decision metadata. The API tests cover reads, proposals and approval outcomes.
Enforce AI lines for chat tools
autobot-backend/services/ai_line_enforcement.py, autobot-backend/chat_workflow/*, autobot-backend/tests/unit/chat_workflow/*, autobot-backend/services/ai_line_enforcement_test.py
Adds shared resolution and enforcement outcomes, then uses them in chat planning and tool dispatch. Approval state is taken from the graph decision, and tests cover line outcomes, holds and refusals.
Enforce AI lines for terminal commands
autobot-backend/services/agent_terminal/*, autobot-backend/services/agent_terminal/ai_line_terminal_18430_test.py
Resolves each command’s AI line before existing auto-approval and queue handling. Tests cover refusal, queued approval and execution conditions.
Check policy wiring and dormant paths
autobot-backend/agent_loop/guard_profile.py, repo_tests/*
Adds checks for policy call sites, line-writing ownership and the dormant AgentLoop path. Updates repository scan reach declarations.

Priority: ➖ Normal

Merge Risk: 🟡 Moderate · up to f42a9

The new approval lines can be bypassed in some cases. One approval in chat can run other held actions the person never saw. In the terminal, auto-approval rules can run line-2 commands, and a queued command still runs after its line changes to "never". Close these gaps before merging.

Pre-merge checks | Passed 3 | Failed 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Out of Scope Changes check Warning The PR includes substantial work not required by #17463. This includes the chat-tool enforcement seam, company AI-line storage and API, migration 098, line-change approval actions, and related approva… Split the unrelated #18430 implementation into a separate pull request linked to that work. Keep this pull request limited to the #17463 terminal policy integration, the minimal profile protection, the single-policy enumeration, and their…
Docstring Coverage Warning Docstring coverage is 35.96% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 178 functions across 45 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Linked Issues check Passed #17463 has coding requirements. The terminal gate calls ApprovalHandler.needs_approval() and passes the role result into enforce_line. The reported tests cover auto_approve_safe=False and the SA…
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: three AI lines for tool actions controlled by one approval enforcement point.

Full details: Out of Scope Changes check

Explanation

The PR includes substantial work not required by #17463. This includes the chat-tool enforcement seam, company AI-line storage and API, migration 098, line-change approval actions, and related approval transaction changes. The PR itself attributes these changes to the broader #18430 work, which is not a directly linked issue target in the supplied issue set.

Resolution

Split the unrelated #18430 implementation into a separate pull request linked to that work. Keep this pull request limited to the #17463 terminal policy integration, the minimal profile protection, the single-policy enumeration, and their supporting tests and documentation.


  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR

🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown
Contributor

✅ SSOT Configuration Compliance: Passing

🎉 No new hardcoded values of either class — ssot and other both block.

Known backlog in pipeline-scripts/hardcoded_values_baseline.txt is suppressed and tracked in #14371.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8


ℹ️ Review info
⚙️ Run configuration
  • Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 3cdd5ce3-8371-4518-9250-191bf866f2ba
📥 Commits

Reviewing files that changed from the base of the PR and between df211d3 and f42a958.

⛔ Files ignored due to path filters (3)
  • autobot-frontend/src/types/generated/api.ts is excluded by !**/generated/**
  • repo_tests/python_file_size_ratchet_baseline.py is excluded by !repo_tests/python_file_size_ratchet_baseline.py
  • scripts/python_file_size_known_large.py is excluded by !scripts/python_file_size_known_large.py
📒 Files selected for processing (45)
  • autobot-backend/agent_loop/guard_profile.py
  • autobot-backend/api/approval_gates.py
  • autobot-backend/chat_workflow/delegation_laundering_test.py
  • autobot-backend/chat_workflow/graph.py
  • autobot-backend/chat_workflow/manager.py
  • autobot-backend/chat_workflow/models.py
  • autobot-backend/chat_workflow/session_role_test.py
  • autobot-backend/chat_workflow/tool_dispatch_guards.py
  • autobot-backend/chat_workflow/tool_handler.py
  • autobot-backend/llc/api/__init__.py
  • autobot-backend/llc/api/ai_lines.py
  • autobot-backend/llc/api/approvals.py
  • autobot-backend/llc/models/__init__.py
  • autobot-backend/llc/models/ai_line.py
  • autobot-backend/llc/services/ai_lines.py
  • autobot-backend/llc/tests/test_ai_lines_api_18430.py
  • autobot-backend/migrations/versions/20261012_098_llc_ai_lines.py
  • autobot-backend/services/agent_terminal/ai_line_gate.py
  • autobot-backend/services/agent_terminal/ai_line_terminal_18430_test.py
  • autobot-backend/services/agent_terminal/service.py
  • autobot-backend/services/ai_line_change_action.py
  • autobot-backend/services/ai_line_change_proposal.py
  • autobot-backend/services/ai_line_enforcement.py
  • autobot-backend/services/ai_line_enforcement_test.py
  • autobot-backend/services/ai_line_view.py
  • autobot-backend/services/approval_gate_service.py
  • autobot-backend/tests/unit/chat_workflow/test_ai_line_seam_18430.py
  • autobot-backend/tests/unit/chat_workflow/test_ai_line_support.py
  • autobot-backend/tests/unit/chat_workflow/test_approval_baseline_18294.py
  • autobot-backend/tests/unit/chat_workflow/test_approval_gate_interrupt.py
  • autobot-backend/tests/unit/chat_workflow/test_governed_execution.py
  • autobot-backend/tests/unit/chat_workflow/test_graph_context_threading.py
  • autobot-backend/tests/unit/chat_workflow/test_tool_handler_builtin_route.py
  • autobot-backend/tests/unit/chat_workflow/test_tool_handler_extract_content.py
  • autobot-backend/tests/unit/chat_workflow/test_tool_handler_pre_action_verifier.py
  • autobot-backend/tests/unit/chat_workflow/test_tool_handler_read_spilled_output.py
  • autobot-backend/tests/unit/chat_workflow/test_tool_handler_repetition_guard.py
  • autobot-backend/tests/unit/chat_workflow/test_tool_handler_web_research.py
  • autobot_shared/ai_lines.py
  • autobot_shared/ai_lines_test.py
  • autobot_shared/tool_catalogue.py
  • repo_tests/ai_line_single_policy_18430_test.py
  • repo_tests/chunker_private_embedding_hooks_18185_test.py
  • repo_tests/reach_scope_claim_17844_test.py
  • repo_tests/store_fact_chokepoint_guard_test.py

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

Comment on lines +61 to +66
def parse_line(value: Any) -> Optional[AiLine]:
"""``AiLine`` for a stored/wire value, ``None`` for anything else (never a guess)."""
try:
return AiLine(value)
except ValueError:
return None

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

Catch TypeError in parse_line as well as ValueError.

AiLine(value) raises ValueError for an unhashable value such as a list or dict. The enum lookup can raise TypeError for some inputs. The docstring says the function returns None "for anything else". Catching both exceptions keeps that contract for wire and JSON-context values, for example approval.context["line"].

Proposed fix
-    except ValueError:
+    except (ValueError, TypeError):
         return None
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def parse_line(value: Any) -> Optional[AiLine]:
"""``AiLine`` for a stored/wire value, ``None`` for anything else (never a guess)."""
try:
return AiLine(value)
except ValueError:
return None
def parse_line(value: Any) -> Optional[AiLine]:
"""``AiLine`` for a stored/wire value, ``None`` for anything else (never a guess)."""
try:
return AiLine(value)
except (ValueError, TypeError):
return None
🧰 Tools
🪛 Ruff (0.16.8)

[warning] 61-61: Dynamically typed expressions (typing.Any) are disallowed in value

(ANN401)

Comment thread autobot-backend/chat_workflow/graph.py Outdated
Line 3 is refused and line 1 runs; only line 2 reaches the rules and this terminal's queue.
Returns ``(is_auto_approved, queue_or_refusal_response)``.
"""
verdict = await resolve_command_line(self.approval_handler, session, command, risk)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift

Recheck the AI line before executing a queued command.

This check runs when the command enters the queue. If the company changes the command to line three while approval is pending, approve_command still reaches _run_tracked without another line check. Re-resolve the line in the approval path and refuse execution when the current outcome is REFUSE.

verdict = await resolve_command_line(self.approval_handler, session, command, risk)
if verdict.outcome is Outcome.REFUSE:
return False, session.get_permission_denied_response(verdict.message, command, risk.value)
if verdict.outcome is Outcome.RUN:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Do not apply auto-approval rules to a line-two command.

When enforce_line returns a hold outcome, this branch continues to check_auto_approve_rules. A matching rule then returns True and runs the command without human approval. Send every hold outcome directly to the approval queue. Test that an existing auto-approval rule cannot bypass line two.

Comment thread autobot-backend/services/ai_line_change_proposal.py Outdated
Comment on lines +163 to +173
async def _existing_hold(session, company_id: Optional[uuid.UUID], requester: str, fingerprint: str) -> Approval | None:
scope = Approval.company_id == company_id if company_id else Approval.company_id.is_(None)
rows = await session.execute(
select(Approval).where(
scope,
Approval.status == ApprovalStatus.PENDING.value,
Approval.requested_by_agent == requester,
Approval.context["fingerprint"].as_string() == fingerprint,
)
)
return rows.scalars().first()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low value

Two identical line-2 holds that run at the same time can file two approvals.

_file_hold does a check (_existing_hold) and then an insert (create_approval). Nothing locks the rows between these two steps, and no unique constraint covers the fingerprint. Two chat turns, or a retry, that run at the same time can both see no pending row. Each one then files its own approval. The impact is limited to a duplicate item in Approvals, because neither call runs the action.

If deduplication must hold under load, add a partial unique index on (company_id, requested_by_agent, context->>'fingerprint') where status = 'pending'. On a conflict, re-read the existing row.


def test_the_dispatch_seam_asks_the_lines_and_nothing_else_for_an_approval_hold() -> None:
dispatch = _sources()[_DISPATCH]
assert "_enforce_ai_line(" in dispatch

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Check for a dispatch call, not a text fragment.

If the dispatch call is removed but def _enforce_ai_line( remains, this assertion still passes. Use calls_of(dispatch, "_enforce_ai_line"). Add a negative fixture with only the definition and a positive fixture with a call. As per path instructions, “Every detector needs a contrast pair: a fixture that SHOULD trip it and one that should not.”

Source: Path instructions

callers = {
rel
for rel, src in _sources().items()
if not rel.startswith("autobot-backend/agent_loop/") and calls_of(src, "AgentLoop")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Include production callers inside the AgentLoop package.

If a production module under autobot-backend/agent_loop/ instantiates AgentLoop, this filter excludes it and the dormancy guard passes. Scan that package too. Exclude only specific, justified call sites if necessary.

@mrveiss

mrveiss commented Oct 12, 2026

Copy link
Copy Markdown
Owner Author

Security review (Opus) at f42a958: blocked. Two defects let a user run line-2 and line-3 actions without a person's approval, and three gaps leave the line system advisory on other paths. The migration renumber (20261012_098) is owner-sequenced; noted, not blocking.

Severity Location Finding Fix
Critical chat_workflow/graph.py:448, :948 approval_decision is never cleared. It is not in initial_state (manager.py:3814), not in execute_tools's return (graph.py:1178), and not in the per-turn reset (~graph.py:248). The Redis checkpointer persists it per thread, so one "approved" makes person_approved=True for every later iteration and every later turn in that session, and all later line-2 actions run unasked. Also, the interrupt shows only the first needs_approval call (graph.py:917), but its yes approves the whole batch. Reset approval_decision after execute_tools and at turn start, and bind the yes to the fingerprints of exactly the calls the person was shown.
High tool_dispatch_guards.py:318, ai_line_enforcement.py:113 company_id comes from ctx.context, the client-supplied request bag (session_work_item.py:88-91 says so). A user can name a company with looser lines, or send none or a malformed one (_parse_company gives None, "unscoped"). The company's stored never is then skipped, the action falls to line 2, and the user approves it: a line-3 bypass. Use the server-verified company (the session's work-item binding or TenantContext), and refuse on a malformed id.
High autobot_shared/ai_lines.py is_mutating → tool_catalogue.py:291 A tool missing from TOOL_EFFECTS (external MCP or plugin tools) counts as read-only, so it runs on line 1. The tool's own catalogue entry decides, and an unknown effect fails open. Unknown effect = mutating, so it lands on line 2.
High api/realtime_session.py:233, api/voice.py:116 Realtime and voice tool calls reach bridge.call_tool without enforce_line, and realtime /tools/call shows no per-route auth. Not traced: A2A (a2a/task_executor.py:434). Route these through enforce_line.
High (class #17161) llc/api/approvals.py:183, :87 decide now runs run_post_approval_actions, and LLC POST /approvals lets any company member create an approval with any payload (no _reject_unregistered_action), then decide it themselves. Example: legacy_transcript_delete with a chosen session_id (legacy_transcript_disposal_action.py:44), whose handler has no decider check. This is a second path into the open #17161 defect. Fix once under #17161 (the dispatcher verifies the registered proposer). Until then, this route dispatches only ai_line.set_line.
Medium ai_line_change_action.py:56, graph.py:448 Self-approval. An admin can propose and approve their own line change, and in chat the prompter approves their own line-2 action with no company-role check. Owner decision needed: does "a person sends" mean someone other than the requester?
Low ai_line_enforcement.py:185 The fingerprint is over redacted params, so calls that differ only in their secrets share one hold. Not verified: whether redact_nested catches a credential in a free-text command. Fingerprint the raw params with HMAC; store only the redacted copy.

Verified fine:

  • Atomic decision: SELECT … FOR UPDATE plus a PENDING check (llc/services/approval.py:140-146); a second call gets 409, never a double run.
  • Transaction fix: commit or rollback on the open transaction instead of begin().
  • execute_line_change: requires APPROVED, a company, and require_company_admin; agents get 403.
  • Fail-closed: an unreadable baseline gates everything and an unreadable store resolves to never (except the malformed-company case above).
  • Baseline relaxation: SLM-only (approval_baseline.py:41).
  • Redaction: the canonical redact_nested.
  • Agent terminal: gated, and the role policy can only tighten.

…e verified company, close the voice and realtime bypass (#18430, #17463)
@mrveiss

mrveiss commented Oct 12, 2026

Copy link
Copy Markdown
Owner Author

Security re-review (Opus) at 310c553: blocked on one High. The Critical and all four previous Highs are fixed. The migration renumber is owner-sequenced; noted, not blocking.

Severity Location Finding Fix
High services/ai_line_change_action.py _requester_id/_four_eyes, reached via llc/api/approvals.py request_approval (POST /llc/approvals) and decide_approval (only={LINE_CHANGE_ACTION}) Four-eyes bypass. The LLC create route accepts any payload as the approval context and any requested_by_agent_id. Admin A of a 2-admin company (1) posts payload={"action":"ai_line.set_line","action_key":"<tool>","line":"autonomous"} with requested_by_agent_id = admin B, then (2) decides it via /llc/approvals/{id}/decide, which still allows exactly this action. decider(A) != requester(B), so it isn't treated as a self-approval: the line is written with no second person and no self_approval flag. The same happens when the requester doesn't parse (None counts as "not self"). Run ai_line.set_line only for approvals created by propose_line_change, using a server-only marker client routes cannot write. Better: both the LLC and generic create routes reject context.action == ai_line.set_line. An unparseable requester refuses. Add a regression test for the forged-requester path.
Medium ai_line_change_action.py _decider_id It tries uuid.UUID(decided_by) before the username lookup. Usernames match ^[a-zA-Z0-9_]+$, so a 32-hex username parses as a UUID. User X named after victim V's undashed UUID can create a generic approval (scope_kind=user, scope_id=V) and approve it, relaxing V's personal lines. Record the decider's verified user id on both routes; never parse a username as an id.
Low _admin_count Counted server-side at decision time. An admin who demotes the others becomes "single admin" and self-approves, flagged as the owner rule allows. Optional: surface self_approval=true changes in Approvals.
Gap a2a/task_executor.py:434 → orchestrator.process_request Not determined whether that path reaches command-executing agents outside enforce_line. The single-policy guard's pinned entry points don't cover them. Filed separately (see below).

Verified fixed:

  • Critical:
    • approved_fingerprints is a dataclass field, set only from state.approval_decision.bound, which bind_decision computes server-side (the client contributes only approve/reason).
    • It is reset each turn (initialize_session), on each new plan, and when there is nothing to show; legacy/fallback defaults to an empty set.
    • The HMAC covers the raw action_key + params, and the binding and dispatch params are identical.
    • Every needs_approval call is bound and shown.
    • The key comes from jwt_secret/secret_key (stable across workers); with no secret it is per-process random, which fails closed.
  • High 1 and the personal scope:
    • principal_scope reads only current_user, and apply_auth_role strips client keys.
    • resolve_scope uses the company first. A malformed company refuses rather than falling back to the personal scope, and a company member cannot drop their org_id.
    • LLC tools refuse without a verified company.
  • High 2 and High 3: an unknown tool counts as mutating, and realtime and voice call enforce_line with the authenticated org.
  • High 4: the decide route runs only ai_line.set_line, subject to the High above.
  • /me scope: the baseline and role floors apply regardless of scope; a personal line cannot go below the baseline and is ignored when a company exists.
  • chat_streaming.py: behaviour-preserving.
  • Voice with a non-UUID id: no scope, so mutating calls are refused and reads run. That is fail-closed and acceptable.

@mrveiss

mrveiss commented Oct 12, 2026

Copy link
Copy Markdown
Owner Author

Security delta review (Opus, verified by the merger from the entry point) at f79a146: blocked on one High. The server_only check runs only on the two create routes; resubmit can still write ai_line.set_line into an existing approval's context, which brings the four-eyes bypass back. The #18477 ffmpeg path is sound.

Severity Location Finding Fix
High api/approval_gates.py:346-363 → services/approval_gate_service.py:177 Four-eyes bypass via resubmit. Verified from the entry point: core_routers.py:523 mounts approval_gates_router with no router dependencies; resubmit depends only on get_current_user and passes body.context straight through; the service writes approval.context = context verbatim. There is no _reject_unregistered_action and no is_server_only_action check. Attack, as admin A of company X: (1) POST /llc/approvals with a harmless payload and requested_by_agent_id = B; the create check passes because there is no action yet. (2) POST /approval-gates/{id}/request-revision. (3) POST /approval-gates/{id}/resubmit with context={"action":"ai_line.set_line","line":…,"action_key":…}. (4) A decides via either route; decider(A) != _requester_id(B), so _four_eyes passes and the line changes with no second person and no flag. The same route lets anyone rewrite a real proposal's line/action_key after review. This class is tracked as open #17349. Call _reject_unregistered_action(body.context) (which now covers server_only) in resubmit. Better, refuse any resubmit that adds or changes context.action. Either fix #17349 in this PR or make it blocked_by of #17463. Add a test: create, request revision, resubmit with the line action, expect 403 and nothing stored.
Low llc/api/approvals.py:98 body.payload.get("action") assumes a dict; the schema is Dict[str, Any], so it is safe today. None.

Verified:

  • Other create paths set fixed, server-side contexts with no caller-chosen action: LLC agent_tools._request_approval, ai_line_enforcement:222, ai_line_change_proposal:53, the admin orphan-storage and oauth-strip gates, compose_tool_handler, legacy_transcript_disposal, collection_space_proposal, sprints:759, workspace_lease_sweep, finding_proposal_service, sprint_autoclose.
  • No action smuggling: is_server_only_action and the dispatcher (approval_execution.py:153) both match exactly, so a case or whitespace variant never dispatches.
  • Verified decider: the generic route uses _user_uuid(current_user) (approval_gates.py:259) and the LLC route _verified_decider(ctx, body) (llc/api/approvals.py:191). The handler refuses on a None decider or requester, and no other path calls run_post_approval_actions.
  • bug(voice): uploading a new voice from an .m4a (or other compressed) file fails with HTTP 500 — the TTS worker only reads formats libsndfile supports #18477 (ffmpeg on untrusted uploads):
    • argv is built via create_subprocess_exec with no shell, from a NamedTemporaryFile path; only an allowlisted suffix reaches it (reference_wav.py:57-61).
    • -protocol_whitelist file and -f are pinned per extension (ffmpeg_service.py:35-54,157-182), so concat/HLS/m3u8 demuxers are impossible.
    • Size is capped before decode, -t caps duration, and a 300s kill-and-reap timeout applies; temp files are removed in finally.
    • Clients see generic errors; stderr and paths go only to logs.

@mrveiss

mrveiss commented Oct 12, 2026

Copy link
Copy Markdown
Owner Author

Delta check at 0390f90: approve. The resubmit bypass is closed.

  • context_change_violation: context.action can never change, including None → an action (:100), so a harmless approval cannot become ai_line.set_line. For a registered action, every field except its declared free-text keys is frozen (:104-107). A changed action routes through _reject_unregistered_action (400/403).
  • Tests (7): harmless→set_line gives 403; a changed line/action_key/scope_id gives 403 with the context untouched; a description-only change succeeds; a non-requester or missing requester gives 403; a destructive target can`t be swapped. The guard pins the route call.

Merger`s call on the username question: acceptable as a LOW. With the action and every reviewed field locked by the guard, the requester match only decides who may edit description and free text, never what executes. It is still username-as-identity, so a follow-up should match on the verified user id only, the same as the decide fix. The migration renumber stays owner-sequenced after #18404/#18444.

Queued for the train after #18444 (migration chain).

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant