Repository navigation
Conversation
if there are failing checks in the PR, then `gh` will return a status of `1` and `rtk` will refuse to parse the output due to using `early_exit_on_failure`. remove the use of `early_exit_on_failure` so it can still parse the output. now if there is an error, we will still get the summarised output with all `0`s, and we'll also pass through the stderr. example if a repository does not exist: ``` CI Checks Summary: [ok] Passed: 0 [FAIL] Failed: 0 GraphQL: Could not resolve to a Repository with the name 'some-org/some-repo'. (repository) ``` with the error passed through to the output regardless, the error handling downstream by the LLM should still work well.
AVG(savings_pct) produced a simple unweighted mean that diluted the reported rate for high-volume commands — grep showed 14.8% while the true weighted rate was 98.6%, a 84-point gap. Replace AVG(savings_pct) with SUM(saved_tokens)*100.0/SUM(input_tokens) in the per-command SQL query so the rate reflects actual byte savings rather than an average of per-invocation percentages. Also: - Rename "Avg%" column header to "Rate" (more accurate label) - Add doc warning on CommandStats type alias against using AVG() - Add regression test proving weighted != unweighted on skewed data Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Florian BRUNIAUX <florian@bruniaux.com>
Signed-off-by: Florian BRUNIAUX <florian@bruniaux.com>
…avg_savings_per_command AVG(savings_pct) over rows produces an unweighted mean: a handful of 0%-savings passthrough calls dilute a filter that genuinely performs well on high-volume invocations. Same root cause as PR #891 (get_by_command). Changes: - low_savings_commands: AVG(savings_pct) → SUM(saved)*100/SUM(input). A command with 1 big call (95%) + 4 small calls (0%) now shows 94.6%, not 19%. Filters correctly excluded from the "needs improvement" list shipped in telemetry. - avg_savings_per_command: inner per-command rate is now weighted (SUM/SUM); outer average across command names stays unweighted (intentional, each filter counts once). Documented in TELEMETRY.md. - Two regression tests added (conn.execute direct inserts for DB isolation). - docs/TELEMETRY.md updated to reflect the weighted formula. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Florian BRUNIAUX <florian@bruniaux.com>
Auto-submit updated manifests to microsoft/winget-pkgs on stable releases using vedantmgoyal9/winget-releaser. Windows users can now install via `winget install rtk-ai.rtk`. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Move placement to make winget command easier to find Add winget commands into other documentation Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Running `rtk init -g` showed a spurious "No hook installed" warning because maybe_warn() ran before the init completed. Skip the warning for Init and Verify commands — they manage the hook themselves. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Ruiming Zhao <267482477+uuzzrm@users.noreply.github.com>
`filter_migrate_status` looked for whitespace after the `202...` id and fell
back to a hardcoded 20 bytes when there was none -- so an id that runs to the
end of the line slices past it:
Migration could not be applied at 2024-01-01T12:00:00
len 53, pos 34, end 20 -> line[34..54] -> end byte index 54 is out of bounds
Release builds set `panic = "abort"`, so `rtk prisma migrate status` dies and
the user loses the command output entirely -- the one thing a filter must never
do. A multibyte character straddling that offset panics the same way, on the
char boundary instead.
The fallback is the rest of the line, which is what the sibling
`filter_migrate_dev` has always used 70 lines above.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Groups matches by file, caps at 5/file and 50 total. --json passes through untouched. ~85% savings on a repo-wide test search. Closes #3856
Review nit: --json is the largest-payload path, so borrow rather than clone result.stdout when skipping the filter.
Adds the ast-grep row/section to README.md, what-rtk-covers.md, and FEATURES.md, matching how rtk grep/rg are documented there.
Every src/cmds/** filter that needs to know "does this flag consume the next token as its value, and where does -- end options" was reimplementing that question independently. git.rs alone racked up 9 one-off bugfix commits on its own LogArg/consumes_next_token_as_value/log_arg_tokens (hardcoded matches! lists edited piecemeal, -- handling gaps), and a codebase survey found the same shape of bug already hit or latent in search.rs, golangci_cmd.rs, and dotnet_cmd.rs. Adds src/core/arg_tokenizer.rs: a single tokenize() that classifies an already-restore_double_dash'd args slice into Long/Short/Positional/ DashDash tokens, linking each value token to the flag that consumes it and vice versa, with a caller-supplied `takes_value(kind, name)` predicate (each wrapped CLI's value-flag list stays exactly as data — only the token-walking around it is now shared). Digit-only short suffixes (git log/head/tail's `-N` shorthand) are kept as one token rather than decomposed into per-digit "flags", since no real CLI defines boolean digit flags. Migrates git.rs's run_log (LogArg/log_arg_tokens/consumes_next_token_as_value -> log_takes_value + tokenize) and run_checkout's four separate hand-rolled scanners (checkout_new_branch_arg/checkout_reset_branch_arg/ checkout_branch_arg/checkout_restored_count) onto one shared tokenization each, keeping their existing flag lists as the predicate body. No intended behavior change; arg_tokenizer's own unit tests encode a regression case for every prior git.rs bugfix commit, and git.rs's existing test suite (updated only where it asserted internal token shape, e.g. dash-free flag text) plus the real-process integration tests in tests/guard_integration_test.rs still pass unchanged. Follow-up (not in this commit): migrate search.rs, dotnet_cmd.rs (which has a latent restore_double_dash gap of its own), and golangci_cmd.rs onto the same tokenizer. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
search.rs's VALUE_FLAGS_SHORT/VALUE_FLAGS_LONG/ClusterResult/parse_cluster
reimplemented the same flag/value/-- classification arg_tokenizer now
centralizes. Migrates extract_pattern_path onto arg_tokenizer::tokenize,
keeping the flag lists as data (now dash-free, matching Token::text) and
the -e/--regexp-routes-to-patterns special case as extract_pattern_path's
own business logic (not something tokenize needs to know about).
Reconstructing the exact flags: Vec<String> shape (grep/rg-facing, not
just internal) needed one thing tokenize()'s per-character Short model
didn't originally have: knowing whether "-r" and "-n" were typed as one
cluster ("-rn", reconstructed glued) or two separate args ("-r" "-n"),
since existing tests pin the glued form for one-arg clusters. Added
Token::source_index (which original args slot a token came from) to
close that gap generically -- every Short token from one cluster shares
a source_index, a consumed separate-token value always has its own.
Also added Token::value() (attached-or-linked) as a small shared
convenience, used here and to simplify git.rs's now-redundant
linked_value/flag_value helpers.
Note: this is a reconstruction-format detail, not a behavior change --
grep/rg parse "-rn" and "-r -n" identically via standard getopt
clustering, and rtk's own has_short_flag() already checks via substring
so it doesn't care which form it sees either.
No intended behavior change. parse_cluster/ClusterResult's own unit
tests are removed (that internal API no longer exists); the existing
extract_pattern_path tests already exercise the same short-cluster/
value-taking/-e behavior end-to-end and all still pass unchanged.
Follow-up (not in this commit): golangci_cmd.rs, and dotnet_cmd.rs
(which has a latent restore_double_dash gap of its own).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
dotnet_cmd.rs never called args_utils::restore_double_dash despite Build/Test/Restore/Format all using trailing_var_arg = true in main.rs, the same clap-strips-`--` hazard git.rs/cargo_cmd.rs/search.rs already hit and fixed (issue #1215). Concretely: `rtk dotnet test -- FullyQualifiedName=MyFilter` lost its `--`, so inject_report_trx_into_args (VsTestBridge MTP mode) couldn't find one to reuse and appended a fresh `-- --report-trx` at the end instead — landing the user's MTP-runtime filter expression BEFORE the separator (misread as a dotnet-test-level arg) instead of after it, and stranding --report-trx with nothing following it. Added the missing restore_double_dash call to both entry points that feed `args` into `--`-sensitive logic (run_dotnet_with_binlog for build/test/restore, run_format). Regression test stubs `dotnet` on PATH (no dotnet SDK in this environment) to capture the real argv rtk would hand it; verified it fails without the fix and passes with it. Not migrated onto arg_tokenizer: investigated during this pass and dotnet's CLI grammar isn't POSIX/GNU-style — has_nologo_arg accepts single-dash multi-letter flags (`-nologo`, `/nologo`) as one atomic name rather than a short-flag cluster, and --logger:trx uses `:` as an attach separator, both incompatible with arg_tokenizer's current clustering model. A caller-selectable dialect (no clustering, `/` prefix, `:`-or-`=` attach) could support this later; not attempted here to keep this fix minimal and low-risk. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…lag checks
dotnet's CLI grammar isn't POSIX/GNU: `-flag`, `--flag`, and `/flag` are
all one atomic flag name (no short-flag clustering, e.g. `-nologo` must
stay one token, not decompose into per-char short flags), and a value
can attach via `:` as well as `=` (`--logger:trx`). Added Dialect::{Posix,
Msbuild} to arg_tokenizer, threaded through a new tokenize_dialect();
tokenize() itself is now a thin Dialect::Posix wrapper, so git.rs/search.rs
are untouched.
Migrated dotnet_cmd.rs's flag/value helpers (has_nologo_arg,
has_trx_logger_arg, has_results_directory_arg, has_report_arg,
has_report_trx_arg, extract_report_arg, has_verify_no_changes_arg,
has_write_mode_override, extract_results_directory_arg) onto
tokenize_dialect(Msbuild), replacing ~9 hand-rolled peekable/prefix
scans with two small shared helpers (dotnet_has_flag/dotnet_flag_value).
Two things the migration had to account for, not just carry over:
- dotnet_cmd.rs's checks were never `--`-boundary aware (--report-trx is
meaningful on either side of `--` depending on TestRunnerMode, and the
original code just flat-scanned the whole arg list) -- unlike git/grep,
where `--` genuinely ends option parsing. tokenize_dialect always stops
classifying at `--` (correct for git.rs/search.rs), so dotnet_cmd.rs's
new with_dotnet_tokens() strips `--` out before tokenizing to reproduce
the original scan-everything behavior. Caught by
test_vstest_bridge_respects_existing_report_trx (would otherwise
double-inject --report-trx when the user already placed it after --).
- has_nologo_arg previously matched only "-nologo"/"/nologo" (not
"--nologo"), and has_results_directory_arg/has_report_arg/
has_verify_no_changes_arg/has_write_mode_override previously matched
only the "--" form -- both incidental gaps from scanning raw strings
per-flag rather than a unified prefix model. Since dotnet's actual
System.CommandLine parser treats -/--/ / as interchangeable prefixes
for every option, recognizing all three forms uniformly for every flag
here is a correctness improvement, not a behavior risk.
inject_report_trx_into_args is untouched: it's pure `--`-relative Vec
splicing, not flag/value classification, so it doesn't need tokenizing.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…-options Discovered this refining the previous commit: dotnet's `--` doesn't mean what git/POSIX `--` means. Git's `--` ends option parsing -- everything after is a literal positional/pathspec, never a flag again. dotnet's `--` is an argument-*forwarding* boundary: what follows is still real flags, just meant for a different receiving parser (the VSTest/MTP test host), which can and does share flag names with dotnet test's own CLI (e.g. --logger, --results-directory are forwarded VSTest-console options, and --report-trx is meaningful on either side of `--` depending on TestRunnerMode::MtpVsTestBridge). The previous commit's dotnet_cmd.rs fix (with_dotnet_tokens stripping `--` out before tokenizing) reproduced the right observable behavior but for the wrong reason -- it papered over the mismatch in the caller instead of naming it. Moved the fix to where it actually belongs: tokenize_dialect now only stops classifying at `--` for Dialect::Posix; Dialect::Msbuild still emits a DashDash token (so callers can find `--`'s position) but keeps classifying flags normally past it. This also let dotnet_cmd.rs drop the owned-copy-plus-closure workaround entirely -- dotnet_tokens() is back to a plain function borrowing straight from the caller's args, same shape as every other tokenizer call site. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…okenizer find_subcommand_index/split_flag_name/golangci_flag_takes_separate_value hand-rolled the same flag/value classification arg_tokenizer now centralizes (GLOBAL_FLAGS_WITH_VALUE was its own one-off "which global flags take a separate value" list). Replaced with a single arg_tokenizer::tokenize(Dialect::Posix) call plus a small predicate (golangci_takes_value); find_subcommand_index now just walks the resulting tokens for the first free positional, using Token::source_index to translate back to the position in the original `args` for the caller (classify_invocation slices global_args/run_args off that index). No intended behavior change: all existing classify_invocation tests pass unchanged. Added two regression cases the prior hand-rolled scanner didn't have tests for: `-c value` (short flag consuming a separate-token value, previously untested) and `--` before any subcommand is found (must fall through to Passthrough, matching golangci-lint's own parser taking over from there). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
`rtk read <file> --head-lines N` read the whole file before slicing, so on a source with no end there was nothing to slice: `head -n 5 /dev/urandom` returns five lines instantly, and the rewritten form never returned at all. That path is reachable now that `head -n N` and `head --lines N` rewrite to it. Read in chunks and stop at the Nth newline, as head does, retrying an interrupted read the way the `fs::read` this replaces did. Byte-for-byte the same answer as before on a regular file -- across chunk boundaries, on CRLF endings and on an unterminated last line; the savings baseline comes from the size on disk, and a source that has no meaningful size claims none. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Forwarding stderr verbatim keeps a golangci-lint config error from vanishing, but it also hands back every `go: downloading …` line a cold module cache produces: a passing `go test` went from 3220 raw bytes to 3216 through rtk, where the stdout filter alone returns 24. Stderr still goes through whole whenever it is the report -- the command failed, or the filter had nothing to show for stdout -- and now also whenever capping would not pay: below the recovery store's floor, when there is no store to point at, and whenever the note and the hint would cost more than the lines they replace. Otherwise the last lines are kept and the rest goes behind the `[full output: …]` handle, which puts that same `go test` at 354 bytes with its warnings intact. The end, not the head: a tool resolves and downloads before it builds, so a cap on the head keeps the chatter and drops the two lines worth forwarding. The kept lines are sliced out of the original rather than re-joined, so CRLF endings survive. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…subcommands The caps counted the lines they held back inside a shown file, but a file the total cap skipped outright was reported as a file and nothing else: 255 of 387 lines on the module's own fixture were in no tally at all, with no way to read them. Count those lines too, and point at the full output with the same `[full output: …]` handle every other capping filter emits -- decided on the text that handle is part of, so a search that ends up printing raw never leaves a stored file with nothing pointing at it. Only `run` produces the `path:line:content` shape this module parses. `scan --report-style short` happens to parse as well and was being capped as if it were a match list, and `lsp` speaks a protocol over the stdin that capturing closes, so it served an editor an empty document. The subcommand is now read in two passes -- the options that may precede one, then `run`'s own grammar once the line is known to be a `run` -- and a line that names any other subcommand, or a `--stdin` run, goes through the passthrough, which inherits stdin. A line this module cannot identify is not filtered: `--color always lsp` reading as a `run` is what closed that stdin. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
hooks/codex/README.md documents that Codex requires it for updatedInput and that its native checks still run. The "deny branch is unreachable" part is true but is dead code, not a behaviour bug. And "dead code" is exercised by test_codex_shared_decision_preserves_host_approval_and_deny test and permissions::load_rules_for already states that ATM it is unreachable (even if it reads as reachable).
reachability is real: |
A windowed `git show <rev>:<path>` points at the rest of the file with `| tail -n +N`. The hook rewrites the bare `git show` in that hint back into `rtk git show`, which windows the dump a second time, so `tail` had only the hint itself left to print: 1 line where the reader asked for 1434. Name the command the hook leaves alone, as the `diff` size hints already do. The differential test now runs the command the hint names instead of simulating it, so a hint that cannot return the blob fails CI. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
fix(read): stop the head window reading past the lines it was asked for
fix(core): cap the stderr a stdout-only filter forwards on a clean run
fix(ast-grep): account for every match line and stop capturing other subcommands
fix(git): make the blob recovery hint work under RTK's own hook
fix(git): stop show reporting on HEAD and log capping in silence
…'s files `--codex` is the one init mode whose paths are the project root, where `RTK.md` is a name RTK does not own. Uninstall removed any file sitting there, so a project holding its own `RTK.md` lost it with no content check, backup or prompt; init overwrote one just as quietly. The file now says whose it is: init writes an `rtk-owned` line above the awareness payload and both sides read it on the first non-blank line, so a payload from another release is still recognised as RTK's -- comparing against the running build's copy would orphan RTK's own file on the first upgrade and leave a numbered backup on every one after -- while a marker a user pasted into the middle of their own notes does not hand the rest of the file over with it. A file without the line is the user's: init moves it to a free `.bak` sibling, uninstall keeps it. Installs predating the line are recognised too, so the change does not strand them. Up to v0.48.0 the payload opened with a heading naming RTK and the mode, which is enough on its own. The releases after it wrote the shared awareness text, whose headings name neither and which a user's own notes may open with, so those are matched whole instead, by a digest of the bytes that shipped -- frozen, because rewording the awareness text must not change which files uninstall recognises as RTK's own. That test is for the project root only. Under `--global` the file sits in the Codex home RTK created it in, where nothing else claims the name and every release before the marker wrote it there unmarked; reading the marker there would keep all of those files forever and back one up on the next install. The project paths are relative names RTK joins itself, so a symlinked `.codex` sent the new `hooks.json` write out of the project entirely -- and a planted `hooks.json.bak` took the existing hooks out through `fs::copy`, which follows a symlink at the destination, even with `.codex` a real directory. Both paths are now resolved link by link, up to a bound that makes a cycle terminate, and refused when the end of the chain lands outside. Resolving one link was not enough: `canonicalize` gives up at the first target that does not exist -- which it will, since RTK creates these files -- and hands back the unresolved path as if it sat where the chain broke, so neither a second hop nor a link above the file was ever looked at. A link standing where the file itself goes also has to sit inside the project rather than merely point there, since a write that cannot resolve its path replaces that link instead of following it. Only the hook is given up when the check refuses: `AGENTS.md` and `RTK.md` are demonstrably where uninstall left them, and refusing to clean them leaves artifacts with no command that removes them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| # Only commits with no behaviour change belong here. | ||
|
|
||
| # chore: reformat under rustfmt style edition 2024 | ||
| ad63139576f77767299ac1c0f4bca5b52fe80442 |
There was a problem hiding this comment.
This git commit will certainly have to change for the master to benefit from it
…nt-aware The containment walk resolved symlink chains with a loop of its own, and the `.bak` picker chose a slot by name alone. Both are shaped here so the Pi/OMP ownership work can build on them rather than growing a second copy of a security-relevant walk that can drift out of agreement with this one. A bare `Option` conflates three facts: "not a symlink", "a link I could not read", and "I gave up". Conflating the first two lets a `readlink` failure on a path `lstat` just called a symlink be compared as if it had resolved, so `SymlinkHop` and `SymlinkChain` keep them apart and `Resolution` carries the distinction out to the caller. The hop bound is inclusive: it counts hops followed, and an exclusive range stopped one short of the length the documentation promised. Containment keeps a check the resolver cannot make. `atomic_write` cannot canonicalize a chain whose end does not exist, so it writes at the path as the filesystem reads it, replacing the last link rather than following it -- which makes every link along the site's own chain a place the write can land, not just the far end. A link outside the project that currently points back in is one an attacker may own and re-aim, so the chain is judged link by link. Ancestor components are traversed rather than sited: whole directory trees hang off one on macOS, and siting those would refuse every absolute target under `/var`. `free_backup_slot` probes with `fs::read` so an unreadable slot counts as taken rather than being overwritten, and reuses a backup whose content already matches so a provisioning loop cannot consume a slot per run. Names are built by appending to the `OsString`: `with_extension` replaces an extension instead of extending it. `BackupSlot` keeps an unreadable source apart from a preserved one, because only a caller that copies needs that answer -- a rename carries content RTK cannot read. `CwdGuard` restores the working directory on the way out. Restoring by hand needs the call under test to return rather than panic, so one failing assertion left every later test inside a deleted `TempDir`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
fix(hooks): keep rtk init --codex inside the project and off the user's files
Re-verified after #4117–#41227 of the 10 items are solved. Confirmed by re-fuzzing, not just re-running the original repros: 1464 Solved: Partially fixed
Still openCodex hook asserts Telemetry admits negative-savings rows with the first argument attached — Fuzzing found no new regressions from the six fix PRs. It did surface a few pre-existing issues outside this release's scope, which I'll raise separately rather than hold the release on. |
fix(docs): add git repo badges
|
Thanks for re-fuzzing rather than re-running the original repros — the permutation counts are what make "solved" mean something here. All three remaining items have an answer; one of them I think changes the framing, so it is first and with the measurements attached.
|
… label `low_savings_commands` labelled each command with the first three words of `rtk_cmd`. That field is RTK's own, but most of what it holds is the user's command line, so the third word is routinely an operand: `rtk ls /usr/bin`, `rtk curl https://…`, `rtk grep <pattern>`, and the TOML filter path stores the user's whole invocation after its `rtk:toml` prefix. Over a real history database, 201 of 2606 labels carried a path, a URL, a flag value or a search term. `top_passthrough` had the same leak and was fixed by grouping on the tool alone. That is too coarse here, because every label begins with `rtk` and the figure is only worth sending per filter -- `rtk git log` and `rtk git status` have to stay apart. So the label keeps the words RTK chose: the prefix, the tool as a basename, and a third word only for a tool that routes by a subcommand of its own, and only when it is shaped like one. Over the same database that is 0 labels carrying an argument, with the per-subcommand granularity intact. The router list is a superset of the tools that declare subcommands in `discover::rules`, cross-checked by a test rather than read at runtime, since nothing in `core` depends on `discover`. A tool missing from it keeps two words, which loses granularity and never leaks. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
fix(tracking): keep the user's arguments out of the telemetry command label
Feats
Fix
Other