fix: passthrough --json/--jq/--template flags in all gh subcommands - #196
shadowofdoom wants to merge 1 commit into
Conversation
view_pr() assumed args[0] was always a PR number, so `gh pr view --json fields` treated "--json" as the PR identifier and appended its own --json, causing `Unknown JSON field: "--json"`. The v0.21.1 fix (should_passthrough_run_view) only covered `gh run view`. This generalizes it with has_output_format_flags() and applies it to all handlers that hardcode --json fields: list_prs, view_pr, pr_status, list_issues, view_issue, list_runs, and run_repo. Also fixes view_pr to handle omitted PR number (gh defaults to current branch). Closes rtk-ai#159 (broader fix) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Tested locally on the branch — LGTM, approve. All 383 tests pass. Manual testing confirms:
Minor nits (non-blocking):
|
|
please review conflict to merge |
…tk-ai#217, rtk-ai#196, rtk-ai#248, rtk-ai#211, rtk-ai#200, rtk-ai#192, rtk-ai#268) Wave 1 (critical bugs): - fix(registry): fi/done moved to IGNORED_EXACT — find no longer shadowed (rtk-ai#246) - fix(playwright): f64 duration, specs[] structure, --reporter=json after subcmd (rtk-ai#193) - fix(gh): should_passthrough_gh_view for --json/--jq/--template/--web in view_pr/issue/run (rtk-ai#217+196) Wave 2 (reliability): - fix(git): is_blob_show_arg — blob show passthrough without trailing-newline trim (rtk-ai#248) - fix(find): parse_find_args with native -name/-type/-maxdepth/-iname support (rtk-ai#211) - fix(main): graceful Clap fallback + parse_failures SQLite table + rtk gain --failures (rtk-ai#200) Wave 3 (UX): - feat(git): global options -C/-c/--git-dir/--work-tree/--no-pager/--no-optional-locks/--bare/--literal-pathspecs (rtk-ai#192) - feat(proxy): streaming output via spawn()+threads instead of buffered output() (rtk-ai#268) Tests: 1091 → 1117 (+26), 0 regressions Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Ran into this exact issue while auditing RTK compatibility with a plugin-heavy Claude Code setup. Sharing some findings that might help justify prioritizing this merge. What breaks in practiceWe use a gh issue list --search "..." --json number,title,state --jq '.[:3]'
gh pr list --head "$BRANCH" --json number,state,reviewDecision --jq '.[]'
gh pr view --json comments --jq '.comments[].body'
gh repo view --json name --jq '.name'The failure mode is subtle: the hook rewrites the command, RTK runs What we verified in the source
Workaround we're using in the meantimeFrom the thread in #188, Is there anything blocking this beyond the merge conflict? Happy to help test if a rebase lands. |
|
Architectural position needed: structured-output flags should always passthrough Beyond this specific fix, I think RTK needs a clear architectural stance on passthrough behavior for any CLI flag that signals structured/machine-readable output. The issue I hit with `gh --json` / `--jq` is not a one-off — it's a fundamental pattern that will repeat for every proxied CLI that supports structured output conventions:
When a Claude Code skill (or any plugin) runs a CLI command and parses the output programmatically — via inline `--jq`, downstream `jq` pipes, or direct JSON deserialization — RTK's reformatting silently breaks the contract. Proposed convention: any CLI that has established object-passing or post-processing conventions (structured output flags, machine-readable modes) should be treated as passthrough when those flags are present, to guarantee correct integration with skill and plugin ecosystems. PR #196's `has_output_format_flags()` is exactly the right pattern. Elevating it to a documented design principle — and applying it systematically to every future CLI proxy RTK adds — would prevent this entire class of breakage upfront, rather than fixing it case by case. |
|
Hi @shadowofdoom! We have multiple gh flag passthrough PRs open (#196, #217, #319, #325, #328). They all conflict with each other. Could you rebase both PRs on latest master first to fix the CI failures, then we'll coordinate which approach to take? Alternatively, feel free to consolidate your two PRs into one — that would help. |
gh commands with --json, --jq, or --template produce structured output that rtk gh filtering would corrupt. Skip rewrite so callers get raw JSON.
|
Fixed in v0.27.0 — gh with --json/--jq/--template now skips rewrite. |
gh commands with --json, --jq, or --template produce structured output that rtk gh filtering would corrupt. Skip rewrite so callers get raw JSON.
Conflict resolution (1 conflict, additive):
- src/discover/registry.rs: keep all tests from both sides
- ours: test_classify_wc, test_classify_wc_bare, test_route_wc
- upstream: test_rewrite_gh_json_skipped, test_rewrite_gh_jq_skipped,
test_rewrite_gh_template_skipped, test_rewrite_gh_api_json_skipped,
test_rewrite_gh_without_json_still_works (rtk-ai#196)
Upstream v0.27.0 changes absorbed:
- fix(registry): RTK_DISABLED=1 env prefix skips rewrite entirely (rtk-ai#345)
- fix(registry): gh --json/--jq/--template skips rewrite to avoid
corrupting structured output (rtk-ai#196)
- fix: RTK_DISABLED ignored, 2>&1 broken, json TOML error (rtk-ai#345,rtk-ai#346,rtk-ai#347)
- docs: version refs, module count, CHANGELOG, ARCHITECTURE
Also fixed pre-existing test race exposed by new parallel tests:
- fix(test): add EnvGuard to test_shared_is_hook_disabled_* in claude.rs
to prevent RTK_ACTIVE race with test_raii_guard_clears_on_panic
(both tests set RTK_ACTIVE without holding ENV_LOCK)
Tests: 1029 pass, 0 fail (parallel), 5 ignored
…nged Bug: rtk hook claude was routing gh pr list --json ... to rtk gh pr list --json ... which corrupts structured JSON output. Mirrors upstream fix registry::rewrite_segment rtk-ai#196 to the binary hook path. Fix: should_passthrough() returns true for gh commands containing --json, --jq, or --template flags — hook emits no output (NoOpinion) so Claude Code runs the original gh command unchanged. Tests: 2 new (test_gh_json_flag_passes_through, test_gh_without_json_not_passthrough); 1031 pass total
…at/gemini-support-v2
Conflict resolution (1 conflict, additive):
- src/main.rs Init command: merge --claude/--gemini flags (gemini branch)
with --hook-type flag (rust-hooks-v2); keep all three flags
- Dispatch: multi-platform logic calls init::run(..., hook_type, ...)
for Claude and init::run_gemini() for Gemini; hook_type threads through
Result: rtk init now supports --claude, --gemini, and --hook-type together
rtk init → Claude + Gemini, script hook (default)
rtk init --claude → Claude only
rtk init --gemini → Gemini only
rtk init --hook-type binary → Claude + Gemini with binary hook
Inherits from rust-hooks-v2 (dbc46c7):
- fix(hook): binary hook passes gh --json/--jq/--template through unchanged
(mirrors upstream registry::rewrite_segment fix rtk-ai#196)
- 2 new tests: test_gh_json_flag_passes_through, test_gh_without_json_not_passthrough
Tests: 1050 pass, 0 fail (parallel), 5 ignored
* fix: prettier reports "All OK" when not installed (#221) Empty or failed prettier output was incorrectly treated as "all files formatted". Now detects empty output and non-zero exit code, shows the actual error message instead of a false positive. * test: add smoke tests for rewrite, verify, proxy, discover, diff, wc, smart, docker, json edge cases Covers bug fixes #196, #344, #345, #346, #347 and previously untested commands. Adds assert_fails helper. 118 assertions total (was 69). * chore: update benchmark.sh with missing commands and fix paths - Add cargo (build/test/clippy/check), diff, smart, wc, curl, wget sections - Fix Python commands: use dedicated rtk ruff/pytest instead of rtk test - Fix Go commands: use dedicated rtk go/golangci-lint, add go build/vet - Make BENCH_DIR absolute so debug files work from temp fixture dirs - Fallback to installed rtk if target/release/rtk not found
`rtk curl` rewrites every `curl URL` to `rtk curl URL` and pipes the
response through `rtk json --schema`, which produces field-type literals
(`field: int`) and a `(N)` array-length suffix. That's a token-savings
win for human-facing exploration of arbitrary third-party APIs, but it
ACTIVELY BREAKS downstream JSON parsing (jq, `python json.load`,
agent-side filtering, anything that round-trips JSON) for private or
internal APIs whose responses are consumed by parsers rather than read
by humans.
Today the only escape hatch is `[hooks] exclude_commands = ["curl"]`,
which disables curl rewriting entirely — losing the token savings on the
3rd-party APIs that motivated the rewrite in the first place.
This change adds a narrow opt-in mechanism: a substring allowlist that
opts specific URLs out of the rewrite while leaving everything else
rewriting as before.
[curl]
bypass_url_markers = [
"localhost:8080/api/",
"//internal.example.com/v1/",
]
Each marker is matched as a substring against the full command segment
(after env-prefix stripping), so it composes cleanly with curl's flag
positioning (`-X POST`, `-H`, `-d`, etc). Bypass is per-segment for
compound commands: `a && curl <internal> | jq` bypasses the curl segment
while `a` and the pipe target still rewrite as normal.
Default `bypass_url_markers = []` preserves historical behavior — users
who don't configure anything see no change.
API surface
-----------
* New `crate::core::config::CurlConfig { bypass_url_markers: Vec<String> }`
* New `crate::discover::registry::RewriteOptions { curl_bypass_url_markers }`
* New `rewrite_command_with_options(cmd, excluded, &opts)` — takes
options explicitly, useful for tests and callers that already have
config in hand.
* Existing `rewrite_command(cmd, excluded)` is unchanged for callers but
now reads `Config.curl.bypass_url_markers` internally and forwards.
Mirrors the existing rtk-ai#196 bypass shape for `gh --json/--jq/--template`:
detect a structured-output consumer and skip schema-mode filtering.
Tests
-----
* 4 new config tests (default empty / [curl] roundtrip / missing
section).
* 9 new registry tests covering: localhost / loopback / hostname
marker bypass; POST with headers + payload; multi-marker OR;
default-empty preserves rewrite; unmatched URL still rewrites;
port-specific narrowness; per-segment behavior in compound commands.
* Full suite: 1699 passed, 0 failed, 6 ignored.
`rtk curl` rewrites every `curl URL` to `rtk curl URL` and pipes the
response through `rtk json --schema`, which produces field-type literals
(`field: int`) and a `(N)` array-length suffix. That's a token-savings
win for human-facing exploration of arbitrary third-party APIs, but it
ACTIVELY BREAKS downstream JSON parsing (jq, `python json.load`,
agent-side filtering, anything that round-trips JSON) for private or
internal APIs whose responses are consumed by parsers rather than read
by humans.
Today the only escape hatch is `[hooks] exclude_commands = ["curl"]`,
which disables curl rewriting entirely — losing the token savings on the
3rd-party APIs that motivated the rewrite in the first place.
This change adds a narrow opt-in mechanism: a substring allowlist that
opts specific URLs out of the rewrite while leaving everything else
rewriting as before.
[curl]
bypass_url_markers = [
"localhost:8080/api/",
"//internal.example.com/v1/",
]
Each marker is matched as a substring against the full command segment
(after env-prefix stripping), so it composes cleanly with curl's flag
positioning (`-X POST`, `-H`, `-d`, etc). Bypass is per-segment for
compound commands: `a && curl <internal> | jq` bypasses the curl segment
while `a` and the pipe target still rewrite as normal.
Default `bypass_url_markers = []` preserves historical behavior — users
who don't configure anything see no change.
API surface
-----------
* New `crate::core::config::CurlConfig { bypass_url_markers: Vec<String> }`
* New `crate::discover::registry::RewriteOptions { curl_bypass_url_markers }`
* New `rewrite_command_with_options(cmd, excluded, &opts)` — takes
options explicitly, useful for tests and callers that already have
config in hand.
* Existing `rewrite_command(cmd, excluded)` is unchanged for callers but
now reads `Config.curl.bypass_url_markers` internally and forwards.
Mirrors the existing rtk-ai#196 bypass shape for `gh --json/--jq/--template`:
detect a structured-output consumer and skip schema-mode filtering.
Tests
-----
* 4 new config tests (default empty / [curl] roundtrip / missing
section).
* 9 new registry tests covering: localhost / loopback / hostname
marker bypass; POST with headers + payload; multi-marker OR;
default-empty preserves rewrite; unmatched URL still rewrites;
port-specific narrowness; per-segment behavior in compound commands.
* Full suite: 1699 passed, 0 failed, 6 ignored.
gh commands with --json, --jq, or --template produce structured output that rtk gh filtering would corrupt. Skip rewrite so callers get raw JSON.
… --json, hook check, RTK_DISABLED, 2>&1, json TOML
) * fix: prettier reports "All OK" when not installed (rtk-ai#221) Empty or failed prettier output was incorrectly treated as "all files formatted". Now detects empty output and non-zero exit code, shows the actual error message instead of a false positive. * test: add smoke tests for rewrite, verify, proxy, discover, diff, wc, smart, docker, json edge cases Covers bug fixes rtk-ai#196, rtk-ai#344, rtk-ai#345, rtk-ai#346, rtk-ai#347 and previously untested commands. Adds assert_fails helper. 118 assertions total (was 69). * chore: update benchmark.sh with missing commands and fix paths - Add cargo (build/test/clippy/check), diff, smart, wc, curl, wget sections - Fix Python commands: use dedicated rtk ruff/pytest instead of rtk test - Fix Go commands: use dedicated rtk go/golangci-lint, add go build/vet - Make BENCH_DIR absolute so debug files work from temp fixture dirs - Fallback to installed rtk if target/release/rtk not found
- Add svn RtkRule to rules.rs so hook auto-rewrites svn status/log/diff/ info/blame/add/commit/update/up to rtk-prefixed equivalents - Add --xml detection to registry rewrite_segment (like gh --json in rtk-ai#196) to skip hook rewriting for machine-readable XML output - Add module-level --xml passthrough in svn run_log and run_info so that even direct `rtk svn log --xml` bypasses filtering Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…surable (rtk-ai#196) saved_tokens is computed with saturating_sub, so a filter that emits MORE tokens than it consumed records as "0% saved" and the regression vanishes from the headline stats. Telemetry showed 1,410 commands (11.5%) with output > input, +25,846 tokens net, all hidden as "0%". Add an inflation_tokens column (output - input, floored at 0) populated on every record(), plus a total_inflation_tokens() accessor. saved_tokens and savings_pct are deliberately UNCHANGED — making saved signed would break the unsigned SUM aggregations in gain.rs and the no_bloat leaked-tokens calc. Migration mirrors the existing exec_time_ms/project_path ALTER TABLE pattern (idempotent; duplicate-column error swallowed). Council design (Codex + mmax, prior round): separate column, do not touch saturating_sub, leave streaming as-is. Council review (this round): SHIP (Codex + mmax, no blocking findings). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…record) (rtk-ai#196) TimedExecution::track clamped output to `estimate_tokens(output).min(input_tokens)` before calling record(). record() derives inflation as `output_tokens.saturating_sub(input_tokens)`, so the clamp guaranteed inflation was ALWAYS 0 in production — the entire rtk-ai#196 column was dead on arrival. The unit test passed only because it called record() directly with un-clamped values, bypassing the clamp (a false green). Fix: drop the `.min(input_tokens)` clamp in track() and pass the real `estimate_tokens(output)`. record() already floors `saved` at 0 via `input.saturating_sub(output)`, so the no-bloat guarantee of #95 is preserved (inflating filter still reports 0 saved / 0% savings, never negative). inflation_tokens now captures the true overflow. track_passthrough passes 0,0 and is unaffected; it was the only other record() caller. Tests: replaced the clamp-asserting tests (which encoded the bug) with tests that assert the corrected behaviour, and added a regression driven through track() (not record() directly) proving an inflating filter records inflation_tokens > 0 while saved_tokens == 0 and savings_pct == 0 — the regression the old suite missed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
rtk-ai#196) The inflation_tokens column now records real data (the clamp that zeroed it was removed), but nothing user-visible exposed it. savings_pct floors at 0, so a filter that emits MORE tokens than the raw command reads as "0% saved" and the regression stays hidden. Surface it additively, with no change to clean-install output: - GainSummary gains total_inflation (SUM over inflation_tokens in the existing project-scoped summary query). gain prints a "Tokens inflated" KPI line ONLY when > 0, so clean installs are unaffected. - WeakFilter gains inflation_tokens (SUM(inflation_tokens) added to the existing GROUP BY rtk_cmd weak-filter query). gain --weak-filters adds an "Inflated" column; clean tools show a dash, inflating tools show the figure so a regression stands out. - JSON export carries total_inflation for machine-readable parity. Both queries reuse the established project-path filter / time-window patterns, so scoping stays consistent. Tests (in-memory tracker / RTK_DB_PATH pattern): summary aggregates inflation across rows; reports 0 on clean data (line omitted); weak filters carry per-tool inflation with a saving tool reporting zero. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… follow-up) run_fallback recorded a parse_failure + a 0-saving `commands` row for every passthrough — including commands contextcrawler never owned (unknown subcommands like `cortextos bus`, `bash -lc ...`). That skewed the `gain` savings % (147 parse-failures + 173 zero-saving rows of pure cron traffic in real data) and flooded `discover` with non-actionable noise. Add is_foreign_command() = clap ErrorKind::InvalidSubcommand and, in the no-TOML passthrough branch, skip BOTH track_passthrough and record_parse_failure for foreign commands. They still pass through and propagate the real exit code — they're just not counted, because they were never a candidate for filtering. Genuine parse failures on commands we DO own (other error kinds) stay tracked; TOML-matched commands are unaffected. Reviewed by mmax (no blockers): InvalidSubcommand is the correct/sufficient signal, exit-code propagation unchanged, tracking is stats-only (no security impact). Test: is_foreign_command distinguishes unknown subcommand (foreign) from bad-args on a known command (real failure). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…(rtk rtk-ai#2035) The compact `git status` path ran `git status --porcelain -b -uall`, but we measure savings against raw `git status` — which defaults to `-unormal` and collapses fully-untracked directories to a single line. `-uall` expands them into every file, so in any repo with a non-ignored untracked dir our output exceeded raw (~29x on a 200-file dir), the opposite of the tool's purpose — and the inflation was invisible because `savings_pct` floors at 0 (rtk-ai#196). Drop `-uall` so untracked dirs collapse exactly like raw. Modified/staged/ renamed/conflict paths are unaffected by `-uall` and remain fully shown by the formatter; state-header and detached-HEAD extraction read from the separately-captured plain status, so they are unchanged. Aligns with upstream rtk PR rtk-ai#2035. Council: Codex + mmax both APPROVE. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Council (codex MEDIUM): the "vs raw output" line was shown only when total_saved != effective_saved. But a huge zero-savings dump — an rtk-ai#196 inflation row or an unfiltered passthrough — contributes 0 to BOTH saved totals while still shrinking effective_input, so the raw and effective percentages diverge (e.g. 0.007% vs 0.92%) yet the raw line stayed hidden. Gate on total_input != effective_input instead: that differs exactly when some command was capped, which is exactly when the two percentages can diverge. mmax SHIP; this closes codex's HOLD. Refs: rtk-ai#208 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K3SAimSBwPUp4MkRXaYXSU
gh --json and gh api were passthrough-only (rtk-ai#196): converting JSON to a bare schema would destroy values, so rtk never touched them and tracked 0% savings on exactly the outputs where field names repeat per row. This adds core/jsonpack: a lossless re-encoding ported from the compaction stage of headroom-core (headroomlabs-ai/headroom, Apache-2.0) with every lossy path removed — no retrieval pointers, no row dropping, no stringified-JSON rewriting. - top-level arrays of objects render as a [N]{col:type} declaration plus CSV rows; empty cell = absent key, bare null = JSON null, lookalike strings stay quoted, mixed columns carry JSON literals - envelopes (gh api) stay valid JSON: dense inner arrays become {"_cols":…,"_rows":…} tables and the body is minified - pack() decodes its own output and requires value equality with the input before emitting; any mismatch (including data that collides with the notation) falls back to the raw bytes — Never Block - --jq/--template (long, short, = and attached forms) keep full passthrough; the registry rewrites bare --json to rtk gh again Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
gh --json and gh api were passthrough-only (rtk-ai#196): converting JSON to a bare schema would destroy values, so rtk never touched them and tracked 0% savings on exactly the outputs where field names repeat per row. This adds core/jsonpack: a lossless re-encoding ported from the compaction stage of headroom-core (headroomlabs-ai/headroom, Apache-2.0) with every lossy path removed — no retrieval pointers, no row dropping, no stringified-JSON rewriting. - top-level arrays of objects render as a [N]{col:type} declaration plus CSV rows; empty cell = absent key, bare null = JSON null, lookalike strings stay quoted, mixed columns carry JSON literals - envelopes (gh api) stay valid JSON: dense inner arrays become {"_cols":…,"_rows":…} tables and the body is minified - pack() decodes its own output and requires value equality with the input before emitting; any mismatch (including data that collides with the notation) falls back to the raw bytes — Never Block - --jq/--template (long, short, = and attached forms) keep full passthrough; the registry rewrites bare --json to rtk gh again Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Rui Botte <nfsbotte@gmail.com>
`rtk rewrite 'gh pr view 123 --json reviews'` exited 1 with no output, so the hook passed the command through unrewritten — `gh pr view --json` is how gh is used in practice, so those calls were never tracked. The rtk-ai#196 guard assumed `rtk gh` would reshape structured output, but `gh_cmd::run` has passed `--json` straight to gh since then, the same stance `gh api` takes. Rewriting is therefore byte-for-byte safe and the guard only cost coverage. - drop the registry special-case for `rtk gh` + --json/--jq/--template - harden the passthrough gate it was masking: `has_json_flag` matched only a bare `--json`, so `gh pr view 42 --json=number` fell into the filter and returned rtk's summary instead of the requested field. `wants_raw_output` now covers --json/--jq/-q/--template/-t in both spellings - flip the rtk-ai#196 registry tests to assert the rewrite, add unit tests for the flag spellings Verified against a real PR: `gh pr view N --json number,title,state,reviews` and the --jq/--template forms are byte-identical through `rtk gh`. Fixes rtk-ai#4067 Signed-off-by: dopaemon <polarisdp@gmail.com>
Summary
gh pr view --json fieldsfailed withUnknown JSON field: "--json"becauseview_pr()treated--jsonas the PR identifiergh run viewviashould_passthrough_run_view()has_output_format_flags()and applies it to all handlers that hardcode--json:list_prs,view_pr,pr_status,list_issues,view_issue,list_runs, andrun_repoview_prto handle omitted PR number (gh defaults to current branch)Reproduction
Test plan
has_output_format_flags)rtk gh pr view --json number,title,statepasses through correctlyrtk gh pr liststill uses RTK's compressed outputrtk gh pr view <number>still uses RTK's compressed output🤖 Generated with Claude Code