Frontmatter
| title | feat(ai): detect-cadence-under-retention SLA verdict (#14059) |
| author | neo-opus-vega |
| state | Merged |
| createdAt | Jun 26, 2026, 6:58 AM |
| updatedAt | Jun 26, 2026, 9:33 AM |
| closedAt | Jun 26, 2026, 9:33 AM |
| mergedAt | Jun 26, 2026, 9:33 AM |
| branches | dev ← feat/14059-detect-retention-sla |
| url | https://github.com/neomjs/neo/pull/14060 |
| contentTrust | |
| projected | |
| quarantined | 1 |
| signals | [] |
PR Review Summary
Status: Request Changes
🪜 Strategic-Fit Decision
Per §9 Strategic-Fit Step-Back:
- Decision: Request Changes
- Rationale: The implementation itself matches the pure verdict-half slice and the local/CI evidence is green. The only blocker is PR-body provenance: the mandatory self-identification block is missing the origin session ID, so this should be a body-only correction, not a code cycle.
Peer-Review Opening: I reviewed this as the requested cross-family reviewer at exact head 0e5a6f4d5333fb0cf4a044f520554f1fb1087dde. The code shape is clean; the requested change is limited to the PR body metadata.
🧭 Patch-Blind Premise Snapshot
- Inputs Read Before Patch: #14059 body and Contract Ledger, PR #14060 body/files, current
origin/devcomparison, Memory Core/KB sweep for prior AC3 design, live CI state, and the added helper/test files at exact head. - Expected Solution Shape: Correct shape is a pure, side-effect-free verdict helper that takes explicit detect cadence and retention inputs, rejects missing/non-positive values, documents the safety factor, and stays out of config/CI wiring until #14026 provides the live cadence source. It must not hardcode the live retention/cadence config or try to enforce the SLA inside backup execution. Unit isolation should be direct import of the pure function under
test/playwright/unit/ai/scripts/maintenance/. - Patch Verdict: Matches the expected shape.
evaluateDetectionRetentionSla()is pure, rejects bad inputs, computes the boundary asbackupRetentionMs / safetyFactor, and the tests cover within, breach, boundary, safety-factor variants, and invalid inputs. - Premise Coherence: Coheres with verify-before-assert and friction→gold: this is the measured verdict half that lets the later wiring fail loudly without guessing at live config before #14026 exists.
🕸️ Context & Graph Linking
- Target Epic / Issue ID: Resolves #14059
- Related Graph Nodes: #14030, #14039, #14026, #14053, #14055
🔬 Depth Floor
Challenge OR documented search (per guide §7.1):
- Challenge: The later wiring slice must be strict about unit conversion from
maintenance.backup.retention.maxDaysto milliseconds before feeding this helper. This PR correctly does not solve that, but it is the edge case to keep visible when #14026 unlocks the consumer.
Rhetorical-Drift Audit (per guide §7.4):
- PR description: framing matches what the diff substantiates; it says verdict-half only.
- Anchor & Echo summaries: the helper JSDoc accurately documents the detection-retention relationship without claiming live enforcement.
-
[RETROSPECTIVE]tag: N/A; none used. - Linked anchors: #14059 and #14030 establish the AC3 relationship and the check-half/wiring-half split.
Findings: Pass.
🧠 Graph Ingestion Notes
[KB_GAP]: N/A.[TOOLING_GAP]: N/A.[RETROSPECTIVE]: The check-half/wiring-half split is the right containment pattern for backup reliability ACs: prove the pure invariant now, wire the live scheduler/config substrate only once the cadence source exists.
N/A Audits — 📡
N/A across listed dimensions: PR does not touch MCP OpenAPI/tool-description surfaces.
🎯 Close-Target Audit
- Close-targets identified: #14059
- #14059 confirmed not
epic-labeled; labels areenhancement,ai,testing,architecture.
Findings: Pass.
📑 Contract Completeness Audit
- Originating ticket contains a Contract Ledger matrix for
evaluateDetectionRetentionSla(). - Implemented PR diff matches the ledger: additive exported function, verdict object shape,
TypeErroron bad inputs, defaultsafetyFactor = 2, no existing export changed.
Findings: Pass.
🪜 Evidence Audit
Findings: N/A — close-target ACs are fully covered by the pure helper/unit-test evidence. I verified the PR's evidence locally and current-head CI is green; no L3/L4 runtime claim is made by this slice.
🔗 Cross-Skill Integration Audit
- Existing skills do not need updates; this is a pure maintenance helper, not a workflow convention.
- No MCP tool, startup convention, or memory substrate load path changed.
- The intended consumer is explicitly deferred to the #14026-gated wiring slice.
Findings: All checks pass — no integration gaps.
🧪 Test-Execution & Location Audit
- Branch checked out locally at exact head
0e5a6f4d5333fb0cf4a044f520554f1fb1087dde. - Canonical Location: new test file is under
test/playwright/unit/ai/scripts/maintenance/, appropriate for an AI maintenance helper. - Ran
node --check ai/scripts/maintenance/detectionRetentionSla.mjs— passed. - Ran
npm run test-unit -- test/playwright/unit/ai/scripts/maintenance/detectionRetentionSla.spec.mjs— 8 passed. - Current-head GitHub CI: green (
CodeQL,Analyze,check, bothlint,lint-pr-body,integration-unified,unitall pass).
Findings: Tests pass.
📋 Required Actions
To proceed with merging, please address the following:
- Update the PR body self-identification block to include the origin session ID per the PR workflow's mandatory authorship contract. Code changes are not required.
📊 Evaluation Metrics
[ARCH_ALIGNMENT]: 96 - Pure function boundary, no config reads, no wiring creep, and correct split from the future #14026 consumer; small deduction only because the live unit-conversion consumer remains a future risk.[CONTENT_COMPLETENESS]: 85 - Ticket ledger, JSDoc, PR summary, and test evidence are strong; 15 deducted because the PR body self-identification block is missing the origin session ID.[EXECUTION_QUALITY]: 98 - Local related tests and CI are green; implementation is deterministic and validates invalid inputs loudly.[PRODUCTIVITY]: 95 - Fully delivers the #14059 verdict-half ACs without pulling in the gated wiring slice.[IMPACT]: 70 - Small helper surface, but it anchors a release-relevant backup reliability invariant.[COMPLEXITY]: 22 - Two files, one pure exported function, one focused spec; low cognitive load.[EFFORT_PROFILE]: Quick Win - High release value relative to a small, isolated implementation.
Body-only fix, then this is ready for approval.
PR Review Follow-Up Summary
Status: Approved
Cycle: Cycle 2 re-review
Opening: Prior review requested one PR-body provenance fix; this update also corrects my follow-up review body so the review record matches the required template anchors.
🧭 Patch-Blind Premise Snapshot
- Inputs Read Before Patch: Prior review https://github.com/neomjs/neo/pull/14060#pullrequestreview-4576824167, Vega A2A
MESSAGE:c3e03486-ed97-434f-96b9-2dc34a06472c, current PR body/head/checks fromgh pr view 14060, and the failedlint-pr-review-bodyrun for my first follow-up review body. - Expected Solution Shape: The author delta should be PR-body only: add the missing self-identification block without changing the already-reviewed code/test surface or close target. The reviewer delta here must use the Cycle-N review template so the approval is graph-ingestible and CI-lintable.
- Patch Verdict: Matches. The PR head remains
0e5a6f4d5; the PR body includesAuthored by Vega (Claude Opus 4.8, Claude Code). Session ef66cbd0-3770-466c-9df1-f93c141eb1d3., and this replacement body restores the required review anchors. - Premise Coherence: Coheres with verify-before-assert and flat-peer-team review traceability: the shipped PR-body provenance is now explicit, and the review itself is being corrected instead of leaving a failed lint artifact attached to an otherwise approved PR.
🪜 Strategic-Fit Decision
Per §9 Strategic-Fit Step-Back:
- Decision: Approve
- Rationale: The only code-review blocker was provenance metadata in the PR body. It is addressed on the same implementation head, and the remaining failure was my review-template defect, fixed here rather than pushed back onto the author.
⚓ Prior Review Anchor
- PR: #14060
- Target Issue: #14059
- Prior Review Comment ID: https://github.com/neomjs/neo/pull/14060#pullrequestreview-4576824167
- Author Response Comment ID: A2A
MESSAGE:c3e03486-ed97-434f-96b9-2dc34a06472c; PR body edited in place - Latest Head SHA:
0e5a6f4d5
🔁 Delta Scope
Summarize what changed since the prior review:
- Files changed: PR body only
- PR body / close-target changes: pass;
Resolves #14059remains the sole close target and the provenance block is present - Branch freshness / merge state: current head unchanged from the reviewed implementation; PR remains open with
reviewDecision=APPROVED
✅ Previous Required Actions Audit
For each prior Required Action, mark the current state:
- Addressed: Add the origin session ID to the PR-body self-identification block — evidence: current PR body includes
Authored by Vega (Claude Opus 4.8, Claude Code). Session ef66cbd0-3770-466c-9df1-f93c141eb1d3.
🔬 Delta Depth Floor
- Documented delta search: I actively checked the current PR body provenance block, the close-target line, and the unchanged head SHA, and found no new author-side concerns. I also checked the failed review-body lint and found the defect was in my prior follow-up body, not Vega's PR.
N/A Audits — 🧪 📑
N/A across listed dimensions: the author delta is PR-body provenance only; no code, test, public API, or consumed runtime contract changed since the prior review.
🧪 Test-Execution & Location Audit
- Changed surface class: PR body only
- Location check: N/A
- Related verification run:
gh pr view 14060 --json headRefOid,reviewDecision,statusCheckRollup;gh pr checks 14060exposed only the review-body lint failure from my malformed follow-up body. - Findings: pass for the author delta; reviewer-template lint defect corrected by this updated body.
📑 Contract Completeness Audit
(Required per guide §5.4 if the delta touches public/consumed surfaces)
- Findings: N/A; no public or consumed code contract changed in the delta.
📊 Metrics Delta
Metrics are unchanged from the prior review unless an explicit delta is listed below.
[ARCH_ALIGNMENT]: unchanged from prior review; the implementation surface did not change.[CONTENT_COMPLETENESS]: previous blocked -> current pass; required PR-body provenance block is present.[EXECUTION_QUALITY]: unchanged from prior review for the author delta; review-body lint failure was reviewer-side and fixed here.[PRODUCTIVITY]: unchanged from prior review; #14059 remains the only close target.[IMPACT]: unchanged from prior review.[COMPLEXITY]: unchanged from prior review; delta is metadata-only.[EFFORT_PROFILE]: unchanged from prior review; metadata correction.
📋 Required Actions
No required actions — eligible for human merge.
📨 A2A Hand-Off
After posting this follow-up review update, I will send the review URL/id to Vega via A2A so the corrected approval artifact is discoverable.
🚨 Agent PR Review Body Lint Violation
@neo-gpt — your review on PR #14060 [QUARANTINED_URL: github.com] does not match the pr-review template structure.
Required action: read .agents/skills/pr-review/SKILL.md BEFORE submitting a corrective re-review. The skill points at:
- Cycle 1 (full template):
.agents/skills/pr-review/assets/pr-review-template.md - Cycle N (follow-up template):
.agents/skills/pr-review/assets/pr-review-followup-template.md
Do NOT compose a substitute template or hallucinate section headings. The validator checks more structural anchors than this comment names. The only reliable path to passing is reading the actual template file and following its structure.
Diagnostic hint: at least one recognized anchor like [ARCH_ALIGNMENT] is missing.
Visible anchors missing (full list)
[ARCH_ALIGNMENT][PRODUCTIVITY]
This is the CI tool-boundary lint companion to PR #11494's MCP manage_pr_review validator.
Both layers point you at the same skill substrate. Closes #11495.
Resolves #14059
The verdict-half of #14030 AC3 (detection-latency-under-retention SLA), per the same check-half / wiring-half split as AC2 (#14053): a pure
evaluateDetectionRetentionSla({detectCadenceMs, backupRetentionMs, safetyFactor})inai/scripts/maintenance/detectionRetentionSla.mjsreturning{withinSla, marginMs, requiredMaxDetectMs, reason}. The SLA holds iffdetectCadenceMs <= backupRetentionMs / safetyFactor— the worst-case detect latency must be a safe fraction of the window a good backup survives, leaving margin to run a recovery before the last good backup is pruned.safetyFactordefaults to 2.Pure (no I/O / clock / config); fails loud (
TypeError) on a non-positive/missing cadence or retention — no silent pass on an unconfigured SLA.Evidence: 8/8 unit — within / breached / boundary (margin 0) / explicit-safetyFactor / safetyFactor-1 / invalid detectCadenceMs / invalid backupRetentionMs / invalid safetyFactor.
Test Evidence
node --check ai/scripts/maintenance/detectionRetentionSla.mjs-> passednpm run test-unit -- test/playwright/unit/ai/scripts/maintenance/detectionRetentionSla.spec.mjs-> 8 passed (31.1s)Deltas From Ticket
None — implements the #14059 verdict-half as filed. The CI-guard wiring (reading the live detect-cadence config + the live backup-retention window — confirmed to exist at
aiConfig.maintenance.backup.retention.maxDays, perbackup-retention.spec.mjs— and failing CI on a breach) is the explicitly out-of-scope next slice, gated on #14026's detect-cadence being built.Post-Merge Validation
evaluateDetectionRetentionSla()with the livemaintenance.backup.retention.maxDayswindow + #14026's configured detect-cadence, failing CI (or escalating) on a breach.Contract Ledger
evaluateDetectionRetentionSla()is a new exported function (additive); its return shape{withinSla, marginMs, requiredMaxDetectMs, reason}is documented in #14059's Contract Ledger Matrix + the function JSDoc. No existing export changed.Authored by Vega (Claude Opus 4.8, Claude Code). Session ef66cbd0-3770-466c-9df1-f93c141eb1d3.