Every other run_* function that inspects args for "--" (run_diff,
run_checkout) calls args_utils::restore_double_dash() first to
re-insert the "--" that clap's trailing_var_arg strips out. run_log
never did, so requests_raw_log_output()/log_arg_tokens()'s own
take_while(|arg| *arg != "--") check was dead code in production:
by the time run_log saw args, the literal "--" was already gone.
Concretely, `rtk git log -- -p` lost its "--" and -p (a file
literally named -p, a valid pathspec) was misdetected as the real
patch flag, routing to raw passthrough instead of RTK's filtered
log. Added an end-to-end regression test (real rtk binary, real git
repo) alongside the existing git_log_patch_output_matches_raw_git
test, since restore_double_dash reads live process args and can't
be exercised by calling run_log's internals directly.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
requests_raw_log_output only recognized -p/-u/--patch variants as needing
the raw path. --stat, --numstat, --name-only, --name-status, --raw,
--shortstat, --dirstat, and --summary change git's raw output shape the
same way -p does, but weren't listed — RTK's injected --pretty=format +
---END--- markers can't coexist with them, so the diffstat/name-list block
got misparsed as the start of the next commit, mangling output silently
instead of just leaving it unfiltered.
Also share one log_arg_tokens() pass across run_log's flag/limit checks
instead of retokenizing per check, and drop a redundant "-n" match arm
already covered by consumes_next_token_as_value.
has_limit_flag, has_format_flag, wants_merges, and parse_user_limit scanned
args positionally with no awareness that a value belonging to --grep,
--author, etc. can itself look like a flag (-5, --pretty, --merges),
reproducing the same misdetection class this branch already fixed for -p.
Unify these into a single log_arg_tokens tokenizer shared by
requests_raw_log_output, real_flag_args, and parse_user_limit, which also
stops at the -- pathspec separator so a literal path like -5 after -- isn't
misread as a flag.
Cross-checked git-log(1)'s full option list against
consumes_next_token_as_value() (using git log <opt> <token> -1 against
real git 2.53.0 to see whether <token> gets swallowed as the option's
value or leaks through as a positional arg). --diff-algorithm and
--diff-filter both take a required, separate-token value
(`git log --diff-algorithm -p` rejects -p as an invalid algorithm
rather than treating it as the patch flag) but were missing from the
list, so a genuine -p right after either was misdetected as the
option's own value and the raw-patch request went undetected.
Every other bracket/optional-value option checked (--format, --pretty,
--stat, -M, -C, -B, etc.) is correctly attached-value-only and stays
out of the list.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Audited every option in consumes_next_token_as_value() against real
git 2.53.0 behavior (git log <opt> <token> -1, checking whether the
token gets swallowed as the option's value or leaks through as a
positional arg). --max-parents and --min-parents behave like -U:
`git log --max-parents 2` fails with "ambiguous argument '2'", so a
real -p right after them was being misread as their value.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
These options only accept an attached value (-U3, --unified=3,
--expand-tabs=4) — verified against git 2.53.0, where
`git log --expand-tabs 4` fails with "fatal: ambiguous argument '4'"
instead of treating 4 as the value. consumes_next_token_as_value()
was swallowing the next token for them anyway, so a real -p right
after one of these was misread as their value and the raw patch
request went undetected.
Addresses review feedback from @aeppling on #3575.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Address review nits on #3266:
- Collapse short_hash to format!("{:x}", Sha256::digest(..))[..6] per suggestion.
- Reword docs: 6 hex = 24 bits is not collision-resistant on its own (birthday
bound ~few thousand slugs); safety here comes from also requiring the same
readable prefix and epoch second.
`BENCH_DIR="$(pwd)/scripts/benchmark"` is a tracked directory: besides the
gitignored `unix/`, `rtk/` and `diff/` output dirs it holds the TypeScript
VM-benchmark harness (`run.ts`, `cleanup.ts`, `rebuild.ts`, `lib/*.ts`,
`cloud-init.yaml`). `rm -rf "$BENCH_DIR"` therefore wiped all 7 tracked files
from the working tree on every local run (`$CI` unset), which is exactly when
a contributor runs the benchmark before pushing.
Wipe only the three gitignored output subdirectories instead. Stale output is
still cleared between runs; the harness survives.
Follow-up to #3430 (rtk-ai/rtk#3430 review).
Review follow-up on #3430:
- Replace the IIFE closure in `CargoTestHandler::format_summary` with a private
`compute_test_summary` on the existing `impl CargoTestHandler`, and apply
`never_worse` at the call site, as suggested.
- Add `test_cargo_test_summary_uses_raw_when_summary_is_larger`: a one-line
compile failure whose "cargo test: N errors, ..." header is larger than the
raw output, so the guard must return the raw output.
RED→GREEN: the new test fails on upstream develop (returns the 105-byte header
form instead of the 62-byte raw), passes here. Full suite: 2612 unit + 78
integration tests pass; benchmark 0 negative (develop baseline: 1).
Review follow-up on #3430:
- Bind `python3 -m http.server 0` so the kernel picks a free port and read the
chosen one from the (unbuffered) server log, instead of hardcoding 8899 which
fails needlessly when that port is already in use.
- Make `cleanup_net_fixtures` failure-tolerant: it runs from an EXIT trap under
`set -e`, so `[ -n "$PID" ] && kill ...` aborted the whole handler whenever
`kill` failed (server already dead), leaking the fixture dir and downloads.
- Give the wget case an explicit skip line instead of silently disappearing.
wget rejects `file://` ("Unsupported scheme"), so there is no offline URL to
fall back to when the loopback server is unavailable.
RED (PR HEAD): fixture dir NOT removed when kill fails; server unusable with
8899 taken. GREEN: fixture dir removed; server up on an ephemeral port.
CargoTestHandler.format_summary could emit a compacted summary larger than
the raw output on tiny runs, same failure mode CargoBuildHandler already
guards. Wrap all return paths with never_worse so the raw output wins when
the summary would be bigger.
The remaining online calls (curl robots.txt + wget /json on mockhttp.org)
were both a network dependency and non-deterministic. Serve fixed local
fixtures over a loopback http.server so curl and wget get real
Content-Type headers (exercising JSON minification), fully offline. curl
falls back to file:// when python3 is unavailable. Clean up the server,
temp fixtures, and the ./data.json download on exit.
Inline evaluate_with_verdict() calls directly in unattestable_passthrough
tests instead of through a locally-named eval() alias, and move the two
verdict-to-outcome tests (allow/deny) up into the parent tests module since
they aren't about unattestable-construct passthrough.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The unattestable_passthrough tests called evaluate(), which calls
check_command() and reads the developer's Claude Code settings files
(.claude/settings.local.json, ~/.claude/settings.json). A local
`Bash(git *)` allow rule turned the expected Ask into Allow, so the two
rewrite assertions failed on that machine and passed everywhere else;
deny rules would likewise have broken the four Passthrough assertions.
Give evaluate() the same verdict-injection seam that permissions.rs
already exposes via check_command_with_rules: evaluate_with_verdict()
holds the decision logic and takes the verdict as a parameter, while
evaluate() stays the thin wrapper that looks it up. The tests pin
PermissionVerdict::Default, so they read no settings at all and assert
the exact outcome again rather than accepting either verdict.
Added coverage for the Allow and Deny verdicts so the mapping stays
tested in both directions without depending on host configuration.
Verified by running the test binary with HOME pointed at settings files
containing deny, ask, allow, and no rules: 8/8 pass in all four, and
with the repo's own settings.local.json in place.
Closes#3146
Moving call sites onto exec_capture swapped exit_code_from_output for
status_to_exit_code, which returns the same 128 + signal but drops the
stderr line explaining that the child was killed rather than exiting.
A command that dies to an OOM kill would just report 137 with no reason.
Route both capture helpers through one function that calls
exit_code_from_output, using the program name as the label so no call
site has to pass one. Covered by a test that terminates a child with
SIGTERM and asserts the 128 + signal code survives.
Addresses the inline review on #2717.
decode_process_output
- Decode a line at a time instead of reinterpreting the whole buffer at
the first bad byte. Valid UTF-8 lines keep their bytes; only lines that
fail UTF-8 validation go through the code page, so one stray byte no
longer mangles output that was almost entirely UTF-8. The line is the
unit because a byte run is not one: GB18030's four-byte sequences embed
bytes in the ASCII digit range, so any rule that ends a run below 0x80
splits them. \n cannot appear as a trail byte in any encoding handled
here, and a process does not switch encoding mid-line.
- A code page result is only accepted when it decodes cleanly, so a UTF-8
line with a corrupt byte falls back to lossy UTF-8 rather than mojibake.
- Replace the hand-written code page table with the codepage crate, as
suggested. That also fixes 54936, which was mapped to GBK and now
correctly resolves to gb18030.
- Add oem_cp for the legacy OEM/DOS pages (437, 850, 852, …) that plain
cmd.exe still defaults to in many locales. encoding_rs implements only
WHATWG encodings, so codepage alone returns None for them.
- Fall back to GetACP when GetConsoleOutputCP reports no console, which
is the piped case rtk normally runs in, and warn once instead of
falling back to lossy silently.
- Cache the code page lookup in a OnceLock.
- The mapping and the walk take the code page as a parameter, so they are
compiled and unit-tested on every platform rather than only Windows.
Call sites
- Route the remaining production sites through stream::exec_capture and
exec_capture_stdin rather than decoding at each one, so future callers
inherit decoding. git commit keeps inherited stdin via the _stdin
variant. Test-only sites go back to from_utf8_lossy: they assert on
rtk's own UTF-8 output, where a console code page has no meaning.
- Decode the streamed path (read_lines_lossy) too — the OEM/ANSI lines
its comment describes were still going straight to U+FFFD.
- curl keeps its body on from_utf8_lossy: a response body is a network
payload whose encoding comes from the HTTP charset, not the local
console, and non-UTF-8 bodies already take the binary passthrough for
#1087. Only curl's own stderr is code page decoded.
git commit summary parsing
- parse_commit_output sliced from byte 1, which panics when the first
line starts with a multi-byte character — git prints hook output before
its summary, and a lossily decoded line starts with a multi-byte
U+FFFD. Locate the bracket pair with find instead, so both indices are
character boundaries.
Verified: unit tests for the walk, GBK, gb18030, CP437/850, mixed lines,
truncated input and every byte value; a test pinning that output without
a code page stays byte-identical to from_utf8_lossy; and the Windows-only
lookup cross-compiled for x86_64-pc-windows-msvc.
requests_raw_log_output() matched bare -p/-u/--patch tokens anywhere in
the args, without checking whether the previous token was an option like
--grep or -S that consumes the next token as its value. `git log --grep
-p` searches commit messages for the literal string "-p" (verified
against real git 2.53.0: no diff output, identical to --grep=-p) but RTK
treated it as a patch request and skipped its own filtering/limit,
dumping raw uncapped git log output for what is actually a plain grep
search.
Skip the value token after any known value-taking git log/diff option
before checking for the patch flags.
requests_raw_log_output() scanned the full args slice for patch flags
like -p, so `git log -- -p` (a literal pathspec named -p) was
misdetected as a raw patch request and skipped RTK's normal filtering.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Review feedback (KuSh): the helper existed but most filter modules still
called String::from_utf8_lossy directly, so non-UTF-8 console output
(e.g. GBK on Chinese-locale Windows) was still mangled for most commands.
Wire decode_process_output through the remaining child-output call sites:
git, go, aws, curl (non-binary paths only — raw binary passthrough for
#1087 is untouched), system read, and discover registry. File-content
decoding (dotnet TRX XML, hook trust snippets) intentionally keeps
from_utf8_lossy since console code pages don't apply there.
Replace hand-written FFI extern block with windows-sys crate binding
for GetConsoleOutputCP to satisfy semgrep unsafe-block rule. Rewrite
test_decode_process_output_gbk to test encoding logic directly via
codepage_to_encoding(936) instead of relying on the CI runner's
actual console code page (which is not GBK on GitHub Actions).
On Windows with non-UTF-8 console code pages (e.g., GBK for Chinese
locale), child process output is mis-decoded by String::from_utf8_lossy,
producing mojibake. Add decode_process_output() that detects the console
output code page via GetConsoleOutputCP() and decodes with encoding_rs.
Replaces from_utf8_lossy in the core capture paths (exec_capture,
exec_capture_stdin, TOML filter path, proxy streaming path). Module-
specific call sites left for follow-up.
Fixes#2452
KuSh's review on this PR flagged two real issues in read_lines_lossy:
- Ok(0) | Err(_) => None treated a genuine I/O error the same as clean
EOF, silently truncating output on a real read failure -- exactly
the "failing build looks clean" class of bug this PR exists to fix,
just relocated rather than removed.
- A fresh Vec was allocated every iteration on a hot streaming path
instead of reusing std's own buffered split.
Replaced the hand-rolled read_until loop with BufReader::split(b'\n'),
per the reviewer's suggested implementation: reuses std instead of
reimplementing it (except CR-stripping, which split() doesn't do),
and now surfaces a genuine I/O error to stderr instead of silently
conflating it with EOF.
Added a test with a Read impl that yields good lines then a real
error, confirming the error path doesn't panic or hang and the
already-read lines are still preserved.
Two follow-ups to https://github.com/rtk-ai/rtk/pull/3391#pullrequestreview-4865940066:
1. Move the summary-verb mapping onto VibeHookPatchOutcome as
summary_verb() -> Option<&'static str>, returning None for Skipped.
The call site becomes 'else if let Some(v) = outcome.summary_verb()'
which collapses the guard and the match into a single decision point
and removes the unreachable!() branch. If a future variant is added,
the compiler forces a decision in summary_verb() and the caller
handles it naturally through the Option.
2. uninstall_vibe now prints a stderr warning when resolve_vibe_dir()
fails instead of silently returning Ok(()). Users asking to uninstall
no longer see an empty response when the home dir can't be resolved.
uninstall_gemini has the same swallow-and-return-Ok pattern; leaving
that untouched here to keep the diff scoped to Vibe, but the same
improvement would apply as a follow-up.
Both are quality improvements with no behavior change on the happy path.
The Semgrep security scan on PR #3391 flagged 2 new fs::remove_file calls
in uninstall_vibe_at as blocking findings under the filesystem-deletion
rule (WARNING severity, but the CI runs semgrep --error which promotes
all findings). Both calls are legitimate uninstall behavior:
- prompt file removal at src/hooks/init.rs:4697 — removes only
~/.vibe/prompts/rtk.md, which RTK installed itself.
- hooks.toml removal at src/hooks/init.rs:4714 — removes the file only
when it becomes empty after stripping the RTK entry, so no orphan
empty file is left behind.
Both suppressions follow the existing repo convention (`// nosemgrep:
<rule-id> -- <justification>` on the line above the code), matching
precedents in src/discover/lexer.rs and src/core/stream.rs.
The comment was previously added in response to review point #5. Removed
per follow-up feedback — the arm itself is self-explanatory in context
alongside the other Host variants.
Every other agent with a dedicated hook implementation carries a
hooks/<agent>/README.md (see antigravity/cline/opencode/copilot/hermes
for the shape). The initial Vibe commit skipped this, leaving Vibe as
the odd one out in the hooks/ layout.
- Add hooks/vibe/README.md following the Copilot template (Rust binary
hook, no shell dependency). Documents the pre_tool hook location,
input JSON shape, rewrite response, passthrough / deny behavior,
and the belt-and-suspenders prompt fallback.
- Fix hooks/README.md Directory Structure entry to point at
vibe/README.md (previously claimed 'no dedicated subdirectory').
Addresses @aeppling's review on #3391:
Blocking fixes:
- run_vibe now returns Ok(()) on malformed JSON (matches run_droid /
run_copilot / run_cursor pattern). Prior code violated the exit-code
contract documented at src/hooks/README.md:100 — a bad payload exited
non-zero and blocked the agent's command. Fixed via a match on
serde_json::from_str with a stderr warning fallback.
- Extract run_vibe_inner(input: &str) -> Option<String> from run_vibe so
the hook contract is unit-testable (mirrors run_droid_inner). Public
run_vibe becomes a thin stdin/stdout wrapper.
- Add 6 runtime tests exercising the hook contract: bash rewrite happy
path, non-bash tool passthrough, empty command passthrough, malformed
JSON returns None, unknown binary passthrough, substitution defers.
Should-fix:
- Telemetry agent detection: add ~/.vibe/hooks.toml to detect_hook_type()
checks in src/core/telemetry.rs, plus the two test enum arrays so Vibe
sessions no longer report as 'unknown' in rtk gain history.
- Dead deny arm: add a comment on Host::Vibe in permissions.rs
documenting that the empty-rules branch is defensive scaffolding for
when Vibe ships native denylist/allowlist config we can honor.
- Broken link: patch_vibe_hooks_toml skip-message now points at
https://www.rtk-ai.app/guide/getting-started/supported-agents#mistral-vibe
instead of a fragment that doesn't resolve.
Nits addressed:
- Install summary no longer prints 'hook installed' when the user chose
PatchMode::Skip or declined the interactive prompt. patch_vibe_hooks_toml
now returns a VibeHookPatchOutcome enum (Installed / AlreadyPresent /
Skipped) and the caller gates the summary on it.
- Document the string-spacing tradeoff on vibe_hooks_toml_has_rtk: a
reformatted 'name="rtk-rewrite"' would defeat idempotency, acceptable
because we control the writer and toml_edit round-trip would clobber
user comments.
- Fix stale line in src/hooks/README.md 'Adding New Functionality':
hook_check.rs::maybe_warn() only checks the Claude Code hook now,
not every agent.
Documentation:
- docs/guide/getting-started/supported-agents.md: frontmatter now lists
Mistral Vibe, drop 'planned' from the intro, tier table row flipped
from 'Planned (#800)' to 'Rust binary (pre_tool) / Yes', replace the
### Mistral Vibe (planned) placeholder with a full user-facing section
modeled on Factory Droid (install/uninstall commands, hook mechanism,
permission semantics, idempotency contract).
- hooks/README.md: agent count 9 -> 10, add Vibe entry to Directory
Structure list, add Vibe row to Supported Agents table, add
'### Mistral Vibe (Rust Binary)' entry to the JSON Formats section
showing the pre_tool input shape and rewrite response shape.
- src/hooks/README.md: agent count 5 -> 6, add Vibe row to per-host
ask-support table.
- README.md: '15 AI coding tools' -> '16'.
No behavior change for existing agents.
Add `rtk init -g --agent vibe` and `rtk hook vibe` to route bash tool
calls through the RTK proxy via Vibe's newly-shipped pre_tool hook.
Implementation follows the Gemini / Droid pattern:
- Native binary hook (`rtk hook vibe`), no shell script dependency.
- Global-only install (`~/.vibe/hooks.toml`); user-scope only.
- Idempotent install: detects existing `name = "rtk-rewrite"` entry.
- Uninstall is surgical: strips only the RTK `[[hooks]]` block and the
`~/.vibe/prompts/rtk.md` prompt file, preserving any other user hooks
byte-for-byte. Removes hooks.toml only when it becomes empty.
- Hook response uses Vibe's documented `hook_specific_output.tool_input`
rewrite contract with a `system_message` for UI visibility.
Vibe hook API reference:
https://docs.mistral.ai/vibe/code/cli/hooksCloses#800.
Nicolas Le Cam