diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 06cb022df..d18a3b357 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -291,7 +291,7 @@ jobs: desc "Rust Token Killer - High-performance CLI proxy to minimize LLM token consumption" homepage "https://www.rtk-ai.app" version "VERSION_PLACEHOLDER" - license "MIT" + license "Apache 2.0" if OS.mac? && Hardware::CPU.arm? url "https://github.com/rtk-ai/rtk/releases/download/TAG_PLACEHOLDER/rtk-aarch64-apple-darwin.tar.gz" diff --git a/Cargo.toml b/Cargo.toml index 8fc8a9305..0b5b82014 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -4,7 +4,7 @@ version = "0.40.0" edition = "2021" authors = ["Patrick Szymkowiak"] description = "Rust Token Killer - High-performance CLI proxy to minimize LLM token consumption" -license = "MIT" +license = "Apache 2.0" homepage = "https://www.rtk-ai.app" repository = "https://github.com/rtk-ai/rtk" readme = "README.md" diff --git a/Formula/rtk.rb b/Formula/rtk.rb index 25a8b2e8a..34c27f9fb 100644 --- a/Formula/rtk.rb +++ b/Formula/rtk.rb @@ -7,7 +7,7 @@ class Rtk < Formula desc "High-performance CLI proxy to minimize LLM token consumption" homepage "https://www.rtk-ai.app" version "0.1.0" - license "MIT" + license "Apache-2.0" on_macos do on_intel do diff --git a/README.md b/README.md index f8d65efe5..d91e21b24 100644 --- a/README.md +++ b/README.md @@ -28,7 +28,8 @@ 中文 • 日本語 • 한국어 • - Espanol + Espanol • + Português

--- @@ -106,10 +107,11 @@ rtk init -g # Claude Code / Copilot (default) rtk init -g --gemini # Gemini CLI rtk init -g --codex # Codex (OpenAI) rtk init -g --agent cursor # Cursor -rtk init --agent windsurf # Windsurf +rtk init -g --agent windsurf # Windsurf rtk init --agent cline # Cline / Roo Code rtk init --agent kilocode # Kilo Code rtk init --agent antigravity # Google Antigravity +rtk init -g --agent pi # Pi rtk init --agent hermes # Hermes # 2. Restart your AI tool, then test @@ -351,7 +353,7 @@ rtk git status ## Supported AI Tools -RTK supports 13 AI coding tools. Each integration rewrites shell commands to `rtk` equivalents for 60-90% token savings where the agent supports command interception. +RTK supports 14 AI coding tools. Each integration rewrites shell commands to `rtk` equivalents for 60-90% token savings where the agent supports command interception. | Tool | Install | Method | |------|---------|--------| @@ -361,10 +363,11 @@ RTK supports 13 AI coding tools. Each integration rewrites shell commands to `rt | **Cursor** | `rtk init -g --agent cursor` | preToolUse hook (hooks.json) | | **Gemini CLI** | `rtk init -g --gemini` | BeforeTool hook | | **Codex** | `rtk init -g --codex` | AGENTS.md + RTK.md instructions | -| **Windsurf** | `rtk init --agent windsurf` | .windsurfrules (project-scoped) | +| **Windsurf** | `rtk init -g --agent windsurf` | .windsurfrules (project-scoped) | | **Cline / Roo Code** | `rtk init --agent cline` | .clinerules (project-scoped) | | **OpenCode** | `rtk init -g --opencode` | Plugin TS (tool.execute.before) | | **OpenClaw** | `openclaw plugins install ./openclaw` | Plugin TS (before_tool_call) | +| **Pi** | `rtk init -g --agent pi` (global) | TypeScript extension (tool_call) | | **Hermes** | `rtk init --agent hermes` | Python plugin adapter (terminal command mutation via `rtk rewrite`) | | **Mistral Vibe** | Planned ([#800](https://github.com/rtk-ai/rtk/issues/800)) | Blocked on upstream | | **Kilo Code** | `rtk init --agent kilocode` | .kilocode/rules/rtk-rules.md (project-scoped) | diff --git a/README_es.md b/README_es.md index 751bd1ba8..10a6a53a5 100644 --- a/README_es.md +++ b/README_es.md @@ -9,7 +9,7 @@

CI Release - License: MIT + License: Apache 2.0 Discord Homebrew

@@ -158,7 +158,7 @@ Unete a la comunidad en [Discord](https://discord.gg/RySmvNF5kF). ## Licencia -Licencia MIT - ver [LICENSE](LICENSE) para detalles. +Licencia Apache 2.0 - ver [LICENSE](LICENSE) para detalles. ## Descargo de responsabilidad diff --git a/README_fr.md b/README_fr.md index d305feaaf..2de284100 100644 --- a/README_fr.md +++ b/README_fr.md @@ -9,7 +9,7 @@

CI Release - License: MIT + License: Apache 2.0 Discord Homebrew

@@ -195,7 +195,7 @@ Rejoignez la communaute sur [Discord](https://discord.gg/RySmvNF5kF). ## Licence -Licence MIT - voir [LICENSE](LICENSE) pour les details. +Licence Apache 2.0 - voir [LICENSE](LICENSE) pour les details. ## Avertissement diff --git a/README_ja.md b/README_ja.md index 23bf4412f..ce946d628 100644 --- a/README_ja.md +++ b/README_ja.md @@ -9,7 +9,7 @@

CI Release - License: MIT + License: Apache 2.0 Discord Homebrew

@@ -157,7 +157,7 @@ rtk discover # 見逃した節約機会を発見 ## ライセンス -MIT ライセンス - 詳細は [LICENSE](LICENSE) を参照。 +Apache 2.0 ライセンス - 詳細は [LICENSE](LICENSE) を参照。 ## 免責事項 diff --git a/README_ko.md b/README_ko.md index a07a4590a..c969e0ed2 100644 --- a/README_ko.md +++ b/README_ko.md @@ -9,7 +9,7 @@

CI Release - License: MIT + License: Apache 2.0 Discord Homebrew

@@ -157,7 +157,7 @@ rtk discover # 놓친 절약 기회 발견 ## 라이선스 -MIT 라이선스 - 자세한 내용은 [LICENSE](LICENSE)를 참조하세요. +Apache 2.0 라이선스 - 자세한 내용은 [LICENSE](LICENSE)를 참조하세요. ## 면책 조항 diff --git a/README_pt.md b/README_pt.md new file mode 100644 index 000000000..767cd49dc --- /dev/null +++ b/README_pt.md @@ -0,0 +1,166 @@ +

+ RTK - Rust Token Killer +

+ +

+ Proxy CLI de alta performance que reduz o consumo de tokens LLM em 60-90% +

+ +

+ CI + Release + License: Apache 2.0 + Discord + Homebrew +

+ +

+ Site • + Instalar • + Solução de problemas • + Arquitetura • + Discord +

+ +

+ English • + Francais • + 中文 • + 日本語 • + 한국어 • + Espanol • + Português +

+ +--- + +rtk filtra e comprime saídas de comandos antes de chegarem ao contexto do seu LLM. Binário Rust único, zero dependências, overhead inferior a 10ms. + +## Economia de tokens (sessão de 30 min no Claude Code) + +| Operação | Frequência | Padrão | rtk | Economia | +|-----------|------------|----------|-----|--------| +| `ls` / `tree` | 10x | 2,000 | 400 | -80% | +| `cat` / `read` | 20x | 40,000 | 12,000 | -70% | +| `grep` / `rg` | 8x | 16,000 | 3,200 | -80% | +| `git status` | 10x | 3,000 | 600 | -80% | +| `cargo test` / `npm test` | 5x | 25,000 | 2,500 | -90% | +| **Total** | | **~118,000** | **~23,900** | **-80%** | + +## Instalacao + +### Homebrew (recomendado) + +```bash +brew install rtk +``` + +### Instalação rápida (Linux/macOS) + +```bash +curl -fsSL https://raw.githubusercontent.com/rtk-ai/rtk/refs/heads/master/install.sh | sh +``` + +### Cargo + +```bash +cargo install --git https://github.com/rtk-ai/rtk +``` + +### Verificação + +```bash +rtk --version # Deve exibir "rtk 0.28.2" +rtk gain # Deve exibir estatísticas de economia +``` + +## Inicio rapido + +```bash +# 1. Instalar hook para Claude Code (recomendado) +rtk init --global + +# 2. Reiniciar Claude Code, depois testar +git status # Reescrito automaticamente para rtk git status +``` + +## Como funciona + +``` + Sem rtk: Com rtk: + + Claude --git status--> shell --> git Claude --git status--> RTK --> git + ^ | ^ | | + | ~2,000 tokens (bruto) | | ~200 tokens | filtro | + +-----------------------------------+ +------- (filtrado) ---+----------+ +``` + +Quatro estratégias: + +1. **Filtragem inteligente** - Elimina ruído (comentários, espaços, boilerplate) +2. **Agrupamento** - Agrega itens similares (arquivos por diretório, erros por tipo) +3. **Truncamento** - Mantém contexto relevante, elimina redundância +4. **Deduplicação** - Colapsa linhas de log repetidas com contadores + +## Comandos + +### Arquivos +```bash +rtk ls . # Árvore de diretórios otimizada +rtk read file.rs # Leitura inteligente +rtk find "*.rs" . # Resultados compactos +rtk grep "pattern" . # Busca agrupada por arquivo +``` + +### Git +```bash +rtk git status # Status compacto +rtk git log -n 10 # Commits em uma linha +rtk git diff # Diff condensado +rtk git push # -> "ok main" +``` + +### Tests +```bash +rtk jest # Jest compacto +rtk vitest # Vitest compacto +rtk pytest # Tests Python (-90%) +rtk go test # Tests Go (-90%) +rtk cargo test # Tests Rust (-90%) +rtk test # Só falhas (-90%) +``` + +### Build & Lint +```bash +rtk lint # ESLint agrupado por regra +rtk tsc # Erros TypeScript agrupados +rtk cargo build # Build Cargo (-80%) +rtk ruff check # Lint Python (-80%) +``` + +### Análises +```bash +rtk gain # Estatísticas de economia +rtk gain --graph # Gráfico ASCII (30 dias) +rtk discover # Descobrir economias perdidas +``` + +## Documentação + +- **[INSTALL.md](INSTALL.md)** - Guia de instalação detalhado +- **[ARCHITECTURE.md](docs/contributing/ARCHITECTURE.md)** - Arquitetura técnica +- **[CONTRIBUTING.md](CONTRIBUTING.md)** - Guia de contribuição + +## Contribuir + +Contribuições são bem-vindas. Abra uma issue ou PR no [GitHub](https://github.com/rtk-ai/rtk). + +Junte-se à comunidade no [Discord](https://discord.gg/RySmvNF5kF). + +## Licença + +Apache License 2.0 - veja [LICENSE](LICENSE) para detalhes. + +## Aviso Legal + +Veja [DISCLAIMER.md](DISCLAIMER.md). diff --git a/README_zh.md b/README_zh.md index 854ca2314..ef8c53b10 100644 --- a/README_zh.md +++ b/README_zh.md @@ -9,7 +9,7 @@

CI Release - License: MIT + License: Apache 2.0 Discord Homebrew

@@ -165,7 +165,7 @@ rtk discover # 发现遗漏的节省机会 ## 许可证 -MIT 许可证 - 详见 [LICENSE](LICENSE)。 +Apache 2.0 许可证 - 详见 [LICENSE](LICENSE)。 ## 免责声明 diff --git a/docs/guide/getting-started/supported-agents.md b/docs/guide/getting-started/supported-agents.md index b7fb920de..d5a0ad87c 100644 --- a/docs/guide/getting-started/supported-agents.md +++ b/docs/guide/getting-started/supported-agents.md @@ -30,7 +30,7 @@ Agent runs "cargo test" |-------|-----------------|---------------------------| | Claude Code | Shell hook (`PreToolUse`) | Yes | | VS Code Copilot Chat | Shell hook (`PreToolUse`) | Yes | -| GitHub Copilot CLI | Shell hook (deny-with-suggestion) | No (agent retries) | +| GitHub Copilot CLI | Shell hook (`preToolUse` `modifiedArgs`) | Yes | | Cursor | Shell hook (`preToolUse`) | Yes | | Gemini CLI | Rust binary (`BeforeTool`) | Yes | | OpenCode | TypeScript plugin (`tool.execute.before`) | Yes | @@ -61,17 +61,29 @@ rtk init --show # shows hook status ### Cursor ```bash -rtk init --global --cursor +rtk init --global --agent cursor ``` Restart Cursor. The hook uses `preToolUse` with Cursor's `updated_input` format. -### VS Code Copilot Chat +### GitHub Copilot (VS Code Chat + CLI) ```bash -rtk init --global --copilot +rtk init --copilot # project-scoped (.github/hooks/) +rtk init --global --copilot # user-scoped (~/.copilot/hooks/, respects $COPILOT_HOME) ``` +Project-scoped writes `.github/hooks/rtk-rewrite.json` (both hosts get transparent rewrite — VS Code Chat via `updatedInput`, Copilot CLI via `modifiedArgs`) plus the RTK block in `.github/copilot-instructions.md`. User-scoped writes the same hook config to `~/.copilot/hooks/rtk-rewrite.json` and the RTK block to `~/.copilot/copilot-instructions.md` (both respect `$COPILOT_HOME` if set). + +Uninstall: + +```bash +rtk init --uninstall --copilot +rtk init --uninstall --global --copilot +``` + +Removes only RTK's hook file (and, for project, the RTK block in `copilot-instructions.md`). Other files in `.github/hooks/` or `~/.copilot/hooks/` and your own instruction content are untouched. + ### Gemini CLI ```bash @@ -128,7 +140,7 @@ The plugin fails open. If `rtk` is missing at load time, the hook is not registe ### Cline / Roo Code ```bash -rtk init --cline # creates .clinerules in current project +rtk init --agent cline # creates .clinerules in current project ``` Cline reads `.clinerules` as custom instructions. RTK adds guidance telling Cline to prefer `rtk ` over raw commands. @@ -136,13 +148,14 @@ Cline reads `.clinerules` as custom instructions. RTK adds guidance telling Clin ### Windsurf ```bash -rtk init --windsurf # creates .windsurfrules in current project +rtk init --global --agent windsurf # creates .windsurfrules in current project ``` ### Codex CLI ```bash -rtk init --codex # creates AGENTS.md or patches existing one +rtk init --codex # project-scoped (AGENTS.md) +rtk init --global --codex # user-global (~/.codex/AGENTS.md) ``` ### Kilo Code diff --git a/openclaw/README.md b/openclaw/README.md index 301d7c0fa..480ec856f 100644 --- a/openclaw/README.md +++ b/openclaw/README.md @@ -83,4 +83,4 @@ Handled by `rtk rewrite` guards: ## License -MIT -- same as RTK. +Apache 2.0 -- same as RTK. diff --git a/openclaw/openclaw.plugin.json b/openclaw/openclaw.plugin.json index 3fce418d7..e08d11ba1 100644 --- a/openclaw/openclaw.plugin.json +++ b/openclaw/openclaw.plugin.json @@ -4,7 +4,7 @@ "version": "1.0.0", "description": "Transparently rewrites shell commands to their RTK equivalents for 60-90% LLM token savings", "homepage": "https://github.com/rtk-ai/rtk", - "license": "MIT", + "license": "Apache-2.0", "configSchema": { "type": "object", "additionalProperties": false, diff --git a/openclaw/package.json b/openclaw/package.json index 18d359ff4..aa0b099f6 100644 --- a/openclaw/package.json +++ b/openclaw/package.json @@ -3,7 +3,7 @@ "version": "1.0.0", "description": "RTK plugin for OpenClaw — rewrites shell commands for 60-90% LLM token savings", "main": "index.ts", - "license": "MIT", + "license": "Apache-2.0", "repository": { "type": "git", "url": "https://github.com/rtk-ai/rtk", diff --git a/src/cmds/git/gh_cmd.rs b/src/cmds/git/gh_cmd.rs index c22c16b40..01cf2ac39 100644 --- a/src/cmds/git/gh_cmd.rs +++ b/src/cmds/git/gh_cmd.rs @@ -162,6 +162,16 @@ fn extract_identifier_and_extra_args(args: &[String]) -> Option<(String, Vec (Option, Vec) { + match extract_identifier_and_extra_args(args) { + Some((id, extra)) => (Some(id), extra), + None => (None, args.to_vec()), + } +} + fn run_gh_json(cmd: Command, label: &str, filter_fn: F) -> Result where F: Fn(&Value) -> String, @@ -319,27 +329,32 @@ fn pr_status_json_fields() -> &'static str { } fn view_pr(args: &[String], _verbose: u8, ultra_compact: bool) -> Result { - let (pr_number, extra_args) = match extract_identifier_and_extra_args(args) { - Some(result) => result, - None => return Err(anyhow::anyhow!("PR number required")), - }; + // `gh pr view` without an identifier defaults to the PR for the current branch. + let (pr_number_opt, extra_args) = parse_optional_identifier(args); if should_passthrough_pr_view(&extra_args) { - return run_passthrough_with_extra("gh", &["pr", "view", &pr_number], &extra_args); + let mut base: Vec<&str> = vec!["pr", "view"]; + if let Some(id) = pr_number_opt.as_deref() { + base.push(id); + } + return run_passthrough_with_extra("gh", &base, &extra_args); } let mut cmd = resolved_command("gh"); + cmd.args(["pr", "view"]); + if let Some(id) = pr_number_opt.as_deref() { + cmd.arg(id); + } cmd.args([ - "pr", - "view", - &pr_number, "--json", "number,title,state,author,body,url,mergeable,reviews,statusCheckRollup", ]); for arg in &extra_args { cmd.arg(arg); } - run_gh_json(cmd, &format!("pr view {}", pr_number), |json| { - format_pr_view(json, ultra_compact) - }) + let label = match pr_number_opt.as_deref() { + Some(id) => format!("pr view {}", id), + None => "pr view".to_string(), + }; + run_gh_json(cmd, &label, |json| format_pr_view(json, ultra_compact)) } fn format_pr_view(json: &Value, ultra_compact: bool) -> String { @@ -419,6 +434,8 @@ fn format_pr_view(json: &Value, ultra_compact: bool) -> String { for line in body_filtered.lines() { out.push_str(&format!(" {}\n", line)); } + } else { + out.push_str("\n (body contained only badges/images/comments)\n"); } } } @@ -427,19 +444,24 @@ fn format_pr_view(json: &Value, ultra_compact: bool) -> String { } fn pr_checks(args: &[String], _verbose: u8, _ultra_compact: bool) -> Result { - let (pr_number, extra_args) = match extract_identifier_and_extra_args(args) { - Some(result) => result, - None => return Err(anyhow::anyhow!("PR number required")), - }; + // `gh pr checks` without an identifier defaults to the PR for the current branch. + let (pr_number_opt, extra_args) = parse_optional_identifier(args); let mut cmd = resolved_command("gh"); - cmd.args(["pr", "checks", &pr_number]); + cmd.args(["pr", "checks"]); + if let Some(id) = pr_number_opt.as_deref() { + cmd.arg(id); + } for arg in &extra_args { cmd.arg(arg); } + let label = match pr_number_opt.as_deref() { + Some(id) => format!("pr checks {}", id), + None => "pr checks".to_string(), + }; runner::run_filtered( cmd, "gh", - &format!("pr checks {}", pr_number), + &label, format_pr_checks, RunOptions::stdout_only() .early_exit_on_failure() @@ -623,27 +645,29 @@ fn format_issue_list(json: &Value, ultra_compact: bool) -> String { } fn view_issue(args: &[String], _verbose: u8) -> Result { - let (issue_number, extra_args) = match extract_identifier_and_extra_args(args) { - Some(result) => result, - None => return Err(anyhow::anyhow!("Issue number required")), - }; + // Let gh emit its own error message when the identifier is missing rather than pre-rejecting. + let (issue_number_opt, extra_args) = parse_optional_identifier(args); if should_passthrough_issue_view(&extra_args) { - return run_passthrough_with_extra("gh", &["issue", "view", &issue_number], &extra_args); + let mut base: Vec<&str> = vec!["issue", "view"]; + if let Some(id) = issue_number_opt.as_deref() { + base.push(id); + } + return run_passthrough_with_extra("gh", &base, &extra_args); } let mut cmd = resolved_command("gh"); - cmd.args([ - "issue", - "view", - &issue_number, - "--json", - "number,title,state,author,body,url", - ]); + cmd.args(["issue", "view"]); + if let Some(id) = issue_number_opt.as_deref() { + cmd.arg(id); + } + cmd.args(["--json", "number,title,state,author,body,url"]); for arg in &extra_args { cmd.arg(arg); } - run_gh_json(cmd, &format!("issue view {}", issue_number), |json| { - format_issue_view(json) - }) + let label = match issue_number_opt.as_deref() { + Some(id) => format!("issue view {}", id), + None => "issue view".to_string(), + }; + run_gh_json(cmd, &label, format_issue_view) } fn format_issue_view(json: &Value) -> String { @@ -672,6 +696,8 @@ fn format_issue_view(json: &Value) -> String { for line in body_filtered.lines() { out.push_str(&format!(" {}\n", line)); } + } else { + out.push_str("\n Description: (body contained only badges/images/comments)\n"); } } } @@ -753,23 +779,32 @@ fn should_passthrough_run_view(extra_args: &[String]) -> bool { } fn view_run(args: &[String], _verbose: u8) -> Result { - let (run_id, extra_args) = match extract_identifier_and_extra_args(args) { - Some(result) => result, - None => return Err(anyhow::anyhow!("Run ID required")), - }; + // `gh run view` without an identifier opens an interactive picker — defer to gh. + let (run_id_opt, extra_args) = parse_optional_identifier(args); if should_passthrough_run_view(&extra_args) { - return run_passthrough_with_extra("gh", &["run", "view", &run_id], &extra_args); + let mut base: Vec<&str> = vec!["run", "view"]; + if let Some(id) = run_id_opt.as_deref() { + base.push(id); + } + return run_passthrough_with_extra("gh", &base, &extra_args); } let mut cmd = resolved_command("gh"); - cmd.args(["run", "view", &run_id]); + cmd.args(["run", "view"]); + if let Some(id) = run_id_opt.as_deref() { + cmd.arg(id); + } for arg in &extra_args { cmd.arg(arg); } - let run_id_owned = run_id.clone(); + let label = match run_id_opt.as_deref() { + Some(id) => format!("run view {}", id), + None => "run view".to_string(), + }; + let run_id_owned = run_id_opt.unwrap_or_default(); runner::run_filtered( cmd, "gh", - &format!("run view {}", run_id), + &label, move |stdout| format_run_view(stdout, &run_id_owned), RunOptions::stdout_only() .early_exit_on_failure() @@ -781,7 +816,11 @@ fn format_run_view(stdout: &str, run_id: &str) -> String { let mut out = String::new(); let mut in_jobs = false; - out.push_str(&format!("Workflow Run #{}\n", run_id)); + if run_id.is_empty() { + out.push_str("Workflow Run\n"); + } else { + out.push_str(&format!("Workflow Run #{}\n", run_id)); + } for line in stdout.lines() { if line.contains("JOBS") { in_jobs = true; @@ -1077,6 +1116,35 @@ mod tests { assert!(extract_identifier_and_extra_args(&args).is_none()); } + // --- parse_optional_identifier tests --- + + #[test] + fn test_parse_optional_identifier_empty_yields_no_id() { + // `gh pr view` (no args) must surface as (None, []) so the caller + // hands the request to gh, which resolves the current branch's PR. + let (id, extra) = parse_optional_identifier(&[]); + assert!(id.is_none()); + assert!(extra.is_empty()); + } + + #[test] + fn test_parse_optional_identifier_only_flags_preserves_flags() { + // Regression: `gh pr view -R rtk-ai/rtk` previously triggered + // "PR number required". Now flags must round-trip into `extra`. + let args: Vec = vec!["-R".into(), "rtk-ai/rtk".into()]; + let (id, extra) = parse_optional_identifier(&args); + assert!(id.is_none()); + assert_eq!(extra, vec!["-R", "rtk-ai/rtk"]); + } + + #[test] + fn test_parse_optional_identifier_with_id_matches_extract() { + let args: Vec = vec!["-R".into(), "rtk-ai/rtk".into(), "42".into()]; + let (id, extra) = parse_optional_identifier(&args); + assert_eq!(id.as_deref(), Some("42")); + assert_eq!(extra, vec!["-R", "rtk-ai/rtk"]); + } + #[test] fn test_extract_identifier_with_web_flag() { let args: Vec = vec!["123".into(), "--web".into()]; @@ -1108,6 +1176,21 @@ mod tests { assert!(!should_passthrough_run_view(&[])); } + #[test] + fn test_format_run_view_with_id() { + let output = format_run_view("", "12345"); + assert!(output.starts_with("Workflow Run #12345\n")); + } + + #[test] + fn test_format_run_view_without_id() { + // `gh run view` with no arg opens an interactive picker — the captured run id + // is empty, so the header must not render an empty `#`. + let output = format_run_view("", ""); + assert!(output.starts_with("Workflow Run\n")); + assert!(!output.contains("#\n")); + } + #[test] fn test_run_view_no_passthrough_other_flags() { assert!(!should_passthrough_run_view(&["--web".into()])); @@ -1468,4 +1551,87 @@ ___ assert!(result.contains("## Test Plan")); assert!(result.contains("Filter HTML comments")); } + + #[test] + fn test_format_pr_view_body_badges_only_shows_fallback_note() { + // PR body that filter_markdown_body would strip entirely: + // HTML comments, badge links, image-only lines, horizontal rules. + let body = "\n\ + [![CI](https://shields.io/badge.svg)](https://ci.example.com)\n\ + ![screenshot](https://example.com/img.png)\n\ + ---\n"; + let json = serde_json::json!({ + "number": 42, + "title": "Test PR", + "state": "OPEN", + "author": { "login": "octocat" }, + "url": "https://github.com/foo/bar/pull/42", + "mergeable": "MERGEABLE", + "body": body, + }); + let out = format_pr_view(&json, false); + assert!( + out.contains("(body contained only badges/images/comments)"), + "expected fallback note when body filters to empty, got:\n{}", + out + ); + } + + #[test] + fn test_format_pr_view_body_with_content_no_fallback_note() { + // Sanity check: real content should NOT trigger the fallback note. + let json = serde_json::json!({ + "number": 42, + "title": "Test PR", + "state": "OPEN", + "author": { "login": "octocat" }, + "url": "https://github.com/foo/bar/pull/42", + "mergeable": "MERGEABLE", + "body": "## Summary\nFix the thing.\n", + }); + let out = format_pr_view(&json, false); + assert!( + !out.contains("(body contained only badges/images/comments)"), + "fallback note should not fire when body has real content, got:\n{}", + out + ); + assert!(out.contains("## Summary")); + assert!(out.contains("Fix the thing.")); + } + + #[test] + fn test_format_pr_view_empty_body_no_fallback_note() { + // Empty body should not trigger the note either (nothing to flag). + let json = serde_json::json!({ + "number": 42, + "title": "Test PR", + "state": "OPEN", + "author": { "login": "octocat" }, + "url": "https://github.com/foo/bar/pull/42", + "mergeable": "MERGEABLE", + "body": "", + }); + let out = format_pr_view(&json, false); + assert!(!out.contains("(body contained only badges/images/comments)")); + } + + #[test] + fn test_format_issue_view_body_badges_only_shows_fallback_note() { + let body = "\n\ + [![status](https://shields.io/s.svg)](https://example.com)\n"; + let json = serde_json::json!({ + "number": 99, + "title": "Test Issue", + "state": "OPEN", + "author": { "login": "octocat" }, + "url": "https://github.com/foo/bar/issues/99", + "body": body, + }); + let out = format_issue_view(&json); + assert!( + out.contains("(body contained only badges/images/comments)"), + "expected fallback note when issue body filters to empty, got:\n{}", + out + ); + } } diff --git a/src/cmds/git/git.rs b/src/cmds/git/git.rs index eaf8d8b5f..b8ccfe923 100644 --- a/src/cmds/git/git.rs +++ b/src/cmds/git/git.rs @@ -1,5 +1,6 @@ //! Filters git output — log, status, diff, and more — keeping just the essential info. +use crate::core::args_utils; use crate::core::stream::{ self, exec_capture, CaptureResult, FilterMode, LineHandler, LineStreamFilter, StdinMode, }; @@ -102,65 +103,6 @@ pub fn run( } } -/// Re-insert `--` before the first path-like argument when clap has consumed it. -/// -/// clap's `trailing_var_arg = true` silently drops `--` when it appears as the -/// first positional argument (before any other positional). This means: -/// `rtk git diff -- file` → args = ["file"] (clap ate `--`) -/// `rtk git diff HEAD -- file` → args = ["HEAD", "--", "file"] (preserved) -/// -/// Without the `--` separator git may treat an unambiguous path as a revision and -/// emit "fatal: ambiguous argument". We re-insert `--` before the first path-like -/// argument; see `normalize_diff_args_impl` for the detection rules. -fn normalize_diff_args(args: &[String]) -> Vec { - normalize_diff_args_impl(args, |p| std::path::Path::new(p).exists()) -} - -/// Testable core of `normalize_diff_args` — accepts an injectable filesystem existence checker. -/// -/// The path-detection logic is: -/// 1. Explicit path prefixes (`.`, `~`) → always a path, no filesystem check needed. -/// 2. Contains path separator (`/`, `\`) → use `path_exists` to distinguish branch names -/// (e.g. `feature/auth`) from real paths (e.g. `src/main.rs`). -/// 3. Bare word with no separator → never a path (avoids injecting `--` when a file -/// happens to share a name with a branch or ref, e.g. a file named `main`). -fn normalize_diff_args_impl(args: &[String], path_exists: F) -> Vec -where - F: Fn(&str) -> bool, -{ - // Already has `--` — nothing to do - if args.iter().any(|a| a == "--") { - return args.to_vec(); - } - let path_start = args.iter().position(|arg| { - if arg.starts_with('-') { - return false; - } - // Explicit path prefixes — always treat as path regardless of existence - if arg.starts_with('.') || arg.starts_with('~') { - return true; - } - // Contains path separator — use filesystem check to distinguish - // branch names (feature/auth) from real paths (src/main.rs) - if arg.contains('/') || arg.contains('\\') { - return path_exists(arg); - } - // Bare word (no separator, no special prefix) — never inject `--` - // This avoids misidentifying a ref/branch as a path even if a same-named - // file happens to exist on disk. - false - }); - match path_start { - Some(idx) => { - let mut out = args[..idx].to_vec(); - out.push("--".to_string()); - out.extend_from_slice(&args[idx..]); - out - } - None => args.to_vec(), - } -} - fn run_diff( args: &[String], max_lines: Option, @@ -170,7 +112,7 @@ fn run_diff( let timer = tracking::TimedExecution::start(); // Re-insert `--` when clap's trailing_var_arg consumed it (issue #1215) - let args = &normalize_diff_args(args); + let args = &args_utils::restore_double_dash(args); // Check if user wants stat output let wants_stat = args @@ -1911,8 +1853,14 @@ mod tests { assert!(uses_compact_status_path(&["-b".to_string()])); assert!(uses_compact_status_path(&["--branch".to_string()])); assert!(uses_compact_status_path(&["-sb".to_string()])); - assert!(uses_compact_status_path(&["-s".to_string(), "-b".to_string()])); - assert!(uses_compact_status_path(&["--short".to_string(), "--branch".to_string()])); + assert!(uses_compact_status_path(&[ + "-s".to_string(), + "-b".to_string() + ])); + assert!(uses_compact_status_path(&[ + "--short".to_string(), + "--branch".to_string() + ])); assert!(!uses_compact_status_path(&["-s".to_string()])); assert!(!uses_compact_status_path(&["--short".to_string()])); assert!(!uses_compact_status_path(&["--porcelain".to_string()])); @@ -2002,134 +1950,6 @@ mod tests { ); } - // ----- normalize_diff_args (issue #1215 + branch-name fix #1431) ----- - // - // Tests use normalize_diff_args_impl with a mock path-existence checker so - // they don't depend on the real filesystem. - - fn exists_mock<'a>(existing: &'a [&'a str]) -> impl Fn(&str) -> bool + 'a { - move |p| existing.contains(&p) - } - - /// Baseline: `--` already present → no-op, args unchanged. - #[test] - fn test_normalize_diff_args_noop_when_separator_present() { - let args = vec![ - "HEAD".to_string(), - "--".to_string(), - "src/main.rs".to_string(), - ]; - assert_eq!(normalize_diff_args_impl(&args, exists_mock(&[])), args); - } - - /// Core regression (issue #1215): clap ate `--` before a real file path. - /// When the path exists on disk, `--` must be re-inserted. - #[test] - fn test_normalize_diff_args_reinserts_separator_before_existing_path() { - let args = vec!["apps/client/frontend/src/MyComponent.tsx".to_string()]; - let normalized = normalize_diff_args_impl( - &args, - exists_mock(&["apps/client/frontend/src/MyComponent.tsx"]), - ); - assert_eq!( - normalized, - vec![ - "--".to_string(), - "apps/client/frontend/src/MyComponent.tsx".to_string() - ], - "-- must be injected before an existing path" - ); - } - - /// Ref before path: ["HEAD", "src/foo.rs"] where src/foo.rs exists → inject after HEAD. - #[test] - fn test_normalize_diff_args_reinserts_separator_after_ref() { - let args = vec!["HEAD".to_string(), "src/foo.rs".to_string()]; - let normalized = normalize_diff_args_impl(&args, exists_mock(&["src/foo.rs"])); - assert_eq!( - normalized, - vec![ - "HEAD".to_string(), - "--".to_string(), - "src/foo.rs".to_string() - ] - ); - } - - /// Flags before path: ["--cached", "src/foo.rs"] where src/foo.rs exists. - #[test] - fn test_normalize_diff_args_reinserts_separator_after_flag() { - let args = vec!["--cached".to_string(), "src/foo.rs".to_string()]; - let normalized = normalize_diff_args_impl(&args, exists_mock(&["src/foo.rs"])); - assert_eq!( - normalized, - vec![ - "--cached".to_string(), - "--".to_string(), - "src/foo.rs".to_string() - ] - ); - } - - /// Pure flags (no paths) → no injection. - #[test] - fn test_normalize_diff_args_no_injection_for_pure_flags() { - let args = vec!["--stat".to_string(), "--cached".to_string()]; - assert_eq!(normalize_diff_args_impl(&args, exists_mock(&[])), args); - } - - /// Dotfile that exists on disk → inject `--`. - #[test] - fn test_normalize_diff_args_dotfile_is_path() { - let args = vec![".gitignore".to_string()]; - let normalized = normalize_diff_args_impl(&args, exists_mock(&[".gitignore"])); - assert_eq!(normalized, vec!["--".to_string(), ".gitignore".to_string()]); - } - - /// A bare ref (HEAD) that doesn't exist as a file → no injection. - #[test] - fn test_normalize_diff_args_no_injection_for_bare_ref() { - let args = vec!["HEAD".to_string()]; - assert_eq!(normalize_diff_args_impl(&args, exists_mock(&[])), args); - } - - /// Branch name with `/` that does NOT exist as a file → no injection. - /// Regression for issue #1431: `rtk git diff feature/user-auth` must not inject `--`. - #[test] - fn test_normalize_diff_args_no_injection_for_branch_with_slash() { - let args = vec!["feature/user-auth".to_string()]; - assert_eq!( - normalize_diff_args_impl(&args, exists_mock(&[])), - args, - "branch names containing '/' must not trigger -- injection" - ); - } - - /// Range syntax with `/` → no injection. - /// Regression: `rtk git diff main...feature/user-auth` produced no output. - #[test] - fn test_normalize_diff_args_no_injection_for_range_with_slash() { - let args = vec!["main...feature/user-auth".to_string()]; - assert_eq!( - normalize_diff_args_impl(&args, exists_mock(&[])), - args, - "revision ranges like main...feature/user-auth must not trigger -- injection" - ); - } - - /// Bare word that happens to exist as a file on disk → still no injection. - /// A file named "main" must not cause `--` to be injected when the user - /// intends `rtk git diff main` as a branch comparison. - #[test] - fn test_normalize_diff_args_no_injection_for_bare_word_even_if_file_exists() { - let args = vec!["main".to_string()]; - assert_eq!( - normalize_diff_args_impl(&args, exists_mock(&["main"])), - args, - "bare words must never trigger -- injection even when a same-named file exists" - ); - } - #[test] fn test_is_blob_show_arg() { assert!(is_blob_show_arg("develop:modules/pairs_backtest.py")); diff --git a/src/cmds/git/glab_cmd.rs b/src/cmds/git/glab_cmd.rs index a0282a221..56885f07c 100644 --- a/src/cmds/git/glab_cmd.rs +++ b/src/cmds/git/glab_cmd.rs @@ -212,6 +212,16 @@ fn extract_identifier_and_extra_args(args: &[String]) -> Option<(String, Vec (Option, Vec) { + match extract_identifier_and_extra_args(args) { + Some((id, extra)) => (Some(id), extra), + None => (None, args.to_vec()), + } +} + /// Check if user explicitly requested JSON/custom output format. /// When present, passthrough to avoid double JSON injection. fn has_output_flag(args: &[String]) -> bool { @@ -409,24 +419,32 @@ fn format_mr_view(json: &Value, ultra_compact: bool) -> String { } fn mr_view(args: &[String], _verbose: u8, ultra_compact: bool) -> Result { - let (mr_number, extra_args) = match extract_identifier_and_extra_args(args) { - Some(pair) => pair, - None => return Err(anyhow::anyhow!("MR number required")), - }; + // `glab mr view` without an identifier defaults to the MR for the current branch. + let (mr_number_opt, extra_args) = parse_optional_identifier(args); // Passthrough for --web, --comments, or explicit output format if should_passthrough_view(&extra_args) { - return run_passthrough_with_extra("glab", &["mr", "view", &mr_number], &extra_args); + let mut base: Vec<&str> = vec!["mr", "view"]; + if let Some(id) = mr_number_opt.as_deref() { + base.push(id); + } + return run_passthrough_with_extra("glab", &base, &extra_args); } let mut cmd = resolved_command("glab"); - cmd.args(["mr", "view", &mr_number, "-F", "json"]); + cmd.args(["mr", "view"]); + if let Some(id) = mr_number_opt.as_deref() { + cmd.arg(id); + } + cmd.args(["-F", "json"]); for arg in &extra_args { cmd.arg(arg); } - run_glab_json(cmd, &format!("mr view {}", mr_number), |json| { - format_mr_view(json, ultra_compact) - }) + let label = match mr_number_opt.as_deref() { + Some(id) => format!("mr view {}", id), + None => "mr view".to_string(), + }; + run_glab_json(cmd, &label, |json| format_mr_view(json, ultra_compact)) } fn mr_create(args: &[String], _verbose: u8) -> Result { @@ -603,25 +621,31 @@ fn format_issue_view(json: &Value) -> String { } fn issue_view(args: &[String], _verbose: u8) -> Result { - let (issue_number, extra_args) = match extract_identifier_and_extra_args(args) { - Some(pair) => pair, - None => return Err(anyhow::anyhow!("Issue number required")), - }; + // Let glab emit its own error message when the identifier is missing rather than pre-rejecting. + let (issue_number_opt, extra_args) = parse_optional_identifier(args); if should_passthrough_view(&extra_args) { - return run_passthrough_with_extra("glab", &["issue", "view", &issue_number], &extra_args); + let mut base: Vec<&str> = vec!["issue", "view"]; + if let Some(id) = issue_number_opt.as_deref() { + base.push(id); + } + return run_passthrough_with_extra("glab", &base, &extra_args); } let mut cmd = resolved_command("glab"); - cmd.args(["issue", "view", &issue_number, "-F", "json"]); + cmd.args(["issue", "view"]); + if let Some(id) = issue_number_opt.as_deref() { + cmd.arg(id); + } + cmd.args(["-F", "json"]); for arg in &extra_args { cmd.arg(arg); } - run_glab_json( - cmd, - &format!("issue view {}", issue_number), - format_issue_view, - ) + let label = match issue_number_opt.as_deref() { + Some(id) => format!("issue view {}", id), + None => "issue view".to_string(), + }; + run_glab_json(cmd, &label, format_issue_view) } // ── CI/Pipeline subcommands ───────────────────────────────────────────── @@ -1235,6 +1259,35 @@ mod tests { assert!(extract_identifier_and_extra_args(&args).is_none()); } + // ── parse_optional_identifier tests ───────────────────────────────── + + #[test] + fn test_parse_optional_identifier_empty_yields_no_id() { + // `glab mr view` (no args) must surface as (None, []) so the caller + // hands the request to glab, which resolves the current branch's MR. + let (id, extra) = parse_optional_identifier(&[]); + assert!(id.is_none()); + assert!(extra.is_empty()); + } + + #[test] + fn test_parse_optional_identifier_only_flags_preserves_flags() { + // Regression: `glab mr view -R group/project` previously triggered + // "MR number required". Now flags must round-trip into `extra`. + let args: Vec = vec!["-R".into(), "group/project".into()]; + let (id, extra) = parse_optional_identifier(&args); + assert!(id.is_none()); + assert_eq!(extra, vec!["-R", "group/project"]); + } + + #[test] + fn test_parse_optional_identifier_with_id_matches_extract() { + let args: Vec = vec!["-R".into(), "group/project".into(), "42".into()]; + let (id, extra) = parse_optional_identifier(&args); + assert_eq!(id.as_deref(), Some("42")); + assert_eq!(extra, vec!["-R", "group/project"]); + } + // ── has_output_flag tests ─────────────────────────────────────────── #[test] diff --git a/src/cmds/go/go_cmd.rs b/src/cmds/go/go_cmd.rs index e962e3e1c..a39e8bb1b 100644 --- a/src/cmds/go/go_cmd.rs +++ b/src/cmds/go/go_cmd.rs @@ -92,11 +92,11 @@ pub fn run_build(args: &[String], verbose: u8) -> Result { eprintln!("Running: go build {}", args.join(" ")); } - runner::run_filtered( + runner::run_filtered_with_exit( cmd, "go build", &args.join(" "), - filter_go_build, + filter_go_build_with_exit, crate::core::runner::RunOptions::with_tee("go_build"), ) } @@ -571,6 +571,10 @@ fn is_go_test_failure_line(line: &str) -> bool { /// Filter go build output - show only errors pub(crate) fn filter_go_build(output: &str) -> String { + filter_go_build_with_exit(output, 0) +} + +fn filter_go_build_with_exit(output: &str, exit_code: i32) -> String { let mut errors: Vec = Vec::new(); for line in output.lines() { @@ -581,7 +585,11 @@ pub(crate) fn filter_go_build(output: &str) -> String { } if errors.is_empty() { - return "Go build: Success".to_string(); + return if exit_code == 0 { + "Go build: Success".to_string() + } else { + format_go_build_failure(output, exit_code) + }; } let mut result = String::new(); @@ -604,6 +612,37 @@ pub(crate) fn filter_go_build(output: &str) -> String { result.trim().to_string() } +fn format_go_build_failure(output: &str, exit_code: i32) -> String { + let lines: Vec = output + .lines() + .map(str::trim) + .filter(|line| !line.is_empty()) + .map(str::to_string) + .collect(); + + if lines.is_empty() { + return format!("Go build: failed (exit {})", exit_code); + } + + let mut result = String::new(); + result.push_str(&format!("Go build: failed (exit {})\n", exit_code)); + result.push_str("═══════════════════════════════════════\n"); + + const MAX_GO_BUILD_ERRORS: usize = CAP_ERRORS; + for (i, line) in lines.iter().take(MAX_GO_BUILD_ERRORS).enumerate() { + result.push_str(&format!("{}. {}\n", i + 1, truncate(line, 120))); + } + + if lines.len() > MAX_GO_BUILD_ERRORS { + result.push_str(&format!( + "\n… +{} more output lines\n", + lines.len() - MAX_GO_BUILD_ERRORS + )); + } + + result.trim().to_string() +} + fn is_go_build_error_line(line: &str) -> bool { let trimmed = line.trim(); if trimmed.is_empty() { @@ -646,6 +685,7 @@ fn is_go_build_error_line(line: &str) -> bool { "go: build failed", "go: error ", "error: ", + "pattern ", "go: updates to go.mod needed", "go: inconsistent vendoring", "no go files in ", @@ -991,6 +1031,28 @@ go: cannot load module missing listed in go.work file: open missing/go.mod: no s assert!(result.contains("go: cannot load module missing listed in go.work file")); } + #[test] + fn test_filter_go_build_preserves_package_pattern_errors() { + let output = r#"pattern ./...: directory prefix . does not contain main module or its selected dependencies +pattern ./...: directory prefix . does not contain modules listed in go.work or their selected dependencies"#; + + let result = filter_go_build(output); + assert!(result.contains("2 errors")); + assert!(result.contains("does not contain main module")); + assert!(result.contains("does not contain modules listed in go.work")); + assert!(!result.contains("Success")); + } + + #[test] + fn test_filter_go_build_nonzero_exit_never_reports_success() { + let output = "opaque go build failure from stderr"; + + let result = filter_go_build_with_exit(output, 1); + assert!(result.contains("Go build: failed (exit 1)")); + assert!(result.contains(output)); + assert!(!result.contains("Success")); + } + #[test] fn test_filter_go_vet_no_issues() { let output = ""; diff --git a/src/cmds/rust/cargo_cmd.rs b/src/cmds/rust/cargo_cmd.rs index 5b8152ce0..8726de40e 100644 --- a/src/cmds/rust/cargo_cmd.rs +++ b/src/cmds/rust/cargo_cmd.rs @@ -1,5 +1,6 @@ //! Filters cargo output — build errors, test results, clippy warnings. +use crate::core::args_utils; use crate::core::runner; use crate::core::stream::{BlockHandler, BlockStreamFilter, StreamFilter}; use crate::core::truncate::{CAP_ERRORS, CAP_LIST, CAP_WARNINGS}; @@ -31,45 +32,6 @@ pub fn run(cmd: CargoCommand, args: &[String], verbose: u8) -> Result { } } -/// Reconstruct args with `--` separator preserved from the original command line. -/// Clap strips `--` from parsed args, but cargo subcommands need it to separate -/// their own flags from test runner flags (e.g. `cargo test -- --nocapture`). -fn restore_double_dash(args: &[String]) -> Vec { - let raw_args: Vec = std::env::args().collect(); - restore_double_dash_with_raw(args, &raw_args) -} - -/// Testable version that takes raw_args explicitly. -fn restore_double_dash_with_raw(args: &[String], raw_args: &[String]) -> Vec { - if args.is_empty() { - return args.to_vec(); - } - - // If args already contain `--` (Clap preserved it), no restoration needed - if args.iter().any(|a| a == "--") { - return args.to_vec(); - } - - // Find `--` in the original command line - let sep_pos = match raw_args.iter().position(|a| a == "--") { - Some(pos) => pos, - None => return args.to_vec(), - }; - - // Count how many of our parsed args appeared before `--` in the original. - // Args before `--` are positional (e.g. test name), args after are flags. - let args_before_sep = raw_args[..sep_pos] - .iter() - .filter(|a| args.contains(a)) - .count(); - - let mut result = Vec::with_capacity(args.len() + 1); - result.extend_from_slice(&args[..args_before_sep]); - result.push("--".to_string()); - result.extend_from_slice(&args[args_before_sep..]); - result -} - // --- Stream handlers --- struct CargoBuildHandler { @@ -291,7 +253,7 @@ where let mut cmd = resolved_command("cargo"); cmd.arg(subcommand); - let restored_args = restore_double_dash(args); + let restored_args = args_utils::restore_double_dash(args); for arg in &restored_args { cmd.arg(arg); } @@ -318,7 +280,7 @@ fn run_cargo_streamed( let mut cmd = resolved_command("cargo"); cmd.arg(subcommand); - let restored_args = restore_double_dash(args); + let restored_args = args_utils::restore_double_dash(args); for arg in &restored_args { cmd.arg(arg); } @@ -1275,6 +1237,7 @@ pub fn run_passthrough(args: &[OsString], verbose: u8) -> Result { #[cfg(test)] mod tests { use super::*; + use crate::core::args_utils::restore_double_dash_with_raw; #[test] fn test_restore_double_dash_with_separator() { diff --git a/src/cmds/system/grep_cmd.rs b/src/cmds/system/grep_cmd.rs index 6a33cf3a4..3f7d9a327 100644 --- a/src/cmds/system/grep_cmd.rs +++ b/src/cmds/system/grep_cmd.rs @@ -33,7 +33,10 @@ pub fn run( // Without this, rg returns 0 matches for files in .gitignore, causing // false negatives that make AI agents draw wrong conclusions. // Using --no-ignore-vcs (not --no-ignore) so .ignore/.rgignore are still respected. - rg_cmd.args(["-n", "--no-heading", "--no-ignore-vcs", &rg_pattern, path]); + // -H: always emit the filename. + // -0: NUL-separate filename. Allows the parser to disambiguate filenames or + // content containing `:digits:` patterns (issue #1436). + rg_cmd.args(["-nH0", "--no-heading", "--no-ignore-vcs", &rg_pattern, path]); if let Some(ft) = file_type { rg_cmd.arg("--type").arg(ft); @@ -50,8 +53,8 @@ pub fn run( let result = exec_capture(&mut rg_cmd) .or_else(|_| { let mut grep_cmd = resolved_command("grep"); - //When we fall back to grep,include all args, not just -rn. - grep_cmd.args(["-rn", pattern, path]).args(extra_args); + // When we fall back to grep, include all args, not just -rnHZ. + grep_cmd.args(["-rnHZ", pattern, path]).args(extra_args); exec_capture(&mut grep_cmd) }) .context("grep/rg failed")?; @@ -108,18 +111,9 @@ pub fn run( let mut by_file: HashMap> = HashMap::new(); for line in result.stdout.lines() { - let parts: Vec<&str> = line.splitn(3, ':').collect(); - - let (file, line_num, content) = if parts.len() == 3 { - let ln = parts[1].parse().unwrap_or(0); - (parts[0].to_string(), ln, parts[2]) - } else if parts.len() == 2 { - let ln = parts[0].parse().unwrap_or(0); - (path.to_string(), ln, parts[1]) - } else { + let Some((file, line_num, content)) = parse_match_line(line) else { continue; }; - let cleaned = clean_line(content, max_line_len, context_re.as_ref(), pattern); by_file.entry(file).or_default().push((line_num, cleaned)); } @@ -166,6 +160,28 @@ pub fn run( Ok(exit_code) } +/// Parses a single rg/grep match line of the form `file\0line_number:content`. +/// +/// Requires the underlying command to be invoked with `-0` (rg) or `-Z` (grep) +/// so the filename is NUL-separated from `line:content`. NUL cannot appear in +/// file paths, so the parser is unambiguous regardless of: +/// - content with `:` or `::` (e.g. `ClassRegistry::init(...)`, issue #1436); +/// - paths with embedded `:` (Windows drive letters, weird filenames like +/// `badly_named:52:file.txt`). +/// +/// Returns `None` for lines that do not match the expected shape (e.g. rg +/// `-A`/`-B` context lines that use `-` as separator). +fn parse_match_line(line: &str) -> Option<(String, usize, &str)> { + lazy_static::lazy_static! { + static ref MATCH_LINE_RE: Regex = Regex::new(r"^([^\x00]+)\x00(\d+):(.*)$").unwrap(); + } + MATCH_LINE_RE.captures(line).and_then(|caps| { + let (_, [file, line_num, content]) = caps.extract(); + let line_num: usize = line_num.parse().ok()?; + Some((file.to_string(), line_num, content)) + }) +} + fn has_format_flag(extra_args: &[String]) -> bool { extra_args.iter().any(|arg| { matches!( @@ -391,6 +407,87 @@ mod tests { // If rg is not installed, skip gracefully (test still passes) } + // --- issue #1436: parse_match_line robustness --- + // Input shape is `file\0line:content` (rg --null / grep -Z). + + #[test] + fn test_parse_match_line_simple() { + let line = "file.php\x0010:use Foo\\Bar;"; + let (file, line_num, content) = parse_match_line(line).unwrap(); + assert_eq!(file, "file.php"); + assert_eq!(line_num, 10); + assert_eq!(content, "use Foo\\Bar;"); + } + + // Issue #1436 reproducer: content with `::` must not split into a phantom + // file bucket. With NUL separation between file and line:content, content + // colons are irrelevant to the parser. + #[test] + fn test_parse_match_line_content_with_double_colon() { + let line = "externalImportShell.class.php\x0081: $this->queueProcessModel = ClassRegistry::init('Collections.QueueProcess');"; + let (file, line_num, content) = parse_match_line(line).unwrap(); + assert_eq!(file, "externalImportShell.class.php"); + assert_eq!(line_num, 81); + assert_eq!( + content, + " $this->queueProcessModel = ClassRegistry::init('Collections.QueueProcess');" + ); + } + + // Windows abs-path safety: drive letter + backslashes must not break the + // parser. The NUL separator makes the file portion unambiguous. + #[test] + fn test_parse_match_line_windows_path() { + let line = "C:\\src\\file.rs\x0042:fn main() {}"; + let (file, line_num, content) = parse_match_line(line).unwrap(); + assert_eq!(file, r"C:\src\file.rs"); + assert_eq!(line_num, 42); + assert_eq!(content, "fn main() {}"); + } + + // Filenames containing `:digits:` (which would fool a greedy `:` parser) + // must still parse correctly under NUL separation. + #[test] + fn test_parse_match_line_filename_with_colons() { + let line = "badly_named:52:file.txt\x001:xxx"; + let (file, line_num, content) = parse_match_line(line).unwrap(); + assert_eq!(file, "badly_named:52:file.txt"); + assert_eq!(line_num, 1); + assert_eq!(content, "xxx"); + } + + // Content that itself contains `:digits:` (e.g. log lines, port numbers, + // line-number-like substrings) must not confuse the parser. + #[test] + fn test_parse_match_line_content_with_digit_colons() { + let line = "log.txt\x007:debug: counter is :42: now"; + let (file, line_num, content) = parse_match_line(line).unwrap(); + assert_eq!(file, "log.txt"); + assert_eq!(line_num, 7); + assert_eq!(content, "debug: counter is :42: now"); + } + + #[test] + fn test_parse_match_line_malformed_returns_none() { + // No NUL separator (e.g. rg/grep invoked without --null/-Z, or a + // context line written with `-`). + assert!(parse_match_line("file.rs:1:content").is_none()); + assert!(parse_match_line("not a match line").is_none()); + // Missing line number after NUL + assert!(parse_match_line("file.rs\x00fn foo()").is_none()); + // Empty + assert!(parse_match_line("").is_none()); + } + + #[test] + fn test_parse_match_line_empty_content() { + let line = "file.rs\x007:"; + let (file, line_num, content) = parse_match_line(line).unwrap(); + assert_eq!(file, "file.rs"); + assert_eq!(line_num, 7); + assert_eq!(content, ""); + } + #[test] fn test_rg_no_ignore_vcs_flag_accepted() { // Verify rg accepts --no-ignore-vcs (used to match grep -r behavior for .gitignore) diff --git a/src/cmds/system/ls.rs b/src/cmds/system/ls.rs index b96f84876..98eb36447 100644 --- a/src/cmds/system/ls.rs +++ b/src/cmds/system/ls.rs @@ -24,6 +24,19 @@ pub fn run(args: &[String], verbose: u8) -> Result { .iter() .any(|a| (a.starts_with('-') && !a.starts_with("--") && a.contains('a')) || a == "--all"); + // Per `man ls`, the long listing is triggered by `-l` and also implied by + // `-g`, `-n`, `-o`, `--full-time` or GNU `--format=long` and `--format=verbose`. + // In any of those cases we preserve permission info as octal. + let show_long = args.iter().any(|a| { + if a == "--full-time" || a == "--format=long" || a == "--format=verbose" { + return true; + } + if a.starts_with('-') && !a.starts_with("--") { + return a.chars().any(|c| matches!(c, 'l' | 'g' | 'n' | 'o')); + } + false + }); + let flags: Vec<&str> = args .iter() .filter(|a| a.starts_with('-')) @@ -74,7 +87,7 @@ pub fn run(args: &[String], verbose: u8) -> Result { "ls", &format!("-la {}", target_display), |raw| { - let (entries, summary, parsed_count) = compact_ls(raw, show_all); + let (entries, summary, parsed_count) = compact_ls(raw, show_all, show_long); // If no lines were parsed (e.g., unrecognized locale), fall back to raw output. // This is safer than returning "(empty)" for a non-empty directory. @@ -124,14 +137,17 @@ fn human_size(bytes: u64) -> String { } } -/// Parse a single `ls -la` line, returning `(file_type_char, size, name)`. +/// Parse a single `ls -la` line, returning `(file_type_char, perms, size, name)`. +/// +/// `perms` is the raw 10-char string from ls (e.g. `-rw-r--r--`); use +/// [`perms_to_octal`] to render it. /// /// Uses the date field as a stable anchor — the date format in `ls -la` is /// always three tokens (`Mon DD HH:MM` or `Mon DD YYYY`), so we locate it /// with a regex, then extract size (rightmost number before the date) and /// filename (everything after the date). This handles owner/group names that /// contain spaces, which break the old fixed-column approach. -fn parse_ls_line(line: &str) -> Option<(char, u64, String)> { +fn parse_ls_line(line: &str) -> Option<(char, String, u64, String)> { // Skip . and .. entries before date parsing (works for non-English locales too) if is_dotdir(line) { return None; @@ -146,7 +162,7 @@ fn parse_ls_line(line: &str) -> Option<(char, u64, String)> { return None; } - let perms = before_parts[0]; + let perms = before_parts[0].to_string(); let file_type = perms.chars().next()?; // Size is the rightmost parseable number before the date. @@ -160,7 +176,7 @@ fn parse_ls_line(line: &str) -> Option<(char, u64, String)> { } } - Some((file_type, size, name)) + Some((file_type, perms, size, name)) } /// Returns true if the line represents a . or .. directory entry. @@ -173,17 +189,60 @@ fn is_dotdir(line: &str) -> bool { line.trim().ends_with('.') || line.trim().ends_with("..") } -/// Parse ls -la output into compact format: -/// name/ (dirs) -/// name size (files) +/// Convert an `ls`-style permission string (e.g. `-rw-r--r--`, `drwxr-xr-x`, +/// `-rwsr-xr-t`) into octal notation (e.g. `644`, `755`, `4755`). +/// +/// Returns `None` if the input does not look like a permission field. +/// Special bits (setuid/setgid/sticky) are encoded as a leading 4th digit when +/// any are set; otherwise we emit a 3-digit value to stay compact. +fn perms_to_octal(perms: &str) -> Option { + if perms.len() < 10 || !perms.is_ascii() { + return None; + } + let b = perms.as_bytes(); + + fn perm_value(read: bool, write: bool, exec: bool) -> u32 { + ((read as u32) << 2) | ((write as u32) << 1) | (exec as u32) + } + + let owner_x = matches!(b[3], b'x' | b's'); + let group_x = matches!(b[6], b'x' | b's'); + let other_x = matches!(b[9], b'x' | b't'); + + let owner = perm_value(b[1] == b'r', b[2] == b'w', owner_x); + let group = perm_value(b[4] == b'r', b[5] == b'w', group_x); + let other = perm_value(b[7] == b'r', b[8] == b'w', other_x); + + let setuid = matches!(b[3], b's' | b'S'); + let setgid = matches!(b[6], b's' | b'S'); + let sticky = matches!(b[9], b't' | b'T'); + let special = perm_value(setuid, setgid, sticky); + + if special > 0 { + Some(format!("{}{}{}{}", special, owner, group, other)) + } else { + Some(format!("{}{}{}", owner, group, other)) + } +} + +/// Parse ls -la output into compact format. +/// +/// Without `show_long`: +/// name/ (dirs) +/// name size (files) +/// +/// With `show_long` (user passed `-l`): +/// 755 name/ (dirs) +/// 644 name size (files) +/// /// Returns (entries, summary, parsed_count) so caller can suppress summary when piped. /// parsed_count tracks how many non-header lines were successfully parsed. /// If parsed_count == 0 but raw had content, caller should fall back to raw output. -fn compact_ls(raw: &str, show_all: bool) -> (String, String, usize) { +fn compact_ls(raw: &str, show_all: bool, show_long: bool) -> (String, String, usize) { use std::collections::HashMap; - let mut dirs: Vec = Vec::new(); - let mut files: Vec<(String, String)> = Vec::new(); // (name, size) + let mut dirs: Vec<(String, Option)> = Vec::new(); // (name, octal_perms) + let mut files: Vec<(String, String, Option)> = Vec::new(); // (name, size, octal_perms) let mut by_ext: HashMap = HashMap::new(); let mut lines_seen: usize = 0; let mut parsed_count: usize = 0; @@ -195,7 +254,7 @@ fn compact_ls(raw: &str, show_all: bool) -> (String, String, usize) { } lines_seen += 1; - let Some((file_type, size, name)) = parse_ls_line(line) else { + let Some((file_type, perms, size, name)) = parse_ls_line(line) else { if is_dotdir(line) { dotdirs += 1; } @@ -208,8 +267,16 @@ fn compact_ls(raw: &str, show_all: bool) -> (String, String, usize) { continue; } + // Only parse perms when the user actually wants the long listing — + // skip the work otherwise. + let octal = if show_long { + perms_to_octal(&perms) + } else { + None + }; + if file_type == 'd' { - dirs.push(name); + dirs.push((name, octal)); } else { // Regular files, symlinks, character/block devices, pipes, sockets let ext = if let Some(pos) = name.rfind('.') { @@ -218,7 +285,7 @@ fn compact_ls(raw: &str, show_all: bool) -> (String, String, usize) { "no ext".to_string() }; *by_ext.entry(ext).or_insert(0) += 1; - files.push((name, human_size(size))); + files.push((name, human_size(size), octal)); } } @@ -237,13 +304,21 @@ fn compact_ls(raw: &str, show_all: bool) -> (String, String, usize) { let mut entries = String::new(); // Dirs first, compact - for d in &dirs { - entries.push_str(d); + for (name, octal) in &dirs { + if let Some(octal) = octal { + entries.push_str(octal); + entries.push_str(" "); + } + entries.push_str(name); entries.push_str("/\n"); } // Files with size - for (name, size) in &files { + for (name, size, octal) in &files { + if let Some(octal) = octal { + entries.push_str(octal); + entries.push_str(" "); + } entries.push_str(name); entries.push_str(" "); entries.push_str(size); @@ -286,7 +361,7 @@ mod tests { drwxr-xr-x 2 user staff 64 Jan 1 12:00 src\n\ -rw-r--r-- 1 user staff 1234 Jan 1 12:00 Cargo.toml\n\ -rw-r--r-- 1 user staff 5678 Jan 1 12:00 README.md\n"; - let (entries, _summary, _) = compact_ls(input, false); + let (entries, _summary, _parsed) = compact_ls(input, false, false); assert!(entries.contains("src/")); assert!(entries.contains("Cargo.toml")); assert!(entries.contains("README.md")); @@ -307,7 +382,7 @@ mod tests { drwxr-xr-x 2 user staff 64 Jan 1 12:00 target\n\ drwxr-xr-x 2 user staff 64 Jan 1 12:00 src\n\ -rw-r--r-- 1 user staff 100 Jan 1 12:00 main.rs\n"; - let (entries, _summary, _) = compact_ls(input, false); + let (entries, _summary, _parsed) = compact_ls(input, false, false); assert!(!entries.contains("node_modules")); assert!(!entries.contains(".git")); assert!(!entries.contains("target")); @@ -320,7 +395,7 @@ mod tests { let input = "total 8\n\ drwxr-xr-x 2 user staff 64 Jan 1 12:00 .git\n\ drwxr-xr-x 2 user staff 64 Jan 1 12:00 src\n"; - let (entries, _summary, _) = compact_ls(input, true); + let (entries, _summary, _parsed) = compact_ls(input, true, false); assert!(entries.contains(".git/")); assert!(entries.contains("src/")); } @@ -328,7 +403,7 @@ mod tests { #[test] fn test_compact_empty() { let input = "total 0\n"; - let (entries, summary, _) = compact_ls(input, false); + let (entries, summary, _parsed) = compact_ls(input, false, false); assert_eq!(entries, "(empty)\n"); assert!(summary.is_empty()); } @@ -338,7 +413,7 @@ mod tests { let input = "total 8\n\ drwxr-xr-x 2 user user 4096 1月 1 12:00 .\n\ drwxr-xr-x 16 user user 20480 1月 1 12:00 ..\n"; - let (entries, summary, parsed_count) = compact_ls(input, false); + let (entries, summary, parsed_count) = compact_ls(input, false, false); assert_eq!(parsed_count, 0); assert_eq!(entries, "(empty)\n"); assert!(summary.is_empty()); @@ -349,7 +424,7 @@ mod tests { let input = "total 0\n\ drwxr-xr-x 2 lumin wheel 64 Apr 23 00:37 .\n\ drwxr-xr-x 16 root wheel 164576 Apr 23 00:37 ..\n"; - let (entries, summary, parsed_count) = compact_ls(input, false); + let (entries, summary, parsed_count) = compact_ls(input, false, false); assert_eq!(parsed_count, 0); assert_eq!(entries, "(empty)\n"); assert!(summary.is_empty()); @@ -362,7 +437,7 @@ mod tests { -rw-r--r-- 1 user staff 1234 Jan 1 12:00 main.rs\n\ -rw-r--r-- 1 user staff 5678 Jan 1 12:00 lib.rs\n\ -rw-r--r-- 1 user staff 100 Jan 1 12:00 Cargo.toml\n"; - let (_entries, summary, _) = compact_ls(input, false); + let (_entries, summary, _parsed) = compact_ls(input, false, false); assert!(summary.contains("Summary: 3 files, 1 dirs")); assert!(summary.contains(".rs")); assert!(summary.contains(".toml")); @@ -382,7 +457,7 @@ mod tests { fn test_compact_handles_filenames_with_spaces() { let input = "total 8\n\ -rw-r--r-- 1 user staff 1234 Jan 1 12:00 my file.txt\n"; - let (entries, _summary, _) = compact_ls(input, false); + let (entries, _summary, _parsed) = compact_ls(input, false, false); assert!(entries.contains("my file.txt")); } @@ -390,7 +465,7 @@ mod tests { fn test_compact_symlinks() { let input = "total 8\n\ lrwxr-xr-x 1 user staff 10 Jan 1 12:00 link -> target\n"; - let (entries, _summary, _) = compact_ls(input, false); + let (entries, _summary, _parsed) = compact_ls(input, false, false); assert!(entries.contains("link -> target")); } @@ -400,7 +475,7 @@ mod tests { let input = "total 48\n\ drwxr-xr-x 2 user staff 64 Jan 1 12:00 src\n\ -rw-r--r-- 1 user staff 1234 Jan 1 12:00 main.rs\n"; - let (entries, summary, _) = compact_ls(input, false); + let (entries, summary, _parsed) = compact_ls(input, false, false); assert!( !entries.contains("Summary:"), "entries must not contain summary" @@ -419,7 +494,7 @@ mod tests { drwxr-xr-x 2 user staff 64 Jan 1 12:00 src\n\ -rw-r--r-- 1 user staff 1234 Jan 1 12:00 main.rs\n\ -rw-r--r-- 1 user staff 5678 Jan 1 12:00 lib.rs\n"; - let (entries, _summary, _) = compact_ls(input, false); + let (entries, _summary, _parsed) = compact_ls(input, false, false); let line_count = entries.lines().count(); assert_eq!( line_count, 3, @@ -434,7 +509,7 @@ mod tests { let input = "total 8\n\ -rw-r--r-- 1 fjeanne utilisa. du domaine 0 Mar 31 16:18 empty.txt\n\ -rw-r--r-- 1 fjeanne utilisa. du domaine 1234 Mar 31 16:18 data.json\n"; - let (entries, _summary, _) = compact_ls(input, false); + let (entries, _summary, _parsed) = compact_ls(input, false, false); assert!( entries.contains("empty.txt"), "should contain 'empty.txt', got: {entries}" @@ -462,7 +537,7 @@ mod tests { // Some systems show year instead of time for old files let input = "total 8\n\ -rw-r--r-- 1 user staff 5678 Dec 25 2024 archive.tar\n"; - let (entries, _summary, _) = compact_ls(input, false); + let (entries, _summary, _parsed) = compact_ls(input, false, false); assert!( entries.contains("archive.tar"), "should contain filename, got: {entries}" @@ -472,38 +547,42 @@ mod tests { #[test] fn test_parse_ls_line_basic() { - let (ft, size, name) = + let (ft, perms, size, name) = parse_ls_line("-rw-r--r-- 1 user staff 1234 Jan 1 12:00 file.txt").unwrap(); assert_eq!(ft, '-'); + assert_eq!(perms, "-rw-r--r--"); assert_eq!(size, 1234); assert_eq!(name, "file.txt"); } #[test] fn test_parse_ls_line_multiline_group() { - let (ft, size, name) = + let (ft, perms, size, name) = parse_ls_line("-rw-r--r-- 1 fjeanne utilisa. du domaine 0 Mar 31 16:18 empty.txt") .unwrap(); assert_eq!(ft, '-'); + assert_eq!(perms, "-rw-r--r--"); assert_eq!(size, 0); assert_eq!(name, "empty.txt"); } #[test] fn test_parse_ls_line_dir_with_space_in_group() { - let (ft, size, name) = + let (ft, perms, size, name) = parse_ls_line("drwxr-xr-x 2 fjeanne utilisa. du domaine 64 Mar 31 16:18 my dir") .unwrap(); assert_eq!(ft, 'd'); + assert_eq!(perms, "drwxr-xr-x"); assert_eq!(size, 64); assert_eq!(name, "my dir"); } #[test] fn test_parse_ls_line_symlink() { - let (ft, size, name) = + let (ft, perms, size, name) = parse_ls_line("lrwxr-xr-x 1 user staff 10 Jan 1 12:00 link -> target").unwrap(); assert_eq!(ft, 'l'); + assert_eq!(perms, "lrwxr-xr-x"); assert_eq!(size, 10); assert_eq!(name, "link -> target"); } @@ -513,7 +592,7 @@ mod tests { // Regression test for #844: `rtk ls /dev/ttyACM*` returned "(empty)" // because character devices (type 'c') were not handled by compact_ls. let input = "crw-rw---- 1 root dialout 166, 0 Apr 22 09:46 /dev/ttyACM0\n"; - let (entries, _summary, _parsed) = compact_ls(input, false); + let (entries, _summary, _parsed) = compact_ls(input, false, false); assert!( entries.contains("/dev/ttyACM0"), "should contain device file, got: {entries}" @@ -525,7 +604,7 @@ mod tests { fn test_compact_device_files_macos_hex_size() { // macOS shows device major/minor as hex (e.g. 0x2000000) let input = "crw-rw-rw- 1 root wheel 0x2000000 Mar 31 19:25 /dev/tty\n"; - let (entries, _summary, _parsed) = compact_ls(input, false); + let (entries, _summary, _parsed) = compact_ls(input, false, false); assert!( entries.contains("/dev/tty"), "should contain device file, got: {entries}" @@ -535,7 +614,7 @@ mod tests { #[test] fn test_compact_block_device() { let input = "brw-rw---- 1 root disk 8, 0 Apr 22 09:46 /dev/sda\n"; - let (entries, _summary, _parsed) = compact_ls(input, false); + let (entries, _summary, _parsed) = compact_ls(input, false, false); assert!( entries.contains("/dev/sda"), "should contain block device, got: {entries}" @@ -549,19 +628,86 @@ mod tests { #[test] fn test_parse_ls_line_year_format() { - let (ft, size, name) = + let (ft, perms, size, name) = parse_ls_line("-rw-r--r-- 1 user staff 5678 Dec 25 2024 old.tar.gz").unwrap(); assert_eq!(ft, '-'); + assert_eq!(perms, "-rw-r--r--"); assert_eq!(size, 5678); assert_eq!(name, "old.tar.gz"); } + #[test] + fn test_perms_to_octal_common() { + assert_eq!(perms_to_octal("-rw-r--r--").as_deref(), Some("644")); + assert_eq!(perms_to_octal("-rwxr-xr-x").as_deref(), Some("755")); + assert_eq!(perms_to_octal("drwxr-xr-x").as_deref(), Some("755")); + assert_eq!(perms_to_octal("-rw-------").as_deref(), Some("600")); + assert_eq!(perms_to_octal("-rwxrwxrwx").as_deref(), Some("777")); + assert_eq!(perms_to_octal("----------").as_deref(), Some("000")); + assert_eq!(perms_to_octal("lrwxr-xr-x").as_deref(), Some("755")); + } + + #[test] + fn test_perms_to_octal_special_bits() { + // setuid + 755 -> 4755 + assert_eq!(perms_to_octal("-rwsr-xr-x").as_deref(), Some("4755")); + // setuid without execute -> 4644 + assert_eq!(perms_to_octal("-rwSr--r--").as_deref(), Some("4644")); + // setgid + 755 -> 2755 + assert_eq!(perms_to_octal("-rwxr-sr-x").as_deref(), Some("2755")); + // sticky bit on /tmp-style dir -> 1777 + assert_eq!(perms_to_octal("drwxrwxrwt").as_deref(), Some("1777")); + // setuid + setgid + sticky + assert_eq!(perms_to_octal("-rwsrwsrwt").as_deref(), Some("7777")); + } + + #[test] + fn test_perms_to_octal_garbage() { + assert_eq!(perms_to_octal(""), None); + assert_eq!(perms_to_octal("short"), None); + } + + #[test] + fn test_compact_long_format_includes_octal() { + let input = "total 48\n\ + drwxr-xr-x 2 user staff 64 Jan 1 12:00 src\n\ + -rw-r--r-- 1 user staff 1234 Jan 1 12:00 Cargo.toml\n\ + -rwxr-xr-x 1 user staff 500 Jan 1 12:00 build.sh\n"; + let (entries, _summary, _parsed) = compact_ls(input, false, true); + assert!( + entries.contains("755 src/"), + "dir should be prefixed with octal perms, got: {entries}" + ); + assert!( + entries.contains("644 Cargo.toml 1.2K"), + "file should be prefixed with octal perms, got: {entries}" + ); + assert!( + entries.contains("755 build.sh 500B"), + "executable should show 755, got: {entries}" + ); + } + + #[test] + fn test_compact_short_format_omits_octal() { + // Without -l, no octal prefix even though we still parse `ls -la` + // under the hood. + let input = "total 48\n\ + -rw-r--r-- 1 user staff 1234 Jan 1 12:00 Cargo.toml\n"; + let (entries, _summary, _parsed) = compact_ls(input, false, false); + assert!( + !entries.contains("644"), + "short format must not include octal perms, got: {entries}" + ); + assert!(entries.contains("Cargo.toml")); + } + #[test] fn test_compact_chinese_locale_fallback() { let input = "total 8\n\ drwxr-xr-x 2 user staff 64 1月 1 12:00 src\n\ -rw-r--r-- 1 user staff 1234 1月 1 12:00 main.rs\n"; - let (entries, summary, parsed_count) = compact_ls(input, false); + let (entries, summary, parsed_count) = compact_ls(input, false, false); assert_eq!(parsed_count, 0); assert!(entries.is_empty()); assert!(summary.is_empty()); diff --git a/src/core/args_utils.rs b/src/core/args_utils.rs new file mode 100644 index 000000000..f5418b0cc --- /dev/null +++ b/src/core/args_utils.rs @@ -0,0 +1,240 @@ +//! Utility functions for argument handling, particularly for restoring "--" escape +//! arguments that clap consumes during parsing. + +/// Restores "--" escape arguments that clap consumed. +/// Handles: +/// - Single/multiple "--" swallowed by clap (restores each at its original position) +/// - "--" already present in parsed (no change) +/// - No "--" in original command (no injection) +/// - Args appearing before/after "--" in original (preserves order) +/// - Interleaved "--" and args (preserves relative positions, e.g., "-- arg1 -- arg2") +/// - Duplicate args on both sides of "--" +/// +/// Returns parsed_args unchanged if raw has same or fewer "--" than parsed +/// (meaning clap didn't consume any, or preserved them). +pub fn restore_double_dash(parsed_args: &[String]) -> Vec { + let raw_args: Vec = std::env::args().collect(); + restore_double_dash_with_raw(parsed_args, &raw_args) +} + +/// Testable version that takes raw_args explicitly. +pub fn restore_double_dash_with_raw(parsed_args: &[String], raw_args: &[String]) -> Vec { + let raw_dash_count = raw_args.iter().filter(|a| a.as_str() == "--").count(); + let parsed_dash_count = parsed_args.iter().filter(|a| a.as_str() == "--").count(); + + if raw_dash_count <= parsed_dash_count { + return parsed_args.to_vec(); + } + + // Find all positions of "--" in raw_args (skip index 0 = "rtk") + let mut dash_positions: Vec = Vec::new(); + for (i, arg) in raw_args.iter().enumerate().skip(1) { + if arg == "--" { + dash_positions.push(i); + } + } + + if dash_positions.is_empty() { + return parsed_args.to_vec(); + } + + // Build result by inserting "--" at correct positions relative to parsed args + let mut result = Vec::new(); + let mut raw_idx = 1; // start after "rtk" + let mut dash_idx = 0; + + while raw_idx < raw_args.len() { + // Check if current position is a "--" that was swallowed + if dash_idx < dash_positions.len() && raw_idx == dash_positions[dash_idx] { + result.push("--".to_string()); + dash_idx += 1; + } else if parsed_args.contains(&raw_args[raw_idx]) { + // This arg is in parsed_args, add it + result.push(raw_args[raw_idx].clone()); + } + raw_idx += 1; + } + + result +} + +#[cfg(test)] +mod tests { + use super::*; + + fn restore_with_raw(parsed: &[&str], raw: &[&str]) -> Vec { + let parsed: Vec = parsed.iter().map(|s| s.to_string()).collect(); + let raw: Vec = raw.iter().map(|s| s.to_string()).collect(); + restore_double_dash_with_raw(parsed.as_slice(), raw.as_slice()) + } + + // ============ Single "--" swallowed ============ + + #[test] + fn test_single_dash_swallowed() { + // rtk git diff -- file → clap gave ["file"], restore "--" + let raw = vec!["rtk", "git", "diff", "--", "file"]; + let parsed = vec!["file"]; + assert_eq!(restore_with_raw(&parsed, &raw), vec!["--", "file"]); + } + + #[test] + fn test_args_before_dash() { + // rtk cargo test name -- --nocapture → args before "--" stay before + let raw = vec!["rtk", "cargo", "test", "name", "--", "--nocapture"]; + let parsed = vec!["name", "--nocapture"]; + assert_eq!( + restore_with_raw(&parsed, &raw), + vec!["name", "--", "--nocapture"] + ); + } + + // ============ Multiple "--" swallowed ============ + + #[test] + fn test_multiple_dashes_all_swallowed() { + // rtk git diff -- -- -- → all 3 "--" swallowed, consecutive in output + let raw = vec!["rtk", "git", "diff", "--", "--", "--"]; + let parsed: Vec<&str> = vec![]; + assert_eq!(restore_with_raw(&parsed, &raw), vec!["--", "--", "--"]); + } + + #[test] + fn test_dashes_with_args_between() { + // rtk git diff -- arg1 -- arg2 → both "--" consumed, preserve positions + let raw = vec!["rtk", "git", "diff", "--", "arg1", "--", "arg2"]; + let parsed = vec!["arg1", "arg2"]; + // Result: each "--" inserted at its original position relative to args + assert_eq!( + restore_with_raw(&parsed, &raw), + vec!["--", "arg1", "--", "arg2"] + ); + } + + #[test] + fn test_multiple_dashes_some_preserved() { + // rtk git diff -- -- → 2 in raw, 1 preserved in parsed + let raw = vec!["rtk", "git", "diff", "--", "--"]; + let parsed = vec!["--"]; + assert_eq!(restore_with_raw(&parsed, &raw), vec!["--", "--"]); + } + + #[test] + fn test_compound_command_with_dashes() { + // Multiple segments with "--" → restore all + let raw = vec!["rtk", "cmd1", "--", "arg1", "&&", "cmd2", "--", "file"]; + let parsed = vec!["file"]; + assert_eq!(restore_with_raw(&parsed, &raw), vec!["--", "--", "file"]); + } + + // ============ "--" already present (no change needed) ============ + + #[test] + fn test_dash_already_preserved() { + // rtk cargo clippy -p pkg -- -D warnings → clap kept "--" + let raw = vec![ + "rtk", "cargo", "clippy", "-p", "pkg", "--", "-D", "warnings", + ]; + let parsed = vec!["-p", "pkg", "--", "-D", "warnings"]; + assert_eq!( + restore_with_raw(&parsed, &raw), + vec!["-p", "pkg", "--", "-D", "warnings"] + ); + } + + #[test] + fn test_trailing_dash_preserved() { + // rtk git diff file -- → trailing "--" preserved + let raw = vec!["rtk", "git", "diff", "file", "--"]; + let parsed = vec!["file", "--"]; + assert_eq!(restore_with_raw(&parsed, &raw), vec!["file", "--"]); + } + + // ============ No "--" in original (no injection) ============ + + #[test] + fn test_no_dash_in_original() { + // Various cases: branch with /, range, bare word, flags only + // All should return args unchanged (no injection) + let cases = vec![ + ( + vec!["rtk", "git", "diff", "feature/auth"], + vec!["feature/auth"], + ), + ( + vec!["rtk", "git", "diff", "main...feature"], + vec!["main...feature"], + ), + (vec!["rtk", "git", "diff", "main"], vec!["main"]), + ( + vec!["rtk", "git", "diff", "--stat", "--cached"], + vec!["--stat", "--cached"], + ), + ]; + for (raw, parsed) in cases { + assert_eq!(restore_with_raw(&parsed, &raw), parsed); + } + } + + // ============ Edge cases ============ + + #[test] + fn test_duplicate_args_both_sides() { + // -p pkg1 -p pkg2 -- -p pkg3 → restore after last -p + let raw = vec![ + "rtk", "cargo", "clippy", "-p", "p1", "-p", "p2", "--", "-p", "p3", + ]; + let parsed = vec!["-p", "p1", "-p", "p2", "-p", "p3"]; + assert_eq!( + restore_with_raw(&parsed, &raw), + vec!["-p", "p1", "-p", "p2", "--", "-p", "p3"] + ); + } + + #[test] + fn test_empty_args() { + let raw = vec!["rtk", "cargo", "test"]; + let parsed: Vec<&str> = vec![]; + assert_eq!(restore_with_raw(&parsed, &raw), Vec::::new()); + } + + #[test] + fn test_cargo_clippy_missing_dash() { + // No "--" in original → no injection + let raw = vec!["rtk", "cargo", "clippy", "-D", "warnings"]; + let parsed = vec!["-D", "warnings"]; + assert_eq!(restore_with_raw(&parsed, &raw), vec!["-D", "warnings"]); + } + + // ============ Git diff specific cases ============ + + #[test] + fn test_git_diff_ref_before_path() { + // rtk git diff HEAD -- file + let raw = vec!["rtk", "git", "diff", "HEAD", "--", "file"]; + let parsed = vec!["HEAD", "file"]; + assert_eq!(restore_with_raw(&parsed, &raw), vec!["HEAD", "--", "file"]); + } + + #[test] + fn test_git_diff_flags_before_path() { + // rtk git diff --cached -- file + let raw = vec!["rtk", "git", "diff", "--cached", "--", "file"]; + let parsed = vec!["--cached", "file"]; + assert_eq!( + restore_with_raw(&parsed, &raw), + vec!["--cached", "--", "file"] + ); + } + + #[test] + fn test_git_diff_multiple_files() { + // Original issue: multiple files caused "fatal: bad revision" + let raw = vec!["rtk", "git", "diff", "--", "file1", "file2", "file3"]; + let parsed = vec!["file1", "file2", "file3"]; + assert_eq!( + restore_with_raw(&parsed, &raw), + vec!["--", "file1", "file2", "file3"] + ); + } +} diff --git a/src/core/mod.rs b/src/core/mod.rs index fe017d910..d5182bd34 100644 --- a/src/core/mod.rs +++ b/src/core/mod.rs @@ -1,5 +1,6 @@ //! Building blocks shared across all RTK modules. +pub mod args_utils; pub mod config; pub mod constants; pub mod display_helpers; diff --git a/src/core/runner.rs b/src/core/runner.rs index 17f0997da..571c7634b 100644 --- a/src/core/runner.rs +++ b/src/core/runner.rs @@ -62,12 +62,79 @@ impl<'a> RunOptions<'a> { } } +pub type CaptureFilter<'a> = Box String + 'a>; +pub type ExitAwareCaptureFilter<'a> = Box String + 'a>; + pub enum RunMode<'a> { - Filtered(Box String + 'a>), + Filtered(CaptureFilter<'a>), + FilteredWithExit(ExitAwareCaptureFilter<'a>), Streamed(Box), Passthrough, } +fn run_captured_filter( + mut cmd: Command, + tool_name: &str, + cmd_label: &str, + filter_fn: F, + opts: RunOptions<'_>, + timer: tracking::TimedExecution, +) -> Result +where + F: Fn(&str, i32) -> String, +{ + let stdin_mode = if opts.inherit_stdin { + StdinMode::Inherit + } else { + StdinMode::Null + }; + let result = stream::run_streaming(&mut cmd, stdin_mode, FilterMode::CaptureOnly) + .with_context(|| format!("Failed to run {}", tool_name))?; + + let exit_code = result.exit_code; + let raw = &result.raw; + let raw_stdout = &result.raw_stdout; + + if opts.skip_filter_on_failure && exit_code != 0 { + if !result.raw_stdout.trim().is_empty() { + print!("{}", result.raw_stdout); + } + if !result.raw_stderr.trim().is_empty() { + eprint!("{}", result.raw_stderr); + } + timer.track(cmd_label, &format!("rtk {}", cmd_label), raw, raw); + return Ok(exit_code); + } + + let text_to_filter = if opts.filter_stdout_only { + raw_stdout + } else { + raw + }; + let filtered = filter_fn(text_to_filter, exit_code); + + if let Some(label) = opts.tee_label { + print_with_hint(&filtered, raw, label, exit_code); + } else if opts.no_trailing_newline { + print!("{}", filtered); + } else { + println!("{}", filtered); + } + + let raw_for_tracking = if opts.filter_stdout_only { + raw_stdout + } else { + raw + }; + timer.track( + cmd_label, + &format!("rtk {}", cmd_label), + raw_for_tracking, + &filtered, + ); + Ok(exit_code) +} + pub fn run( mut cmd: Command, tool_name: &str, @@ -79,58 +146,22 @@ pub fn run( let cmd_label = format!("{} {}", tool_name, args_display); match mode { - RunMode::Filtered(filter_fn) => { - let stdin_mode = if opts.inherit_stdin { - StdinMode::Inherit - } else { - StdinMode::Null - }; - let result = stream::run_streaming(&mut cmd, stdin_mode, FilterMode::CaptureOnly) - .with_context(|| format!("Failed to run {}", tool_name))?; - - let exit_code = result.exit_code; - let raw = &result.raw; - let raw_stdout = &result.raw_stdout; - - if opts.skip_filter_on_failure && exit_code != 0 { - if !result.raw_stdout.trim().is_empty() { - print!("{}", result.raw_stdout); - } - if !result.raw_stderr.trim().is_empty() { - eprint!("{}", result.raw_stderr); - } - timer.track(&cmd_label, &format!("rtk {}", cmd_label), raw, raw); - return Ok(exit_code); - } - - let text_to_filter = if opts.filter_stdout_only { - raw_stdout - } else { - raw - }; - let filtered = filter_fn(text_to_filter); - - if let Some(label) = opts.tee_label { - print_with_hint(&filtered, raw, label, exit_code); - } else if opts.no_trailing_newline { - print!("{}", filtered); - } else { - println!("{}", filtered); - } - - let raw_for_tracking = if opts.filter_stdout_only { - raw_stdout - } else { - raw - }; - timer.track( - &cmd_label, - &format!("rtk {}", cmd_label), - raw_for_tracking, - &filtered, - ); - Ok(exit_code) - } + RunMode::Filtered(filter_fn) => run_captured_filter( + cmd, + tool_name, + &cmd_label, + move |text, _| filter_fn(text), + opts, + timer, + ), + RunMode::FilteredWithExit(filter_fn) => run_captured_filter( + cmd, + tool_name, + &cmd_label, + move |text, exit_code| filter_fn(text, exit_code), + opts, + timer, + ), RunMode::Streamed(filter) => { let result = stream::run_streaming(&mut cmd, StdinMode::Null, FilterMode::Streaming(filter)) @@ -182,6 +213,25 @@ where ) } +pub fn run_filtered_with_exit( + cmd: Command, + tool_name: &str, + args_display: &str, + filter_fn: F, + opts: RunOptions<'_>, +) -> Result +where + F: Fn(&str, i32) -> String, +{ + run( + cmd, + tool_name, + args_display, + RunMode::FilteredWithExit(Box::new(filter_fn)), + opts, + ) +} + pub fn run_passthrough(tool: &str, args: &[std::ffi::OsString], verbose: u8) -> Result { if verbose > 0 { eprintln!("{} passthrough: {:?}", tool, args); diff --git a/src/core/telemetry.rs b/src/core/telemetry.rs index 97e057be1..426c1e1fc 100644 --- a/src/core/telemetry.rs +++ b/src/core/telemetry.rs @@ -368,11 +368,14 @@ fn detect_hook_type() -> String { } } - // Check project-level hooks + // Check project-level hooks (Claude script + project-scoped Copilot config) if let Ok(cwd) = std::env::current_dir() { if cwd.join(".claude/hooks/rtk-rewrite.sh").exists() { return "claude".to_string(); } + if cwd.join(".github/hooks/rtk-rewrite.json").exists() { + return "copilot".to_string(); + } } "none".to_string() @@ -571,7 +574,7 @@ mod tests { assert!(stats.low_savings_commands.len() <= 5); assert!((0.0..=100.0).contains(&stats.avg_savings_per_command)); assert!( - ["claude", "gemini", "codex", "cursor", "none", "unknown"] + ["claude", "gemini", "codex", "cursor", "copilot", "none", "unknown"] .iter() .any(|&h| stats.hook_type.starts_with(h)), "Unexpected hook type: {}", @@ -583,7 +586,8 @@ mod tests { fn test_detect_hook_type_returns_known() { let ht = detect_hook_type(); assert!( - ["claude", "gemini", "codex", "cursor", "none", "unknown"].contains(&ht.as_str()), + ["claude", "gemini", "codex", "cursor", "copilot", "none", "unknown"] + .contains(&ht.as_str()), "Unexpected hook type: {}", ht ); diff --git a/src/discover/provider.rs b/src/discover/provider.rs index ee76c7a16..a9b630a2b 100644 --- a/src/discover/provider.rs +++ b/src/discover/provider.rs @@ -129,7 +129,7 @@ impl ClaudeProvider { /// `/home/chris/2_project` → `-home-chris-2-project` /// `C:\Users\foo\bar` → `C:-Users-foo-bar` pub fn encode_project_path(path: &str) -> String { - const SANITIZED_CHARS: &[char] = &['/', '.', '_', '\\']; + const SANITIZED_CHARS: &[char] = &['/', '.', '_', '\\', ' ', '[', ']']; path.chars() .map(|c| { @@ -421,6 +421,17 @@ mod tests { assert!(encoded.contains("Sites")); } + #[test] + fn test_encode_path_with_spaces() { + // Even if run on Unix, encoding should replace backslashes to match Claude's behavior + assert_eq!( + ClaudeProvider::encode_project_path( + r"/home/user/projects/[QZX-7K42] - Análise Genérica de Exemplo" + ), + "-home-user-projects--QZX-7K42----An-lise-Gen-rica-de-Exemplo" + ); + } + #[test] fn test_discover_sessions_missing_projects_dir_returns_empty() { let temp_home = tempfile::tempdir().unwrap(); diff --git a/src/discover/registry.rs b/src/discover/registry.rs index bb7b11f2a..4fd716828 100644 --- a/src/discover/registry.rs +++ b/src/discover/registry.rs @@ -440,6 +440,25 @@ fn strip_trailing_redirects(cmd: &str) -> (&str, &str) { (cmd_part, redir_part) } +lazy_static! { + /// Matches a bash line-continuation: a backslash immediately followed by + /// `\n` or `\r\n`, *plus* any horizontal whitespace on the line before AND + /// after the break. This is what bash already collapses to a single space + /// before executing the command — rtk's hook matcher needs to do the same + /// so commands authored across multiple lines still hit the rewrite rules. + /// Consuming the trailing whitespace prevents double spaces in cases like + /// `git diff \HEAD~1`. + static ref LINE_CONTINUATION_RE: Regex = + Regex::new(r"(?m)[ \t\x0B\x0C]*\\\r?\n[ \t\x0B\x0C]*").unwrap(); +} + +/// Replace every bash line continuation with a single space, mirroring what +/// bash does before dispatching the command. Returns a borrowed `&str` when the +/// input contains no continuations, so the common fast path allocates nothing. +fn collapse_line_continuations(s: &str) -> std::borrow::Cow<'_, str> { + LINE_CONTINUATION_RE.replace_all(s, " ") +} + /// Returns `None` if the command is unsupported or ignored (hook should pass through). /// /// Handles compound commands (`&&`, `||`, `;`) by rewriting each segment independently. @@ -464,7 +483,12 @@ pub fn rewrite_command( excluded: &[String], transparent_prefixes: &[String], ) -> Option { - let trimmed = cmd.trim(); + // Bash line continuations (`\`, `\`) and the leading whitespace that + // follows are syntactically equivalent to a single space, but `cmd.trim()` does + // not unwrap them so a leading backslash-newline used to defeat the whole matcher. + // Normalize first, then trim. See issue #1564. + let normalized = collapse_line_continuations(cmd); + let trimmed = normalized.trim(); if trimmed.is_empty() { return None; } @@ -3884,4 +3908,68 @@ mod tests { Some("rtk git log | head | tail && rtk git status".into()) ); } + + // --- line-continuation handling (issue #1564) ------------------- + + #[test] + fn test_rewrite_leading_backslash_newline() { + // The exact reproduction from #1564: a leading `\` made + // the matcher see `\` as the command and bail out. + assert_eq!( + rewrite_command_no_prefixes("\\\ngit diff HEAD~1", &[]), + Some("rtk git diff HEAD~1".into()) + ); + } + + #[test] + fn test_rewrite_leading_backslash_crlf() { + // CRLF line ending — same shape, Windows shells / Git Bash. + assert_eq!( + rewrite_command_no_prefixes("\\\r\ngit diff HEAD~1", &[]), + Some("rtk git diff HEAD~1".into()) + ); + } + + #[test] + fn test_rewrite_internal_backslash_newline() { + // Embedded line continuation between subcommand and args: + // `git diff \HEAD~1` is exactly equivalent to + // `git diff HEAD~1` per bash semantics. + assert_eq!( + rewrite_command_no_prefixes("git diff \\\nHEAD~1", &[]), + Some("rtk git diff HEAD~1".into()) + ); + } + + #[test] + fn test_rewrite_backslash_newline_with_indent() { + // Continuation followed by indentation — also collapsed. + assert_eq!( + rewrite_command_no_prefixes("git \\\n diff HEAD~1", &[]), + Some("rtk git diff HEAD~1".into()) + ); + } + + #[test] + fn test_rewrite_no_line_continuation_unchanged() { + // Sanity check: a command without any `\` should match + // unchanged. This pins that the normalization step does not + // regress the no-op fast path. + assert_eq!( + rewrite_command_no_prefixes("git diff HEAD~1", &[]), + Some("rtk git diff HEAD~1".into()) + ); + } + + #[test] + fn test_collapse_line_continuations_no_op() { + // Helper-level: no continuations → returns Borrowed (no + // allocation). We can only spot-check the equality here, but + // the `Cow::Borrowed` variant is implied by `replace_all` + // when no replacement occurs. + assert_eq!( + collapse_line_continuations("git diff HEAD~1"), + std::borrow::Cow::::Borrowed("git diff HEAD~1"), + ); + } } diff --git a/src/discover/report.rs b/src/discover/report.rs index c2c523e24..af4dd453d 100644 --- a/src/discover/report.rs +++ b/src/discover/report.rs @@ -1,8 +1,8 @@ //! Data types for reporting which commands RTK can and cannot optimize. use crate::hooks::constants::{ - CURSOR_DIR, HERMES_DIR, HERMES_PLUGINS_SUBDIR, HERMES_PLUGIN_MANIFEST_FILE, HERMES_PLUGIN_NAME, - HOOKS_SUBDIR, REWRITE_HOOK_FILE, + COPILOT_HOOK_FILE, CURSOR_DIR, GITHUB_DIR, HERMES_DIR, HERMES_PLUGINS_SUBDIR, + HERMES_PLUGIN_MANIFEST_FILE, HERMES_PLUGIN_NAME, HOOKS_SUBDIR, REWRITE_HOOK_FILE, }; use serde::Serialize; use std::path::Path; @@ -52,13 +52,19 @@ pub struct UnsupportedEntry { pub struct AgentIntegrationStatus { pub cursor_hook_installed: bool, pub hermes_plugin_installed: bool, + pub copilot_hook_installed: bool, } impl AgentIntegrationStatus { pub fn detect() -> Self { - dirs::home_dir() + let mut status = dirs::home_dir() .map(|home| Self::detect_from_home(&home)) - .unwrap_or_default() + .unwrap_or_default(); + // Copilot is project-scoped (.github/hooks/), unlike the home-based agents. + status.copilot_hook_installed = std::env::current_dir() + .map(|cwd| Self::copilot_hook_installed_in(&cwd)) + .unwrap_or(false); + status } fn detect_from_home(home: &Path) -> Self { @@ -74,8 +80,16 @@ impl AgentIntegrationStatus { .join(HERMES_PLUGIN_NAME) .join(HERMES_PLUGIN_MANIFEST_FILE) .is_file(), + copilot_hook_installed: false, } } + + fn copilot_hook_installed_in(dir: &Path) -> bool { + dir.join(GITHUB_DIR) + .join(HOOKS_SUBDIR) + .join(COPILOT_HOOK_FILE) + .exists() + } } /// Full discover report. @@ -221,6 +235,10 @@ fn append_agent_notes(out: &mut String, status: AgentIntegrationStatus) { if status.hermes_plugin_installed { out.push_str("\nNote: Hermes plugin is installed; Hermes sessions are tracked via `rtk gain` (discover scans Claude Code only)\n"); } + + if status.copilot_hook_installed { + out.push_str("\nNote: GitHub Copilot sessions are tracked via `rtk gain` (discover scans Claude Code only)\n"); + } } /// Format report as JSON. @@ -362,6 +380,7 @@ mod tests { report.agent_status = AgentIntegrationStatus { cursor_hook_installed: true, hermes_plugin_installed: true, + copilot_hook_installed: true, }; let output = format_json(&report); @@ -369,5 +388,42 @@ mod tests { assert_eq!(json["agent_status"]["cursor_hook_installed"], true); assert_eq!(json["agent_status"]["hermes_plugin_installed"], true); + assert_eq!(json["agent_status"]["copilot_hook_installed"], true); + } + + #[test] + fn test_agent_status_detects_copilot_hook_in_project() { + let temp = tempfile::tempdir().unwrap(); + let hook = temp + .path() + .join(GITHUB_DIR) + .join(HOOKS_SUBDIR) + .join(COPILOT_HOOK_FILE); + std::fs::create_dir_all(hook.parent().unwrap()).unwrap(); + std::fs::write(&hook, "{}").unwrap(); + + assert!(AgentIntegrationStatus::copilot_hook_installed_in( + temp.path() + )); + assert!(!AgentIntegrationStatus::copilot_hook_installed_in( + tempfile::tempdir().unwrap().path() + )); + } + + #[test] + fn test_format_text_reports_copilot_detected() { + let mut report = make_report(0, 0); + report.agent_status = AgentIntegrationStatus { + copilot_hook_installed: true, + ..AgentIntegrationStatus::default() + }; + + let output = format_text(&report, 10, false); + + assert!( + output.contains("GitHub Copilot sessions are tracked via `rtk gain`"), + "Expected Copilot note in output but got:\n{}", + output + ); } } diff --git a/src/hooks/constants.rs b/src/hooks/constants.rs index 506e88cdf..23d2e1089 100644 --- a/src/hooks/constants.rs +++ b/src/hooks/constants.rs @@ -22,6 +22,12 @@ pub const CURSOR_DIR: &str = ".cursor"; pub const CODEX_DIR: &str = ".codex"; pub const GEMINI_DIR: &str = ".gemini"; +pub const GITHUB_DIR: &str = ".github"; +pub const COPILOT_HOOK_FILE: &str = "rtk-rewrite.json"; +pub const COPILOT_INSTRUCTIONS_FILE: &str = "copilot-instructions.md"; +pub const COPILOT_USER_DIR: &str = ".copilot"; +pub const COPILOT_HOME_ENV: &str = "COPILOT_HOME"; + pub const PI_DIR: &str = ".pi/agent"; pub const PI_LOCAL_DIR: &str = ".pi"; pub const PI_EXTENSIONS_SUBDIR: &str = "extensions"; diff --git a/src/hooks/hook_cmd.rs b/src/hooks/hook_cmd.rs index 36825e3c5..8870bf8d7 100644 --- a/src/hooks/hook_cmd.rs +++ b/src/hooks/hook_cmd.rs @@ -31,8 +31,10 @@ fn read_stdin_limited() -> Result { enum HookFormat { /// VS Code Copilot Chat / Claude Code: `tool_name` + `tool_input.command`, supports `updatedInput`. VsCode { command: String }, - /// GitHub Copilot CLI: camelCase `toolName` + `toolArgs` (JSON string), deny-with-suggestion only. - CopilotCli { command: String }, + /// GitHub Copilot CLI: camelCase `toolName` + `toolArgs` (JSON string), supports `modifiedArgs` for transparent rewrite. + /// Carries the full parsed `toolArgs` object so we can rewrite `command` while preserving + /// host-supplied metadata (description, initial_wait, mode, …) the tool requires. + CopilotCli { command: String, args: Value }, /// Non-bash tool, already uses rtk, or unknown format — pass through silently. PassThrough, } @@ -42,7 +44,9 @@ enum HookFormat { pub fn run_copilot() -> Result<()> { let input = read_stdin_limited()?; - let input = input.trim(); + // Strip leading BOM(s) before trimming: some Windows hosts prepend UTF-8 + // BOMs to hook stdin (confirmed for Cursor), which serde_json rejects. + let input = strip_leading_bom(&input).trim(); if input.is_empty() { return Ok(()); } @@ -57,7 +61,7 @@ pub fn run_copilot() -> Result<()> { match detect_format(&v) { HookFormat::VsCode { command } => handle_vscode(&command), - HookFormat::CopilotCli { command } => handle_copilot_cli(&command), + HookFormat::CopilotCli { command, args } => handle_copilot_cli(&command, &args), HookFormat::PassThrough => Ok(()), } } @@ -91,6 +95,7 @@ fn detect_format(v: &Value) -> HookFormat { { return HookFormat::CopilotCli { command: cmd.to_string(), + args: tool_args, }; } } @@ -153,28 +158,46 @@ fn handle_vscode(cmd: &str) -> Result<()> { Ok(()) } -fn handle_copilot_cli(cmd: &str) -> Result<()> { - if permissions::check_command(cmd) == PermissionVerdict::Deny { - audit_log("deny", cmd, ""); - return Ok(()); +fn handle_copilot_cli(cmd: &str, args: &Value) -> Result<()> { + if let Some(response) = copilot_cli_response(cmd, args) { + let _ = writeln!(io::stdout(), "{response}"); } + Ok(()) +} - let rewritten = match get_rewritten(cmd) { - Some(r) => r, - None => return Ok(()), - }; +fn copilot_cli_response(cmd: &str, args: &Value) -> Option { + copilot_cli_response_for_verdict(cmd, args, permissions::check_command(cmd)) +} +fn copilot_cli_response_for_verdict( + cmd: &str, + args: &Value, + verdict: PermissionVerdict, +) -> Option { + if verdict == PermissionVerdict::Deny { + audit_log("deny", cmd, ""); + return None; + } + let rewritten = get_rewritten(cmd)?; audit_log("rewrite", cmd, &rewritten); - let output = json!({ - "permissionDecision": "deny", - "permissionDecisionReason": format!( - "Token savings: use `{}` instead (rtk saves 60-90% tokens)", - rewritten - ) + let mut modified = args.clone(); + if let Some(obj) = modified.as_object_mut() { + obj.insert("command".into(), Value::String(rewritten)); + } + + // Copilot CLI v1.0.54 only applies `modifiedArgs` when permissionDecision is + // either "allow" or absent. Omitting permissionDecision lets Copilot's normal + // permission flow run on the rewritten command — `rtk *` is not on its + // auto-allow list, so the user sees a prompt for the rewritten command. + let mut response = json!({ + "permissionDecisionReason": "RTK auto-rewrite", + "modifiedArgs": modified, }); - let _ = writeln!(io::stdout(), "{output}"); - Ok(()) + if verdict == PermissionVerdict::Allow { + response["permissionDecision"] = json!("allow"); + } + Some(response) } // ── Gemini hook ─────────────────────────────────────────────── @@ -575,6 +598,25 @@ mod tests { assert!(matches!(detect_format(&v), HookFormat::PassThrough)); } + #[test] + fn test_copilot_bom_prefixed_payload_is_recognized() { + // Windows hosts may prepend one or two UTF-8 BOMs to hook stdin + // (confirmed for Cursor). run_copilot strips them before parsing; + // verify both Copilot formats still parse after the same handling. + for raw in [ + format!("\u{feff}{}", copilot_cli_input("git status")), + format!("\u{feff}\u{feff}{}", copilot_cli_input("git status")), + ] { + let cleaned = strip_leading_bom(&raw).trim(); + let v: Value = serde_json::from_str(cleaned).expect("BOM-stripped JSON must parse"); + assert!(matches!(detect_format(&v), HookFormat::CopilotCli { .. })); + } + + let raw = format!("\u{feff}{}", vscode_input("Bash", "git status")); + let v: Value = serde_json::from_str(strip_leading_bom(&raw).trim()).unwrap(); + assert!(matches!(detect_format(&v), HookFormat::VsCode { .. })); + } + #[test] fn test_detect_unknown_is_passthrough() { assert!(matches!(detect_format(&json!({})), HookFormat::PassThrough)); @@ -600,6 +642,99 @@ mod tests { assert!(get_rewritten("cat <<'EOF'\nhello\nEOF").is_none()); } + // --- Copilot CLI handler: transparent rewrite via modifiedArgs --- + + fn cli_args(cmd: &str) -> Value { + json!({ "command": cmd }) + } + + #[test] + fn test_copilot_cli_default_verdict_omits_permission_decision() { + let r = copilot_cli_response_for_verdict( + "cargo test", + &cli_args("cargo test"), + PermissionVerdict::Default, + ) + .unwrap(); + assert!( + r.get("permissionDecision").is_none(), + "Default must NOT set permissionDecision — Copilot then runs its normal prompt flow on the rewritten command" + ); + assert_eq!(r["modifiedArgs"]["command"], "rtk cargo test"); + } + + #[test] + fn test_copilot_cli_explicit_allow_returns_allow_with_rewrite() { + let r = copilot_cli_response_for_verdict( + "cargo test", + &cli_args("cargo test"), + PermissionVerdict::Allow, + ) + .unwrap(); + assert_eq!(r["permissionDecision"], "allow"); + assert_eq!(r["modifiedArgs"]["command"], "rtk cargo test"); + } + + #[test] + fn test_copilot_cli_deny_verdict_returns_none() { + assert!(copilot_cli_response_for_verdict( + "cargo test", + &cli_args("cargo test"), + PermissionVerdict::Deny + ) + .is_none()); + } + + #[test] + fn test_copilot_cli_passthrough_unsupported() { + assert!(copilot_cli_response("htop", &cli_args("htop")).is_none()); + } + + #[test] + fn test_copilot_cli_passthrough_already_rtk() { + assert!(copilot_cli_response("rtk cargo test", &cli_args("rtk cargo test")).is_none()); + } + + #[test] + fn test_copilot_cli_passthrough_heredoc() { + let cmd = "cat < } } +/// Resolve the final write target: if `path` is a symlink, follow it so +/// the atomic rename lands on the real file and the symlink is preserved. +fn resolve_atomic_target(path: &Path) -> PathBuf { + fs::canonicalize(path).unwrap_or_else(|_| path.to_path_buf()) +} + /// Atomic write using tempfile + rename /// Prevents corruption on crash/interrupt +/// Follows symlinks so the link itself is preserved. fn atomic_write(path: &Path, content: &str) -> Result<()> { - let parent = path.parent().with_context(|| { + let target = resolve_atomic_target(path); + let parent = target.parent().with_context(|| { format!( "Cannot write to {}: path has no parent directory", - path.display() + target.display() ) })?; @@ -402,10 +411,10 @@ fn atomic_write(path: &Path, content: &str) -> Result<()> { .with_context(|| format!("Failed to write {} bytes to temp file", content.len()))?; // Atomic rename - temp_file.persist(path).with_context(|| { + temp_file.persist(&target).with_context(|| { format!( "Failed to atomically replace {} (disk full?)", - path.display() + target.display() ) })?; @@ -3845,7 +3854,9 @@ fn uninstall_gemini(ctx: InitContext) -> Result> { // ── Copilot integration ───────────────────────────────────── +// PreToolUse = VS Code schema, preToolUse = Copilot CLI schema (same file, both hosts). const COPILOT_HOOK_JSON: &str = r#"{ + "version": 1, "hooks": { "PreToolUse": [ { @@ -3854,6 +3865,15 @@ const COPILOT_HOOK_JSON: &str = r#"{ "cwd": ".", "timeout": 5 } + ], + "preToolUse": [ + { + "type": "command", + "bash": "rtk hook copilot", + "powershell": "rtk hook copilot", + "cwd": ".", + "timeoutSec": 5 + } ] } } @@ -3901,8 +3921,8 @@ pub fn run_copilot(ctx: InitContext) -> Result<()> { /// `cargo test`'s default parallel execution). fn run_copilot_at(base: &Path, ctx: InitContext) -> Result<()> { let InitContext { dry_run, .. } = ctx; - let github_dir = base.join(".github"); - let hooks_dir = github_dir.join("hooks"); + let github_dir = base.join(GITHUB_DIR); + let hooks_dir = github_dir.join(HOOKS_SUBDIR); if !dry_run { fs::create_dir_all(&hooks_dir) @@ -3912,7 +3932,7 @@ fn run_copilot_at(base: &Path, ctx: InitContext) -> Result<()> { // 1. Upsert RTK marker block in copilot-instructions.md (preserves user content). // Done BEFORE writing the hook config so a malformed file aborts the install // without leaving a stale hook on disk. - let instructions_path = github_dir.join("copilot-instructions.md"); + let instructions_path = github_dir.join(COPILOT_INSTRUCTIONS_FILE); write_rtk_block( &instructions_path, COPILOT_INSTRUCTIONS, @@ -3922,7 +3942,7 @@ fn run_copilot_at(base: &Path, ctx: InitContext) -> Result<()> { )?; // 2. Write hook config (only reached if the upsert above succeeded). - let hook_path = hooks_dir.join("rtk-rewrite.json"); + let hook_path = hooks_dir.join(COPILOT_HOOK_FILE); write_if_changed(&hook_path, COPILOT_HOOK_JSON, "Copilot hook config", ctx)?; if dry_run { @@ -3939,6 +3959,210 @@ fn run_copilot_at(base: &Path, ctx: InitContext) -> Result<()> { Ok(()) } +/// Entry point for `rtk init --uninstall --copilot` (project-scoped, like install). +pub fn uninstall_copilot(ctx: InitContext) -> Result<()> { + let InitContext { dry_run, .. } = ctx; + let removed = uninstall_copilot_at(Path::new("."), ctx)?; + + if removed.is_empty() { + println!("RTK Copilot support was not installed (nothing to remove)"); + } else { + let header = if dry_run { + "[dry-run] would uninstall RTK (GitHub Copilot):" + } else { + "RTK uninstalled (GitHub Copilot):" + }; + println!("{}", header); + for item in &removed { + println!(" - {}", item); + } + if !dry_run { + println!("\nRestart your IDE or Copilot CLI session to apply changes."); + } + } + + if dry_run { + print_dry_run_footer(); + } + Ok(()) +} + +/// Same as [`uninstall_copilot`] but operates relative to an explicit base path. +fn uninstall_copilot_at(base: &Path, ctx: InitContext) -> Result> { + let InitContext { dry_run, .. } = ctx; + let github_dir = base.join(GITHUB_DIR); + let mut removed = Vec::new(); + + let hook_path = github_dir.join(HOOKS_SUBDIR).join(COPILOT_HOOK_FILE); + if hook_path.exists() { + if dry_run { + println!( + "[dry-run] would remove hook config: {}", + hook_path.display() + ); + } else { + // nosemgrep: filesystem-deletion -- Copilot uninstall removes only the RTK-managed hook config. + fs::remove_file(&hook_path) + .with_context(|| format!("Failed to remove hook: {}", hook_path.display()))?; + } + removed.push(format!("Hook config: {}", hook_path.display())); + } + + let instructions_path = github_dir.join(COPILOT_INSTRUCTIONS_FILE); + if instructions_path.exists() { + let content = fs::read_to_string(&instructions_path) + .with_context(|| format!("Failed to read {}", instructions_path.display()))?; + if content.contains(RTK_BLOCK_START) { + let (cleaned, did_remove) = remove_rtk_block(&content); + if did_remove { + if dry_run { + println!( + "[dry-run] would remove rtk-instructions block from {}", + instructions_path.display() + ); + } else { + atomic_write(&instructions_path, &cleaned).with_context(|| { + format!("Failed to write {}", instructions_path.display()) + })?; + } + removed.push(format!( + "{}: removed rtk-instructions block", + COPILOT_INSTRUCTIONS_FILE + )); + } + } + } + + Ok(removed) +} + +fn copilot_user_dir() -> Result { + if let Ok(custom) = std::env::var(COPILOT_HOME_ENV) { + return Ok(PathBuf::from(custom)); + } + let home = dirs::home_dir().context("could not determine home directory")?; + Ok(home.join(COPILOT_USER_DIR)) +} + +pub fn run_copilot_global(ctx: InitContext) -> Result<()> { + let copilot_dir = copilot_user_dir()?; + run_copilot_global_at(&copilot_dir, ctx) +} + +fn run_copilot_global_at(copilot_dir: &Path, ctx: InitContext) -> Result<()> { + let InitContext { dry_run, .. } = ctx; + let hooks_dir = copilot_dir.join(HOOKS_SUBDIR); + + if !dry_run { + fs::create_dir_all(&hooks_dir) + .with_context(|| format!("Failed to create {} directory", hooks_dir.display()))?; + } + + let instructions_path = copilot_dir.join(COPILOT_INSTRUCTIONS_FILE); + write_rtk_block( + &instructions_path, + COPILOT_INSTRUCTIONS, + "Copilot user-level instructions", + "rtk init --global --copilot", + ctx, + )?; + + let hook_path = hooks_dir.join(COPILOT_HOOK_FILE); + write_if_changed( + &hook_path, + COPILOT_HOOK_JSON, + "Copilot global hook config", + ctx, + )?; + + if dry_run { + print_dry_run_footer(); + } else { + println!("\nGitHub Copilot global integration installed (user-scoped).\n"); + println!(" Hook config: {}", hook_path.display()); + println!(" Instructions: {}", instructions_path.display()); + println!("\n Applies to all Copilot CLI sessions on this machine."); + println!(" Restart your Copilot CLI session to activate.\n"); + } + + Ok(()) +} + +pub fn uninstall_copilot_global(ctx: InitContext) -> Result<()> { + let copilot_dir = copilot_user_dir()?; + let InitContext { dry_run, .. } = ctx; + let removed = uninstall_copilot_global_at(&copilot_dir, ctx)?; + + if removed.is_empty() { + println!("RTK global Copilot support was not installed (nothing to remove)"); + } else { + let header = if dry_run { + "[dry-run] would uninstall RTK (global GitHub Copilot):" + } else { + "RTK uninstalled (global GitHub Copilot):" + }; + println!("{}", header); + for item in &removed { + println!(" - {}", item); + } + if !dry_run { + println!("\nRestart your Copilot CLI session to apply changes."); + } + } + + if dry_run { + print_dry_run_footer(); + } + Ok(()) +} + +fn uninstall_copilot_global_at(copilot_dir: &Path, ctx: InitContext) -> Result> { + let InitContext { dry_run, .. } = ctx; + let hook_path = copilot_dir.join(HOOKS_SUBDIR).join(COPILOT_HOOK_FILE); + let mut removed = Vec::new(); + + if hook_path.exists() { + if dry_run { + println!( + "[dry-run] would remove hook config: {}", + hook_path.display() + ); + } else { + // nosemgrep: filesystem-deletion -- Copilot global uninstall removes only the RTK-managed hook config. + fs::remove_file(&hook_path) + .with_context(|| format!("Failed to remove hook: {}", hook_path.display()))?; + } + removed.push(format!("Hook config: {}", hook_path.display())); + } + + let instructions_path = copilot_dir.join(COPILOT_INSTRUCTIONS_FILE); + if instructions_path.exists() { + let content = fs::read_to_string(&instructions_path) + .with_context(|| format!("Failed to read {}", instructions_path.display()))?; + if content.contains(RTK_BLOCK_START) { + let (cleaned, did_remove) = remove_rtk_block(&content); + if did_remove { + if dry_run { + println!( + "[dry-run] would remove rtk-instructions block from {}", + instructions_path.display() + ); + } else { + atomic_write(&instructions_path, &cleaned).with_context(|| { + format!("Failed to write {}", instructions_path.display()) + })?; + } + removed.push(format!( + "{}: removed rtk-instructions block", + COPILOT_INSTRUCTIONS_FILE + )); + } + } + } + + Ok(removed) +} + #[cfg(test)] mod tests { use super::*; @@ -5133,6 +5357,48 @@ mod tests { assert_eq!(written, content); } + #[cfg(unix)] + #[test] + fn test_atomic_write_preserves_symlink() { + use std::os::unix::fs::symlink; + + let temp = TempDir::new().unwrap(); + let target_path = temp.path().join("real-settings.json"); + let link_path = temp.path().join("settings.json"); + + fs::write(&target_path, "{}").expect("seed target file"); + symlink(&target_path, &link_path).expect("create symlink"); + + atomic_write(&link_path, "{\"hooks\":{}}").unwrap(); + + let meta = fs::symlink_metadata(&link_path).unwrap(); + assert!(meta.file_type().is_symlink(), "symlink must survive"); + let written = fs::read_to_string(&target_path).unwrap(); + assert_eq!(written, "{\"hooks\":{}}"); + } + + #[cfg(unix)] + #[test] + fn test_atomic_write_preserves_relative_symlink() { + use std::os::unix::fs::symlink; + + let temp = TempDir::new().unwrap(); + let subdir = temp.path().join("real"); + fs::create_dir(&subdir).unwrap(); + let target_path = subdir.join("settings.json"); + let link_path = temp.path().join("settings.json"); + + fs::write(&target_path, "{}").expect("seed target file"); + symlink(Path::new("real/settings.json"), &link_path).expect("create relative symlink"); + + atomic_write(&link_path, "{\"patched\":true}").unwrap(); + + let meta = fs::symlink_metadata(&link_path).unwrap(); + assert!(meta.file_type().is_symlink(), "symlink must survive"); + let written = fs::read_to_string(&target_path).unwrap(); + assert_eq!(written, "{\"patched\":true}"); + } + // Test for preserve_order round-trip #[test] fn test_preserve_order_round_trip() { @@ -6254,6 +6520,274 @@ mod tests { assert!(content.contains("rtk cargo test")); } + #[test] + fn test_copilot_hook_json_serves_both_vscode_and_cli_schemas() { + let v: serde_json::Value = serde_json::from_str(COPILOT_HOOK_JSON).unwrap(); + + let vscode = &v["hooks"]["PreToolUse"][0]; + assert_eq!(vscode["command"], "rtk hook copilot"); + assert!(vscode["timeout"].is_number(), "VS Code uses `timeout`"); + + assert_eq!(v["version"], 1, "Copilot CLI requires top-level version"); + let cli = &v["hooks"]["preToolUse"][0]; + assert_eq!(cli["bash"], "rtk hook copilot"); + assert_eq!(cli["powershell"], "rtk hook copilot"); + assert!( + cli["timeoutSec"].is_number(), + "Copilot CLI uses `timeoutSec`" + ); + } + + #[test] + fn test_copilot_init_writes_dual_schema_to_disk() { + let temp = TempDir::new().unwrap(); + run_copilot_at(temp.path(), InitContext::default()).unwrap(); + + let hook_path = temp + .path() + .join(".github") + .join("hooks") + .join("rtk-rewrite.json"); + let v: serde_json::Value = + serde_json::from_str(&fs::read_to_string(&hook_path).unwrap()).unwrap(); + + assert_eq!(v["hooks"]["PreToolUse"][0]["command"], "rtk hook copilot"); + assert_eq!(v["version"], 1); + assert_eq!(v["hooks"]["preToolUse"][0]["bash"], "rtk hook copilot"); + } + + #[test] + fn test_copilot_uninstall_removes_hook_and_block() { + let temp = TempDir::new().unwrap(); + run_copilot_at(temp.path(), InitContext::default()).unwrap(); + + let hook_path = temp + .path() + .join(".github") + .join("hooks") + .join("rtk-rewrite.json"); + let instructions_path = temp.path().join(".github").join("copilot-instructions.md"); + assert!(hook_path.exists()); + + let removed = uninstall_copilot_at(temp.path(), InitContext::default()).unwrap(); + + assert!(!removed.is_empty()); + assert!(!hook_path.exists(), "hook config must be removed"); + let instructions = fs::read_to_string(&instructions_path).unwrap(); + assert!( + !instructions.contains(RTK_BLOCK_START), + "RTK block must be removed" + ); + } + + #[test] + fn test_copilot_uninstall_preserves_user_instructions() { + let temp = TempDir::new().unwrap(); + let github_dir = temp.path().join(".github"); + fs::create_dir_all(&github_dir).unwrap(); + let instructions_path = github_dir.join("copilot-instructions.md"); + fs::write(&instructions_path, "# My rules\n\nAlways use pnpm.\n").unwrap(); + + run_copilot_at(temp.path(), InitContext::default()).unwrap(); + uninstall_copilot_at(temp.path(), InitContext::default()).unwrap(); + + let after = fs::read_to_string(&instructions_path).unwrap(); + assert!(after.contains("Always use pnpm."), "user content preserved"); + assert!(!after.contains(RTK_BLOCK_START), "RTK block removed"); + } + + #[test] + fn test_copilot_uninstall_dry_run_keeps_files() { + let temp = TempDir::new().unwrap(); + run_copilot_at(temp.path(), InitContext::default()).unwrap(); + let hook_path = temp + .path() + .join(".github") + .join("hooks") + .join("rtk-rewrite.json"); + + let ctx = InitContext { + verbose: 0, + dry_run: true, + }; + uninstall_copilot_at(temp.path(), ctx).unwrap(); + + assert!(hook_path.exists(), "dry-run must not remove hook config"); + } + + #[test] + fn test_copilot_uninstall_nothing_when_absent() { + let temp = TempDir::new().unwrap(); + let removed = uninstall_copilot_at(temp.path(), InitContext::default()).unwrap(); + assert!(removed.is_empty(), "nothing to remove in a clean project"); + } + + #[test] + fn test_copilot_install_does_not_touch_other_hooks() { + let temp = TempDir::new().unwrap(); + let hooks_dir = temp.path().join(".github").join("hooks"); + fs::create_dir_all(&hooks_dir).unwrap(); + let other_hook = hooks_dir.join("user-policy.json"); + let other_content = + r#"{"hooks":{"sessionStart":[{"type":"command","command":"echo hi"}]}}"#; + fs::write(&other_hook, other_content).unwrap(); + + run_copilot_at(temp.path(), InitContext::default()).unwrap(); + + assert!(other_hook.exists(), "third-party hook file must remain"); + assert_eq!( + fs::read_to_string(&other_hook).unwrap(), + other_content, + "third-party hook content must be unchanged by rtk install" + ); + } + + #[test] + fn test_copilot_uninstall_does_not_touch_other_hooks() { + let temp = TempDir::new().unwrap(); + run_copilot_at(temp.path(), InitContext::default()).unwrap(); + + let hooks_dir = temp.path().join(".github").join("hooks"); + let other_hook = hooks_dir.join("user-policy.json"); + let other_content = + r#"{"hooks":{"sessionStart":[{"type":"command","command":"echo hi"}]}}"#; + fs::write(&other_hook, other_content).unwrap(); + + uninstall_copilot_at(temp.path(), InitContext::default()).unwrap(); + + assert!( + other_hook.exists(), + "third-party hook file must survive rtk uninstall" + ); + assert_eq!( + fs::read_to_string(&other_hook).unwrap(), + other_content, + "third-party hook content must be unchanged by rtk uninstall" + ); + assert!( + !hooks_dir.join("rtk-rewrite.json").exists(), + "rtk's own hook must still be removed" + ); + } + + #[test] + fn test_copilot_global_install_writes_hook() { + let temp = TempDir::new().unwrap(); + run_copilot_global_at(temp.path(), InitContext::default()).unwrap(); + + let hook_path = temp.path().join("hooks").join("rtk-rewrite.json"); + assert!(hook_path.exists()); + let v: serde_json::Value = + serde_json::from_str(&fs::read_to_string(&hook_path).unwrap()).unwrap(); + assert_eq!(v["version"], 1); + assert_eq!(v["hooks"]["PreToolUse"][0]["command"], "rtk hook copilot"); + assert_eq!(v["hooks"]["preToolUse"][0]["bash"], "rtk hook copilot"); + } + + #[test] + fn test_copilot_global_install_writes_instructions() { + let temp = TempDir::new().unwrap(); + run_copilot_global_at(temp.path(), InitContext::default()).unwrap(); + let instructions = temp.path().join(COPILOT_INSTRUCTIONS_FILE); + assert!( + instructions.exists(), + "user-level instructions must be written" + ); + let content = fs::read_to_string(&instructions).unwrap(); + assert!(content.contains(RTK_BLOCK_START)); + assert!(content.contains("rtk cargo test")); + } + + #[test] + fn test_copilot_global_install_preserves_existing_user_instructions() { + let temp = TempDir::new().unwrap(); + let instructions = temp.path().join(COPILOT_INSTRUCTIONS_FILE); + fs::write(&instructions, "# My rules\n\nAlways use pnpm.\n").unwrap(); + + run_copilot_global_at(temp.path(), InitContext::default()).unwrap(); + + let content = fs::read_to_string(&instructions).unwrap(); + assert!( + content.contains("Always use pnpm."), + "user content must be preserved" + ); + assert!(content.contains(RTK_BLOCK_START)); + } + + #[test] + fn test_copilot_global_uninstall_preserves_user_instructions() { + let temp = TempDir::new().unwrap(); + let instructions = temp.path().join(COPILOT_INSTRUCTIONS_FILE); + fs::write(&instructions, "# My rules\n\nAlways use pnpm.\n").unwrap(); + + run_copilot_global_at(temp.path(), InitContext::default()).unwrap(); + uninstall_copilot_global_at(temp.path(), InitContext::default()).unwrap(); + + let content = fs::read_to_string(&instructions).unwrap(); + assert!(content.contains("Always use pnpm.")); + assert!(!content.contains(RTK_BLOCK_START), "RTK block removed"); + } + + #[test] + fn test_copilot_global_uninstall_removes_hook() { + let temp = TempDir::new().unwrap(); + run_copilot_global_at(temp.path(), InitContext::default()).unwrap(); + let hook_path = temp.path().join("hooks").join("rtk-rewrite.json"); + assert!(hook_path.exists()); + + let removed = uninstall_copilot_global_at(temp.path(), InitContext::default()).unwrap(); + assert!(!removed.is_empty()); + assert!(!hook_path.exists()); + } + + #[test] + fn test_copilot_global_uninstall_nothing_when_absent() { + let temp = TempDir::new().unwrap(); + let removed = uninstall_copilot_global_at(temp.path(), InitContext::default()).unwrap(); + assert!(removed.is_empty()); + } + + #[test] + fn test_copilot_global_install_does_not_touch_other_hooks() { + let temp = TempDir::new().unwrap(); + let hooks_dir = temp.path().join("hooks"); + fs::create_dir_all(&hooks_dir).unwrap(); + let other = hooks_dir.join("notification-hooks.json"); + let payload = r#"{"version":1,"hooks":{"agentStop":[{"type":"command","bash":"true"}]}}"#; + fs::write(&other, payload).unwrap(); + + run_copilot_global_at(temp.path(), InitContext::default()).unwrap(); + + assert_eq!(fs::read_to_string(&other).unwrap(), payload); + } + + #[test] + fn test_copilot_global_uninstall_does_not_touch_other_hooks() { + let temp = TempDir::new().unwrap(); + run_copilot_global_at(temp.path(), InitContext::default()).unwrap(); + let hooks_dir = temp.path().join("hooks"); + let other = hooks_dir.join("notification-hooks.json"); + let payload = r#"{"version":1,"hooks":{"agentStop":[{"type":"command","bash":"true"}]}}"#; + fs::write(&other, payload).unwrap(); + + uninstall_copilot_global_at(temp.path(), InitContext::default()).unwrap(); + + assert!(other.exists()); + assert_eq!(fs::read_to_string(&other).unwrap(), payload); + assert!(!hooks_dir.join("rtk-rewrite.json").exists()); + } + + #[test] + fn test_copilot_global_install_dry_run_writes_nothing() { + let temp = TempDir::new().unwrap(); + let ctx = InitContext { + verbose: 0, + dry_run: true, + }; + run_copilot_global_at(temp.path(), ctx).unwrap(); + assert!(!temp.path().join("hooks").join("rtk-rewrite.json").exists()); + } + #[test] fn test_copilot_init_refuses_malformed_block() { let temp = TempDir::new().unwrap(); diff --git a/src/main.rs b/src/main.rs index 22e6cbca8..992f865a2 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1356,6 +1356,17 @@ fn validate_pnpm_filters(filters: &[String], command: &PnpmCommands) -> Option code, Err(e) => { @@ -1821,6 +1832,12 @@ fn run_cli() -> Result { }; if show { hooks::init::show_config(codex)?; + } else if uninstall && copilot { + if global { + hooks::init::uninstall_copilot_global(ctx)?; + } else { + hooks::init::uninstall_copilot(ctx)?; + } } else if uninstall { uninstall_init_dispatch( agent, @@ -1841,7 +1858,11 @@ fn run_cli() -> Result { }; hooks::init::run_gemini(global, hook_only, patch_mode, ctx)?; } else if copilot { - hooks::init::run_copilot(ctx)?; + if global { + hooks::init::run_copilot_global(ctx)?; + } else { + hooks::init::run_copilot(ctx)?; + } } else if agent == Some(AgentTarget::Pi) { hooks::init::run_pi_mode(global, ctx)? } else if agent == Some(AgentTarget::Kilocode) { @@ -3153,6 +3174,40 @@ mod tests { } } + #[test] + #[ignore] // Integration test: requires `cargo build` first + fn test_broken_pipe_does_not_crash() { + let bin_path = std::path::PathBuf::from(env!("CARGO_MANIFEST_DIR")) + .join("target") + .join("debug") + .join("rtk"); + assert!( + bin_path.exists(), + "Debug binary not found at {:?} - run `cargo build` first", + bin_path + ); + + let mut child = std::process::Command::new(&bin_path) + .args(["git", "log", "--oneline", "-50"]) + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()) + .spawn() + .expect("Failed to spawn rtk"); + + // Read one byte then drop stdout to close the pipe. + let mut stdout = child.stdout.take().unwrap(); + let mut buf = [0u8; 1]; + let _ = std::io::Read::read(&mut stdout, &mut buf); + + let status = child.wait().expect("Failed to wait for rtk"); + let code = status.code().unwrap_or(-1); + + assert_ne!( + code, 134, + "rtk crashed with SIGABRT (exit 134) on broken pipe - SIGPIPE handler missing" + ); + } + #[test] fn test_ultra_compact_long_form_still_works() { let cli = Cli::try_parse_from(["rtk", "--ultra-compact", "git", "status"]).unwrap();