Repository navigation
fix(assistant): keep the discovery Session to read and report tools - #6256
Conversation
The proactive discovery Session could create Matters that the conversation cannot see, adopt unassigned Matters, mutate conversation memory indexes, and control initiative settings. Its catalog and execution now share one visibility policy, and a discovery-only exclusion list removes these tools. Read tools, DiscoveryReport and DiscoveryContext remain available. The conversation, the Bot and index workers keep their existing tools. The exclusion list is intentionally scoped to verified behavior; whether MemorySourceQuery and MemoryExtract belong in discovery is still open. Refs #6247, fixes #6249, refs #6250 Generated-by: Claude Code Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
Reviewed the six-file change at 149a79ca18c6cf56e20a7d889514b2191a898391 (+289/−8). I found no confirmed P0–P2 issue in the scoped assistant-tool restriction.
The discovery-session filter in index-initiative/host.ts excludes the listed Matter, memory-index mutation, initiative-control and Bot-only tools. AssistantRuntime.visibleTools is shared by catalog filtering and the public assistant-tool execution wrapper, so directly invoking an excluded assistant tool is also rejected. The filter follows the persisted discovery-session identity and leaves owner conversations, Bot workers and index workers on their existing paths. The six new tests exercise delegation, adoption, memory mutation, catalog visibility, existing task isolation and retained read access.
Fresh dependencies/patches, the normal root build, all 195 assistant tests and 56 focused Host tests passed. Rebuilding the real test bundle with only the base service and initiative modules makes four of the six new tests fail; the two existing-behavior guards still pass. Changed-file Biome, ASF headers, diff checks, the epoch guard (227; no wire change) and static merge against main passed.
This is not a complete read-only sandbox: core shell/edit tools and arbitrary external tool effects are outside this exclusion list, and MemorySourceQuery / MemoryExtract remain allowed as documented. I did not validate hostile model behavior or real external plugins. #6253 overlaps this work and currently conflicts in index-initiative/host.ts; reconcile their policies if both are retained.
CI failed while the unchanged ShellRun crash-recovery test parsed a partially written JSON fixture. Its test, shell manager, npm dependency files and workflow match the base, and the focused test passes locally; I have not independently reproduced that failure on main. I reran the failed job once; the new result is pending. This review is not a green-CI merge recommendation.
CI attempt 2 has also failed: the same unchanged ShellRun crash-recovery fixture parsed incomplete JSON, this time in its |
|
@me2seeks @jackwener @xxhZs could one of you review this? It is the follow-up to the discovery-boundary RFC (#6247) and its bug reports (#6249, #6250). What changed: the proactive discovery Session can no longer create or adopt conversation Matters, mutate conversation memory indexes, or change initiative settings. The catalog and execution share one visibility policy, and read tools stay available. The conversation, the Bot and index workers are unchanged. The |
me2seeks
left a comment
There was a problem hiding this comment.
Automated review performed with Codex; this does not represent an independent human review.
Reviewed current head 149a79c against the scope of #6247, #6249 and #6250. No blocking issue found in the scoped assistant-tool restriction. Discovery identity comes from the persisted Host binding, and the shared visibility policy rejects excluded assistant tools both in the catalog and when cached handles are invoked. Conversation, Bot and Matter/index-worker paths retain their existing roles.
Locally verified in an isolated exact-head worktree with Node 24.18.1: all 195 assistant tests, including the six discovery-boundary tests, passed; 56 related Host tests passed when bundled from this head with source aliases. The dependency manifests and lockfile match the existing installation. Current-head CI is green and there are no unresolved review threads.
Scope note: the discovery-specific exclusion list is an incremental restriction, not the complete allowlist discussed in the RFC or a read-only sandbox. Native shell/edit and arbitrary external effects are outside this review's validation; MemorySourceQuery/MemoryExtract policy and the broader RFC product decisions remain open. These documented follow-ups do not block this targeted fix.
Summary
The proactive discovery Session runs open-ended investigation in the background. At
origin/main@59f61cf7dit could do more than investigate and report:MatterDelegate, bug(assistant): MatterDelegate from the discovery session creates a Matter the conversation cannot see #6249);MatterAdopt);MemoryIndex*, bug(assistant): discovery session can mutate conversation memory indexes and adopt unassigned Matters #6250);This PR gives the discovery Session its own exclusion list. The same visibility policy now governs the model-facing catalog and execution, so a tool hidden from a Session cannot run in it either. Read tools,
DiscoveryReportandDiscoveryContextstay available. The conversation, the Bot and index workers are unchanged.Fixes #6249. Refs #6250 (open: the allowlist decision for
MemorySourceQueryandMemoryExtract; native shell and edit tools in the discovery Session were not verified). Refs #6247 (RFC).Behavior change
DISCOVERY_EXCLUDED_TOOLS(index-initiative/discovery-capabilities.ts).DiscoveryReport.Verification
Regression tests were written first and run against the unfixed code: 4 of 6 failed, and the 2 that passed are guards for behavior that was already correct (conversation-delegated tasks cannot be controlled from discovery; discovery can still read conversation memory).
npm run typecheck(runtime-host, Node 24.21.0)discovery-boundary.test.ts, 6 testsnpm run test:assistant(memory-network, index-initiative, proactive-matters)npm run test:dist(runtime-host)biome checkandasf-license-headers checkon changed filesNote on
test:dist: the full run exposed one pre-existing failure and several load-sensitive timeouts.assistant Host fallback registers a task workspace …fails onorigin/main@59f61cf7das well. I checked this by stashing this change and rebuilding the clean baseline. It is unrelated to the discovery boundary and is not fixed here.interrupt atomically …,two Clients share one execution …,invocation-owned detached Host retires …,non-serving Runtime Host kernel) pass when run in isolation. They failed only under the full concurrent run.assistant-initiative-boundary.test.tscame from this change: the test harness did not providetools.filter, and the new fail-closed check rejected it. The harness now provides it.Review focus
MatterRead,MatterReadFile,InboxListandInboxReadare excluded because they are worker-only or Bot-only. Please confirm none is needed by discovery.MemorySourceQueryandMemoryExtractremain available. Native shell and edit tools were not exercised because the test fixture exposes plugin tools only.AI use
Tool(s) and scope: Claude Code (Anthropic) investigated the code paths, wrote the regression tests and the change, and drafted this description. The checks above were run locally by the tool. Human review is pending; the contributor of record owns the change's accuracy and licensing. Commits carry
Generated-by: Claude Code.Checklist
test:dist)Does this PR entail a change in behavior?
🤖 Generated with Claude Code