Repository navigation
chore(sync): merge upstream/develop (37 commits), upstream wins on 2 conflicts - #160
Merged
Merged
Conversation
Bash's default $IFS is space/tab/newline, never a bare CR. The lexer's newline arm already conflated a lone \r with \n/\r\n as a command separator, and its generic whitespace arm independently treated \r as a word boundary too — two places that had to independently get this right and didn't. Fixes both: - tokenize_inner's newline arm now only splits on \n or the \r of a real CRLF pair; a lone \r falls through and stays glued to its word. - tokenize_inner's whitespace arm no longer treats \r as a boundary, via a new shared is_word_boundary_whitespace() predicate. - permissions.rs::command_matches_pattern now normalizes through that same shared predicate instead of str::split_whitespace() (which is \r-inclusive), so an allow rule like "git status" can no longer auto-approve "git status\rrm -rf ~" by collapsing the embedded CR into a space before matching. - registry.rs's raw_breaks parity check updated to match the corrected newline model (counts \n and CRLF's \r, never a lone \r). Regression tests added in lexer.rs and permissions.rs reproducing the exact divergence.
tokenize_inner, QuoteScan (registry.rs), and shell_split each independently tracked "am I inside a quote" - QuoteScan's own comment already admitted it was reimplementing "the same model the lexer applies" from scratch. Extract the one rule all three actually encode (an unescaped quote char opens a span if none is open, only the same character closes it) into a single advance_quote_state() primitive, and drive all three from it: - tokenize_inner's Option<char> quote tracking now calls it instead of duplicating the open/close branching inline. - QuoteScan switches its internal (in_single, in_double) bool pair to the same Option<char> model, built on the same primitive - its external (offset, byte, in_single_before, in_double_before) API and all 5 downstream consumers (comment_start, bracket/test-bracket balance, ansi_c_quote_defeats_lexer, quotes_balanced) are unchanged. - shell_split switches from its own bool-pair toggle to the same primitive, as a byproduct simplifying its match arms to one merged case. Behavior-preserving: all 2708 existing tests pass unchanged, including the ~750 lines of lexer.rs quote/escape coverage - a future quote-state correction now only needs to happen once.
split_token_spans was a from-scratch, quote-blind whitespace splitter with a single caller (golangci-lint's global-flag pre-processing). Replace it with the lexer's tokenize(), reusing its already-tracked byte offsets - genuine dedup, and a real fix: a quoted global-flag value containing a space (`--config "a path/x.yml"`) used to get mis-split at the space inside the quotes, which made parse_golangci_run_parts miss the `run` subcommand entirely and fall back to leaving the command unclassified. New regression test covers the quoted-value case; all other existing golangci-lint classification tests pass unchanged.
split_for_permissions (permission gate), split_on_operators / split_command_chain (analytics/discovery classification), and rewrite_compound's inline token walk (actual rewrite) all segment the same kind of compound-command string, but deliberately differently: pipe-stop behavior, whether background `&` or `(`/`)` grouping counts as a boundary, and whether a trailing redirect gets truncated all diverge across the three. That's intentional - the permission gate must stay the most conservative - but it was undocumented and untested as a set, so a future edit to any one of them could silently drift further from the other two without anything catching it. - Cross-referencing doc comments on all three functions, including a comparison table on split_for_permissions. - A new segmenter_consistency test module in registry.rs pinning today's actual, verified output for all three across four representative inputs (background &, subshell grouping, pipe+&&, redirect-in-segment) - including a real quirk this surfaced: in `(git status; cargo build)`, the rewritten output only prefixes "cargo build", not "git status", because the leading `(` glued to the first segment defeats rewrite_segment's own command matching while the trailing `)` on the second segment doesn't. Pinned as documented existing behavior, not changed here. No behavior change; pure documentation and test coverage.
Two follow-ups from /code-review high on this branch:
1. parse_golangci_run_parts's switch to the full shell tokenize() (Phase
3) fixed the intended quoted-value bug but introduced a real
regression: tokenize() splits unquoted shell metacharacters (*, ?,
`, (, ), {, }, !) into their own tokens even outside quotes, so an
unquoted glob value like `--config *.yml` desynced the flag-skip
loop and got the whole command misclassified as Unsupported.
split_token_spans's actual job - "was there a space here", not full
shell syntax - is genuinely different from tokenize()'s, so it
should not have been replaced by it. Restored split_token_spans as
its own function, now built on the shared advance_quote_state /
is_word_boundary_whitespace primitives instead of being quote-blind,
which is what actually fixes the original quoted-value bug without
the metacharacter regression. New regression test for the unquoted
case.
2. Extracted is_crlf_at() and used it in both tokenize_inner's
newline-operator guard and registry.rs's raw_breaks parity check -
this exact duplication (two independent "is this \r part of a CRLF
pair" checks) was flagged as a follow-up during the PR rtk-ai#3600 review
that prompted this whole consolidation, and had been reintroduced
here without actually being fixed.
All 2714 tests pass, clippy clean.
…s on it
Even after the previous consolidation, split_token_spans and shell_split
each still ran their own scanning loop over the raw command string,
just now sharing the same quote/whitespace *rules* with tokenize_inner
rather than the same *scan*. The gap between "one lexer token" and
"one bash word" is exactly the tokens tokenize() splits apart (e.g.
Shellism("*") + Arg(".yml") for an unquoted "*.yml") that sit with no
gap between them in the original string - bash sees one word there.
Add coalesce_words(cmd, tokens): merges directly-adjacent tokens from
tokenize()'s output into single words. Rebuild split_token_spans as a
one-line wrapper around tokenize() + coalesce_words, removing its own
quote-aware scanning loop entirely - golangci-lint's flag parser now
runs zero bespoke scanning code.
Existing quoted-value and unquoted-glob golangci-lint regression tests
(added when this exact call site regressed once already) pass
unchanged. All 2716 tests green, clippy clean.
shell_split's per-char scanning loop duplicated the same quote/escape handling tokenize_inner already does, differing only in two genuinely distinct concerns: it splits on whitespace only (not shell operators), and it resolves quotes/escapes into argv-ready text rather than preserving them. Split that into two composable pieces instead of one bespoke loop: - resolve_word_text(): given one coalesced word's raw text, strips quote characters and resolves backslash escapes - the inverse of what tokenize_inner preserves, built on the same advance_quote_state so it can't drift from the tokenizer's own quote model. - shell_split() becomes tokenize() + coalesce_words() (from the previous commit) + resolve_word_text() per word - no scanning of its own. All 15 existing shell_split tests pass unchanged (verified byte-for- byte before writing this commit). One real, intentional behavior refinement: word boundaries now come from the shared is_word_boundary_whitespace (bash's actual default $IFS - space, tab, newline) instead of shell_split's previous bespoke ' '|'\t'-only check, so an embedded unquoted newline is now correctly treated as a word boundary. Covered by a new explicit test, since this wouldn't have been caught by the existing suite otherwise. Also added a test combining an unquoted glob directly adjacent to a quoted segment (the same token-coalescing case that mattered for split_token_spans, exercised here through shell_split's quote-stripped output). This affects 3 real call sites: hooks/mod.rs::is_claude_hook_command, registry.rs::search_uses_pattern_file, and main.rs's `rtk proxy '...'` argv construction (the highest-risk one - it directly controls what gets exec'd). All covered by the existing suite, all green. 2718 tests pass, clippy clean.
Add `--agent omp` to `rtk init` (with `-g`, `--uninstall`, `--show`) for the Oh My Pi coding agent (https://github.com/can1357/oh-my-pi). OMP loads the same `hooks/pi/rtk.ts` extension via its built-in legacy-pi-compat layer, which remaps the Pi package imports to OMP's bundled equivalent — so no separate OMP implementation is needed and the rewrite behavior stays byte-identical (mutualization). - Local scope: <project>/.omp/extensions/rtk.ts - Global scope: ~/.omp/agent/extensions/rtk.ts - `--uninstall` is three-way safe: missing → no-op, stock content → removed, modified RTK content → bail with manual-removal guidance - `--show` reports both scopes (installed / stock / modified / absent) Co-authored-by: makoMakoGo <makoMakoGo@users.noreply.github.com>
The extension is shared with OMP via its legacy-pi-compat layer. Add
guarded helpers that are strict no-ops on Pi: a persistent
"RTK disabled: <reason>" session status registered on session_start
(OMP wipes one-shot notify toasts on the initial render) and a
setLabel("RTK") UI label set before the version probe, so the
extension is identifiable in the session UI even when rtk is missing
or too old.
Co-authored-by: makoMakoGo <makoMakoGo@users.noreply.github.com>
- Restrict is_word_boundary_whitespace to bash's actual $IFS (space/tab/newline) instead of Rust's char::is_whitespace(), which wrongly counts non-IFS Unicode whitespace like NBSP as a word boundary and broke shell_split on pasted commands containing it. - Restore the permission gate's conservative lone-CR segmentation. tokenize_inner takes a NewlineMode (None/Bash/Conservative) instead of a bool, so split_for_permissions can opt into treating a bare \r as a command separator (matching pre-consolidation behavior) while every other caller keeps the bash-accurate "lone CR stays glued to its word" rule. The mode lives inside the shared char loop, so it stays quote/escape-aware and doesn't split inside quoted text. - Trim doc comments down to their essential rationale: dropped references to review tooling/history, and cut multi-paragraph explanations to one or two lines, letting the code carry the rest.
Per review feedback on rtk-ai#3681 (document new shared systems in their module README): lexer.rs's public functions are consumed well outside discover/ (hooks/permissions.rs, hooks/mod.rs, main.rs's rtk proxy), so document them as shared infrastructure rather than leaving them as an internal implementation detail.
# Conflicts: # hooks/README.md
…quotes shell_split resolved `\` as an escape everywhere outside single quotes, so every backslash in a double-quoted Windows path was eaten: `"C:\Program Files\rtk.exe"` came back as `C:Program Filesrtk.exe`. Bash only lets `\` escape `$`, `` ` ``, `"`, `\` or a newline inside double quotes; before anything else it is a literal character. Match that, which fixes quoted Windows paths for every shell_split caller at once (hooks/mod.rs's hook-install detection, rtk proxy's argv construction, registry.rs::search_uses_pattern_file) instead of per call site. Unquoted backslashes still escape, as bash does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`go install github.com/golangci/golangci-lint/cmd/golangci-lint@latest` resolves on the unversioned module path, which stops at v1.64.8 (March 2025, the last v1). v2 is published under /v2, so `@latest` never moved. Since runners picked up Go 1.27, that pinned v1.64.8 fails before it lints anything -- its vendored go/types rejects the newer export data: could not load export data: internal error in importing "internal/goarch" (export data version 4 is greater than maximum supported version 2) It writes 628 bytes to stderr, leaves stdout empty and exits 3, which is the 157-token benchmark row that turns the job red. Reproduced in golang:1.27 and verified there: the /v2 path installs v2.13.2 and lints the benchmark fixture cleanly. `@latest` is kept deliberately -- the /v2 in the path is what carries the fix, and a future major would publish under /v3 rather than being picked up silently. This is also the only configuration in which RTK's v2 output branch is exercised at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The golangci-lint row has never measured the filter. The fixture is clean Go, so golangci-lint prints an empty report, the raw side counts zero tokens and the row scores as skipped whatever rtk does -- it read 0 -> 8 for as long as it was green. Adds five functions that ignore returned errors, which errcheck reports. Verified in golang:1.27 against golangci-lint v2.13.2, running the fixture exactly as it appears here: golangci-lint 220 -> 35 GOOD (84%) go test 30 -> 8 GOOD (73%) unchanged go build 0 -> 0 SKIP unchanged go vet 0 -> 0 SKIP unchanged go build, go vet and go test stay clean, so the neighbouring rows keep measuring what they measured before. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…xture golangci_v2_json.txt is pretty-printed across 144 lines; golangci-lint emits its JSON on a single line, so the fixture was a shape real output never takes. The savings assertion counted whitespace-separated words, so the indentation was doing the work -- against real single-line output that same helper reads 40 "words" for 4 KB and reports 30% for a filter that is doing its job. Measured with estimate_tokens, the estimator RTK bills with, the real figure is 95.3%. Replaced with output captured from golangci-lint in golang:1.27: a clean v2.13.2 report, a v2.13.2 report carrying 3 errcheck and 3 ineffassign findings, and the 628 bytes v1.64.8 writes to stderr when its vendored go/types cannot read Go 1.27 export data. That last one backs a test the suite had no equivalent of: whatever the filter is handed -- empty, whitespace, error text, truncated JSON, v1 or v2 -- it must say something. Returning an empty string is what let `rtk golangci-lint run` print nothing at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`parse_major_version` required the major number to parse as a bare integer, so a version reported as `v2.13.2` fell through to the v1 fallback. RTK would then send v2 `--out-format=json`, a flag v2 removed, and the run would die on an unknown flag instead of linting. The prefix is not stable across builds: a `go install` of v1.64.8 reports `has version v1.64.8` while the same build of v2.13.2 reports `has version 2.13.2`, so neither spelling can be assumed. Today's CI install happens to take the unprefixed form and detects correctly -- this closes the case that does not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The lexer and permission-gate sides of the lone-CR rule are covered, but `rewrite_multiline_block` had no test for it: a quoted `\r` staying one command, a bare `\r` taking a single prefix while only the `\n` starts a new line, and the raw-break parity count ignoring a quoted lone `\r` instead of reading it as a line the lexer hid. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Treat pre-existing extensions without ownership state as uncertain, cover project-scope aliases, clean canonical sidecars after symlink removal, and classify protected extension overwrites as breaking. BREAKING CHANGE: non-interactive installs of modified or unrelated Pi/OMP extensions now require --auto-patch to approve overwrites.
The env-prefix stripper treated `sudo` like `env` / `VAR=val` and rewrote `sudo docker ps` into `sudo rtk docker ps`. That breaks at runtime: `rtk` lives in ~/.local/bin, which is not on sudo's secure_path, so the rewritten command fails with "rtk: command not found" under root (reported in #146). And where rtk *is* on secure_path, `sudo rtk` would run the whole rtk binary as root — an unnecessary-privilege footgun. Drop `sudo` from the env-prefix regex so sudo commands pass through untouched. The permission verdict path is unaffected (it never used this regex and already matches sudo commands as-is, e.g. `sudo:*` rules). env / VAR= prefixes and transparent builtins (noglob, command, …) still rewrite normally. Verified: `sudo docker ps` / `sudo -u root docker ps` / `sudo noglob git status` are no longer rewritten; `env FOO=bar docker ps`, `FOO=bar docker ps`, `noglob git status` still are. fmt/clippy clean, full test suite green. Refs #146 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ting names count_find_names and count_find_total treat every unrecognised line as a row of names. Since find began reporting hidden and gitignored matches, its output ends with `... (N filtered)` and a `[see remaining: ...]` pointer, so `rtk find '*' --max 10` counted 19 names (10 + 3 + 6 words) and the --max cap check failed. Skip both lines, as the header, `+N more` and `ext:` lines already are.
…ure-lines fix(benchmark): skip find's disclosure note and tee pointer when counting names
refactor(lexer): consolidate raw-command lexing behind shared primitives
fix(ci): unbreak the golangci-lint benchmark row
The "Env Prefix Handling" section of src/discover/README.md still listed `sudo` as a stripped prefix and used it in the chained example; two comments in registry.rs made the same claim. The `sudo head` case in test_head_tail_honour_exclude_commands no longer tested exclusion — it returns None whether or not `head` is excluded, because sudo blocks the rewrite outright. Dropped it; the `RUST_LOG=debug tail` case still covers the env-prefix path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
fix(rewrite): stop rewriting sudo commands (pass them through)
Prevent Windows checkout conversion from corrupting embedded hook fixtures and make the CRLF regression independent of the source checkout line endings.
feat(omp)!: add Oh My Pi (OMP) support
…conflicts Conflicted Sync of upstream/develop e53ec1c (dev-0.49.0-rc.407) into develop. - .gitattributes: upstream's hooks/**/*.ts -text block verbatim; the fork's *.sh text eol=lf rule survives (upstream has no .sh rule). - scripts/benchmark.sh: upstream's file verbatim. e2e11b7 (find-count regexes) is superseded by upstream d952a6b (PR rtk-ai#3863) and 1c0437d (2>&1 on the rtk side of the Go rows) by upstream's golangci-lint v2 move; both duplicates. - src/hooks/init.rs: fork-authored amendment appending the three fork revisions of hooks/pi/rtk.ts (upstream's file plus the -- terminator) to KNOWN_PI_PLUGIN_HASHES, so the new history test passes and `rtk init --agent pi --uninstall` recognises a fork-installed extension. - scripts/fork-delta.sh: HEALED_SHAS entry for e2e11b7; landing page delta regenerated (60 fixes). Writeup: claudedocs/sync-conflict-2026-09-05.md Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
1 of 3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Conflicted Sync of
upstream/develope53ec1c(dev-0.49.0-rc.407; 37 commits, 31 non-merge, since the 2026-09-03 sync). Two conflicts, one gate failure, one fork-authored amendment. Full writeup:claudedocs/sync-conflict-2026-09-05.md.Commits
fb12c83No cherry-picks; the 37 incoming commits keep their upstream authorship through the merge. No change to
Cargo.toml,build.rs, orCargo.lockin the range.Resolution — upstream wins
.gitattributes— upstream'shooks/**/*.ts -textblock verbatim. The fork's*.sh text eol=lfrule (a45d0d9) survives:git show upstream/develop:.gitattributes | grep shis blank.scripts/benchmark.sh— upstream's file, blob-identical. Two fork changes dropped as duplicates:e2e11b7(fork fix(benchmark): count find names only, not the disclosure note and tee pointer #158) find-count regexes → upstream merged Adrien Eppling'sd952a6b(fix(benchmark): skip find's disclosure note and tee pointer when counting names rtk-ai/rtk#3863) for the same lines. Added toHEALED_SHAS.1c0437d(fork fix(ci): install cargo-audit with --locked #157)2>&1on the rtk side of the Go rows → upstream solved the golangci-lint v1 / Go 1.27 failure its own way (v2 in CI4db7b15, fixture with findings0731055, v-prefixed version parsec730255). It auto-merged;--theirsdropped it deliberately..github/workflows/ci.ymlauto-merged: fork push trigger, semgrep-baseline fallback andcargo install cargo-audit --lockedkept; upstream'sgolangci-lint/v2install andfetch-depth: 0taken.Gate failure → fork amendment (
src/hooks/init.rs)Upstream added
KNOWN_PI_PLUGIN_HASHES(ad51059) andtest_all_git_pi_plugin_revisions_are_allowlisted(157d5c2), which walks every git revision ofhooks/pi/rtk.ts. The fork's Pi hook is upstream's plus the--terminator (8253401, fork #36). Still additive, on the real binary:Upstream's
Rewritehas onlytrailing_var_arg/allow_hyphen_values, nodisable_help_flag. So the terminator stays, and the three fork blob hashes are appended to the allowlist (comment marks them as fork revisions; upstream's ten entries untouched and in order):dfe5632d…eb56dd08+--8253401through the 2026-08 syncs46f41389…3eb16108+--ee9e9d9throughdevelop8a496cff…5e80e811+--Repro, real binaries
Before = release build of the merged tree without the amendment; after = with it. Local scope,
RTK_DB_PATHscratch. "stock" isdevelop'shooks/pi/rtk.ts(what a fork release installs); "modified" adds a// user modificationline.Unit-level before, from the first gate run:
current Pi extension hash 8a496cff… is missing from KNOWN_PI_PLUGIN_HASHESandPi extension revision 0728c555… has unallowlisted hash dfe5632d….Quality gate
x64 host toolchain:
cargo fmt --allclean ·cargo clippy --all-targets0 warnings ·cargo test --all3288 unit tests passed, 0 failed, 8 ignored; all integration suites green incl. upstream's newomp_init_test.git diff --cached --checkclean.Review round
Independent read-only reviewer over the staged resolution, 11 items, all passed: no markers/mojibake;
.gitattributesandci.ymlare upstream's plus the documented fork hunks;benchmark.shblob-identical;hooks/pi/rtk.tsdiffers by the--hunk alone;init.rsvs upstream is byte-for-byte the fork's pre-existing PowerShell-matcher divergence plus the allowlist addition; all three hashes recomputed independently and all 62 revisions the history test walks are allowlisted. No findings; two writeup nits taken.Fork Delta
scripts/fork-delta.shregenerated: 60 fixes (e2e11b7healed; the two #159 stderr commits the page had not yet picked up are now listed).Deliberately not done
cargo-audit --lockedhalf. Trimming it is fix(ci): make develop CI green — benchmark find caps, cargo-audit --locked rtk-ai/rtk#3861 work.--terminator has no upstream PR (upstream fix(hook): prevent flag injection in rtk-rewrite hooks (#1350) rtk-ai/rtk#2475 closed unmerged;8253401is fork-authored). Original-fix obligation, not in the Upstream PRs owed (deferred, not cancelled) #84 ledger yet.🤖 Generated with Claude Code