Conversation
✅ Deploy Preview for golemcloud canceled.
|
There was a problem hiding this comment.
There's one thing we should probably change before merge (comment on durable_host/tool/mod.rs), and two things I wasn't sure about and would rather ask than assume. All inline.
On tests — the ticket asks for coverage of enumeration, lookup, empty environments, unknown names, access filtering and revision isolation. The first four are there. Revision isolation only really shows up indirectly through the replay test, and nothing sets up a second environment, so the "don't expose tools from unrelated environments" part isn't checked anywhere.
Tiny one: the comment at the top of durable_host/tool/mod.rs (lines 15-17) ends with "until the tool runtime is implemented". AGENTS.md asks that comments say what the code does now rather than what's coming later. The "layered on in later steps" bit was already dropped — this is just the leftover.
| } | ||
| } | ||
| } else { | ||
| Ok(Vec::new()) |
There was a problem hiding this comment.
If parsed_agent_id() up at line 89 comes back None, we land here and return an empty list. That empty list doesn't just go back to the caller — it goes through handle.complete(...) and gets written to the oplog, so every replay from then on sees "no tools" as well, and there's no way to tell it apart from an environment that genuinely has none.
GOL-30 says to filter by the calling agent's accessible set. If we can't work out who the caller is, we don't have an accessible set — that's a bug on our side rather than an empty answer, so I think this should return an error. classify_tool_discovery_error and terminal_tool_discovery_error are already set up in this file
Same thing happens at line 197.
There was a problem hiding this comment.
This is an error now (in practice, if it's None it means that an oplog processor plugin called this host function)
| } | ||
| } | ||
| } else { | ||
| Ok(None) |
There was a problem hiding this comment.
Same as line 129 — when agent_type is None we save an empty answer.
If valid_tool_name is None because someone passed a name that isn't a valid tool name, returning None looks right to me — GOL-30 wants get-tool to answer unknown names with none. It's only the missing agent id that I think should be an error.
| let result = self | ||
| .state | ||
| .environment_state_service | ||
| .get_accessible_tool(environment_id, agent_type, valid_tool_name) |
There was a problem hiding this comment.
Each of these two functions goes and fetches the environment state on its own, so a get-all-tools and a get-tool in the same invocation can end up looking at different deployments. An agent could list a tool and then fail to find it a moment later.
GOL-30 asks to use the same environment-state snapshot that GOL-29 produces, which reads like it wants one snapshot held for the whole invocation. But the live test here deliberately checks that get-all-tools picks up a new deployment part way through, so I'm guessing seeing changes as they happen is what you actually wanted. Which of the two is it?
There was a problem hiding this comment.
This now works the same as the agent type related host functions - an agent sees the deployment it's originated from, not the latest.
| .registered_tools | ||
| .get(tool_name) | ||
| .cloned() | ||
| .ok_or_else(|| ToolDiscoveryError::InconsistentSnapshot { |
There was a problem hiding this comment.
This lookup, and the one at line 114, match a binding to a tool on the name alone. Neither the binding's deployment_revision nor the tool's gets checked against the deployment's.
Worth flagging that the test at line 483 doesn't cover this, even though it reads like it might — deployment_state() builds every tool at revision 1, so asserting they all come back at revision 1 passes regardless of what this function does with revisions. It'd pass just as happily if the join ignored them entirely, which is what it does.
In practice it's fine as long as GOL-29 always hands over a snapshot from a single revision, which I'd expect it to. So this is a question rather than a complaint: is leaving the revision check entirely to the code that builds the snapshot the split you meant? Happy for the answer to be yes — just want it to be a decision rather than an accident.
| .get(tool_name) | ||
| .cloned() | ||
| .map(Some) | ||
| .ok_or_else(|| ToolDiscoveryError::InconsistentSnapshot { |
There was a problem hiding this comment.
Small thing: this error is word for word the same as the one at line 87. Creating a little dangling_binding(agent_type, tool_name) or similar helper would make sure these 2 stay in sync, unless in the future it is ok for them to drift?
| ) | ||
| } | ||
|
|
||
| #[test] |
There was a problem hiding this comment.
Couple of gaps against the coverage the ticket asks for, on access filtering and revision isolation:
Nothing here uses a second environment — every test runs against context.default_environment_id, so we never actually check that a tool in one environment stays hidden from another.
Nothing covers a stale revision either. Revision isolation only shows up indirectly, through the replay test. A case where a binding or a RegisteredTool carries an older deployment_revision than the deployment would also address the comment I raised over on environment_state.rs:87.
kmatasfp
left a comment
kmatasfp
left a comment
There was a problem hiding this comment.
One follow-up on performance, inline.
| deployment | ||
| .registered_tools | ||
| .get(tool_name) | ||
| .cloned() |
There was a problem hiding this comment.
Each call here deep-clones every tool it returns, and the source is already behind an Arc<EnvironmentState> from the cache — so a shared, immutable tool definition gets copied in full, schema graph and all, every time. DiscoveredTool::from then destructures it and drops everything from source bar the component id. There's a second Vec on top of that, allocated in get_all_tools_model for the .map(DiscoveredTool::from).collect().
The same .cloned() is in get_accessible_tool_from_snapshot at line 115. That one only copies a single tool rather than the whole accessible set, so it matters less, but it's the same wasted copy and the same fix.
One agent doing this occasionally is nothing, but with a lot of agents each calling get-all-tools per model turn it could start to add up — and it looks fairly easy to avoid. If both functions built DiscoveredTool directly they'd only need to clone definition and the component id, and the full copies and the second Vec would go away.
2 participants
Resolves GOL-30