PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The fix is correct, minimal, and exactly what #15831 asked for. My one substantive note is house-pattern consistency, not correctness — and gating a correct three-line fix from a first-time contributor on polish would be the wrong trade. Noted below so the pattern is learnable rather than enforced.

Peer-Review Opening: Welcome, and thank you — this is a genuinely clean first contribution. You found the right line, the guard is in the right place, and you wrote a test that asserts on the classification result rather than on side effects, which is the harder instinct to have on a first pass. Notes below are learnable-pattern, not blocking.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #15831 (the ticket's own measurement — .DS_Store classified as alreadyLinked on a live machine), symlinkDataDir on current dev including every sibling skip branch, the function's @returns classification contract, and the live .neo-ai-data child set.
• Expected Solution Shape: A guard that keeps dot-entries out of the hydration set without weakening the deliberate blocklist-not-allowlist design, plus a test that fails against unfixed code. It must not silently change what the function reports, because callers consume the classification.
• Patch Verdict: Matches. The guard is placed before the blocklist check, which is correct — a dot-entry is not "deliberately seat-local", it is not substrate at all, so classifying it as blocklisted would be a different lie from the one being fixed.
• Premise Coherence: Coheres with verify-before-assert — the ticket measured the misclassification on a live machine before prescribing, and the test asserts the classification rather than the filesystem.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #15831
• Related Graph Nodes: symlinkDataDir's blocklist-not-allowlist contract; the reconcile-probe consumers of its classification (#15872, #15842)

🔬 Depth Floor

Challenge 1 — the new skip is the only silent one, and the function's own comment argues against that.

Every sibling branch both logs and records:

log(`symlink skip (blocklisted): ${name}`);   result.blocklisted.push(name);
log(`symlink skip (already linked): ${name}`); result.alreadyLinked.push(name);
log(`reconcile divergent (…): ${name} …`);     result.divergent.push({…});

and the blocklist branch carries this comment verbatim:

"A blocklisted child is deliberately seat-local … 'we skipped it' is not an observation."

The dot-entry continue records nothing and logs nothing, so a .DS_Store now vanishes from a return value whose entire job is classification ({linked, alreadyLinked, clobbered, skippedNoSource, blocklisted, divergent, seatOnly, resolved}). An operator reconciling a seat sees no trace that one was seen and deliberately ignored.

Why this is a note and not a required action: before your fix the entry was reported wrongly (alreadyLinked); after it, it is reported not at all. That is strictly better, and no consumer currently reads a "dot entries seen" signal. A one-line log() — or a result.skippedDotEntry bucket if you'd rather — would put it in the house pattern, and I'd merge it either way.

Challenge 2 — two assertions in the new test cannot fail.

expect(await fs.pathExists(path.join(fakeWorktree, dataDir, '.DS_Store'))).toBe(false);
expect(await fs.pathExists(path.join(fakeWorktree, dataDir, 'sqlite'))).toBe(false);

The call is dryRun: true, so nothing is created either way — the sqlite line proves that directly, since sqlite is in result.linked and still does not exist on disk. Both lines would pass against unfixed code.

The load-bearing assertions are the ones above them, and those are exactly right:

expect(result.linked).toContain('sqlite');
expect(result.linked).not.toContain('.DS_Store');
expect(result.alreadyLinked).not.toContain('.DS_Store');

alreadyLinked is the sharp one — it is the bucket #15831 measured the bug in, so that line is the red proof. Worth knowing the distinction rather than changing anything: a passing assertion that could not have failed reads as coverage without being it, and this is a house-wide sharp edge, not a beginner's mistake. (Several of us shipped exactly this shape today.)

🧠 Graph Ingestion Notes

• [KB_GAP]: None.
• [TOOLING_GAP]: None.
• [RETROSPECTIVE]: The ticket is the reason this review is short. It measured the defect on a live machine, named the design decision that caused it (blocklist-not-allowlist, deliberate and correct), and scoped the fix to three lines — so the patch had almost no room to drift. A well-shaped ticket does most of a reviewer's work before the reviewer arrives.

🎯 Close-Target Audit

• Close-targets identified: #15831
• Confirmed not epic-labeled

Findings: Pass.

N/A Audits — 📑 🪜 📡 🔗

N/A across listed dimensions: a three-line guard in a build/migration script touches no contract ledger, no OpenAPI description, no skill or convention surface, and its close-target ACs are covered by the unit test.

🧪 Test-Evidence & Location Audit

• Execution evidence: the new spec sits in the canonical location beside the existing symlinkDataDir suite and follows its fakeMainCheckout / seedMainSubdirs fixture idiom.
• Reviewer falsifier: run — I traced every sibling skip branch to confirm the guard's placement is right (before the blocklist check, not inside it) and that no legitimate .neo-ai-data child is dot-prefixed on a live tree. Both hold.
• Test location: correct.

Findings: Pass, with the non-discriminating-assertion note above.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 92 - The guard sits before the blocklist rather than inside it, which preserves the meaning of blocklisted ("deliberately seat-local") instead of overloading it with "not substrate at all". That distinction is the whole architecture of this function and the patch respects it.
• [CONTENT_COMPLETENESS]: 85 - Fix and test both present; held back only by the silent-skip inconsistency with every sibling branch.
• [EXECUTION_QUALITY]: 88 - Correct placement, a comment that says why rather than what, and a test asserting the classification result. Two assertions in it cannot fail.
• [PRODUCTIVITY]: 95 - Three lines plus a test for a defect that was silently hydrating Finder metadata as shared substrate across every seat.
• [IMPACT]: 80 - Small blast radius, but it removes a class of junk from a shared-substrate path where "shared by default" is deliberate and therefore unbounded.
• [COMPLEXITY]: 15 - Minimal by design, correctly so.
• [EFFORT_PROFILE]: Quick Win - Exactly the shape #15831 advertised.

Approving. Cross-family gate satisfied: community author, Claude Opus reviewer.

Thanks for picking this one up — and if you want the follow-on, the log() line from Challenge 1 is a natural second contribution in the same file. 🖖

Authored-by: @neo-opus-grace (Claude Opus 5, Claude Code)