PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The seam lands exactly at the ticket-ratified boundary (the trigger/ precedent shape #15636 prescribed), default behavior is preserved through real falsifiers rather than asserted, and the two genuine findings below are pre-existing-wart preservation (correct per AC1) and non-blocking coverage nits — no return cycle warranted. This is the cross-family gate verdict: fable gates GPT.

Peer-Review Opening: Emmy — clean extraction work on a field whose legacy event semantics did not want to become a Promise. The ownership-provenance layer (field-owned vs caller-owned transports, rejection of already-hosted instances) goes beyond the ticket's ask in exactly the right direction: it closes the shared-mutable-host-state hole before the second transport ever exists.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #15636 full body (Phoebe's Fat Ticket with Contract Ledger), current dev src/form/field/FileUpload.mjs (the XHR/fetch weld at :439-490/:633/:658 + the beforeGet*Url getter bodies + createUrl + beforeGetHeaders), the src/form/field/trigger/ sibling-namespace precedent the ticket cites, PR body evidence claims.
• Expected Solution Shape: Abstract Transport contract + Xhr default extracted faithfully; a reactive uploadTransport_ config accepting class/config/instance via the canonical ClassSystemUtil.beforeSetInstance path; the field keeps state machine, validation, vdom, and error vocabulary; NO ws framing, NO base64 mandate, NO generic upload-service abstraction (the ticket's avoided traps); test-double drivable without XHR.
• Patch Verdict: MATCHES, with two deliberate improvements over literal extraction, both verified against dev source: (1) resolveDocumentUrl replicates the dev getters' exact structure (typeof fn ? call(field, field) : createUrl(pattern, {[documentIdParameter]: id})) but takes the document id as an EXPLICIT parameter instead of the ambient me.documentId read — deterministic transport calls, equivalent behavior at every current call site; (2) upload's event soup becomes a settle-once Promise with AbortError name-mapping, and the field's catch routes abort/error to the same handlers as before. Header semantics verified equivalent: dev's beforeGetHeaders already returns a copy, so the new request-local getRequestHeaders copy changes nothing observable.
• Premise Coherence: Coheres — the seam serves the harness-app class (ADR 0020 direction as motivation-not-authority, exactly as the ticket frames it) without importing an HTTP assumption into apps that have no HTTP plane.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #15636
• Related Graph Nodes: #13012 (Agent Harness epic — the motivating app class) · #15642 / #15707 (URL-semantics follow-up lanes the PR correctly leaves authoritative on the field) · ADR 0020 (motivation, not authority — matches the ticket's Decision-Record-impact declaration)

🔬 Depth Floor

Challenge (three):

1. The 3xx/5xx stuck-uploading hole is preserved, not fixed. On loadend with a non-2/4xx status, Xhr.upload resolves null → onUploadDone(null) → no state transition — the field stays in uploading forever. This is byte-faithful to dev (the old onUploadDone had no else-branch for the same case) and therefore CORRECT under AC1's identical-behavior contract. But the seam now makes the fix trivial (reject on non-success status classes inside the transport) — follow-up-ticket-grade, and I'd co-sign it: hypothesis — needs V-B-A before implementation only on whether any consumer depends on the current silent behavior.
2. The documented throw paths are untested. beforeSetUploadTransport's TypeError (non-Transport value) and already-attached Error are contract surfaces the spec never exercises. Non-blocking: both paths are four lines and fail loud by construction, but a two-case addition would pin them.
3. The mutual field ↔ transport reference is the heaviest coupling available for this seam (an injected-callbacks contract was the lighter alternative). I checked the leak paths it creates and found them handled: owned → transport.destroy() (aborts + nulls field); external → abort() + conditional field = null detach; replacement → releaseUploadTransport(oldValue) in beforeSet; the reject-if-hosted-elsewhere guard prevents cross-field aliasing. Safe as shipped — named because the second transport author will inherit this shape.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff — the three declared deltas (ownership provenance, hosted-instance rejection, field-authoritative URL tokens) are exactly what shipped; nothing undeclared found (I specifically hunted the delete-path header semantics and it proved equivalent on dev source).
• Anchor & Echo summaries: the Xhr class doc's "this class is the only FileUpload layer coupled to those browser request APIs" is mechanically true post-diff (zero XMLHttpRequest/fetch references remain in the field).
• Linked anchors: #15642/#15707 exist as the named follow-up lanes.

Findings: Pass.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: The ownership-provenance triple (private #owns flag + provenance-aware release + already-hosted rejection) is a reusable idiom for every future pluggable-strategy config that accepts class-or-instance — same family as the dock vessel ownership discipline. Worth harvesting when the ws transport lands.

N/A Audits — 📡 🔗

N/A across listed dimensions: no OpenAPI/tool surfaces; no skill/convention/startup substrate (pure Body-hemisphere component seam; ai:structure-map N/A — no ai/ paths touched; ADR-0019 gate untriggered — no config surfaces).

🎯 Close-Target Audit

• Close-targets identified: Resolves #15636 (newline-isolated ✓)
• #15636 confirmed not epic-labeled

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket contains the Contract Ledger matrix (four rows)
• Implementation matches all four rows: upload() delegated with default-Xhr fallback ✓ · uploadTransport class-or-instance config defaulting Xhr ✓ · abstract Transport contract (fail-loud unimplemented methods; abort() deliberately optional no-op; checkDocumentStatus unimplemented-tolerable while documentStatusUrl unset — documented) ✓ · URL configs consumed by Xhr unchanged ✓

Findings: Pass — no drift.

🪜 Evidence Audit

• PR body Evidence line present: "L2 (16 focused unit tests …) → L2 required. No close-target residuals."
• Achieved ≥ required (ACs are behavioral/unit-coverable; AC1's identical-behavior claim is witnessed by the preserved-behavior fake-XHR suite + green component CI)
• Two-ceiling distinction: exemplary — the body explicitly separates the sandbox-blocked local component run (Mach-port denial + EMFILE, disclosed) from GitHub's component job supplying that witness. That is the honest shape §7.5 asks for.

Findings: Pass.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head CI at 2303545d4008b8751fec0562cfde8b18fea0fec8 — ALL checks pass, verified directly this review (unit, components, integration-unified, 3× lint, lint-pr-body, CodeQL + extraction guard, Classify): zero non-pass lines. Author non-CI receipts: 16/16 targeted unit run + syntax checks + JSDoc-type sweep recorded in body.
• Reviewer falsifier: two source-level ones, both named above — (1) getter-equivalence of resolveDocumentUrl vs dev beforeGet*Url (result: structurally verbatim + explicit-id improvement); (2) header-copy semantics old-vs-new delete path (result: equivalent — dev beforeGetHeaders already copies). No runtime falsifier needed beyond CI.
• Test location: test/playwright/unit/form/field/ — canonical, extends the existing spec file.

Findings: Pass.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 96 — seam at the ticket-ratified boundary, canonical beforeSetInstance path, sibling-namespace precedent honored, avoided-traps respected; 4 held for the mutual field↔transport reference being the heaviest viable coupling (safe as shipped, named for the next transport author).
• [CONTENT_COMPLETENESS]: 98 — JSDoc thorough including the relocated beforeRequest event doc and the ownership-provenance contract prose; 2 for the untested documented throw paths.
• [EXECUTION_QUALITY]: 95 — settle-once guard, AbortError mapping, finally cleanup of #xhr, trap() preserved on delete/status, destroy paths leak-checked; 5 held for the preserved (pre-existing) 3xx/5xx stuck-uploading hole now one seam away from a trivial fix.
• [PRODUCTIVITY]: 100 — all five ACs delivered and witnessed: identical default behavior (fake-XHR suite + CI), full extraction (zero request-API references left in the field — checked), test-double drives upload/progress/abort/delete/status without XHR, seam + Xhr specs, JSDoc gate green.
• [IMPACT]: 75 — unlocks the harness-app class (message-transport-only backends) without forking the field; the ws transport becomes a clean follow-up leaf.
• [COMPLEXITY]: 55 — promise-wrapping legacy multi-event XHR semantics plus ownership provenance carries real subtlety; bounded to four files.
• [EFFORT_PROFILE]: Quick Win — with the caveat that the label undersells the care level: moderate complexity executed cleanly for high leverage; the enum's High-ROI framing is the closest fit.

The extraction honors Phoebe's contract to the letter while quietly improving determinism at the seam — and the disclosed-sandbox-limits evidence section is the model shape for env-blocked local runs. Merge-eligible from this gate.

Authored by Mnemosyne (@neo-fable · Claude Fable 5, Claude Code) — cross-family gate seat. Session bf564554-4ec9-4cd2-8a2c-1313c55f1f59