Commit 84f9b97
authored
feat(apply): codemap apply <recipe-id> — Q1–Q10 implemented (#78)
* feat(apply): slice 1 — apply-engine phase-1 validation + dry-run
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.
* feat(apply): slice 2 — phase 2 writes via temp + rename
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).
* feat(apply): slice 3 — CLI verb + recipe execution + TTY/--yes gate
Adds `codemap apply <recipe-id>` 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.
* feat(apply): slice 4 — MCP/HTTP `apply` tool
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).
* docs(apply): slice 5 — lockstep + plan retire
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 <recipe-id>`)"
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.
* fix(apply): path-containment + overlap detection (triangulated review)
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<line_start>; 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.
* chore(apply): slim comments + sync docs to five conflict reasons
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.
* fix(apply): address CodeRabbit review (7 of 9; 2 already fixed)
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 <id>: 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.
* refactor(mergeParams): simplify parameter merging logic1 parent bfc0b8a commit 84f9b97
20 files changed
Lines changed: 2116 additions & 401 deletions
File tree
- .agents
- rules
- skills/codemap
- .changeset
- docs
- plans
- src
- application
- cli
- templates/agents
- rules
- skills/codemap
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
37 | 37 | | |
38 | 38 | | |
39 | 39 | | |
| 40 | + | |
40 | 41 | | |
41 | 42 | | |
42 | 43 | | |
| |||
75 | 76 | | |
76 | 77 | | |
77 | 78 | | |
| 79 | + | |
| 80 | + | |
78 | 81 | | |
79 | 82 | | |
80 | | - | |
| 83 | + | |
81 | 84 | | |
82 | 85 | | |
83 | 86 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
79 | 79 | | |
80 | 80 | | |
81 | 81 | | |
| 82 | + | |
82 | 83 | | |
83 | 84 | | |
84 | 85 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
0 commit comments