Conversation
Review: PR #232Thanks for this PR — great initiative! The two-tier routing design (static name matching + package.json detection) is clean and the Local test results (14 commands)
Comparison — The main selling point of this PR (smart routing to vitest/playwright filters) doesn't work in practice. Bugs to fix1. Vitest/Playwright filter routing is broken
Fix: Apply 2. Reproduced: added
Fix: Add a denylist of known pnpm native subcommands ( 3.
Fix: On non-zero exit, don't apply the boilerplate filter — show the raw error output. Other items
What works well
Thanks for the solid work — keep it up! Happy to re-review once the vitest routing and subcommand interception are fixed. |
|
thanks for the review @pszymkowiak for the denylist, I have made what you asked but what do you think of inverting the logic ? |
|
Re-reviewed after your updates — all 3 critical bugs from the original review are fixed. Code is solid, 493 tests pass, manual testing OK. LGTM on the code side. Regarding your question about inverting the logic: you're right, approach 2 is better. Checking "is this in package.json scripts?" is more future-proof than maintaining a denylist of Blocker: there's a merge conflict that needs resolving before we can merge. Please rebase on master. Once the conflict is fixed, this is good to go. |
Route pnpm scripts to specialized filters (vitest, tsc, eslint, prettier, playwright) via static name matching and package.json auto-detection. Supports both `rtk pnpm run test` and `rtk pnpm test` shorthand. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Move 5 lazy_static regex definitions from inside filter_pnpm_run_output() to module level, matching RTK codebase convention (QUAL-04) - Fix "npm test" -> "pnpm test" label in FilterRoute::TestRunner arm (QUAL-03) - Fix pre-existing cargo fmt issues in registry.rs and go_cmd.rs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…G-03) - Add NATIVE_PNPM_COMMANDS const sorted array with 41 native pnpm commands - Check denylist FIRST in is_pnpm_script() before route_script/package.json - Excludes run/test/start (script shortcuts that go through smart routing) - Add test_native_commands_not_intercepted for 16 key native commands - Add test_native_commands_sorted to guard binary_search correctness - Add test_native_denylist_does_not_block_script_shortcuts Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…-01) - Change apply_filter return type from tuple to Result<(String, &str)> - Remove std::panic::catch_unwind (no-op with panic=abort in release) - Add empty/whitespace input guard returning Err for fallback - Update run_script caller to match Ok/Err with fallback to stripped output - Fallback warning only in verbose mode (-v) - Update 6 existing label tests + integration test to use .unwrap() - Add test_apply_filter_empty_output_returns_error - Add test_apply_filter_whitespace_only_returns_error Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…QUAL-02) - Add PackageScripts struct caching package.json scripts map (read once per invocation) - Update is_pnpm_script, route_script, run_script to accept cached scripts param - Delete detect_tool_from_package_json (logic moved to PackageScripts::detect_tool) - Update main.rs PnpmCommands::Run and ::Other to load cache once and pass through - Add 12 new tests for PackageScripts, update all existing tests for new signatures - Eliminate redundant package.json reads (was 2-3 per invocation, now exactly 1) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…UG-01, BUG-02, BUG-04) - Feed stdout-only to filter_pnpm_run_output (not concatenated stdout+stderr) - filter_pnpm_run_output returns empty string for pure boilerplate (not "ok +") - "ok +" gated on success path in run_script (exit code 0 AND empty stripped stdout) - Add strip_pnpm_stderr: removes ELIFECYCLE boilerplate but preserves ERR_PNPM messages - Add ELIFECYCLE_ONLY regex for stderr stripping (separate from combined ELIFECYCLE|ERR_PNPM) - Failure path: show filtered stdout + stripped stderr (no fake "ok +") - Tee recovery still gets combined stdout+stderr via raw_for_tee - Add 8 new tests, update 2 existing test assertions Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ywright (QUAL-05) - test_full_pipeline_vitest_savings: pnpm stdout -> strip boilerplate -> VitestParser -> 60%+ savings - test_full_pipeline_playwright_savings: pnpm stdout -> strip boilerplate -> PlaywrightParser -> 60%+ savings - Realistic inline JSON fixtures with 25 vitest tests across 5 suites, 10 playwright specs across 3 suites - Pipeline validated: filter_pnpm_run_output strips pnpm headers, apply_filter compresses via specialized parsers Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove "typecheck", "format", "lint", "test", and prefix matching from Tier 1 static routing in route_script(). Only truly unambiguous names (vitest, tsc, prettier) remain — everything else routes via package.json detection (Tier 2), which correctly identifies the actual tool. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The ELIFECYCLE regex matched both ELIFECYCLE and ERR_PNPM patterns, causing `rtk pnpm run test` to show empty output when run outside a project directory. Switch to ELIFECYCLE_ONLY so ERR_PNPM_* messages pass through to the user. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace hardcoded NATIVE_PNPM_COMMANDS blocklist (40 entries) with allowlist logic — unknown commands now default to passthrough instead of requiring explicit enumeration of every native pnpm subcommand. Eliminates stale-list maintenance burden when pnpm adds new commands. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ion) PackageScripts::load() only looked for package.json in CWD, so running `rtk pnpm run test:e2e` from a subdirectory (e.g. src/) would miss the project root's package.json and skip script-based filter routing. Add find_package_json() that walks up to 10 parent directories, matching pnpm's own workspace resolution behavior. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Thanks @pszymkowiak! Merge conflict is resolved. I've refactored to the allowlist approach: is_pnpm_script() now reads package.json scripts instead of maintaining a denylist of native pnpm commands. On top of that, package.json resolution now walks parent directories to mirror pnpm's own behavior. Ready for merge when you are. |
|
Hi @denneulin, the three-tier routing in
Also: both this PR and your #226 touch |
Replace the flat `rtk npm <args>` catch-all with a proper subcommand enum matching the pnpm architecture (PR rtk-ai#232): rtk npm run build → next_cmd filter rtk npm run typecheck → tsc_cmd filter rtk npm run lint → lint_cmd filter rtk npm run <other> → boilerplate stripping (run_script) rtk npm install / ci … → raw passthrough (run_passthrough) Both `rtk npm run build` and `rtk npm build` now correctly invoke `npm run build`; the earlier double-run bug (rtk-ai#438) is structurally impossible with explicit subcommand parsing. Closes rtk-ai#438
Replace the flat `rtk npm <args>` catch-all with a proper subcommand enum matching the pnpm architecture (PR rtk-ai#232): rtk npm run build → next_cmd filter rtk npm run typecheck → tsc_cmd filter rtk npm run lint → lint_cmd filter rtk npm run <other> → boilerplate stripping (run_script) rtk npm install / ci … → raw passthrough (run_passthrough) Both `rtk npm run build` and `rtk npm build` now correctly invoke `npm run build`; the earlier double-run bug (rtk-ai#438) is structurally impossible with explicit subcommand parsing. Closes rtk-ai#438
|
|
|
Hi! Two things needed before we can review:
Thanks! |
|
Hey We are cleaning up the codebase and improving the project structure for better onboarding. As part of this effort, PR #826 reorganizes No logic changes — only file moves and import path updates. What you need to doRebase your branch on git fetch origin && git rebase origin/developGit detects renames automatically. If you get import conflicts, update the paths: use crate::git; // now: use crate::cmds::git::git;
use crate::tracking; // now: use crate::core::tracking;
use crate::config; // now: use crate::core::config;
use crate::init; // now: use crate::hooks::init;
use crate::gain; // now: use crate::analytics::gain;Need help rebasing? Tag @aeppling |
Replace the flat `rtk npm <args>` catch-all with a proper subcommand enum matching the pnpm architecture (PR rtk-ai#232): rtk npm run build → next_cmd filter rtk npm run typecheck → tsc_cmd filter rtk npm run lint → lint_cmd filter rtk npm run <other> → boilerplate stripping (run_script) rtk npm install / ci … → raw passthrough (run_passthrough) Both `rtk npm run build` and `rtk npm build` now correctly invoke `npm run build`; the earlier double-run bug (rtk-ai#438) is structurally impossible with explicit subcommand parsing. Closes rtk-ai#438
Add transparent pnpm script support so developers can run normal pnpm commands (pnpm <script>, pnpm --filter X <script>, pnpm run <script>) and have RTK automatically intercept, rewrite, and compress output. Hook rewrite layer (rules.rs + registry.rs): - New rule: pnpm <script-name-with-colon-or-dash> → rtk pnpm run <script> - Preserves --filter/-F flags for monorepo patterns - Handles run-script edge case via rule ordering (RegexSet last-match) Smart script routing (pnpm_cmd.rs, ported from PR rtk-ai#232): - Reads package.json to detect underlying tool (vitest, eslint, tsc, etc.) - Routes to specialized parsers for maximum token compression Vitest streaming (new VitestStreamFilter): - Replaces exec_capture with run_streamed for vitest routes - Real-time line-by-line output prevents AI tool timeouts - Suppresses pnpm boilerplate and passing test lines - Shows failures inline as they happen (capped at 10) - Produces compact summary on exit: PASS (6166) | 545 suites | 408s - 94-96% token savings on typical runs Invisible to users: works with all 12 supported AI tools via existing hook infrastructure. No config changes needed. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Add transparent pnpm script support so developers can run normal pnpm commands (pnpm <script>, pnpm --filter X <script>, pnpm run <script>) and have RTK automatically intercept, rewrite, and compress output. Hook rewrite layer (rules.rs + registry.rs): - New rule: pnpm <script-name-with-colon-or-dash> → rtk pnpm run <script> - Preserves --filter/-F flags for monorepo patterns - Handles run-script edge case via rule ordering (RegexSet last-match) Smart script routing (pnpm_cmd.rs, ported from PR rtk-ai#232): - Reads package.json to detect underlying tool (vitest, eslint, tsc, etc.) - Routes to specialized parsers for maximum token compression Vitest streaming (new VitestStreamFilter): - Replaces exec_capture with run_streamed for vitest routes - Real-time line-by-line output prevents AI tool timeouts - Suppresses pnpm boilerplate and passing test lines - Shows failures inline as they happen (capped at 10) - Produces compact summary on exit: PASS (6166) | 545 suites | 408s - 94-96% token savings on typical runs Invisible to users: works with all 12 supported AI tools via existing hook infrastructure. No config changes needed.
Add transparent pnpm script support so developers can run normal pnpm commands (pnpm <script>, pnpm --filter X <script>, pnpm run <script>) and have RTK automatically intercept, rewrite, and compress output. Hook rewrite layer (rules.rs + registry.rs): - New rule: pnpm <script-name-with-colon-or-dash> → rtk pnpm run <script> - Preserves --filter/-F flags for monorepo patterns - Handles run-script edge case via rule ordering (RegexSet last-match) Smart script routing (pnpm_cmd.rs, ported from PR rtk-ai#232): - Reads package.json to detect underlying tool (vitest, eslint, tsc, etc.) - Routes to specialized parsers for maximum token compression Vitest streaming (new VitestStreamFilter): - Replaces exec_capture with run_streamed for vitest routes - Real-time line-by-line output prevents AI tool timeouts - Suppresses pnpm boilerplate and passing test lines - Shows failures inline as they happen (capped at 10) - Produces compact summary on exit: PASS (6166) | 545 suites | 408s - 94-96% token savings on typical runs Invisible to users: works with all 12 supported AI tools via existing hook infrastructure. No config changes needed.
Add transparent pnpm script support so developers can run normal pnpm commands (pnpm <script>, pnpm --filter X <script>, pnpm run <script>) and have RTK automatically intercept, rewrite, and compress output. Hook rewrite layer (rules.rs + registry.rs): - New rule: pnpm <script-name-with-colon-or-dash> → rtk pnpm run <script> - Preserves --filter/-F flags for monorepo patterns - Handles run-script edge case via rule ordering (RegexSet last-match) Smart script routing (pnpm_cmd.rs, ported from PR rtk-ai#232): - Reads package.json to detect underlying tool (vitest, eslint, tsc, etc.) - Routes to specialized parsers for maximum token compression Vitest streaming (new VitestStreamFilter): - Replaces exec_capture with run_streamed for vitest routes - Real-time line-by-line output prevents AI tool timeouts - Suppresses pnpm boilerplate and passing test lines - Shows failures inline as they happen (capped at 10) - Produces compact summary on exit: PASS (6166) | 545 suites | 408s - 94-96% token savings on typical runs Invisible to users: works with all 12 supported AI tools via existing hook infrastructure. No config changes needed.
|
Hi @denneulin — closing as stale + superseded by active work. Update on what's happened since my "LGTM on the code side" comment in
Both target On this PR specifically, three blockers persist 84 days after the
Your three-tier routing in |
…-ai#232, rtk-ai#233) Filter-engine cluster, authored by the codex (gpt-5.6) worker: - rtk-ai#226 whole-blob ANSI/OSC state-machine sanitisation (drops unterminated OSC/DCS incl. across line breaks); CTXCRL_TOML_DEBUG logs a redacted name/fingerprint not the raw command; AggressiveFilter counts the signature brace (no early body exit leaking later secrets); hard UTF-8-safe byte ceiling before allocation. - rtk-ai#232 language-aware stripping: Go receiver signatures, JS/TS template literals + raw strings in block-comment scanning, Python raw/f/''' docstrings. - rtk-ai#233 user TOML-filter regex bounded (size/compile limits, ReDoS); total byte ceiling on many-small-lines; filter-trust EnvOverride audited. Refs: rtk-ai#226 rtk-ai#232 rtk-ai#233 Co-Authored-By: Codex (gpt-5.6-sol) <noreply@openai.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K3SAimSBwPUp4MkRXaYXSU
Large security release completing the codex-5.6-max sweep + 5-voice council audit (rtk-ai#210-rtk-ai#233): rtk-ai#219/rtk-ai#220 (hook-tamper validation rebuild + trust store), rtk-ai#225 (non-Claude handler gating), rtk-ai#212-rtk-ai#218/rtk-ai#230 (permission gate + lexer), rtk-ai#211 (Tirith deadlock), rtk-ai#222 (config injection), rtk-ai#226/rtk-ai#232/rtk-ai#233 (filter engine). The two big clusters were authored by the Codex worker in isolated worktrees, driver-verified and non-author-council-reviewed. See CHANGELOG. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K3SAimSBwPUp4MkRXaYXSU
Summary
rtk pnpm run <script>andrtk pnpm <script>shorthand with smart routing to specialized filterspackage.json auto-detection for custom scripts (e.g.,
test:e2e→ detectsplaywright test→PlaywrightParser)
filter_tsc_output,filter_generic_lint,extract_test_summarytopub(crate)for cross-modulereuse
Token Savings
pnpm [run] test(vitest)pnpm [run] typecheck(tsc)pnpm [run] lint(eslint)pnpm [run] prettier --checkpnpm [run] test:e2e(playwright)pnpm [run] build(unrouted)How It Works
rtk pnpm run test→PnpmCommands::Run→run_script("test")→ boilerplate strip → TestRunner filterrtk pnpm test→PnpmCommands::Other→is_pnpm_script("test")→run_script("test")rtk pnpm test:e2e→ readspackage.json→ detectsplaywright test→ PlaywrightParserrtk pnpm store prune→is_pnpm_script("store")= false → passthroughTest plan
apply_filter labels, integration, is_pnpm_script)
cargo fmt --all && cargo clippy --all-targets && cargo test --all— 437 tests passrtk pnpm run test,rtk pnpm test,rtk pnpm run lint