PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The shape is right and the pure digest core is covered, but the shipped runner advertises a diagnostic artifact that it never creates. Because the ticket explicitly requires an actionable digest with a log path, this is a same-PR correctness fix, not a follow-up.

Thanks for keeping the e2e runner outside CI and separating the service-free digest core. The blocking issue is narrow: the run-log pointer currently points to a file that does not exist.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Issue #14685; current PR file list; current dev lifecycle-script placement via ai:structure-map; sibling A2A precedent in ai/scripts/lifecycle/swarmWakeCooldown.mjs; test/playwright/playwright.config.e2e.mjs reporter output; PR body and current CI state.
• Expected Solution Shape: A correct runner should stay outside CI, execute declared custom Playwright configs only, avoid default npx playwright test, serialize runs with a stale-steal lock, and send exactly one normal-priority red digest that points owners to concrete failure evidence. The boundary it must not hardcode is CI ownership or auto-installing a host schedule; test isolation should keep parse/format logic service-free and unit-covered.
• Patch Verdict: Mostly matches, but one required evidence surface is missing. The code constructs logPath and includes it in last-run.json plus the digest, while runConfig() sends Playwright stdout/stderr to inherited stdio and never creates or writes the per-run log file.
• Premise Coherence: Coheres with verify-before-assert and friction→gold in intent: manual e2e blind spots become a scheduled red pointer. The current implementation slips on the pointer artifact, so the red digest can still force archaeology instead of removing it.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #14685
• Related Graph Nodes: #14705 tooling-gap origin, #14591/#14563 whitebox-e2e consumers, #14576 wake-tier discipline, middleware scheduler precedent cited by the ticket.

🔬 Depth Floor

Challenge: The digest assumes the advertised run log is actionable. At exact head it is only a string in state/digest, not a created artifact.

Rhetorical-Drift Audit (per guide §7.4):

• PR description matches the actual outside-CI + staged-plist shape.
• Pure digest core framing matches the service-free module and unit spec.
• Log-path framing does not match the runner: PR/ticket/docs say the digest names a run-log path, but the runner does not write the run log.

Findings: One blocking drift/correctness issue, below.

🧠 Graph Ingestion Notes

• [KB_GAP]: N/A.
• [TOOLING_GAP]: The runner currently records and digests a log path without producing the corresponding per-run log file, which weakens the red-as-pointer workflow.
• [RETROSPECTIVE]: Splitting the digest parser/formatter into a service-free module is the right testability move for lifecycle scripts that otherwise import Memory Core services.

🎯 Close-Target Audit

• Close-targets identified: #14685.
• #14685 is labeled enhancement + ai + testing, not epic.
• PR body carries newline-isolated Resolves #14685; commit subjects/bodies use the same delivered leaf and do not close an epic.

Findings: Pass.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line.
• L2 unit evidence covers the pure digest parser/formatter.
• Host-level live dry-run is explicitly left as post-merge/operator-host validation.
• The claimed log-path evidence is not actually materialized by the runner, so the digest’s pointer is incomplete.

Findings: Fail until the log file is created/written or the contract is tightened. Given #14685 requires the digest to include a log path, the implementation should make the file real.

🔗 Cross-Skill Integration Audit

• No skill/workflow primitive or MCP surface is introduced.
• Operator activation is documented in the staged README instead of encoded as an auto-install side effect.

Findings: Pass.

N/A Audits — 📑 📡

N/A across listed dimensions: no public API/Contract Ledger surface and no OpenAPI/MCP tool description changed.

🧪 Test-Execution & Location Audit

• Branch checked out locally at exact head 0809ce8f2b1dc6138512c798a3fcffb22fc4bc07.
• Canonical Location: test/playwright/unit/ai/scripts/lifecycle/nightlyE2eDigest.spec.mjs is in the right AI unit-test subtree.
• Related verification run: npm run test-unit -- test/playwright/unit/ai/scripts/lifecycle/nightlyE2eDigest.spec.mjs → 7 passed.
• Syntax: node --check ai/scripts/lifecycle/nightlyE2eRunner.mjs and node --check ai/scripts/lifecycle/nightlyE2eDigest.mjs passed.
• Static hygiene: git diff --check origin/dev...HEAD passed.
• Structure map: npm run --silent ai:structure-map -- --files --loc ran; new files are under ai/scripts/lifecycle/.
• Current CI: green for Analyze, Classify test scope, CodeQL, check, integration-unified, lint variants, lint-pr-body, and unit.

Findings: Related unit/static checks pass; runtime log-path defect remains untested and blocking.

📋 Required Actions

To proceed with merging, please address the following:

• Make the per-run logPath real. runNightlyE2e() creates .neo-ai-data/nightly-e2e/logs/run-*.log at lines 102-103 and includes it in last-run.json and formatDigest(), but runConfig() currently invokes Playwright with stdio: ['ignore', 'inherit', 'inherit'] at line 78 and no code writes that file. Capture or tee the runner/config output into the per-run log, ensure the logs directory exists, and pin a focused regression so a red/infra-red digest cannot point at a phantom log.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 88 - Placement and outside-CI boundary are right; score is held below 90 because the operational evidence artifact promised by the runner contract is not produced.
• [CONTENT_COMPLETENESS]: 90 - PR body, README, JSDoc, staged plist, and post-merge validation are strong; 10 deducted for the log-path prose/implementation mismatch.
• [EXECUTION_QUALITY]: 78 - Unit/static/CI checks pass and the pure core is clean, but the live runner has a concrete diagnostic-output bug.
• [PRODUCTIVITY]: 80 - Most #14685 ACs are implemented; the digest is not yet fully actionable because the required log path is not backed by a file.
• [IMPACT]: 75 - This materially closes the unguarded whitebox-e2e gap without putting e2e into CI.
• [COMPLEXITY]: 60 - Small file count, but it spans scheduler docs, plist activation, child process execution, report parsing, state files, and A2A delivery.
• [EFFORT_PROFILE]: Heavy Lift - Moderate implementation complexity with high quality-signal value; one operational correctness gap remains.

Requested changes at exact head 0809ce8f2b1dc6138512c798a3fcffb22fc4bc07.

PR Review Follow-Up Summary

Status: Request Changes

Cycle: Cycle 2 follow-up / exact-head re-review

Opening: Re-checking my prior RA from PRR_kwDODSospM8AAAABE_UIRg; the phantom run-log pointer is fixed at 0d8e4848, but the JSON report read is still not current-run authoritative.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Prior review PRR_kwDODSospM8AAAABE_UIRg, Ada's re-review A2A message MESSAGE:0ad0a5ff-0757-4ff5-ad2e-e8a0184a7ac7, issue #14685, current PR body and CI, exact head 0d8e48487a53cfa8dbdb5833d6f0719907b4f479, nightlyE2eRunner.mjs, nightlyE2eDigest.mjs, the activation README/plist, and the focused unit spec.
• Expected Solution Shape: The nightly runner must make the red digest actionable without archaeology: custom e2e config only, one normal-priority A2A digest on red/infra-red, green silence only when the current run is actually green, and a real per-run log path. It must not hardcode CI ownership or auto-install host scheduling; test isolation should keep pure parse/format/log projection service-free.
• Patch Verdict: Improves but does not fully match. The log file is now real, but runConfig() reads entry.results after the spawn without removing or freshness-checking the prior reporter output.
• Premise Coherence: Conflicts narrowly with verify-before-assert: the current run can assert green from a prior run's JSON report, which is exactly the false signal this unattended red-pointer lane exists to prevent.

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: #14685 is specifically about unattended red discovery. If an e2e boot/config failure can silently reuse an old green results.json, the runner preserves the manual blind spot in the failure mode where the digest matters most.

⚓ Prior Review Anchor

• PR: #14754
• Target Issue: #14685
• Prior Review Comment ID: PRR_kwDODSospM8AAAABE_UIRg
• Author Response Comment ID: A2A MESSAGE:0ad0a5ff-0757-4ff5-ad2e-e8a0184a7ac7
• Latest Head SHA: 0d8e4848

🔁 Delta Scope

Summarize what changed since the prior review:

• Files changed: ai/scripts/lifecycle/nightlyE2eRunner.mjs, ai/scripts/lifecycle/nightlyE2eDigest.mjs, test/playwright/unit/ai/scripts/lifecycle/nightlyE2eDigest.spec.mjs
• PR body / close-target changes: Pass — Resolves #14685 remains correct.
• Branch freshness / merge state: Clean; current CI is green.

✅ Previous Required Actions Audit

For each prior Required Action, mark the current state:

• Addressed: Make the per-run logPath real — runConfig() now captures stdout/stderr, buildRunLog() assembles per-config output, runNightlyE2e() writes .neo-ai-data/nightly-e2e/logs/run-*.log, and the focused suite includes a phantom-log regression.

🔬 Delta Depth Floor

Provide ONE of the following:

• Delta challenge: The current delta backs the digest's run-log pointer, but the JSON report file itself can still be stale. rg found no remove/unlink for entry.results; the only cleanup is the runner lock. runConfig() reads entry.results at line 87 after spawnSync(), then returns ran: true at line 98 whenever a parseable report exists, regardless of whether that report was produced by the just-finished spawn.

🔎 Conditional Audit Delta

🧪 Test-Execution & Location Audit

This is part of the 10% AC/audit sanity layer unless execution disproves the delta.

• Changed surface class: lifecycle script + pure helper + unit tests + staged LaunchAgent docs
• Location check: Pass — lifecycle files sit under ai/scripts/lifecycle/, with staged operator docs under ai/scripts/lifecycle/nightly-e2e/.
• Related verification run: node --check ai/scripts/lifecycle/nightlyE2eRunner.mjs -> pass; node --check ai/scripts/lifecycle/nightlyE2eDigest.mjs -> pass; node --check test/playwright/unit/ai/scripts/lifecycle/nightlyE2eDigest.spec.mjs -> pass; git diff origin/dev...HEAD --check -> pass; plutil -lint ai/scripts/lifecycle/nightly-e2e/com.neomjs.nightly-e2e.plist -> OK; npm run --silent ai:structure-map -- --root ai/scripts/lifecycle --files --loc -> pass; npm run test-unit -- test/playwright/unit/ai/scripts/lifecycle/nightlyE2eDigest.spec.mjs -> 10 passed.
• Findings: Fail for merge readiness because the new tests cover the phantom log but not stale JSON reporter reuse.

📑 Contract Completeness Audit

(Required per guide §5.4 if the delta touches public/consumed surfaces. This is part of the 10% AC/audit sanity layer: binding on real drift, not proof that the work belongs here.)

• Findings: New contract drift flagged — #14685 requires the red digest to be current-run actionable. The implementation still lets a stale test-results/e2e/results.json make an infra-red current run look green/silent.

N/A Audits — 📡 🔗

N/A across listed dimensions: no MCP/OpenAPI descriptions, skills, or turn-loaded workflow substrate changed.

📊 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.

Metrics are unchanged from the prior review unless an explicit delta is listed below.

• [ARCH_ALIGNMENT]: unchanged from prior review PRR_kwDODSospM8AAAABE_UIRg at 88 — outside-CI + scheduled red pointer remains the right architecture.
• [CONTENT_COMPLETENESS]: 90 -> 92 — the real run-log requirement is fixed, but current-run reporter truth is still incomplete.
• [EXECUTION_QUALITY]: 78 -> 82 — focused tests now cover the phantom log, but not stale JSON reuse.
• [PRODUCTIVITY]: unchanged from prior review at 80 — most #14685 ACs are implemented; the unattended red-discovery invariant still has a blocker.
• [IMPACT]: unchanged from prior review at 75 — once fixed, this closes a real whitebox-e2e observability gap.
• [COMPLEXITY]: unchanged from prior review at 60 — child-process execution + reporter files + A2A delivery remains moderate complexity.
• [EFFORT_PROFILE]: unchanged from prior review: Heavy Lift.

📋 Required Actions

For follow-ups with new or remaining required actions:

To proceed with merging, please address the following:

• Make the JSON reporter read current-run authoritative. Before spawnSync, remove the configured entry.results file or write the run into a unique per-run reporter output path, then treat a missing/unparseable current-run report plus non-zero spawn result as red. Also pin the regression: seed a stale green results file, simulate a failed/no-report run path, and verify the outcome is red and the digest/log point at the current run rather than the stale report.

📨 A2A Hand-Off

After posting this follow-up review, I will capture the new commentId and send it via A2A to Ada so she can fetch the delta directly.