Conversation
…t per action batch When a security analyzer is configured, its risk assessment was computed and then discarded unless the confirmation policy happened to consult it (only ConfirmRisky does). Under NeverConfirm a configured analyzer ran, cost money, and left no trace. - Add SecurityAnalysisEvent (source=environment, not LLM-visible): analyzer kind, policy kind, risks per action id, optional details per action id. - Add SecurityAnalysis(risk, details) and SecurityAnalyzerBase.analyze_action / analyze_actions so analyzers can attach probabilities, confidence, or a rationale. analyze_pending_actions is kept as a risk-only wrapper. - Emit one event per analyzed action batch from Agent._requires_user_confirmation, before the policy decides. No event when no analyzer is configured. - Allowlist the analyzer-specific `details` payload in the OpenAPI weak-schema ratchet, scoped to this field (same precedent as HookExecutionEvent.hook_input).
|
@OpenHands please see the CI failures, investigate and address them. Commit and push here on PR branch. Note that it’s from a fork so set remote properly. |
|
I'm on it! enyst can track my progress at all-hands.dev |
openrouter/moonshotai/kimi-k2.5 now reports reasoning_effort support from LiteLLM's upstream model database, drifting the pinned expectation. Flip it to match, same as the kimi-k2-thinking sibling (OpenHands#4877). Co-authored-by: openhands <openhands@all-hands.dev>
|
Both CI failures on PR #5182 are resolved and pushed to the fork branch What was failing
What I did
ResultLatest commit CI is now clean on the PR. |
enyst
left a comment
enyst
left a comment
There was a problem hiding this comment.
❌ QA Report: FAIL
The audit event works in the exercised SDK paths, but live Haiku before/after QA reproduced a custom-analyzer confirmation bypass and unredacted secret persistence.
Does this PR achieve its stated goal?
Partially. On fd5904e5, verdicts and details reach callbacks and survive writing, closing, and reopening the conversation; one three-action batch produces one audit event, and audit events are excluded from the LLM view. However, this is not behavior-preserving for an existing batch-analyzer override, and the new persisted details bypass registered-secret masking.
| Phase | Result |
|---|---|
| Environment setup | ✅ Locked SDK runtime installed with uv sync --frozen --package openhands-sdk --no-dev; SDK imports successfully |
| CI status | ✅ Current head: 33 successful, 6 skipped; none failing/pending |
| Functional verification | ❌ 12 live Haiku scenarios on each revision, plus local dispatch checks; two reproduced issues |
Issues found
- [P1] Existing
analyze_pending_actions()overrides are no longer consulted. A custom batch analyzer returning HIGH previously stopped for confirmation; this head instead uses its per-action LOW result and executes the tool. See inline comment atagent.py:1075. - [P1] Analyzer detail/error strings are published and persisted without secret masking. A registered, synthetic credential in an exception survives unchanged in the audit callback and reopened event log, although the registry correctly masks it when invoked. See inline comment at
agent.py:1087–1091.
Before/after commands, observed output, and interpretation
No pytest, linters, or mocks were used. Live mode uses Conversation.run() / arun() with litellm_proxy/anthropic/claude-haiku-4-5-20251001 on the user-authorized eval proxy, a real filesystem-writing tool, actual analyzers, callbacks, and close/reopen persistence. The model was selected from the proxy's /v1/models response (latest available Haiku generation: 4.5). The same 12 cases were also independently exercised using typed responses through real sync/async SDK dispatch; the evidence below is from the live runs.
Setup:
uv sync --frozen --package openhands-sdk --no-dev
.venv/bin/python -c 'from openhands.sdk import Agent, Conversation, LLM; print("SDK import OK")'Observed: SDK import OK.
Live invocations (with LITELLM_API_KEY available in the environment):
git checkout --detach fd5904e5403d695b1cfaa847296dbe60182d7e46
OPENHANDS_SUPPRESS_BANNER=1 LOG_LEVEL=ERROR env -u LMNR_PROJECT_API_KEY -u OTEL_ENDPOINT -u OTEL_EXPORTER_OTLP_ENDPOINT -u OTEL_EXPORTER_OTLP_TRACES_ENDPOINT QA_LLM_API_KEY="$LITELLM_API_KEY" .venv/bin/python /tmp/pr5182_qa.py --live shadow
OPENHANDS_SUPPRESS_BANNER=1 LOG_LEVEL=ERROR env -u LMNR_PROJECT_API_KEY -u OTEL_ENDPOINT -u OTEL_EXPORTER_OTLP_ENDPOINT -u OTEL_EXPORTER_OTLP_TRACES_ENDPOINT QA_LLM_API_KEY="$LITELLM_API_KEY" .venv/bin/python /tmp/pr5182_qa.py --live legacy none async batch error detailed always secret-error resume reject shadow-high
git checkout --detach bd5fff06c2fe79d6e0e5d192bbf22339482f2805
OPENHANDS_SUPPRESS_BANNER=1 LOG_LEVEL=ERROR env -u LMNR_PROJECT_API_KEY -u OTEL_ENDPOINT -u OTEL_EXPORTER_OTLP_ENDPOINT -u OTEL_EXPORTER_OTLP_TRACES_ENDPOINT QA_LLM_API_KEY="$LITELLM_API_KEY" .venv/bin/python /tmp/pr5182_qa.py --live shadow legacy none async batch error detailed always secret-error resume reject shadow-high
git checkout --detach fd5904e5403d695b1cfaa847296dbe60182d7e46Selected actual result fields (BASE/HEAD labels added; random action IDs omitted):
BASE {"scenario": "shadow", "status": "finished", "writes": 1, "audit_events": 0, "risks": []}
HEAD {"scenario": "shadow", "status": "finished", "writes": 1, "audit_events": 1, "risks": [["LOW"]]}
BASE {"scenario": "legacy", "status": "waiting_for_confirmation", "writes": 0, "audit_events": 0, "risks": [], "direct_legacy_risks": ["HIGH"]}
HEAD {"scenario": "legacy", "status": "finished", "writes": 1, "audit_events": 1, "risks": [["LOW"]], "direct_legacy_risks": ["HIGH"]}
BASE {"scenario": "secret-error", "status": "waiting_for_confirmation", "writes": 0, "audit_events": 0, "risks": [], "registry_masks_fake_secret": true, "raw_fake_secret_in_event_files": false}
HEAD {"scenario": "secret-error", "status": "waiting_for_confirmation", "writes": 0, "audit_events": 1, "risks": [["HIGH"]], "registry_masks_fake_secret": true, "raw_fake_secret_in_event_files": true}
- Feature delta:
shadowexecutes the same single write on both revisions; only the head emits/persists the LOW verdict.shadow-highalso executes under NeverConfirm while recording HIGH and the analyzer error, as intended. - Confirmation regression:
legacyoverrides the previously called public batch method to return HIGH, whilesecurity_riskreturns LOW. The direct batch call still returns HIGH on both revisions, but only the base waits. On the head the marker file is actually written and the event incorrectly reports LOW for this integration. - New disclosure:
secret-errorregistersqa-fake-secret-5182-not-real(not a real credential) and raises an exception containing it. Base has no copy in its event files; head has an unredacted copy inSecurityAnalysisEvent.details, the callback payload, and the reopened log.registry_masks_fake_secret=trueconfirms this is not an unregistered-secret case. This finding concerns the newly persisted/client-visible artifact, not the pre-existing server error log.
Full observed scenario matrix:
| Scenario | Base | Head |
|---|---|---|
shadow |
finished; writes=1; audits=0 | finished; writes=1; audits=1 |
legacy |
waiting_for_confirmation; writes=0; audits=0 | finished; writes=1; audits=1 |
none |
finished; writes=1; audits=0 | finished; writes=1; audits=0 |
async |
finished; writes=1; audits=0 | finished; writes=1; audits=1 |
batch |
finished; writes=3; audits=0 | finished; writes=3; audits=1 |
error |
waiting_for_confirmation; writes=0; audits=0 | waiting_for_confirmation; writes=0; audits=1 |
detailed |
waiting_for_confirmation; writes=0; audits=0 | waiting_for_confirmation; writes=0; audits=1 |
always |
waiting_for_confirmation; writes=0; audits=0 | waiting_for_confirmation; writes=0; audits=1 |
secret-error |
waiting_for_confirmation; writes=0; audits=0 | waiting_for_confirmation; writes=0; audits=1 |
resume |
waiting_for_confirmation; writes=0; audits=0 | waiting_for_confirmation; writes=0; audits=1 |
reject |
waiting_for_confirmation; writes=0; audits=0 | waiting_for_confirmation; writes=0; audits=1 |
shadow-high |
finished; writes=1; audits=0 | finished; writes=1; audits=1 |
Additional actual outputs:
- Every head scenario that emitted an audit:
audit_ids_match=true,callback_audits=1,reopened_audits=1,audit_roundtrip_equal=true,audit_in_llm_view=false. - Executed audited batches:
audit_precedes_execution=true; the trailing lone FinishAction does not add another audit. batch:actions=3,writes=3, exactly one audit with three LOW risks.detailed: MEDIUM plus{"confidence": 0.42, "rationale": "AUDIT_ONLY_5182"}persists while execution waits.resume: after closing/reopening the waiting conversation, publicAgent.step()executes the pending tool:writes_after_approval=1; headaudit_count_after_approval=1(no duplicate).reject: publicreject_pending_actions()leaveswrites_after_rejection=0on both revisions.- All 24 live scenario results have
error=null.
These results confirm the ordinary event behavior and nearby confirmation paths, but do not justify approval while the two security-sensitive regressions remain.
Reproduction harness — save as /tmp/pr5182_qa.py
import argparse
import asyncio
import json
import os
import tempfile
from pathlib import Path
from litellm import ModelResponse
from pydantic import SecretStr
from openhands.sdk import LLM, Agent, Conversation
from openhands.sdk.context.view import View
from openhands.sdk.conversation.state import ConversationExecutionStatus
from openhands.sdk.llm import LLMResponse, Message, MessageToolCall
from openhands.sdk.llm.utils.metrics import MetricsSnapshot
from openhands.sdk.event import ActionEvent, ObservationEvent
from openhands.sdk.security import SecurityAnalyzerBase, SecurityRisk
from openhands.sdk.security import analyzer as analyzer_module
from openhands.sdk.security.confirmation_policy import AlwaysConfirm, ConfirmRisky, NeverConfirm
from openhands.sdk.security.llm_analyzer import LLMSecurityAnalyzer
from openhands.sdk.tool import Action, Observation, Tool, ToolDefinition, ToolExecutor, register_tool
class WriteMarkerAction(Action):
text: str
class WriteMarkerObservation(Observation):
pass
class WriteMarkerExecutor(ToolExecutor):
def __init__(self, path):
self.path = path
def __call__(self, action, conversation=None):
with self.path.open('a') as f:
f.write(action.text + '\n')
return WriteMarkerObservation.from_text('Marker written successfully. Finish now.')
class WriteMarkerTool(ToolDefinition):
@classmethod
def create(cls, conv_state):
return [cls(description='Write a harmless marker in the temporary QA workspace.',
action_type=WriteMarkerAction, observation_type=WriteMarkerObservation,
executor=WriteMarkerExecutor(Path(conv_state.workspace.working_dir) / 'marker.txt'))]
class LegacyBatchAnalyzer(SecurityAnalyzerBase):
def security_risk(self, action):
return SecurityRisk.LOW
def analyze_pending_actions(self, pending_actions):
return [(action, SecurityRisk.HIGH) for action in pending_actions]
class ExplodingAnalyzer(SecurityAnalyzerBase):
def security_risk(self, action):
raise RuntimeError('analyzer unavailable')
class SecretErrorAnalyzer(SecurityAnalyzerBase):
def security_risk(self, action):
raise RuntimeError('upstream rejected credential qa-fake-secret-5182-not-real')
class DetailedAnalyzer(SecurityAnalyzerBase):
def security_risk(self, action):
return SecurityRisk.MEDIUM
def analyze_action(self, action):
return analyzer_module.SecurityAnalysis(risk=SecurityRisk.MEDIUM,
details={'confidence': 0.42, 'rationale': 'AUDIT_ONLY_5182'})
register_tool(WriteMarkerTool.name, WriteMarkerTool)
parser = argparse.ArgumentParser()
parser.add_argument('scenarios', nargs='+')
parser.add_argument('--live', action='store_true')
args = parser.parse_args()
for name in args.scenarios:
policy = NeverConfirm()
analyzer = LLMSecurityAnalyzer()
if name == 'none':
analyzer = None
elif name == 'legacy':
analyzer, policy = LegacyBatchAnalyzer(), ConfirmRisky()
elif name == 'error':
analyzer, policy = ExplodingAnalyzer(), ConfirmRisky()
elif name == 'secret-error':
analyzer, policy = SecretErrorAnalyzer(), ConfirmRisky()
elif name == 'detailed':
analyzer, policy = DetailedAnalyzer(), ConfirmRisky(threshold=SecurityRisk.MEDIUM)
elif name in ('always', 'resume', 'reject'):
policy = AlwaysConfirm()
elif name == 'shadow-high':
analyzer = ExplodingAnalyzer()
llm = LLM(model=os.environ.get('QA_LLM_MODEL', 'litellm_proxy/anthropic/claude-haiku-4-5-20251001'),
base_url=os.environ.get('QA_LLM_BASE_URL', 'https://llm-proxy.eval.all-hands.dev'),
api_key=SecretStr(os.environ['QA_LLM_API_KEY'] if args.live else 'unused-local-qa'),
temperature=0, max_output_tokens=4096, extended_thinking_budget=None, reasoning_effort="none", num_retries=1)
agent = Agent(llm=llm, tools=[Tool(name=WriteMarkerTool.name)])
with tempfile.TemporaryDirectory(prefix='qa5182-') as tmp:
root = Path(tmp)
received = []
kwargs = dict(agent=agent, workspace=tmp, persistence_dir=str(root / 'saved'),
visualizer=None, max_iteration_per_run=4, callbacks=[received.append])
conv = Conversation(**kwargs)
conv.set_confirmation_policy(policy)
if analyzer is not None:
conv.set_security_analyzer(analyzer)
if name == 'secret-error':
conv.update_secrets({'QA_SYNTHETIC': 'qa-fake-secret-5182-not-real'})
prompt = 'Call write_marker exactly once with text="qa-marker-5182" and security_risk="LOW". Then finish. Do not call any other tool except finish.'
if name == 'batch':
prompt = 'In your FIRST response make THREE parallel write_marker tool calls, with texts "one", "two", "three", security_risk="LOW" for each. Then finish. Do not issue them sequentially.'
conv.send_message(prompt)
try:
if args.live:
if name == 'async':
asyncio.run(conv.arun())
else:
conv.run()
else:
n = 3 if name == 'batch' else 1
msg = Message(role='assistant', content=[], tool_calls=[
MessageToolCall(id=f'qa-call-{i}', name=WriteMarkerTool.name,
arguments=json.dumps({'text': f'qa-marker-{i}', 'security_risk': 'LOW'}),
origin='completion') for i in range(n)])
response = LLMResponse(message=msg, metrics=MetricsSnapshot(),
raw_response=ModelResponse(id='qa-response'))
with conv.state:
conv.state.execution_status = ConversationExecutionStatus.RUNNING
if name == 'async':
asyncio.run(agent._ahandle_tool_calls(msg, response, conv, conv.state, conv._on_event))
else:
agent._handle_tool_calls(msg, response, conv, conv.state, conv._on_event)
if conv.state.execution_status != ConversationExecutionStatus.WAITING_FOR_CONFIRMATION:
finish = Message(role='assistant', content=[], tool_calls=[
MessageToolCall(id='qa-finish', name='finish', origin='completion',
arguments=json.dumps({'message': 'done'}))])
response = LLMResponse(message=finish, metrics=MetricsSnapshot(),
raw_response=ModelResponse(id='qa-finish-response'))
agent._handle_tool_calls(finish, response, conv, conv.state, conv._on_event)
error = None
except Exception as exc:
error = f'{type(exc).__name__}: {exc}'
events = list(conv.state.events)
analysis_events = [e for e in events if type(e).__name__ == 'SecurityAnalysisEvent']
actions = [e for e in events if isinstance(e, ActionEvent) and e.tool_name == WriteMarkerTool.name]
view = View.from_events(events)
record = dict(scenario=name, status=conv.state.execution_status.value,
writes=len((root / 'marker.txt').read_text().splitlines()) if (root / 'marker.txt').exists() else 0,
actions=len(actions), audit_events=len(analysis_events),
risks=[list(e.risks.values()) for e in analysis_events],
details=[e.details for e in analysis_events],
audit_ids_match=set().union(*(e.risks.keys() for e in analysis_events)) == {a.id for a in actions},
audit_in_llm_view=any(type(e).__name__ == 'SecurityAnalysisEvent' for e in view.events),
callback_audits=sum(type(e).__name__ == 'SecurityAnalysisEvent' for e in received), error=error)
if analysis_events and any(isinstance(e, ObservationEvent) for e in events):
record['audit_precedes_execution'] = events.index(analysis_events[0]) < next(i for i,e in enumerate(events) if isinstance(e,ObservationEvent))
if name == 'legacy':
record['direct_legacy_risks'] = [risk.value for _,risk in analyzer.analyze_pending_actions(actions)]
conv_id = conv.id
conv.close()
reopened = Conversation(**{**kwargs, 'conversation_id': conv_id, 'callbacks': []})
restored = [e for e in reopened.state.events if type(e).__name__ == 'SecurityAnalysisEvent']
record['reopened_audits'] = len(restored)
record['audit_roundtrip_equal'] = [e.model_dump() for e in analysis_events] == [e.model_dump() for e in restored]
if name == 'secret-error':
record['registry_masks_fake_secret'] = conv.state.secret_registry.mask_secrets_in_output('qa-fake-secret-5182-not-real') == '<secret-hidden>'
record['raw_fake_secret_in_event_files'] = any('qa-fake-secret-5182-not-real' in p.read_text() for p in (root / 'saved').rglob('events/*.json'))
if name == 'resume':
with reopened.state:
reopened.state.execution_status = ConversationExecutionStatus.RUNNING
reopened.agent.step(reopened, on_event=reopened._on_event)
record['writes_after_approval'] = len((root / 'marker.txt').read_text().splitlines())
record['audit_count_after_approval'] = sum(type(e).__name__ == 'SecurityAnalysisEvent' for e in reopened.state.events)
elif name == 'reject':
reopened.reject_pending_actions('Rejected during QA')
record['writes_after_rejection'] = int((root / 'marker.txt').exists())
record['observations_after_rejection'] = sum(isinstance(e, ObservationEvent) for e in reopened.state.events)
reopened.close()
print('QA_RESULT ' + json.dumps(record, default=str), flush=True)Scope and setup notes
The initial DeepSeek attempt returned Insufficient Balance; the user then authorized the eval proxy and live Haiku runs succeeded. The initial Haiku harness cap of 600 output tokens conflicted with the SDK's default extended-thinking budget; the successful runs use max_output_tokens=4096, extended_thinking_budget=None, and reasoning_effort="none". These were harness/provider setup issues, not findings against this PR.
Server REST/WebSocket delivery and benchmark performance were not exercised; live Python SDK conversations, callbacks, file writes, persistence, and resume/rejection paths were. Per repository policy, a human maintainer should assess lightweight eval evidence for this execution-path change before approving.
Taste rating: 🟡 Acceptable approach, but the compatibility and redaction boundaries need fixing. Add regression coverage for the two inline reproducers, including close/reopen persistence and callback output for masking.
[RISK ASSESSMENT]
- Overall PR: 🔴 HIGH. It changes the source of confirmation-policy decisions and introduces a durable, client-visible channel for analyzer diagnostics. No dependency changes. Do not auto-merge; request human review of analyzer backward compatibility, redaction, and execution-path/eval impact.
Verdict: Needs rework. Submitting COMMENT rather than REQUEST_CHANGES, as required by this repository. The audit path must preserve the decision source and mask diagnostics before publication.
This review was generated by an AI agent (OpenHands) on behalf of the requesting user.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.- Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
Resolve with AI? Install the iterate skill in your agent and run
/iterateto automatically drive this PR through CI, review, and QA until it's merge-ready.Was this review helpful? React with 👍 or 👎 to give feedback.
| risk | ||
| for _, risk in state.security_analyzer.analyze_pending_actions( | ||
| action_events | ||
| analyses = state.security_analyzer.analyze_actions(action_events) |
There was a problem hiding this comment.
🔴 Critical [P1]: preserve existing batch-analyzer overrides when changing the dispatch API.
This replaces the call to analyze_pending_actions() with analyze_actions(). The compatibility wrapper only preserves callers of the old method, not custom analyzers that override it: the agent now silently ignores their batch verdicts.
I reproduced this in live Haiku-driven conversations on base and head with an otherwise unchanged analyzer:
class LegacyBatchAnalyzer(SecurityAnalyzerBase):
def security_risk(self, action):
return SecurityRisk.LOW
def analyze_pending_actions(self, pending_actions):
return [(action, SecurityRisk.HIGH) for action in pending_actions]Under ConfirmRisky(), base stops at waiting_for_confirmation with 0 file writes. This head finishes with 1 actual file write, and the new audit says LOW, even though calling that same analyzer's batch method still returns HIGH. This can bypass confirmation for existing custom integrations that implement batch/context-dependent assessment.
Please preserve the old override contract when bridging to detailed analyses (without analyzing twice), or provide an explicit migration rather than silently selecting a different verdict source. Add a regression exercising a legacy batch override through agent dispatch and asserting both the confirmation outcome and the emitted verdict.
AI-generated feedback from OpenHands on behalf of the requesting user.
| details={ | ||
| action.id: analysis.details | ||
| for action, analysis in analyses | ||
| if analysis.details is not None | ||
| }, |
There was a problem hiding this comment.
🔴 Critical [P1]: mask registered secrets before emitting analyzer details.
analysis.details is passed directly into an event that the callback chain persists and publishes; the exception fallback now also puts str(e) in these details. There is no general secret-masking step in that callback chain.
Reproduced using only a synthetic value: register QA_SYNTHETIC="qa-fake-secret-5182-not-real", then have an analyzer raise RuntimeError("upstream rejected credential qa-fake-secret-5182-not-real"). On this head, the callback and the event read back after closing/reopening both contain the literal value in details[action_id]["error"]. The registry's own mask_secrets_in_output() correctly returns <secret-hidden> for it; base did not put this error text in the event log. Backend request errors and analyzer rationales can contain credential-bearing URLs/headers, so this creates a new durable/client-visible disclosure path.
Please apply the conversation secret registry's recursive model masking to the completed SecurityAnalysisEvent before on_event, covering both successful details and exception details. Add coverage checking callback payloads and persisted/reopened events, while preserving the HIGH fallback.
AI-generated feedback from OpenHands on behalf of the requesting user.
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Taste rating: good 👍
No material findings. This is a clean, additive change:
- Gated & inert by default: emission only happens when
state.security_analyzer is not None; with no analyzer the dispatch path is byte-identical to before (just an extra defaulted parameter). Zero cost for existing users, as claimed. - Union registration works:
DiscriminatedUnionMixinauto-registersSecurityAnalysisEvent— confirmed by the OpenAPI export needing (and getting) the scoped allowlist entry fordetails. The event is notLLMConvertibleEvent, so it never enters the model's context, and the JSON round-trip through theEventunion is tested. - Compat preserved:
analyze_pending_actions()is now a risk-only wrapper overanalyze_actions()with identical semantics, including HIGH-on-exception (verified against the existing tests intests/sdk/security/test_security_analyzer.py, which still exercise the public shape). - No double emission: the confirmation-resume path in
LocalConversationexecutes stored pending actions directly without re-dispatching the LLM response, so a batch is analyzed and recorded exactly once. Both sync and async_handle_tool_callspaths threadon_event. - Test coverage is right: no-analyzer, NeverConfirm (recorded but not enforced), ConfirmRisky (details + WAITING_FOR_CONFIRMATION), multi-action batch keyed by id, analyzer exception → HIGH + error detail, union round-trip, and the compat wrapper. Tests assert observable behavior through the real step path, not mock wiring.
Non-blocking observations (no action needed):
details: dict[str, Any]relies on future analyzers keeping values JSON-serializable for persistence; nothing in this PR populates non-serializable data (the error path usesstr(e)), so this is fine as designed._requires_user_confirmationis private, so the new optional parameter is not a public-API break; no in-repo overrides exist.
[RISK ASSESSMENT] MEDIUM — the diff itself is low-risk and well-tested, but it modifies the agent's response-dispatch loop (agent.py, response_dispatch.py). Per this repository's review policy, changes to planning/loop logic are in the eval-risk category and should not be auto-approved without eval evidence (no eval-monitor link is provided here). That said, the change is provably inert for benchmarks: it only fires with a security analyzer configured (benchmarks don't configure one) and the event is never shown to the LLM, so LLM context and behavior are unchanged on the default path. Flagging for a human maintainer to confirm lightweight evals aren't needed (or to add the integration-test label per repo convention for agent-decision-logic changes) and merge.
Verdict: COMMENT — no blocking issues; human maintainer sign-off requested per the eval-risk gate.
Key architectural insight: recording the analyzer's verdict as a first-class event (rather than a field on ActionEvent) keeps the acting LLM's self-label (ActionEvent.security_risk) and the independent analyzer's verdict cleanly distinguishable in the same auditable log — the right ownership boundary for shadow-mode and audit use cases.
Improve this review? If feedback seems incorrect or irrelevant, update the repository's
.agents/skills/custom-codereview-guide.md(with the/codereviewtrigger), then re-request review. The reviewer reads the guide from the PR head.Resolve with AI? Install the iterate skill and run
/iterate.Was this review helpful? React with 👍 or 👎.
|
This comment was posted by an AI agent (OpenHands). |
3 participants
HUMAN:
This PR proposes a small event to make part of the EventLog for security analysis:
SecurityAnalysisEvent, to save the analysis details that scored an action.We already do something like it in
ConversationUpdateStatus. This is intended for audit of the analyzer, UI, logging, and I’d like to see the Jev probabilities from a Jev-based analyzer in a follow-up. It’s a small Event object that will store those probabilities.AGENT:
I'm smolpaws, an AI agent working with Engel Nyst (@enyst). This PR was written in a conversation running Claude Fable 5.1 (
fable-5-1-eval).Why
When a security analyzer is configured,
Agent._requires_user_confirmationcomputes a risk per pending action and then hands the list to the confirmation policy. OnlyConfirmRiskyreads it;AlwaysConfirmandNeverConfirmignore the argument. So with an analyzer set and confirmation disabled (NeverConfirm, the default), the analyzer runs on every action batch, costs a call, and its verdict is dropped on the floor: not on the event, not persisted, not visible in any UI.This matters for two upcoming uses:
Summary
SecurityAnalysisEvent(event/security.py):source="environment", notLLMConvertibleso the model never sees it. Fields:analyzer(kind),policy(kind),risks: dict[EventID, SecurityRisk],details: dict[EventID, dict[str, Any]].SecurityAnalysis(risk, details)plusSecurityAnalyzerBase.analyze_action()(overridable, defaults to wrappingsecurity_risk()) andanalyze_actions(). This lets an analyzer attach probabilities, confidence, or a rationale. Existing analyzers need no change;analyze_pending_actions()is kept as a risk-only wrapper over the new path, so its behaviour (including HIGH on exception) is unchanged. On exception the detail carries{"error": ...}._requires_user_confirmationbefore the policy decides.on_eventis threaded through the twoResponseDispatchMixincall sites. No analyzer configured → no event, zero cost for existing users.SecurityAnalysisEvent.details(analyzer-specific by design), same precedent asHookExecutionEvent.hook_input.check_agent_server_openapi_quality.pypasses.Grain: one event per LLM response that produced actions, keyed by action id, so the UI can badge each action and audit can join later. A lone
FinishAction/ThinkActionnever reaches the analyzer today and still doesn't.Issue Number
Refs #4259 — records the security analyzer's verdict as an auditable, reviewer-facing artifact for agent actions.
How to Test
uv run pytest tests/sdk/agent/test_security_analysis_event.pyuv run pytest tests/sdk/security tests/sdk/agent tests/sdk/event tests/sdk/conversation.github/scripts/export_agent_server_openapi.pythen.github/scripts/check_agent_server_openapi_quality.pyOptional end-to-end: start a conversation with an
LLMSecurityAnalyzerandNeverConfirm(e.g.examples/01_standalone_sdk/16_llm_security_analyzer.py), run an action, and confirm aSecurityAnalysisEventlands in the persisted event log with the analyzer's verdict keyed by action id.API notes
Additive only. New event kind in the union; new public
SecurityAnalysis; two new methods onSecurityAnalyzerBase;_requires_user_confirmation(private) gains an optionalon_event.Condensation/ActionEventshapes untouched. The existingActionEvent.security_riskis still the acting LLM's self-label set at parse time; this event records what the analyzer said, which is a different thing and now distinguishable.Ran
Details
- `uv run pytest tests/sdk/agent/test_security_analysis_event.py` → 7 passed (no analyzer → no event; verdict recorded under `NeverConfirm` and action still runs; details + `WAITING_FOR_CONFIRMATION` under `ConfirmRisky`; one event per 3-action batch keyed by id; analyzer exception → HIGH + error detail; JSON round-trip through the `Event` union; `analyze_pending_actions` unchanged). - `uv run pytest tests/sdk/security tests/sdk/agent tests/sdk/event tests/sdk/conversation` → 894 passed; the 2 failures (`test_acp_agent::…seeded_file`, `test_local_conversation_plugins::…secret_refs`) fail identically on clean `upstream/main` in this environment (local secret-file tests), unrelated. - `pre-commit run --files ` → ruff format/lint, pycodestyle, pyright, forbidden-dynamic-attributes, import rules, tool registration: all passed. - `.github/scripts/export_agent_server_openapi.py` + `check_agent_server_openapi_quality.py` → passed (100 allowlisted weak locations; the new one is the scoped `details` entry).deepseek/deepseek-chat) +TerminalTool,LLMSecurityAnalyzer+NeverConfirm, prompt "runecho hello-from-e2e":FinishActionis correctly not analyzed), the command executed underNeverConfirm, and the event is in the persisted log.Not done
details; that's for the analyzers that have something to say (an LLM guardrail's rationale, a calibrated model's probabilities).Jev-Fast-Audit
⚡ Jev fast audit · estimates · 0.61s · commit fd5904e
Strongest signal: No primary concern selected.
Evidence: No primary concern to locate.
Coverage: complete supplied coverage; 16/16 hunks, 8/8 files.
All estimates and evidence