From e0677b9929b39e574e8ecef3afdcaf78c4ca9197 Mon Sep 17 00:00:00 2001 From: Sutu Sebastian Date: Wed, 6 May 2026 12:15:51 +0300 Subject: [PATCH 1/9] =?UTF-8?q?feat(apply):=20slice=201=20=E2=80=94=20appl?= =?UTF-8?q?y-engine=20phase-1=20validation=20+=20dry-run?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pure transport-agnostic engine implementing phase-1 validation and the dry-run output shape from the merged plan (Q1–Q10 in docs/plans/codemap-apply.md). No CLI, no MCP/HTTP wiring, no write branch yet (Slice 2 lands the latter). Re-locks Q8 to substring-match (a) — the original "exact byte-match" draft contradicted the existing buildDiffJson formatter contract and would have made every shipped rename-preview row conflict (the recipe emits before_pattern = old_name as the bare identifier, not the full line). New phase-1 mirrors application/output-formatters.ts buildDiffJson verbatim: actual.includes(before) for the match check, first-occurrence substring replace for the transformation (Slice 2), $-pre-escape per GetSubstitution. Slice scope: - src/application/apply-engine.ts — applyDiffPayload({rows, projectRoot, dryRun}) returning Q5's ApplyJsonPayload envelope. dryRun=false with a clean phase 1 throws NotImplemented (Slice 2 fills in the write). - src/application/apply-engine.test.ts — 14 unit tests covering happy paths, all three conflict reasons, row-shape validation, deterministic files[] sort, and the Slice-2 guard semantics. - docs/plans/codemap-apply.md — Q8 re-lock + edge-case table refresh. Tests: 14/14 pass. Typecheck / lint / format clean. --- docs/plans/codemap-apply.md | 42 +-- src/application/apply-engine.test.ts | 383 +++++++++++++++++++++++++++ src/application/apply-engine.ts | 227 ++++++++++++++++ 3 files changed, 631 insertions(+), 21 deletions(-) create mode 100644 src/application/apply-engine.test.ts create mode 100644 src/application/apply-engine.ts diff --git a/docs/plans/codemap-apply.md b/docs/plans/codemap-apply.md index ed4db52..ad60b7a 100644 --- a/docs/plans/codemap-apply.md +++ b/docs/plans/codemap-apply.md @@ -224,40 +224,40 @@ Each gets a "Resolution" subsection below as it crystallises (mirrors the recipe - **Promotion path is additive.** If real users complain about reindex-then-retry friction, ship (b) as additional phase-1 metadata: `summary.already_applied: N` + per-file `rows_already_applied`. No shape break for existing consumers; existing fields remain accurate. - **Q8 — `before_pattern` matching semantics.** - - **(a) Exact match** — including whitespace; `disk[line_start..line_start + N] === before_pattern.split('\n')`. + - **(a) Substring match (single line)** — `disk[line_start - 1].includes(before_pattern)` and replace via `disk[line_start - 1].replace(before_pattern, after_pattern)` (first occurrence). Matches the existing `buildDiffJson` formatter contract. - **(b) Whitespace-tolerant** — collapse runs of whitespace before comparing. - - **(c) Multi-line `before_pattern` support** — folded into (a)'s phase-1 implementation; not a separate option. + - **(c) Whole-line exact match** — `disk[line_start - 1] === before_pattern`. ### Q8 Resolution - **Locked: (a) Exact match.** Multi-line support is (a)'s implementation detail, not a separate option. + **Locked: (a) Substring match (single line) — verbatim with the existing `buildDiffJson` formatter contract.** **Phase-1 algorithm:** - 1. Load file at `file_path` (1-indexed line array). - 2. Split `before_pattern` on `\n` → N lines. - 3. Compare `disk[line_start..line_start + N - 1]` line-by-line to those N lines, byte-exact. - 4. Match → row passes validation. - 5. Mismatch → conflict; `actual_at_line` is `disk[line_start..line_start + N - 1].join('\n')`. + 1. Load file at `file_path`; split into 1-indexed lines via `source.split(/\r?\n/)`. + 2. `actual = lines[line_start - 1]`. + 3. If file unreadable → conflict (`reason: "file missing"`). + 4. If `actual === undefined` (line past EOF) → conflict (`reason: "line out of range"`). + 5. If `!actual.includes(before_pattern)` → conflict (`reason: "line content drifted"`); `actual_at_line = actual`. + 6. Otherwise → row passes; phase-2 transformation is `actual.replace(before_pattern, after_pattern)` (first occurrence). Pre-escape `$` in `after_pattern` per `String.prototype.replace`'s `GetSubstitution` rule (`replace(/\$/g, "$$$$")` — mirrors `buildDiffJson` line 475). Reasoning: - - **Moat-A clean.** `before_pattern === disk` is observation; "close enough" is interpretation. Codemap does the former. - - **Predictable for agents.** A whitespace-tolerant comparator opens the door to "match if you can"; agents can't reason about the false-positive surface. - - **Recipe-author control.** Recipes already produce the rows — if normalization is wanted, the recipe SQL does it (e.g. `before_pattern` derived from `replace(source, char(9), ' ')`). - - **(c) folds into (a).** `before_pattern` is `TEXT` and accepts `\n` today; phase-1 just splits on `\n` and reads the corresponding N consecutive lines. No schema delta. Documented as a recipe-author idiom. - - **Reject (b).** Tolerance grows over time (collapse runs → trim → ignore EOL → ignore blank lines → …); each addition is more interpretation. Promotion path is recipe-side: a future recipe-frontmatter flag (`apply.whitespace_tolerant: true`) keeps the verdict where it belongs. + - **Matches the formatter contract verbatim.** `application/output-formatters.ts` `buildDiffJson` uses `actual.includes(before)` + `actual.replace(before, after)` with the `$` pre-escape. The exemplar `templates/recipes/rename-preview.sql` emits `before_pattern = old_name` (the bare identifier, e.g. `"foo"`), NOT the full line. Whole-line exact match would conflict every time on `const foo = 1;`. + - **Single line per row.** Hunks today are 1-line removals + 1-line additions (`old_count: 1, new_count: 1` in `buildDiffJson`). Multi-line transforms are out of scope for v1; promote with a schema-aware extension if real recipes need it. + - **Reject (b) whitespace-tolerant.** Tolerance grows over time (collapse runs → trim → ignore EOL → …); each addition is more interpretation. Recipe-side normalization is the right substrate (recipe SQL controls what `before_pattern` contains). + - **Reject (c) whole-line exact.** Contradicts the formatter precedent and breaks every shipped recipe row. **Edge cases handled by (a):** - | Disk state | `before_pattern` | Outcome | - | ---------------------------------- | ---------------------------- | ------------------------------------------------------------------------- | - | File missing | any | conflict; `reason: "file missing"` | - | `line_start` past EOF | any | conflict; `reason: "line out of range"` | - | Trailing whitespace differs | exact recipe text | conflict (correctly — file did drift) | - | EOL differs (`\r\n` vs `\n`) | recipe authored on `\n` host | conflict; user normalizes EOL or recipe accepts both via parameterisation | - | `before_pattern` has trailing `\n` | — | counted as N+1 lines; phase-1 reads N+1 lines (last is empty) | + | Disk state | `before_pattern` | Outcome | + | -------------------------------------- | ---------------- | ------------------------------------------------------------------------------------------------------------------------------- | + | File missing | any | conflict; `reason: "file missing"` | + | `line_start` past EOF | any | conflict; `reason: "line out of range"` | + | `before_pattern` not in line | any | conflict; `reason: "line content drifted"`; `actual_at_line` shows what's there | + | `before_pattern` appears twice in line | any | only the first occurrence replaces (matches formatter); recipe author should split into two rows or use a more specific pattern | + | `after_pattern` contains `$` / `$1` | any | escaped before `replace()` so identifiers like `$inject` round-trip safely | - **Q9 — Test approach.** - - **Unit:** `apply-engine.ts` — `applyDiffPayload({rows, projectRoot, dryRun})` + helpers. Temp-dir scenarios for happy path / conflict / dry-run / file-missing / out-of-range / multi-line `before_pattern`. + - **Unit:** `apply-engine.ts` — `applyDiffPayload({rows, projectRoot, dryRun})` + helpers. Temp-dir scenarios for happy path / conflict / dry-run / file-missing / out-of-range / `$`-pattern escape / row-shape validation. - **Integration:** subprocess CLI tests against a fixture project (mirrors `cmd-query-recency.test.ts`) — full pipeline from `codemap apply rename-preview --params … --yes` through to disk-state assertions. TTY-prompt path tested via `--yes` flag (skipping prompt); non-TTY-no-`--yes` rejection tested explicitly. - **Golden:** add a recipe-execution scenario to `fixtures/golden/scenarios.json` covering the dry-run output shape (deterministic; doesn't write to disk). diff --git a/src/application/apply-engine.test.ts b/src/application/apply-engine.test.ts new file mode 100644 index 0000000..801485d --- /dev/null +++ b/src/application/apply-engine.test.ts @@ -0,0 +1,383 @@ +import { describe, expect, it } from "bun:test"; +import { mkdtempSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { applyDiffPayload } from "./apply-engine"; +import type { ApplyJsonPayload } from "./apply-engine"; + +function tmpProject(): string { + return mkdtempSync(join(tmpdir(), "codemap-apply-test-")); +} + +function writeSource(root: string, relPath: string, content: string): void { + writeFileSync(join(root, relPath), content, "utf8"); +} + +describe("applyDiffPayload — phase 1 + dry-run (Slice 1)", () => { + describe("happy paths", () => { + it("returns a clean dry-run envelope for a single-row valid input", () => { + const root = tmpProject(); + writeSource(root, "a.ts", "const foo = 1;\n"); + + const result = applyDiffPayload({ + rows: [ + { + file_path: "a.ts", + line_start: 1, + before_pattern: "foo", + after_pattern: "bar", + }, + ], + projectRoot: root, + dryRun: true, + }); + + expect(result.mode).toBe("dry-run"); + expect(result.applied).toBe(false); + expect(result.conflicts).toEqual([]); + expect(result.files).toEqual([{ file_path: "a.ts", rows_applied: 0 }]); + expect(result.summary).toEqual({ + files: 1, + files_modified: 0, + rows: 1, + rows_applied: 0, + conflicts: 0, + files_with_conflicts: 0, + }); + }); + + it("collapses N rows targeting the same file into one ApplyFile entry", () => { + const root = tmpProject(); + writeSource(root, "a.ts", "const foo = 1;\nconst foo2 = 2;\n"); + + const result = applyDiffPayload({ + rows: [ + { + file_path: "a.ts", + line_start: 1, + before_pattern: "foo", + after_pattern: "bar", + }, + { + file_path: "a.ts", + line_start: 2, + before_pattern: "foo2", + after_pattern: "bar2", + }, + ], + projectRoot: root, + dryRun: true, + }); + + expect(result.files).toEqual([{ file_path: "a.ts", rows_applied: 0 }]); + expect(result.summary.files).toBe(1); + expect(result.summary.rows).toBe(2); + }); + + it("sorts files[] by path for deterministic envelope output", () => { + const root = tmpProject(); + writeSource(root, "z.ts", "const foo = 1;\n"); + writeSource(root, "a.ts", "const foo = 1;\n"); + writeSource(root, "m.ts", "const foo = 1;\n"); + + const result = applyDiffPayload({ + rows: ["z.ts", "a.ts", "m.ts"].map((file_path) => ({ + file_path, + line_start: 1, + before_pattern: "foo", + after_pattern: "bar", + })), + projectRoot: root, + dryRun: true, + }); + + expect(result.files.map((f) => f.file_path)).toEqual([ + "a.ts", + "m.ts", + "z.ts", + ]); + }); + + it("matches substrings inside lines (not whole-line exact) — mirrors buildDiffJson", () => { + const root = tmpProject(); + writeSource(root, "a.ts", " const foo = 1;\n"); + + const result = applyDiffPayload({ + rows: [ + { + file_path: "a.ts", + line_start: 1, + before_pattern: "foo", + after_pattern: "bar", + }, + ], + projectRoot: root, + dryRun: true, + }); + + expect(result.conflicts).toEqual([]); + expect(result.files).toHaveLength(1); + }); + }); + + describe("conflict reporting (Q3 scan-and-collect)", () => { + it("reports `file missing` when the path doesn't exist", () => { + const root = tmpProject(); + + const result = applyDiffPayload({ + rows: [ + { + file_path: "ghost.ts", + line_start: 1, + before_pattern: "foo", + after_pattern: "bar", + }, + ], + projectRoot: root, + dryRun: true, + }); + + expect(result.conflicts).toEqual([ + { + file_path: "ghost.ts", + line_start: 1, + before_pattern: "foo", + actual_at_line: "", + reason: "file missing", + }, + ]); + expect(result.files).toEqual([]); + expect(result.summary.conflicts).toBe(1); + expect(result.summary.files_with_conflicts).toBe(1); + }); + + it("reports `line out of range` when line_start exceeds EOF", () => { + const root = tmpProject(); + writeSource(root, "a.ts", "const foo = 1;\n"); + + const result = applyDiffPayload({ + rows: [ + { + file_path: "a.ts", + line_start: 99, + before_pattern: "foo", + after_pattern: "bar", + }, + ], + projectRoot: root, + dryRun: true, + }); + + expect(result.conflicts[0]).toMatchObject({ + file_path: "a.ts", + line_start: 99, + reason: "line out of range", + actual_at_line: "", + }); + }); + + it("reports `line content drifted` with the actual disk content", () => { + const root = tmpProject(); + writeSource(root, "a.ts", "const bar = 1;\n"); + + const result = applyDiffPayload({ + rows: [ + { + file_path: "a.ts", + line_start: 1, + before_pattern: "foo", + after_pattern: "baz", + }, + ], + projectRoot: root, + dryRun: true, + }); + + expect(result.conflicts[0]).toEqual({ + file_path: "a.ts", + line_start: 1, + before_pattern: "foo", + actual_at_line: "const bar = 1;", + reason: "line content drifted", + }); + }); + + it("collects ALL conflicts (Q3 scan-and-collect, not fail-fast)", () => { + const root = tmpProject(); + writeSource(root, "a.ts", "const a = 1;\n"); + writeSource(root, "b.ts", "const b = 2;\n"); + + const result = applyDiffPayload({ + rows: [ + { + file_path: "a.ts", + line_start: 1, + before_pattern: "FOO", + after_pattern: "X", + }, + { + file_path: "b.ts", + line_start: 1, + before_pattern: "BAR", + after_pattern: "Y", + }, + { + file_path: "ghost.ts", + line_start: 1, + before_pattern: "BAZ", + after_pattern: "Z", + }, + ], + projectRoot: root, + dryRun: true, + }); + + expect(result.conflicts).toHaveLength(3); + expect(result.summary.files_with_conflicts).toBe(3); + // Q2 (c) — any conflict aborts the run; nothing reported as applicable. + expect(result.files).toEqual([]); + }); + + it("clears files[] entirely when ANY row conflicts (Q2 (c) all-or-nothing)", () => { + const root = tmpProject(); + writeSource(root, "good.ts", "const foo = 1;\n"); + // bad.ts intentionally missing. + + const result = applyDiffPayload({ + rows: [ + { + file_path: "good.ts", + line_start: 1, + before_pattern: "foo", + after_pattern: "bar", + }, + { + file_path: "bad.ts", + line_start: 1, + before_pattern: "foo", + after_pattern: "bar", + }, + ], + projectRoot: root, + dryRun: true, + }); + + expect(result.conflicts).toHaveLength(1); + expect(result.files).toEqual([]); + }); + }); + + describe("row-shape validation", () => { + it("silently skips rows missing required keys (mirrors buildDiffJson)", () => { + const root = tmpProject(); + writeSource(root, "a.ts", "const foo = 1;\n"); + + const result = applyDiffPayload({ + rows: [ + { + file_path: "a.ts", + line_start: 1, + before_pattern: "foo", + after_pattern: "bar", + }, + { file_path: "a.ts", line_start: 1, before_pattern: "foo" }, // missing after + { + file_path: "a.ts", + line_start: 0, + before_pattern: "foo", + after_pattern: "bar", + }, // line_start not positive + { line_start: 1, before_pattern: "foo", after_pattern: "bar" }, // missing file_path + ] as Record[], + projectRoot: root, + dryRun: true, + }); + + expect(result.summary.rows).toBe(1); + expect(result.conflicts).toEqual([]); + }); + + it("returns the empty envelope for an empty row set", () => { + const root = tmpProject(); + + const result: ApplyJsonPayload = applyDiffPayload({ + rows: [], + projectRoot: root, + dryRun: true, + }); + + expect(result).toEqual({ + mode: "dry-run", + applied: false, + files: [], + conflicts: [], + summary: { + files: 0, + files_modified: 0, + rows: 0, + rows_applied: 0, + conflicts: 0, + files_with_conflicts: 0, + }, + }); + }); + }); + + describe("apply-mode (Slice 1 — guard until Slice 2 lands)", () => { + it("throws a clear NotImplemented error when dryRun=false has clean phase 1", () => { + const root = tmpProject(); + writeSource(root, "a.ts", "const foo = 1;\n"); + + expect(() => + applyDiffPayload({ + rows: [ + { + file_path: "a.ts", + line_start: 1, + before_pattern: "foo", + after_pattern: "bar", + }, + ], + projectRoot: root, + dryRun: false, + }), + ).toThrow(/Slice 2/); + }); + + it("does NOT throw on dryRun=false when conflicts abort phase 2", () => { + const root = tmpProject(); + writeSource(root, "a.ts", "const foo = 1;\n"); + + const result = applyDiffPayload({ + rows: [ + { + file_path: "a.ts", + line_start: 1, + before_pattern: "MISSING", + after_pattern: "X", + }, + ], + projectRoot: root, + dryRun: false, + }); + + expect(result.mode).toBe("apply"); + expect(result.applied).toBe(false); + expect(result.conflicts).toHaveLength(1); + }); + + it("does NOT throw on dryRun=false when no rows are applicable", () => { + const root = tmpProject(); + + const result = applyDiffPayload({ + rows: [], + projectRoot: root, + dryRun: false, + }); + + expect(result.mode).toBe("apply"); + expect(result.summary.rows).toBe(0); + }); + }); +}); diff --git a/src/application/apply-engine.ts b/src/application/apply-engine.ts new file mode 100644 index 0000000..008e729 --- /dev/null +++ b/src/application/apply-engine.ts @@ -0,0 +1,227 @@ +/** + * Pure transport-agnostic engine for `codemap apply ` — substrate- + * shaped fix executor. Consumes the same `{file_path, line_start, + * before_pattern, after_pattern}` row contract `buildDiffJson` emits and + * either previews (dry-run) or applies (Slice 2) the edits to disk. + * + * Slice 1 ships phase-1 validation + dry-run output only; the write branch + * (phase 2) lands in Slice 2 with atomic temp-rename. See + * [`docs/plans/codemap-apply.md`](../../docs/plans/codemap-apply.md) for the + * full design (Q1–Q10 locked). + * + * TOCTOU note: phase-1 reads disk, phase-2 writes disk. The window between + * is a deliberate v1 simplification (Q2) — apply isn't an adversarial verb, + * so we don't add lock-file machinery. + */ + +import { readFileSync } from "node:fs"; +import { join } from "node:path"; + +/** Required input columns on every recipe row. */ +export interface ApplyInputRow extends Record { + file_path: string; + line_start: number; + before_pattern: string; + after_pattern: string; +} + +export interface ApplyDiffPayloadOpts { + /** Recipe / SQL row set. Rows missing required keys are silently skipped. */ + rows: Record[]; + /** Absolute or process-cwd-relative root used when reading `file_path` from disk. */ + projectRoot: string; + /** `true` previews; `false` writes (Slice 2). */ + dryRun: boolean; +} + +/** + * Per-file roll-up. `rows_applied` is `0` in dry-run (Slice 1) and `0` in + * apply mode when phase 1 collected any conflicts (Q2 (c) — all-or-nothing + * per recipe-run aborts before any writes). + */ +export interface ApplyFile { + file_path: string; + rows_applied: number; + warnings?: string[]; +} + +/** One conflict emitted by phase-1 validation. */ +export interface ConflictRow { + file_path: string; + line_start: number; + before_pattern: string; + /** Disk content at `line_start` (1-indexed); `""` if file missing / line out of range. */ + actual_at_line: string; + /** Human-readable reason; one of the fixed strings below. */ + reason: ConflictReason; +} + +export type ConflictReason = + | "file missing" + | "line out of range" + | "line content drifted"; + +/** Q5 envelope shape — single shape across `dry-run` and `apply` modes. */ +export interface ApplyJsonPayload { + mode: "dry-run" | "apply"; + /** `true` only when `mode === "apply"` AND zero conflicts AND at least one row applied. */ + applied: boolean; + files: ApplyFile[]; + conflicts: ConflictRow[]; + summary: { + /** Distinct `file_path`s in the input row set. */ + files: number; + /** `0` in dry-run; equals `files[].length` post-apply when conflict-free. */ + files_modified: number; + /** Total well-shaped input rows (rows missing required keys are skipped). */ + rows: number; + /** `0` in dry-run; sum of `files[].rows_applied` post-apply. */ + rows_applied: number; + conflicts: number; + /** Distinct `file_path`s with at least one conflict. */ + files_with_conflicts: number; + }; +} + +/** Internal — one validated edit collected during phase 1. */ +interface PendingEdit { + line_start: number; + before_pattern: string; + after_pattern: string; +} + +/** + * Run the apply pipeline. Always runs phase 1 (validation); phase 2 (write) + * is gated on `!dryRun && conflicts.length === 0` and lands in Slice 2. + */ +export function applyDiffPayload(opts: ApplyDiffPayloadOpts): ApplyJsonPayload { + const { rows, projectRoot, dryRun } = opts; + + // Phase 1: validate every row against current disk state. + const conflicts: ConflictRow[] = []; + const pending = new Map(); + let validRows = 0; + + for (const row of rows) { + const filePath = readString(row, "file_path"); + const lineStart = readPositiveInt(row, "line_start"); + const before = readString(row, "before_pattern"); + const after = readString(row, "after_pattern"); + if ( + filePath === undefined || + lineStart === undefined || + before === undefined || + after === undefined + ) { + continue; + } + validRows++; + + let source: string; + try { + source = readFileSync(join(projectRoot, filePath), "utf8"); + } catch { + conflicts.push({ + file_path: filePath, + line_start: lineStart, + before_pattern: before, + actual_at_line: "", + reason: "file missing", + }); + continue; + } + const lines = source.split(/\r?\n/); + const actual = lines[lineStart - 1]; + if (actual === undefined) { + conflicts.push({ + file_path: filePath, + line_start: lineStart, + before_pattern: before, + actual_at_line: "", + reason: "line out of range", + }); + continue; + } + if (!actual.includes(before)) { + conflicts.push({ + file_path: filePath, + line_start: lineStart, + before_pattern: before, + actual_at_line: actual, + reason: "line content drifted", + }); + continue; + } + + const edits = pending.get(filePath); + if (edits === undefined) { + pending.set(filePath, [ + { line_start: lineStart, before_pattern: before, after_pattern: after }, + ]); + } else { + edits.push({ + line_start: lineStart, + before_pattern: before, + after_pattern: after, + }); + } + } + + // Phase 2 (write) lands in Slice 2. Every code path below is dry-run-shaped. + if (!dryRun && conflicts.length === 0 && pending.size > 0) { + throw new Error( + "apply-engine: write path lands in Slice 2 — pass dryRun: true for now.", + ); + } + + // Q2 (c) — any conflict aborts the whole run; `files[]` shows nothing + // applied. Dry-run mirrors that shape so the envelope is the same across + // modes. + const files: ApplyFile[] = + conflicts.length === 0 + ? [...pending.keys()].sort().map((file_path) => ({ + file_path, + rows_applied: 0, + })) + : []; + + const filesWithConflicts = new Set(conflicts.map((c) => c.file_path)).size; + const distinctInputFiles = new Set(); + for (const row of rows) { + const filePath = readString(row, "file_path"); + if (filePath !== undefined) distinctInputFiles.add(filePath); + } + + return { + mode: dryRun ? "dry-run" : "apply", + applied: false, + files, + conflicts, + summary: { + files: distinctInputFiles.size, + files_modified: 0, + rows: validRows, + rows_applied: 0, + conflicts: conflicts.length, + files_with_conflicts: filesWithConflicts, + }, + }; +} + +function readString( + row: Record, + key: string, +): string | undefined { + const value = row[key]; + return typeof value === "string" && value.length > 0 ? value : undefined; +} + +function readPositiveInt( + row: Record, + key: string, +): number | undefined { + const value = row[key]; + return Number.isInteger(value) && typeof value === "number" && value > 0 + ? value + : undefined; +} From 564ba325f184999951c5d42e4070594203692930 Mon Sep 17 00:00:00 2001 From: Sutu Sebastian Date: Wed, 6 May 2026 12:20:53 +0300 Subject: [PATCH 2/9] =?UTF-8?q?feat(apply):=20slice=202=20=E2=80=94=20phas?= =?UTF-8?q?e=202=20writes=20via=20temp=20+=20rename?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 2 lands behind the `!dryRun && conflicts.length === 0` gate per Q2 (c). Each modified file is written to a sibling temp path then `rename`d into place — POSIX-atomic per file, so concurrent readers see either the pre- or post-rename content, never a torn write. Implementation: - Phase-1 caches each source's text; phase-2 reuses the cache (one read per file across both phases). TOCTOU window collapses to the gap between phase-1 read and phase-2 rename — accepted per Q2. - Phase-2 splits on raw "\n" (not /\r?\n/) so CRLF lines retain their trailing \r and round-trip when joined back with "\n". Phase-1 conflict reporting still strips the \r so `actual_at_line` is clean. - Edits applied per-file in descending line order — defensive default for when multi-line transforms land (today's single-line rows are order-independent). - `$`-pre-escape on `after_pattern` per GetSubstitution rule (mirrors buildDiffJson) so identifiers like `$inject` round-trip safely. - Temp paths use `crypto.randomBytes(6)` so concurrent applies don't collide; cleanup on success is implicit (rename atomically removes the source name). Tests: 20/20 pass. Failure-mode coverage: chmod 0o555 on the project dir to force the temp-write to fail; dry-run no-op-on-disk; no temp siblings left behind on success; conflict short-circuits before any writes (good.ts untouched when bad.ts is missing). --- src/application/apply-engine.test.ts | 246 ++++++++++++++++++++++++--- src/application/apply-engine.ts | 171 +++++++++++++------ 2 files changed, 340 insertions(+), 77 deletions(-) diff --git a/src/application/apply-engine.test.ts b/src/application/apply-engine.test.ts index 801485d..d9af892 100644 --- a/src/application/apply-engine.test.ts +++ b/src/application/apply-engine.test.ts @@ -1,7 +1,14 @@ import { describe, expect, it } from "bun:test"; -import { mkdtempSync, writeFileSync } from "node:fs"; +import { + chmodSync, + mkdirSync, + mkdtempSync, + readFileSync, + readdirSync, + writeFileSync, +} from "node:fs"; import { tmpdir } from "node:os"; -import { join } from "node:path"; +import { dirname, join } from "node:path"; import { applyDiffPayload } from "./apply-engine"; import type { ApplyJsonPayload } from "./apply-engine"; @@ -11,10 +18,16 @@ function tmpProject(): string { } function writeSource(root: string, relPath: string, content: string): void { - writeFileSync(join(root, relPath), content, "utf8"); + const absPath = join(root, relPath); + mkdirSync(dirname(absPath), { recursive: true }); + writeFileSync(absPath, content, "utf8"); } -describe("applyDiffPayload — phase 1 + dry-run (Slice 1)", () => { +function readSource(root: string, relPath: string): string { + return readFileSync(join(root, relPath), "utf8"); +} + +describe("applyDiffPayload", () => { describe("happy paths", () => { it("returns a clean dry-run envelope for a single-row valid input", () => { const root = tmpProject(); @@ -324,38 +337,141 @@ describe("applyDiffPayload — phase 1 + dry-run (Slice 1)", () => { }); }); - describe("apply-mode (Slice 1 — guard until Slice 2 lands)", () => { - it("throws a clear NotImplemented error when dryRun=false has clean phase 1", () => { + describe("apply-mode (phase-2 writes)", () => { + it("writes the transformed content to disk and reports applied=true", () => { const root = tmpProject(); writeSource(root, "a.ts", "const foo = 1;\n"); - expect(() => - applyDiffPayload({ - rows: [ - { - file_path: "a.ts", - line_start: 1, - before_pattern: "foo", - after_pattern: "bar", - }, - ], - projectRoot: root, - dryRun: false, - }), - ).toThrow(/Slice 2/); + const result = applyDiffPayload({ + rows: [ + { + file_path: "a.ts", + line_start: 1, + before_pattern: "foo", + after_pattern: "bar", + }, + ], + projectRoot: root, + dryRun: false, + }); + + expect(result.mode).toBe("apply"); + expect(result.applied).toBe(true); + expect(result.files).toEqual([{ file_path: "a.ts", rows_applied: 1 }]); + expect(result.summary).toEqual({ + files: 1, + files_modified: 1, + rows: 1, + rows_applied: 1, + conflicts: 0, + files_with_conflicts: 0, + }); + expect(readSource(root, "a.ts")).toBe("const bar = 1;\n"); }); - it("does NOT throw on dryRun=false when conflicts abort phase 2", () => { + it("applies multiple rows to the same file in descending line order", () => { const root = tmpProject(); - writeSource(root, "a.ts", "const foo = 1;\n"); + writeSource( + root, + "a.ts", + "const foo = 1;\nconst bar = 2;\nconst baz = 3;\n", + ); const result = applyDiffPayload({ rows: [ { file_path: "a.ts", line_start: 1, - before_pattern: "MISSING", - after_pattern: "X", + before_pattern: "foo", + after_pattern: "ONE", + }, + { + file_path: "a.ts", + line_start: 2, + before_pattern: "bar", + after_pattern: "TWO", + }, + { + file_path: "a.ts", + line_start: 3, + before_pattern: "baz", + after_pattern: "THREE", + }, + ], + projectRoot: root, + dryRun: false, + }); + + expect(result.applied).toBe(true); + expect(result.files[0]?.rows_applied).toBe(3); + expect(readSource(root, "a.ts")).toBe( + "const ONE = 1;\nconst TWO = 2;\nconst THREE = 3;\n", + ); + }); + + it("preserves CRLF line endings via raw `\\n` split + join", () => { + const root = tmpProject(); + writeSource(root, "a.ts", "const foo = 1;\r\nconst bar = 2;\r\n"); + + const result = applyDiffPayload({ + rows: [ + { + file_path: "a.ts", + line_start: 1, + before_pattern: "foo", + after_pattern: "FOO", + }, + ], + projectRoot: root, + dryRun: false, + }); + + expect(result.applied).toBe(true); + expect(readSource(root, "a.ts")).toBe( + "const FOO = 1;\r\nconst bar = 2;\r\n", + ); + }); + + it("escapes `$` in after_pattern per GetSubstitution (mirrors buildDiffJson)", () => { + const root = tmpProject(); + writeSource(root, "a.ts", "const inject = 1;\n"); + + const result = applyDiffPayload({ + rows: [ + { + file_path: "a.ts", + line_start: 1, + before_pattern: "inject", + after_pattern: "$inject", + }, + ], + projectRoot: root, + dryRun: false, + }); + + expect(result.applied).toBe(true); + expect(readSource(root, "a.ts")).toBe("const $inject = 1;\n"); + }); + + it("does NOT write any file when conflicts abort phase 2 (Q2 (c))", () => { + const root = tmpProject(); + writeSource(root, "good.ts", "const foo = 1;\n"); + // bad.ts intentionally missing. + const goodBefore = readSource(root, "good.ts"); + + const result = applyDiffPayload({ + rows: [ + { + file_path: "good.ts", + line_start: 1, + before_pattern: "foo", + after_pattern: "bar", + }, + { + file_path: "bad.ts", + line_start: 1, + before_pattern: "foo", + after_pattern: "bar", }, ], projectRoot: root, @@ -365,9 +481,11 @@ describe("applyDiffPayload — phase 1 + dry-run (Slice 1)", () => { expect(result.mode).toBe("apply"); expect(result.applied).toBe(false); expect(result.conflicts).toHaveLength(1); + // good.ts must be untouched even though its row would have validated. + expect(readSource(root, "good.ts")).toBe(goodBefore); }); - it("does NOT throw on dryRun=false when no rows are applicable", () => { + it("returns applied=false / mode=apply when no rows are applicable", () => { const root = tmpProject(); const result = applyDiffPayload({ @@ -377,7 +495,83 @@ describe("applyDiffPayload — phase 1 + dry-run (Slice 1)", () => { }); expect(result.mode).toBe("apply"); - expect(result.summary.rows).toBe(0); + expect(result.applied).toBe(false); + expect(result.files).toEqual([]); + expect(result.summary.rows_applied).toBe(0); + }); + + it("does NOT leave behind any `*.codemap-apply-*.tmp` siblings", () => { + const root = tmpProject(); + writeSource(root, "a.ts", "const foo = 1;\n"); + + applyDiffPayload({ + rows: [ + { + file_path: "a.ts", + line_start: 1, + before_pattern: "foo", + after_pattern: "bar", + }, + ], + projectRoot: root, + dryRun: false, + }); + + const siblings = readdirSync(root); + expect(siblings.some((s) => s.includes("codemap-apply-"))).toBe(false); + }); + }); + + describe("failure modes", () => { + it("propagates the writeFileSync error when the file is read-only (chmod 0o444)", () => { + const root = tmpProject(); + writeSource(root, "a.ts", "const foo = 1;\n"); + // Chmod the directory to read-only so the temp-file write fails. + // (The file itself can still be `rename`d to, but writeFileSync of + // the temp sibling needs write permission on the parent dir.) + chmodSync(root, 0o555); + + try { + expect(() => + applyDiffPayload({ + rows: [ + { + file_path: "a.ts", + line_start: 1, + before_pattern: "foo", + after_pattern: "bar", + }, + ], + projectRoot: root, + dryRun: false, + }), + ).toThrow(); + } finally { + chmodSync(root, 0o755); + } + }); + + it("is a no-op on disk when dry-run is requested even with valid rows", () => { + const root = tmpProject(); + writeSource(root, "a.ts", "const foo = 1;\n"); + const before = readSource(root, "a.ts"); + + const result = applyDiffPayload({ + rows: [ + { + file_path: "a.ts", + line_start: 1, + before_pattern: "foo", + after_pattern: "bar", + }, + ], + projectRoot: root, + dryRun: true, + }); + + expect(result.mode).toBe("dry-run"); + expect(result.files).toEqual([{ file_path: "a.ts", rows_applied: 0 }]); + expect(readSource(root, "a.ts")).toBe(before); }); }); }); diff --git a/src/application/apply-engine.ts b/src/application/apply-engine.ts index 008e729..98900d5 100644 --- a/src/application/apply-engine.ts +++ b/src/application/apply-engine.ts @@ -2,19 +2,27 @@ * Pure transport-agnostic engine for `codemap apply ` — substrate- * shaped fix executor. Consumes the same `{file_path, line_start, * before_pattern, after_pattern}` row contract `buildDiffJson` emits and - * either previews (dry-run) or applies (Slice 2) the edits to disk. + * either previews (dry-run) or applies the edits to disk. * - * Slice 1 ships phase-1 validation + dry-run output only; the write branch - * (phase 2) lands in Slice 2 with atomic temp-rename. See + * Phase 1 validates every row against current disk state; phase 2 (gated on + * `!dryRun && conflicts.length === 0`) writes each modified file via temp + + * rename for crash-safe per-file atomicity. See * [`docs/plans/codemap-apply.md`](../../docs/plans/codemap-apply.md) for the * full design (Q1–Q10 locked). * - * TOCTOU note: phase-1 reads disk, phase-2 writes disk. The window between - * is a deliberate v1 simplification (Q2) — apply isn't an adversarial verb, - * so we don't add lock-file machinery. + * TOCTOU note: phase-1 caches the source it validated; phase-2 transforms + * the cached source and writes the result. The window between read and + * write is a deliberate v1 simplification (Q2) — apply isn't an adversarial + * verb, so we don't add lock-file machinery. + * + * EOL note: phase-2 splits source on raw `"\n"` (NOT `/\r?\n/`) so CRLF + * lines retain their trailing `\r` and round-trip when joined back with + * `"\n"`. Patterns that include a literal `\r` are out of scope for v1; + * recipe authors should target identifier-shaped patterns. */ -import { readFileSync } from "node:fs"; +import { randomBytes } from "node:crypto"; +import { readFileSync, renameSync, writeFileSync } from "node:fs"; import { join } from "node:path"; /** Required input columns on every recipe row. */ @@ -30,14 +38,14 @@ export interface ApplyDiffPayloadOpts { rows: Record[]; /** Absolute or process-cwd-relative root used when reading `file_path` from disk. */ projectRoot: string; - /** `true` previews; `false` writes (Slice 2). */ + /** `true` previews; `false` writes (gated on a clean phase 1). */ dryRun: boolean; } /** - * Per-file roll-up. `rows_applied` is `0` in dry-run (Slice 1) and `0` in - * apply mode when phase 1 collected any conflicts (Q2 (c) — all-or-nothing - * per recipe-run aborts before any writes). + * Per-file roll-up. `rows_applied` is `0` in dry-run mode and `0` in apply + * mode when phase 1 collected any conflicts (Q2 (c) — all-or-nothing per + * recipe-run aborts before any writes). */ export interface ApplyFile { file_path: string; @@ -92,14 +100,15 @@ interface PendingEdit { /** * Run the apply pipeline. Always runs phase 1 (validation); phase 2 (write) - * is gated on `!dryRun && conflicts.length === 0` and lands in Slice 2. + * is gated on `!dryRun && conflicts.length === 0`. */ export function applyDiffPayload(opts: ApplyDiffPayloadOpts): ApplyJsonPayload { const { rows, projectRoot, dryRun } = opts; - // Phase 1: validate every row against current disk state. const conflicts: ConflictRow[] = []; const pending = new Map(); + // Phase-1 reads each file at most once; phase 2 reuses the cached source. + const sourceCache = new Map(); let validRows = 0; for (const row of rows) { @@ -117,19 +126,25 @@ export function applyDiffPayload(opts: ApplyDiffPayloadOpts): ApplyJsonPayload { } validRows++; - let source: string; - try { - source = readFileSync(join(projectRoot, filePath), "utf8"); - } catch { - conflicts.push({ - file_path: filePath, - line_start: lineStart, - before_pattern: before, - actual_at_line: "", - reason: "file missing", - }); - continue; + let source = sourceCache.get(filePath); + if (source === undefined) { + try { + source = readFileSync(join(projectRoot, filePath), "utf8"); + } catch { + conflicts.push({ + file_path: filePath, + line_start: lineStart, + before_pattern: before, + actual_at_line: "", + reason: "file missing", + }); + continue; + } + sourceCache.set(filePath, source); } + // Phase-1 splits on `/\r?\n/` so the `actual_at_line` reported in + // conflicts doesn't carry a stray `\r`. Phase-2 re-splits on raw `\n` + // for round-trip-safe writes (see EOL note in module docstring). const lines = source.split(/\r?\n/); const actual = lines[lineStart - 1]; if (actual === undefined) { @@ -167,24 +182,6 @@ export function applyDiffPayload(opts: ApplyDiffPayloadOpts): ApplyJsonPayload { } } - // Phase 2 (write) lands in Slice 2. Every code path below is dry-run-shaped. - if (!dryRun && conflicts.length === 0 && pending.size > 0) { - throw new Error( - "apply-engine: write path lands in Slice 2 — pass dryRun: true for now.", - ); - } - - // Q2 (c) — any conflict aborts the whole run; `files[]` shows nothing - // applied. Dry-run mirrors that shape so the envelope is the same across - // modes. - const files: ApplyFile[] = - conflicts.length === 0 - ? [...pending.keys()].sort().map((file_path) => ({ - file_path, - rows_applied: 0, - })) - : []; - const filesWithConflicts = new Set(conflicts.map((c) => c.file_path)).size; const distinctInputFiles = new Set(); for (const row of rows) { @@ -192,18 +189,90 @@ export function applyDiffPayload(opts: ApplyDiffPayloadOpts): ApplyJsonPayload { if (filePath !== undefined) distinctInputFiles.add(filePath); } + // Q2 (c) — any conflict aborts the whole run; dry-run never writes either. + // Both branches return the same envelope shape (Q5). + if (dryRun || conflicts.length > 0) { + const files: ApplyFile[] = + conflicts.length === 0 + ? [...pending.keys()].sort().map((file_path) => ({ + file_path, + rows_applied: 0, + })) + : []; + return { + mode: dryRun ? "dry-run" : "apply", + applied: false, + files, + conflicts, + summary: { + files: distinctInputFiles.size, + files_modified: 0, + rows: validRows, + rows_applied: 0, + conflicts: conflicts.length, + files_with_conflicts: filesWithConflicts, + }, + }; + } + + // Phase 2 — apply edits per-file via temp + rename. + const writtenFiles: ApplyFile[] = []; + let appliedRows = 0; + for (const filePath of [...pending.keys()].sort()) { + const edits = pending.get(filePath)!; + const cachedSource = sourceCache.get(filePath)!; + // Re-split the cached source on raw `"\n"` (not `/\r?\n/`) so CRLF + // lines retain their trailing `\r` and rejoin losslessly. + const fileLines = cachedSource.split("\n"); + // Apply edits in descending line order — defensive default for when + // multi-line transforms land (today every row is a single-line + // replacement so order doesn't actually matter). + edits.sort((a, b) => b.line_start - a.line_start); + for (const edit of edits) { + const idx = edit.line_start - 1; + const actual = fileLines[idx]; + // The cached source was validated in phase 1; re-checking here guards + // against future regressions where the cache and validation drift. + if (actual === undefined || !actual.includes(edit.before_pattern)) { + throw new Error( + `apply-engine: phase-2 invariant violated at ${filePath}:${edit.line_start} — phase-1 cache out of sync.`, + ); + } + // Pre-escape `$` per `String.prototype.replace` GetSubstitution rule + // so identifiers like `$inject` round-trip safely (mirrors + // `buildDiffJson`). + fileLines[idx] = actual.replace( + edit.before_pattern, + edit.after_pattern.replace(/\$/g, "$$$$"), + ); + } + + const newContent = fileLines.join("\n"); + const absPath = join(projectRoot, filePath); + // Sibling temp + `rename`: POSIX-atomic when source and destination + // share a filesystem (always true for siblings). A concurrent reader + // sees either the pre-rename or post-rename content — never a torn + // write. + const tempPath = `${absPath}.codemap-apply-${randomBytes(6).toString("hex")}.tmp`; + writeFileSync(tempPath, newContent, "utf8"); + renameSync(tempPath, absPath); + + writtenFiles.push({ file_path: filePath, rows_applied: edits.length }); + appliedRows += edits.length; + } + return { - mode: dryRun ? "dry-run" : "apply", - applied: false, - files, - conflicts, + mode: "apply", + applied: writtenFiles.length > 0, + files: writtenFiles, + conflicts: [], summary: { files: distinctInputFiles.size, - files_modified: 0, + files_modified: writtenFiles.length, rows: validRows, - rows_applied: 0, - conflicts: conflicts.length, - files_with_conflicts: filesWithConflicts, + rows_applied: appliedRows, + conflicts: 0, + files_with_conflicts: 0, }, }; } From a24f715afc00f0df7268f26c80a8457d7bc1118b Mon Sep 17 00:00:00 2001 From: Sutu Sebastian Date: Wed, 6 May 2026 12:28:42 +0300 Subject: [PATCH 3/9] =?UTF-8?q?feat(apply):=20slice=203=20=E2=80=94=20CLI?= =?UTF-8?q?=20verb=20+=20recipe=20execution=20+=20TTY/--yes=20gate?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds `codemap apply ` as a positional verb (per Q4) wired through the same dispatch as every other CLI command. Recipe execution reuses `queryRows` + the existing `--params` plumbing (`parseParamsCli` + `resolveRecipeParams`); rows feed straight into `applyDiffPayload`. Q6 gating matrix implemented: - TTY no `--yes` → phase-1 dry-run preview, prompt `Proceed? [y/N]` on stderr, default-N, phase-2 only on `y` (uses node:readline). - TTY `--yes` → no prompt; proceed if validation clean. - Non-TTY no `--yes` (no `--dry-run`) → reject with stderr message ("Pass --yes for non-interactive runs, or --dry-run for preview."). - `--dry-run` + `--yes` → mutually exclusive, parse-time error. - `--json` everywhere routes errors as `{"error":"..."}` envelopes. Files: - src/cli/cmd-apply.ts — argv parser + run loop. Mirrors cmd-impact's shape (positional + flags + JSON envelope). - src/cli/cmd-apply.test.ts — 10 subprocess integration tests: dry-run no-op, --yes happy path (with cross-file import rename via rename-preview), Q7 (a) idempotent re-run after reindex, Q6 non-TTY rejection (text + JSON), unknown recipe id, missing positional, mut- ex check, --help prints without bootstrap. - src/cli/main.ts + bootstrap.ts — register the verb. realpath note: tests `realpathSync` the temp project root so oxc- resolver's symlink-dereferenced `resolved_path` aligns with the indexed file paths (without it the import-rename rows in rename- preview return empty on macOS where /tmp → /private/tmp). Tests: 10/10 integration + 20/20 engine. Typecheck / lint / format clean. --- src/cli/bootstrap.ts | 4 + src/cli/cmd-apply.test.ts | 276 ++++++++++++++++++++++++++++++ src/cli/cmd-apply.ts | 343 ++++++++++++++++++++++++++++++++++++++ src/cli/main.ts | 25 +++ 4 files changed, 648 insertions(+) create mode 100644 src/cli/cmd-apply.test.ts create mode 100644 src/cli/cmd-apply.ts diff --git a/src/cli/bootstrap.ts b/src/cli/bootstrap.ts index 52890fb..dc38c3e 100644 --- a/src/cli/bootstrap.ts +++ b/src/cli/bootstrap.ts @@ -49,6 +49,9 @@ Targeted reads (precise lookup by symbol name): Impact analysis (graph walk for refactor blast-radius): codemap impact [--direction up|down|both] [--depth N] [--via ] [--limit N] [--summary] [--json] +Apply (substrate-shaped fix executor; consumes the diff-json row contract): + codemap apply [--params k=v[,k=v]] [--dry-run] [--yes] [--json] + Coverage ingest (Istanbul JSON or LCOV from any test runner): codemap ingest-coverage [--json] # path = file or dir; format auto-detected @@ -87,6 +90,7 @@ export function validateIndexModeArgs(rest: string[]): void { if (rest[0] === "show") return; if (rest[0] === "snippet") return; if (rest[0] === "impact") return; + if (rest[0] === "apply") return; if (rest[0] === "ingest-coverage") return; if (rest[0] === "pr-comment") return; diff --git a/src/cli/cmd-apply.test.ts b/src/cli/cmd-apply.test.ts new file mode 100644 index 0000000..f2598b3 --- /dev/null +++ b/src/cli/cmd-apply.test.ts @@ -0,0 +1,276 @@ +/** + * End-to-end CLI coverage for `codemap apply `. Exercises the + * full pipeline (bootstrap → recipe execution → apply-engine → disk-state + * assertions). Uses the same per-test temp project + full-index pattern as + * cmd-query-recency.test.ts. + * + * The TTY-prompt path (Q6 (a) interactive) is NOT tested here — Q9 locked + * "TTY-prompt path tested via --yes flag (skipping prompt); non-TTY-no-yes + * rejection tested explicitly." Bun.spawn's stdout is non-TTY by default, + * so subprocess invocations exercise exactly the non-TTY policy. + */ + +import { + afterEach, + beforeAll, + beforeEach, + describe, + expect, + it, +} from "bun:test"; +import { + existsSync, + mkdirSync, + mkdtempSync, + readFileSync, + realpathSync, + rmSync, + writeFileSync, +} from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +const repoRoot = join(import.meta.dir, "..", ".."); +const indexTs = join(repoRoot, "src", "index.ts"); +let bunBin: string | null = null; + +interface CliResult { + exitCode: number; + out: string; + err: string; +} + +async function runCli( + args: string[], + envOverride: Record = {}, +): Promise { + if (bunBin === null) { + throw new Error("cmd-apply.test: bunBin not initialised by beforeAll."); + } + const proc = Bun.spawn([bunBin, indexTs, ...args], { + cwd: repoRoot, + stdout: "pipe", + stderr: "pipe", + env: { ...process.env, ...envOverride }, + }); + const exitCode = await proc.exited; + const out = await new Response(proc.stdout).text(); + const err = await new Response(proc.stderr).text(); + return { exitCode, out, err }; +} + +let projectRoot: string; + +const tinySource = `import { helper } from "./helper"; + +export function entry(): number { + return helper() + 1; +} + +export const VALUE = "x"; +`; +const helperSource = `export function helper(): number { + return 42; +} +`; + +beforeAll(() => { + bunBin = Bun.which("bun"); + if (!bunBin || !existsSync(indexTs)) { + throw new Error( + `cmd-apply.test: cannot locate Bun (${bunBin}) or src entry (${indexTs}).`, + ); + } +}); + +beforeEach(async () => { + // realpath: oxc-resolver returns the canonical (symlink-dereferenced) + // path for resolved imports. On macOS `tmpdir()` (`/var/folders/...`) + // and `/tmp` are both symlinks, so without realpath the project root + // and `imports.resolved_path` disagree on prefix and the import-rename + // join in `rename-preview.sql` returns 0 rows. + projectRoot = realpathSync(mkdtempSync(join(tmpdir(), "codemap-cli-apply-"))); + mkdirSync(join(projectRoot, "src"), { recursive: true }); + writeFileSync(join(projectRoot, "src", "entry.ts"), tinySource, "utf8"); + writeFileSync(join(projectRoot, "src", "helper.ts"), helperSource, "utf8"); + writeFileSync(join(projectRoot, "package.json"), "{}\n", "utf8"); + const idx = await runCli(["--full"], { CODEMAP_ROOT: projectRoot }); + expect(idx.exitCode).toBe(0); +}); + +afterEach(() => { + rmSync(projectRoot, { recursive: true, force: true }); +}); + +function readFile(rel: string): string { + return readFileSync(join(projectRoot, rel), "utf8"); +} + +describe("codemap apply — CLI integration", () => { + describe("--dry-run", () => { + it("emits the dry-run envelope without touching disk", async () => { + const before = readFile("src/helper.ts"); + const r = await runCli( + [ + "apply", + "rename-preview", + "--params", + "old=helper,new=worker", + "--dry-run", + "--json", + ], + { CODEMAP_ROOT: projectRoot }, + ); + expect(r.exitCode).toBe(0); + const env = JSON.parse(r.out); + expect(env.mode).toBe("dry-run"); + expect(env.applied).toBe(false); + expect(env.summary.rows_applied).toBe(0); + expect(env.summary.rows).toBeGreaterThan(0); + expect(readFile("src/helper.ts")).toBe(before); + }); + + it("rejects --dry-run + --yes as mutually exclusive", async () => { + const r = await runCli( + [ + "apply", + "rename-preview", + "--params", + "old=helper,new=worker", + "--dry-run", + "--yes", + ], + { CODEMAP_ROOT: projectRoot }, + ); + expect(r.exitCode).toBe(1); + expect(r.err).toContain("mutually exclusive"); + }); + }); + + describe("--yes (apply path)", () => { + it("writes the rename to disk and reports applied=true", async () => { + const r = await runCli( + [ + "apply", + "rename-preview", + "--params", + "old=helper,new=worker", + "--yes", + "--json", + ], + { CODEMAP_ROOT: projectRoot }, + ); + expect(r.exitCode).toBe(0); + const env = JSON.parse(r.out); + expect(env.mode).toBe("apply"); + expect(env.applied).toBe(true); + expect(env.summary.rows_applied).toBeGreaterThan(0); + // Definition site should have been renamed. + expect(readFile("src/helper.ts")).toContain("function worker("); + expect(readFile("src/helper.ts")).not.toContain("function helper("); + // Import specifier on the consumer should have been renamed too. + expect(readFile("src/entry.ts")).toContain("import { worker }"); + }); + + it("renaming an already-applied symbol is a no-op after reindex (Q7 (a))", async () => { + const first = await runCli( + [ + "apply", + "rename-preview", + "--params", + "old=helper,new=worker", + "--yes", + "--json", + ], + { CODEMAP_ROOT: projectRoot }, + ); + expect(first.exitCode).toBe(0); + + // Re-index so the recipe sees post-rename state. + const reindex = await runCli(["--full"], { CODEMAP_ROOT: projectRoot }); + expect(reindex.exitCode).toBe(0); + + const second = await runCli( + [ + "apply", + "rename-preview", + "--params", + "old=helper,new=worker", + "--yes", + "--json", + ], + { CODEMAP_ROOT: projectRoot }, + ); + expect(second.exitCode).toBe(0); + const env = JSON.parse(second.out); + expect(env.summary.rows).toBe(0); + expect(env.summary.rows_applied).toBe(0); + }); + }); + + describe("Q6 — non-TTY gate", () => { + it("rejects non-TTY apply without --yes / --dry-run", async () => { + const r = await runCli( + ["apply", "rename-preview", "--params", "old=helper,new=worker"], + { CODEMAP_ROOT: projectRoot }, + ); + expect(r.exitCode).toBe(1); + expect(r.err).toContain("--yes"); + }); + + it("emits the rejection as JSON envelope under --json", async () => { + const r = await runCli( + [ + "apply", + "rename-preview", + "--params", + "old=helper,new=worker", + "--json", + ], + { CODEMAP_ROOT: projectRoot }, + ); + expect(r.exitCode).toBe(1); + const env = JSON.parse(r.out); + expect(env.error).toContain("--yes"); + }); + }); + + describe("error paths", () => { + it("emits unknown-recipe error with the catalog hint", async () => { + const r = await runCli( + ["apply", "no-such-recipe-id", "--dry-run", "--json"], + { CODEMAP_ROOT: projectRoot }, + ); + expect(r.exitCode).toBe(1); + const env = JSON.parse(r.out); + expect(env.error).toContain("unknown recipe"); + expect(env.error).toContain("rename-preview"); // catalog hint includes known ids + }); + + it("requires a positional recipe id", async () => { + const r = await runCli(["apply", "--dry-run"], { + CODEMAP_ROOT: projectRoot, + }); + expect(r.exitCode).toBe(1); + expect(r.err).toContain("missing "); + }); + + it("rejects unknown options", async () => { + const r = await runCli(["apply", "rename-preview", "--no-such-flag"], { + CODEMAP_ROOT: projectRoot, + }); + expect(r.exitCode).toBe(1); + expect(r.err).toContain("unknown option"); + }); + }); + + describe("--help", () => { + it("prints usage without bootstrapping", async () => { + const r = await runCli(["apply", "--help"]); + expect(r.exitCode).toBe(0); + expect(r.out).toContain("Usage: codemap apply "); + expect(r.out).toContain("--dry-run"); + expect(r.out).toContain("--yes"); + }); + }); +}); diff --git a/src/cli/cmd-apply.ts b/src/cli/cmd-apply.ts new file mode 100644 index 0000000..dde6683 --- /dev/null +++ b/src/cli/cmd-apply.ts @@ -0,0 +1,343 @@ +import { createInterface } from "node:readline/promises"; + +import { applyDiffPayload } from "../application/apply-engine"; +import type { ApplyJsonPayload } from "../application/apply-engine"; +import { queryRows } from "../application/index-engine"; +import { + getQueryRecipeParams, + getQueryRecipeSql, + listQueryRecipeIds, +} from "../application/query-recipes"; +import { + mergeParams, + parseParamsCli, + resolveRecipeParams, +} from "../application/recipe-params"; +import type { RecipeParamValues } from "../application/recipe-params"; +import { getProjectRoot } from "../runtime"; +import { bootstrapCodemap } from "./bootstrap-codemap"; + +interface ApplyOpts { + root: string; + configFile: string | undefined; + stateDir?: string | undefined; + recipeId: string; + params: RecipeParamValues | undefined; + dryRun: boolean; + yes: boolean; + json: boolean; +} + +/** Print `codemap apply` usage. */ +export function printApplyCmdHelp(): void { + console.log(`Usage: codemap apply [--params k=v[,k=v]] [--dry-run] [--yes] [--json] + +Apply the diff hunks a recipe describes (one per row of {file_path, +line_start, before_pattern, after_pattern}) to disk. The recipe SQL is +the synthesis surface; codemap is the executor — substrate, not verdict. + +Args: + Same ids \`codemap query --recipe\` accepts. The recipe + must produce rows with the diff-json column shape. + +Flags: + --params k=v[,k=v] Bind values for parametrised recipes. Repeatable; + last value wins on duplicate keys. + --dry-run Preview only — phase-1 validates against current disk; + no file is written. Mutually exclusive with --yes. + --yes Skip the TTY confirmation prompt. Required for non-TTY + contexts (CI, agents, MCP). + --json Emit the structured envelope (one JSON object) on + stdout. Errors emit \`{"error":"..."}\`. + --help, -h Show this help. + +Output (JSON, all cases): + { "mode": "dry-run" | "apply", "applied": , + "files": [ {file_path, rows_applied, warnings?}, ... ], + "conflicts": [ {file_path, line_start, before_pattern, + actual_at_line, reason}, ... ], + "summary": { files, files_modified, rows, rows_applied, + conflicts, files_with_conflicts } } + +Exit codes: + 0 Clean apply (or clean dry-run with zero conflicts). + 1 Any conflicts detected; phase 2 aborted (or any failure). + +Examples: + codemap apply rename-preview --params old=foo,new=bar --dry-run + codemap apply rename-preview --params old=foo,new=bar --yes + codemap apply rename-preview --params old=foo,new=bar --yes --json +`); +} + +/** Parse argv after bootstrap split. `rest[0]` must be `"apply"`. */ +export function parseApplyRest(rest: string[]): + | { kind: "help" } + | { kind: "error"; message: string } + | { + kind: "run"; + recipeId: string; + params: RecipeParamValues | undefined; + dryRun: boolean; + yes: boolean; + json: boolean; + } { + if (rest[0] !== "apply") { + throw new Error("parseApplyRest: expected apply"); + } + + let recipeId: string | undefined; + let params: RecipeParamValues | undefined; + let dryRun = false; + let yes = false; + let json = false; + + for (let i = 1; i < rest.length; i++) { + const a = rest[i]!; + if (a === "--help" || a === "-h") return { kind: "help" }; + if (a === "--json") { + json = true; + continue; + } + if (a === "--dry-run") { + dryRun = true; + continue; + } + if (a === "--yes") { + yes = true; + continue; + } + if (a === "--params") { + const next = rest[i + 1]; + if (next === undefined) { + return { + kind: "error", + message: `codemap apply: "--params" requires a value (k=v[,k=v]).`, + }; + } + params = mergeParams(params, parseParamsCli(next)); + i++; + continue; + } + if (a.startsWith("-")) { + return { + kind: "error", + message: `codemap apply: unknown option "${a}". Run \`codemap apply --help\` for usage.`, + }; + } + if (recipeId !== undefined) { + return { + kind: "error", + message: `codemap apply: unexpected extra argument "${a}". Pass exactly one .`, + }; + } + recipeId = a; + } + + if (recipeId === undefined) { + return { + kind: "error", + message: `codemap apply: missing . Run \`codemap apply --help\` for usage.`, + }; + } + if (dryRun && yes) { + return { + kind: "error", + message: `codemap apply: --dry-run and --yes are mutually exclusive (--dry-run never writes).`, + }; + } + + return { kind: "run", recipeId, params, dryRun, yes, json }; +} + +/** + * Run `codemap apply `. Bootstraps, resolves the recipe SQL, + * executes it, validates rows against disk, and either previews or writes + * per Q6's TTY/`--yes` gate. Sets `process.exitCode = 1` on any failure or + * conflicts (no `process.exit`) so piped stdout isn't truncated. + */ +export async function runApplyCmd(opts: ApplyOpts): Promise { + try { + const sql = getQueryRecipeSql(opts.recipeId); + if (sql === undefined) { + const known = listQueryRecipeIds().join(", "); + emitError( + `codemap apply: unknown recipe "${opts.recipeId}". Known: ${known}.`, + opts.json, + ); + return; + } + + await bootstrapCodemap(opts); + + const resolved = resolveRecipeParams({ + recipeId: opts.recipeId, + declared: getQueryRecipeParams(opts.recipeId), + provided: opts.params, + }); + if (!resolved.ok) { + emitError(resolved.error, opts.json); + return; + } + + let rows: unknown[]; + try { + rows = queryRows(sql, resolved.values); + } catch (err) { + const msg = err instanceof Error ? err.message : String(err); + emitError( + `codemap apply: recipe SQL execution failed — ${msg}`, + opts.json, + ); + return; + } + + const projectRoot = getProjectRoot(); + const isTTY = process.stdout.isTTY === true; + + // Q6 gate (a): non-TTY without --yes / --dry-run is rejected. + if (!isTTY && !opts.yes && !opts.dryRun) { + emitError( + `codemap apply: this verb writes files. Pass --yes for non-interactive runs, or --dry-run for preview.`, + opts.json, + ); + return; + } + + // Direct paths (no prompt): --dry-run or --yes. + if (opts.dryRun || opts.yes) { + const result = applyDiffPayload({ + rows: rows as Record[], + projectRoot, + dryRun: opts.dryRun, + }); + emitResult(result, opts); + return; + } + + // Interactive path: phase-1 dry-run preview → prompt → phase-2 apply. + // Phase-1 runs twice in the proceed case (once for the preview, once + // when applyDiffPayload re-runs phase-1 before phase-2). Cost is two + // file reads per pending file; acceptable for v1 since apply isn't a + // hot path. + const preview = applyDiffPayload({ + rows: rows as Record[], + projectRoot, + dryRun: true, + }); + if (preview.conflicts.length > 0) { + emitResult(preview, opts); + return; + } + if (preview.files.length === 0) { + // Nothing to apply — emit the dry-run envelope and exit 0. + emitResult(preview, opts); + return; + } + + printPromptSummary(preview, opts.recipeId, rows); + const proceed = await promptYesNo(); + if (!proceed) { + // User aborted; treat as a clean dry-run + stderr note. Exit 0 (the + // user explicitly chose not to apply; that's not an error). + console.error("apply: aborted by user."); + emitResult(preview, opts); + return; + } + + const applyResult = applyDiffPayload({ + rows: rows as Record[], + projectRoot, + dryRun: false, + }); + emitResult(applyResult, opts); + } catch (err) { + const msg = err instanceof Error ? err.message : String(err); + emitError(msg, opts.json); + } +} + +function emitResult(result: ApplyJsonPayload, opts: ApplyOpts): void { + if (opts.json) { + console.log(JSON.stringify(result)); + } else { + renderTerminal(result, opts.recipeId, opts.dryRun); + } + if (result.conflicts.length > 0) { + process.exitCode = 1; + } +} + +function renderTerminal( + result: ApplyJsonPayload, + recipeId: string, + dryRun: boolean, +): void { + if (result.conflicts.length > 0) { + console.log( + `apply ${recipeId}: aborted (${result.summary.conflicts} conflicts in ${result.summary.files_with_conflicts} files); see --json for details`, + ); + return; + } + if (dryRun) { + if (result.files.length === 0) { + console.log(`apply ${recipeId} --dry-run: no rows applicable.`); + return; + } + console.log( + `apply ${recipeId} --dry-run: would modify ${result.summary.files} files (${result.summary.rows} rows). Re-run without --dry-run to apply.`, + ); + return; + } + if (!result.applied) { + console.log(`apply ${recipeId}: no rows applicable.`); + return; + } + console.log( + `apply ${recipeId}: modified ${result.summary.files_modified} files, applied ${result.summary.rows_applied} rows.`, + ); +} + +function printPromptSummary( + preview: ApplyJsonPayload, + recipeId: string, + rows: unknown[], +): void { + // Count input rows per file_path so the prompt shows per-file scope. + // The engine's `files[].rows_applied` is 0 in dry-run mode (per Q5); + // re-deriving from the input row set is the cheapest correct path. + const perFile = new Map(); + for (const row of rows) { + if (typeof row !== "object" || row === null) continue; + const fp = (row as Record)["file_path"]; + if (typeof fp !== "string") continue; + perFile.set(fp, (perFile.get(fp) ?? 0) + 1); + } + console.error( + `apply ${recipeId}: ${preview.summary.files} files, ${preview.summary.rows} rows`, + ); + for (const file of preview.files) { + const n = perFile.get(file.file_path) ?? 0; + console.error(` - ${file.file_path} (${n} ${n === 1 ? "row" : "rows"})`); + } + console.error(""); +} + +async function promptYesNo(): Promise { + const rl = createInterface({ input: process.stdin, output: process.stderr }); + try { + const answer = await rl.question("Proceed? [y/N] "); + return /^y(es)?$/i.test(answer.trim()); + } finally { + rl.close(); + } +} + +function emitError(message: string, json: boolean): void { + if (json) { + console.log(JSON.stringify({ error: message })); + } else { + console.error(message); + } + process.exitCode = 1; +} diff --git a/src/cli/main.ts b/src/cli/main.ts index 329ba02..e39d3be 100644 --- a/src/cli/main.ts +++ b/src/cli/main.ts @@ -271,6 +271,31 @@ Copies bundled agent templates into .agents/ under the project root. return; } + if (rest[0] === "apply") { + const { parseApplyRest, printApplyCmdHelp, runApplyCmd } = + await import("./cmd-apply.js"); + const parsed = parseApplyRest(rest); + if (parsed.kind === "help") { + printApplyCmdHelp(); + return; + } + if (parsed.kind === "error") { + console.error(parsed.message); + process.exit(1); + } + await runApplyCmd({ + root, + configFile, + stateDir, + recipeId: parsed.recipeId, + params: parsed.params, + dryRun: parsed.dryRun, + yes: parsed.yes, + json: parsed.json, + }); + return; + } + if (rest[0] === "audit") { const { parseAuditRest, printAuditCmdHelp, runAuditCmd } = await import("./cmd-audit.js"); From 1fd85ce88ce3c69714fcdc97f9afd36ac6e12fe3 Mon Sep 17 00:00:00 2001 From: Sutu Sebastian Date: Wed, 6 May 2026 12:32:33 +0300 Subject: [PATCH 4/9] =?UTF-8?q?feat(apply):=20slice=204=20=E2=80=94=20MCP/?= =?UTF-8?q?HTTP=20`apply`=20tool?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Registers `apply` as the 13th tool over both MCP (stdio) and HTTP transports. Dispatches the same `applyDiffPayload` engine the CLI uses; output envelope is identical to the CLI's --json output (Q5). - src/application/tool-handlers.ts — `handleApply(args, root)` + Zod schema (`applyArgsSchema`). Q6 gate enforced: non-TTY transports always require `yes: true` (no prompt to fall back on). dry_run + yes rejected as mutually exclusive. Unknown recipe returns 404. - src/application/mcp-server.ts — `registerApplyTool` mirrors the impact tool's shape; description encodes the Q5 envelope + Q2 (c) all-or-nothing semantics so agents can reason about the tool without reading docs. - src/application/http-server.ts — adds `apply` to TOOL_NAMES + the POST /tool/{name} dispatcher case. - src/application/tool-handlers.test.ts — 4 handleApply tests (404, yes-required, mutex, dry-run envelope shape). 104 mcp/http server tests still green; tool catalogs are inferred from TOOL_NAMES so the new tool surfaces automatically in /tools listings. Per the plan's Slice 4 lock: `query_batch` does NOT get an apply analogue (Moat-A: batched writes are verdict-shaped; consumers compose multiple apply calls if they need cross-recipe writes). --- src/application/http-server.ts | 9 +++ src/application/mcp-server.ts | 15 +++++ src/application/tool-handlers.test.ts | 91 ++++++++++++++++++++++++++- src/application/tool-handlers.ts | 83 +++++++++++++++++++++++- 4 files changed, 195 insertions(+), 3 deletions(-) diff --git a/src/application/http-server.ts b/src/application/http-server.ts index c3aa1ad..02078dc 100644 --- a/src/application/http-server.ts +++ b/src/application/http-server.ts @@ -14,9 +14,11 @@ import { } from "../runtime"; import { listResources, readResource } from "./resource-handlers"; import { + applyArgsSchema, auditArgsSchema, contextArgsSchema, dropBaselineArgsSchema, + handleApply, handleAudit, handleContext, handleDropBaseline, @@ -88,6 +90,7 @@ const TOOL_NAMES = [ "show", "snippet", "impact", + "apply", "save_baseline", "list_baselines", "drop_baseline", @@ -474,6 +477,12 @@ async function dispatchTool( result = handleImpact(r.value); break; } + case "apply": { + const r = validate(applyArgsSchema, args, "apply"); + if (!r.ok) return writeJson(res, 400, { error: r.error }, opts.version); + result = handleApply(r.value, opts.root); + break; + } case "save_baseline": { const r = validate(saveBaselineArgsSchema, args, "save_baseline"); if (!r.ok) return writeJson(res, 400, { error: r.error }, opts.version); diff --git a/src/application/mcp-server.ts b/src/application/mcp-server.ts index 82c0146..a40d2e6 100644 --- a/src/application/mcp-server.ts +++ b/src/application/mcp-server.ts @@ -15,9 +15,11 @@ import { import { listQueryRecipeCatalog } from "./query-recipes"; import { readResource } from "./resource-handlers"; import { + applyArgsSchema, auditArgsSchema, contextArgsSchema, dropBaselineArgsSchema, + handleApply, handleAudit, handleContext, handleDropBaseline, @@ -120,6 +122,7 @@ export function createMcpServer(opts: ServerOpts): McpServer { registerShowTool(server, opts); registerSnippetTool(server, opts); registerImpactTool(server); + registerApplyTool(server, opts); registerResources(server); return server; @@ -269,6 +272,18 @@ function registerImpactTool(server: McpServer): void { ); } +function registerApplyTool(server: McpServer, opts: ServerOpts): void { + server.registerTool( + "apply", + { + description: + "Apply the diff hunks a recipe describes (one per row of {file_path, line_start, before_pattern, after_pattern}) to disk. Substrate-shaped fix executor — recipe SQL is the synthesis surface, codemap executes. Args: recipe (id), params (k=v map for parametrised recipes), dry_run (preview only; phase-1 validates against current disk; no file is written), yes (required for the write path — non-TTY transports always need explicit consent; mutually exclusive with dry_run). Result envelope (same shape across modes): {mode: 'dry-run'|'apply', applied: bool, files: [{file_path, rows_applied, warnings?}], conflicts: [{file_path, line_start, before_pattern, actual_at_line, reason}], summary: {files, files_modified, rows, rows_applied, conflicts, files_with_conflicts}}. Q2 (c) all-or-nothing — any conflict aborts the whole run before any file is touched.", + inputSchema: applyArgsSchema, + }, + (args) => wrapToolResult(handleApply(args, opts.root)), + ); +} + /** * Register codemap's four MCP resources. Payloads come from the shared * `application/resource-handlers.ts` module — same lazy-cache used by the diff --git a/src/application/tool-handlers.test.ts b/src/application/tool-handlers.test.ts index 0c391d0..3bdd5df 100644 --- a/src/application/tool-handlers.test.ts +++ b/src/application/tool-handlers.test.ts @@ -1,12 +1,19 @@ import { afterEach, beforeEach, describe, expect, it } from "bun:test"; -import { mkdirSync, mkdtempSync, rmSync } from "node:fs"; +import { + mkdirSync, + mkdtempSync, + readFileSync, + realpathSync, + rmSync, + writeFileSync, +} from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { resolveCodemapConfig } from "../config"; import { closeDb, createTables, openDb } from "../db"; import { initCodemap } from "../runtime"; -import { handleQueryRecipe } from "./tool-handlers"; +import { handleApply, handleQueryRecipe } from "./tool-handlers"; let projectRoot: string; @@ -76,3 +83,83 @@ describe("handleQueryRecipe params", () => { }); }); }); + +describe("handleApply", () => { + it("returns 404 for an unknown recipe", () => { + const result = handleApply( + { recipe: "no-such-recipe-id", dry_run: true }, + projectRoot, + ); + expect(result).toMatchObject({ + ok: false, + status: 404, + error: expect.stringContaining("unknown recipe"), + }); + }); + + it("rejects a write request without yes (Q6 — non-TTY transports)", () => { + const result = handleApply( + { + recipe: "rename-preview", + params: { old: "runQuery", new: "runQry" }, + }, + projectRoot, + ); + expect(result).toMatchObject({ + ok: false, + error: expect.stringContaining("yes: true"), + }); + }); + + it("rejects dry_run + yes as mutually exclusive", () => { + const result = handleApply( + { + recipe: "rename-preview", + params: { old: "runQuery", new: "runQry" }, + dry_run: true, + yes: true, + }, + projectRoot, + ); + expect(result).toMatchObject({ + ok: false, + error: expect.stringContaining("mutually exclusive"), + }); + }); + + it("returns the dry-run envelope shape on a parametrised recipe", () => { + // Realpath the project root so oxc-resolver's symlink-derefed + // resolved_path aligns with the indexed file paths (mirrors the + // CLI integration test). + const realRoot = realpathSync(projectRoot); + // Write the actual source file the indexed symbol points at so + // phase-1 can read it when the recipe row resolves. + writeFileSync( + join(realRoot, "src", "query.ts"), + "export function runQuery() {}\n", + "utf8", + ); + const result = handleApply( + { + recipe: "rename-preview", + params: { old: "runQuery", new: "runQry", kind: "function" }, + dry_run: true, + }, + realRoot, + ); + expect(result.ok).toBe(true); + if (result.ok) { + const payload = result.payload as Record; + expect(payload.mode).toBe("dry-run"); + expect(payload.applied).toBe(false); + expect(payload.summary).toMatchObject({ + rows: 1, + rows_applied: 0, + }); + // Disk untouched. + expect(readFileSync(join(realRoot, "src", "query.ts"), "utf8")).toBe( + "export function runQuery() {}\n", + ); + } + }); +}); diff --git a/src/application/tool-handlers.ts b/src/application/tool-handlers.ts index c05513f..504351f 100644 --- a/src/application/tool-handlers.ts +++ b/src/application/tool-handlers.ts @@ -29,6 +29,7 @@ import { getFilesChangedSince } from "../git-changed"; import type { GroupByMode } from "../group-by"; import { GROUP_BY_MODES } from "../group-by"; import { getProjectRoot } from "../runtime"; +import { applyDiffPayload } from "./apply-engine"; import { makeWorktreeReindex, resolveAuditBaselines, @@ -38,7 +39,7 @@ import { import { buildContextEnvelope } from "./context-engine"; import { findImpact } from "./impact-engine"; import type { ImpactBackend, ImpactDirection } from "./impact-engine"; -import { getCurrentCommit } from "./index-engine"; +import { getCurrentCommit, queryRows } from "./index-engine"; import { formatAnnotations, formatDiff, @@ -775,6 +776,86 @@ export function handleImpact(args: ImpactArgs): ToolResult { } } +// === apply ================================================================== + +export const applyArgsSchema = { + recipe: z.string().min(1, "recipe must be a non-empty string"), + params: z + .record(z.string(), z.union([z.string(), z.number(), z.boolean()])) + .optional(), + dry_run: z.boolean().optional(), + /** + * Q6 (a) — non-TTY contexts (every MCP / HTTP transport) require an + * explicit `yes: true` to write. There's no prompt to fall back on. + */ + yes: z.boolean().optional(), +}; + +export interface ApplyArgs { + recipe: string; + params?: RecipeParamValues; + dry_run?: boolean; + yes?: boolean; +} + +/** + * Substrate-shaped fix executor. Reuses {@link applyDiffPayload} so the + * MCP / HTTP envelope is identical to the CLI's `--json` output (Q5). + * + * Q6 gating — over MCP / HTTP the only valid non-`dry_run` invocation + * carries `yes: true`. There is no TTY prompt; rejecting non-`yes` here + * is the agent-shaped equivalent of the CLI's non-TTY rejection. + */ +export function handleApply(args: ApplyArgs, root: string): ToolResult { + try { + const sql = getQueryRecipeSql(args.recipe); + if (sql === undefined) { + return err( + `codemap: unknown recipe "${args.recipe}". List available recipes via the codemap://recipes resource.`, + 404, + ); + } + + const dryRun = args.dry_run === true; + if (!dryRun && args.yes !== true) { + return err( + `codemap apply: this tool writes files. Pass {yes: true} for non-interactive runs, or {dry_run: true} for preview.`, + ); + } + if (dryRun && args.yes === true) { + return err( + `codemap apply: dry_run and yes are mutually exclusive (dry_run never writes).`, + ); + } + + const resolvedParams = resolveRecipeParams({ + recipeId: args.recipe, + declared: getQueryRecipeParams(args.recipe), + provided: args.params, + }); + if (!resolvedParams.ok) return err(resolvedParams.error); + + let rows: unknown[]; + try { + rows = queryRows(sql, resolvedParams.values); + } catch (e) { + return err( + `codemap apply: recipe SQL execution failed — ${e instanceof Error ? e.message : String(e)}`, + 500, + ); + } + + const payload = applyDiffPayload({ + rows: rows as Record[], + projectRoot: root, + dryRun, + }); + return ok(payload); + } catch (e) { + return err(e instanceof Error ? e.message : String(e), 500); + } +} + // === shared format helpers =================================================== /** From 30080f62a16181ed6ec63752dd74b5b9f2ee7034 Mon Sep 17 00:00:00 2001 From: Sutu Sebastian Date: Wed, 6 May 2026 12:40:33 +0300 Subject: [PATCH 5/9] =?UTF-8?q?docs(apply):=20slice=205=20=E2=80=94=20lock?= =?UTF-8?q?step=20+=20plan=20retire?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Final slice — lifts the durable design from the plan into reference docs and retires the plan file per docs/README.md Rule 3. - docs/architecture.md — new "Apply wiring" section (engine + phase-1 algorithm + phase-2 atomic temp-rename + Q6 gate + Q5 envelope + Q7 idempotency) plus "Boundary verification — apply write path" SQL kit. Layering table mentions `apply-engine.ts`. - docs/glossary.md — `codemap apply` / apply tool entry. - docs/roadmap.md — backlog entry removed (shipped). - docs/plans/codemap-apply.md — DELETED (closing-state lifecycle per docs-governance skill: delete + lift, never "Slim & keep in plans/"). - .agents/rules/codemap.md + .agents/skills/codemap/SKILL.md — Apply row in CLI table, "Apply (`bun src/index.ts apply `)" paragraph, MCP `apply` tool listed alongside `impact`. - templates/agents/rules/codemap.md + templates/agents/skills/codemap/ SKILL.md — same updates in the published-package mirror (uses `codemap` instead of `bun src/index.ts`). - .changeset/codemap-apply.md — minor bump; summarises Q1–Q10 locks + boundary discipline anchor. Boundary kit verified empty after a fresh reindex of the apply files; 140/140 tests pass across apply-engine + tool-handlers + cmd-apply + mcp/http-server suites. --- .agents/rules/codemap.md | 7 +- .agents/skills/codemap/SKILL.md | 1 + .changeset/codemap-apply.md | 29 ++ docs/architecture.md | 34 ++- docs/glossary.md | 4 + docs/plans/codemap-apply.md | 367 ----------------------- docs/roadmap.md | 1 - templates/agents/rules/codemap.md | 5 +- templates/agents/skills/codemap/SKILL.md | 1 + 9 files changed, 71 insertions(+), 378 deletions(-) create mode 100644 .changeset/codemap-apply.md delete mode 100644 docs/plans/codemap-apply.md diff --git a/.agents/rules/codemap.md b/.agents/rules/codemap.md index a4b44df..e8ab2b0 100644 --- a/.agents/rules/codemap.md +++ b/.agents/rules/codemap.md @@ -37,6 +37,7 @@ A local database (default **`.codemap/index.db`**) indexes structure: symbols, i | Targeted read (metadata) | — | `bun src/index.ts show [--kind ] [--in ] [--json]` — file:line + signature | | Targeted read (source text) | — | `bun src/index.ts snippet [--kind ] [--in ] [--json]` — same lookup + source from disk + stale flag | | Impact (blast-radius walker) | — | `bun src/index.ts impact [--direction up\|down\|both] [--depth N] [--via ] [--limit N] [--summary] [--json]` — replaces hand-composed `WITH RECURSIVE` queries | +| Apply (substrate fix executor) | — | `bun src/index.ts apply [--params k=v[,k=v]] [--dry-run] [--yes] [--json]` — executes the diff hunks a recipe describes (one per `{file_path, line_start, before_pattern, after_pattern}` row). Q6 gate: TTY prompt; non-TTY needs `--yes` (or `--dry-run`). Q2 (c) all-or-nothing — any conflict aborts before any file is touched. | | Coverage ingest | — | `bun src/index.ts ingest-coverage [--runtime] [--json]` — Istanbul (`coverage-final.json`) and LCOV (`lcov.info`) auto-detect from path; V8 is **opt-in** via `--runtime` (treats `` as a `NODE_V8_COVERAGE=...`-style directory of `coverage-*.json` dumps). Joinable to `symbols` for "untested AND dead" queries. Local-only — no SaaS aggregation. | | SARIF / GH annotations | — | `bun src/index.ts query --recipe deprecated-symbols --format sarif` · `… --format annotations` | | `--ci` aggregate flag | — | `bun src/index.ts query -r deprecated-symbols --ci` (or `audit --base origin/main --ci`) — aliases `--format sarif` + non-zero exit when findings/additions surfaced + suppresses the no-locatable-rows stderr warning. Mutually exclusive with `--json` / `--format `. | @@ -73,11 +74,13 @@ Validation: SQL is rejected at load time if it starts with DML/DDL (DELETE/DROP/ **Targeted reads (`show` / `snippet`)**: precise lookup by exact symbol name without composing SQL. `show` returns metadata (`file_path:line_start-line_end` + `signature`); `snippet` returns the source text from disk plus `stale` / `missing` flags. Both share the same flag set (`--kind ` to filter by `symbols.kind`, `--in ` for file-scope filter — directory prefix or exact file). Output envelope is `{matches, disambiguation?}` — single match → `{matches: [{...}]}`; multi-match adds `disambiguation: {n, by_kind, files, hint}` so agents narrow without re-scanning. Name match is exact / case-sensitive — for fuzzy use `query` with `LIKE '%name%'`. Snippet stale-file behavior: `source` is always returned when the file exists; `stale: true` means the line range may have shifted (re-index with `bun src/index.ts` or `--files ` before acting on the source). -**Impact (`bun src/index.ts impact `)**: symbol/file blast-radius walker — replaces hand-composed `WITH RECURSIVE` queries that agents struggle to write. Target auto-resolves: contains `/` or matches `files.path` → file target; otherwise symbol (case-sensitive). Walks compatible graphs by target kind: **symbol** → `calls` (callers / callees by name); **file** → `dependencies` + `imports` (`resolved_path` only). `--via ` overrides; mismatched explicit choices land in `skipped_backends` (no error). Cycle-detected via `WITH RECURSIVE` path-string + `instr` check; bounded by `--depth N` (default 3, `0` = unbounded but still cycle-detected and limit-capped) and `--limit N` (default 500). Output envelope: `{target, direction, via, depth_limit, matches: [{depth, direction, edge, kind, name?, file_path}], summary: {nodes, max_depth_reached, by_kind, terminated_by: 'depth'|'limit'|'exhausted'}}`. `--summary` trims `matches` for cheap CI-gate consumption (`jq '.summary.nodes'`) but preserves the count. SARIF / annotations not supported (graph traversal, not findings). Pure transport-agnostic engine in `application/impact-engine.ts`; CLI / MCP / HTTP all dispatch the same `findImpact` function. +**Impact (`bun src/index.ts impact `)**: symbol/file blast-radius walker — replaces hand-composed `WITH RECURSIVE` queries that agents struggle to write. Target auto-resolves: contains `/` or matches `files.path` → file target; otherwise symbol (case-sensitive). Walks compatible graphs by target kind: **symbol** → `calls` (callers / callees by name); **file** → `dependencies` + `imports` (`resolved_path` only). `--via ` overrides; mismatched explicit choices land in `skipped_backends` (no error). Cycle-detected via `WITH RECURSIVE` path-string + `instr` check; bounded by `--depth N` (default 3, `0` = unbounded but still cycle-detected and limit-capped) and `--limit N` (default 500). Output envelope: `{target, direction, via, depth_limit, matches: [{depth, direction, edge, kind, name?, file_path}], summary: {nodes, max_depth_reached, by_kind, terminated_by: 'depth'|'limit'|'exhausted'}}`. `--summary` trims `matches` for cheap CI-gate consumption (`jq '.summary.nodes'` + +**Apply (`bun src/index.ts apply `)**: substrate-shaped fix executor over the existing `--format diff-json` row contract — recipe SQL is the synthesis surface, codemap executes. Phase 1 validates every `{file_path, line_start, before_pattern, after_pattern}` row against current disk via `actual.includes(before_pattern)` (substring match — same contract `buildDiffJson` uses); collects three conflict reasons (`file missing` / `line out of range` / `line content drifted`). Phase 2 (gated on `!dryRun && conflicts.length === 0`) writes via sibling temp + `rename` for POSIX-atomic per-file writes. **Q2 (c) all-or-nothing** — any conflict aborts the whole run before any file is touched. **Q6 gate** — TTY prompts `Proceed? [y/N]` (default-N) on stderr; non-TTY (CI / agents / MCP / HTTP) requires `--yes` (or `yes: true`) explicitly; `--dry-run` + `--yes` mutually exclusive. Q7 idempotency: re-running on already-applied code reports `line content drifted` with `actual_at_line` showing the post-rename content — re-run `bun src/index.ts` to refresh, then re-run apply (vacuous clean pass). Output envelope (identical across modes): `{mode: 'dry-run'|'apply', applied: bool, files: [{file_path, rows_applied, warnings?}], conflicts: [{file_path, line_start, before_pattern, actual_at_line, reason}], summary: {files, files_modified, rows, rows_applied, conflicts, files_with_conflicts}}`. Pure transport-agnostic engine in `application/apply-engine.ts`; CLI / MCP / HTTP all dispatch the same `applyDiffPayload` function.) but preserves the count. SARIF / annotations not supported (graph traversal, not findings). Pure transport-agnostic engine in `application/impact-engine.ts`; CLI / MCP / HTTP all dispatch the same `findImpact` function. **MCP server (`bun src/index.ts mcp`)**: stdio MCP (Model Context Protocol) server — agents call codemap as JSON-RPC tools instead of shelling out to the CLI on every read. v1 ships one tool per CLI verb plus six resources (`codemap://recipes` + `codemap://recipes/{id}` are live read every call so inline `last_run_at` / `run_count` recency stays fresh; `codemap://schema` + `codemap://skill` lazy-cache; `codemap://files/{path}` + `codemap://symbols/{name}` always live): -- **Tools:** `query` / `query_batch` / `query_recipe` / `audit` / `save_baseline` / `list_baselines` / `drop_baseline` / `context` / `validate` / `show` / `snippet` / `impact`. Snake_case keys (Codemap convention matching MCP spec examples + reference servers — spec is convention-agnostic; CLI stays kebab). +- **Tools:** `query` / `query_batch` / `query_recipe` / `audit` / `save_baseline` / `list_baselines` / `drop_baseline` / `context` / `validate` / `show` / `snippet` / `impact` / `apply`. Snake_case keys (Codemap convention matching MCP spec examples + reference servers — spec is convention-agnostic; CLI stays kebab). - **`query_batch` (MCP-only):** N statements in one round-trip. Items are `string | {sql, summary?, changed_since?, group_by?}` — string form inherits batch-wide flag defaults, object form overrides on a per-key basis. Per-statement errors are isolated. - **`save_baseline` (polymorphic):** one tool, `{name, sql? | recipe?}` with runtime exclusivity check (mirrors the CLI's single `--save-baseline=` verb). - **Resources:** `codemap://recipes` (catalog — live), `codemap://recipes/{id}` (one recipe — live), `codemap://schema` (live DDL from `sqlite_schema`; lazy-cached), `codemap://skill` (bundled SKILL.md text; lazy-cached), `codemap://files/{path}` (per-file roll-up: symbols, imports, exports, coverage — live), `codemap://symbols/{name}` (symbol lookup with `{matches, disambiguation?}` envelope; `?in=` filter mirrors `show --in` — live). Recipe catalogs read live every call so inline `last_run_at` / `run_count` recency reflects mutations during the server lifetime; `schema` / `skill` cache because their inputs don't change mid-session. diff --git a/.agents/skills/codemap/SKILL.md b/.agents/skills/codemap/SKILL.md index ee0a99d..688e4cf 100644 --- a/.agents/skills/codemap/SKILL.md +++ b/.agents/skills/codemap/SKILL.md @@ -79,6 +79,7 @@ Each emitted delta carries its own `base` metadata so mixed-baseline audits are - **`show`** — `{name, kind?, in?}`. Exact, case-sensitive symbol name lookup. Returns `{matches: [{name, kind, file_path, line_start, line_end, signature, ...}], disambiguation?: {n, by_kind, files, hint}}`. Single match → `{matches: [{...}]}`; multi-match adds the disambiguation envelope so you narrow without re-scanning. Fuzzy lookup belongs in `query` with `LIKE`. - **`snippet`** — `{name, kind?, in?}`. Same lookup as `show` but each match also carries `source` (file lines from disk at `line_start..line_end`), `stale` (true when content_hash drifted since indexing — line range may have shifted), `missing` (true when file is gone). Per Q-6 (settled): `source` is always returned when the file exists; agent decides whether to act on stale content or run `codemap` / `codemap --files ` to re-index first. No auto-reindex side-effects from this read tool. - **`impact`** — `{target, direction?, via?, depth?, limit?, summary?}`. Symbol/file blast-radius walker — replaces hand-composed `WITH RECURSIVE` queries that agents struggle to write reliably. `target` is a symbol name (case-sensitive, exact) OR a project-relative file path (auto-detected by `/` or by matching `files.path`). `direction`: `up` (callers / dependents), `down` (callees / dependencies), `both` (default). `via`: `dependencies`, `calls`, `imports`, `all` (default — every backend compatible with the resolved target kind: symbol → `calls`; file → `dependencies` + `imports`; mismatched explicit choices land in `skipped_backends`, no error). `depth` default 3, `0` = unbounded (still cycle-detected and limit-capped). `limit` default 500. `summary: true` trims `matches` for cheap CI-gate consumption (`jq '.summary.nodes'`) but preserves the count. Result: `{target, direction, via, depth_limit, matches: [{depth, direction, edge, kind, name?, file_path}], summary: {nodes, max_depth_reached, by_kind, terminated_by: 'depth'|'limit'|'exhausted'}}`. Cycle detection is approximate-but-bounded — bounded depth + `LIMIT` keep cyclic graphs cheap; `terminated_by` reports the dominant stop reason. SARIF / annotations not supported (impact rows are graph traversals, not findings). +- **`apply`** — `{recipe, params?, dry_run?, yes?}`. Substrate-shaped fix executor — runs the same recipe `query_recipe` runs, then applies the diff hunks each row describes (`{file_path, line_start, before_pattern, after_pattern}`) to disk. Recipe SQL is the synthesis surface; codemap is the executor. Phase 1 validates every row against current disk via substring-match (`actual.includes(before_pattern)`) — the same contract `--format diff-json` uses. Phase 2 (gated on no conflicts) writes via sibling temp + `rename` for POSIX-atomic per-file writes. **Q2 (c) all-or-nothing** — any conflict in any file aborts phase 2 entirely; partial writes never ship. Over MCP/HTTP `yes: true` is required for the write path (no TTY prompt to fall back on); `dry_run: true` previews without writing; the two are mutually exclusive. Re-running on already-applied code reports a `line content drifted` conflict whose `actual_at_line` shows the post-rename content — re-run `bun src/index.ts` to refresh the index, then re-run `apply` for a clean vacuous pass. Result envelope (identical across modes): `{mode: 'dry-run'|'apply', applied, files: [{file_path, rows_applied, warnings?}], conflicts: [{file_path, line_start, before_pattern, actual_at_line, reason}], summary: {files, files_modified, rows, rows_applied, conflicts, files_with_conflicts}}`. Floor "No fix engine" preserved — codemap doesn't synthesise edits, it only executes the hunks the recipe row described. **Resources (mostly lazy-cached on first `read_resource`; recipes / one-recipe live-read every call so the inline recency fields stay fresh):** diff --git a/.changeset/codemap-apply.md b/.changeset/codemap-apply.md new file mode 100644 index 0000000..9b64d0b --- /dev/null +++ b/.changeset/codemap-apply.md @@ -0,0 +1,29 @@ +--- +"@stainless-code/codemap": minor +--- + +`codemap apply ` — substrate-shaped fix executor over the existing `--format diff-json` row contract. The recipe SQL describes the transformation (`{file_path, line_start, before_pattern, after_pattern}` rows); codemap is the executor. Floor "No fix engine" preserved — codemap doesn't synthesise edits, it only executes the hunks the recipe row described. + +**Three transports, one engine:** + +- **CLI:** `codemap apply [--params k=v[,k=v]] [--dry-run] [--yes] [--json]` +- **MCP tool:** `apply` (registered alongside `impact` / `show` / `snippet`) +- **HTTP:** `POST /tool/apply` + +All three dispatch the same pure `applyDiffPayload` engine in `application/apply-engine.ts`. + +**Decisions worth knowing (Q1–Q10 locked in `docs/plans/codemap-apply.md`, lifted into `docs/architecture.md § Apply wiring` on this PR):** + +- **Apply-by-default, `--dry-run` opts into preview.** Verb-name semantics + `git apply` / `terraform apply` precedent. +- **Per-recipe-run all-or-nothing (Q2 (c)).** Phase 1 validates every row first; any conflict aborts phase 2 entirely before any file is touched. Cross-file invariants matter — `rename-preview` produces a definition row + N import rows, and partial application leaves the project syntactically broken. +- **Scan-and-collect conflicts (Q3 (b)).** Phase 1 walks every row and collects all conflicts in one pass — better remediation UX than fail-fast. +- **TTY prompt + `--yes` gate (Q6 (a)).** Interactive contexts (TTY) get a `Proceed? [y/N]` prompt with default-N; non-interactive contexts (CI / agents / MCP / HTTP) require `--yes` (or `yes: true`) explicitly. `--dry-run` + `--yes` mutually exclusive. +- **Substring match per row, single-line (Q8 (a)).** Mirrors `buildDiffJson`'s contract verbatim — `actual.includes(before_pattern)` + `actual.replace(before, after)` with `$`-pre-escape per `String.prototype.replace`'s GetSubstitution rule. Exemplar: `templates/recipes/rename-preview.sql` emits `before_pattern = old_name` (the bare identifier). +- **Atomic per-file writes via temp + rename.** Sibling `.codemap-apply-.tmp` then `renameSync` — POSIX-atomic so concurrent readers see either pre-rename or post-rename content, never a torn write. +- **Q7 idempotency (conflict-only path).** Re-running on already-applied code reports `line content drifted` with `actual_at_line` showing the post-rename content; user re-runs `codemap` to refresh the index → next run produces 0 rows → vacuous clean apply. +- **Single envelope shape across modes (Q5).** `{mode, applied, files, conflicts, summary}` — same shape for `dry-run` and `apply`; consumers pattern-match on `mode` + `applied`. +- **No SARIF / annotations.** Apply is a write action, not a findings list. + +**Boundary discipline (Q10):** only `cli/cmd-apply.ts` + `application/tool-handlers.ts` may import the apply engine — re-runnable kit at `docs/architecture.md § Boundary verification — apply write path`. + +Plan: PR #77 (merged). Implementation: this PR. diff --git a/docs/architecture.md b/docs/architecture.md index 26670fa..c73189b 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -16,13 +16,13 @@ A local SQLite database (`.codemap/index.db`) indexes the project tree and store ## Layering -| Layer | Role | -| -------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | -| **`cli/`** (`bootstrap`, `main`, `cmd-*`) | Parses argv; **dynamic `import()`** loads only the command chunk (`cmd-index`, `cmd-query`, `cmd-agents`) so `--help` / `version` / `agents init` avoid the indexer. | -| **`api.ts`** | Public programmatic surface: `createCodemap()`, `Codemap` (`query`, `index`), re-exports `runCodemapIndex` for advanced use. | -| **`application/`** | Pure transport-agnostic engines + handlers: `run-index.ts` / `index-engine.ts` (orchestration + indexing); `query-engine.ts` (`executeQuery` / `executeQueryBatch`); `audit-engine.ts` (`runAudit` + `resolveAuditBaselines` + `runAuditFromRef` + `makeWorktreeReindex`); `audit-worktree.ts` (sha-keyed cache + atomic populate); `context-engine.ts` (`buildContextEnvelope`); `validate-engine.ts` (`computeValidateRows` + `toProjectRelative`); `show-engine.ts` (lookup + envelope builders); `impact-engine.ts` (`findImpact` — graph blast-radius walker); `coverage-engine.ts` (`upsertCoverageRows` core + `ingestIstanbul` / `ingestLcov` / `ingestV8` parsers; schema in [§ Schema → coverage](#schema)); `query-recipes.ts` + `recipes-loader.ts` (recipe registry); `output-formatters.ts` (SARIF + GH annotations + Mermaid `flowchart LR` with bounded-input contract); `watcher.ts` (chokidar-backed debounced reindex; pure helpers + injectable backend); `tool-handlers.ts` + `resource-handlers.ts` (transport-agnostic tool / resource handlers shared by MCP + HTTP); `mcp-server.ts` (MCP transport — stdio); `http-server.ts` (HTTP transport — `node:http`). Engines depend on `db.ts` / `runtime.ts`; **never** on `cli/`. | -| **`adapters/`** | `LanguageAdapter` registry; built-ins call `parser.ts` / `css-parser.ts` / `markers.ts` from `parse-worker-core`. | -| **`runtime.ts` / `config.ts` / `db.ts` / …** | Config, SQLite, resolver, workers. | +| Layer | Role | +| -------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| **`cli/`** (`bootstrap`, `main`, `cmd-*`) | Parses argv; **dynamic `import()`** loads only the command chunk (`cmd-index`, `cmd-query`, `cmd-agents`) so `--help` / `version` / `agents init` avoid the indexer. | +| **`api.ts`** | Public programmatic surface: `createCodemap()`, `Codemap` (`query`, `index`), re-exports `runCodemapIndex` for advanced use. | +| **`application/`** | Pure transport-agnostic engines + handlers: `run-index.ts` / `index-engine.ts` (orchestration + indexing); `query-engine.ts` (`executeQuery` / `executeQueryBatch`); `audit-engine.ts` (`runAudit` + `resolveAuditBaselines` + `runAuditFromRef` + `makeWorktreeReindex`); `audit-worktree.ts` (sha-keyed cache + atomic populate); `context-engine.ts` (`buildContextEnvelope`); `validate-engine.ts` (`computeValidateRows` + `toProjectRelative`); `show-engine.ts` (lookup + envelope builders); `impact-engine.ts` (`findImpact` — graph blast-radius walker); `apply-engine.ts` (`applyDiffPayload` — substrate-shaped fix executor over the diff-json row contract); `coverage-engine.ts` (`upsertCoverageRows` core + `ingestIstanbul` / `ingestLcov` / `ingestV8` parsers; schema in [§ Schema → coverage](#schema)); `query-recipes.ts` + `recipes-loader.ts` (recipe registry); `output-formatters.ts` (SARIF + GH annotations + Mermaid `flowchart LR` with bounded-input contract); `watcher.ts` (chokidar-backed debounced reindex; pure helpers + injectable backend); `tool-handlers.ts` + `resource-handlers.ts` (transport-agnostic tool / resource handlers shared by MCP + HTTP); `mcp-server.ts` (MCP transport — stdio); `http-server.ts` (HTTP transport — `node:http`). Engines depend on `db.ts` / `runtime.ts`; **never** on `cli/`. | +| **`adapters/`** | `LanguageAdapter` registry; built-ins call `parser.ts` / `css-parser.ts` / `markers.ts` from `parse-worker-core`. | +| **`runtime.ts` / `config.ts` / `db.ts` / …** | Config, SQLite, resolver, workers. | `index.ts` is the package entry: re-exports the public API and runs `cli/main` only when executed as the main module (Node/Bun `codemap` binary). @@ -131,6 +131,8 @@ A local SQLite database (`.codemap/index.db`) indexes the project tree and store **Impact wiring:** **`src/cli/cmd-impact.ts`** (argv — `` + `--direction up|down|both` + `--depth N` + `--via dependencies|calls|imports|all` + `--limit N` + `--summary` + `--json`; bootstrap absorbs `--root`/`--config`) + **`src/application/impact-engine.ts`** (engine — `findImpact({db, target, direction?, via?, depth?, limit?})`). Pure transport-agnostic walker over the calls + dependencies + imports graphs; CLI / MCP / HTTP all dispatch the same engine function via `tool-handlers.ts`'s `handleImpact`. Target auto-resolves: contains `/` or matches `files.path` → file target; otherwise symbol (case-sensitive). Walks compatible backends per resolved kind: **symbol** → `calls` (callers / callees by `caller_name` / `callee_name`); **file** → `dependencies` (`from_path` / `to_path`) + `imports` (`file_path` / `resolved_path`, `IS NOT NULL` filter). `--via ` overrides; mismatched explicit choices land in `skipped_backends` (no error — agents see why their backend selection yielded fewer rows than expected). One `WITH RECURSIVE` per (direction, backend) combo with cycle detection via path-string `instr` check (SQLite has no native cycle predicate); JS-side merge + dedup by `(direction, kind, name?, file_path)` keeping the shallowest depth. `--depth 0` uses an unbounded sentinel (`UNBOUNDED_DEPTH_SENTINEL = 1_000_000`); cycle detection + `LIMIT` keep cyclic graphs cheap regardless. Termination reason classification: `limit` (truncated) > `depth` (any node sat at the cap) > `exhausted`. Result envelope: `{target, direction, via, depth_limit, matches: [{depth, direction, edge, kind, name?, file_path}], summary: {nodes, max_depth_reached, by_kind, terminated_by}, skipped_backends?}`. `--summary` blanks `matches` (transport bandwidth saver) but preserves `summary.nodes` so CI gates (`jq '.summary.nodes'`) still see the count. SARIF / annotations not supported (graph traversal, not findings — the parser accepts the flag combos but the engine only emits JSON). +**Apply wiring:** **`src/cli/cmd-apply.ts`** (argv — `` + `--params` + `--dry-run` + `--yes` + `--json`; bootstrap absorbs `--root`/`--config`) + **`src/application/apply-engine.ts`** (engine — `applyDiffPayload({rows, projectRoot, dryRun})`). Pure transport-agnostic substrate-shaped fix executor: consumes the existing `--format diff-json` row contract from any recipe (`{file_path, line_start, before_pattern, after_pattern}`), validates each row against current disk, and either previews (dry-run) or writes (apply). CLI / MCP / HTTP all dispatch the same engine via `tool-handlers.ts`'s `handleApply`. **Phase 1** (always) reads each file at most once into `sourceCache`, splits on `/\r?\n/` for conflict reporting, checks `actual.includes(before_pattern)` (substring match — mirrors `buildDiffJson`'s contract; `rename-preview` emits `before_pattern = old_name` as the bare identifier, so whole-line exact match would conflict every time). Conflicts collect three reasons (`file missing` / `line out of range` / `line content drifted`) — Q3 scan-and-collect, not fail-fast. **Phase 2** (gated on `!dryRun && conflicts.length === 0`) re-splits the cached source on raw `"\n"` (preserves CRLF as trailing `\r` per line; rejoining with `"\n"` round-trips losslessly), applies each file's edits in descending line order via `actual.replace(before, after)` with `$`-pre-escape (`replace(/\$/g, "$$$$")` — matches `buildDiffJson`'s GetSubstitution defence so identifiers like `$inject` round-trip safely), writes to a sibling temp path (`.codemap-apply-.tmp`), then `renameSync` into place — POSIX-atomic per file; concurrent readers see either pre-rename or post-rename content, never a torn write. **Q2 (c) all-or-nothing**: any conflict in any file aborts phase 2 entirely before any file is touched. **Q6 gate**: TTY no `--yes` → phase-1 preview + `Proceed? [y/N]` prompt on stderr (default-N, `node:readline/promises`); TTY `--yes` → no prompt; non-TTY (CI / agents / MCP) without `--yes`/`--dry-run` rejected with stderr message. `--dry-run` + `--yes` mutually exclusive (parse-time error). MCP/HTTP transports (`handleApply`) require `yes: true` for the write path — there's no prompt to fall back on; `dry_run + yes` rejected as mutually exclusive. Result envelope (Q5; identical across modes): `{mode: 'dry-run'|'apply', applied: bool, files: [{file_path, rows_applied, warnings?}], conflicts: [{file_path, line_start, before_pattern, actual_at_line, reason}], summary: {files, files_modified, rows, rows_applied, conflicts, files_with_conflicts}}`. `applied: true` only when `mode === 'apply'` AND zero conflicts AND at least one row applied. Q7 idempotency: re-running on already-applied code reports a `line content drifted` conflict with `actual_at_line` showing the post-rename content; the user reads it and re-runs `codemap` to refresh the index → next run produces 0 rows (recipe finds nothing to rename) → vacuous clean apply. SARIF / annotations not supported (write action, not findings). TOCTOU: phase-1 reads through `sourceCache`; phase-2 transforms the cached source and writes — the gap between read and rename is a deliberate v1 simplification (apply isn't adversarial). Per Q10, only `cli/cmd-apply.ts` + `application/tool-handlers.ts` (+ the test files) may import `apply-engine.ts` for production execution — re-runnable forbidden-edge query at [§ Boundary verification — apply write path](#boundary-verification--apply-write-path). + **Show / snippet wiring:** **`src/cli/cmd-show.ts`** + **`src/cli/cmd-snippet.ts`** — sibling CLI verbs sharing the same parser shape (`` + `--kind` + `--in ` + `--json`) and the pure engine **`src/application/show-engine.ts`** (`findSymbolsByName({db, name, kind?, inPath?})` for the lookup; `readSymbolSource({match, projectRoot, indexedContentHash?})` + `getIndexedContentHash(db, filePath)` for the snippet-side FS read; **`buildShowResult`** + **`buildSnippetResult`** envelope builders — same engine the MCP show/snippet tools call). Both verbs return the same `{matches, disambiguation?}` envelope per plan § 4 uniformity — single match → `{matches: [{...}]}`; multi-match adds `{n, by_kind, files, hint}`. Snippet matches add `source` / `stale` / `missing` fields (additive — no shape divergence). **`--in `** is normalized through `toProjectRelative(projectRoot, p)` (from **`src/application/validate-engine.ts`**) so `--in ./src/cli/`, `--in src/cli`, and `--in src/cli/cmd-show.ts` all resolve identically. Stale-file behavior on `snippet`: `hashContent` (from **`src/hash.ts`** — same primitive `cmd-validate.ts` uses) compares the on-disk content_hash against `files.content_hash`; mismatch sets `stale: true` but the source IS still returned (read tool, no auto-reindex side-effects). MCP tools `show` and `snippet` register parallel to the CLI surface (see [§ MCP wiring](#cli-usage)). **Recipes wiring:** **`src/application/recipes-loader.ts`** (pure transport-agnostic loader) + **`src/application/query-recipes.ts`** (cache + public API — `getQueryRecipeSql` / `getQueryRecipeActions` / `getQueryRecipeParams` / `listQueryRecipeIds` / `listQueryRecipeCatalog` / `getQueryRecipeCatalogEntry`, shared by CLI + MCP). Recipes live as file pairs: **`.sql`** + optional **`.md`**. The loader reads `templates/recipes/` (bundled, ships in npm package next to `templates/agents/`) and `/.codemap/recipes/` (project-local — root-only resolution per the registry plan, no walk-up). Project recipes win on id collision; entries that override a bundled id carry **`shadows: true`** in the catalog so agents reading `codemap://recipes` at session start see when a recipe behaves differently from the documented bundled version. Per-row **`actions`** templates and recipe **`params`** declarations live in YAML frontmatter on each `.md` — uniform shape across bundled + project. Param types are `string | number | boolean`; CLI passes values via repeatable `--params key=value[,key=value]`, MCP / HTTP pass nested `params: {key: value}` to `query_recipe`. Validation runs before SQL binding; missing / unknown / malformed params return the same `{error}` envelope as query failures. Hand-rolled YAML parser is scoped to block-list `actions:` and `params:` only (no `js-yaml` dep). Load-time validation rejects empty SQL and DML / DDL keywords (`INSERT` / `UPDATE` / `DELETE` / `DROP` / `CREATE` / `ALTER` / `ATTACH` / `DETACH` / `REPLACE` / `TRUNCATE` / `VACUUM` / `PRAGMA`) with recipe-aware error messages — defence in depth alongside the runtime `PRAGMA query_only=1` backstop in `query-engine.ts` (PR #35). `.codemap/index.db` is gitignored; `.codemap/recipes/` is NOT (verified via `git check-ignore`) — recipes are git-tracked source code authored for human review. @@ -603,6 +605,24 @@ bun src/index.ts query --json " Expected: `[]`. Non-empty = a new write site appeared without a docs update — escalate per [`audit-pr-architecture` skill](../.agents/skills/audit-pr-architecture/SKILL.md). Read-path imports (`enrichWithRecency` / `loadRecipeRecency`) are unrestricted by design. +### Boundary verification — apply write path + +`apply-engine.ts` exports a write-only verb; only `cli/cmd-apply.ts` + `application/tool-handlers.ts` (+ the test files) may import `applyDiffPayload`. Re-runnable kit: + +```bash +bun src/index.ts query --json " + SELECT DISTINCT file_path FROM imports + WHERE source LIKE '%application/apply-engine%' + AND (specifiers LIKE '%\"applyDiffPayload\"%') + AND file_path NOT IN ('src/cli/cmd-apply.ts', + 'src/application/tool-handlers.ts', + 'src/application/apply-engine.test.ts', + 'src/cli/cmd-apply.test.ts') +" +``` + +Expected: `[]`. Non-empty = a new caller appeared without a docs update — escalate per [`audit-pr-architecture` skill](../.agents/skills/audit-pr-architecture/SKILL.md). + ## SQLite Performance Configuration ### `bun:sqlite` API diff --git a/docs/glossary.md b/docs/glossary.md index cd02c22..6d96ae0 100644 --- a/docs/glossary.md +++ b/docs/glossary.md @@ -31,6 +31,10 @@ See **language adapter**. A `.agents/rules/.md` file with YAML frontmatter. Distinct from a **skill** (longer, scenario-specific). Distinct from a **bundled recipe** (which is SQL, not Markdown). +### `codemap apply` / apply tool + +Substrate-shaped fix executor — reads the same `{file_path, line_start, before_pattern, after_pattern}` row contract `--format diff-json` emits and applies the hunks to disk. Recipe SQL is the synthesis surface; codemap is the executor (Moat-A clean — verdict-shape "should we fix this?" stays on the recipe author). CLI: `codemap apply [--params k=v[,k=v]] [--dry-run] [--yes] [--json]`. MCP: `apply` tool. HTTP: `POST /tool/apply`. **Phase 1** validates every row against current disk via `actual.includes(before_pattern)` (substring match — mirrors `buildDiffJson`'s contract); collects three conflict reasons (`file missing` / `line out of range` / `line content drifted`). **Phase 2** (gated on `!dryRun && conflicts.length === 0`) writes each modified file via sibling temp + `renameSync` for POSIX-atomic per-file writes, with `$`-pre-escape on `after_pattern` per `String.prototype.replace` GetSubstitution rule. **Q2 (c) all-or-nothing** — any conflict in any file aborts phase 2 entirely; partial writes never ship. **Q6 gate** — TTY no `--yes` triggers a `Proceed? [y/N]` prompt (default-N) on stderr; non-TTY contexts (CI / agents / MCP / HTTP) require `--yes` (or `yes: true`) explicitly. Result envelope (Q5; identical across modes): `{mode, applied, files, conflicts, summary}`. Re-running on already-applied code reports a `line content drifted` conflict whose `actual_at_line` shows the post-rename content — user re-runs `codemap` to refresh the index, then re-runs apply for a vacuous clean pass (Q7). Engine: `application/apply-engine.ts` (pure; `applyDiffPayload`). Boundary: only `cli/cmd-apply.ts` + `application/tool-handlers.ts` may import the engine — re-runnable kit at [`docs/architecture.md` § Boundary verification — apply write path](./architecture.md#boundary-verification--apply-write-path). Floor "No fix engine" preserved — codemap doesn't synthesise edits, it only executes the hunks the recipe row described. + ### audit Two-snapshot structural-drift command: `codemap audit` diffs the live `.codemap/index.db` against a base snapshot and emits `{head, deltas}` where each `deltas[]` carries `{base, added, removed}`. v1 ships three deltas: `files`, `dependencies`, `deprecated`. Each delta pins a canonical SQL projection (in `V1_DELTAS`) and a required-columns list — projects baseline rows down to that subset before diffing so schema bumps that add columns don't break pre-bump baselines. Three mutually-exclusive top-level snapshot sources: `--baseline ` (auto-resolve `-files` / `-dependencies` / `-deprecated` from `query_baselines`), `---baseline ` (explicit per-delta — composes with the others), and `--base ` (worktree + reindex against a git committish — see § A `audit --base`). Distinct from `codemap query --baseline` (that's one query, one diff; audit composes multiple per-delta diffs into one envelope). Distinct from code-quality audit tools (e.g. `knip` for unused exports, `jscpd` for duplication, framework-specific complexity linters) — those produce verdicts on dead code / dupes / complexity, which are explicit non-goals per [`roadmap.md` § Non-goals (v1)](./roadmap.md#non-goals-v1); codemap audit stays structural. diff --git a/docs/plans/codemap-apply.md b/docs/plans/codemap-apply.md deleted file mode 100644 index ad60b7a..0000000 --- a/docs/plans/codemap-apply.md +++ /dev/null @@ -1,367 +0,0 @@ -# `codemap apply ` — plan - -> **Status:** open · plan iterating. M effort. Substrate-shaped fix engine; consumes the existing `--format diff-json` row contract from any recipe (rename-preview ships as the exemplar). -> -> **Motivator:** today the `--format diff` / `--format diff-json` formatters EMIT a unified diff or structured hunks, but they're read-only previews — codemap never writes files. `codemap apply ` closes that loop: the consumer's recipe SQL describes a transformation as `{file_path, line_start, before_pattern, after_pattern}` rows; `apply` re-validates each row against current disk state and applies the hunks as actual edits. Substrate, not verdict — the recipe IS the SQL; codemap is the executor. -> -> **Tier:** M effort (~1 week). No new schema. No new engines for the heavy lifting (formatter logic is shared with `--format diff`). Conflict detection (`before_pattern` re-validation) reuses existing `formatDiffJson` plumbing. New surface is the verb itself + per-file write path + dry-run / conflict-resolution UX. - ---- - -## Pre-locked decisions - -These are committed to v1 (lifted from the [`roadmap.md § Backlog`](../roadmap.md#backlog) entry). Questions opened against them must justify against the linked floors / moats. - -| # | Decision | Source | -| --- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------- | -| L.1 | **Substrate, not verdict — Moat-A clean.** The recipe's SQL defines the transformation; `apply` is the executor. Reviewer test: "is this also expressible as `query --recipe X --format diff-json`?" — yes by construction (the rows are the same). | [Moat A](../roadmap.md#moats-load-bearing) | -| L.2 | **Reuses the existing `--format diff-json` row contract.** Required columns: `{file_path, line_start, before_pattern, after_pattern}`. New `apply` engine consumes the same `DiffJsonPayload` shape `formatDiffJson` emits. No schema additions, no new SQL primitives. | `application/output-formatters.ts` `DiffOpts` / `DiffJsonPayload` | -| L.3 | **Conflict detection re-validates `before_pattern` against current disk state.** Mirrors what `formatDiff` already does at format-time (per the existing `stale` / `missing` flags); `apply` upgrades the soft warning into a hard rejection of the conflicting hunk. | `formatDiff` + `formatDiffJson` precedent | -| L.4 | **Floor "No fix engine" preserved.** `apply` does NOT synthesise edits — it only executes the hunks the SQL row described. No semantic refactoring, no AST rewriting beyond what the recipe's `before_pattern` / `after_pattern` describe textually. | [Floor "No fix engine"](../roadmap.md#floors-v1-product-shape) | -| L.5 | **Default-safe — destructive action behind explicit opt-in.** Exact shape (dry-run-by-default vs `--yes` confirmation vs both) resolved by Q1 / Q6 below. | Project posture; matches `codemap audit` / `codemap query` v1 read-only default | -| L.6 | **Failure-mode isolation, write-side semantics.** Partial-apply behavior (transactional shape) is a deliberate v1 decision — Q2 resolves the exact contract. | New design surface | -| L.7 | **Read-side primitives unchanged.** `--format diff` and `--format diff-json` remain read-only previews — no behavior change to consumers that already pipe `query --recipe X --format diff` to a viewer. `apply` is a separate verb with its own write surface. | Backwards-compat | -| L.8 | **Input source is recipe execution (live), not stdin or a file.** `apply` runs the recipe through the same engine `query --recipe X --format diff-json` does (with `--params`), then consumes the rows directly. Out of scope for v1: `apply --from ` (revisit if a CI pipeline needs to apply a captured payload — easy additive). | Simplification — keeps the moat-A "recipe IS the SQL" framing intact | -| L.9 | **No `codemap apply` SARIF / annotations output.** Apply emits a structured success/failure envelope (Q5); SARIF is for findings, not write actions. | Output-formatter scope | - ---- - -## Open decisions (iterate as the plan converges) - -Each gets a "Resolution" subsection below as it crystallises (mirrors the recipe-recency pattern). - -- **Q1 — Default mode: dry-run-by-default vs apply-by-default vs both-with-flag.** Three shapes: - - **(a) Dry-run by default; `--apply` flag opts into writing.** Conservative — `codemap apply ` previews; `codemap apply --apply ` writes. Trade-off: the verb name "apply" preceding a non-applying default is confusing; users shell-history-replay an apply command and get unexpected dry-run output. - - **(b) Apply by default; `--dry-run` flag opts into preview.** Conventional — matches `npm install`, `git apply`, `terraform apply`. Pairs with Q6's confirmation gate as the safety net (no need for double-flagging). - - **(c) No default — require explicit `--apply` OR `--dry-run`.** Force-the-decision. Verbose; doesn't pair well with bash agent invocations. - - ### Q1 Resolution - - **Locked: (b) Apply by default; `--dry-run` opts into preview.** - - Reasoning: - - **Verb-name semantics.** A user typing `codemap apply X` and getting a dry-run is surprising. Ecosystem precedent (`npm install`, `git apply`, `terraform apply`, `cargo install`) all default to applying. - - **Pairs cleanly with Q6's confirmation gate.** Safety lives in the gate (TTY prompt + `--yes` for non-TTY), not in a double-protect default. - - **Reject (c).** Codemap has 2 modes (apply / preview); forcing both flags is friction without payoff. - - **Moat-A:** apply is a substrate verb — the recipe SQL is the synthesis surface. Defaulting to dry-run would imply codemap is making a safety judgment about the recipe; defaulting to apply puts the safety in the gate. - -- **Q2 — Transactional shape: all-or-nothing scope.** When the recipe emits hunks across N files and one file conflicts (per L.3), what happens to the others? - - **(a) Per-row** — each hunk applied independently; conflicts skip just that hunk. - - **(b) Per-file** — all hunks for a file apply together or none do. - - **(c) Per-recipe-run** — all-or-nothing across the entire row set. - - **(d) User-selected** — flag like `--mode={per-row, per-file, per-recipe}`. - - ### Q2 Resolution - - **Locked: (c) Per-recipe-run, all-or-nothing.** - - Reasoning: - - **Cross-file invariants matter for the value-add case.** `rename-preview` produces a definition row in file A + import rows in files B/C. (a) leaves files syntactically broken (half-renamed identifier sets). (b) keeps each file syntactic in isolation but breaks cross-file references (B/C importing a name that no longer exists in A). Only (c) preserves project-level consistency. - - **Two-phase implementation, no rollback machinery.** Validate-all-rows-first against current disk → if any conflict, abort with full report → otherwise write-all. The TOCTOU window between validate and write is negligible (apply isn't adversarial). - - **Re-run model.** Fix the conflict, re-run; the recipe re-executes against the now-fresh index and produces a clean row set. (c)'s "abort and report" naturally flows into that loop. - - **`git apply` precedent.** Default all-or-nothing; `--reject` opts into per-row partial. Codemap mirrors with a future `--allow-partial` flag if demand emerges. - - **(d) over-engineered for v1.** Demand can drive a `--allow-partial` toggle later; v1 ships one safe default. - -- **Q3 — Conflict-failure shape (collapsed by Q2 (c)).** Q2's all-or-nothing makes the original "skip-with-warning" path incoherent (no skip when the unit is the whole run). Reduces to "what does the failure report carry?": - - **(a) Fail-fast** — abort on first conflict; report just that one. - - **(b) Scan-and-collect** — phase-1 validation walks ALL rows, collects every conflict, then aborts with a full report. - - **(c) Auto-reindex-and-retry** — when conflict suggests staleness, re-run the recipe with a fresh index and retry once. - - ### Q3 Resolution - - **Locked: (b) Scan-and-collect.** - - Reasoning: - - **Free under Q2's two-phase implementation.** Phase 1 already walks every row; collecting all conflicts is the same cost as collecting the first. - - **Better remediation UX.** Full conflict list lets the user fix everything in one pass before retrying; (a) fail-fast forces N retry cycles for N conflicts. - - **Reject (c) auto-reindex.** Masks the staleness signal — if the index is stale, the right user-action is `codemap` then retry, not silent self-healing. Promotion gated on recurring pain. - - **Output shape** (extends Q5's envelope): - - ```json - { - "applied": false, - "conflicts": [ - { - "file_path": "...", - "line_start": 42, - "before_pattern": "...", - "actual_at_line": "...", - "reason": "line content drifted" - } - ], - "summary": { - "files": 0, - "rows_applied": 0, - "conflicts": 3, - "files_with_conflicts": 2 - } - } - ``` - - Exit code `1`; stderr one-liner points at the JSON for humans; `--json` puts the full envelope on stdout. - -- **Q4 — CLI surface.** Verb shape: - - **(a) `codemap apply `** — positional. - - **(b) `codemap apply --recipe `** — named flag, consistent with `query`. - - **(c) Both — accept positional, alias `--recipe`.** - - ### Q4 Resolution - - **Locked: (a) Positional only.** - - Reasoning: - - **Verb-shape, not query-shape.** Apply is a directed action; the recipe id is the _target_. Mirrors `codemap show ` / `codemap snippet ` / `codemap impact ` / `codemap pr-comment ` / `codemap ingest-coverage `. `query --recipe` is the exception (its positional slot is the SQL). - - **Reads naturally with params.** `codemap apply rename-preview --params old=foo,new=bar --yes`. Adding `--recipe` would be more typing for zero meaning gain. - - **Reject (c) "both."** Two ways to spell the same thing creates docs / completion / agent-prompt enumeration friction. Trivially additive if demand emerges. - - **Locked flag set for v1:** - - | Flag | Behavior | - | ------------------------------------- | ------------------------------------------------------ | - | `` | Required positional; same ids `query --recipe` accepts | - | `--params k=v[,k=v]` | Bind values for parametrised recipes (matches `query`) | - | `--dry-run` | Preview only; no writes (per Q1) | - | `--yes` | Skip TTY prompt; required for non-TTY (per Q6) | - | `--json` | Machine-readable envelope on stdout | - | `--root` / `--config` / `--state-dir` | Inherited from `bootstrapCodemap` | - -- **Q5 — Output envelope shape.** What does the JSON output look like, and is it the same shape for `--dry-run` and apply? - - **(a) Mirror / extend `DiffJsonPayload`** — re-use the formatter shape with `applied: boolean` per hunk. - - **(b) Standalone `ApplyJsonPayload`** — new shape designed for write-action semantics; same shape across both modes. - - ### Q5 Resolution - - **Locked: (b) Standalone `ApplyJsonPayload`, single shape across modes.** - - ```typescript - interface ApplyJsonPayload { - mode: "dry-run" | "apply"; - applied: boolean; // true only when mode=apply AND zero conflicts - files: ApplyFile[]; // files that were/would-be modified - conflicts: ConflictRow[]; // phase-1 validation results in both modes - summary: { - files: number; // distinct file_paths in row set - files_modified: number; // 0 in dry-run - rows: number; // total input rows - rows_applied: number; // 0 in dry-run - conflicts: number; - files_with_conflicts: number; - }; - } - interface ApplyFile { - file_path: string; - rows_applied: number; // 0 in dry-run; non-zero only after write - warnings?: string[]; - } - interface ConflictRow { - file_path: string; - line_start: number; - before_pattern: string; - actual_at_line: string; - reason: string; // "line drifted" | "file missing" | … - } - ``` - - Reasoning: - - **Same shape across modes.** Both `--dry-run` and apply run phase-1 validation and produce `conflicts: [...]`. Apply additionally writes after a clean validation; sets `applied: true` and populates `files[].rows_applied`. Consumers pattern-match on `mode` + `applied`. - - **Reject (a) extending `DiffJsonPayload`.** Couples read-only preview formatter to write semantics — same kind of read/write coupling Slice 3 of recipe-recency unwound. - - **`--dry-run` does NOT emit `DiffJsonPayload`.** Raw hunks are already available via `query --recipe X --format diff-json`. `apply --dry-run` adds value because it runs the validation pass and includes `conflicts: [...]` — "what would actually happen", not "what does the recipe describe." - - **Exit codes:** `0` on clean apply OR clean dry-run; `1` on any conflicts (both modes). - - **Text mode (default, no `--json`):** one-line human summary — - - Apply success: `apply rename-preview: modified 3 files, applied 5 rows` - - Apply with conflicts: `apply rename-preview: aborted (3 conflicts in 2 files); see --json for details` - - Dry-run: `apply rename-preview --dry-run: would modify 3 files (5 rows). Re-run without --dry-run to apply.` - -- **Q6 — Permission gating: confirmation prompt + `--yes`.** - - **(a) TTY prompt + `--yes` to skip** — interactive gets y/n; non-interactive requires `--yes`. - - **(b) Always-prompt** — even with `--yes`. - - **(c) `--yes` only — no prompt** — non-interactive everywhere. - - ### Q6 Resolution - - **Locked: (a) TTY prompt + `--yes` to skip.** - - | Context | Behavior | - | ------------------------ | --------------------------------------------------------------------------------------------------- | - | TTY, no `--yes` | Phase 1 runs. Print summary. Prompt `Proceed? [y/N]`. Default `N`. Phase 2 only on `y`. | - | TTY, `--yes` | Skip prompt; proceed if validation clean. | - | Non-TTY, no `--yes` | Reject: `codemap apply: this verb writes files. Pass --yes for non-interactive runs, or --dry-run.` | - | Non-TTY, `--yes` | Proceed if validation clean. | - | Any context, `--dry-run` | Skip the prompt entirely; `--yes` is a no-op. | - - Reasoning: - - **Ecosystem precedent.** Matches `gh` / `git` / `cargo` / `npm uninstall`. Universal pattern; users don't get surprised. - - **TTY detection via `process.stdout.isTTY`** (Node + Bun support identically). - - **Reject (b).** Prompting under `--yes` adds friction with no payoff; `--yes` should mean "I know what I'm doing." - - **Reject (c).** TTY users lose the safety net for free. - - **Default-N on prompt.** Pressing Enter aborts. Matches `gh repo delete` / `cargo uninstall`. - - **Prompt content** (printed before the y/N): - - ```text - apply rename-preview: 3 files, 5 rows - - src/foo.ts (2 rows) - - src/bar.ts (1 row) - - src/baz.ts (2 rows) - - Proceed? [y/N] - ``` - -- **Q7 — Idempotency: re-running on already-applied code.** If the user re-runs `codemap apply rename-preview --params old=foo,new=bar` on a project where the rename already landed: - - **(a) Conflict-only path** — `before_pattern` (`foo`) doesn't match disk (`bar`); phase-1 reports as a conflict with `actual_at_line: "bar"`. User reads the diagnostic, runs `codemap` to refresh; re-run produces 0 rows (recipe finds nothing to rename) → vacuous clean apply. - - **(b) Three-state phase-1: target / already-applied / conflict** — extra branch: if `after_pattern` is at `line_start`, treat as already-applied (silent skip, `summary.already_applied: N`). Re-runs without reindex are no-ops. - - ### Q7 Resolution - - **Locked: (a) Conflict-only path.** - - Reasoning: - - **Simpler state machine.** Two states (apply target / conflict) vs three (target / already-applied / conflict). Less to document, less to test, less to misread. - - **Conflict report is self-explanatory.** Q5's `actual_at_line` field shows what's on disk; user sees `actual_at_line: "bar"` against `before_pattern: "foo"` and immediately understands "the rename already landed." No interpretation lost vs (b). - - **Forces index hygiene.** Re-running on a stale index hits a real conflict — the right user-action is `codemap` then retry, not silent self-healing. Same reasoning that rejected Q3 (c) auto-reindex. - - **Moat-A clean.** Codemap reports `disk[line] != before_pattern`; it does NOT interpret "this looks like the rename you already ran." Verdict-shape avoided. - - **Promotion path is additive.** If real users complain about reindex-then-retry friction, ship (b) as additional phase-1 metadata: `summary.already_applied: N` + per-file `rows_already_applied`. No shape break for existing consumers; existing fields remain accurate. - -- **Q8 — `before_pattern` matching semantics.** - - **(a) Substring match (single line)** — `disk[line_start - 1].includes(before_pattern)` and replace via `disk[line_start - 1].replace(before_pattern, after_pattern)` (first occurrence). Matches the existing `buildDiffJson` formatter contract. - - **(b) Whitespace-tolerant** — collapse runs of whitespace before comparing. - - **(c) Whole-line exact match** — `disk[line_start - 1] === before_pattern`. - - ### Q8 Resolution - - **Locked: (a) Substring match (single line) — verbatim with the existing `buildDiffJson` formatter contract.** - - **Phase-1 algorithm:** - 1. Load file at `file_path`; split into 1-indexed lines via `source.split(/\r?\n/)`. - 2. `actual = lines[line_start - 1]`. - 3. If file unreadable → conflict (`reason: "file missing"`). - 4. If `actual === undefined` (line past EOF) → conflict (`reason: "line out of range"`). - 5. If `!actual.includes(before_pattern)` → conflict (`reason: "line content drifted"`); `actual_at_line = actual`. - 6. Otherwise → row passes; phase-2 transformation is `actual.replace(before_pattern, after_pattern)` (first occurrence). Pre-escape `$` in `after_pattern` per `String.prototype.replace`'s `GetSubstitution` rule (`replace(/\$/g, "$$$$")` — mirrors `buildDiffJson` line 475). - - Reasoning: - - **Matches the formatter contract verbatim.** `application/output-formatters.ts` `buildDiffJson` uses `actual.includes(before)` + `actual.replace(before, after)` with the `$` pre-escape. The exemplar `templates/recipes/rename-preview.sql` emits `before_pattern = old_name` (the bare identifier, e.g. `"foo"`), NOT the full line. Whole-line exact match would conflict every time on `const foo = 1;`. - - **Single line per row.** Hunks today are 1-line removals + 1-line additions (`old_count: 1, new_count: 1` in `buildDiffJson`). Multi-line transforms are out of scope for v1; promote with a schema-aware extension if real recipes need it. - - **Reject (b) whitespace-tolerant.** Tolerance grows over time (collapse runs → trim → ignore EOL → …); each addition is more interpretation. Recipe-side normalization is the right substrate (recipe SQL controls what `before_pattern` contains). - - **Reject (c) whole-line exact.** Contradicts the formatter precedent and breaks every shipped recipe row. - - **Edge cases handled by (a):** - - | Disk state | `before_pattern` | Outcome | - | -------------------------------------- | ---------------- | ------------------------------------------------------------------------------------------------------------------------------- | - | File missing | any | conflict; `reason: "file missing"` | - | `line_start` past EOF | any | conflict; `reason: "line out of range"` | - | `before_pattern` not in line | any | conflict; `reason: "line content drifted"`; `actual_at_line` shows what's there | - | `before_pattern` appears twice in line | any | only the first occurrence replaces (matches formatter); recipe author should split into two rows or use a more specific pattern | - | `after_pattern` contains `$` / `$1` | any | escaped before `replace()` so identifiers like `$inject` round-trip safely | - -- **Q9 — Test approach.** - - **Unit:** `apply-engine.ts` — `applyDiffPayload({rows, projectRoot, dryRun})` + helpers. Temp-dir scenarios for happy path / conflict / dry-run / file-missing / out-of-range / `$`-pattern escape / row-shape validation. - - **Integration:** subprocess CLI tests against a fixture project (mirrors `cmd-query-recency.test.ts`) — full pipeline from `codemap apply rename-preview --params … --yes` through to disk-state assertions. TTY-prompt path tested via `--yes` flag (skipping prompt); non-TTY-no-`--yes` rejection tested explicitly. - - **Golden:** add a recipe-execution scenario to `fixtures/golden/scenarios.json` covering the dry-run output shape (deterministic; doesn't write to disk). - - ### Q9 Resolution - - **Locked: three-layer approach as drafted, with two explicit non-tests:** - 1. **No TOCTOU race tests.** Q2 accepted the validate→write window as negligible (apply isn't adversarial). Document the assumption in the engine docstring; don't add flaky timing-based tests. - 2. **No filesystem DI.** Match `coverage-engine.ts` precedent — direct `readFileSync` / `writeFileSync` with temp-dir scoping at the test boundary. No injection overhead. - 3. **No clock injection.** Apply's logic is time-independent (unlike recipe-recency); skip the `clock` seam. - - Reasoning: - - **Mirrors recipe-recency precedent.** Same three-layer split, same subprocess-style integration tests, same golden-fixture extension. - - **Failure-mode tests live in unit + integration.** Read-only file (`fs.chmodSync(0o444)` in temp dir), partial-write crash simulation (kill mid-write — covered as a unit test with mocked `writeFileSync`). - - **Fixture project for integration.** Reuses `tests/fixtures/sample-project/` shape; adds a tiny rename-target file the integration test can assert against. - -- **Q10 — Boundary discipline + plan-file lifecycle.** - - ### Q10 Resolution - - **Locked: codify imports + delete-on-ship.** - 1. **Boundary check (write-path).** Only `cli/cmd-apply.ts` and `application/tool-handlers.ts` (the MCP/HTTP `apply` tool) import `apply-engine.ts` for production execution. Tests import directly. Codified via the same SQL kit recipe-recency uses (mirror at `architecture.md § Boundary verification — apply write path`): - - ```sql - SELECT file_path - FROM imports - WHERE source LIKE '%application/apply-engine%' - AND file_path NOT LIKE '%test%' - AND file_path NOT LIKE '%cli/cmd-apply%' - AND file_path NOT LIKE '%application/tool-handlers%'; - -- Must return 0 rows. - ``` - - 2. **Plan-file lifecycle.** Delete this file after Slice 5 ships per `docs/README.md` Rule 3. Lift durable design into `architecture.md § codemap apply wiring`: - - Engine API (`applyDiffPayload` signature + envelope shape from Q5). - - Phase-1 algorithm (Q8 exact-match + multi-line splitting). - - Exit-code contract (Q5 — `0` clean / `1` conflicts). - - Q6 TTY/`--yes` gate matrix. - - Q10 boundary SQL kit (re-runnable, like recipe-recency's). - - Update `docs/glossary.md` with a `codemap apply` entry. - - Update `agent rule + skill` in lockstep per `docs/README.md` Rule 10. - - Reasoning: - - **Boundary discipline mirrors recipe-recency.** Same write-path / read-path split (apply has no read-path — it's a write-only verb, so the SQL kit is simpler). - - **Closing-state lifecycle is "delete + lift," not "slim & keep in plans/."** Per `docs-governance` skill — plans don't survive the ship. - ---- - -## High-level architecture (sketch — refines as Qs lock) - -Three new pieces; engine reuses existing diff-formatter primitives. - -1. **Engine (`src/application/apply-engine.ts`, new)** — pure transport-agnostic `applyDiffPayload({rows, projectRoot, dryRun})`. Mirrors `coverage-engine.ts` shape: - - Validates row shape (same `DiffOpts` contract). - - **Phase 1 (both modes):** for each row, read source, validate `before_pattern` against `disk[line_start..line_start + N]` per Q8. - - **Phase 2 (apply only, all-validated):** write each modified file once via `writeFileSync` to a sibling temp path, then `rename` (atomic — guards against torn writes mid-process-crash). All-or-nothing per Q2 (c) — phase 2 is skipped entirely if phase 1 collects any conflicts. - - Returns the Q5-shaped result envelope; `mode` is derived from `dryRun` (`dryRun ? "dry-run" : "apply"`). - -2. **CLI shell (`src/cli/cmd-apply.ts`, new)** — argv parsing + bootstrap + dispatch. Mirrors `cmd-impact.ts` shape (positional target + flags + JSON envelope). Resolves `--recipe` → SQL → executes via `executeQuery` → passes rows to `apply-engine`. - -3. **MCP/HTTP transport** — `application/tool-handlers.ts` gets a new `apply` tool. Same args envelope shape as `query_recipe` (recipe id + params + format) plus `dry_run` (preview, per Q1) and `yes` (non-interactive write gate, per Q6) keys. Apply-by-default mirrors the CLI; non-TTY transports always require `yes: true` because they have no prompt to fall back on. Resource handlers untouched (apply is a tool, not a resource). - -No schema delta. No new SQL primitives. The engine consumes the existing `query-engine.ts` `executeQuery` output. - ---- - -## Implementation slices (tracer bullets) - -Per [`tracer-bullets`](../../.agents/rules/tracer-bullets.md) — ship one vertical slice end-to-end before expanding. - -1. **Slice 1: engine + dry-run path.** `apply-engine.ts` with `applyDiffPayload({rows, projectRoot, dryRun: true})` returning the Q5-shaped envelope. Unit tests covering happy path / conflict / file-missing. No CLI yet — verify via direct engine import. -2. **Slice 2: write path.** Add `dryRun: false` branch — `writeFileSync` per file with atomic temp-rename. Q2 transactional unit enforced. Failure-mode unit tests (read-only file, partial-write crash simulation). -3. **Slice 3: CLI + recipe execution.** `cmd-apply.ts` argv + bootstrap + dispatch. `--params` plumbing matches `query --recipe`. Q6 TTY-prompt + `--yes` gate. Subprocess integration tests against a fixture project (mirrors `cmd-query-recency.test.ts`). -4. **Slice 4: MCP/HTTP tool.** `apply` tool registration + `tool-handlers.ts` handler. Output shape matches the CLI's `--json` envelope. `query_batch` doesn't get an `apply` analogue (Moat-A: batched writes are verdict-shaped; consumers compose multiple `apply` calls). -5. **Slice 5: docs lockstep + plan retire.** `docs/architecture.md` § `apply` wiring; `docs/glossary.md` `codemap apply` entry; agent rule + skill in lockstep (Rule 10); roadmap entry removed; **plan file deleted** per Rule 3. - ---- - -## Test approach - -Covered inline at Q9. Each slice ships its own tests; Slice 5 runs the docs / agent-surface lockstep. - ---- - -## Risks / non-goals - -| Item | Mitigation | -| --------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| **Non-goal:** verdict-shaped fix engine (auto-detect dead code, refactor suggestions, AST rewrite). | Per L.4 (Floor "No fix engine"). `apply` only executes the diff hunks the SQL row describes — recipe SQL is the synthesis surface, not the engine. | -| **Non-goal:** apply from arbitrary stdin / file payload (`apply --from `). | Per L.8. Recipe-execution-only for v1 keeps "the recipe IS the SQL" framing intact. Promote if a CI pipeline asks with a concrete shape. | -| **Non-goal:** SARIF / annotations output for `apply`. | Per L.9. SARIF is for findings; apply has its own envelope. Q5 resolves the shape. | -| **Risk:** apply leaves files in a broken syntactic state on conflict. | Per Q2 (c) per-recipe-run all-or-nothing — any conflict in any file aborts the whole run before phase 2; partially-edited files never ship. Atomic temp-rename guarantees no torn writes mid-process-crash. | -| **Risk:** stale index → stale `line_start` → wrong line edited. | Per L.3 — `before_pattern` re-validation catches every staleness mode. Recipe is re-run inside `apply` (per L.8) so the rows reflect the current index, not a captured snapshot. | -| **Risk:** non-TTY contexts (CI, MCP) accidentally write without confirmation. | Per Q6 (a) — non-TTY requires explicit `--yes`. Agents must opt in; CI workflows must list the flag. | -| **Risk:** plan abandoned mid-iteration. | Per [`docs/README.md` Rule 8](../README.md), close as `Status: Rejected (YYYY-MM-DD) — `. The engine slice (Slice 1) is independently useful even if the CLI never lands (could become a programmatic API). | - ---- - -## Cross-references - -- [`docs/roadmap.md § Backlog`](../roadmap.md#backlog) — `codemap apply` entry (deleted by Slice 5 cleanup). -- [`docs/architecture.md`](../architecture.md) — destination for the durable design (Slice 5). -- `application/output-formatters.ts` `DiffOpts` / `DiffJsonPayload` / `formatDiffJson` — input contract reused verbatim. -- `templates/recipes/rename-preview.{sql,md}` — the exemplar parametrised recipe consumed by `apply`. -- [`docs/README.md` Rule 3](../README.md) — plan-file convention (this file's location + deletion-on-ship). -- [`docs/README.md` Rule 10](../README.md) — agent rule + skill lockstep update (Slice 5). -- [`.agents/rules/tracer-bullets.md`](../../.agents/rules/tracer-bullets.md) — slice cadence. -- [`.agents/skills/docs-governance/SKILL.md § Closing a plan`](../../.agents/skills/docs-governance/SKILL.md#closing-a-plan) — delete-on-ship discipline. diff --git a/docs/roadmap.md b/docs/roadmap.md index b4399d9..3f00de6 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -55,7 +55,6 @@ Soft constraints — describe shipped reality. Decided-but-unshipped flips live - [ ] **`history` table** (deferred — revisit-triggered) — temporal queries: "when did symbol X get `@deprecated`?", "coverage trend over last 50 commits", "files that became dead this week". `audit --base ` covers the most-common temporal question (PR-scoped diff) without schema growth, so the table earns its place only when bigger questions emerge. Two shapes (per-commit snapshots ~N × DB size; append-only event log heavier CTE walks); both pay an N-reindexes backfill cost (~30s per reindex). **Revisit triggers:** two consumers ship `jq`-based "audit-runs-over-time" workflows, OR `query_baselines` evolution becomes a recurring agent need. - [ ] **`codemap audit` verdict + thresholds** (v1.x) — `verdict: "pass" | "warn" | "fail"` driven by an `audit.deltas[].{added_max, action}` field on the config object (`.codemap/config.{ts,js,json}`). Triggers: two consumers ship `jq`-based threshold scripts with similar shapes, OR one consumer asks with a concrete config sketch. Until then, raw deltas + consumer-side `jq` is the CI exit-code idiom. **Likely accelerant:** the Marketplace Action (next item) shipping is the most plausible path to firing the trigger — once `- uses: stainless-code/codemap@v1` is the dominant CI path, real `jq` threshold scripts will surface. - [ ] **GitHub Marketplace Action — `stainless-code/codemap@v1`** — composite action wrapping `codemap audit --base ${{ github.base_ref }} --ci` (default) + auto-detect package manager (via [`package-manager-detector`](https://github.com/antfu-collective/package-manager-detector)) + opt-in PR-comment writer. Most primitives already shipped (PR #43 SARIF/annotations on `query`, PR #26 `--changed-since`/`--group-by`/`--summary`, PR #30 baselines, PR #52 `audit --base `, PR #72 boundary-violations). **Two genuine new CLI surfaces** ship alongside: `--format sarif` on `audit` (today only emits `--json`) and the `--ci` aggregate flag on `query` + `audit` (alias for `--format sarif` + non-zero exit + quiet). Action version stream is independent of CLI version (CLI at `0.4.0`; Action publishes at its own `v1.0.0`). Distribution multiplier: Marketplace is the dominant discovery surface for this tool cohort and codemap is currently absent from it. Plan: [`plans/github-marketplace-action.md`](./plans/github-marketplace-action.md). Effort: M. -- [ ] **`codemap apply `** — substrate-shaped fix engine: apply any diff hunks your SQL describes, with conflict detection (re-parse, verify `before_pattern` still matches at `line_start`). Reuses the existing `--format diff-json` formatter (which already emits `{file_path, line_start, before_pattern, after_pattern}` rows from any recipe — see `rename-preview.sql` for an exemplar). **Substrate, not verdict:** the consumer's recipe SQL defines the transformation; codemap is the executor. Reviewer test (Moat A): "is this also expressible as `query --recipe X --format diff-json`?" — yes, because the recipe IS the SQL. Effort: ~1 week (M). **Needs a plan PR before impl** — design questions: dry-run mode, multi-hunk transactional apply, conflict-resolution UX. - [ ] **AST-hash duplication** — `symbols.body_hash` column (normalized AST hash via oxc, computed at parse time — Rust-native, fast) + bundled `duplicates.sql` recipe joining on `body_hash` (`SELECT * FROM symbols GROUP BY body_hash HAVING COUNT(*) > 1`). **Different shape from token-level suffix-array dupes** (catches structurally-identical functions, not copy-paste with renamed variables). Substrate addition — consumer writes the JOIN that decides "this is a problem"; no severity, no suppression-by-default. Effort: ~2 weeks (M). **Needs a plan PR before impl** — design questions: which oxc visitor scope (function bodies only? expressions? include comments?), what counts as "structurally identical" (rename-aware? whitespace-tolerant?), schema delta. - [ ] **Falsifiable benchmark CI** — codemap vs `find` + `grep` + `Read`-loop agent-discovery on named fixtures (zod, fastify, vue-core, next.js). Numbers land in [`docs/benchmark.md`](./benchmark.md) and ~3 surface in `MARKETPLACE.md`. Replaces the unfalsifiable "sub-millisecond" claim with named-fixture comparisons that any consumer can re-run. Effort: M. - [ ] **Repo-structure conversion (codemap itself: flat → monorepo)** — tracked decision, not a backlog item to ship. Default bias: stay flat until a trigger fires (C.9 community plugins ship as separate packages, OR a user asks for `codemap-core` library export, OR a second distro emerges). Full analysis + three options + reference layouts (oxc / knip / biome / vitest) + revisit triggers in [`plans/lsp-diagnostic-push.md § Repo-structure tradeoffs`](./plans/lsp-diagnostic-push.md#repo-structure-tradeoffs-canonical-home-for-the-monorepo-vs-flat-decision). Don't convert preemptively. diff --git a/templates/agents/rules/codemap.md b/templates/agents/rules/codemap.md index 2dee163..ce9b079 100644 --- a/templates/agents/rules/codemap.md +++ b/templates/agents/rules/codemap.md @@ -42,6 +42,7 @@ Install **[@stainless-code/codemap](https://www.npmjs.com/package/@stainless-cod | Targeted read (metadata) | `codemap show [--kind ] [--in ] [--json]` — file:line + signature | | Targeted read (source text) | `codemap snippet [--kind ] [--in ] [--json]` — same lookup + source from disk + stale flag | | Impact (blast-radius walker) | `codemap impact [--direction up\|down\|both] [--depth N] [--via ] [--limit N] [--summary] [--json]` — replaces hand-composed `WITH RECURSIVE` queries | +| Apply (substrate fix executor) | `codemap apply [--params k=v[,k=v]] [--dry-run] [--yes] [--json]` — executes the diff hunks a recipe describes (one per `{file_path, line_start, before_pattern, after_pattern}` row). Q6 gate: TTY prompt; non-TTY needs `--yes` (or `--dry-run`). Q2 (c) all-or-nothing — any conflict aborts before any file is touched. | | Coverage ingest | `codemap ingest-coverage [--runtime] [--json]` — Istanbul (`coverage-final.json`) and LCOV (`lcov.info`) auto-detect from path; V8 is **opt-in** via `--runtime` (treats `` as a `NODE_V8_COVERAGE=...`-style directory of `coverage-*.json` dumps). Joinable to `symbols` for "untested AND dead" queries. Local-only — no SaaS aggregation. | | SARIF / GH annotations | `codemap query --recipe deprecated-symbols --format sarif` · `… --format annotations` | | `--ci` aggregate flag | `codemap query -r deprecated-symbols --ci` (or `audit --base origin/main --ci`) — aliases `--format sarif` + non-zero exit when findings/additions surfaced + suppresses the no-locatable-rows stderr warning. Mutually exclusive with `--json` / `--format `. | @@ -82,9 +83,11 @@ Validation: SQL is rejected at load time if it starts with DML/DDL (DELETE/DROP/ **Impact (`codemap impact `)**: symbol/file blast-radius walker — replaces hand-composed `WITH RECURSIVE` queries that agents struggle to write. Target auto-resolves: contains `/` or matches `files.path` → file target; otherwise symbol (case-sensitive). Walks compatible graphs by target kind: **symbol** → `calls` (callers / callees by name); **file** → `dependencies` + `imports` (`resolved_path` only). `--via ` overrides; mismatched explicit choices land in `skipped_backends` (no error). Cycle-detected via `WITH RECURSIVE` path-string + `instr` check; bounded by `--depth N` (default 3, `0` = unbounded but still cycle-detected and limit-capped) and `--limit N` (default 500). Output envelope: `{target, direction, via, depth_limit, matches: [{depth, direction, edge, kind, name?, file_path}], summary: {nodes, max_depth_reached, by_kind, terminated_by: 'depth'|'limit'|'exhausted'}}`. `--summary` trims `matches` for cheap CI-gate consumption (`jq '.summary.nodes'`) but preserves the count. SARIF / annotations not supported (graph traversal, not findings). Pure transport-agnostic — same logic across CLI, MCP, and HTTP. +**Apply (`codemap apply `)**: substrate-shaped fix executor over the existing `--format diff-json` row contract — recipe SQL is the synthesis surface, codemap executes. Phase 1 validates every `{file_path, line_start, before_pattern, after_pattern}` row against current disk via `actual.includes(before_pattern)` (substring match — same contract `buildDiffJson` uses); collects three conflict reasons (`file missing` / `line out of range` / `line content drifted`). Phase 2 (gated on `!dryRun && conflicts.length === 0`) writes via sibling temp + `rename` for POSIX-atomic per-file writes. **Q2 (c) all-or-nothing** — any conflict aborts the whole run before any file is touched. **Q6 gate** — TTY prompts `Proceed? [y/N]` (default-N) on stderr; non-TTY (CI / agents / MCP / HTTP) requires `--yes` (or `yes: true`) explicitly; `--dry-run` + `--yes` mutually exclusive. Q7 idempotency: re-running on already-applied code reports `line content drifted` with `actual_at_line` showing the post-rename content — re-run `codemap` to refresh the index, then re-run apply (vacuous clean pass). Output envelope (identical across modes): `{mode: 'dry-run'|'apply', applied: bool, files: [{file_path, rows_applied, warnings?}], conflicts: [{file_path, line_start, before_pattern, actual_at_line, reason}], summary: {files, files_modified, rows, rows_applied, conflicts, files_with_conflicts}}`. Pure transport-agnostic engine in `application/apply-engine.ts`; CLI / MCP / HTTP all dispatch the same `applyDiffPayload` function. + **MCP server (`codemap mcp`)**: stdio MCP (Model Context Protocol) server — agents call codemap as JSON-RPC tools instead of shelling out to the CLI on every read. v1 ships one tool per CLI verb plus six resources (`codemap://recipes` + `codemap://recipes/{id}` are live read every call so inline `last_run_at` / `run_count` recency stays fresh; `codemap://schema` + `codemap://skill` lazy-cache; `codemap://files/{path}` + `codemap://symbols/{name}` always live): -- **Tools:** `query` / `query_batch` / `query_recipe` / `audit` / `save_baseline` / `list_baselines` / `drop_baseline` / `context` / `validate` / `show` / `snippet` / `impact`. Snake_case keys (Codemap convention matching MCP spec examples + reference servers — spec is convention-agnostic; CLI stays kebab). +- **Tools:** `query` / `query_batch` / `query_recipe` / `audit` / `save_baseline` / `list_baselines` / `drop_baseline` / `context` / `validate` / `show` / `snippet` / `impact` / `apply`. Snake_case keys (Codemap convention matching MCP spec examples + reference servers — spec is convention-agnostic; CLI stays kebab). - **`query_batch` (MCP-only):** N statements in one round-trip. Items are `string | {sql, summary?, changed_since?, group_by?}` — string form inherits batch-wide flag defaults, object form overrides on a per-key basis. Per-statement errors are isolated. - **`save_baseline` (polymorphic):** one tool, `{name, sql? | recipe?}` with runtime exclusivity check (mirrors the CLI's single `--save-baseline=` verb). - **Resources:** `codemap://recipes` (catalog — live), `codemap://recipes/{id}` (one recipe — live), `codemap://schema` (live DDL from `sqlite_schema`; lazy-cached), `codemap://skill` (bundled SKILL.md text; lazy-cached), `codemap://files/{path}` (per-file roll-up: symbols, imports, exports, coverage — live), `codemap://symbols/{name}` (symbol lookup with `{matches, disambiguation?}` envelope; `?in=` filter mirrors `show --in` — live). Recipe catalogs read live every call so inline `last_run_at` / `run_count` recency reflects mutations during the server lifetime; `schema` / `skill` cache because their inputs don't change mid-session. diff --git a/templates/agents/skills/codemap/SKILL.md b/templates/agents/skills/codemap/SKILL.md index 33b48f4..33bd306 100644 --- a/templates/agents/skills/codemap/SKILL.md +++ b/templates/agents/skills/codemap/SKILL.md @@ -79,6 +79,7 @@ Each emitted delta carries its own `base` metadata so mixed-baseline audits are - **`show`** — `{name, kind?, in?}`. Exact, case-sensitive symbol name lookup. Returns `{matches: [{name, kind, file_path, line_start, line_end, signature, ...}], disambiguation?: {n, by_kind, files, hint}}`. Single match → `{matches: [{...}]}`; multi-match adds the disambiguation envelope so you narrow without re-scanning. Fuzzy lookup belongs in `query` with `LIKE`. - **`snippet`** — `{name, kind?, in?}`. Same lookup as `show` but each match also carries `source` (file lines from disk at `line_start..line_end`), `stale` (true when content_hash drifted since indexing — line range may have shifted), `missing` (true when file is gone). `source` is always returned when the file exists; agent decides whether to act on stale content or run `codemap` / `codemap --files ` to re-index first. No auto-reindex side-effects from this read tool. - **`impact`** — `{target, direction?, via?, depth?, limit?, summary?}`. Symbol/file blast-radius walker — replaces hand-composed `WITH RECURSIVE` queries that agents struggle to write reliably. `target` is a symbol name (case-sensitive, exact) OR a project-relative file path (auto-detected by `/` or by matching `files.path`). `direction`: `up` (callers / dependents), `down` (callees / dependencies), `both` (default). `via`: `dependencies`, `calls`, `imports`, `all` (default — every backend compatible with the resolved target kind: symbol → `calls`; file → `dependencies` + `imports`; mismatched explicit choices land in `skipped_backends`, no error). `depth` default 3, `0` = unbounded (still cycle-detected and limit-capped). `limit` default 500. `summary: true` trims `matches` for cheap CI-gate consumption (`jq '.summary.nodes'`) but preserves the count. Result: `{target, direction, via, depth_limit, matches: [{depth, direction, edge, kind, name?, file_path}], summary: {nodes, max_depth_reached, by_kind, terminated_by: 'depth'|'limit'|'exhausted'}}`. Cycle detection is approximate-but-bounded — bounded depth + `LIMIT` keep cyclic graphs cheap; `terminated_by` reports the dominant stop reason. SARIF / annotations not supported (impact rows are graph traversals, not findings). +- **`apply`** — `{recipe, params?, dry_run?, yes?}`. Substrate-shaped fix executor — runs the same recipe `query_recipe` runs, then applies the diff hunks each row describes (`{file_path, line_start, before_pattern, after_pattern}`) to disk. Recipe SQL is the synthesis surface; codemap is the executor. Phase 1 validates every row against current disk via substring-match (`actual.includes(before_pattern)`) — the same contract `--format diff-json` uses. Phase 2 (gated on no conflicts) writes via sibling temp + `rename` for POSIX-atomic per-file writes. **Q2 (c) all-or-nothing** — any conflict in any file aborts phase 2 entirely; partial writes never ship. Over MCP/HTTP `yes: true` is required for the write path (no TTY prompt to fall back on); `dry_run: true` previews without writing; the two are mutually exclusive. Re-running on already-applied code reports a `line content drifted` conflict whose `actual_at_line` shows the post-rename content — re-run `codemap` to refresh the index, then re-run `apply` for a clean vacuous pass. Result envelope (identical across modes): `{mode: 'dry-run'|'apply', applied, files: [{file_path, rows_applied, warnings?}], conflicts: [{file_path, line_start, before_pattern, actual_at_line, reason}], summary: {files, files_modified, rows, rows_applied, conflicts, files_with_conflicts}}`. Floor "No fix engine" preserved — codemap doesn't synthesise edits, it only executes the hunks the recipe row described. **Resources (mostly lazy-cached on first `read_resource`; recipes / one-recipe live-read every call so the inline recency fields stay fresh):** From bdf7ef3811802bd1d23e6cf7a9fbba6c042879c3 Mon Sep 17 00:00:00 2001 From: Sutu Sebastian Date: Wed, 6 May 2026 13:08:57 +0300 Subject: [PATCH 6/9] fix(apply): path-containment + overlap detection (triangulated review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Lands four fixes from a triangulated review of three independent agent audits (Composer, GPT-5.5, Codex). Two HIGH-severity correctness bugs were each reproducible against the prior `apply-engine.ts` in 30 seconds: F1 (HIGH) — Path traversal. Pre-fix: applyDiffPayload({ rows: [{ file_path: "../outside.ts", ... }], projectRoot: "/tmp/proj/", dryRun: false }) returned `applied: true` and mutated a sibling-of-root file. Now phase 1 resolves the project root once and rejects (a) absolute `file_path` inputs and (b) any candidate whose `path.resolve(resolvedRoot, file_path)` lands outside it. New conflict reason: `path escapes project root`. F2 (HIGH) — Phase-2 partial cross-file write. Pre-fix: two rows on the same `(file_path, line_start)` both passed phase-1 (substring check against original source); phase-2 applied the first replace, the second's substring assertion failed, the function threw — AFTER earlier files in alphabetical order had already been `renameSync`d. The "Q2 (c) all-or-nothing" guarantee was demonstrably broken. Now phase 1 maintains a per-file Set; the second hit at the same line emits a `duplicate edit on same line` conflict before any write. F3 (MEDIUM, doc-first) — Same-line `before_pattern` ambiguity. The formatter precedent (`buildDiffJson`) uses `actual.replace(before, after)`, which rewrites only the leftmost occurrence. `const foo = foo();` with `before = "foo"` becomes `const bar = foo();` — variable renamed, recursive call broken, `applied: true` reported. This mirrors the formatter exactly and the `--format diff` preview shows the same shape, so the audit's recommendation of an engine-level fix would diverge preview from execution. Documented as a deliberate limitation in the engine docstring + `architecture.md § Apply wiring` caveat instead; test pins the current behaviour so a future engine change lands as a deliberate breaking change rather than silent drift. F4 + F6 (LOW) — `apply-engine.ts` docstring no longer points at the deleted plan (now links to `docs/architecture.md` for durable design); `apply-engine` added to the `application/` row of the Key Files table in architecture.md (it was meant to be in that enumeration alongside the other 14 engines). Tests: 25 unit tests (8 new — three F1 paths, one F2 repro, one F3 limitation pin, plus existing happy-paths / failure-modes); 41 pass across the apply path. Boundary kit returns []. Changeset entry amended with the path-containment + overlap-detection bullets so the release notes carry the security-relevant fixes. Triangulated audit doc + the three source agent reviews are NOT checked in — they served their purpose for this fix-up commit and removing them avoids stale "review backlog" cruft per docs-governance. Follow-up (separate PR): the audit also surfaced that `DEFAULT_EXCLUDE_DIR_NAMES` in src/config.ts doesn't include `.codemap`, so `audit --base` followed by `--full` walks the audit-cache subtree. Tracked separately because the gap predates this PR. --- .changeset/codemap-apply.md | 4 +- docs/architecture.md | 42 +++---- src/application/apply-engine.test.ts | 157 +++++++++++++++++++++++++++ src/application/apply-engine.ts | 95 ++++++++++++++-- 4 files changed, 269 insertions(+), 29 deletions(-) diff --git a/.changeset/codemap-apply.md b/.changeset/codemap-apply.md index 9b64d0b..3a510c8 100644 --- a/.changeset/codemap-apply.md +++ b/.changeset/codemap-apply.md @@ -18,7 +18,9 @@ All three dispatch the same pure `applyDiffPayload` engine in `application/apply - **Per-recipe-run all-or-nothing (Q2 (c)).** Phase 1 validates every row first; any conflict aborts phase 2 entirely before any file is touched. Cross-file invariants matter — `rename-preview` produces a definition row + N import rows, and partial application leaves the project syntactically broken. - **Scan-and-collect conflicts (Q3 (b)).** Phase 1 walks every row and collects all conflicts in one pass — better remediation UX than fail-fast. - **TTY prompt + `--yes` gate (Q6 (a)).** Interactive contexts (TTY) get a `Proceed? [y/N]` prompt with default-N; non-interactive contexts (CI / agents / MCP / HTTP) require `--yes` (or `yes: true`) explicitly. `--dry-run` + `--yes` mutually exclusive. -- **Substring match per row, single-line (Q8 (a)).** Mirrors `buildDiffJson`'s contract verbatim — `actual.includes(before_pattern)` + `actual.replace(before, after)` with `$`-pre-escape per `String.prototype.replace`'s GetSubstitution rule. Exemplar: `templates/recipes/rename-preview.sql` emits `before_pattern = old_name` (the bare identifier). +- **Substring match per row, single-line (Q8 (a)).** Mirrors `buildDiffJson`'s contract verbatim — `actual.includes(before_pattern)` + `actual.replace(before, after)` with `$`-pre-escape per `String.prototype.replace`'s GetSubstitution rule. Exemplar: `templates/recipes/rename-preview.sql` emits `before_pattern = old_name` (the bare identifier). When `before_pattern` appears more than once on the line (e.g. `const foo = foo();`), only the leftmost is replaced — same shape `--format diff` previews; recipe authors normalise their SQL if they need a different occurrence. +- **Path-containment guard.** Every `file_path` is rejected with a `path escapes project root` conflict if it's absolute or if `path.resolve(projectRoot, file_path)` lands outside the project root. Defends the CLI + MCP + HTTP write paths against malicious or malformed recipe rows. +- **Overlap detection.** Two rows targeting the same `(file_path, line_start)` are rejected with a `duplicate edit on same line` conflict during phase 1. Without it, the second row's substring assertion would fail mid-phase-2 (after earlier files in alphabetical order had already been renamed) — that would leave the project in a partial-write state and violate Q2 (c). - **Atomic per-file writes via temp + rename.** Sibling `.codemap-apply-.tmp` then `renameSync` — POSIX-atomic so concurrent readers see either pre-rename or post-rename content, never a torn write. - **Q7 idempotency (conflict-only path).** Re-running on already-applied code reports `line content drifted` with `actual_at_line` showing the post-rename content; user re-runs `codemap` to refresh the index → next run produces 0 rows → vacuous clean apply. - **Single envelope shape across modes (Q5).** `{mode, applied, files, conflicts, summary}` — same shape for `dry-run` and `apply`; consumers pattern-match on `mode` + `applied`. diff --git a/docs/architecture.md b/docs/architecture.md index c73189b..415556e 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -92,26 +92,26 @@ A local SQLite database (`.codemap/index.db`) indexes the project tree and store ## Key Files -| File | Purpose | -| ------------------------------------------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `index.ts` | Package entry — re-exports `api` / `config`, runs CLI when main | -| `cli/` | CLI — bootstrap argv, lazy command modules, `query` / `validate` / `context` / `agents init` / index modes | -| `api.ts` | Programmatic API — `createCodemap`, `Codemap`, `runCodemapIndex` | -| `application/` | Pure transport-agnostic engines (`run-index`, `index-engine`, `query-engine`, `audit-engine`, `context-engine`, `validate-engine`, `show-engine`, `impact-engine`, `coverage-engine`, `query-recipes`, `recipes-loader`, `mcp-server`, `http-server`, `watcher`) | -| `worker-pool.ts` | Parallel parse workers (Bun / Node) | -| `db.ts` | SQLite adapter — schema DDL, typed CRUD, connection management | -| `parser.ts` | TS/TSX/JS/JSX extraction via `oxc-parser` — symbols (with JSDoc + generics + return types), type members, imports, exports, components, markers | -| `css-parser.ts` | CSS extraction via `lightningcss` — custom properties, classes, keyframes, `@theme` blocks | -| `resolver.ts` | Import path resolution via `oxc-resolver` — respects `tsconfig` aliases, builds dependency graph | -| `constants.ts` | Shared constants — e.g. `LANG_MAP` | -| `glob-sync.ts` | Include globs — Bun `Glob` vs `tinyglobby` on Node ([packaging § Node vs Bun](./packaging.md#node-vs-bun)) | -| `markers.ts` | Shared marker extraction (`TODO`/`FIXME`/`HACK`/`NOTE`) + `extractSuppressions` for opt-in `// codemap-ignore-{next-line,file} ` directives — used by all parsers | -| `parse-worker.ts` | Worker thread entry point — reads, parses, and extracts file data in parallel | -| `adapters/` | `LanguageAdapter` types and built-in TS/CSS/text implementations | -| `parsed-types.ts` | Shared `ParsedFile` shape for workers and adapters | -| `agents-init.ts` / `agents-init-interactive.ts` | `codemap agents init` — see [agents.md](./agents.md) (granular template + IDE writes, pointer upsert, **`--interactive`**, `.gitignore`) | -| `benchmark.ts` (+ `benchmark-default-scenarios.ts`, `benchmark-config.ts`, `benchmark-common.ts`) | SQL vs traditional timing; optional **`CODEMAP_BENCHMARK_CONFIG`** JSON — [benchmark.md § Custom scenarios](./benchmark.md#custom-scenarios-codemap_benchmark_config) | -| `config.ts` | `/config.{ts,js,json}` load path, **Zod** user schema (`codemapUserConfigSchema`), `resolveCodemapConfig` | +| File | Purpose | +| ------------------------------------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `index.ts` | Package entry — re-exports `api` / `config`, runs CLI when main | +| `cli/` | CLI — bootstrap argv, lazy command modules, `query` / `validate` / `context` / `agents init` / index modes | +| `api.ts` | Programmatic API — `createCodemap`, `Codemap`, `runCodemapIndex` | +| `application/` | Pure transport-agnostic engines (`run-index`, `index-engine`, `query-engine`, `audit-engine`, `context-engine`, `validate-engine`, `show-engine`, `impact-engine`, `apply-engine`, `coverage-engine`, `query-recipes`, `recipes-loader`, `mcp-server`, `http-server`, `watcher`) | +| `worker-pool.ts` | Parallel parse workers (Bun / Node) | +| `db.ts` | SQLite adapter — schema DDL, typed CRUD, connection management | +| `parser.ts` | TS/TSX/JS/JSX extraction via `oxc-parser` — symbols (with JSDoc + generics + return types), type members, imports, exports, components, markers | +| `css-parser.ts` | CSS extraction via `lightningcss` — custom properties, classes, keyframes, `@theme` blocks | +| `resolver.ts` | Import path resolution via `oxc-resolver` — respects `tsconfig` aliases, builds dependency graph | +| `constants.ts` | Shared constants — e.g. `LANG_MAP` | +| `glob-sync.ts` | Include globs — Bun `Glob` vs `tinyglobby` on Node ([packaging § Node vs Bun](./packaging.md#node-vs-bun)) | +| `markers.ts` | Shared marker extraction (`TODO`/`FIXME`/`HACK`/`NOTE`) + `extractSuppressions` for opt-in `// codemap-ignore-{next-line,file} ` directives — used by all parsers | +| `parse-worker.ts` | Worker thread entry point — reads, parses, and extracts file data in parallel | +| `adapters/` | `LanguageAdapter` types and built-in TS/CSS/text implementations | +| `parsed-types.ts` | Shared `ParsedFile` shape for workers and adapters | +| `agents-init.ts` / `agents-init-interactive.ts` | `codemap agents init` — see [agents.md](./agents.md) (granular template + IDE writes, pointer upsert, **`--interactive`**, `.gitignore`) | +| `benchmark.ts` (+ `benchmark-default-scenarios.ts`, `benchmark-config.ts`, `benchmark-common.ts`) | SQL vs traditional timing; optional **`CODEMAP_BENCHMARK_CONFIG`** JSON — [benchmark.md § Custom scenarios](./benchmark.md#custom-scenarios-codemap_benchmark_config) | +| `config.ts` | `/config.{ts,js,json}` load path, **Zod** user schema (`codemapUserConfigSchema`), `resolveCodemapConfig` | ## CLI usage @@ -131,7 +131,7 @@ A local SQLite database (`.codemap/index.db`) indexes the project tree and store **Impact wiring:** **`src/cli/cmd-impact.ts`** (argv — `` + `--direction up|down|both` + `--depth N` + `--via dependencies|calls|imports|all` + `--limit N` + `--summary` + `--json`; bootstrap absorbs `--root`/`--config`) + **`src/application/impact-engine.ts`** (engine — `findImpact({db, target, direction?, via?, depth?, limit?})`). Pure transport-agnostic walker over the calls + dependencies + imports graphs; CLI / MCP / HTTP all dispatch the same engine function via `tool-handlers.ts`'s `handleImpact`. Target auto-resolves: contains `/` or matches `files.path` → file target; otherwise symbol (case-sensitive). Walks compatible backends per resolved kind: **symbol** → `calls` (callers / callees by `caller_name` / `callee_name`); **file** → `dependencies` (`from_path` / `to_path`) + `imports` (`file_path` / `resolved_path`, `IS NOT NULL` filter). `--via ` overrides; mismatched explicit choices land in `skipped_backends` (no error — agents see why their backend selection yielded fewer rows than expected). One `WITH RECURSIVE` per (direction, backend) combo with cycle detection via path-string `instr` check (SQLite has no native cycle predicate); JS-side merge + dedup by `(direction, kind, name?, file_path)` keeping the shallowest depth. `--depth 0` uses an unbounded sentinel (`UNBOUNDED_DEPTH_SENTINEL = 1_000_000`); cycle detection + `LIMIT` keep cyclic graphs cheap regardless. Termination reason classification: `limit` (truncated) > `depth` (any node sat at the cap) > `exhausted`. Result envelope: `{target, direction, via, depth_limit, matches: [{depth, direction, edge, kind, name?, file_path}], summary: {nodes, max_depth_reached, by_kind, terminated_by}, skipped_backends?}`. `--summary` blanks `matches` (transport bandwidth saver) but preserves `summary.nodes` so CI gates (`jq '.summary.nodes'`) still see the count. SARIF / annotations not supported (graph traversal, not findings — the parser accepts the flag combos but the engine only emits JSON). -**Apply wiring:** **`src/cli/cmd-apply.ts`** (argv — `` + `--params` + `--dry-run` + `--yes` + `--json`; bootstrap absorbs `--root`/`--config`) + **`src/application/apply-engine.ts`** (engine — `applyDiffPayload({rows, projectRoot, dryRun})`). Pure transport-agnostic substrate-shaped fix executor: consumes the existing `--format diff-json` row contract from any recipe (`{file_path, line_start, before_pattern, after_pattern}`), validates each row against current disk, and either previews (dry-run) or writes (apply). CLI / MCP / HTTP all dispatch the same engine via `tool-handlers.ts`'s `handleApply`. **Phase 1** (always) reads each file at most once into `sourceCache`, splits on `/\r?\n/` for conflict reporting, checks `actual.includes(before_pattern)` (substring match — mirrors `buildDiffJson`'s contract; `rename-preview` emits `before_pattern = old_name` as the bare identifier, so whole-line exact match would conflict every time). Conflicts collect three reasons (`file missing` / `line out of range` / `line content drifted`) — Q3 scan-and-collect, not fail-fast. **Phase 2** (gated on `!dryRun && conflicts.length === 0`) re-splits the cached source on raw `"\n"` (preserves CRLF as trailing `\r` per line; rejoining with `"\n"` round-trips losslessly), applies each file's edits in descending line order via `actual.replace(before, after)` with `$`-pre-escape (`replace(/\$/g, "$$$$")` — matches `buildDiffJson`'s GetSubstitution defence so identifiers like `$inject` round-trip safely), writes to a sibling temp path (`.codemap-apply-.tmp`), then `renameSync` into place — POSIX-atomic per file; concurrent readers see either pre-rename or post-rename content, never a torn write. **Q2 (c) all-or-nothing**: any conflict in any file aborts phase 2 entirely before any file is touched. **Q6 gate**: TTY no `--yes` → phase-1 preview + `Proceed? [y/N]` prompt on stderr (default-N, `node:readline/promises`); TTY `--yes` → no prompt; non-TTY (CI / agents / MCP) without `--yes`/`--dry-run` rejected with stderr message. `--dry-run` + `--yes` mutually exclusive (parse-time error). MCP/HTTP transports (`handleApply`) require `yes: true` for the write path — there's no prompt to fall back on; `dry_run + yes` rejected as mutually exclusive. Result envelope (Q5; identical across modes): `{mode: 'dry-run'|'apply', applied: bool, files: [{file_path, rows_applied, warnings?}], conflicts: [{file_path, line_start, before_pattern, actual_at_line, reason}], summary: {files, files_modified, rows, rows_applied, conflicts, files_with_conflicts}}`. `applied: true` only when `mode === 'apply'` AND zero conflicts AND at least one row applied. Q7 idempotency: re-running on already-applied code reports a `line content drifted` conflict with `actual_at_line` showing the post-rename content; the user reads it and re-runs `codemap` to refresh the index → next run produces 0 rows (recipe finds nothing to rename) → vacuous clean apply. SARIF / annotations not supported (write action, not findings). TOCTOU: phase-1 reads through `sourceCache`; phase-2 transforms the cached source and writes — the gap between read and rename is a deliberate v1 simplification (apply isn't adversarial). Per Q10, only `cli/cmd-apply.ts` + `application/tool-handlers.ts` (+ the test files) may import `apply-engine.ts` for production execution — re-runnable forbidden-edge query at [§ Boundary verification — apply write path](#boundary-verification--apply-write-path). +**Apply wiring:** **`src/cli/cmd-apply.ts`** (argv — `` + `--params` + `--dry-run` + `--yes` + `--json`; bootstrap absorbs `--root`/`--config`) + **`src/application/apply-engine.ts`** (engine — `applyDiffPayload({rows, projectRoot, dryRun})`). Pure transport-agnostic substrate-shaped fix executor: consumes the existing `--format diff-json` row contract from any recipe (`{file_path, line_start, before_pattern, after_pattern}`), validates each row against current disk, and either previews (dry-run) or writes (apply). CLI / MCP / HTTP all dispatch the same engine via `tool-handlers.ts`'s `handleApply`. **Phase 1** (always) resolves the project root via `path.resolve(projectRoot)` once, then for each row: rejects absolute `file_path` inputs and any candidate whose `path.resolve(resolvedRoot, file_path)` lands outside `resolvedRoot` (conflict `path escapes project root` — guards CLI + MCP + HTTP write paths against `../escape.ts`-style traversal); rejects duplicate `(file_path, line_start)` tuples (conflict `duplicate edit on same line` — without this, two phase-1-passing rows targeting the same line would split the run mid-phase-2 because the first replace invalidates the second's substring assertion, leaving Q2 (c) cross-file partial state). Reads each file at most once into `sourceCache`, splits on `/\r?\n/` for conflict reporting, checks `actual.includes(before_pattern)` (substring match — mirrors `buildDiffJson`'s contract; `rename-preview` emits `before_pattern = old_name` as the bare identifier, so whole-line exact match would conflict every time). Conflicts collect five reasons (`file missing` / `line out of range` / `line content drifted` / `path escapes project root` / `duplicate edit on same line`) — Q3 scan-and-collect, not fail-fast. **Phase 2** (gated on `!dryRun && conflicts.length === 0`) re-splits the cached source on raw `"\n"` (preserves CRLF as trailing `\r` per line; rejoining with `"\n"` round-trips losslessly), applies each file's edits in descending line order via `actual.replace(before, after)` with `$`-pre-escape (`replace(/\$/g, "$$$$")` — matches `buildDiffJson`'s GetSubstitution defence so identifiers like `$inject` round-trip safely), writes to a sibling temp path (`.codemap-apply-.tmp`), then `renameSync` into place — POSIX-atomic per file; concurrent readers see either pre-rename or post-rename content, never a torn write. **Q2 (c) all-or-nothing**: any conflict in any file aborts phase 2 entirely before any file is touched. **Q6 gate**: TTY no `--yes` → phase-1 preview + `Proceed? [y/N]` prompt on stderr (default-N, `node:readline/promises`); TTY `--yes` → no prompt; non-TTY (CI / agents / MCP) without `--yes`/`--dry-run` rejected with stderr message. `--dry-run` + `--yes` mutually exclusive (parse-time error). MCP/HTTP transports (`handleApply`) require `yes: true` for the write path — there's no prompt to fall back on; `dry_run + yes` rejected as mutually exclusive. Result envelope (Q5; identical across modes): `{mode: 'dry-run'|'apply', applied: bool, files: [{file_path, rows_applied, warnings?}], conflicts: [{file_path, line_start, before_pattern, actual_at_line, reason}], summary: {files, files_modified, rows, rows_applied, conflicts, files_with_conflicts}}`. `applied: true` only when `mode === 'apply'` AND zero conflicts AND at least one row applied. Q7 idempotency: re-running on already-applied code reports a `line content drifted` conflict with `actual_at_line` showing the post-rename content; the user reads it and re-runs `codemap` to refresh the index → next run produces 0 rows (recipe finds nothing to rename) → vacuous clean apply. **Same-line ambiguity caveat (documented limitation):** `actual.replace(before_pattern, after_pattern)` rewrites only the **first** occurrence on the line. When `before_pattern` appears twice (e.g. `const foo = foo();` with `before = "foo"`) only the leftmost is replaced; the engine still reports `applied: true`. This mirrors `buildDiffJson`'s formatter contract verbatim — recipe authors who hit it normalise their SQL to emit a more specific pattern, or accept it (the formatter's `--format diff` preview shows the same shape). Promotion path: tighten phase-1 to conflict on ambiguity in a future PR if real users complain, but only alongside the formatter so preview and execution stay in lockstep. SARIF / annotations not supported (write action, not findings). TOCTOU: phase-1 reads through `sourceCache`; phase-2 transforms the cached source and writes — the gap between read and rename is a deliberate v1 simplification (apply isn't adversarial). Per Q10, only `cli/cmd-apply.ts` + `application/tool-handlers.ts` (+ the test files) may import `apply-engine.ts` for production execution — re-runnable forbidden-edge query at [§ Boundary verification — apply write path](#boundary-verification--apply-write-path). **Show / snippet wiring:** **`src/cli/cmd-show.ts`** + **`src/cli/cmd-snippet.ts`** — sibling CLI verbs sharing the same parser shape (`` + `--kind` + `--in ` + `--json`) and the pure engine **`src/application/show-engine.ts`** (`findSymbolsByName({db, name, kind?, inPath?})` for the lookup; `readSymbolSource({match, projectRoot, indexedContentHash?})` + `getIndexedContentHash(db, filePath)` for the snippet-side FS read; **`buildShowResult`** + **`buildSnippetResult`** envelope builders — same engine the MCP show/snippet tools call). Both verbs return the same `{matches, disambiguation?}` envelope per plan § 4 uniformity — single match → `{matches: [{...}]}`; multi-match adds `{n, by_kind, files, hint}`. Snippet matches add `source` / `stale` / `missing` fields (additive — no shape divergence). **`--in `** is normalized through `toProjectRelative(projectRoot, p)` (from **`src/application/validate-engine.ts`**) so `--in ./src/cli/`, `--in src/cli`, and `--in src/cli/cmd-show.ts` all resolve identically. Stale-file behavior on `snippet`: `hashContent` (from **`src/hash.ts`** — same primitive `cmd-validate.ts` uses) compares the on-disk content_hash against `files.content_hash`; mismatch sets `stale: true` but the source IS still returned (read tool, no auto-reindex side-effects). MCP tools `show` and `snippet` register parallel to the CLI surface (see [§ MCP wiring](#cli-usage)). diff --git a/src/application/apply-engine.test.ts b/src/application/apply-engine.test.ts index d9af892..9fa1504 100644 --- a/src/application/apply-engine.test.ts +++ b/src/application/apply-engine.test.ts @@ -522,6 +522,163 @@ describe("applyDiffPayload", () => { }); }); + describe("path-containment guard (F1 — triangulated review 2026-05-06)", () => { + it("rejects `..`-traversal and never writes outside the project root", () => { + const root = tmpProject(); + writeSource(root, "in.ts", "const foo = 1;\n"); + // Sibling-of-root file the malicious row would otherwise hit. + const outsidePath = join(root, "..", "outside.ts"); + writeFileSync(outsidePath, "const foo = 1;\n", "utf8"); + const before = readFileSync(outsidePath, "utf8"); + + const result = applyDiffPayload({ + rows: [ + { + file_path: "../outside.ts", + line_start: 1, + before_pattern: "foo", + after_pattern: "PWNED", + }, + ], + projectRoot: root, + dryRun: false, + }); + + expect(result.applied).toBe(false); + expect(result.conflicts).toEqual([ + { + file_path: "../outside.ts", + line_start: 1, + before_pattern: "foo", + actual_at_line: "", + reason: "path escapes project root", + }, + ]); + expect(readFileSync(outsidePath, "utf8")).toBe(before); + }); + + it("rejects absolute file_path inputs", () => { + const root = tmpProject(); + + const result = applyDiffPayload({ + rows: [ + { + file_path: "/etc/passwd", + line_start: 1, + before_pattern: "root", + after_pattern: "X", + }, + ], + projectRoot: root, + dryRun: false, + }); + + expect(result.applied).toBe(false); + expect(result.conflicts[0]).toMatchObject({ + reason: "path escapes project root", + }); + }); + + it("rejects nested traversal that resolves outside the root", () => { + const root = tmpProject(); + + const result = applyDiffPayload({ + rows: [ + { + file_path: "src/../../sibling.ts", + line_start: 1, + before_pattern: "foo", + after_pattern: "X", + }, + ], + projectRoot: root, + dryRun: false, + }); + + expect(result.conflicts[0]?.reason).toBe("path escapes project root"); + }); + }); + + describe("overlap detection (F2 — triangulated review 2026-05-06)", () => { + it("emits a conflict on duplicate (file_path, line_start) and writes nothing", () => { + const root = tmpProject(); + writeSource(root, "a.ts", "const foo = 1;\n"); + writeSource(root, "b.ts", "const foo = 1;\n"); + const aBefore = readSource(root, "a.ts"); + const bBefore = readSource(root, "b.ts"); + + const result = applyDiffPayload({ + rows: [ + { + file_path: "a.ts", + line_start: 1, + before_pattern: "foo", + after_pattern: "AAA", + }, + { + file_path: "b.ts", + line_start: 1, + before_pattern: "foo", + after_pattern: "BBB", + }, + // Duplicate on b.ts:1 — pre-fix this triggered a phase-2 throw + // AFTER a.ts had already been renamed (Q2 (c) violation). + { + file_path: "b.ts", + line_start: 1, + before_pattern: "foo", + after_pattern: "CCC", + }, + ], + projectRoot: root, + dryRun: false, + }); + + expect(result.applied).toBe(false); + expect(result.conflicts).toHaveLength(1); + expect(result.conflicts[0]).toMatchObject({ + file_path: "b.ts", + line_start: 1, + reason: "duplicate edit on same line", + }); + // CRITICAL — neither file may have been touched; Q2 (c) holds. + expect(readSource(root, "a.ts")).toBe(aBefore); + expect(readSource(root, "b.ts")).toBe(bBefore); + }); + }); + + describe("same-line ambiguity (F3 — documented limitation)", () => { + it("rewrites only the first occurrence on a line — matches buildDiffJson", () => { + // Recipe authors who hit this normalise their SQL to emit a more + // specific pattern. Documented in the engine docstring + architecture.md + // Apply wiring caveat. This test pins the current behaviour so a + // future engine-level fix lands as a deliberate breaking change + // rather than silent drift. + const root = tmpProject(); + writeSource(root, "x.ts", "const foo = foo();\n"); + + const result = applyDiffPayload({ + rows: [ + { + file_path: "x.ts", + line_start: 1, + before_pattern: "foo", + after_pattern: "bar", + }, + ], + projectRoot: root, + dryRun: false, + }); + + expect(result.applied).toBe(true); + expect(result.summary.rows_applied).toBe(1); + // Variable renamed; recursive call site untouched. Recipe author + // accepts this (formatter preview shows the same shape) or splits + // their SQL into a more specific pattern. + expect(readSource(root, "x.ts")).toBe("const bar = foo();\n"); + }); + }); + describe("failure modes", () => { it("propagates the writeFileSync error when the file is read-only (chmod 0o444)", () => { const root = tmpProject(); diff --git a/src/application/apply-engine.ts b/src/application/apply-engine.ts index 98900d5..346612f 100644 --- a/src/application/apply-engine.ts +++ b/src/application/apply-engine.ts @@ -6,9 +6,24 @@ * * Phase 1 validates every row against current disk state; phase 2 (gated on * `!dryRun && conflicts.length === 0`) writes each modified file via temp + - * rename for crash-safe per-file atomicity. See - * [`docs/plans/codemap-apply.md`](../../docs/plans/codemap-apply.md) for the - * full design (Q1–Q10 locked). + * rename for crash-safe per-file atomicity. Full design (Q1–Q10 locks) + * lives in [`docs/architecture.md`](../../docs/architecture.md) under the + * Apply wiring subsection. + * + * Path-containment guard: every `file_path` is resolved against + * `projectRoot` (no `realpath` — `resolve` normalises `..` segments) and + * rejected via a `"path escapes project root"` conflict if the result lands + * outside the root. Absolute `file_path` inputs are also rejected — the row + * contract is project-relative. + * + * Same-line ambiguity caveat: phase-2 uses + * `actual.replace(before_pattern, after_pattern)` — first-occurrence only. + * This mirrors `buildDiffJson`'s formatter contract verbatim. When a line + * contains the pattern more than once (e.g. `const foo = foo();` with + * `before = "foo"`) only the leftmost occurrence is rewritten; the call + * site is left intact and `applied: true` is reported. Recipe authors + * either accept this (the formatter preview shows the same shape) or + * normalise their SQL to emit a more specific pattern. * * TOCTOU note: phase-1 caches the source it validated; phase-2 transforms * the cached source and writes the result. The window between read and @@ -23,7 +38,7 @@ import { randomBytes } from "node:crypto"; import { readFileSync, renameSync, writeFileSync } from "node:fs"; -import { join } from "node:path"; +import { isAbsolute, resolve, sep } from "node:path"; /** Required input columns on every recipe row. */ export interface ApplyInputRow extends Record { @@ -67,7 +82,9 @@ export interface ConflictRow { export type ConflictReason = | "file missing" | "line out of range" - | "line content drifted"; + | "line content drifted" + | "path escapes project root" + | "duplicate edit on same line"; /** Q5 envelope shape — single shape across `dry-run` and `apply` modes. */ export interface ApplyJsonPayload { @@ -109,6 +126,15 @@ export function applyDiffPayload(opts: ApplyDiffPayloadOpts): ApplyJsonPayload { const pending = new Map(); // Phase-1 reads each file at most once; phase 2 reuses the cached source. const sourceCache = new Map(); + // Resolve the project root once so the path-containment check uses + // a canonicalised prefix. `resolve` normalises trailing `/` and `.` + // segments without dereferencing symlinks (matching the candidate's + // resolution semantics — no false positives for symlinked roots). + const resolvedRoot = resolve(projectRoot); + // Per-file set of line_starts already seen in the row stream — used + // to fire the "duplicate edit on same line" conflict before phase 2 + // could throw the cross-file partial-write that drops Q2 (c). + const seenLines = new Map>(); let validRows = 0; for (const row of rows) { @@ -126,10 +152,25 @@ export function applyDiffPayload(opts: ApplyDiffPayloadOpts): ApplyJsonPayload { } validRows++; + // Path-containment guard: reject absolute paths and any candidate + // whose resolved form lands outside `resolvedRoot`. Without this the + // engine would happily honour `file_path: "../escape.ts"` and write + // sibling-of-root files (CLI + MCP + HTTP all share this engine). + if (isAbsolute(filePath) || !isWithinRoot(resolvedRoot, filePath)) { + conflicts.push({ + file_path: filePath, + line_start: lineStart, + before_pattern: before, + actual_at_line: "", + reason: "path escapes project root", + }); + continue; + } + let source = sourceCache.get(filePath); if (source === undefined) { try { - source = readFileSync(join(projectRoot, filePath), "utf8"); + source = readFileSync(resolve(resolvedRoot, filePath), "utf8"); } catch { conflicts.push({ file_path: filePath, @@ -168,6 +209,30 @@ export function applyDiffPayload(opts: ApplyDiffPayloadOpts): ApplyJsonPayload { continue; } + // Overlap detection — one (file_path, line_start) tuple may have at + // most one row. Without this guard, two rows on the same line both + // pass phase-1's substring check (validated against original source); + // phase-2 then applies the first replace, the second's invariant + // assertion fails, the function throws AFTER earlier files in + // alphabetical order have already been renamed — partial cross-file + // state, no envelope returned. + const seen = seenLines.get(filePath); + if (seen !== undefined && seen.has(lineStart)) { + conflicts.push({ + file_path: filePath, + line_start: lineStart, + before_pattern: before, + actual_at_line: actual, + reason: "duplicate edit on same line", + }); + continue; + } + if (seen === undefined) { + seenLines.set(filePath, new Set([lineStart])); + } else { + seen.add(lineStart); + } + const edits = pending.get(filePath); if (edits === undefined) { pending.set(filePath, [ @@ -248,7 +313,10 @@ export function applyDiffPayload(opts: ApplyDiffPayloadOpts): ApplyJsonPayload { } const newContent = fileLines.join("\n"); - const absPath = join(projectRoot, filePath); + // `resolvedRoot` is captured from the closure — phase-1 already + // verified the candidate is inside it, so this `resolve` collapses + // back to the same in-root absolute path. + const absPath = resolve(resolvedRoot, filePath); // Sibling temp + `rename`: POSIX-atomic when source and destination // share a filesystem (always true for siblings). A concurrent reader // sees either the pre-rename or post-rename content — never a torn @@ -294,3 +362,16 @@ function readPositiveInt( ? value : undefined; } + +/** + * `true` iff the resolved candidate lands inside `resolvedRoot` (or is + * the root itself). Both sides go through the same `resolve` semantics — + * no `realpath` / symlink dereference — so a project root that lives + * behind a symlink doesn't false-positive on its own descendants. + */ +function isWithinRoot(resolvedRoot: string, candidate: string): boolean { + const resolved = resolve(resolvedRoot, candidate); + if (resolved === resolvedRoot) return true; + const prefix = resolvedRoot.endsWith(sep) ? resolvedRoot : resolvedRoot + sep; + return resolved.startsWith(prefix); +} From aaabc1388ac488b37d4c5050bf459c215db93433 Mon Sep 17 00:00:00 2001 From: Sutu Sebastian Date: Wed, 6 May 2026 13:16:19 +0300 Subject: [PATCH 7/9] chore(apply): slim comments + sync docs to five conflict reasons MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Concise-comments sweep on the apply surface — module docstring goes from a six-section narrative to three named call-outs (same-line ambiguity / TOCTOU / EOL); inline comments drop redundant prose where the next line of code already says it. Net 65 lines removed across src/ with no behavioural change. Docs sync: post-fix the engine collects FIVE conflict reasons (added `path escapes project root` + `duplicate edit on same line` in commit bdf7ef3), but the agent rule, the published-package agent rule, and the glossary all still said "three." Updated all three to enumerate the full set + briefly describe what each new guard rejects. Touched: - src/application/apply-engine.ts — slim docstring + 6 inline blocks. - src/application/apply-engine.test.ts — slim test rationale where the assertion already conveyed it. - src/cli/cmd-apply.ts — collapse two same-branch returns into one union; slim Q6/path-derivation comments. - src/application/tool-handlers.ts — slim handleApply schema/header doc to one sentence each. - .agents/rules/codemap.md + templates/agents/rules/codemap.md + docs/glossary.md — three → five conflict reasons + new-guard one-liners. Tests + typecheck + format + boundary kit all green. --- .agents/rules/codemap.md | 2 +- docs/glossary.md | 2 +- src/application/apply-engine.test.ts | 14 +--- src/application/apply-engine.ts | 116 +++++++++------------------ src/application/tool-handlers.ts | 15 ++-- src/cli/cmd-apply.test.ts | 2 - src/cli/cmd-apply.ts | 24 ++---- templates/agents/rules/codemap.md | 2 +- 8 files changed, 56 insertions(+), 121 deletions(-) diff --git a/.agents/rules/codemap.md b/.agents/rules/codemap.md index e8ab2b0..b0896b3 100644 --- a/.agents/rules/codemap.md +++ b/.agents/rules/codemap.md @@ -76,7 +76,7 @@ Validation: SQL is rejected at load time if it starts with DML/DDL (DELETE/DROP/ **Impact (`bun src/index.ts impact `)**: symbol/file blast-radius walker — replaces hand-composed `WITH RECURSIVE` queries that agents struggle to write. Target auto-resolves: contains `/` or matches `files.path` → file target; otherwise symbol (case-sensitive). Walks compatible graphs by target kind: **symbol** → `calls` (callers / callees by name); **file** → `dependencies` + `imports` (`resolved_path` only). `--via ` overrides; mismatched explicit choices land in `skipped_backends` (no error). Cycle-detected via `WITH RECURSIVE` path-string + `instr` check; bounded by `--depth N` (default 3, `0` = unbounded but still cycle-detected and limit-capped) and `--limit N` (default 500). Output envelope: `{target, direction, via, depth_limit, matches: [{depth, direction, edge, kind, name?, file_path}], summary: {nodes, max_depth_reached, by_kind, terminated_by: 'depth'|'limit'|'exhausted'}}`. `--summary` trims `matches` for cheap CI-gate consumption (`jq '.summary.nodes'` -**Apply (`bun src/index.ts apply `)**: substrate-shaped fix executor over the existing `--format diff-json` row contract — recipe SQL is the synthesis surface, codemap executes. Phase 1 validates every `{file_path, line_start, before_pattern, after_pattern}` row against current disk via `actual.includes(before_pattern)` (substring match — same contract `buildDiffJson` uses); collects three conflict reasons (`file missing` / `line out of range` / `line content drifted`). Phase 2 (gated on `!dryRun && conflicts.length === 0`) writes via sibling temp + `rename` for POSIX-atomic per-file writes. **Q2 (c) all-or-nothing** — any conflict aborts the whole run before any file is touched. **Q6 gate** — TTY prompts `Proceed? [y/N]` (default-N) on stderr; non-TTY (CI / agents / MCP / HTTP) requires `--yes` (or `yes: true`) explicitly; `--dry-run` + `--yes` mutually exclusive. Q7 idempotency: re-running on already-applied code reports `line content drifted` with `actual_at_line` showing the post-rename content — re-run `bun src/index.ts` to refresh, then re-run apply (vacuous clean pass). Output envelope (identical across modes): `{mode: 'dry-run'|'apply', applied: bool, files: [{file_path, rows_applied, warnings?}], conflicts: [{file_path, line_start, before_pattern, actual_at_line, reason}], summary: {files, files_modified, rows, rows_applied, conflicts, files_with_conflicts}}`. Pure transport-agnostic engine in `application/apply-engine.ts`; CLI / MCP / HTTP all dispatch the same `applyDiffPayload` function.) but preserves the count. SARIF / annotations not supported (graph traversal, not findings). Pure transport-agnostic engine in `application/impact-engine.ts`; CLI / MCP / HTTP all dispatch the same `findImpact` function. +**Apply (`bun src/index.ts apply `)**: substrate-shaped fix executor over the existing `--format diff-json` row contract — recipe SQL is the synthesis surface, codemap executes. Phase 1 validates every `{file_path, line_start, before_pattern, after_pattern}` row against current disk via `actual.includes(before_pattern)` (substring match — same contract `buildDiffJson` uses); collects five conflict reasons (`file missing` / `line out of range` / `line content drifted` / `path escapes project root` / `duplicate edit on same line`). The `path escapes project root` guard rejects absolute `file_path` inputs and any candidate whose resolved form lands outside `projectRoot`; the `duplicate edit on same line` guard rejects two-or-more rows targeting the same `(file_path, line_start)` so phase 2 doesn't split mid-loop and leak Q2 (c). Phase 2 (gated on `!dryRun && conflicts.length === 0`) writes via sibling temp + `rename` for POSIX-atomic per-file writes. **Q2 (c) all-or-nothing** — any conflict aborts the whole run before any file is touched. **Q6 gate** — TTY prompts `Proceed? [y/N]` (default-N) on stderr; non-TTY (CI / agents / MCP / HTTP) requires `--yes` (or `yes: true`) explicitly; `--dry-run` + `--yes` mutually exclusive. Q7 idempotency: re-running on already-applied code reports `line content drifted` with `actual_at_line` showing the post-rename content — re-run `bun src/index.ts` to refresh, then re-run apply (vacuous clean pass). Output envelope (identical across modes): `{mode: 'dry-run'|'apply', applied: bool, files: [{file_path, rows_applied, warnings?}], conflicts: [{file_path, line_start, before_pattern, actual_at_line, reason}], summary: {files, files_modified, rows, rows_applied, conflicts, files_with_conflicts}}`. Pure transport-agnostic engine in `application/apply-engine.ts`; CLI / MCP / HTTP all dispatch the same `applyDiffPayload` function.) but preserves the count. SARIF / annotations not supported (graph traversal, not findings). Pure transport-agnostic engine in `application/impact-engine.ts`; CLI / MCP / HTTP all dispatch the same `findImpact` function. **MCP server (`bun src/index.ts mcp`)**: stdio MCP (Model Context Protocol) server — agents call codemap as JSON-RPC tools instead of shelling out to the CLI on every read. v1 ships one tool per CLI verb plus six resources (`codemap://recipes` + `codemap://recipes/{id}` are live read every call so inline `last_run_at` / `run_count` recency stays fresh; `codemap://schema` + `codemap://skill` lazy-cache; `codemap://files/{path}` + `codemap://symbols/{name}` always live): diff --git a/docs/glossary.md b/docs/glossary.md index 6d96ae0..4b3a53d 100644 --- a/docs/glossary.md +++ b/docs/glossary.md @@ -33,7 +33,7 @@ A `.agents/rules/.md` file with YAML frontmatter. Distinct from a **skill* ### `codemap apply` / apply tool -Substrate-shaped fix executor — reads the same `{file_path, line_start, before_pattern, after_pattern}` row contract `--format diff-json` emits and applies the hunks to disk. Recipe SQL is the synthesis surface; codemap is the executor (Moat-A clean — verdict-shape "should we fix this?" stays on the recipe author). CLI: `codemap apply [--params k=v[,k=v]] [--dry-run] [--yes] [--json]`. MCP: `apply` tool. HTTP: `POST /tool/apply`. **Phase 1** validates every row against current disk via `actual.includes(before_pattern)` (substring match — mirrors `buildDiffJson`'s contract); collects three conflict reasons (`file missing` / `line out of range` / `line content drifted`). **Phase 2** (gated on `!dryRun && conflicts.length === 0`) writes each modified file via sibling temp + `renameSync` for POSIX-atomic per-file writes, with `$`-pre-escape on `after_pattern` per `String.prototype.replace` GetSubstitution rule. **Q2 (c) all-or-nothing** — any conflict in any file aborts phase 2 entirely; partial writes never ship. **Q6 gate** — TTY no `--yes` triggers a `Proceed? [y/N]` prompt (default-N) on stderr; non-TTY contexts (CI / agents / MCP / HTTP) require `--yes` (or `yes: true`) explicitly. Result envelope (Q5; identical across modes): `{mode, applied, files, conflicts, summary}`. Re-running on already-applied code reports a `line content drifted` conflict whose `actual_at_line` shows the post-rename content — user re-runs `codemap` to refresh the index, then re-runs apply for a vacuous clean pass (Q7). Engine: `application/apply-engine.ts` (pure; `applyDiffPayload`). Boundary: only `cli/cmd-apply.ts` + `application/tool-handlers.ts` may import the engine — re-runnable kit at [`docs/architecture.md` § Boundary verification — apply write path](./architecture.md#boundary-verification--apply-write-path). Floor "No fix engine" preserved — codemap doesn't synthesise edits, it only executes the hunks the recipe row described. +Substrate-shaped fix executor — reads the same `{file_path, line_start, before_pattern, after_pattern}` row contract `--format diff-json` emits and applies the hunks to disk. Recipe SQL is the synthesis surface; codemap is the executor (Moat-A clean — verdict-shape "should we fix this?" stays on the recipe author). CLI: `codemap apply [--params k=v[,k=v]] [--dry-run] [--yes] [--json]`. MCP: `apply` tool. HTTP: `POST /tool/apply`. **Phase 1** validates every row against current disk via `actual.includes(before_pattern)` (substring match — mirrors `buildDiffJson`'s contract); collects five conflict reasons (`file missing` / `line out of range` / `line content drifted` / `path escapes project root` / `duplicate edit on same line`). The path-containment guard rejects absolute `file_path` inputs and `../`-traversal that resolves outside `projectRoot`; the overlap guard rejects two-or-more rows targeting the same `(file_path, line_start)`. **Phase 2** (gated on `!dryRun && conflicts.length === 0`) writes each modified file via sibling temp + `renameSync` for POSIX-atomic per-file writes, with `$`-pre-escape on `after_pattern` per `String.prototype.replace` GetSubstitution rule. **Q2 (c) all-or-nothing** — any conflict in any file aborts phase 2 entirely; partial writes never ship. **Q6 gate** — TTY no `--yes` triggers a `Proceed? [y/N]` prompt (default-N) on stderr; non-TTY contexts (CI / agents / MCP / HTTP) require `--yes` (or `yes: true`) explicitly. Result envelope (Q5; identical across modes): `{mode, applied, files, conflicts, summary}`. Re-running on already-applied code reports a `line content drifted` conflict whose `actual_at_line` shows the post-rename content — user re-runs `codemap` to refresh the index, then re-runs apply for a vacuous clean pass (Q7). Engine: `application/apply-engine.ts` (pure; `applyDiffPayload`). Boundary: only `cli/cmd-apply.ts` + `application/tool-handlers.ts` may import the engine — re-runnable kit at [`docs/architecture.md` § Boundary verification — apply write path](./architecture.md#boundary-verification--apply-write-path). Floor "No fix engine" preserved — codemap doesn't synthesise edits, it only executes the hunks the recipe row described. ### audit diff --git a/src/application/apply-engine.test.ts b/src/application/apply-engine.test.ts index 9fa1504..a59b04c 100644 --- a/src/application/apply-engine.test.ts +++ b/src/application/apply-engine.test.ts @@ -621,8 +621,7 @@ describe("applyDiffPayload", () => { before_pattern: "foo", after_pattern: "BBB", }, - // Duplicate on b.ts:1 — pre-fix this triggered a phase-2 throw - // AFTER a.ts had already been renamed (Q2 (c) violation). + // Duplicate on b.ts:1 — pre-fix triggered a phase-2 throw after a.ts had already renamed. { file_path: "b.ts", line_start: 1, @@ -649,11 +648,8 @@ describe("applyDiffPayload", () => { describe("same-line ambiguity (F3 — documented limitation)", () => { it("rewrites only the first occurrence on a line — matches buildDiffJson", () => { - // Recipe authors who hit this normalise their SQL to emit a more - // specific pattern. Documented in the engine docstring + architecture.md - // Apply wiring caveat. This test pins the current behaviour so a - // future engine-level fix lands as a deliberate breaking change - // rather than silent drift. + // Pins current behaviour so a future engine change lands as a deliberate + // breaking change. See module docstring § Same-line ambiguity. const root = tmpProject(); writeSource(root, "x.ts", "const foo = foo();\n"); @@ -672,9 +668,7 @@ describe("applyDiffPayload", () => { expect(result.applied).toBe(true); expect(result.summary.rows_applied).toBe(1); - // Variable renamed; recursive call site untouched. Recipe author - // accepts this (formatter preview shows the same shape) or splits - // their SQL into a more specific pattern. + // Declaration renamed; recursive call site untouched. expect(readSource(root, "x.ts")).toBe("const bar = foo();\n"); }); }); diff --git a/src/application/apply-engine.ts b/src/application/apply-engine.ts index 346612f..8e02d63 100644 --- a/src/application/apply-engine.ts +++ b/src/application/apply-engine.ts @@ -1,39 +1,26 @@ /** * Pure transport-agnostic engine for `codemap apply ` — substrate- - * shaped fix executor. Consumes the same `{file_path, line_start, - * before_pattern, after_pattern}` row contract `buildDiffJson` emits and - * either previews (dry-run) or applies the edits to disk. + * shaped fix executor over the `{file_path, line_start, before_pattern, + * after_pattern}` row contract `buildDiffJson` emits. * - * Phase 1 validates every row against current disk state; phase 2 (gated on - * `!dryRun && conflicts.length === 0`) writes each modified file via temp + - * rename for crash-safe per-file atomicity. Full design (Q1–Q10 locks) - * lives in [`docs/architecture.md`](../../docs/architecture.md) under the - * Apply wiring subsection. + * Phase 1 validates every row against current disk; phase 2 (gated on + * `!dryRun && conflicts.length === 0`) writes each modified file via + * sibling temp + `rename` for crash-safe per-file atomicity. Full design + * (Q1–Q10 locks, semantics, exit codes) at + * [`docs/architecture.md`](../../docs/architecture.md) § Apply wiring. * - * Path-containment guard: every `file_path` is resolved against - * `projectRoot` (no `realpath` — `resolve` normalises `..` segments) and - * rejected via a `"path escapes project root"` conflict if the result lands - * outside the root. Absolute `file_path` inputs are also rejected — the row - * contract is project-relative. + * Three behaviours called out here because the code alone won't betray them: * - * Same-line ambiguity caveat: phase-2 uses - * `actual.replace(before_pattern, after_pattern)` — first-occurrence only. - * This mirrors `buildDiffJson`'s formatter contract verbatim. When a line - * contains the pattern more than once (e.g. `const foo = foo();` with - * `before = "foo"`) only the leftmost occurrence is rewritten; the call - * site is left intact and `applied: true` is reported. Recipe authors - * either accept this (the formatter preview shows the same shape) or - * normalise their SQL to emit a more specific pattern. - * - * TOCTOU note: phase-1 caches the source it validated; phase-2 transforms - * the cached source and writes the result. The window between read and - * write is a deliberate v1 simplification (Q2) — apply isn't an adversarial - * verb, so we don't add lock-file machinery. - * - * EOL note: phase-2 splits source on raw `"\n"` (NOT `/\r?\n/`) so CRLF - * lines retain their trailing `\r` and round-trip when joined back with - * `"\n"`. Patterns that include a literal `\r` are out of scope for v1; - * recipe authors should target identifier-shaped patterns. + * - **Same-line ambiguity.** `actual.replace(before, after)` is first- + * occurrence — mirrors `buildDiffJson` verbatim. `const foo = foo();` + * with `before = "foo"` rewrites only the leftmost; recipe authors + * normalise their SQL or accept the formatter-parity behaviour. + * - **TOCTOU.** Phase-1 caches the source it validated; phase-2 transforms + * the cache and writes. The read→rename window is a v1 simplification + * (apply isn't adversarial; no lock files). + * - **EOL.** Phase-2 splits on raw `"\n"` (NOT `/\r?\n/`) so CRLF lines + * keep their trailing `\r` and round-trip on rejoin. Patterns containing + * `\r` are out of scope for v1. */ import { randomBytes } from "node:crypto"; @@ -126,14 +113,12 @@ export function applyDiffPayload(opts: ApplyDiffPayloadOpts): ApplyJsonPayload { const pending = new Map(); // Phase-1 reads each file at most once; phase 2 reuses the cached source. const sourceCache = new Map(); - // Resolve the project root once so the path-containment check uses - // a canonicalised prefix. `resolve` normalises trailing `/` and `.` - // segments without dereferencing symlinks (matching the candidate's - // resolution semantics — no false positives for symlinked roots). + // No `realpath` — same `resolve` semantics on both sides keep symlinked + // roots from false-positiving on their own descendants. const resolvedRoot = resolve(projectRoot); - // Per-file set of line_starts already seen in the row stream — used - // to fire the "duplicate edit on same line" conflict before phase 2 - // could throw the cross-file partial-write that drops Q2 (c). + // Tracks `(file_path, line_start)` tuples already collected so the + // duplicate-edit conflict fires in phase 1 — without it the second row + // would split phase 2 mid-loop and leak Q2 (c) all-or-nothing. const seenLines = new Map>(); let validRows = 0; @@ -152,10 +137,8 @@ export function applyDiffPayload(opts: ApplyDiffPayloadOpts): ApplyJsonPayload { } validRows++; - // Path-containment guard: reject absolute paths and any candidate - // whose resolved form lands outside `resolvedRoot`. Without this the - // engine would happily honour `file_path: "../escape.ts"` and write - // sibling-of-root files (CLI + MCP + HTTP all share this engine). + // Path-containment guard — without it `file_path: "../escape.ts"` would + // write sibling-of-root files (CLI + MCP + HTTP all share this engine). if (isAbsolute(filePath) || !isWithinRoot(resolvedRoot, filePath)) { conflicts.push({ file_path: filePath, @@ -183,9 +166,8 @@ export function applyDiffPayload(opts: ApplyDiffPayloadOpts): ApplyJsonPayload { } sourceCache.set(filePath, source); } - // Phase-1 splits on `/\r?\n/` so the `actual_at_line` reported in - // conflicts doesn't carry a stray `\r`. Phase-2 re-splits on raw `\n` - // for round-trip-safe writes (see EOL note in module docstring). + // Phase-1 splits on `/\r?\n/` so `actual_at_line` is `\r`-free; phase-2 + // re-splits on raw `\n` for round-trip-safe writes (module docstring § EOL). const lines = source.split(/\r?\n/); const actual = lines[lineStart - 1]; if (actual === undefined) { @@ -209,13 +191,8 @@ export function applyDiffPayload(opts: ApplyDiffPayloadOpts): ApplyJsonPayload { continue; } - // Overlap detection — one (file_path, line_start) tuple may have at - // most one row. Without this guard, two rows on the same line both - // pass phase-1's substring check (validated against original source); - // phase-2 then applies the first replace, the second's invariant - // assertion fails, the function throws AFTER earlier files in - // alphabetical order have already been renamed — partial cross-file - // state, no envelope returned. + // Reject the second-and-subsequent rows targeting one line — see + // `seenLines` declaration for the Q2 (c) violation this guards. const seen = seenLines.get(filePath); if (seen !== undefined && seen.has(lineStart)) { conflicts.push({ @@ -254,8 +231,7 @@ export function applyDiffPayload(opts: ApplyDiffPayloadOpts): ApplyJsonPayload { if (filePath !== undefined) distinctInputFiles.add(filePath); } - // Q2 (c) — any conflict aborts the whole run; dry-run never writes either. - // Both branches return the same envelope shape (Q5). + // Q2 (c) — any conflict aborts the run; dry-run never writes. Same Q5 envelope. if (dryRun || conflicts.length > 0) { const files: ApplyFile[] = conflicts.length === 0 @@ -280,32 +256,26 @@ export function applyDiffPayload(opts: ApplyDiffPayloadOpts): ApplyJsonPayload { }; } - // Phase 2 — apply edits per-file via temp + rename. + // Phase 2 — write each modified file via sibling temp + `rename`. const writtenFiles: ApplyFile[] = []; let appliedRows = 0; for (const filePath of [...pending.keys()].sort()) { const edits = pending.get(filePath)!; const cachedSource = sourceCache.get(filePath)!; - // Re-split the cached source on raw `"\n"` (not `/\r?\n/`) so CRLF - // lines retain their trailing `\r` and rejoin losslessly. const fileLines = cachedSource.split("\n"); - // Apply edits in descending line order — defensive default for when - // multi-line transforms land (today every row is a single-line - // replacement so order doesn't actually matter). + // Descending order is a defensive default — single-line replacements + // are index-stable today, but multi-line transforms aren't. edits.sort((a, b) => b.line_start - a.line_start); for (const edit of edits) { const idx = edit.line_start - 1; const actual = fileLines[idx]; - // The cached source was validated in phase 1; re-checking here guards - // against future regressions where the cache and validation drift. if (actual === undefined || !actual.includes(edit.before_pattern)) { throw new Error( `apply-engine: phase-2 invariant violated at ${filePath}:${edit.line_start} — phase-1 cache out of sync.`, ); } - // Pre-escape `$` per `String.prototype.replace` GetSubstitution rule - // so identifiers like `$inject` round-trip safely (mirrors - // `buildDiffJson`). + // Pre-escape `$` per `String.prototype.replace` GetSubstitution so + // identifiers like `$inject` round-trip safely (mirrors `buildDiffJson`). fileLines[idx] = actual.replace( edit.before_pattern, edit.after_pattern.replace(/\$/g, "$$$$"), @@ -313,14 +283,9 @@ export function applyDiffPayload(opts: ApplyDiffPayloadOpts): ApplyJsonPayload { } const newContent = fileLines.join("\n"); - // `resolvedRoot` is captured from the closure — phase-1 already - // verified the candidate is inside it, so this `resolve` collapses - // back to the same in-root absolute path. const absPath = resolve(resolvedRoot, filePath); - // Sibling temp + `rename`: POSIX-atomic when source and destination - // share a filesystem (always true for siblings). A concurrent reader - // sees either the pre-rename or post-rename content — never a torn - // write. + // POSIX-atomic when src + dst share a filesystem (true for siblings) — + // concurrent readers see pre- or post-rename content, never a torn write. const tempPath = `${absPath}.codemap-apply-${randomBytes(6).toString("hex")}.tmp`; writeFileSync(tempPath, newContent, "utf8"); renameSync(tempPath, absPath); @@ -363,12 +328,7 @@ function readPositiveInt( : undefined; } -/** - * `true` iff the resolved candidate lands inside `resolvedRoot` (or is - * the root itself). Both sides go through the same `resolve` semantics — - * no `realpath` / symlink dereference — so a project root that lives - * behind a symlink doesn't false-positive on its own descendants. - */ +/** `true` iff `resolve(resolvedRoot, candidate)` lands inside `resolvedRoot`. */ function isWithinRoot(resolvedRoot: string, candidate: string): boolean { const resolved = resolve(resolvedRoot, candidate); if (resolved === resolvedRoot) return true; diff --git a/src/application/tool-handlers.ts b/src/application/tool-handlers.ts index 504351f..8b2ee40 100644 --- a/src/application/tool-handlers.ts +++ b/src/application/tool-handlers.ts @@ -784,10 +784,7 @@ export const applyArgsSchema = { .record(z.string(), z.union([z.string(), z.number(), z.boolean()])) .optional(), dry_run: z.boolean().optional(), - /** - * Q6 (a) — non-TTY contexts (every MCP / HTTP transport) require an - * explicit `yes: true` to write. There's no prompt to fall back on. - */ + /** Q6 (a) — required for the write path; non-TTY transports have no prompt to fall back on. */ yes: z.boolean().optional(), }; @@ -799,12 +796,10 @@ export interface ApplyArgs { } /** - * Substrate-shaped fix executor. Reuses {@link applyDiffPayload} so the - * MCP / HTTP envelope is identical to the CLI's `--json` output (Q5). - * - * Q6 gating — over MCP / HTTP the only valid non-`dry_run` invocation - * carries `yes: true`. There is no TTY prompt; rejecting non-`yes` here - * is the agent-shaped equivalent of the CLI's non-TTY rejection. + * Substrate-shaped fix executor — reuses {@link applyDiffPayload} so the + * envelope is identical across CLI / MCP / HTTP (Q5). Non-`dry_run` + * invocations require `yes: true`; this is the agent-shaped equivalent + * of the CLI's non-TTY rejection. */ export function handleApply(args: ApplyArgs, root: string): ToolResult { try { diff --git a/src/cli/cmd-apply.test.ts b/src/cli/cmd-apply.test.ts index f2598b3..c9cfb02 100644 --- a/src/cli/cmd-apply.test.ts +++ b/src/cli/cmd-apply.test.ts @@ -165,10 +165,8 @@ describe("codemap apply — CLI integration", () => { expect(env.mode).toBe("apply"); expect(env.applied).toBe(true); expect(env.summary.rows_applied).toBeGreaterThan(0); - // Definition site should have been renamed. expect(readFile("src/helper.ts")).toContain("function worker("); expect(readFile("src/helper.ts")).not.toContain("function helper("); - // Import specifier on the consumer should have been renamed too. expect(readFile("src/entry.ts")).toContain("import { worker }"); }); diff --git a/src/cli/cmd-apply.ts b/src/cli/cmd-apply.ts index dde6683..be03cab 100644 --- a/src/cli/cmd-apply.ts +++ b/src/cli/cmd-apply.ts @@ -195,7 +195,7 @@ export async function runApplyCmd(opts: ApplyOpts): Promise { const projectRoot = getProjectRoot(); const isTTY = process.stdout.isTTY === true; - // Q6 gate (a): non-TTY without --yes / --dry-run is rejected. + // Q6 (a): non-TTY without --yes / --dry-run is rejected. if (!isTTY && !opts.yes && !opts.dryRun) { emitError( `codemap apply: this verb writes files. Pass --yes for non-interactive runs, or --dry-run for preview.`, @@ -204,7 +204,6 @@ export async function runApplyCmd(opts: ApplyOpts): Promise { return; } - // Direct paths (no prompt): --dry-run or --yes. if (opts.dryRun || opts.yes) { const result = applyDiffPayload({ rows: rows as Record[], @@ -215,22 +214,15 @@ export async function runApplyCmd(opts: ApplyOpts): Promise { return; } - // Interactive path: phase-1 dry-run preview → prompt → phase-2 apply. - // Phase-1 runs twice in the proceed case (once for the preview, once - // when applyDiffPayload re-runs phase-1 before phase-2). Cost is two - // file reads per pending file; acceptable for v1 since apply isn't a - // hot path. + // Interactive path: dry-run preview → prompt → apply. Phase-1 runs + // twice on accept (preview + the apply call's own pass) — two FS reads + // per pending file; fine for a non-hot CLI path. const preview = applyDiffPayload({ rows: rows as Record[], projectRoot, dryRun: true, }); - if (preview.conflicts.length > 0) { - emitResult(preview, opts); - return; - } - if (preview.files.length === 0) { - // Nothing to apply — emit the dry-run envelope and exit 0. + if (preview.conflicts.length > 0 || preview.files.length === 0) { emitResult(preview, opts); return; } @@ -238,8 +230,6 @@ export async function runApplyCmd(opts: ApplyOpts): Promise { printPromptSummary(preview, opts.recipeId, rows); const proceed = await promptYesNo(); if (!proceed) { - // User aborted; treat as a clean dry-run + stderr note. Exit 0 (the - // user explicitly chose not to apply; that's not an error). console.error("apply: aborted by user."); emitResult(preview, opts); return; @@ -303,9 +293,7 @@ function printPromptSummary( recipeId: string, rows: unknown[], ): void { - // Count input rows per file_path so the prompt shows per-file scope. - // The engine's `files[].rows_applied` is 0 in dry-run mode (per Q5); - // re-deriving from the input row set is the cheapest correct path. + // `files[].rows_applied` is 0 in dry-run (Q5); recount from input rows. const perFile = new Map(); for (const row of rows) { if (typeof row !== "object" || row === null) continue; diff --git a/templates/agents/rules/codemap.md b/templates/agents/rules/codemap.md index ce9b079..6f35d0b 100644 --- a/templates/agents/rules/codemap.md +++ b/templates/agents/rules/codemap.md @@ -83,7 +83,7 @@ Validation: SQL is rejected at load time if it starts with DML/DDL (DELETE/DROP/ **Impact (`codemap impact `)**: symbol/file blast-radius walker — replaces hand-composed `WITH RECURSIVE` queries that agents struggle to write. Target auto-resolves: contains `/` or matches `files.path` → file target; otherwise symbol (case-sensitive). Walks compatible graphs by target kind: **symbol** → `calls` (callers / callees by name); **file** → `dependencies` + `imports` (`resolved_path` only). `--via ` overrides; mismatched explicit choices land in `skipped_backends` (no error). Cycle-detected via `WITH RECURSIVE` path-string + `instr` check; bounded by `--depth N` (default 3, `0` = unbounded but still cycle-detected and limit-capped) and `--limit N` (default 500). Output envelope: `{target, direction, via, depth_limit, matches: [{depth, direction, edge, kind, name?, file_path}], summary: {nodes, max_depth_reached, by_kind, terminated_by: 'depth'|'limit'|'exhausted'}}`. `--summary` trims `matches` for cheap CI-gate consumption (`jq '.summary.nodes'`) but preserves the count. SARIF / annotations not supported (graph traversal, not findings). Pure transport-agnostic — same logic across CLI, MCP, and HTTP. -**Apply (`codemap apply `)**: substrate-shaped fix executor over the existing `--format diff-json` row contract — recipe SQL is the synthesis surface, codemap executes. Phase 1 validates every `{file_path, line_start, before_pattern, after_pattern}` row against current disk via `actual.includes(before_pattern)` (substring match — same contract `buildDiffJson` uses); collects three conflict reasons (`file missing` / `line out of range` / `line content drifted`). Phase 2 (gated on `!dryRun && conflicts.length === 0`) writes via sibling temp + `rename` for POSIX-atomic per-file writes. **Q2 (c) all-or-nothing** — any conflict aborts the whole run before any file is touched. **Q6 gate** — TTY prompts `Proceed? [y/N]` (default-N) on stderr; non-TTY (CI / agents / MCP / HTTP) requires `--yes` (or `yes: true`) explicitly; `--dry-run` + `--yes` mutually exclusive. Q7 idempotency: re-running on already-applied code reports `line content drifted` with `actual_at_line` showing the post-rename content — re-run `codemap` to refresh the index, then re-run apply (vacuous clean pass). Output envelope (identical across modes): `{mode: 'dry-run'|'apply', applied: bool, files: [{file_path, rows_applied, warnings?}], conflicts: [{file_path, line_start, before_pattern, actual_at_line, reason}], summary: {files, files_modified, rows, rows_applied, conflicts, files_with_conflicts}}`. Pure transport-agnostic engine in `application/apply-engine.ts`; CLI / MCP / HTTP all dispatch the same `applyDiffPayload` function. +**Apply (`codemap apply `)**: substrate-shaped fix executor over the existing `--format diff-json` row contract — recipe SQL is the synthesis surface, codemap executes. Phase 1 validates every `{file_path, line_start, before_pattern, after_pattern}` row against current disk via `actual.includes(before_pattern)` (substring match — same contract `buildDiffJson` uses); collects five conflict reasons (`file missing` / `line out of range` / `line content drifted` / `path escapes project root` / `duplicate edit on same line`). The `path escapes project root` guard rejects absolute `file_path` inputs and any candidate whose resolved form lands outside `projectRoot`; the `duplicate edit on same line` guard rejects two-or-more rows targeting the same `(file_path, line_start)` so phase 2 doesn't split mid-loop and leak Q2 (c). Phase 2 (gated on `!dryRun && conflicts.length === 0`) writes via sibling temp + `rename` for POSIX-atomic per-file writes. **Q2 (c) all-or-nothing** — any conflict aborts the whole run before any file is touched. **Q6 gate** — TTY prompts `Proceed? [y/N]` (default-N) on stderr; non-TTY (CI / agents / MCP / HTTP) requires `--yes` (or `yes: true`) explicitly; `--dry-run` + `--yes` mutually exclusive. Q7 idempotency: re-running on already-applied code reports `line content drifted` with `actual_at_line` showing the post-rename content — re-run `codemap` to refresh the index, then re-run apply (vacuous clean pass). Output envelope (identical across modes): `{mode: 'dry-run'|'apply', applied: bool, files: [{file_path, rows_applied, warnings?}], conflicts: [{file_path, line_start, before_pattern, actual_at_line, reason}], summary: {files, files_modified, rows, rows_applied, conflicts, files_with_conflicts}}`. Pure transport-agnostic engine in `application/apply-engine.ts`; CLI / MCP / HTTP all dispatch the same `applyDiffPayload` function. **MCP server (`codemap mcp`)**: stdio MCP (Model Context Protocol) server — agents call codemap as JSON-RPC tools instead of shelling out to the CLI on every read. v1 ships one tool per CLI verb plus six resources (`codemap://recipes` + `codemap://recipes/{id}` are live read every call so inline `last_run_at` / `run_count` recency stays fresh; `codemap://schema` + `codemap://skill` lazy-cache; `codemap://files/{path}` + `codemap://symbols/{name}` always live): From 1c43bac922143586863ce179abc543b97150c60c Mon Sep 17 00:00:00 2001 From: Sutu Sebastian Date: Wed, 6 May 2026 13:51:37 +0300 Subject: [PATCH 8/9] fix(apply): address CodeRabbit review (7 of 9; 2 already fixed) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Triaged 9 actionable comments via pr-comment-fact-check. Each finding verified against the source on aaabc13; 2 were already addressed by the prior commit (CodeRabbit auto-tagged with "✅ Addressed in aaabc13"); 7 are new fixes here: F1 (paragraph merge in .agents/rules/codemap.md, partial earlier-fix): CodeRabbit's auto-tag was optimistic — the conflict-reason count was synced in aaabc13 but the Impact section's tail (`... --summary trims …`jq '.summary.nodes'``) was still stitched onto the END of the Apply paragraph. Restored the section break. F3 (after_pattern: "" silently dropped): `readString` rejected empty strings, so a deletion-shaped row got silently skipped by phase-1's required-keys check. New `readStringAllowEmpty` helper for `after_pattern` only — empty `before_pattern` still rejected (would match anywhere on the line). Regression test deletes a `// FIXME(team): ` prefix. F4 (cache-key dedup `a.ts` vs `./a.ts`): Pre-fix, the cache + pending + seenLines maps used the raw `file_path` as their key. Two rows naming the same disk file via different spellings created two cache entries → second write clobbered the first edit. New `canonicalizeFilePath` collapses every spelling to a project-relative form. Symlink-realpath defense remains documented as a separate (heavier) follow-up. F5 (Q2 (c) over-promised on I/O failures): CodeRabbit's "🔴 Heavy lift" — a writeFileSync/renameSync mid-loop failure leaves files 1..N-1 already renamed with no rollback. Full transactional rollback (per-file backups + restore-on-throw) is deferred. Honest fix: weakened the Q2 (c) claim in `architecture.md § Apply wiring` to "all-or-nothing (semantic) — phase-1 conflicts abort phase 2 entirely; phase-2 I/O failures are NOT transactional across files." Engine docstring carries the same caveat as a fourth call-out. F6 (TTY check used wrong stream): Gate checked `process.stdout.isTTY` but `promptYesNo()` reads from `process.stdin` and writes to `process.stderr`. So `codemap apply foo | tee log.txt` (interactive stdin, piped stdout) was rejected as non-TTY. Now gates on `stdin.isTTY && stderr.isTTY`. F7 (user-cancel rendered "no rows applicable"): Abort path called `emitResult(preview, opts)` with `opts.dryRun === false`, so `renderTerminal` fell through to "no rows applicable" — contradicting the user's explicit cancel. Terminal mode now prints `apply : aborted by user; no files written.`; JSON consumers still get the full preview envelope. F9 (skill files missed two conflict reasons): `.agents/skills/codemap/SKILL.md` + `templates/agents/skills/ codemap/SKILL.md` apply tool description didn't enumerate the 5 conflict reasons. Synced. Tests: 44/44 (3 new — `./a.ts` dedup, deletion via empty after_pattern, empty-before-still-rejected). Typecheck / lint / format clean. --- .agents/rules/codemap.md | 4 +- .agents/skills/codemap/SKILL.md | 2 +- docs/architecture.md | 2 +- src/application/apply-engine.test.ts | 77 ++++++++++++++++++++++++ src/application/apply-engine.ts | 72 +++++++++++++++++----- src/cli/cmd-apply.ts | 21 +++++-- templates/agents/skills/codemap/SKILL.md | 2 +- 7 files changed, 157 insertions(+), 23 deletions(-) diff --git a/.agents/rules/codemap.md b/.agents/rules/codemap.md index b0896b3..fd34a67 100644 --- a/.agents/rules/codemap.md +++ b/.agents/rules/codemap.md @@ -74,9 +74,9 @@ Validation: SQL is rejected at load time if it starts with DML/DDL (DELETE/DROP/ **Targeted reads (`show` / `snippet`)**: precise lookup by exact symbol name without composing SQL. `show` returns metadata (`file_path:line_start-line_end` + `signature`); `snippet` returns the source text from disk plus `stale` / `missing` flags. Both share the same flag set (`--kind ` to filter by `symbols.kind`, `--in ` for file-scope filter — directory prefix or exact file). Output envelope is `{matches, disambiguation?}` — single match → `{matches: [{...}]}`; multi-match adds `disambiguation: {n, by_kind, files, hint}` so agents narrow without re-scanning. Name match is exact / case-sensitive — for fuzzy use `query` with `LIKE '%name%'`. Snippet stale-file behavior: `source` is always returned when the file exists; `stale: true` means the line range may have shifted (re-index with `bun src/index.ts` or `--files ` before acting on the source). -**Impact (`bun src/index.ts impact `)**: symbol/file blast-radius walker — replaces hand-composed `WITH RECURSIVE` queries that agents struggle to write. Target auto-resolves: contains `/` or matches `files.path` → file target; otherwise symbol (case-sensitive). Walks compatible graphs by target kind: **symbol** → `calls` (callers / callees by name); **file** → `dependencies` + `imports` (`resolved_path` only). `--via ` overrides; mismatched explicit choices land in `skipped_backends` (no error). Cycle-detected via `WITH RECURSIVE` path-string + `instr` check; bounded by `--depth N` (default 3, `0` = unbounded but still cycle-detected and limit-capped) and `--limit N` (default 500). Output envelope: `{target, direction, via, depth_limit, matches: [{depth, direction, edge, kind, name?, file_path}], summary: {nodes, max_depth_reached, by_kind, terminated_by: 'depth'|'limit'|'exhausted'}}`. `--summary` trims `matches` for cheap CI-gate consumption (`jq '.summary.nodes'` +**Impact (`bun src/index.ts impact `)**: symbol/file blast-radius walker — replaces hand-composed `WITH RECURSIVE` queries that agents struggle to write. Target auto-resolves: contains `/` or matches `files.path` → file target; otherwise symbol (case-sensitive). Walks compatible graphs by target kind: **symbol** → `calls` (callers / callees by name); **file** → `dependencies` + `imports` (`resolved_path` only). `--via ` overrides; mismatched explicit choices land in `skipped_backends` (no error). Cycle-detected via `WITH RECURSIVE` path-string + `instr` check; bounded by `--depth N` (default 3, `0` = unbounded but still cycle-detected and limit-capped) and `--limit N` (default 500). Output envelope: `{target, direction, via, depth_limit, matches: [{depth, direction, edge, kind, name?, file_path}], summary: {nodes, max_depth_reached, by_kind, terminated_by: 'depth'|'limit'|'exhausted'}}`. `--summary` trims `matches` for cheap CI-gate consumption (`jq '.summary.nodes'`) but preserves the count. SARIF / annotations not supported (graph traversal, not findings). Pure transport-agnostic engine in `application/impact-engine.ts`; CLI / MCP / HTTP all dispatch the same `findImpact` function. -**Apply (`bun src/index.ts apply `)**: substrate-shaped fix executor over the existing `--format diff-json` row contract — recipe SQL is the synthesis surface, codemap executes. Phase 1 validates every `{file_path, line_start, before_pattern, after_pattern}` row against current disk via `actual.includes(before_pattern)` (substring match — same contract `buildDiffJson` uses); collects five conflict reasons (`file missing` / `line out of range` / `line content drifted` / `path escapes project root` / `duplicate edit on same line`). The `path escapes project root` guard rejects absolute `file_path` inputs and any candidate whose resolved form lands outside `projectRoot`; the `duplicate edit on same line` guard rejects two-or-more rows targeting the same `(file_path, line_start)` so phase 2 doesn't split mid-loop and leak Q2 (c). Phase 2 (gated on `!dryRun && conflicts.length === 0`) writes via sibling temp + `rename` for POSIX-atomic per-file writes. **Q2 (c) all-or-nothing** — any conflict aborts the whole run before any file is touched. **Q6 gate** — TTY prompts `Proceed? [y/N]` (default-N) on stderr; non-TTY (CI / agents / MCP / HTTP) requires `--yes` (or `yes: true`) explicitly; `--dry-run` + `--yes` mutually exclusive. Q7 idempotency: re-running on already-applied code reports `line content drifted` with `actual_at_line` showing the post-rename content — re-run `bun src/index.ts` to refresh, then re-run apply (vacuous clean pass). Output envelope (identical across modes): `{mode: 'dry-run'|'apply', applied: bool, files: [{file_path, rows_applied, warnings?}], conflicts: [{file_path, line_start, before_pattern, actual_at_line, reason}], summary: {files, files_modified, rows, rows_applied, conflicts, files_with_conflicts}}`. Pure transport-agnostic engine in `application/apply-engine.ts`; CLI / MCP / HTTP all dispatch the same `applyDiffPayload` function.) but preserves the count. SARIF / annotations not supported (graph traversal, not findings). Pure transport-agnostic engine in `application/impact-engine.ts`; CLI / MCP / HTTP all dispatch the same `findImpact` function. +**Apply (`bun src/index.ts apply `)**: substrate-shaped fix executor over the existing `--format diff-json` row contract — recipe SQL is the synthesis surface, codemap executes. Phase 1 validates every `{file_path, line_start, before_pattern, after_pattern}` row against current disk via `actual.includes(before_pattern)` (substring match — same contract `buildDiffJson` uses); collects five conflict reasons (`file missing` / `line out of range` / `line content drifted` / `path escapes project root` / `duplicate edit on same line`). The `path escapes project root` guard rejects absolute `file_path` inputs and any candidate whose resolved form lands outside `projectRoot`; the `duplicate edit on same line` guard rejects two-or-more rows targeting the same `(file_path, line_start)` so phase 2 doesn't split mid-loop and leak Q2 (c). Phase 2 (gated on `!dryRun && conflicts.length === 0`) writes via sibling temp + `rename` for POSIX-atomic per-file writes. **Q2 (c) all-or-nothing** — any conflict aborts the whole run before any file is touched. **Q6 gate** — TTY prompts `Proceed? [y/N]` (default-N) on stderr; non-TTY (CI / agents / MCP / HTTP) requires `--yes` (or `yes: true`) explicitly; `--dry-run` + `--yes` mutually exclusive. Q7 idempotency: re-running on already-applied code reports `line content drifted` with `actual_at_line` showing the post-rename content — re-run `bun src/index.ts` to refresh, then re-run apply (vacuous clean pass). Output envelope (identical across modes): `{mode: 'dry-run'|'apply', applied: bool, files: [{file_path, rows_applied, warnings?}], conflicts: [{file_path, line_start, before_pattern, actual_at_line, reason}], summary: {files, files_modified, rows, rows_applied, conflicts, files_with_conflicts}}`. Pure transport-agnostic engine in `application/apply-engine.ts`; CLI / MCP / HTTP all dispatch the same `applyDiffPayload` function. **MCP server (`bun src/index.ts mcp`)**: stdio MCP (Model Context Protocol) server — agents call codemap as JSON-RPC tools instead of shelling out to the CLI on every read. v1 ships one tool per CLI verb plus six resources (`codemap://recipes` + `codemap://recipes/{id}` are live read every call so inline `last_run_at` / `run_count` recency stays fresh; `codemap://schema` + `codemap://skill` lazy-cache; `codemap://files/{path}` + `codemap://symbols/{name}` always live): diff --git a/.agents/skills/codemap/SKILL.md b/.agents/skills/codemap/SKILL.md index 688e4cf..f06ec48 100644 --- a/.agents/skills/codemap/SKILL.md +++ b/.agents/skills/codemap/SKILL.md @@ -79,7 +79,7 @@ Each emitted delta carries its own `base` metadata so mixed-baseline audits are - **`show`** — `{name, kind?, in?}`. Exact, case-sensitive symbol name lookup. Returns `{matches: [{name, kind, file_path, line_start, line_end, signature, ...}], disambiguation?: {n, by_kind, files, hint}}`. Single match → `{matches: [{...}]}`; multi-match adds the disambiguation envelope so you narrow without re-scanning. Fuzzy lookup belongs in `query` with `LIKE`. - **`snippet`** — `{name, kind?, in?}`. Same lookup as `show` but each match also carries `source` (file lines from disk at `line_start..line_end`), `stale` (true when content_hash drifted since indexing — line range may have shifted), `missing` (true when file is gone). Per Q-6 (settled): `source` is always returned when the file exists; agent decides whether to act on stale content or run `codemap` / `codemap --files ` to re-index first. No auto-reindex side-effects from this read tool. - **`impact`** — `{target, direction?, via?, depth?, limit?, summary?}`. Symbol/file blast-radius walker — replaces hand-composed `WITH RECURSIVE` queries that agents struggle to write reliably. `target` is a symbol name (case-sensitive, exact) OR a project-relative file path (auto-detected by `/` or by matching `files.path`). `direction`: `up` (callers / dependents), `down` (callees / dependencies), `both` (default). `via`: `dependencies`, `calls`, `imports`, `all` (default — every backend compatible with the resolved target kind: symbol → `calls`; file → `dependencies` + `imports`; mismatched explicit choices land in `skipped_backends`, no error). `depth` default 3, `0` = unbounded (still cycle-detected and limit-capped). `limit` default 500. `summary: true` trims `matches` for cheap CI-gate consumption (`jq '.summary.nodes'`) but preserves the count. Result: `{target, direction, via, depth_limit, matches: [{depth, direction, edge, kind, name?, file_path}], summary: {nodes, max_depth_reached, by_kind, terminated_by: 'depth'|'limit'|'exhausted'}}`. Cycle detection is approximate-but-bounded — bounded depth + `LIMIT` keep cyclic graphs cheap; `terminated_by` reports the dominant stop reason. SARIF / annotations not supported (impact rows are graph traversals, not findings). -- **`apply`** — `{recipe, params?, dry_run?, yes?}`. Substrate-shaped fix executor — runs the same recipe `query_recipe` runs, then applies the diff hunks each row describes (`{file_path, line_start, before_pattern, after_pattern}`) to disk. Recipe SQL is the synthesis surface; codemap is the executor. Phase 1 validates every row against current disk via substring-match (`actual.includes(before_pattern)`) — the same contract `--format diff-json` uses. Phase 2 (gated on no conflicts) writes via sibling temp + `rename` for POSIX-atomic per-file writes. **Q2 (c) all-or-nothing** — any conflict in any file aborts phase 2 entirely; partial writes never ship. Over MCP/HTTP `yes: true` is required for the write path (no TTY prompt to fall back on); `dry_run: true` previews without writing; the two are mutually exclusive. Re-running on already-applied code reports a `line content drifted` conflict whose `actual_at_line` shows the post-rename content — re-run `bun src/index.ts` to refresh the index, then re-run `apply` for a clean vacuous pass. Result envelope (identical across modes): `{mode: 'dry-run'|'apply', applied, files: [{file_path, rows_applied, warnings?}], conflicts: [{file_path, line_start, before_pattern, actual_at_line, reason}], summary: {files, files_modified, rows, rows_applied, conflicts, files_with_conflicts}}`. Floor "No fix engine" preserved — codemap doesn't synthesise edits, it only executes the hunks the recipe row described. +- **`apply`** — `{recipe, params?, dry_run?, yes?}`. Substrate-shaped fix executor — runs the same recipe `query_recipe` runs, then applies the diff hunks each row describes (`{file_path, line_start, before_pattern, after_pattern}`) to disk. Recipe SQL is the synthesis surface; codemap is the executor. Phase 1 validates every row against current disk via substring-match (`actual.includes(before_pattern)`) — the same contract `--format diff-json` uses; collects five conflict reasons (`file missing` / `line out of range` / `line content drifted` / `path escapes project root` / `duplicate edit on same line`). Phase 2 (gated on no conflicts) writes via sibling temp + `rename` for POSIX-atomic per-file writes. **Q2 (c) all-or-nothing** — any conflict in any file aborts phase 2 entirely; partial writes never ship. Over MCP/HTTP `yes: true` is required for the write path (no TTY prompt to fall back on); `dry_run: true` previews without writing; the two are mutually exclusive. Re-running on already-applied code reports a `line content drifted` conflict whose `actual_at_line` shows the post-rename content — re-run `bun src/index.ts` to refresh the index, then re-run `apply` for a clean vacuous pass. Result envelope (identical across modes): `{mode: 'dry-run'|'apply', applied, files: [{file_path, rows_applied, warnings?}], conflicts: [{file_path, line_start, before_pattern, actual_at_line, reason}], summary: {files, files_modified, rows, rows_applied, conflicts, files_with_conflicts}}`. Floor "No fix engine" preserved — codemap doesn't synthesise edits, it only executes the hunks the recipe row described. **Resources (mostly lazy-cached on first `read_resource`; recipes / one-recipe live-read every call so the inline recency fields stay fresh):** diff --git a/docs/architecture.md b/docs/architecture.md index 415556e..f7dc5b4 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -131,7 +131,7 @@ A local SQLite database (`.codemap/index.db`) indexes the project tree and store **Impact wiring:** **`src/cli/cmd-impact.ts`** (argv — `` + `--direction up|down|both` + `--depth N` + `--via dependencies|calls|imports|all` + `--limit N` + `--summary` + `--json`; bootstrap absorbs `--root`/`--config`) + **`src/application/impact-engine.ts`** (engine — `findImpact({db, target, direction?, via?, depth?, limit?})`). Pure transport-agnostic walker over the calls + dependencies + imports graphs; CLI / MCP / HTTP all dispatch the same engine function via `tool-handlers.ts`'s `handleImpact`. Target auto-resolves: contains `/` or matches `files.path` → file target; otherwise symbol (case-sensitive). Walks compatible backends per resolved kind: **symbol** → `calls` (callers / callees by `caller_name` / `callee_name`); **file** → `dependencies` (`from_path` / `to_path`) + `imports` (`file_path` / `resolved_path`, `IS NOT NULL` filter). `--via ` overrides; mismatched explicit choices land in `skipped_backends` (no error — agents see why their backend selection yielded fewer rows than expected). One `WITH RECURSIVE` per (direction, backend) combo with cycle detection via path-string `instr` check (SQLite has no native cycle predicate); JS-side merge + dedup by `(direction, kind, name?, file_path)` keeping the shallowest depth. `--depth 0` uses an unbounded sentinel (`UNBOUNDED_DEPTH_SENTINEL = 1_000_000`); cycle detection + `LIMIT` keep cyclic graphs cheap regardless. Termination reason classification: `limit` (truncated) > `depth` (any node sat at the cap) > `exhausted`. Result envelope: `{target, direction, via, depth_limit, matches: [{depth, direction, edge, kind, name?, file_path}], summary: {nodes, max_depth_reached, by_kind, terminated_by}, skipped_backends?}`. `--summary` blanks `matches` (transport bandwidth saver) but preserves `summary.nodes` so CI gates (`jq '.summary.nodes'`) still see the count. SARIF / annotations not supported (graph traversal, not findings — the parser accepts the flag combos but the engine only emits JSON). -**Apply wiring:** **`src/cli/cmd-apply.ts`** (argv — `` + `--params` + `--dry-run` + `--yes` + `--json`; bootstrap absorbs `--root`/`--config`) + **`src/application/apply-engine.ts`** (engine — `applyDiffPayload({rows, projectRoot, dryRun})`). Pure transport-agnostic substrate-shaped fix executor: consumes the existing `--format diff-json` row contract from any recipe (`{file_path, line_start, before_pattern, after_pattern}`), validates each row against current disk, and either previews (dry-run) or writes (apply). CLI / MCP / HTTP all dispatch the same engine via `tool-handlers.ts`'s `handleApply`. **Phase 1** (always) resolves the project root via `path.resolve(projectRoot)` once, then for each row: rejects absolute `file_path` inputs and any candidate whose `path.resolve(resolvedRoot, file_path)` lands outside `resolvedRoot` (conflict `path escapes project root` — guards CLI + MCP + HTTP write paths against `../escape.ts`-style traversal); rejects duplicate `(file_path, line_start)` tuples (conflict `duplicate edit on same line` — without this, two phase-1-passing rows targeting the same line would split the run mid-phase-2 because the first replace invalidates the second's substring assertion, leaving Q2 (c) cross-file partial state). Reads each file at most once into `sourceCache`, splits on `/\r?\n/` for conflict reporting, checks `actual.includes(before_pattern)` (substring match — mirrors `buildDiffJson`'s contract; `rename-preview` emits `before_pattern = old_name` as the bare identifier, so whole-line exact match would conflict every time). Conflicts collect five reasons (`file missing` / `line out of range` / `line content drifted` / `path escapes project root` / `duplicate edit on same line`) — Q3 scan-and-collect, not fail-fast. **Phase 2** (gated on `!dryRun && conflicts.length === 0`) re-splits the cached source on raw `"\n"` (preserves CRLF as trailing `\r` per line; rejoining with `"\n"` round-trips losslessly), applies each file's edits in descending line order via `actual.replace(before, after)` with `$`-pre-escape (`replace(/\$/g, "$$$$")` — matches `buildDiffJson`'s GetSubstitution defence so identifiers like `$inject` round-trip safely), writes to a sibling temp path (`.codemap-apply-.tmp`), then `renameSync` into place — POSIX-atomic per file; concurrent readers see either pre-rename or post-rename content, never a torn write. **Q2 (c) all-or-nothing**: any conflict in any file aborts phase 2 entirely before any file is touched. **Q6 gate**: TTY no `--yes` → phase-1 preview + `Proceed? [y/N]` prompt on stderr (default-N, `node:readline/promises`); TTY `--yes` → no prompt; non-TTY (CI / agents / MCP) without `--yes`/`--dry-run` rejected with stderr message. `--dry-run` + `--yes` mutually exclusive (parse-time error). MCP/HTTP transports (`handleApply`) require `yes: true` for the write path — there's no prompt to fall back on; `dry_run + yes` rejected as mutually exclusive. Result envelope (Q5; identical across modes): `{mode: 'dry-run'|'apply', applied: bool, files: [{file_path, rows_applied, warnings?}], conflicts: [{file_path, line_start, before_pattern, actual_at_line, reason}], summary: {files, files_modified, rows, rows_applied, conflicts, files_with_conflicts}}`. `applied: true` only when `mode === 'apply'` AND zero conflicts AND at least one row applied. Q7 idempotency: re-running on already-applied code reports a `line content drifted` conflict with `actual_at_line` showing the post-rename content; the user reads it and re-runs `codemap` to refresh the index → next run produces 0 rows (recipe finds nothing to rename) → vacuous clean apply. **Same-line ambiguity caveat (documented limitation):** `actual.replace(before_pattern, after_pattern)` rewrites only the **first** occurrence on the line. When `before_pattern` appears twice (e.g. `const foo = foo();` with `before = "foo"`) only the leftmost is replaced; the engine still reports `applied: true`. This mirrors `buildDiffJson`'s formatter contract verbatim — recipe authors who hit it normalise their SQL to emit a more specific pattern, or accept it (the formatter's `--format diff` preview shows the same shape). Promotion path: tighten phase-1 to conflict on ambiguity in a future PR if real users complain, but only alongside the formatter so preview and execution stay in lockstep. SARIF / annotations not supported (write action, not findings). TOCTOU: phase-1 reads through `sourceCache`; phase-2 transforms the cached source and writes — the gap between read and rename is a deliberate v1 simplification (apply isn't adversarial). Per Q10, only `cli/cmd-apply.ts` + `application/tool-handlers.ts` (+ the test files) may import `apply-engine.ts` for production execution — re-runnable forbidden-edge query at [§ Boundary verification — apply write path](#boundary-verification--apply-write-path). +**Apply wiring:** **`src/cli/cmd-apply.ts`** (argv — `` + `--params` + `--dry-run` + `--yes` + `--json`; bootstrap absorbs `--root`/`--config`) + **`src/application/apply-engine.ts`** (engine — `applyDiffPayload({rows, projectRoot, dryRun})`). Pure transport-agnostic substrate-shaped fix executor: consumes the existing `--format diff-json` row contract from any recipe (`{file_path, line_start, before_pattern, after_pattern}`), validates each row against current disk, and either previews (dry-run) or writes (apply). CLI / MCP / HTTP all dispatch the same engine via `tool-handlers.ts`'s `handleApply`. **Phase 1** (always) resolves the project root via `path.resolve(projectRoot)` once, then for each row: rejects absolute `file_path` inputs and any candidate whose `path.resolve(resolvedRoot, file_path)` lands outside `resolvedRoot` (conflict `path escapes project root` — guards CLI + MCP + HTTP write paths against `../escape.ts`-style traversal); rejects duplicate `(file_path, line_start)` tuples (conflict `duplicate edit on same line` — without this, two phase-1-passing rows targeting the same line would split the run mid-phase-2 because the first replace invalidates the second's substring assertion, leaving Q2 (c) cross-file partial state). Reads each file at most once into `sourceCache`, splits on `/\r?\n/` for conflict reporting, checks `actual.includes(before_pattern)` (substring match — mirrors `buildDiffJson`'s contract; `rename-preview` emits `before_pattern = old_name` as the bare identifier, so whole-line exact match would conflict every time). Conflicts collect five reasons (`file missing` / `line out of range` / `line content drifted` / `path escapes project root` / `duplicate edit on same line`) — Q3 scan-and-collect, not fail-fast. **Phase 2** (gated on `!dryRun && conflicts.length === 0`) re-splits the cached source on raw `"\n"` (preserves CRLF as trailing `\r` per line; rejoining with `"\n"` round-trips losslessly), applies each file's edits in descending line order via `actual.replace(before, after)` with `$`-pre-escape (`replace(/\$/g, "$$$$")` — matches `buildDiffJson`'s GetSubstitution defence so identifiers like `$inject` round-trip safely), writes to a sibling temp path (`.codemap-apply-.tmp`), then `renameSync` into place — POSIX-atomic per file; concurrent readers see either pre-rename or post-rename content, never a torn write. **Q2 (c) all-or-nothing (semantic)**: any phase-1 conflict aborts phase 2 entirely before any file is touched. Phase-2 I/O failures (`writeFileSync` / `renameSync`) are NOT transactional across files — per-file atomicity holds (temp + rename), but a crash on file N leaves files `1..N-1` already renamed with no rollback; cross-file rollback would require pre-write backups + restore-on-throw and is deferred to a future PR. **Q6 gate**: TTY no `--yes` → phase-1 preview + `Proceed? [y/N]` prompt on stderr (default-N, `node:readline/promises`); TTY `--yes` → no prompt; non-TTY (CI / agents / MCP) without `--yes`/`--dry-run` rejected with stderr message. `--dry-run` + `--yes` mutually exclusive (parse-time error). MCP/HTTP transports (`handleApply`) require `yes: true` for the write path — there's no prompt to fall back on; `dry_run + yes` rejected as mutually exclusive. Result envelope (Q5; identical across modes): `{mode: 'dry-run'|'apply', applied: bool, files: [{file_path, rows_applied, warnings?}], conflicts: [{file_path, line_start, before_pattern, actual_at_line, reason}], summary: {files, files_modified, rows, rows_applied, conflicts, files_with_conflicts}}`. `applied: true` only when `mode === 'apply'` AND zero conflicts AND at least one row applied. Q7 idempotency: re-running on already-applied code reports a `line content drifted` conflict with `actual_at_line` showing the post-rename content; the user reads it and re-runs `codemap` to refresh the index → next run produces 0 rows (recipe finds nothing to rename) → vacuous clean apply. **Same-line ambiguity caveat (documented limitation):** `actual.replace(before_pattern, after_pattern)` rewrites only the **first** occurrence on the line. When `before_pattern` appears twice (e.g. `const foo = foo();` with `before = "foo"`) only the leftmost is replaced; the engine still reports `applied: true`. This mirrors `buildDiffJson`'s formatter contract verbatim — recipe authors who hit it normalise their SQL to emit a more specific pattern, or accept it (the formatter's `--format diff` preview shows the same shape). Promotion path: tighten phase-1 to conflict on ambiguity in a future PR if real users complain, but only alongside the formatter so preview and execution stay in lockstep. SARIF / annotations not supported (write action, not findings). TOCTOU: phase-1 reads through `sourceCache`; phase-2 transforms the cached source and writes — the gap between read and rename is a deliberate v1 simplification (apply isn't adversarial). Per Q10, only `cli/cmd-apply.ts` + `application/tool-handlers.ts` (+ the test files) may import `apply-engine.ts` for production execution — re-runnable forbidden-edge query at [§ Boundary verification — apply write path](#boundary-verification--apply-write-path). **Show / snippet wiring:** **`src/cli/cmd-show.ts`** + **`src/cli/cmd-snippet.ts`** — sibling CLI verbs sharing the same parser shape (`` + `--kind` + `--in ` + `--json`) and the pure engine **`src/application/show-engine.ts`** (`findSymbolsByName({db, name, kind?, inPath?})` for the lookup; `readSymbolSource({match, projectRoot, indexedContentHash?})` + `getIndexedContentHash(db, filePath)` for the snippet-side FS read; **`buildShowResult`** + **`buildSnippetResult`** envelope builders — same engine the MCP show/snippet tools call). Both verbs return the same `{matches, disambiguation?}` envelope per plan § 4 uniformity — single match → `{matches: [{...}]}`; multi-match adds `{n, by_kind, files, hint}`. Snippet matches add `source` / `stale` / `missing` fields (additive — no shape divergence). **`--in `** is normalized through `toProjectRelative(projectRoot, p)` (from **`src/application/validate-engine.ts`**) so `--in ./src/cli/`, `--in src/cli`, and `--in src/cli/cmd-show.ts` all resolve identically. Stale-file behavior on `snippet`: `hashContent` (from **`src/hash.ts`** — same primitive `cmd-validate.ts` uses) compares the on-disk content_hash against `files.content_hash`; mismatch sets `stale: true` but the source IS still returned (read tool, no auto-reindex side-effects). MCP tools `show` and `snippet` register parallel to the CLI surface (see [§ MCP wiring](#cli-usage)). diff --git a/src/application/apply-engine.test.ts b/src/application/apply-engine.test.ts index a59b04c..758d9b9 100644 --- a/src/application/apply-engine.test.ts +++ b/src/application/apply-engine.test.ts @@ -644,6 +644,83 @@ describe("applyDiffPayload", () => { expect(readSource(root, "a.ts")).toBe(aBefore); expect(readSource(root, "b.ts")).toBe(bBefore); }); + + it("dedups `a.ts` and `./a.ts` to the same canonical key", () => { + const root = tmpProject(); + writeSource(root, "a.ts", "const foo = 1;\n"); + + const result = applyDiffPayload({ + rows: [ + { + file_path: "a.ts", + line_start: 1, + before_pattern: "foo", + after_pattern: "X", + }, + // Same disk file via a different spelling — pre-fix this evaded + // the overlap guard and racing writes clobbered each other. + { + file_path: "./a.ts", + line_start: 1, + before_pattern: "foo", + after_pattern: "Y", + }, + ], + projectRoot: root, + dryRun: false, + }); + + expect(result.applied).toBe(false); + expect(result.conflicts[0]?.reason).toBe("duplicate edit on same line"); + expect(readSource(root, "a.ts")).toBe("const foo = 1;\n"); + }); + }); + + describe("after_pattern allows empty string (deletion)", () => { + it("deletes the leftmost match of `before_pattern` when `after_pattern` is empty", () => { + const root = tmpProject(); + writeSource(root, "a.ts", "// FIXME(team): this comment\nconst x = 1;\n"); + + const result = applyDiffPayload({ + rows: [ + { + file_path: "a.ts", + line_start: 1, + before_pattern: "FIXME(team): ", + after_pattern: "", + }, + ], + projectRoot: root, + dryRun: false, + }); + + expect(result.applied).toBe(true); + expect(result.summary.rows_applied).toBe(1); + expect(readSource(root, "a.ts")).toBe("// this comment\nconst x = 1;\n"); + }); + + it("still rejects a row with empty `before_pattern`", () => { + // Empty `before_pattern` would match anywhere on the line — a row + // with `before: ""` is malformed, not a deletion intent. + const root = tmpProject(); + writeSource(root, "a.ts", "const foo = 1;\n"); + + const result = applyDiffPayload({ + rows: [ + { + file_path: "a.ts", + line_start: 1, + before_pattern: "", + after_pattern: "anything", + }, + ], + projectRoot: root, + dryRun: true, + }); + + expect(result.summary.rows).toBe(0); // row silently dropped + expect(result.files).toEqual([]); + }); }); describe("same-line ambiguity (F3 — documented limitation)", () => { diff --git a/src/application/apply-engine.ts b/src/application/apply-engine.ts index 8e02d63..9a8c85d 100644 --- a/src/application/apply-engine.ts +++ b/src/application/apply-engine.ts @@ -9,12 +9,18 @@ * (Q1–Q10 locks, semantics, exit codes) at * [`docs/architecture.md`](../../docs/architecture.md) § Apply wiring. * - * Three behaviours called out here because the code alone won't betray them: + * Four behaviours called out here because the code alone won't betray them: * * - **Same-line ambiguity.** `actual.replace(before, after)` is first- * occurrence — mirrors `buildDiffJson` verbatim. `const foo = foo();` * with `before = "foo"` rewrites only the leftmost; recipe authors * normalise their SQL or accept the formatter-parity behaviour. + * - **Phase-2 I/O failures are NOT transactional across files.** Q2 (c) + * guarantees no partial state when phase 1 collects a conflict — phase 2 + * doesn't run. But once phase 2 starts, a `writeFileSync` / `renameSync` + * crash on file N leaves files `1..N-1` already renamed with no rollback. + * Per-file atomicity (temp + rename) is preserved; cross-file rollback + * would require pre-write backups + a restore loop and is deferred. * - **TOCTOU.** Phase-1 caches the source it validated; phase-2 transforms * the cache and writes. The read→rename window is a v1 simplification * (apply isn't adversarial; no lock files). @@ -126,7 +132,10 @@ export function applyDiffPayload(opts: ApplyDiffPayloadOpts): ApplyJsonPayload { const filePath = readString(row, "file_path"); const lineStart = readPositiveInt(row, "line_start"); const before = readString(row, "before_pattern"); - const after = readString(row, "after_pattern"); + // `after_pattern: ""` is the deletion case — `actual.replace(before, "")` + // strips the leftmost match. Empty `before_pattern` stays disallowed + // (would match anywhere on the line, including the start). + const after = readStringAllowEmpty(row, "after_pattern"); if ( filePath === undefined || lineStart === undefined || @@ -150,13 +159,19 @@ export function applyDiffPayload(opts: ApplyDiffPayloadOpts): ApplyJsonPayload { continue; } - let source = sourceCache.get(filePath); + // Canonicalise to a single key so `a.ts`, `./a.ts`, and `src/../a.ts` + // all dedup into the same cache + pending entry. Without this, two + // rows naming the same disk file via different spellings would race in + // phase 2 — second writeFileSync clobbers the first edit. + const canonicalPath = canonicalizeFilePath(resolvedRoot, filePath); + + let source = sourceCache.get(canonicalPath); if (source === undefined) { try { - source = readFileSync(resolve(resolvedRoot, filePath), "utf8"); + source = readFileSync(resolve(resolvedRoot, canonicalPath), "utf8"); } catch { conflicts.push({ - file_path: filePath, + file_path: canonicalPath, line_start: lineStart, before_pattern: before, actual_at_line: "", @@ -164,7 +179,7 @@ export function applyDiffPayload(opts: ApplyDiffPayloadOpts): ApplyJsonPayload { }); continue; } - sourceCache.set(filePath, source); + sourceCache.set(canonicalPath, source); } // Phase-1 splits on `/\r?\n/` so `actual_at_line` is `\r`-free; phase-2 // re-splits on raw `\n` for round-trip-safe writes (module docstring § EOL). @@ -172,7 +187,7 @@ export function applyDiffPayload(opts: ApplyDiffPayloadOpts): ApplyJsonPayload { const actual = lines[lineStart - 1]; if (actual === undefined) { conflicts.push({ - file_path: filePath, + file_path: canonicalPath, line_start: lineStart, before_pattern: before, actual_at_line: "", @@ -182,7 +197,7 @@ export function applyDiffPayload(opts: ApplyDiffPayloadOpts): ApplyJsonPayload { } if (!actual.includes(before)) { conflicts.push({ - file_path: filePath, + file_path: canonicalPath, line_start: lineStart, before_pattern: before, actual_at_line: actual, @@ -193,10 +208,10 @@ export function applyDiffPayload(opts: ApplyDiffPayloadOpts): ApplyJsonPayload { // Reject the second-and-subsequent rows targeting one line — see // `seenLines` declaration for the Q2 (c) violation this guards. - const seen = seenLines.get(filePath); + const seen = seenLines.get(canonicalPath); if (seen !== undefined && seen.has(lineStart)) { conflicts.push({ - file_path: filePath, + file_path: canonicalPath, line_start: lineStart, before_pattern: before, actual_at_line: actual, @@ -205,14 +220,14 @@ export function applyDiffPayload(opts: ApplyDiffPayloadOpts): ApplyJsonPayload { continue; } if (seen === undefined) { - seenLines.set(filePath, new Set([lineStart])); + seenLines.set(canonicalPath, new Set([lineStart])); } else { seen.add(lineStart); } - const edits = pending.get(filePath); + const edits = pending.get(canonicalPath); if (edits === undefined) { - pending.set(filePath, [ + pending.set(canonicalPath, [ { line_start: lineStart, before_pattern: before, after_pattern: after }, ]); } else { @@ -228,7 +243,14 @@ export function applyDiffPayload(opts: ApplyDiffPayloadOpts): ApplyJsonPayload { const distinctInputFiles = new Set(); for (const row of rows) { const filePath = readString(row, "file_path"); - if (filePath !== undefined) distinctInputFiles.add(filePath); + if (filePath === undefined) continue; + // Count distinct disk targets, not distinct spellings — same as the + // dedup applied to the cache + pending keys. + if (isAbsolute(filePath) || !isWithinRoot(resolvedRoot, filePath)) { + distinctInputFiles.add(filePath); + } else { + distinctInputFiles.add(canonicalizeFilePath(resolvedRoot, filePath)); + } } // Q2 (c) — any conflict aborts the run; dry-run never writes. Same Q5 envelope. @@ -318,6 +340,15 @@ function readString( return typeof value === "string" && value.length > 0 ? value : undefined; } +/** Like {@link readString} but admits `""` — used for `after_pattern` (deletion). */ +function readStringAllowEmpty( + row: Record, + key: string, +): string | undefined { + const value = row[key]; + return typeof value === "string" ? value : undefined; +} + function readPositiveInt( row: Record, key: string, @@ -335,3 +366,16 @@ function isWithinRoot(resolvedRoot: string, candidate: string): boolean { const prefix = resolvedRoot.endsWith(sep) ? resolvedRoot : resolvedRoot + sep; return resolved.startsWith(prefix); } + +/** + * Canonical project-relative form for the `pending` / `sourceCache` / + * `seenLines` keys. `a.ts`, `./a.ts`, `src/../a.ts` all collapse to `a.ts`. + * Caller has already verified `isWithinRoot(resolvedRoot, candidate)` so + * the result is guaranteed in-tree. + */ +function canonicalizeFilePath(resolvedRoot: string, candidate: string): string { + const absolute = resolve(resolvedRoot, candidate); + if (absolute === resolvedRoot) return ""; + const prefix = resolvedRoot.endsWith(sep) ? resolvedRoot : resolvedRoot + sep; + return absolute.slice(prefix.length); +} diff --git a/src/cli/cmd-apply.ts b/src/cli/cmd-apply.ts index be03cab..968f261 100644 --- a/src/cli/cmd-apply.ts +++ b/src/cli/cmd-apply.ts @@ -193,10 +193,14 @@ export async function runApplyCmd(opts: ApplyOpts): Promise { } const projectRoot = getProjectRoot(); - const isTTY = process.stdout.isTTY === true; + // Gate on the streams the prompt actually uses (stdin for input, + // stderr for output) — `process.stdout` may be a pipe even in + // interactive sessions (`codemap apply foo | tee log.txt`). + const canPrompt = + process.stdin.isTTY === true && process.stderr.isTTY === true; // Q6 (a): non-TTY without --yes / --dry-run is rejected. - if (!isTTY && !opts.yes && !opts.dryRun) { + if (!canPrompt && !opts.yes && !opts.dryRun) { emitError( `codemap apply: this verb writes files. Pass --yes for non-interactive runs, or --dry-run for preview.`, opts.json, @@ -230,8 +234,17 @@ export async function runApplyCmd(opts: ApplyOpts): Promise { printPromptSummary(preview, opts.recipeId, rows); const proceed = await promptYesNo(); if (!proceed) { - console.error("apply: aborted by user."); - emitResult(preview, opts); + // Don't fall through to `emitResult` in terminal mode — the dry-run + // envelope's `applied: false` would render as "no rows applicable", + // which contradicts the user's explicit cancellation. JSON consumers + // still get the full preview envelope. + if (opts.json) { + emitResult(preview, opts); + } else { + console.log( + `apply ${opts.recipeId}: aborted by user; no files written.`, + ); + } return; } diff --git a/templates/agents/skills/codemap/SKILL.md b/templates/agents/skills/codemap/SKILL.md index 33bd306..5d0087d 100644 --- a/templates/agents/skills/codemap/SKILL.md +++ b/templates/agents/skills/codemap/SKILL.md @@ -79,7 +79,7 @@ Each emitted delta carries its own `base` metadata so mixed-baseline audits are - **`show`** — `{name, kind?, in?}`. Exact, case-sensitive symbol name lookup. Returns `{matches: [{name, kind, file_path, line_start, line_end, signature, ...}], disambiguation?: {n, by_kind, files, hint}}`. Single match → `{matches: [{...}]}`; multi-match adds the disambiguation envelope so you narrow without re-scanning. Fuzzy lookup belongs in `query` with `LIKE`. - **`snippet`** — `{name, kind?, in?}`. Same lookup as `show` but each match also carries `source` (file lines from disk at `line_start..line_end`), `stale` (true when content_hash drifted since indexing — line range may have shifted), `missing` (true when file is gone). `source` is always returned when the file exists; agent decides whether to act on stale content or run `codemap` / `codemap --files ` to re-index first. No auto-reindex side-effects from this read tool. - **`impact`** — `{target, direction?, via?, depth?, limit?, summary?}`. Symbol/file blast-radius walker — replaces hand-composed `WITH RECURSIVE` queries that agents struggle to write reliably. `target` is a symbol name (case-sensitive, exact) OR a project-relative file path (auto-detected by `/` or by matching `files.path`). `direction`: `up` (callers / dependents), `down` (callees / dependencies), `both` (default). `via`: `dependencies`, `calls`, `imports`, `all` (default — every backend compatible with the resolved target kind: symbol → `calls`; file → `dependencies` + `imports`; mismatched explicit choices land in `skipped_backends`, no error). `depth` default 3, `0` = unbounded (still cycle-detected and limit-capped). `limit` default 500. `summary: true` trims `matches` for cheap CI-gate consumption (`jq '.summary.nodes'`) but preserves the count. Result: `{target, direction, via, depth_limit, matches: [{depth, direction, edge, kind, name?, file_path}], summary: {nodes, max_depth_reached, by_kind, terminated_by: 'depth'|'limit'|'exhausted'}}`. Cycle detection is approximate-but-bounded — bounded depth + `LIMIT` keep cyclic graphs cheap; `terminated_by` reports the dominant stop reason. SARIF / annotations not supported (impact rows are graph traversals, not findings). -- **`apply`** — `{recipe, params?, dry_run?, yes?}`. Substrate-shaped fix executor — runs the same recipe `query_recipe` runs, then applies the diff hunks each row describes (`{file_path, line_start, before_pattern, after_pattern}`) to disk. Recipe SQL is the synthesis surface; codemap is the executor. Phase 1 validates every row against current disk via substring-match (`actual.includes(before_pattern)`) — the same contract `--format diff-json` uses. Phase 2 (gated on no conflicts) writes via sibling temp + `rename` for POSIX-atomic per-file writes. **Q2 (c) all-or-nothing** — any conflict in any file aborts phase 2 entirely; partial writes never ship. Over MCP/HTTP `yes: true` is required for the write path (no TTY prompt to fall back on); `dry_run: true` previews without writing; the two are mutually exclusive. Re-running on already-applied code reports a `line content drifted` conflict whose `actual_at_line` shows the post-rename content — re-run `codemap` to refresh the index, then re-run `apply` for a clean vacuous pass. Result envelope (identical across modes): `{mode: 'dry-run'|'apply', applied, files: [{file_path, rows_applied, warnings?}], conflicts: [{file_path, line_start, before_pattern, actual_at_line, reason}], summary: {files, files_modified, rows, rows_applied, conflicts, files_with_conflicts}}`. Floor "No fix engine" preserved — codemap doesn't synthesise edits, it only executes the hunks the recipe row described. +- **`apply`** — `{recipe, params?, dry_run?, yes?}`. Substrate-shaped fix executor — runs the same recipe `query_recipe` runs, then applies the diff hunks each row describes (`{file_path, line_start, before_pattern, after_pattern}`) to disk. Recipe SQL is the synthesis surface; codemap is the executor. Phase 1 validates every row against current disk via substring-match (`actual.includes(before_pattern)`) — the same contract `--format diff-json` uses; collects five conflict reasons (`file missing` / `line out of range` / `line content drifted` / `path escapes project root` / `duplicate edit on same line`). Phase 2 (gated on no conflicts) writes via sibling temp + `rename` for POSIX-atomic per-file writes. **Q2 (c) all-or-nothing** — any conflict in any file aborts phase 2 entirely; partial writes never ship. Over MCP/HTTP `yes: true` is required for the write path (no TTY prompt to fall back on); `dry_run: true` previews without writing; the two are mutually exclusive. Re-running on already-applied code reports a `line content drifted` conflict whose `actual_at_line` shows the post-rename content — re-run `codemap` to refresh the index, then re-run `apply` for a clean vacuous pass. Result envelope (identical across modes): `{mode: 'dry-run'|'apply', applied, files: [{file_path, rows_applied, warnings?}], conflicts: [{file_path, line_start, before_pattern, actual_at_line, reason}], summary: {files, files_modified, rows, rows_applied, conflicts, files_with_conflicts}}`. Floor "No fix engine" preserved — codemap doesn't synthesise edits, it only executes the hunks the recipe row described. **Resources (mostly lazy-cached on first `read_resource`; recipes / one-recipe live-read every call so the inline recency fields stay fresh):** From b9448bdf4c7939db0b4f8553822ea2f870c18c31 Mon Sep 17 00:00:00 2001 From: Sutu Sebastian Date: Wed, 6 May 2026 16:32:00 +0300 Subject: [PATCH 9/9] refactor(mergeParams): simplify parameter merging logic --- src/application/recipe-params.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/application/recipe-params.ts b/src/application/recipe-params.ts index 915ba58..3716890 100644 --- a/src/application/recipe-params.ts +++ b/src/application/recipe-params.ts @@ -48,7 +48,7 @@ export function mergeParams( base: RecipeParamValues | undefined, next: RecipeParamValues, ): RecipeParamValues { - return { ...(base ?? {}), ...next }; + return { ...base, ...next }; } /**