feat(runtime): guide Auto tool selection by final tool surface - #3705
Conversation
18a5aba to
191282e
Compare
yunaremaia
left a comment
There was a problem hiding this comment.
Nice, clean slice — the eligibility matrix is well chosen (bypass needs no anti-bypass guidance, explore has no mutation tools to name), the fragment is deterministic and pure, and the tests cover both the positive path and every exclusion branch including the partial-tool-surface case. The bump of INTERACTIVE_RUN_COMPOSER_REVISION for the prompt-shape change is also the right call.
One coupling worth making explicit while this is fresh:
The Bash gate lives two checks away from its supporting fact. In interactive-run-composer.ts you compute
shellAvailable: input.shell?.setupError === undefined,which maps an absent shell to undefined rather than false. The resolver then only treats literal false as disqualifying, so a session with no shell at all still passes the shellAvailable check — it survives solely because a missing shell also removes 'Bash' from tools, and resolveAutoToolGuidance independently requires toolNames.includes('Bash'). That's correct today, but the safety of the first check depends on an invariant maintained by a different subsystem (whatever guarantees "no shell ⇒ no Bash tool entry"). If that ever drifts — say a future surface exposes a remote-shell Bash entry without a local input.shell — the guidance would advertise shell-first workflows into a session that cannot run them, and nothing here would fail loudly.
Two cheap ways to pin it down; either works:
- Treat absence as absence: pass
shellAvailable: input.shell !== undefined && input.shell.setupError === undefinedand keep the resolver's=== falsecheck (or switch both to truthiness). One expression, no cross-module assumption. - Keep the current wiring but add one line to the resolver doc comment stating that
shellAvailable === undefinedmeans "unknown" and eligibility then rests entirely on thetoolNamesgate — so the next reader knows the pairing is intentional.
Not blocking — behavior is correct as written — but I'd sleep better with option 1.
|
Thanks @yunaremaia — fixed in commit bf96cd0 using option 1. An absent shell plan is now explicitly treated as unavailable by requiring both a defined shell plan and no setup error, so Auto guidance no longer relies on the separate final-tool-surface gate to stay safe. I also updated the positive prompt fixtures to include a valid shell plan and added a regression case proving that a Bash-shaped tool surface without a shell plan does not receive the guidance. Verification: Runtime Auto guidance 4/4 passing; Runtime Host 1137/1137 passing; Biome and git diff --check clean. |
Astro-Han
left a comment
There was a problem hiding this comment.
I reviewed this head and found no blocking issues.
The fix adds Auto-mode tool guidance as a pure function that chooses whether to inject a deterministic prompt based on the final tool surface and Host permission snapshot. It is minimal (1 file + 1 call site), correctly handles restricted surfaces and permission bypass cases, and bumps the composer revision to invalidate caches.
Checks on bf96cd058c are test: pending — code side is GO. Minor note: this adds a small new concept/module (+72 lines); the increment is valuable for recovery (#3507) and well isolated, so no action required.
简体中文
该头未发现阻断问题,轻度增熵有价值。|
Holding review here — the premise question is on #3507. Implementation quality isn't the issue: 443 lines across 5 files, pure resolver, well-gated eligibility, no design doc in the PR. Nicely contained. But one thing is visible from the diff and worth raising now: the fragment doesn't do what the issue asks for. #3507 quotes a directive instruction — do the work through Bash wherever it can, fall back to dedicated tools only when Bash genuinely can't. What So both paths lead to the same place: if the retry-loop problem is real, this text probably doesn't move it; if it isn't real, we've added permanent tokens to every Auto session's system prompt. Either way it needs an eval A/B rather than a code review. Could you run one alongside this PR — same task set, fragment on vs off, tool-failure retry rate and completion rate? If it moves the numbers I'll review the implementation properly. 简体中文先暂停评审——前提问题写在 #3507。实现质量不是问题:443 行 5 个文件,纯函数 resolver,准入条件收得很干净,PR 里也没有塞设计文档,边界很好。 但有一点从 diff 就能看出来,值得现在提:这个片段并没有在做 issue 要求的事。#3507 引用的是一条指令性的提示——能用 Bash 完成的就用 Bash,只有 Bash 确实做不到时才回退到专用工具。而 所以两条路通向同一个地方:如果重试循环问题真实存在,这段文字大概率推不动它;如果不存在,我们就是在每个 Auto 会话的系统提示里永久多加了一段。无论哪种,需要的是 eval 对照,而不是 code review。 能否和这个 PR 一起跑一组?同一批任务,开/关片段,看工具失败重试率和完成率。数字有变化,我就正式评审实现。 |
Hold per AstroHan — definition needs re-review, P2 pending
6613c70 to
0e4bc4a
Compare
Generated-by: Codex
Generated-by: Codex
0e4bc4a to
8620d8a
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for closing the shell-availability edge case. I reviewed exact head 8620d8afdc6fef8e570ad013470112def1f872d9.
This PR solves a real guidance gap without moving execution authority: Auto-mode advice is derived only after the Host permission snapshot and final model-visible tool surface are known, and it remains a pure prompt fragment. The follow-up now treats a missing shell plan as unavailable at the composition seam and adds the corresponding main-session regression, so the guidance no longer depends on the separate invariant that “no shell” also removes Bash.
The production change stays small and within the existing interactive composer; the larger test surface is justified by the eligibility/exclusion matrix rather than a new runtime state machine. I found no remaining P0-P2 issue on this head, and all reported exact-head checks are green.
中文对照
感谢补上 shell availability 的边界情况。我复核了最新提交 8620d8afdc6fef8e570ad013470112def1f872d9。
这个 PR 解决了真实的 Auto 模式工具选择指导缺口,但没有移动执行权威:只有在 Host permission snapshot 和最终 model-visible tool surface 已确定后,才生成一段纯 prompt guidance。后续修复在 composition seam 把缺失 shell plan 明确视为不可用,并补了主会话回归,因此不再依赖“没有 shell 时另一个子系统也一定会移除 Bash”这一跨模块假设。
生产改动仍然很小,并留在现有 interactive composer 内;较大的测试部分是在覆盖资格/排除矩阵,没有引入新的 runtime 状态机。这个 head 上我没有发现剩余的 P0-P2,GitHub 当前报告的 latest-head checks 也都通过。
AI-assisted review: Codex traced the final tool-surface and shell-plan composition; I independently verified the issue value, incremental fix, exact head, and severity.
* refactor(runtime): remove unvalidated Auto tool guidance Remove the prompt fragment, composition wiring, export, and feature-specific tests added by #3705. Maka has no demonstrated retry-loop reproduction or A/B evidence for this guidance, and the implemented best-fit wording does not match the issue’s CLI-first premise. Generated-by: OpenAI Codex * test(core): remove speculative sanitizer coverage Remove the direct sanitizer matrix added by #3692. It was not tied to a reported regression, planned behavior change, or failing contract, while existing session-name and foreign-session tests continue to cover the real trust-adjacent behavior. Generated-by: OpenAI Codex
Closes #3507
What changed
packages/runtime/src/system-prompt/auto-tool-guidance.ts.Scope
This changes prompt guidance only. It does not change permission classification, sandbox or approval execution, Bash schema/implementation, persistence protocols, or the
headless-coding-v1contract.Verification
git diff --checkpassed.AI use
Tool: OpenAI Codex assisted with issue analysis, implementation, tests, and verification.