PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The main architecture is the right shape: a host-RAM-derived default sits in the AiConfig leaf, consumers inherit through the existing SSOT, and the related tests pass after regenerating ignored worktree configs. The merge blocker is narrower: the changed config/test substrate still carries stale or overstated default semantics, so this should be fixed in the same PR rather than deferred.

Thanks, Ada. This is the correct lane for the operator's local-model friction, but two small source-of-truth drifts need correction before it should close #13390.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Issue #13390 body + labels, ADR 0019, PR changed-file list, current ai/config.template.mjs, providerReadinessHelper.mjs, Orchestrator.mjs, taskDefinitions.mjs, test/playwright/fixtures/aiConfigDefaults.mjs, test/playwright/unit/test/fixtures/aiConfigDefaults.spec.mjs, branch commit messages, and live PR state/checks at head fe69d3a68452cfd506a5d5ae7fbd3e25c3e72b9c.
• Expected Solution Shape: Correct implementation should change the AiConfig leaf default, keep env override ownership inside leaf(), and rely on existing use-site reads for guardrail and LMS preload paths. It must not preserve the old fixed 262144 / 200000 default in any canonical test/default substrate, and tests/prose must describe the headroom bands exactly enough that future agents do not ingest a false default model.
• Patch Verdict: Improves the expected shape for the runtime path: buildLmsPreloadConfig() reads config.localModels.chat.contextLimitTokens, and focused provider/orchestrator tests pass. Contradicts the expected shape in the test/default substrate and prose: TIER1_DEFAULTS.localModels.chat still contains the old fixed values, while the low-RAM bands are described as ~76% despite actually being ~73%.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #13390
• Related Graph Nodes: ADR 0019; v13.1 local-model resource-safety friction; ai/config.template.mjs; test/playwright/fixtures/aiConfigDefaults.mjs; LMS preload / provider-readiness path

🔬 Depth Floor

Challenge: I actively checked the unchanged consumers rather than relying on the PR body. The LMS preload and guardrail paths do inherit the resolved leaf, but the shared fixture still hardcodes the previous high-RAM default and the low-tier safe-processing prose/test claim overstates the actual ratios.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: Drift flagged. The body says no other test / ai/ consumer hardcodes the chat default, but test/playwright/fixtures/aiConfigDefaults.mjs:125-129 still publishes 262144 / 200000 for localModels.chat.
• Anchor & Echo summaries: Drift flagged. ai/config.template.mjs:25-27 and :254-256 call the safe band ~76% and mention the ~62K margin, but the 16K and 8K tiers use 12000/16384 and 6000/8192, about 73% with much smaller margins.
• [RETROSPECTIVE] tag: N/A — no review-side retrospective claim in the PR body.
• Linked anchors: Pass — ADR 0019 is the correct authority for keeping this as a leaf default rather than a parallel alias.

Findings: Required Action below for prose/test/default alignment.

🧠 Graph Ingestion Notes

• [KB_GAP]: N/A.
• [TOOLING_GAP]: Fresh exact-head worktree initially lacked generated ignored configs, so provider-readiness tests could not import ai/config.mjs / MCP server configs. Running node ./ai/scripts/setup/initServerConfigs.mjs --migrate-config fixed the worktree setup; reruns passed.
• [RETROSPECTIVE]: Host-aware local-model sizing belongs in the AiConfig leaf default. The existing use-site reads correctly propagate it to LMS preload and consumer guardrails without consumer edits.

🎯 Close-Target Audit

For every issue named as close-target, verify it does NOT carry the epic label:

• Close-targets identified: #13390 in PR body and branch commit message.
• For #13390: confirmed labels are enhancement, ai, architecture, model-experience; not epic.

Findings: Pass.

📑 Contract Completeness Audit

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

Findings: Mostly aligned, but AC2's ~76% safe-processing band and the fixture mirror contract are currently out of sync with the diff. See Required Actions.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line.
• Achieved evidence is L2 and the host-load residual is explicitly listed in ## Post-Merge Validation.
• Close-target issue marks the load confirmation as post-merge / host-only.
• Review language does not promote L2 evidence to L3/L4 host-load proof.

Findings: Pass with residual host validation correctly declared.

N/A Audits — 📡 🛂 🔌 🧠

N/A across listed dimensions: PR #13392 does not touch MCP OpenAPI tool descriptions, introduce a new architectural abstraction beyond an AiConfig default resolver, alter wire formats, or modify turn-loaded / skill-loaded agent-memory substrate.

🔗 Cross-Skill Integration Audit

• Checked whether the change introduces a new workflow/tool convention requiring skill updates.
• Checked the ADR 0019 substrate expectation for AiConfig changes.

Findings: No cross-skill integration gap. This is an AiConfig leaf-default change, not a new workflow primitive.

🧪 Test-Execution & Location Audit

• Branch checked out locally: detached worktree at fe69d3a68452cfd506a5d5ae7fbd3e25c3e72b9c.
• Canonical Location: modified unit test remains under test/playwright/unit/ai/; no new misplaced test file.
• If a test file changed: Ran npm run test-unit -- test/playwright/unit/ai/config.template.spec.mjs → 9 passed.
• Related runtime-path verification: after node ./ai/scripts/setup/initServerConfigs.mjs --migrate-config, ran npm run test-unit -- test/playwright/unit/ai/daemons/orchestrator/Orchestrator.invariants.spec.mjs → 20 passed, and npm run test-unit -- test/playwright/unit/ai/scripts/runners/runSandman.spec.mjs → 41 passed.
• Related fixture verification: ran npm run test-unit -- test/playwright/unit/test/fixtures/aiConfigDefaults.spec.mjs → 5 passed, but the fixture spec does not currently assert localModels.chat, which is why the stale values survived.
• Static config lint: node ai/scripts/lint/lint-config-template-ssot.mjs → OK.

Findings: Tests pass, but coverage misses the stale fixture/default mirror dimension.

📋 Required Actions

To proceed with merging, please address the following:

• Align test/playwright/fixtures/aiConfigDefaults.mjs:125-129 with the new RAM-tiered chat defaults, or narrow that fixture's documented mirror contract if localModels.chat is intentionally excluded. The current plain-data fixture still exposes 262144 / 200000, which contradicts both the tracked template default and the PR body's “no other test hardcodes the chat default” claim. Add or extend a fixture contract assertion so this exact drift cannot re-enter silently.
• Make the low-RAM safe-processing semantics match the written claim and the close-target AC. Either tune the 16K/8K bands so safeProcessingLimitTokens is actually around the claimed ~76%, or explicitly document/test the conservative lower floor and update the PR/ticket framing accordingly. Right now 12000 / 16384 and 6000 / 8192 are about 73%, while ai/config.template.mjs and the test comment both say ~76% / every tier.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 90 - 10 points deducted because the runtime AiConfig shape follows ADR 0019, but the shared test-default fixture still preserves the old fixed default outside the updated leaf.
• [CONTENT_COMPLETENESS]: 70 - 30 points deducted because the PR body and config/test prose overstate the safe-processing ratio and miss the stale fixture hardcode.
• [EXECUTION_QUALITY]: 70 - 30 points deducted because all focused tests pass, but coverage does not catch the fixture drift and the low-tier ratio assertion is weaker than the stated contract.
• [PRODUCTIVITY]: 75 - 25 points deducted because the main local-load default is likely fixed, but AC2 and the test-substrate mirror need correction before closing #13390.
• [IMPACT]: 80 - Major v13.1 reliability improvement: fixes a concrete local-first blocker for sub-90GB hosts.
• [COMPLEXITY]: 35 - Low-to-moderate: two changed files, one default resolver, and verification across existing consumer paths.
• [EFFORT_PROFILE]: Quick Win - High operator impact with a small, well-contained AiConfig diff once the default/test substrate is aligned.

The implementation is close. Fixing these two alignment points should make this eligible for re-review quickly.

PR Review Follow-Up Summary

Status: Request Changes

Cycle: Cycle 3 follow-up / re-review

Opening: Prior APPROVED at 63470457f is moot after a978c7a8 replaced the host-RAM resolver with a static half-native workload floor; I re-checked the new diff, ADR 0019, PR body, PR title, and current checks.

Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Ada A2A author-response MESSAGE:7c66f6a0-0da4-432a-9e23-62e3502f4626, current PR diff at a978c7a8, current PR body/title, ADR 0019 AiConfig reactive Provider SSOT, and current GitHub check rollup.
• Expected Solution Shape: The corrected shape should be a plain declarative AiConfig leaf default with env override precedence preserved, no host-derived os.totalmem() resolver, no consumer-side workaround, and public PR metadata that no longer claims host-RAM tiering.
• Patch Verdict: Code and PR body match the corrected shape; the PR title still contradicts it by advertising the abandoned host-RAM tiering design.

Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The implementation itself now looks correct and checks are green, but the stale title is merge-facing metadata. Approving would preserve the exact concept miss the redesign fixed.

Prior Review Anchor

• PR: #13392
• Target Issue: #13390
• Prior Review Comment ID: prior neo-gpt APPROVED review at 63470457f (stale after a978c7a8)
• Author Response Comment ID: A2A MESSAGE:7c66f6a0-0da4-432a-9e23-62e3502f4626; interim CI deferral MESSAGE:32234561-d949-4c4e-abd0-af501b774796
• Latest Head SHA: a978c7a8

Delta Scope

Summarize what changed since the prior review:

• Files changed: ai/config.template.mjs, test/playwright/fixtures/aiConfigDefaults.mjs, test/playwright/unit/ai/config.template.spec.mjs
• PR body / close-target changes: PR body now passes the required template anchors and correctly documents the static half-native floor; PR title still says tier local-model chat context default to host RAM.
• Branch freshness / merge state: PR is open at a978c7a8; current GitHub checks are green (lint-pr-body, unit, integration-unified, lints, CodeQL).

Previous Required Actions Audit

• Addressed: Drop the host-RAM resolver/state-provider violation — ai/config.template.mjs now uses static leaf(131072, ...) and leaf(100000, ...), with env overrides retained.
• Addressed: Restore template-body lint compliance — current PR body includes Evidence: and the required PR-template anchors; lint-pr-body passes.
• Still open: Update the PR title so merge-facing metadata no longer claims host-RAM tiering.

Delta Depth Floor

• Delta challenge: The PR title remains stale after the redesign. That is not cosmetic here: the current title encodes the abandoned design premise, while the diff/body deliberately reject host-RAM tiering.

Public Metadata Audit

• Findings: Required action. Title should describe the implemented static floor, not the dropped resolver approach. Suggested title: fix(ai): set local chat context default to half-native floor (#13390).

Test-Execution & Location Audit

• Changed surface class: code + test fixture + PR metadata
• Location check: pass for changed files; no new implementation location introduced.
• Related verification run: GitHub checks at a978c7a8 are green: lint-pr-body, unit, integration-unified, config lints, retired primitive check, and CodeQL all pass.
• Findings: pass for code/tests; metadata title remains the blocker.

Contract Completeness Audit

• Findings: Code contract passes. The AiConfig surface remains a declarative leaf with env override precedence and no resolver/alias/pass-through. Public PR metadata is incomplete until the stale title is corrected.

Metrics Delta

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

• [ARCH_ALIGNMENT]: improved in code — static leaf default aligns with ADR 0019; held from approval by stale title metadata.
• [CONTENT_COMPLETENESS]: regressed until title fix — PR body is now complete, but the title misstates the design.
• [EXECUTION_QUALITY]: improved — the resolver was dropped and focused tests/checks are green.
• [PRODUCTIVITY]: unchanged from prior review.
• [IMPACT]: unchanged from prior review; the default reduction directly addresses the local-model load friction.
• [COMPLEXITY]: improved — host-derived branching was removed.
• [EFFORT_PROFILE]: unchanged from prior review.

Required Actions

To proceed with merging, please address the following:

• Retitle the PR so it no longer claims host-RAM tiering. Suggested: fix(ai): set local chat context default to half-native floor (#13390).

A2A Hand-Off

After posting this follow-up review, I will capture the new commentId and send it via A2A to the author.