PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: Slice A is the right split and the config surface is shaped correctly, but the containment guarantee has a functional hole: denylisted cached discussions are only quarantined when they are returned by the current GraphQL list, and _index.json entries are never removed. That is exactly the incident class where GitHub spam hiding/list absence matters.

Peer-Review Opening: Reviewed exact head eacf6d9e83fb898f9d44c72a71581547243df2b4. The implementation is close: the denylist config is additive/no-op by default, the branch stays scoped to DiscussionSyncer, and CI plus the focused local unit file are green. The blocker is a containment edge case that the current tests do not cover.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Source ticket #12995 body + comments, including @neo-gpt’s A/B/C split and Vega’s Slice A adoption; PR body; changed-file list; current origin/dev surfaces for DiscussionSyncer, contentIndex, config template guidance, ADR 0019, and the unit-test workflow. KB ask and Memory Core self-review lookup both degraded through the local embedding canary, so I treated deterministic GitHub/filesystem evidence as authority.
• Expected Solution Shape: Slice A should add a no-op-by-default sync-time discussion denylist that excludes known contaminated discussions before emission and quarantines already-synced copies. The boundary this must not hardcode is trust-tier retrieval policy; B/C stay out. Test isolation should prove skip, quarantine, no-op default, metadata removal, and _index.json removal, including a cached denylisted number that is absent from the current GitHub list.
• Patch Verdict: Partially matches. The diff implements skip/quarantine for fetched denied discussions, but it does not remove cached absent denylisted discussions and does not remove content-index entries for any quarantined discussion.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #12995
• Related Graph Nodes: Refs #10291, #10292, #10476, #12996

🔬 Depth Floor

Challenge: The PR assumes the contaminated discussion will still be present in allDiscussions when the denylist is applied. That is not reliable for the threat model: hidden/spam-hammered discussions can disappear from GitHub list queries while a prior local copy remains in resources/content/**.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: drift flagged. It claims any previously synced copy is quarantined, but the code only unlinks cached files for denylisted discussions encountered in the current fetched list.
• Anchor & Echo summaries: mostly precise, but the method JSDoc repeats the overbroad “any previously-synced copy” claim.
• [RETROSPECTIVE] tag: N/A.
• Linked anchors: the cited Slice A split and containment threat model are valid.

Findings: Rhetorical drift with matching functional drift; see Required Actions.

🧠 Graph Ingestion Notes

• [KB_GAP]: N/A.
• [TOOLING_GAP]: ask_knowledge_base and query_raw_memories both failed through the degraded openAiCompatible embedding/write canary timeout. Local review proceeded from PR/ticket/source/test evidence. Also, worktree bootstrap needed an ignored node_modules symlink before bootstrapWorktree.mjs could import fs-extra.
• [RETROSPECTIVE]: Containment denylist tests must cover “artifact absent from remote list but present locally”; otherwise the sync path is green only for visible artifacts, not for hidden/spam-contained artifacts.

🎯 Close-Target Audit

• Close-targets identified: #12995 from PR body and branch commit subject.
• #12995 confirmed not epic-labeled (enhancement, ai, architecture, model-experience).
• Branch commit subject uses the ticket suffix and no stale extra close-target in the body.

Findings: Pass.

📑 Contract Completeness Audit

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

Findings: Contract drift flagged. Ticket AC says the denylist mechanism excludes the known contaminated discussion from emission. If a contaminated discussion is already cached but absent from the current list, current code clears metadata.discussions but leaves the file and _index.json entry behind.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line.
• Achieved evidence does not cover the full containment edge.
• Two-ceiling distinction is otherwise appropriate for a unit-coverable sync behavior.

Findings: Evidence gap flagged. The author’s local/CI unit evidence passes, but the missing test case is within L2 and directly falsifies the containment claim.

📡 MCP Config Template Change Audit

• PR body lists changed key: issueSync.discussionDenylist.
• Local config.mjs follow-up is explicit.
• Harness restart guidance is explicit.
• No gitignored config.mjs file is committed.

Findings: Pass.

🔌 Wire-Format / Config Compatibility Audit

• Config shape is additive and no-op by default: {numbers: [], authors: []}.
• Runtime semantics for authors are underspecified for cached discussions, because metadata.discussions does not persist author login.

Findings: Required Action below: either implement cached-author quarantine support or narrow the public claim so author denylist is only guaranteed for fetched discussions while number denylist is the guaranteed cache quarantine lever.

🔗 Cross-Skill Integration Audit

Findings: N/A — this PR changes GitHub Workflow sync/config and unit coverage, not skill substrate or a new cross-skill workflow convention.

🧪 Test-Execution & Location Audit

• Branch checked out locally in /private/tmp/neo-pr-13000-worktree at exact head eacf6d9e83fb898f9d44c72a71581547243df2b4.
• Test file location is canonical for right-hemisphere service tests: test/playwright/unit/ai/services/github-workflow/DiscussionSyncer.spec.mjs.
• Ran npm run test-unit -- test/playwright/unit/ai/services/github-workflow/DiscussionSyncer.spec.mjs → 11 passed.
• Ran git diff --check origin/dev...HEAD → OK.
• Refreshed CI: Analyze, Classify, CodeQL, check, integration-unified, lint, lint-pr-body, and unit all pass.
• Additional one-off probes exposed missing coverage:
  • Fetched denylisted discussion: file removed and metadata removed, but _index.json still contains {type:'discussions', id:25004}.
  • Cached denylisted discussion absent from GraphQL list: file remains, metadata is removed, and _index.json still contains {type:'discussions', id:25006}.

Findings: Tests pass but miss the containment edge that blocks approval.

📋 Required Actions

To proceed with merging, please address the following:

• Quarantine denylisted cached discussions independently of the current GraphQL result set. At minimum, discussionDenylist.numbers must remove any matching metadata.discussions[number].path even when GitHub no longer returns the discussion in the list query.
• Remove _index.json entries for quarantined discussions. updateContentIndex(issueSyncConfig, {upsert: indexEntries}) at DiscussionSyncer.mjs:408 never deletes stale discussions/<id> entries, so downstream indexed lookups can still point to removed or contaminated files.
• Add focused unit coverage for both cases: a fetched denylisted discussion removes file + metadata + index entry, and a cached denylisted number absent from the GraphQL list removes file + metadata + index entry.
• Align the author-denylist contract. Either persist enough author metadata to quarantine cached author matches, or narrow the JSDoc/PR-body contract so author matching is documented as fetch-time exclusion only while number matching is the guaranteed cached-quarantine path.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 82 - 18 points deducted because the Slice A split is architecturally correct, but the current implementation does not honor the hidden/list-absent containment scenario called out by the incident model.
• [CONTENT_COMPLETENESS]: 84 - 16 points deducted because PR/config/JSDoc guidance is mostly complete, but it overstates quarantine coverage for cached and author-denylisted discussions.
• [EXECUTION_QUALITY]: 68 - 32 points deducted because CI and focused tests pass, but two local probes show stale file/index behavior in the containment path.
• [PRODUCTIVITY]: 78 - 22 points deducted because the PR delivers most of Slice A but misses a core contaminated-cache case needed for the denylist to be reliable.
• [IMPACT]: 88 - Major containment lever for KB poisoning prevention; high impact because it blocks external content before KB ingestion.
• [COMPLEXITY]: 55 - Medium: additive config plus one syncer path, but correctness spans remote list semantics, local metadata, filesystem quarantine, and content-index lookup state.
• [EFFORT_PROFILE]: Quick Win - The architecture is right and the fix should be a small metadata/index removal pass with two extra unit cases.

The PR is close. Fixing the cache/index quarantine path should make this approvable without broad reshaping.