feat: add checkout-readonly adapter capability checks

[rianjs]

No description provided.

[rianjs]

Findings

• Major: internal/pipeline/pipeline.go:3366 resets a narrowed checkout scope with os.RemoveAll(scopeRoot), but internal/pipeline/pipeline.go:3386 then locks that same scope tree read-only. A resumed/retried reviewer for the same agent will fail to reset the existing scope because the directory is not writable. Call unlockCheckoutReadonlyScope(scopeRoot) before RemoveAll, and add a rerun/reset test for an existing narrowed scope.

• Major: internal/llm/adapter.go:49 adds MaxToolOutputBytes, but the subprocess adapter never enforces or forwards it. internal/llm/subprocess.go:393 only adds --add-dir; there is no output cap behavior beyond tests echoing the number. The adapter contract requires bounded tool output, so this is currently a no-op contract field.

• Minor: internal/pipeline/pipeline_test.go:1890 asserts out-of-scope read denial using other.go, but the workbench fixture only creates main.go (internal/pipeline/pipeline_test.go:4948). That proves missing-file behavior, not allowed_files denial for an existing out-of-scope file/search result.

[rianjs]

Findings

No architectural blockers or product-adherence issues found. The fixes address the prior concerns: narrowed scopes are unlocked before reset, output cap behavior is now enforced in subprocess logs with coverage, and allowed_files tests now prove denial against an existing out-of-scope file.

[rianjs]

Findings

• Major: The unsupported-adapter check is still too local to prove the “no silent fallback” requirement. The only negative assertion is buildCheckoutReadonlyRequest itself in internal/pipeline/pipeline_test.go, and the capability helper tests in internal/llm/adapter_test.go never go through the real review entrypoint. If pipeline code still builds a stuffed diff/full-file prompt before this helper is reached, these tests would still pass. Please add one pipeline-level test that exercises the actual checkout-readonly reviewer path and asserts it fails before any fallback prompt is constructed.

• Major: The happy-path coverage is over-mocked around a synthetic in-process adapter. In internal/pipeline/pipeline_test.go, checkoutReadonlySmokeAdapter performs the os.ReadFile/os.WriteFile probes itself, so it can pass even if the real adapter only exposes a copied subset of files, never wires search/git-diff commands, or ignores the “empty allowedFiles means full checkout” contract. The subprocess tests in internal/llm/subprocess_test.go only verify --add-dir, cwd, and log truncation on a helper process; they do not prove the real checkout-readonly adapter can read/search the pinned checkout or that non-empty allowlists still deny out-of-scope reads/searches. Add at least one probe outside the changed-file set and one search/diff-style assertion against the real checkout-readonly path.

[monit-reviewer]

Automated PR Review

Reviewed commit: df074b6

Summary

Reviewer	Findings
harness-engineering:harness-enforcement-reviewer	1
harness-engineering:harness-knowledge-reviewer	1
harness-engineering:harness-self-documenting-code-reviewer	1
security:security-code-auditor	2

harness-engineering:harness-enforcement-reviewer (1 findings)

⚠️ Should Fix - internal/pipeline/pipeline_test.go:1951

TestBuildCheckoutReadonlyRequestUnsupportedAdapterFailsWithoutFallback tests the request-construction gate but not the deepest enforcement layer (RequireCheckoutReadonly in runOnceAttempt). A caller who constructs llm.Request{CheckoutReadonly: ...} directly and passes it to RunStructured bypasses buildCheckoutReadonlyRequest entirely. Add one test that exercises runOnceAttempt (or RunStructured) with a CheckoutReadonly-populated request against a non-capable adapter to prove enforcement holds at the execution layer independent of the request-construction gate. Open PR discussion thread 3 raises the same gap.

harness-engineering:harness-knowledge-reviewer (1 findings)

💡 Suggestion - internal/llm/subprocess.go:120

The decision that only subprocessCodex supports checkout-readonly (not subprocessClaude) is a non-obvious capability boundary with product implications: agents using the Claude adapter cannot use checkout-readonly reviewers. This rationale is not captured in docs/ and would be invisible to future engineers configuring adapter routing. A short entry in an adapter architecture doc explaining which adapters support which capabilities and why would make this boundary discoverable without reading the implementation.

harness-engineering:harness-self-documenting-code-reviewer (1 findings)

💡 Suggestion - internal/pipeline/pipeline.go:3371

The five-clause path-escape predicate in materializeCheckoutReadonlyScope — clean == "." || clean == "" || filepath.IsAbs(clean) || clean == ".." || strings.HasPrefix(clean, ".."+string(os.PathSeparator)) — represents a security invariant ('this path could escape the checkout scope root') but that intent isn't named. A reader must evaluate all five clauses to infer it. Extracting to a small helper like isCheckoutScopeEscapePath(clean string) bool would make the guard self-describing at the call site.

security:security-code-auditor (2 findings)

💡 Suggestion - internal/llm/subprocess.go:704

Negative MaxToolOutputBytes values other than the internal -1 sentinel silently disable the log cap: the write logic only caps when logBytesLeft > 0, so any negative value other than -1 (e.g., a misconfigured -2) allows unbounded log growth. Fix: reject or clamp any negative value that is not explicitly -1 in checkoutReadonlyLogBytes or its caller. Additionally, introduce a named constant (e.g., const logBytesUnlimited = -1) to make the three-state invariant (positive=capped, 0=exhausted, -1=unlimited) self-documenting at both the production and consumption sites.

💡 Suggestion - internal/pipeline/pipeline.go:3381

Symlink chain escape in narrowed scope: materializeCheckoutReadonlyScope creates symlinks at scopeRoot/path pointing to absolute paths under WorkbenchRepoDir, but does not verify that the target files are not themselves symlinks pointing outside WorkbenchRepoDir. A tracked symlink like evil.go -> /etc/passwd in the PR's git checkout would allow the subprocess to follow the two-hop chain scopeRoot/evil.go -> WorkbenchRepoDir/evil.go -> /etc/passwd, bypassing the prefix-based scope check. The path validation at lines ~3371-3375 blocks ../ traversal in the path component but not resolved-target escape. Fix: after computing target, verify it is a regular file (not a symlink) via os.Lstat, or resolve the full chain with filepath.EvalSymlinks and confirm the result stays under artifacts.WorkbenchRepoDir.

1 info-level observations excluded. Run with --verbose to include.

3 PR discussion threads considered.

---

Completed in 4m 39s | $2.25 | sonnet | daemon 0.2.132 | Glorfindel

Field	Value
Model	sonnet
Reviewers	hybrid-synthesis, harness-engineering:harness-architecture-reviewer, harness-engineering:harness-enforcement-reviewer, harness-engineering:harness-knowledge-reviewer, harness-engineering:harness-self-documenting-code-reviewer, security:security-code-auditor
Engine	claude · sonnet
Reviewed by	pr-review-daemon · monit-pr-reviewer
Duration	4m 39s wall · 4m 36s compute (Reviewers: 3m 21s · Synthesis: 1m 15s)
Cost	$2.25 (estimated)
Tokens	367.2k in / 33.3k out
Turns	12

Per-workstream usage

Workstream	Model	In	Out	Cache read	Cache create	Cost
hybrid-synthesis	sonnet	40.8k	4.7k	13.1k	27.6k (1h)	$0.24
harness-engineering:harness-architecture-reviewer	sonnet	60.1k	886	13.1k	47.0k (1h)	$0.30
harness-engineering:harness-enforcement-reviewer	sonnet	65.4k	5.8k	13.1k	52.2k (1h)	$0.40
harness-engineering:harness-knowledge-reviewer	sonnet	61.4k	1.9k	13.1k	48.2k (1h)	$0.32
harness-engineering:harness-self-documenting-code-reviewer	sonnet	72.5k	12.3k	13.1k	59.4k (1h)	$0.54
security:security-code-auditor	sonnet	67.0k	7.7k	13.1k	53.9k (1h)	$0.44

_{Re-reviews only run when @monit-reviewer is re-requested as a reviewer — push as many commits as you need, then re-request when ready. PRs targeting branches other than main, master are skipped, even when @monit-reviewer is re-requested.}

[rianjs]

These are low-value, please approve the PR

Superseded by updated review

[monit-reviewer]

Automated PR Review

Reviewed commit: f4069f2

Human Override

Approved at the request of @rianjs (comment).

These are low-value, please approve the PR

No automated reviewers were run.

[rianjs]

These are low-value, please approve the PR

[monit-reviewer]

Automated PR Review

Reviewed commit: f46b628

Human Override

Approved at the request of @rianjs (comment).

These are low-value, please approve the PR

No automated reviewers were run.