feat(index): add opt-in discovery resource limits

[liuchong]

Related to #1347.

Problem

Indexing accepts whatever a repository contains. A tree carrying a vendored monorepo, a generated dump, or a runaway build directory is discovered in full, and the first sign of trouble is a host under memory pressure with nothing that attributes it to indexing. There is currently no way for an operator to say "not larger than this".

What this changes

Two opt-in limits, evaluated during discovery against accepted source files only, so ignored and excluded paths never count against an operator's budget:

Key	Meaning
index_max_files	Maximum number of accepted source files
index_max_source_mb	Maximum total size of accepted source files

Both default to off; without configuration this PR changes no behaviour.

Crossing a limit fails the whole attempt with a structured resource_limit_exceeded result naming the resource, the observed value and the limit. No partial graph is published, and an existing serving index keeps answering. A partial graph would be worse than no graph: it looks complete and is silently wrong.

Limits are read from the CLI-managed _config.db, not from MCP request arguments. A supervised parent replaces any caller-supplied internal policy before spawning its worker, and the worker rejects a missing or incomplete contract, so the CLI, the daemon and the supervised worker all enforce the same decision. A test pins the forged-override path.

Testing

make -f Makefile.cbm test and make -f Makefile.cbm lint-ci on macOS. New coverage: discovery counting and rejection, policy parsing and validation boundaries, worker-contract propagation and forged-override rejection, end-to-end preservation of the previously published index.

One pre-existing failure (tests/test_cli.c:6484, agent-client registry) is present on the unmodified base commit as well and is unrelated to this change.

Stack

This is the first of six PRs that replace #1348. Each is independently reviewable and mergeable in order; later ones build on earlier ones.

1. this PR — discovery limits
2. feat(index): add worker resource watchdogs #1724 — worker watchdogs
3. feat(index): enforce opt-in storage resource limits #1725 — storage limits
4. feat(index): add opt-in index resource profiles #1726 — resource profiles
5. feat(index): record and report the latest index attempt #1727 — attempt record and freshness
6. feat(mcp): warn on answers served from a failed rebuild #1728 — stale-index warning on answers

[github-actions]

Thanks for opening this — it has been seen, and it is queued.

This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence.

Current review status: working through a backlog. 0.9.1-rc.1 is out, so the release freeze that held reviews is over — but it left a large queue of open pull requests behind it, and we are reading through them oldest-first. The background is in discussion #1144.

What that means for this PR, concretely:

• It will not be closed for inactivity. No stale bot touches pull requests here.
• It may still sit a while before a human reads it. That is on us, not on you.
• Older PRs are read first, so a recent one is not being skipped — it is behind a queue.

Things that will genuinely speed it up whenever review does happen:

• Keep it rebased on main — the tree is moving quickly right now, and a conflicting branch cannot be reviewed as the diff you intended.
• Get CI green, or say which failures you believe are pre-existing.
• Keep the change to one claim. Bundled features and refactors get split before they get merged, which costs you a round trip.
• Every commit needs a sign-off (git commit -s) — CI enforces DCO.

If this fixes a bug, a reproduction we can run is worth more than a description of the symptom.

Thanks for contributing, and sorry in advance for the wait.

[DeusData]

This fixes exactly what I blocked #1348 on, and I verified it rather than taking the claim. One small defect and I will merge.

Default-off is now true by construction, not by convention. I traced all four gates:

1. cbm_index_policy_init zeroes the struct, and cbm_index_limit_u64_t carries its own enabled flag — so zero means disabled. Critically, there is no ..._defaults() function anywhere in the diff; the shape that caused the problem is gone rather than corrected.
2. cbm_index_policy_default_value() returns "off" for every policy key.
3. cbm_config_load_index_policy passes that as cbm_config_get's default, and cbm_config_get returns the default when the row is absent — so an unconfigured install loads "off".
4. pipeline.c passes a policy pointer only when cbm_index_policy_enabled(), and fl_add guards the whole accounting block on a non-NULL policy plus each limit's own enabled.

discover_resource_policy_off_matches_legacy_discovery is the test I most wanted to see: it runs discovery twice, once with no policy and once with an initialised-but-off policy, and asserts identical counts, identical per-file rel_path and size, and violation.resource == CBM_INDEX_RESOURCE_NONE. That is the "nothing changes for people who did not ask" property stated as an assertion rather than a promise.

Two config keys instead of eighteen. index_max_files and index_max_source_mb, both defaulting to "off", both reachable through the existing config set/get/list/reset with no new subcommand — and reset works because "off" is declared in CONFIG_KEYS rather than implied. The CBM_INDEX_MAX_*_VALUE constants are ceilings on what an operator may configure, not defaults, which I checked because that is an easy thing to misread.

Three things you hardened beyond what I asked, which are worth naming:

• The caller-policy strip is now a while loop rather than a single remove_key, so a request carrying duplicate _cbm_index_policy keys cannot smuggle one past. That is a real attack shape against JSON objects and it was not in the original.
• The worker refuses anything short of the exact full key set, not merely a present object. A future key added to the policy cannot silently arrive unset in a worker.
• Discovery violations map to a distinct CBM_PIPELINE_RESOURCE_LIMIT at all four sites rather than collapsing into CBM_NOT_FOUND, so "we refused" is now distinguishable from "we failed".

And index_policy_mcp_rejects_forged_override_and_preserves_serving_index is the strongest test in the PR: it forges both keys to "off", proves server config still wins with observed: 2, limit: 1, and then proves the pre-existing database was untouched by comparing indexed_at byte-for-byte. Abort-not-truncate is pinned too, including the late-breach case via the before_final_manifest hook.

Worth noting: because every new test calls a symbol that does not exist on main, the suite would not even compile against the base. That is a compile-level guarantee against vacuous tests, which is stronger than the usual argument.

---

The one fix before merge — a silent failure.

config_index_policy_write prints only when validation fails:

if (!cbm_index_policy_set(&candidate, key, value, error, sizeof(error))) {
    (void)fprintf(stderr, "error: %s\n", error);
    return CLI_ERR;
}
return cbm_config_set(config, key, value);

but the caller now suppresses the generic message for policy keys:

if (!config_key_is_index_policy(argv[CLI_SKIP_ONE])) {
    (void)fprintf(stderr, "error: failed to set %s\n", argv[CLI_SKIP_ONE]);
}

So if validation passes and the cbm_config_set write then fails — a locked or corrupt _config.db — config set index_max_files 100 exits non-zero having printed nothing at all. The suppression assumes the helper always spoke, and it does not on that path. Either print in the write-failure branch of the helper, or narrow the suppression to the validation case.

Two things to confirm rather than change:

1. index_run_supervised_path returns NULL when policy load fails, so on a corrupt _config.db the auto-index attempt is dropped after logging index.policy. Fail-closed is the right default and consistent with the rest of the design — I just want it to be a decision for the unattended auto-index path specifically, where nobody is watching the log.
2. fl->limit_exceeded now carries two meanings — the pre-existing count_only file-cap early stop and a policy breach. No current caller passes a policy in count-only mode, so this is latent, but it is the kind of latent that bites the next person.

And one completeness check I would like your eye on, because it is the failure mode your own commit message warns about: the worker argv contract is now mandatory and fail-closed, so any harness that spawns --index-worker and hand-builds an index_repository payload must supply the full key set. You updated the two that do. I enumerated the other --index-worker sites — index_supervisor.c, daemon/bootstrap.c, and several test files — and they look like argv-plumbing that never constructs that payload, so I believe the change is complete. Please confirm, since a miss there produces exactly the quietly-broken guard you were guarding against.

Fix the CLI message and I will merge this. Thank you for taking the reshaping seriously instead of arguing for the original — this is a much better change than #1348 was, and the trust boundary is stronger than what I asked for.

[liuchong]

Fixed, and the three checks confirmed.

The silent write failure is fixed. config_index_policy_write now speaks in the write-failure branch, so the caller's suppression stays correct on every path the helper can return from:

int rc = cbm_config_set(config, key, value);
if (rc != 0) {
    (void)fprintf(stderr, "error: failed to set %s\n", key);
}
return rc;

index_policy_cli_set_reports_a_failed_write pins it. Making the write fail without corrupting anything needed a fixture rather than a mock: it creates _config.db with config as a view, so validation passes and the INSERT is what fails. The test asserts a non-zero exit with the key named on stderr. Amended into the original commit rather than stacked on top, so the slice stays one reviewable diff.

1. Fail-closed on the unattended path — deliberate, and specifically for that path. With no policy loadable there is no bound to enforce, and unattended is exactly where an unbounded index is least survivable. The cost is the one you named: nobody reads that log line. #1727 and #1728 are what close that gap — which is also why I understand judging them separately.

2. fl->limit_exceeded — confirmed, and mutually exclusive by accident rather than by construction. fl->max_files is -1 unless count_only, and the policy breach only fires when fl->resource_policy is non-NULL, so today the two producers cannot both run. Nothing enforces that, which is your point exactly. If you want it structural, the narrowest change is a second flag for the count-only stop:

    if (fl->max_files >= 0 && fl->count >= fl->max_files) {
        fl->capped = true;
        return;
    }

with file_list_should_stop returning fl->failed || fl->limit_exceeded || fl->capped. Behaviour is identical; the two meanings stop sharing a bit. Say the word and I will add it here or in a follow-up — I left it out because it changes a struct the later slices also touch, and you asked me to confirm this one rather than change it.

3. Worker argv contract — confirmed complete. Your enumeration matches mine. Every production spawn goes through the strip-and-inject in index_args_with_repo_path, so the payload is built in one place and cannot arrive short. index_supervisor.c and daemon/bootstrap.c plumb argv without ever constructing an index_repository payload, as you read them. The only hand-built payloads are the two shell harnesses, and both now build the full key set through a shared cbm_test_index_worker_policy_json helper in scripts/test-runtime.sh rather than each spelling it out — so a future key is a one-line change rather than a silent miss.

On the branch. Rebased onto current main and green. I also opened #1796 for the daemon start defect behind the Windows guard failures that were showing on this stack.

[DeusData]

Maintainer decision: the concept is accepted. Opt-in indexing resource limits are something this project wants, and #1723–#1728 can proceed as a programme rather than needing to argue the direction again at each slice.

Two things decided that, beyond the code:

The problem is one we have actually hit, not a hypothetical. Indexing here has driven a host into OOM before, and "the first sign of trouble is a host under memory pressure with nothing that attributes it to indexing" is an accurate description of what that looks like from the outside.

And the default-off, opt-in shape means the surface costs nothing until someone asks for it. A feature that changes no behaviour unconfigured is a very different proposition from one that imposes a policy.

The trust boundary is the part that earned the most confidence. Reading limits from the CLI-managed _config.db rather than from MCP request arguments, having the supervised parent overwrite any caller-supplied policy before spawning its worker, having the worker reject an incomplete contract, and then pinning the forged-override path with a test — that is the design of someone who thought about who gets to set a limit, not just how to enforce one. Failing the whole attempt rather than publishing a partial graph is right for the same reason you gave: a partial graph looks complete and is silently wrong.

What happens next

Each slice still gets reviewed on its own merits. Accepting the programme is not accepting six diffs in advance — and I would expect slices 3 through 6 (storage limits, resource profiles, attempt records, stale-index warnings) to get closer attention than this one, because that is where the configuration surface actually grows. Slice 1 is the cheapest and most self-contained of the six.

Please rebase this onto current main and let CI re-run. Its checks are from 22 August and are no longer informative: main was briefly broken today by a duplicate-symbol merge and repaired by #1993, and #1703 landed after that. Nothing on this branch is implicated.

Do not chase the test-lsan-macos failure. It is tests/test_watcher.c:1795 — index_call_count == 0, expected 1 — which is a watcher-poll timing test already on our known-flaky list. Your change is not the cause.

One process note so it does not surprise you later: this PR touches Makefile.cbm and scripts/test-runtime.sh, which our tooling treats as an enforcement surface, so it needs a separate maintainer clearance before merge. That is procedural rather than a concern about the change — I will handle it when the rebased diff comes up for merit review.

Thank you for splitting #1348 into six independently reviewable pieces and for saying plainly which ones depend on which. That is what makes accepting the direction possible without accepting everything sight-unseen.

[liuchong]

Rebased onto current main (b769ad63). CI is green again — ci-ok included.

No code changes beyond the rebase merge: the watcher keys that landed on main in the meantime (watcher_enabled / auto_watch) are kept alongside the two index_max_* keys. Happy to wait on the clearance you mentioned for Makefile.cbm and scripts/test-runtime.sh.

[DeusData]

Reviewed on merit. The concept was accepted previously; this is the review of the slice itself, and the safety properties are the part I checked hardest.

Opt-in and default-off, so the no-config path is unchanged. That is what makes this safe to take at all — a limit that defaults to a value is a behaviour change for every existing user; one that defaults to off is a capability.

Counting only accepted source files is the right denominator. Charging an operator's budget for paths their ignore rules already excluded would make the limit mean something different from what the key says, and would make it unpredictable as ignore rules change.

Failing the whole attempt rather than publishing a partial graph is the decision I would have argued for, and your justification is exactly right:

A partial graph would be worse than no graph: it looks complete and is silently wrong.

That is the same principle behind several other fixes landing this week — a wrong answer that looks authoritative costs more than a refusal. Keeping an existing serving index answering while the new attempt fails is the other half of getting this right.

The policy provenance is the part I looked at most closely, because "operator sets a limit" and "caller sets a limit" are very different things. Reading limits from the CLI-managed _config.db rather than from MCP request arguments, having the supervised parent replace any caller-supplied internal policy before spawning the worker, and having the worker reject a missing or incomplete contract, means the CLI, the daemon and the worker all enforce one decision that a request cannot forge. index_policy_mcp_rejects_forged_override_and_preserves_serving_index pins that, which is the test I would have asked for if it were absent.

And the worker fails closed. The comment in scripts/test-runtime.sh states it plainly — a worker that finds no complete policy refuses to start rather than index unbounded. For a resource guard, failing closed is the only defensible direction: a guard that degrades to "no limit" when its contract is missing protects nobody on exactly the runs where something has already gone wrong.

Mechanics

Your two clearance findings are Makefile.cbm and scripts/test-runtime.sh. Both are benign and necessary: one line adding src/foundation/index_policy.c to the build, and a test helper that hands the worker the same policy object a real supervisor would. Neither weakens a gate — the second arguably strengthens the test harness's fidelity to production.

You are 31 commits behind main, which has taken a dozen merges today, so the current green no longer describes what would land. I am rebasing the branch to get a trustworthy run rather than merging on a stale result.

Merging on that green. Thank you — this is a carefully built change, and stating the operator-versus-caller distinction in the description saved me a great deal of tracing.

[liuchong]

Quick process question before I touch anything — mainly so I do not disrupt your merge cadence or burn CI for no reason. I can bring the branches up to current main at any time; I just do not want to restart matrices or reshape the stack against a plan you already have.

This branch is ~158 commits behind main, but still reports CLEAN / mergeable with a green CI matrix from the earlier refresh. Do you want me to rebase #1723 onto current main before you merge, or leave it as-is and let you take it from here?

Separately, for #1724–#1728: should I rebase those onto current main now, or wait and rebase each one only after the previous slice merges (as in your earlier review notes)?

[liuchong]

I noticed #1723 had gone conflicting again, so I rebased it onto current main to keep it from blocking merge. While doing that I also rebased #1724–#1728 onto the refreshed stack so the later slices stay aligned rather than drifting further behind.

No intentional feature changes in this refresh — just conflict resolution against what landed on main in the meantime. In particular, for #1726 I dropped the old “normalize stored session roots” approach in favor of what #2070 already landed: keep the policy spelling native and fold separators only when comparing index args. That avoids fighting #2070’s design.

CI is restarting on the new heads.

[liuchong]

@DeusData rebased #1723 onto current main (1db8bace) again so it stays mergeable.

One real conflict worth naming: main now uses CBM_PIPELINE_ABORT_OVER_BUDGET = -5 for the memory-budget abort path, which collided with this slice’s old CBM_PIPELINE_RESOURCE_LIMIT = -5. I kept the new abort / preserve / persist response branches from main, and moved the discovery resource-limit code to -6 so the two causes stay distinguishable. No intentional feature change beyond that rebase merge.

#1724–#1728 are restacked on the refreshed tip as well. CI is restarting.

Quick process ask: what is the merge plan for this slice now? You had already reviewed it on merit and previously said you would merge on green, but main keeps moving and this branch has had to rebase several times. If it still needs to wait, that is fine — I just want to know whether to keep refreshing it, or leave the next rebase to your merge cadence so we do not burn CI / reshape the stack against a plan you already have.

[DeusData]

You asked a direct process question on 2026-09-06 and got eleven days of silence while continuing to rebase this branch four times to keep it mergeable. That is the wrong way round, and I am sorry.

Answering the question first, since it is the one that costs you time: please do not rebase proactively from here. I will tell you when a rebase is actually needed. You have been absorbing the cost of every move on main — 09-02, 09-07, 09-09 — and restarting matrices for a queue position that was not yours to manage. If this goes conflicting again while it waits, that is ours to deal with, not a reason for you to burn another CI matrix.

Status: approved and queued, and the blocker is on our side. The slice was reviewed on merit on 2026-09-02, the concept was accepted for #1723–#1728 as a programme, and it currently reads MERGEABLE / CLEAN with 34 checks green.

What is holding it is our build-clearance gate, not your code. This PR edits Makefile.cbm and scripts/test-runtime.sh — files that execute during build and test — so it needs a maintainer's explicit clearance decision before an external head is built here. That decision is pending with the maintainer now. Nothing for you to change.

On the constant collision you named — you made the right call, and I verified it rather than taking your word for it. main defines:

#define CBM_PIPELINE_ABORT_OVER_BUDGET (-5)

and your branch now uses CBM_PIPELINE_RESOURCE_LIMIT (-6), leaving -5 alone. Two distinct failure conditions sharing one return code is the kind of defect that merges clean and then produces a wrong diagnosis months later — a discovery-limit stop reported as a memory-budget abort, or the reverse. Catching it during a rebase and saying so in the thread is exactly what I would want.

On stack order: #1723 is the base and lands first; #1724–#1728 follow in sequence once it does. Please leave the upper slices alone until then — rebasing them onto a base that has not landed is work that gets thrown away.

Thank you for the patience and for the care taken on every one of those rebases.

[DeusData]

Merged as 92abefa. Before merging I built and tested the actual merge result against current main rather than trusting the green from your last rebase — index_policy, discover, pipeline, daemon_application and mcp: 788 passed, 0 failed, plus both worker shell contracts green. The mcp suite was included deliberately, since main had just changed the coverage-freshness path in the same file; no interaction.

Thank you for the four rebases you should never have had to do, and for the -5/-6 catch. #1724 is next — please rebase it onto main now that this has landed, and stop there; I will tell you when each subsequent slice's turn comes so you are not chasing main across the whole stack at once.