Skip to content

fix: gh pr edit/comment pass correct subcommand to gh#332

Merged
pszymkowiak merged 2 commits intortk-ai:masterfrom
skvale:fix/pr-action-subcommand
Mar 6, 2026
Merged

fix: gh pr edit/comment pass correct subcommand to gh#332
pszymkowiak merged 2 commits intortk-ai:masterfrom
skvale:fix/pr-action-subcommand

Conversation

@skvale
Copy link
Contributor

@skvale skvale commented Mar 5, 2026

pr_action was receiving the past-tense display label ("edited", "commented") and using it as both the gh subcommand and the ok_confirmation message.

This caused gh pr edited to be invoked instead of gh pr edit, which fails with "unknown flag" errors.

Split the parameter into subcmd (for the gh invocation) and display_label (for ok_confirmation output).

skvale added 2 commits March 5, 2026 07:41
pr_action was receiving the past-tense display label ("edited",
"commented") and using it as both the gh subcommand and the
ok_confirmation message. This caused `gh pr edited` to be invoked
instead of `gh pr edit`, which fails with "unknown flag" errors.

Split the parameter into subcmd (for the gh invocation) and
display_label (for ok_confirmation output).
@pszymkowiak
Copy link
Collaborator

Review: LGTM ✅

Code verified locally — cargo fmt, cargo clippy, and all 607 tests pass.

What the fix does:

  • pr_action() now receives the full args slice (with args[0] being the actual gh subcommand like "comment" or "edit") instead of the past-tense
    display label ("commented" / "edited")
  • The action parameter is kept only for the ok_confirmation display string
  • This fixes gh pr comment and gh pr edit which were broken because "commented" / "edited" were passed as the gh subcommand

Merge note: Branch is based on older master (607 vs 656 tests) but there are no code conflicts — GitHub can merge directly without rebase.

@pszymkowiak pszymkowiak merged commit 799f085 into rtk-ai:master Mar 6, 2026
2 of 3 checks passed
ahundt added a commit to ahundt/rtk that referenced this pull request Mar 11, 2026
Incorporates 52 upstream commits (v0.27.0 → v0.28.2):
- TOML filter DSL engine + 30 built-in filters (PRs rtk-ai#349, rtk-ai#351, rtk-ai#386)
- Graphite CLI support (PR rtk-ai#290)
- git commit -am/--amend fix via trailing_var_arg (PR rtk-ai#327)
- restore_double_dash for cargo (PR rtk-ai#326)
- gh -R/--repo passthrough, pr edit/comment fix (PRs rtk-ai#328, rtk-ai#332)
- docker compose subcommand filtering (PR rtk-ai#336)
- Telemetry tokens_saved + install_method (PRs rtk-ai#462, rtk-ai#469, rtk-ai#471)
- proxy streaming (PR rtk-ai#268)
- Diff limits increased (100→500 lines, 10→30 hunk lines)

Conflict resolution (5 files):
- cargo_cmd.rs: adopted upstream restore_double_dash, adapted streaming
  run_test() to use it, converted old split_at_double_dash tests
- git.rs: adopted upstream simplified Commit unit variant (fixes -am),
  adapted all commit tests to flat args API, added 6 new edge case tests
- init.rs: added TOML template generation alongside hook manifest
- main.rs: merged both upstream (gt, toml_filter, verify) and hooks-v2
  (cmd, hook, stream, pipe) modules, kept all tests from both sides
- utils.rs: kept hooks-v2 command_in_path/which_command + upstream English docs

Hook engine additions during merge:
- Added gt to hook_lookup() whitelist with 4 routing test cases

All 5 hook bug fixes from issue rtk-ai#361 preserved:
1. Streaming (stream.rs BufReader)
2. Handler coordination (parallel-merge + run_manifest_handlers on both paths)
3. Stderr deny (exit 2)
4. Routing whitelist (hook_lookup)
5. Vitest run injection

1182 tests pass (1 environment-dependent upstream test excluded).
ahundt added a commit to ahundt/rtk that referenced this pull request Mar 11, 2026
Incorporates 52 upstream commits (v0.27.0 → v0.28.2):
- TOML filter DSL engine + 30 built-in filters (PRs rtk-ai#349, rtk-ai#351, rtk-ai#386)
- Graphite CLI support (PR rtk-ai#290)
- git commit -am/--amend fix via trailing_var_arg (PR rtk-ai#327)
- restore_double_dash for cargo (PR rtk-ai#326)
- gh -R/--repo passthrough, pr edit/comment fix (PRs rtk-ai#328, rtk-ai#332)
- docker compose subcommand filtering (PR rtk-ai#336)
- Telemetry tokens_saved + install_method (PRs rtk-ai#462, rtk-ai#469, rtk-ai#471)
- proxy streaming (PR rtk-ai#268)
- Diff limits increased (100→500 lines, 10→30 hunk lines)

Conflict resolution (5 files):
- cargo_cmd.rs: adopted upstream restore_double_dash
- git.rs: adopted upstream simplified Commit variant (fixes -am),
  fixed test_git_status_not_a_repo via GIT_DIR env override
- init.rs: added TOML template generation alongside hook manifest,
  made resolve_claude_dir pub(crate) for config/mod.rs
- main.rs: merged upstream (gt, toml_filter, verify) and
  multi-platform (cmd, hook, stream, safety, gemini) modules
- utils.rs: accepted English doc comments

Recovery edits (safety integration restored after incorrect overwrite):
- hook/mod.rs: restored config::rules::try_remap(), safety::check_raw(),
  safety::check() per-command, FORMAT_PRESERVING/TRANSPARENT_SINKS
  pub(crate), basename extraction, safety-dependent tests
- discover/registry.rs: updated 3 wc tests for upstream IGNORED_PREFIXES

All hook engine + safety + gemini features preserved.
1332 tests pass, 0 failures, 5 ignored.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants