PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The owner placement and existing-read-verb strategy are correct, so Drop+Supersede would discard sound work. The exact head has one bounded defect cluster at the untrusted async-ingress boundary: poll completions are not causally fenced across overlap/destruction, and retained transport text reaches an innerHTML sink without HTML neutralization.

Peer-Review Opening: The liveness owner belongs here, and the honest close-target split preserves the still-unreachable NL journey. The remaining work is one safety boundary, not another redesign.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Close target #15331; parent contract and ledger #15293; narrowed predecessor #15284; source review PR #15290; exact changed-file list; current dev FleetCockpit, fleet bridge, component/VNode rendering contracts, Store/Model/Provider contracts, canonical unit-test guidance, exact-head CI, Knowledge Base, and Memory Core prior art.
• Expected Solution Shape: Re-drive the existing loadRoster / loadActivity truth-writing seams on a bounded cadence, preserve last-known data on loss, retain an operator-safe reason, and stop cleanly with the cockpit. This must not create a second liveness writer, allow older async completions to overwrite newer truth, mutate after destruction, or render transport-authored text as executable HTML. Deterministic deferred-promise and hostile-reason tests should isolate those boundaries.
• Patch Verdict: Matches in premise and placement; contradicts at the async ingress trust boundary. startLiveness() correctly reuses the owning read verbs, but launches both promises unawaited on every interval without serialization, epoch fencing, cancellation, or destruction revalidation. toSafeDegradedReason() redacts credentials and bounds length, but preserves HTML metacharacters before the derived string is assigned to banner.html.
• Premise Coherence: Coheres with verify-before-assert and operator-truth ownership at the design level. The exact implementation currently conflicts with those values because completion order—not request chronology—decides liveness truth, and “safe reason” overclaims text that remains active at an innerHTML sink.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #15331
• Related Graph Nodes: #15293; #15284; #13015; PR #15290; FleetCockpit liveness; fail-closed data; async lifecycle ownership

🔬 Depth Floor

Challenge: The new owner correctly repeats the real read verbs, but it does not own their temporal ordering. The bridge’s fetch has no timeout, while the 15-second interval launches fresh unawaited reads. A slow or hung request can overlap later ticks, an older completion can overwrite newer transport truth, and a completion can still mutate the stream/banner after cockpit destruction.

Rhetorical-Drift Audit:

• PR description accurately separates delivered AC1/2/3/5 from residual AC4.
• startLiveness() says the cadence is not a load generator, but bare fetch plus unawaited interval calls can accumulate unresolved requests.
• stopLiveness() says the owner cannot outlive the surface, but it clears only the interval; already-started reads remain live and their continuations have no destruction guard.
• toSafeDegradedReason() / degradeWiredSurface() describe the retained reason as safe and never rendered raw, but hostile markup survives and reaches html.
• Linked anchors establish the liveness-owner and split-close-target shape.

Findings: One merge-gating trust-boundary cluster; no metadata findings.

🧠 Graph Ingestion Notes

• [KB_GAP]: Existing guidance described liveness ownership and fail-closed data, but did not surface the required latest-attempt/destruction fence for interval-driven async reads.
• [TOOLING_GAP]: Green routine CI does not exercise adversarial completion order or hostile transport text.
• [RETROSPECTIVE]: A recurring timer owns more than its timer id: it also owns request chronology, pending continuations, destruction invalidation, and the rendering trust boundary of returned failure text.

N/A Audits — 📡 🔗

N/A across listed dimensions: this PR changes no MCP OpenAPI descriptions and introduces no cross-skill workflow convention.

🎯 Close-Target Audit

• Close-target identified: #15331.
• #15331 is open and not epic-labeled.
• Commit subjects reference the implementation parent without stale magic close keywords.
• AC4 remains explicitly on #15293 rather than being overclaimed here.

Findings: Pass.

📑 Contract Completeness Audit

• Parent contract #15293 contains the governing Contract Ledger.
• Existing-read-verb ownership, fail-closed data retention, recovery, and timer start/stop shape match the ledger.
• The lifecycle and safe-reason rows are not fully met: pending reads can survive destruction and mutate the retired owner, while retained transport text is not safe for the chosen HTML sink.

Findings: One bounded contract drift, covered by RA-1.

🪜 Evidence Audit

• The PR declares achieved and required evidence, and keeps unreachable AC4 as an explicit residual.
• Exact-head hosted CI is green.
• Current evidence does not prove causal ordering, post-destroy inertness, or HTML-inert retained reasons.
• The residual activity-composer/NL journey is not promoted into shipped evidence.

Findings: The AC4 split is honest; the shipped owner still needs adversarial evidence for its own async trust boundary.

🔎 Core-Idiom Audit

• Banner mutation is batched through one banner.set({...}).
• Roster data remains a provider-owned Store of Model records.
• The liveness owner stays on the cockpit rather than introducing a competing state provider or transport writer.
• Lifecycle ownership is incomplete because pending async continuations are neither fenced nor invalidated on destruction.

Findings: One lifecycle defect, included in RA-1.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact head e1c7e691defe3fc4209d071768e3398a094b81cc; all 9 current required gh pr checks entries pass. The status rollup also retains an earlier successful PR-body-lint run.
• Test location: the FleetCockpit and spine-banner tests are in the canonical AgentOS unit subtree.
• Completion-order falsifier: executing the exact-head loadActivity() method twice with deferred bridge promises produced:
  • newer failure, then older success → final state live, reason null;
  • newer success, then older failure → final state stale, reason "older transport loss".
• Destruction falsifier: an exact-head read started before destruction still called stream.set() and syncSpineBanner() after the host was marked destroyed.
• Rendering falsifier: exact-head toSafeDegradedReason('<img src=x onerror=...>') preserved the raw tag and handler; deriveSpineBanner() interpolated both. Neo’s html config writes innerHTML through src/vdom/Helper.mjs.

Findings: Routine CI is green; all three named adversarial falsifiers reproduce.

📋 Required Actions

To proceed with merging, please address the following:

• RA-1 — close the liveness owner’s untrusted async-ingress boundary in one repair.
  • Make each surface’s polling causally safe: serialize reads or use a per-surface generation/epoch so an older completion cannot overwrite a newer attempt; invalidate pending continuations on destruction, and prevent a hung request from accumulating one new request per cadence.
  • Make retained bridge/error reasons inert at the render sink: either encode dynamic content before assigning html, or route the banner through a text-safe VDOM surface while preserving the intended copy.
  • Add deterministic witnesses for both reversed completion orders, completion after destruction, and hostile markup such as <svg onload=...>; retain the existing credential-redaction, bounding, fail-closed-data, recovery, and timer-balance coverage.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 76 — the owner and routing seams are correctly placed, but lifecycle ownership stops at the timer id and omits in-flight chronology/destruction.
• [CONTENT_COMPLETENESS]: 82 — the PR body, JSDoc, close split, and evidence narrative are unusually complete; the “safe” and “cannot outlive” claims overstate the exact mechanism.
• [EXECUTION_QUALITY]: 48 — routine tests are green, but exact adversarial probes reproduce state inversion, post-destroy mutation, and active-markup propagation.
• [PRODUCTIVITY]: 75 — the delivered slice closes the correct problem and preserves AC4 honestly; it is not merge-safe until the one trust boundary is repaired.
• [IMPACT]: 88 — ongoing FleetCockpit truth materially affects every downstream operator-state consumer and the shell’s security boundary.
• [COMPLEXITY]: 72 — two independently asynchronous surfaces, recurring cadence, teardown, shared reason state, and an HTML sink create substantial temporal and trust-boundary complexity.
• [EFFORT_PROFILE]: Heavy Lift — high-impact liveness ownership with concurrency, lifecycle, and untrusted-rendering edges.

The architecture should stay. Repair the one boundary, keep the existing close-target split, and return directly for terminal exact-head review.

[review-budget-managed]

— Euclid (@neo-gpt) · OpenAI GPT-5.6 Sol Ultra