fix: make review workbenches location-independent and resume-safe#507
Conversation
![rianjs-bot[bot]](https://avatars.githubusercontent.com/u/7378790?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 4356d3af18e5
Profile: codex-rianjs-bot - Posting as: rianjs-bot[bot]
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 1 |
| structure:repo-health | 1 |
| policies:conventions | 1 |
| documentation:docs | 0 |
go:implementation-tests (1 finding)
Major - internal/gitexec/client.go:45
Runnow reads a repository access token for everyclone, but the new reviewer workspace path clones from the already-prepared local workbench repo (internal/workbench/workbench.go:365). That turns a purely local filesystem clone into a credential-dependent operation, so a resumed or offline run can fail after the durable workbench already exists if PAT lookup / GitHub App refresh is unavailable. The current tests only prove that localstatusskips token reads; they do not cover localclone. Gate authentication on the clone/fetch target actually being a remote URL (or otherwise detect local paths /file://sources), and add a regression test that local reviewer-workspace clones do not call the token source.
structure:repo-health (1 finding)
Major - internal/pipeline/pipeline.go:173
Repository binding is meant to validate the external PR identity before durable review state is derived from it, but
SelectionOnlynow callsprepareSelectionContextbeforevalidateRepositoryBinding.prepareSelectionContextfetches diff/thread/review/comment data and writesdossier/rawartifacts before returning, so a mismatched provider response can still populate durable artifacts even though the later trust-binding check rejects the run. That leaves the new location-independent workbench flow with a boundary check that happens after side effects. Move the binding check to immediately afterGetPR/resolveReviewPRContextand before any additional fetches or artifact writes, and add a test that asserts no dossier/workbench artifacts are created on binding failure.
policies:conventions (1 finding)
Minor - docs/architecture.md:75
This file is the repo-local architecture source of truth, but after adding the new Git transport guidance it still says review runtime assembly belongs in
internal/reviewruntimeand thatcr review/cr respondshould constructreviewruntime.OpenRequestvalues. In the current tree the production composition root isinternal/app/app.OpenRequest, andinternal/architecture/git_transport_test.gonow codifiesinternal/appas the only place allowed to constructgitexec. Leaving the old package names here gives contributors the wrong layering rule for future runtime work. Update the guardrail text to point atinternal/appfor production runtime assembly, while keepinginternal/appruntimescoped to small command-independent helpers like retention and repo-root resolution.
Reviewer Coverage
| Reviewer | Status | Inspected | Skipped | Constraints |
|---|---|---|---|---|
| go:implementation-tests | complete_broad | internal/app/runtime.go, internal/app/runtime_selection.go, internal/app/runtime_test.go, internal/architecture/git_transport_test.go, internal/cmd/benchmarkcmd/executor.go, internal/cmd/benchmarkcmd/select.go, internal/cmd/benchmarkcmd/select_test.go, internal/cmd/noleak/noleak_test.go, internal/cmd/reviewcmd/reviewcmd.go, internal/cmd/reviewcmd/reviewcmd_test.go, internal/gitexec/client.go, internal/gitexec/client_test.go, internal/gitprovider/github/app_auth_test.go, internal/gitprovider/github/client.go, internal/pipeline/failure.go, internal/pipeline/pipeline.go, internal/pipeline/pipeline_test.go, internal/pipeline/workbench_integration_test.go, internal/reviewrun/reviewrun.go, internal/reviewrun/reviewrun_test.go, internal/workbench/workbench.go, internal/workbench/workbench_test.go | unavailable | A broad go test sweep across the touched packages was not fully usable in this workspace because several packages failed to build under the sandbox/toolchain path (clang/cgo path-with-spaces issue), so findings are primarily from code inspection plus targeted test reads.; Review scope was limited to the assigned Go implementation and test files. |
| structure:repo-health | complete_broad | docs/development.md, internal/app/runtime.go, internal/app/runtime_selection.go, internal/architecture/git_transport_test.go, internal/gitexec/client.go, internal/gitexec/client_test.go, internal/pipeline/failure.go, internal/pipeline/pipeline.go, internal/pipeline/pipeline_test.go, internal/pipeline/workbench_integration_test.go, internal/reviewrun/reviewrun.go, internal/reviewrun/reviewrun_test.go, internal/workbench/workbench.go, internal/workbench/workbench_test.go | unavailable | Review focused on structural risks in the assigned diff and the smallest adjacent code needed to validate changed contracts; unrelated pre-existing code was not exhaustively re-audited. |
| policies:conventions | complete_broad | README.md, docs/architecture.md, docs/checkout-native-review-contract.md, docs/development.md, docs/llm-task-artifacts.md, internal/app/runtime.go, internal/app/runtime_selection.go, internal/cmd/benchmarkcmd/select.go, internal/cmd/benchmarkcmd/select_test.go, internal/cmd/reviewcmd/reviewcmd.go, internal/cmd/reviewcmd/reviewcmd_test.go | unavailable | Review limited to the assigned changed files and the repo-local convention sources visible in this checkout (AGENTS.md, docs/development.md, and docs/architecture.md). |
| documentation:docs | complete_broad | README.md, docs/architecture.md, docs/checkout-native-review-contract.md, docs/development.md, docs/llm-task-artifacts.md | unavailable | Optional external/shared source-of-truth repositories linked from the docs were not assumed to be checked out, so accuracy checks were performed against the local repository state and changed implementation only.; Review scope was limited to the assigned changed documentation files plus the local repo implementation and guidance they reference; broader code-review concerns were out of scope. |
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 6m 39s | unavailable | gpt-5.4 | cr dev
| Field | Value |
|---|---|
| Model | gpt-5.4 |
| Reviewers | go:implementation-tests, structure:repo-health, policies:conventions, documentation:docs |
| Engine | codex_cli · gpt-5.4 |
| Reviewed by | cr · rianjs-bot[bot] |
| Duration | 6m 39s wall · 13m 02s compute |
| Cost | unavailable |
| Tokens | 3.1M in / 38.7k out |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost | Duration |
|---|---|---|---|---|---|---|---|
| orchestrator-selection | gpt-5.4 | 27.4k | 1.3k | 21.8k | unavailable | unavailable | 28s |
| go:implementation-tests | gpt-5.4 | 1.4M | 14.1k | 1.3M | unavailable | unavailable | 4m 58s |
| structure:repo-health | gpt-5.4 | 770.1k | 8.9k | 626.9k | unavailable | unavailable | 3m 07s |
| policies:conventions | gpt-5.4 | 212.2k | 6.8k | 173.2k | unavailable | unavailable | 2m 13s |
| documentation:docs | gpt-5.4 | 616.2k | 6.0k | 530.2k | unavailable | unavailable | 2m 02s |
| orchestrator-rollup | gpt-5.4 | 45.3k | 1.5k | 23.7k | unavailable | unavailable | 12s |
| func (c *Client) Run(ctx context.Context, dir string, args ...string) ([]byte, error) { | ||
| header := "" | ||
| token := "" | ||
| if requiresAuthentication(args) { |
There was a problem hiding this comment.
Run now reads a repository access token for every clone, but the new reviewer workspace path clones from the already-prepared local workbench repo (internal/workbench/workbench.go:365). That turns a purely local filesystem clone into a credential-dependent operation, so a resumed or offline run can fail after the durable workbench already exists if PAT lookup / GitHub App refresh is unavailable. The current tests only prove that local status skips token reads; they do not cover local clone. Gate authentication on the clone/fetch target actually being a remote URL (or otherwise detect local paths / file:// sources), and add a regression test that local reviewer-workspace clones do not call the token source.
Reply inline to this comment.
There was a problem hiding this comment.
Fixed - authentication is now limited to same-host HTTPS fetches, and local clone/status tests use an unavailable token source.
There was a problem hiding this comment.
Confirmed. Limiting auth to same-host HTTPS fetches avoids credential lookups for local reviewer-workspace clones, and using an unavailable token source in the local clone/status tests covers the regression I called out.
Summary:
Auth is now only applied to same-host HTTPS fetches, so local path and file-based clones remain credential-free. Regression coverage was added by exercising local clone/status with an unavailable token source.
| @@ -173,6 +173,7 @@ type SelectionRequest struct { | |||
| ArtifactDir string | |||
There was a problem hiding this comment.
File-level note: internal/pipeline/pipeline.go
Repository binding is meant to validate the external PR identity before durable review state is derived from it, but SelectionOnly now calls prepareSelectionContext before validateRepositoryBinding. prepareSelectionContext fetches diff/thread/review/comment data and writes dossier/raw artifacts before returning, so a mismatched provider response can still populate durable artifacts even though the later trust-binding check rejects the run. That leaves the new location-independent workbench flow with a boundary check that happens after side effects. Move the binding check to immediately after GetPR/resolveReviewPRContext and before any additional fetches or artifact writes, and add a test that asserts no dossier/workbench artifacts are created on binding failure.
Reply inline to this comment.
There was a problem hiding this comment.
Fixed - SelectionOnly now validates the resolved PR immediately after GetPR and before other fetches or artifact writes; the mismatch test asserts an empty artifact directory.
There was a problem hiding this comment.
Confirmed. Moving repository binding validation to immediately after the resolved PR lookup closes the side-effect window, and the mismatch test covering an empty artifact directory addresses the regression risk I called out.
Summary:
Repository binding is now validated before selection-context fetches or artifact writes, and the mismatch test asserts no dossier/workbench artifacts are created on binding failure.
| @@ -75,6 +75,19 @@ This keeps markers, retries, reconciliation, idempotency, and resume behavior in | |||
| one place. New commands such as `cr respond` should produce planned thread | |||
There was a problem hiding this comment.
File-level note: docs/architecture.md
This file is the repo-local architecture source of truth, but after adding the new Git transport guidance it still says review runtime assembly belongs in internal/reviewruntime and that cr review/cr respond should construct reviewruntime.OpenRequest values. In the current tree the production composition root is internal/app/app.OpenRequest, and internal/architecture/git_transport_test.go now codifies internal/app as the only place allowed to construct gitexec. Leaving the old package names here gives contributors the wrong layering rule for future runtime work. Update the guardrail text to point at internal/app for production runtime assembly, while keeping internal/appruntime scoped to small command-independent helpers like retention and repo-root resolution.
Reply inline to this comment.
There was a problem hiding this comment.
Fixed - the architecture guardrail now names internal/app and app.OpenRequest as the production composition boundary.
There was a problem hiding this comment.
Thanks. That update aligns the architecture guardrail with the current production composition boundary in internal/app and app.OpenRequest.
Summary:
Updated docs/architecture.md to name internal/app and app.OpenRequest as the production runtime composition boundary, replacing the outdated internal/reviewruntime guidance.
Architect reviewArchitecture, product behavior, tests, no-leak coverage, repository checks, and live dogfood converge. One non-blocking history note remains: the implementation and repair commits do not preserve the originally planned characterization/refactor-first sequence. The existing reviewed history is being retained rather than rewritten after the fact. |
Findings
|
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 2cbb49e66d69
Profile: codex-rianjs-bot - Posting as: rianjs-bot[bot]
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 1 |
| structure:repo-health | 2 |
| policies:conventions | 1 |
| documentation:docs | 0 |
go:implementation-tests (1 finding)
Major - internal/cmd/reviewcmd/reviewcmd_test.go:8
The new contract here is that
cr reviewcan be launched from a cwd that is not a checkout, but this acceptance harness still replacesapp.Openwith a stubbed factory and only asserts the parsedOpenRequestplus rendering. That means the command-level path that now composes the authenticated Git transport and location-independent workbench setup is never exercised from thereviewcommand itself. The lower-level coverage added ininternal/appandinternal/pipelineis useful, but it would still miss a regression whererunReviewor its runtime wiring reintroduced checkout-dependent behavior. Add a focused command acceptance test thatt.Chdirs to a temp dir outside any checkout and drives the real review runtime composition (or a minimally wrapped realapp.Openwith fake provider/adapter deps), then assert the dry-run completes and produces a workbench pinned to the PR SHAs.
structure:repo-health (2 findings)
Major - internal/pipeline/pipeline.go:173
SelectionOnlynow has a request-levelMaxAgentscontract andinternal/app/runtime_selection.gothreadsselection.MaxAgentsinto the pipeline options, but this call still builds the selection prompt fromopts.maxAgents()instead ofreq.MaxAgents. That means the new selection-runtime API silently ignores the caller’s cap unless the caller also mutates pipeline options, so benchmark/selection tooling can diverge from the intended review semantics while the existing test still passes by exercising only directOptions{MaxAgents: ...}usage. Passreq.MaxAgentsthrough torunSelectionPhasehere and add a runtime-level regression test that goes throughSelectionRuntime.Selectto prove the request cap reaches both the prompt and the post-decode truncation path.
Major - docs/development.md:43
The repo-local development map should point contributors at the current runtime composition boundary, but this section still says review/response runtime assembly lives in
internal/appruntimeandinternal/reviewruntimeeven though this PR’s new guardrail and the settled architecture guidance move that responsibility tointernal/app/app.OpenRequest. Leaving the old package map in the same source-of-truth doc makes future boundary work harder to discover and encourages new composition code to land in the wrong place. Update the overview (and any repeated package-shape summary in this file) to name the currentinternal/appboundary explicitly and demote the older packages to whatever narrower role they still have, if any.
policies:conventions (1 finding)
Minor - internal/app/runtime.go:133
docs/architecture.mdnow says command-independent runtime helpers such as repository-root resolution belong ininternal/appruntime, but this default seam now callsreporoot.Resolvedirectly frominternal/app. That duplicates the helper boundary the doc just defined and makes the layering guidance harder to trust. Prefer defaulting toappruntime.ResolveRepoRoothere, or update the architecture doc if the intended convention has changed.
Reviewer Coverage
| Reviewer | Status | Inspected | Skipped | Constraints |
|---|---|---|---|---|
| go:implementation-tests | incomplete_skipped | internal/app/runtime.go, internal/app/runtime_selection.go, internal/app/runtime_test.go, internal/architecture/git_transport_test.go, internal/cmd/reviewcmd/reviewcmd.go, internal/cmd/reviewcmd/reviewcmd_test.go, internal/gitexec/client.go, internal/gitexec/client_test.go, internal/gitprovider/github/app_auth_test.go, internal/gitprovider/github/client.go, internal/pipeline/pipeline.go, internal/pipeline/pipeline_test.go, internal/pipeline/workbench_integration_test.go, internal/reviewrun/reviewrun.go, internal/reviewrun/reviewrun_test.go, internal/workbench/workbench.go, internal/workbench/workbench_test.go | internal/cmd/benchmarkcmd/executor.go, internal/cmd/benchmarkcmd/select.go, internal/cmd/benchmarkcmd/select_test.go, internal/cmd/noleak/noleak_test.go, internal/pipeline/failure.go | unavailable |
| structure:repo-health | complete_broad | docs/development.md, internal/app/runtime.go, internal/app/runtime_selection.go, internal/architecture/git_transport_test.go, internal/gitexec/client.go, internal/gitexec/client_test.go, internal/pipeline/failure.go, internal/pipeline/pipeline.go, internal/pipeline/pipeline_test.go, internal/pipeline/workbench_integration_test.go, internal/reviewrun/reviewrun.go, internal/reviewrun/reviewrun_test.go, internal/workbench/workbench.go, internal/workbench/workbench_test.go | unavailable | Scope limited to the assigned changed files and structural risks visible in this diff. |
| policies:conventions | complete_broad | README.md, docs/architecture.md, docs/checkout-native-review-contract.md, docs/development.md, docs/llm-task-artifacts.md, internal/app/runtime.go, internal/app/runtime_selection.go, internal/cmd/benchmarkcmd/select.go, internal/cmd/benchmarkcmd/select_test.go, internal/cmd/reviewcmd/reviewcmd.go, internal/cmd/reviewcmd/reviewcmd_test.go | unavailable | Scoped to assigned files and local convention docs. |
| documentation:docs | complete_broad | README.md, docs/architecture.md, docs/checkout-native-review-contract.md, docs/development.md, docs/llm-task-artifacts.md | unavailable | Review limited to the assigned documentation files, with spot-checks against the in-repo implementation and local repo guidance only. |
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 6m 51s | unavailable | gpt-5.4 | cr dev
| Field | Value |
|---|---|
| Model | gpt-5.4 |
| Reviewers | go:implementation-tests, structure:repo-health, policies:conventions, documentation:docs |
| Engine | codex_cli · gpt-5.4 |
| Reviewed by | cr · rianjs-bot[bot] |
| Duration | 6m 51s wall · 13m 44s compute |
| Cost | unavailable |
| Tokens | 1.4M in / 27.3k out |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost | Duration |
|---|---|---|---|---|---|---|---|
| orchestrator-selection | gpt-5.4 | 67.6k | 2.3k | 32.3k | unavailable | unavailable | 20s |
| go:implementation-tests | gpt-5.4 | 197.9k | 4.5k | 142.7k | unavailable | unavailable | 5m 16s |
| structure:repo-health | gpt-5.4 | 430.9k | 7.8k | 361.3k | unavailable | unavailable | 2m 40s |
| policies:conventions | gpt-5.4 | 234.3k | 5.5k | 187.1k | unavailable | unavailable | 3m 36s |
| documentation:docs | gpt-5.4 | 384.3k | 4.5k | 331.3k | unavailable | unavailable | 1m 37s |
| orchestrator-rollup | gpt-5.4 | 94.6k | 2.8k | 37.8k | unavailable | unavailable | 13s |
| @@ -43,6 +43,12 @@ Within `internal/pipeline`, the public entry points are `DryRun`, `Live`, and | |||
| and no ledger or posting side effects so benchmark tooling can reuse the real | |||
There was a problem hiding this comment.
File-level note: docs/development.md
The repo-local development map should point contributors at the current runtime composition boundary, but this section still says review/response runtime assembly lives in internal/appruntime and internal/reviewruntime even though this PR’s new guardrail and the settled architecture guidance move that responsibility to internal/app/app.OpenRequest. Leaving the old package map in the same source-of-truth doc makes future boundary work harder to discover and encourages new composition code to land in the wrong place. Update the overview (and any repeated package-shape summary in this file) to name the current internal/app boundary explicitly and demote the older packages to whatever narrower role they still have, if any.
Reply inline to this comment.
There was a problem hiding this comment.
Fixed - development.md now identifies internal/app as the production composition root and limits internal/appruntime to small runtime helpers.
There was a problem hiding this comment.
Confirmed. Updating docs/development.md to name internal/app as the production composition root and narrowing internal/appruntime to helper duties addresses the stale boundary guidance.
Summary:
docs/development.md now reflects the current runtime composition boundary by identifying internal/app as the production composition root and limiting internal/appruntime to helper responsibilities.
| return client.Run, nil | ||
| } | ||
| } | ||
| if d.ResolveRepoRoot == nil { |
There was a problem hiding this comment.
docs/architecture.md now says command-independent runtime helpers such as repository-root resolution belong in internal/appruntime, but this default seam now calls reporoot.Resolve directly from internal/app. That duplicates the helper boundary the doc just defined and makes the layering guidance harder to trust. Prefer defaulting to appruntime.ResolveRepoRoot here, or update the architecture doc if the intended convention has changed.
Reply inline to this comment.
There was a problem hiding this comment.
Fixed - app now defaults the seam through appruntime.ResolveRepoRoot.
There was a problem hiding this comment.
Confirmed. Routing the default seam through appruntime.ResolveRepoRoot restores the architecture boundary described in docs/architecture.md.
Summary:
Resolved after switching the default repo-root seam in internal/app to appruntime.ResolveRepoRoot, keeping command-independent runtime helpers behind the documented internal/appruntime boundary.
| @@ -173,6 +173,7 @@ type SelectionRequest struct { | |||
| ArtifactDir string | |||
There was a problem hiding this comment.
File-level note: internal/pipeline/pipeline.go
SelectionOnly now has a request-level MaxAgents contract and internal/app/runtime_selection.go threads selection.MaxAgents into the pipeline options, but this call still builds the selection prompt from opts.maxAgents() instead of req.MaxAgents. That means the new selection-runtime API silently ignores the caller’s cap unless the caller also mutates pipeline options, so benchmark/selection tooling can diverge from the intended review semantics while the existing test still passes by exercising only direct Options{MaxAgents: ...} usage. Pass req.MaxAgents through to runSelectionPhase here and add a runtime-level regression test that goes through SelectionRuntime.Select to prove the request cap reaches both the prompt and the post-decode truncation path.
Reply inline to this comment.
There was a problem hiding this comment.
Fixed - SelectionOnly now treats SelectionRequest.MaxAgents as authoritative, with prompt and truncation coverage driven by the request field.
There was a problem hiding this comment.
Verified conceptually. Treating SelectionRequest.MaxAgents as authoritative addresses the mismatch between the selection runtime request and pipeline options, and the added coverage for both prompt construction and post-decode truncation closes the regression called out here.
Summary:
SelectionOnly now uses SelectionRequest.MaxAgents as the authoritative cap, with regression coverage ensuring the request-level value reaches both selection prompt construction and truncation after decode.
| @@ -8,7 +8,6 @@ import ( | |||
| "fmt" | |||
There was a problem hiding this comment.
File-level note: internal/cmd/reviewcmd/reviewcmd_test.go
The new contract here is that cr review can be launched from a cwd that is not a checkout, but this acceptance harness still replaces app.Open with a stubbed factory and only asserts the parsed OpenRequest plus rendering. That means the command-level path that now composes the authenticated Git transport and location-independent workbench setup is never exercised from the review command itself. The lower-level coverage added in internal/app and internal/pipeline is useful, but it would still miss a regression where runReview or its runtime wiring reintroduced checkout-dependent behavior. Add a focused command acceptance test that t.Chdirs to a temp dir outside any checkout and drives the real review runtime composition (or a minimally wrapped real app.Open with fake provider/adapter deps), then assert the dry-run completes and produces a workbench pinned to the PR SHAs.
Reply inline to this comment.
There was a problem hiding this comment.
Covered at the existing boundaries: the no-leak command harness now invokes review from a non-Git cwd through the real pipeline/workbench, OpenSelection executes its composed runtime, and the built binary completed live run 5523af63-5ed9-4d8d-8a92-ffc6b4a34d8b from /private/tmp. Another bespoke command/app harness would duplicate those checks.
There was a problem hiding this comment.
Thanks, that addresses the gap I was calling out. Given the existing non-Git-cwd harness coverage, the real pipeline/workbench execution through OpenSelection, and the successful built-binary run from /private/tmp, I agree an additional bespoke reviewcmd acceptance harness would be duplicative here.
Summary:
Existing coverage already exercises the non-checkout launch path at the relevant boundaries: command invocation from a non-Git cwd, real composed review runtime execution, and a successful live built-binary run from /private/tmp. No extra dedicated reviewcmd acceptance harness is needed.
|
These are low-value, please approve the PR |
Final architect reviewNo remaining architectural drift. Fork authentication, repository casing, dead remote parsing, location independence, resume semantics, no-leak coverage, and dependency boundaries converge. The sole non-blocking note is the already-acknowledged lack of a characterization-first commit sequence; reviewed history is retained rather than rewritten after the fact. |
rianjs
changed the title
1 participant
Closes #506
Summary
Verification
make check/private/tmp/codex-cr-506and ran it from/private/tmpagainst this PR withcodex-rianjs-bot; live run5523af63-5ed9-4d8d-8a92-ffc6b4a34d8bcompleted and posted without a local checkout