chore: remove audit residue - #3242
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (16)
💤 Files with no reviewable changes (6)
Included review availability: Your plan provides up to 3 included reviews per hour; 2 remain after this review. 📝 WalkthroughSummaryThis PR removes unused audit residue in four areas:
The changes remove duplicate authorities and unused consumers. They do not change product behavior, persistence, IPC, or database formats. Runtime V1 compatibility remains follow-up work under Source of truthThe PR extends the existing Runtime The Desktop changes remove an obsolete admission gate. Runtime Host projections and validation remain authoritative. Scope and complexityThe changes form the smallest coherent cleanup described by the objectives. The added CLI helper is necessary to retain existing local TUI behavior after removing the duplicate parser. The local The deleted chat-readiness tests and module remove obsolete behavior and regression coverage for that removed gate. The remaining Runtime and projection documentation reflects the current ownership model. Complexity deltaThe PR removes:
The PR adds:
It removes authorities, branches, configuration, public surface, and test-maintenance burden. Total maintenance complexity decreases. ValidationThe PR summary reports targeted package tests, typecheck, lint, formatting checks, diff validation, surface inventory, and a repository-wide zero-reference audit. Existing unrelated root test failures are documented. The final status of required checks is unverified from the available evidence. Review-relevant risksThe diff removes Desktop chat-readiness validation and its tests. This can affect session admission and error behavior, although Runtime Host validation remains documented as authoritative. This protected behavior requires independent human review under repository policy. The diff removes CLI parser exports and changes token-grammar ownership. This can affect public package contracts and The person performing the merge must review the final diff. A maintainer makes the final determination. WalkthroughThe change removes unused desktop readiness code and tests, consolidates CLI skill-token handling on shared grammar, deletes obsolete audit tooling, removes a cost-baseline script alias, and updates runtime ownership documentation. ChangesDesktop readiness cleanup
CLI skill-token consolidation
Audit residue removal
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This cleanup removes unused checks, scripts, and stale readiness code while preserving the documented product behavior and compatibility scope; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Summary by QodoRemove orphaned audit tooling and duplicate runtime authorities
AI Description
Diagram
High-Level Assessment
Files changed (11)
|
Astro-Han
left a comment
There was a problem hiding this comment.
The current diff removes only orphaned tooling or duplicate authorities: complete skill-token parsing remains owned by Runtime/Core, the TUI keeps only its partial autocomplete prefix seam, and Desktop execution readiness remains owned by Runtime Host while the renderer projection stays advisory. The deleted chat-readiness module has no remaining production consumer, and no removed script/module references remain outside archived documentation.
No P0-P3 findings. git diff --check and the exact-head zero-reference audit are clean. CI and the bot reviews are still running, so this approval is not yet a merge-readiness claim.
AI-assisted review disclosure: Codex reviewed exact head 55a64c5, all four commits against the live base, current references, CI, and thread state; no external model was used.
中文说明
当前 diff 只删除无生产消费者的工具或重复权威:完整 skill token 解析仍由 Runtime/Core 负责,TUI 只保留局部 autocomplete prefix;Desktop 执行 readiness 仍由 Runtime Host 负责,renderer projection 只是提示。没有剩余引用,也没有 P0-P3。CI 和机器人审查仍在运行,因此当前是代码 APPROVE,不代表已经 merge-ready。
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require reviewTip of the day💡 Did you know, you can show, collapse, or hide each part of a finding: code, evidence, and all |
|
The only failing check is the known timing race tracked in #2916. After steering I reproduced the same failure once locally, followed by 5/5 passing focused repetitions; the unchanged test passed 6/6 focused runs on current upstream/main. This PR does not modify the composer, slash-menu implementation, or the E2E. I added this failure variant and its CI evidence to #2916. Could a maintainer please rerun the failed |
|
CI reruned! |
|
One thing before merging this PR, Could we state if there is any AI tools used in the process? Thanks! |
Astro-Han
left a comment
There was a problem hiding this comment.
One provenance item remains before merge: the PR body does not currently state whether generative tooling contributed, and none of the four commit messages carries a Generated-by: trailer.
Please add an explicit AI-use declaration to the PR body. If no generative tool made a substantive contribution, saying so is sufficient and no trailer is needed. If one did, please name the tool and scope in the body and add Generated-by: <tool> to each materially AI-authored commit, preserving it in the final squash commit.
This is a provenance/governance gate rather than a code finding; the earlier technical approval otherwise stands.
中文说明
合并前还缺一项来源说明:当前 PR body 没有明确是否使用生成式工具,4 个 commit 也都没有 Generated-by: trailer。
如果没有实质 AI 辅助,在 PR body 明确写明即可,不需要 trailer;如果有,请在 body 中说明工具和范围,并为每个包含实质 AI 内容的 commit 补上对应 trailer,最终 squash 时继续保留。
这属于 provenance/governance gate,不是新的代码 finding;此前技术审查结论不变。
55a64c5 to
07ecab7
Compare
|
Resolved the provenance gate:
The commits were rewritten only to add metadata. The aggregate tree hash remains |
07ecab7 to
5331982
Compare
|
Resolved the merge conflicts by rebasing onto current Conflict resolution preserves the Desktop authority cleanup already merged in #3252, including its narrower compatibility-projection exports and current Runtime Host ownership wording. The overlapping Desktop deletion was dropped from this branch; the fourth commit now contains only three stale active-code comment fixes not covered by #3252. The Astryx, DeepSeek, and CLI authority cleanups remain unchanged in intent. Fresh verification on the rebased tree passed: full build, typecheck, lint, format check, Astryx inventory, CLI/Core/Runtime Host/Desktop tests, diff check, and repository-wide zero-reference/conflict-marker audits. All four commits retain |
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks — the deletions check out, and I verified each of them rather than taking the description's word for it. Reviewed exact head 53319827cf828c827c6b488e797c0a2bf7eef788.
packages/cli/src/skill-token.ts really was duplicate authority: parseSkillInvocationTokens and stripSkillInvocationTokens already live in packages/runtime/src/skill-invocation.ts, which is what interactive-turn-coordinator.ts and root-turn-coordinator.ts actually call, and the CLI copies had no consumer anywhere in the repository. The Astryx alignment script is genuinely orphaned — .github/workflows/ci.yml runs astryx:surface-inventory and astryx:theme, never the alignment check — and a search at this head finds no surviving reference to either deleted script or to the removed cost:deepseek-baseline entry. The comment cleanups are real fixes, not cosmetics: hasConnectionSecret and resolveConnectionSecret no longer exist anywhere in apps/desktop, so that comment was pointing at nothing.
Two P2s inline, both about the shape of the PR rather than about any deletion being wrong. Also worth noting factually: no CI checks are reported at this head, so the twelve local commands in the description are currently the only evidence for a change that removes a package.json script and 3,190 lines.
Review disclosure: this review was prepared with Claude Code, which read the diff at this head, ran the zero-reference searches described above against the head tree and against main, and compared this PR's CLI changes against #3267. Nothing here was reproduced by execution. The human contributor reviewed this before posting.
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
5331982 to
a108efd
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for narrowing this — both of my earlier P2s are resolved by the change itself. Re-reviewed exact head a108efd45a79ec80a9479102c125b2710e2a7f78.
The three CLI files are gone, so the collision with #3267 is gone with them and that PR can own the skill-token consolidation on its own; packages/cli/src/skill-token.ts is untouched here, which is the right end state. The hardcoded [A-Za-z0-9._-] copy went with it, so the grammar-duplication finding no longer applies to this PR either.
What remains is one coherent removal pass, and I re-verified it at this head rather than assuming it carried over: no reference anywhere in the tree to check-astryx-alignment, the removed cost:deepseek-baseline script, or the deleted DeepSeek baseline; CI runs astryx:surface-inventory and astryx:theme, never the alignment check, so the surface inventory gate is genuinely retained; and the three comment edits are real fixes rather than cosmetics, since hasConnectionSecret and resolveConnectionSecret no longer exist anywhere in apps/desktop.
No findings. Approving — the required test check is still running at this head, and branch protection will hold the merge until it is green.
|
Review follow-up is pushed at The refreshed #3242 has no diff in the Windows packaging/update paths, and the same workflow completed successfully in the adjacent run 32344311384. I attempted a failed-job rerun, but GitHub requires repository admin rights. Could a maintainer please rerun the failed Fresh local verification at |
Related #3218
Related #3215
Related #3267
Summary
The original branch also removed the obsolete Desktop chat-readiness module. That deletion and the associated compatibility-projection cleanup are now supplied by
mainthrough #3252. The rebase preserves #3252 unchanged and retains only the stale-comment cleanup that it did not cover.The original branch also consolidated the duplicate CLI
/skill:token authority. That exact three-file change is already owned by #3267, so it has been removed from this PR to avoid overlapping review and a guaranteed merge conflict. This PR therefore no longer independently closes #3218; the remaining CLI item belongs to #3267.These changes remove unused consumers and stale authority references without changing product behavior, persistence, IPC, or database formats. Runtime V1 compatibility remains follow-up work under #3215.
Verification
AI use
Tool(s) and scope: OpenAI Codex contributed to implementation of the cleanup changes, repository analysis, conflict resolution, test execution, review follow-up, and PR preparation. The human contributor reviewed the work and submitted the contribution. Each materially AI-authored commit includes
Generated-by: OpenAI Codex; the trailer must be retained in the final squash commit.