Skip to content

Add direct Rush reporter demo path - #5997

Open
Sean Larkin (TheLarkInn) wants to merge 5 commits into
copilot/reporter-r5a-operation-adapterfrom
copilot/reporter-r5b-demo-reporters
Open

Add direct Rush reporter demo path#5997
Sean Larkin (TheLarkInn) wants to merge 5 commits into
copilot/reporter-r5a-operation-adapterfrom
copilot/reporter-r5b-demo-reporters

Conversation

@TheLarkInn

Copy link
Copy Markdown
Member

Part of #5978

Stack

Parent: #5996 (copilot/reporter-r5a-operation-adapter)

This PR is based directly on the R5A operation adapter and intentionally excludes the parallel R6 #5993 bootstrap work. Keep auto-merge disabled while stack ancestors remain open.

Demo path

The direct current-version rush build path now transfers visible operation presentation to the frontend-selected reporters when either an explicit non-legacy --reporter is present or the repository useRushReporter experiment is enabled.

  • the selected primary reporter owns its visible destination;
  • the always-on file reporter owns a complete invocation log under common/temp/rush-logs;
  • the retained StreamCollator writes to a no-op destination, preserving scheduling/writer completion semantics without duplicate terminal output;
  • non-operation Rush terminal messages are converted into semantic reporter messages, while operation summaries come from authoritative operationCompleted events;
  • RUSH_REPORTER=legacy removes reporter controls before launching Rush and returns immediately to the existing legacy path.

The file reporter spools active operation output to owner-only temporary files, then appends each operation as a grouped block in completion order. This preserves stdout/stderr chunk order without unbounded heap buffering.

Reproducible demo

rush build --to @microsoft/rush
node apps/rush/src/test/sandbox/reporter-demo/run.mjs

The script exercises:

node apps/rush/bin/rush build --only @rushstack/rush-reporter
node apps/rush/bin/rush build --only @rushstack/rush-reporter --reporter=plaintext
node apps/rush/bin/rush build --only @rushstack/rush-reporter --reporter=json --log-level=debug
node apps/rush/bin/rush build --only @rushstack/rush-reporter --reporter=ai
RUSH_REPORTER=legacy node apps/rush/bin/rush build --only @rushstack/rush-reporter --reporter=json

Observed result:

  • plaintext reconstructs phase-aware grouped operation output and prints an absolute full-log path;
  • JSON stdout is valid payload-only NDJSON with no blank/control lines;
  • AI emits bounded status/final records, suppresses warning details on failure, and includes the exact full-log path;
  • legacy rollback matches the feature-off transcript after normalizing runtime durations;
  • failed parser/selection paths retain their actionable message and flush through sessionCompleted.

Repository opt-in remains "useRushReporter": true in common/config/rush/experiments.json; removing it or setting RUSH_REPORTER=legacy provides immediate rollback.

Validation

  • rush test --only @rushstack/rush-reporter --only @microsoft/rush-lib --only @microsoft/rush --verbose
    • reporter: 301 tests
    • rush-lib: 770 tests
    • apps/rush: 25 tests
  • rush build --only @rushstack/rush-reporter --only @microsoft/rush-lib --only @microsoft/rush --verbose
  • reporter snapshots/goldens in the targeted suite
  • rush check
  • rush change --verify --no-fetch
  • direct demo matrix above
  • direct failed JSON invocation confirmed final commandResult, commandCompleted, and sessionCompleted, with the actionable error in the full log

Coverage includes success, cache hit, warnings, operation failure, blocked and aborted operations, silent operations, parallel grouping, stdout/stderr ordering, TTY/non-TTY behavior, quiet/verbose/debug controls, watch-cycle completion, and legacy flag-off parity.

Non-goals

@TheLarkInn Sean Larkin (TheLarkInn) left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent R5B review completed against #5978 and RFC phase 4.

Fixed in e61332b:

  • command-specific --json remains the sole stdout owner; incompatible reporter combinations fail before launch;
  • older repository engines force the legacy path and reporter teardown runs on selector failure;
  • file/plaintext grouping is disk-backed without per-graph descriptor growth or unbounded heap retention; stale spools are retained/cleaned safely;
  • watch errors use the semantic terminal, final artifact completeness precedes machine results, zero-width TTYs fall back to 80 columns, and AI parser failures retain actionable context;
  • the demo now self-checks legacy rollback, plaintext/JSON/AI/file/quiet/failure/help/command-JSON output, one visible writer, chunk ordering, final flush, and owner-only logs.

Validation: build succeeded for @rushstack/rush-reporter, @microsoft/rush-lib, and @microsoft/rush; tests 304/773/28; rush check; rush change --verify --no-fetch; expanded demo and real TTY run passed. No unresolved review threads. Auto-merge remains disabled while #5996 and its ancestors are open.

@TheLarkInn
Sean Larkin (TheLarkInn) force-pushed the copilot/reporter-r5b-demo-reporters branch from e61332b to e30aa36 Compare August 28, 2026 08:40
@TheLarkInn

Copy link
Copy Markdown
Member Author

Final restack correction: #5997 was force-replayed onto the exact current remote #5996 tip ac92280377966dc0da67ec6972ea086219abfebb and pushed with --force-with-lease=refs/heads/copilot/reporter-r5b-demo-reporters:e61332b35ccc0964a1f3f240c4a328975b880c24.

Final #5997 tip: e30aa365e5527213bf91ef64b8f8171e3357cc07 (R5B commits 45e43a810d, e30aa365e5). The old and new parent-to-head binary diffs have the identical SHA-256 84086932b410faa5239a08d091c1bf7a92e874bedd84834abbd76fc7af5c4718; the slice remains exactly 31 files with no R5A conflict-resolution delta. GitHub now reports MERGEABLE/CLEAN and auto-merge remains disabled.

Post-restack validation: reporter 304, rush-lib 773, apps/rush 28; expanded reporter demo; rush check; rush change --verify --no-fetch.

@TheLarkInn

Copy link
Copy Markdown
Member Author

Combined deep review of current head e30aa365e5.

  1. HIGH old-engine forceLegacy silently strips explicit non-legacy reporter controls. Reject unsupported explicit requests instead of changing user intent.
  2. MEDIUM MinimalRushConfiguration.commonTempFolder ignores RUSH_TEMP_FOLDER, so logs and purge target different locations.
  3. MEDIUM legacy verbose configuration discovery output is dropped at the repository root. Restore the original predicate and separator.
  4. MEDIUM artifactAvailable.complete is a live getter, so one event can serialize differently across reporter queues. Snapshot the value.
  5. MEDIUM file spooling opens and closes a file for every output chunk on the main thread. Keep bounded persistent descriptors and close them at operation finalization.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: d6318e80-5da9-4858-a147-817e8692f10e
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: d6318e80-5da9-4858-a147-817e8692f10e
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: d6318e80-5da9-4858-a147-817e8692f10e
@TheLarkInn
Sean Larkin (TheLarkInn) force-pushed the copilot/reporter-r5b-demo-reporters branch from e30aa36 to 7159901 Compare August 28, 2026 20:08
@TheLarkInn

Copy link
Copy Markdown
Member Author

Review fixes published at 715990145a68fd8e9b03ce4ecc01b9612c062d87 on exact #5996 head 11a7f8462dd40b1d3c0d83ffafc5f0b748a80394.

Fixed: incompatible-engine controls remain explicit and are never silently stripped; RUSH_TEMP_FOLDER uses Rush normalization for logs and purge; legacy discovery text and blank lines are restored; artifact completeness is a frozen boolean emitted after file closure; file and plaintext spools use persistent bounded descriptors with exact cleanup. Also fixed output-control ownership, purge-log survival, action-name sanitization, TTY multiline activity, verbose versus debug filtering, AI fallback counts, and file-only parser failures.

Validation: reporter 314, rush-lib 786, apps/rush 70; all affected builds; direct demo matrix at /tmp/rush-reporter-demo-5RtqS3; TTY, CI, cache, watch coverage; rush check; stacked change verification. PR is CLEAN and auto-merge remains disabled pending fresh review.

@TheLarkInn

Copy link
Copy Markdown
Member Author

Four-review gate on 715990145a confirms the prior fixes. Two actionable findings remain:

  1. HIGH The lock-conflict reason is emitted as informational text before AlreadyReportedError. The default reporter can erase it, leaving only a generic command failure. Preserve the actionable reason as an error or structured diagnostic.
  2. MEDIUM Watch iterations re-register and re-complete the same operation IDs, but default, plaintext, and AI reporters accumulate lifetime totals. Later cycles can report 20/20 for a 10-operation graph. Reset or replace per-cycle counts on re-registration or watch-cycle completion.

Dismissible low follow-on: configuration discovery should use the effective RUSH_PREVIEW_VERSION when deciding legacy output. Auto-merge remains disabled.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: d6318e80-5da9-4858-a147-817e8692f10e
@TheLarkInn

Copy link
Copy Markdown
Member Author

Round-two findings fixed at 523124198d221cef9ec72c81dd1cfb26eb67023f.

  • Lock contention now emits the actionable reason at error severity in reporter mode while preserving legacy output without duplication.
  • Default, plaintext, and AI watch totals reset per graph cycle, including repeated IDs, silent operations, failure, and recovery.
  • Discovery now uses the effective RUSH_PREVIEW_VERSION, keeping machine stdout clean when the preview matches the frontend.

Validation: reporter 317, rush-lib 787, apps/rush 71; all affected builds; direct demo /tmp/rush-reporter-demo-2ckBQl; preview /tmp/r5b-r2-preview-2ktFGu; real lock contention /tmp/r5b-r2-lock-TtwdV3; TTY/CI/watch coverage; rush check; stacked change verification. PR is CLEAN and auto-merge remains disabled pending fresh review.

@TheLarkInn

Copy link
Copy Markdown
Member Author

Four-review rerun on 523124198d confirms the prior runtime fixes. One MEDIUM concurrency issue remains: a queued watch iteration can register known operation IDs before the prior watchCycleCompleted event. Reporter state can then mix abort completions and next-cycle completions, producing totals such as 20/10.

Required fix: use an explicit cycle boundary or cycle identity that handles overlapping scheduling. Do not assume re-registration occurs after cycle completion.

One dismissible low follow-on remains: make the AI final record consistently per-cycle or session-cumulative after fail-then-success watch recovery. The repeated command-name concern remains dismissed because no supported failing entry point was reproduced.

Track explicit operation graph iteration identities across lifecycle events and isolate reporter totals, diagnostics, and output spools by watch cycle.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: d6318e80-5da9-4858-a147-817e8692f10e
@TheLarkInn

Copy link
Copy Markdown
Member Author

Round-three findings fixed at 97f807ed6549387b6aab2eccec197123196b725d.

  • Operation lifecycle events now carry an explicit graph iteration ID, and default, plaintext, AI, and file reporters isolate overlapping generations.
  • Superseded scheduled iterations no longer register before execution, completed lifecycle-only cycles are released, and watch state remains bounded.
  • AI final counts, failed projects, diagnostics, and error totals now use the same recovered cycle scope.
  • Grouped output remains ordered and isolated, active file spools retain the real operation ID, and lock contention renders the actionable reason exactly once.

Validation: reporter 317, rush-lib 788, apps/rush 71; affected builds and daemon dependency chain; full demo /tmp/rush-reporter-demo-Iiphvo; lock contention /tmp/r5b-r3-lock-IxxlNG; preview JSON /tmp/r5b-r3-preview-mYC7Pr; rush check; stacked change verification. #5997 remains on exact #5996 head 11a7f8462dd40b1d3c0d83ffafc5f0b748a80394, with auto-merge disabled pending fresh review.

@TheLarkInn

Copy link
Copy Markdown
Member Author

Final combined review of 97f807ed65 found no critical, high, or medium actionable issue. All lock, watch concurrency, AI recovery scope, preview, temp, artifact, spool, compatibility, and output findings are fixed.

One dismissible low follow-on remains: the exported regroupOperationOutput helper still groups by operation ID without iteration ID for third-party watch reporters. The daemon completion concern was dismissed because the current multiplexer forwards completion, and the repeated command-name concern remains unreproduced. This head is low-only.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Needs triage

Development

Successfully merging this pull request may close these issues.

1 participant