From 967a6a62c5521e102d4edd1db7a16f607e791b14 Mon Sep 17 00:00:00 2001 From: altair823 Date: Thu, 30 Apr 2026 12:42:25 +0000 Subject: [PATCH] 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) --- crates/kb-source-fs/src/connector.rs | 26 +++++++++++++++++++------- crates/kb-source-fs/src/walker.rs | 17 +++++++++++++++-- 2 files changed, 34 insertions(+), 9 deletions(-) diff --git a/crates/kb-source-fs/src/connector.rs b/crates/kb-source-fs/src/connector.rs index fe98c1e..ed94bc9 100644 --- a/crates/kb-source-fs/src/connector.rs +++ b/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())); } diff --git a/crates/kb-source-fs/src/walker.rs b/crates/kb-source-fs/src/walker.rs index 95b6b3d..6f399cc 100644 --- a/crates/kb-source-fs/src/walker.rs +++ b/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 { - // Broken symlink etc. — skip silently. + Err(err) => { + tracing::debug!( + path = %path.display(), + error = %err, + "skipping: canonicalize failed (broken/permission-denied symlink target)" + ); continue; } }