feat(qwen,goose): record delegated fs I/O and gate it with result-phase policy

[EnesYilmazcode]

Related issue

N/A. This completes the deferred follow-up from #1100, tracked in docs/QWEN_FOLLOWUPS.md ("File I/O recording / content policy"). No separate issue.

Summary

#1100 routed the ACP harnesses' file reads/writes back through Omnigent's OSEnvironment but left two layers as documented follow-ups: the delegated I/O was invisible in history and no content policy ran on it. This wires both onto the existing _handle_fs_read / _handle_fs_write handlers, in qwen and goose:

• History recording. Each delegated read/write emits a paired ToolCallRequest + ToolCallComplete onto the turn stream, which the harness adapter already renders as observed function_call / function_call_output items. The handlers buffer the records and run_turn drains them right after dispatching the request; the buffer is reset per turn so a prior turn's ops don't leak in.
• Result-phase content policy. The read/written bytes run through PHASE_TOOL_RESULT policy. An explicit POLICY_ACTION_DENY refuses the op (read: the bytes never reach the agent; write: evaluated before OSEnvironment.write, so the write never happens). It fails open otherwise (policy unwired, an eval error, or any non-deny verdict), matching PHASE_TOOL_RESULT's advisory semantics (FAIL_CLOSED_PHASES is PHASE_TOOL_CALL only). Result policy here gates on content only: the server reads result-phase tool identity from request_data, which the harness policy round-trip doesn't carry, so the payload is {"result": content} (matching the existing producer in runner/tool_dispatch.py). A real read/write failure records ERROR, a denial records BLOCKED, success records SUCCESS; malformed requests still raise before any record.

Applied to both ACP harnesses since they share the identical dispatch pattern (same as #1100 and the history-replay change). docs/QWEN_FOLLOWUPS.md is updated to check the item off and note the content-only scope.

Test Plan

• 10 new tests (5 per harness): a delegated read and write each emit a paired ToolCallRequest + ToolCallComplete with a shared call_id; a PHASE_TOOL_RESULT deny refuses a read and records it BLOCKED; a deny prevents a write (the OSEnvironment.write is never called) and records BLOCKED; run_turn surfaces a delegated op's records onto the turn stream.
• Each new test fails on the unfixed code (verified by stashing the production change).
• pytest tests/inner/test_qwen_executor.py tests/inner/test_goose_executor.py tests/inner/test_qwen_agent_integration.py — 155 passed.
• ruff check + ruff format --check clean; no lock-file changes.
• The e2e_ui / per-harness e2e suites are gateway-bound and were not run locally.

Type of change

• Bug fix
• Feature
• Refactor / chore
• Docs
• Test / CI
• Breaking change

Test coverage

• Unit tests added / updated
• Integration tests added / updated
• E2E tests added / updated
• Manual verification completed
• Existing tests cover this change
• Not applicable

Coverage notes

The new unit tests drive the real dispatch (_respond_to_agent_request) and the real run_turn loop with a fake OSEnvironment and a fake policy evaluator, so they cover both the recording and the deny/fail-open paths without a live qwen/goose binary. Two design calls worth a look: (1) for writes I run result-phase policy before OSEnvironment.write so an explicit deny actually prevents the write. That bends "result phase = after the side effect," but the content is known up front and blocking a not-yet-incurred write is the useful behavior. (2) Result-phase policy is content-only here, because the harness policy round-trip (evaluate_policy(phase, data)) carries no request_data, so the server can't see the fs tool's name for the result phase. Happy to adjust either if you would rather keep the phase semantics strict or thread request_data through the round-trip.

[EnesYilmazcode]

@serena-ruan this finishes the file-I/O recording / content-policy follow-up you left in QWEN_FOLLOWUPS.md after #1100, in both qwen and goose. Would you be up for reviewing? Two design calls are flagged in the description: writes are gated before the write happens, and result-phase policy is content-only here since the harness round-trip carries no request_data. Happy to adjust either.

[EnesYilmazcode]

Heads up on the red Pytest (inner-rest) check: it failed in the apt setup step, not on the tests. The log shows Failed to fetch https://packages.microsoft.com/repos/azure-cli ... 403 Forbidden ... no longer signed (exit 100), so pytest never ran in that shard. The other pytest shards in this run are green, and the qwen/goose inner tests pass locally (155 passed). A re-run should clear it once the mirror recovers.

[EnesYilmazcode]

Status now that the run finished: the remaining red checks are not from this change.

• Pytest (misc): the only failure is test_runner_background_turn_emits_failed_when_spawn_env_build_raises, a claude-sdk runner test this PR doesn't touch. It tripped a 10s SSE-status timeout under the xdist workers (assert ['running'] == ['running', 'failed']) and passes locally on this branch (1 passed in ~1s). Reads as a pre-existing async flake.
• Coverage (70% < 80%): downstream of the inner-rest apt failure. That shard never ran, so its coverage (including this PR's new inner tests) is missing from the aggregate.

Rebasing on the latest main to re-trigger CI.

[github-actions]

⏰ Reviewer SLA — this PR has been awaiting review from @dbczumar for more than 5 working days. Adding @PattaraS as a second reviewer.

[PattaraS]

Thanks for tackling a real observability and policy gap, and for the thorough Qwen/Goose test coverage. I see two blockers in the current design:

1. The write path calls PHASE_TOOL_RESULT before the filesystem write so that a denial can prevent the side effect. That changes the meaning of the result phase, omits the tool name/path from policy context, and inherits result-phase fail-open behavior. Please evaluate delegated writes through PHASE_TOOL_CALL before the write, carrying the tool identity, path, and content, then emit PHASE_TOOL_RESULT after the operation with the actual result. This preserves the existing fail-closed contract for side effects.

2. At turn start, stale ACP filesystem requests are handled before self._fs_events.clear(). Their ToolCall events are buffered and then immediately discarded, so those reads/writes still occur without a persisted audit record. Please either drain those events into the appropriate history or preserve them until they can be emitted.

The duplicated Qwen/Goose implementation is a secondary maintainability concern; a shared helper would reduce semantic drift. The underlying feature is worthwhile, but I do not think we should establish the current phase behavior as the policy contract.

[EnesYilmazcode]

Thanks PattaraS, fair read and I agree with both.

1. Phase semantics. You're right that gating a write through the result phase is the wrong contract: it's content-only, drops the tool identity and path, and inherits result-phase fail-open, so a policy timeout would let the write through. I'll move the write gate to PHASE_TOOL_CALL before the write, carrying {name, arguments: {path, content}} (the same shape _decide_permission already uses) and emit PHASE_TOOL_RESULT after with the real result, so it's fail-closed with full context.

One split I'd like to confirm on reads specifically: a read's content doesn't exist until the read runs, so the pre-op call can carry name and path but not content. I'd gate the read at PHASE_TOOL_CALL on name/path, and keep the content check on the returned bytes at the result phase but framed as advisory (whether content reaches the model), not as a side-effect gate. Does that division work for you, or would you prefer reads handled differently?

2. Stale requests at turn start. Good catch, that's a real hole in the new audit path: those events get buffered and then cleared before they reach history. I'll preserve them so the read/write is recorded (drain before the clear).

3. Duplication. Agreed, I'll factor the shared qwen/goose logic into one helper to reduce drift.

I'll push the revision. Thanks for the careful review.

[EnesYilmazcode]

Pushed the revision. Both blockers are addressed:

1. Phase semantics. Delegated writes now gate at PHASE_TOOL_CALL with {name, path, content} before the write, failing closed on an eval error (and on ASK, since delegated fs has no elicitation path). Reads gate on the path at the call phase and keep the content check at the result phase for whether the bytes reach the model. New tests cover the call gate, the fail-closed path, and ASK.

2. Audit records. Stale prior-turn server fs requests answered at turn start now drain their ToolCall events into history instead of clearing them, so the I/O they performed is recorded. A regression test drives run_turn with a stale queued request and asserts the events surface (it fails on the old clear()).

On the duplication: a clean shared helper needs consolidating the ACP error types and helpers, which are now triplicated across qwen, goose, and the acp_executor added in #2152. I kept this PR focused on the two correctness fixes rather than bundle that refactor with a security change, and I am happy to do the shared-helper extraction as a follow-up that covers all three uniformly. Let me know if you would rather have it in here.

[PattaraS]

Thanks for the revision. The call-phase and stale-event parts look addressed, but I may still be missing the post-write result phase.

At the latest head, both write handlers appear to run PHASE_TOOL_CALL, then env.write(...), then record the ToolCall success and return (qwen_executor.py:864-883, goose_executor.py:726-745). I do not see a PHASE_TOOL_RESULT evaluation after the write carrying the actual env.write() result, despite your earlier note that you would add that step. The new write tests also appear to cover the call-phase gate, fail-closed behavior, and ASK, but not a post-operation result-phase event.

Could you please add that post-write PHASE_TOOL_RESULT evaluation and a regression test in both harnesses? We could be wrong about the intended plumbing or may have overlooked where this is emitted; if so, please tell us why and point us to the relevant path.

[EnesYilmazcode]

Hey good point, I added it now.

Both write handlers run PHASE_TOOL_RESULT after env.write() with the actual result, and a deny records BLOCKED and refuses the response. It can't undo the write since it runs after the op.

I also renamed _fs_content_policy_denies to _fs_result_policy_denies, since it was read-specific and now takes any result. Read behavior is unchanged.

New test in both harnesses covers the payload and the deny path. Both fail on the old head.

[github-actions]

Closing this PR because it has been labeled waiting-on-author for 7 days without an author reply or new commit.

The label was last applied on 2026-07-17T09:26:33Z. If you are ready to continue, please reopen this PR or open a new one.

[PattaraS]

LGTM.

[PattaraS]

/review

[github-actions]

🏷️ Doc impact: no-doc-update

Internal changes to the goose/qwen ACP executors adding audit-event recording and policy gating for delegated fs ops, plus tests and an internal followups note — no user-facing surface, integration, or built-in policy changed.

_{Auto-classified on merge. Set the label manually before merging to override. · run}

[omnigent-ci]

Polly AI Review

Blocking issues

1. Delegated-fs call_id leaks into the adapter's MCP correlation queue and mis-pairs a later tool call (qwen + goose).

Each recorded fs op emits a ToolCallRequest whose metadata={"call_id": "fsacp_…"}. When that event reaches ExecutorAdapter._translate_event, the ToolCallRequest branch unconditionally pushes any stamped call_id onto self._pending_mcp_call_ids (adapter lines ~907–920):

tool_use_id = _call_id_from_metadata(event.metadata)
if tool_use_id is not None:
    self._pending_mcp_call_ids.append(tool_use_id)

The fs ToolCallComplete branch does not pop that queue — only _stable_tool_executor (the Omnigent-MCP dispatch path) pops it. In qwen/goose nothing else ever yields a ToolCallRequest, so before this PR that queue was always empty for them and every MCP dispatch got a fresh id. Now, in any turn that performs a delegated fs read/write and then calls an Omnigent MCP tool (sys_session_send, load_skill, web_fetch, …), _stable_tool_executor pops the stale fsacp_… id and reuses it as the MCP dispatch's call_id. The fs op has already emitted a function_call_output under that same fsacp_… id, so the two collide — downstream pairs strictly by call_id, producing a duplicate/mispaired history item (the MCP result renders against the fs card, and the fs op or the MCP call orphans). This corrupts persisted conversation history for the common case of "read a file, then call a tool." The fs events should not feed the MCP correlation queue (e.g. don't stamp a queue-eligible call_id, or have the adapter distinguish these observed-only fs ids).

Non-blocking notes

• Write result-phase policy never sees the written bytes. The PR summary says "the read/written bytes run through PHASE_TOOL_RESULT policy," but _handle_fs_write passes write_result (the OSEnvironment.write outcome, e.g. {"bytes": 3}) to _fs_result_policy_denies, not the content. So for writes, content is only ever gated at the call phase; result-phase write gating is effectively a no-op on content. Combined with the documented "denial refuses the response without undoing the write," result-phase write policy provides little real protection — worth stating plainly rather than as "content policy on written bytes."

• Call-phase gate fails closed for reads, not just writes. _fs_call_policy_denies is shared by read and write and returns True (deny) on any eval exception, with the comment "call phase fails closed for side effects." A read is not a side effect. The practical consequence is that a transient policy-server hiccup blocks all delegated reads, regressing file-read delegation that previously worked with no policy wired. Consider failing open for reads (matching the advisory read semantics) while keeping writes fail-closed.

• Scope beyond the title. The change adds a PHASE_TOOL_CALL gate on delegated fs (with ASK→deny since there's no elicitation path), which is a larger behavior change than the "result-phase policy" framing suggests. The call-phase gate is the only layer that actually prevents a bad write; it deserves top billing in the description.

• "Buffer reset per turn" is really a drain-at-turn-start. There's no explicit reset of _fs_events; leftovers are surfaced by the start-of-turn drain. Functionally fine, but a prior turn's undrained op would be attributed to the next turn's history. Low risk given the immediate drains, but the description's wording overstates the isolation.

Summary

The feature is well-tested (10 focused tests exercising the real dispatch and run_turn paths, including fail-open/closed and stale-op cases) and the security direction is sound — it adds gating rather than weakening a boundary, and sandbox roots are unchanged. However, stamping a call_id on the fs ToolCallRequest leaks into the shared adapter correlation queue and will mis-pair a subsequent Omnigent MCP tool call in the same turn, corrupting history rendering/persistence; that should be fixed before merge. The write-side result-phase policy and the read fail-closed behavior are also worth reconsidering, as they don't deliver the protection the description implies. Recommend addressing the correlation-queue collision, then re-verifying a mixed fs-op + MCP-tool turn end-to-end.

---

_{Automated review by Polly · workflow run}