Add structured progress logging to cr review and review pipeline

[rianjs]

Summary

Add structured progress logging to cr review and the review pipeline so long-running GitHub and LLM operations emit regular stderr breadcrumbs with symmetric start/finish timing and stable structured fields.

This follows the non-review progress logging slice already shipped in #363 and extends the same contract to the review path, which is now the highest-value gap.

Scope

Add review-path progress breadcrumbs for:

• top-level cr review command envelope
• GitHub IO in and around PR metadata, installation/auth, checkout/diff/fetch, threads/comments, and posting
• LLM task lifecycle events, including provider, harness/adapter, model, reasoning effort, and task/session ids
• major review stages such as selection, slicing, per-agent execution, synthesis, and outbox/posting
• failure/retry/resume breadcrumbs, including resumed task/session identifiers where available

Logging contract

• progress writes to stderr only
• result payloads stay on stdout
• --quiet suppresses progress only
• progress lines stay single-line, structured, and easy to substring-parse
• every meaningful IO/LLM/stage operation should be wrapped in symmetric start / finish or error events with duration

Waypoints

1. Review command envelope logging
2. GitHub IO breadcrumbs
3. LLM task/session lifecycle breadcrumbs
4. Review stage breadcrumbs
5. Resume / retry / error breadcrumbs and quiet-mode validation

Required empirical checks

Each waypoint commit must include a real local-binary verification that inspects stderr output from the built CLI before moving to the next waypoint. Unit tests alone are not sufficient.

Expected checks include:

• local cr review ... --dry-run stderr contains the new waypoint breadcrumbs
• stdout payload shape is unchanged
• --quiet suppresses progress while preserving stdout and ordinary error behavior
• resume/failure paths emit the expected stderr markers when reproducible locally

Acceptance criteria

• running cr review in the terminal gives regular feedback during long GitHub and LLM operations
• stderr includes stable structured fields for command/op/target/duration and relevant LLM metadata
• failures and resumes leave visible breadcrumbs instead of silent stalls
• the implementation is covered by focused tests around the shared logging component and the review-path integration points

[rianjs]

Converged implementation plan

Issue 364: Add structured progress logging to cr review and review pipeline

Goal

Extend the structured stderr progress logging contract from the non-review commands into cr review, including GitHub IO, durable LLM task lifecycle, major review stages, and resume/error/retry breadcrumbs.

Constraints

• Progress writes to stderr only.
• JSON and text command payloads remain on stdout only.
• --quiet suppresses progress only.
• Breadcrumbs remain single-line structured records using the existing internal/progress component.
• Changes stay surgical and follow current ownership boundaries.
• Every waypoint commit requires a real local-binary stderr verification before proceeding.
• Error/resume/retry breadcrumbs need deterministic automated coverage in addition to local-binary spot checks.

Current code shape

• internal/cmd/reviewcmd owns command entry, runtime construction, command stderr sink, and stdout rendering.
• internal/pipeline owns dry-run and live review orchestration, durable LLM tasks, and most stage boundaries.
• internal/reviewrun owns live resume/posting flow.
• GitHub IO currently sits behind gitprovider.ReadProvider plus the live-review provider/posting path assembled in review runtime construction.
• LLM task metadata already records adapter/model/effort/session data in docs/llm-task-artifacts.md and internal/pipeline.

Breadcrumb schema contract

Every emitted progress line for this slice must use the existing base schema plus these review-path requirements:

• required base fields: event, command, op, target
• required finish/error fields: duration_ms, status
• optional correlation fields when relevant: phase, task_id, agent, adapter, model, effort, session_id, resume_session_id, run_id, attempt, max_attempts, retry_after_ms
• event lifecycle remains start, finish, or error
• allowed status values for this slice: ok, error
• retry/resume/reuse state should be conveyed with dedicated ops or explicit fields such as attempt, max_attempts, retry_after_ms, resume_session_id, and reuse_source, not by overloading status
• duration_ms is wall-clock elapsed milliseconds for the scoped op
• no prompt contents, model raw output, token values, auth material, or arbitrary provider payloads may appear in stderr

Session and provider identifier policy:

• run_id: allow raw emitted run ids
• task_id: allow raw durable task ids
• session_id / resume_session_id: allow only orchestrator/provider session ids already persisted in ledger/task metadata and already used as resume handles; otherwise emit a stable redacted form such as sid:<short-hash>
• never emit provider tokens, private installation identifiers not already persisted as resume handles, prompt text, output text, or arbitrary provider response bodies
• any dynamic suffix used in target values, such as agent ids, repo paths, or session names, must reject control characters/newlines and use the same quoting/sanitization path as other structured fields
• redact filesystem paths and error strings through the existing progress sanitization path
• add focused tests proving prompt text, raw structured output, and secret-like strings do not reach stderr

Ownership model

To avoid scattered logging, instrumentation ownership is explicit:

• internal/cmd/reviewcmd: command envelope and runtime-assembly breadcrumbs, plus provider decoration setup
• internal/pipeline: stage breadcrumbs, LLM task lifecycle breadcrumbs, resume/retry breadcrumbs, and dry-run provider call hooks used during pipeline execution
• internal/reviewrun: live-flow resume/posting breadcrumbs not naturally owned by pipeline
• provider implementations remain mostly untouched; instead the runtime assembles decorated provider interfaces that emit stable breadcrumbs around provider calls

Validation strategy

Use two complementary proof paths throughout the ticket:

1. Real local-binary stderr captures after every waypoint commit

• build /tmp/cr364 immediately before every capture
• assert the binary changed with a stale-binary guard such as rtk test /tmp/cr364 -nt <touched file> or rebuild unconditionally before each run
• use a known PR target for positive-path captures
• verify stderr contains required breadcrumbs and stdout remains clean

2. Deterministic automated coverage for failure/retry/resume/live-sensitive paths

• command tests for stderr/stdout separation and --quiet
• fake/decorated provider tests for timeout/error/retry breadcrumbs
• pipeline tests for LLM task lifecycle, resume, and isolated/blocking failure breadcrumbs
• reviewrun/live-path tests for retry-posts and posting-related breadcrumbs where local live execution is unsafe or too environment-dependent

Planned approach

Waypoint 1: Review command envelope logging

Add top-level review command breadcrumbs in internal/cmd/reviewcmd for:

• PR URL parse / validation
• config load
• profile resolve
• runtime construction
• dry-run vs live dispatch
• final render / exit path

Implementation notes:

• Reuse internal/progress.Logger from root options stderr/quiet settings.
• Add a narrow review progress helper in internal/cmd/reviewcmd so command-owned ops stay local.
• Thread a review progress interface into runtime construction without pulling Cobra into lower layers.

Deterministic coverage:

• internal/cmd/reviewcmd tests asserting stderr breadcrumbs for dry-run command execution
• --quiet command test proving stdout parity and stderr suppression

Empirical gate before next waypoint:

1. Build /tmp/cr364 with go build -o /tmp/cr364 ./cmd/cr.
2. Run /tmp/cr364 review <known PR> --dry-run --json 1>/tmp/w1.stdout 2>/tmp/w1.stderr.
3. Verify stderr contains top-level review breadcrumbs for config/profile/runtime/dispatch/render.
4. Verify stdout remains valid JSON and contains no progress lines.
5. Run the same command in text mode and verify stdout text is unchanged while stderr carries the breadcrumbs.
6. Run the command with --quiet in both JSON and text modes and verify stderr progress is absent while stdout matches the corresponding non-quiet payload.

Waypoint 2: GitHub IO breadcrumbs

Instrument GitHub IO through decorated provider/runtime wrappers rather than scattered callsites.

Minimum contract for this waypoint:

• runtime identity / installation lookup where applicable
• PR metadata fetch
• diff fetch or compare fetch
• file/tree reads used during review preparation
• inline thread/comment reads
• live posting/update operations reachable through decorated live-flow dependencies

Implementation notes:

• Define the decorator owner in internal/cmd/reviewcmd runtime assembly.
• Use stable op names like fetch_pr, fetch_diff, read_file, list_threads, post_action.
• Define canonical target values up front: pr, profile, runtime, selection, rollup, agent:<id>, file:<repo-path>, threads, posts, and session:<name>; do not log raw URLs or filesystem paths as targets, and sanitize dynamic suffixes before emission.
• Emit error breadcrumbs on retryable failures, timeouts, and provider errors.
• When a retry will occur, emit a dedicated retry op or additional retry fields with status=error or status=ok preserved for outcome so retries are visible rather than collapsed into one final error.
• Avoid per-call spam when concurrency or repeated reads would overwhelm output; aggregate where appropriate.

Deterministic coverage:

• fake provider tests that force timeout/error on GetPR, GetDiff, GetFileAtRef, ListTreeAtRef, and thread/comment reads
• live-path tests for posting wrappers where safe

Empirical gate before next waypoint:

1. Rebuild /tmp/cr364.
2. Run the local dry-run review on the same PR and capture stderr.
3. Verify stderr shows the required GitHub IO breadcrumbs with durations and canonical targets.
4. Run one deterministic automated failure/retry case proving a provider timeout/error emits an error breadcrumb and a visible retry breadcrumb when applicable.
5. Parse every cr progress ... stderr line against the breadcrumb schema rather than relying only on substrings, while allowing unrelated non-progress diagnostics to be ignored or asserted separately.
6. Re-run the local command with --quiet to confirm suppression still works.

Waypoint 3: LLM task/session lifecycle breadcrumbs

Instrument durable LLM task execution in internal/pipeline for:

• task start/end/error
• phase (selection, reviewer agent, rollup)
• adapter/harness name
• provider/model/effort
• provider session id and/or resume session id when allowed by the identifier policy
• resumed task/session breadcrumbs when reusing or resuming a task

Implementation notes:

• Use a focused pipeline progress interface owned by pipeline, implemented from reviewcmd using internal/progress.
• Use existing LLM task metadata and run IDs as the source of truth for task/session fields.
• Keep payload-safe summaries only; never log prompt text or raw outputs.

Deterministic coverage:

• pipeline tests for successful task execution breadcrumbs
• pipeline tests for blocking failure, isolated reviewer failure, resume-from-metadata, and resumed session reuse breadcrumbs
• no-leak assertions for prohibited strings

Empirical gate before next waypoint:

1. Rebuild /tmp/cr364.
2. Run local dry-run review far enough to reach real LLM work.
3. Verify stderr includes structured LLM breadcrumbs with task/stage metadata, adapter, model, effort, allowed session ids, and retry metadata where applicable.
4. Verify stdout payload is unchanged in JSON and text modes.
5. Parse every cr progress ... stderr line against the breadcrumb schema.
6. Back the real capture with deterministic tests for failure/error/retry breadcrumbs.

Waypoint 4: Major review stage breadcrumbs

Instrument coarse-grained pipeline and live-review stages:

• selection phase
• review artifact/slice preparation
• per-agent reviewer execution
• synthesis / rollup
• live outbox/posting / thread-resolution execution

Implementation notes:

• Prefer coarse stage spans over noisy per-internal-step spans.
• Use partial-order assertions in tests where concurrency may reorder sibling reviewer events.
• Ensure stage spans complement rather than duplicate LLM-task spans.

Deterministic coverage:

• pipeline tests for required stage events and partial-order expectations
• reviewrun/live-path tests for posting/outbox breadcrumbs

Empirical gate before next waypoint:

1. Rebuild /tmp/cr364.
2. Run local dry-run review and verify the stage breadcrumbs appear in required partial order.
3. If safe locally, run a retry-posts or limited live path and inspect posting/outbox breadcrumbs.
4. Parse every cr progress ... stderr line against the breadcrumb schema and required partial-order constraints.
5. Verify --quiet still suppresses only progress.

Waypoint 5: Resume / retry / error breadcrumbs and final validation

Add explicit breadcrumbs for:

• reuse of existing run state
• resumed named/provider sessions
• retry-posts path
• blocking vs isolated LLM failures where surfaced to the caller
• final quiet-mode and no-leak validation coverage

Implementation notes:

• Use existing resume and task metadata logic in internal/pipeline and internal/reviewrun.
• Provide stable resumed identifiers under the identifier policy.
• Keep live/resume instrumentation in the layer that already owns the behavior.

Deterministic coverage:

• resume-path tests in pipeline and reviewrun
• retry-posts tests showing stderr breadcrumbs for recovered posting work
• no-leak validation for resumed identifiers and sanitized errors

Empirical gate before final PR:

1. Rebuild /tmp/cr364.
2. Reproduce at least one local resume or retry-posts path and capture stderr.
3. Verify resumed task/session identifiers, retry metadata, and failure breadcrumbs are present.
4. Parse every cr progress ... stderr line against the breadcrumb schema.
5. Run a final --quiet comparison in JSON and text modes proving suppression without stdout drift.
6. Run repo tests/lint/build and then continue with PR/TDD/daemon/CI/merge workflow.

Local-binary capture target strategy

Positive-path local captures should use one stable known PR target for consistency.
For unsafe or environment-sensitive flows:

• use deterministic tests as the primary proof for provider/LLM failures and retry behavior
• use local-binary captures only where the environment can safely and repeatably exercise the path
• if live posting is not safe against the chosen PR target, use retry-posts or an already-known safe path rather than leaving the behavior unverified

Test strategy summary

• Unit tests for any new progress metadata rendering helpers
• internal/cmd/reviewcmd command tests for stderr/stdout separation and --quiet in JSON and text modes
• fake/decorated provider tests for GitHub IO breadcrumbs and error handling
• focused internal/pipeline tests for LLM lifecycle, stage, and resume breadcrumbs
• internal/reviewrun tests for retry-posts/live posting breadcrumbs
• no-leak assertions for prompt contents, raw output, and secret-like strings

Risks to manage

• review path spans multiple layers; the decorator/interface split must prevent logger coupling creep
• progress volume can get noisy if stage/task events are too fine-grained
• local live validation may remain environment-sensitive, so deterministic tests must carry the risky paths
• concurrency can reorder sibling operations, so tests must assert required events and partial order rather than brittle full-order logs

Success criteria

• cr review emits regular structured stderr feedback during real GitHub and LLM work
• stderr shows enough metadata to prove the process is alive and what stage it is in
• stdout result payloads remain unchanged in JSON and text modes
• --quiet suppresses progress only
• each waypoint is proven by a real local-binary stderr capture before the next commit
• deterministic tests prove error, timeout, retry, resume, and no-leak behavior that local captures cannot safely cover every time