TDD: remote judgment — unblock gate parks from a phone (draft)

[itsHabib]

Summary

Technical Design Document at docs/features/remote-judgment/spec.md. Design review, not code review — nothing executable changes here.

The headline finding while grounding this: the Slack-button → ngrok → escalate serve → gate resolve path already ships (#130/#140/#150, evidence files under docs/features/escalation-plane/). So this TDD is a gap-close + extension on one binary, not a new component — and it deliberately avoids the word "executor" (taken by the gate executor GitHub-App merge bridge).

Decisions that need a call (§4, §10)

• §4.3 confirm modal with a serve-signed, short-lived, user-bound token in private_metadata — is the binding (esc, decision, uid, channel, ts, grant, exp) enough, and is the layer order (Slack sig → token → allowlist → grant) right?
• §4.5 thread post-back reads container.message_ts off the interaction payload so flare stores nothing new — does Amendment 3 survive cleanly?
• §4.9 batch (P3) is blocked on FOLLOWUPS "Still open (1)/(2)" in gate (run-scoped vs subject-scoped open-park; terminal act not linearized). Agree a batch over that is unsafe?
• Q2 flip cmdJudge's requireOpenEscalation in P0 (the "second writer" revisit trigger is met by intent replay)?
• Q3 may the phone trigger gate judge -auto at all (P2b)?

Rollout (§9)

Commitment boundary is P0 (no-regret hardening) + P1 (confirm modal) → validation gate (§11: ≥10 real phone stamps, zero double-apply, zero off-grant, one induced outage, one induced SIGKILL). P2a/P2b/P3/P4 are dossier stubs until the gate passes.

Dossier: project workbench, phases remote-judgment-p0 … remote-judgment-p4; tasks materialized for P0 + P1 only.

🤖 Generated with Claude Code

[itsHabib]

@codex review
@claude review
@cursor review

This is a design review, not a code review — the only file is docs/features/remote-judgment/spec.md. Please weigh in on:

• §4.1 — the claim that the Slack→tunnel→escalate serve→gate resolve path already ships and this is a gap-close. Anything in the "what exists / what doesn't" inventory wrong?
• §4.3 / §7.2 — the confirm-modal token (serve-signed HMAC in private_metadata, ≤10 min, bound to escalation + decision + Slack user id + message). Holes?
• §4.5 — reading container.message_ts off the interaction payload for the thread post-back so flare keeps storing nothing (Amendment 3).
• §4.9 / §9 — P3 batch blocked on the two open gate races in FOLLOWUPS.
• §10 Q2/Q3 — flipping cmdJudge's open-park guard; whether the phone may trigger gate judge -auto.

[claude]

Claude finished @itsHabib's task in 2m 46s —— View job

---

Design Review — docs/features/remote-judgment/spec.md

Tasks

• Load tools
• Read spec document + related context files
• Review §4.1 — existing path inventory
• Review §4.3 / §7.2 — confirm-modal token
• Review §4.5 — thread post-back / Amendment 3
• Review §4.9 / §9 — P3 batch + FOLLOWUPS races
• Review §10 Q2/Q3 — cmdJudge guard flip + phone → gate judge -auto
• Post final review

---

§4.1 — "What already ships" inventory

The claim is correct and I can verify it directly from the code and evidence files.

What is confirmed shipped:

• escalate serve with Slack signature verification (±5 min window, constant-time compare, pre-parse rejection) — EVIDENCE-escalate-serve-phase1.md
• ESCALATE_ALLOWED_SLACK_USERS allowlist, refuse-to-start without it — Phase 1+2
• Async ack (≤3s) + background gate resolve + response_url replace_original outcome — FOLLOWUPS confirms feat(escalate): ack the Slack tap within 3s, resolve async (escalation seam) #150 done
• flare renders ActionApprove/ActionBlock buttons gated on resolve_actions opt-in, only on resolvable parks — EVIDENCE-flare-resolve-buttons-phase2.md
• contracts/escalation shared vocabulary so neither side hard-codes the string — Phase 2 evidence
• Per-escalation mutex in serve; concurrent double-tap resolves exactly once — Phase 1 transcript
• gate next -json already projects parked[].escalation (the artifact id) AND ready_to_merge[].merge_command (observe/inbox.go:42,77) — so both seams §4.5 and §4.6 depend on already exist

What the spec correctly identifies as missing:

• Confirm step (one tap stamps today)
• Outcome in thread (only the card is replace_original'd)
• SIGKILL durability (intents.jsonl journal) — FOLLOWUPS confirms still open
• -state on the paste-resolve line — fix(flare): rescue #238's round-2 fixes stranded by the squash-merge #240 open
• SLACK_BOT_TOKEN for views.open / chat.postMessage
• Heartbeat file + card "ingress" line
• Real phone-tap evidence (Phase 3 runbook exists; only operator infra + the tap remain per EVIDENCE-escalate-e2e-phase3.md)

One small inventory gap to tighten: Phase 3's e2e (cmd/escalate/e2e, 10 cases) is described as "SCAFFOLDED" in escalate-serve.md, which could read to a reviewer as "not yet CI-green." The spec's §4.1 says "three evidence files" — it might be worth noting that the e2e is deterministic CI (loopback + stubgate), not just a runbook, to pre-empt any "build it again" instinct. Not a correctness issue, just a doc clarity nit.

No inventory items appear wrong. The worst-case outcome (building escalate serve a second time) is averted — the existing component is accurately described.

---

§4.3 / §7.2 — Confirm-modal token

The token binding (esc, decision, uid, channel, ts, grant, exp) is sound. Notes on each layer and two holes worth sealing:

Layer order (Slack sig → token → allowlist → grant) is correct. The Slack sig is the cheapest and widest reject (unsigned = attacker can't reach serve at all). Token verification comes next (no store round-trip), then allowlist (same), then the grant check (shells gate). Inverting any of these would either add latency to the widest-reject path or allow a stale-grant tap to consume a token validation.

Binding fields — sufficient? Yes, with one note:

• uid binds the identity; ts binds the specific card instance (not just the escalation). This is the right granularity — two concurrent Approve taps on the same escalation from the same user each get their own token keyed to the same ts, so the second submit is handled by gate's errStaleEscalation, not by a token collision. That's correct.
• grant is in the token, but §4.1 says serve reads the grant from gate next -json (never from a payload field). The token carries grant as a consistency check (step 4: "mismatch = refuse"), not as the authoritative source. This is the right design — the token's grant is a hint; the inbox is the authority. This should be made explicit in the spec (§7.1 step 4 describes the mismatch → refuse path but doesn't label it as "defense in depth against a re-park"). Worth a one-line clarification.
• channel + ts are needed for the thread post-back (§4.5), so they're doubly load-bearing: both decode-correctness and observability.

Hole 1 — key bootstrap race. ESCALATE_TOKEN_KEY is generated if absent. The spec says "generated if absent, chmod 600" but doesn't say when: on first view_submission? On serve start? If it's generated lazily (on first tap), a race between two concurrent first-taps could both call "file absent" and write different keys, corrupting one. The safe design is: generate-on-start, before the listener opens. The spec should state this explicitly.

Hole 2 — token key rotation. The spec defers key custody to a FOLLOWUPS entry (Q1), which is fine. But it doesn't say what happens to in-flight modal tokens when the key rotates. The answer implied by the design (tokens become invalid → user sees "expired" → tap again) is correct and fail-closed, but this should be stated explicitly as a rotation invariant, not left as an implicit consequence.

§7.2 misuse scenarios are correctly handled. The expired/forged/cross-user/replay cases all fail closed and at the right layer. One scenario not covered in §7.2: serve is SIGKILLed between views.open (token issued) and the view_submission arriving. On restart, the new serve has the same key (persisted) so the token is still valid — the modal submit works and the intent is journaled. This is the correct and safe behavior; worth a one-line note in §7.2 or §7.6 since it shows the key persistence + token design are coherent across restarts.

---

§4.5 — Thread post-back, Amendment 3

The design is correct and Amendment 3 holds cleanly. Verification:

block_actions payload carries container.message_ts and channel.id. Serve copies them into the token. On outcome, serve calls chat.postMessage with thread_ts. flare's notify package has no net/http client calls toward escalate, and gate next -json is the only external read serve makes (the console read seam). No new flare state, no coupling.

One edge case to name: what happens when container.message_ts is absent from the payload? (Slack sends it for button taps in messages, but not in all contexts — e.g. if the card is somehow rendered as an ephemeral or in App Home.) The spec should state the fallback: if ts is absent, serve posts the outcome to response_url only (already planned), and the thread line is skipped rather than erroring. The fallback should be documented at §6.5 invariant level, not prose-only.

Amendment 3 widening question (§10 Q5): The spec correctly says "no" — retro-editing old cards when ingress goes down would require flare to keep ts permanently, which is a new authoritative-state responsibility Amendment 3 exists to prevent. The current design (flare status + paste line) is the right call. The boundary is correct.

---

§4.9 / §9 — P3 batch blocked on FOLLOWUPS races

Agree that batch over the current open-park semantics is unsafe to ship. The FOLLOWUPS analysis is precise:

• Still open (1): newestTerminal is run-scoped but parkedRuns is subject-scoped (PR-scoped). In a batch of 20, the operator approves esc_y for PR feat(custody): guard mint-key creation behind explicit -init (no silent orphan key) [M1] #99 (run 1), but PR feat(custody): guard mint-key creation behind explicit -init (no silent orphan key) [M1] #99 was re-gated and is now on run 2 with esc_w. The run-scoped check passes (esc_y is still run 1's newest terminal), the judgment appends, and the action becomes PR feat(custody): guard mint-key creation behind explicit -init (no silent orphan key) [M1] #99's newest subject-scoped terminal — hiding esc_w from the inbox. That's exactly the "stale card hides a newer park" failure the spec describes. A human approving a stale card one at a time has a fighting chance of noticing the subsequent flare page; a batch of 20 does not.

• Still open (2): the terminal act append is not under the same predicate as the judgment append. Another run can park in the gap; the batch resolve's action then lands after it and suppresses it. This is the harder fix (wider blast radius, touches the shared act path).

The proposal to gate P3 on both FOLLOWUPS items closing in gate is correct. No alternative path exists — a P3 shipping over the current semantics would have a class of double-resolve failures that are non-observable (they resolve silently, hiding a newer park) rather than the "☑️ already resolved" the single-tap path surfaces. That's the safety gap.

One sequencing note: the spec says P3 depends on "FOLLOWUPS (1)(2) closed in gate" but doesn't say who owns confirming they're closed before P3 materializes. The dossier stub should carry that gate-check as a pre-condition task, not just a phase dependency — otherwise P3 can start and discover mid-flight that the fixes didn't land.

---

§10 Q2 — Flip cmdJudge's requireOpenEscalation

Agree with the proposal: flip in P0, not P2b, and with the reasoning. The stated revisit condition in FOLLOWUPS ("Still open (3)") is: "if concurrent writers on one run ever stop being hypothetical — a second escalate serve, or agents judging the same run in parallel." P0's intent-replay is exactly a second writer on the same run (serve replays an undone intent, potentially concurrently with an operator typing gate judge). The trigger is met.

The one-line change (cmdJudge sets requireOpenEscalation: true) is the right scope. The retry a human operator would see in the improbable race is preferable to a judgment landing silently against a superseded park — especially now that the path is phone-driven, where the user has no terminal context to notice a silent anomaly.

However: the spec says "flip in P0, not P2b" in §10 but §9's P0 task list doesn't mention it. The task "merge #240 (-state on the paste line)" is there; this flip should be added to the P0 task list explicitly so it doesn't fall out of scope.

Test coverage concern: TestJudgeDoesNotTakeTheResolveOnlyOpenGuard is a pinning test that asserts the flag is NOT set on cmdJudge. Flipping the flag means this test needs to be inverted / renamed — otherwise CI goes red on P0. The spec should call this out as a required test change, not just "one line."

---

§10 Q3 — May the phone trigger gate judge -auto?

This is the operator's call, but the spec's framing is useful to pressure-test:

Option (a) — allow, same as operator typing it: the grant ceiling bounds the verdict identically whether the operator types it locally or the phone triggers it. The model can't exceed the ceiling, the judgment is one-shot, and the human stamp (the final gate resolve) still follows. The authority model is unchanged. This is the least-surprising path.

The actual risk is different from what's stated. The spec frames Q3 as "the verdict is the model's" but the authority concern is actually about latency and reversibility: a local gate judge -auto runs in the operator's terminal where they can SIGKILL it if they see the evidence summary and decide the context is wrong. A phone-triggered judge runs in a background goroutine; the operator can't abort it once dispatched. The card shows "⏳ asking judge" and the verdict arrives asynchronously. This is fine if the operator explicitly tapped "Ask judge" — they requested it — but the spec should acknowledge it.

Option (b) — advisory-only (dry mode): requires a gate feature that doesn't exist. Unless dry mode is worth building for other reasons, this is a deferred dependency that could stall P2b indefinitely.

Option (c) — drop P2b: is too conservative given that the grant ceiling already bounds the model judgment. P2b is valuable precisely when the operator wants a quick sanity check before stamping.

Recommendation: option (a) with an explicit acknowledgment that the request is non-cancellable once dispatched (show a "cancel" button that 404s — or just document it). The spec should add this to §7.7's "model-judge request" flow: "if the card shows ⏳ and the operator changes their mind, the judgment will still land and the human stamp is still required to act on it."

---

Overall assessment

The design is sound and the gap-close framing is accurate. The spec correctly avoids building a new component, correctly threads the boundary laws (flare/escalate/gate separation, Amendment 3, no new authority), and correctly gates its riskiest phase (batch) on pre-conditions that aren't met.

Items I'd want resolved before P0 starts:

1. Token key generation timing — specify generate-on-start (not lazy), or document the race and close it.
2. requireOpenEscalation flip added to P0 task list — and the corresponding test rename called out.
3. container.message_ts absent fallback — state the behavior at §6.5 invariant level.

Items worth a clarification pass (nits, not blockers):

• Phase 3 e2e is deterministic CI (not just a runbook) — worth one line in §4.1 to head off "let's build the scaffolding."
• Token's grant field role: "consistency check" vs. "authoritative source" — one line in §7.1 step 4.
• Key rotation invariant for in-flight tokens — one line in §7.2 or §5.
• P2b judge non-cancellable — one line in §7.7.
• P3 dossier stub should carry a gate-confirmation pre-condition task, not just a phase dependency.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2be5e4f0be

ℹ️ 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".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Block remote stamps on the subject-scoped gate races

When the same PR re-parks after serve's gate next -json lookup but before the old resolve completes, requireOpenEscalation checks only the old run, so the stale judgment can still land and its later act can hide the genuinely newer subject-level park. Therefore the claimed backstop here is not valid for the race documented in FOLLOWUPS (1)/(2), and restricting only P3 batch does not make the committed P0/P1 single-stamp path satisfy FR4; either close those races before rollout or explicitly weaken the safety claim and acceptance gate.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Preserve recovery context before invoking gate

If serve is killed after gate resolve records a pass but before Slack delivery or the done marker, replay cannot perform the behavior claimed here: the escalation has left gate next -json's parked list, while the persisted intent contains only the escalation id and no run id or merge command with which to join ready_to_merge[]. The replay therefore stops at ErrNotParked and can only report “already resolved,” losing the pinned merge command; persist the resolved run/result before invoking gate or provide a gate read seam that recovers them from the escalation.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Make the intent append durable before acknowledging

An append that merely returns before the 200 response is not sufficient to close the referenced HARD-crash follow-up: its bytes may still be only in the OS page cache, and a power loss can also leave a torn final JSONL record. Specify an fsync/fdatasync durability boundary before ack plus defined torn-tail recovery; otherwise an acknowledged decision can still disappear despite the stated persist-before-ack guarantee.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Retain a way to update the source card after modal submit

A Slack view_submission does not carry the original button interaction's response_url, and the proposed signed metadata contains only esc, decision, user, channel, timestamp, grant, and expiry. Consequently the later response_url replace_original shown in §3/§7.1 cannot be performed after the modal is submitted, leaving the original buttons and status intact; either carry the response URL in the signed token/intent or use chat.update with the already-bound channel and message timestamp.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Resolve the model-judge authority contradiction before P2b

gate judge -auto immediately writes the escalation's one-shot judgment and may emit the would_merge action; there is no subsequent gate resolve or human pass/block stamp. Thus option (a) cannot also satisfy FR6's promise that the model verdict “still goes through the human stamp” or the document's claim that the phone only transports a human decision. P2b must use an advisory-only mode followed by an explicit confirmed human resolve, be dropped, or explicitly redefine the authority model rather than treating the Ask tap as equivalent.

Useful? React with 👍 / 👎.

[itsHabib]

Scope note from the PR-sweep session (engineering owner): most of phase 1 already exists in production on this machine — flare watch (launchd: com.workbench.flare-watch) delivers every gate park to the phone channel within seconds, and escalate serve -addr 127.0.0.1:8099 is running behind a cloudflared tunnel as the Slack interactive-action ingress (SLACK_SIGNING_SECRET + ESCALATE_ALLOWED_SLACK_USERS), driving gate resolve from a tap. Please scope this TDD to what is actually missing: (1) gate judge pass/block with a written -why from the card (today only resolve), (2) executing the judged run's pinned merge command after a remote decision (today a local session must still run it), (3) batch/multi-park cards, (4) tunnel-down and replay behavior on the card. Don't re-design the ingress or the Slack auth — document them as the substrate.

[itsHabib]

Live failure from the first real remote approval — design input for this TDD.

Tonight the operator approved a park from Slack (workbench#242, run run_162e1184c5704e6c). The tap worked end to end: escalate serve ingested it, gate judge recorded jdg_2fa4ad5c33f1baf6 ("approved in Slack by @mhdevstuff"), and gate resolve recorded res_f31d75be87ff1d35. The PR did not merge. Gate re-escalated immediately:

esc_e31a924fe24668d0  grant_tier_exceeded:
verdict tier T3 exceeds grant ceiling T1

The run had been opened under a T1 grant while the change (it edits gate itself) reduces to T3 — even though a live operator-minted T3 grant for the same repo existed the whole time (grt_6ec082a7ceee4f6d).

Three requirements this implies:

1. Pre-check before the button. The card must verify the escalation's grant can actually authorize the verdict's tier and remaining cycles before rendering an approve action. If it can't, say so on the card ("approving will not merge: verdict T3 > grant ceiling T1") rather than offering a tap that records a decision and changes nothing.
2. Resolve against any qualifying live grant, not just the run's. The ingress currently resolves against the grant recorded on the escalation. When that one is too narrow but another live grant for the same repo and action covers the verdict, the card should offer it. Otherwise a re-tap loops on the identical refusal forever — the operator taps, sees "resolved", and nothing merges.
3. The decision is spent but unusable. gate judge is one-shot, so after this failure the recorded approval cannot be re-applied by judging again; recovery needs escalate resolve against the new escalation with a wider grant. The card should own that recovery path, since the operator cannot mint or re-target a grant from a phone.

Also worth covering: the card should surface a terminal outcome back to the thread ("merged <sha>" / "still parked: "), because today a successful-looking tap is indistinguishable from this failure.