Skip to content

refactor: remove unvalidated review additions - #4264

Merged
Astro-Han merged 2 commits into
mainfrom
refactor/remove-unproven-review-additions
Aug 30, 2026
Merged

refactor: remove unvalidated review additions#4264
Astro-Han merged 2 commits into
mainfrom
refactor/remove-unproven-review-additions

Conversation

@Astro-Han

Copy link
Copy Markdown
Contributor

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

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 HttpsProxyAgent type mismatch; the resulting incomplete Runtime build leaves Runtime Host without withOpenAiCodexHistoryCompactionFallback. Hosted exact-head CI is therefore required before this leaves draft.

AI use

  • Generative tooling made a substantive contribution

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

  • The cumulative diff removes the superseded behavior and its tests
  • Formatting, diff checks, and the available focused caller tests pass
  • Hosted exact-head CI passes

Does this PR entail a change in behavior?

  • Yes — it removes unvalidated Auto-mode prompt guidance
  • No
中文说明

这个 PR 合并清理两项没有通过问题价值门槛的改动:删除 #3705 增加的 Auto 模式工具选择提示及其完整实现/测试,同时删除 #3692 在没有真实回归或计划中行为变更支撑下增加的 sanitizer 直接测试矩阵。

#3705 的问题只在 Claude Code 中观察到,Maka 没有复现或 A/B;实际实现也不是 issue 所描述的 CLI-first。#3692 则把“缺少直接测试”本身当成了需求。两者都不应该因为实现整洁、CI 绿色就获得合并价值。

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
@github-actions github-actions Bot added the effort/L Under 1000 readable lines label Aug 30, 2026
@Astro-Han
Astro-Han marked this pull request as ready for review August 30, 2026 12:23

@hqhq1025 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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', () => {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Astro-Han
Astro-Han merged commit be8608e into main Aug 30, 2026
8 checks passed
@Astro-Han
Astro-Han deleted the refactor/remove-unproven-review-additions branch August 30, 2026 12:41

@zhiiw zhiiw left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 to 95c80a715^, and the #3692 test file is byte-identical to 7bc66fdd1^ — 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-guidance survives anywhere at this head.
  • The author's local build blocker does not reproduce here: the reported Slack SDK / HttpsProxyAgent type 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; core session-name + foreign-session 38/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 -shm during fixture teardown, in a test this PR does not touch; the hosted test job 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 重开为取证提案的后续形态正确。

saltand pushed a commit to saltand/maka-agent that referenced this pull request Aug 31, 2026
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/L Under 1000 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants