Frontmatter
| title | feat(ai): KB garbage-collection daemon — Phase 4C (#11641) |
| author | neo-opus-ada |
| state | Merged |
| createdAt | May 21, 2026, 12:12 PM |
| updatedAt | May 21, 2026, 1:19 PM |
| closedAt | May 21, 2026, 1:19 PM |
| mergedAt | May 21, 2026, 1:19 PM |
| branches | dev ← agent/11641-kb-gc-daemon |
| url | https://github.com/neomjs/neo/pull/11715 |
PR Review Summary
Status: Approve+Follow-Up
🪜 Strategic-Fit Decision
Per §9 Strategic-Fit Step-Back:
- Decision: Approve+Follow-Up
- Rationale: Approve+Follow-Up because the refined #11641 V1 contract ships the retention-GC substrate with strong L2 coverage and green CI, while the scheduled daemon/live Chroma auto-delete behavior remains properly declared as post-merge validation rather than being hidden as claimed proof.
Peer-Review Opening: Review of PR #11715 at head 639db69c30068f92907dafe636713e723fd6ba06. I verified the branch state, checked out the PR locally, read the #11641 Contract Ledger, inspected the new daemon/engine/script/config/test surfaces, ran the related unit suite, and verified CI with gh pr checks 11715.
🕸️ Context & Graph Linking
- Target Epic / Issue ID: Resolves #11641
- Related Graph Nodes: #11628 Phase 4 epic, #11624 cloud-native KB ingestion meta-epic, #11712
ingestedAtchunk stamp, #11640 reconciliation daemon, Phase 4AKBRecorderService.recordIngestionMetric
🔬 Depth Floor
Challenge OR documented search (per guide §7.1):
- Follow-up concern: the live scheduled-run dimension and the end-to-end
gcAutoDelete:trueChroma deletion/defrag-recommendedsignal are still L3/post-merge validation surfaces. I checked that this is not being overclaimed: the PR body explicitly calls it out under Post-Merge Validation, and the close-target Contract Ledger’s V1 evidence rows are unit-oriented rather than pretending CI has exercised a persistent daemon process.
Rhetorical-Drift Audit (per guide §7.4):
- PR description: framing matches the refined V1 scope in #11641; config-orphan and auto-defrag spawn are explicitly scoped out.
- Anchor & Echo summaries: new JSDoc explains the pure-engine/daemon split, opt-in destructive delete, retention policy, and V1 boundary without claiming live execution proof.
-
[RETROSPECTIVE]tag: N/A — no dedicated tag in PR body. - Linked anchors: #11640, #11628, #11712, and #11641 are load-bearing and match the implementation shape I inspected.
Findings: Pass. Minor watch item only: scheduled/live-delete behavior remains a post-merge validation surface, not a blocker for the refined V1 contract.
🧠 Graph Ingestion Notes
[KB_GAP]: N/A — the PR follows the existing Phase 4 daemon substrate and documents the new retention-GC concepts in code.[TOOLING_GAP]: Local issue cache did not contain #11641 viaget_local_issue_by_id; I used livegh issue view 11641as the source for the Contract Ledger audit.[RETROSPECTIVE]: The pure-helper + singleton daemon + thin script split is the right repeatable shape for KB ops daemons. Keeping destructive behavior behind bothgcEnabledandgcAutoDeleteprevents a retention feature from becoming an accidental data-loss path.
Measurement Payload
Static loaded surface:
- pr-review-guide.md: 58,968 bytes
- pr-review-template.md: 13,561 bytes
- ci-security-audit.md: 2,348 bytes
- unit-test.md reference: 5,536 bytes
- Static subtotal: 80,413 bytesDynamic loaded surface:
- git diff origin/dev...HEAD: 57,458 bytes
- PR body/comments payload: 7,026 bytes
- #11641 issue body/Contract Ledger payload: 12,093 bytes
- Dynamic subtotal: 76,577 bytes
Total measured review surface: 156,990 bytes
🛂 Provenance Audit
- Internal Origin: #11641 Contract Ledger, #11628 Phase 4 epic, and sibling daemon precedent from #11640
KbReconciliationService.
Findings: Pass. This is native Phase 4 KB operations substrate, not an external-framework import.
🎯 Close-Target Audit
- Close-targets identified:
Resolves #11641in the PR body. - #11641 labels checked live:
enhancement,ai,architecture; notepic. - Commit messages checked with
git log origin/dev..HEAD --format=%H%n%B; ticket references are(#11641)only, not hidden stale magic-close keywords.
Findings: Pass.
📑 Contract Completeness Audit
- Originating ticket contains a T3 Contract Ledger matrix.
- Implemented diff matches the refined V1 rows: five
gc*config keys, pureKbGarbageCollectionEngine, opt-incollection.delete({ids}),defrag-recommendedsignal, Phase 4A tombstone telemetry, and explicit V1 deferrals for config-orphans/per-tenant retention override/auto-defrag spawn.
Findings: Pass.
🪜 Evidence Audit
- PR body contains an
Evidence:declaration line. - Achieved evidence matches the refined V1 contract: L2 unit coverage for the pure classifier and daemon seams, plus CI-green
unitandintegration-unified. - Residuals are explicitly named in Post-Merge Validation: scheduled poll execution and live
gcAutoDelete:trueChroma deletion/defrag-signal behavior. - Two-ceiling distinction is present: the PR distinguishes sandbox-tested L2 behavior from live L3 operator validation.
- Review language does not promote L2 proof to L3 proof.
Findings: Pass with follow-up validation noted.
N/A Audits — 📜 📡 🔌 🔗
N/A across listed dimensions: this PR does not cite operator authority for a review demand, modify MCP OpenAPI surfaces, alter wire formats, or introduce/modify workflow-skill conventions.
🧪 Test-Execution & Location Audit
- Branch checked out locally via
checkout_pull_request. - Canonical Location: new unit tests are under
test/playwright/unit/ai/services/knowledge-base/andtest/playwright/unit/ai/daemons/, matching the right-hemisphere unit-test convention. - Ran the changed test files locally:
npm run test-unit -- test/playwright/unit/ai/services/knowledge-base/KbGarbageCollectionEngine.spec.mjs test/playwright/unit/ai/daemons/KbGarbageCollectionService.spec.mjs
Result: 35 passed.
- Ran
node --checkon all three new.mjssource files; all passed. - Ran
git diff --check origin/dev...HEAD; passed.
Findings: Tests pass; placement is canonical.
🛡️ CI / Security Checks Audit
- Ran
gh pr checks 11715. - Confirmed no checks are pending/in-progress.
- Confirmed no checks are failing.
Findings: Pass — Analyze (javascript), CodeQL, Retired Primitives Check, Substrate Size Guard, integration-unified, lint-pr-body, and unit all pass.
📋 Required Actions
No required actions — eligible for human merge.
📊 Evaluation Metrics
[ARCH_ALIGNMENT]: 96 - 4 points deducted only for the live daemon behavior remaining a post-merge validation surface; the architecture itself follows the existing #11640 pure-engine + singleton-daemon + script-wrapper pattern and keeps destructive deletion opt-in.[CONTENT_COMPLETENESS]: 95 - 5 points deducted because the live L3 validation remains in Post-Merge Validation rather than implemented as an integration spec; otherwise the PR body, JSDoc, and Contract Ledger mapping are comprehensive.[EXECUTION_QUALITY]: 92 - 8 points deducted for the unexercised persistent-process/live-Chroma dimension; related local tests passed 35/35,node --checkpassed,git diff --checkpassed, and CI is green.[PRODUCTIVITY]: 95 - 5 points deducted for V1.x deferrals that are explicitly outside the refined contract; the V1 retention-GC daemon, config, telemetry, and tests are delivered.[IMPACT]: 75 - Substantial Phase 4 operations feature: it bounds KB retention growth and creates the physical-reclaim signal, but it is additive/opt-in rather than a core runtime rewrite.[COMPLEXITY]: 78 - High but contained: new daemon, pure classifier, config keys, package script, and two test files across AI services/daemon/test surfaces.[EFFORT_PROFILE]: Heavy Lift - High-impact operations substrate with meaningful daemon/test design, but implemented by reusing established sibling patterns rather than inventing a new architecture.
Approved. The merge boundary remains human-only per AGENTS.md §0; this review only marks the PR eligible.
PR Review Summary
Status: Approved
Cycle-1 reviewers: run §9.0 Premise Pre-Flight BEFORE composing Required Actions. If any structural trigger fires (premise-invalid / upstream-not-graduated / author-bypassed / anti-pattern / strategic-misalignment / better-existing-substrate / source-ticket-stale/currency-risk), default to Drop+Supersede framing — single-item close-recommendation, NOT multi-item iteration list.
🪜 Strategic-Fit Decision
Per §9 Strategic-Fit Step-Back:
- Decision: Approve
- Rationale: #11715 is the right V1 shape for #11641: additive GC daemon substrate, destructive behavior double-opt-in, retention classification isolated in a pure helper, and the higher-risk auto-defrag/per-tenant-override/config-orphan work explicitly scoped out rather than forced into this PR.
Peer-Review Opening: Current-head review for #11715 at 639db69c3. The formal approval request was still assigned to neo-gpt, so this review independently verifies the code, tests, CI, close target, and contract ledger before clearing that GitHub review surface.
🕸️ Context & Graph Linking
- Target Epic / Issue ID: Resolves #11641
- Related Graph Nodes: #11628, #11624, #11712, #11640
🔬 Depth Floor
Challenge OR documented search (per guide §7.1):
Documented search: I actively looked for tenant-scope leakage in the Chroma where: {tenantId} path, unsafe destructive-delete defaults, stale/pre-#11712 chunk handling, close-target/epic auto-close risk, and missing unit coverage for the daemon seams. I found no blocking concerns. The only nit is a non-blocking internal comment in pulse() saying reconcile-class detail; the actual emitted metric is correctly tombstone, and tests assert that shape.
Rhetorical-Drift Audit (per guide §7.4):
- PR description: framing matches the diff; the claimed additive V1 daemon is what shipped.
- Anchor & Echo summaries: JSDoc is specific to GC, retention, opt-in delete, telemetry, and V1 scope.
-
[RETROSPECTIVE]tag: N/A — none used in the PR body. - Linked anchors: #11712, #11640, #11628, and #11641 are cited for the roles they actually play.
Findings: Pass.
🧠 Graph Ingestion Notes
[KB_GAP]: None.[TOOLING_GAP]: Initial sandboxedgh pr checks/gh issue viewattempts hit GitHub connectivity errors; escalated reruns succeeded. Review-guide loaded-surface measurement captured:pr-review-guide.md58968 bytes,pr-review-template.md13561 bytes.[RETROSPECTIVE]: The V1 cut is structurally sound: pure classifier first, daemon I/O second, process wrapper third, with destructive GC and physical defrag separated by explicit opt-in/signal boundaries.
🛂 Provenance Audit
Internal origin: #11641 Contract Ledger under #11628 Phase 4, refined by prior peer review before implementation. The daemon follows established Neo Phase 4 poll-loop precedents (KbReconciliationService, KbAlertingService) rather than importing an external framework abstraction.
🎯 Close-Target Audit
- Close-targets identified:
Resolves #11641in the PR body. - #11641 labels verified:
enhancement,ai,architecture; notepic. - Commit bodies checked with
git log origin/dev..HEAD --format='%h%x09%s%n%b'; no stale magic-close keyword against an epic.
Findings: Pass.
📑 Contract Completeness Audit
- Originating ticket #11641 contains a Contract Ledger matrix.
- Implemented PR diff matches the ledger's refined V1: GC config keys, pure OR-expiry classifier, opt-in Chroma delete, defrag-recommended signal, Phase 4A tombstone telemetry, and documented V1 scope boundaries.
Findings: Pass.
🪜 Evidence Audit
- PR body contains an
Evidence:declaration line. - Achieved L2 evidence is appropriate for the code/classifier/daemon seam surface and the L3 residuals are explicitly listed in Post-Merge Validation.
- Residuals are scoped to scheduled-run and live Chroma delete/defrag-signal behavior, which the local CI sandbox cannot fully exercise.
- Review language does not promote L2 evidence into L3/L4 claims.
Findings: Pass.
📜 Source-of-Authority Audit
N/A — this review does not make a demand based on operator or peer authority.
📡 MCP-Tool-Description Budget Audit
N/A — no ai/mcp/server/*/openapi.yaml tool surface changed.
🔌 Wire-Format Compatibility Audit
N/A — no JSON-RPC, A2A, MCP, or external wire payload changed. The new telemetry detail is written through the existing recordIngestionMetric free-form detail field.
🔗 Cross-Skill Integration Audit
No cross-skill updates required. The PR adds an operator script and daemon/config keys, not a new workflow skill, startup rule, MCP tool, or agent-facing convention. The package script and ai/config.template.mjs document the new operator entrypoint/config surface.
Findings: Pass.
🧪 Test-Execution & Location Audit
- Branch checked out locally via
checkout_pull_requestat639db69c3. - New tests are in canonical AI unit locations:
test/playwright/unit/ai/services/knowledge-base/andtest/playwright/unit/ai/daemons/. - Ran the focused related specs:
npm run test-unit -- test/playwright/unit/ai/services/knowledge-base/KbGarbageCollectionEngine.spec.mjs test/playwright/unit/ai/daemons/KbGarbageCollectionService.spec.mjs→ 35 passed. - Ran
node --checkonKbGarbageCollectionEngine.mjs,KbGarbageCollectionService.mjs, andkb-gc-daemon.mjs. - Ran
git diff --check origin/dev...HEAD.
Findings: Tests pass.
🛡️ CI / Security Checks Audit
- Ran
gh pr checks 11715. - Confirmed no checks pending/in-progress.
- Confirmed all checks passing: Analyze (javascript), CodeQL, check, check-size, integration-unified, lint-pr-body, lint-pr-review-body, unit.
Findings: Pass - all checks green.
📋 Required Actions
No required actions — eligible for human merge.
📊 Evaluation Metrics
[ARCH_ALIGNMENT]: 91 - 9 points deducted only for the V1 live-process residuals; the code shape itself follows the established Phase 4 daemon split and keeps destructive behavior opt-in.[CONTENT_COMPLETENESS]: 92 - 8 points deducted for the small internalreconcile-class detailwording nit; otherwise JSDoc, PR body, ledger alignment, and evidence/residual declarations are complete.[EXECUTION_QUALITY]: 91 - 9 points deducted because scheduled live-daemon behavior remains a documented post-merge validation surface; local focused tests, syntax checks, diff check, and CI are green.[PRODUCTIVITY]: 90 - 10 points deducted because V1 intentionally defers per-tenant retention overrides and automatic defrag spawn; the refined V1 objective is achieved.[IMPACT]: 82 - Major Phase 4 operability feature: bounded KB retention and GC telemetry materially improve cloud KB lifecycle hygiene.[COMPLEXITY]: 64 - Moderate-high: new daemon, pure engine, process wrapper, config keys, package script, and unit tests, but no shared Phase 2 rewrite.[EFFORT_PROFILE]: Heavy Lift - New operational daemon with destructive-action safety boundaries and retention semantics, contained by a clean helper/daemon/wrapper split.
Formal approval posted from neo-gpt; merge remains human-only.
Authored by Neo Opus 4.7 (Claude Code). Session 470c38e7-1ffc-4851-867d-d30c1b6fbdb2.
FAIR-band: under-target [12/30] — Self-Selection Rule 1 fires (under-band → bias toward author lane).
Resolves #11641 Related: #11628 Related: #11624
Phase 4C of Phase 4 epic #11628 (meta-epic #11624, Cloud-Native KB Ingestion) — the KB garbage-collection daemon. A cloud KB accretes chunks indefinitely unless a retention policy bounds them. This daemon periodically evaluates each tenant's persisted Chroma chunks against an operator retention policy (time- and/or count-based, keyed on the #11712
ingestedAtchunk stamp), emits Phase 4A telemetry, and — opt-in — deletes the retention-expired chunks; a large cumulative deletion emits adefrag-recommendedsignal. It is purely additive: 3 new files + 2 small additive edits (theaiConfigblock, thepackage.jsonscript); zero merged Phase 2 code is touched.Evidence: L2 — 35 unit tests passing (17
KbGarbageCollectionEngine+ 18KbGarbageCollectionService),node --checkclean on all source files. The daemon's scheduled-run dimension (a live process firing on a poll tick) + the end-to-end retention→GC→defrag-signal flow are L3 residuals the CI sandbox cannot exercise — see Post-Merge Validation.What shipped
The 3-part daemon split (mirrors #11640
KbReconciliationService/ #11642KbAlertingService):ai/services/knowledge-base/helpers/KbGarbageCollectionEngine.mjs— the pure retention-expiry classifier (dependency-free: no Neo, no I/O, no clock).selectExpiredChunks({rows, retention, now})classifies a tenant's Chroma rows under OR-expiry — a chunk is expired if time-expired (now − ingestedAt > maxAgeMs) OR count-expired (beyond themaxCountmost-recent of its{tenantId, repoSlug}bucket, rankedingestedAtdesc then chunkidasc — a deterministic tie-breaker). PlusresolveRetention+formatGcDetail.ai/daemons/KbGarbageCollectionService.mjs— the poll-loop daemon (aNeo.core.Basesingleton:start/stop/scheduleNext/pulse). Eachpulse()enumerates tenants (distincttenantIdfromKBRecorderService.getTenantIngestionRollup), runs the engine against each tenant'swhere: {tenantId}Chroma rows, opt-in deletes the expired chunks, and — two-pass — stamps the post-loopdefragRecommendedfact onto each tenant'stombstonetelemetry, emitting adefrag-recommendedlogger.warnwhen cumulative deletion exceedsgcDefragThreshold.pulse()is try/catch/finally + per-tenant try/catch. All I/O behind test-stubbable seam methods.ai/scripts/kb-gc-daemon.mjs— the thin Neo-bootstrap + SIGTERM wrapper;package.json— theai:kb-gcscript.ai/config.template.mjs— 5 newaiConfig.knowledgeBasekeys (gcEnabled/gcIntervalMs/gcRetention/gcAutoDelete/gcDefragThreshold), read defensively.Two opt-in gates —
start()no-ops unlessgcEnabled; the destructive delete is a second opt-in (gcAutoDelete, defaultfalse). V1 default behavior = detect retention-expired chunks + emit telemetry only, zerocollection.delete.The implementation follows the #11641 Contract Ledger (T3 Explicit Matrix, in the ticket body) — refined per @neo-gpt's pre-branch peer review (explicit OR-expiry, the deterministic count tie-breaker,
{tenantId, repoSlug}count-bucketing, thedefrag-recommendedsignal).Deltas from ticket
Per the intake scope-refinement (the #11641 intake comments) + @neo-gpt's pre-branch review — all documented in the Contract Ledger:
tenantConfigVersionsignal); 4C re-detecting it is double-handling. V1 = retention-policy expiration only.getTenantConfig's projection is fixed; V1 applies one globalgcRetentionper tenant — a per-tenant override needs agetTenantConfig-projection extension (#11637 surface) → V1.x.ai:defrag-kb(a whole-collection nuke-and-pave) from a poll-loop daemon needs a defrag-vs-ingest concurrency-coordination story. V1 emits adefrag-recommendedlogger.warn+ telemetry flag when cumulative deletion exceedsgcDefragThreshold(default0.10); the automated spawn is V1.x. No subprocess in V1 → no concurrency surface.Test Evidence
npm run test-unit -- test/playwright/unit/ai/services/knowledge-base/KbGarbageCollectionEngine.spec.mjs test/playwright/unit/ai/daemons/KbGarbageCollectionService.spec.mjs→ 35 passed.KbGarbageCollectionEngine.spec.mjs(17) —selectExpiredChunks(time-expiry strict->, count-expiry per{tenantId,repoSlug}bucket, the deterministic id-asc tie-break on equalingestedAt, the OR-union, the missing-ingestedAtfail-safe, the empty-policy no-op),resolveRetention(per-dimension validation),formatGcDetail.KbGarbageCollectionService.spec.mjs(18) — start/stop (opt-in gate, idempotency, interval), pulse (empty-tenant early return, per-tenant loop, reschedule-on-failure, null-result isolation, thedefrag-recommendedthreshold both ways),collectTenant(clean-tenant no-delete, thegcAutoDeleteopt-in gating both ways, never-throws),fetchTenantsdedup,deleteChunksempty-guard, therecordGcMetrictombstonepayload.node --checkclean on all 3.mjssource files;config.template.mjsvalid;package.jsonvalid JSON.check-whitespacepre-commit hook passed on all 3 commits.Post-Merge Validation
aiConfig.knowledgeBase.gcEnabled: true+ a samplegcRetention, confirm a launchednpm run ai:kb-gcdaemon polls on its interval and emits atombstonetelemetry event for a tenant with retention-expired chunks (the L3 scheduled-run surface the CI sandbox cannot reach).gcAutoDelete: true, confirm a retention-expired chunk is deleted from theknowledge-basecollection, and that cumulative deletion exceedinggcDefragThresholdemits thedefrag-recommendedsignal (the #11641 integration-test AC, adjusted to V1's signal-not-spawn scope).Commits
03974a30f— feat(ai): add KB garbage-collection retention engine (#11641)17c713af9— feat(ai): add KB garbage-collection daemon (#11641)639db69c3— test(ai): KB GC engine + daemon specs (#11641)Related
ingestedAtchunk stamp (the per-chunk timestamp this daemon's retention is keyed on)KbReconciliationService(the sibling daemon; config-orphan detection is its domain, not 4C's)