Skip to content

feat(benchmark): isolate pinned reviews from saved sessions - #663

Merged
zzwong merged 9 commits into
mainfrom
zzwong/issue-628/pinned-benchmark-discussion-isolation
Oct 17, 2026
Merged

zzwong merged 9 commits into
mainfrom
zzwong/issue-628/pinned-benchmark-discussion-isolation

Conversation

@zzwong

@zzwong zzwong commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Review correction — 2026-10-17

Commit 758a22b binds isolation verification to the original review stdout using a SHA-256 digest persisted in per-run metrics, summaries and the manifest. Comparison rejects changed artifacts or missing original digests for isolated runs and excludes their replacement findings from anchor scoring. Non-isolated historical results retain their existing behavior.

The new valid-isolated-replacement regression failed on the previous implementation as expected. After the fix, go test -tags=keyring_nopassage ./internal/benchmark ./internal/cmd/benchmarkcmd -count=1 and the same two-package command with -race passed on macOS arm64 using Go 1.26.8. Repository make lint passed with zero issues; git diff --check passed. Review approved production head 758a22b99069478d4987116ccc72560175264d4e. Final head f69522c8e76c805b159624495945f951515dcb68 adds only four test lines so the cannot-upgrade regression reaches the saved-verification guard with a valid original digest. The focused test passed, removing the guard failed at the intended assertion, both benchmark packages passed race tests, and package lint reported zero issues. Both review threads were accepted and resolved. GitHub dismissed the earlier approval on push, so a fresh review approved exact final head f69522c8e76c805b159624495945f951515dcb68 with no findings. Hosted CI run 39005258468 passed all 12 jobs on exact final head f69522c8e76c805b159624495945f951515dcb68, including normal, opt-out and static-smoke tests, the repeated ledger/dbmig race check, installed Pi runtime, lint, all platform builds and metadata checks. The final review approved this exact head with no findings; all finding threads are resolved.

Submission update — 2026-10-17

Parent #627 merged into main as 5fd7b31ea67d609cdd15c0b21810eeedb194c83a. This PR now targets main. A forward merge of main produced head 52124fdf726a4c2e63e8c361f3f0dec8064d2702 without rewriting branch history.

The complete source tree remains exactly 310dc9cc37b2b23c4fe0512368f3c3e5fbc475d4, identical to the previously verified head. The diff against main contains only the intended 11 benchmark files. This integration-only head is historical; the reviewed corrective head and its checks are recorded below.

Current draft refresh — 2026-10-15

Refreshed this existing draft onto independently reviewed and hosted-CI-verified #627 head e05a66cd4e8703f875b423740b1f312a10475edc in commit 3f81c3b3570ad8fa5333c41cbab9c643a0333f0d. The exact combined tree is 310dc9cc37b2b23c4fe0512368f3c3e5fbc475d4.

The original #663 head b83a5c471143df15a9f4f883ff644d236e254718 remains the first parent and refreshed #627 is the second parent. Publication preserved existing ancestry without force-pushing. This draft was originally based on #627; the submission update below records its integration after the parent merged.

Source preservation and review

  • Retain all 11 benchmark files byte-for-byte from the original draft: pinned isolation defaults and opt-out, both executor paths, compatibility failure behavior, provenance and comparison-time artifact verification.
  • The 33 incoming parent-change paths and 11 benchmark paths are disjoint. All 415 other parent leaves/modes remain unchanged; the complete combined tree has 426 leaves.
  • Preserve every file from feat(review): replay a PR without its existing discussion #627's verified refresh, including both result fields, session/cohort protections, thread-failure handling, JSON disambiguation, cleanup ordering and regression tests.
  • Independent review accepted the exact combined tree and the existing regression plan. GitHub's regenerated synthetic merge tree independently matched the proposed tree before the draft branch was refreshed.

Validation for this exact combined tree

  • PASS: all 19 pinned input blob identities, complete parent/original-head tree reconstruction, unchanged parent leaves/modes and exact original benchmark bytes.
  • PASS: nonwriting patch applicability against pinned feat(review): replay a PR without its existing discussion #627; Go 1.26.8 gofmt -l on ten Go files reported no unformatted files.
  • PASS: remote combined tree's 426 leaves and created commit's exact ordered parents/tree match the reviewed candidate.
  • NOT RUN locally: compilation, runtime tests, focused benchmark race, lint, aggregate, opt-out/static smoke or installed-Pi runtime.
  • PASS: hosted CI run 38747869478 completed successfully for this exact head; all 12 jobs passed, verified at 2026-10-15 14:07 UTC. The synthetic PR merge tree matches the reviewed/published combined tree.
  • Regular, no1password opt-out and static-smoke jobs each passed all 104 benchmark top-level tests / 198 PASS nodes, with zero benchmark skips/failures. All 12 benchmark isolation regression groups and the three inherited feat(review): replay a PR without its existing discussion #627 regression groups passed in each mode.
  • Installed-Pi passed eight top-level tests / 24 PASS nodes with zero skips. Standard/static full profiles each retained 22 skipped test/subtest nodes; opt-out retained 35. Historical local failures remain recorded below.
  • Current CI expands to 12 jobs. Its race job covers ledger/dbmig; focused benchmark and pipeline/view race remain separately unrun for this combined tree.

The previous draft's results below are historical. Parent #627's green CI does not validate the benchmark-combined tree. Historical failures and skipped scopes are retained and do not establish current merge readiness.

Historical record for previous head b83a5c4

Historical: Summary

Closes #628. Stacked on #627 at 2a2dc5e9fcb1439f996d9474496adaf47085eb33; merge #627 first and refresh this stack if its parent changes.

  • Default pinned benchmark cases to discussion-free reviews, with a per-case without_discussion boolean and explicit false opt-out.
  • Apply the same policy in subprocess and in-process executors, using feat(review): replay a PR without its existing discussion #627 to avoid saved reviewer/orchestrator session contamination.
  • Preserve effective, requested and response-verified isolation in doctor output, manifests, metrics, summaries and comparisons.
  • Keep older --cr-bin compatibility explicit: do not drop the flag or silently retry with saved sessions. Missing verification fails the cell while preserving raw output and the actual child exit code.
  • Recheck isolation against the current review JSON during comparison. Missing, empty, malformed or replaced artifacts cannot retain stale verification; a replaced artifact cannot upgrade an originally unverified execution.
  • Add suite, executor, comparison and replay/relocation-boundary regressions and document defaults and opt-out behavior.

This PR contains only the 11-file benchmark delta relative to #627. The prerequisite owns discussion-free review, offline workspaces, symlink/schema protection and durable session isolation.

Historical: Exact source and execution mapping

Published commit b83a5c471143df15a9f4f883ff644d236e254718 has sole parent 2a2dc5e9fcb1439f996d9474496adaf47085eb33 and exactly the tested tree 89a83913f415bf830877e3f6f9ad1acfce406a25, from local candidate e3affda02f88a2280d20a9aa14361806be12c79c. The published parent has the same tree as locally tested #627 candidate fdf72520f13f6efa694b7ba4137602ff11444256. Publication reconstructs those exact source trees with remote parentage; intermediate local commits were not uploaded.

The original integrated snapshot de9f2e90 passed complete benchmark normal/race checks and a separate build. Its first literal make check stopped at G304 in a benchmark test helper; no aggregate tests/build ran on that attempt. Independently reviewed successor e3affda0 changes only two test files to confine reads to the caller's actual fixture root with os.OpenRoot; assertions and production code are unchanged. Final benchmark race, full normal aggregate, vet, separate build, module verification, opt-out and static-smoke used e3affda0.

All shared #627 source, tests and module dependencies are byte-identical between parent and stack. The parent focused normal/race runs used fdf72520; the full shared-code gates ran once on the final stack, not as a duplicate full parent run.

Historical: Local validation

  • Original benchmark normal: 104 top-level tests / 198 PASS nodes; three-repeat race: 312 top-level executions / 594 PASS nodes.
  • Final benchmark three-repeat race: 312 top-level executions / 594 PASS nodes. Final normal benchmark execution is included in the full aggregate. No benchmark skips, failures or race reports.
  • feat(review): replay a PR without its existing discussion #627 focused normal/race also passed: 40 / 120 top-level executions, including offline repair/retry, schema rejection, symlink obligations and saved-session preservation.
  • Final 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 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. This PR remains a draft, stacked on #627; refresh and revalidate if its parent changes.

Historical: Hosted CI

CI run 37735430437 completed successfully for exact head b83a5c471143df15a9f4f883ff644d236e254718: 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.

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.
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.
Base automatically changed from zzwong/issue-626/replay-without-discussion to main October 17, 2026 05:02

@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: 52124fdf726a
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 1
architecture:solid-reviewer-agnostic (1 finding)

Major - internal/cmd/benchmarkcmd/compare.go:529

U-L1: Bind verification to the original execution artifact. By inspection, replacing a previously verified cell's review.json with another isolated cell's valid JSON preserves verification and completed status, even when its run ID and SHAs differ. This contradicts BENCHMARKING.md's replacement-invalidates-verification contract and attributes replacement anchor results to the original execution. Persist a digest of the original review stdout and require a match during comparison; add a regression using a valid replacement with without_discussion=true.

Reviewer Coverage

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

  • go:implementation-tests — complete (broad); scope: 10 files; examples: internal/benchmark/discussion_test.go, internal/benchmark/suite.go, internal/cmd/benchmarkcmd/benchmarkcmd.go, internal/cmd/benchmarkcmd/benchmarkcmd_test.go, internal/cmd/benchmarkcmd/compare.go; 5 omitted; body-inspected: 10 files; examples: internal/benchmark/discussion_test.go, internal/benchmark/suite.go, internal/cmd/benchmarkcmd/benchmarkcmd.go, internal/cmd/benchmarkcmd/benchmarkcmd_test.go, internal/cmd/benchmarkcmd/compare.go; 5 omitted; skipped: none; context (not coverage): 1 file; examples: docs/development.md; constraints: Static review only; tests were inspected but not executed. Review focused on the assigned Go implementation and behavioral coverage.
  • policies:conventions — complete (broad); scope: 7 files; examples: BENCHMARKING.md, internal/benchmark/suite.go, internal/cmd/benchmarkcmd/benchmarkcmd.go, internal/cmd/benchmarkcmd/benchmarkcmd_test.go, internal/cmd/benchmarkcmd/compare.go; 2 omitted; body-inspected: 7 files; examples: BENCHMARKING.md, internal/benchmark/suite.go, internal/cmd/benchmarkcmd/benchmarkcmd.go, internal/cmd/benchmarkcmd/benchmarkcmd_test.go, internal/cmd/benchmarkcmd/compare.go; 2 omitted; skipped: none; context (not coverage): 3 files; examples: CONTRIBUTING.md, docs/development.md, internal/cmd/benchmarkcmd/discussion_test.go; constraints: Shared Open CLI Collective standards were linked but their contents were not available in the inspected repository; no missing policy contents were inferred. Static inspection only; tests were not executed.
  • structure:repo-health — complete (broad); scope: 8 files; examples: BENCHMARKING.md, internal/benchmark/suite.go, internal/cmd/benchmarkcmd/benchmarkcmd.go, internal/cmd/benchmarkcmd/compare.go, internal/cmd/benchmarkcmd/discussion_relocation_test.go; 3 omitted; body-inspected: 8 files; examples: BENCHMARKING.md, internal/benchmark/suite.go, internal/cmd/benchmarkcmd/benchmarkcmd.go, internal/cmd/benchmarkcmd/compare.go, internal/cmd/benchmarkcmd/discussion_relocation_test.go; 3 omitted; skipped: none; constraints: Static inspection only; tests were not executed. Review focused on the eight assigned files and their isolation-policy contract.
  • security:code-auditor — complete (broad); scope: 6 files; examples: internal/benchmark/suite.go, internal/cmd/benchmarkcmd/compare.go, internal/cmd/benchmarkcmd/discussion_relocation_test.go, internal/cmd/benchmarkcmd/discussion_test.go, internal/cmd/benchmarkcmd/executor.go; 1 omitted; body-inspected: 6 files; examples: internal/benchmark/suite.go, internal/cmd/benchmarkcmd/compare.go, internal/cmd/benchmarkcmd/discussion_relocation_test.go, internal/cmd/benchmarkcmd/discussion_test.go, internal/cmd/benchmarkcmd/executor.go; 1 omitted; skipped: none; constraints: Static review only; no tests or runtime verification were performed. Review focused on the six assigned files and their visible isolation and artifact-handling boundaries.
  • architecture:solid-reviewer-agnostic — complete (broad); scope: 5 files; examples: internal/benchmark/suite.go, internal/cmd/benchmarkcmd/benchmarkcmd.go, internal/cmd/benchmarkcmd/compare.go, internal/cmd/benchmarkcmd/executor.go, internal/cmd/benchmarkcmd/run.go; body-inspected: 5 files; examples: internal/benchmark/suite.go, internal/cmd/benchmarkcmd/benchmarkcmd.go, internal/cmd/benchmarkcmd/compare.go, internal/cmd/benchmarkcmd/executor.go, internal/cmd/benchmarkcmd/run.go; skipped: none; context (not coverage): 10 files; examples: .codereview/agents/policies/conventions/prompt.md, AGENTS.md, BENCHMARKING.md, CLAUDE.md, Makefile; 5 omitted; constraints: External harness-engineering PRINCIPLES.md was unavailable through repository-only tools. Review scope was the five assigned production files; related documentation and tests were inspected as context. Static review only; available tools do not permit running build, test, or lint commands.

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


Completed in 1m 03s | $0.17 | 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 03s wall · 1m 47s compute
Cost $0.17
Tokens 60.8k in / 2.2k out

Per-workstream usage

  • orchestrator-selection — openai-codex/gpt-6.1-sol
    • In: 9.6k
    • Out: 680
    • Cache read: 0
    • Cache create: 0
    • Cost: $0.03
    • Duration: 10s
  • go:implementation-tests — openai-codex/gpt-6.1-sol
    • In: 11.2k
    • Out: 257
    • Cache read: 23.8k
    • Cache create: 0
    • Cost: $0.03
    • Duration: 14s
  • policies:conventions — openai-codex/gpt-6.1-sol
    • In: 17.7k
    • Out: 161
    • Cache read: 62.2k
    • Cache create: 0
    • Cost: $0.04
    • Duration: 12s
  • structure:repo-health — openai-codex/gpt-6.1-sol
    • In: 3.5k
    • Out: 138
    • Cache read: 60.3k
    • Cache create: 0
    • Cost: $0.01
    • Duration: 17s
  • security:code-auditor — openai-codex/gpt-6.1-sol
    • In: 8.9k
    • Out: 172
    • Cache read: 22.8k
    • Cache create: 0
    • Cost: $0.02
    • Duration: 10s
  • architecture:solid-reviewer-agnostic — openai-codex/gpt-6.1-sol
    • In: 5.7k
    • Out: 661
    • Cache read: 72.7k
    • Cache create: 0
    • Cost: $0.03
    • Duration: 39s
  • orchestrator-rollup — openai-codex/gpt-6.1-sol
    • In: 4.2k
    • Out: 96
    • Cache read: 0
    • Cache create: 0
    • Cost: $0.01
    • Duration: 2s

Comment thread internal/cmd/benchmarkcmd/compare.go
monit-reviewer
monit-reviewer previously approved these changes Oct 17, 2026

@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: 758a22b99069
Profile: pi-codex-reviewer-gh - Posting as: monit-reviewer

Summary

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

Minor - internal/cmd/benchmarkcmd/discussion_test.go:242

This regression now passes for the wrong reason: it never sets ReviewJSONSHA256, so the artifact reader rejects the JSON before the saved-verification guard is exercised. Removing the run.WithoutDiscussionVerified check from reconciliation would still pass this test. Set the digest to match the written artifact and assert the run remains unverified; that isolates the intended cannot-upgrade contract.

Reviewer Coverage

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

  • go:implementation-tests — complete (constrained); scope: 10 files; examples: internal/benchmark/discussion_test.go, internal/benchmark/suite.go, internal/cmd/benchmarkcmd/benchmarkcmd.go, internal/cmd/benchmarkcmd/benchmarkcmd_test.go, internal/cmd/benchmarkcmd/compare.go; 5 omitted; body-inspected: 10 files; examples: internal/benchmark/discussion_test.go, internal/benchmark/suite.go, internal/cmd/benchmarkcmd/benchmarkcmd.go, internal/cmd/benchmarkcmd/benchmarkcmd_test.go, internal/cmd/benchmarkcmd/compare.go; 5 omitted; skipped: none; constraints: Static inspection only; tests were not run because the available review tools are read-only.
  • policies:conventions — complete (constrained); scope: 7 files; examples: BENCHMARKING.md, internal/benchmark/suite.go, internal/cmd/benchmarkcmd/benchmarkcmd.go, internal/cmd/benchmarkcmd/benchmarkcmd_test.go, internal/cmd/benchmarkcmd/compare.go; 2 omitted; body-inspected: 7 files; examples: BENCHMARKING.md, internal/benchmark/suite.go, internal/cmd/benchmarkcmd/benchmarkcmd.go, internal/cmd/benchmarkcmd/benchmarkcmd_test.go, internal/cmd/benchmarkcmd/compare.go; 2 omitted; skipped: none; context (not coverage): 4 files; examples: AGENTS.md, CLAUDE.md, CONTRIBUTING.md, docs/development.md; constraints: Read-only inspection; tests and lint were not run. Review limited to the seven assigned files and visible repository conventions. External shared standards were not available in the provided context.
  • structure:repo-health — complete (constrained); scope: 8 files; examples: BENCHMARKING.md, internal/benchmark/suite.go, internal/cmd/benchmarkcmd/benchmarkcmd.go, internal/cmd/benchmarkcmd/compare.go, internal/cmd/benchmarkcmd/discussion_relocation_test.go; 3 omitted; body-inspected: 8 files; examples: BENCHMARKING.md, internal/benchmark/suite.go, internal/cmd/benchmarkcmd/benchmarkcmd.go, internal/cmd/benchmarkcmd/compare.go, internal/cmd/benchmarkcmd/discussion_relocation_test.go; 3 omitted; skipped: none; constraints: Static review only; tests and lint were not executed. Review focused on the eight assigned files and repository-health risks.
  • security:code-auditor — complete (constrained); scope: 6 files; examples: internal/benchmark/suite.go, internal/cmd/benchmarkcmd/compare.go, internal/cmd/benchmarkcmd/discussion_relocation_test.go, internal/cmd/benchmarkcmd/discussion_test.go, internal/cmd/benchmarkcmd/executor.go; 1 omitted; body-inspected: 6 files; examples: internal/benchmark/suite.go, internal/cmd/benchmarkcmd/compare.go, internal/cmd/benchmarkcmd/discussion_relocation_test.go, internal/cmd/benchmarkcmd/discussion_test.go, internal/cmd/benchmarkcmd/executor.go; 1 omitted; skipped: none; constraints: Static inspection only; tests were not executed. Review limited to the six assigned files.
  • architecture:solid-reviewer-agnostic — complete (constrained); scope: 5 files; examples: internal/benchmark/suite.go, internal/cmd/benchmarkcmd/benchmarkcmd.go, internal/cmd/benchmarkcmd/compare.go, internal/cmd/benchmarkcmd/executor.go, internal/cmd/benchmarkcmd/run.go; body-inspected: 5 files; examples: internal/benchmark/suite.go, internal/cmd/benchmarkcmd/benchmarkcmd.go, internal/cmd/benchmarkcmd/compare.go, internal/cmd/benchmarkcmd/executor.go, internal/cmd/benchmarkcmd/run.go; 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: Static review only; repository tools do not support running build, test, or lint commands. Reported verification was not independently reproduced. The external harness-engineering PRINCIPLES.md was unavailable through repository-only tools.

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


Completed in 57s | $0.11 | 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 57s wall · 1m 53s compute
Cost $0.11
Tokens 35.7k in / 1.3k out

Per-workstream usage

  • go:implementation-tests — openai-codex/gpt-6.1-sol
    • In: 381
    • Out: 375
    • Cache read: 42.9k
    • Cache create: 0
    • Cost: $0.01
    • Duration: 24s
  • policies:conventions — openai-codex/gpt-6.1-sol
    • In: 14.6k
    • Out: 159
    • Cache read: 37.9k
    • Cache create: 0
    • Cost: $0.03
    • Duration: 20s
  • structure:repo-health — openai-codex/gpt-6.1-sol
    • In: 5.7k
    • Out: 178
    • Cache read: 24.6k
    • Cache create: 0
    • Cost: $0.02
    • Duration: 15s
  • security:code-auditor — openai-codex/gpt-6.1-sol
    • In: 3.6k
    • Out: 118
    • Cache read: 44.0k
    • Cache create: 0
    • Cost: $0.01
    • Duration: 9s
  • architecture:solid-reviewer-agnostic — openai-codex/gpt-6.1-sol
    • In: 7.3k
    • Out: 361
    • Cache read: 66.9k
    • Cache create: 0
    • Cost: $0.02
    • Duration: 40s
  • orchestrator-rollup — openai-codex/gpt-6.1-sol
    • In: 4.1k
    • Out: 75
    • Cache read: 0
    • Cache create: 0
    • Cost: $0.01
    • Duration: 3s

Comment thread internal/cmd/benchmarkcmd/discussion_test.go

@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: f69522c8e76c
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 (constrained); scope: 10 files; examples: internal/benchmark/discussion_test.go, internal/benchmark/suite.go, internal/cmd/benchmarkcmd/benchmarkcmd.go, internal/cmd/benchmarkcmd/benchmarkcmd_test.go, internal/cmd/benchmarkcmd/compare.go; 5 omitted; body-inspected: 10 files; examples: internal/benchmark/discussion_test.go, internal/benchmark/suite.go, internal/cmd/benchmarkcmd/benchmarkcmd.go, internal/cmd/benchmarkcmd/benchmarkcmd_test.go, internal/cmd/benchmarkcmd/compare.go; 5 omitted; skipped: none; context (not coverage): 1 file; examples: docs/development.md; constraints: Static review only; tests and lint were inspected but not executed.
  • policies:conventions — complete (constrained); scope: 7 files; examples: BENCHMARKING.md, internal/benchmark/suite.go, internal/cmd/benchmarkcmd/benchmarkcmd.go, internal/cmd/benchmarkcmd/benchmarkcmd_test.go, internal/cmd/benchmarkcmd/compare.go; 2 omitted; body-inspected: 7 files; examples: BENCHMARKING.md, internal/benchmark/suite.go, internal/cmd/benchmarkcmd/benchmarkcmd.go, internal/cmd/benchmarkcmd/benchmarkcmd_test.go, internal/cmd/benchmarkcmd/compare.go; 2 omitted; skipped: none; context (not coverage): 1 file; examples: .codereview/agents/policies/conventions/prompt.md; constraints: Review limited to assigned files and visible repository conventions; external shared standards were not inspected. Static inspection only; tests and lint were not executed.
  • structure:repo-health — complete (constrained); scope: 8 files; examples: BENCHMARKING.md, internal/benchmark/suite.go, internal/cmd/benchmarkcmd/benchmarkcmd.go, internal/cmd/benchmarkcmd/compare.go, internal/cmd/benchmarkcmd/discussion_relocation_test.go; 3 omitted; body-inspected: 8 files; examples: BENCHMARKING.md, internal/benchmark/suite.go, internal/cmd/benchmarkcmd/benchmarkcmd.go, internal/cmd/benchmarkcmd/compare.go, internal/cmd/benchmarkcmd/discussion_relocation_test.go; 3 omitted; skipped: none; constraints: Static review only; tests and lint were inspected but not executed. Review scope was the eight assigned files.
  • security:code-auditor — complete (constrained); scope: 6 files; examples: internal/benchmark/suite.go, internal/cmd/benchmarkcmd/compare.go, internal/cmd/benchmarkcmd/discussion_relocation_test.go, internal/cmd/benchmarkcmd/discussion_test.go, internal/cmd/benchmarkcmd/executor.go; 1 omitted; body-inspected: 6 files; examples: internal/benchmark/suite.go, internal/cmd/benchmarkcmd/compare.go, internal/cmd/benchmarkcmd/discussion_relocation_test.go, internal/cmd/benchmarkcmd/discussion_test.go, internal/cmd/benchmarkcmd/executor.go; 1 omitted; skipped: none; constraints: Static security review of the six assigned files; tests were inspected but not executed.
  • architecture:solid-reviewer-agnostic — complete (constrained); scope: 5 files; examples: internal/benchmark/suite.go, internal/cmd/benchmarkcmd/benchmarkcmd.go, internal/cmd/benchmarkcmd/compare.go, internal/cmd/benchmarkcmd/executor.go, internal/cmd/benchmarkcmd/run.go; body-inspected: 5 files; examples: internal/benchmark/suite.go, internal/cmd/benchmarkcmd/benchmarkcmd.go, internal/cmd/benchmarkcmd/compare.go, internal/cmd/benchmarkcmd/executor.go, internal/cmd/benchmarkcmd/run.go; skipped: none; context (not coverage): 9 files; examples: .codereview/agents/policies/conventions/prompt.md, .codereview/agents/structure/repo-health/prompt.md, AGENTS.md, CLAUDE.md, docs/architecture.md; 4 omitted; constraints: Read-only tools permit source inspection only; build, test, race, and lint results were not independently verified. Review limited to the five assigned production files, with surrounding documentation and tests inspected as context. The external harness-engineering PRINCIPLES.md was not inspected because repository-only inspection was permitted.

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


Completed in 43s | $0.25 | 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 43s wall · 1m 22s compute
Cost $0.25
Tokens 111.1k in / 1.0k out

Per-workstream usage

  • go:implementation-tests — openai-codex/gpt-6.1-sol
    • In: 9.1k
    • Out: 159
    • Cache read: 47.6k
    • Cache create: 0
    • Cost: $0.02
    • Duration: 16s
  • policies:conventions — openai-codex/gpt-6.1-sol
    • In: 35.3k
    • Out: 198
    • Cache read: 22.7k
    • Cache create: 0
    • Cost: $0.07
    • Duration: 11s
  • structure:repo-health — openai-codex/gpt-6.1-sol
    • In: 16.9k
    • Out: 156
    • Cache read: 49.2k
    • Cache create: 0
    • Cost: $0.04
    • Duration: 15s
  • security:code-auditor — openai-codex/gpt-6.1-sol
    • In: 33.8k
    • Out: 117
    • Cache read: 7.3k
    • Cache create: 0
    • Cost: $0.07
    • Duration: 10s
  • architecture:solid-reviewer-agnostic — openai-codex/gpt-6.1-sol
    • In: 12.1k
    • Out: 327
    • Cache read: 63.2k
    • Cache create: 0
    • Cost: $0.03
    • Duration: 25s
  • orchestrator-rollup — openai-codex/gpt-6.1-sol
    • In: 3.9k
    • Out: 53
    • 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:12
@zzwong
zzwong merged commit 0504865 into main Oct 17, 2026
12 checks passed
@zzwong
zzwong deleted the zzwong/issue-628/pinned-benchmark-discussion-isolation branch October 17, 2026 05:15
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.

Benchmark: keep pinned cases from reusing saved review sessions

2 participants