coilyco-flight-deck/ward

Fork 0

Evaluate replacing in-container review with brokered QA roles #792

New issue

Closed

opened 2026-07-09 09:09:04 +00:00 by coilyco-ops · 3 comments

coilyco-ops commented

2026-07-09 09:09:04 +00:00

Member

Question

Should ward deprecate the in-container review gate in favor of brokered QA roles?

Context

A Codex engineer run hit the review gate and failed closed because no reviewer family was runnable inside the worker container. The immediate issues are tracked as:

ward#790 - blocked no-reviewer output still says advisory-only.
ward#791 - Codex reviewer launch probe fails before same-family review can run.

The larger concern is architectural: in-container review may be asking the worker container to host peer harnesses in an environment that is intentionally minimized and often missing the exact auth, stdin, model, endpoint, or local-service setup those harnesses need.

Hypothesis to evaluate

Replace or demote ward agent review as a pre-landing in-container gate. Instead, introduce a brokered QA role or workflow where the host dispatch broker launches one or more fresh QA containers against the exact branch/commit under review, posts structured review verdicts, and lets the director or engineer landing workflow gate on those durable QA outcomes.

Potential shape:

warded qa owner/repo#N or an internal qa role launched by the engineer/director.
QA gets a fresh clone at the candidate branch/commit, the issue contract, CI output, and the diff.
QA writes a structured issue comment or check-like marker with pass/block, reason, confidence, reviewed commit, and reviewer family.
The engineer/director gates merge on CI green AND QA passed for this commit.
In-container ward agent review becomes optional diagnostic / local fast path, or is removed after a transition.

Analyze

Please produce a design recommendation that covers:

Whether to deprecate in-container review, keep it, or run a hybrid.
Failure modes of each option: auth, stdin/TTY, stale branch, TOCTOU between QA and merge, duplicate comments, reservation conflicts, cost/latency, and local-model endpoints.
How brokered QA would prove it reviewed the exact commit that later lands.
How director merge eligibility should consume QA outcomes.
What new role/subcommand or data model is needed, if any.
A migration plan that avoids weakening the current fail-closed quality gate.
Minimum implementation slices and tests.

Acceptance

The advisor comment makes a clear recommendation, not just a pros/cons list.
The recommendation names the smallest viable next implementation issue.
It identifies any cases where in-container review should remain useful.
It calls out what must be true before existing review gates can be deprecated.

Codex, via director surface

## Question Should ward deprecate the in-container review gate in favor of brokered QA roles? ## Context A Codex engineer run hit the review gate and failed closed because no reviewer family was runnable inside the worker container. The immediate issues are tracked as: * ward#790 - blocked no-reviewer output still says advisory-only. * ward#791 - Codex reviewer launch probe fails before same-family review can run. The larger concern is architectural: in-container review may be asking the worker container to host peer harnesses in an environment that is intentionally minimized and often missing the exact auth, stdin, model, endpoint, or local-service setup those harnesses need. ## Hypothesis to evaluate Replace or demote `ward agent review` as a pre-landing in-container gate. Instead, introduce a brokered QA role or workflow where the host dispatch broker launches one or more fresh QA containers against the exact branch/commit under review, posts structured review verdicts, and lets the director or engineer landing workflow gate on those durable QA outcomes. Potential shape: * `warded qa owner/repo#N` or an internal `qa` role launched by the engineer/director. * QA gets a fresh clone at the candidate branch/commit, the issue contract, CI output, and the diff. * QA writes a structured issue comment or check-like marker with pass/block, reason, confidence, reviewed commit, and reviewer family. * The engineer/director gates merge on `CI green AND QA passed for this commit`. * In-container `ward agent review` becomes optional diagnostic / local fast path, or is removed after a transition. ## Analyze Please produce a design recommendation that covers: * Whether to deprecate in-container review, keep it, or run a hybrid. * Failure modes of each option: auth, stdin/TTY, stale branch, TOCTOU between QA and merge, duplicate comments, reservation conflicts, cost/latency, and local-model endpoints. * How brokered QA would prove it reviewed the exact commit that later lands. * How director merge eligibility should consume QA outcomes. * What new role/subcommand or data model is needed, if any. * A migration plan that avoids weakening the current fail-closed quality gate. * Minimum implementation slices and tests. ## Acceptance * The advisor comment makes a clear recommendation, not just a pros/cons list. * The recommendation names the smallest viable next implementation issue. * It identifies any cases where in-container review should remain useful. * It calls out what must be true before existing review gates can be deprecated.  Codex, via director surface

coilyco-ops commented

2026-07-09 09:13:24 +00:00

Author

Member

🔎 ward agent advisor

ward agent advisor --harness codex ran a one-shot deep research pass on this question:

Read the issue title, body, and comment thread below as the research brief. Identify the decisions, options, open questions, and risks, then give a clear advisory recommendation that can be posted back on the issue.

Additional framing from the dispatcher:
Make a concrete recommendation on deprecating in-container review in favor of brokered QA roles. Be opinionated and identify the first implementation slice.

Recommendation

Adopt a hybrid transition with brokered QA becoming the authoritative merge gate for PR-based landing, and do not fully deprecate in-container review yet.

The concrete recommendation is:

Keep the current in-container review fail-closed for now as the safety floor.
Add a brokered, commit-bound QA path for pull-requests-and-merge first and make director merge consume that artifact, not just the worker's review summary: string.
Once that path is stable, demote in-container review from required gate to optional fast-path / diagnostic for PR workflows.
Only after parity is proven should ward consider removing the in-container gate for those workflows.

If ward tries to replace the current gate immediately, it risks swapping a known-bad failure mode for a larger unfinished control-plane gap. But the repo evidence is strong that the long-term primary gate should move out of the worker container.

Why this is the right direction

What the code says today

The current review gate is explicitly in-container, wired into the engineer seed, and runs after CI green and before PR open / merge. See docs/dispatch-review.md and cmd/ward/agent_review.go.
Reviewer availability is decided inside the worker container by reviewerAvailable, which requires both the reviewer binary and its launch probe to pass in that environment. See cmd/ward/agent_review.go.
For Codex specifically, the launch probe is a real codex exec ... "Reply with exactly ok." subprocess with /dev/null stdin and a 30s timeout. See internal/agents/codex/smoke.go.
ward#791 shows that this is already brittle in practice: the Codex probe reached the CLI, printed the startup banner, then entered a stdin-reading path ("Reading additional input from stdin...") and got classified as unavailable.
ward#790 shows a second symptom of the same architecture: when no reviewer can run, the panel is converted to a fail-closed block, but user-facing text still says ADVISORY-ONLY REVIEW. That contradiction comes from internal/reviewpanel/panel.go plus the advisory-to-block conversion in cmd/ward/agent_review.go.
Director merge currently trusts a string parsed from the latest WARD-OUTCOME comment. directorMergeEligibility reads workflow: and review summary: from the latest outcome comment and only checks that the review summary starts with passed:. See cmd/ward/agent_director_merge.go and parseDirectorRunMeta in cmd/ward/agent_director.go.
That means the current merge lane has no durable proof of which commit was actually reviewed.
The Forgejo client already has evidence that commit binding is available but unused: the checked-in swagger lock includes head_sha, while listOpenPullRequests currently projects PRs down to a lean issue shape and drops that data. See cmd/ward/opsassets/forgejo.swagger.lock.generated.json and cmd/ward/forgejo_ops.go.

Architectural conclusion

The worker container is the wrong place to be the only review authority.

Its job is to build the candidate change. Asking that same minimized environment to also host peer harnesses, credentials, local-model endpoints, and harness-specific stdin/TTY assumptions is exactly the coupling that ward#791 exposed. A fresh QA container launched by the broker can still fail, but that failure is:

isolated from the worker's runtime,
attributable to the reviewer family actually being tested,
repeatable against a pinned commit,
and durable on the issue / PR thread.

That is a much better boundary for a merge gate.

Option analysis

1. Keep in-container review as the long-term primary gate

I do not recommend this.

Pros:

Reviews the live worktree before anything is pushed.
Cheap for same-family review when the worker harness is already healthy.
Simple synchronous control flow inside one run.

Failure modes:

Auth / model / endpoint coupling: reviewer availability depends on the worker container having the right peer setup.
stdin / TTY drift: exactly the Codex failure in ward#791.
local-model endpoint dependence: opencode/goose-style reviewers depend on endpoint routing that may exist on some runs and not others.
duplicate outcome comments: ward agent review already posts a WARD-OUTCOME comment, and the worker later posts its own final WARD-OUTCOME, which creates noisy, stale metadata surfaces.
weak merge proof: the director later consumes a human-readable summary string, not a commit-bound review artifact.

Conclusion: useful as a local fast path, not strong enough as the only durable gate.

2. Deprecate in-container review immediately and replace it wholesale with brokered QA

I also do not recommend doing this in one cut.

Pros:

Better separation of concerns.
Easier harness-specific auth and probe handling.
Easier exact-commit verification.

Failure modes if done too early:

No existing QA artifact schema yet.
No current merge policy that checks reviewed SHA vs current PR head SHA.
No current broker launch action or role for QA.
No duplicate suppression / supersession rules yet.
No reservation model yet for child QA jobs.

Conclusion: right destination, wrong immediate move.

3. Hybrid: brokered QA becomes authoritative for PR merge, in-container review remains transitional and diagnostic

This is the recommended path.

It keeps today's fail-closed floor while building the correct long-term boundary.

Proposed design

Scope the first authoritative brokered QA lane narrowly

Start with pull-requests-and-merge only.

Reason:

That workflow already has a delayed merge boundary and a director merge sweep.
The PR is already open, so there is a stable branch / commit / CI surface for QA to inspect.
Director merge is where commit-proof matters most.
It avoids blocking direct-main on a brand-new asynchronous control path.

New artifact: a commit-bound QA verdict

Introduce a structured issue or PR comment marker, for example:

WARD-QA: pass
repo: coilyco-flight-deck/ward
issue: #792
pr: #123
reviewed_sha: <40-char sha>
reviewed_branch: issue-792
reviewer_family: codex
workflow: pull-requests-and-merge
confidence: 0.82
reason: ...

The exact wire format can be a small fenced JSON block if preferred, but the important fields are:

reviewed_sha
reviewer_family
verdict
reason
confidence
issue / pr
workflow
timestamp / run identity

Exact-commit proof

The authoritative rule should be:

QA must record the exact commit SHA it reviewed.
Director merge must fetch the current PR head SHA.
Merge is allowed only if there is a passing QA verdict for that exact SHA.
If the branch moves after QA comments, the verdict is stale and merge is blocked until QA reruns.

This closes the TOCTOU gap much better than the current review summary starts with passed: string check.

Director merge eligibility

Change merge eligibility from:

WARD-OUTCOME: done
workflow: pull-requests-and-merge
review summary: passed: ...

To:

WARD-OUTCOME: done
workflow: pull-requests-and-merge
PR head SHA == QA reviewed_sha
latest non-superseded QA verdict for that SHA is pass
required CI checks are green

The human-readable review summary: line can stay in WARD-OUTCOME for readability, but it should become informational, not authoritative.

Role / subcommand shape

Smallest good shape:

add an internal maintenance subcommand first, not a public top-level role:
- ward agent qa <owner/repo#N> --pr <n> --sha <sha> --family <family>
have the dispatch broker grow a narrow qa launch action that starts a fresh QA container against that ref/sha

Why not a public warded qa first:

the public role roster is currently engineer, director, advisor
the control-plane contract is still unsettled
first slice should stabilize the artifact and merge policy before hardening the UX surface

If the flow proves out, it can later graduate to warded qa ....

Failure modes and mitigations

Auth

Current risk: reviewer auth is checked inside the worker container.
Brokered QA: move auth/probe responsibility into the QA run itself.
Mitigation: a QA failure is a fail-closed block, but it is now attributable to reviewer_family=<x> and reviewed_sha=<y>.

stdin / TTY

Current risk: Codex probe already shows CLI behavior drift around stdin.
Brokered QA: still possible, but isolated to the QA harness and easier to reproduce independently.
Mitigation: persist the probe failure as a QA block reason, not a missing reviewer mystery.

Stale branch / TOCTOU

Current risk: current merge lane does not bind review to commit.
Mitigation: merge only if PR head SHA == reviewed_sha.

Duplicate comments

Current risk: ward agent review posts a WARD-OUTCOME comment and the worker later posts another one.
Brokered QA mitigation: use a dedicated WARD-QA: marker, and have policy read the latest matching artifact for the current SHA.
Supersession can be implicit: newer SHA wins, older SHA is stale.

Reservation conflicts

Current risk: QA must not behave like a second engineer trying to reserve the same issue.
Mitigation: QA should be a child review job, not an issue-owning peer run.
It should not take the normal engineer reservation on the issue thread.

Cost / latency

Brokered QA cost is real: new container, new clone, new model call.
Mitigation: first slice only on pull-requests-and-merge, where latency is already acceptable and merge already waits on CI.
Keep same-family single-reviewer QA first. Do not start with multi-family quorum.

Local-model endpoints

Current risk: local-model reviewers depend on worker reachability to Ollama/tailnet routes.
Recommendation: first brokered QA slice should support cloud-backed families only (codex, claude) and treat local-model families as later optional expansion.
That keeps the first slice from depending on tailnet / local endpoint topology.

What should remain useful about in-container review

Even after brokered QA exists, in-container review is still useful as:

a fast local diagnostic during the engineer run,
a same-family preflight when the harness is known-good,
a way to review the live unpushed worktree before a PR exists,
a debugging tool when brokered QA disagrees with the worker.

That makes it a good optional fast-path. It does not need to remain the authoritative merge gate once brokered QA is proven.

Migration plan

Fix the current gate regressions first.
- Land ward#790 and ward#791 so the existing fail-closed path is truthful and same-family Codex review is not spuriously unavailable.
- This avoids weakening quality while the new path is built.
Add the QA artifact without changing merge policy yet.
- New WARD-QA comment schema.
- New parser and tests.
- New Forgejo read path that can see PR head SHA instead of dropping it in listOpenPullRequests.
Add brokered QA for pull-requests-and-merge only.
- One family first.
- Fail closed on missing / stale / blocked QA.
- Director merge begins consuming QA verdicts for the exact PR head SHA.
Demote in-container review for PR workflows only after parity is real.
- Keep it optional / diagnostic.
- Leave direct-main on the old gate until there is a synchronous brokered QA story for that lane.
Only then consider deprecation.

Smallest viable next implementation issue

The smallest viable next issue is:

Add commit-bound QA verdicts and make pull-requests-and-merge director merge require a passing QA verdict for the current PR head SHA.

That issue should include:

a WARD-QA verdict schema,
a Forgejo client read path that exposes PR head SHA,
an internal ward agent qa maintenance subcommand or equivalent broker action for one reviewer family,
director merge policy changes to consume QA verdicts by exact SHA,
regression tests for stale-SHA refusal, missing-QA refusal, and pass-on-exact-match.

This is the right first slice because it creates the durable commit-bound contract the architecture currently lacks. Without that contract, replacing in-container review would just move the fragility elsewhere.

Before deprecating the existing review gate, all of this must be true

Brokered QA can post a durable verdict tied to an exact commit SHA.
Director merge consumes that artifact, not a free-text summary line.
A moved PR head invalidates prior QA automatically.
QA failure modes are fail-closed and actionable.
Duplicate / stale verdicts have deterministic supersession rules.
The first supported reviewer family is reliable enough that normal PR runs are not constantly environment-blocked.
The current fail-closed floor remains in place until the above is proven.

Net: yes, ward should move toward brokered QA as the primary quality gate, but by adding a commit-bound QA lane first and deprecating in-container review only after that lane is real.

Researched and posted automatically by ward agent advisor --harness codex (ward#179). This is one-shot research, not a carried change - verify before acting on it.

— Codex, via ward agent

### 🔎 ward agent advisor `ward agent advisor --harness codex` ran a one-shot **deep** research pass on this question: > Read the issue title, body, and comment thread below as the research brief. Identify the decisions, options, open questions, and risks, then give a clear advisory recommendation that can be posted back on the issue. > > Additional framing from the dispatcher: > Make a concrete recommendation on deprecating in-container review in favor of brokered QA roles. Be opinionated and identify the first implementation slice. --- ## Recommendation Adopt a **hybrid transition with brokered QA becoming the authoritative merge gate for PR-based landing**, and **do not fully deprecate in-container review yet**. The concrete recommendation is: - **Keep the current in-container review fail-closed for now** as the safety floor. - **Add a brokered, commit-bound QA path for `pull-requests-and-merge` first** and make **director merge consume that artifact**, not just the worker's `review summary:` string. - Once that path is stable, **demote in-container review from required gate to optional fast-path / diagnostic for PR workflows**. - Only after parity is proven should ward consider removing the in-container gate for those workflows. If ward tries to replace the current gate immediately, it risks swapping a known-bad failure mode for a larger unfinished control-plane gap. But the repo evidence is strong that **the long-term primary gate should move out of the worker container**. ## Why this is the right direction ### What the code says today - The current review gate is explicitly **in-container**, wired into the engineer seed, and runs **after CI green and before PR open / merge**. See `docs/dispatch-review.md` and `cmd/ward/agent_review.go`. - Reviewer availability is decided **inside the worker container** by `reviewerAvailable`, which requires both the reviewer binary and its launch probe to pass in that environment. See `cmd/ward/agent_review.go`. - For Codex specifically, the launch probe is a real `codex exec ... "Reply with exactly ok."` subprocess with `/dev/null` stdin and a 30s timeout. See `internal/agents/codex/smoke.go`. - `ward#791` shows that this is already brittle in practice: the Codex probe reached the CLI, printed the startup banner, then entered a stdin-reading path (`"Reading additional input from stdin..."`) and got classified as unavailable. - `ward#790` shows a second symptom of the same architecture: when no reviewer can run, the panel is converted to a fail-closed block, but user-facing text still says `ADVISORY-ONLY REVIEW`. That contradiction comes from `internal/reviewpanel/panel.go` plus the advisory-to-block conversion in `cmd/ward/agent_review.go`. - Director merge currently trusts a **string parsed from the latest `WARD-OUTCOME` comment**. `directorMergeEligibility` reads `workflow:` and `review summary:` from the latest outcome comment and only checks that the review summary starts with `passed:`. See `cmd/ward/agent_director_merge.go` and `parseDirectorRunMeta` in `cmd/ward/agent_director.go`. - That means the current merge lane has **no durable proof of which commit was actually reviewed**. - The Forgejo client already has evidence that commit binding is available but unused: the checked-in swagger lock includes `head_sha`, while `listOpenPullRequests` currently projects PRs down to a lean issue shape and drops that data. See `cmd/ward/opsassets/forgejo.swagger.lock.generated.json` and `cmd/ward/forgejo_ops.go`. ### Architectural conclusion The worker container is the wrong place to be the **only** review authority. Its job is to build the candidate change. Asking that same minimized environment to also host peer harnesses, credentials, local-model endpoints, and harness-specific stdin/TTY assumptions is exactly the coupling that `ward#791` exposed. A fresh QA container launched by the broker can still fail, but that failure is: - isolated from the worker's runtime, - attributable to the reviewer family actually being tested, - repeatable against a pinned commit, - and durable on the issue / PR thread. That is a much better boundary for a merge gate. ## Option analysis ### 1. Keep in-container review as the long-term primary gate I do **not** recommend this. Pros: - Reviews the live worktree before anything is pushed. - Cheap for same-family review when the worker harness is already healthy. - Simple synchronous control flow inside one run. Failure modes: - **Auth / model / endpoint coupling**: reviewer availability depends on the worker container having the right peer setup. - **stdin / TTY drift**: exactly the Codex failure in `ward#791`. - **local-model endpoint dependence**: opencode/goose-style reviewers depend on endpoint routing that may exist on some runs and not others. - **duplicate outcome comments**: `ward agent review` already posts a `WARD-OUTCOME` comment, and the worker later posts its own final `WARD-OUTCOME`, which creates noisy, stale metadata surfaces. - **weak merge proof**: the director later consumes a human-readable summary string, not a commit-bound review artifact. Conclusion: useful as a local fast path, not strong enough as the only durable gate. ### 2. Deprecate in-container review immediately and replace it wholesale with brokered QA I also do **not** recommend doing this in one cut. Pros: - Better separation of concerns. - Easier harness-specific auth and probe handling. - Easier exact-commit verification. Failure modes if done too early: - No existing QA artifact schema yet. - No current merge policy that checks reviewed SHA vs current PR head SHA. - No current broker launch action or role for QA. - No duplicate suppression / supersession rules yet. - No reservation model yet for child QA jobs. Conclusion: right destination, wrong immediate move. ### 3. Hybrid: brokered QA becomes authoritative for PR merge, in-container review remains transitional and diagnostic This is the recommended path. It keeps today's fail-closed floor while building the correct long-term boundary. ## Proposed design ### Scope the first authoritative brokered QA lane narrowly Start with **`pull-requests-and-merge` only**. Reason: - That workflow already has a delayed merge boundary and a director merge sweep. - The PR is already open, so there is a stable branch / commit / CI surface for QA to inspect. - Director merge is where commit-proof matters most. - It avoids blocking `direct-main` on a brand-new asynchronous control path. ### New artifact: a commit-bound QA verdict Introduce a structured issue or PR comment marker, for example: ```text WARD-QA: pass repo: coilyco-flight-deck/ward issue: #792 pr: #123 reviewed_sha: <40-char sha> reviewed_branch: issue-792 reviewer_family: codex workflow: pull-requests-and-merge confidence: 0.82 reason: ... ``` The exact wire format can be a small fenced JSON block if preferred, but the important fields are: - `reviewed_sha` - `reviewer_family` - `verdict` - `reason` - `confidence` - `issue` / `pr` - `workflow` - timestamp / run identity ### Exact-commit proof The authoritative rule should be: - QA must record the **exact commit SHA** it reviewed. - Director merge must fetch the **current PR head SHA**. - Merge is allowed only if there is a **passing QA verdict for that exact SHA**. - If the branch moves after QA comments, the verdict is stale and merge is blocked until QA reruns. This closes the TOCTOU gap much better than the current `review summary starts with passed:` string check. ### Director merge eligibility Change merge eligibility from: - `WARD-OUTCOME: done` - `workflow: pull-requests-and-merge` - `review summary: passed: ...` To: - `WARD-OUTCOME: done` - `workflow: pull-requests-and-merge` - PR head SHA == QA `reviewed_sha` - latest non-superseded QA verdict for that SHA is `pass` - required CI checks are green The human-readable `review summary:` line can stay in `WARD-OUTCOME` for readability, but it should become **informational**, not authoritative. ### Role / subcommand shape Smallest good shape: - add an **internal maintenance subcommand** first, not a public top-level role: - `ward agent qa <owner/repo#N> --pr <n> --sha <sha> --family <family>` - have the **dispatch broker** grow a narrow `qa` launch action that starts a fresh QA container against that ref/sha Why not a public `warded qa` first: - the public role roster is currently `engineer`, `director`, `advisor` - the control-plane contract is still unsettled - first slice should stabilize the artifact and merge policy before hardening the UX surface If the flow proves out, it can later graduate to `warded qa ...`. ## Failure modes and mitigations ### Auth - **Current risk**: reviewer auth is checked inside the worker container. - **Brokered QA**: move auth/probe responsibility into the QA run itself. - **Mitigation**: a QA failure is a fail-closed `block`, but it is now attributable to `reviewer_family=<x>` and `reviewed_sha=<y>`. ### stdin / TTY - **Current risk**: Codex probe already shows CLI behavior drift around stdin. - **Brokered QA**: still possible, but isolated to the QA harness and easier to reproduce independently. - **Mitigation**: persist the probe failure as a QA block reason, not a missing reviewer mystery. ### Stale branch / TOCTOU - **Current risk**: current merge lane does not bind review to commit. - **Mitigation**: merge only if `PR head SHA == reviewed_sha`. ### Duplicate comments - **Current risk**: `ward agent review` posts a `WARD-OUTCOME` comment and the worker later posts another one. - **Brokered QA mitigation**: use a dedicated `WARD-QA:` marker, and have policy read the latest matching artifact for the current SHA. - Supersession can be implicit: newer SHA wins, older SHA is stale. ### Reservation conflicts - **Current risk**: QA must not behave like a second engineer trying to reserve the same issue. - **Mitigation**: QA should be a **child review job**, not an issue-owning peer run. - It should not take the normal engineer reservation on the issue thread. ### Cost / latency - **Brokered QA cost is real**: new container, new clone, new model call. - **Mitigation**: first slice only on `pull-requests-and-merge`, where latency is already acceptable and merge already waits on CI. - Keep same-family single-reviewer QA first. Do not start with multi-family quorum. ### Local-model endpoints - **Current risk**: local-model reviewers depend on worker reachability to Ollama/tailnet routes. - **Recommendation**: first brokered QA slice should support **cloud-backed families only** (`codex`, `claude`) and treat local-model families as later optional expansion. - That keeps the first slice from depending on tailnet / local endpoint topology. ## What should remain useful about in-container review Even after brokered QA exists, in-container review is still useful as: - a **fast local diagnostic** during the engineer run, - a **same-family preflight** when the harness is known-good, - a way to review the **live unpushed worktree** before a PR exists, - a debugging tool when brokered QA disagrees with the worker. That makes it a good optional fast-path. It does **not** need to remain the authoritative merge gate once brokered QA is proven. ## Migration plan 1. **Fix the current gate regressions first**. - Land `ward#790` and `ward#791` so the existing fail-closed path is truthful and same-family Codex review is not spuriously unavailable. - This avoids weakening quality while the new path is built. 2. **Add the QA artifact without changing merge policy yet**. - New `WARD-QA` comment schema. - New parser and tests. - New Forgejo read path that can see PR head SHA instead of dropping it in `listOpenPullRequests`. 3. **Add brokered QA for `pull-requests-and-merge` only**. - One family first. - Fail closed on missing / stale / blocked QA. - Director merge begins consuming QA verdicts for the exact PR head SHA. 4. **Demote in-container review for PR workflows only after parity is real**. - Keep it optional / diagnostic. - Leave `direct-main` on the old gate until there is a synchronous brokered QA story for that lane. 5. **Only then consider deprecation**. ## Smallest viable next implementation issue The smallest viable next issue is: **Add commit-bound QA verdicts and make `pull-requests-and-merge` director merge require a passing QA verdict for the current PR head SHA.** That issue should include: - a `WARD-QA` verdict schema, - a Forgejo client read path that exposes PR head SHA, - an internal `ward agent qa` maintenance subcommand or equivalent broker action for one reviewer family, - director merge policy changes to consume QA verdicts by exact SHA, - regression tests for stale-SHA refusal, missing-QA refusal, and pass-on-exact-match. This is the right first slice because it creates the **durable commit-bound contract** the architecture currently lacks. Without that contract, replacing in-container review would just move the fragility elsewhere. ## Before deprecating the existing review gate, all of this must be true - Brokered QA can post a durable verdict tied to an exact commit SHA. - Director merge consumes that artifact, not a free-text summary line. - A moved PR head invalidates prior QA automatically. - QA failure modes are fail-closed and actionable. - Duplicate / stale verdicts have deterministic supersession rules. - The first supported reviewer family is reliable enough that normal PR runs are not constantly environment-blocked. - The current fail-closed floor remains in place until the above is proven. Net: **yes, ward should move toward brokered QA as the primary quality gate, but by adding a commit-bound QA lane first and deprecating in-container review only after that lane is real.** --- Researched and posted automatically by `ward agent advisor --harness codex` (ward#179). This is one-shot research, not a carried change - verify before acting on it.   — Codex, via `ward agent`

coilyco-ops commented

2026-07-09 09:14:36 +00:00

Author

Member

🔎 ward agent advisor

ward agent advisor --harness codex ran a one-shot quick research pass on this question:

Read the issue title, body, and comment thread below as the research brief. Identify the decisions, options, open questions, and risks, then give a clear advisory recommendation that can be posted back on the issue.

Additional framing from the dispatcher:
Make a concrete recommendation on deprecating in-container review in favor of brokered QA roles. Be opinionated and identify the first implementation slice.

Recommendation

Ward should move to a hybrid model, with brokered QA becoming the authoritative merge gate for pull-requests-and-merge, while in-container review stays in place temporarily as the fail-closed safety floor and local diagnostic path.

The recommendation is not to keep the current in-container gate as the long-term authority, and not to rip it out immediately. The worker container is the wrong place to be the only review authority. The repo already shows why:

ward agent review decides reviewer availability inside the worker container via reviewerAvailable(...) and per-family launch probes in cmd/ward/agent_review.go.
Codex availability depends on a real bounded CLI probe with /dev/null stdin in internal/agents/codex/smoke.go, which matches the issue's concern about harness/runtime brittleness.
Director merge eligibility currently trusts a parsed free-text review summary: from the latest WARD-OUTCOME comment in cmd/ward/agent_director_merge.go and cmd/ward/agent_director.go.
The documented merge rule is still effectively: workflow: pull-requests-and-merge + WARD-OUTCOME: done + review summary starts with passed:. See docs/agent-director-pr-merge.md.

That architecture is too weak for a durable merge gate because it couples review authority to the engineer container's auth, stdin/TTY behavior, model endpoint shape, and local runtime. It also does not prove that the reviewed commit is the one later merged.

Why brokered QA should be the destination

A brokered QA run launched as a fresh container against a pinned branch/commit is the better boundary:

Auth and local setup failures are isolated to the QA role instead of contaminating the engineer's landing path.
stdin/TTY quirks become QA-runtime bugs, not mysterious "no reviewer available" outcomes inside the worker.
Local-model endpoints can be supported selectively instead of making every worker container able to host every reviewer family.
The QA result can be made durable and commit-bound, which closes the current TOCTOU gap.

The authoritative rule should become:

QA records the exact commit SHA it reviewed.
Director merge reads the current PR head SHA.
Merge is allowed only when CI is green and there is a passing QA verdict for that exact SHA.
If the branch moves, the QA verdict is automatically stale and merge blocks until QA reruns.

That is the key architectural improvement. Without it, replacing the current gate just moves the fragility.

Recommended shape

1. Keep in-container review for now, but demote its future role

In-container review should remain useful for:

a fast local preflight on the live unpushed worktree,
a diagnostic path when brokered QA fails unexpectedly,
a same-family cheap check when the worker harness is known-good,
workflows that have not yet been migrated to an authoritative brokered gate.

It should not remain the long-term authoritative merge gate for PR-based landing.

2. Add a commit-bound QA artifact

Ward needs a dedicated QA verdict artifact instead of overloading review summary: in WARD-OUTCOME.

A small structured marker is enough, for example a fenced JSON block or a dedicated WARD-QA: header carrying:

reviewed_sha
reviewer_family
verdict (pass or block)
reason
confidence
workflow
issue
pr
timestamp / run identity

The exact wire format matters less than the commit binding and deterministic parsing.

3. Make director merge consume QA, not summary prose

Director merge eligibility should change from:

linked issue ended WARD-OUTCOME: done
workflow is pull-requests-and-merge
review summary: starts with passed:

To:

linked issue ended WARD-OUTCOME: done
workflow is pull-requests-and-merge
PR head SHA equals QA reviewed_sha
latest non-superseded QA verdict for that SHA is pass
required CI is green

The human-readable review summary: can stay for operator context, but it should become informational only.

4. Start narrowly

The first brokered QA slice should target pull-requests-and-merge only.

That is the smallest safe place to introduce the new control plane because:

there is already a delayed merge boundary,
the PR gives a stable branch + commit + CI surface,
the director already owns the final merge decision,
the TOCTOU problem matters most there.

I would not start with direct-main.

Failure modes by option

Keep in-container review as primary gate

I do not recommend this.

Main failure modes:

Auth coupling to the engineer container.
stdin/TTY drift, already implicated by the Codex probe shape.
Local endpoint dependence for local-model families.
Weak proof of what commit was reviewed.
Duplicate/out-of-band comments, since review output and final worker outcome are separate surfaces.

Immediate wholesale replacement with brokered QA

I do not recommend this either.

Main failure modes:

no current commit-bound QA schema,
no current director policy reading reviewed SHA vs PR head SHA,
no settled supersession rules for duplicate/stale QA verdicts,
no explicit reservation semantics for child QA jobs,
too much control-plane change at once.

Hybrid transition

This is the right path.

Main risks and mitigations:

Auth / probe failures: fail closed, but as an attributable QA verdict tied to a family and SHA.
Stale branch / TOCTOU: block automatically when PR head SHA no longer matches reviewed SHA.
Duplicate comments: use a dedicated QA marker and read the latest non-superseded verdict for the current SHA.
Reservation conflicts: QA must be a child review job, not an issue-owning peer engineer run.
Cost / latency: accept the extra container and model call only on pull-requests-and-merge first.
Local-model endpoints: first slice should support cloud-backed families first. Local endpoint families can follow later.

What new role/subcommand is needed

The smallest good first shape is an internal QA action, not necessarily a polished public role on day one.

Something like:

ward agent qa <owner/repo#N> --pr <n> --sha <sha> --family <family>

or an equivalent broker-only internal launch path is enough for the first slice.

I would avoid treating warded qa as the first required deliverable. The important thing is the artifact and merge policy, not the final UX spelling.

Migration plan

Fix the current review-gate regressions first.
- ward#790 must make blocked no-reviewer output truthful.
- ward#791 must make same-family Codex review runnable again.
- This keeps the current fail-closed floor intact during transition.
Add the durable QA artifact and parser without changing merge policy yet.
- New QA marker/schema.
- Parser tests.
- Forgejo read path for the PR head SHA.
Add brokered QA for pull-requests-and-merge only.
- One reviewer family first.
- Fail closed on missing, stale, or blocked QA.
Change director merge eligibility to require a passing QA verdict for the current PR head SHA.
- review summary: becomes informational.
Only after stability is proven, demote in-container review for PR-merge workflows to optional diagnostic/fast-path.

Smallest viable next implementation issue

The smallest next implementation slice is:

Add commit-bound QA verdicts and make pull-requests-and-merge director merge require a passing QA verdict for the current PR head SHA.

That slice should include:

a dedicated QA verdict marker/schema,
a Forgejo read path that exposes PR head SHA,
a narrow QA launch path for one reviewer family,
director merge policy changes to consume QA verdicts by exact SHA,
tests for:
- missing QA blocks merge,
- stale QA SHA blocks merge,
- exact-SHA pass allows merge,
- duplicate older QA verdicts do not override the current SHA.

What must be true before deprecating the current gate

Ward should not deprecate the in-container review gate until all of this is true:

brokered QA produces a durable, commit-bound verdict,
director merge consumes that artifact, not free-text summary prose,
PR head movement invalidates prior QA automatically,
QA failures are fail-closed and operator-actionable,
duplicate/stale QA verdicts have deterministic supersession rules,
the first supported reviewer family is reliable enough for normal PR runs,
current fail-closed protection remains intact until the new lane proves itself.

Bottom line

Ward should deprecate in-container review as the authoritative PR merge gate over time, but not yet. The right move is a hybrid transition where brokered, commit-bound QA becomes authoritative for pull-requests-and-merge first, and in-container review remains temporarily as the fail-closed floor and a useful diagnostic/local fast path.

Researched and posted automatically by ward agent advisor --harness codex (ward#179). This is one-shot research, not a carried change - verify before acting on it.

— Codex, via ward agent

### 🔎 ward agent advisor `ward agent advisor --harness codex` ran a one-shot **quick** research pass on this question: > Read the issue title, body, and comment thread below as the research brief. Identify the decisions, options, open questions, and risks, then give a clear advisory recommendation that can be posted back on the issue. > > Additional framing from the dispatcher: > Make a concrete recommendation on deprecating in-container review in favor of brokered QA roles. Be opinionated and identify the first implementation slice. --- ## Recommendation Ward should move to a **hybrid model**, with **brokered QA becoming the authoritative merge gate for `pull-requests-and-merge`**, while **in-container review stays in place temporarily as the fail-closed safety floor and local diagnostic path**. The recommendation is **not** to keep the current in-container gate as the long-term authority, and **not** to rip it out immediately. The worker container is the wrong place to be the only review authority. The repo already shows why: - `ward agent review` decides reviewer availability **inside the worker container** via `reviewerAvailable(...)` and per-family launch probes in [`cmd/ward/agent_review.go`](/workspace/ward/cmd/ward/agent_review.go:372). - Codex availability depends on a real bounded CLI probe with `/dev/null` stdin in [`internal/agents/codex/smoke.go`](/workspace/ward/internal/agents/codex/smoke.go:18), which matches the issue's concern about harness/runtime brittleness. - Director merge eligibility currently trusts a parsed **free-text** `review summary:` from the latest `WARD-OUTCOME` comment in [`cmd/ward/agent_director_merge.go`](/workspace/ward/cmd/ward/agent_director_merge.go:98) and [`cmd/ward/agent_director.go`](/workspace/ward/cmd/ward/agent_director.go:750). - The documented merge rule is still effectively: `workflow: pull-requests-and-merge` + `WARD-OUTCOME: done` + `review summary starts with passed:`. See [`docs/agent-director-pr-merge.md`](/workspace/ward/docs/agent-director-pr-merge.md:8). That architecture is too weak for a durable merge gate because it couples review authority to the engineer container's auth, stdin/TTY behavior, model endpoint shape, and local runtime. It also does **not** prove that the reviewed commit is the one later merged. ## Why brokered QA should be the destination A brokered QA run launched as a **fresh container against a pinned branch/commit** is the better boundary: - **Auth and local setup** failures are isolated to the QA role instead of contaminating the engineer's landing path. - **stdin/TTY** quirks become QA-runtime bugs, not mysterious "no reviewer available" outcomes inside the worker. - **Local-model endpoints** can be supported selectively instead of making every worker container able to host every reviewer family. - The QA result can be made **durable and commit-bound**, which closes the current TOCTOU gap. The authoritative rule should become: - QA records the **exact commit SHA** it reviewed. - Director merge reads the **current PR head SHA**. - Merge is allowed only when **CI is green** and there is a **passing QA verdict for that exact SHA**. - If the branch moves, the QA verdict is automatically stale and merge blocks until QA reruns. That is the key architectural improvement. Without it, replacing the current gate just moves the fragility. ## Recommended shape ### 1. Keep in-container review for now, but demote its future role In-container review should remain useful for: - a **fast local preflight** on the live unpushed worktree, - a **diagnostic path** when brokered QA fails unexpectedly, - a **same-family cheap check** when the worker harness is known-good, - workflows that have **not yet** been migrated to an authoritative brokered gate. It should **not** remain the long-term authoritative merge gate for PR-based landing. ### 2. Add a commit-bound QA artifact Ward needs a dedicated QA verdict artifact instead of overloading `review summary:` in `WARD-OUTCOME`. A small structured marker is enough, for example a fenced JSON block or a dedicated `WARD-QA:` header carrying: - `reviewed_sha` - `reviewer_family` - `verdict` (`pass` or `block`) - `reason` - `confidence` - `workflow` - `issue` - `pr` - timestamp / run identity The exact wire format matters less than the **commit binding** and deterministic parsing. ### 3. Make director merge consume QA, not summary prose Director merge eligibility should change from: - linked issue ended `WARD-OUTCOME: done` - workflow is `pull-requests-and-merge` - `review summary:` starts with `passed:` To: - linked issue ended `WARD-OUTCOME: done` - workflow is `pull-requests-and-merge` - PR head SHA equals QA `reviewed_sha` - latest non-superseded QA verdict for that SHA is `pass` - required CI is green The human-readable `review summary:` can stay for operator context, but it should become informational only. ### 4. Start narrowly The first brokered QA slice should target **`pull-requests-and-merge` only**. That is the smallest safe place to introduce the new control plane because: - there is already a delayed merge boundary, - the PR gives a stable branch + commit + CI surface, - the director already owns the final merge decision, - the TOCTOU problem matters most there. I would **not** start with `direct-main`. ## Failure modes by option ### Keep in-container review as primary gate I do **not** recommend this. Main failure modes: - **Auth coupling** to the engineer container. - **stdin/TTY drift**, already implicated by the Codex probe shape. - **Local endpoint dependence** for local-model families. - **Weak proof** of what commit was reviewed. - **Duplicate/out-of-band comments**, since review output and final worker outcome are separate surfaces. ### Immediate wholesale replacement with brokered QA I do **not** recommend this either. Main failure modes: - no current commit-bound QA schema, - no current director policy reading reviewed SHA vs PR head SHA, - no settled supersession rules for duplicate/stale QA verdicts, - no explicit reservation semantics for child QA jobs, - too much control-plane change at once. ### Hybrid transition This is the right path. Main risks and mitigations: - **Auth / probe failures**: fail closed, but as an attributable QA verdict tied to a family and SHA. - **Stale branch / TOCTOU**: block automatically when PR head SHA no longer matches reviewed SHA. - **Duplicate comments**: use a dedicated QA marker and read the latest non-superseded verdict for the current SHA. - **Reservation conflicts**: QA must be a child review job, not an issue-owning peer engineer run. - **Cost / latency**: accept the extra container and model call only on `pull-requests-and-merge` first. - **Local-model endpoints**: first slice should support cloud-backed families first. Local endpoint families can follow later. ## What new role/subcommand is needed The smallest good first shape is an **internal QA action**, not necessarily a polished public role on day one. Something like: - `ward agent qa <owner/repo#N> --pr <n> --sha <sha> --family <family>` or an equivalent broker-only internal launch path is enough for the first slice. I would avoid treating `warded qa` as the first required deliverable. The important thing is the **artifact and merge policy**, not the final UX spelling. ## Migration plan 1. **Fix the current review-gate regressions first.** - `ward#790` must make blocked no-reviewer output truthful. - `ward#791` must make same-family Codex review runnable again. - This keeps the current fail-closed floor intact during transition. 2. **Add the durable QA artifact and parser without changing merge policy yet.** - New QA marker/schema. - Parser tests. - Forgejo read path for the PR head SHA. 3. **Add brokered QA for `pull-requests-and-merge` only.** - One reviewer family first. - Fail closed on missing, stale, or blocked QA. 4. **Change director merge eligibility to require a passing QA verdict for the current PR head SHA.** - `review summary:` becomes informational. 5. **Only after stability is proven, demote in-container review for PR-merge workflows to optional diagnostic/fast-path.** ## Smallest viable next implementation issue The smallest next implementation slice is: **Add commit-bound QA verdicts and make `pull-requests-and-merge` director merge require a passing QA verdict for the current PR head SHA.** That slice should include: - a dedicated QA verdict marker/schema, - a Forgejo read path that exposes PR head SHA, - a narrow QA launch path for one reviewer family, - director merge policy changes to consume QA verdicts by exact SHA, - tests for: - missing QA blocks merge, - stale QA SHA blocks merge, - exact-SHA pass allows merge, - duplicate older QA verdicts do not override the current SHA. ## What must be true before deprecating the current gate Ward should not deprecate the in-container review gate until all of this is true: - brokered QA produces a **durable, commit-bound verdict**, - director merge consumes that artifact, not free-text summary prose, - PR head movement invalidates prior QA automatically, - QA failures are fail-closed and operator-actionable, - duplicate/stale QA verdicts have deterministic supersession rules, - the first supported reviewer family is reliable enough for normal PR runs, - current fail-closed protection remains intact until the new lane proves itself. ## Bottom line Ward should **deprecate in-container review as the authoritative PR merge gate over time**, but **not yet**. The right move is a **hybrid transition** where **brokered, commit-bound QA becomes authoritative for `pull-requests-and-merge` first**, and in-container review remains temporarily as the fail-closed floor and a useful diagnostic/local fast path. --- Researched and posted automatically by `ward agent advisor --harness codex` (ward#179). This is one-shot research, not a carried change - verify before acting on it.   — Codex, via `ward agent`

coilyco-ops added the

consult

labels

2026-07-09 18:09:21 +00:00

coilyco-ops referenced this issue

2026-07-09 18:26:10 +00:00

Add commit-bound QA verdicts to gate PR director merges #844

coilyco-ops commented

2026-07-09 18:26:21 +00:00

Author

Member

Consult interrogation: design work is complete. The carrying implementation issue is #844 for commit-bound QA verdicts gating PR director merges. Closing this consult shell.

coilyco-ops closed this issue

2026-07-09 18:26:22 +00:00