refactor: remove unvalidated review additions - #4264
Conversation
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
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
hqhq1025
left a comment
There was a problem hiding this comment.
I reviewed exact head f4481cef337d6cf504b6c5065f61616505855fa8 and found no P0-P3 issues.
This change removes the Auto-mode guidance feature and its Host wiring, package export, and tests, restoring the affected files to their state before #3705. It also removes the direct sanitizer test matrix while leaving the production sanitizer and the existing session-name and foreign-session boundary coverage unchanged.
Validation completed successfully: clean dependency install, npm run build:test, full typecheck, Core tests (732/732), Runtime tests (3090 passed, 13 skipped), full lint and formatting checks, changed-file checks, git diff --check, a clean merge-tree against current main, and all hosted checks. Runtime Host reported 1426 passed and 12 skipped with one sandbox-context failure; the same failure reproduced unchanged on exact base 2cb1044d8a19135de9ef2a9f37e42c72ed1948f7, so it is not attributable to this pull request.
Remaining uncertainty: I did not run a product-level A/B evaluation for the removed prompt fragment. That evidence should be required before the feature is reintroduced.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
hqhq1025
left a comment
There was a problem hiding this comment.
I reviewed exact head f4481cef337d6cf504b6c5065f61616505855fa8 and found no P0-P3 issues. All hosted checks are successful, and the current main branch changes only an unrelated Storybook file; the merge tree remains clean.
The sole local Runtime Host sandbox-context failure reproduced unchanged on exact base 2cb1044d8a19135de9ef2a9f37e42c72ed1948f7, so it is not attributable to this pull request.
Review notice: This review was prepared by an automated review agent operated by hqhq1025 and is published at the direction of AstroHan, who has read these findings and is the human accountable for them.
jackwener
left a comment
There was a problem hiding this comment.
Reviewed the latest head f4481cef3. No P0 or P1 — approving. One P3 inline. All checks are terminal green on this head, including test, so the renderer-architecture breakage I reported on #4260 has cleared.
The Auto tool guidance removal is a clean revert. The import, the resolveAutoToolGuidance call, the prompt fragment, and the now-unused permissionMode composer input all go together, and nothing anywhere still references the module or its export. The six tests removed from execution-model-composition.test.ts are precisely the ones covering the deleted feature — injection eligibility, child/restricted exclusion, the side-conversation boundary, permission forwarding — so they are moot rather than lost; 27 unrelated tests remain in that file.
Reverting INTERACTIVE_RUN_COMPOSER_REVISION from '2' to '1' is the right call, and I checked why. composerRevision is persisted through run-composition.ts, so a revision meaning two different prompt contents would be a genuine hazard. It does not happen here: revision '2' was introduced by #3705, the same commit being reverted, so restoring '1' restores exactly the content '1' always denoted. Revision-to-content stays a true function, and anything recorded under '2' simply no longer matches and recomposes, which is correct.
The obvious worry does not materialise. Deleting text-sanitize.test.ts does not leave the security-critical behaviour uncovered. sanitizeUnicodeText itself remains, with two live call sites at foreign-session.ts:117 and session-name.ts:91 — but foreign-session.test.ts still carries literal NUL, zero-width space and right-to-left override characters in its fixtures, and session-name.test.ts still covers the deprecated Cf bidi-adjacent controls at U+206A–206F. Direction spoofing and control characters are therefore still exercised through both call sites. My P3 is about the narrower contract those two tests do not pin.
Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.
| ); | ||
| }); | ||
|
|
||
| test('caps length by code points without splitting surrogate pairs', () => { |
There was a problem hiding this comment.
P3 — this is the assertion I would most want kept.
sanitizeUnicodeText is not removed by this PR. It keeps two production call sites, foreign-session.ts:117 on the imported-session path and session-name.ts:91. After this change no test file anywhere references it by name.
The spoofing-relevant behaviour survives, and I checked rather than assumed: foreign-session.test.ts still has literal NUL, ZWSP and RLO in its fixtures, and session-name.test.ts covers U+206A–206F. So the security case is still exercised at both call sites.
What this file uniquely pinned was the helper's own contract, and I do not find it covered elsewhere: NFC normalisation so equivalent spellings compare equal, whitespace-run collapsing and trimming, the silent cap via an empty suffix, the sanitises-to-nothing case, and this one — capping by code points without splitting a surrogate pair.
That last property is the one worth preserving. A truncation that cuts a pair in half is easy to reintroduce, neither call-site test appears to cover it, and the failure yields a lone surrogate rather than a loud error. Restoring a few of these assertions, either here or folded into the call-site tests, would keep the guarantee cheaply. Not a blocker — I understand the intent of dropping speculative coverage, and I am only naming which specific guarantees leave with it.
zhiiw
left a comment
There was a problem hiding this comment.
Reviewed at exact head f4481cef (verified unchanged at review time; all checks completed/success on this head, including test 10m56s and windows-latest).
Exact-revert removal, verified rather than assumed:
- The restoration is byte-exact: every
#3705-touched file (auto-tool-guidance.ts, its test, the runtime export, the composer wiring, the composition test removals) is byte-identical to95c80a715^, and the#3692test file is byte-identical to7bc66fdd1^— I diffed both sides against those parents directly. No partial revert, no silent edits riding along. - Zero residual references: no import, export, or textual reference to
auto-tool-guidancesurvives anywhere at this head. - The author's local build blocker does not reproduce here: the reported Slack SDK /
HttpsProxyAgenttype errors are environmental to that checkout. On a real Windows machine at this head: clean forced rebuild of all workspace libs plus Desktop main, both green; coresession-name+foreign-session38/38; the one local failure I hit (Host auxiliary calls preserve resolved DeepSeek reasoning settings) is the pre-existing Windows flake class — EBUSY unlinking a temp SQLite-shmduring fixture teardown, in a test this PR does not touch; the hostedtestjob runs the same file green on this head. - Scope discipline: no replacement path, flag, or compatibility layer added; the behavior change is exactly "the two additions are gone".
On the value judgment itself: the removal rationale (unreproduced behavior claim, no eval A/B, wording diverging from the CLI-first premise) is a maintainer call and reads consistently with the cited history; the reopen-#3507-as-evidence-proposal follow-up is the right shape.
Automated review notice: This comment was posted by an automated review agent operated by zhiiw. It is not an independent human review and does not replace one.
简体中文
精确回退,全部核实而非假设:#3705 侧全部文件与 95c80a7^ 逐字节一致,#3692 测试文件与 7bc66fd^ 逐字节一致(两侧直接对父提交 diff);auto-tool-guidance 全树零残留。作者本机的构建阻塞(Slack SDK/HttpsProxyAgent 类型错误)在我的真 Windows 机器上不复现——本 head 干净强制重建 libs + Desktop main 全绿,core 两套件 38/38。本机唯一失败是既有 Windows flake 类(fixture 拆除时 EBUSY 删 sqlite-shm,落在 PR 未触碰的测试里;hosted test 在同一 head 跑同文件绿)。范围纪律:无替代路径/开关/兼容层。价值判断本身是维护者裁决,与所引历史一致;#3507 重开为取证提案的后续形态正确。
* refactor(runtime): remove unvalidated Auto tool guidance Remove the prompt fragment, composition wiring, export, and feature-specific tests added by apache#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 apache#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
Summary
Remove two recently merged additions that did not pass the repository’s problem-value gate:
This intentionally restores the affected files to their state before those two merges. It adds no replacement path, feature flag, compatibility layer, or new test authority.
Why
#3705 was merged even though the reported retry-loop behavior had not been reproduced in Maka or measured with an eval A/B. Its implemented “choose the best-fit tool” wording also did not match #3507’s CLI-first premise. Implementation correctness did not establish product value.
#3692 was motivated only by the absence of direct unit tests. It was not tied to a reported regression, planned sanitizer change, or failing contract, while the existing session-name and foreign-session suites already exercise the real trust-adjacent behavior.
After this lands, #3507 should be reopened as an evidence-gathering proposal: reproduce the Maka failure mode and compare the prompt fragment on/off before adding permanent Auto-session guidance.
Refs #3507, #3705, #3692.
Verification
npm --workspace @maka/core run buildnode --test packages/core/dist/__tests__/session-name.test.js packages/core/dist/__tests__/foreign-session.test.js— 38 passednpx biome check packages/runtime-host/src/__tests__/execution-model-composition.test.ts packages/runtime-host/src/server/interactive-run-composer.ts packages/runtime/package.jsongit diff --check95c80a715^and the test(core): unit-test the shared Unicode sanitizer #3692 test file matches7bc66fdd1^Local Runtime and Runtime Host validation could not complete because this checkout currently fails before the affected tests: Runtime compilation cannot resolve the Slack SDK types and reports an existing
HttpsProxyAgenttype mismatch; the resulting incomplete Runtime build leaves Runtime Host withoutwithOpenAiCodexHistoryCompactionFallback. Hosted exact-head CI is therefore required before this leaves draft.AI use
Tool and scope: OpenAI Codex traced the prior review decisions, removed the two exact merge deltas, ran focused verification, and drafted this PR. The human maintainer identified the incorrect value judgment and owns the correction.
Checklist
Does this PR entail a change in behavior?
中文说明
这个 PR 合并清理两项没有通过问题价值门槛的改动:删除 #3705 增加的 Auto 模式工具选择提示及其完整实现/测试,同时删除 #3692 在没有真实回归或计划中行为变更支撑下增加的 sanitizer 直接测试矩阵。
#3705 的问题只在 Claude Code 中观察到,Maka 没有复现或 A/B;实际实现也不是 issue 所描述的 CLI-first。#3692 则把“缺少直接测试”本身当成了需求。两者都不应该因为实现整洁、CI 绿色就获得合并价值。