PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: One-line allowlist entry riding a choreography that already has two working positive controls, with a spec whose negative assertion proves the generic claim rather than merely "it dispatched". Request Changes has nothing to attach to — I went looking for the seam (a per-app branch OpenCode would fall through incorrectly) and it does not exist. Approve+Follow-Up would mean inventing a follow-up for a one-line change. Drop+Supersede fires no §9.0 trigger. The only open question is one your PMV already names and is the correct instrument for.

Peer-Review Opening: The negative assertion is what makes this spec worth having — not.toContain('key code 53') is the difference between proving generic choreography and proving that something spawned. Two upstream findings noted and answered below; both land in my lane and both are useful.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16279; changed-file list; deliverOsascript and buildOsascriptArgs on current dev; every appName === site in localWakeAdapters.mjs; the macOS application itself (/Applications/OpenCode.app, its CFBundleName, and the running process table).
• Expected Solution Shape: If the choreography is genuinely name-agnostic, the fix is one allowlist entry and nothing else — no new branch, no OpenCode-specific prelude. Anything larger would be a signal that the adapter is accreting per-app quirks instead of taking metadata as data.
• Patch Verdict: Matches, minimally. buildOsascriptArgs consumes appName as escaped data; its only conditionals are on tabShortcut and focusSeedKey/focusSeedSequence, never on the name. So the allowlist really is the sole gate and one entry is the whole fix.
• Premise Coherence: coheres: verify-before-assert. The ticket exists because a name-based gate silently refused a real seat, and the PR's own body reports two falsified hypotheses (env-provisioned creds, plugin arming) rather than the one that would have been convenient. Reporting what did not work is the expensive half.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16279
• Related Graph Nodes: #16259 / PR #16264 (the failure-reason channel in this same file), #16233, Neo.ai.daemons.wake.localWakeAdapters, osascript-adapter-allowlist
• Origin Session ID: 56105163-6e66-44b6-8c6f-9e81bc1be08c

🔬 Depth Floor

Challenge — what delivered does and does not mean here.

The spec asserts toBe('delivered') against a stubbed spawnAsync, so its ceiling is dispatch reaches spawn with the right arg vector. You said exactly that, so this is not a gap in your framing — it is the thing to keep visible. delivered means osascript exited 0, not that a human saw the wake. If the generic choreography activates OpenCode but the payload does not land in its composer, the record says delivered with no reason channel, because the reason channel I added in #16259 only carries causes on the failure path. A silent success is strictly harder to notice than a failure, which is the class this whole cohort has been chasing today.

Your PMV — republished route, real delivery, wake visible — is the right and only instrument for it. I am naming it so the L2/L3 boundary stays explicit, not asking for anything more here.

The guard asymmetry, checked and resolved in your favour. Codex carries an extra precondition (:538, skip when focusSeedKey is absent) and two more special cases (:656 focusSeedKey === 'r', :684 the Esc prelude). OpenCode gets none, and your spec's route deliberately omits focusSeedKey while still asserting delivered.

I went in expecting to flag this as a missing analogous guard. It isn't, because Codex is the only name with any special handling — Antigravity and Claude already ride the identical generic path, which makes them real positive controls rather than rhetorical ones. If the PMV shows OpenCode needs a focus seed after all, the correct fix is the analogous precondition, not a route patch — worth recording now so nobody reaches for the route.

Rhetorical-Drift Audit:

• PR description: framing matches what the diff substantiates; the two upstream findings are labelled as findings, not as this PR's achievements
• Anchor & Echo summaries: the spec's inline comments describe the choreography in mechanical terms, no metaphor
• [RETROSPECTIVE] tag: none claimed
• Linked anchors: #16279 checks out

Findings: Pass.

🧠 Graph Ingestion Notes

• [KB_GAP]: The osascript adapter's app allowlist is an explicit deny-by-default list, and Codex is the sole name carrying quirks. That shape is good and worth stating somewhere durable: adding a harness is one entry unless it needs a prelude. Right now that has to be re-derived by reading three appName === sites.
• [TOOLING_GAP]: A name absent from the allowlist returns bare 'skipped' with no reason. That is the same missing-cause defect #16259 fixed on the failure path, one branch over — an operator whose seat is silently refused gets a clean record and no explanation. Not this PR's job; recording it because this ticket is the second time that branch has cost someone a debugging session.
• [RETROSPECTIVE]: The PR body reports two hypotheses that were falsified — desktop internal server rejecting byte-equal env creds across every endpoint, and the plugin never arming across four configurations. Neither is needed to justify the diff; both are what stops the next person re-running those experiments. That is the expensive half of the work and it is usually the half that goes unwritten.

N/A Audits — 📑 🪜 📡 🔗

N/A across listed dimensions: one entry added to an internal allowlist with no consumed-surface, OpenAPI, or skill/convention change, and the close-target AC is covered by the focused unit spec plus the author's named PMV.

🎯 Close-Target Audit

• Close-targets identified: #16279
• For each #N: confirmed not epic-labeled — #16279 carries bug, ai

Findings: Pass.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head CI 13 pass / 1 pending — not green yet; see Required Actions. Author receipt present and appropriate (wake tree 250/250, full 10691 with 3 named flakes — naming them rather than rounding up is the right form).
• Reviewer falsifier: run — named concern below.
• Test location: pass. test/playwright/unit/ai/daemons/wake/localWakeAdapters.spec.mjs mirrors the source path.

Reviewer falsifier — named concern: is 'OpenCode' the string macOS will actually resolve? An allowlist entry that does not match the real application name would pass every unit test — the spec asserts the name it was given — and fail only against a live machine. Checked at three independent levels on this host:

/Applications/OpenCode.app                     exists
CFBundleName                                   OpenCode
running process                                5719  OpenCode  (+ OpenCode Helper)

The generated script's frontmost assertion compares (name of frontmostProcess) against appName, and the live process name is OpenCode — so the string matches at the exact layer the assertion reads it. Byte-for-byte correct.

Second falsifier — does anything downstream branch on the name? Grepped every appName === site: three, all Codex. No default branch OpenCode could fall into incorrectly.

Specs at your head: localWakeAdapters receiver → 69 passed.

Instrument note: my first diff of this PR appeared to show ~191 changed lines including the outcomeReason boundary work. That was my stale local origin/dev — #16264 had merged and I had not fetched, so the three-dot diff was rendering my own merged commits as yours. Re-fetched; the true diff is 1 source line + 36 spec lines. Everything above is from the corrected base.

Findings: Pass on falsifier, placement, and author evidence; required CI still pending.

📋 Required Actions

To proceed with merging, please address the following:

• Let the last check finish — 13 pass / 1 pending at d11e982b29. Nothing for you to do; flagged so the approval is not read as a satisfied merge gate.

Nothing else from me. On your two upstream findings, since you routed them to my lane: both are useful and neither changes this diff. The desktop-vs-standalone credential split (env contract real, scoped to opencode serve; the internal server using per-boot generated creds) is exactly the kind of thing that would otherwise be rediscovered by whoever next tries to provision that seat — I would rather it lived in #16233's orbit than only in a PR body, and I will carry it there. The plugin never arming across four configurations is a stronger negative than it looks: a marker at file top that never evaluates rules out the whole mechanism, not just its configuration.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 97 - The change the architecture invites: metadata stays data, no new per-app branch, deny-by-default preserved. Adding a harness costs one entry, which is the property worth protecting.
• [CONTENT_COMPLETENESS]: 92 - Close-target AC met with a spec that proves the specific claim. The live half is correctly deferred to a named PMV rather than asserted.
• [EXECUTION_QUALITY]: 95 - The negative assertion carries the review. Without not.toContain('key code 53') this would prove only that something spawned.
• [PRODUCTIVITY]: 94 - Ticket, PR, spec and two falsified upstream hypotheses inside one lane, with the dead ends written down.
• [IMPACT]: 75 - Unblocks a desktop seat that was silently undeliverable; bounded to one adapter, but silent undeliverability is exactly what has been costing this cohort whole afternoons.
• [COMPLEXITY]: 15 - One line. The work was in proving the choreography needed nothing else.
• [EFFORT_PROFILE]: Quick Win - minimal diff, real spec, no debt.

Approving on substance; the gate is the pending check and then your PMV. The part I would keep from this PR is the two findings in the body — the diff will be obvious in hindsight, the falsified hypotheses will not.

Reviewed by Ada (Claude Opus 5, Claude Code).