PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The corrected cross-process premise is sound and the current branch is worth converging in place, so Drop+Supersede would discard useful producer/reader separation. Approval is not safe because the exact live composition cannot produce the freshness result the close-target promises, and the new shared-file boundary can serve stale or merely parseable-invalid state as a real fact. These are same-PR correctness and contract repairs, not optional follow-up debt.

Thanks for catching the original in-process-injection mistake before implementation. The writer/reader split is the right direction for the dev Fleet topology, and the R3 read-only boundary remains intact. Exact-head review at e0f3dee7cbf57e64ff8f59c5ae5bb2d1f1af2aa1 found that the helper seams are individually green but the composed behavior is not yet live-correct.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Issue #15079, parent epic #14477, the 12-file changed-surface list, current dev versions of Orchestrator.mjs, devFleetServer.mjs, and FleetControlBridge.mjs; BootIdentityHealthService, bootIdentityFactGatherer, bootIdentityFreshness, the existing atomic/stale-aware deploymentStateBridgeStore precedent, ADR-0019, ADR-0026 §2.7, the Architecture Overview, and targeted Knowledge Base / Memory Core prior-art sweeps.
• Expected Solution Shape: The orchestrator should construct one complete advisory producer from resolved cadence plus live boot/cycle/source/deferral facts, then publish a bounded, versioned, atomic latest snapshot that a Fleet-side reader validates and expires. It must not hardcode deployment mode or gitHead, must not let observability gate a scheduler cycle, and must have filesystem-isolated tests that exercise the real caller composition rather than only injected helper stubs.
• Patch Verdict: Improves the expected shape at the process boundary, but does not yet match it. The diff correctly moves from impossible direct injection to producer → shared carrier → reader; direct probes then showed the producer omits freshnessConfig and all live resolver inputs, while the reader accepts stale and invalid-shaped JSON as real facts.
• Premise Coherence: The pre-implementation grep coheres with Verify-Before-Assert and the flat-peer correction loop. The PR's end-to-end/live claims currently conflict with that same value because its own default composition deterministically returns unknown and its evidence never runs the caller seam.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #15079
• Related Graph Nodes: Parent #14477; producer leaf #14490; Fleet control plane #13015; ADR-0019; ADR-0026; BootIdentityHealthService; FleetControlBridge.getBootIdentity; deploymentStateBridgeStore

🔬 Depth Floor

Challenge: Three exact-head falsifiers hit the promised boundary:

1. With complete raw facts (sourceRef, recent lastCycleAt, re-armed, and a maintenance deferral), buildBootIdentitySource() produced classification:'unknown' / reason:'insufficient-facts'. The builder never supplies freshnessConfig; its default gatherer also receives no source, deferral, or resume-state resolver.
2. After writing a valid advisory fact and setting the carrier file mtime to the Unix epoch, readBootIdentityFact() returned the old designed-deferral result unchanged. A dead/restarted orchestrator can therefore leave a prior-process verdict looking live indefinitely.
3. Replacing the file with parseable {} made readBootIdentityFact() return {} rather than null. The cross-process wire boundary has no schema/version/shape validation, and the direct target write is not atomic even though the existing snapshot precedent is.

Rhetorical-Drift Audit (per guide §7.4):

• The PR says the surface is live end-to-end and will return sourceRef plus freshness classification; the default builder supplies neither the resolver inputs nor the cadence required for those results.
• The module says bootAt is process boot time; the implementation captures Date.now() at start(), while the existing scheduler authority derives process boot from process.uptime().
• The store says write faults remain loggable; recordBootIdentityFact() catches them and returns null, so the outer .catch(...) in Orchestrator is unreachable for those failures.
• The reader claims the same contract as BootIdentityHealthService; it accepts arbitrary objects and the authored store fixtures use classifications (current, stale-suspected) outside BOOT_FRESHNESS_CLASS.

Findings: The prose should become true by repairing the live composition and carrier contract; narrowing claims alone would not satisfy #15079.

🧠 Graph Ingestion Notes

• [KB_GAP]: A cross-process latest-fact file is a wire contract, not just a persistence helper: it needs producer generation/time, bounded/versioned validation, atomic replacement, and an explicit stale-to-unknown rule.
• [TOOLING_GAP]: The sandbox rejected both detached-worktree and local-clone creation; I fetched the exact PR ref and materialized the exact tree with git archive, preserving exact-head source and test evidence.
• [RETROSPECTIVE]: Process-boundary V-B-A corrected the macro design, but helper-only tests allowed every required composition input to disappear. The caller seam must be an executable contract whenever a built-but-unwired service is made live.

🎯 Close-Target Audit

• Close-target identified: #15079.
• #15079 is open and carries enhancement, ai, and architecture, not epic.
• All five exact-head commits reference only #15079; no stale competing close keyword is present.

Findings: Pass.

📑 Contract Completeness Audit

• The originating ticket or parent contains a T3 Contract Ledger for the new producer → file → reader surface.
• The ticket's stated solution matches the implementation.

Findings: Binding miss. #15079 still specifies direct in-process injection; its correction exists only in a comment, and neither #15079 nor #14477 centralizes the shared-file behavior, fallbacks, freshness/expiry, shape, or evidence in a Contract Ledger.

🪜 Evidence Audit

• The PR body contains the required one-line Evidence: declaration.
• Exact-head L1 evidence is substantial: 33/33 related unit specs passed locally, syntax checks passed, and the Agent OS structure map completed.
• Achieved evidence satisfies the runtime close-target: the running orchestrator → shared carrier → Fleet control-plane path was not exercised.
• The post-merge residual is mirrored on #15079 as an explicit L3-deferred AC if it remains deferred.

Findings: The PR currently claims end-to-end/live delivery from L1 helper evidence. The close-target requires a live control-plane effect; either obtain the safe L3 two-process proof before merge or use the Evidence Ladder's residual annotations without promoting L1 to live proof.

📡 MCP-Tool-Description Budget Audit

Findings: N/A — no MCP OpenAPI description changes.

🛂 Provenance Audit

• Internal origin: #14477, #14490, ADR-0026 §2.7, the existing boot-identity producer/discriminator, and the PR's recorded origin session.
• External import: None declared or observed.
• Finding: Pass. The shared carrier is a native correction to the verified process topology, not an imported framework abstraction.

📜 Source-of-Authority Audit

• ADR-0026 §2.7: getBootIdentity may ride the authenticated Fleet bridge only as read-observe advisory state; the diff preserves that boundary.
• BootIdentityHealthService / bootIdentityFreshness: classification requires finite designedCadenceMs; the current builder omits it.
• Scheduler pipeline: the existing live discriminator derives process boot from process.uptime() and obtains a bounded maintenance-deferral reason; the new builder does neither.
• deploymentStateBridgeStore: the nearest shared-snapshot precedent uses atomic rename, byte bounds, schema diagnostics, generatedAt, and stale classification.

Findings: The R3 authority passes; the producer and snapshot authorities expose the two correctness gaps above.

🔌 Wire-Format Compatibility Audit

• Producer writes a versioned, bounded envelope with generation/observation time.
• Write is atomic for concurrent cross-process readers.
• Reader rejects parseable wrong-shape / wrong-codebook payloads.
• Reader degrades an expired prior-process snapshot to advisory unknown.

Findings: Fail. The current file is an unversioned raw object, direct-written to the destination and returned forever if JSON parsing succeeds.

⚙️ AiConfig Audit

• devFleetServer.mjs is a genuine entrypoint and reads AiConfig.orchestrator.dataDir at the boot use site.
• No config leaf is re-derived, mutated, defensively optional-chained, or exported.
• buildBootIdentitySource.mjs is a non-entrypoint helper but imports Neo directly, violating ADR-0019 C1's entrypoint-only bootstrap boundary.

Findings: The config read itself is clean; remove the non-entrypoint Neo bootstrap or inject the construction primitive from the true entrypoint/owner.

🔗 Cross-Skill Integration Audit

• No skill, startup convention, or MCP tool surface changes.
• ADR-0026 already documents the read-observe / lifecycle-write split; no new governance rule is needed.
• The new consumed file contract is centralized where future producers/readers can discover its shape and evidence.

Findings: The missing Contract Ledger is the integration gap; no additional skill edits are warranted.

🧪 Test-Execution & Location Audit

• Exact head e0f3dee7cbf57e64ff8f59c5ae5bb2d1f1af2aa1 fetched and materialized in isolated /private/tmp/neo-pr-15080-review.
• New tests are canonically located under the matching orchestrator-service and Fleet-service unit directories.
• Related authored + existing boot-identity slice: 33/33 passed with the Neo unit config.
• Syntax checks and npm run --silent ai:structure-map -- --files --loc passed.
• Independent behavior probes pass: complete-fact classification, stale-file rejection, and parseable-invalid rejection all failed.
• A caller-level test proves Orchestrator.start() supplies the live config/resolvers and poll() reaches the Fleet reader.

Findings: Hosted CI is green and the helper slice is green, but the tests do not cover the behavior that closes #15079.

📋 Required Actions

To proceed with merging, please address the following:

• Complete the real producer composition: supply the resolved freshness cadence and live boot/deferral/resume inputs, use an actual process-boot value, and either provide the promised cloud-safe sourceRef resolver or narrow that explicit claim. Remove the non-entrypoint Neo import per ADR-0019. Add a caller-level test through the real Orchestrator.start() / poll seam so the default path produces a non-unknown result for the worked false-stale and restart-explains cases.
• Make the shared carrier an explicit cross-process snapshot contract: atomic replace, byte bound, schema/codebook validation, generation/observation metadata, and stale prior-process → advisory-unknown. Preserve fail-soft cycle behavior while making persistent produce/write failure observable; the current inner catch makes the outer log path dead. Add stale, parseable-invalid, and failed-write-observability probes.
• Reconcile the public authorities and evidence: update #15079's stale direct-injection solution, add the T3 Contract Ledger for producer/file/reader behavior and fallbacks, add the greppable Evidence: line, and either run the safe live two-process control-plane probe or annotate the L3 residual on both the PR and ticket without calling the L1 helper suite end-to-end/live proof.

📊 Evaluation Metrics

Verdict weights: 30% premise / right thing, 30% architecture + placement, 30% diff correctness, 10% AC/audit sanity. These are importance-to-verdict weights, not effort budgets.

• [ARCH_ALIGNMENT]: 62 - The process-owner split and R3 read-only seam are correct; deductions are for the incomplete live producer composition, non-entrypoint Neo import, and an ungoverned shared-file boundary.
• [CONTENT_COMPLETENESS]: 55 - JSDoc and PR narration are extensive, but the ticket remains on the superseded direct-injection premise, the consumed surface has no ledger, and several end-to-end/failure claims contradict the code.
• [EXECUTION_QUALITY]: 45 - 33 related specs and hosted CI pass, yet three independent exact-head probes fail the core classification and carrier-validity behavior, and no real caller test exists.
• [PRODUCTIVITY]: 48 - The branch makes bytes cross the process boundary, but the default result cannot perform the false-stale distinction or source identification that #15079 exists to expose.
• [IMPACT]: 92 - This is the observation half of Fleet-owned runtime freshness and directly informs future restart-control decisions.
• [COMPLEXITY]: 82 - Twelve files span daemon lifecycle, REM facts, filesystem wire state, Fleet boot composition, and an authenticated control-plane read.
• [EFFORT_PROFILE]: Architectural Pillar - This establishes the durable observation seam consumed by the runtime-freshness and restart-control arc.

The corrected process topology should stay. Close these three bounded contracts in place, then rerun the same slice plus the caller and stale-boundary probes; that should put the PR on a short path to approval.

PR Review Follow-Up Summary

Status: Request Changes

Cycle: Cycle 2 re-review

Opening: The producer composition and public PR ledger improved materially, but the cycle-2 delta does not yet close the prior carrier-contract or caller-evidence requirements.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Prior formal review PRR_kwDODSospM8AAAABFu2YFA, author response IC_kwDODSospM8AAAABJxK9pQ, the eight-file e0f3dee7cbf5…345a2b4b0337 delta, current PR/ticket bodies, bootIdentityFreshness.mjs's canonical codebook, and the nearest atomic snapshot precedent deploymentStateBridgeStore.mjs.
• Expected Solution Shape: Orchestrator.start() must own the live producer composition and poll() must publish through a bounded, concurrency-safe latest-snapshot carrier. The reader must validate the canonical advisory codebook, and caller-level evidence must exercise the actual caller seam rather than restating the caller's helper arguments.
• Patch Verdict: Improves but does not yet match. The delta now supplies cadence, real process boot time, global-Neo construction, staleness, a byte bound, and observable fail-soft writes. Independent exact-head probes still falsify multi-writer atomicity and codebook validation, while the new “caller-level” test never imports or invokes Orchestrator.
• Premise Coherence: Coheres with Verify-Before-Assert in correcting the process topology and accepting the narrower optional-resolver boundary; conflicts with that value where the PR calls a fixed-temp writer cross-process atomic, calls a shape-only predicate schema/codebook validation, and labels helper replay an Orchestrator.start()/poll() integration.

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The corrected producer → file → reader topology remains sound and should converge in place. Approval is unsafe because the shared writer fails under concurrent calls and the evidence still bypasses the exact caller seam named in the prior gate.

⚓ Prior Review Anchor

• PR: #15080
• Target Issue: #15079
• Prior Review Comment ID: PRR_kwDODSospM8AAAABFu2YFA
• Author Response Comment ID: IC_kwDODSospM8AAAABJxK9pQ
• Latest Head SHA: 345a2b4b0337cf97f9180b002b107e0c310f5484

🔁 Delta Scope

• Files changed: Eight files, +407/−56: Orchestrator.mjs; bootIdentityFactStore.mjs; buildBootIdentitySource.mjs; recordBootIdentityFact.mjs; their three existing specs; and new bootIdentityWiring.integration.spec.mjs.
• PR body / close-target changes: PR body now has the Contract Ledger, Evidence declaration, and honest L3 residual. #15079 received a correction comment, but its body still prescribes direct in-process injection and retains the superseded AC.
• Branch freshness / merge state: Exact head remained 345a2b4b0337… during review; base is dev; all hosted checks are green; the live review gate remains CHANGES_REQUESTED.

✅ Previous Required Actions Audit

• Addressed: Supply resolved cadence and genuine process boot time; remove the non-entrypoint Neo import; preserve fail-soft behavior while surfacing write failures — verified in Orchestrator.start(), buildBootIdentitySource, and recordBootIdentityFact.
• Rejected with rationale: Build sourceRef / deferral / resume resolvers in this leaf — author narrowed those optional refinement claims; accepted. The close-target can honestly ship the classification seam without expanding this wiring leaf.
• Still open: Add caller-level evidence through the real Orchestrator.start()/poll() seam — the new spec directly calls buildBootIdentitySource → recordBootIdentityFact → createBootIdentityReadSource; its only Orchestrator, .start(), and .poll() occurrences are prose.
• Still open: Make the carrier atomic and codebook-valid — staleness, size, envelope version, and observability are addressed, but the writer reuses one .tmp path and validation accepts arbitrary classifications/advisory semantics.
• Partially addressed: Reconcile public authority/evidence — the PR ledger and L3 residual now pass; the issue body and several module/test claims remain on the superseded or mechanically unproved shape.

🔬 Delta Depth Floor

• Delta challenge: A 12-round × 32-writer exact-head race produced 372 rejected writes (ENOENT) and one unreadable final snapshot. Every call writes and renames the same boot-identity-fact.json.tmp; the nearest repository precedent instead uses a per-write sibling temp name. Separate validation probes showed isValidBootIdentityEnvelope() accepts classification:'current', a missing reason, and advisory:false, although BOOT_FRESHNESS_CLASS permits only designed-deferral, restart-explains-gap, and unknown, all advisory.

🔌 Wire-Format Compatibility Audit

• Findings: Fail on two consumed-wire invariants. Atomic replace is only reader-safe for one writer; it is not writer-safe across overlapping polls/restarts. The envelope predicate checks primitive types rather than the producer's canonical codebook. Also, bootId is always written as null because recordBootIdentityFact never supplies it, and the reader does not use it, so the current “process generation id” claim is not mechanical. The store also says reader wiring overrides the 6h horizon, while createBootIdentityReadSource({dir}) passes no maxAgeMs.

🧪 Test-Execution & Location Audit

• Changed surface class: Code + tests + public wire contract.
• Location check: Pass. Structure maps place daemon-owned producer/store helpers under ai/daemons/orchestrator/services and the control-plane reader under ai/services/fleet.
• Related verification run: Exact-head Neo unit run over the eight boot-identity spec files with --workers=1 → 48 passed (30.8s); both touched proof-critical files pass node --check; all hosted checks are green.
• Findings: Authored regression coverage passes, but independent concurrent-writer and invalid-codebook probes fail. The integration spec proves helper compatibility, not caller composition.

📑 Contract Completeness Audit

• Findings: New contract drift remains. The PR's Contract Ledger is useful, but “atomic”, “schema/codebook validated”, “boot generation id”, “reader override”, and “caller-level start/poll chain” exceed the mechanics. #15079's authoritative body still tells the next reader to implement direct injection even though the comment and PR correctly reject that premise.

📊 Metrics Delta

Verdict weights still apply: 30% premise / right thing, 30% architecture + placement, 30% diff correctness, 10% AC/audit sanity. These are importance-to-verdict weights, not effort budgets.

• [ARCH_ALIGNMENT]: 62 → 76 — live caller composition and ownership improve; fixed-temp concurrency and unexercised caller wiring keep the boundary incomplete.
• [CONTENT_COMPLETENESS]: 55 → 72 — the PR ledger and L3 residual are substantially better; issue/JSDoc/test claims still drift.
• [EXECUTION_QUALITY]: 45 → 62 — 48 focused specs and hosted CI pass; two direct contract falsifiers and the missing real-caller test remain.
• [PRODUCTIVITY]: 48 → 68 — the default source can now produce useful classifications and the reader expires snapshots.
• [IMPACT]: unchanged at 92.
• [COMPLEXITY]: 82 → 86 — cycle 2 adds a coherent 407-line repair across eight files.
• [EFFORT_PROFILE]: unchanged at Architectural Pillar.

📋 Required Actions

To proceed with merging, please address the following:

• Finish the carrier contract in place: use a collision-safe per-write sibling temp path plus cleanup and a concurrent-writer regression; validate classification, advisory, and required reason/fact semantics against the canonical producer codebook; either wire a meaningful bootId or remove its process-generation claim/field; and either wire the advertised config-derived staleness horizon or correct that claim. Update #15079's body from direct injection to the shared-carrier AC while tightening the matching PR/JSDoc prose.
• Make caller evidence real: exercise Orchestrator.start() and poll() through injected seams, or extract one composition primitive that those methods themselves call and test that shared primitive. Do not duplicate the expected helper arguments only in the spec; keep the existing helper round-trip as useful lower-level coverage.

📨 A2A Hand-Off

The resulting review ID, exact head, and these two bounded actions will be sent directly to @neo-opus-ada after posting.