p1-1: address review (walker module doc, TODO markers, .kbignore ADR)

- walker.rs: document why we pick walkdir over ignore::WalkBuilder
  (explicit canonical-path comparison for sibling-subtree symlinks).
- walker.rs: log canonicalize failures via tracing::debug! (was a silent
  `Err(_) => continue`) so broken/permission-denied symlink targets are
  observable at debug verbosity.
- connector.rs: TODO marker on the scope.include debug-log noting the
  filter belongs at the extractor router (P1-2/P1-3).
- connector.rs: TODO marker on expand_tilde to hoist tilde + ${VAR}
  expansion into a kb-config helper once available.
- connector.rs: comment on the .kbignore read documenting the
  re-read-on-every-scan() contract.
- connector.rs test: tighten the `.kbignore`-itself ADR comment and
  upgrade the assertion to actively pin "`.kbignore` IS emitted" instead
  of "either is fine"; future drift will now fail the test.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

crates/kb-source-fs/src/connector.rs

@@ -80,10 +80,17 @@ impl SourceConnector for FsSourceConnector {
         // can layer a per-call narrowing.
         let mut excludes = self.default_exclude.clone();
         excludes.extend(scope.exclude.iter().cloned());
+        // .kbignore is re-read on every scan() so users can edit it without
+        // restarting any long-running process.
         let kbignore = read_kbignore(&root)?;
 
         let overrides = build_overrides(&root, &excludes, &kbignore)?;
 
+        // TODO(P1-2/P1-3 router): apply SourceScope::include glob filter at the
+        // extractor router layer once that crate lands. SourceConnector emits all
+        // non-excluded files; routing by include-glob is a downstream concern
+        // (design §6.2 + §7.2 are silent on this split, treat it as router work).
+        //
         // `scope.include` is intentionally ignored at this stage of the
         // pipeline: per §6.2 the workspace-level include lives in
         // `WorkspaceCfg` and is enforced by the asset writer / extractors.
@@ -159,6 +166,9 @@ impl SourceConnector for FsSourceConnector {
     }
 }
 
+// TODO(kb-config): hoist tilde + ${VAR} expansion into a kb-config helper
+// once that crate gains a path-expansion API. Today this duplicates logic
+// that P1-6 (store-sqlite) and future crates will also need.
 /// Expand a leading `~` to the current user's home directory. No-op for
 /// any other shape (absolute, relative, `${VAR}`-style).
 fn expand_tilde(s: &str) -> PathBuf {
@@ -242,13 +252,15 @@ mod tests {
                 .unwrap();
         let v = conn.scan(&SourceScope::default()).unwrap();
         let names: Vec<_> = v.iter().map(|a| a.workspace_path.0.clone()).collect();
-        // .kbignore itself starts with `.` and is not in DEFAULT_EXCLUDES,
-        // so it is *not* automatically hidden — but the task spec only
-        // requires `*.tmp` and `.DS_Store` / `._*` filtering, and the
-        // `.kbignore` file is a legitimate "Other(\"\")" asset. Either
-        // present-or-absent is acceptable; the assertion below pins
-        // current behaviour: .kbignore appears, b.tmp does not.
-        assert!(names.contains(&".kbignore".to_string()));
+        // Decision: `.kbignore` itself IS emitted as a RawAsset (MediaType::Other("")).
+        // Rationale: a config file that affects ingest is itself part of the
+        // workspace contents; the markdown extractor (P1-2) will reject Other("")
+        // on its own. If we ever decide to omit `.kbignore` from the asset list,
+        // this test will catch it.
+        assert!(
+            names.contains(&".kbignore".to_string()),
+            ".kbignore must be emitted as an asset; got: {names:?}"
+        );
         assert!(names.contains(&"a.md".to_string()));
         assert!(!names.contains(&"b.tmp".to_string()));
     }

crates/kb-source-fs/src/walker.rs

@@ -17,6 +17,15 @@
 //! `a -> b -> a`. `walkdir` does NOT detect cycles for us when
 //! `follow_links(true)`; we layer our own visited-set on top, keyed by the
 //! canonical path of every entry, and skip any entry we've already seen.
+//!
+//! ## Why `walkdir` instead of `ignore::WalkBuilder`?
+//!
+//! `ignore::WalkBuilder` bundles gitignore semantics + cycle detection in
+//! one API. We use `walkdir` directly because we need explicit control
+//! over canonical-path comparison for sibling-subtree symlinks (a case
+//! `walkdir`'s ancestor-only check can miss). Override-based filtering
+//! still uses the `ignore` crate's `Override` matcher, just decoupled from
+//! its walker.
 
 use std::collections::HashSet;
 use std::path::{Path, PathBuf};
@@ -136,8 +145,12 @@ pub(crate) fn walk_files(root: &Path, overrides: &Override) -> Result<Vec<PathBu
                         continue;
                     }
                 }
-                Err(_) => {
-                    // Broken symlink etc. — skip silently.
+                Err(err) => {
+                    tracing::debug!(
+                        path = %path.display(),
+                        error = %err,
+                        "skipping: canonicalize failed (broken/permission-denied symlink target)"
+                    );
                     continue;
                 }
             }