Repository navigation
Conversation
`rtk pytest` prints its summary first and the tee pointers last, so rewriting the producer in `pytest -q 2>&1 | tail -3` made tail return only the pointers: no counts and no failure names on a failing run. Safe pipe consumers now record whether they select lines by position (head, tail; not cat), and producers listed in POSITION_SHIFTING_PRODUCERS stay raw in front of one. Other producers and `| cat` keep the rtk-ai#3171 rewrite. Fixes rtk-ai#4445
📊 Automated PR Analysis
SummaryFixes a bug where rewriting Review Checklist
Linked issues: #4445 Analyzed automatically by wshm · This is an automated analysis, not a human review. |
Protocol-zero-0
left a comment
There was a problem hiding this comment.
Independently validated this narrow fix against its own parent eb225a1 and head f365206 with real pytest 9.0.3 on Linux.
For a fixture with 12 failures and 5 passing tests, both python3 -m pytest test_probe.py -q 2>&1 | tail -3 and pytest test_probe.py -q 2>&1 | cat | tail -3 lose the session counts on the parent and return recall pointers. This head leaves those producers raw, so the selected output again contains the final failure names and 12 failed, 5 passed. All raw/parent/head executions retain exit 1 under bash -o pipefail.
Nine rewrite probes cover tail, head, an intermediate cat, cat-only, standalone pytest, a compound command, stderr piping, following tail, and final-stage grep. Cat-only and standalone pytest still rewrite; the compound's independent trailing pytest still rewrites; existing conservative stderr/following-tail behavior is preserved. Twelve real pytest executions cover the two failing positional pipelines plus cat-only and standalone controls. Both versions built with cargo build --locked --offline; 21 targeted pipeline unit tests and 36 hook-decision protocol integration tests pass on the head.
This approval covers the pytest positional-consumer fix for #4445. It does not establish that other producer filters preserve line positions, or replace the broader design discussion in #4382/#4207. I did not run the complete suite, clippy, or cross-platform tests.
Resolve a conflict in src/discover/README.md: take develop's wording (case terminators, quoted shell wrappers) and keep this branch's sentence on positional consumers (rtk-ai#4445).
Protocol-zero-0
left a comment
There was a problem hiding this comment.
Re-verified the develop merge at 2df1f37 against its current develop parent ffa5651 and the previously reviewed f365206. The conflict is resolved, and the fix still preserves the pytest summary before positional consumers.
On Linux with real pytest 9.0.3, a fixture with 12 failures and 5 passes retains the failure names and final counts through tail -3, cat | tail -3, and bash -c wrapping the tail pipeline. Current develop reproduces the missing-summary behavior in those three paths. Standalone pytest and pytest | cat still rewrite. Complete case statements stay raw in all three binaries. Exit behavior matches raw execution, including the nested bash pipeline's existing exit 0 when pipefail is not enabled inside that shell.
Validation: 14 rewrite cases across three binaries (42 calls), 32 real pytest executions, 680 discover tests and 36 hook decision protocol tests pass; fmt and clippy with warnings denied pass. The README retains develop's case-terminator and shell-wrapper wording alongside this positional-consumer rule. Full repository tests and other platforms were not run.
My approval stands for this merged head. #4382 remains the broader alternative; the integration choice stays with the maintainers.
Summary
Since #3171/#3343 the producer in front of a display-only consumer gets rewritten, so
python3 -m pytest tests/ -q 2>&1 | tail -3becamertk pytest ... | tail -3.rtk pytestprints its summary first and the tee pointers last. On a failing run,tail -3then returned only the pointers, with no counts and no failure names.SafePipeConsumergets apositionalfield:headandtailare positional,catis not.analyze_pipelinerecords whether any consumer is positional.RewriteContext::PipelineProducercarries that flag. A producer listed in the newPOSITION_SHIFTING_PRODUCERS(currently onlyrtk pytest) stays raw when a positional consumer follows it.pytestalone,pytest | cat, other producers in front ofhead/tail, or final-stagegrep/rg.src/discover/README.mddocuments the new rule.This is the narrow form of the fix #4207 proposes for grep/rg. The table can take other producers whose rtk output moves lines (grep/rg from #4207,
git logcaps) as separate, reviewable changes. #4382 takes a broader route: it stops rewriting every pipeline producer. If that lands, this change is no longer needed.Fixes #4445
Test plan
test_rewrite_pipe_reordering_producer_before_positional_consumer_stays_rawfailed on develop (left: Some("rtk pytest t.py -q 2>&1 | tail -3")) and passes with the fixtest_rewrite_pipe_producer_batch_rules_rewrittennow checkspytest | cat(it used to expectpytest | tail -20to be rewritten)cargo fmt --all --checkcargo clippy --all-targetswith zero warningscargo test: 3894 unit tests plus the integration suites, 0 failuresrtk rewrite "python3 -m pytest tests/ -q 2>&1 | tail -3"exits 1 (left raw);rtk rewrite "pytest -q"still givesrtk pytest -q