Skip to content

fix(doctor): multi_source_drift completes on real brains — prune + wire env levers + raise file cap (SCA-3773)#3

Open
pai-scaffolde wants to merge 1 commit into
masterfrom
fix/multi-source-drift-walk-prune
Open

fix(doctor): multi_source_drift completes on real brains — prune + wire env levers + raise file cap (SCA-3773)#3
pai-scaffolde wants to merge 1 commit into
masterfrom
fix/multi-source-drift-walk-prune

Conversation

@pai-scaffolde

Copy link
Copy Markdown
Owner

Problem (SCA-3773)

gbrain doctor's multi_source_drift check walks each non-default source's filesystem to find pages misrouted to default. On a brain of normal size it hit its walk budget and reported "check skipped, walk too large" — drift detection became a silent blind spot that reads as healthy. The warning told operators to tune GBRAIN_DRIFT_LIMIT / GBRAIN_DRIFT_TIMEOUT_MS, but those env vars were never read — the documented fix did nothing.

Root causes (both fixed)

  1. File-count cap too low (10K). A brain ingesting transcript/learning history blows past 10K markdown files in a single source — the live brain that surfaced this had 13.5K transcript files in one source. The walk is fast (13.5K cross-checked in <300ms), so time was never the binding constraint; the count cap was. Raised 10K → 50K; one source needs >65K files to approach Postgres's bind-param ceiling. Timeout raised 5s → 15s as the real hang-guard.
  2. node_modules/vendor/dist never pruned. When a source local_path points at a working repo, the dotfile-only skip missed these (none are dot-prefixed), so the walk descended dependency trees sync never indexes. The walk now applies the canonical pruneDir gate from sync.ts, so it considers exactly the files sync ingests — a perf fix (the scaffolde-ai source walk dropped from ~7.8s → ~0.14s) and a correctness alignment.

Also

  • GBRAIN_DRIFT_LIMIT / GBRAIN_DRIFT_TIMEOUT_MS are now honored via resolveDriftLimit / resolveDriftTimeoutMs (explicit call-site opts still win for tests; invalid/non-positive values fall back with a once-per-process stderr warning, mirroring sync_freshness's _resolveSyncFreshnessHours).
  • Walk drops the per-entry lstatSync for readdirSync({ withFileTypes: true }) — symlink-following semantics unchanged (Dirent.isDirectory() does not follow symlinks).

Acceptance criteria

  • multi_source_drift completes (not skipped) in a normal gbrain health run
  • Resulting status reflects real drift state, not a timeout artifact
  • Tuning documented (CHANGELOG + inline comments) so it doesn't regress on dataset growth

Verification

  • Live end-to-end: gbrain doctor from this branch against the affected brain now reports multi_source_drift: ok — No cross-source slug drift detected. (was: warn — skipped, FS walk hit limit/timeout). Combined walk of all 3 sources completes in ~286ms.
  • Unit: test/multi-source-drift.test.ts 10/10 pass (added: node_modules/vendor/dist pruned-not-counted; GBRAIN_DRIFT_LIMIT honored when opts omit it; resolver parse + garbage-fallback).
  • Regression: test/doctor.test.ts 83/83 pass. Full tsc --noEmit adds zero new errors (5 pre-existing in unrelated files).

🤖 Generated with Claude Code

…re env levers + raise file cap (SCA-3773)

The `multi_source_drift` doctor check walks each non-default source's
filesystem to find pages misrouted to `default`. On any brain of normal
size it hit its walk budget and reported "check skipped, walk too large",
turning drift detection into a silent blind spot that read as healthy.
The warning told operators to tune GBRAIN_DRIFT_LIMIT /
GBRAIN_DRIFT_TIMEOUT_MS, but those env vars were never read — a dead-end
lever.

Root causes (both fixed):

1. File-count cap too low. A brain ingesting transcript/learning history
   blows past 10K markdown files in a single source (the brain that
   surfaced this had 13.5K transcript files in one source). The walk is
   fast — 13.5K files cross-checked in <300ms — so time was never the
   binding constraint; the count cap was. Raised 10K -> 50K. The 15s
   timeout (up from 5s) remains the real hang-guard.

2. node_modules/vendor/dist never pruned. When a source local_path points
   at a working repo, the dotfile-only skip missed these (none are
   dot-prefixed), so the walk descended dependency trees sync never
   indexes. The walk now applies the canonical pruneDir gate from sync.ts,
   so it considers exactly the files sync ingests — both a perf fix
   (scaffolde-ai source walk ~7.8s -> ~0.14s) and a correctness alignment.

Also:
- GBRAIN_DRIFT_LIMIT / GBRAIN_DRIFT_TIMEOUT_MS are now honored via
  resolveDriftLimit / resolveDriftTimeoutMs (explicit opts still win for
  tests; invalid values fall back with a once-per-process stderr warning,
  mirroring _resolveSyncFreshnessHours).
- Walk drops the per-entry lstatSync in favor of readdirSync withFileTypes
  (symlink-following semantics unchanged).

Verified end-to-end: live `gbrain doctor` now reports
`multi_source_drift: ok — No cross-source slug drift detected` (was:
`warn — skipped, FS walk hit limit/timeout`). 10/10 unit tests pass;
doctor.test.ts 83/83.

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

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c38d97f1e9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread package.json
},
"license": "MIT",
"version": "0.42.51.0"
"version": "0.42.52.0"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Keep VERSION in sync with the package version

This release bump updates package.json to 0.42.52.0 but leaves the tracked VERSION file at 0.42.51.0. The release instructions treat VERSION and the changelog as branch-scoped release metadata, so any release tooling or operator check that reads VERSION will still see the previous release while the package/changelog advertise 0.42.52.0. Please update the VERSION file in the same commit as the package bump.

Useful? React with 👍 / 👎.

// Explicit opts (tests) win; otherwise honor the documented env-var levers
// GBRAIN_DRIFT_LIMIT / GBRAIN_DRIFT_TIMEOUT_MS (pre-SCA-3773 these were named
// in the doctor warning but never actually read — the lever was a dead end).
const limit = opts.limit ?? resolveDriftLimit();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Cap the env-driven drift limit before batching

When an operator raises GBRAIN_DRIFT_LIMIT above ~65k to handle a source larger than the new 50k default, this value is used unchanged and the later batchProbeExistence call builds one SQL statement with one bind parameter per slug plus the source id. On Postgres brains that exceeds the 65,535-parameter limit, so the doctor check throws and the surrounding best-effort catch drops multi_source_drift instead of reporting drift. Please clamp this env value below the bind ceiling or chunk the DB probe.

Useful? React with 👍 / 👎.

Comment on lines +159 to +166
if (entry.isDirectory()) {
// Apply the canonical sync prune gate so the drift walk considers
// exactly the files the sync walker indexes: skip `node_modules`,
// `vendor`, `dist`, `build`, `venv`, `ops`, `.raw`, dot-dirs, and git
// submodules. This is both the perf fix (these trees dominate walk
// time on a working-repo source) and a correctness alignment — a file
// sync never ingests can't be a real drift candidate (SCA-3773).
if (!pruneDir(name, d)) continue;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Fall back when dirent types are unavailable

On filesystems where readdir(..., { withFileTypes: true }) cannot provide a directory type (for example some network/FUSE mounts), Dirent.isDirectory() is false even for real directories. The old lstatSync(full).isDirectory() path still recursed in that environment, but this change treats those directories as ordinary non-markdown entries and misses all nested pages, making multi_source_drift report clean on mounted brains with people/*.md-style layouts. Please fall back to lstatSync when the dirent type is not definitive.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant