diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 8d15f48..e7b0214 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -113,6 +113,8 @@ not merely age the downstream numbers, it can invert their conclusion. - Use the PR template (`.github/pull_request_template.md`). - **Merge strategy:** Merge commits only. No squash, no rebase. Branch history is preserved. - Manually verify the MCP server starts cleanly and `reindex` runs without error before requesting review. + "`reindex` runs without error" means against a scratch index or a test target, **never the live + one**: MCP Inspector's fixed 60 s request timeout kills any full rebuild partway (ADR-049, B-057). - Update the ADR Implementation Log before merging (Major changes only). ## 6. Commit Messages diff --git a/docs/adr/ADR-049-safe-reindex-and-tool-output.md b/docs/adr/ADR-049-safe-reindex-and-tool-output.md new file mode 100644 index 0000000..93bbf55 --- /dev/null +++ b/docs/adr/ADR-049-safe-reindex-and-tool-output.md @@ -0,0 +1,284 @@ +# ADR-049: A Safe Reindex, Read-Only Hints, Redacted Output and a Cheaper Tool Surface + +**Status:** accepted +**Date:** 2026-10-20 +**Branch:** `feature/adr-049-safe-reindex-and-tool-output` +**Reviewer:** @edb +**Backlog:** [B-057](../backlog.md#b-057) (main), [B-045](../backlog.md#b-045), +[B-018](../backlog.md#b-018) (the output half), [B-019](../backlog.md#b-019), +[B-021](../backlog.md#b-021) (point 3 only) +**Depends on:** none +**Depended on by:** none yet. ADR-050 (symbols, built in parallel) gets `readOnlyHint` and redaction +on its new tools through `_tool` with no change of its own; @edb adds its tool names to the routing +line after both merge. + +## Context + +**B-057, the 2026-09-29 incident.** An agent called `reindex` through MCP Inspector twice with no +arguments. `changed_files_only` defaulted to `False`, so each call was a full rebuild, and Inspector's +fixed 60 s timeout killed the server partway through both: + +1. Run 1: `_snapshot_index` copied the good index to `.pre-full-reindex/`, the index was wiped, and + the rebuild was killed. +2. Run 2: `_snapshot_index` began with `shutil.rmtree(backup_dir)` ("left by a killed earlier run"), + which deleted the only good copy, then snapshotted run 1's half-built index and was killed too. + +Two independent defects: the backup is deleted on the assumption that it is stale, and a bare call is +the destructive one. + +**B-045.** No tool declares MCP tool annotations. Every tool but `reindex` only reads, and a client +that sees `readOnlyHint` can auto-approve them. + +**B-018.** Tools return raw chunk text. A credential committed to an indexed repo (a service-account +JSON, a token in a config file) goes straight into the agent's context and its transcript. There is +no redaction anywhere in `src/`. + +**B-019.** Every session with the server connected pays for the tool listing and the server +instructions before its first call. Nobody has measured how much that is. + +**B-021 point 3.** The server instructions say when the index is stale but never when to prefer it +over Grep, and they are the one text every connected session already receives. + +## Decision + +### 1. A backup on disk is the good copy (B-057, defect 1) + +- **`_snapshot_index` never deletes `.pre-full-reindex/`.** If it exists when a full rebuild starts, + the run that made it never finished, so the live index is suspect and the backup is the copy that + run was protecting. The new run **restores it first**, then reuses it as its own backup (the live + index now equals it, so there is nothing new to copy). +- **A snapshot becomes the backup only once it is complete.** It is copied into + `.pre-full-reindex.partial/` and renamed to `.pre-full-reindex/` in one `os.replace`. The wipe comes + after the rename. So a `.partial/` left behind was killed while copying, before anything was wiped: + the live index is still good, and the partial copy is the one thing that is safe to delete. +- **`_restore_index` copies; it never moves files out of the backup or deletes it.** The old one + moved the `.faiss` files with `os.replace`, so a kill mid-restore left a backup missing files, and + the next restore would have deleted the live copies of the ones already moved. +- **The backup is removed in exactly two places, both after the live index is known good:** when the + rebuild has finished and saved (`run_incremental` returned), and when a failed rebuild's restore has + finished. A kill at any point before either leaves it in place for the next full rebuild. + +### 2. A full rebuild has to be asked for (B-057, defect 2) + +`reindex(changed_files_only: bool = True)`. The parameter keeps its name, so a saved call that passes +it means what it meant before; only the bare call changes, from a full rebuild to an incremental one. +The docstring now leads with the full rebuild being slow (tens of minutes) and destructive, and says +never to run one against an index you need from a client with a request timeout. Every caller in the +repo (the TUI, the tests) already passed the argument; the TUI's fallback when it can't read its own +field is now incremental too. + +### 3. CONTRIBUTING §5 + +"`reindex` runs without error" now says: on a scratch index or a test target, never the live one, +because Inspector's 60 s timeout kills any full rebuild. + +### 4. Tool annotations derived in `_tool` (B-045) + +`_tool` passes `ToolAnnotations` to FastMCP for every tool it registers. `readOnlyHint` is +`reads_index`, so every read tool, including ones other branches are adding now, gets +`readOnlyHint: true` without its author doing anything. A writer declares the rest at its decorator: +`reindex` is `@_tool(reads_index=False, destructiveHint=True, idempotentHint=False)`. + +- `destructiveHint=True`: a full rebuild wipes the index first. +- `idempotentHint=False`, deliberately. An incremental run is close to idempotent, but a client may + retry an idempotent call that timed out, and a retried full rebuild after a timeout is the incident. +- `openWorldHint` is left unset: it would add bytes to every tool's listing for no client behavior we + rely on. + +`mcp` is pinned `>=1.28,<2`. `FastMCP.tool(annotations=...)` and `mcp.types.ToolAnnotations` exist +throughout that range. + +### 5. Redaction at the MCP boundary (B-018, output half) + +`_tool`'s async wrapper runs every tool's returned text through `redact.redact()` before FastMCP sees +it. The plain module functions, which the TUI and the tests call, are unchanged, and the index stays +faithful. `src/redact.py` holds the patterns and replaces each match with `[REDACTED:]`: + +| Kind | Matches | +|---|---| +| `private-key-field` | the string value of a `"private_key"` / `'private_key'` key (service-account JSON, Python dicts) | +| `private-key` | a PEM `-----BEGIN … PRIVATE KEY-----` block through its END line, or to the end of the text when a chunk cuts it off | +| `github-token` | `ghp_` / `gho_` / `ghs_` / `ghu_` / `ghr_` + 36, `github_pat_` + 50 or more | +| `slack-token` | `xox[abprs]-` + 10 or more | +| `aws-access-key` | `AKIA` / `ASIA` + 16 uppercase letters or digits | +| `google-api-key` | `AIza` + 35 | +| `anthropic-key` | `sk-ant-` + 20 or more | +| `openai-key` | `sk-proj-` / `sk-svcacct-` / `sk-admin-` + 20 or more, or `sk-` + 20 or more letters and digits with no hyphen | +| `stripe-key` | `sk_live_` / `sk_test_` / `rk_live_` / `rk_test_` + 24 or more, `whsec_` + 32 or more | +| `aws-secret-key` | 40 characters of base64 after a `secret_access_key` / `SecretAccessKey` name (value only) | +| `url-credential` | the password in `scheme://user:password@` (value only) | +| `connection-string-password` | `Password=…;` / `Pwd=…;` in a connection string (value only) | +| `secret-assignment` | a quoted literal of 8 or more non-space characters assigned to, or keyed by, a name containing `password`, `passwd`, `secret`, `api_key`, `apikey`, `access_token` or `auth_token` (value only) | + +False positives are kept low by requiring each prefix's real length and alphabet and a word boundary +in front: `sk_` names, `sk-` kebab-case class names, short hex, and `AKIA` in prose pass through. +The last four kinds have no prefix of their own, so they key on the name and replace only the value, +leaving the name readable. They accept false positives by decision (@edb, 2026-10-20): a long literal +assigned to `secret_name` is redacted too. A value with a space (a label or prompt), a call, an empty +or short string, a bare 40-hex SHA, and a value an earlier pattern already replaced pass through. Over +this repo's own `src/`, `tools/`, `docs/` and toml, the only line they change is the comment in +`redact.py` that describes `Password=…;`. +Not matching every token format is accepted: this catches the families that leak in practice, and +the scan-policy half of B-018 (not indexing `.env*` and key files at all) is another branch's. + +### 6. The routing line (B-021 point 3) + +Both branches of `_server_instructions` gain one sentence: for how code works, where a symbol is used, +what writes a field, or blast radius, call `semantic_code_search`, `trace_data_flow`, `what_writes` or +`analyze_blast_radius` before Grep; use Grep for exact strings and for files your branch changed. + +### 7. Measure the tool surface, trim only the bloated (B-019) + +Measured with Inspector's `tools/list` (table in the Implementation Log). Three changes, smallest +risk first: + +- **`_tool` sends each docstring dedented** (`inspect.cleandoc`). FastMCP sends `__doc__` as is, so + every line carried four spaces of indentation: 634 characters across 14 tools, with no content. +- **`_tool` registers with `structured_output=False`.** For a `-> str` tool FastMCP generates an + `outputSchema` of `{"result": string}` (about 150 characters per tool) and returns the text twice, + once as content and once as `structuredContent`. The schema tells an agent nothing. +- **Three descriptions are trimmed:** `what_writes` (a support-call narrative), `investigate_architecture` + (the retrieval pipeline's internals, which no caller acts on) and `index_status` (implementation + detail). Each keeps its when-to-call and when-not-to-call lines. The rest are left alone: none is + bloated once dedented, and `trace_data_flow` and `map_module_communities` belong to ADR-053. + +## Consequences + +**Better:** +- No sequence of killed full rebuilds can lose the last good index: the backup outlives every kill + and is removed only once the live index is good again. +- A bare `reindex()` costs seconds and destroys nothing. +- Clients can auto-approve every read tool, and every future one gets the hint for free. +- A committed credential no longer reaches an agent's context through this server. + +**Worse:** +- A full rebuild that starts on a leftover backup first copies it back, which is more disk I/O + before the wipe. Restoring now copies the `.faiss` files instead of moving them. +- If an incremental run finished the half-built index after a kill, the leftover backup is now + older than the live index. The next full rebuild restores it before starting, and if that rebuild + is killed too, the fallback is the older, but finished, index. An incremental catches it up. +- A backup written by the old code was not renamed into place, so one left from a kill *during* its + snapshot can be partial. That window existed only before this ADR, and only during the copy. +- A redacted chunk no longer shows its literal. That is the point, but a test fixture that looks + like a key will read as `[REDACTED:…]` in tool output. + +- Tool results no longer carry `structuredContent`. A client that read the `result` field instead of + the text content would have to switch. None of ours does (Claude Code reads the content). + +**Neutral:** +- A client that called `reindex()` bare expecting a full rebuild now gets an incremental one, and + its result says "Incremental". +- Error text from a tool that raises is not redacted: it carries paths and messages, not chunk text. +- The routing line makes the instructions 215 characters longer, and the listing is still 3,392 + characters shorter than before (Implementation Log). + +## Open Questions for @edb + +1. ~~**`structured_output=False`.** It is a one-argument revert in `_tool` if any client turns out to + read `structuredContent`. Keep it?~~ **Answered 2026-10-20 (@edb): keep it false.** +2. ~~**Other secret families.** AWS *secret* keys (40 characters of base64 with no prefix), Stripe + `sk_live_`, and generic `password = "…"` assignments are not matched: each has no prefix that + separates it from ordinary code, or (for Stripe) collides with the `sk_` names this ADR keeps + untouched. Add any of them, accepting the false positives?~~ **Answered 2026-10-20 (@edb): add + them.** Built as the last five rows of §5's table. +3. **An incremental run that completes a half-built index** leaves the old backup in place (only a + full rebuild removes it). Should a finished incremental run, which means the watchdog's, also + remove it? That code is in `_ReindexDebouncer`, outside this ADR's functions. +4. **The routing line** names four tools. Add `get_file_outline` / `get_symbol_source` after ADR-050 + merges, as planned. + +## Alternatives Considered + +| Option | Why rejected | +|--------|-------------| +| Refuse a full rebuild while a backup exists | Safe, but it leaves a wiped live index serving until someone restores by hand. Restoring is what the human would do anyway. | +| Keep the leftover backup and skip the restore | Same end state for the rebuild, but the live index stays half-built for the length of the new run, and the "live equals backup at start" invariant is lost. | +| Rename the parameter to `full: bool = False` | Clearer, but every saved call that passes `changed_files_only` would break against the schema. | +| Redact at index time | The index would no longer be faithful, and the TUI's chunk browser and tests would see altered text. Redacting where text leaves the process covers every tool at one point. | +| Hand-written list of read-only tools | Goes stale the moment another branch adds a tool. | +| `idempotentHint=True` on `reindex` | Invites a client's automatic retry of a timed-out full rebuild. | + +## Implementation Log + +- [x] B-057: `_snapshot_index` restores a leftover backup; atomic snapshot; copying restore; backup + removed only after a finished save or a finished restore +- [x] B-057: `reindex` defaults to incremental; docstring; TUI fallback +- [x] B-057: CONTRIBUTING §5 line +- [x] B-045: annotations derived in `_tool` +- [x] B-018: `src/redact.py` and the pass in `_tool`'s wrapper +- [x] B-021: routing line in both instruction branches; length before and after +- [x] B-019: `tools/list` measured; table; trims; totals before and after +- [x] Tests, gates, Inspector +- [x] Open question 2: Stripe, AWS secret key, URL, connection-string and assignment patterns +- [ ] ADR-050's tool names in the routing line, after both merge (@edb) + +**Stack for every measurement below:** Linux cloud container, Python 3.12.3, `mcp` 1.30.0, MCP +Inspector 2.8.0, CPU only, no embedder or model downloaded (Hugging Face unreachable). Before is +`871b55c`; after is this branch. + +**B-019: the tool listing.** Characters of compact JSON from `tools/list --strict --format json`, +run from an empty directory. "Whole entry" is the tool's full listing object (name, description, +input and output schemas, annotations). + +| Tool | Description before | after | Input schema before | after | Annotations after | Whole entry before | after | +|---|---:|---:|---:|---:|---:|---:|---:| +| `what_writes` | 1,564 | 961 | 199 | 199 | 21 | 1,992 | 1,272 | +| `map_module_communities` | 1,222 | 1,157 | 299 | 299 | 21 | 1,759 | 1,576 | +| `investigate_architecture` | 1,071 | 687 | 223 | 223 | 21 | 1,535 | 1,025 | +| `index_status` | 1,122 | 741 | 177 | 177 | 21 | 1,529 | 1,031 | +| `find_unabstracted_collection_reads` | 906 | 848 | 277 | 277 | 21 | 1,444 | 1,255 | +| `verify_candidate_edges` | 894 | 840 | 207 | 207 | 21 | 1,336 | 1,163 | +| `reindex` | 859 | 800 | 144 | 143 | 68 | 1,207 | 1,090 | +| `find_test_coverage` | 724 | 674 | 222 | 222 | 21 | 1,171 | 1,006 | +| `find_dead_code` | 725 | 679 | 200 | 200 | 21 | 1,143 | 986 | +| `semantic_code_search` | 561 | 527 | 135 | 135 | 21 | 922 | 771 | +| `detect_pattern_violations` | 249 | 231 | 340 | 340 | 21 | 817 | 677 | +| `analyze_blast_radius` | 287 | 265 | 227 | 227 | 21 | 733 | 594 | +| `find_similar_code` | 203 | 189 | 153 | 153 | 21 | 567 | 439 | +| `trace_data_flow` | 91 | 81 | 154 | 154 | 21 | 451 | 329 | +| **Total** | **10,478** | **8,680** | **2,957** | **2,956** | **341** | **16,606** | **13,214** | + +Where the 3,392 characters (−20%) went: 634 of indentation, about 2,100 of output schemas, 1,151 +from the three trimmed descriptions, less the 341 of annotations added. The input schemas are +unchanged (`reindex` is one character shorter: `true` for `false`). The largest description is +`map_module_communities` at 1,157, which is ADR-053's to trim; no description comes near the +2,892 characters found on segmem_mcp. + +**B-021: the server instructions.** Rendered for a folder index and a git-mode index, each with a +recorded commit: folder mode **600 → 815** characters, git mode **570 → 785**. The routing sentence +replaces "Use the index for how the rest of the codebase works", which it says more precisely. + +**Notes:** +- 2026-10-20: **"Byte-identical" needs one qualification for `graph.db`.** The restore uses SQLite's + backup API (as `_snapshot_index` always has, to read a consistent snapshot through the WAL), and + the backup API bumps three 4-byte counters in the database header of the file it writes: the + file change counter (offset 24), the schema cookie (40) and version-valid-for (92). The replay + test compares the `.faiss` files byte for byte and `graph.db` byte for byte with those twelve + header bytes masked, every page included. A raw file copy would match exactly but could miss + pages still in the WAL. +- 2026-10-20: **How the tests simulate a kill.** A `Killed(BaseException)` raised from the embedder + inside a real `run_incremental` (stubbed embedder, character-level tokenizer, summarizer off, a + one-text embed window so part of the repo is written first). `_reindex` catches `BaseException` + and restores, which a killed process never gets to do, so the fixture makes that handler's + restore die too. The replay fails on `871b55c` (7 of the 9 new reindex tests) and passes here. +- 2026-10-20: the commit-msg hook is not executable in a fresh clone (`git` ignores it), so the + `[ADR-049 …]` tags on this branch's commits were added by hand in the hook's format. +- 2026-10-20: `pyproject.toml`'s `py-modules` list does not include `redact` (nor `config`, + `index_lock`, `scan_policy` and nine other modules already in `src/`). The editable install + and the tests don't need it; a non-editable install of every one of those is already broken the + same way. Not changed here: not this ADR's file. +- 2026-10-20: **Tests.** New: `tests/test_safety_reindex.py` (9), `tests/test_safety_redact.py` (32), + `tests/test_safety_tool_surface.py` (9). In this container the suite is 654 passed, 48 failed, + 2 skipped on `871b55c`, and 704 passed, 48 failed, 2 skipped here: the same 48 fail on both, all + because the embedder's tokenizer cannot be fetched from Hugging Face here. flake8 on `src` is + clean; `conformance_eval`, `resolution_eval` and `l5x_fixture_teeth` pass on both. +- 2026-10-20: **MCP Inspector 2.8.0**, from an empty `/tmp/insp`, never calling `reindex`: + - `tools/list --strict`: exit 0. Every tool but `reindex` declares `readOnlyHint: true`; + `reindex` declares `readOnlyHint: false, destructiveHint: true, idempotentHint: false`; no tool + has an `outputSchema`. + - `index_status since=1d`: exit 0, reports no recorded build. `index_status since=bogus`: exit 5, + `isError: true`, names the accepted forms. + - `semantic_code_search` with no `query`: exit 5, `isError: true` (validation error). + With `query=parser`: exit 5, `isError: true`, because no embedder can load here. Its success + path, with redaction applied, is covered in pytest + (`test_tool_output_is_redacted_at_the_mcp_boundary`). diff --git a/src/MCPServer.py b/src/MCPServer.py index b0aeb0b..9a8294c 100644 --- a/src/MCPServer.py +++ b/src/MCPServer.py @@ -63,6 +63,11 @@ def _server_instructions(repo_path: str | None = None) -> str: file=sys.stderr, flush=True) built = (f"commit {commit[:10]}, last verified {verified}" if commit else "no finished build yet") + # ADR-049 (B-021 point 3): when to reach for the index instead of Grep. + routing = ("For how the rest of the codebase works, where a symbol is used, what writes a " + "field, or blast radius, call `semantic_code_search`, `trace_data_flow`, " + "`what_writes` or `analyze_blast_radius` before Grep; use Grep for exact " + "strings and for the files your branch changed. ") if ref is not None: # ADR-042 §5: the index is a commit, read from git, not any folder. from index_location import ref_display @@ -71,8 +76,8 @@ def _server_instructions(repo_path: str | None = None) -> str: "folder's working tree, and not your branch. For any file your branch changed " f"(git diff --name-only {commit[:10] if commit else ''}...HEAD, plus " "uncommitted edits), Read the file itself: index hits for it show the indexed " - "version, and line numbers may be off. Use the index for how the rest of the " - f"codebase works. It follows {ref} on its own when that ref moves; do not call " + f"version, and line numbers may be off. {routing}" + f"It follows {ref} on its own when that ref moves; do not call " "`reindex` to pick up branch work. `index_status` reports freshness." ) return ( @@ -80,7 +85,7 @@ def _server_instructions(repo_path: str | None = None) -> str: "It does not see edits in other branches or worktrees. For any file your branch " "changed (git diff --name-only ...HEAD, plus uncommitted edits), Read " "the file itself: index hits for it show the indexed version, and line numbers may " - "be off. Use the index for how the rest of the codebase works. " + f"be off. {routing}" "Never call `reindex` from a git worktree made for separate or parallel work; it " "refuses there, and one server per index keeps it current on its own. " "`index_status` reports freshness." @@ -213,7 +218,7 @@ def bound(*args, **kwargs): _read_limiter: "anyio.CapacityLimiter | None" = None -def _tool(*, reads_index: bool = True): +def _tool(*, reads_index: bool = True, **hints): """Register ``fn`` as an MCP tool that runs in a worker thread. FastMCP calls a plain ``def`` tool directly on the event loop, so one slow @@ -223,7 +228,22 @@ def _tool(*, reads_index: bool = True): (``reads_index=False``) does neither: it would only load the index it is about to rebuild, and a rebuild must not hold up searches. The module keeps the plain function, so the TUI and the tests still call it directly. + + ADR-049: ``readOnlyHint`` is ``reads_index``, so every read tool declares it + without its author doing anything; a writer passes its other annotations + (``destructiveHint=...``) as ``hints``. The wrapper redacts secrets from the + returned text, so only what leaves the server is scrubbed: the plain + function's output is unchanged. Every session pays for the listing (B-019), + so the docstring is sent dedented, and without the ``{"result": string}`` + output schema FastMCP would generate for a ``-> str`` tool. """ + import inspect + from mcp.types import ToolAnnotations + from redact import redact + if "readOnlyHint" in hints: + raise TypeError("readOnlyHint is derived from reads_index") + annotations = ToolAnnotations(readOnlyHint=reads_index, **hints) + def register(fn): bound = _bind_index(fn) if reads_index else fn @@ -232,12 +252,15 @@ async def run_in_thread(*args, **kwargs): global _read_limiter call = functools.partial(bound, *args, **kwargs) if not reads_index: - return await anyio.to_thread.run_sync(call) - if _read_limiter is None: - _read_limiter = anyio.CapacityLimiter(1) - return await anyio.to_thread.run_sync(call, limiter=_read_limiter) + out = await anyio.to_thread.run_sync(call) + else: + if _read_limiter is None: + _read_limiter = anyio.CapacityLimiter(1) + out = await anyio.to_thread.run_sync(call, limiter=_read_limiter) + return redact(out) if isinstance(out, str) else out - mcp.tool()(run_in_thread) + mcp.tool(description=inspect.cleandoc(fn.__doc__ or ""), annotations=annotations, + structured_output=False)(run_in_thread) return bound return register @@ -1269,16 +1292,11 @@ def investigate_architecture(target_concept: str, deep: bool = False) -> str: """ PREFERRED ENTRY POINT for all architectural investigations. Replaces raw `semantic_code_search` when you want a complete picture of how a - concept, feature, data type, or function flows through the system. - - Internally runs the full Retrieve-Traverse-Rerank pipeline: - 1. Semantic Search — FAISS tier-1 index, top-50 by cosine similarity. - 2. Graph Expansion — one-hop call-graph traversal via SQLite for top-5 seeds. - 3. Ranking — Reciprocal Rank Fusion; a reranker model only when enabled in indexer.toml. + concept, feature, data type, or function flows through the system: semantic + search plus one hop of the call graph around the best hits. - When deep=True, runs up to 3 iterative retrieval rounds with explored-node memory - and query enrichment from prior evidence, stopping early when the score plateau - indicates diminishing returns. + deep=True runs up to 3 retrieval rounds, each enriched by the last; slower, for + when one round comes back thin. Returns a Markdown report with: - XML tags (source, lines, fqn, type) for each retrieved chunk, showing @@ -1544,8 +1562,8 @@ def is_test_file(fp: str) -> bool: return context -@_tool(reads_index=False) -def reindex(changed_files_only: bool = False) -> str: +@_tool(reads_index=False, destructiveHint=True, idempotentHint=False) +def reindex(changed_files_only: bool = True) -> str: """ Rebuilds the index from its source: this folder's files, or, when indexer.toml sets `[indexer] source = "git:"`, that ref's commit, never your branch or your edits. @@ -1553,11 +1571,11 @@ def reindex(changed_files_only: bool = False) -> str: Rarely needed: the server's watchdog keeps the index current on its own (index_status shows whether it is). Never call it to pick up branch work. - changed_files_only=True — incremental: only processes files added, modified, or - deleted since the last run. Fast for frequent refreshes. - changed_files_only=False — full (default): clears all index state first, then - re-indexes every file from scratch. Use after major - refactors or when the incremental index appears corrupted. + changed_files_only=True — incremental (default): only files added, modified, or + deleted since the last run. Fast. + changed_files_only=False — full: WIPES the index, then re-embeds every file. Slow + (tens of minutes) and destructive; only when the index is + corrupted, and never from a client with a request timeout. Returns a summary of chunks added/updated/removed, then reloads the in-memory indexes. """ @@ -1639,6 +1657,7 @@ def _reindex(changed_files_only: bool) -> str: if not changed_files_only: # #50: keep a copy of the index to restore if the rebuild raises (embedder OOM, # a failed model download), rather than leaving it empty until the next rerun. + # ADR-049: a backup a killed run left behind is restored here, never deleted. _backup_dir = _snapshot_index(INDEX_DIR, db_path) # Wipe all index state so run_incremental treats everything as new if os.path.exists(db_path): @@ -1676,12 +1695,16 @@ def _reindex(changed_files_only: bool) -> str: if _backup_dir is not None: sys.stdout = _old_stdout _restore_index(_backup_dir, INDEX_DIR, db_path) + # ADR-049: removed only once the restore has finished; a kill before + # this line leaves the backup for the next full rebuild to restore. + shutil.rmtree(_backup_dir, ignore_errors=True) _reload_indexes() print("[MCP] reindex: full rebuild failed; the previous index was restored") raise finally: sys.stdout = _old_stdout if _backup_dir is not None: + # run_incremental returned, so the new index is saved: the backup is done. shutil.rmtree(_backup_dir, ignore_errors=True) # Reload in-memory state so subsequent MCP tool calls see the updated index. The @@ -1723,21 +1746,16 @@ def index_status(since: str = "1d", limit: int = 20) -> str: """Report index freshness and which files changed recently (ADR-025 §6). Use this to answer "what changed in this codebase lately?" and "is the index - current with the code?" — e.g. before trusting the other tools on recently - edited files, or to feed a downstream multi-project context hub. Output is - agent-parseable: absolute ISO-8601 UTC timestamps and full repo-relative - paths, never prose you have to re-parse. - - `since` accepts a relative window ("1d", "12h", "30m", "7d") or an absolute - ISO-8601 timestamp ("2026-07-15T00:00:00Z"). Files whose CONTENT changed after - that point are listed — content_changed_at, not index-write time — so a file - re-indexed today but unchanged in a week does not show up. Files with no git - history (untracked / vendored) carry a NULL stamp and are correctly excluded. - `limit` caps the list at its newest entries (default 20; 0 lists all); the - count shown is always the full one. - - Reports: last_verified_at, last_indexed_commit vs current HEAD (with the list - of diverged files when stale), files_total, and the recent-change list. + current with the code?", e.g. before trusting the other tools on recently + edited files. Timestamps are ISO-8601 UTC; paths are repo-relative. + + `since` is a relative window ("1d", "12h", "30m") or an ISO-8601 timestamp. + Lists files whose CONTENT changed after it (not when they were re-indexed); + files with no git history are left out. `limit` caps the list at its newest + entries (default 20; 0 lists all); the count shown is always the full one. + + Reports: last_verified_at, last_indexed_commit vs current HEAD (with the files + that diverged when stale), files_total, and the recent-change list. """ import os import subprocess @@ -2056,32 +2074,23 @@ def verify_candidate_edges(symbol: str, anchor_file: str = "") -> str: @_tool() def what_writes(tag: str, include_readers: bool = False) -> str: """ - Given a PLC tag, reports EXACTLY which routines write it and at which rungs. + Given a PLC tag, reports EXACTLY which routines write it and at which rungs: + "an alarm will not clear, so what sets that bit?". Go one hop at a time: the + next question is `what_writes` on whichever condition looks wrong. - This is the question a controls engineer asks first on a support call: an alarm - will not clear, a valve will not open, a sequence will not advance — so what - sets that bit? Answer it one hop at a time and let the human prune: the next - question is `what_writes` on whichever condition looks wrong. - - **This is a lookup, not a search.** It reads resolved `writes` edges straight - out of the graph — no embedding, no ranking, no reranker. Every result is a - real write position the extractor resolved, and the list is complete. Do not - reach for semantic_code_search or trace_data_flow for this question; both rank, - and a ranked answer to "what writes X" can silently omit a writer. + **A lookup, not a search:** it reads resolved `writes` edges from the graph, so + the list is complete. Do not use semantic_code_search or trace_data_flow for + this; both rank, and a ranked answer can silently omit a writer. Inputs: - tag — the tag as an engineer types it (e.g. 'Valve_Open'), or a - scoped FQN ('Filler.Step') for a program-scoped tag - include_readers — also list where the tag is READ. Off by default because - widely-read tags produce long lists; turn it on when you - are asking "what uses this" rather than "what sets this" - - Output: each writing routine with its rung numbers, plus alias/module-I/O - mapping when the tag is an alias. - - What this CANNOT tell you: which of several conditions is *currently* true. - That needs a live connection to the controller. This enumerates the candidates - — which is the half you can do from the phone without going online. + tag — as an engineer types it ('Valve_Open'), or a scoped FQN + ('Filler.Step') for a program-scoped tag + include_readers — also list where the tag is READ ("what uses this" rather + than "what sets this"); off by default, as the lists get long + + Output: each writing routine with its rung numbers, plus the alias/module-I/O + mapping when the tag is an alias. It cannot tell which condition is *currently* + true; that needs a live connection to the controller. """ print(f"\n[MCP] what_writes: tag='{tag}' readers={include_readers}") db = _db() @@ -2396,13 +2405,25 @@ def _snapshot_index(index_dir: str, db_path: str) -> str: The database is copied with SQLite's backup API, which reads a consistent snapshot through the WAL. Returns the backup directory. + + ADR-049 (B-057): a backup that already exists was left by a full rebuild that + never finished, so it is the good copy and the live index is suspect. It is + restored and reused, never deleted. A new snapshot is copied into a + ``.partial`` directory and renamed into place only once complete, so a + ``.partial`` left behind was killed before the wipe and is safe to discard. """ backup_dir = os.path.join(index_dir, ".pre-full-reindex") - shutil.rmtree(backup_dir, ignore_errors=True) # left by a killed earlier run - os.makedirs(backup_dir) + if os.path.isdir(backup_dir): + print("[MCP] reindex: an earlier full rebuild never finished; restoring the " + "index it backed up before starting this one") + _restore_index(backup_dir, index_dir, db_path) + return backup_dir + partial = backup_dir + ".partial" + shutil.rmtree(partial, ignore_errors=True) + os.makedirs(partial) if os.path.exists(db_path): src = sqlite3.connect(db_path) - dst = sqlite3.connect(os.path.join(backup_dir, "graph.db")) + dst = sqlite3.connect(os.path.join(partial, "graph.db")) try: src.backup(dst) finally: @@ -2410,12 +2431,14 @@ def _snapshot_index(index_dir: str, db_path: str) -> str: src.close() for name in os.listdir(index_dir): if name.endswith(".faiss"): - shutil.copy2(os.path.join(index_dir, name), os.path.join(backup_dir, name)) + shutil.copy2(os.path.join(index_dir, name), os.path.join(partial, name)) + os.replace(partial, backup_dir) return backup_dir def _restore_index(backup_dir: str, index_dir: str, db_path: str) -> None: - """Put back what `_snapshot_index` saved, then delete the backup.""" + """Put back what `_snapshot_index` saved. The backup is kept: it is copied, + never moved, so a restore killed partway can simply be run again (ADR-049).""" saved_db = os.path.join(backup_dir, "graph.db") if os.path.exists(saved_db): src = sqlite3.connect(saved_db) @@ -2430,8 +2453,9 @@ def _restore_index(backup_dir: str, index_dir: str, db_path: str) -> None: if name.endswith(".faiss") and name not in saved_faiss: os.remove(os.path.join(index_dir, name)) for name in saved_faiss: - os.replace(os.path.join(backup_dir, name), os.path.join(index_dir, name)) - shutil.rmtree(backup_dir, ignore_errors=True) + tmp = os.path.join(index_dir, name + ".restoring") + shutil.copy2(os.path.join(backup_dir, name), tmp) + os.replace(tmp, os.path.join(index_dir, name)) def _reload_indexes() -> None: diff --git a/src/redact.py b/src/redact.py new file mode 100644 index 0000000..0673a83 --- /dev/null +++ b/src/redact.py @@ -0,0 +1,92 @@ +"""Secret redaction for text leaving the MCP server (ADR-049 §5, B-018). + +Applied once, in MCPServer._tool's wrapper, to every tool's returned text. The +index stays faithful and the plain tool functions stay unredacted; only what an +agent receives is scrubbed. Each match becomes a visible ``[REDACTED:]`` +marker, so a redaction fails loud instead of silently dropping text. + +Patterns require each token family's real prefix, length and alphabet, with no +word character in front, so ordinary code (``sk_`` names, ``sk-`` class names, +short hex, ``AKIA`` in prose) passes through untouched. +Families with no prefix (an AWS secret key, a password) are matched by the name +they are assigned to, and only the value is replaced. +""" +import re + +# Not preceded by a character that would make the prefix part of a longer word. +_START = r"(?(?P["'])private_key(?P=q)\s*:\s*)(?P["'])(?:(?!(?P=vq))[^\\]|\\.)+(?P=vq)""")), + ("private-key", re.compile( + # The body is base64, line breaks (real, or a JSON string's literal \n) and + # the Proc-Type/DEK-Info headers, so code between two header constants + # is never taken for a key. + r"-----BEGIN [A-Z0-9 ]*PRIVATE KEY-----" + r"(?:[A-Za-z0-9+/=\s\\:,\-]*?-----END [A-Z0-9 ]*PRIVATE KEY-----" + # No END line: the chunk was cut off inside the key; take its base64 lines. + r"|(?:(?:\s|\\n)*[A-Za-z0-9+/=]{16,})+)")), + ("github-token", re.compile(_START + r"(?:gh[pousr]_[A-Za-z0-9]{36}|github_pat_[A-Za-z0-9_]{50,})" + _END)), + ("slack-token", re.compile(_START + r"xox[abprs]-[A-Za-z0-9-]{10,}" + _END)), + ("aws-access-key", re.compile(_START + r"(?:AKIA|ASIA)[0-9A-Z]{16}" + _END)), + ("google-api-key", re.compile(_START + r"AIza[0-9A-Za-z_\-]{35}" + _END)), + ("anthropic-key", re.compile(_START + r"sk-ant-[A-Za-z0-9_\-]{20,}" + _END)), + ("openai-key", re.compile( + _START + r"sk-(?:(?:proj|svcacct|admin)-[A-Za-z0-9_\-]{20,}|[A-Za-z0-9]{20,})" + _END)), + # Stripe secret and restricted keys, and webhook signing secrets. The 24-character + # tail keeps `sk_live = ...` and `sk_test_mode` names untouched; publishable pk_ keys + # are public by design. + ("stripe-key", re.compile(_START + r"(?:(?:sk|rk)_(?:live|test)_[A-Za-z0-9]{24,}|whsec_[A-Za-z0-9]{32,})" + _END)), + # The patterns below have no prefix of their own, so they key on the name the value is + # assigned to and replace only the value (the "val" group), skipping a value an earlier + # pattern already replaced. They accept some false positives: a long literal assigned to + # a name like `secret_name` is redacted too. + # An AWS secret key is 40 characters of base64; only a name saying so separates it + # from a git SHA. Before the generic assignment, which would also match the name. + ("aws-secret-key", re.compile( + r"(?i)(?[A-Za-z0-9/+]{40})(?![A-Za-z0-9/+=])")), + # user:password@ in a URL (database URLs, git remotes with a token). + ("url-credential", re.compile( + r"(?i)\b[a-z][a-z0-9+.\-]*://[^\s:/@\"'<>]+:(?!\[REDACTED:)(?P[^\s@/\"'<>]+)@")), + # Password=...; in a connection string. The trailing ; keeps `pwd = get()` out. + ("connection-string-password", re.compile( + r"(?i)(?[^;\"'\s]+)(?=\s*;)")), + # A quoted literal of 8 or more non-space characters assigned to (or keyed by) a name + # containing password, passwd, secret, api_key, apikey, access_token or auth_token. + # A value with a space is prose (a label or prompt), not a credential. + ("secret-assignment", re.compile( + r"(?i)(?|[:=])\s*(?P[\"'`])(?!\[REDACTED:)" + r"(?P[^\s\"'`\\]{8,})(?P=q)")), +] + + +def _marker(kind: str) -> str: + return f"[REDACTED:{kind}]" + + +def _replace_val(kind: str): + def sub(m: re.Match) -> str: + whole = m.group(0) + start, end = m.start("val") - m.start(), m.end("val") - m.start() + return whole[:start] + _marker(kind) + whole[end:] + return sub + + +def redact(text: str) -> str: + """``text`` with every recognized secret replaced by ``[REDACTED:]``.""" + for kind, pattern in _PATTERNS: + if kind == "private-key-field": + text = pattern.sub(lambda m: f"{m['head']}{m['vq']}{_marker(kind)}{m['vq']}", text) + elif "val" in pattern.groupindex: + text = pattern.sub(_replace_val(kind), text) + else: + text = pattern.sub(_marker(kind), text) + return text diff --git a/src/tui/app.py b/src/tui/app.py index b175cee..7c79eb9 100644 --- a/src/tui/app.py +++ b/src/tui/app.py @@ -506,8 +506,9 @@ def _collect_params(self) -> dict: else: params[p.name] = val except Exception: + # ADR-049: an unreadable Mode field means incremental, never a full rebuild. params[p.name] = ( - False if p.name in ("deep", "changed_files_only") else "" + False if p.name == "deep" else True if p.name == "changed_files_only" else "" ) return params diff --git a/tests/test_safety_redact.py b/tests/test_safety_redact.py new file mode 100644 index 0000000..8a5fc09 --- /dev/null +++ b/tests/test_safety_redact.py @@ -0,0 +1,144 @@ +"""ADR-049 §5 (B-018): secrets are redacted from tool output, with a marker naming +the kind, and ordinary code passes through untouched. + +Every fixture here is obviously fake: the right prefix and length, filled with x. +""" +import pytest + +from redact import redact + +X = "x" + +FAKE_PEM = "-----BEGIN PRIVATE KEY-----\nFAKE\n-----END PRIVATE KEY-----" + +SECRETS = [ + ("private-key", FAKE_PEM), + ("private-key", "-----BEGIN RSA PRIVATE KEY-----\nFAKE\n-----END RSA PRIVATE KEY-----"), + ("private-key", "-----BEGIN OPENSSH PRIVATE KEY-----\nFAKE\n-----END OPENSSH PRIVATE KEY-----"), + ("github-token", "ghp_" + X * 36), + ("github-token", "gho_" + X * 36), + ("github-token", "ghs_" + X * 36), + ("github-token", "github_pat_" + X * 22 + "_" + X * 59), + ("slack-token", "xoxb-0000000000-" + X * 12), + ("slack-token", "xoxp-" + X * 20), + ("slack-token", "xoxa-" + X * 20), + ("aws-access-key", "AKIA" + "X" * 16), + ("google-api-key", "AIza" + X * 35), + ("openai-key", "sk-" + X * 48), + ("openai-key", "sk-proj-" + X * 40), + ("anthropic-key", "sk-ant-api03-" + X * 40), +] + + +@pytest.mark.parametrize("kind,secret", SECRETS, ids=[f"{k}-{i}" for i, (k, _) in enumerate(SECRETS)]) +def test_each_secret_kind_is_replaced_by_a_marker_naming_it(kind, secret): + text = f'TOKEN = "{secret}"\nprint(TOKEN)\n' + out = redact(text) + assert secret not in out + assert out == f'TOKEN = "[REDACTED:{kind}]"\nprint(TOKEN)\n' + + +def test_a_service_account_private_key_field_is_redacted_as_the_field(): + text = ('{"type": "service_account", "private_key_id": "abc123",\n' + ' "private_key": "-----BEGIN PRIVATE KEY-----\\nFAKE\\n-----END PRIVATE KEY-----\\n",\n' + ' "client_email": "svc@example.invalid"}') + out = redact(text) + assert "FAKE" not in out and "BEGIN" not in out + assert '"private_key": "[REDACTED:private-key-field]"' in out + assert '"private_key_id": "abc123"' in out and "svc@example.invalid" in out + + +def test_a_python_dict_private_key_field_is_redacted(): + out = redact("creds = {'private_key': 'FAKEFAKEFAKE', 'project': 'demo'}") + assert out == "creds = {'private_key': '[REDACTED:private-key-field]', 'project': 'demo'}" + + +def test_a_key_cut_off_by_the_chunk_boundary_is_still_redacted(): + """A chunk can end inside a key, so there is no END line to match up to.""" + text = "KEY = '''\n-----BEGIN PRIVATE KEY-----\n" + "FAKE" * 16 + "\n" + "FAKE" * 16 + "\n" + out = redact(text) + assert "FAKE" not in out and out == "KEY = '''\n[REDACTED:private-key]\n" + + +def test_text_after_a_cut_off_key_survives(): + text = "-----BEGIN PRIVATE KEY-----\n" + "FAKE" * 16 + "\ndef after():\n pass\n" + assert redact(text) == "[REDACTED:private-key]\ndef after():\n pass\n" + + +@pytest.mark.parametrize("text", [ + "sk_live = load_key()\nsk_test_mode = True\nmask_sk_value = sk_[0]", + "class='sk-button-primary-large-variant'", + "risk-assessment-and-mitigation-plan-for-q3", + "digest = '0xdeadbeef'; short = 'deadbeefcafebabe0123'", + "The AKIA prefix marks a long-term AWS access key; ASIA marks a temporary one.", + "Google keys start with AIza, so check the prefix.", + "ghp_token = os.environ['GITHUB_TOKEN']", + "if token.startswith('xoxb-'): send()", + "-----BEGIN PUBLIC KEY-----\nMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEA\n-----END PUBLIC KEY-----", + 'PEM_HEADER = "-----BEGIN PRIVATE KEY-----"\nPEM_FOOTER = "-----END PRIVATE KEY-----"', + 'cfg = {"private_key": key_from_vault(), "private_key_id": "abc"}', + "task-" + "a" * 30, +]) +def test_ordinary_code_passes_through_untouched(text): + assert redact(text) == text + + +def test_a_secret_inside_a_longer_token_is_not_split(): + """A prefix in the middle of a longer identifier is not the start of a token.""" + text = "my_ghp_" + X * 36 + " xAKIA" + "X" * 16 + assert redact(text) == text + + +# The families with no prefix of their own (@edb, 2026-10-20: add them and accept the false +# positives). Only the value is replaced; the name it is assigned to stays readable. +NAMED_SECRETS = [ + ("stripe-key", 'STRIPE = "sk_live_' + X * 24 + '"', 'STRIPE = "[REDACTED:stripe-key]"'), + ("stripe-key", 'k = "rk_test_' + X * 30 + '"', 'k = "[REDACTED:stripe-key]"'), + ("stripe-key", 'hook = "whsec_' + X * 32 + '"', 'hook = "[REDACTED:stripe-key]"'), + ("aws-secret-key", "aws_secret_access_key = " + X * 40, + "aws_secret_access_key = [REDACTED:aws-secret-key]"), + ("aws-secret-key", '"SecretAccessKey": "' + X * 38 + '/+"', + '"SecretAccessKey": "[REDACTED:aws-secret-key]"'), + ("url-credential", 'DB = "postgres://app:' + X * 12 + '@db.example.invalid/prod"', + 'DB = "postgres://app:[REDACTED:url-credential]@db.example.invalid/prod"'), + ("connection-string-password", + '"Server=db;Database=erp;User Id=sa;Password=' + X * 10 + ';"', + '"Server=db;Database=erp;User Id=sa;Password=[REDACTED:connection-string-password];"'), + ("secret-assignment", 'password = "' + X * 12 + '"', 'password = "[REDACTED:secret-assignment]"'), + ("secret-assignment", "DB_PASSWD: '" + X * 8 + "'", "DB_PASSWD: '[REDACTED:secret-assignment]'"), + ("secret-assignment", '{"client_secret": "' + X * 20 + '"}', + '{"client_secret": "[REDACTED:secret-assignment]"}'), + ("secret-assignment", "const apiKey = `" + X * 16 + "`;", "const apiKey = `[REDACTED:secret-assignment]`;"), + ("secret-assignment", 'AuthToken := "' + X * 16 + '"', 'AuthToken := "[REDACTED:secret-assignment]"'), +] + + +@pytest.mark.parametrize("kind,text,expected", NAMED_SECRETS, + ids=[f"{k}-{i}" for i, (k, _, _) in enumerate(NAMED_SECRETS)]) +def test_a_named_secret_has_only_its_value_replaced(kind, text, expected): + assert redact(text) == expected + + +@pytest.mark.parametrize("text", [ + 'password = get_password()', + 'password = os.environ["DB_PASSWORD"]', + 'label = "Enter your password"', + 'PASSWORD_PROMPT = "Your password:"', + 'password = ""', + 'pwd = "short"', + 'pwd = os.getcwd()', + 'commit = "' + "a" * 40 + '"', + 'sk_live = load_key()\nsk_test_mode = True', + 'pk_live_' + X * 24, + 'url = "https://example.invalid/path:8080@v2"', + 'url = "https://example.invalid:8443/api"', +]) +def test_code_near_the_named_families_passes_through(text): + assert redact(text) == text + + +def test_a_token_marker_is_not_redacted_again_by_a_named_pattern(): + """The token pattern names the kind; the assignment around it must not rename it.""" + assert redact('API_KEY = "sk-ant-api03-' + X * 40 + '"') == 'API_KEY = "[REDACTED:anthropic-key]"' + assert (redact("https://user:ghp_" + X * 36 + "@github.com/o/r.git") + == "https://user:[REDACTED:github-token]@github.com/o/r.git") diff --git a/tests/test_safety_reindex.py b/tests/test_safety_reindex.py new file mode 100644 index 0000000..658ac8a --- /dev/null +++ b/tests/test_safety_reindex.py @@ -0,0 +1,281 @@ +"""ADR-049 (B-057): a killed full rebuild never costs the only good copy of the +index, and a bare `reindex()` is not a full rebuild. + +Replays the 2026-09-29 incident in-process: two full rebuilds, each killed +partway, after which the original index must still be restorable byte for byte. +A real build runs through run_incremental with a stubbed embedder and the +summarizer off, so no model loads (as test_index_data_safety does). A kill is a +BaseException raised from inside the build, and the restore that `_reindex`'s +handler would run is made to die as well: a killed process runs no handler. +""" +from __future__ import annotations + +import hashlib +import os +import sqlite3 + +import anyio +import numpy as np +import pytest +from types import SimpleNamespace + +import core +import incremental_indexer as ii +import MCPServer +from core import embed_dimension + +BACKUP = ".pre-full-reindex" + + +class Killed(BaseException): + """The server process dying (Inspector's 60 s timeout): nothing after it runs.""" + + +def _fake_embed(texts, batch_size=32): + rng = np.random.default_rng(len(texts)) + return rng.standard_normal((len(texts), embed_dimension())).astype(np.float32) + + +class _CharTokenizer: + """One token per character, so the chunker runs without fetching the embedder's + tokenizer from the network.""" + def encode(self, text, add_special_tokens=False): + return [ord(c) for c in text] + + def decode(self, tokens): + return "".join(chr(t) for t in tokens) + + +def _checkpoint(db_path): + """Fold the WAL into graph.db, so the file's bytes are the whole database.""" + con = sqlite3.connect(db_path) + try: + con.execute("PRAGMA wal_checkpoint(TRUNCATE)") + finally: + con.close() + + +def _db_bytes(data: bytes) -> bytes: + """graph.db's bytes without the three 4-byte header counters SQLite's backup API + bumps on the file it writes: the file change counter (offset 24), the schema + cookie (40), so open connections reload the schema, and version-valid-for (92). + Every other byte, every page included, is compared as is.""" + return data[:24] + data[28:40] + data[44:92] + data[96:] + + +def _fingerprint(index_dir): + """{file: sha256} of graph.db and every .faiss file, after a checkpoint.""" + db_path = os.path.join(index_dir, "graph.db") + if os.path.exists(db_path): + _checkpoint(db_path) + out = {} + for name in sorted(os.listdir(index_dir)): + if name == "graph.db" or name.endswith(".faiss"): + with open(os.path.join(index_dir, name), "rb") as fh: + data = fh.read() + out[name] = hashlib.sha256(_db_bytes(data) if name == "graph.db" else data).hexdigest() + return out + + +def _file_rows(index_dir): + con = sqlite3.connect(os.path.join(index_dir, "graph.db")) + try: + return con.execute("SELECT COUNT(*) FROM files").fetchone()[0] + finally: + con.close() + + +@pytest.fixture +def live(tmp_path, monkeypatch): + """A repo of six files, fully indexed, wired so `_reindex` rebuilds it.""" + repo = tmp_path / "repo" + (repo / "src").mkdir(parents=True) + for i in range(6): + (repo / "src" / f"m{i}.py").write_text(f"def f{i}():\n return {i}\n", encoding="utf-8") + index = tmp_path / "index" + index.mkdir() + monkeypatch.setattr(core, "embed_batch", _fake_embed) + monkeypatch.setattr(core.jina_tokenizer, "_tokenizer", _CharTokenizer()) + monkeypatch.setattr(ii, "summarization_enabled", lambda: False) + monkeypatch.setattr(ii, "INDEX_DIR", str(index)) + monkeypatch.setattr(ii, "DB_PATH", str(index / "graph.db")) + ii.run_incremental(str(repo), interactive=False) + real_run = ii.run_incremental + monkeypatch.setattr(ii, "run_incremental", lambda **kw: real_run(str(repo), **kw)) + monkeypatch.setattr(MCPServer, "_reload_indexes", lambda: None) + return str(index) + + +@pytest.fixture +def kill_partway(monkeypatch): + """``arm()`` makes the next full rebuild die after embedding its first batch; + ``revive()`` is the server starting again afterwards.""" + real_restore = MCPServer._restore_index + state = {"dead": False} + + def arm(): + state["dead"] = False + calls = [] + + def embed_then_die(texts, batch_size=32): + calls.append(1) + if len(calls) > 1: + state["dead"] = True + raise Killed + return _fake_embed(texts, batch_size) + + # Small batches, so the build dies with part of the repo written. + monkeypatch.setattr(core, "embed_batch", embed_then_die) + monkeypatch.setattr(ii, "_EMBED_WINDOW_TEXTS", 1) + + def restore_unless_dead(*args): + if state["dead"]: + raise Killed # the process is gone; the handler never runs + return real_restore(*args) + + def revive(): + state["dead"] = False + monkeypatch.setattr(core, "embed_batch", _fake_embed) + + monkeypatch.setattr(MCPServer, "_restore_index", restore_unless_dead) + return SimpleNamespace(arm=arm, revive=revive) + + +def test_two_killed_full_rebuilds_leave_the_original_index_restorable(live, kill_partway): + """The incident: run 1 killed, run 2 killed. Run 2 used to delete run 1's backup.""" + original = _fingerprint(live) + assert any(n.endswith(".faiss") for n in original) + + kill_partway.arm() + with pytest.raises(Killed): + MCPServer._reindex(changed_files_only=False) + kill_partway.revive() + assert os.path.isdir(os.path.join(live, BACKUP)), "run 1's kill took the backup with it" + assert _file_rows(live) < 6 # the live index is half-built + + kill_partway.arm() + with pytest.raises(Killed): + MCPServer._reindex(changed_files_only=False) + kill_partway.revive() + assert _file_rows(live) < 6 + backup = os.path.join(live, BACKUP) + assert os.path.isdir(backup), "run 2 deleted the only good copy" + + MCPServer._restore_index(backup, live, os.path.join(live, "graph.db")) + assert _fingerprint(live) == original + + +def test_a_full_rebuild_after_a_killed_one_finishes_and_removes_the_backup(live, kill_partway): + original = _fingerprint(live) + kill_partway.arm() + with pytest.raises(Killed): + MCPServer._reindex(changed_files_only=False) + kill_partway.revive() + + MCPServer._reindex(changed_files_only=False) + + assert not os.path.exists(os.path.join(live, BACKUP)) + assert _file_rows(live) == 6 + assert {n for n in _fingerprint(live) if n.endswith(".faiss")} == \ + {n for n in original if n.endswith(".faiss")} + + +def test_a_backup_found_at_startup_is_restored_never_deleted(live): + original = _fingerprint(live) + db_path = os.path.join(live, "graph.db") + backup = MCPServer._snapshot_index(live, db_path) + # What a killed run leaves: the live index wiped, the backup still there. + con = sqlite3.connect(db_path) + with con: + con.execute("DELETE FROM files") + con.close() + for name in os.listdir(live): + if name.endswith(".faiss"): + os.remove(os.path.join(live, name)) + + again = MCPServer._snapshot_index(live, db_path) + + assert again == backup and os.path.isdir(backup) + assert _fingerprint(live) == original # restored before the new run wipes it + assert _fingerprint(backup) == original + + +def test_a_snapshot_killed_while_copying_is_discarded_and_the_live_index_kept(live): + """A .partial directory was never renamed into place, so the wipe never ran.""" + original = _fingerprint(live) + partial = os.path.join(live, BACKUP + ".partial") + os.makedirs(partial) + with open(os.path.join(partial, "tier1_surgical.faiss"), "wb") as fh: + fh.write(b"\x00" * 16) # a truncated copy + + backup = MCPServer._snapshot_index(live, os.path.join(live, "graph.db")) + + assert not os.path.exists(partial) + assert _fingerprint(live) == original + assert _fingerprint(backup) == original + + +def test_a_restore_killed_partway_can_run_again(live, monkeypatch): + """The restore copies out of the backup instead of moving files, so a kill + mid-restore leaves the backup whole.""" + original = _fingerprint(live) + db_path = os.path.join(live, "graph.db") + backup = MCPServer._snapshot_index(live, db_path) + for name in os.listdir(live): + if name.endswith(".faiss"): + os.remove(os.path.join(live, name)) + + import shutil + real_copy = shutil.copy2 + copies = [] + + def copy_then_die(src, dst, **kw): + copies.append(src) + if len(copies) > 1: + raise Killed + return real_copy(src, dst, **kw) + + monkeypatch.setattr(shutil, "copy2", copy_then_die) + with pytest.raises(Killed): + MCPServer._restore_index(backup, live, db_path) + monkeypatch.setattr(shutil, "copy2", real_copy) + + assert _fingerprint(backup) == original + MCPServer._restore_index(backup, live, db_path) + assert _fingerprint(live) == original + + +# -------------------------------------------------------------------------- +# A bare reindex() is incremental +# -------------------------------------------------------------------------- + +@pytest.fixture +def recorded_reindex(monkeypatch, tmp_path): + seen = [] + monkeypatch.setattr(MCPServer, "_reindex", lambda changed_files_only: seen.append(changed_files_only) or "ok") + import index_lock + monkeypatch.setattr(index_lock, "worktree_refusal", lambda path: None) # tests run in worktrees + monkeypatch.setattr(ii, "INDEX_DIR", str(tmp_path)) + return seen + + +def test_a_bare_reindex_call_is_incremental(recorded_reindex): + MCPServer.reindex() + assert recorded_reindex == [True] + + +def test_a_bare_reindex_through_mcp_is_incremental(recorded_reindex): + """What a client sends when it omits the argument: the schema's default.""" + anyio.run(MCPServer.mcp.call_tool, "reindex", {}) + assert recorded_reindex == [True] + + +def test_a_full_rebuild_still_runs_when_asked_for_by_name(recorded_reindex): + anyio.run(MCPServer.mcp.call_tool, "reindex", {"changed_files_only": False}) + assert recorded_reindex == [False] + + +def test_the_listed_schema_defaults_to_incremental(): + (t,) = [t for t in MCPServer.mcp._tool_manager.list_tools() if t.name == "reindex"] + assert t.parameters["properties"]["changed_files_only"]["default"] is True + assert "destructive" in t.description and "Slow" in t.description diff --git a/tests/test_safety_tool_surface.py b/tests/test_safety_tool_surface.py new file mode 100644 index 0000000..c0c6318 --- /dev/null +++ b/tests/test_safety_tool_surface.py @@ -0,0 +1,108 @@ +"""ADR-049 §4–6: every read tool declares readOnlyHint (B-045), tool output is +redacted at the MCP boundary only (B-018), and the server instructions route +agents to the index before Grep (B-021). + +The tool lists come from the FastMCP registry, not a hand-written list, so a tool +added later is covered without touching this file. +""" +import inspect +import os +import sys +from types import SimpleNamespace + +import anyio +import pytest + +sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..", "src")) + +import MCPServer as M # noqa: E402 + +FAKE_TOKEN = "ghp_" + "x" * 36 +FAKE_PEM = "-----BEGIN PRIVATE KEY-----\nFAKE\n-----END PRIVATE KEY-----" + + +def _registered(): + return M.mcp._tool_manager.list_tools() + + +def test_every_tool_but_reindex_is_read_only(): + tools = _registered() + assert any(t.name == "reindex" for t in tools) + for t in tools: + if t.name == "reindex": + continue + assert t.annotations is not None and t.annotations.readOnlyHint is True, t.name + + +def test_reindex_declares_it_writes_and_is_not_safe_to_retry(): + (t,) = [t for t in _registered() if t.name == "reindex"] + assert t.annotations.readOnlyHint is False + assert t.annotations.destructiveHint is True + assert t.annotations.idempotentHint is False # a client must not auto-retry a timed-out rebuild + + +def test_the_listing_carries_the_hints_to_clients(): + listed = anyio.run(M.mcp.list_tools) + hints = {t.name: t.annotations.readOnlyHint for t in listed} + assert hints.pop("reindex") is False + assert hints and all(hints.values()) + + +def test_the_listing_sends_dedented_descriptions_and_no_output_schema(): + """B-019: every session pays for the listing, so it carries no indentation and + no generated ``{"result": string}`` schema, which tells an agent nothing.""" + for t in anyio.run(M.mcp.list_tools): + assert t.outputSchema is None, t.name + assert t.description and t.description == inspect.cleandoc(t.description), t.name + + +def test_read_only_cannot_be_overridden_by_hand(): + with pytest.raises(TypeError, match="derived from reads_index"): + M._tool(readOnlyHint=False) + + +class _Chunk(SimpleNamespace): + pass + + +@pytest.fixture +def search_returning_secrets(monkeypatch): + """semantic_code_search over a stubbed index whose only chunk holds two fake secrets.""" + chunk = _Chunk(file="src/settings.py", scope="settings", tier="tier1_surgical", + text=f'GITHUB_TOKEN = "{FAKE_TOKEN}"\nKEY = """{FAKE_PEM}"""\n') + monkeypatch.setattr(M, "_ensure_indexes", lambda: SimpleNamespace(db=None)) + monkeypatch.setattr(M, "_search", lambda query, top_n=10: [chunk]) + monkeypatch.setattr(M, "jina_tokenizer", SimpleNamespace(count_tokens=len)) + import incremental_indexer + monkeypatch.setattr(incremental_indexer, "chunker_version_warning", lambda db: "") + + +def _text(result) -> str: + content = result[0] if isinstance(result, tuple) else result + return "".join(getattr(block, "text", "") for block in content) + + +def test_tool_output_is_redacted_at_the_mcp_boundary(search_returning_secrets): + out = _text(anyio.run(M.mcp.call_tool, "semantic_code_search", {"query": "settings"})) + assert "src/settings.py" in out # the result itself still arrives + assert FAKE_TOKEN not in out and "FAKE" not in out + assert '"[REDACTED:github-token]"' in out and "[REDACTED:private-key]" in out + + +def test_the_plain_function_is_not_redacted(search_returning_secrets): + """The TUI and the tests call the plain function; the index stays faithful.""" + out = M.semantic_code_search("settings") + assert FAKE_TOKEN in out and FAKE_PEM in out and "[REDACTED" not in out + + +@pytest.mark.parametrize("git_mode", [False, True]) +def test_both_instruction_branches_route_to_the_index_before_grep(tmp_path, monkeypatch, git_mode): + import index_location + monkeypatch.setattr(index_location, "git_ref", lambda path: "main" if git_mode else None) + monkeypatch.setattr(index_location, "ref_display", lambda path, ref: ref, raising=False) + text = M._server_instructions(str(tmp_path)) + assert ("read straight from git" in text) is git_mode + assert "before Grep" in text and "exact strings" in text + registered = {t.name for t in _registered()} + for name in ("semantic_code_search", "trace_data_flow", "what_writes", "analyze_blast_radius"): + assert f"`{name}`" in text and name in registered # names only tools that exist