Repository navigation
Conversation
📝 Walkthrough
Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to 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 |
|
✅ SSOT Configuration Compliance: Passing🎉 No new hardcoded values of either class — Known backlog in |
There was a problem hiding this comment.
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
⛔ Files ignored due to path filters (3)
autobot-frontend/src/types/generated/api.tsis excluded by!**/generated/**repo_tests/python_file_size_ratchet_baseline.pyis excluded by!repo_tests/python_file_size_ratchet_baseline.pyscripts/python_file_size_known_large.pyis excluded by!scripts/python_file_size_known_large.py
📒 Files selected for processing (45)
autobot-backend/agent_loop/guard_profile.pyautobot-backend/api/approval_gates.pyautobot-backend/chat_workflow/delegation_laundering_test.pyautobot-backend/chat_workflow/graph.pyautobot-backend/chat_workflow/manager.pyautobot-backend/chat_workflow/models.pyautobot-backend/chat_workflow/session_role_test.pyautobot-backend/chat_workflow/tool_dispatch_guards.pyautobot-backend/chat_workflow/tool_handler.pyautobot-backend/llc/api/__init__.pyautobot-backend/llc/api/ai_lines.pyautobot-backend/llc/api/approvals.pyautobot-backend/llc/models/__init__.pyautobot-backend/llc/models/ai_line.pyautobot-backend/llc/services/ai_lines.pyautobot-backend/llc/tests/test_ai_lines_api_18430.pyautobot-backend/migrations/versions/20261012_098_llc_ai_lines.pyautobot-backend/services/agent_terminal/ai_line_gate.pyautobot-backend/services/agent_terminal/ai_line_terminal_18430_test.pyautobot-backend/services/agent_terminal/service.pyautobot-backend/services/ai_line_change_action.pyautobot-backend/services/ai_line_change_proposal.pyautobot-backend/services/ai_line_enforcement.pyautobot-backend/services/ai_line_enforcement_test.pyautobot-backend/services/ai_line_view.pyautobot-backend/services/approval_gate_service.pyautobot-backend/tests/unit/chat_workflow/test_ai_line_seam_18430.pyautobot-backend/tests/unit/chat_workflow/test_ai_line_support.pyautobot-backend/tests/unit/chat_workflow/test_approval_baseline_18294.pyautobot-backend/tests/unit/chat_workflow/test_approval_gate_interrupt.pyautobot-backend/tests/unit/chat_workflow/test_governed_execution.pyautobot-backend/tests/unit/chat_workflow/test_graph_context_threading.pyautobot-backend/tests/unit/chat_workflow/test_tool_handler_builtin_route.pyautobot-backend/tests/unit/chat_workflow/test_tool_handler_extract_content.pyautobot-backend/tests/unit/chat_workflow/test_tool_handler_pre_action_verifier.pyautobot-backend/tests/unit/chat_workflow/test_tool_handler_read_spilled_output.pyautobot-backend/tests/unit/chat_workflow/test_tool_handler_repetition_guard.pyautobot-backend/tests/unit/chat_workflow/test_tool_handler_web_research.pyautobot_shared/ai_lines.pyautobot_shared/ai_lines_test.pyautobot_shared/tool_catalogue.pyrepo_tests/ai_line_single_policy_18430_test.pyrepo_tests/chunker_private_embedding_hooks_18185_test.pyrepo_tests/reach_scope_claim_17844_test.pyrepo_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.
| 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 |
There was a problem hiding this comment.
🎯 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.
| 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)
| 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) |
There was a problem hiding this comment.
🔒 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: |
There was a problem hiding this comment.
🔒 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.
| 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() |
There was a problem hiding this comment.
🗄️ 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 |
There was a problem hiding this comment.
🎯 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") |
There was a problem hiding this comment.
🎯 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.
|
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 (
Verified fine:
|
…cope the LLC tools by the verified company (#18430)
|
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.
Verified fixed:
|
|
Security delta review (Opus, verified by the merger from the entry point) at f79a146: blocked on one High. The
Verified:
|
|
Delta check at 0390f90: approve. The resubmit bypass is closed.
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). |
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 isservices/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.
enforce_ai_linereplaces the work-item hold (enforce_work_item_approvalis retired, subsumed) and runs from_seam_guard_message(extracted from_dispatch_tool_call, same order, to stay inside the function-length limit)._tool_call_needs_approvalnow asks the same resolver (call_needs_approval). A person's yes setsLLMIterationContext.person_approved(its own field, so a request cannot forge it); it satisfies line 2 and never line 3.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_actionapplies an approved change only if the decider is an owner or admin of that company.api.tsregenerated with the pinned openapi-typescript (163 added lines, no other change).#17463.
needs_approval()and theApprovalHandlerwrapper now have their production caller (terminal gate).minimalguard profile is documented DEV-ONLY, and a guard fails ifAgentLoopgains a production caller.Found on the way, fixed here.
POST /api/llc/approvals/{id}/decideraised "a transaction is already begun" for every real decision, becauseload_authorizedbegins the session's transaction and the handler then calledsession.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-gatesonly decides unscoped approvals, so it cannot stand in.Ratchets. Re-pinned
chunker-private-hook-scanandstore-fact-chokepointfloors mid-window (this PR adds 17 tracked files); lowered the size ceilings forgraph.py,tool_handler.py,agent_terminal/service.py(all shrank).Acceptance criteria
auto_approve_*honoured on the live path, test withauto_approve_safe=Falseservices/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_commandApprovalHandler.needs_approval()has a production callerai_line_gate.py(role_requires_approval=), pinned byai_line_single_policy_18430_test.py::test_the_role_policy_is_consulted_by_the_terminal_and_only_thereminimalcannot disable approval on a reachable path, or dev-only plus a guardagent_loop/guard_profile.py:32(DEV-ONLY),..._test.py::test_the_agent_loop_has_no_production_caller_...DECISION_FUNCTIONSinrepo_tests/ai_line_single_policy_18430_test.py:56, and the module docstring ofservices/ai_line_enforcement.pyai_line_enforcement.py:225(enforce_line);ai_line_enforcement_test.py::test_line_two_...,::test_line_three_...; seam:test_ai_line_seam_18430.pyautobot_shared/ai_lines.py:45;::test_an_unset_mutating_action_behaves_as_line_two...llc/api/ai_lines.py:102;test_ai_lines_api_18430.py::test_a_proposal_files_an_approval_and_changes_nothingVerification
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.api/voice_integration_test.py(live TTS worker returns 401, unrelated to enforcement) and flakyhttp_client_test/openai_provider_test/test_org_role_authority, all passing on re-run or independent of this change.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 failuretest_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_17844alone (10 passed).alembic heads:20261012_098 (head). Branch is 0 behind main. Pre-push ran without an override.declare(... min_fraction=0.45),REACH.examinedandREACH.completedboth used, so it cannot pass on an empty scan.Review round 9a (fixes in this PR)
request_approvalstores 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 nothingverified_company_idis overlaid from the authenticatedorg_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_decisionis_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 toTOOL_EFFECTS(READ only) and the*_indexvariants aliased.ai_lines_test.py,test_ai_line_seam_18430.py::test_an_unknown_tool_is_held_when_unset_and_a_known_read_runsRealtimeMCPBridge.call_tool, which now callsenforce_linewith the authenticated org. A2A:a2a/task_executor.pyonly callsorchestrator.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_linerun_post_approval_actions(..., only=); the LLC decide route passes onlyai_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_actionexecute_line_changerefuses 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 carriesself_approval: true. A line-2 action may be approved by its requester, bound to the shown calls.test_ai_lines_api_18430.pyai_line_enforcement_test.py::test_the_fingerprint_is_an_hmac_of_the_raw_call_and_is_not_the_plain_hashTo stay under the size ratchets the two chat SSE generators moved from
api/chat.pytoapi/chat_streaming.pyunchanged apart from the newauth_org_idargument.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.
llc_ai_linesgainedscope_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.resolve_scope(company, user)feeds the sameresolve_lineandenforce_lineon 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.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-flaggedself_approval. The signed-in user's id comes from the session, never the URL or the body.verified_company_idandverified_user_idfrom the authenticated principal; the request's owncompany_id/user_idare stripped. The terminal resolves its verified owner's user id._handle_llc_toolscoped by the verified company only (andverified_user_idas the actor), and a session with none is refused beforedispatch_llc_tool. Test:test_llc_tool_scope_18430.py.Fingerprint key.
shown_call_fingerprintis HMAC-SHA256 keyed fromjwt_secretorsecret_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).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::test_a_user_with_no_company_runs_a_known_read::test_a_forged_company_is_still_refused_even_for_a_signed_in_usertest_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_changetest_llc_tool_scope_18430.pyReview round 9c (fixes in this PR)
POST /llc/approvalstook any payload and any requester, so admin A could create anai_line.set_lineapproval naming B as requester and decide it as a non-selfai_line.set_lineis registeredserver_only; BOTH create routes (LLCPOST /llc/approvals, generic/approval-gates) refuse it with 403. Onlypropose_line_changeand/me/changescreate 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; guardtest_the_line_change_cannot_be_forged_...decided_bywas parsed as a UUID before a username lookup, so a username spelling another user's undashed UUID could decide that user's personal linesset_verified_decider:ctx.user_idon the LLC route, the principal'sidon 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_linesNot covered here: other registered post-approval actions are still creatable through the generic route (they predate this PR); only
ai_line.set_lineis 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/createforwarded 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 boundedFFmpegService(get_ffmpeg_service().extract_audio, extension allowlist, pinned demuxer, duration capAUTOBOT_VOICE_REFERENCE_MAX_SECONDS, 24 kHz mono). No second converter.read_capped_audiostill runs first.voice.unsupported_audio; an extension outside the allowlist is 415 with the same code; a missing ffmpeg is 503voice.converter_unavailable. None is a 500.tts-worker.py.j2create_voice): a libsndfile decode failure returns 422 with the reason; everything else is unchanged. The client maps a worker 422 to the same 4xx.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}/resubmitwrotebody.contextverbatim and accepted anyone, so it routed around the create-route refusal: create something harmless, request revision, resubmit withai_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 changecontext.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_linedeclares none). An approval naming no action keeps a freely editable context._reject_unregistered_actionfirst, so an unregistered or server-only action gets the same status and detail as on create (400 / 403); an allowed-but-changed one is 403context.<key> cannot change on resubmit.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.llc/tests/test_approval_resubmit_17349.py): harmless approval resubmitted asset_linegives 403 and no transition; a real personalset_lineproposal with a changed line,action_keyorscope_idgives 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.context.actionthrough the create path, same status and detailtest_an_unregistered_action_is_refused_on_resubmit_as_on_createtest_a_harmless_approval_cannot_be_resubmitted_as_a_set_line_proposalasserts it staysrevision_requestedtest_only_the_requester_can_resubmit,..._no_recorded_requester_...test_a_registered_destructive_action_cannot_have_its_target_swappedKnown gaps
source/risk_tiergo into the approvalcontext(_file_hold) until that record lands.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