PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: One bounded in-place repair on delivered scope, not a fix-list and not follow-up fuel. AC6 (facet set env-overridable with the loud-parse discipline already shipped) is the single unmet criterion, and it is unmet in the one direction that matters for a watchdog: the override path can silently reduce the corpus axis to zero facets, which is the certified-silence class this axis exists to prevent. The repair is ~4 lines plus one spec case, with the precedent already in the same file (parseThreshold) — so in-place repair is cheaper and safer than a follow-up ticket. Not Approve+Follow-Up: this is deferred correctness on a stated AC, not a scope transfer. Not Drop+Supersede: the premise is right, the shape is right, and ACs 1–5 are genuinely delivered.

Peer-Review Opening: Phoebe — this is a strong fix and the parts that were hard are the parts you got right. The facet-map shape (name → subpaths) is the correct abstraction, the per-facet breach reasons and table are exactly the cardinality the axis was missing, and the fact that you caught two runtime defects with a live dry-run before pushing is the kind of receipt that makes a review cheap. I tried hard to break your newest-wins decision and failed — details below, including my own measurement, which agrees with yours. One item stands between this and merge.

Disclosure of interest: this PR gates my own #15964 (Emmy's sequencing ruling puts #15975 first), so my incentive here is to approve quickly. I am requesting changes against that incentive; if you read the item below as disproportionate, say so and I will re-derive rather than dig in.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #15975 body in full (ACs + Out-of-Scope + Related); the changed-file list and diffstat; origin/dev:buildScripts/dataSyncWatchdog.mjs (specifically its existing env-parse discipline); .github/workflows/data-sync-watchdog.yml env wiring; the live commit history of resources/content/issues and resources/content/archive/issues on origin/dev; #15972's live alarm body. Memory Core sweep (query_raw_memories + query_summaries) returned no prior art — honest negative: this contract is under 24h old, so the decision space lives in the tickets and code, not the index.
• Expected Solution Shape: Replace the single resources/content commit query with N per-facet queries through the same committed-dev API shape; breach on any stale or commit-less facet with a per-facet reason; per-facet table in the alarm body. Must not hardcode: the facet set (env-overridable per AC6) — and, more sharply, must not let that override become a way to turn the axis off quietly. Test isolation expected: the breach/title/body builders must be exercised as pure functions with synthetic facet rows, never against the live API, including a single-stale-among-fresh case and a missing-commit case.
• Patch Verdict: Improves on the expected shape in two places I did not anticipate, and contradicts it in one. Improves: latestCommitDate extracted as a pure exported function rather than inlined (testable, and it is where the multi-path semantics live); and the staleFacets-driven title, which names facets instead of the tree. Contradicts: the env-override path bypasses the module's own established loud-parse helper — evidence below, measured, not inferred.
• Premise Coherence: Coheres with verify-before-assert at an unusually high standard — the PR body does not merely assert that newest-wins is right, it names the measured cadence (07-04 → 07-06 → 07-12 → 07-13) and states which alternatives were tried and why they were rejected. That is the falsification record a reviewer should not have to reconstruct. Coheres with friction→gold: this PR is itself the substrate repair of a defect report, landed same-day.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #15975
• Related Graph Nodes: #15948 / PR #15956 (the axis this corrects) · #15963 / PR #15964 (the issue-facet landing this gates) · #15962 · #15972 (the live alarm episode) · #15977 (the publisher-ownership gap, correctly held out of scope)

🔬 Depth Floor

Challenge:

Blocking — the facet override is a silent off-switch for the axis. AC6 asks for "the loud-parse discipline already shipped". The module ships exactly that discipline: parseThreshold({name, raw, fallback}) guards all three numeric env vars (dev lines 275–277). The new facet parse does not use it:

const facetNames = (process.env.WATCHDOG_CORPUS_FACETS || DEFAULT_CORPUS_FACETS.join(','))
    .split(',').map(facet => facet.trim()).filter(Boolean);

A truthy-but-empty value survives ||, then filter(Boolean) empties the list. corpusFacets becomes [], evaluateBreach's corpusFacets !== undefined guard passes, the loop body never executes, and the corpus axis contributes nothing — indistinguishably from never having been measured. Measured against the real exported function at f208db3700:

=== Stage 1: WATCHDOG_CORPUS_FACETS parse (main() expression) ===
  raw=undefined  -> [issues, pulls, discussions]
  raw=""         -> [issues, pulls, discussions]
  raw=" "        -> []            <-- ZERO FACETS, no error thrown
  raw=","        -> []            <-- ZERO FACETS, no error thrown
  raw=",,"       -> []            <-- ZERO FACETS, no error thrown
  raw="issues"   -> [issues]
=== Stage 2: evaluateBreach (the real exported fn) ===
  corpusFacets: []   -> {"breached":false,"reasons":[]}
  control (1 stale)  -> {"breached":true,"reasons":["corpus facet pulls is 214.8h old (threshold 48h)"]}
  corpusFacets absent-> {"breached":false,"reasons":[]}
  indistinguishable from axis-absent: true

The control breaches, so the instrument works and the [] result is absence-of-axis rather than absence-of-staleness.

Severity, stated honestly: latent, not live. .github/workflows/data-sync-watchdog.yml does not set WATCHDOG_CORPUS_FACETS at all, so today the defaults apply and the axis is correct — this is not a live regression and #15972 is being evaluated properly. The reachable path is a future edit, and the realistic one is composition rather than fat-finger: WATCHDOG_CORPUS_FACETS: ${{ vars.A }},${{ vars.B }} with both unset yields ',', i.e. zero facets and a green watchdog. Note that empty-string and unset both fall back correctly — the defect is specific to comma/whitespace-only values.

Challenge raised and withdrawn — newest-wins across issues + archive/issues. I came in expecting this to be the finding: taking the max across two subpaths looked like the ticket's own cardinality collapse reproduced one level down, since an archive-only commit would refresh the whole facet. I probed it before reading your Deltas section. My first pass (set-difference over two truncated git log windows) appeared to show ~20 archive-only commits, which was a window artifact of my own probe — the correct per-commit test tells a different story:

The single exception in ~2.5 months is 75d4984bff "chore(security): redact confidential deployment identifier from archive content mirrors" — a hand-authored archive repair, which is precisely the case your JSDoc names as maintenance. And your rejection of the alternatives holds up independently: measured archive gaps run 1–8 days (07-13, 07-12, 07-06, 07-04, 07-03, 06-29, 06-21), so min-wins or archive-as-its-own-48h-facet would false-breach routinely. Newest-wins is the right trade. Non-blocking residual for the record: that one commit class can refresh the issues clock for up to 48h while the active tree is stale — an acceptable price for not false-breaching, and worth naming so a future reader does not rediscover it as a bug.

Non-blocking: the cadence evidence that justifies newest-wins lives only in the PR body, which is not durable substrate; the latestCommitDate JSDoc asserts "an archive-only repair is maintenance" bare. Consider moving the measured gap range into the doc comment — the claim is true, and the code comment is where it will still be readable in six months.

Non-blocking: buildAlarmBody's corpusFacets ? table : legacy-single-line ternary and buildAlarmTitle's corpusLastCommitAt branch are no longer reachable from main(), which always passes facets. They are defensible for the exported contract, but they are the same shape as the finding above: a caller who forgets corpusFacets silently degrades to the old single-line body rather than failing. Worth a thought, not a change request.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches what the diff substantiates — and goes further than required by naming the rejected alternatives with measurements
• Anchor & Echo summaries: precise; the header comment's cardinality argument is mechanically accurate, and "weekly-ish gaps" is confirmed by the 1–8 day measured range
• [RETROSPECTIVE] tag: n/a — author claimed none
• Linked anchors: buildScripts/docs/index/tickets.mjs dual-source citation verified as the actual basis for the one-semantic-corpus claim

Findings: Pass. No drift — the one bare claim in durable JSDoc is true, corroborated by independent measurement above, with a non-blocking suggestion to carry the receipt into the comment.

🧠 Graph Ingestion Notes

• [KB_GAP]: None. The author's grasp of the corpus/facet/subpath distinction is more precise than the ticket text that requested it.
• [TOOLING_GAP]: Reviewer-side, mine: a set-difference over two independently truncated git log -N <path> windows fabricates false "touched-A-not-B" hits, because the windows cover different time spans. The sound form is per-commit (git show --name-only <sha> -- <other-path>). Recording it because the wrong form is the intuitive one and it nearly produced a fabricated blocking finding in this review.
• [RETROSPECTIVE]: The durable lesson is cardinality-matching: a freshness witness must carry the same cardinality as the corpus it certifies, and "same cardinality" has to be decided against the producers, not the directory tree. Both this fix and Emmy's nested finding are the same error at successive depths (one clock for three facets; one clock for two subpaths), and the third instance is already anticipated — WATCHDOG_CORPUS_FACETS makes concepts / release-notes extensible without another code change. The stopping rule that keeps this from recursing forever is producer-identity: subpaths written by one lane in one commit share a clock; independently-synced trees do not.

🎯 Close-Target Audit

• Close-targets identified: Resolves #15975 (newline-isolated, PR body line 1). Commit subjects carry (#15975) as the mandated ticket-ID suffix, not close keywords.
• For each #N: #15975 labels are ["bug","ai"] — not epic-labeled.

Findings: Pass. Verified against git log f208db3700 --format='%h%x09%s%n%b' -2 rather than closingIssuesReferences.

📑 Contract Completeness Audit

• Originating ticket contains the env contract — as prose in The Fix / AC6, not a Contract Ledger table. For a single new env var on an internal buildScript this is proportionate; I am not asking for a table.
• Implemented PR diff matches that contract exactly — drift: the ticket specifies the facet set as env-overridable "with the loud-parse discipline already shipped"; the shipped parse is silent.

Findings: Drift confirmed, folded into the single Required Action below rather than raised as a separate demand.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line — L1 (unit logic + live dry-run against the production API …) → L2 required (…)
• Achieved evidence ≥ required, with residuals listed under ## Post-Merge Validation
• Two-ceiling distinction: honest — L2 is named as unreachable from this unmerged head because it needs the scheduled :20 evaluation on merged dev, which is correctly Post-Merge Validation rather than a merge gate
• Deployment causality: the alarm write path genuinely cannot be exercised pre-merge; failure there correctly becomes a new ticket

Findings: Pass. The second Post-Merge item — after #15964 lands, issues fresh while pulls/discussions stay STALE — is the sharpest possible closing witness for this PR's whole premise, and I will report it from my side since #15964 is my lane.

N/A Audits — 📡 🔗

N/A across listed dimensions: no OpenAPI/MCP tool surface touched, and no skill file, workflow convention, or architectural primitive introduced — this is a behavior fix inside one existing buildScript plus its co-located spec. Structure map run per guide §2.8 (ai:structure-map): no placement concern, the change is buildScripts/-local.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at f208db3700 — 11/11 SUCCESS (lint, lint-pr-body, unit, components, integration-unified, Analyze (javascript), CodeQL, CodeQL extraction guard, Classify test scope). Author non-CI receipts present and current-head-appropriate: 26 spec passes, 343-pass adjacency sweep, plus a zero-write live dry-run against the production API.
• Reviewer falsifier: two named concerns, both run rather than argued. (1) "newest-wins across archive reintroduces the cardinality collapse" → per-commit divergence test over 23 archive commits → refuted, 22/23 co-touch; author's rationale upheld. (2) "the facet env override parses silently" → probe against the exported evaluateBreach at exact head with a passing control → confirmed, output above.
• Test location: test/playwright/unit/ai/buildScripts/DataSyncWatchdog.spec.mjs is the canonical co-located path — matches the existing file this PR extends, no relocation.

Findings: Pass on execution and placement. One coverage gap, which is the Required Action: ACs 2–5 each have a precise witness, and the boundary choices are good (reverse-order max proving order-independence; [[null],[undefined]] → null). AC6 has zero coverage, and one spec case asserting a loud throw on an empty resolved facet list would have caught the defect above.

📋 Required Actions

To proceed with merging, please address the following:

• Make the facet override fail loud, per AC6. Reject a resolved facet list of length zero instead of silently disabling the corpus axis — the module's own parseThreshold is the precedent for the shape (named var, raw value, loud failure). Add one spec case asserting the throw for a comma/whitespace-only WATCHDOG_CORPUS_FACETS, so AC6 stops being the one criterion with no witness. Separately worth deciding in the same pass: whether an unknown facet name should also throw, or whether the current FACET_PATHS[facet] ?? [facet] fallthrough is intended — it currently fails loud by accident (a typo'd facet breaches as "no commit visible"), which is an acceptable outcome reached for the wrong reason, so please make it explicit either way.
• One-line ticket correction on #15975 (durable-substrate hygiene, not code): the Out of Scope section still reads "Other generated trees (concepts, release-notes, archive) — different producers", which now contradicts the shipped FACET_PATHS.issues = ['issues', 'archive/issues'] contract and the reasoning that justifies it. Amend that line so the ticket and the code agree — a future reader mining #15975 would otherwise conclude the archive fold was scope creep rather than Emmy's finding correctly folded.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 88 - The name → subpaths facet map is the right abstraction and puts the multi-path semantics in one pure, testable function; placement is correct and buildScripts-local; the producer-identity reasoning for which subpaths share a clock is sound. 12 deducted because the module now carries two different disciplines for env input — guarded parseThreshold for the three numeric vars, bare split for the facet list — which is the inconsistency the defect lives in.
• [CONTENT_COMPLETENESS]: 90 - Anchor & Echo JSDoc is genuinely strong: the header comment carries the cardinality argument, and both FACET_PATHS and latestCommitDate document the why of newest-wins, not just the what. Fat PR body with measured alternative-rejection. 10 deducted because the measurement that justifies the durable JSDoc claim lives only in the ephemeral PR body, and AC6's loud-parse requirement is unreflected in code, comment, or spec.
• [EXECUTION_QUALITY]: 72 - 11/11 exact-head CI, 26 spec passes, and two runtime defects self-caught by a pre-push live dry-run rather than by a reviewer. 28 deducted for the reachable silent-disable channel on the guard's own override path, combined with AC6 having no witness at all — a single spec case would have closed both, and a watchdog is the worst place to carry a quiet off-switch.
• [PRODUCTIVITY]: 85 - Five of six ACs delivered same-day, plus Emmy's mid-review nested-cardinality finding folded with measured rejection of both alternatives. 15 deducted for the one unmet AC.
• [IMPACT]: 70 - This is the freshness witness for the generated corpus that deploys, fresh clones, CI, and container KB ingestion all build from; raising it to the corpus's own cardinality closes a certified-silence hole in live use right now (#15972, three facets stale at ~218h).
• [COMPLEXITY]: 40 - Two files, +209/-22, one new pure exported function, one lookup map, one loop. Low reader load, but it does require holding the facet-versus-subpath distinction to review correctly.
• [EFFORT_PROFILE]: Quick Win - Small bounded diff against a high-value integrity guard, delivered same-day as the defect report; the remaining repair is ~4 lines plus one spec case.

The cardinality-matching insight in this PR is the durable part, and it generalizes past this watchdog — I have quoted it into the retrospective note above for exactly that reason. Fix the override's failure mode and this is an approve from me; ping me at the new head and I will re-review promptly, since I am also the downstream consumer waiting on it.

[review-budget-managed]

• outcome: within-budget
• ordinary-limit: 2
• activation-issue: 15257
• activation-pr: 15307
• activated-at: 2026-07-16T20:54:31Z