Skip to content

fix(gh): rewrite gh --json/--jq/--template instead of deferring - #4069

Open
dopaemon wants to merge 1 commit into
rtk-ai:developfrom
dopaemon:fix/gh-json-rewrite
Open

dopaemon wants to merge 1 commit into
rtk-ai:developfrom
dopaemon:fix/gh-json-rewrite

Conversation

@dopaemon

Copy link
Copy Markdown

Fixes #4067

Problem

$ rtk rewrite 'gh pr view 123 --json reviews'
$ echo $?
1

Exit 1 / empty output is the "no rewrite" verdict, so the hook passes the command through untouched. Since almost every real gh pr view call carries --json, that whole family was invisible to rtk — the issue reports it as the single largest missed-savings command in rtk discover.

Why the guard is stale

registry.rs skipped the rewrite for rtk gh whenever the args contained --json/--jq/--template (#196), on the grounds that rtk gh would corrupt structured output. That hasn't been true for a while: gh_cmd::run checks the same flags first and hands the call to run_passthrough, which streams gh's own bytes and propagates its exit code — the same stance gh api documents ("Passthrough preserves the full response and tracks metrics at 0% savings").

So the guard bought nothing and cost coverage.

Changes

  • src/discover/registry.rs: drop the rtk gh + --json/--jq/--template special-case. gh pr view 123 --json reviews now rewrites to rtk gh pr view 123 --json reviews.

  • src/cmds/git/gh_cmd.rs: harden the passthrough gate the registry guard was masking. has_json_flag matched only a bare --json, so the = spelling slipped into the filter path. On current develop:

    $ rtk gh pr view 4066 --repo rtk-ai/rtk --json=number
    [open] PR #4066: fix(read): fall back to raw bytes ...   # rtk's summary, not the requested field

    wants_raw_output replaces it and covers --json/--jq/-q/--template/-t in both --flag value and --flag=value forms. Without this, lifting the registry guard would have made that wrong-output path reachable from the hook.

  • Tests: the four fix: passthrough --json/--jq/--template flags in all gh subcommands #196 registry tests now assert the rewrite instead of None (plus one for the issue's exact gh pr view 123 --json reviews), and three unit tests cover the flag spellings.

Verification

Byte-comparison against a real PR, gh vs rtk gh:

$ gh pr view 4066 --repo rtk-ai/rtk --json number,title,state,reviews > a.json
$ rtk gh pr view 4066 --repo rtk-ai/rtk --json number,title,state,reviews > b.json
$ cmp a.json b.json && echo identical
identical                       # 3025 bytes, exit 0 both

$ # --jq and --template forms: identical as well
$ rtk rewrite 'gh pr view 123 --json reviews'
rtk gh pr view 123 --json reviews

cargo fmt --all + cargo clippy --all-targets clean; cargo test --bin rtk → 3615 passed, plus the hook/gh integration binaries (hook_decision_protocol_test, gh_pr_checks_failure_test, copilot_selfheal_test, hook_warning_scope_test) green.

Not included

The issue's secondary note (git log -1 | grep -n foo leaves the post-pipe stage unrewritten) is a separate, deliberate mechanism — PipelineSafety only rewrites stages a filter can safely occupy. Worth its own issue rather than folding into this fix.

@rtk-wshm-sync-bot

Copy link
Copy Markdown

wshm · Automated triage by AI

📊 Automated PR Analysis

🐛 Type bug-fix
🟡 Risk medium

Summary

Removes a stale registry guard that skipped rewriting gh commands containing --json/--jq/--template, since rtk gh already passthroughs those flags unchanged via run_passthrough. Also hardens the passthrough flag detection (renamed has_json_flag to wants_raw_output) to catch --json=, --jq, -q, --template, and -t in both space and equals forms, fixing a case where --json=field wasn't detected and rtk's own summary was shown instead of raw gh output.

Review Checklist

  • Tests present
  • Breaking change
  • Docs updated

Linked issues: #4067, #196


Analyzed automatically by wshm · This is an automated analysis, not a human review.

@gibkim

gibkim commented Oct 26, 2026 •

Copy link
Copy Markdown

@dopaemon are you still planning to update this PR? It now conflicts with develop.

I ran into the same gap. I fed the 4,000 most frequent distinct command lines (out of about 7,400) from my own agent history through classify_command and rewrite_command on current develop (e05c7bd), and 33 of them are classified as supported but never rewritten. All 33 are gh pr view|list, gh run view|list, gh repo view|list or gh api with --json/--jq. So this is still the largest such group.

If you no longer have time for it, I'd be happy to rebase it onto develop (keeping your commits and authorship) or to open a follow-up PR. If you'd rather finish it yourself, that's fine too, just let me know.

@dopaemon

Copy link
Copy Markdown
Author

@dopaemon are you still planning to update this PR? It now conflicts with develop.

I ran into the same gap. I fed the 4,000 most frequent distinct command lines (out of about 7,400) from my own agent history through classify_command and rewrite_command on current develop (e05c7bd), and 33 of them are classified as supported but never rewritten. All 33 are gh pr view|list, gh run view|list, gh repo view|list or gh api with --json/--jq. So this is still the largest such group.

If you no longer have time for it, I'd be happy to rebase it onto develop (keeping your commits and authorship) or to open a follow-up PR. If you'd rather finish it yourself, that's fine too, just let me know.

Hey, thanks for checking in and for the detailed stats!

I'm definitely still planning to wrap this up. I've been a bit tied up recently, but I'll rebase onto develop and sort out the conflicts shortly. Hang tight, will push an update soon!

`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>
@dopaemon
dopaemon force-pushed the fix/gh-json-rewrite branch from 5f90d75 to f9a8372 Compare October 26, 2026 11:42
@dopaemon

Copy link
Copy Markdown
Author

@aeppling Pls review this PR, Thks

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

Labels

bug Something isn't working gh rewrite

Projects

None yet

Development

Successfully merging this pull request may close these issues.

rewrite: gh pr view --json <field> fails silently (exit 1, no rewrite)

3 participants