Repository navigation
fix(mt#5060): Watch the daemon's whole src/ tree and admit .json modules in the tray watcher - #3717
Conversation
The backend watcher's src/ root was `src/cockpit`; the daemon is `bun run src/cli.ts`, whose static closure has 465 files under src/ of which only 143 are under src/cockpit. The module-class filter was also `.ts`-only, so the two generated .json imports were dropped even where watched. Measured 2026-09-10: 4 of 4 pulls touching closure files outside the watched roots produced no restart; 6 of 6 watched-root pulls did. The daemon served a two-generations-old interceptor catalog for ~16h. - `cockpit_backend_roots` now starts from `daemon_source_tree` (`src/`), the entry point's own tree — a superset of the closure by construction, the same stance mt#4230 took with `packages/*/src` discovery. - `is_backend_module_file` admits `.json`. - `cockpit_backend_src` stays as the presence gate only. - Tests revised for the new root; two new tests pin the src/ subtree coverage and the generated-json case; the web/ exclusion is asserted under the wider root. - The five prose descriptions of the watched set updated; skill recompiled. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TPNyYTreM4F7PRbvh4xA5b
Minsky Reviewer StatusVerdict: APPROVED — no blocking findings Commands
|
There was a problem hiding this comment.
Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2
Solid, well-scoped change that widens backend watcher coverage to the daemon’s actual closure and admits generated JSON modules; tests and prose were updated coherently. I found no blocking issues. Non-blocking nits: (1) *.test.json/*.spec.json will still trigger restarts — consider excluding JSON test files like TS test suffixes; (2) Rustdoc comments use JSDoc {@link ...} syntax — use Rust intra-doc links instead; (3) with src/ as a root, the freshness walk cost grows — consider small optimizations/caching if it becomes noticeable; (4) the web exclusion applies anywhere in a root-relative path — confirm that broader policy remains intended. With these addressed or tracked, this PR looks ready to merge.
Findings
- [NON-BLOCKING] cockpit-tray/src-tauri/src/watcher_backend.rs:285 —
.jsonadmitted as module but test-file variants (*.test.json/*.spec.json) are not excluded
is_backend_module_fileadds.jsontoEXTS(cockpit-tray/src-tauri/src/watcher_backend.rs:285-306), butTEST_EXTSexcludes only TS/MTS/CTS test/spec suffixes. If a*.test.jsonor*.spec.jsonappears under a watched root (not in an excluded dir), it will spuriously trigger a daemon restart. The PR asserts only two.jsonundersrc/today, but this is a future-footgun. Suggestion: add".test.json"and".spec.json"toTEST_EXTS(and consider mirroring any other test file-class suffixes you rely on). - [NON-BLOCKING] cockpit-tray/src-tauri/src/watcher_backend.rs:137 — Rustdoc link syntax uses
{@link ...}which Rust does not render as intra-doc links
Several doc comments added in this PR use{@link ...}(e.g.,watcher_backend.rs:146-175,:201-216,:247-258). That's JSDoc syntax, not Rust's. Rustdoc will render these literally rather than linking. Prefer Rust intra-doc links like[`daemon_source_tree`]and fully-qualified paths for items to keep docs navigable. - [NON-BLOCKING] cockpit-tray/src-tauri/src/watcher_backend.rs:319 — Broadened
src/root increases filesystem walk cost for staleness checks; consider bounding or caching
newest_backend_mtimenow walks the entiresrc/tree recursively (excluding components inBACKEND_WALK_EXCLUDES) andnewest_backend_mtime_acrosstakes a max across roots. With the widenedsrc/root, adoption-time checks (supervisor.rsuses this during spawn) and runtime debounce evaluations may traverse significantly more files, especially in large repos. Tests assert behavior, but there is no guard against pathological trees (e.g., large vendored directories not caught by excludes). Non-blocking: consider cheap optimizations (e.g., early-return on encountering a file newer than the tracked daemon start, or memoizing per-root newest mtime across bursts) to keep overhead predictable. - [NON-BLOCKING] cockpit-tray/src-tauri/src/watcher_backend.rs:262 —
backend_path_is_excludedmatches component equal toweb; ensure no unintended exclusions from common names
The exclusion applies when any path component equals"web"(backend_path_is_excludedviais_backend_excluded_dir). Under the widenedsrc/root, paths likecockpit/web/...are correctly excluded, but any future backend subdir named literallyweb(outside the cockpit frontend) would also be excluded. This may be intended, but it's now a broader policy decision. Non-blocking: consider scoping thewebexclusion tocockpit/webif you ever add non-frontendweb/subtrees elsewhere.
Documentation impact
- updated-in-pr — Multiple user/developer docs were updated to reflect the widened watcher coverage and
.jsonmodule class:.claude/skills/cockpit-tray-dev/SKILL.md,.minsky/skills/cockpit-tray-dev/content.md,cockpit-tray/README.md(backend watcher roots and relevance wording),cockpit-tray/CLAUDE.md(wiring comment),docs/architecture/cockpit.md(backend auto-restart description), andsrc/cockpit/CLAUDE.md(operator guidance). These changes align prose with the new behavior; no additional docs appear to need updates beyond what the PR already touches.
Affected: .claude/skills/cockpit-tray-dev/SKILL.md, .minsky/skills/cockpit-tray-dev/content.md, cockpit-tray/README.md, cockpit-tray/CLAUDE.md, docs/architecture/cockpit.md, src/cockpit/CLAUDE.md
Cockpit PreviewPreview URL: https://cockpit-preview-production.up.railway.app Deployed from commit Updated by Cockpit Preview. |
…doc links
Two non-blocking findings addressed in-class: `.test.json` / `.spec.json`
join `TEST_EXTS` alongside the `.json` module class (with assertions), and
the docblocks this PR wrote use [`item`] intra-doc links instead of JSDoc
`{@link}`; `cargo doc` resolves all seven with no warning in this file. The
other two findings (walk cost over ~1k files at spawn; the global `web`
component exclusion) are intended and documented in the docblocks.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TPNyYTreM4F7PRbvh4xA5b
There was a problem hiding this comment.
Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2
Reverification complete: the fix widens the backend watcher to the daemon’s entry tree (src/) and admits .json modules, with comprehensive unit tests and supervisor wiring using the same root set. Web remains excluded under the wider root, and the adoption-time staleness check and uptime hint reuse the same logic. Prose consumers were updated across README/CLAUDE/docs to prevent stale guidance. I find no new defects introduced by the follow-up commit; prior non-blocking notes were addressed. One criterion (live reproduction evidence) rests on out-of-repo logs and is unverifiable here, but does not block merge. Verdict: APPROVE.
Spec verification
| Criterion | Status | Evidence |
|---|---|---|
The live behavior is reproduced or falsified, and recorded either way. Reproduced at planning — ## Planning finding — 2026-09-11. |
Unverifiable | Reproduction evidence relies on out-of-repo daemon logs under ~/.local/state/minsky/logs/cockpit-daemon*.log and local reflog correlation (PR body and spec). Those artifacts are not in-repo; reviewer cannot verify from the diff alone per Out-of-repo references policy. |
A merged change confined to any file in the daemon's static import closure under src/** — src/generated/interceptor-catalog.json, src/adapters/**, src/domain/**, src/mcp/**, … — causes the tray-supervised daemon to restart within RESTART_DEBOUNCE, exactly as a src/cockpit or packages/*/src change does today. |
Met | cockpit-tray/src-tauri/src/watcher_backend.rs: cockpit_backend_roots() now includes daemon_source_tree(repo_root) → src/ (lines ~152–181) plus discovered packages/*/src; is_relevant_backend_change() filters root-relative paths with web exclusion. Tests backend_roots_cover_a_src_subdirectory_nobody_named_in_code and newest_mtime_across_sees_a_generated_json_only_change assert coverage. |
is_relevant_backend_change accepts the .json module class, so the two generated JSON artifacts the daemon imports are relevant changes. |
Met | watcher_backend.rs: is_backend_module_file() admits .json and extends TEST_EXTS with .test.json/.spec.json; tests in is_relevant_backend_change_filters_web_test_and_nonts assert generated/interceptor-catalog.json and generated/completion-manifest.json are relevant while test/fixture JSON are not. |
The adoption-time staleness check (adopt_decision → newest_backend_mtime_across) and the (src @ …) uptime hint gain the same coverage, because they read the same root set and relevance filter — no second edit site (mt#4230's single-list invariant holds). |
Met | cockpit-tray/src-tauri/src/supervisor.rs: in do_spawn(), source mtime recorded via cockpit_backend_root(path).and_then(|r| newest_backend_mtime_across(&cockpit_backend_roots(&r))), reusing the same root-set and filter logic as the watcher. Docstrings and README describe single-list invariant. |
The chosen root-set mechanism is recorded with its basis, and explicitly addresses mt#4230's hardcoded-set argument rather than silently adding one more literal. Decided at planning: widen the src/ root from the src/cockpit subdirectory to src/ itself — the entry point's own tree. |
Met | watcher_backend.rs docblocks for daemon_source_tree and cockpit_backend_roots explain the entry-point-tree rationale, discovery posture, dynamic-import coverage, and trade-offs; prose consumers updated (README/CLAUDE.md/docs) echo the basis. |
Roots stay derived where derivation is possible — a change adding a new src/ subdirectory the daemon imports must not require editing this list again. |
Met | daemon_source_tree() returns repo_root.join("src"); test backend_roots_cover_a_src_subdirectory_nobody_named_in_code asserts a new src/zzz-future/ module is covered without naming. |
The existing src/cockpit and packages/*/src behavior is unchanged; web/** edits still never restart the daemon (mt#2299 acceptance test 6 keeps holding through the web component exclusion, which is evaluated on the root-relative path and so still matches cockpit/web/… under the wider root). |
Met | BACKEND_WALK_EXCLUDES includes "web" and backend_path_is_excluded() matches components anywhere; tests in is_relevant_backend_change_filters_web_test_and_nonts assert cockpit/web/* remains excluded under the widened src/ root. |
The five prose descriptions of the watched set are updated to the new coverage (enumerated under ## Scope). |
Met | Updated docs: src/cockpit/CLAUDE.md Operator dev loop (now cites mt#5060 and src/**); cockpit-tray/README.md Backend auto-restart section; cockpit-tray/CLAUDE.md two-layer summary; docs/architecture/cockpit.md Daemon lifecycle and tray; .claude/skills/cockpit-tray-dev/SKILL.md regenerated with updated text. |
Does NOT cover — Over-restart on src/ modules outside the daemon's closure is accepted. |
Met | Design documented in daemon_source_tree docblock; implementation watches src/ superset by construction. Debounce bounds cost; no change required. |
Does NOT cover — package.json at the repo root stays unwatched to avoid restart-before-install loops. |
Met | is_backend_module_file() only admits .ts/.mts/.cts/.json; repo-root package.json is outside watched roots and not a module class; unchanged by this PR. |
Does NOT cover — src/cockpit/web/lib/entity-codec.ts is dual-consumed and remains excluded by the web directory rule. |
Met | BACKEND_WALK_EXCLUDES includes "web"; path-component exclusion still filters cockpit/web/** under the widened src/ root. |
| Does NOT cover — Dynamic imports are not captured by a static-closure enumeration; watching the entry tree is the chosen superset posture. | N/A | Addressed by design via the src/ tree-root superset; no separate behavior to verify beyond the daemon_source_tree(repo_root) implementation and its tests. |
Documentation impact
- updated-in-pr — Documentation and operator-facing prose were updated to reflect the widened backend watcher coverage and
.jsonmodule class:src/cockpit/CLAUDE.md(Operator dev loop cites mt#5060),cockpit-tray/README.md(Backend auto-restart now statessrc/**+packages/*/srcand.json),cockpit-tray/CLAUDE.md,docs/architecture/cockpit.md(Daemon lifecycle and tray), and the compiled.claude/skills/cockpit-tray-dev/SKILL.md. These updates prevent stale guidance aboutsrc/cockpit-only coverage.
Affected: src/cockpit/CLAUDE.md, cockpit-tray/README.md, cockpit-tray/CLAUDE.md, docs/architecture/cockpit.md, .claude/skills/cockpit-tray-dev/SKILL.md
Summary
The tray's backend watcher covered
src/cockpitplus discoveredpackages/*/src. The daemon isbun run src/cli.ts, whose static in-repo closure (measured with aBun.buildrecording plugin, 2026-09-11) is 1,194 files — 465 undersrc/, of which only 143 are undersrc/cockpit. The other 322 (src/adapters216,src/commands47,src/mcp39,src/utils10,src/domain3,src/generated2, …) were unwatched. Independently,is_backend_module_filewas.ts/.mts/.cts-only, so the two generated.jsonimports (src/generated/interceptor-catalog.json,completion-manifest.json) were rejected even where a root covered them.Reproduced from the system's own records rather than by touching the principal's daemon: the daemon boot log (
~/.local/state/minsky/logs/cockpit-daemon*.log) against the localmainreflog and each pull's file set. On 2026-09-10, 6 of 6 pulls that touched a watched root restarted the daemon within 7s; 0 of 4 pulls that touched closure files only outside the watched roots did. The daemon booted at 07:30:53Z and served that tree until 00:18:38Z the next day — through asrc/domain/calibrationchange at 08:27 and twointerceptor-catalog.jsonchanges at 15:14 and 20:39 — until an unrelatedpackages/domainmerge restarted it. Full table in the task spec's## Planning finding — 2026-09-11.mt#4230's docblock stated the closure as
src/**pluspackages/**and its fix widened only thepackages/**half.Key changes
cockpit-tray/src-tauri/src/watcher_backend.rsdaemon_source_tree(repo_root)→src/.cockpit_backend_rootsstarts from it instead ofsrc/cockpit. A tree root is a superset of the closure by construction — a relative import from an entry undersrc/resolves withinsrc/or (via@minsky/*) withinpackages/*/src— and cannot drift as newsrc/subdirectories appear, the same stance mt#4230 took with package discovery. Rejected alternatives and the accepted residuals (over-restart on ~89 non-closuresrc/modules, measured at zero occurrences in the 24h sample;package.json; the sharedweb/lib/entity-codec.ts) are recorded in the task spec.is_backend_module_fileadmits.json. Undersrc/outside the excluded dirs the only.jsonfiles are the two generated artifacts, both imported at boot.cockpit_backend_src(src/cockpit) stays as the presence gate forcockpit_backend_rootonly.webexclusion still applies under the wider root becauseBACKEND_WALK_EXCLUDESmatches path components anywhere in the root-relative path —cockpit/web/…is excluded exactly asweb/…was.(src @ …)uptime hint read the same root set and filter, so they gain the coverage with no second edit site.cockpit-tray/src-tauri/src/supervisor.rs— wiring comment only.src/cockpit/CLAUDE.md,cockpit-tray/README.md,cockpit-tray/CLAUDE.md,docs/architecture/cockpit.md,.minsky/skills/cockpit-tray-dev/content.md(+ its compiled.claude/skills/cockpit-tray-dev/SKILL.md).Testing
Spec ATs use the task's own numbering.
Execution evidence:
AT2 — unit tests over the root set and the relevance filter. Revised:
backend_roots_include_the_daemons_real_import_closure(assertssrc, assertssrc/cockpitis NOT listed beside it),backend_roots_skip_a_package_with_no_src_dir,backend_roots_degrade_when_packages_are_absent(both now expectvec![src]),is_relevant_backend_change_filters_web_test_and_nonts(addscockpit/server.ts,adapters/shared/commands/calibration.ts,generated/interceptor-catalog.json,generated/completion-manifest.jsonas relevant;cockpit/web/App.tsx,cockpit/web/lib/entity-codec.ts,__fixtures__/sample.json,cockpit/web/package.jsonas not). New:backend_roots_cover_a_src_subdirectory_nobody_named_in_code,newest_mtime_across_sees_a_generated_json_only_change(with thesrc/cockpit-only walk as the root-half negative control inside the test).AT3 — the crate builds and its full test suite passes;
cargo fmt --checkclean on both touched files;bun run format:checkclean.AT1 — post-merge confirmation on the rebuilt tray (this session, after merge + rebuild + reinstall): a
mainfast-forward confined tosrc/generated/**or another closure file outsidesrc/cockpitboots the daemon within the debounce window (newPersistenceService initialized successfullyline incockpit-daemon.log;/api/healthcommitadvances). The pre-fix negative control for this AT is the planning measurement above: 0 of 4 such pulls booted the daemon on 2026-09-10.AT4 — build/install-class deliverable (
deployed < usable): rebuild + reinstall the tray after merge and confirm the running app carries the change. Done in this session after merge; the merge alone does not deliver it.Negative control: reverted the two production predicates in place (
cockpit_backend_rootsback tovec![cockpit_backend_src(repo_root)],EXTSback to[".ts", ".mts", ".cts"]) with the revised tests kept, and ran the same module.Each half of the fix has its own failing assertion in the control (the
.jsonrejection pins the module class; the missingsrcroot pins the root set). Both predicates restored afterwards;grep -c "NEGATIVE CONTROL"→ 0 before the final run.Deploy verification:
src/cockpit/CLAUDE.mdis classified deploy surface byisDeploySurfaceFile(it sits undersrc/**, whichdeploy-minsky-mcp.ymldeploys on push to main); every other changed file isfalse. It is a documentation file with no runtime effect, but the predicate is the authority, so after merge this session runsdeployment_wait-for-latest(service: "minsky-mcp", notBefore: <mergeDate>, expectCommitSha: <merge sha>)and readsbuildIdentity, then probes/healthforservice: "minsky-mcp". The change that matters — the tray binary — is not deployed by any pipeline; its delivery is AT4's rebuild + reinstall, verified by AT1.Coordinate
mt#3589 (TODO, watcher reliability INSIDE a watched root) — note for its owner: in the 24h sample every watched-root pull restarted the daemon within 7s (6/6) on the currently-installed tray build.
🤖 Generated with Claude Code
https://claude.ai/code/session_01TPNyYTreM4F7PRbvh4xA5b