feat(config): add watcher_enabled config key to disable the background watcher (#335)

[nguiaSoren]

Summary

Adds a persistent watcher_enabled config key (default true) that fully
disables the background watcher when set to false. Closes #335.

Per the maintainer's contract on #335: watcher_enabled defaults to true,
false prevents the watcher thread from starting and from registering
projects, and manual index_repository continues to work normally. Scope is
limited to this one switch — no other watcher policy changes.

Why (and how it differs from auto_watch)

auto_watch (default true) already gates per-session registration — but
the watcher thread still runs idle even with auto_watch=false. #335 asks
for a way to stop the watcher entirely (the doc-heavy-repo use case: every save
churns the graph baseline). watcher_enabled is that master switch: when
false, main() never creates the watcher, so the poll thread never starts and
nothing is registered. auto_watch is left unchanged as the finer-grained
per-session registration control. Both are now documented together.

(Note: the watcher polls for git changes now, not raw mtime as the original
0.6.0-era issue described; the user-visible concern — auto-reindex churn — is the
same, and this switch addresses it.)

The change

• src/cli/cli.h / src/cli/cli.c — CBM_CONFIG_WATCHER_ENABLED
  ("watcher_enabled") plus a small NULL-safe predicate
  cbm_config_watcher_enabled(cfg) (returns the persisted bool, default true).
  Listed in config usage help and config list output, alongside the sibling
  keys.
• src/main.c — gate watcher creation + set_watcher on the predicate; the
  existing thread-create is already gated on g_watcher, so a disabled watcher
  means no thread and (via the existing NULL guard in
  register_watcher_if_enabled) no registration. cbm_mcp_server_set_config
  stays outside the gate so auto_index / auto_watch still read config when
  the watcher is off. Logs watcher.disabled reason=config when skipped.
• tests/test_cli.c — cli_config_watcher_enabled_default_and_persist
  drives the predicate main() gates on: default on, false/0/off disable,
  true/1/on re-enable, persistence across reopen, and a NULL-config default
  (a config-store failure must never silently disable the watcher). The existing
  dump_verify-style tests feed synthetic values; this exercises the real seam.
• docs/CONFIGURATION.md / README.md — document watcher_enabled, and add
  the previously-missing auto_watch row so the watcher knobs are described
  together.

Verification

• Clean scripts/test.sh (ASan + UBSan) green: 6329 passed, 1 skipped,
  including the new test.
• Behavioral proof on the built binary (isolated CBM_CACHE_DIR):
  config set watcher_enabled false → server logs watcher.disabled reason=config and no watcher thread starts; default/true → watcher starts
  normally (watcher.disabled count 0); manual index_repository unaffected.
• No Makefile.cbm change (all touched files already compiled); no config set
  allowlist to update; no CHANGELOG in the repo.

All commits are DCO signed-off.

[DeusData]

Thanks for implementing the approved #335 contract and keeping the change focused. The exact head is security-clean, DCO-compliant, and CI is green. Before final review, two verification gaps need tightening:

1. The current test proves config parsing and persistence, but it would still pass if the main() gate were removed. Please add a process-level regression using an isolated cache/config that proves watcher.disabled reason=config is emitted, watcher.start is absent, no project registration occurs, and manual index_repository remains available.
2. Document that watcher_enabled is read at process startup, so changing it requires restarting or reconnecting the MCP server. Also clarify the auto_watch wording: it gates session registration, while the new key controls whether the watcher subsystem starts at all.

The separate key itself was explicitly approved in #335, so no redesign to overload auto_watch is requested. Review will resume against the updated exact head.

[nguiaSoren]

Thanks for the careful review, @DeusData — both gaps are addressed in the updated head, and I kept it to the switch as you noted.

1. Process-level regression (tests/test_watcher_disabled.sh, wired into scripts/test.sh as Step 5c).
It drives the real MCP server binary with an isolated CBM_CACHE_DIR and a tiny git fixture (CBM_INDEX_SUPERVISOR=0 so indexing/registration land in-process deterministically), across three runs:

• disabled + manual index_repository — asserts watcher.disabled reason=config is emitted, watcher.start is absent, watcher.watch (registration) is absent, and the manual index_repository call is served and runs the pipeline (remains available with the watcher off);
• disabled + auto_index=true — indexing still runs, but no watcher.start and no registration;
• enabled positive control — watcher.start and watcher.watch both appear and watcher.disabled does not.

The positive control is what makes it bite: I verified it by neutering the main() gate (if (1 || cbm_config_watcher_enabled(...))), rebuilding, and confirming the test fails (watcher.start present, watcher.disabled not emitted) — then restored the gate and it passes. So it guards the wiring, not just the predicate, and would catch a removed gate.

2. Docs. docs/CONFIGURATION.md (and a companion note in README.md) now state that watcher_enabled — like auto_watch — is read once at process startup, so changing it takes effect on the next server start (restart or reconnect the MCP client), not on a running server. They also spell out the distinction: auto_watch gates whether a session registers its own project with an already-running watcher, while watcher_enabled controls whether the watcher subsystem starts at all.

No redesign — the separate key stays as approved in #335. Full scripts/test.sh is green including the new Step 5c. Ready for another look against the updated head.

[DeusData]

Reviewed in full. The config layer is clean and the contract is exactly right — what needs work is that the gate itself now targets code that no longer exists.

You implemented the agreed contract precisely. I checked it against issue #335, where the contract was written out verbatim: default true, false prevents the watcher thread from starting or registering projects, manual index_repository keeps working, plus config parsing/persistence coverage, a test proving no watcher initialisation when disabled, and user-facing docs. That is what this PR does, and nothing more. Worth saying explicitly: the new-config-key surface was opened by the maintainer, so this is not a direction question — it is agreed work.

Default preservation is exact, which is the thing that would matter most if it were wrong. cbm_config_get_bool(cfg, key, true) returns true when the key is absent and when the config store fails to open — so a config failure can never silently disable the watcher. Your test pins precisely that case. The accepted spellings (true/1/on, false/0/off) match cbm_config_get_bool on current main, and auto_watch is untouched. Behaviour with the key absent is bit-for-bit what it is today.

Coherence when disabled is clean too: the watcher object is simply never created, the thread-create is already gated on g_watcher, registration early-returns on a NULL watcher, and you deliberately kept cbm_mcp_server_set_config outside the gate so auto_index and auto_watch still read config. Read once at startup, so no live-reload race — and the docs say so.

The credit I most want to give: you noticed yourself that the unit test would still pass if the main() gate were deleted, and added a process-level test to close that hole. Catching your own vacuous-test gap is exactly the discipline this project asks for, and it is rarer than it should be.

What needs re-porting. The daemon rework moved watcher creation out of main.c entirely — on current main, main.c has zero watcher references and cbm_watcher_new is called only from src/daemon/host.c:578. So the gate has no home where you put it.

That makes this a re-port with genuine semantic decisions rather than a conflict resolution, and they are worth thinking about before you write code:

• Where does the check belong now — at watcher construction in the daemon host, or earlier at daemon startup?
• What should a daemon already running do if the key changes? Today the daemon is a long-lived process, so "read once at startup" means something different from when this was a per-invocation main().
• Does a disabled watcher change anything about daemon lifecycle — should the daemon still start, and if so, what does it own?

The config layer, the tests, and the docs should all survive largely intact; it is the ~one gate that moves.

Two smaller notes:

• scripts/test.sh gaining Step 5c is fine and clearly declared, but note that every step-zero contract runs on every CI leg, so a flaky process-level test there is expensive. Yours launches a real binary against a temp git fixture — worth making sure it waits on an asserted state rather than a sleep, since a timeout deciding a test is something we treat as a defect here.
• Your branch is 367 commits behind, so please rebase before re-porting rather than after.

Nothing here is a design objection — the contract is agreed and your implementation of it is faithful. It is purely that the target moved.

[nguiaSoren]

Thanks — and you were right on every point. Rebased first as you asked (394 behind by the time I started), then re-ported. src/main.c is now byte-identical to main; the gate lives in the daemon.

Where it went. host_state_prepare() in src/daemon/host.c, at watcher construction rather than earlier at daemon startup. runtime_config is opened three lines above cbm_watcher_new there, so the config handle is already in hand, and it keeps the property the original design leaned on: a NULL watcher makes every downstream path skip itself. That turned out to need almost nothing new — host_background_stop already NULL-checks, host_background_join gates on watcher_started, cbm_watcher_free is NULL-safe, register_watcher_if_enabled still early-returns on a NULL watcher, cbm_daemon_application_new copies the field plainly so NULL is fine, and the one http_server consumer is guarded. Two things did need changing: the !host->watcher clause in the startup check (a deliberately-unbuilt watcher must not fail daemon startup, while an allocation failure with the watcher enabled still must), and host_background_start, which creates the thread unconditionally — unlike the old main.c, it had no if (watcher) guard to inherit.

Key changes while a daemon is running. Read once, when the daemon builds its host state. I considered live re-reading for symmetry with auto_watch and decided against it: it adds a thread start/stop surface while serving, and the switch is defined in #335 as preventing the thread from starting. But your question exposed a real error in my round-2 docs — I had written that both keys are read at process startup. auto_watch is actually consulted per registration (application.c:440), and reconnecting a client no longer restarts a long-lived daemon, so "restart your MCP client" was wrong advice. The docs now state the asymmetry and give the actual procedure (daemon stop, then daemon status).

Daemon lifecycle. Unchanged — it still starts and still owns IPC, HTTP/UI, project locks and the application. Only the watcher and the watch_store that exists solely to back it go unbuilt. Worth recording explicitly: auto_index keeps working with the watcher off. I verified that rather than assumed it — the disabled run still logs index.supervisor.reap outcome=clean and writes the project DB.

The process test. Rewritten, and your note about the sleep was the more important of the two. No assertion is decided by a timeout now. The daemon logs to $CBM_CACHE_DIR/logs/cbm-daemon.log in a fixed order — watcher.disabled from host_state_prepare, then daemon.start, then the thread's own watcher.start, then the join, then daemon.stop — so each run retires its daemon and waits for that closed lifecycle before asserting. Absence over a closed lifecycle log is a fact rather than a race. Every wait is a bounded poll on an asserted state following wait_for_daemon_stop in scripts/soak-test.sh, and an exhausted budget fails loudly with the reason instead of passing quietly. Session stdin is held open through a FIFO until the last JSON-RPC response actually lands, not for an interval.

One gap that needed care: registration is asynchronous and unreachable until the project DB exists (application_refresh_watch_locked requires application_regular_db_exists), so a closed lifecycle alone would not have carried the negative — the session could just have ended first. The disabled+auto_index run now waits for the DB, i.e. registration's own precondition, before closing; and the enabled control waits for watcher.watch itself, so its absence in the disabled runs is evidence rather than noise.

It also honours CBM_TEST_BINARY like test_worker_watchdog.sh — the old hardcoded build/c path would have broken the BUILD_DIR= sanitizer and container legs.

Re-verified the gate-removal check at the new site, since that was the point of the test existing: neutering the host.c gate and rebuilding fails it immediately — watcher.disabled absent, watcher.start and watcher.watch both present. Restored byte-identical afterwards.

The config layer, the unit test and the docs survived the rebase intact; only comments moved from main() to the daemon host, and I corrected the unit-test comment, which still claimed it proved non-initialisation. It pins the config layer; the process test proves the gate.