From 42ec86b93513f071afa5e900e75b3591dbc8913b Mon Sep 17 00:00:00 2001 From: flupkede Date: Tue, 18 Aug 2026 19:34:11 +0200 Subject: [PATCH 1/3] [chore] root-md cleanup: dissolve stray root mds and docs/ into local .docs/ Root markdown is now limited to the allowlist (AGENTS.md, AGENTS.develop.md, CLAUDE.md, README.md, README_CSharp.md, CHANGELOG.md, RELEASING.md). Diagnosis write-ups, plans, test scenarios and worklogs move to the gitignored .docs/ folder (content preserved locally, untracked); the tracked docs/ folder is dissolved. Rule documented in AGENTS.md "Root file hygiene (markdown)" and AGENTS.develop.md "Key conventions for agents"; .gitignore lists .docs/ explicitly. CLAUDE.md already is the one-line pointer to AGENTS.md. Review-fixes: - [Important] AGENTS.md claims a pre-commit root-md guard that doesn't exist yet -> guard added in the next commit on this branch (stage 2) - [Minor] .gitignore comment said the .*/ rule is "below" when it is above -> corrected - [Minor][pre-existing] src/serve/tests.rs doc-comment pointed at docs/diagnose-federated-keep-warm.md (a path that never existed) -> now .docs/DIAGNOSE_FEDERATED_KEEP_WARM.md --- .gitignore | 6 + AGENTS.develop.md | 6 + AGENTS.md | 15 +- DIAGNOSE_FEDERATED_KEEP_WARM.md | 220 -------------- DIAGNOSE_FIND_IMPACT_ROUTING.md | 271 ------------------ PLAN_TYPESCRIPT_SCIP.md | 239 --------------- TEST-SCENARIO-remote-mount-semantic-search.md | 252 ---------------- docs/federated-silent-poll/worklog.md | 187 ------------ .../watcher-reindex-tui-visibility/worklog.md | 87 ------ src/serve/tests.rs | 2 +- 10 files changed, 27 insertions(+), 1258 deletions(-) delete mode 100644 DIAGNOSE_FEDERATED_KEEP_WARM.md delete mode 100644 DIAGNOSE_FIND_IMPACT_ROUTING.md delete mode 100644 PLAN_TYPESCRIPT_SCIP.md delete mode 100644 TEST-SCENARIO-remote-mount-semantic-search.md delete mode 100644 docs/federated-silent-poll/worklog.md delete mode 100644 docs/watcher-reindex-tui-visibility/worklog.md diff --git a/.gitignore b/.gitignore index 08b24af6..d1e6e952 100644 --- a/.gitignore +++ b/.gitignore @@ -36,6 +36,12 @@ criterion/ # Testing /test-repos/ +# Local-only markdown (diagnoses, plans, worklogs, test scenarios). +# The repo root keeps only its allowlisted .md files — see AGENTS.md +# "Root file hygiene (markdown)". Also covered by the `.*/` rule above; +# listed explicitly so the intent survives a future edit of that rule. +.docs/ + # codesearch database (local index, binary files) .codesearch.db/ test_tools.jsonl diff --git a/AGENTS.develop.md b/AGENTS.develop.md index 45ec1d5b..ac88fd8b 100644 --- a/AGENTS.develop.md +++ b/AGENTS.develop.md @@ -155,6 +155,12 @@ Single-file native binaries, no runtime dependencies. macOS build is manual-trig - **Never write separate `AGENTS_xxx.md` sibling files** unless explicitly requested. OpenCode reads `AGENTS.md` only. Out-of-repo planning goes to `C:\WorkArea\AI\codesearch\instructions\`. +- **Root file hygiene (markdown)**: the repo root keeps only `AGENTS.md`, + `AGENTS.develop.md`, `CLAUDE.md`, `README.md`, `README_CSharp.md`, + `CHANGELOG.md`, `RELEASING.md`. Any other markdown (diagnoses, plans, + test scenarios, worklogs) goes into `.docs/` (gitignored, local-only), + never committed, and never in a recreated tracked `docs/` folder. + Enforced by the `pre-commit` hook root-md allowlist guard. - **Path normalization**: all path comparisons must go through a single normalize utility. Windows UNC prefixes (`\\?\C:\`), backslash/forward-slash mismatches, and worktree `.git` file resolution have each caused subtle bugs in the past. diff --git a/AGENTS.md b/AGENTS.md index d1c175cb..cb792e19 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -20,7 +20,7 @@ Release narratives live in `CHANGELOG.md`; this list keeps only the load-bearing - **Language coverage** — 17 tree-sitter grammars (table in README). `find_impact` has SCIP symbol precision for **C#** (bundled `scip-csharp`) and **TypeScript** (`npx scip-typescript`, host-resolved). Protobuf is Niveau 1 (text-aware chunking on `message`/`enum`/`service`/`rpc`) only — no `scip-protobuf` emitter exists today. - **Scale-to-zero-safe federation: a federated peer is NEVER polled on a timer** — ⚠️ **design constraint, do not "improve" this.** Background polling of *local* repos is fine; a *federated* peer must never be contacted on any cadence. The embedded TUI's discovery tick is **config-only** (`REMOTE_ROW_REFRESH_SECS` = 5s, zero HTTP): it rebuilds mounted-remote rows from the `remote_mounts` allowlist so mount/unmount edits and `l` reloads surface, and contacts nobody. A peer is contacted only by (a) an **activity poke** — a real federated tool call just hit it, detected via `remote_peer_activity` in `ServeState`, refreshing that one peer, never a fan-out — or (b) the explicit `i` info-overlay keypress. Idle mounts therefore render activity as `-`, which is the correct steady state, not a fault. **Rejected reasoning (was shipped twice, PR #181/#184, and reverted):** "polling no faster than the host's idle-suspend term is harmless." It is not — each poll *woke* the peer's scale-to-zero replica, which then self-warmed for its own full idle window (~1h), giving ~50% duty cycle on a peer nobody queried (measured: wakes 120/121/120 min apart, zero searches). Not keeping a peer awake past its suspend term is strictly weaker than not waking it, and the two windows are unrelated values anyway (local host vs. remote peer). - **Standalone remote TUI auth** — `codesearch serve tui --url ...` resolves the API key from `repos.json` (`remotes.*.url` match) or a `--api-key` override and threads the authenticated client through every TUI action, with distinct errors for "no key configured" vs. "key rejected (401)". -- **Keep-warm ping observability + spurious-wake fix** *(branch `fix/federated-silent-poll-diagnosis`)* — the `keep_warm_url` self-ping loop logs every ping (`debug!` on success, `warn!` on failure) instead of discarding both outcomes, and warns at startup when the target host isn't this server's own bind host — **except on a wildcard bind** (`0.0.0.0` / `::`), where our externally-visible host is unknown so the comparison proves nothing; without that carve-out the warning fired on every cold start of the *only* deployment where keep-warm is correct (Azure binds `0.0.0.0`, target is the ingress FQDN), which just trains operators to ignore it. Rule lives in the testable `keep_warm_foreign_target` helper. Keep-warm also **requires a real recorded tool call**: the old `most_recent_tool_call().unwrap_or(start)` fallback meant any wake that wasn't a tool call (`/status` and `/healthz` don't call `record_tool_call`) made the replica self-warm for its whole idle window — reachable *only* when the wake wasn't real work, so its sole practical effect was rewarding spurious wakes (~11× amplification). Full diagnosis, with Azure Log Analytics ground truth: `DIAGNOSE_FEDERATED_KEEP_WARM.md`. +- **Keep-warm ping observability + spurious-wake fix** *(branch `fix/federated-silent-poll-diagnosis`)* — the `keep_warm_url` self-ping loop logs every ping (`debug!` on success, `warn!` on failure) instead of discarding both outcomes, and warns at startup when the target host isn't this server's own bind host — **except on a wildcard bind** (`0.0.0.0` / `::`), where our externally-visible host is unknown so the comparison proves nothing; without that carve-out the warning fired on every cold start of the *only* deployment where keep-warm is correct (Azure binds `0.0.0.0`, target is the ingress FQDN), which just trains operators to ignore it. Rule lives in the testable `keep_warm_foreign_target` helper. Keep-warm also **requires a real recorded tool call**: the old `most_recent_tool_call().unwrap_or(start)` fallback meant any wake that wasn't a tool call (`/status` and `/healthz` don't call `record_tool_call`) made the replica self-warm for its whole idle window — reachable *only* when the wake wasn't real work, so its sole practical effect was rewarding spurious wakes (~11× amplification). Full diagnosis, with Azure Log Analytics ground truth: `.docs/DIAGNOSE_FEDERATED_KEEP_WARM.md`. - **CLI aliases** — `ls` for `list` (`index`/`groups`/`remote`), `rm` for `remove`. `index rm ` resolves a registered alias before falling back to path interpretation. > ℹ️ **Remote write verbs** (`add`, `reindex --force`) require a read-write peer; the cloud peer rejects them (`--force` → HTTP 500 "could only be opened read-only; cannot force-reindex"). An **incremental** `reindex` (no `--force`) of an already-registered repo *does* succeed on the cloud peer — that is the custom-kb auto-refresh path. `list` is always safe. `rm` is not durable — the next cold start re-registers from the restored snapshot. @@ -65,6 +65,19 @@ Common mistake: a subagent runs `/git pr create` with no explicit `--base`, the > > Why the strategy and not the option: against the regressed merge-base, `-X ours` still runs a real three-way merge that treats both sides' content as additions and drags master's stale lines in — a Frankenstein diff (`src/mcp/mod.rs` gained +333 stale lines this way on the v1.2.0 attempt). `-s ours` ignores master's tree entirely and keeps develop's content exactly, which is the desired result here (in this scenario develop's tree already equals master's content); the merge commit only exists to record master as a parent so the merge-base advances. Confirmed empirically on the v1.2.0 release: the `develop → master` PR #185 came back `CONFLICTING`; the throwaway `release/v1.2.0` branch built with `git merge -s ours origin/master` produced an empty content diff and merged clean (#186). +## Root file hygiene (markdown) + +Agents love dropping `*.md` files (diagnoses, plans, worklogs, test scenarios) at the repo root. Don't. The repo root keeps **only** these markdown files: + +- `AGENTS.md`, `AGENTS.develop.md` — agent instructions (and its develop source) +- `CLAUDE.md` — one-line pointer to `AGENTS.md`, nothing else +- `README.md`, `README_CSharp.md` — user-facing docs +- `CHANGELOG.md`, `RELEASING.md` — release infrastructure + +Any other markdown — diagnosis write-ups, implementation plans, test scenarios, worklogs — goes into **`.docs/`** (gitignored, local-only). Never commit it, and never recreate a tracked `docs/` folder; the old `docs/` was dissolved into `.docs/` for this reason. + +Enforced by the `pre-commit` hook (root-md allowlist guard — see `.githooks/README.md`): a commit that adds a root-level `*.md` outside the allowlist is rejected. Dot-folders (`.githooks/`, `.github/`, `.claude/`, …) are exempt — this rule polices only loose files at the root. + ## Notes for OpenCode / agents - **Validation:** `cargo check` and `cargo clippy` for iteration. No `--release` builds — always dev/debug until the very end. diff --git a/DIAGNOSE_FEDERATED_KEEP_WARM.md b/DIAGNOSE_FEDERATED_KEEP_WARM.md deleted file mode 100644 index 609d0b2b..00000000 --- a/DIAGNOSE_FEDERATED_KEEP_WARM.md +++ /dev/null @@ -1,220 +0,0 @@ -# Diagnosis — a federated cloud peer waking up with nobody querying it - -_Branch: `fix/federated-silent-poll-diagnosis` — 2026-08-05_ - -> **Status: root cause CONFIRMED against Azure Log Analytics ground truth.** -> An earlier revision of this document blamed a misconfigured local -> `CODESEARCH_KEEP_WARM_URL`. That hypothesis was **disproven** — see -> [What was ruled out](#what-was-ruled-out) §4. The confirmed cause is a -> two-part defect described in [Root cause](#root-cause). The corresponding -> fixes are listed in [Fixes](#fixes). - -## The requirement being violated - -Background polling of **local** repos is fine and expected. Background -polling of a **federated peer** must never happen — it was an explicit -design constraint from the original federation design, restated by the -reporting user as: - -> "hij mag die repos pollen LOKAAL maar niet federated !!! dat had ik -> nochthans in de specs effectief gezegd bij het ontwerp" - -Two things follow, and conflating them is what caused three round-trips on -this same behaviour: - -- A peer staying warm for its full idle window **after real use** is - *correct*. That is what keep-warm is for. -- A peer being **woken** with no federated query behind it is the defect — - as is it then staying warm for an hour off that spurious wake. - -"Cannot keep a peer awake past the host's own suspend term" is a strictly -**weaker** property than "never wakes it", and only the latter was ever the -requirement. - -## Symptom reported - -A local `codesearch serve` instance kept a mounted cloud federation peer (an -Azure Container Apps replica, `minReplicas: 0`) alive. Quitting the local -instance stopped it. The peer would wake, stay up ~1 hour, sleep, and wake -again — with no federated searches performed in between. Nothing appeared in -the local logs for any of it. - -## Ground truth - -From Log Analytics (`ContainerAppSystemLogs_CL` / `ContainerAppConsoleLogs_CL`) -on the deployed peer, over a period with **zero** federated searches: - -| Observation | Value | -|---|---| -| Interval between wakes | **120, 121, 120 minutes** | -| Warm period per wake | **~67 min** (1h idle window + 5min KEDA `cooldownPeriod`) | -| Nightly sleeps | exactly **2h00m30s** apart | -| Resulting duty cycle | **≈13.4h warm/day, ~56%** — at zero searches | - -The 120-minute spacing is the tell: it is the **local** host's -`DEFAULT_IDLE_SUSPEND_SECS` (2h), not any value configured on the peer. - -Each wake additionally paid for an `azcopy sync` of the docs blob and a -`git pull` of the KB repo. - -## Root cause - -Two independent defects, one triggering and one amplifying. - -### Defect 1 — the trigger: the TUI polled federated peers on a timer - -`spawn_remote_discovery` in `src/serve/tui.rs` used -`Duration::from_secs(state.idle_suspend_secs())` as a baseline poll interval -and, on each elapse, ran a `JoinSet` `/status` fan-out to **every** -configured peer. On the local host that value is 2h — matching the observed -cadence exactly. - -Each fan-out woke the peer's scale-to-zero replica. Nothing else was needed: -the poll *itself* was the ingress traffic. - -The reasoning that shipped this — recorded here so it is not reintroduced a -fourth time — was that polling no faster than the host's own suspend term is -harmless. It is not, for two separate reasons: - -1. Not keeping a peer awake *past* its suspend term says nothing about not - *waking* it. The peer's warm time is bounded, but its wake **count** is - not zero, and each wake costs a full warm window. -2. The two windows are unrelated values. `idle_suspend_secs` was read from - the **local** process (2h default); the window the woken peer then - honoured was the **peer's** (~1h). PR #181's description claimed the - cadence was "1h on the cloud deploy" — it was reading the local value. - -### Defect 2 — the amplifier: keep-warm rewarded spurious wakes - -The cloud keep-warm loop in `src/serve/mod.rs` computed its idle check as: - -```rust -let last = kw_state.most_recent_tool_call().unwrap_or(start); -``` - -`/status` and `/healthz` do **not** call `record_tool_call`. So a replica -woken by anything other than a genuine tool call found no recorded tool -call, fell back to the process start time, and self-pinged its own ingress -every `KEEP_WARM_INTERVAL_SECS` (120s) for the entire idle window. - -The critical observation is that this fallback is **unreachable in the case -it was written for**: a real tool call always sets `last_tool_call`, so the -`unwrap_or` only ever fires when the wake was *not* real work. Its whole -practical effect was to convert a momentary spurious wake into a full warm -hour — roughly **11× amplification** (~67 min instead of the ~6 min a bare -wake would have cost). - -### How they combine - -Defect 1 wakes the peer every 2h. Defect 2 then holds it up for ~67 min per -wake. Neither alone produces the observed 56% duty cycle; together they do. - -## What was ruled out - -1. **Explicit federated tool calls** (`federated_search`, - `federated_project_search`, `federated_get_chunk` in `src/mcp/mod.rs`) — - the only callers of `record_remote_peer_activity`, and only reached when a - project resolves to a federated alias. No federation-shaped log lines - existed in a full day's logs for either the reporting instance or an - unrelated local hub used to cross-check. -2. **`Watch-CodesearchServeReplicas.ps1`** — does poll `/status` every 20s, - but last ran 2026-07-05, well before the observed window. -3. **A stale binary re-introducing an old bug** — the reported startup banner - was `v1.2.1`. Worth upgrading, but the 2h cadence exists in that version - too. -4. **A misconfigured local `CODESEARCH_KEEP_WARM_URL`** *(the earlier - revision's stated root cause — disproven)*, on four independent grounds: - - The env var is set **nowhere** locally: not in the process environment, - not in `HKCU`, not in `HKLM`, not in any shell profile. - - The one-time `🔥 keep-warm enabled` line appears in **zero** local logs - from 2026-04-26 onward. - - That absence is meaningful: `init_serve_logger` is *always* file-only in - serve mode, unconditional on `--no-tui`, and those logs do carry other - `INFO` lines — so the line would have been captured had it fired. - - No local `codesearch` process held any connection on `:443`. - -Note that the earlier revision also ruled out TUI federated polling, on the -grounds that `maybe_spawn_tui` is gated on `!no_tui && is_tty()`. That gating -is real, but the conclusion was wrong: the reporting user's *waking* instance -was a normal TTY serve with the TUI running. Only the separate `--no-tui` -cross-check instance was exempt. - -## Fixes - -### Shipped earlier on this branch (commit `55fa36b`) - -Keep-warm observability, in `src/serve/mod.rs`: - -1. **Per-ping logging** — success at `debug!`, failure at `warn!`. Previously - `let _ = client.get(&ping_url)...send().await;` discarded both, leaving a - single one-time "enabled" line as the feature's only trace. -2. **Startup misconfiguration warning** — `extract_host_from_url` (no new - dependency) compares the keep-warm target host against the server's own - bind host and warns when they differ. - -Tests: `src/serve/tests.rs::keep_warm_host_extraction_tests`. - -### Defect 1 — no timer poll of federated peers - -`spawn_remote_discovery` no longer polls on any cadence. The periodic tick is -**config-only** (`REMOTE_ROW_REFRESH_SECS` = 5s, zero HTTP): it rebuilds -mounted-remote rows from the `remote_mounts` allowlist so mount/unmount edits -and `l` reloads surface promptly, and contacts nobody. - -A peer is contacted only by: - -- an **activity poke** — a real federated tool call just landed on that peer, - so it is demonstrably already awake; only that peer is refreshed, never a - fan-out, so an idle sibling peer is untouched; -- the explicit **`i`** info-overlay keypress on a remote row. - -Consequences: an idle mount renders its activity as `-`, which is now the -correct steady state rather than a fault. `ServeState::idle_suspend_secs` -(field, env init, getter and `--idle-suspend-secs` override) is removed — it -existed only to feed the poll cadence and became write-only. The keep-warm -task resolves flag > env > default directly, so `--idle-suspend-secs` is -unchanged. The `initial_cycle` startup gate is gone: every cycle is now -config-only, so it had nothing left to gate. - -Also fixed in passing: the snapshot emit was gated on a non-empty peer list, -so removing the *last* peer from `repos.json` left its rows on screen -forever. It is now unconditional. - -### Defect 2 — keep-warm requires a real tool call - -The `unwrap_or(start)` fallback is removed: with no tool call recorded there -is nothing to keep warm for, so the loop simply does not ping. A freshly -deployed replica now sleeps until first real use instead of self-warming for -an hour, which is the intended behaviour of scale-to-zero. - -### Follow-up — the `55fa36b` warning false-positived on the correct deploy - -The startup "target isn't self" warning fired on the **only deployment where -keep-warm is correct**: on Azure the process binds `0.0.0.0` while -`keep_warm_url` is the ingress FQDN, so `looks_like_self` was false and the -warning fired on every cold start. A wildcard bind means the -externally-visible host is genuinely unknown, so the comparison cannot -conclude anything and must stay silent — a check that cries wolf on the -correct configuration trains operators to ignore the case that matters. - -Fixed alongside Defect 2. The rule now lives in a testable -`keep_warm_foreign_target(ping_url, self_host) -> Option` helper -(`None` = do not warn), covered by tests for wildcard binds, a genuine -foreign host, a matching host, loopback targets, and an unparseable URL. - -## Residual surface (known, not currently exploitable) - -The MCP **`status` tool** passes `allow_unscoped = true`, but when it is -*project-scoped* (or the replica is single-repo) `is_multi` is false, so the -`!allow_unscoped || !is_multi` guard lets it through and it **does** record a -tool call. An automated poller calling the MCP `status` *tool* with -`project=` would therefore still buy a full warm window. - -No such poller is known to exist: both `Watch-CodesearchServeReplicas.ps1` -and `FederationClient::list_repos` use the **HTTP** `/status` endpoint -(`status_handler`), which does not record. Noted here so that if the -symptom ever recurs, this is the first place to look. - -## Local repos - -Unaffected by all of the above, by design. diff --git a/DIAGNOSE_FIND_IMPACT_ROUTING.md b/DIAGNOSE_FIND_IMPACT_ROUTING.md deleted file mode 100644 index 2f912aef..00000000 --- a/DIAGNOSE_FIND_IMPACT_ROUTING.md +++ /dev/null @@ -1,271 +0,0 @@ -# DIAGNOSE — Waarom kiest de agent zelden `find_impact`? - -> **Status:** DIAGNOSE-EERST. Dit document levert geen fix, maar een reproduceerbare -> analyse met gehard bewijs uit de broncode, een hypotheses-overzicht, een geïsoleerde -> oorzaak, en pas daarna gefaseerde fix-opties (geen blinde oplossing). -> **Symptoom:** de agent pakt voor "wie roept X aan / wat breekt als ik X hernoem" -> vrijwel altijd `find kind=usages` (BM25/tekst-benadering) of `search(semantic)`, -> zelden `find_impact` — terwijl `find_impact` het enige SCIP-backed call-graph-pad is. -> **Repo:** `codesearch-git`. Validatie: `cargo check` + `cargo clippy -D warnings`. - ---- - -## 1. Doel & scope - -**In scope** -- Vaststellen **waarom** de agent `find_impact` mijdt, met bewijs op 3 lagen: - server-instructies, tool-descriptions, en deploy-realiteit. -- De keuze instrumenteerbaar maken (zowel server- als agent-kant). -- Gefundeerde fix-opties aandragen — niet blind één implementeren. - -**Niet in scope (pas ná isolatie)** -- De daadwerkelijke code-fix. Die volgt uit de gekozen optie in §7. -- TS/andere-talen SCIP-backends (apart plan: `PLAN_TYPESCRIPT_SCIP.md`). - ---- - -## 2. Symptoom & observatie - -| Vraagtype | Verwachte tool | Werkelijk gekozen (observatie) | -|-----------|----------------|--------------------------------| -| "wie roept `foo()` aan?" | `find_impact` | `find kind=usages` of `search` | -| "wat breekt als ik `Bar` hernoem?" | `find_impact` | `find kind=usages` | -| "toon call-graph van `X`" | `find_impact` | `search(semantic)` of `find` | - -Het gedrag is **consistent reproduceerbaar**: stel de vraag in een willekeurige -agent-sessie die codesearch-MCP gebruikt → agent kiest `find`/`search`, niet `find_impact`. - ---- - -## 3. Bewijsmateriaal uit de broncode (hard evidence) - -De oorzaak is niet verborgen — ze staat letterlijk in wat de server aan de agent -voert. Drie lagen, allemaal in `src/mcp/mod.rs`: - -### 3.1 Server-instructies (worden in de agent system-prompt geïnjecteerd) -`INSTRUCTIONS_TEMPLATE` (`src/mcp/mod.rs:7915-7953`) — exacte regels die de agent ziet: - -``` -PICK THE RIGHT TOOL FOR THE TASK: - "who calls X?" / "what breaks if I rename X?" - → find_impact (C# via SCIP; other languages: use find kind="usages") ← 7931 -RULES: - - search(semantic) is the DEFAULT for code lookup. Don't skip it. ← 7944 - - find_impact for C# refactors; find(kind="usages") for other languages. ← 7945 -``` - -**Drie biases in deze tekst:** -1. Regel 7931 routeert "who calls X?" voor **elke niet-C# taal** expliciet naar `find kind=usages`. -2. Regel 7944 positioneert `search(semantic)` als de **DEFAULT** — alles wat niet expliciet anders is, valt terug op search. -3. Regel 7945 kadermt `find_impact` als "C# **refactors**" — smal, niet als algemene call-graph-tool. - -### 3.2 `find_impact` tool-description (`src/mcp/mod.rs:6236`) -``` -"Symbol impact analysis — find all references ... (SCIP). - ... More accurate than text-based `find kind=\"usages\"` ... - Languages: C# today (requires the `scip-csharp` helper ...). - For Rust/Python/Go/etc., use `find` with `kind=\"usages\"` as a text-based fallback - until SCIP backends for those languages ship." ← ACTIEVE DOORVERWIJZING WEG -``` -De tool-description **zelf** zegt de agent om `find_impact` te vermijden voor niet-C#. -Dit is de sterkste bias: de tool die we willen promoten, ontmoedigt zichzelf. - -### 3.3 `find` tool-description (`src/mcp/mod.rs:4611`) -``` -"- `usages`: find all call-sites and references to a symbol" -``` -Generiek, geen caveat, geen verwijzing dat `find_impact` preciezer is. `find` presenteert -zich als het algemene antwoord op "who calls X" — voor **alle** talen, zonder drempel. - -### 3.4 README + zoekresultaat-meta (versterking) -- `README.md:307-321`: publieke docs framen `find_impact` als "Currently supports **C#**", - "Requires the `-with-csharp` release variant". -- **Ironische meta-observatie:** de server emit bij zwakke zoekresultaten zelf een - `suggested_tool: "find with kind=usages"` note — dus het systeem adviseert actief `find`, - nooit `find_impact`. - -### 3.5 Deploy-realiteit (de derde laag) -`find_impact` faalt als er geen `scip-csharp` helper is (`mcp/mod.rs:6332-6347`, -`is_available()` check → retourneert een error-JSON met `hint_for_agent`). Op een -serve-hub **zonder** `-with-csharp` variant faalt `find_impact` dus altijd. Een agent -die het één keer probeert en een error terugkrijgt, leert het daarna vermijden — -self-reinforcing. `find kind=usages` faalt nooit (puur tekst-index, altijd aan). - ---- - -## 4. (a) Reproduceren & instrumenteren - -Doel: **meetbaar** maken welke tool de agent kiest en waarom, bij welke queries. - -### 4.1 Wat de server al logt (server-kant = "welke tool") -`tracing::info!` bij elk tool-call: -- `find_impact`: `mcp/mod.rs:6242` (symbol_name, file, line, language, project) -- `find`: `mcp/mod.rs:4622` (symbol, kind, project, group) -- `search`: aparte `📥 search` log - -→ **De "welke tool" is al traceerbaar** via de serve-logs. Wat ontbreekt is aggregatie. - -### 4.2 Wat de server NIET kan loggen (agent-kant = "waarom") -De keuze "find_impact vs find" wordt in het **LLM-hoofd** van de agent gemaakt, vóór de -tool-aanroep. De server ziet alleen de uitkomst. Om het "waarom" te vangen: - -| Laag | Wat loggen | Hoe | -|------|-----------|-----| -| Server | tool-callfrequentie per type + per taal + outcome (ok/fout) | structured counter/metrics naast tracing; bv. `tool_calls{tool="find_impact",lang="csharp",outcome="ok"}` | -| Server | of `find_impact` faalde door `!is_available` vs `No symbol indexer` | aparte outcome-labels op de counter | -| Agent-harness (opencode/claude) | de tool-selectie-reasoning vóór de call | opencode-session-logs / een wrapper die de assistant-tekst vóór tool_usecapt met "find_impact\|find\|search" | -| Eval-set | 20 vaste queries → welke tool wordt gekozen | herhaalbare harness-run (zie 4.4) | - -### 4.3 Instrumentatie-voorstel (klein, niet-invasief) -1. **Tally in serve-modus:** een in-memory `HashMap<(tool, language, outcome), u64>`, - exposed via `status kind=index` of een nieuw `/metrics`-veld. Laag risico, lokaal in - `CodesearchService`. Bewijst de frequentie-kloof kwantitatief. -2. **Outcome-differentiatie:** onderscheid `Ok` / `NoIndexer` / `HelperUnavailable` / - `Empty` bij `find_impact` — toont aan of het falen (§3.5) de oorzaak is. - -### 4.4 Repro-harness (deterministisch) -Een klein script/set prompts (20 stuks) met mixed intent: -- 8× "who calls / what breaks" (zou → find_impact) -- 6× "find code about X" (zou → search semantic) -- 6× "where is X defined / imports" (zou → find definition/imports) - -Draaien tegen een C# repo **met** scip-csharp én een C# repo **zonder**. Tellen welk % -"who calls" naar find_impact gaat. Vóór fix = baseline, na fix = meting. - ---- - -## 5. (b) Hypotheses (systematisch afgelopen) - -| # | Hypothese | Bewijs nu | Status | -|---|-----------|-----------|--------| -| H1 | Tool-descriptions/afbakening onduidelijk: `find_impact` framt zichzelf als C#-only en raadt `find kind=usages` aan | §3.2 — tool-desc bevat actieve doorverwijzing weg | **Sterk ondersteund** | -| H2 | `find` presenteert zich als de algemene weg; geen caveat dat `find_impact` preciezer is | §3.3 — find-desc "find all call-sites" zonder drempel | **Sterk ondersteund** | -| H3 | Server-instructies routeren "who calls X?" voor niet-C# expliciet weg van find_impact | §3.1 — regels 7931/7945 | **Sterk ondersteund** | -| H4 | Overlappende affordances: zowel find_impact als find kind=usages beantwoorden "who calls X" → agent kiest de generiekere | §3.2+§3.3 combi | Ondersteund (gevolg van H1+H2) | -| H5 | Server-side routing/ranking verbergt find_impact | §4.1 — geen routering die find_impact verbergt; tool is altijd geregistreerd | **Verworpen** | -| H6 | Deploy-realiteit: zonder scip-csharp faalt find_impact → agent leert vermijden | §3.5 — is_available-error | Ondersteund (versterkt H1 voor niet-C#-deploy) | -| H7 | `search(semantic)` als DEFAULT schuift find_impact naar de marge | §3.1 regel 7944 | Ondersteund (zwakker, secundair) | - -**Conclusie H1–H4+H6 zijn allemaal ondersteund en versterken elkaar** → de oorzaak is -multicausaal maar concentreert zich in **framing/afbakening** (beschrijvingen + instructies), -niet in server-routing (H5 verworpen). - ---- - -## 6. (c) Vermoedelijke oorzaak — geïsoleerd - -> **De agent mijdt `find_impact` niet ondanks, maar **door** de documentatie.** - -Eén samengestelde oorzaak, drie dragers: - -1. **Zelf-ontmoedigende tool-description** (`mcp/mod.rs:6236`): `find_impact` zegt letterlijk - "For Rust/Python/Go/etc., use `find` with `kind=usages`". Een agent die deze tekst leest - vóór tool-selectie, volgt die instructie op — correct gedrag, foute uitkomst. -2. **Asymmetrische framing**: `find kind=usages` (4611) claimt zonder voorbehoud "find all - call-sites and references"; `find_impact` geeft zichzelf een taal-drempel. De generiekere - tool wint bij ambiguity. -3. **Server-instructies versterken** (7915-7953): routeert "who calls X?" voor niet-C# - expliciet naar `find kind=usages`, en positioneert `search(semantic)` als default. - -**Dus: het probleem zit in de tekstlaag (descriptions + INSTRUCTIONS_TEMPLATE), niet in -code-logica of routing.** Dat maakt het goed te fixen, maar ook makkelijk te onderschatten -— de "fix" is bewerken van strings, geen refactor. H6 (deploy-falen) is een versterker: -zelfs als de tekst is herzien, blijft `find_impact` falen op een serve-hub zonder scip-csharp; -dat moet via §7-optie B (delegatie) of de losse TS-SCIP-track worden opgelost. - ---- - -## 7. (d) Fix-opties (gefaseerd, niet blind — kies na diagnose-bevestiging) - -### Optie A — Tool-descriptions + instructies herzien (kleinste, eerste stap) -**Wat:** -- `find_impact`-desc (6236): verwijder de actieve doorverwijzing "use find kind=usages". - Hernoem naar taal-neutraal: "Precision symbol impact via SCIP where available; falls back - to lexical matching for languages without a SCIP backend." Maak van SCIP een bonus, niet - een voorwaarde in de framing. -- `find`-desc (4611): voeg bij `usages` een caveat — "lexical/text-based; for IDE-precise - call-graphs use `find_impact`". -- `INSTRUCTIONS_TEMPLATE` (7931/7945): routeer "who calls X?" → `find_impact` als **default**, - niet als C#-uitzondering. `find kind=usages` als fallback alleen als find_impact geen index heeft. -- README (307-321): maak find_impact de aanbevolen call-graph-tool, scip-csharp als - "precision boost" i.p.v. harde vereiste in de framing. - -**Voorspeld effect:** bij de repro-harness (§4.4) stijgt het find_impact-aandeel voor -"who calls X" aanzienlijk — mits een SCIP-index aanwezig is (want anders faalt hij, H6). -**Risico:** op niet-C# repos zonder backend blijft hij falen → agent ziet errors → A alleen -is onvoldoende; combineer met B of de TS-track. - -### Optie B — `find kind=usages` transparant delegatie naar SCIP (middel, structureel) -**Wat:** in `find_usages` (achter `find kind=usages`), detecteer of er een -`SymbolIndexer` voor de betreffende taal/repo beschikbaar + has_index is. Zo ja: roep -`indexer.find_references()` aan (het SCIP-pad) en voeg die resultaten bovenop/ipv de -lexicale match. Zo nee: huidige tekst-based fallback. - -**Effect:** de agent hoeft niets te kiezen — `find kind=usages` wordt automatisch precies -waar SCIP beschikbaar is._lost de asymmetrie (H2) op zonder de agent te belasten. Houdt -`find_impact` als expliciete "geef me alleen SCIP"-tool voor agents die dat willen forceren. -**Risico:** "transparente" upgrade kan verrassingen geven (andere resultaat-volumen/ --volgorde); documenteer + feature-flag (`CODESEARCH_FIND_DELEGATES_TO_SCIP`, default aan). -Complexiteit: ~1 functie in `find_usages` + taal-detect per query (de file-ext logica uit -`find_impact` 6295 hergebruiken, maar dan generiek). - -### Optie C — Tools samenvoegen (grootst, breekend) -**Wat:** één `find_references`-tool (of `find_impact` hernoemen) die altijd SCIP-voorrang -geeft en valt terug op lexicaal. `find kind=usages` afschaffen of als alias behouden. -**Effect:** elimineert de ambiguity volledig (H4 weg). Maar: breaking voor agents/harnesses -die `find kind=usages` aanroepen; migratiekosten; grotere review. -**Risico:** backward-compat, alias-beheer. **Alleen kiezen als A+B onvoldoende blijken.** - -### Optie D — Language-aware routing binnen `find` (klein, complementair) -**Wat:** de `suggested_tool`-note die de server nu emit (§3.4 meta) uitbreiden: bij een -"who calls"-aardige query op een C# repo, suggesteer `find_impact` i.p.v. `find kind=usages`. -**Effect:** nudges de agent in-session, zonder tool-schema's te raken. -**Risico:** klein; louter aanvullend op A/B. - -### Aanbevolen volgorde -1. **A eerst** (tekst-laag, goedkoop, direct meetbaar in repro-harness). -2. **B als structurele oplossing** (lost H6 op: ook zonder find_impact-aanroep krijgt de - agent SCIP-kwaliteit via de vertrouwde `find`-tool). -3. C alleen als A+B in de eval niet voldoen. -4. D als finishing touch. -De losse TS-SCIP-track (`PLAN_TYPESCRIPT_SCIP.md`) breidt de **dekking** van find_impact uit -(meer talen met écht SCIP); dit diagnose-plan los de **keuze**-bias op. Beide zijn -complementair. - ---- - -## 8. Review-sectie — open ontwerpkeuzes & risico's - -| Keuze | Opties | Risico / afweging | -|-------|--------|-------------------| -| Verwijderen vs. verzachten van "use find kind=usages" in find_impact-desc | hard verwijderen kan agent in niet-C# zonder backend op een falende tool zetten | combineer altijd met B (delegatie) of een duidelijke runtime-foutmelding die wéér naar find_impact... → nee: naar `find kind=usages` als echte fallback (geen cirkel) | -| Delegatie default aan/uit | default AAN = transparante upgrade; default UIT = backward-compat | feature-flag, default aan na evaluatieperiode | -| Meten vóór/na | repro-harness is handmatig vandaag | overweeg een klein geautomatiseerd eval-script in `tests/` of `eval/` | -| `search(semantic)` als DEFAULT-handhaving | verwijderen verzwakt de grep-guard die search beschermt | behouden, maar herformuleer zodat find_impact niet onder "code lookup" valt maar onder "impact/call-graph" als eigen categorie | -| find_impact op repo zonder index | vandaag: error → agent vermijdt | bij delegatie (B) wordt dit onzichtbaar goed; zonder B: betere foutmelding die de agent niet de hele tool laat vermijden | -| Backward-compat van tool-schema's | samenvoegen (C) breekt callers | alleen bij voldoende wins; anders A+B behouden beide tools | - -**Belangrijkste review-waarschuwing:** niet de server-logica is kapot (H5 verworpen) — -de agent volgt de instructies correct. Een "fix" die alleen code-logica aanraakt zonder de -tekstlaag (descriptions/instructies) raakt het hoofdbewijs niet. - ---- - -## 9. Acceptatiecriteria voor de diagnose (waneer is "oorzaak bewezen"?) -- [ ] Repro-harness (§4.4) levert een baseline: % "who calls X" → find_impact vóór fix. -- [ ] Server-tally (§4.3) toont kwantitatief de kloof (find_impact vs find kind=usages). -- [ ] H1–H4+H6 bevestigd, H5 verworpen — met code-citaten uit §3. -- [ ] Eén gekozen fix-optie (A en/of B) geïmplementeerd → repro-harness na fix toont - meetbare stijging van find_impact-aandeel (bij A) of SCIP-kwaliteit bij find (bij B). -- [ ] Geen regressie: bestaande `find kind=usages` op niet-C# repo's blijft werken. - ---- - -## 10. Verwijzingen -- Server-instructies (agent system-prompt bron): `src/mcp/mod.rs:7915-7953` (`INSTRUCTIONS_TEMPLATE`) -- `find_impact`-description + handler: `src/mcp/mod.rs:6236-6380` (taal-detect 6291-6329, is_available 6332-6347) -- `find`-description + dispatch: `src/mcp/mod.rs:4611-4670` -- `suggested_tool`-meta (search-result nudge): emit in zoekresultaat-output -- Publieke docs: `README.md:307-321` (find_impact C#-only framing), `README.md:241-329` (tool reference) -- Instructie-test guard: `src/mcp/mod.rs:407-433` (`test_no_deprecated_tool_aliases_in_instructions`) -- Complementair plan: `PLAN_TYPESCRIPT_SCIP.md` (dekking-uitbreiding, niet keuze-bias) diff --git a/PLAN_TYPESCRIPT_SCIP.md b/PLAN_TYPESCRIPT_SCIP.md deleted file mode 100644 index 34fd9061..00000000 --- a/PLAN_TYPESCRIPT_SCIP.md +++ /dev/null @@ -1,239 +0,0 @@ -# PLAN — TypeScript SCIP-indexering (find_impact + call-graph voor TS) - -> **Status:** PLANNING — geen code geschreven. Dit document is het oppakpunt voor de implementatie. -> **Doel:** `find_impact` en de call-graph voor TypeScript (.ts/.tsx) laten werken zoals nu voor C#, -> door de bestaande C#-SCIP-pijplijn te spiegelen met Sourcegraph `scip-typescript`. -> **Branch-target:** PRs tegen `develop` (zie AGENTS.md gitflow). - ---- - -## 1. Doel & scope - -**In scope** -- `TypeScriptSymbolIndexer` implementeert het bestaande `SymbolIndexer`-trait, gevoed door `scip-typescript` (Sourcegraph, npm CLI). -- `find_impact` (MCP-tool) routeert `.ts`/`.tsx`/`.mts`/`.cts` bestanden naar de TS-indexer. -- Single-pass indexering: `rebuild()` schrijft defs **en** refs in één run naar LMDB (geen two-phase lazy model nodig — zie §3). -- File-watcher pakt `.ts`/`.tsx`-wijzigingen op en triggert een TS-debounced rebuild. -- Tests bewijzen dat `find_impact` op een TS-symbool alle call-sites teruggeeft. - -**Out of scope (follow-up)** -- TS in de release-bundel shippen (`-with-ts` archive / `helpers/typescript/`) — optioneel, scip-typescript is een npm-package dus `npx` volstaat op de host. -- Incrementele `RebuildScope::Files` voor TS (single-pass maakt full-rebuild op kleine/ middelgrote repo's al snel genoeg; incrementeel is een latere optimalisatie). - ---- - -## 2. Hoe C#-SCIP nu werkt (baseline voor spiegeling) - -De TS-feature moet dezelfde raakvlakken gebruiken. Dit is de C#-status quo: - -### 2.1 Trait + registry (`src/symbols/mod.rs`) -- **Trait `SymbolIndexer`** (regels 113-168): `language()`, `rebuild(repo_path, db_path, RebuildScope)`, `find_references(db_path, symbol)`, `find_references_by_position(db_path, file, line)`, `index_age()`, `is_available()`, `has_index()`, `applies_to(repo_path)`, `as_any()`. -- **`SymbolIndexerRegistry`** (regels 172-232): houdt `Vec>`. `new()` (regel 181) registreert **uitsluitend** `CSharpSymbolIndexer::new()`. `get(language)` is case-insensitive. Methodes: `available_languages()`, `installed_languages()` (filter op `is_available()`), `has_index_for()`, `indexed_languages()`. -- **Gedeelde types:** `SymbolReference{file,start_line,end_line,kind}`, `FindImpactResult`, `SymbolIndexError`, `RebuildScope` (Full | Project(PathBuf) | Files{changed,deleted}), `RebuildSummary`, `PrewarmSummary`. - -### 2.2 C#-adapter (`src/symbols/csharp.rs`, 1740 regels) -`struct CSharpSymbolIndexer` implementeert het trait: -- `detect_helper()` / `resolve_helper_path()` / `validate_helper_path()` (239-353) — zoekt `scip-csharp` via env `CODESEARCH_SCIP_CSHARP` of `helpers/csharp/`. -- `find_solution(repo)` / `find_csproj_for_file(repo, file)` (355-391) — entrypoint-detectie (.sln/.csproj). -- `open_scip_env(db_path)` (398-429) — opent LMDB env in `db_path/scip/`, pre-createert 5 named DBs. -- `invoke_index_helper(...)` (433-504) — spawnt `scip-csharp index --solution X --output Y [--filter-project Z]`. -- `invoke_find_refs_helper()` (509-609), `invoke_batch_find_refs_helper()` (954-1033) — **lazy ref-resolutie** subcommands. -- Trait-impl (1124-1641): `language()`="csharp", `applies_to()` checkt .sln/.csproj, `is_available()` checkt `detect_helper()`. - -### 2.3 Two-phase lazy reference model (C#-specifiek — TS doet dit ANDERS) -1. `rebuild()` → `scip-csharp index` emit **alleen definities** → snel. -2. `find_references()` resolvet refs on-demand: defs uit LMDB → cache-check → cache-miss → `scip-csharp find-refs` voor dat symbool → cache resultaat. -3. Pre-warm: `scip-csharp batch-find-refs` resolvet alle refs in één workspace-sessie. -- **C# helper output = custom JSON** (NIET standaard SCIP protobuf), geparseerd door `parse_json_index` in `scip_parse.rs` (regels 136-197). - -### 2.4 LMDB-schema (`db_path/scip/`, 5 named DBs — keys namespaced door SCIP-symbol-scheme taal-prefix) -| DB | key | value | -|----|-----|-------| -| `scip_symbols` | full SCIP symbol | bincode `Vec` | -| `scip_meta` | `"last_rebuild_ts"` | timestamp — **let op: NIET per-taal!** | -| `scip_positions` | `"file:line"` | `Vec` | -| `scip_simple_names` | simple name | `Vec` | -| `scip_ref_cache` | symbol | bincode `Vec` | - -### 2.5 Dispatch-punten die vandaag HARDCODED op C# staan (moeten generaliseren of een TS-tak krijgen) -| Locatie | Regel | Wat het doet | Voor TS | -|---------|-------|--------------|---------| -| `src/mcp/mod.rs` find_impact | 6296 | file-ext → language: alleen `"cs"` | voeg `"ts"/"tsx"/"mts"/"cts"` → LANG_TYPESCRIPT | -| `src/index/manager.rs` tracking | 1124, 1138, 1152 | trackt `.cs` modified/deleted/rename | voeg `.ts`/`.tsx` tracking toe | -| `src/index/manager.rs` debounce-flush | 1236 | `reg.get(LANG_CSHARP)` (hardcoded) | dispatch generiek over registry OF parallelle TS-tak | -| `src/index/manager.rs` notifier-type | — | `CSharpRebuildNotifier` callback | generaliseer of `TsRebuildNotifier` | -| `src/serve/mod.rs` Phase-3 pre-warm | 1045 | `symbol_registry.get(LANG_CSHARP)` | pre-warm loop over alle registry-talen | -| `src/serve/mod.rs` status | — | `CSharpIndexStatus::None/Ready` | generaliseer naar per-taal status-map | - ---- - -## 3. Het TS-pad: spiegelen met scip-typescript - -### 3.1 Kritieke verschillen met C# -| Aspect | C# (scip-csharp) | TS (scip-typescript) | -|--------|------------------|----------------------| -| **Output-formaat** | custom JSON | **standaard SCIP protobuf `.scip`** | -| **Referentiemodel** | two-phase lazy (defs dan refs) | **single-pass** (defs + refs samen) | -| **Entrypoint** | `.sln` / `.csproj` | `tsconfig.json` | -| **Runtime** | self-contained .NET exe | Node CLI: `npx scip-typescript index` | -| **find_references** | on-demand subprocess + cache | **alleen LMDB-lees** (geen subprocess) | - -### 3.2 Consequentie voor de implementatie -1. **Protobuf-parse nodig.** scip-typescript is fixed binary-formaat → optie B (eigen TS-helper die JSON emit) is niet haalbaar. Keuze: de `scip` Rust-crate (Sourcegraph) toevoegen + een parser in nieuw `src/symbols/scip_proto.rs` die `.scip` → zelfde `ScipIndex`-shape mapt als `scip_parse.rs` nu voor JSON doet. Daarna is alle storage/resolution-code herbruikbaar. -2. **Geen two-phase.** TS `rebuild()` vult in één pass `scip_symbols` + `scip_positions` + `scip_simple_names` én de refs. `find_references()` leest alleen LMDB (snel, geen subprocess). `scip_ref_cache` is voor TS leeg/overbodig — schrijven kan geen kwaad (keys namespaced). -3. **`is_available()`** voor TS = detecteer of `scip-typescript` oplosbaar is via env `CODESEARCH_SCIP_TYPESCRIPT` (pad naar binary) of via `npx` op PATH + Node aanwezig. -4. **`applies_to()`** voor TS = zoek een `tsconfig.json` in `repo_path` (root of één niveau diep). - ---- - -## 4. Betrokken files & functies (concreet) - -### 4.1 Nieuwe files -| File | Inhoud | -|------|--------| -| `src/symbols/scip_proto.rs` | `parse_scip_protobuf(bytes) -> ScipIndex` via `scip` crate. Herbruikt `ScipReference`/`ScipIndex` uit `scip_parse.rs`. | -| `src/symbols/typescript.rs` | `struct TypeScriptSymbolIndexer` impl `SymbolIndexer`. Mirrot van `csharp.rs` structuur: `detect_helper()`, `find_tsconfig(repo)`, `open_scip_env()` (hergebruik), `invoke_index_helper()`, trait-impl. | -| `tests/symbols_typescript_test.rs` | Gated integratie-test (zelfde gate-patroon als `symbols_csharp_test.rs`), TS-fixture. | -| `tests/fixtures/ts-sample/` | Klein TS-project: `tsconfig.json` + 2-3 `.ts` files met een functie + call-sites. | - -### 4.2 Te wijzigen files (exacte raakvlakken) -| File | Wijziging | -|------|-----------| -| `Cargo.toml` | voeg `scip` dependency toe (Sourcegraph crate) | -| `src/symbols/mod.rs` regel 181 | `SymbolIndexerRegistry::new()` registreer óók `typescript::TypeScriptSymbolIndexer::new()` | -| `src/symbols/mod.rs` | voeg `pub mod typescript;` + `pub mod scip_proto;` toe | -| `src/constants.rs` | `LANG_TYPESCRIPT="typescript"`, `SCIP_TYPESCRIPT_HELPER_ENV="CODESEARCH_SCIP_TYPESCRIPT"`, `SCIP_TYPESCRIPT_HELPER_NAME="scip-typescript"`, `TS_DEBOUNCE_MS` | -| `src/mcp/mod.rs` regel 6296 | find_impact auto-detect: map `"ts"/"tsx"/"mts"/"cts"` → `LANG_TYPESCRIPT` | -| `src/mcp/mod.rs` regel 6236 | update tool-description (nu: "C# today") → voeg TS toe | -| `src/index/manager.rs` regels 1124/1138/1152 | voeg `.ts`/`.tsx`-tracking velden toe (`ts_files_modified/deleted/last_event_time`) | -| `src/index/manager.rs` regel 1236 | dispatch: óf registry-loop, óf parallelle TS-tak na C#-tak | -| `src/index/manager.rs` notifier | generaliseer `CSharpRebuildNotifier` naar generieke `SymbolRebuildNotifier` (boxed callback) | -| `src/serve/mod.rs` regel 1045 | Phase-3 pre-warm: itereren over registry in plaats van hardcoded `LANG_CSHARP` | -| `src/serve/mod.rs` | vervang `CSharpIndexStatus` door `HashMap` (per-taal) | - -### 4.3 Niet-wijzigen (herbruikbaar) -- `src/symbols/scip_parse.rs` structs (`ScipReference`, `ScipIndex`) — de protobuf-parser mapped hiernaartoe. -- LMDB-schema (de 5 named DBs) — keys zijn namespaced door SCIP-symbol-scheme, dus C# en TS co-existeren in dezelfde `db_path/scip/`. -- `RebuildScope`, `RebuildSummary`, `PrewarmSummary`, `SymbolReference`, `FindImpactResult` types. - ---- - -## 5. Per-taal indexer-selectie (hoe taal-bepaling werkt) - -Twee routes die beide TS moeten ondersteunen: - -### 5.1 Expliciet (MCP find_impact `request.language`) -`SymbolIndexerRegistry::get(language)` is case-insensitive en retourneert de indexer waarvan `language()` overeenkomt. `LANG_TYPESCRIPT="typescript"` → `registry.get("typescript")` werkt automatisch zodra geregistreerd. - -### 5.2 Auto-detect (file-extensie) -`src/mcp/mod.rs:6296` — huidige map is **enkel** `"cs" → LANG_CSHARP`, else fallback naar eerste `installed_languages()`. **Toevoegen:** -```rust -match ext { "cs" => LANG_CSHARP, "ts"|"tsx"|"mts"|"cts" => LANG_TYPESCRIPT, _ => /* fallback */ } -``` -Fallback = huidig gedrag (eerste installed language) — ongewijzigd. - -### 5.3 Applicability (welke indexer pakt een repo op?) -- `applies_to(repo_path)` per indexer: C# checkt `.sln`/`.csproj`, TS checkt `tsconfig.json`. -- `installed_languages()` filtert op `is_available()` (helper gevonden). Een host zonder Node/scip-typescript ziet TS simpelweg niet — geen crash. - ---- - -## 6. Implementatie-stages (volgorde voor PR(s)) - -| # | Stage | Doel | Validering | -|---|-------|------|------------| -| 1 | Protobuf-binding | `scip` crate + `scip_proto.rs::parse_scip_protobuf()` | unit-test: fixture `.scip` file → `ScipIndex` met verwacht # defs/refs | -| 2 | TypeScriptSymbolIndexer | nieuw `typescript.rs`, implementeert trait | `cargo check` + `cargo clippy -D warnings` | -| 3 | Registratie + constants | `mod.rs:181` registreer TS; `constants.rs` lang/env | `installed_languages()` bevat "typescript" als Node aanwezig | -| 4 | find_impact auto-detect | `mcp/mod.rs:6296` map TS-extensies | handmatige smoke: find_impact op een TS-file | -| 5 | File-watcher TS-tracking | `manager.rs` `.ts`/`.tsx` + dispatch | bewerk een `.ts` → debounce-flush triggert rebuild | -| 6 | Tests | `tests/symbols_typescript_test.rs` + fixture | `cargo test --test symbols_typescript_test` groen | -| 7 | Pre-warm + status generaliseren | `serve/mod.rs` registry-loop | startup log toont TS pre-warm | -| 8 | (optioneel) Release-bundling | `release.yml` `-with-ts` | archive bevat scip-typescript binary | - -Stages 1-6 zijn de MVP (find_impact werkt op TS). 7-8 zijn afronding. - ---- - -## 7. Test-strategie: bewijs dat find_impact op een TS-symbool alle call-sites teruggeeft - -### 7.1 Fixture-ontwerp (`tests/fixtures/ts-sample/`) -``` -ts-sample/ - tsconfig.json # compilerOptions, minimal - src/ - math.ts # export function add(a, b) ← TARGET definitie - consumer.ts # import { add }; add(1,2); add(3,4) ← 2 call-sites - other.ts # import { add }; const r = add(5,6) ← 1 call-site -``` -Doel: `add` heeft 1 definitie + 3 call-sites verdeeld over 2 files. - -### 7.2 Integratie-test (`tests/symbols_typescript_test.rs`) -Gated (zelfde patroon als `symbols_csharp_test.rs`: skip als `scip-typescript`/Node niet oplosbaar, geen real embedding nodig). Test-flow: -1. `TypeScriptSymbolIndexer::new()` -2. `.rebuild(&fixture_root, db_path, RebuildScope::Full)` → `assert!(summary.ok)` -3. `.has_index(db_path)` → `true` -4. `.find_references(db_path, "add")` (via simple_name) → `assert_eq!(refs.len(), 4)` (1 def + 3 calls) OF via full SCIP-symbol key -5. `.find_references_by_position(db_path, "src/math.ts", )` → retourneert de `add`-symbol key -6. Cross-check: voor elke call-site file komt deze voor in `refs.iter().map(|r| r.file)` - -### 7.3 find_impact end-to-end (optioneel, handmatig) -Na opstarten van `codesearch serve` met de TS-fixture als project: roep de `find_impact` MCP-tool aan met `{file:"src/math.ts", line:}` en verifieer dat het resultaat overeenkomt met de integratie-test (4 occurrences over 3 files). - -### 7.4 Negative tests -- `find_references` op een onbekend symbool → lege `Vec`, geen panic. -- `.is_available()` op een host zonder Node → `false`; `installed_languages()` bevat geen "typescript". - ---- - -## 8. Review — openstaande ontwerpkeuzes (beslissen vóór/ten tijde van implementatie) - -### 8.1 `scip_meta` is NIET per-taal (design-issue) -`scip_meta` gebruikt key `"last_rebuild_ts"` zonder taal-prefix. Bij twee talen in dezelfde `db_path/scip/` overschrijven C# en TS elkaars timestamp. **Optie:** key namespacen `"last_rebuild_ts:csharp"` / `"last_rebuild_ts:typescript"`. Niet-breaking voor lezers die via `index_age()` gaan. **Beslissing:** namespacen — lokaal in `typescript.rs` een eigen key gebruiken, en later C# migreren. - -### 8.2 File-watcher dispatch: generaliseren vs. parallelle tak -- **Optie A (generiek):** vervang hardcoded `reg.get(LANG_CSHARP)` (manager.rs:1236) door een loop `for lang in registry.indexed_languages()`. Schoon, schaalbaar naar meer talen, maar raakt `CSharpRebuildNotifier`-type (moet generiek `SymbolRebuildNotifier` worden) — grotere refactor. -- **Optie B (parallelle tak):** voeg een tweede `if`-blok voor TS toe, spiegelend het C#-blok. Minder netjes, lokaal, lager risico. -- **Beslissing:** start met Optie B (snel MVP), refactor naar A zodra er een derde taal komt. Documenteer als TODO. - -### 8.3 scip-typescript distributie: `npx` vs. gebundelde binary -- C# shipt een self-contained exe in `helpers/csharp/` + `-with-csharp` release-archives. -- scip-typescript is een npm-package: `npx scip-typescript` werkt als Node op PATH staat. Geen bundling nodig voor development. Voor offline/air-gapped deploy: optie om `npm pack`-tarball te bundelen (follow-up, niet MVP). -- **Beslissing:** MVP = `npx` (env `CODESEARCH_SCIP_TYPESCRIPT` voor override-pad). Bundling = out of scope (§1). - -### 8.4 Incrementele rebuild (RebuildScope::Files) voor TS -C# ondersteunt `Files{changed,deleted}` via csproj-groepering + `--filter-project`. scip-typescript heeft geen file-filter flag — herbouwt steeds de hele tsconfig-projectroot. **Beslissing:** MVP ondersteunt alleen `Full`; `Files` valt terug op `Full` (log + proceed). Voor grote monorepo's is dit later te optimaliseren (per-tsconfig groeperen, zie §8.5). - -### 8.5 Monorepo met meerdere tsconfig.json -`applies_to()` zoekt nu één `tsconfig.json`. Een monorepo met `packages/*/tsconfig.json` vereist het C#-equivalent van `find_csproj_for_file` → een `find_tsconfig_for_file(repo, file)`. **Beslissing:** MVP pakt root-tsconfig; per-file-tsconfig-resolutie = follow-up. In scope zetten als de test-fixture dat meteen nodig maakt. - -### 8.6 Pre-warm: heeft TS het nodig? -TS heeft geen two-phase lazy model → `rebuild()` populate direct alle refs → geen `batch-find-refs` pre-warm nodig. De registry-loop in `serve/mod.rs:1045` mag TS dus overslaan of gewoon `rebuild` aanroepen als index ontbreekt/stale is. **Beslissing:** registry-loop roept per indexer een `prewarm()`-methode aan; C# doet zijn batch-find-refs, TS is no-op (of `rebuild` als index koud). Voeg optionele default-methode `prewarm()` toe aan het trait. - -### 8.7 `scip` crate keuze -Sourcegraph publiceert een `scip` Rust-crate (protobuf bindings + helpers). Alternatief: handmatige `prost`-build tegen de `.proto`. **Beslissing:** gebruik de `scip` crate (onderhouden, zelfde schema als scip-typescript output). Lock versie in `Cargo.toml`; als de crate afwijkt, val terug op `prost`-build. - ---- - -## 9. Acceptatie-criteria (MVP = stages 1-6) -- [ ] `cargo check` + `cargo clippy -D warnings` groen. -- [ ] `cargo test --test symbols_typescript_test` groen op een host met Node + scip-typescript. -- [ ] `find_impact` MCP-tool retourneert voor een TS-functie alle call-sites (≥3 over 2 files in de fixture). -- [ ] `find_impact` auto-detect routeert `.ts`/`.tsx` naar TS-indexer (geen C#-fallback). -- [ ] `installed_languages()` bevat "typescript" als Node aanwezig, niet anders. -- [ ] Host zonder Node: geen crash, TS-indexer gewoon afwezig. -- [ ] C#-pijplijn ongewijzigd werken (geen regressie — bestaande C#-tests groen). - ---- - -## 10. Verwijzingen -- C# trait + registry: `src/symbols/mod.rs:113-232` -- C# adapter (referentie-impl): `src/symbols/csharp.rs` -- JSON-parser (shape om naartoe te mappen): `src/symbols/scip_parse.rs:136-197` -- find_impact MCP-tool: `src/mcp/mod.rs:6238-6369` (taal-detect 6291-6329) -- File-watcher dispatch: `src/index/manager.rs:1124-1340` -- Startup pre-warm: `src/serve/mod.rs:1045` -- Constants: `src/constants.rs` (LANG_CSHARP, SCIP_CSHARP_*, HELPERS_SUBDIR) -- Bestaande tests: `tests/symbols_csharp_test.rs`, `helpers/csharp/tests/IndexerTests.cs` -- scip-typescript (Sourcegraph): https://github.com/sourcegraph/scip-typescript -- scip Rust-crate: https://github.com/sourcegraph/scip-rust diff --git a/TEST-SCENARIO-remote-mount-semantic-search.md b/TEST-SCENARIO-remote-mount-semantic-search.md deleted file mode 100644 index cfe8b715..00000000 --- a/TEST-SCENARIO-remote-mount-semantic-search.md +++ /dev/null @@ -1,252 +0,0 @@ -# Test Scenario — Semantic findability across remote-mounted doc projects - -Acceptance test for the `features/remote-mount-selection` work: does codesearch -**find the right content in the right mounted doc project — and *not* surface it -where it doesn't belong** — and does the federated `get_chunk` round-trip work -end-to-end (the Stage A `ambiguous_chunk_id` fix). - -The corpus is six product-documentation indexes mounted from the `cloud` peer, a -natural mix of **PIM** and **DAM** products. That overlap (two PIMs, three DAMs) -is exactly what makes findability testable: a PIM concept *should* surface in a -PIM index and *should not* have a genuine match in a DAM index, and vice-versa. - ---- - -## 0. Preconditions - -| # | Check | How | -|---|-------|-----| -| P1 | `serve` is active and the six mounts are present | `status(kind="projects")` → `remote_projects[]` lists `cloud/akeneo`, `cloud/example-dam`, `cloud/bynder`, `cloud/custom-kb`, `cloud/digizuite`, `cloud/inriver` | -| P2 | The `docs` group federates the peer | `status(kind="projects")` → `groups.docs == ["@cloud"]` | -| P3 | **The serve process runs the Stage-A binary** | A remote search result's `chunk_ref` is namespaced `"cloud/inriver:"`, **not** the legacy `"cloud:"`. See ⚠️ below. | - -> ⚠️ **Known state at time of writing:** the live serve still runs a **pre-Stage-A -> binary** — remote results come back with legacy `chunk_ref` `"cloud:"` (no -> alias). Section **D** is therefore the gating regression test: it is expected to -> reproduce the old `ambiguous_chunk_id` bug on the current binary and to pass only -> after serve is rebuilt/restarted with the fixed binary. Redeploy, then re-run. - -### The mounted products (concept map) - -| Mount | Product | Domain | Signature concepts (owns) | -|-------|---------|--------|---------------------------| -| `cloud/inriver` | inriver | **PIM** | entity/link model, variants, channels, syndication, Enrich, Control Center | -| `cloud/akeneo` | Akeneo | **PIM** | product families, attribute groups, categories, reference entities, connectors | -| `cloud/example-dam` | example-dam | **DAM + MO** | Marketing Operations, workflow designer, DAM records, classifications, review/approval | -| `cloud/bynder` | Bynder | **DAM** | asset portal, brand guidelines, Studio, collections, asset workflow | -| `cloud/digizuite` | Digizuite | **DAM** | DAM Center, media renditions, transformations, publishing destinations | -| `cloud/custom-kb` | custom KB | mixed | wildcard — no assumption | - ---- - -## ⚖️ Scoring caveat — READ THIS BEFORE JUDGING RESULTS - -Result `score` is **RRF (Reciprocal Rank Fusion)** — a *rank-based* number, not an -absolute similarity. In calibration the **top hit scored ~0.0476 in every index**, -including one where the query had no genuine match. So: - -- **Never judge findability by the score number.** The top score is ~0.0476 whether - the match is perfect or garbage. -- **Judge findability by the returned content**: does the top-ranked chunk's - `path` + body actually address the queried concept? -- A **true positive** = the top 1–3 chunks are *on-topic* docs for the concept. -- A **true negative** = the top chunks are *off-topic* (release notes, unrelated - features) — the concept simply isn't documented in that product. - ---- - -## A. True positives — semantic recall in the owning product - -Each query is phrased in **different words** than the docs use, so a plain keyword -match would miss it. Semantic search must still surface the right doc. -Run with `search(mode="semantic", project="", query="…", limit=5)`. - -| # | Project | Query (natural language) | PASS = top 1–3 chunks are about… | Calibrated? | -|---|---------|--------------------------|----------------------------------|-------------| -| A1 | `cloud/inriver` | "how are product entities linked to variants and sales channels" | inriver **entity / elastic data model** (e.g. `…/What-is-an-entity.md`, channel/link docs) | ✅ verified — hit `getting-started/elastic-data-model-common-terminology/…What-is-an-entity.md` | -| A2 | `cloud/akeneo` | "grouping product attributes into families and attribute groups" | Akeneo **families / attribute groups** docs | ⬜ to verify | -| A3 | `cloud/example-dam` | "digital asset review and approval workflow" | example-dam **Marketing Operations workflow** (e.g. `…/workflow_admin/workflow_designer_concepts…`) | ✅ verified — hit `Marketing_Operations_Help/workflow_admin/workflow_designer_concepts.html.md` | -| A4 | `cloud/bynder` | "set up an asset approval workflow and organize assets into collections" | Bynder **Asset-Workflow / collections** (e.g. `…/Asset-Workflow/…Asset-Workflow.md`) | ⬜ to verify (Asset-Workflow.md already appeared as a side hit under B1) | -| A5 | `cloud/digizuite` | "generate media renditions and publish them to a destination" | Digizuite **renditions / transformation / publishing** docs | ⬜ to verify | - -**Expected:** all five PASS. Record the top chunk `path` + `chunk_ref` for each in -the results table. - ---- - -## B. True negatives — a concept that lives in the *other* domain - -Take a concept a product genuinely **does not have** and query the product that -lacks it. Semantic search will still return *something* (it always ranks the -top-k), so PASS is defined by **off-topic** content, not an empty result. - -| # | Project | Query (from the *wrong* domain) | PASS = top chunks are OFF-topic (concept absent) | Result | -|---|---------|--------------------------------|--------------------------------------------------|--------| -| B1 | `cloud/bynder` (DAM) | "how are product entities linked to variants and sales channels" (PIM) | No PIM entity/link model; hits are generic DAM articles | ✅ **clean negative** — `Product-Feedback…`, `…AI-Agents…`, Studio; no PIM model | -| B2 | `cloud/example-dam` (DAM) | "how are product entities linked to variants and sales channels" (PIM) | No PIM entity model in a DAM/MO product | ✅ **clean negative** — `system_types_reference`, DAM `RecordLink` field, `clients_associated_programs`; no PIM entity/variant/channel model | -| B3 | `cloud/inriver` (PIM) | "automatically generate cropped image renditions and file derivatives from a master asset" (DAM) | No rendition/derivative engine in a PIM | ✅ **clean negative** — top hits are release notes / product announcements; inriver has no image-rendition transformation | -| B4 | `cloud/akeneo` (PIM) | "track marketing campaign budget spend and program financial actuals" (example-dam MO) | No budget/financials in a PIM catalog | ✅ **clean negative** — top hits are Google-Shopping insights / Studio analytics; Akeneo has no marketing-spend tracking | - -> **📌 Key lesson — how to design a clean true-negative probe.** -> A clean true negative needs a concept with **no adjacent feature** in the target -> product. Two examples of what *not* to do, found while building this scenario: -> - "brand guidelines portal" against `cloud/inriver` → matched inriver's own -> **Brand Store** (`…Introduction-to-the-new-Brand-Store.md`). -> - "workflow designer and task approvals" against `cloud/akeneo` → matched Akeneo's -> own **collaboration workflows** (`…what-are-collaboration-workflows.md`). -> -> Neither was the queried DAM/MO concept, but both are *genuine* features of the PIM -> product — so semantic search correctly surfaced the nearest real concept. That is -> **semantic search working**, not a scoping failure. The B3/B4 queries above were -> therefore sharpened to concepts that are truly unique to the *other* domain -> (rendition generation = DAM engine; marketing budget = example-dam MO), which produce -> clean negatives. Rule of thumb: probe with a **product-unique** concept, never a -> generic verb like "workflow" or "portal". -> -> A real regression would be a DAM index returning an **on-topic PIM entity-model** -> doc for B1/B2 — that would mean mis-scoped mounts or corpus contamination. - ---- - -## C. Cross-product overlap — shared concept, per-product answers - -A concept the three DAMs **all** share ("metadata fields on a digital asset"). -Query each DAM individually, then the whole peer via the group. - -| # | Scope | Query | PASS = | -|---|-------|-------|--------| -| C1 | `project=cloud/example-dam` | "add and edit metadata fields on a digital asset" | example-dam field/classification docs | -| C2 | `project=cloud/bynder` | "add and edit metadata fields on a digital asset" | Bynder metaproperty/tagging docs | -| C3 | `project=cloud/digizuite` | "add and edit metadata fields on a digital asset" | Digizuite metadata docs | -| C4 | `group=docs` | "add and edit metadata fields on a digital asset" | Fused results from **multiple** peers; each result carries the correct `source`/`chunk_ref` for its origin | - -**Expected:** C1–C3 each return that product's own vocabulary; C4 interleaves hits -from more than one DAM and every result is correctly attributed. (C4 also exercises -RRF fusion across federated peers.) - ---- - -## D. `get_chunk` namespaced round-trip — Stage A acceptance / regression - -This is the **gating** test for the fix. inriver on the peer is a multi-repo index, -which is exactly the shape that triggered the original `ambiguous_chunk_id` bug. - -**Steps** -1. `search(project="cloud/inriver", query="what is an entity", limit=3)` → note the top result's `chunk_ref`. -2. `get_chunk(chunk_ref="", context_lines=5)`. - -| Binary | Step 1 `chunk_ref` shape | Step 2 result | -|--------|--------------------------|---------------| -| **Old (pre-Stage-A, current live serve)** | legacy `"cloud:"` — alias dropped | ❌ FAIL — `ambiguous_chunk_id` (peer can't disambiguate the multi-repo index), the bug that started this | -| **New (Stage-A binary)** | namespaced `"cloud/inriver:"` | ✅ PASS — returns the chunk body; `project=inriver` is forwarded to the peer so the lookup is unambiguous | - -**PASS criteria (new binary):** -- `chunk_ref` is `"cloud/inriver:"` (namespaced). -- `get_chunk` returns the chunk `content` (the entity-definition prose), **not** an error. -- A legacy `"cloud:"` ref still resolves via the group-scope fallback (backward-compat) — optional extra check. - ---- - -## E. Fail-open sanity (web-guard interplay) — optional - -Confirms the guard doesn't over-block once mounts exist and steers correctly. - -| # | Setup | Action | PASS = | -|---|-------|--------|--------| -| E1 | mounts present (P1) | trigger a `WebSearch` on a product-doc question | web-guard **denies once** with guidance to `search(project="cloud/…")` + `get_chunk(chunk_ref=…)` | -| E2 | same query retried within 5 min | repeat the `WebSearch` | guard **allows** it (retry-escape) | -| E3 | `remote_mounts` empty in `repos.json` | trigger a `WebSearch` | guard **passes through** (fail-open, nothing to steer toward) | - ---- - -## F. Cross-vendor overlap + isolation (5 scenarios) - -Where **B** proved isolation (a concept absent from the wrong domain), **F** proves -the complementary half: a concept **shared** across vendors must surface hits from -**multiple vendors at once** via `group="docs"` (federated RRF fusion) — while a -domain-specific concept still stays absent from the other domain (isolation). - -Run each **overlap query** with `search(group="docs", …)` and confirm ≥2 vendors -return *on-topic* hits. Run each **isolation probe** with `project=""` -and confirm *off-topic* results. All rows below were executed (Run 1). - -> **How `group="docs"` fusion reads:** RRF interleaves each peer's rank-1 hit at the -> same top score (~0.0476), so a healthy overlap looks like *one strong hit per -> relevant vendor* stacked at the top. Judge by the `path`, not the score. - -### F1 — Category hierarchy *(cross-domain organizational concept)* -- **Overlap** `group=docs`: *"organize products into a category hierarchy or category tree"* -- **On-topic, multi-vendor:** akeneo `…/serenity-what-is-a-category.md`, bynder `…/Glossary/…What-is-a-Taxonomy.md`, digizuite `…/api/tree/nodes/item/…` -- **Adjacent (not wrong):** example-dam `expense_hierarchies` (MO financial), inriver release notes, custom-kb classification pickers -- **Verdict:** ✅ PASS — 3 vendors on-topic across **both** domains (PIM akeneo + DAM bynder/digizuite) - -### F2 — Product data completeness *(PIM-owned overlap + DAM isolation)* -- **Overlap** `group=docs`: *"measure product data completeness and enrichment quality"* -- **On-topic PIM:** inriver `…/working-in-enrich/…different-completeness-rules….md`, akeneo `…/understand-data-quality.md` -- **Isolation probe** `project=cloud/bynder`: top hits `Tips-For-Measuring-Success-And-Adoption`, `Stibo-Integration`, `Collections-Dashboard` — **no** product-completeness concept -- **Verdict:** ✅ PASS — two PIMs own it; a pure DAM does not (clean isolation) - -### F3 — Asset access permissions *(DAM-owned overlap)* -- **Overlap** `group=docs`: *"restrict who can view or download an asset using permissions and rights"* -- **On-topic:** bynder `…/Permission-Management/…Customize-User-Permissions-to-Download-Assets.md`, digizuite `…/api/assets/security/…`, example-dam `…/rights_reference.html.md`, akeneo (DAM module) `…/set-rights-on-your-asset-families.md` -- **Isolation signal** inriver: `…/entities/…Locking-Entities.md` — its own *entity-locking*, not asset download → stays in its lane -- **Verdict:** ✅ PASS — 3 DAMs + Akeneo's DAM module converge; PIM entity-locking is adjacent, not a false hit - -### F4 — Asset version history *(DAM-owned overlap + clean PIM isolation)* -- **Overlap** `group=docs`: *"keep version history of an asset and revert to a previous version"* -- **On-topic:** example-dam `…/digital_assets_creating_versions.html.md`, bynder `…/Upload/…Upload-New-Version-of-an-Asset.md`, digizuite `…/api/assets/create-versions.md`, akeneo `…/how-to-view-and-restore-a-previous-version-of-an-asset.md` -- **Isolation probe** `project=cloud/inriver`: **release notes only** — inriver (PIM) has no asset-versioning/revert -- **Verdict:** ✅ PASS — strongest 4-vendor DAM overlap + clean PIM isolation - -### F5 — Publish / syndicate to a channel *(true cross-domain overlap — the highlight)* -- **Overlap** `group=docs`: *"publish or syndicate content out to an external channel or destination"* -- **On-topic PIM:** inriver `…October-2025…Syndication-Workflows….md`, akeneo `…/managing-and-distributing-enhanced-content.md` -- **On-topic DAM:** example-dam `…/integration_workbench_publishers_concept.html.md`, bynder `…/Guide-to-Delivering-Multi-Channel-Content-with-Content-Workflow.md`, digizuite `…/api/admin/mediatranscode.md` -- **Verdict:** ✅ PASS — on-topic hits from **both** domains; the best single demonstration of full-peer federated fusion - -### Verdict — F (overlap + isolation) - -**5/5 PASS.** Federated RRF fusion surfaces the right *set* of vendors for a shared -concept (F1/F5 span both domains; F3/F4 converge the DAMs), and isolation still holds -where a concept is domain-specific (F2 PIM-only, F4 PIM has no asset versioning). This -is the positive counterpart to B: not just "not found in the wrong place", but -"found across all the right places, each correctly attributed". - ---- - -## Run 1 — executed results (Stage-A binary, serve restarted) - -`chunk_ref` came back **namespaced** (`cloud/:`) and `source` = `cloud/` -on every remote result → **P3 PASS**, the Stage-A fix is live. - -| Case | Scope | Query | Top chunk `path` | `chunk_ref` | Verdict | -|------|-------|-------|------------------|-------------|---------| -| A1 | cloud/inriver | product↔variant↔channel | `…/elastic-data-model…/What-is-an-entity.md` + `…/Intelligent-linking-of-Entities…md` | `cloud/inriver:1004` | ✅ PASS | -| A2 | cloud/akeneo | families/attribute groups | `…/serenity-what-is-a-family.md` + `…/manage-attribute-inheritance.md` | `cloud/akeneo:3770` | ✅ PASS | -| A3 | cloud/example-dam | asset review/approval | `…/workflow_admin/workflow_designer_concepts.html.md` | `cloud/example-dam:9645` | ✅ PASS | -| A4 | cloud/bynder | approval workflow + collections | `…/Asset-Workflow/…Asset-Workflow-Assets.md` + `…Asset-Workflow.md` | `cloud/bynder:1458` | ✅ PASS | -| A5 | cloud/digizuite | renditions + publish | `…/LegacyService/POST/api/renditions/_assetId_.md` | `cloud/digizuite:491` | ✅ PASS | -| B1 | cloud/bynder | PIM entity model (neg) | off-topic (Product-Feedback, AI-Agents) | — | ✅ PASS (clean neg) | -| B2 | cloud/example-dam | PIM entity model (neg) | off-topic (`system_types_reference`, DAM `RecordLink`) | — | ✅ PASS (clean neg) | -| B3 | cloud/inriver | DAM rendition/derivative engine (neg) | off-topic (release notes / product announcements) | — | ✅ PASS (clean neg) | -| B4 | cloud/akeneo | example-dam MO budget/financials (neg) | off-topic (Google-Shopping insights, Studio analytics) | — | ✅ PASS (clean neg) | -| C1 | cloud/example-dam | asset metadata fields | `…/Asset_Studio_Help/MetadataTemplates.htm.md` | `cloud/example-dam:5874` | ✅ PASS | -| C2 | cloud/bynder | asset metadata fields | `…/Upload/…Understanding-And-Using-Metadata.md` | `cloud/bynder:1116` | ✅ PASS | -| C3 | cloud/digizuite | asset metadata fields | `…/GET/api/metafield/asset-info.md` + `…/POST/api/metadata/editor.md` | `cloud/digizuite:1008` | ✅ PASS | -| C4 | group=docs | asset metadata fields | fused: digizuite + inriver + custom-kb + bynder + example-dam + akeneo | mixed, each correctly attributed | ✅ PASS (RRF fusion + attribution) | -| D | cloud/inriver | `get_chunk("cloud/inriver:1004")` | returned full "What is an entity?" body, **no `ambiguous_chunk_id`** | `cloud/inriver:1004` | ✅ **PASS (gating)** | -| E1–E3 | web-guard | — | — | — | ⬜ not run this pass | - -### Verdict — Run 1 - -- **A (recall): 5/5 PASS** — semantic search finds the right doc in the owning product even when the query wording differs from the docs. -- **B (isolation): 4/4 clean negatives** — no cross-domain contamination. (B3/B4 were sharpened to product-unique concepts after the first draft's generic probes matched the PIMs' own adjacent features — see the 📌 note; that was test-design, not a product bug.) -- **C (overlap/fusion): 4/4 PASS** — per-product answers are product-specific, and `group=docs` fuses all six peers with correct `source`/`chunk_ref` attribution. -- **D (Stage-A gating): PASS** — namespaced `chunk_ref` round-trips; the original `ambiguous_chunk_id` bug is fixed on the live binary. - -**Overall: PASS.** The remote-mount semantic search behaves as designed; the only -follow-up is refining the true-negative probes (B3/B4) to product-unique concepts. - -**Overall PASS criterion (for re-runs) =** all A PASS (recall) **and** B shows no -on-topic cross-domain hit (isolation) **and** D PASS on the Stage-A binary (round-trip). -C and E are supporting evidence. diff --git a/docs/federated-silent-poll/worklog.md b/docs/federated-silent-poll/worklog.md deleted file mode 100644 index 49be0788..00000000 --- a/docs/federated-silent-poll/worklog.md +++ /dev/null @@ -1,187 +0,0 @@ -# Worklog — federated peer woken with no query behind it - -| | | -|---|---| -| **Branch** | `fix/federated-silent-poll-diagnosis` | -| **Base SHA** | `55fa36b` (🐛 fix: log keep-warm pings + warn when target isn't self) | -| **Scope** | Stop a scale-to-zero federated cloud peer being woken, and kept warm, with no federated query behind it. Local repo polling must stay untouched. | -| **Status** | **Shipped and verified in production.** Merged to `develop` via PR #192 (`b6cb48f`); deployed to the cloud peer as revision `codesearch-serve--0000024`. | -| **Latest test result** | CI green on PR #192 (test-linux, test-windows, csharp-integration-tests, CodeQL, Analyze). Locally: `cargo test --lib --bins` → **1134 passed, 42 ignored**; `cargo fmt --check` and `cargo clippy --all-targets -- -D warnings` clean | - -## The requirement - -Background polling of **local** repos is fine. Background polling of a -**federated peer** must never happen — an explicit design constraint from the -original federation design, restated by the user as: - -> "hij mag die repos pollen LOKAAL maar niet federated !!! dat had ik nochthans -> in de specs effectief gezegd bij het ontwerp" - -A peer staying warm for an hour *after real use* is **correct** and was -explicitly confirmed as such. The defect was the peer being **woken** with no -query, and then staying warm off that spurious wake. - -## Stage 0 — diagnosis (no commit) - -The pre-existing `DIAGNOSE_FEDERATED_KEEP_WARM.md` blamed a misconfigured local -`CODESEARCH_KEEP_WARM_URL`. **Disproven** on four independent grounds: the env -var is set nowhere locally (process env, HKCU, HKLM, every shell profile); the -one-time `🔥 keep-warm enabled` line appears in zero logs from 2026-04-26 on; -that absence is meaningful because `init_serve_logger` is always file-only in -serve mode and those logs do carry other INFO lines; and no local process held -a `:443` connection. `Watch-CodesearchServeReplicas.ps1` was also eliminated -(polls `/status` every 20s, but last ran 2026-07-05). - -Azure Log Analytics ground truth, over a window with **zero** federated -searches: wakes **120 / 121 / 120 minutes** apart, each warm period **~67 min**, -nightly sleeps exactly 2h00m30s apart → **≈13.4h warm/day, ~56% duty cycle**. -The 120-minute spacing is the tell: it is the **local** host's 2h default, not -any value configured on the peer. - -Two defects, one triggering and one amplifying: - -1. **Trigger** — the TUI's `spawn_remote_discovery` used - `state.idle_suspend_secs()` as a baseline poll interval and ran a `JoinSet` - `/status` fan-out to every peer. The poll *itself* was the ingress traffic. -2. **Amplifier** — keep-warm's `most_recent_tool_call().unwrap_or(start)`. - `/status` and `/healthz` never call `record_tool_call`, so any non-tool-call - wake self-warmed for the full idle window. Unreachable in the case it was - written for, so its only practical effect was rewarding spurious wakes - (~11×). - -## Stage 1 — remove the federated timer poll - -- **Commit:** `6f1d1c5` · **Review:** ⚠️ PASS WITH REMARKS (round 1) → - fixes amended → **PASS, zero code defects** (round 2, cap reached). -- `spawn_remote_discovery` no longer polls on any cadence; the tick is - **config-only** (`REMOTE_ROW_REFRESH_SECS` = 5s, zero HTTP) so mount/unmount - edits and `l` reloads still surface. Contact is activity-poke (single peer, - never a fan-out) or the `i` keypress only. -- Removed `ServeState::idle_suspend_secs` (field, env init, getter, - `--idle-suspend-secs` override) — write-only once the cadence went. - Keep-warm resolves flag > env > default itself, so the flag still works. -- Round-1 fixes amended in: `tui_common.rs` `activity_stale` doc; and the - snapshot emit, previously gated on a non-empty peer list, which left rows on - screen forever after the last peer was removed. - -**Files:** `src/constants.rs`, `src/serve/mod.rs`, `src/serve/tui.rs`, -`src/serve/tui_common.rs` - -## Stage 2 — keep-warm requires a real tool call - -- **Commit:** `12edcf2` · **Review:** ✅ **PASS, zero findings.** -- The `unwrap_or(start)` fallback is gone; with no recorded tool call the loop - does not ping and lets the host suspend the replica. -- Reviewer independently confirmed warm-after-real-use does **not** regress: an - inbound federated search forces `project=`, reaching - `record_tool_call`; and `last_tool_call` is **insert-only** (no - `remove`/`clear`/`retain`, untouched by repo idle-eviction), so once one real - query lands the old behaviour holds for the process lifetime. -- Also fixed the `55fa36b` startup warning, which false-positived on the only - correct deployment: Azure binds `0.0.0.0` while the target is the ingress - FQDN. Wildcard bind ⇒ external host unknown ⇒ stay silent. Rule extracted to - the testable `keep_warm_foreign_target` helper (5 new tests). - -**Files:** `src/serve/mod.rs`, `src/serve/tests.rs` - -## Stage 3 — documentation - -- **Commit:** `3bcf153` · **Review:** ✅ **PASS** (final full-branch review, - `55fa36b...3bcf153`) — "Nothing further is owed on this branch." -- `DIAGNOSE_FEDERATED_KEEP_WARM.md` rewritten (moved from `docs/`): confirmed - root cause, ground truth, the violated requirement, and the **rejected - reasoning** recorded so it is not re-litigated a fourth time. -- `AGENTS.md` bullet rewritten as an explicit design constraint; it had - asserted the removed cadence as current and named a deleted field — the very - mechanism by which this behaviour was re-introduced twice. -- `CHANGELOG.md`: fix entry under `[1.2.4] (unreleased)`. An earlier draft had - put it under `[1.2.0]` — a **real tag** that shipped #181/#184; the reviewer - caught this, and both original entries were restored **verbatim** (confirmed - by diff) and marked superseded. -- `README.md`: grep-guard bullet corrected to the `/healthz` liveness probe. - -**Files:** `AGENTS.md`, `CHANGELOG.md`, `README.md`, -`DIAGNOSE_FEDERATED_KEEP_WARM.md` *(new)*, `docs/diagnose-federated-keep-warm.md` *(deleted)* - -## Stage 4 — the branch had no CI at all - -- **Commit:** `4add0d1` · **Review:** covered by the final full-branch review. -- While preparing the PR it turned out `ci.yml`'s push trigger is a **prefix - allowlist** that did not include `fix/**`. Every `fix/...` branch — the - repo's own documented naming convention — had therefore merged into - `develop` without ever running fmt, clippy or a single test. The PR still - showed green because CodeQL is a separate `pull_request`-triggered workflow - and was the only check present. -- Added `fix/**`, plus a comment explaining the footgun and how to verify - (`gh pr checks ` must list the CI jobs, not just CodeQL). -- Proven by self-test: `08276de` → CodeQL only; `4add0d1` → CI + CodeQL. -- `chore/**` was added later, in PR #193. - -**Files:** `.github/workflows/ci.yml` - -## Stage 5 — deployment and production verification - -- **Merged:** PR #192 → `develop` (`b6cb48f`), auto-bumped to 1.2.5. -- **Local instance:** deployed by the user via `copy-to-common`. This carries - Defect 1 (the TUI timer poll), which only ever ran on the *local* side — so - the trigger was removed first. -- **Cloud peer:** image `codesearch-serve:8d7261e6d` built from a clean - `git archive` of `develop` and deployed as revision - `codesearch-serve--0000024`. Config verified intact across the update: 12 - env vars, 4 secretRefs, `CODESEARCH_IDLE_SUSPEND_SECS=1800`. `/healthz` - returned 200 in 147 ms. Registry size 160,647,888 B vs 160,629,714 B for the - previous image — an 18 KB difference, so no size regression. -- **Idle window** was separately reduced 3600 → 1800 s at the user's request, - halving the cost of any wake that does still occur. -- **Verified:** the replica scaled to 0 about 10 minutes after deploy and - **stayed at 0 across six consecutive one-minute checks with no traffic**. - This is positive evidence rather than mere absence of symptoms: under the old - binary `most_recent_tool_call()` would have been `None`, fallen back to the - process start time, and self-pinged every 120 s for the full 30-minute idle - window — reaching 0 at 10 minutes was not possible. - -**Build note:** two `az acr build` runs failed at the identical step -(`COPY --from=builder /models.tar.gz`) with -`failed to export image: ... layer does not exist`. Layer digests differed -between runs, so it was not a poisoned cache; the Dockerfile is byte-identical -to the one that built the previously deployed image. Root cause sits in the ACR -Tasks build agent (registry is Basic SKU), not in this repo. A local -`docker build` + `docker push` succeeded first time and was used instead. - -## Why this took three attempts across three PRs - -PR #181 introduced the cadence on the reasoning that polling no faster than the -host's suspend term is harmless; PR #184 named "waking the scale-to-zero cloud -peer for no real reason" as the defect and then explicitly sanctioned that same -cadence. The flaw: **not keeping a peer awake past its suspend term is strictly -weaker than not waking it**, and the two windows were unrelated values anyway -(local host vs. remote peer). Both PRs correctly said "local repos unaffected", -confirming the local/federated split was real — but honoured it in only one -direction. - -## Open follow-ups - -- **Residual, known and not currently exploitable:** the MCP `status` **tool**, - when project-scoped, *does* record a tool call (`allow_unscoped=true` reduces - the guard to `!is_multi`), so an automated poller of that tool would still buy - a warm window. No such poller exists — both `Watch-CodesearchServeReplicas.ps1` - and `FederationClient::list_repos` use the HTTP `/status` endpoint, which does - not record. First place to look if the symptom recurs. -- **Verified in production** (see Stage 5) over a ~6-minute window. Worth - re-running the original Log Analytics query over a **full day** to confirm the - duty cycle: was ≈13.4 h warm/day (~56%) at zero searches, expected now ≈0 with - wakes only behind real federated queries. The short window proves the - self-ping is gone; only a 24 h sample proves nothing else wakes it. -- **ACR Tasks cannot currently build this image** (Stage 5 build note). The - local `docker build` path works, but a CI/automated deploy would hit the same - failure. Worth a look before anyone automates the cloud deploy. - -## Security note - -None. No auth, network-exposure or data-handling surface changed; the branch -strictly *reduces* outbound traffic. - - diff --git a/docs/watcher-reindex-tui-visibility/worklog.md b/docs/watcher-reindex-tui-visibility/worklog.md deleted file mode 100644 index 61cfb85a..00000000 --- a/docs/watcher-reindex-tui-visibility/worklog.md +++ /dev/null @@ -1,87 +0,0 @@ -# Worklog — watcher reindex / TUI visibility - -- **Branch:** `fix/watcher-reindex-tui-visibility` -- **Base SHA:** `09b451aaa7372285f76d50fad71a035aa40e4fd5` (develop) -- **Scope:** Make watcher-triggered reindexes visible in the serve TUI and rebuild - C#/TypeScript symbols on branch switch. Three user-reported gaps: - A) branch switch never rebuilds symbols (find_impact goes stale); - B) the C# indicator never shows "Indexing" during a watcher rebuild; - C) symbol-rebuild log lines lack a repo label; - plus (gap #1) ordinary text-batch reindexes never signal "Indexing" in the TUI. -- **Status:** ✅ COMPLETE — all 3 stages + DRY refactor committed; every per-stage - review and the final full-branch review PASSED. Not pushed (awaiting user). -- **Latest test result:** `cargo fmt --all --check`, `cargo check --all-targets`, - `cargo clippy --all-targets -- -D warnings` clean; 609 lib tests pass. -- **Final review:** ✅ PASS on full diff `09b451a..78c9310` (holistic signal/label - balance, single-source-of-truth rebuild helper, no stale 2-arg notifier sites). - -## Root cause (from code, not memory) - -| Watcher path | Signalled `indexing_cb` (general TUI "Indexing")? | Set `CSharpIndexStatus::Indexing`? | Rebuilt symbols? | -|---|---|---|---| -| Text batch flush (`process_batch_with_stores`) | ❌ no (gap #1) | n/a | n/a | -| Branch switch | ✅ yes (text refresh only) | ❌ no | ❌ no — discards `.cs/.ts` buffers (gap A) | -| `.cs` debounce | ✅ yes | ❌ no (gap B) | ✅ yes (incremental) | -| `.ts` debounce | ✅ yes | n/a (no TS notifier) | ✅ yes (full) | - -The serve-layer helper `trigger_symbol_rebuild` (src/serve/mod.rs) already sets -`CSharpIndexStatus::Indexing` + `begin_indexing` + Full rebuild, but the watcher -in `IndexManager` cannot reach it — it only holds the two callbacks. - -## Stages - -### Stage 1/3 — Text-batch TUI visibility + repo-label logging (gap #1 + Fix C text paths) -- Commit: `3d43993da4d40d4711a6bb9d443bee3019e21ffe` — review: ✅ PASS (no remarks). -- Wrapped the FSW text-batch flush in `indexing_cb(true/false)` so ordinary file - edits surface as "Indexing" in the TUI (the `IndexingStatusCallback` doc already - claimed it fired on "batch flushes"; it never did). -- Added a `repo_label` (repo directory name = serve alias) to the watcher task and - interpolated it into batch-flush and branch-change log lines. -- Files: `src/index/manager.rs`. - -### Stage 2/3 — C# indicator shows "Indexing" during watcher rebuild (Fix B) -- Commit: `2dbafa3d0b8cd4f6996267b40c2c6806cb769121` — review: ✅ PASS (no remarks). -- Refactored `CSharpRebuildNotifier` from `Fn(bool, Option)` to a 3-state - `SymbolRebuildSignal { Started, Succeeded, Failed(String) }`. The watcher emits - `Started` right after the applies/available gate, so `make_csharp_notifier` sets - `CSharpIndexStatus::Indexing` for the rebuild duration (was Ready/Error only). -- Guards (`!applies_to`, `!is_available`) return BEFORE `Started`, so the indicator - is never left stuck on `Indexing`. -- Also labelled every C# rebuild log line with `repo_label`; refreshed two stale - callback doc comments. -- Files: `src/index/manager.rs` (new file: no), `src/index/mod.rs`, `src/serve/mod.rs`. - -### Stage 3/3 — Branch-switch symbol rebuild (Fix A) -- Commits: `928273d6ee0e66f6f66f64fdcc8126a2e063919b` (feature) — - review: ⚠️ PASS WITH REMARKS (1 Important: duplicated full-rebuild block); - `a5f66c819d45bf0c9d49d74293ee299e5f06f9e9` (remark fix) — extracted - `IndexManager::run_full_rebuild_logged`; re-review ⚠️ PASS WITH REMARKS - (one 4th copy left in the `.ts` path); `78c9310aa5f45b452d0929a586a96b7fb8a4b6cc` - (fold-in) — routed the `.ts` debounce rebuild through the same helper → - single source of truth for all four full-rebuild paths. 609 lib tests pass. -- Added `IndexManager::spawn_branch_change_symbol_rebuild(...)`: after the - branch-change text refresh, a fire-and-forget `spawn_blocking` runs a - `RebuildScope::Full` rebuild for every applicable+available language (C# + TS). - Full scope is correct — a branch switch rewrites arbitrary files, so no - incremental scope can be computed. -- Toggles the general `indexing_cb` label around the whole rebuild (only when a - language actually applies → no TUI flash otherwise); C# also drives the - `SymbolRebuildSignal` indicator. -- Files: `src/index/manager.rs`. - -## Follow-ups / notes -- **Deletions-only `.cs` debounce bug (pre-existing, found in Stage 2 review):** - when only `.cs` deletions are buffered (no modifications), the debounce path - builds empty `groups`/`ungrouped`, skips both the fallback and the per-group - loop, and emits `Started`→`Succeeded` WITHOUT running any rebuild — so the - forwarded `cs_deleted` set is never purged from LMDB and deleted symbols - linger. `manager.rs` grouped `.cs` path. Not fixed here (out of scope); a Full - rebuild (or `Files{changed:[], deleted}`) when `groups.is_empty() && !cs_deleted.is_empty()` - would fix it. Branch-switch deletions ARE now handled (Stage 3 Full rebuild). -- TypeScript watcher path updates no TUI symbol status (no TS notifier). Out of - scope this iteration; candidate follow-up. -- Watcher symbol-rebuild paths have no per-repo mutex guard (already a tracked - follow-up); concurrent rebuilds on the same repo are benign (alias-keyed). - Rapid successive branch switches could overlap Full rebuilds — same tradeoff. - - diff --git a/src/serve/tests.rs b/src/serve/tests.rs index 33fdf2f5..62ee5ee0 100644 --- a/src/serve/tests.rs +++ b/src/serve/tests.rs @@ -1661,7 +1661,7 @@ mod allowed_hosts_tests { /// Tests for `extract_host_from_url` — used solely by the keep-warm /// misconfiguration sanity check (a keep-warm target host that doesn't look /// like "self" gets a loud warning; see the diagnosis this shipped with in -/// docs/diagnose-federated-keep-warm.md). +/// `.docs/DIAGNOSE_FEDERATED_KEEP_WARM.md`). mod keep_warm_host_extraction_tests { use super::*; From a31388118975bc579268aa17fead1491b56a252d Mon Sep 17 00:00:00 2001 From: flupkede Date: Tue, 18 Aug 2026 19:45:10 +0200 Subject: [PATCH 2/3] [chore] pre-commit root-md allowlist guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rejects any commit that introduces (adds/copies/renames) a root-level *.md outside the allowlist (AGENTS.md, AGENTS.develop.md, CLAUDE.md, README.md, README_CSharp.md, CHANGELOG.md, RELEASING.md); the message points at .docs/ and AGENTS.md "Root file hygiene (markdown)". Only introductions are checked (diff-filter=ACR); dot-folders are out of scope by construction. .githooks/ README.md documents the guard; CHANGELOG gets a new pending [1.3.1] section (Cargo.toml already builds 1.3.1). Verified by direct hook runs: stray root add blocked (rc=1), rename allowlisted->stray blocked (rc=1), allowlisted modify / nested add / .docs add all pass (rc=0). Review-fixes (from stage 1 review, resolved here): - [Important] AGENTS.md claimed a pre-commit root-md guard that did not exist -> guard now exists in .githooks/pre-commit with matching allowlist; .githooks/README.md table row updated Review-fixes (from stage 2 review round 1, resolved here): - [Important] non-ASCII root md names escaped the guard: core.quotePath (default true) C-quotes them, so the *.md case missed the quoted trailing quote char -> diff now runs with -c core.quotePath=false; verified DIAGNOSE_ü.md now blocked rc=1 [Debt, recorded not fixed] allowlist restated in 3 live sites (pre-commit, .githooks/README.md, AGENTS.develop.md) — documentation-inevitable duplication, stable at 7 files. --- .githooks/README.md | 20 +++++++++++++++++++- .githooks/pre-commit | 44 ++++++++++++++++++++++++++++++++++++++++++-- CHANGELOG.md | 6 ++++++ 3 files changed, 67 insertions(+), 3 deletions(-) diff --git a/.githooks/README.md b/.githooks/README.md index ed16d300..0ac5ed19 100644 --- a/.githooks/README.md +++ b/.githooks/README.md @@ -12,12 +12,30 @@ never seems to fire, check that setting first. | Hook | What it does | |---|---| -| `pre-commit` | Runs `cargo fmt` and stages the result, so CI's fmt check can't fail. | +| `pre-commit` | Runs `cargo fmt` and stages the result, so CI's fmt check can't fail. Then rejects any commit that introduces a root-level `*.md` outside the allowlist (see below). | | `pre-push` | Blocks direct pushes to `master`; runs the QC gate (skipped when the branch changes no Rust); scans tracked files for customer references. | | `post-checkout` | Creates `AGENTS.md` from `AGENTS.develop.md` on branch switch, if absent. | Any hook can be bypassed with `git push --no-verify` / `git commit --no-verify`. +## Root-md allowlist guard (`pre-commit`) + +Agents love dropping `*.md` files (diagnoses, plans, worklogs, test scenarios) +at the repo root. The `pre-commit` hook rejects any commit that **introduces** +(added/copied/renamed) a root-level `*.md` outside this allowlist: + +- `AGENTS.md`, `AGENTS.develop.md` — agent instructions +- `CLAUDE.md` — one-line pointer to `AGENTS.md` +- `README.md`, `README_CSharp.md` — user-facing docs +- `CHANGELOG.md`, `RELEASING.md` — release infrastructure + +Everything else belongs in **`.docs/`** (gitignored, local-only) — see +AGENTS.md, section *Root file hygiene (markdown)*. Only introductions are +checked: modifying an already-tracked stray is only possible after a +deliberate `--no-verify` bypass, where the file itself — not the commit — is +the violation. Dot-folders (`.githooks/`, `.github/`, `.claude/`, …) are out +of scope: a root-level file cannot be inside one. + ## `customer-patterns.local` The `pre-push` leak scan reads its patterns from `.githooks/customer-patterns.local` diff --git a/.githooks/pre-commit b/.githooks/pre-commit index 4add85b1..330a344a 100755 --- a/.githooks/pre-commit +++ b/.githooks/pre-commit @@ -1,7 +1,8 @@ #!/bin/bash -# pre-commit hook: format Rust code only. +# pre-commit hook: format Rust code + root-md allowlist guard. # -# Runs `cargo fmt` and stages any reformatting so CI's fmt-check can't fail. +# 1. Runs `cargo fmt` and stages any reformatting so CI's fmt-check can't fail. +# 2. Rejects commits that introduce a root-level *.md outside the allowlist. # # Installed via `git config core.hooksPath .githooks` — see .githooks/README.md. # @@ -27,4 +28,43 @@ if [ -n "$FMT_CHANGED" ]; then echo "pre-commit: staged rustfmt changes ($FMT_CHANGED)" fi +# --- root-md allowlist guard ------------------------------------------------- +# The repo root keeps only its sanctioned markdown files (AGENTS.md, section +# "Root file hygiene (markdown)"). Anything else — diagnoses, plans, test +# scenarios, worklogs — belongs in .docs/ (gitignored, local-only). Blocking +# here is cheaper than a follow-up cleanup commit after a stray lands on +# develop. +# +# Only introductions are checked (added/copied/renamed). Modifying an already +# tracked stray is only possible after a deliberate --no-verify bypass — the +# file itself is the violation then, and review catches it. Dot-folders +# (.githooks/, .github/, .claude/, ...) are out of scope by construction: a +# root-level file cannot be inside one. +ALLOWED_ROOT_MD="AGENTS.md AGENTS.develop.md CLAUDE.md README.md README_CSharp.md CHANGELOG.md RELEASING.md" + +while IFS= read -r staged; do + # root level = no slash anywhere in the staged (post-rename) path + case "$staged" in + */*) continue ;; + esac + case "${staged,,}" in + *.md) ;; + *) continue ;; + esac + for allowed in $ALLOWED_ROOT_MD; do + [ "$staged" = "$allowed" ] && continue 2 + done + echo "" + echo "pre-commit: BLOCKED — root-level '$staged' is not on the md allowlist." + echo " Root markdown is limited to: $ALLOWED_ROOT_MD" + echo " Diagnoses, plans, worklogs and test scenarios belong in .docs/ (gitignored)." + echo " See AGENTS.md, section \"Root file hygiene (markdown)\"." + echo " Deliberate? Use: git commit --no-verify" + echo "" + exit 1 +# core.quotePath=false: with the default (true), git C-quotes non-ASCII paths +# (DIAGNOSE_ü.md -> "DIAGNOSE_\303\274.md"), and the quoted trailing `"` makes +# the *.md pattern miss — a stray with a non-ASCII name would sail through. +done < <(git -c core.quotePath=false diff --cached --name-only --diff-filter=ACR) + exit 0 diff --git a/CHANGELOG.md b/CHANGELOG.md index 80b6ed42..cfbf1473 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,12 @@ more PRs land; when the release is actually tagged, the same section is finalized in place with a date — no renaming/migration step needed. --> +## [1.3.1] + +### Added + +- **`pre-commit` hook now enforces the root-md allowlist** (`.githooks/`): a commit that introduces (adds/copies/renames) a root-level `*.md` outside `AGENTS.md`, `AGENTS.develop.md`, `CLAUDE.md`, `README.md`, `README_CSharp.md`, `CHANGELOG.md`, `RELEASING.md` is rejected with a pointer to `.docs/`. Shipped together with the cleanup itself: the stray root mds (`DIAGNOSE_*`, `PLAN_*`, `TEST-SCENARIO-*`) and the tracked `docs/` folder were dissolved into the gitignored `.docs/` folder, and the rule is documented in AGENTS.md, section *Root file hygiene (markdown)*. Requires `git config core.hooksPath .githooks` (already set in existing clones; fresh clones see `.githooks/README.md`). + ## [1.3.0] - 2026-08-15 ### Added From 107d93f440c18ad12afcc167f41a6cfd12939f75 Mon Sep 17 00:00:00 2001 From: flupkede Date: Tue, 18 Aug 2026 20:06:18 +0200 Subject: [PATCH 3/3] [worker] final review: bash 3.2 compatibility for root-md guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the ${var,,} lowercase expansion (bash >= 4 only; fatal "bad substitution" on stock macOS bash 3.2, which would block every commit containing a root-level file) with tr 'A-Z' 'a-z'. Verified: EVIL.MD and DIAGNOSE_ü.md still blocked rc=1, allowlisted modify rc=0, clean tree rc=0. Review-fixes: - [Important] guard used ${staged,,}, unsupported on macOS bash 3.2 -> tr-based lowering, behavior unchanged - [Debt, corrected count] allowlist restated in 5 live sites (pre-commit, .githooks/README.md, AGENTS.md, AGENTS.develop.md, CHANGELOG.md), not 3 as previously recorded --- .githooks/pre-commit | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.githooks/pre-commit b/.githooks/pre-commit index 330a344a..d6416a49 100755 --- a/.githooks/pre-commit +++ b/.githooks/pre-commit @@ -47,7 +47,10 @@ while IFS= read -r staged; do case "$staged" in */*) continue ;; esac - case "${staged,,}" in + # tr, not ${var,,}: the parameter-expansion lowercase needs bash >= 4 and + # stock macOS still ships bash 3.2, where it is a fatal "bad substitution" + # that would block EVERY commit containing a root-level file. + case "$(printf '%s' "$staged" | tr 'A-Z' 'a-z')" in *.md) ;; *) continue ;; esac