Repository navigation
feat(hooks): add Trae IDE integration - #3008
Conversation
5900881 to
22f3fe8
Compare
|
@KuSh, could you please help review this PR and approve/run the fork CI when you have a moment? I have addressed the safety gate, canonical Trae documentation, and audit coverage. The full local gate passes: formatting, clippy with warnings denied, and all tests (2583 passed, 8 ignored, 0 failed). If everything looks good, I would appreciate your help moving it toward merge. Thank you! |
|
@KuSh Could you please review this PR and approve the fork CI run when you have a moment? I have synced with the latest Local validation passes: formatting, strict Clippy, all-feature tests (3799 passed, 8 ignored), release build, test-presence check, and a temporary-directory install/rewrite/uninstall smoke check. The PR description now contains the current validation results. Upstream cross-platform CI is still pending. Thank you for helping move this toward merge! |
KuSh
left a comment
There was a problem hiding this comment.
Round 1 of 3 — CHANGES REQUESTED
Claim: install and uninstall a Trae PreToolUse hook for RunCommand (project and global, plus ~/.trae-cn when it exists) and rewrite commands through rtk, leaving approval to Trae.
Scope: accept (frozen at round 1). This is the surviving Trae PR — #1447 was closed as a duplicate of it, #2411 and #2056 are closed too, and nothing else open touches Trae. Registration parity with the Codex and Cursor siblings is complete across decision.rs, permissions.rs and main.rs.
Ran: both binaries built in separate target dirs and hash-checked before trusting any transcript. Project and global install, --show, --uninstall, --dry-run, a second install on top (idempotent), an existing hooks.json full of unrelated user hooks through install → uninstall (round-trips exactly, userSetting and PostToolUse untouched), ~/.trae-cn opt-in, CRLF config, BOM config, a malformed sibling target (preflight holds — nothing written), a read-only target, six malformed PreToolUse payloads, empty and closed stdin, and rtk hook check --agent trae diffed against --agent codex. Gates green on the head and on the head merged onto the latest develop (0a71fcf3, clean merge, 3801 tests). The rewrite this hook installs delivers 46–65% on git status / git log -20, against the 20% floor (CONTRIBUTING.md).
CI @ca24862: never built. Run 34968708266 is action_required; only the CLA check and a skipped check-target are green. I have approved the run — please check it once it finishes.
Blocks merge (1, frozen at round 1)
src/hooks/init.rs:5276— presence and removal probes gate on fields RTK does not control (2 sites::5214+:5228and:5276; both must change). See the inline comment for the transcript and the fix.
Optional — will not hold merge
These are all "the new Trae code hand-rolls a helper that already exists and loses behaviour in the copy". I built and measured each fix rather than guessing, so the before → after is real, but none of them holds the merge.
-
BOM-blindness, 3 sites —
run_trae(hook_cmd.rs),read_trae_hooks_jsonandremove_trae_hooks_json_paths(init.rs) parse with a rawserde_json::from_str.read_json_file(14 callers) and every sibling handler inhook_cmd.rsgo throughstrip_leading_bom/from_json_str. Given the same UTF-8-BOM'dhooks.json:rtk init -g --agent cursor → exit 0, installed rtk init -g --agent trae → exit 1, "Failed to parse Trae hooks file ... as JSON"and a BOM'd
PreToolUsepayload makes the hook skip the rewrite entirely, wherertk hook clauderewrites it. Routing the twoinit.rsreaders throughread_json_filefixes the install and uninstall paths and removes the duplication at the same time;run_traeneedsstrip_leading_bom(&input).trim()like its five siblings. -
src/hooks/mod.rs:42—is_trae_hook_commandre-implementsis_rtk_hook_commanddirectly above it, minus itsrtk.exearm, so an entry registered with the Windows spelling is invisible to both the idempotence probe and the uninstaller ("nothing to remove", entry left behind, and a reinstall then appends a duplicate). One line:is_rtk_hook_command(command, "trae"). -
src/hooks/init.rs:5171— the write loop is not all-or-nothing. With~/.trae-cnread-only,rtk init -g --agent traeexits 1 naming only.trae-cn, but~/.traehas already been patched and no message says so. Re-running is idempotent so nothing is lost — the error should just name what it applied. (The docstring's "malformed.traeor.trae-cnconfiguration cannot cause a partially-applied update" is accurate as written: I checked it, and preflight does cover malformed config. This is the write-failure case, not the malformed one.)
Checked and correct — no need to re-verify
process_trae_payloadpreservestool_input(clones it, replaces onlycommand;descriptionandtimeoutsurvive) and emits nothing on every defer path.- Omitting
permissionDecisionis right, andHookDecision::Denyis unreachable forHost::Traebecause its rule vectors are empty — so the missing deny audit entry is not observable. - Preflight-before-write against malformed config, backup-before-
atomic_writeordering, theresults/pathszip alignment,insert_trae_hook_entrynot mutating a rejected root, and--dry-runwriting nothing. - Unrelated hooks and unrelated top-level keys survive install and uninstall byte for byte.
- Sibling registration parity — nothing a sibling registers is missing for Trae.
Hypotheses built and dropped
rtk init --shownot reporting Trae — it does not report Codex, Gemini, Droid, Pi or OMP either, and the docs this PR adds never claim it does. Pre-existing, not this PR's job.- A
permissionDecisionorhookSpecificOutputfield-loss bug — ruled out by reading the clone and confirming it against the real output. error[E0308]: type mismatchin the test output — a pre-existing compile-fail test, present on unmodifieddevelop.- The 60% savings floor in
.claude/rules/cli-testing.md— stale (the floor is 20%), and it does not apply to a hook integration in any case.
Overlaps worth knowing about
- #3878 (
refactor(hooks): split init.rs into per-agent submodules, +11295/−11123) will conflict with this PR's +364 ininit.rs. My view: #3008 lands first — it is older, far smaller, and #3878 rebases mechanically. Nothing for you to do here. - #4028 touches the same
hooks/README.mdagent counter this PR bumps 11 → 12. Trivial; whichever lands second adjusts.
Your questions
- "Could you please review this PR and approve the fork CI run when you have a moment?" → Done on both counts: this is the review, and I have approved run
34968708266. Worth knowing that every round so far has been judged on local gates only — CI has never actually built this branch, so please watch that run.
One more thing
The PR closes no issues. #3234 ("Add hook support for Trae IDE (PreToolUse on RunCommand tool)") and #1676 ("[Agent] Add Trae.ai IDE support") are exactly what this implements — adding Closes #3234 and Closes #1676 to the description would close them on merge.
Next
Fix the matcher/timeout gating; ideally take the three optional items in the same push, and add the two Closes lines. Then let CI finish and I will approve.
Rounds: 1/3. Threads: 1 open (the blocking item), 0 resolved this round.
CI finished —
|
|
@KuSh The two blocking items and all three optional review items are addressed. Current head: ceae215; the branch includes
The PR description now reflects the current implementation and validation and includes Local macOS validation at ceae215: formatting, strict all-target/all-feature Clippy, all-feature tests (3832 passed, 8 ignored, 0 failed), diff whitespace and test-presence checks pass. The CLI reproductions of the review scenarios also pass. I have not verified a new Windows CI run and am not treating local results as cross-platform CI evidence. Please re-review the current head and approve/run the fork CI if approval is required. |
…by command Matching the RTK command alone settled the round-1 defect, where a matcher or timeout the user had edited made uninstall report "nothing to remove" and a reinstall append a duplicate. It also went one step too far in both directions, and this restores the two distinctions that were lost with it. Presence now only counts a registration that would run for the tool RTK serves: a group with no matcher, or one naming RunCommand among its `|`-separated tools. Anything unrecognised counts as not covering RunCommand, so an install adds an entry that fires rather than skipping one that never would. Removal stays matcher-blind, which is what round 1 asked for, but skips an entry whose explicit type is not `command`: a `prompt` entry is by construction the user's, and deleting it also took the group and its `matcher` and `description` keys. The "Already updated" list was also per-run, so the rerun the documentation recommends after a write failure reported "none" while a target was fully installed. It is now seeded from the preflight, and the uninstall path — which has the same two-target failure mode — reports it too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
KuSh
left a comment
There was a problem hiding this comment.
Round 2 of 3 — APPROVED
Claim: install and uninstall a Trae PreToolUse hook for RunCommand (project and global, plus ~/.trae-cn when it exists) and rewrite commands through rtk, leaving approval to Trae.
Scope: accept (frozen at round 1, not reopened).
CI: green on your ceae215f across all ten jobs, and green again on the fixup head 64a7010b.
Thank you for this one — every item came back with a regression test that actually pins it, and you reverted the interim RTK_AUDIT_DIR writer change rather than letting #1681 leak into scope. That was the right call.
Blocks merge — empty (was 2)
Both verified by mechanism, against a round-1 binary and a round-2 binary built in separate target directories and hash-checked:
ca248624 |
ceae215f |
|
|---|---|---|
uninstall, matcher RunCommand|WriteFile |
"nothing to remove", hook left live | removed |
reinstall after editing timeout to 60 |
two rtk hook trae groups |
one |
test (windows-latest) |
failed | green on the real runner |
All three optional items are fixed as well: BOM tolerance on all three sites (install, uninstall, hook stdin), both rtk.exe spellings recognised, and the partial-write diagnostic naming what had already been applied. I re-ran the whole round-1 matrix on the new head — idempotence, byte-identical user-content round-trip, CRLF, preflight-before-write, dry-run, malformed payloads, hook check parity with Codex — all still correct.
I also checked your new tests by mutation rather than by reading them: reintroducing the matcher gate fails test_trae_customized_hooks_… and test_trae_windows_command_…, and un-stripping the BOM fails trae_hook_rewrites_bom_prefixed_payloads. They are load-bearing. And I verified all four claims the new docs paragraph makes, including "rerun the install; completed targets will not receive duplicate hooks".
Fixup pushed — 64a7010b
Four things, and the first is mine rather than yours.
- Presence was matcher-blind. Asking you to match on the command alone fixed the round-1 defect and went one step too far:
rtk hook traeregistered only under a matcher that excludesRunCommandcounted as installed, sortk init --agent traereported "already present" and skipped adding a registration that would actually fire. Presence now counts a group with nomatcher, or one namingRunCommandamong its|-separated tools; anything unrecognised counts as not covering it, so we add an entry that fires rather than skip one that never would. Removal stays matcher-blind, which is what round 1 asked for. - Removal was type-blind. A user-authored
{"type": "prompt", "command": "rtk hook trae"}entry was deleted and its group pruned, taking the user'smatcheranddescriptionwith it. A.bakmade it recoverable, but apromptentry is by construction not something RTK installs. Removal now skips an entry whose explicittypeis notcommand, while still matching one with notypeat all — which is the common hand-written shape your own tests use. Already updatedwas per-run. A target that preflighted asAlreadyPresentnever enteredpending, so it could never be listed. On the rerun your docs recommend, the message saidAlready updated: nonewhile~/.trae/hooks.jsonwas fully installed — which is exactly the moment a user might hand-add a second entry. It is now seeded from the preflight.- Uninstall had the same two-target failure mode with no diagnostic, so it now reports it too. Your docs paragraph only promised this for install, so this was an asymmetry rather than a broken claim.
This is why the ("ReadFile", 5, "prompt") row is gone from test_trae_customized_hooks_… — it asserted 1 and 2 as intended behaviour, so the code could not change without it. Five tests replace it, and each of the four fixes is mutation-verified: reverting any one fails exactly one named test. Gates on the fixup are fmt clean, clippy clean, 3837 passed / 0 failed, and I re-ran every previously-fixed behaviour to confirm nothing regressed.
If you disagree with any of it — particularly 1, where the right answer depends on how Trae actually reads matcher, which you can check and I cannot — say so and I will take it back out. You know that surface better than I do.
Your questions
- "Please re-review the current head and approve/run the fork CI if approval is required." → Done. Approval is required on every push from a fork, so I approved the run on
ceae215fand again on64a7010b. Worth knowing for future PRs: until a maintainer approves it, a fork branch is never built, and a green CLA check alone does not mean CI passed.
Next
Nothing — approving. Closes #3234 and Closes #1676 are in place, so both close on merge.
Rounds: 2/3. Threads: 0 open, 1 resolved.
Summary
Add native Trae IDE integration:
rtk init --agent traeinstalls aPreToolUsehook forRunCommand, andrtk hook traerewrites supported commands while leaving approval to Trae.Closes #3234
Closes #1676
Behavior
.trae/hooks.json; global mode manages~/.trae/hooks.jsonand existing~/.trae-cn/hooks.json.rtk.exeand quoted executable paths. Customized matcher/timeout fields do not cause duplicate registration or prevent removal; unrelated hooks and configuration are preserved.command, and omitspermissionDecision. Unattestable shell constructs defer without rewriting.HOME. Rewrite/BOM integration coverage remains cross-platform. Shared audit-log path behavior is left to hook-audit log path uses Linux /tmp on Windows, doesn't honor XDG_DATA_HOME #1681.Validation
Includes
developat6d104308c56c0a51250f8a200e5056787128fb65via merge commit651441c.Local macOS validation on September 18, 2026, at
ceae215ff7841f5857dabd6295ddc92ba2d7b76d:cargo fmt --all --checkcargo clippy --all-targets --all-features -- -D warningscargo test --all-features: 3832 passed, 8 ignored, 0 failedgit diff --checkbash scripts/check-test-presence.sh upstream/developrtk.exeidempotency/removal; partial-write diagnostics and retry.Upstream cross-platform CI and maintainer approval remain pending; these local checks do not establish Windows CI success.