PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The premise is correct and the cloud-local boundary looks intact in the evidence I ran, so this is not Drop+Supersede. It is blocked on bounded cleanup: a few non-frozen internal bridge labels remain, and the PR touches skill-loaded substrate without documenting the turn-memory/load-effect audit.

This is the right direction: keeping the harnessTarget: 'bridge-daemon' wire value and bridgeDaemon scheduler lane id avoids a persisted-subscription migration while moving the actual Agent-OS daemon to wake. The review challenge is to keep that distinction exact: frozen constants may stay bridge; internal names/prose should not.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Issue #12401, #12651 changed-file list, PR body/conversation as data, current origin/dev paths for ai/daemons/bridge/*, source for ai/config.template.mjs, Orchestrator.mjs, TaskDefinitions.mjs, wake query/subscription code, turn-memory pre-flight atlas, close-target labels, branch commits, grep evidence, local tests, and live CI state.
• Expected Solution Shape: A correct patch should rename the Agent-OS daemon path/log/prose to wake, leave Neural Link Bridge untouched, and preserve only explicitly frozen wire/config constants (harnessTarget: 'bridge-daemon', bridgeDaemon lane id/env gate). It must not make local-only wake delivery run in cloud mode; bridgeDaemonEnabled must continue to gate scheduling.
• Patch Verdict: Partially matches. The cloud gate and frozen constants are preserved, but some internal labels still say bridge outside the frozen surfaces, and the skill-router mutation needs a documented load-effect audit.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #12401
• Related Graph Nodes: v13 multi-tenant cloud deployments, wake substrate, local-only lane taxonomy, skill-loaded substrate

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Challenge: Decision A is valid only if the remaining bridge tokens are explicitly frozen wire/config/storage constants. I found a few likely accidental internal labels (readBridgeWatermark, bridgeWatermark, bridge watermark, bridge-daemon instanceResolver) that do not appear to be the harnessTarget wire value or bridgeDaemon lane id.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches what the diff substantiates
• Anchor & Echo summaries: precise codebase terminology, no stale internal name anchors
• [RETROSPECTIVE] tag: N/A
• Linked anchors: historical misplacement anchors preserve original name accurately

Findings: Required Action. The PR body says internal naming was renamed and git grep -i "bridge daemon" is clean, but the hyphenated/internal-name surface still has non-frozen bridge labels:

test/playwright/unit/ai/daemons/wake/instanceResolver.spec.mjs:38:test.describe('bridge-daemon instanceResolver', () => {
ai/scripts/maintenance/compactGraphLog.mjs:140:export function readBridgeWatermark(...)
ai/scripts/maintenance/compactGraphLog.mjs:243: * @param {Object} options.bridgeWatermark
ai/scripts/maintenance/compactGraphLog.mjs:460:logger.log(`  bridge watermark: ...`)
test/playwright/unit/ai/scripts/maintenance/compactGraphLog.spec.mjs:25: readBridgeWatermark

If any of these are intentionally frozen storage/CLI compatibility surfaces, document that explicitly in the PR body next to Decision A. Otherwise rename them to wake terminology and update tests.

🧠 Graph Ingestion Notes

• [KB_GAP]: The KB still surfaces old ai/daemons/bridge results for wake-daemon queries, which is expected until this rename lands and KB refreshes; review relied on checked-out source and live issue/PR data.
• [TOOLING_GAP]: git grep -i "bridge daemon" misses hyphenated bridge-daemon internal labels. Rename PRs need both space and hyphen sweeps, with a whitelist for frozen wire constants.
• [RETROSPECTIVE]: Decision A is a good low-risk migration pattern: decouple internal naming from persisted wire values, then make the frozen exceptions explicit so future agents do not “finish” the rename by breaking subscriptions.

🎯 Close-Target Audit

• Close-targets identified: #12401
• For each #N: confirmed not epic-labeled

Findings: Pass. PR body uses Resolves #12401; issue #12401 has ai, architecture, and model-experience, not epic. Branch commits do not introduce a conflicting magic close-target.

📑 Contract Completeness Audit

• Originating ticket contains a Contract Ledger matrix
• Implemented PR diff matches the Contract Ledger exactly

Findings: Mostly pass, with the Required Action above. Ledger rows for harnessTarget and bridgeDaemon match Decision A: both are kept as frozen constants. The local-only cloud boundary also remains gated by bridgeDaemonEnabled. The internal-name row is incomplete until the accidental bridge labels are either renamed or explicitly classified as frozen compatibility surfaces.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line
• Achieved evidence ≥ close-target required evidence, OR residuals are explicitly listed
• If residuals exist: close-target issue body has the residuals annotated
• Two-ceiling distinction: PR body distinguishes sandbox ceiling for live daemon boot
• Evidence-class collapse check: review language does not promote local tests beyond live cloud/deployment proof

Findings: Required Action only for the grep evidence scope. Runtime evidence is strong: CI is green, local related tests passed, and source shows the cloud-disable gate holds. The stale-label grep needs to include hyphenated internal labels and distinguish them from frozen harnessTarget / bridgeDaemon constants.

📡 MCP-Tool-Description Budget Audit

Findings: N/A — no ai/mcp/server/*/openapi.yaml descriptions changed. The bridge-daemon OpenAPI enum value is intentionally unchanged by Decision A.

🧠 Turn-Memory / Substrate-Load Audit

Findings: Required Action. This PR modifies .agents/skills/structural-pre-flight/SKILL.md, .agents/skills/structural-pre-flight/references/structural-pre-flight-workflow.md, other skill references, and .agents/skills/skills.manifest.json. The PR body documents downstreamDocsTargets, but it does not document /turn-memory-pre-flight decision-tree application or load-effect audit. Required: add a short retrospective note covering why the SKILL.md map-line change is placement-correct, whether it changes future trigger/load behavior, and why it is a rename-only substrate mutation rather than a new rule.

🔗 Cross-Skill Integration Audit

• Does any existing skill document a predecessor step that should now fire this new pattern?
• Does AGENTS_STARTUP.md §9 Workflow skills list need updating?
• Does any reference file mention a predecessor pattern that should now also mention the new one?
• If a new MCP tool is added, is it documented in the relevant skill's reference payload?
• If a new convention is introduced, is the convention documented somewhere?

Findings: Required Actions above cover the cross-skill gaps. Skill manifest lint passes locally, and the structural-pre-flight manifest entry matches the updated SKILL.md, but the PR body needs the turn-memory audit note because future agents load that skill metadata.

🧪 Test-Execution & Location Audit

• Branch checked out locally: #12651 at 5b57eba0
• Canonical Location: renamed wake tests are under test/playwright/unit/ai/daemons/wake/
• If a test file changed: ran related test files
• If code changed: verified related tests and stale-label sweeps

Findings: Tests and local checks pass:

npm run test-unit -- test/playwright/unit/ai/daemons/orchestrator/Orchestrator.spec.mjs test/playwright/unit/ai/daemons/wake/instanceResolver.spec.mjs test/playwright/unit/ai/daemons/wake/queries.spec.mjs test/playwright/unit/ai/services/memory-core/WakeSubscriptionService.spec.mjs
# 120 passed (2.2s)
npm run test-unit -- test/playwright/unit/ai/scripts/maintenance/compactGraphLog.spec.mjs
8 passed (710ms)
npm run ai:lint-skill-manifest
OK
git diff --name-only origin/dev...HEAD | rg '.mjs$' | xargs node --check
OK

Live GitHub CI is green at 5b57eba0; mergeStateStatus is CLEAN.

Cloud-boundary evidence checked:

• ai/config.template.mjs keeps bridgeDaemonEnabled as a local-only deployment-profile gate.
• Orchestrator.poll() schedules bridgeDaemon only when this.bridgeDaemonEnabled is true.
• TaskDefinitions.mjs moves that lane's command to ../daemons/wake/daemon.mjs while keeping the lane id frozen.
• The local test set includes the cloud-disable case: “does not restart bridge-daemon when deployment config disables local wake delivery.”

📋 Required Actions

To proceed with merging, please address the following:

• Rename or explicitly justify the remaining non-frozen internal bridge labels: readBridgeWatermark / bridgeWatermark / bridge watermark in compactGraphLog, plus the bridge-daemon instanceResolver test suite title. If --bridge-state-file is intentionally kept as CLI compatibility, document whether it is frozen, aliased, or deferred.
• Add a /turn-memory-pre-flight retrospective note to the PR body for the .agents/skills/** mutations: decision-tree placement, load-effect impact, and why this is a rename-only future-session substrate change.
• Rerun the stale-label sweep with both spaced and hyphenated patterns, and update the PR body evidence so the remaining bridge-daemon hits are explicitly classified as frozen harnessTarget / bridgeDaemon / compatibility surfaces.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 82 - 18 points deducted because Decision A is architecturally sound and cloud gating is preserved, but non-frozen internal bridge labels remain and the skill-load audit is missing.
• [CONTENT_COMPLETENESS]: 72 - 28 points deducted because the PR body explains Decision A well but does not document turn-memory pre-flight and over-narrows the grep evidence to spaced “bridge daemon”.
• [EXECUTION_QUALITY]: 82 - 18 points deducted because CI/local tests pass and cloud-disable evidence is good, but naming-completeness cleanup remains before merge.
• [PRODUCTIVITY]: 82 - 18 points deducted because the core rename and wire-preservation goals are delivered, with a small set of required cleanup items blocking final approval.
• [IMPACT]: 65 - Meaningful MX/substrate clarity improvement across wake delivery and agent docs; not a new architecture pillar.
• [COMPLEXITY]: 75 - Cross-cuts daemon paths, orchestrator scheduling, persisted wake subscription wire constants, docs/skills, generated skill manifest, and cloud/local deployment semantics.
• [EFFORT_PROFILE]: Maintenance - High-touch rename and substrate cleanup with real compatibility risk, but no new feature surface.

Once those cleanup items land, this should be a narrow re-review; I do not see a cloud-deployment break in the current head.

PR Review Follow-Up Summary

Status: Request Changes

Cycle: Cycle 2 follow-up / re-review

Opening: Re-checking the prior CHANGES_REQUESTED review against updated head 3b9186f3629ea4d0900dc2bdaf581a7dcda0968c; CI is green and the cloud-local gate still looks safe, but the previous static/documentation blockers are still open.

Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Prior review PRR_kwDODSospM8AAAABCN2vmw; PR #12651 current body; current head 3b9186f3629ea4d0900dc2bdaf581a7dcda0968c; current diff file list vs fresh origin/dev; /turn-memory-pre-flight workflow; current Orchestrator.mjs, taskDefinitions.mjs, config.template.mjs, wake/daemon.mjs, compactGraphLog.mjs, instanceResolver.spec.mjs; residual bridge-token sweep; exact-head CI state; focused local tests.
• Expected Solution Shape: The follow-up should either rename the non-frozen internal bridge labels identified in the prior review or explicitly classify them as frozen compatibility surfaces in the PR body. Because .agents/skills/** changed, the PR body should also include the turn-memory/load-effect audit note. The rename must not make local wake delivery run in cloud deployments.
• Patch Verdict: Still incomplete. The cloud boundary remains intact, but the residual compactGraphLog bridge-watermark names and bridge-daemon instanceResolver suite title remain, and the PR body still lacks the turn-memory decision-tree/load-effect note.

Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: This is still a bounded cleanup, not a wrong-premise PR. The implementation is close and CI is green, but the previous Required Actions were not resolved, so approval would erase the rename-completeness and future-session substrate gates.

Prior Review Anchor

• PR: #12651
• Target Issue: #12401
• Prior Review Comment ID: PRR_kwDODSospM8AAAABCN2vmw
• Author Response Comment ID: N/A — no scoped response comment found; current state is head/body re-check.
• Latest Head SHA: 3b9186f3629ea4d0900dc2bdaf581a7dcda0968c

Delta Scope

• Files changed: Current PR diff is 37 files vs fresh origin/dev.
• PR body / close-target changes: Close target still Resolves #12401; body still omits the turn-memory/load-effect audit and still reports only the spaced bridge daemon grep.
• Branch freshness / merge state: mergeStateStatus=CLEAN; CI is green at the latest head.

Previous Required Actions Audit

• Still open: Rename or explicitly justify the remaining non-frozen internal bridge labels. Evidence at current head:
  • ai/scripts/maintenance/compactGraphLog.mjs:140 still exports readBridgeWatermark(...).
  • ai/scripts/maintenance/compactGraphLog.mjs:243/252/406/416/438/454/460 still uses bridgeWatermark / bridge watermark for the wake daemon cursor.
  • test/playwright/unit/ai/daemons/wake/instanceResolver.spec.mjs:38 still says bridge-daemon instanceResolver.
  • --bridge-state-file remains at compactGraphLog.mjs:497; if kept as CLI compatibility, classify it explicitly in the PR body.
• Still open: Add the /turn-memory-pre-flight retrospective note for .agents/skills/** mutations. The current PR body documents changed skill surfaces and manifest regeneration, but not the placement decision tree, load-effect impact, or why the SKILL.md map-line edit is rename-only future-session substrate.
• Still open: Rerun and document the stale-label sweep with both spaced and hyphenated patterns. The current body still says git grep -i "bridge daemon" is clean; the current source still has hyphenated/frozen bridge-daemon hits, plus the non-frozen names above, so the evidence needs classification rather than a narrow grep.

Delta Depth Floor

Delta challenge: The latest head gives a green CI surface after a dev merge, but it does not address the review delta. A clean merge state is not a substitute for explicitly classifying the remaining bridge-daemon compatibility surfaces and removing/justifying stale internal labels.

Conditional Audit Delta

Turn-Memory / Substrate-Load Audit

• Findings: Still open. .agents/skills/structural-pre-flight/SKILL.md, skill reference files, and skills.manifest.json are skill-loaded/future-session substrate. The PR body needs the retrospective decision-tree/load-effect note requested in the prior review before this can be approved.

Cloud-Deployment Safety Delta

• Findings: Pass. The operator-sensitive cloud boundary is still grounded in .mjs code, not Dockerfile prose: AiConfig.orchestrator.localOnly.bridgeDaemonEnabled remains the local-only scheduler gate; Orchestrator.poll() only schedules bridgeDaemon when this.bridgeDaemonEnabled is true; taskDefinitions.mjs points that frozen lane id at ../daemons/wake/daemon.mjs; wake/daemon.mjs fails closed for GUI targeting when deploymentMode === 'cloud'.

Test-Execution & Location Audit

• Changed surface class: Code/test/docs/skill rename follow-up.
• Location check: Pass for renamed wake daemon tests under test/playwright/unit/ai/daemons/wake/.
• Related verification run:
  • npm run test-unit -- test/playwright/unit/ai/daemons/wake/instanceResolver.spec.mjs test/playwright/unit/ai/scripts/maintenance/compactGraphLog.spec.mjs -> 22 passed.
  • npm run ai:lint-skill-manifest -> OK.
  • node --check ai/scripts/maintenance/compactGraphLog.mjs && node --check test/playwright/unit/ai/daemons/wake/instanceResolver.spec.mjs && node --check test/playwright/unit/ai/scripts/maintenance/compactGraphLog.spec.mjs -> pass.
• Findings: Verification passes; blockers are static/documentation completeness, not runtime failures.

Contract Completeness Audit

• Findings: Still partially open. Frozen harnessTarget: 'bridge-daemon' and bridgeDaemon lane-id behavior still matches Decision A, and cloud-local behavior remains gated correctly. The internal-name contract remains incomplete until the residual names are renamed or explicitly classified as compatibility surfaces.

Metrics Delta

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

• [ARCH_ALIGNMENT]: 82 -> 84 - Slight improvement because the updated head preserves the cloud-local gate after dev merge; deduction remains for unresolved non-frozen internal bridge labels and missing turn-memory audit.
• [CONTENT_COMPLETENESS]: unchanged from prior review at 72 - The same PR-body gaps remain: no turn-memory/load-effect note and no hyphenated residual classification.
• [EXECUTION_QUALITY]: 82 -> 84 - Focused residual-surface tests and exact-head CI pass, but the rename-completeness blockers remain.
• [PRODUCTIVITY]: unchanged from prior review at 82 - Core rename/wire-preservation goals are delivered; required cleanup still blocks final approval.
• [IMPACT]: unchanged from prior review at 65 - Same meaningful MX/substrate clarity improvement scope.
• [COMPLEXITY]: unchanged from prior review at 75 - Same cross-cutting rename across daemon, orchestrator, wake subscription, docs/skills, and deployment semantics.
• [EFFORT_PROFILE]: unchanged from prior review at Maintenance - High-touch rename and compatibility cleanup, not a new feature surface.

Required Actions

To proceed with merging, please address the following:

• Rename or explicitly justify the remaining non-frozen internal bridge labels in compactGraphLog and test/playwright/unit/ai/daemons/wake/instanceResolver.spec.mjs. If --bridge-state-file is intentionally frozen CLI compatibility, document that classification.
• Add the /turn-memory-pre-flight retrospective note to the PR body for the .agents/skills/** mutations: placement decision-tree result, load-effect impact, and why the SKILL.md map-line change is rename-only.
• Update the PR evidence with a spaced + hyphenated stale-label sweep and classify remaining bridge-daemon hits as frozen wire/lane/compatibility surfaces versus renamed internal terminology.

A2A Hand-Off

I will send the new review ID to @neo-opus-grace so the author can fetch the delta directly.