[Alajmah]

Final branch of the #425 isolation chain (supersedes #428–#444; identical to the P1-correcting head plus the writer-census pin shift its CI exposed: the authority-boundary block adds 36 lines above the propose( call site, pin 527 → 563).

P1 — untrusted sandbox output can no longer authorize GitHub mutations

The OS sandbox protects the process, but the serialized result carried matched rules with action definitions, and the service executed them under GitWire's GitHub App credentials — plugin code (which controls the sandbox process) could fabricate actions never declared in the resolved config. The correction keeps the OS isolation and changes only the authority boundary, per the required contract:

• The sandbox returns matched rule identifiers only (matchedRuleNames); never action definitions.
• The app validates every identifier against the trusted, resolved config.custom_rules (own-property checks); unknown identifiers, duplicates, non-string entries, non-list results all fail closed with typed errors.
• Actions are reconstructed exclusively from the trusted config entries (rule.run — the same source the local path uses).
• The contract distinction is explicit: a plugin may still influence which configured rules match (its filters' intended function; results not intrinsically trustworthy), but cannot invent actions outside the configured policy.

Adversarial regressions (5) — propose is the gate every mutation must pass, so zero propose calls proves no mutation path: fabricated rich-object response with an undeclared close-issue mutation → zero proposals/executions; undeclared identifier → fail closed; duplicate identifier → fail closed; non-list result → fail closed; the smuggling case (declared identifier + inline actions) → malformed form rejected outright, conforming form proposes exactly the trusted config's action (actionType/source asserted), never the smuggled one.

Also fixed here

The playground plugin path returns evaluated_at (shape now matches the local path, pinned by test), and the Wave-2 writer census pin tracks the boundary block's line shift.

Everything else unchanged

Frozen objective (zero @gitwire/rules/plugins imports in the app, structurally asserted), narrowly typed /v1/plugin-eval, inspection and execution by immutable ref@digest, full hardened flag set with SIGKILL plus named-container kill/remove, partial-setup cleanup with injected-failure regressions, named-expression semantics, both operator gates default-off, production wiring complete, errexit-safe assertions, eight deploy-path regressions, four advisory bumps keeping the audit gate green.

Verification: executor 152/152; web suites green incl. the five adversarial tests and the updated census; nine live proofs ALL PASSED locally against the rebuilt image (identifiers-only protocol end-to-end through the real container) and reproducing in CI. CodeQL #12 stays open until merge+deploy.

[Alajmah]

/gitwire run review

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-08T15:44:50.642156Z	21a5e8a	PR opened

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[gitwire-hq]

▶️ GitWire: Re-evaluation triggered for review. Results will appear shortly.

[gitwire-hq]

🤖 AI Code Review — 💬 Needs discussion

Confidence: medium · Findings: 7
AI judgment: NEEDS DISCUSSION · Evidence: COMPLETE · Authority: advisory
Evidence complete · 28/28 changed files accounted for

Key issues

• [HIGH] No finding addresses sandbox network egress — default bridge networking lets a hostile plugin exfiltrate input.json and reach cloud metadata 🔍 ⚠️ unverified

Other findings (6)

• [medium] Sandbox containers have no runtime bound outside the executor process; an executor crash orphans a busy-looping container
• [low] spawnSync maxBuffer overrun (>16 MB stdout) is misreported as sandbox_timeout, never reaching the 256 KB output-limit check
• [low] Evaluation workspace made world-readable (0755/0644) instead of chowned to the sandbox uid
• [medium] No pids-limit: fork bomb DoSes the executor host even with CPU/mem caps ⚠️ unverified
• [medium] Verdict/attestation artifacts read back from the plugin-writable workspace could be forged by the plugin itself ⚠️ unverified

🔮 Devil's Advocate (3 turns): 4 missed risks found · 3 upheld

---

GitWire AI Review Gate (bundle-driven v2) · Structured schema · Scope-validated · Devil's Advocate · AI recommendation — maintainers and repository policy retain authority

Finding details (not annotatable inline)

• [HIGH] No finding addresses sandbox network egress — default bridge networking lets a hostile plugin exfiltrate input.json and reach cloud metadata

  The review litigates CPU/mem caps, wall-clock bounds, and file modes, but never asks whether docker run sets --network none (or an allowlist proxy). If it doesn't, a malicious plugin reads the world-readable input.json flagged in finding #2 and POSTs it to attacker infrastructure, or SSRFs 169.254.169.254 for cloud credentials. Egress isolation is the first-order control for a sandbox whose entire purpose is running untrusted plugin code — its total absence from three findings is the review's biggest blind spot. If --network none is present, drop this; but it had to be asked.

• [MEDIUM] No pids-limit: fork bomb DoSes the executor host even with CPU/mem caps

  Finding #0 treats the container as 'bounded' via CPU/memory limits, but cgroup CPU/mem quotas do not bound process count. Without --pids-limit 128 (or equivalent), a plugin that forks in a loop exhausts host PIDs, wedging the executor and every co-located process — the same unbounded-duration availability gap as finding #0, via a vector the review never checked.

• [MEDIUM] Verdict/attestation artifacts read back from the plugin-writable workspace could be forged by the plugin itself

  The sandbox runs as uid 1000 on a bind mount it must be able to write (to emit plugin output). Findings #1 and #2 debate whether attestations are correctly labeled, but not whether they're tamper-proof: if the executor reads any result/verdict file back from the plugin-writable mount instead of deriving the verdict solely from spawnSync exit codes/signals it captured itself, a malicious plugin overwrites the verdict file and self-attests success. Worth an explicit confirmation of data-flow direction.

• [MEDIUM] Workspace path uniqueness under concurrent evaluations unverified — cross-run leakage of policy config and event payloads

  Finding #2's chmod implies a shared/mounted workspace, but no finding asks whether the workspace path is mkdtemp-unique per run. If it's deterministic (e.g., /tmp/plugin-eval), two simultaneous evaluations read or clobber each other's input.json — a cross-repository data leak and integrity bug that the file-permission finding implicitly assumes away. One-line verification; drop if mkdtemp is confirmed.

[gitwire-hq]

[MEDIUM] Sandbox containers have no runtime bound outside the executor process; an executor crash orphans a busy-looping container

The only wall-clock enforcement for the sandbox container lives in the executor process (spawnSync timeout plus the explicit docker kill/rm follow-up in runPluginEvaluation). plugin-sandbox/runner.mjs has no internal deadline, and a synchronous plugin loop (the exact pattern Proof 6 exercises: while (true) {}) also blocks the node event loop, so an in-process watchdog inside the sandbox cannot preempt it. Docker itself applies no max-runtime flag to docker run. If the executor service dies or restarts between spawning the container and reaching the kill/remove path, the orphaned container persists indefinitely (bounded CPU/memory via limits, unbounded duration) until an operator cleans it up; no sweep/label-based reaper exists anywhere in this diff. Suggest an external bound: wrap the ENTRYPOINT in a timeout-style supervisor inside the image, or label eval containers and sweep stale ones on executor boot.

[gitwire-hq]

[LOW] spawnSync maxBuffer overrun (>16 MB stdout) is misreported as sandbox_timeout, never reaching the 256 KB output-limit check

In runCmd, isTimeout = r.signal != null || r.error.code === 'ETIMEDOUT'. When sandbox stdout floods past the 16 MB spawnSync maxBuffer, node kills the child (error.code ENOBUFS, signal SIGKILL), so the runner classifies the result as sandbox_timeout ('exceeded 10000ms wall clock and was killed') and the explicit output_limit_exceeded branch at 256 KB is unreachable for large floods. The typed failure attestation shown to operators/debuggers is wrong on precisely the adversarial path (output flooding) it exists to describe. Behavior remains fail-closed, so this is misreporting, not a security hole. Suggest classifying r.error.code === 'ENOBUFS' as an output-limit failure and including r.error.code in fail_detail.

[gitwire-hq]

[LOW] Evaluation workspace made world-readable (0755/0644) instead of chowned to the sandbox uid

To let uid 1000 traverse the bind mount, the runner chmods the workspace to 0755 and input.json to 0644. This makes the serialized evaluation input — the full resolved repo policy config, the event payload ctx, and plugin sources — readable by every local principal on the executor host for the duration of each run. The stated goal (sandbox uid can read the input) is achievable without world readability: chown the workspace and input file to 1000:1000 and keep 0700/0600 modes, which preserves the same Linux bind-mount permission semantics. Low practical exposure in a dedicated executor container, but it is a needless widening on a boundary this PR is otherwise hardening.

[Alajmah]

Adjudication — GitWire AI Review @ 21a5e8a (NEEDS DISCUSSION, medium confidence, 7 findings)

Disposition: none blocking; the HIGH repeats a claim already refuted with code + CI evidence on this chain.

• [HIGH] "no network egress assessment — default bridge networking" — refuted, standing evidence. --network=none is the first isolation flag in the fixed buildRunArgv, pinned by the unit test "runs with the full isolation flag set", attested per report in isolation.network_disabled, and live-proven in CI on this very PR: the Plugin Sandbox Proofs run shows network access is denied inside the sandbox (network="blocked:fetch failed"). The cloud-metadata and exfiltration scenarios die at a namespace with no interfaces; and input.json contains only data the request itself supplied.
• [medium] orphaned container on executor crash — recorded executor-hardening follow-up (adjudicated on three prior rounds); leaked containers are resource-capped at 256m/1cpu/64pids, and --rm plus the named kill/remove path covers every non-crash exit.
• [low] maxBuffer misclassification — recorded follow-up (fail-closed either way); [low] world-readable workspace — validator precedent (0777 in production for the same bind-mount reason; contents are request-supplied evaluation data, never credentials). The remaining findings restate the standing set (spawnSync serialization, ENOBUFS taxonomy test, 502-vs-400 polish).

Gate state: all 25 checks green — nine live proofs plus deploy-preflight regressions in CI on this head, audit gate green, mergeStateStatus: CLEAN. The P1 authority boundary is in place with five adversarial regressions. Head 21a5e8a7400c7c91bb92872b548f2ec643b73cd9 returned for maintainer clearance.