feat: iii-native harness, tool→function rename, worker layout standardization

[ytallo]

Summary

Reshape the harness around an iii-native model: the engine drives all dispatch through agent_call, workers expose standardized manifest/config surfaces, and "tool" is renamed to "function" across the codebase to match the bus terminology. Bundles supporting work that landed alongside (PR #103 shell consolidation, #104 agent_call polish, #105 tool→function rename) and reconciles with main.

What's in here

Harness — iii-native dispatch

• agent_call dispatcher routes function calls through the engine instead of bypassing it. Slimmed down in Tier 2 polish (iii-native harness: slim agent_call + Tier 2 polish #104).
• Browser eviction + error handling improvements.
• Makefile for local demo operations; demo.sh adds approval-gate to WORKERS and per-worker env (POLICY_DENIED_FUNCTIONS=bridge::trigger for policy-denylist).
• iii.worker.yaml adds iii-sandbox and approval-gate deps.

Harness/web

• Composer rewrite (slash/at/history modes), headless useCommandMenu, bus function palette (Cmd-J).
• WS-pushed event handlers replace SSE + 4 s poll; live status strip + status tab.
• /repair, /fork, /export md|json, fork-from-message.
• Per-session workspace cwd; iii-client over iii-browser-sdk.
• Reducer rewritten to entry-id-keyed Map (idempotent + replay-safe); session reads via session-tree.
• Markdown rendering and collapsible views for function call/result blocks.

tool → function rename (#105)

Renamed across crates, the harness UI, skills, and docs. Approval-gate and policy-denylist now key on function IDs (with legacy compatibility shims).

Worker layout standardization

~13 workers refactored to use shared manifest/config modules: auth-credentials, harness, hook-fanout, llm-budget, models-catalog, policy-denylist, provider-anthropic, provider-openai, provider-router, session-inbox, session-tree, shell-bash, shell-filesystem, subagent. Function namespaces renamed to match crate names.

Shell worker consolidation (#103)

shell-bash + shell-filesystem collapsed into a single shell worker.

session-tree

• New ::list (paginated + ordered), ::ensure (idempotent create), ::reconcile (repair state↔tree drift).
• entry_id added to ::messages response (BREAKING).
• turn-orchestrator dual-writes messages to session-tree as a best-effort mirror.

Bug fix — parallel function results (provider-anthropic)

Anthropic rejects requests where parallel tool_use IDs are answered across multiple user messages. The wire converter previously emitted one user message per FunctionResult, breaking any turn with parallel calls. Now consecutive FunctionResults collapse into a single user message with multiple tool_result blocks.

Other

• Approval-gate added to EXPECTED_WORKERS.
• Phase A E2E acceptance: dual-write + fork + reconcile.
• Trigger types and function IDs updated for durability.

Test plan

• cargo test green across provider-anthropic, harness, turn-orchestrator, session-tree, policy-denylist, hook-fanout
• Phase A E2E acceptance suite passes (dual-write, fork, reconcile)
• New regression test parallel_function_results_collapse_into_one_user_message covers the Anthropic wire-shape fix
• Smoke the demo harness end-to-end: start engine, run a session that triggers parallel function calls, confirm no Anthropic 400s
• Verify approval-gate + policy-denylist still gate correctly on function IDs

Notes for reviewers

• entry_id on session-tree::messages is a breaking response shape change; downstream consumers go through session-tree so the migration is in-tree.
• The merge commit (b7c7e4a) reconciles squash-merge history mismatch with main (Phase A/B already landed there as squashes); resolutions favored this branch for files where it's a strict superset.

[coderabbitai]

Warning

Rate limit exceeded

@ytallo has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 34 minutes and 1 second before requesting another review.

You’ve run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 6d4adab8-79ad-4964-9046-8bc172c74498

📥 Commits

Reviewing files that changed from the base of the PR and between 1d515c2 and f236f13.

📒 Files selected for processing (1)

• provider-anthropic/src/lib.rs

📝 Walkthrough

Walkthrough

Migrates from tool-based to function-based contracts. Adds agent_call dispatcher and function lifecycle handlers; updates orchestrator persistence, transitions, and system prompt; aligns providers and streaming to FunctionCall/FunctionResult; moves policy/approval to agent::before_function_call; refreshes harness UI, docs, Makefile, and many tests.

Changes

Function-call migration

Layer / File(s)	Summary
Schemas & Contracts provider-*/**/harness-types/*, session-tree/crates/harness-types/*, turn-orchestrator/crates/harness-types/*, harness/web/src/types.ts	Renames Tool* → Function* across public types, content, and events; adds serde aliases; updates re-exports.
Core Orchestration turn-orchestrator/src/*	Adds agent_call dispatcher; replaces ToolPrepare/Execute/Finalize with FunctionPrepare/Execute/Finalize; updates persistence keys, staging, transitions, system_prompt; removes tools_catalog.
Providers & Streaming provider-anthropic/*, provider-openai/*, provider-base/*, provider-router/*	Aligns streaming, deltas, finish reasons, and registration to FunctionCall/FunctionResult and functioncall_* events; updates OpenAI/Anthropic wire conversions and provider adapter signatures.
Policy & Approval policy-denylist/*, approval-gate/*	Subscribe to agent::before_function_call; deny by function_id; approvals keyed by function_call_id with legacy fallbacks; README/config/env parsing updated.
Harness server/UI harness/src/*, harness/web/src/*, harness/Makefile, harness/scripts/*	Fanout eviction on function_not_found; new fs inline-read helper; skill register helpers; UI components (FunctionCallBlock, FunctionResultBlock, Markdown); filesystem panel uses shell::fs::*; approvals UI uses function_call_id.
Docs/Config/Tests READMEs, ARCHITECTURE, *.worker.yaml, many tests, .gitignore, package.json	Documentation and manifests updated; added markdown deps; broad unit/integration/e2e test changes to function_* naming; new demo Makefile and scripts.

Sequence Diagram(s)

sequenceDiagram
  participant Web as Web UI
  participant TO as Turn Orchestrator
  participant Pol as Policy Denylist
  participant Prov as Provider
  participant Eng as III Engine
  Web->>TO: run start → agent_call(function,payload)
  TO->>Pol: before_function_call(function_id)
  Pol-->>TO: allow/deny
  TO->>Prov: stream(functions)
  Prov-->>TO: Functioncall* deltas
  TO->>Eng: iii.trigger(function_id,payload)
  Eng-->>TO: FunctionResult
  TO-->>Web: events + function_results

Loading

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Possibly related PRs

• iii-hq/workers#80 — Overlapping consolidation of shell workers and harness changes.
• iii-hq/workers#31 — Related consolidation of shell worker surface and function IDs.
• iii-hq/workers#102 — Prior fanout/WS subscription plumbing extended here with eviction and function_not_found handling.

Suggested reviewers

• sergiofilhowz

Poem

I hop from tools to functions bright,
A sleeker path, a cleaner flight;
Approvals nod, providers sing,
Orchestrator leads the spring.
With agent_call, we bound ahead—
Carrots compiled, the diff is fed. 🐇✨

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/iii-native-harness

[coderabbitai]

Actionable comments posted: 14

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

session-tree/src/lib.rs (1)

702-707: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Stale "tool results" reference in rustdoc.

The migration switched to function terminology, but the public docstring for export_html still says "tool results dim".

📝 Proposed doc fix

 /// Returns a self-contained HTML document rendering the active path.
 ///
-/// User messages are styled cyan, assistant white-on-dark, tool results dim,
-/// thinking blocks italic. All CSS is inline. Special HTML characters in
-/// content are escaped.
+/// User messages are styled cyan, assistant white-on-dark, function results
+/// dim, thinking blocks italic. All CSS is inline. Special HTML characters in
+/// content are escaped.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@session-tree/src/lib.rs` around lines 702 - 707, The docstring for
export_html is stale: update the wording to replace "tool results dim" with the
current terminology ("function results dim" or simply "function results dim")
and ensure the rest of the docs reflect the migration to function terminology;
edit the doc comment above the pub async fn export_html<S: SessionStore +
?Sized> to mention "function results dim" (or another chosen current term) and
keep the rest of the styling descriptions and HTML-escaping note unchanged.

turn-orchestrator/README.md (1)

85-85: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Stale "Tool names" wording in the approval_required field description.

Line 85 still reads "Tool names requiring human approval before execution" — with the PR-wide tool_* → function_* migration this should say "Function names".

📝 Proposed fix

-| `approval_required` | string[] | no | `[]` | Tool names requiring human approval before execution. |
+| `approval_required` | string[] | no | `[]` | Function names requiring human approval before execution. |

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@turn-orchestrator/README.md` at line 85, Update the README entry for the
approval_required field: replace the phrase "Tool names requiring human approval
before execution" with "Function names requiring human approval before
execution" so it reflects the repo-wide tool_* → function_* migration; locate
the table row that defines `approval_required` and update its description string
accordingly.

🧹 Nitpick comments (22)

turn-orchestrator/crates/harness-types/src/stream_event.rs (1)

11-12: ⚡ Quick win

Add wire-contract tests for the renamed stop/event tags.

This change relies on function_call / functioncall_* serializing forward while tool / toolcall_* still deserialize for backward compatibility, but none of that contract is exercised here. A serde typo would only show up once cross-worker streaming hits one of these paths. The same gap exists in the sibling harness-types crates.

Also applies to: 76-86

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@turn-orchestrator/crates/harness-types/src/stream_event.rs` around lines 11 -
12, Test the serde "wire contract" for the renamed tags by adding unit tests
that assert forward serialization uses the new names and backward compatibility
still deserializes the old names: write tests in harness-types (e.g., in
stream_event.rs tests module) that serialize the enum variant FunctionCall and
verify the output contains "function_call" / "functioncall_*" tags, and
separately deserialize JSON strings using the legacy "tool" / "toolcall_*" names
to ensure they still map to FunctionCall; do the same for the renamed stop/event
variants referenced around the same area (lines ~76-86) so both new serialized
forms and legacy deserialization are covered. Ensure tests fail if serde
rename/alias attributes are wrong and keep them as part of the crate test suite.

turn-orchestrator/crates/harness-types/src/content.rs (1)

18-21: ⚡ Quick win

Backfill the legacy toolResult compatibility test too.

This file now aliases toolResult / tool_call_id, but the new coverage only proves the legacy toolCall path. A small serde test here would catch a schema typo before old persisted function results stop loading. The same gap appears in the sibling harness-types crates.

Suggested test

+    #[test]
+    fn function_result_block_legacy_tool_result_type() {
+        let json = r#"{"type":"toolResult","tool_call_id":"call_1","content":[{"type":"text","text":"ok"}],"is_error":false}"#;
+        let back: ContentBlock = serde_json::from_str(json).unwrap();
+        assert_eq!(
+            back,
+            ContentBlock::FunctionResult {
+                function_call_id: "call_1".into(),
+                content: vec![ContentBlock::Text(TextContent { text: "ok".into() })],
+                is_error: false,
+            }
+        );
+    }

Also applies to: 71-84

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@turn-orchestrator/crates/harness-types/src/content.rs` around lines 18 - 21,
Add a serde unit test that ensures the FunctionResult variant accepts both the
new and legacy field names: deserialize JSON payloads using "functionResult" and
the legacy "toolResult" (and using "tool_call_id" as well as the current
"function_call_id") and assert the enum deserializes to the FunctionResult
variant with the expected function_call_id value; apply the same pattern to the
other affected variant(s) around lines 71-84 in the same crate (and mirror the
test in the sibling harness-types crates) so any schema/alias typos are caught.

session-tree/crates/harness-types/src/agent_message.rs (1)

109-118: ⚡ Quick win

Strengthen legacy-shape test to verify field aliases actually populate fields.

The current matches!(m_old, AgentMessage::FunctionResult(_)) assertion will pass even if the tool_call_id/tool_name aliases silently fail to map to function_call_id/function_id (e.g., serde would just leave them as default empty strings if the aliases were ever removed). Asserting on the inner fields locks in the back-compat contract.

♻️ Proposed assertion strengthening

-        let m: AgentMessage = serde_json::from_str(json).unwrap();
-        let m_old: AgentMessage = serde_json::from_str(json_old).unwrap();
-        assert!(matches!(m, AgentMessage::FunctionResult(_)));
-        assert!(matches!(m_old, AgentMessage::FunctionResult(_)));
+        let m: AgentMessage = serde_json::from_str(json).unwrap();
+        let m_old: AgentMessage = serde_json::from_str(json_old).unwrap();
+        let AgentMessage::FunctionResult(fr) = m else { panic!("expected FunctionResult") };
+        assert_eq!(fr.function_call_id, "c1");
+        assert_eq!(fr.function_id, "x");
+        let AgentMessage::FunctionResult(fr_old) = m_old else { panic!("expected FunctionResult") };
+        assert_eq!(fr_old.function_call_id, "c1");
+        assert_eq!(fr_old.function_id, "x");

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@session-tree/crates/harness-types/src/agent_message.rs` around lines 109 -
118, The legacy-shape test function function_result_legacy_tool_result_role
currently only checks the variant; update it to deserialize m_old into
AgentMessage::FunctionResult, extract the inner struct (match on
AgentMessage::FunctionResult(payload) or use if let) and assert that
payload.function_call_id == "c1" and payload.function_id == "x" to ensure the
tool_call_id/tool_name aliases actually populate function_call_id/function_id;
do the same for m to assert its function_call_id/function_id too so both shapes
are validated.

session-tree/src/lib.rs (1)

819-825: 💤 Low value

Leftover tool naming in the FunctionResult render arm.

The local tr binding and the emitted HTML class tool-result (also defined at line 738) are leftovers from tool naming. They still work because the class string and the CSS rule match, but they're now inconsistent with the rest of the function-based contract and with the user-visible "function result" label. Consumers consuming the exported HTML for styling overrides will see a tool-result class that no longer matches the public model.

♻️ Proposed rename for consistency

-  .tool-result {{ background: `#161b22`; border-left-color: `#6e7681`; color: `#8b949e`; opacity: 0.85; }}
+  .function-result {{ background: `#161b22`; border-left-color: `#6e7681`; color: `#8b949e`; opacity: 0.85; }}

-        AgentMessage::FunctionResult(tr) => {
-            let body = render_blocks_html(&tr.content);
-            let name = html_escape(&tr.function_id);
+        AgentMessage::FunctionResult(fr) => {
+            let body = render_blocks_html(&fr.content);
+            let name = html_escape(&fr.function_id);
             format!(
-                "<div class=\"entry tool-result\"><div class=\"role\">function result · {name}</div>{body}</div>\n"
+                "<div class=\"entry function-result\"><div class=\"role\">function result · {name}</div>{body}</div>\n"
             )
         }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@session-tree/src/lib.rs` around lines 819 - 825, The FunctionResult match arm
uses a leftover binding name `tr` and emits a CSS class "tool-result" which is
inconsistent with the function-based contract and the visible label "function
result"; change the binding name (e.g., `fr` or `result`) in
AgentMessage::FunctionResult to reflect "function result", update the emitted
HTML class from "tool-result" to "function-result" (and update any corresponding
CSS rule that defines .tool-result to .function-result), and keep the use of
render_blocks_html(&tr.content) and html_escape(&tr.function_id) but reference
the new binding name so the code and CSS are consistent with the public
"function result" wording.

turn-orchestrator/src/agent_call.rs (3)

79-95: 💤 Low value

Avoid hardcoded line references in code comments.

iii.rs:1701 (line 81) and iii.rs:1155 (line 90) become stale on the next refactor of iii_sdk and silently mislead future readers. Prefer naming the variant/symbol (IIIError::Remote { code: "function_not_found" }, IIIError::Timeout) which is what the code already matches against.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@turn-orchestrator/src/agent_call.rs` around lines 79 - 95, The comments in
is_function_not_found and is_timeout reference hardcoded file:line pointers
(e.g. `iii.rs:1701`, `iii.rs:1155`) which will go stale; update those comments
to remove the line references and instead cite the actual symbols/variants being
matched (e.g. `IIIError::Remote { code: "function_not_found" }` in
is_function_not_found and `IIIError::Timeout` in is_timeout) and briefly state
that these functions match those variants exactly rather than using substring
Display matching.

---

286-288: 💤 Low value

Remove the personal local path from the comment.

The comment references an absolute path on the author's machine (/Users/ytallolayon/.claude/plans/...). It will be a dangling/private reference for any other contributor and unnecessarily leaks the developer's local layout. Either drop these two lines or replace them with a stable in-repo reference (e.g., a doc/issue link).

🧹 Suggested cleanup

-    // ── Adversarial unit tests added per plan
-    // /Users/ytallolayon/.claude/plans/let-s-implement-more-tests-refactored-flask.md
+    // ── Adversarial unit tests

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@turn-orchestrator/src/agent_call.rs` around lines 286 - 288, In
turn-orchestrator/src/agent_call.rs remove or replace the personal absolute path
string
"/Users/ytallolayon/.claude/plans/let-s-implement-more-tests-refactored-flask.md"
found in the comment block around the Adversarial unit tests note; either delete
the two comment lines or substitute a stable in-repo reference (e.g., docs/ or
an issue URL) so the comment no longer contains a local machine path.

---

193-388: 💤 Low value

Optional: collapse tests and dispatch_tests into one module.

There are two #[cfg(test)] mod blocks in this file. Idiomatic Rust uses a single tests module per file; splitting them into two named modules adds friction without a clear benefit, especially since both share the same super::* imports. Feel free to merge for consistency with the rest of the workspace.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@turn-orchestrator/src/agent_call.rs` around lines 193 - 388, There are two
separate #[cfg(test)] modules (`tests` and `dispatch_tests`) which should be
merged into a single tests module for idiomatic Rust; collapse `dispatch_tests`
into the existing `mod tests` (or vice‑versa) so there is only one #[cfg(test)]
mod tests { ... } containing all tests, keep a single use super::* and any extra
imports (e.g., serde_json::json, harness_types) at top of that module, and
ensure all referenced symbols (agent_call_tool, validate_function_field,
decode_or_passthrough, TOOL_NAME, FUNCTION_ID, is_timeout,
is_function_not_found, IIIError) remain accessible after the merge.

turn-orchestrator/crates/harness-types/src/agent_event.rs (1)

1-188: ⚖️ Poor tradeoff

Note: three near-identical copies of agent_event.rs across crates.

turn-orchestrator, provider-router, and session-tree each ship a separate harness-types/src/agent_event.rs that is essentially byte-identical. That's pre-existing in the repo, but every cross-cutting rename (like this one) has to be done in lockstep three times. If you have appetite later, a shared harness-types crate would remove that maintenance tax. Not blocking this PR.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@turn-orchestrator/crates/harness-types/src/agent_event.rs` around lines 1 -
188, There are three near-identical copies of agent_event.rs (containing the
ApprovalDecision and AgentEvent enums) across crates which creates maintenance
drift; consolidate by extracting a single shared harness-types crate (or a
common module) and move the canonical definitions of ApprovalDecision and
AgentEvent (plus any dependent types like FunctionResultMessage, AgentMessage,
AssistantMessageEvent) into it, then update the three crate Cargo.toml and any
use/import paths to depend on that new harness-types crate and remove the
duplicate files to ensure a single source of truth.

turn-orchestrator/crates/harness-types/src/function.rs (1)

69-92: 💤 Low value

Optional: derive PartialEq for consistency with sibling types.

FunctionCall and FunctionResult derive PartialEq but PreparedFunctionCall and FinalizedFunctionCall do not. If you ever want to assert these in tests (similar to the function_call_roundtrip test below) you'll need to add it then. Not blocking — only mention if the omission was unintentional.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@turn-orchestrator/crates/harness-types/src/function.rs` around lines 69 - 92,
The review notes PreparedFunctionCall and FinalizedFunctionCall lack PartialEq
even though sibling types do; add PartialEq to their derives so tests can assert
equality (update #[derive(Debug, Clone, Serialize, Deserialize)] to
#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] for both the
PreparedFunctionCall enum and the FinalizedFunctionCall struct) and ensure
serialization attributes (serde tags/aliases) remain unchanged for FunctionCall,
FunctionResult, PreparedFunctionCall, and FinalizedFunctionCall.

turn-orchestrator/src/system_prompt.rs (1)

8-8: 💤 Low value

Wording drift: BASE_BODY says "tool result" mid function-rename.

The rest of the prompt (and the wider PR) standardizes on agent_call + function, but this sentence still says "If a tool result contains blocked: true". Worth aligning to "function result" to avoid confusing the model after the rest of the surface flipped to function terminology.

✏️ Suggested wording fix

-Paths must be absolute. If a tool result contains `blocked: true`, a policy
-refused it — explain which policy and stop, do not retry.
+Paths must be absolute. If a function result contains `blocked: true`, a policy
+refused it — explain which policy and stop, do not retry.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@turn-orchestrator/src/system_prompt.rs` at line 8, BASE_BODY still uses the
phrase "tool result" which conflicts with the rest of the prompt that
standardized on "agent_call" and "function"; update the string constant
BASE_BODY to replace "tool result" with "function result" (and adjust
surrounding wording if needed) so the sentence reads e.g. "If a function result
contains `blocked: true`, a policy refused it — explain which policy and stop,
do not retry." Ensure references to `agent_call` and `function` remain intact.

harness/web/src/export.ts (1)

66-70: 💤 Low value

Redundant double cast — single cast already covers both fields.

The first as { function_call_id?: string; tool_call_id?: string } already exposes tool_call_id, so the second cast on line 69 is unnecessary.

🧹 Suggested simplification

-      const rid =
-        (message as { function_call_id?: string; tool_call_id?: string })
-          .function_call_id ??
-        (message as { tool_call_id?: string }).tool_call_id ??
-        "unknown";
+      const rid =
+        (message as { function_call_id?: string; tool_call_id?: string })
+          .function_call_id ??
+        (message as { function_call_id?: string; tool_call_id?: string })
+          .tool_call_id ??
+        "unknown";

Or more concisely:

-      const rid =
-        (message as { function_call_id?: string; tool_call_id?: string })
-          .function_call_id ??
-        (message as { tool_call_id?: string }).tool_call_id ??
-        "unknown";
+      const { function_call_id, tool_call_id } = message as {
+        function_call_id?: string;
+        tool_call_id?: string;
+      };
+      const rid = function_call_id ?? tool_call_id ?? "unknown";

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@harness/web/src/export.ts` around lines 66 - 70, The assignment to rid uses a
redundant double cast; change it to a single typed access so you only cast
message once (e.g., cast to { function_call_id?: string; tool_call_id?: string }
and then use .function_call_id ?? .tool_call_id ?? "unknown"). Update the
expression that sets rid (the variable named rid in this block) to remove the
second (message as { tool_call_id?: string }) cast and rely on the first cast or
a single local typed variable to access both fields.

turn-orchestrator/tests/integration.rs (1)

4-9: 💤 Low value

function_schemas_key not covered by the namespace test.

state_keys_namespace_by_session doesn't assert that function_schemas_key(s).contains(s), while every other key added to the state_keys_distinct_per_facet array is expected to follow that invariant.

➕ Suggested addition

 fn state_keys_namespace_by_session() {
     let s = "sess-1";
     assert!(turn_orchestrator::messages_key(s).contains(s));
     assert!(turn_orchestrator::turn_state_key(s).contains(s));
     assert!(turn_orchestrator::run_request_key(s).contains(s));
+    assert!(turn_orchestrator::function_schemas_key(s).contains(s));
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@turn-orchestrator/tests/integration.rs` around lines 4 - 9, The test
state_keys_namespace_by_session omits asserting that function_schemas_key(s)
contains the session namespace; update that test to include an assertion like
assert!(turn_orchestrator::function_schemas_key(s).contains(s)) so it matches
the other keys in state_keys_distinct_per_facet (turn_state_key, messages_key,
run_request_key) and ensures function_schemas_key follows the same
session-scoped invariant.

provider-openai/crates/harness-types/src/stream_event.rs (1)

11-12: ⚡ Quick win

Add serde tests for the legacy aliases.

These aliases are the backward-compatibility contract for older stored events/clients, so a couple of deserialize/serialize tests for "tool" and "toolcall_*" would lock the migration down and catch accidental renames later.

Also applies to: 76-86

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@provider-openai/crates/harness-types/src/stream_event.rs` around lines 11 -
12, Add serde unit tests that assert backward-compatibility aliases still
serialize and deserialize correctly: write tests for the enum containing the
FunctionCall variant (rename annotated with #[serde(rename = "function_call",
alias = "tool")]) to deserialize from the legacy "tool" string and serialize
back to the canonical "function_call"; likewise add tests for the other legacy
"toolcall_*" aliases referenced in the same enum (lines ~76-86) by deserializing
example JSON strings using those legacy names and asserting they round-trip to
the expected enum variants and serialize to the canonical names. Ensure tests
use serde_json::from_str and serde_json::to_string and reference the enum type
(e.g., StreamEvent or the enum declared in stream_event.rs) and the FunctionCall
variant so CI will catch accidental renames.

provider-anthropic/crates/harness-types/src/agent_message.rs (1)

109-118: ⚡ Quick win

Consider adding a canonical-shape serialization assertion.

The new test only verifies deserialization of legacy variants. Adding a roundtrip + a serde_json::to_value assertion that the canonical write shape uses role: "function_result", function_call_id, and function_id would pin the wire contract so a future serde rename can't silently regress writers consuming the new shape.

♻️ Suggested addition

+    #[test]
+    fn function_result_canonical_serialization_shape() {
+        let m = AgentMessage::FunctionResult(FunctionResultMessage {
+            function_call_id: "c1".into(),
+            function_id: "x".into(),
+            content: vec![],
+            details: serde_json::json!({}),
+            is_error: false,
+            timestamp: 0,
+        });
+        let v = serde_json::to_value(&m).unwrap();
+        assert_eq!(v["role"], "function_result");
+        assert!(v.get("function_call_id").is_some());
+        assert!(v.get("function_id").is_some());
+        assert!(v.get("tool_call_id").is_none());
+        assert!(v.get("tool_name").is_none());
+    }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@provider-anthropic/crates/harness-types/src/agent_message.rs` around lines
109 - 118, Add a canonical-shape serialization assertion to the existing test
function_result_legacy_tool_result_role: after deserializing legacy and current
JSON into AgentMessage::FunctionResult, construct or reuse one of the parsed
FunctionResult variants and call serde_json::to_value (or serde_json::to_string)
to assert the serialized form uses role: "function_result" and the new field
names function_call_id and function_id (not the old tool_call_id/tool_name),
ensuring the canonical writer shape is pinned for future serde renames.

policy-denylist/src/main.rs (1)

35-46: 💤 Low value

Consider logging a deprecation warning when the legacy env var is used.

When POLICY_DENIED_FUNCTIONS is unset and POLICY_DENIED_TOOLS is consumed as a fallback, operators have no signal that they're on the deprecated path. A single tracing::warn! here would make the rename observable in deployment logs without changing behavior.

♻️ Suggested change

     } else if let Ok(denied) = std::env::var("POLICY_DENIED_TOOLS") {
         let denied_functions = parse_denied_functions(&denied);
         if !denied_functions.is_empty() {
+            tracing::warn!(
+                "POLICY_DENIED_TOOLS is deprecated; rename to POLICY_DENIED_FUNCTIONS"
+            );
             cfg.denied_functions = denied_functions;
         }
     }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@policy-denylist/src/main.rs` around lines 35 - 46, When the code falls back
from POLICY_DENIED_FUNCTIONS to the legacy POLICY_DENIED_TOOLS, add a
tracing::warn! to make the deprecation visible; locate the branch that reads
POLICY_DENIED_TOOLS (the else if that calls parse_denied_functions and sets
cfg.denied_functions) and insert a single tracing::warn! message indicating
POLICY_DENIED_TOOLS is deprecated and will be removed, then proceed to
parse_denied_functions(&denied) and set cfg.denied_functions as before. Ensure
the message references the legacy env var name so operators can find and migrate
it.

turn-orchestrator/src/states/assistant.rs (1)

199-222: 💤 Low value

Rename assistant_tool test fixture for consistency with the rename.

The fixture now constructs ContentBlock::FunctionCall and StopReason::FunctionCall, but it's still named assistant_tool. Renaming to assistant_function_call (or similar) keeps the test surface aligned with the function-call terminology used in the rest of the PR.

♻️ Suggested change

-    fn assistant_tool() -> AssistantMessage {
+    fn assistant_function_call() -> AssistantMessage {
         AssistantMessage {
             content: vec![ContentBlock::FunctionCall {
                 id: "x".into(),
                 function_id: "read".into(),
                 arguments: json!({}),
             }],
             stop_reason: StopReason::FunctionCall,
@@
     #[test]
     fn extract_function_calls_collects_function_blocks_only() {
         assert!(extract_function_calls(&assistant_text()).is_empty());
-        let calls = extract_function_calls(&assistant_tool());
+        let calls = extract_function_calls(&assistant_function_call());
         assert_eq!(calls.len(), 1);
         assert_eq!(calls[0].function_id, "read");
     }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@turn-orchestrator/src/states/assistant.rs` around lines 199 - 222, Rename the
test fixture function assistant_tool to assistant_function_call to match the
function-call terminology: change the function name for the helper that
constructs an AssistantMessage containing ContentBlock::FunctionCall and
StopReason::FunctionCall, and update any references to it in tests (for example
the call in extract_function_calls_collects_function_blocks_only that currently
calls assistant_tool()). Keep the function body and signature unchanged, only
rename the identifier to assistant_function_call so tests and fixtures remain
consistent.

provider-anthropic/crates/provider-base/src/iii_register.rs (1)

158-164: 💤 Low value

Wire field tools is intentional and consistently used — optional suggestion to accept both keys for forward-compat.

The trigger payload reads only the tools key, and this is the intentional wire contract: the orchestrator sends "tools", and all callers consistently use this field name. The internal type rename from AgentTool to AgentFunction did not (and should not) affect the wire contract.

Accepting both functions and tools keys here is purely optional for future-proofing. There is no current evidence of callers attempting to send the new canonical functions key. If this alignment is desired, apply the suggestion; otherwise, the current code is correct and doesn't require changes.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@provider-anthropic/crates/provider-base/src/iii_register.rs` around lines 158
- 164, The payload parsing currently only reads "tools" into a
Vec<AgentFunction>; to accept both "tools" and the optional future "functions"
key, attempt to read "functions" first then fall back to "tools" (or merge both)
and map into Vec<AgentFunction>, returning IIIError::Handler on serde errors;
update the parsing expression that constructs tools (the code using
payload.get("tools") -> serde_json::from_value -> transpose ->
IIIError::Handler) to check payload.get("functions") and payload.get("tools")
accordingly and combine/choose them into the final Vec<AgentFunction>.

turn-orchestrator/src/states/provisioning.rs (1)

53-68: ⚡ Quick win

Run the index fetch and skills list concurrently to save a round-trip on bootstrap.

fetch_uri("iii://skills") and list_root_skill_uris(...) are independent calls that today execute sequentially, adding an avoidable RTT to every first turn. They can be issued in parallel; only the body-batch step needs to wait for the list.

♻️ Proposed parallelization

 async fn fetch_skills_bootstrap(iii: &III) -> Option<String> {
-    let index = fetch_uri(iii, "iii://skills").await;
-    let root_uris = list_root_skill_uris(iii).await;
+    let (index, root_uris) = tokio::join!(
+        fetch_uri(iii, "iii://skills"),
+        list_root_skill_uris(iii),
+    );
     let bodies = if root_uris.is_empty() {
         None
     } else {
         fetch_uris_batched(iii, &root_uris).await
     };

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@turn-orchestrator/src/states/provisioning.rs` around lines 53 - 68, The two
independent I/O calls in fetch_skills_bootstrap (fetch_uri(iii, "iii://skills")
and list_root_skill_uris(iii)) should be issued concurrently to remove an extra
RTT: use an async join (e.g., tokio::join! or futures::join) to run fetch_uri
and list_root_skill_uris in parallel, then, after awaiting the join result, call
fetch_uris_batched(iii, &root_uris). Ensure you keep the existing match logic
and only await fetch_uris_batched when the joined list is non-empty; refer to
fetch_skills_bootstrap, fetch_uri, list_root_skill_uris, and fetch_uris_batched
to locate the changes.

provider-anthropic/src/lib.rs (1)

195-260: 💤 Low value

Optional: parameter name tools is now stale; consider functions.

functions_to_wire, stream, and stream_inner all take a parameter called tools even though the type is now Vec<AgentFunction>. The wire-level Anthropic field ("tools") is unchanged, but at the Rust API surface the name is misleading post-migration.

♻️ Suggested rename

-pub fn functions_to_wire(tools: &[harness_types::AgentFunction]) -> Vec<serde_json::Value> {
-    tools
+pub fn functions_to_wire(functions: &[harness_types::AgentFunction]) -> Vec<serde_json::Value> {
+    functions
         .iter()
         .map(|t| {
@@
 pub async fn stream(
     cfg: Arc<AnthropicConfig>,
     system_prompt: String,
     messages: Vec<harness_types::AgentMessage>,
-    tools: Vec<harness_types::AgentFunction>,
+    functions: Vec<harness_types::AgentFunction>,
 ) -> ReceiverStream<AssistantMessageEvent> {

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@provider-anthropic/src/lib.rs` around lines 195 - 260, Rename the misleading
parameter name "tools" to "functions" in the public API functions
functions_to_wire, stream, and stream_inner (and their signatures/usages) to
reflect the Vec<harness_types::AgentFunction> type; update any internal variable
bindings, calls, and references (e.g., in functions_to_wire's iterator, stream's
arguments passed into stream_inner, and stream_inner's parameter list) to use
"functions" consistently, while leaving the wire-level field names untouched and
preserving existing behavior and function names.

harness/web/src/types.ts (1)

233-240: 💤 Low value

Consider documenting the "at least one id" invariant on PendingApproval.

All identifier fields are now optional, which makes the type accept records with no id at all. Since the runtime always has one of {function_call_id, tool_call_id} (and one of {function_id, tool_name}), a short JSDoc note would help downstream consumers understand the invariant without resorting to non-null assertions.

📝 Suggested doc comment

+/**
+ * Pending approval shape during the tool→function rename window.
+ * Invariant: at least one of `function_call_id`/`tool_call_id` is present,
+ * and at least one of `function_id`/`tool_name` is present.
+ */
 export interface PendingApproval {
   function_call_id?: string;
   tool_call_id?: string;
   function_id?: string;
   tool_name?: string;
   args?: unknown;
   expires_at?: number;
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@harness/web/src/types.ts` around lines 233 - 240, Add a JSDoc comment above
the PendingApproval interface that documents the runtime invariant: that at
least one of function_call_id or tool_call_id will be present and at least one
of function_id or tool_name will be present (and optionally note args/expires_at
semantics); reference the exact field names (function_call_id, tool_call_id,
function_id, tool_name) so downstream consumers know they must treat those as
mutually optional but collectively required without changing the type itself.

approval-gate/src/lib.rs (1)

498-501: 💤 Low value

Stale test name references tool_call_id.

Function under test now keys on function_call_id. The test name is still pending_key_includes_session_and_tool_call_id. Cosmetic; rename when convenient.

♻️ Suggested rename

-    fn pending_key_includes_session_and_tool_call_id() {
+    fn pending_key_includes_session_and_function_call_id() {
         assert_eq!(pending_key("s1", "tc-1"), "s1/tc-1");
     }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@approval-gate/src/lib.rs` around lines 498 - 501, The test name is outdated:
rename the test function pending_key_includes_session_and_tool_call_id to
reflect the new key name (e.g.,
pending_key_includes_session_and_function_call_id) so it matches the
implementation that keys on function_call_id; update the test identifier for the
function pending_key and any references to the old test name so CI and test
reporting use the correct descriptive name.

turn-orchestrator/src/persistence.rs (1)

252-312: LGTM — clean dual-key migration pattern.

staging_get_with_legacy plus the function_call/tool_call wrapper alias on read makes the migration backward-compatible without touching the write path. Once a session has been prepared under the new code, reads always hit the new key.

One small operational note: the legacy keys are never deleted, so per-session legacy entries stay in state storage as cold data. Bounded and harmless, but you could tack on an opportunistic state::delete of the legacy key after the first successful new write if you want to keep the namespace tidy.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@turn-orchestrator/src/persistence.rs` around lines 252 - 312, Add an
opportunistic cleanup to remove the legacy per-session key after we successfully
write the new prepared-key: inside save_prepared_calls (after the state_set call
that writes to staging_key(session_id, PREPARED_KEY)), call the delete helper
(e.g. state_delete or state::delete) on staging_key(session_id,
LEGACY_PREPARED_KEY) and await it, but ignore/delete errors so it’s non-fatal;
this keeps staging_get_with_legacy working while preventing stale legacy entries
from accumulating.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@harness/docs/sandbox-skill.md`:
- Around line 22-26: The fenced code block containing the diagram (the
triple-backtick block wrapping "sandbox::create  →  sandbox::exec /
sandbox::fs::*  →  sandbox::stop ...") is missing a language identifier which
triggers MD040; update that block header from ``` to ```text so the block begins
with ```text and keep the closing ``` unchanged to satisfy markdown linting.

In `@harness/Makefile`:
- Around line 19-20: The Makefile documents DEMO_ENGINE_WS but the CLI probe
targets still call iii (in the engine/start/verify targets and sections around
the III invocation) against the default local engine; update those targets so
the same engine URL used to spawn workers (III_URL="$(DEMO_ENGINE_WS)" /
III_DEMO_ENGINE_URL) is also used for readiness and verification probes—pass the
remote URL consistently to iii (e.g., via the III_URL env or the iii
--url/--engine-url flag used in the engine, start, and verify recipe commands)
so engine, start, and verify all point at DEMO_ENGINE_WS rather than the local
default.
- Around line 34-41: The Makefile's WORKERS variable is missing the iii-sandbox
worker, so runs like `make all` never build or start it; add `iii-sandbox` to
the WORKERS list in the Makefile (the WORKERS definition block) so the
build/start targets include that worker and the runtime published by
harness/src/lib.rs matches the started demos.

In `@harness/scripts/demo.sh`:
- Around line 110-117: The current demo script unconditionally sets
POLICY_DENIED_FUNCTIONS via
extra_env+=(POLICY_DENIED_FUNCTIONS="bridge::trigger") which overwrites any
user-provided value; update the policy-denylist branch in the case for symbol
policy-denylist to merge with an existing POLICY_DENIED_FUNCTIONS (e.g., prepend
or append "bridge::trigger" to the current $POLICY_DENIED_FUNCTIONS when it is
non-empty) before adding the entry to extra_env so user values are preserved, or
if you decide to keep the override instead, add a clear comment explaining that
this invocation intentionally replaces any user setting; reference the extra_env
array and the POLICY_DENIED_FUNCTIONS variable and the policy-denylist case to
locate the change.

In `@harness/src/fanout.rs`:
- Around line 292-305: Extract the eviction+error-handling block into a shared
helper (e.g., try_push_with_eviction or push_to_browser) that takes the browser
id, the send result/error, and references to fanout_for_gc and any logger;
inside the helper call is_function_not_found(&e) for errors, take
fanout_for_gc.write().await and call state.evict_browser(&browser_id) when
needed, emit the same tracing::debug when evicted and tracing::trace for other
errors, and return a boolean/Unit indicating success; then replace the
duplicated eviction logic in the ui::session::event::* path and the
ui::sessions::changed::*, ui::approval::*, ui::cost::tick::*, and
ui::workers::changed::* fanout sites to call this helper instead of inlining the
eviction logic so all UI fanout paths share the same behavior.

In `@harness/src/lib.rs`:
- Around line 416-426: The sandbox orientation skill is being published
unconditionally; change the logic around the iii.trigger call so you only
publish when iii-sandbox is actually available: check for the presence/health of
the sandbox worker or the specific function before calling TriggerRequest with
function_id "skills::register" (or inspect iii’s function list/health endpoint),
and only call build_sandbox_skill_register_payload() / iii.trigger(...) when
that check succeeds; if the availability check fails, skip the trigger and do
not register the "sandbox" orientation (see build_sandbox_skill_register_payload
and the existing iii.trigger usage to locate the code to modify).

In `@harness/web/src/components/FunctionResultBlock.tsx`:
- Around line 86-100: The markdown branch currently only renders when format ===
"markdown" && !truncated, so when a long result is truncated but the user clicks
the "show more" button (toggling expanded to true) the component still renders
the <pre> path; update the render condition in FunctionResultBlock to allow
rendering Markdown when expanded by changing the check to something like format
=== "markdown" && (!truncated || expanded) (and keep the alternate <pre> path
for the other case), ensuring the existing setExpanded toggle and
truncated/expanded state names are used so the full content displays as rendered
Markdown when expanded.

In `@harness/web/src/components/SessionView.tsx`:
- Around line 97-110: blockText currently only extracts text/content and
otherwise stringifies the whole block, so
function-result/tool_result/functionResult blocks (checked in SessionView where
b.type is used and FunctionResultBlock is rendered) lose the actual payload;
update the blockText(b) helper to also look for common result fields such as
b.output and b.result (and nested payload.output/payload.result if present),
returning their string form (or JSON-stringified inner object) before falling
back to JSON.stringify(b) so FunctionResultBlock receives the actual result text
instead of the entire envelope.

In `@harness/web/src/components/StatusTab.tsx`:
- Line 9: The top-of-file comment describing the events feed filters is stale
and lists wrong filter names; update or remove it so it matches the actual
FilterKind and rendered filter buttons used in StatusTab (e.g., the FilterKind
type and the filter buttons in the component where filters are rendered). Locate
the comment near the file header and either replace the
`(agent/state/function/error)` list with the current filter names (`all |
approval | cost | workers`) or remove the comment entirely to avoid misleading
documentation.

In `@harness/web/src/styles.css`:
- Line 849: Replace the deprecated CSS rule "word-break: break-word" with the
recommended combination: set "word-break: normal" and add "overflow-wrap:
anywhere" for the affected selectors; update the declarations in the rules for
".md code", ".block-body", and ".palette-code" so each uses "word-break: normal"
plus "overflow-wrap: anywhere" to preserve behavior and satisfy Stylelint.

In `@policy-denylist/iii.worker.yaml`:
- Around line 10-13: The default denied_functions entries ("bash:rm -rf", sudo,
curl-pipe-bash) use legacy tool-name strings and won't match function IDs;
update the denied_functions list to use real function_id patterns (for example
replace with entries like shell::bash::*, shell::filesystem::*,
network::curl::*), or make denied_functions an empty list and add a clear
comment instructing operators to populate it with appropriate function_id
values; ensure you edit the denied_functions block so it contains
function_id-style patterns rather than legacy names.

In `@policy-denylist/src/lib.rs`:
- Around line 14-15: DEFAULT_DENIED_FUNCTIONS currently contains legacy
shell-token patterns that will never match the new exact equality check in
handle_event which uses check_denylist; update the defaults to real function ids
(e.g., replace "bash:rm -rf", "sudo", "curl-pipe-bash" with the canonical
function ids your project intends to block such as "shell::bash::exec",
"system::sudo::run", "network::curl::exec" or whatever the real ids are), or
alternatively add a second predicate (e.g., check_denylist_substring or
check_denylist_extended) and call it from handle_event alongside check_denylist
to perform substring/shell-token matching for backward compatibility while
leaving check_denylist as exact-match.

In `@policy-denylist/src/main.rs`:
- Around line 170-172: Remove the leaked absolute local path from the inline
comment in policy-denylist/src/main.rs (the comment block mentioning
"Adversarial unit tests" that contains "/Users/ytallolayon/.claude/plans/...");
either delete that comment entirely or replace it with a neutral,
repo-appropriate note such as "Adversarial unit tests added per internal plan"
or a short explanation of the tests' purpose, ensuring no personal filesystem
paths or usernames remain.

In `@turn-orchestrator/src/states/functions.rs`:
- Around line 110-170: The loop currently emits FunctionExecutionStart
(events::emit with AgentEvent::FunctionExecutionStart) before checking prefilled
or cached executed calls, causing duplicate Start events on retries; move the
events::emit call that creates the FunctionExecutionStart down so it occurs only
after the prefilled check and the persistence::find_executed_call check (i.e.,
after the two early-continue paths) and before dispatching
crate::agent_call::dispatch, preserving the existing AgentEvent payload
construction and using the same fc, record.session_id and iii identifiers.

---

Outside diff comments:
In `@session-tree/src/lib.rs`:
- Around line 702-707: The docstring for export_html is stale: update the
wording to replace "tool results dim" with the current terminology ("function
results dim" or simply "function results dim") and ensure the rest of the docs
reflect the migration to function terminology; edit the doc comment above the
pub async fn export_html<S: SessionStore + ?Sized> to mention "function results
dim" (or another chosen current term) and keep the rest of the styling
descriptions and HTML-escaping note unchanged.

In `@turn-orchestrator/README.md`:
- Line 85: Update the README entry for the approval_required field: replace the
phrase "Tool names requiring human approval before execution" with "Function
names requiring human approval before execution" so it reflects the repo-wide
tool_* → function_* migration; locate the table row that defines
`approval_required` and update its description string accordingly.

---

Nitpick comments:
In `@approval-gate/src/lib.rs`:
- Around line 498-501: The test name is outdated: rename the test function
pending_key_includes_session_and_tool_call_id to reflect the new key name (e.g.,
pending_key_includes_session_and_function_call_id) so it matches the
implementation that keys on function_call_id; update the test identifier for the
function pending_key and any references to the old test name so CI and test
reporting use the correct descriptive name.

In `@harness/web/src/export.ts`:
- Around line 66-70: The assignment to rid uses a redundant double cast; change
it to a single typed access so you only cast message once (e.g., cast to {
function_call_id?: string; tool_call_id?: string } and then use
.function_call_id ?? .tool_call_id ?? "unknown"). Update the expression that
sets rid (the variable named rid in this block) to remove the second (message as
{ tool_call_id?: string }) cast and rely on the first cast or a single local
typed variable to access both fields.

In `@harness/web/src/types.ts`:
- Around line 233-240: Add a JSDoc comment above the PendingApproval interface
that documents the runtime invariant: that at least one of function_call_id or
tool_call_id will be present and at least one of function_id or tool_name will
be present (and optionally note args/expires_at semantics); reference the exact
field names (function_call_id, tool_call_id, function_id, tool_name) so
downstream consumers know they must treat those as mutually optional but
collectively required without changing the type itself.

In `@policy-denylist/src/main.rs`:
- Around line 35-46: When the code falls back from POLICY_DENIED_FUNCTIONS to
the legacy POLICY_DENIED_TOOLS, add a tracing::warn! to make the deprecation
visible; locate the branch that reads POLICY_DENIED_TOOLS (the else if that
calls parse_denied_functions and sets cfg.denied_functions) and insert a single
tracing::warn! message indicating POLICY_DENIED_TOOLS is deprecated and will be
removed, then proceed to parse_denied_functions(&denied) and set
cfg.denied_functions as before. Ensure the message references the legacy env var
name so operators can find and migrate it.

In `@provider-anthropic/crates/harness-types/src/agent_message.rs`:
- Around line 109-118: Add a canonical-shape serialization assertion to the
existing test function_result_legacy_tool_result_role: after deserializing
legacy and current JSON into AgentMessage::FunctionResult, construct or reuse
one of the parsed FunctionResult variants and call serde_json::to_value (or
serde_json::to_string) to assert the serialized form uses role:
"function_result" and the new field names function_call_id and function_id (not
the old tool_call_id/tool_name), ensuring the canonical writer shape is pinned
for future serde renames.

In `@provider-anthropic/crates/provider-base/src/iii_register.rs`:
- Around line 158-164: The payload parsing currently only reads "tools" into a
Vec<AgentFunction>; to accept both "tools" and the optional future "functions"
key, attempt to read "functions" first then fall back to "tools" (or merge both)
and map into Vec<AgentFunction>, returning IIIError::Handler on serde errors;
update the parsing expression that constructs tools (the code using
payload.get("tools") -> serde_json::from_value -> transpose ->
IIIError::Handler) to check payload.get("functions") and payload.get("tools")
accordingly and combine/choose them into the final Vec<AgentFunction>.

In `@provider-anthropic/src/lib.rs`:
- Around line 195-260: Rename the misleading parameter name "tools" to
"functions" in the public API functions functions_to_wire, stream, and
stream_inner (and their signatures/usages) to reflect the
Vec<harness_types::AgentFunction> type; update any internal variable bindings,
calls, and references (e.g., in functions_to_wire's iterator, stream's arguments
passed into stream_inner, and stream_inner's parameter list) to use "functions"
consistently, while leaving the wire-level field names untouched and preserving
existing behavior and function names.

In `@provider-openai/crates/harness-types/src/stream_event.rs`:
- Around line 11-12: Add serde unit tests that assert backward-compatibility
aliases still serialize and deserialize correctly: write tests for the enum
containing the FunctionCall variant (rename annotated with #[serde(rename =
"function_call", alias = "tool")]) to deserialize from the legacy "tool" string
and serialize back to the canonical "function_call"; likewise add tests for the
other legacy "toolcall_*" aliases referenced in the same enum (lines ~76-86) by
deserializing example JSON strings using those legacy names and asserting they
round-trip to the expected enum variants and serialize to the canonical names.
Ensure tests use serde_json::from_str and serde_json::to_string and reference
the enum type (e.g., StreamEvent or the enum declared in stream_event.rs) and
the FunctionCall variant so CI will catch accidental renames.

In `@session-tree/crates/harness-types/src/agent_message.rs`:
- Around line 109-118: The legacy-shape test function
function_result_legacy_tool_result_role currently only checks the variant;
update it to deserialize m_old into AgentMessage::FunctionResult, extract the
inner struct (match on AgentMessage::FunctionResult(payload) or use if let) and
assert that payload.function_call_id == "c1" and payload.function_id == "x" to
ensure the tool_call_id/tool_name aliases actually populate
function_call_id/function_id; do the same for m to assert its
function_call_id/function_id too so both shapes are validated.

In `@session-tree/src/lib.rs`:
- Around line 819-825: The FunctionResult match arm uses a leftover binding name
`tr` and emits a CSS class "tool-result" which is inconsistent with the
function-based contract and the visible label "function result"; change the
binding name (e.g., `fr` or `result`) in AgentMessage::FunctionResult to reflect
"function result", update the emitted HTML class from "tool-result" to
"function-result" (and update any corresponding CSS rule that defines
.tool-result to .function-result), and keep the use of
render_blocks_html(&tr.content) and html_escape(&tr.function_id) but reference
the new binding name so the code and CSS are consistent with the public
"function result" wording.

In `@turn-orchestrator/crates/harness-types/src/agent_event.rs`:
- Around line 1-188: There are three near-identical copies of agent_event.rs
(containing the ApprovalDecision and AgentEvent enums) across crates which
creates maintenance drift; consolidate by extracting a single shared
harness-types crate (or a common module) and move the canonical definitions of
ApprovalDecision and AgentEvent (plus any dependent types like
FunctionResultMessage, AgentMessage, AssistantMessageEvent) into it, then update
the three crate Cargo.toml and any use/import paths to depend on that new
harness-types crate and remove the duplicate files to ensure a single source of
truth.

In `@turn-orchestrator/crates/harness-types/src/content.rs`:
- Around line 18-21: Add a serde unit test that ensures the FunctionResult
variant accepts both the new and legacy field names: deserialize JSON payloads
using "functionResult" and the legacy "toolResult" (and using "tool_call_id" as
well as the current "function_call_id") and assert the enum deserializes to the
FunctionResult variant with the expected function_call_id value; apply the same
pattern to the other affected variant(s) around lines 71-84 in the same crate
(and mirror the test in the sibling harness-types crates) so any schema/alias
typos are caught.

In `@turn-orchestrator/crates/harness-types/src/function.rs`:
- Around line 69-92: The review notes PreparedFunctionCall and
FinalizedFunctionCall lack PartialEq even though sibling types do; add PartialEq
to their derives so tests can assert equality (update #[derive(Debug, Clone,
Serialize, Deserialize)] to #[derive(Debug, Clone, PartialEq, Serialize,
Deserialize)] for both the PreparedFunctionCall enum and the
FinalizedFunctionCall struct) and ensure serialization attributes (serde
tags/aliases) remain unchanged for FunctionCall, FunctionResult,
PreparedFunctionCall, and FinalizedFunctionCall.

In `@turn-orchestrator/crates/harness-types/src/stream_event.rs`:
- Around line 11-12: Test the serde "wire contract" for the renamed tags by
adding unit tests that assert forward serialization uses the new names and
backward compatibility still deserializes the old names: write tests in
harness-types (e.g., in stream_event.rs tests module) that serialize the enum
variant FunctionCall and verify the output contains "function_call" /
"functioncall_*" tags, and separately deserialize JSON strings using the legacy
"tool" / "toolcall_*" names to ensure they still map to FunctionCall; do the
same for the renamed stop/event variants referenced around the same area (lines
~76-86) so both new serialized forms and legacy deserialization are covered.
Ensure tests fail if serde rename/alias attributes are wrong and keep them as
part of the crate test suite.

In `@turn-orchestrator/src/agent_call.rs`:
- Around line 79-95: The comments in is_function_not_found and is_timeout
reference hardcoded file:line pointers (e.g. `iii.rs:1701`, `iii.rs:1155`) which
will go stale; update those comments to remove the line references and instead
cite the actual symbols/variants being matched (e.g. `IIIError::Remote { code:
"function_not_found" }` in is_function_not_found and `IIIError::Timeout` in
is_timeout) and briefly state that these functions match those variants exactly
rather than using substring Display matching.
- Around line 286-288: In turn-orchestrator/src/agent_call.rs remove or replace
the personal absolute path string
"/Users/ytallolayon/.claude/plans/let-s-implement-more-tests-refactored-flask.md"
found in the comment block around the Adversarial unit tests note; either delete
the two comment lines or substitute a stable in-repo reference (e.g., docs/ or
an issue URL) so the comment no longer contains a local machine path.
- Around line 193-388: There are two separate #[cfg(test)] modules (`tests` and
`dispatch_tests`) which should be merged into a single tests module for
idiomatic Rust; collapse `dispatch_tests` into the existing `mod tests` (or
vice‑versa) so there is only one #[cfg(test)] mod tests { ... } containing all
tests, keep a single use super::* and any extra imports (e.g., serde_json::json,
harness_types) at top of that module, and ensure all referenced symbols
(agent_call_tool, validate_function_field, decode_or_passthrough, TOOL_NAME,
FUNCTION_ID, is_timeout, is_function_not_found, IIIError) remain accessible
after the merge.

In `@turn-orchestrator/src/persistence.rs`:
- Around line 252-312: Add an opportunistic cleanup to remove the legacy
per-session key after we successfully write the new prepared-key: inside
save_prepared_calls (after the state_set call that writes to
staging_key(session_id, PREPARED_KEY)), call the delete helper (e.g.
state_delete or state::delete) on staging_key(session_id, LEGACY_PREPARED_KEY)
and await it, but ignore/delete errors so it’s non-fatal; this keeps
staging_get_with_legacy working while preventing stale legacy entries from
accumulating.

In `@turn-orchestrator/src/states/assistant.rs`:
- Around line 199-222: Rename the test fixture function assistant_tool to
assistant_function_call to match the function-call terminology: change the
function name for the helper that constructs an AssistantMessage containing
ContentBlock::FunctionCall and StopReason::FunctionCall, and update any
references to it in tests (for example the call in
extract_function_calls_collects_function_blocks_only that currently calls
assistant_tool()). Keep the function body and signature unchanged, only rename
the identifier to assistant_function_call so tests and fixtures remain
consistent.

In `@turn-orchestrator/src/states/provisioning.rs`:
- Around line 53-68: The two independent I/O calls in fetch_skills_bootstrap
(fetch_uri(iii, "iii://skills") and list_root_skill_uris(iii)) should be issued
concurrently to remove an extra RTT: use an async join (e.g., tokio::join! or
futures::join) to run fetch_uri and list_root_skill_uris in parallel, then,
after awaiting the join result, call fetch_uris_batched(iii, &root_uris). Ensure
you keep the existing match logic and only await fetch_uris_batched when the
joined list is non-empty; refer to fetch_skills_bootstrap, fetch_uri,
list_root_skill_uris, and fetch_uris_batched to locate the changes.

In `@turn-orchestrator/src/system_prompt.rs`:
- Line 8: BASE_BODY still uses the phrase "tool result" which conflicts with the
rest of the prompt that standardized on "agent_call" and "function"; update the
string constant BASE_BODY to replace "tool result" with "function result" (and
adjust surrounding wording if needed) so the sentence reads e.g. "If a function
result contains `blocked: true`, a policy refused it — explain which policy and
stop, do not retry." Ensure references to `agent_call` and `function` remain
intact.

In `@turn-orchestrator/tests/integration.rs`:
- Around line 4-9: The test state_keys_namespace_by_session omits asserting that
function_schemas_key(s) contains the session namespace; update that test to
include an assertion like
assert!(turn_orchestrator::function_schemas_key(s).contains(s)) so it matches
the other keys in state_keys_distinct_per_facet (turn_state_key, messages_key,
run_request_key) and ensures function_schemas_key follows the same
session-scoped invariant.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add a language identifier to the fenced code block.

This currently violates MD040 and may fail markdown linting.

Suggested patch

-```
+```text
 sandbox::create  →  sandbox::exec / sandbox::fs::*  →  sandbox::stop
                  \                                  /
                   → sandbox::list (any time, read-only)

</details>

<!-- suggestion_start -->

<details>
<summary>📝 Committable suggestion</summary>

> ‼️ **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.

```suggestion

🧰 Tools

🪛 markdownlint-cli2 (0.22.1)

[warning] 22-22: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@harness/docs/sandbox-skill.md` around lines 22 - 26, The fenced code block
containing the diagram (the triple-backtick block wrapping "sandbox::create  → 
sandbox::exec / sandbox::fs::*  →  sandbox::stop ...") is missing a language
identifier which triggers MD040; update that block header from ``` to ```text so
the block begins with ```text and keep the closing ``` unchanged to satisfy
markdown linting.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

The demo worker set is missing iii-sandbox.

harness/src/lib.rs now treats iii-sandbox as expected runtime infrastructure and publishes a sandbox skill, but make all still never builds or starts that worker. The demo will therefore advertise sandbox capabilities while every sandbox function call fails.

Suggested fix

 WORKERS := \
   turn-orchestrator provider-router \
   session-tree session-inbox \
   models-catalog hook-fanout policy-denylist \
   shell-bash shell-filesystem subagent \
   provider-anthropic provider-openai \
   auth-credentials llm-budget \
-  skills approval-gate
+  skills approval-gate iii-sandbox

📝 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

	WORKERS := \
	turn-orchestrator provider-router \
	session-tree session-inbox \
	models-catalog hook-fanout policy-denylist \
	shell-bash shell-filesystem subagent \
	provider-anthropic provider-openai \
	auth-credentials llm-budget \
	skills approval-gate
	WORKERS := \
	turn-orchestrator provider-router \
	session-tree session-inbox \
	models-catalog hook-fanout policy-denylist \
	shell-bash shell-filesystem subagent \
	provider-anthropic provider-openai \
	auth-credentials llm-budget \
	skills approval-gate iii-sandbox

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@harness/Makefile` around lines 34 - 41, The Makefile's WORKERS variable is
missing the iii-sandbox worker, so runs like `make all` never build or start it;
add `iii-sandbox` to the WORKERS list in the Makefile (the WORKERS definition
block) so the build/start targets include that worker and the runtime published
by harness/src/lib.rs matches the started demos.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

POLICY_DENIED_FUNCTIONS silently overrides any existing user environment value.

extra_env+=(POLICY_DENIED_FUNCTIONS="bridge::trigger") unconditionally replaces whatever the user may have set in their shell. If the policy-denylist worker supports a list and the user already has entries configured, this demo invocation will silently drop them and run with only bridge::trigger denied.

Consider prepending to the existing value if set, or at minimum documenting this override behavior in the comment:

🛠️ Suggested fix

-    policy-denylist)
-      extra_env+=(POLICY_DENIED_FUNCTIONS="bridge::trigger")
-      ;;
+    policy-denylist)
+      # Prepend bridge::trigger to any caller-supplied list so demo-required
+      # deny entries are always present without dropping user additions.
+      local existing="${POLICY_DENIED_FUNCTIONS:-}"
+      local denied="bridge::trigger${existing:+,$existing}"
+      extra_env+=(POLICY_DENIED_FUNCTIONS="$denied")
+      ;;

📝 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

	local -a extra_env=()
	case "$w" in
	policy-denylist)
	extra_env+=(POLICY_DENIED_FUNCTIONS="bridge::trigger")
	;;
	esac
	env III_URL="$DEMO_ENGINE_WS" "${extra_env[@]}" nohup "$bin" "${run_args[@]}" >>"$logfile" 2>&1 &
	local -a extra_env=()
	case "$w" in
	policy-denylist)
	# Prepend bridge::trigger to any caller-supplied list so demo-required
	# deny entries are always present without dropping user additions.
	local existing="${POLICY_DENIED_FUNCTIONS:-}"
	local denied="bridge::trigger${existing:+,$existing}"
	extra_env+=(POLICY_DENIED_FUNCTIONS="$denied")
	;;
	esac
	env III_URL="$DEMO_ENGINE_WS" "${extra_env[@]}" nohup "$bin" "${run_args[@]}" >>"$logfile" 2>&1 &

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@harness/scripts/demo.sh` around lines 110 - 117, The current demo script
unconditionally sets POLICY_DENIED_FUNCTIONS via
extra_env+=(POLICY_DENIED_FUNCTIONS="bridge::trigger") which overwrites any
user-provided value; update the policy-denylist branch in the case for symbol
policy-denylist to merge with an existing POLICY_DENIED_FUNCTIONS (e.g., prepend
or append "bridge::trigger" to the current $POLICY_DENIED_FUNCTIONS when it is
non-empty) before adding the entry to extra_env so user values are preserved, or
if you decide to keep the override instead, add a clear comment explaining that
this invocation intentionally replaces any user setting; reference the extra_env
array and the POLICY_DENIED_FUNCTIONS variable and the policy-denylist case to
locate the change.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Stale-browser cleanup only covers ui::session::event::*.

This eviction path never runs for ui::sessions::changed::*, ui::approval::*, ui::cost::tick::*, or ui::workers::changed::*, so an all-sessions subscriber that disappears without calling ui::unsubscribe will still leak and keep generating function_not_found failures on every poll. Please move the eviction logic into a shared push helper and reuse it for all UI fanout paths.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@harness/src/fanout.rs` around lines 292 - 305, Extract the
eviction+error-handling block into a shared helper (e.g., try_push_with_eviction
or push_to_browser) that takes the browser id, the send result/error, and
references to fanout_for_gc and any logger; inside the helper call
is_function_not_found(&e) for errors, take fanout_for_gc.write().await and call
state.evict_browser(&browser_id) when needed, emit the same tracing::debug when
evicted and tracing::trace for other errors, and return a boolean/Unit
indicating success; then replace the duplicated eviction logic in the
ui::session::event::* path and the ui::sessions::changed::*, ui::approval::*,
ui::cost::tick::*, and ui::workers::changed::* fanout sites to call this helper
instead of inlining the eviction logic so all UI fanout paths share the same
behavior.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify deprecated keyword usage is eliminated in this file.
rg -n 'word-break:\s*break-word' harness/web/src/styles.css
# Expected: no matches.

Repository: iii-hq/workers

Length of output: 150

---

🏁 Script executed:

# Check context around line 2056 to see what CSS rule it applies to
sed -n '2045,2065p' harness/web/src/styles.css | cat -n

Repository: iii-hq/workers

Length of output: 681

---

🏁 Script executed:

# Also verify the exact CSS at each location with surrounding context
echo "=== Line 849 context ===" && sed -n '840,858p' harness/web/src/styles.css | cat -n
echo "" && echo "=== Line 1013 context ===" && sed -n '1004,1022p' harness/web/src/styles.css | cat -n

Repository: iii-hq/workers

Length of output: 1143

---

🌐 Web query:

word-break break-word deprecated CSS modern replacement overflow-wrap

💡 Result:

Yes, word-break: break-word is deprecated in CSS.[1][2] The MDN documentation explicitly marks it as deprecated and states it has the same effect as overflow-wrap: anywhere combined with word-break: normal, regardless of the overflow-wrap value.[1][3] This is confirmed in the CSS Text specification context and browser implementations.[4][5]

Citations:

• 1: https://dev.mozilla.org/en-US/docs/Web/CSS/Reference/Properties/word-break
• 2: https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Properties/word-break
• 3: https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Properties/overflow-wrap
• 4: https://deku.posstree.com/en/web/css-overflow-wrap/
• 5: word-break: break-word; vs overflow-wrap: anywhere; mdn/content#15676

---

Replace deprecated word-break: break-word in three locations to clear Stylelint errors

The deprecated keyword appears at lines 849, 1013, and 2056. Switch to overflow-wrap: anywhere combined with word-break: normal for equivalent behavior with current CSS specifications.

Affected selectors and suggested fix

Line 849 (.md code):

  .md code {
    font-family: var(--mono);
    font-size: 0.88em;
    background: var(--paper-3);
    padding: 1px 5px;
    color: var(--ink);
-   word-break: break-word;
+   overflow-wrap: anywhere;
+   word-break: normal;
  }

Line 1013 (.block-body):

  .block-body {
    margin: 0;
    padding: var(--space-2) var(--space-3) var(--space-3);
    border-top: 1px solid var(--rule);
    font-family: var(--mono);
    font-size: 11.5px;
    line-height: 1.5;
    color: var(--ink-2);
    white-space: pre-wrap;
-   word-break: break-word;
+   overflow-wrap: anywhere;
+   word-break: normal;
    overflow-x: auto;
    max-height: 360px;
    overflow-y: auto;
  }

Line 2056 (.palette-code):

    font-family: var(--mono);
    font-size: 12px;
    color: var(--ink);
    overflow: auto;
    max-height: 240px;
    white-space: pre-wrap;
-   word-break: break-word;
+   overflow-wrap: anywhere;
+   word-break: normal;
  }

📝 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

	word-break: break-word;
	overflow-wrap: anywhere;
	word-break: normal;

🧰 Tools

🪛 Stylelint (17.10.0)

[error] 849-849: Deprecated keyword "break-word" for property "word-break" (declaration-property-value-keyword-no-deprecated)

(declaration-property-value-keyword-no-deprecated)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@harness/web/src/styles.css` at line 849, Replace the deprecated CSS rule
"word-break: break-word" with the recommended combination: set "word-break:
normal" and add "overflow-wrap: anywhere" for the affected selectors; update the
declarations in the rules for ".md code", ".block-body", and ".palette-code" so
each uses "word-break: normal" plus "overflow-wrap: anywhere" to preserve
behavior and satisfy Stylelint.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Confirm the function IDs actually registered by shell-bash / shell-filesystem workers
# to propose accurate replacement examples.
rg -n "function_id\|register_function\|RegisterFunctionInput" --type rust -A 3 -B 1

Repository: iii-hq/workers

Length of output: 40

---

🏁 Script executed:

# First, find the YAML file and confirm its contents
fd "iii.worker.yaml" -x cat {}

Repository: iii-hq/workers

Length of output: 8477

---

🏁 Script executed:

# Search for patterns that define or register functions more broadly
# Look for function definitions, not just Rust-specific patterns
rg "function" --type yaml -l | head -20

Repository: iii-hq/workers

Length of output: 144

---

🏁 Script executed:

# Search for actual function ID examples in the codebase
rg "::.*::" --type yaml -A 2 -B 2

Repository: iii-hq/workers

Length of output: 4459

---

🏁 Script executed:

# Look for worker definitions and how function_id is used
rg "denied_functions|denied_tools" -A 5 -B 5

Repository: iii-hq/workers

Length of output: 14782

---

Default denied_functions entries don't match the function-ID naming convention.

After the migration to function_id-based matching, the three default entries ("bash:rm -rf", sudo, curl-pipe-bash) are legacy tool-name-style strings. The actual function IDs in the system use a namespace::function pattern (e.g., shell::bash::*, shell::filesystem::*). These defaults will not match any registered function IDs and provide no protection. Operators who deploy with defaults will get no actual enforcement.

Consider either replacing the defaults with realistic example function IDs that exist in the worker set, or making the default list empty with an explicit comment directing operators to populate it.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@policy-denylist/iii.worker.yaml` around lines 10 - 13, The default
denied_functions entries ("bash:rm -rf", sudo, curl-pipe-bash) use legacy
tool-name strings and won't match function IDs; update the denied_functions list
to use real function_id patterns (for example replace with entries like
shell::bash::*, shell::filesystem::*, network::curl::*), or make
denied_functions an empty list and add a clear comment instructing operators to
populate it with appropriate function_id values; ensure you edit the
denied_functions block so it contains function_id-style patterns rather than
legacy names.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

DEFAULT_DENIED_FUNCTIONS entries look like shell command patterns, not function ids — they will never match under the new semantics.

handle_event now matches function_call.function_id against the denylist with exact-string equality (check_denylist). Function ids in this codebase look like shell::bash::exec, but the defaults "bash:rm -rf", "sudo", "curl-pipe-bash" are shell-token patterns from the old tool-call world. Under the new contract these defaults are dead — any deployment relying on the built-in defaults silently has no policy.

Either replace them with real iii function ids the project wants to deny by default (e.g., shell::bash::exec or whatever the equivalent is), or move shell-substring matching into a separate predicate so the defaults remain meaningful.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@policy-denylist/src/lib.rs` around lines 14 - 15, DEFAULT_DENIED_FUNCTIONS
currently contains legacy shell-token patterns that will never match the new
exact equality check in handle_event which uses check_denylist; update the
defaults to real function ids (e.g., replace "bash:rm -rf", "sudo",
"curl-pipe-bash" with the canonical function ids your project intends to block
such as "shell::bash::exec", "system::sudo::run", "network::curl::exec" or
whatever the real ids are), or alternatively add a second predicate (e.g.,
check_denylist_substring or check_denylist_extended) and call it from
handle_event alongside check_denylist to perform substring/shell-token matching
for backward compatibility while leaving check_denylist as exact-match.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Remove the leaked local filesystem path.

Line 171 references an absolute path under a developer's home directory (/Users/ytallolayon/.claude/plans/...). This leaks the author's username into the repository, references a file that doesn't exist for any other reader, and provides no value as inline documentation. Drop the comment or rephrase it without the local path.

🧹 Suggested fix

-    // ── Adversarial unit tests added per plan
-    // /Users/ytallolayon/.claude/plans/let-s-implement-more-tests-refactored-flask.md
-
+    // ── Adversarial unit tests for parse_denied_functions edge cases.
+

📝 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

	// ── Adversarial unit tests added per plan
	// /Users/ytallolayon/.claude/plans/let-s-implement-more-tests-refactored-flask.md
	// ── Adversarial unit tests for parse_denied_functions edge cases.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@policy-denylist/src/main.rs` around lines 170 - 172, Remove the leaked
absolute local path from the inline comment in policy-denylist/src/main.rs (the
comment block mentioning "Adversarial unit tests" that contains
"/Users/ytallolayon/.claude/plans/..."); either delete that comment entirely or
replace it with a neutral, repo-appropriate note such as "Adversarial unit tests
added per internal plan" or a short explanation of the tests' purpose, ensuring
no personal filesystem paths or usernames remain.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Duplicate FunctionExecutionStart emitted on retry of handle_execute.

The loop emits FunctionExecutionStart at the top before checking the prefilled/recorded early-continue paths. When FunctionExecute is retried after a crash (e.g., orchestrator restart while halfway through the loop), already-completed calls re-emit a Start followed by their cached End — so consumers/observers see two Start events for one logical execution. Most UIs key by function_call_id and tolerate this, but event-sourced subscribers or metrics pipelines counting starts will overcount.

Hoisting the start emit below the cache checks makes the lifecycle event-per-call exactly-once on retry.

🐛 Suggested fix

     for (fc, prefilled) in prepared {
-        events::emit(
-            iii,
-            &record.session_id,
-            &AgentEvent::FunctionExecutionStart {
-                function_call_id: fc.id.clone(),
-                function_id: fc.function_id.clone(),
-                args: fc.arguments.clone(),
-            },
-        )
-        .await;
         if let Some(blocked) = prefilled {
             persistence::upsert_executed_call(&mut results, (fc.clone(), blocked.clone(), true));
             persistence::save_executed_calls(iii, &record.session_id, &results).await;
             let evt = build_function_execution_event(&fc, &blocked, true);
             events::emit(iii, &record.session_id, &evt).await;
             continue;
         }
         if let Some((_, recorded, recorded_is_error)) =
             persistence::find_executed_call(&results, &fc.id).cloned()
         {
             let evt = build_function_execution_event(&fc, &recorded, recorded_is_error);
             events::emit(iii, &record.session_id, &evt).await;
             continue;
         }
+        events::emit(
+            iii,
+            &record.session_id,
+            &AgentEvent::FunctionExecutionStart {
+                function_call_id: fc.id.clone(),
+                function_id: fc.function_id.clone(),
+                args: fc.arguments.clone(),
+            },
+        )
+        .await;
         let mut augmented = match fc.arguments.clone() {

📝 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

	for (fc, prefilled) in prepared {
	events::emit(
	iii,
	&record.session_id,
	&AgentEvent::FunctionExecutionStart {
	function_call_id: fc.id.clone(),
	function_id: fc.function_id.clone(),
	args: fc.arguments.clone(),
	},
	)
	.await;
	if let Some(blocked) = prefilled {
	persistence::upsert_executed_call(&mut results, (fc.clone(), blocked.clone(), true));
	persistence::save_executed_calls(iii, &record.session_id, &results).await;
	let evt = build_function_execution_event(&fc, &blocked, true);
	events::emit(iii, &record.session_id, &evt).await;
	continue;
	}
	if let Some((_, recorded, recorded_is_error)) =
	persistence::find_executed_call(&results, &fc.id).cloned()
	{
	let evt = build_function_execution_event(&fc, &recorded, recorded_is_error);
	events::emit(iii, &record.session_id, &evt).await;
	continue;
	}
	let mut augmented = match fc.arguments.clone() {
	Value::Object(o) => Value::Object(o),
	other => json!({ "arguments": other }),
	};
	if let Some(obj) = augmented.as_object_mut() {
	obj.insert("session_id".into(), json!(record.session_id));
	obj.insert("function_call_id".into(), json!(fc.id));
	obj.insert("function_id".into(), json!(fc.function_id));
	obj.insert(
	"function_call".into(),
	json!({
	"id": fc.id.clone(),
	"function_id": fc.function_id.clone(),
	"arguments": fc.arguments.clone(),
	}),
	);
	}
	let result = crate::agent_call::dispatch(
	iii,
	&record.session_id,
	&json!(fc.function_id.clone()),
	augmented,
	)
	.await;
	let is_error = result
	.details
	.get("error")
	.and_then(Value::as_str)
	.is_some();
	persistence::upsert_executed_call(&mut results, (fc.clone(), result.clone(), is_error));
	persistence::save_executed_calls(iii, &record.session_id, &results).await;
	let evt = build_function_execution_event(&fc, &result, is_error);
	events::emit(iii, &record.session_id, &evt).await;
	}
	for (fc, prefilled) in prepared {
	if let Some(blocked) = prefilled {
	persistence::upsert_executed_call(&mut results, (fc.clone(), blocked.clone(), true));
	persistence::save_executed_calls(iii, &record.session_id, &results).await;
	let evt = build_function_execution_event(&fc, &blocked, true);
	events::emit(iii, &record.session_id, &evt).await;
	continue;
	}
	if let Some((_, recorded, recorded_is_error)) =
	persistence::find_executed_call(&results, &fc.id).cloned()
	{
	let evt = build_function_execution_event(&fc, &recorded, recorded_is_error);
	events::emit(iii, &record.session_id, &evt).await;
	continue;
	}
	events::emit(
	iii,
	&record.session_id,
	&AgentEvent::FunctionExecutionStart {
	function_call_id: fc.id.clone(),
	function_id: fc.function_id.clone(),
	args: fc.arguments.clone(),
	},
	)
	.await;
	let mut augmented = match fc.arguments.clone() {
	Value::Object(o) => Value::Object(o),
	other => json!({ "arguments": other }),
	};
	if let Some(obj) = augmented.as_object_mut() {
	obj.insert("session_id".into(), json!(record.session_id));
	obj.insert("function_call_id".into(), json!(fc.id));
	obj.insert("function_id".into(), json!(fc.function_id));
	obj.insert(
	"function_call".into(),
	json!({
	"id": fc.id.clone(),
	"function_id": fc.function_id.clone(),
	"arguments": fc.arguments.clone(),
	}),
	);
	}
	let result = crate::agent_call::dispatch(
	iii,
	&record.session_id,
	&json!(fc.function_id.clone()),
	augmented,
	)
	.await;
	let is_error = result
	.details
	.get("error")
	.and_then(Value::as_str)
	.is_some();
	persistence::upsert_executed_call(&mut results, (fc.clone(), result.clone(), is_error));
	persistence::save_executed_calls(iii, &record.session_id, &results).await;
	let evt = build_function_execution_event(&fc, &result, is_error);
	events::emit(iii, &record.session_id, &evt).await;
	}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@turn-orchestrator/src/states/functions.rs` around lines 110 - 170, The loop
currently emits FunctionExecutionStart (events::emit with
AgentEvent::FunctionExecutionStart) before checking prefilled or cached executed
calls, causing duplicate Start events on retries; move the events::emit call
that creates the FunctionExecutionStart down so it occurs only after the
prefilled check and the persistence::find_executed_call check (i.e., after the
two early-continue paths) and before dispatching crate::agent_call::dispatch,
preserving the existing AgentEvent payload construction and using the same fc,
record.session_id and iii identifiers.

[coderabbitai]

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)

harness/scripts/demo.sh (2)

39-47: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Start iii-sandbox in the demo worker roster.

The harness now expects and teaches sandbox functionality, but this launcher never builds or spawns iii-sandbox. In the local demo, that turns every sandbox-oriented flow into a runtime failure.

Suggested fix

 WORKERS=(
   turn-orchestrator provider-router
   session-tree session-inbox
   models-catalog hook-fanout policy-denylist
   shell subagent
   provider-anthropic provider-openai
   auth-credentials llm-budget
   skills approval-gate
+  iii-sandbox
 )

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@harness/scripts/demo.sh` around lines 39 - 47, The WORKERS array in demo.sh
is missing the sandbox worker; add the token "iii-sandbox" to the WORKERS=(...)
list so the launcher builds and spawns the sandbox process (ensure the string is
quoted/space-separated consistently with other entries and that any scripts
iterating over WORKERS will automatically include it when launching/building
workers).

---

126-149: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Use DEMO_ENGINE_WS for the CLI probes too.

The worker processes inherit III_URL="$DEMO_ENGINE_WS", but cmd_engine, cmd_start, and cmd_verify still probe iii --use-default-config without overriding III_URL. Any non-default III_DEMO_ENGINE_URL makes the health checks talk to a different engine than the one the workers register with.

Suggested fix

-    if iii --use-default-config trigger --function-id engine::queue::list_topics --timeout-ms 1000 >/dev/null 2>&1; then
+    if III_URL="$DEMO_ENGINE_WS" iii --use-default-config trigger --function-id engine::queue::list_topics --timeout-ms 1000 >/dev/null 2>&1; then
       echo "==> engine ready after ${i}s (pid $(cat "$pidfile"))"
       return 0
     fi
   done
@@
-  if ! iii --use-default-config trigger --function-id engine::queue::list_topics --timeout-ms 1000 >/dev/null 2>&1; then
+  if ! III_URL="$DEMO_ENGINE_WS" iii --use-default-config trigger --function-id engine::queue::list_topics --timeout-ms 1000 >/dev/null 2>&1; then
     echo "    engine not reachable — run \`./scripts/demo.sh engine\` first (or use \`all\`)"; exit 1
   fi
@@
-  iii --use-default-config trigger --function-id harness::status
+  III_URL="$DEMO_ENGINE_WS" iii --use-default-config trigger --function-id harness::status
@@
-  iii --use-default-config trigger --function-id models::list || true
+  III_URL="$DEMO_ENGINE_WS" iii --use-default-config trigger --function-id models::list || true

Also applies to: 151-177

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@harness/scripts/demo.sh` around lines 126 - 149, Health-check invocations
currently call the iii CLI without forcing the engine URL, causing probes to
target the wrong engine when a non-default DEMO_ENGINE_WS is used; update the
health-check calls in cmd_engine (and the similar probes in cmd_start and
cmd_verify) to run with III_URL="$DEMO_ENGINE_WS" (e.g., prepend
III_URL="$DEMO_ENGINE_WS" to the iii --use-default-config trigger/health
commands) so the CLI probes the same engine instance the workers register with;
keep existing pid handling (engine.pid) and logging behavior unchanged.

harness/web/src/components/Composer.tsx (1)

85-103: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Directory detection broken in entriesToMenuItems — uses deprecated kind field not sent by shell::fs::ls.

entriesToMenuItems checks only e.kind === "dir" for both sorting and the isDir meta flag. The Rust backend (shell/src/fs/wire.rs) sends is_dir: bool exclusively and never populates kind. As a result, every directory from shell::fs::ls is treated as a file: sorting fails (directories not grouped first), and acceptItem inserts paths with trailing spaces instead of descending into them.

FilesystemPanel.tsx (lines 30–35) already implements the correct precedence, checking is_dir === true first before falling back to kind and mode. Align entriesToMenuItems to the same pattern:

Suggested fix

 function entriesToMenuItems(dir: string, entries: FsEntry[]): MenuItem[] {
+  const isDirEntry = (e: FsEntry): boolean =>
+    e.is_dir === true ||
+    e.kind === "dir" ||
+    (typeof e.mode === "string" && e.mode.startsWith("d"));
   const sorted = [...entries].sort((a, b) => {
-    const aDir = a.kind === "dir" ? 1 : 0;
-    const bDir = b.kind === "dir" ? 1 : 0;
+    const aDir = isDirEntry(a) ? 1 : 0;
+    const bDir = isDirEntry(b) ? 1 : 0;
     if (aDir !== bDir) return bDir - aDir;
     return a.name.localeCompare(b.name);
   });
   return sorted.map((e) => {
     const abs = joinPath(dir, e.name);
-    const isDir = e.kind === "dir";
+    const isDir = isDirEntry(e);
     return {
       kind: "file" as const,
       id: abs,
       label: e.name + (isDir ? "/" : ""),
       description: abs,
       meta: { isDir, dir },
     };
   });
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@harness/web/src/components/Composer.tsx` around lines 85 - 103,
entriesToMenuItems currently treats directories by checking only e.kind ===
"dir", which is wrong because FsEntry from the backend provides is_dir: boolean;
update entriesToMenuItems to detect directories the same way as FilesystemPanel:
compute isDir as (e.is_dir === true) || e.kind === "dir" || (typeof e.mode ===
"number" && (e.mode & 0o40000) !== 0), then use that isDir for sorting
(directories first), for label suffix (append "/"), and for meta.isDir so
consumers like acceptItem behave correctly; keep other fields (id, label,
description, meta.dir) unchanged.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@harness/ARCHITECTURE.md`:
- Around line 78-92: The README/architecture text is inconsistent with the
authoritative EXPECTED_WORKERS array (lib.rs) which defines 16 workers; update
all numeric references ("14 specialized workers", "The 15 expected workers",
"harness + 14 workers") to "16" and expand the role table to include the three
missing worker entries: skills, approval-gate, and iii-sandbox (add their roles
in the same table format alongside existing rows such as turn-orchestrator,
provider-router, session-tree, session-inbox, models-catalog, auth-credentials,
policy-denylist, llm-budget, hook-fanout, shell, subagent, provider-anthropic,
provider-openai) so the document matches EXPECTED_WORKERS.

In `@harness/Makefile`:
- Around line 109-132: The _spawn-one target starts workers but omits setting
the POLICY_DENIED_FUNCTIONS override for the policy-denylist worker, leaving the
bridge::trigger bypass open; update the conditional that handles the worker
named "policy-denylist" (within the _spawn-one recipe that checks $$w and uses
nohup to launch $$bin) to export/set POLICY_DENIED_FUNCTIONS=bridge::trigger in
the env when launching that worker (similar to how harness/shell get III_URL),
so the nohup invocation for "policy-denylist" includes that environment variable
and writes the pidfile/log like the other branches.

In `@harness/shell-config.yaml`:
- Around line 39-49: The YAML exposes the real host filesystem because
fs.host_root is null, fs.allow_unjailed is true, and sandbox.enabled is false;
change the defaults to use a dedicated demo root and safe defaults instead of
unjailed host access by setting fs.host_root to a confined demo directory, set
fs.allow_unjailed to false, and enable sandbox (sandbox.enabled: true) unless an
explicit opt-in flag is provided; update denylist_paths as a secondary safeguard
and add a clear config comment or an explicit opt-in key (e.g.,
allow_unjailed_opt_in) so unjailed host access cannot be enabled by accident.

In `@harness/web/src/App.tsx`:
- Around line 35-44: Update the example denylist in the comment to include
"bridge::trigger" (alongside the existing shell::fs entries) and add a brief
note that ARCHITECTURE.md requires blocking bridge::trigger because the model
can call agent_call(function="bridge::trigger", payload={...}) to recursively
dispatch functions and bypass name-matched rules; ensure the env var examples
(POLICY_DENIED_FUNCTIONS / legacy POLICY_DENIED_TOOLS) and the example list
match the canonical denylist from harness/scripts/demo.sh and mention
policy-denylist as the enforcement point.

---

Outside diff comments:
In `@harness/scripts/demo.sh`:
- Around line 39-47: The WORKERS array in demo.sh is missing the sandbox worker;
add the token "iii-sandbox" to the WORKERS=(...) list so the launcher builds and
spawns the sandbox process (ensure the string is quoted/space-separated
consistently with other entries and that any scripts iterating over WORKERS will
automatically include it when launching/building workers).
- Around line 126-149: Health-check invocations currently call the iii CLI
without forcing the engine URL, causing probes to target the wrong engine when a
non-default DEMO_ENGINE_WS is used; update the health-check calls in cmd_engine
(and the similar probes in cmd_start and cmd_verify) to run with
III_URL="$DEMO_ENGINE_WS" (e.g., prepend III_URL="$DEMO_ENGINE_WS" to the iii
--use-default-config trigger/health commands) so the CLI probes the same engine
instance the workers register with; keep existing pid handling (engine.pid) and
logging behavior unchanged.

In `@harness/web/src/components/Composer.tsx`:
- Around line 85-103: entriesToMenuItems currently treats directories by
checking only e.kind === "dir", which is wrong because FsEntry from the backend
provides is_dir: boolean; update entriesToMenuItems to detect directories the
same way as FilesystemPanel: compute isDir as (e.is_dir === true) || e.kind ===
"dir" || (typeof e.mode === "number" && (e.mode & 0o40000) !== 0), then use that
isDir for sorting (directories first), for label suffix (append "/"), and for
meta.isDir so consumers like acceptItem behave correctly; keep other fields (id,
label, description, meta.dir) unchanged.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b69a86f1-e508-490c-9e2d-dcc14dfd946c

📥 Commits

Reviewing files that changed from the base of the PR and between b7c7e4a and 1d515c2.

⛔ Files ignored due to path filters (3)

• harness/Cargo.lock is excluded by !**/*.lock
• shell-bash/Cargo.lock is excluded by !**/*.lock
• shell-filesystem/Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (56)

• harness/ARCHITECTURE.md
• harness/Makefile
• harness/iii.worker.yaml
• harness/scripts/demo.sh
• harness/shell-config.yaml
• harness/src/fs.rs
• harness/src/lib.rs
• harness/web/src/App.tsx
• harness/web/src/components/Composer.tsx
• harness/web/src/components/FilesystemPanel.tsx
• harness/web/src/palette.test.ts
• harness/web/src/palette.ts
• harness/web/src/reducer.test.ts
• harness/web/src/types.ts
• harness/web/src/useStatus.test.ts
• shell-bash/Cargo.toml
• shell-bash/README.md
• shell-bash/build.rs
• shell-bash/iii.worker.yaml
• shell-bash/src/config.rs
• shell-bash/src/detect_clis.rs
• shell-bash/src/exec.rs
• shell-bash/src/lib.rs
• shell-bash/src/main.rs
• shell-bash/src/manifest.rs
• shell-bash/src/register.rs
• shell-bash/src/which.rs
• shell-bash/tests/integration.rs
• shell-bash/tests/manifest.rs
• shell-filesystem/Cargo.toml
• shell-filesystem/README.md
• shell-filesystem/build.rs
• shell-filesystem/iii.worker.yaml
• shell-filesystem/src/config.rs
• shell-filesystem/src/lib.rs
• shell-filesystem/src/main.rs
• shell-filesystem/src/manifest.rs
• shell-filesystem/src/ops/chmod.rs
• shell-filesystem/src/ops/edit.rs
• shell-filesystem/src/ops/grep.rs
• shell-filesystem/src/ops/ls.rs
• shell-filesystem/src/ops/mkdir.rs
• shell-filesystem/src/ops/mod.rs
• shell-filesystem/src/ops/mv.rs
• shell-filesystem/src/ops/read.rs
• shell-filesystem/src/ops/rm.rs
• shell-filesystem/src/ops/sed.rs
• shell-filesystem/src/ops/stat.rs
• shell-filesystem/src/ops/write.rs
• shell-filesystem/src/register.rs
• shell-filesystem/tests/integration.rs
• shell-filesystem/tests/manifest.rs
• turn-orchestrator/src/agent_call.rs
• turn-orchestrator/src/run_start.rs
• turn-orchestrator/src/states/functions.rs
• turn-orchestrator/src/system_prompt.rs

💤 Files with no reviewable changes (37)

• shell-bash/build.rs
• shell-bash/tests/manifest.rs
• shell-bash/src/lib.rs
• shell-bash/Cargo.toml
• shell-filesystem/build.rs
• shell-filesystem/tests/integration.rs
• shell-bash/src/detect_clis.rs
• shell-filesystem/Cargo.toml
• shell-bash/README.md
• shell-filesystem/iii.worker.yaml
• shell-filesystem/src/ops/ls.rs
• shell-filesystem/src/ops/stat.rs
• shell-bash/src/register.rs
• shell-filesystem/tests/manifest.rs
• shell-bash/src/main.rs
• shell-filesystem/src/ops/mv.rs
• shell-bash/tests/integration.rs
• shell-filesystem/src/ops/mkdir.rs
• shell-bash/iii.worker.yaml
• shell-filesystem/src/lib.rs
• shell-filesystem/src/manifest.rs
• shell-bash/src/exec.rs
• shell-filesystem/src/ops/edit.rs
• shell-filesystem/src/ops/mod.rs
• shell-filesystem/src/ops/chmod.rs
• shell-bash/src/which.rs
• shell-filesystem/src/register.rs
• shell-bash/src/manifest.rs
• shell-bash/src/config.rs
• shell-filesystem/src/ops/write.rs
• shell-filesystem/src/ops/grep.rs
• shell-filesystem/src/ops/rm.rs
• shell-filesystem/src/main.rs
• shell-filesystem/src/ops/sed.rs
• shell-filesystem/README.md
• shell-filesystem/src/ops/read.rs
• shell-filesystem/src/config.rs

✅ Files skipped from review due to trivial changes (2)

• turn-orchestrator/src/run_start.rs
• harness/src/fs.rs

🚧 Files skipped from review as they are similar to previous changes (5)

• harness/web/src/reducer.test.ts
• turn-orchestrator/src/agent_call.rs
• harness/web/src/useStatus.test.ts
• turn-orchestrator/src/system_prompt.rs
• turn-orchestrator/src/states/functions.rs

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
fd -t f 'lib.rs' | xargs rg -nP -C2 'EXPECTED_WORKERS'
fd -t f 'iii.worker.yaml' | xargs rg -nP -C2 'depends|workers'

Repository: iii-hq/workers

Length of output: 3124

---

🏁 Script executed:

# Get the complete EXPECTED_WORKERS array definition
sed -n '80,105p' harness/src/lib.rs

Repository: iii-hq/workers

Length of output: 753

---

🏁 Script executed:

# Check ARCHITECTURE.md for the specific lines mentioned
sed -n '1,5p' harness/ARCHITECTURE.md
sed -n '78,92p' harness/ARCHITECTURE.md  
sed -n '100,105p' harness/ARCHITECTURE.md

Repository: iii-hq/workers

Length of output: 1659

---

Update worker count to 16 and complete the role table.

The authoritative EXPECTED_WORKERS array (lib.rs:80–96) contains 16 workers, not 14 or 15. Line 3 states "14 specialized workers," the heading at Line 78 says "The 15 expected workers," Line 102 says "harness + 14 workers," and the role table at Lines 82–92 explicitly lists only 13. Update all three references to 16, and add the missing workers to the table: skills, approval-gate, and iii-sandbox.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@harness/ARCHITECTURE.md` around lines 78 - 92, The README/architecture text
is inconsistent with the authoritative EXPECTED_WORKERS array (lib.rs) which
defines 16 workers; update all numeric references ("14 specialized workers",
"The 15 expected workers", "harness + 14 workers") to "16" and expand the role
table to include the three missing worker entries: skills, approval-gate, and
iii-sandbox (add their roles in the same table format alongside existing rows
such as turn-orchestrator, provider-router, session-tree, session-inbox,
models-catalog, auth-credentials, policy-denylist, llm-budget, hook-fanout,
shell, subagent, provider-anthropic, provider-openai) so the document matches
EXPECTED_WORKERS.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

policy-denylist is started without the bridge deny override.

Unlike harness/scripts/demo.sh, this launcher never sets POLICY_DENIED_FUNCTIONS=bridge::trigger when it starts policy-denylist. That leaves the recursive bridge::trigger bypass path open in the Make-based demo.

Suggested fix

 _spawn-one:
 	`@w`="$(W)"; \
 	bin_name="$$(awk '/^bin:/{sub(/^bin:[ \t]*/, ""); sub(/[ \t]+$$/, ""); print; exit}' "$(WORKERS_REPO)/$$w/iii.worker.yaml" 2>/dev/null || echo "$$w")"; \
 	bin="$(WORKERS_REPO)/$$w/target/release/$$bin_name"; \
 	pidfile="$(PIDS_DIR)/$$w.pid"; \
 	logfile="$(LOGS_DIR)/$$w.log"; \
+	extra_env=(); \
+	if [[ "$$w" == "policy-denylist" ]]; then \
+	  extra_env+=(POLICY_DENIED_FUNCTIONS="bridge::trigger"); \
+	fi; \
 	if [[ -f "$$pidfile" ]] && kill -0 "$$(cat $$pidfile)" 2>/dev/null; then \
 	  echo "    [skip]  $$w (already running, pid $$(cat $$pidfile))"; exit 0; \
 	fi; \
@@
-	  env III_URL="$(DEMO_ENGINE_WS)" nohup "$$bin" --config "$(WORKERS_REPO)/harness/config.yaml" --url "$(DEMO_ENGINE_WS)" >> "$$logfile" 2>&1 & \
+	  env III_URL="$(DEMO_ENGINE_WS)" "$${extra_env[@]}" nohup "$$bin" --config "$(WORKERS_REPO)/harness/config.yaml" --url "$(DEMO_ENGINE_WS)" >> "$$logfile" 2>&1 & \
 	  echo $$! > "$$pidfile"; \
 	elif [[ "$$w" == "shell" ]]; then \
-	  env III_URL="$(DEMO_ENGINE_WS)" nohup "$$bin" --config "$(WORKERS_REPO)/harness/shell-config.yaml" >> "$$logfile" 2>&1 & \
+	  env III_URL="$(DEMO_ENGINE_WS)" "$${extra_env[@]}" nohup "$$bin" --config "$(WORKERS_REPO)/harness/shell-config.yaml" >> "$$logfile" 2>&1 & \
 	  echo $$! > "$$pidfile"; \
 	else \
-	  env III_URL="$(DEMO_ENGINE_WS)" nohup "$$bin" >> "$$logfile" 2>&1 & \
+	  env III_URL="$(DEMO_ENGINE_WS)" "$${extra_env[@]}" nohup "$$bin" >> "$$logfile" 2>&1 & \
 	  echo $$! > "$$pidfile"; \
 	fi; \
 	echo "    [start] $$w pid=$$(cat $$pidfile) log=$$logfile"

🧰 Tools

🪛 checkmake (0.3.2)

[warning] 109-109: Target body for "_spawn-one" exceeds allowed length of 5 lines (23).

(maxbodylength)

---

[warning] 109-109: Required target "clean" is missing from the Makefile.

(minphony)

---

[warning] 109-109: Required target "test" is missing from the Makefile.

(minphony)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@harness/Makefile` around lines 109 - 132, The _spawn-one target starts
workers but omits setting the POLICY_DENIED_FUNCTIONS override for the
policy-denylist worker, leaving the bridge::trigger bypass open; update the
conditional that handles the worker named "policy-denylist" (within the
_spawn-one recipe that checks $$w and uses nohup to launch $$bin) to export/set
POLICY_DENIED_FUNCTIONS=bridge::trigger in the env when launching that worker
(similar to how harness/shell get III_URL), so the nohup invocation for
"policy-denylist" includes that environment variable and writes the pidfile/log
like the other branches.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

This demo config exposes the host filesystem to shell::fs::*.

With host_root: null, allow_unjailed: true, and sandbox.enabled: false, the agent can read, write, mkdir, and rm essentially anywhere on the developer machine except the two denylisted paths. The shell command allowlist does not constrain those filesystem functions. Please default this to a dedicated demo root, or require an explicit opt-in before enabling unjailed host access.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@harness/shell-config.yaml` around lines 39 - 49, The YAML exposes the real
host filesystem because fs.host_root is null, fs.allow_unjailed is true, and
sandbox.enabled is false; change the defaults to use a dedicated demo root and
safe defaults instead of unjailed host access by setting fs.host_root to a
confined demo directory, set fs.allow_unjailed to false, and enable sandbox
(sandbox.enabled: true) unless an explicit opt-in flag is provided; update
denylist_paths as a secondary safeguard and add a clear config comment or an
explicit opt-in key (e.g., allow_unjailed_opt_in) so unjailed host access cannot
be enabled by accident.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Guidance comment is missing bridge::trigger from the example denylist — contradicts the security note in ARCHITECTURE.md.

harness/ARCHITECTURE.md (Line 145) explicitly states the denylist must include bridge::trigger, otherwise the model can call agent_call(function="bridge::trigger", payload={...}) to recursively dispatch any function and bypass the name-matched rules. The example here is the most authoritative copy-pastable hint for someone wiring up policy-denylist, so it should match that requirement (and ideally mirror the canonical list in harness/scripts/demo.sh).

🛡️ Suggested fix — include `bridge::trigger` and call out why

-// Permission still lives in `policy-denylist`, which subscribes to
-// `agent::before_function_call` and refuses by function id. Set its env var when
-// starting the worker, e.g.:
-//   POLICY_DENIED_FUNCTIONS=shell::fs::rm,shell::fs::sed,shell::fs::edit,shell::fs::chmod,shell::fs::mv
-// (Legacy `POLICY_DENIED_TOOLS` is still read if the new name is unset.)
+// Permission still lives in `policy-denylist`, which subscribes to
+// `agent::before_function_call` and refuses by function id. Set its env var
+// when starting the worker. `bridge::trigger` MUST be denied to block
+// recursive dispatch via `agent_call(function="bridge::trigger", ...)` — see
+// harness/ARCHITECTURE.md "Trust boundary". Example:
+//   POLICY_DENIED_FUNCTIONS=bridge::trigger,shell::fs::rm,shell::fs::sed,shell::fs::edit,shell::fs::chmod,shell::fs::mv
+// (Legacy `POLICY_DENIED_TOOLS` is still read if the new name is unset.)

📝 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

	// Function catalog: a single `agent_call` tool plus server-built system prompt —
	// see turn-orchestrator `agent_call.rs` and `system_prompt.rs`. The client
	// does not send `tools` or `system_prompt` on `run::start` (override still
	// accepted if you pass a non-empty `system_prompt` for experiments).
	//
	// Permission still lives in `policy-denylist`, which subscribes to
	// `agent::before_tool_call` and refuses by name. Set its env var when
	// `agent::before_function_call` and refuses by function id. Set its env var when
	// starting the worker, e.g.:
	// POLICY_DENIED_TOOLS=shell::filesystem::rm,shell::filesystem::sed,shell::filesystem::edit,shell::filesystem::chmod,shell::filesystem::mv
	const BASE_SYSTEM_PROMPT =
	"You have filesystem tools that operate inside a sandbox. Use them when the user asks to read, inspect, create, or modify files. Paths must be absolute (e.g. /tmp/notes.md). Some destructive ops may be denied by policy — if a tool result contains `blocked`, explain which policy refused and stop, do not retry.";
	function buildSystemPrompt(skillsIndex: string | null, cwd: string): string {
	const cwdSection = cwd
	? `## Working directory\n${cwd}\nPrefer paths under this directory. Use absolute paths.\n\n`
	: "";
	const skillsSection = skillsIndex
	? `## Available skills
	${skillsIndex}
	Use the \`skill::fetch\` tool to load any \`iii://\` URI you see above when you need its full content.`
	: "## Available skills\n\n(Skills index not loaded — call `skill::fetch` with `uri: \"iii://skills\"` to discover what's registered.)";
	return `${BASE_SYSTEM_PROMPT}\n\n${cwdSection}${skillsSection}`;
	}
	// POLICY_DENIED_FUNCTIONS=shell::fs::rm,shell::fs::sed,shell::fs::edit,shell::fs::chmod,shell::fs::mv
	// (Legacy `POLICY_DENIED_TOOLS` is still read if the new name is unset.)
	// Function catalog: a single `agent_call` tool plus server-built system prompt —
	// see turn-orchestrator `agent_call.rs` and `system_prompt.rs`. The client
	// does not send `tools` or `system_prompt` on `run::start` (override still
	// accepted if you pass a non-empty `system_prompt` for experiments).
	//
	// Permission still lives in `policy-denylist`, which subscribes to
	// `agent::before_function_call` and refuses by function id. Set its env var
	// when starting the worker. `bridge::trigger` MUST be denied to block
	// recursive dispatch via `agent_call(function="bridge::trigger", ...)` — see
	// harness/ARCHITECTURE.md "Trust boundary". Example:
	// POLICY_DENIED_FUNCTIONS=bridge::trigger,shell::fs::rm,shell::fs::sed,shell::fs::edit,shell::fs::chmod,shell::fs::mv
	// (Legacy `POLICY_DENIED_TOOLS` is still read if the new name is unset.)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@harness/web/src/App.tsx` around lines 35 - 44, Update the example denylist in
the comment to include "bridge::trigger" (alongside the existing shell::fs
entries) and add a brief note that ARCHITECTURE.md requires blocking
bridge::trigger because the model can call
agent_call(function="bridge::trigger", payload={...}) to recursively dispatch
functions and bypass name-matched rules; ensure the env var examples
(POLICY_DENIED_FUNCTIONS / legacy POLICY_DENIED_TOOLS) and the example list
match the canonical denylist from harness/scripts/demo.sh and mention
policy-denylist as the enforcement point.