PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: §9.0 Premise Pre-Flight run against all seven triggers — none fires. The instrument itself is the best fail-closed design I have reviewed today and I want it merged. One Required Action, and it is two strings in one array: the alarm's Contract Ledger row promises it names "every reference site", and it names three of five. I am raising it rather than folding it into a follow-up because the omitted site is a spec whose entire premise dissolves on conversion, and because I approved past a ledger/reality mismatch on #16094 earlier today and it only came right by the author's own initiative. Everything else is verified clean and enumerated below, so this should be one commit with no re-audit.

Peer-Review Opening: This is a genuinely good instrument, and the parts that are easy to get wrong are all right: the timeout owns the verdict rather than loadURL, with the reason documented inline where the next reader will need it; messages.length === 1 refuses to pass on mixed evidence; a loaded marker plus errors is its own contradiction branch rather than being resolved in favour of either; app.whenReady().then(main, onError) fails closed on readiness failure; and extracting the classifier means all six branches are provable without booting Electron. A capability probe whose passing state is a negative result is exactly where instruments certify themselves, and this one does not. One required action below.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16036 (labels, assignee, state); the changed-file list; both new modules read in full at the PR head; harness/main.mjs getSecureWebPreferences() to establish what the real windows configure; harness/package.json script wiring; a repo-wide git grep preload.cjs census at the head tree; adapterWitness.spec.mjs:183-200 read before asserting its failure mode; learn/agentos/decisions/ listing to confirm ADR-0034 exists.
• Expected Solution Shape: A real pinned-Electron witness whose only passing state is the documented rejection, wired ahead of both smoke legs, with every inconclusive outcome red. It must not hardcode a machine-specific path or Electron version as routing authority, must test the same window configuration the product ships, and its support-detected message must be actionable without further investigation.
• Patch Verdict: Matches, and improves on the shape in one respect — splitting the verdict into a pure helper turns a "run it on a Mac and see" capability claim into six unit-provable branches, which the ticket did not ask for. Contradicts on one detail: the support-detected message is the entire actionable payload of the alarm, and it is incomplete against its own ledger claim.
• Premise Coherence: Coheres with verify-before-assert at the instrument level, which is the hard version — the probe refuses to convert absence of evidence into evidence of absence (silence and timeout are both red), and it refuses to let a label be the witness by requiring the specific documented rejection rather than "an error occurred."

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16036
• Related Graph Nodes: ADR-0034 (Electron shell architecture), ADR-0020 (agent harness concept), harness/preload.cjs, adapterWitness.spec.mjs forced-duplication guard

🔬 Depth Floor

Challenge: the probe duplicates the product's security configuration instead of deriving it. getSecureWebPreferences() (main.mjs:109-118) returns backgroundThrottling / contextIsolation / nodeIntegration / preload / sandbox / webSecurity; the probe hardcodes the same set minus webSecurity. There is no live divergence — webSecurity defaults to true, so the effective configuration matches today, and I am not claiming a current defect. The concern is the drift direction: if getSecureWebPreferences() ever changes in a way that touches preload evaluation, the probe keeps certifying the old window shape and keeps reporting constraint-confirmed while the product has moved. That is a false green in the one instrument whose job is to notice a capability change. Deriving from the shared source and overriding only preload would make the witness structurally unable to drift from its subject. getSecureWebPreferences is not currently exported, so this is a small extraction rather than a one-liner — which is why it is a challenge and not a Required Action.

Second, smaller: messages.length === 1 means any additional benign preload-error or main-frame did-fail-load on some future machine turns both smoke legs red. That is the safe direction and the message names the observed errors, so diagnosis is immediate — I would keep it. Noting it so that when it eventually fires on noise, nobody reads it as a regression.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff, including the L3 claim — a real pinned Electron renderer is genuinely L3 and is not dressed up further.
• Anchor & Echo: @module/@summary on both new files use precise terminology; the inline comment explaining why the timeout outranks loadURL is the highest-value comment in the diff.
• [RETROSPECTIVE] tag: N/A.
• Linked anchors: verified — ADR-0034 exists at learn/agentos/decisions/0034-electron-shell-architecture.md, and the README's constraint statement matches the classifier's expected error string exactly.

Findings: Pass.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: The durable pattern here is worth naming beyond this file: a constraint we accept ("X is impossible today") normally decays into folklore, because nothing re-tests it and the workaround outlives the reason. This PR converts an accepted constraint into a self-notifying witness that fails when the constraint lifts — the inverse of a TODO. The transferable rule: when a decision record says "not possible with the current runtime," the honest artifact is a probe that goes red on the day that stops being true, not a comment saying so. Requires exactly what this one has: the specific documented failure, not a generic error class, and inconclusive-fails-closed so the witness cannot rot into a silent pass.

🎯 Close-Target Audit

• Close-targets identified: #16036
• For each #N: confirmed not epic-labeled — #16036 carries only ai, assigned neo-gpt

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket contains a Contract Ledger matrix — the PR body carries a 3-row ledger
• Implemented PR diff matches the Contract Ledger exactly (no drift)

Findings: Drift flagged, and it is the Required Action. Row 2 states the delivered behavior as "A successfully exposed marker fails red and states that conversion is unblocked, including every reference site and ADR update." A head-tree census of preload.cjs returns five sites; CONVERSION_STEPS names three plus the ADR. Detail in Required Actions. Per guide §5.4 I cannot tick this box while the alarm's payload is narrower than the row claims.

🪜 Evidence Audit

• PR body contains an Evidence: declaration — L3 (real Electron 43.1.0 renderer, UI smoke, and isolated Brain smoke on exact head) → L3 required (all runtime acceptance criteria in #16036). No residuals.
• Achieved evidence ≥ required — L3 is the real ceiling here and is genuinely reached: the probe boots an actual sandboxed renderer under the pinned runtime, and both smoke legs ran on the exact head.
• Two-ceiling distinction: correctly stated; no sandbox-ceiling excuse is offered because none is needed.
• Evidence-class collapse check: no collapse — the five classifier branch tests are correctly described as unit-level and are not promoted to runtime proof of the branches they model.
• Deployment causality: the Post-Merge Validation item is correctly scoped to the next pinned-Electron update, which is genuinely unreachable from this head.

Findings: Pass.

📡 MCP-Tool-Description Budget Audit

N/A — no OpenAPI surface in scope.

🧪 Test-Evidence & Location Audit

• Execution evidence: CI green at 07b6d7a963 (all reported checks SUCCESS); author receipts are per-surface and exact-head — probe pass with the observed Electron message quoted, both smoke legs to exit 0 with assetFailures: [] / rendererErrors: [], 95/95 harness unit, 5/5 focused branch spec, both preflight modes, git diff --check.
• Reviewer falsifier: run. Named concern — "the alarm's stated reference sites are incomplete." Method: git grep -n preload.cjs over the head tree excluding the resources/content mirror, then reading adapterWitness.spec.mjs:183-200 to confirm the failure mode rather than infer it. Result: concern confirmed, two sites unnamed, one of them load-bearing.
• Test location: pass — test/playwright/unit/harness/preloadEsmProbeOutcome.spec.mjs sits beside its siblings.

Findings: Pass on author evidence; reviewer falsifier confirmed the ledger gap.

📋 Required Actions

To proceed with merging, please address the following:

• Add the two missing reference sites to CONVERSION_STEPS (preloadEsmProbeOutcome.mjs:8-13), so the alarm's payload matches ledger row 2's "every reference site". The census at 07b6d7a963 returns five sites; you name main.mjs, electron-builder.yml, and preload.spec.mjs. Unnamed:

  1. test/playwright/unit/harness/adapterWitness.spec.mjs:187-200 — this is the important one, and "repoint the path" is the wrong instruction for it. Its describe is "the forced CJS/ESM duplication cannot drift" and its comment states the ADAPTER_STATES list is "duplicated by necessity rather than by choice" because a sandboxed ESM preload cannot import the shared module. It then readFiles harness/preload.cjs by hard path and regex-matches the declaration. On conversion it breaks on the path — but the real point is that its premise dissolves: an ESM preload can import the shared module, so the correct action is to delete the duplication and this drift guard, not to repoint them. That deletion is arguably the conversion's main payoff, and an alarm that omits it leaves a future maintainer re-asserting a guard against a constraint that no longer exists. Suggested entry: 'delete the forced ADAPTER_STATES duplication and its drift guard in adapterWitness.spec.mjs — an ESM preload can import the shared module'.
  2. learn/benefits/ArchitectureOverview.md:470 lists preload.cjs in the harness/ key-files column. Suggested entry: 'update the harness row in learn/benefits/ArchitectureOverview.md'.

  Two array entries. The harness/README.md conversion paragraph should carry the same two additions so the two surfaces do not diverge — they currently state the identical list, which is the right shape and worth preserving.

Nothing else needs re-auditing. Verified clean and found no issue: no machine-local constant used as routing authority (Electron version is read from process.versions, never compared against a literal; probe dir is mkdtemp, cleaned in finally); marker observation runs in the correct world for contextIsolation: true; did-fail-load is filtered to the main frame so subresource noise cannot poison the single-error predicate; presmoke and presmoke:brain both invoke the probe so neither leg can boot the product unwitnessed; ADR-0034 exists; and the README addition states the current mechanism rather than riding the past, which is the line I hold on substrate that outlives its migration.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 88 - Correct placement (probe beside the harness it witnesses, pure classifier beside it, spec in the mirrored unit tree), no Electron import in the testable half, no bundler and no sandbox relaxation. 12 deducted for duplicating the product's webPreferences rather than deriving them.
• [CONTENT_COMPLETENESS]: 76 - Three-row ledger, explicit Deltas, six enumerated outcome branches, README operator guidance. Deducted because the one payload that must be complete — the alarm's instruction set — names 3 of 5 sites.
• [EXECUTION_QUALITY]: 92 - Timeout-as-authority over loadURL with the reasoning inline, single-error predicate, contradiction as its own branch, fail-closed on readiness failure, finally-scoped cleanup. This is careful work.
• [PRODUCTIVITY]: 88 - One commit, five files, real L3 receipts on the exact head, no scope creep into the conversion itself.
• [IMPACT]: 82 - Converts an accepted constraint from folklore into a witness that fires the day it lifts; the pattern generalises to every "not possible with the current runtime" line in our decision records.
• [COMPLEXITY]: 64 - Electron lifecycle ordering, preload worlds, and event-versus-promise authority, all of which are easy to get subtly wrong and are not.
• [EFFORT_PROFILE]: Quick Win - Small diff, high durability, and the repair is two strings.

🌿 A constraint nobody re-tests becomes folklore, and the workaround outlives the reason. This makes the constraint tell us when it stops being true — so the only thing that has to be complete is what it says when it does.

[review-budget-managed]

• outcome: within-budget
• ordinary-limit: 2
• activation-issue: 15257
• activation-pr: 15307
• activated-at: 2026-07-16T20:54:31Z