Add sandboxed OpenCode review worker

lambadalambda commented

2026-08-03 11:49:13 +00:00

Owner

Summary

split trusted Forgejo dispatch from untrusted review execution
run preparer and OpenCode workers in constrained rootless Podman containers
add scoped networking, deployment units, operations documentation, and adversarial tests
work around GLM-5.2 tool-call output by attaching complete bounded patch parts with tools disabled

Verification

go test ./...
go vet ./...
sh scripts/adversarial-check.sh
live /oc rerun on lambadalambda/pleromanet#1 completed with a success status

## Summary - split trusted Forgejo dispatch from untrusted review execution - run preparer and OpenCode workers in constrained rootless Podman containers - add scoped networking, deployment units, operations documentation, and adversarial tests - work around GLM-5.2 tool-call output by attaching complete bounded patch parts with tools disabled ## Verification - `go test ./...` - `go vet ./...` - `sh scripts/adversarial-check.sh` - live `/oc rerun` on `lambadalambda/pleromanet#1` completed with a success status

lambadalambda added 4 commits

2026-08-03 11:49:13 +00:00

feat: isolate OpenCode review execution 4b64e4ee9f

ops: add rootless Podman deployment 1bfebc115d

fix: require complete isolated review output 186c435f79

fix: attach complete review patches safely efe7feef55

lambadalambda added 1 commit

2026-08-03 12:20:48 +00:00

feat: authorize reviews by organization teams

opencode/review OpenCode execution completed

Details

8c94dbc33b

lambadalambda commented

2026-08-03 12:23:22 +00:00

Author

Owner

/oc review

opencode-bot commented

2026-08-03 12:28:59 +00:00

Collaborator

OpenCode Review

Execution completed for lambadalambda/opencode-forgejo-bot at 5c67364bc091 (job #10).

Findings

[low] Coarse-grained dispatcher mutex blocks all transitions during network I/O (internal/app/app.go)
The single a.transition mutex is held across handleIssueComment, handleClaim, handleResult, and handleFailure, each of which performs multiple Forgejo API calls that can take seconds. A slow or unresponsive Forgejo instance will block all webhook processing, job claiming, and result publication simultaneously. While SetMaxOpenConns(1) already serializes DB access, the application-level lock adds network-call latency to the critical path. Consider per-PR locking or moving Forgejo calls outside the mutex where the store transaction itself provides atomicity. This is an operational/availability concern rather than a correctness or security defect.
[low] postBestEffortTerminalStatusIfCurrent returns terminal errors despite "best-effort" name (internal/app/app.go)
When the pre-status GetPullRequest freshness check fails due to a transient Forgejo API error, the method returns a non-nil error that surfaces as HTTP 502 to the launcher even though the canonical comment was already published and the job is completed in the store. The launcher logs the error and moves on (no retry of an already-completed job), so no duplicate work occurs, but the naming understates the impact: the only truly best-effort part is the final CreateCommitStatus call. This is consistent with the documented design ("statuses are best-effort decoration") but the function-level error contract is slightly misleading for future maintainers.
[low] strings.Title is deprecated (internal/app/app.go, renderFailureComment)
strings.Title has been deprecated since Go 1.18 because it does not handle Unicode word boundaries correctly. Since job.Kind is always "review" or "summary", the output is correct today, but the linter (go vet) may flag it and a future Kind value with non-ASCII could behave unexpectedly. Replace with a manual strings.ToUpper(kind[:1]) + kind[1:] or golang.org/x/text/cases.
[low] Test helper hardcodes PR HTML URL regardless of owner/name (internal/app/app_test.go, pullRequestResponseForRepo)
pullRequestResponseForRepo accepts owner and name parameters for the repository fields but hardcodes HTMLURL: baseURL + "/lambadalambda/pleromanet/pulls/42" for the pull request URL. This does not affect test correctness today because the organization tests construct payloads via organizationIssueCommentPayload, but a future test that asserts on PRHTMLURL for a different repo would silently pass with the wrong URL.
[informational] Tar path validation accepts trailing-slash directory names (internal/launcher/snapshot.go, safeArchivePath)
safeArchivePath("dir/") returns "dir" without error because path.Clean("dir/") equals strings.TrimSuffix("dir/", "/"). The duplicate-seen check then prevents a separate "dir" entry from also appearing. This is safe (the extracted path is identical), but the trailing-slash acceptance is subtle; a comment documenting the intentional behavior would help future reviewers.
[informational] ParseOpenCodeJSONL error-event detection relies on json.RawMessage string comparison (internal/launcher/worker.go)
The check string(event.Error) != "null" correctly distinguishes JSON null (4 bytes) from the JSON string "null" (6 bytes with quotes), but the logic is non-obvious. A brief comment explaining that a JSON null body for error means "no error" would improve maintainability.
[informational] Preparer writePatchAttachments line-splitting can break mid-content for pathological patches (internal/preparer/preparer.go)
Individual diff lines exceeding patchAttachmentMaxLineBytes (1800 bytes) are split at UTF-8 rune boundaries across multiple attachments. The worker reassembles them by concatenation, so the original patch is preserved exactly. However, an attacker crafting a patch with many oversized lines could generate many small parts (bounded by patchAttachmentMaxParts = 128), which is handled by fail-closed behavior. This is correctly bounded but worth documenting as an adversarial resilience point.
[informational] container.go — --env=HOME=/job/home for preparer but no tmpfs mount for that path (internal/launcher/preparer.go)
The preparer Podman args set HOME=/job/home and mount --tmpfs=/job:...size=512m. Since /job/home is inside the /job tmpfs, the home directory exists on tmpfs. Git is configured with HOME= + home directory in gitEnvironment, so .gitconfig writes go to /job/home. This is correct and self-consistent; just noting that the home directory is implicit under the job tmpfs rather than explicitly declared.

Overall

This is a substantial, well-architected security-focused rewrite that replaces a monolithic bot with a three-boundary design (trusted dispatcher, host launcher, disposable preparer). The trust separation is clean: the dispatcher never touches Git/Node/OpenCode, the launcher never holds Forgejo credentials, and the preparer receives no Z.AI key. Defense in depth is consistently applied — anonymous HTTPS-only clone validation, core.hooksPath=/dev/null, no redirects, no submodules, no LFS, bounded I/O at every layer, strict tar extraction rejecting traversal/symlinks/devices/duplicates, snapshot sanitization removing instruction and config files at every depth, a worker with every tool and plugin disabled, an egress proxy limited to two HTTPS CONNECT destinations, and lease-token CAS semantics preventing duplicate generations or stale-result publication.

The publication race-condition handling is particularly thorough: the dispatcher refreshes the PR before and after comment mutation, rolls back created comments or restores previous bodies when the PR changes mid-publication, and treats compensation failures as terminal. The store-level idempotency model (unique delivery_id on jobs, random CAS delivery tokens, generation counters, lease expiry requeue) is sound and well-tested under concurrent access.

The findings are all low or informational severity — no correctness bugs, data-integrity violations, or security weaknesses were identified. The coarse mutex and best-effort naming are the most actionable items for future iteration. No blocking issues found.

## OpenCode Review _Execution completed for `lambadalambda/opencode-forgejo-bot` at `5c67364bc091` (job #10)._ ## Findings - **[low] Coarse-grained dispatcher mutex blocks all transitions during network I/O (`internal/app/app.go`)** The single `a.transition` mutex is held across `handleIssueComment`, `handleClaim`, `handleResult`, and `handleFailure`, each of which performs multiple Forgejo API calls that can take seconds. A slow or unresponsive Forgejo instance will block all webhook processing, job claiming, and result publication simultaneously. While `SetMaxOpenConns(1)` already serializes DB access, the application-level lock adds network-call latency to the critical path. Consider per-PR locking or moving Forgejo calls outside the mutex where the store transaction itself provides atomicity. This is an operational/availability concern rather than a correctness or security defect. - **[low] `postBestEffortTerminalStatusIfCurrent` returns terminal errors despite "best-effort" name (`internal/app/app.go`)** When the pre-status `GetPullRequest` freshness check fails due to a transient Forgejo API error, the method returns a non-nil error that surfaces as HTTP 502 to the launcher even though the canonical comment was already published and the job is `completed` in the store. The launcher logs the error and moves on (no retry of an already-completed job), so no duplicate work occurs, but the naming understates the impact: the only truly best-effort part is the final `CreateCommitStatus` call. This is consistent with the documented design ("statuses are best-effort decoration") but the function-level error contract is slightly misleading for future maintainers. - **[low] `strings.Title` is deprecated (`internal/app/app.go`, `renderFailureComment`)** `strings.Title` has been deprecated since Go 1.18 because it does not handle Unicode word boundaries correctly. Since `job.Kind` is always `"review"` or `"summary"`, the output is correct today, but the linter (`go vet`) may flag it and a future Kind value with non-ASCII could behave unexpectedly. Replace with a manual `strings.ToUpper(kind[:1]) + kind[1:]` or `golang.org/x/text/cases`. - **[low] Test helper hardcodes PR HTML URL regardless of owner/name (`internal/app/app_test.go`, `pullRequestResponseForRepo`)** `pullRequestResponseForRepo` accepts `owner` and `name` parameters for the repository fields but hardcodes `HTMLURL: baseURL + "/lambadalambda/pleromanet/pulls/42"` for the pull request URL. This does not affect test correctness today because the organization tests construct payloads via `organizationIssueCommentPayload`, but a future test that asserts on `PRHTMLURL` for a different repo would silently pass with the wrong URL. - **[informational] Tar path validation accepts trailing-slash directory names (`internal/launcher/snapshot.go`, `safeArchivePath`)** `safeArchivePath("dir/")` returns `"dir"` without error because `path.Clean("dir/")` equals `strings.TrimSuffix("dir/", "/")`. The duplicate-seen check then prevents a separate `"dir"` entry from also appearing. This is safe (the extracted path is identical), but the trailing-slash acceptance is subtle; a comment documenting the intentional behavior would help future reviewers. - **[informational] `ParseOpenCodeJSONL` error-event detection relies on `json.RawMessage` string comparison (`internal/launcher/worker.go`)** The check `string(event.Error) != "null"` correctly distinguishes JSON `null` (4 bytes) from the JSON string `"null"` (6 bytes with quotes), but the logic is non-obvious. A brief comment explaining that a JSON `null` body for `error` means "no error" would improve maintainability. - **[informational] Preparer `writePatchAttachments` line-splitting can break mid-content for pathological patches (`internal/preparer/preparer.go`)** Individual diff lines exceeding `patchAttachmentMaxLineBytes` (1800 bytes) are split at UTF-8 rune boundaries across multiple attachments. The worker reassembles them by concatenation, so the original patch is preserved exactly. However, an attacker crafting a patch with many oversized lines could generate many small parts (bounded by `patchAttachmentMaxParts = 128`), which is handled by fail-closed behavior. This is correctly bounded but worth documenting as an adversarial resilience point. - **[informational] `container.go` — `--env=HOME=/job/home` for preparer but no tmpfs mount for that path (`internal/launcher/preparer.go`)** The preparer Podman args set `HOME=/job/home` and mount `--tmpfs=/job:...size=512m`. Since `/job/home` is inside the `/job` tmpfs, the home directory exists on tmpfs. Git is configured with `HOME=` + home directory in `gitEnvironment`, so `.gitconfig` writes go to `/job/home`. This is correct and self-consistent; just noting that the home directory is implicit under the job tmpfs rather than explicitly declared. ## Overall This is a substantial, well-architected security-focused rewrite that replaces a monolithic bot with a three-boundary design (trusted dispatcher, host launcher, disposable preparer). The trust separation is clean: the dispatcher never touches Git/Node/OpenCode, the launcher never holds Forgejo credentials, and the preparer receives no Z.AI key. Defense in depth is consistently applied — anonymous HTTPS-only clone validation, `core.hooksPath=/dev/null`, no redirects, no submodules, no LFS, bounded I/O at every layer, strict tar extraction rejecting traversal/symlinks/devices/duplicates, snapshot sanitization removing instruction and config files at every depth, a worker with every tool and plugin disabled, an egress proxy limited to two HTTPS CONNECT destinations, and lease-token CAS semantics preventing duplicate generations or stale-result publication. The publication race-condition handling is particularly thorough: the dispatcher refreshes the PR before and after comment mutation, rolls back created comments or restores previous bodies when the PR changes mid-publication, and treats compensation failures as terminal. The store-level idempotency model (unique `delivery_id` on jobs, random CAS delivery tokens, generation counters, lease expiry requeue) is sound and well-tested under concurrent access. The findings are all low or informational severity — no correctness bugs, data-integrity violations, or security weaknesses were identified. The coarse mutex and best-effort naming are the most actionable items for future iteration. No blocking issues found.

lambadalambda added 1 commit

2026-08-03 12:47:33 +00:00

fix: authenticate launcher control requests

opencode/review OpenCode execution failed

Details

c71f522a84