Skip to content

feat(review): replay a PR without its existing discussion - #627

Merged
zzwong merged 4 commits into
mainfrom
zzwong/issue-626/replay-without-discussion
Oct 17, 2026
Merged

zzwong merged 4 commits into
mainfrom
zzwong/issue-626/replay-without-discussion

Conversation

@zzwong

@zzwong zzwong commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Current draft refresh — 2026-10-15

Refreshed this existing draft against main 9c63b3e493f170a9eb42d0865537821a6fb30e00 in commit e05a66cd4e8703f875b423740b1f312a10475edc. The exact published tree is fac7212863a7a150330453acab36fdc00b22e6fc.

The original draft head 2a2dc5e9fcb1439f996d9474496adaf47085eb33 is the first parent and pinned main is the second parent. The branch was updated without force-pushing; existing draft commits remain in its ancestry. Merge preparation authorized on 2026-10-17; review and current merge checks are being completed before submission.

Source changes and review

  • Retain both pipeline.Result.ThreadAnalysisFailures from current main and WithoutDiscussion from this PR.
  • Preserve main's isolated-thread-failure result, rollup, plan-summary and view/JSON behavior; schema-based JSON candidate disambiguation; and Claude cleanup ordering.
  • Preserve the PR's discussion-free session/cohort guards, replay identity, offline primary/repair/retry requests, symlink handling and relocation checks.
  • Add source regression coverage for ordinary review continuing with an isolated thread failure, replay producing no thread failures, and JSON retaining the discussion mode and thread-failure fields.
  • Independent source review accepted this exact tree. All 405 untouched main leaves/modes remain unchanged; the main-to-candidate patch changes 18 paths.

Validation for this exact refresh

  • PASS: pinned source Git-blob identities and reconstructed main/candidate tree identities.
  • PASS: nonwriting patch applicability check against pinned main.
  • PASS: Go 1.26.8 gofmt on all 17 changed Go files, with zero byte changes.
  • PASS: remote commit parents/tree and all 18 changed file blob identities/modes match the reviewed candidate.
  • NOT RUN locally: compilation, Go runtime tests, focused race checks, lint, repository aggregate, opt-out/static smoke and installed-Pi runtime checks.
  • PASS: hosted CI run 38745494215 completed successfully for this exact head; all 12 jobs passed, verified at 2026-10-15 13:51 UTC. The synthetic PR merge tree also equals the reviewed/published tree.
  • The three new/extended regression groups passed in regular, no1password opt-out and static-smoke jobs. The installed-Pi gate passed 8 top-level tests / 24 PASS nodes with zero skips. Standard/static profiles each retained 22 skipped test/subtest nodes; opt-out retained 35. These skipped scopes and the historical local failures below are not erased by hosted success.
  • Current CI expands to 12 PR jobs. Its race job covers ledger/dbmig, not focused pipeline/view race coverage; that additional race validation remains unrun.

The earlier head's CI and local results below are historical. They do not validate this new tree, erase earlier failures, or establish merge readiness.

Historical record for previous head 2a2dc5e

Historical: Summary

Adds cr review --without-discussion for a first-pass replay of a pinned PR. It requires --dry-run or --no-post and both review SHAs.

  • Skip PR discussion reads and leave saved orchestrator sessions and reviewer cohorts untouched.
  • Match the discussion setting when resuming incomplete runs and include it in durable task fingerprints, the run marker, dossier and JSON output.
  • Strip reviewer-workspace remotes and retain the existing Claude/Codex offline tool restrictions through primary review, coverage repair and validation retry. These controls narrow the tool surface; they are not an operating-system sandbox guarantee.
  • Preserve main's pinned symlink metadata, schema-3 task protection, relocation coverage and approval rules. A symlink-to-regular transition still requires reading the ordinary pinned-head body.
  • Add combined offline relocation, repair, symlink and retry regressions. PR title, body, diff and file inputs remain available.

Historical: Source and execution mapping

Refreshed against main 999e59ddfc74a67ffeef3c4b4a78a8528365afe6 (#654). The published commit 2a2dc5e9fcb1439f996d9474496adaf47085eb33 has exactly the tested #627 source tree 45fe9fbd546e0de817621e9500a7c9d2588c9cc7, from local candidate fdf72520f13f6efa694b7ba4137602ff11444256.

The existing PR head 3a622e65a3b87db28e21cff7be0aeb81ccfe5ff3 remains its first parent and current main is its second parent. Publication preserves the existing branch ancestry without force-pushing; intermediate local integration commits were not uploaded.

The #627 focused normal/race checks ran at fdf72520. The full shared-code gates ran once on the dependent #628 stack, whose #627 source, tests and module files are byte-identical. Its original de9f2e90 snapshot passed benchmark normal/race checks and a separate build, but its first literal make check stopped at a G304 warning in a benchmark test helper. The independently reviewed e3affda0 successor changes only two benchmark test files to use fixture-rooted reads; no assertions or production code changed. Final aggregate, lint, build, vet, module and CI-mode checks used that successor. This PR does not contain the benchmark delta.

Historical: Local validation

  • feat(review): replay a PR without its existing discussion #627 focused normal: 40 top-level tests / 127 PASS nodes. Three-repeat focused race: 120 top-level executions / 381 PASS nodes. No skips or race reports in matching tests.
  • Explicit coverage includes offline primary/repair/retry, pinned symlink identity, head-body obligations, schema 1/2 rejection under schema 3, preserved PR sessions and Claude/Codex offline arguments.
  • Final shared-stack tidy, formatting, repository-pinned lint (zero issues), separate go build -v ./..., go vet ./... and go mod verify passed. Module files stayed unchanged.
  • Aggregate failures retained: final literal make check failed at test (exit 2), full no1password opt-out tests failed (exit 1), and literal CGO-disabled make test-static-smoke failed (exit 2). Aggregate build was not reached; separate build passed.

All three modes failed only these existing pipeline groups:

  1. TestReviewPipelineAcceptanceHarnessPiRPCPermissionBoundedDryRunAllowsDossierDomainVocabulary
  2. TestDryRunSelectionOverridesApplyOnlyToSelection, including model_and_effort, model_only and effort_only
  3. TestLivePlansPendingActionsWithoutCompletingRun

Each complete failure-case set reproduced on pristine exact main under its matching mode. The diagnostic is the existing /tmp ancestor-worktree source-trust warning. The readonly marker and security checks were left unchanged. No Claude timing failure occurred in these attempts.

Historical: Limits and remaining gates

Linux amd64, Go 1.26.8, golangci-lint 2.12.2, serial packages, fresh credential-free state and warmed offline caches. The focused name filter matched no tests in symlinkmetadata (normal/race) or runartifact (normal); both packages' full tests ran in the aggregate modes.

Standard/static runs retained five skips: the no1password-only UI case, Node-dependent Pi helper process-group case, installed-Pi extension case, opt-in real Claude background job, and Windows-only cross-volume case. Opt-out mode retained 18 skips: 14 1Password-enabled UI cases plus the same Node/Pi/real-Claude/Windows cases. Node/Pi were absent from the minimal validation PATH. No dependency installs, real provider/credential calls or paid benchmarks were used. Full repository-wide race, real authentication/OAuth and non-Linux local validation were not run.

Independent source and evidence review accepted this qualified draft workflow. Local aggregate success and merge readiness are not claimed. Merge order is #627 first, then #663.

Historical: Hosted CI

CI run 37735071753 completed successfully for exact head 2a2dc5e9fcb1439f996d9474496adaf47085eb33: all 10 jobs passed, including test, no1password opt-out, static-smoke, lint, identity/title checks and Linux/macOS/Windows builds. The synthetic merge tree also matches the tested tree. These hosted results do not erase the failed local aggregates or expand the skipped/not-run scope above.

Closes #626

Add `cr review --without-discussion` for first-pass replays of pinned
dry-run reviews. It requires --dry-run (or --no-post) and both
--review-base-sha and --review-head-sha.

Pinned reviews already skipped the live thread, review, and issue-comment
reads, but a replay still resumed the PR's default orchestrator session
and reviewer cohort sessions, which carry the earlier discussion, and it
overwrote that cohort. With the flag, the run reads no discussion,
resumes and updates no PR-scoped session or cohort, and so selection,
reviewer, and rollup prompts see only the PR title, description, and diff.

The run marker, dossier discussion artifacts, and JSON run output record
without_discussion, and an incomplete run resumes only into a run with the
same setting. Reviewer task fingerprints include the setting as well.
A --without-discussion replay kept its discussion out of every prompt, but
a reviewer with a shell could still fetch it with gh, curl, or the GitHub
API. Such replays now remove the git remotes from the workbench and each
reviewer checkout, deny claude_cli reviewers WebFetch, WebSearch, and Bash
commands that reach the network (gh, curl, wget, git fetch/pull/ls-remote
and similar), and run codex_cli reviewers with web search and sandbox
network access off. The adapters' argument validation requires these
restrictions exactly when the workspace is marked offline.

This narrows the tool surface rather than sandboxing the network, and the
README says so. --fresh-session is now rejected with --without-discussion,
since a replay never reuses or resets PR sessions.
Preserve isolated thread failure handling, schema disambiguation and cleanup ordering while retaining discussion-free replay. Add regression coverage for replay/thread-failure interactions.

Independent source review, patch applicability and gofmt passed. Compilation, runtime tests, race checks, lint and hosted CI for this exact tree have not run at publication. Keep this PR draft; do not merge into main.
zzwong added a commit that referenced this pull request Oct 15, 2026
Retain the existing 11-file benchmark delta byte-for-byte over verified #627. Preserve all parent protections and regressions. Source review, tree identities, patch applicability and gofmt passed.

Compilation, runtime tests, race, lint and hosted CI for this combined tree have not run at publication. Preserve earlier failures and skipped scope. Keep this PR draft; do not merge into main.

@monit-reviewer monit-reviewer left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Automated PR Review

Reviewed commit: e05a66cd4e87
Profile: pi-codex-reviewer-gh - Posting as: monit-reviewer

Summary

Reviewer Findings
go:implementation-tests 0
policies:conventions 0
structure:repo-health 0
security:code-auditor 0
architecture:solid-reviewer-agnostic 0

Reviewer Coverage

Complete coverage details are retained in the local coverage.json artifact.

  • go:implementation-tests — complete (broad); scope: 17 files; examples: internal/cmd/reviewcmd/reviewcmd.go, internal/cmd/reviewcmd/reviewcmd_test.go, internal/dossier/dossier.go, internal/llm/adapter.go, internal/llmadapters/subprocess.go; 12 omitted; body-inspected: 17 files; examples: internal/cmd/reviewcmd/reviewcmd.go, internal/cmd/reviewcmd/reviewcmd_test.go, internal/dossier/dossier.go, internal/llm/adapter.go, internal/llmadapters/subprocess.go; 12 omitted; skipped: none; context (not coverage): 1 file; examples: docs/development.md; constraints: Static review only; compilation, runtime tests, and race checks were not run.
  • policies:conventions — complete (broad); scope: 8 files; examples: README.md, internal/cmd/reviewcmd/reviewcmd.go, internal/cmd/reviewcmd/reviewcmd_test.go, internal/pipeline/pipeline.go, internal/pipeline/without_discussion_test.go; 3 omitted; body-inspected: 8 files; examples: README.md, internal/cmd/reviewcmd/reviewcmd.go, internal/cmd/reviewcmd/reviewcmd_test.go, internal/pipeline/pipeline.go, internal/pipeline/without_discussion_test.go; 3 omitted; skipped: none; context (not coverage): 4 files; examples: AGENTS.md, CONTRIBUTING.md, docs/development.md, docs/review-lifecycle.md; constraints: Shared standards and external harness principles were not available in the provided repository context; no requirements were inferred from their links. Static review only; tests were inspected, not executed.
  • structure:repo-health — complete (broad); scope: 9 files; examples: internal/cmd/reviewcmd/reviewcmd.go, internal/dossier/dossier.go, internal/llm/adapter.go, internal/llmadapters/subprocess.go, internal/pipeline/pipeline.go; 4 omitted; body-inspected: 9 files; examples: internal/cmd/reviewcmd/reviewcmd.go, internal/dossier/dossier.go, internal/llm/adapter.go, internal/llmadapters/subprocess.go, internal/pipeline/pipeline.go; 4 omitted; skipped: none; constraints: Source-only review using pinned repository reads; compilation and runtime tests were not executed.
  • security:code-auditor — complete (broad); scope: 10 files; examples: internal/llm/adapter.go, internal/llmadapters/subprocess.go, internal/llmadapters/subprocess_test.go, internal/pipeline/pipeline.go, internal/pipeline/without_discussion_relocation_test.go; 5 omitted; body-inspected: 10 files; examples: internal/llm/adapter.go, internal/llmadapters/subprocess.go, internal/llmadapters/subprocess_test.go, internal/pipeline/pipeline.go, internal/pipeline/without_discussion_relocation_test.go; 5 omitted; skipped: none; constraints: Static inspection only; no tests or runtime security checks were executed. Offline controls were assessed as tool-surface restrictions, not an operating-system sandbox.
  • architecture:solid-reviewer-agnostic — complete (broad); scope: 8 files; examples: internal/cmd/reviewcmd/reviewcmd.go, internal/dossier/dossier.go, internal/llm/adapter.go, internal/llmadapters/subprocess.go, internal/pipeline/pipeline.go; 3 omitted; body-inspected: 8 files; examples: internal/cmd/reviewcmd/reviewcmd.go, internal/dossier/dossier.go, internal/llm/adapter.go, internal/llmadapters/subprocess.go, internal/pipeline/pipeline.go; 3 omitted; skipped: none; context (not coverage): 8 files; examples: .codereview/agents/policies/conventions/prompt.md, AGENTS.md, CLAUDE.md, docs/architecture.md, docs/development.md; 3 omitted; constraints: External harness PRINCIPLES.md was outside the permitted repository inspection surface. Read-only source review; build, tests, and lint were not run. Hosted CI results described in the PR were not independently verified.

0 PR discussion threads considered. 0 summarized; 0 resolved.


Completed in 1m 08s | $0.15 | openai-codex/gpt-6.1-sol | cr 0.10.328
Field Value
Model openai-codex/gpt-6.1-sol
Reviewers go:implementation-tests, policies:conventions, structure:repo-health, security:code-auditor, architecture:solid-reviewer-agnostic
Engine pi_rpc · openai-codex/gpt-6.1-sol
Reviewed by cr · monit-reviewer
Duration 1m 08s wall · 2m 04s compute
Cost $0.15
Tokens 49.1k in / 2.1k out

Per-workstream usage

  • orchestrator-selection — openai-codex/gpt-6.1-sol
    • In: 9.4k
    • Out: 804
    • Cache read: 0
    • Cache create: 0
    • Cost: $0.03
    • Duration: 13s
  • go:implementation-tests — openai-codex/gpt-6.1-sol
    • In: 8.9k
    • Out: 272
    • Cache read: 46.2k
    • Cache create: 0
    • Cost: $0.03
    • Duration: 18s
  • policies:conventions — openai-codex/gpt-6.1-sol
    • In: 5.4k
    • Out: 163
    • Cache read: 38.3k
    • Cache create: 0
    • Cost: $0.02
    • Duration: 16s
  • structure:repo-health — openai-codex/gpt-6.1-sol
    • In: 16.0k
    • Out: 202
    • Cache read: 37.1k
    • Cache create: 0
    • Cost: $0.04
    • Duration: 17s
  • security:code-auditor — openai-codex/gpt-6.1-sol
    • In: 3.8k
    • Out: 163
    • Cache read: 36.2k
    • Cache create: 0
    • Cost: $0.01
    • Duration: 14s
  • architecture:solid-reviewer-agnostic — openai-codex/gpt-6.1-sol
    • In: 1.6k
    • Out: 440
    • Cache read: 102.0k
    • Cache create: 0
    • Cost: $0.02
    • Duration: 41s
  • orchestrator-rollup — openai-codex/gpt-6.1-sol
    • In: 3.9k
    • Out: 47
    • Cache read: 0
    • Cache create: 0
    • Cost: $0.01
    • Duration: 2s

@zzwong
zzwong marked this pull request as ready for review October 17, 2026 05:02
@zzwong
zzwong merged commit 5fd7b31 into main Oct 17, 2026
12 checks passed
@zzwong
zzwong deleted the zzwong/issue-626/replay-without-discussion branch October 17, 2026 05:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Replay a PR review without its existing discussion

2 participants