feat(apply): codemap apply <recipe-id> — Q1–Q10 implemented#78
Open
SutuSebastian wants to merge 9 commits intomainfrom
Open
feat(apply): codemap apply <recipe-id> — Q1–Q10 implemented#78SutuSebastian wants to merge 9 commits intomainfrom
SutuSebastian wants to merge 9 commits intomainfrom
Conversation
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.
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).
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.
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).
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.
🦋 Changeset detectedLatest commit: b9448bd The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughAdds a transport-agnostic "apply" feature: a two-phase apply engine that validates recipe-produced diff rows and, when consented, performs atomic per-file writes. Includes CLI, MCP, and HTTP wiring, tests, docs, and a minor recipe-params merge tweak. ChangesCodemap Apply Feature
Sequence DiagramsequenceDiagram
participant CLI as CLI
participant Handler as ToolHandler
participant Engine as ApplyEngine
participant Disk as DiskIO
CLI->>Handler: parse args and call handleApply(args, root)
Handler->>Handler: gate: require yes or dry_run (non-TTY requires yes)
Handler->>Handler: resolve recipe params and run recipe SQL -> rows
Handler->>Engine: applyDiffPayload({ rows, projectRoot, dryRun })
Engine->>Disk: read files (per-path cache) for validation
Engine->>Engine: Phase 1: validate rows (containment, existence, line ranges, content drift, duplicates)
alt conflicts or dry_run
Engine-->>Handler: return ApplyJsonPayload (applied=false, conflicts, summary)
else no conflicts and apply mode
Engine->>Disk: write per-file to temp file
Engine->>Disk: rename temp -> target (atomic)
Engine-->>Handler: return ApplyJsonPayload (applied=true, files, summary)
end
Handler-->>CLI: tool result (payload)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Contributor
Author
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
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.
Contributor
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.agents/rules/codemap.md:
- Around line 77-80: The "Impact" and "Apply" sections in the codemap doc are
accidentally merged causing a sentence break and stale conflict-reason list;
split the stitched paragraph so the Impact description (starting with "Impact
(`bun src/index.ts impact <target>`)") ends cleanly before the Apply section,
then restore the Apply header and its correct description for "Apply (`bun
src/index.ts apply <recipe-id>`)": update the conflict reasons list to include
the two newer reasons (so Phase 1 reports all current conflict reasons) and
ensure references to applyDiffPayload and findImpact /
application/apply-engine.ts / application/impact-engine.ts remain accurate; keep
the output envelope and behavior notes intact and ensure the two sections are
separated into distinct paragraphs with no crossover text.
In `@docs/glossary.md`:
- Around line 36-37: Update the Phase-1 conflict reasons list in
docs/glossary.md to match the current apply engine: include the two additional
reasons now emitted by application/apply-engine.ts (applyDiffPayload) — namely
"path escapes project root" and "duplicate edit on same line" — alongside the
existing "file missing", "line out of range", and "line content drifted"; ensure
the glossary wording and ordering reflect the engine's exact reason strings and
that any references to the Phase-1 validation behavior (e.g.,
actual.includes(before_pattern) and the Q2 all-or-nothing rule) remain accurate.
In `@src/application/apply-engine.ts`:
- Around line 155-184: The path containment check and cache keys must use a
canonical on-disk path (resolved + realpath + normalized) so symlinks and
lexical duplicates (e.g. "a.ts" vs "./a.ts") can't escape or be double-booked:
replace uses of the raw filePath in isWithinRoot, sourceCache.get/set,
readFileSync(resolve(..., filePath)), and any pending-writes map keys with a
single canonical variable computed via path.resolve + fs.realpathSync (or async
realpath) and path.normalize (e.g., compute canonicalPath =
normalize(realpathSync(resolve(resolvedRoot, filePath)))) and then use
canonicalPath for the containment check, file reads, cache keys, conflict
objects (file_path), and any write/pending maps; apply the same change to the
other similar blocks noted (around the other ranges) so all cache/write lookups
share the same canonical target.
- Around line 283-330: The loop in apply-engine.ts can leave earlier files
permanently changed if writeFileSync/renameSync fails mid-loop; to make the
multi-file apply transactional, create per-file backups and a rollback on error:
before overwriting each absPath, move the original to a backup (e.g., backupPath
= `${absPath}.codemap-backup-${rand}.tmp`) or copy it, then rename the tempPath
into place; push {absPath, backupPath} into a backups list; surround the
write/rename sequence in a try/catch that on any error iterates the backups in
reverse and restores each backup (rename backupPath back to absPath) and cleans
up temps/backups, then rethrow the error; update references to
writeFileSync/renameSync/tempPath/absPath and the surrounding loop (the code
that builds writtenFiles and appliedRows) so you only commit
writtenFiles/appliedRows after successful completion and ensure all temp/backup
files are removed on success.
- Around line 141-149: The check is incorrectly skipping valid rows because
readString currently treats an explicitly empty after_pattern as missing; update
the code so empty-string after patterns are allowed: either modify readString to
accept an allowEmpty boolean (e.g., readString(row, "after_pattern",
{allowEmpty: true}) and return "" for present-but-empty cells) or add a new
helper (readAllowEmptyString/readNullableString) and use it when reading
after_pattern, then keep the validation only rejecting undefined (not empty
string) for the variables filePath, lineStart, before and allow after === "" as
valid; apply the same change to the other analogous read/validation block that
handles recipe rows.
In `@src/cli/cmd-apply.ts`:
- Around line 240-245: The abort branch currently calls emitResult(preview,
opts) while proceed is false, causing downstream logic to treat it as a real
apply and print "apply <id>: no rows applicable."; update that call to indicate
a dry run so the terminal path treats it as a user-cancelled preview — e.g.,
call emitResult(preview, { ...opts, dryRun: true }) or otherwise set opts.dryRun
= true before calling emitResult in the block where proceed is false (the
variables/functions to change are the proceed check and the emitResult(preview,
opts) invocation).
- Around line 195-205: The gate currently checks process.stdout.isTTY but
promptYesNo() uses process.stdin and process.stderr, so replace the isTTY check
with TTY checks on the exact streams the prompt uses: require
process.stdin.isTTY === true and process.stderr.isTTY === true (or check each
separately as your policy prefers) before allowing interactive prompting; keep
the existing opts.yes and opts.dryRun allowances and ensure promptYesNo
(referenced at promptYesNo) will only be reachable when stdin/stderr are TTYs so
it won't attempt to prompt on non-TTY input.
In `@templates/agents/rules/codemap.md`:
- Around line 86-87: Update the Phase 1 conflict-reason list in the Apply
documentation to match the current engine: add "path escapes project root" and
"duplicate edit on same line" to the three existing reasons so the list now
reflects all five conflict reasons used by the apply logic; reference the
runtime behavior implemented by application/apply-engine.ts and the public apply
entry applyDiffPayload so readers can trace the source of these conflict
reasons.
In `@templates/agents/skills/codemap/SKILL.md`:
- Line 82: The Phase 1 conflict list in the SKILL.md description for the apply
tool is missing two conflict reasons; update the prose under the "Phase 1
validates every row" paragraph (the description of the apply executor / `apply`
signature) to include the additional conflict reasons `path escapes project
root` and `duplicate edit on same line` so the documented Phase-1 validation
list matches the runtime engine contract and the `conflicts` entries described
in the result envelope.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9a27c1ad-fa9d-4e83-b52f-7937c2da1487
📒 Files selected for processing (19)
.agents/rules/codemap.md.agents/skills/codemap/SKILL.md.changeset/codemap-apply.mddocs/architecture.mddocs/glossary.mddocs/plans/codemap-apply.mddocs/roadmap.mdsrc/application/apply-engine.test.tssrc/application/apply-engine.tssrc/application/http-server.tssrc/application/mcp-server.tssrc/application/tool-handlers.test.tssrc/application/tool-handlers.tssrc/cli/bootstrap.tssrc/cli/cmd-apply.test.tssrc/cli/cmd-apply.tssrc/cli/main.tstemplates/agents/rules/codemap.mdtemplates/agents/skills/codemap/SKILL.md
💤 Files with no reviewable changes (2)
- docs/plans/codemap-apply.md
- docs/roadmap.md
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.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/glossary.md`:
- Around line 36-37: Update the Q6 gate wording so the non-TTY `--yes` / `yes:
true` requirement only applies when performing a write (i.e., when `!dryRun` /
`dry_run: false`) rather than to all non-TTY runs; explicitly state that non-TTY
dry-run is allowed without `--yes`; use MCP-style flag names `yes` and `dry_run`
in the transport/HTTP examples and ensure the CLI example shows `--dry-run`
mapping to `dry_run`. Locate and edit the paragraph describing the TTY/`--yes`
behavior (the Q6 gate sentence around the `Proceed? [y/N]` prompt) and any
transport/HTTP examples in the same glossary block so they reflect the scoped
requirement.
In `@src/application/apply-engine.test.ts`:
- Around line 530-531: The test currently writes to a fixed shared path by using
join(root, "..", "outside.ts") which can collide across runs; change the test so
outsidePath is unique per run (e.g. derive it from tmpdir or mkdtemp/mkdirSync
or append a random/pid/timestamp/uuid) before calling writeFileSync, and clean
up after the test; update the code that constructs outsidePath (the join(root,
"..", ... ) invocation) and keep using writeFileSync/outsidePath as before.
In `@src/cli/cmd-apply.ts`:
- Around line 110-118: The "--params" branch currently only checks for undefined
and accepts the next token even if it's another flag (e.g., "--params
--dry-run"); update the check in the handler that inspects variable a for
"--params" so that after reading const next = rest[i + 1] you also reject any
next that looks like a flag (e.g., startsWith("-")) and return the same error
shape (kind: "error", message: `codemap apply: "--params" requires a value
(k=v[,k=v]).`) instead of calling mergeParams/parseParamsCli; keep using
mergeParams and parseParamsCli when next is valid so the rest of the logic is
unchanged.
In `@templates/agents/rules/codemap.md`:
- Around line 86-87: Update the Q6 gating sentence to explicitly state that
non-TTY runs require --yes (or yes: true) except when --dry-run is used; mention
that --dry-run + --yes remain mutually exclusive and that CLI/agents/HTTP/CI
behavior follows the same applyDiffPayload gating (TTY prompts on stderr with
default N). Locate the Q6 paragraph referencing TTY prompts and the non-TTY
requirement and reword it to: non-TTY requires explicit --yes unless running
--dry-run, which is allowed without --yes, and preserve the mutual-exclusion
rule and consistent terminology for --dry-run/--yes across the document.
In `@templates/agents/skills/codemap/SKILL.md`:
- Line 82: Update the SKILL.md description of the apply action (symbol: apply)
to remove the absolute claim “partial writes never ship” and instead state that
phase-1 (conflict detection via substring-match /
actual.includes(before_pattern)) gates whether phase-2 runs, but phase-2
per-file writes (sibling temp + rename) are not cross-file transactional — I/O
failures during phase-2 may leave some files written and others not; keep the
all-or-nothing semantics as “abort phase-2 entirely if any conflicts detected in
phase-1” and explicitly note the caveat that phase-2 is best-effort per-file
atomic via rename but does not provide a multi-file atomic transaction; preserve
descriptions of yes/dry_run semantics and re-run behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1de750c0-3942-4e8c-83f8-273beb681c12
📒 Files selected for processing (12)
.agents/rules/codemap.md.agents/skills/codemap/SKILL.mddocs/architecture.mddocs/glossary.mdsrc/application/apply-engine.test.tssrc/application/apply-engine.tssrc/application/recipe-params.tssrc/application/tool-handlers.tssrc/cli/cmd-apply.test.tssrc/cli/cmd-apply.tstemplates/agents/rules/codemap.mdtemplates/agents/skills/codemap/SKILL.md
Comment on lines
+36
to
+37
| 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 <recipe-id> [--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. | ||
|
|
There was a problem hiding this comment.
Narrow the non-TTY yes requirement to write mode only.
Line 36 currently implies non-TTY always needs --yes / yes: true; that should be scoped to non-dry-run apply. Non-TTY dry-run is allowed and should be stated explicitly to keep docs accurate.
Based on learnings: “For non-TTY transports, require yes: true unconditionally since there is no interactive prompt fallback” (for write path), and use MCP-style key naming (yes, dry_run) in transport docs.
🧰 Tools
🪛 LanguageTool
[grammar] ~36-~36: Ensure spelling is correct
Context: ...patternperString.prototype.replace` GetSubstitution rule. Q2 (c) all-or-nothing — any c...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/glossary.md` around lines 36 - 37, Update the Q6 gate wording so the
non-TTY `--yes` / `yes: true` requirement only applies when performing a write
(i.e., when `!dryRun` / `dry_run: false`) rather than to all non-TTY runs;
explicitly state that non-TTY dry-run is allowed without `--yes`; use MCP-style
flag names `yes` and `dry_run` in the transport/HTTP examples and ensure the CLI
example shows `--dry-run` mapping to `dry_run`. Locate and edit the paragraph
describing the TTY/`--yes` behavior (the Q6 gate sentence around the `Proceed?
[y/N]` prompt) and any transport/HTTP examples in the same glossary block so
they reflect the scoped requirement.
Comment on lines
+530
to
+531
| const outsidePath = join(root, "..", "outside.ts"); | ||
| writeFileSync(outsidePath, "const foo = 1;\n", "utf8"); |
There was a problem hiding this comment.
Use a unique out-of-root target in the traversal test.
join(root, "..", "outside.ts") resolves to a shared /tmp/outside.ts, which can race across tests and leak state between runs.
Suggested fix
- const outsidePath = join(root, "..", "outside.ts");
+ const outsideRoot = mkdtempSync(join(tmpdir(), "codemap-apply-outside-"));
+ const outsidePath = join(outsideRoot, "outside.ts");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/application/apply-engine.test.ts` around lines 530 - 531, The test
currently writes to a fixed shared path by using join(root, "..", "outside.ts")
which can collide across runs; change the test so outsidePath is unique per run
(e.g. derive it from tmpdir or mkdtemp/mkdirSync or append a
random/pid/timestamp/uuid) before calling writeFileSync, and clean up after the
test; update the code that constructs outsidePath (the join(root, "..", ... )
invocation) and keep using writeFileSync/outsidePath as before.
Comment on lines
+110
to
+118
| 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)); |
There was a problem hiding this comment.
Reject flag tokens as --params values.
Line 112 only checks undefined; --params --dry-run is currently parsed as params input instead of a direct CLI error.
Suggested fix
- if (next === undefined) {
+ if (next === undefined || next.startsWith("-")) {
return {
kind: "error",
message: `codemap apply: "--params" requires a value (k=v[,k=v]).`,
};
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cli/cmd-apply.ts` around lines 110 - 118, The "--params" branch currently
only checks for undefined and accepts the next token even if it's another flag
(e.g., "--params --dry-run"); update the check in the handler that inspects
variable a for "--params" so that after reading const next = rest[i + 1] you
also reject any next that looks like a flag (e.g., startsWith("-")) and return
the same error shape (kind: "error", message: `codemap apply: "--params"
requires a value (k=v[,k=v]).`) instead of calling mergeParams/parseParamsCli;
keep using mergeParams and parseParamsCli when next is valid so the rest of the
logic is unchanged.
Comment on lines
+86
to
+87
| **Apply (`codemap apply <recipe-id>`)**: 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. | ||
|
|
There was a problem hiding this comment.
Clarify non-TTY gating to include the dry-run exception.
Line 86 currently reads as if non-TTY always requires --yes, but Line 45 (and runtime behavior) allows non-TTY with --dry-run and no --yes. Please make this sentence explicit to avoid conflicting guidance.
As per coding guidelines: “Documentation files must be kept up-to-date with code changes” and “Use consistent terminology across all documentation.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@templates/agents/rules/codemap.md` around lines 86 - 87, Update the Q6 gating
sentence to explicitly state that non-TTY runs require --yes (or yes: true)
except when --dry-run is used; mention that --dry-run + --yes remain mutually
exclusive and that CLI/agents/HTTP/CI behavior follows the same applyDiffPayload
gating (TTY prompts on stderr with default N). Locate the Q6 paragraph
referencing TTY prompts and the non-TTY requirement and reword it to: non-TTY
requires explicit --yes unless running --dry-run, which is allowed without
--yes, and preserve the mutual-exclusion rule and consistent terminology for
--dry-run/--yes across the document.
| - **`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 <path>` 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; 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. |
There was a problem hiding this comment.
Apply docs overstate cross-file transactional safety.
Line 82 currently says “partial writes never ship,” but phase-2 I/O failures are not cross-file transactional. Please scope the guarantee to phase-1 conflict gating and explicitly note the phase-2 caveat.
Suggested wording
- **Q2 (c) all-or-nothing** — any conflict in any file aborts phase 2 entirely; partial writes never ship.
+ **Q2 (c) all-or-nothing (phase-1 semantic gate)** — any conflict in any file aborts phase 2 entirely before any file is touched.
+ If phase 2 starts, writes are per-file atomic (`temp` + `rename`), but cross-file rollback is not guaranteed on I/O failure.As per coding guidelines: “Documentation files must be kept up-to-date with code changes.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - **`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. | |
| - **`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 (phase-1 semantic gate)** — any conflict in any file aborts phase 2 entirely before any file is touched. If phase 2 starts, writes are per-file atomic (`temp` + `rename`), but cross-file rollback is not guaranteed on I/O failure. 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. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@templates/agents/skills/codemap/SKILL.md` at line 82, Update the SKILL.md
description of the apply action (symbol: apply) to remove the absolute claim
“partial writes never ship” and instead state that phase-1 (conflict detection
via substring-match / actual.includes(before_pattern)) gates whether phase-2
runs, but phase-2 per-file writes (sibling temp + rename) are not cross-file
transactional — I/O failures during phase-2 may leave some files written and
others not; keep the all-or-nothing semantics as “abort phase-2 entirely if any
conflicts detected in phase-1” and explicitly note the caveat that phase-2 is
best-effort per-file atomic via rename but does not provide a multi-file atomic
transaction; preserve descriptions of yes/dry_run semantics and re-run behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements the
codemap applyplan (PR #77, merged) across 5 tracer-bullet slices. Each commit is a self-contained vertical slice that ran format/lint/typecheck/tests via the pre-commit hook before landing.codemap apply <recipe-id>is a substrate-shaped fix executor: 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.Commit-by-commit
e0677b9apply-engine.tsphase-1 validation + dry-run envelope. Re-locks Q8 to substring-match (the original "exact byte-match" draft contradicted the existingbuildDiffJsonformatter contract —rename-previewemitsbefore_pattern = old_nameas the bare identifier). 14 unit tests.564ba32renameSyncfor POSIX-atomic per-file writes. CRLF round-trip via raw\nsplit. 6 more unit tests includingchmod 0o555failure mode.a24f715cmd-apply.tsCLI verb (positional, per Q4) + recipe execution + Q6 TTY/--yesgate matrix. 10 subprocess integration tests including Q7 (a) idempotent re-run after reindex.1fd85ceapplytool registration. SameapplyDiffPayloadengine;yes: truerequired over non-TTY transports. 4 handler unit tests.30080f6docs/architecture.md § Apply wiring+ boundary SQL kit;glossary.mdentry; agent rule + skill + templates updated; roadmap backlog entry removed; plan file deleted per Rule 3.Q1–Q10 locks (mirroring the merged plan)
--dry-runopts into previewcodemap apply <recipe-id>)ApplyJsonPayload, single shape across modes--yes; non-TTY rejects without--yesbuildDiffJsonverbatimTest plan
apply-engine.test.ts,tool-handlers.test.ts,cmd-apply.test.ts(140 total in the touched suites; 0 fail).docs/architecture.md § Boundary verification — apply write pathreturns[]after a fresh reindex.codemap apply rename-preview --params old=foo,new=bar --yes --jsonagainst a fresh fixture project renames the definition + import specifiers and reportsapplied: truewith correctsummary.rows_applied.Cross-references
src/application/apply-engine.ts.src/cli/cmd-apply.ts.src/application/tool-handlers.tshandleApply.docs/architecture.md § Apply wiring+§ Boundary verification — apply write path.docs/glossary.mdcodemap apply/ apply tool entry..changeset/codemap-apply.md(minor bump).Summary by CodeRabbit
New Features
Documentation
Tests
Chore