Skip to content

fix(fork): follow upstream's design decisions after the 2026-10-09 sync - #199

Open
kylehgc wants to merge 17 commits into
developfrom
fix/post-sync-2026-10-09-alignment
Open

kylehgc wants to merge 17 commits into
developfrom
fix/post-sync-2026-10-09-alignment

Conversation

@kylehgc

@kylehgc kylehgc commented Oct 9, 2026

Copy link
Copy Markdown
Owner

Follow-up to the 2026-10-09 Conflicted Sync (fork PR #168). That PR left one item unaudited: fork code that auto-merged outside the conflict hunks. Auditing it found two kinds of difference from upstream. Gaps that upstream has not addressed stay. Behaviour upstream decided on purpose now follows upstream.

Dropped: upstream decided this on purpose

Fork behaviour Upstream's decision Result
Claude hook sets permissionDecision: "ask" on Default/Ask rewrites (e93cde8) KuSh on rtk-ai#3018: upstream omits it on purpose so Claude Code asks and can remember the answer. rtk-ai#3031 was closed as no longer applicable. hook_cmd.rs payload handling is upstream's again
Fallback for hook payloads keyed by input (c8d6e28) rtk-ai#2535: no Claude Code hook event sends that payload, and the fallback widens what can trigger the hook removed; tests/hook_proof_test.rs and both of its claims withdrawn
rtk init also registers a PowerShell matcher (cc5b6d3) rtk-ai#2075: passing PowerShell through the bash rewriter mangles \ paths, takes over aliases like ls/ps/curl, and bypasses PowerShell deny rules init/claude.rs, init/mod.rs byte-identical to e4f0509
-- before "$CMD" in the legacy hooks/{claude,cursor}/rtk-rewrite.sh (7db14a3) rtk-ai#2475 was closed as legacy-only, with no more contributions wanted there. Upstream never had -- there. #168's claim that c6f484f dropped it was wrong and is corrected in the sync doc. byte-identical to e4f0509
Prettier filter reads the stderr report and summarises it on stdout (fork PR #117) rtk-ai#3772: upstream forwards that report on its own stream. KuSh chose rtk-ai#3571 for the remaining rtk format case. upstream's filter, plus an adoption of rtk-ai#3571

Kept or fixed: gaps upstream has not decided

  • git machine-output passthrough (46d16ea by 李冠辰, from fix(git): keep machine output raw rtk-ai/rtk#2573, and da407dc). This branch first dropped it (ab5d0f5) on a misreading of KuSh's close of fix(git): keep machine output raw rtk-ai/rtk#2573, then reverted that (afd4595): he lists the porcelain and --pretty=format: newlines as open bugs, and his "log --format=%H already prints exactly what git prints" holds only for short logs. On e4f0509, git log --format=%H silently stops at 50 commits, and git log -z --format=%H -5 comes back as 121 bytes of 205. Both cases are now in the proof test. KuSh's open fix(git): announce the cap on the filtered git log path rtk-ai/rtk#4152 would keep user formats capped but announce the cap; once it merges, the log half becomes a design difference and goes. That trigger is recorded in the sync doc. Related: without the passthrough, status -z goes through with_trailing_newline, and upstream's open fix(git): keep the trailing newline on filtered 'git status' output rtk-ai/rtk#3682 then adds a stray newline after the last NUL. I reproduced that on a build of its head and commented there.
  • Tests:
    • detect_linter's tests no longer create directories in the checkout.
    • The hermes Windows fake now expects the -- the plugin sends.
    • The adopted prettier test now launches rtk through common::rtk_command().
  • CI: stale.yml runs only on rtk-ai/rtk. On the fork the job's token step failed every week, and falling back to GITHUB_TOKEN would start closing the fork's own issues and PRs.

Commits

Commit Author Kind
c14c608 revert(hooks): drop the Claude ask decision and the input-key fallback kylehgc fork
42edde8 revert(init): drop the PowerShell matcher from rtk init kylehgc fork
88566c4 revert(hooks): restore upstream's legacy shell-script hooks kylehgc fork
ab5d0f5 fix(git): follow upstream on log formats, keep -z NUL-terminated kylehgc fork, reverted by afd4595
2abf372 test(lint): stop the detect_linter tests touching the checkout kylehgc fork
439a9dc test(hermes): expect -- in the Windows fake rtk invocation kylehgc fork
fb91e9c ci(stale): run the stale job only on rtk-ai/rtk kylehgc fork
3f2275e revert(prettier): restore upstream's prettier filter kylehgc fork
3d27402 fix(prettier): never report a failed run as formatted guyoron1 cherry-pick of rtk-ai#3571 (aa5f31e9)
89df848 test(prettier): spawn rtk through common::rtk_command() kylehgc amendment
9cebd10 docs(fork): record the design alignment in claims, delta and sync notes kylehgc fork
afd4595 Revert "fix(git): follow upstream on log formats, keep -z NUL-terminated" kylehgc fork
09016a5 test(git): pin the stash help passthrough on the path run() takes kylehgc fork
67106bd test(git): prove log -z and uncapped --format reach the caller whole kylehgc fork
57e09a9, 51bf9ae docs(fork): keep the passthrough, with its trigger; heal the reverted commit kylehgc fork

The cherry-pick is byte-identical to aa5f31e9 except for one conflict line. The tracked label uses &display_args(args) (upstream's current form, which the sync brought in) instead of &args.join(" "), plus that function's import. The amendment is needed because the repo's own isolation scan, tests_that_spawn_rtk_isolate_their_data, fails on CARGO_BIN_EXE_rtk in tests/prettier_failed_run_test.rs. I watched it fail before the change.

Repro: release builds, upstream e4f0509 vs this branch

The prettier --check here is a fake: it prints Checking formatting... on stdout and [warn] src/bad.js on stderr, then exits 1.

upstream:  rtk format prettier --check src/bad.js   (exit 1)
           Prettier: All files formatted correctly
branch:    rtk format prettier --check src/bad.js   (exit 1)
           Prettier: 1 files need formatting
           1. src/bad.js
both:      rtk prettier --check src/bad.js -> stdout "Checking formatting...",
           report on stderr, exit 1

Machine output compared byte for byte with native git. The first block is a scratch repo; the last two rows are this checkout's full history:

                                      upstream  branch
git status --porcelain                DIFFERS   same
git status --porcelain=v2             DIFFERS   same
git status -z                         same      same
git log --format=%H -3                same      same
git log --pretty=format:%H -3         DIFFERS   same
git log -z --name-only -3             same      same
git diff --name-only/--name-status/--numstat/--raw   same  same

                              native   upstream   branch
git log -z --format=%H -5        205        121      205
git log --format=%H           107789       2050   107789   (upstream: 50 commits, no notice)

Hook and init output are identical to upstream on both binaries:

rtk hook claude  (git status, default mode)
  {"hookSpecificOutput":{"hookEventName":"PreToolUse","permissionDecisionReason":"RTK auto-rewrite","updatedInput":{"command":"rtk git status"}}}
rtk hook claude  ("input"-keyed payload)  -> no output, exit 0
rtk init -g --auto-patch  -> "matcher": "Bash" only
rtk rewrite "git log | wc -l"  -> exit 1 (pipeline producer stays raw)

Quality gate

Windows x64 host toolchain, on 51bf9ae (the head; the origin/develop merge touches only the README delta block).

  • cargo fmt --all -- --check: clean
  • cargo clippy --all-targets: 0 warnings
  • cargo test --all: 0 failures (4049 unit tests plus every integration suite)
  • cargo build --release: ok

tests/prettier_failed_run_test.rs and tests/stderr_only_failure_test.rs are #![cfg(unix)], so Linux CI is their first run.

Not done here

🤖 Generated with Claude Code

kylehgc and others added 17 commits October 9, 2026 12:58
Both were the fork's opinion about behaviour upstream decided on purpose.

- `permissionDecision: "ask"` on a Default/Ask rewrite: upstream omits the
  field so Claude Code prompts and can remember the answer (KuSh on
  rtk-ai#3018; rtk-ai#3031 closed as no longer applicable).
- `input`-keyed payload fallback: no Claude Code hook event sends that shape;
  declined on rtk-ai#2535 as unreachable and widening the trigger surface.

hook_cmd.rs's payload handling is upstream's again. tests/hook_proof_test.rs
proved only these two claims and goes with them. The already-rtk deny path
(rtk-ai#3195) and the stdin-stall fail-open (rtk-ai#2565) stay.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Upstream deliberately registers the Claude hook for Bash only. Routing
PowerShell text through the bash rewriter mangles `\` paths, takes over
PowerShell aliases (ps, ls, curl) and bypasses PowerShell deny rules (KuSh on
rtk-ai#2075). Both files are byte-identical to upstream e4f0509 again.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The fork passed `--` to `rtk rewrite` in the deprecated shell-script hooks.
Upstream closed that fix (rtk-ai#2475, issue rtk-ai#1350) as legacy-only
and asked for no more contributions there. Byte-identical to e4f0509 again.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Drop the fork's machine-output passthrough for `status`/`log`. Upstream already
prints raw for --name-only/--name-status/--numstat/--raw/-z/--format=%H, and
deliberately sends user `--pretty=format:` through filter_log_output's line
truncation (KuSh on rtk-ai#2573). The machine-output proof test drops the
`--pretty=format:` case accordingly.

Keep the one real gap: upstream drops the final newline from
`status --porcelain` (v1 and v2). `with_trailing_newline` restores it, but it
also appended `\n` after `-z`'s final NUL, a stray record for `xargs -0`. It
now leaves NUL-terminated output alone; the new unit test was red before.

Also rewrite the stale `user_args` doc and its test around the stash help
passthrough it actually serves.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The fork's existing-path check in detect_linter read `Path::new(&args[0])`
against the process cwd, so its tests leaned on the repo having `src/` and one
created (and removed) a `biome/` directory in the developer's checkout. Route
the check through user_dirs::in_working_dir, which is the same relative path in
production and the isolated project under test, and run both tests in a temp
project via test_isolation::enter, per upstream's machine-independence rules.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The plugin calls `rtk rewrite -- <cmd>`; the Windows fake still expected the
argv without `--`, so the installed-flow test failed on Windows.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The stale policy is upstream's and needs upstream's App secrets; on the fork
the token step failed every week. Falling back to GITHUB_TOKEN would instead
start closing the fork's own issues and PRs.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The fork read prettier's stderr report and summarised it on stdout (fork PR
#117). Upstream fixed the same report loss its own way in rtk-ai#3772: the
runner forwards the stderr report on its own stream, and KuSh picked
rtk-ai#3571 for the remaining `rtk format` case. Take upstream's filter and
its stderr-stream test verbatim; the next commit adopts rtk-ai#3571.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The filter printed "All files formatted correctly" for any output that
had no parsable file list. Prettier >= 1.19 writes its --check findings
("[warn] <file>", "[error] ...") to stderr, so a syntax error or a
non-matching pattern (exit 2), and a failing check seen through
`rtk format`, had no list to parse and came out as formatted next to a
non-zero exit code.

- give the filter the exit code: a failing run with no parsable file
  list passes what prettier printed through instead of a success line
- recognise "[warn] <file>" lines, which `rtk format` hands the filter
  because it filters stdout and stderr together
- `rtk prettier` keeps filtering stdout only and forwarding stderr, so
  prettier's own diagnostics still reach the user untouched
Amendment to the adopted rtk-ai#3571 commit. Its integration test spawned
`CARGO_BIN_EXE_rtk` directly, so the child wrote into the developer's real rtk
data directory, and core::test_isolation's source scan
(tests_that_spawn_rtk_isolate_their_data) fails the suite on it, on every
platform, though the test itself is unix-only. Same shape as the sibling
stderr_only_failure_test.rs.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- fork-claims.tsv: withdraw the ask-decision and input-key claims with their
  code; the machine-output claim now names the one gap left, the
  `status --porcelain` final newline.
- fork-delta.sh: audit the commits this alignment and the sync superseded into
  HEALED_SHAS, each with its reason; regenerate the delta block (36 fixes).
- README: the fidelity prose cited failing-test stderr, which upstream now
  forwards (rtk-ai#3772); name three gaps that still have proof tests.
- sync-conflict-2026-10-09.md: correct the claim that upstream dropped the
  hook scripts' `--` in c6f484f (it never had it), and record the follow-up.
- fork-improvements-candidates-2026-10-09.md: rewritten after the A/B pass;
  every verified gap already has an upstream PR.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…-10-09-alignment

# Conflicts:
#	.github/README.md
…ted"

This reverts commit ab5d0f5. It dropped the machine-output passthrough
(李冠辰's 46d16ea, from rtk-ai#2573, and da407dc) on a misreading of
KuSh's close of that PR. He did not call user log formats a design choice.
He listed the `--pretty=format:` newline and the porcelain newline as
remaining bugs ("an issue or a narrow PR ... would be welcome"), and said
`log --format=%H` already prints exactly what git prints. That holds only for
short logs. Upstream e4f0509 and ab5d0f5 both:

  git log --format=%H          (60 commits)  native 2460 B, rtk 2050 B (-50 cap)
  git log -z --format=%H -5                  native  205 B, rtk  121 B
  git log --format=%s -3       (200-char subjects) native 615 B, rtk 363 B

The passthrough prints all three byte-for-byte. With it back, `status -z`
never reaches with_trailing_newline, so the NUL guard goes with the revert.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`run` now splits the restored stash region itself (upstream's restore), so
`user_args` only rebuilds git's argv for the help passthrough. The old doc and
test described the pre-sync double-dash restore, a path production no longer
takes. Rewrite both around split_stash_region.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Two cases the machine-output proof missed, both reproduced on upstream
e4f0509 in this checkout: `log -z --format=%H -5` comes back 121 bytes of 205
(the NUL-joined records are one line to the width cap), and `log --format=%H`
stops at 50 commits of the full history. fork-proof checks out full history,
so both discriminate there.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Correct the alignment notes after reverting ab5d0f5. The passthrough is not
a design difference yet: upstream silently caps `log --format=%H` at 50
commits and cuts `log -z` records today. KuSh's open rtk-ai#4152 would
keep user formats capped but announce it; when that merges, the log half
goes. Un-heal 46d16ea/da407dc, widen the machine-output claim to the log
cases its test now proves, and fix two stale sync-doc rows (prettier,
PowerShell) the review caught.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kylehgc

kylehgc commented Oct 9, 2026

Copy link
Copy Markdown
Owner Author

Review round 1. An independent review agent read the branch with upstream's code-reviewer.md standards, and each finding was checked on real binaries.

Finding Disposition
Dropping the git machine-output passthrough rested on a misreading of KuSh's rtk-ai#2573 close. On e4f0509, log --format=%H silently caps at 50 commits and log -z --format=%H -5 comes back as 121 of 205 bytes. Fixed: reverted (afd4595) and both cases added to the proof test (67106bd). The trigger for dropping it later is rtk-ai#4152 merging, recorded in the sync doc.
Two sync-doc rows (prettier, PowerShell) were stale, and the candidates doc gave the follow-up credit for the is_rtk_prefixed fix, which landed in the sync. Fixed in 57e09a9.
The README proof block still lists the withdrawn hook claims. That block is generated, so I'll dispatch fork-proof.yml after merge.
git_machine_output_test runs in the checkout, so on a clean checkout the porcelain cases are vacuous. This predates the branch. The unit tests cover it, and fork-proof's upstream worktree is dirty, so the claim still discriminates there. Not changed here.
hooks/README.md OpenCode section shows rtk rewrite --, but upstream's rtk.ts doesn't call it. Already on develop, out of scope.

The reviewer also confirmed the following:

  • Restored files are byte-identical to e4f0509.
  • hook_cmd.rs keeps only the deny path and the stdin fail-open.
  • The cherry-pick differs from aa5f31e9 only by display_args.
  • no_invented_stdout_diagnostic_when_the_tool_used_stderr still passes under fix(prettier): never report a failed run as formatted rtk-ai/rtk#3571 (traced through guard_stdout).
  • Every claim test and HEALED_SHAS entry resolves.

Re-gated on 51bf9ae: fmt, clippy and tests all green.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants