Annotate rust_consume_stream as implemented but not integrated (#127) - #164
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
This PR resolves the Key changes:
The docs also align terminology ( WalkthroughMarks ChangesDeferred consume dispatch annotation and guard
Poem
🚥 Pre-merge checks | ✅ 19 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (19 passed)
📋 Issue PlannerLet us write the prompt for your AI agent so you can ship faster (with fewer bugs). View plan for ticket: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
d438c13 to
8a32149
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
8a32149 to
e0405ba
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
e0405ba to
657187f
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
657187f to
88f88f8
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai review |
✅ Action performedReview finished.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
7fac2af to
8b84f66
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
2814da7 to
b076755
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Record the tee hot-path baseline findings in the rust consume-stream component documentation. Keep the current branch's annotation intact: `rust_consume_stream` is implemented and tested, but production dispatch must still wait for the Phase 2 dispatcher, fallback path, and parity property tests. Clarify the design diagrams and backend-selection text so they no longer imply that stdout or stderr consumption is routed through Rust today.
Clarify that the tee profiling baseline provides hotspot evidence for Phase 2, not acceptance of the consume dispatcher. Keep the 20% median wall-time gate tied to a future Rust-versus-Python dispatcher benchmark.
Use a NumPy-style `Notes` section for the `rust_consume_stream` status note, keeping the helper documentation consistent with the rest of its docstring. Switch the not-integrated guard from raw text matching to AST symbol reference detection so comments and docstrings do not create false positives when checking production modules.
Split the consume-stream status guard so the docstring marker and the production-reference scan are checked independently. Add maintainer documentation for the current `rust_consume_stream` state: it is implemented, tested, and exported, but production consume dispatch remains deferred to ADR-002 Phase 2 until dispatcher wiring, Python fallback, and parity/property coverage land.
Add roadmap phases 5-8 translating the tee hot-path profiling baseline and the issue #90/#127 follow-up into outcome-oriented work: - Phase 5 lands the Rust-free wins first (read-size plateau bump and lean per-line observe-hook emission) so the consume dispatcher must beat a tuned Python baseline. - Phase 6 hardens the PyO3 boundary (#90), establishes the 20% wall-time acceptance gate, wires _consume_stream_dispatch, and records the outcome. - Phase 7 scopes raw-sink echo/tee acceleration, profiling first to exclude kernel-bound PTY cost. - Phase 8 collects deferred reliability work (Rust pump FD close race, perf frame attribution) and the native-orchestration go/no-go decisions. Each phase carries a falsifiable GIST idea; tasks cite ADR-002, the design doc, the profiling baseline, and the relevant issues. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Handle AST scan read and parse failures explicitly in the production reference guard, and add coverage for invalid candidate modules. Update the consume-stream documentation to use current `echo=True` terminology and clarify that only pump-side routing has Rust backing today.
Replace the awkward consume-routing wording in ADR-002 and tighten the design guide so it says final stream consumption still uses the Python path until the Phase 2 consume dispatcher is wired and tested.
Avoid implying that a consume dispatcher is already wired for line-callback streams. State that stream consumption stays on the Python pathway until the Phase 2 consume dispatcher exists.
Use the specific module-reference scan error in the guard test and tighten the consume-stream documentation wording. Expand the consume-dispatch roadmap gate so it covers fallback regressions, parity before routing, and outcome-specific documentation dependencies.
Make the production-reference scan error a normal exception and report scan failures from the guard test with an explicit assertion message.
Link the developer guide introduction to ADR-002 and ADR-004 so maintainers can reach the governing stream and docstring decisions early. State that production consume paths continue to use the pure-Python `_consume_stream` implementation until Phase 2 is complete.
Preserve module scan causes in the `rust_consume_stream` guard and report production references with package-relative paths so failures identify the actual module. Tighten the roadmap's performance criteria with objective benchmark and profiler artefact requirements, and add the missing parity-proof dependency before consume benchmarking.
d104210 to
382d120
Compare
|
@coderabbitai Have the following now been resolved? docs/roadmap.md (2)
Add 6.1.2 here, or the benchmark can start before the proptest gate that the success criteria depend on. 🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In
State explicit thresholds or benchmark artefacts for both tasks so completion is objectively testable. 🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In |
|
@coderabbitai Have the following now been resolved? docs/developers-guide.md (2)
As per coding guidelines, link early to the design document, repository layout document, accepted decision records, and other normative references. 🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In Source: Coding guidelines Based on learnings, keep pure Python first-class as the behavioural reference implementation. 🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In Source: Learnings |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== docs/roadmap.md around Phase 5 tasks (220-270) ==="
sed -n '220,270p' docs/roadmap.md
echo
echo "=== docs/roadmap.md around Phase 6 benchmark task (310-335) ==="
sed -n '310,335p' docs/roadmap.md
echo
echo "=== Search for soft qualifiers and dependency marker ==="
rg -n 'measurably|roughly|6\.1\.2|Requires:' docs/roadmap.mdLength of output: 5379
These comments can be marked resolved. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== docs/developers-guide.md lines 1-20 ==="
sed -n '1,20p' docs/developers-guide.md
echo
echo "=== docs/developers-guide.md lines 43-58 ==="
sed -n '43,58p' docs/developers-guide.md
echo
echo "=== ADR references near top ==="
rg -n 'ADR-002|ADR-003|ADR-004' docs/developers-guide.md
echo
echo "=== consume-path wording ==="
rg -n '_consume_stream|not yet routed through production consume paths|production consume paths|pure-Python' docs/developers-guide.mdLength of output: 3421
Mark both comments resolved. |
|
@coderabbitai Have the following failed checks now been resolved? If further work is required, please provide an AI agent prompt for the remaining work to be done to address these failures. Do not treat warnings as optional or aspirational. Where a change is out of scope for this PR, propose a GitHub issue unless one exists already. (Treat documentation and validation coverage as in scope).
❌ Failed checks (1 error, 1 warning)
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== docs/cuprum-design.md lines 1880-1910 ==="
sed -n '1880,1910p' docs/cuprum-design.md
echo
echo "=== echo_io occurrences in docs/cuprum-design.md ==="
rg -n 'echo_io' docs/cuprum-design.md || true
echo
echo "=== Relevant routing wording in docs/cuprum-design.md ==="
rg -n 'echo=True|RunOutputOptions\(echo=True\)|pump-side routing|_pump_stream_dispatch|final stdout/stderr consumption|pure-Python|_consume_stream' docs/cuprum-design.mdLength of output: 4353 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== docs/cuprum-design.md lines 1880-1910 ==="
sed -n '1880,1910p' docs/cuprum-design.md
echo
echo "=== echo_io occurrences in docs/cuprum-design.md ==="
rg -n 'echo_io' docs/cuprum-design.md || true
echo
echo "=== Relevant routing wording in docs/cuprum-design.md ==="
rg -n 'echo=True|RunOutputOptions\(echo=True\)|pump-side routing|_pump_stream_dispatch|final stdout/stderr consumption|pure-Python|_consume_stream' docs/cuprum-design.mdLength of output: 4353
Mark this comment resolved. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== Outline: cuprum/unittests/test_rust_streams.py ==="
ast-grep outline cuprum/unittests/test_rust_streams.py --view expanded | sed -n '1,220p'
echo
echo "=== Guard test section in cuprum/unittests/test_rust_streams.py ==="
python - <<'PY'
from pathlib import Path
path = Path("cuprum/unittests/test_rust_streams.py")
lines = path.read_text().splitlines()
for i, line in enumerate(lines, 1):
if line.startswith("def test_rust_consume_stream_not_referenced_in_production"):
start = max(1, i - 5)
end = min(len(lines), i + 45)
for j in range(start, end + 1):
print(f"{j}: {lines[j-1]}")
break
PY
echo
echo "=== Searches in cuprum/unittests/test_rust_streams.py ==="
rg -n -C2 'relative_to\(package_root\)|scan_errors|__cause__|referencing\.append|_ModuleReferenceScanError' cuprum/unittests/test_rust_streams.py
echo
echo "=== Outline: docs/developers-guide.md ==="
ast-grep outline docs/developers-guide.md --view expanded | sed -n '1,160p' || true
echo
echo "=== Top of docs/developers-guide.md ==="
sed -n '1,20p' docs/developers-guide.md
echo
echo "=== ADR title searches in docs/developers-guide.md and docs/adr-004* ==="
rg -n -C1 'ADR-002|ADR-003|ADR-004|Interrogate docstring' docs/developers-guide.md docs/adr-004* || trueLength of output: 9294
No further work is required for these two checks, so do not open a follow-up issue and do not generate an AI agent prompt. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== Outline: cuprum/unittests/test_rust_streams.py ==="
ast-grep outline cuprum/unittests/test_rust_streams.py --view expanded | sed -n '1,220p'
echo
echo "=== Relevant block: cuprum/unittests/test_rust_streams.py ==="
python - <<'PY'
from pathlib import Path
path = Path("cuprum/unittests/test_rust_streams.py")
lines = path.read_text().splitlines()
for i, line in enumerate(lines, 1):
if "def test_rust_consume_stream_not_referenced_in_production" in line:
start = max(1, i - 5)
end = min(len(lines), i + 45)
for j in range(start, end + 1):
print(f"{j}: {lines[j-1]}")
break
PY
echo
echo "=== Search for relative_to / cause handling in cuprum/unittests/test_rust_streams.py ==="
rg -n -C2 'relative_to\(package_root\)|__cause__|scan_errors|referencing.append|str\(path\)|path.name' cuprum/unittests/test_rust_streams.py
echo
echo "=== Top of docs/developers-guide.md ==="
sed -n '1,20p' docs/developers-guide.md
echo
echo "=== ADR title search in docs/developers-guide.md ==="
rg -n 'ADR-002|ADR-003|ADR-004|Interrogate docstring-coverage gate|Interrogate docstring coverage gate' docs/developers-guide.mdLength of output: 7343
No further work is required for these two checks. Mark both resolved. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== Outline: cuprum/unittests/test_rust_streams.py ==="
ast-grep outline cuprum/unittests/test_rust_streams.py --view expanded || true
echo
echo "=== Relevant symbols in cuprum/unittests/test_rust_streams.py ==="
rg -n -C3 '_ModuleReferenceScanError|test_rust_consume_stream_not_referenced_in_production|relative_to\(package_root\)|__cause__|scan_errors|referencing.append' cuprum/unittests/test_rust_streams.py
echo
echo "=== Slice: cuprum/unittests/test_rust_streams.py around guard test ==="
python3 - <<'PY'
from pathlib import Path
path = Path("cuprum/unittests/test_rust_streams.py")
lines = path.read_text().splitlines()
for i, line in enumerate(lines, 1):
if line.startswith("def test_rust_consume_stream_not_referenced_in_production"):
start = max(1, i - 5)
end = min(len(lines), i + 45)
for j in range(start, end + 1):
print(f"{j}: {lines[j-1]}")
break
PY
echo
echo "=== Top of docs/developers-guide.md ==="
sed -n '1,20p' docs/developers-guide.md
echo
echo "=== ADR title references in docs/developers-guide.md ==="
rg -n -C2 'ADR-002|ADR-003|ADR-004|docstring' docs/developers-guide.md
echo
echo "=== Canonical ADR-004 title from file headings ==="
fd -a 'adr-004*' docs | while read -r f; do
echo "--- $f ---"
sed -n '1,8p' "$f"
doneLength of output: 11739
That matches the heading in No further work remains here. Mark both checks resolved. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
2 participants
Summary
This branch resolves the asymmetric status of
rust_consume_streamby annotating it as implemented but not yet integrated, per the ADR-002 migration plan.Closes #127.
The pump half of the Rust acceleration story is wired (
_pump_stream_dispatchwith Rust-then-Python fallback), butrust_consume_stream— though shipped, public, tested, and documented — has no production caller. ADR-002 gates consume-routing on its Phase 1 measurement schema, so of the two resolutions the issue offers, this takes the annotation path; wiring a_consume_stream_dispatchnow would bypass that evidence gate.Review walkthrough
Validation
make check-fmt: passmake lint: passmake typecheck: passmake test: pass (603 passed, 44 skipped; Rust suite passes with 4 passed)make markdownlint: passmake nixie: passcoderabbit review --agent: 0 findingsNotes
When Phase 2 work lands (parity property tests per #90, dispatcher with fallback), the marker and the guard test are removed together — the guard's failure message says exactly that.
References