Skip to content

Add Rush reporter frontend controls - #5989

Open
Sean Larkin (TheLarkInn) wants to merge 6 commits into
copilot/reporter-r2a-experiment-configfrom
copilot/reporter-r2b-frontend-host-controls
Open

Add Rush reporter frontend controls#5989
Sean Larkin (TheLarkInn) wants to merge 6 commits into
copilot/reporter-r2a-experiment-configfrom
copilot/reporter-r2b-frontend-host-controls

Conversation

@TheLarkInn

@TheLarkInn Sean Larkin (TheLarkInn) commented Aug 28, 2026

Copy link
Copy Markdown
Member

Summary

  • create the authoritative ReporterHost/ReporterManager in apps/rush before Rush version selection and pass selected rush-lib only an IReporterEventSink
  • register and parse global --reporter, --output, --log-level, --quiet, --verbose, and --debug controls while preserving command-specific --json
  • keep the pre-major default on the legacy path; explicit non-legacy --reporter or the repository useRushReporter experiment enables reporters, agent auto-selection remains disabled, and RUSH_REPORTER=legacy strips reporter-only controls before legacy-engine handoff
  • retain existing -v meanings for rush list, rush change, and phased build commands

Stack

This is the R2B child of #5987. Keep auto-merge disabled while any ancestor is open. Merge and retarget #5985, #5986, and #5987 in order; only after #5987 merges should this PR be retargeted to main and considered for auto-merge.

Validation

  • rush build --to @microsoft/rush --verbose
  • rush test --only @microsoft/rush --only @microsoft/rush-lib --only @rushstack/rush-reporter --verbose
  • node libraries/reporter/scripts/generateBootstrapProtocol.js --check
  • rush check
  • rush change --verify --no-fetch
  • gh pr diff 5989 contains only the 18-file R2B frontend/control slice

Legacy safety

  • frontend-owned --reporter, --output, and --log-level controls are removed before every Rush engine handoff, including repository-pinned older engines
  • no opt-in registers a visible reporter, including TTY, non-TTY, CI, and agent environments
  • RUSH_REPORTER=legacy remains the emergency override even when non-legacy or contradictory reporter controls are present
  • legacy combinations such as --quiet --debug remain valid when the reporter path is disabled
  • rush-pnpm --reporter and rushx arguments remain outside Rush reporter parsing

Repository opt-in

  • useRushReporter is read before version selection
  • TTY selects the interactive reporter; CI and generic non-TTY select plaintext
  • agent detection is deliberately excluded from repository opt-in selection until R8

Non-goals

  • no bootstrap handoff replay or prelude work (R6)
  • no operation stream rendering or StreamCollator replacement (R5)

Part of #5975

@TheLarkInn

Copy link
Copy Markdown
Member Author

Stack integration review found and fixed three actionable issues in 42b0fae: frontend-only value flags no longer leak to repository-pinned engines; legacy verbosity combinations remain compatible, including under RUSH_REPORTER=legacy; and the repository useRushReporter opt-in is consumed before version selection with deterministic TTY/CI/non-TTY behavior while agent auto-selection stays disabled. Ordering gate: after #5985 merges and both PRs move to main, #5987 must merge before #5989 because #5987 owns the schema/API for the experiment that #5989 consumes.

@TheLarkInn
Sean Larkin (TheLarkInn) changed the base branch from copilot/reporter-r1a-package-wiring to copilot/reporter-r2a-experiment-config August 28, 2026 04:33
@TheLarkInn
Sean Larkin (TheLarkInn) force-pushed the copilot/reporter-r2b-frontend-host-controls branch from 42b0fae to c54a29b Compare August 28, 2026 04:33
@TheLarkInn

Copy link
Copy Markdown
Member Author

Linearized stack update: rebased the two R2B-only commits onto fresh origin/copilot/reporter-r2a-experiment-config and force-pushed with lease. New head is c54a29b; base is now copilot/reporter-r2a-experiment-config. The GitHub diff exactly matches the local R2A-to-R2B file list, with no R2A schema/API files included. Combined targeted tests, rush check, and changefile verification pass. Auto-merge remains disabled while #5985 and #5987 are open.

Create the authoritative frontend reporter host before version selection, register global reporter controls, and preserve legacy output unless a non-legacy reporter is explicitly selected.

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

Copilot-Session: d6318e80-5da9-4858-a147-817e8692f10e
Consume the repository experiment before Rush version selection, keep agent detection out of pre-major defaults, strip frontend-only controls before engine handoff, and preserve legacy verbosity compatibility.

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-r2b-frontend-host-controls branch from c54a29b to 732e704 Compare August 28, 2026 04:37
@TheLarkInn

Copy link
Copy Markdown
Member Author

Linear stack update: R2B is now head 732e704 based on #5987. Its GitHub diff is exactly the 18-file frontend/control slice. Final-tip build/tests, generation check, rush check, and change verification pass; auto-merge remains disabled while ancestors are open.

@TheLarkInn

Copy link
Copy Markdown
Member Author

Combined deep review of current head 732e7048f7.

  1. HIGH RushReporterHost.ts:184-244 parses and strips reporter-shaped arguments after the -- separator. Pass-through tool arguments can be consumed or rejected. Stop both scans at --.
  2. MEDIUM RushFrontend.ts:37-66 initializes frontend-owned reporters without an exactly-once success close path. Files, descriptors, and latest.log can remain incomplete until process exit. Add an awaitable close contract for success, failure, parser rejection, and termination.

Stop reporter control scans at the pass-through separator and add an exactly-once frontend close contract across success, failure, and termination paths.

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

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

Copy link
Copy Markdown
Member Author

Fixed both deep-review findings in 709357f. Reporter control scans and stripping now stop at the first standalone --, preserving all pass-through arguments. Frontend-owned reporters now use one awaitable close promise across normal success, engine and parser failures, selector failures, explicit exits, older-engine beforeExit, and bounded signal termination. Tests passed: reporter 295, rush-lib 752, apps/rush 27, rush check, and stacked-base change verification.

@TheLarkInn

Copy link
Copy Markdown
Member Author

Four-review rerun on 709357fced confirms the original separator and normal close fixes. Five actionable findings remain:

  1. CRITICAL Global --verbose collides with existing action flags. rush build --verbose, rush rebuild --verbose, and rush check --verbose can fail as ambiguous. Preserve existing action syntax.
  2. HIGH Global reporter parameter names can collide with repository custom command parameters. Detect conflicts early or avoid parser-level collisions.
  3. HIGH An older selected engine can still initialize reporters, strip an explicit request, and never close on process.exit(). Reject unsupported explicit requests and keep old engines on a closed legacy path.
  4. MEDIUM A signal can join an existing longer close and miss the advertised two-second wall-clock deadline.
  5. MEDIUM Signal and beforeExit handlers are installed even when reporting is disabled, changing the no-opt-in legacy path.

Useful low follow-ons: close errors should not replace the command result, and current -- preservation is frontend-only because Rush actions do not yet consume a remainder. Descendant remediation stays paused.

Keep reporter controls out of ts-command-line globals, gate incompatible engines before initialization, and enforce bounded signal and close-error behavior.

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 fixed in 5f8ef5e. Reporter options are no longer ts-command-line globals, so existing build, rebuild, check, and repository custom parameters retain their action-owned behavior. Explicit non-legacy reporter requests reserve reporter value controls; implicit opt-in preserves custom output and log-level parameters. Incompatible engines now reject explicit reporters before initialization and otherwise stay on disabled legacy behavior. Lifecycle handlers are installed only when enabled, signals use one two-second wall-clock deadline, and close errors set failure state without replacing command errors. Tests passed: reporter 295, rush-lib 757, apps/rush 38, regenerated snapshots, rush check, and stacked-base change verification.

@TheLarkInn

Copy link
Copy Markdown
Member Author

Four-review rerun on 5f8ef5e948 confirms all prior compatibility and lifecycle fixes. Two related flag-ownership gaps remain:

  1. HIGH An unsupported --reporter value is still claimed by the frontend, breaking custom commands that historically own --reporter, including common test reporter values. Preserve it unless the frontend has an explicit supported reporter request or repository opt-in ownership.
  2. MEDIUM RUSH_REPORTER=legacy plus explicit --reporter=legacy can strip valid custom --output and --log-level parameters. Strip only the reporter selection flag in that case.

Auto-merge remains disabled. Descendant remediation stays paused.

Preserve unsupported custom reporter values until frontend ownership is unambiguous, and narrow emergency legacy stripping to the reporter selection flag.

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

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

Copy link
Copy Markdown
Member Author

Final focused ownership fix is in c732e19. Unsupported custom --reporter values such as junit now pass through unchanged until repository opt-in, a supported reporter name, or a reporter-shaped output establishes frontend ownership. RUSH_REPORTER=legacy with explicit --reporter=legacy strips only the selection flag, preserving custom output, log-level, and verbose parameters. Incompatible engines retain the explicit failure and legacy-safe escape paths with clearer contract wording. Tests passed: reporter 295, rush-lib 757, apps/rush 44, parser snapshots, rush check, and stacked-base change verification.

@TheLarkInn

Copy link
Copy Markdown
Member Author

Four-review gate on c732e195b5 confirms the prior findings are fixed. One MEDIUM compatibility gap remains: a repository custom command that owns a value-less --reporter flag still fails during the frontend ownership probe before Rush loads the command definition.

Required fix: make the probe tolerant of missing values and raise requires a value only after frontend ownership is established. Add a custom flag fixture.

One dismissible low follow-on remains: repository opt-in currently rejects contradictory legacy verbosity aliases that the legacy path accepts.

Probe reporter ownership without requiring a value, then enforce strict reporter parsing only after frontend ownership is established.

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

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

Copy link
Copy Markdown
Member Author

Final narrow fix is in 62d9e7a. The first reporter ownership probe now tolerates a value-less custom --reporter flag and only performs the strict missing-value check after opt-in, a supported reporter value, or reporter-shaped output establishes frontend ownership. A real custom flag fixture verifies end-to-end pass-through. Implicit repository opt-in now applies debug, verbose, then quiet alias precedence, while explicit log-level contradictions still fail. Tests passed: reporter 295, rush-lib 758, apps/rush 46, parser snapshots, rush check, and stacked-base change verification.

@TheLarkInn

Copy link
Copy Markdown
Member Author

Final combined review of 62d9e7af04 found no critical, high, medium, or useful low issues. All recorded findings have fixes and regression coverage.

The repeated command-name concern was dismissed because no failing invocation was reproduced and the full frontend path verification found no affected supported entry point. This head is review-clean. Auto-merge remains disabled while ancestors are open.

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