fix(intent): report unresolved plan steps instead of injecting placeholder comments - #66
Merged
Merged
Conversation
…older comments
`add-parameter` without a `param.default` has no argument to pass at the call
sites. The planner used to fabricate one: a literal `/* TODO: add x */` string
inserted at every reference. That is not a comment in Python, so a plan the
CLI reported as successful wrote a syntax error into the user's source. It is
also wrong in Go and Rust in a subtler way — it compiles as a comment and
silently drops the argument.
The fix removes placeholders entirely rather than making them language-aware.
A placeholder in a plan step is the planner admitting it could not compute the
edit, so it now surfaces where the caller can act on it:
- `EditPlan.unresolved: UnresolvedItem[]` — `{file, operation, reason, resolution}`
- `impactSummary` names the unresolved count instead of hiding it
- `executePlan` refuses a partial plan up front (`UNSUPPORTED_OPERATION`,
exit 1) rather than applying the signature edit and leaving the call sites
broken; `--yes` applies only the computable steps
- the human output prints each reason with the concrete next step
Deliberately did not add a per-language `commentSyntax` table: with
placeholders gone there is no consumer for it, so it would ship as dead code.
Measured before/after on a real Python pair: the old code exited 2 with
`Edit was discarded: the result does not parse` — the #13 parse gate was the
only thing keeping the placeholder off disk, and the intent failed opaquely
after a partial apply. New code exits 1 with the unresolved list, both files
byte-identical, `py_compile` clean.
Tests: `tests/intent-unresolved.test.ts` — 16 tests across .ts/.py/.go/.rs
covering the blocked plan, the `--yes` path, the resolvable-with-default case,
and a grep regression guard so the string cannot come back. 553 pass / 0 fail.
Closes #16
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Owner
Author
|
🎉 This PR is included in version 3.0.3 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This was referenced Aug 17, 2026
bigknoxy
added a commit
that referenced
this pull request
Aug 17, 2026
* fix: normalize paths before deduping intent plan steps (#41) The intent planner deduped reference files by raw string, so the same file reached as "src/a.ts", "./src/a.ts", and "/abs/proj/src/a.ts" produced one plan step per spelling, and a reference to the definition file spelled differently from the definition escaped the `!== definition.file` filter and got renamed a second time. Adds `src/core/path-normalize.ts` with `normalizePath` and `pathsEqual`, and routes the two `refFiles` computations in `generatePlan` through them. Rebuilt on current main rather than merged. The original branch introduced these helpers as `src/core/paths.ts`, which is now the write-boundary module (assertWritable, safeWrite, atomicWrite, findProjectRoot, PathDeniedError and the unoverridable deny-list) imported by seven modules; landing it as authored would have deleted every write guard in the codebase. The helpers live in a separate module so the two concerns cannot collide again. Also dropped from the original branch: - The remove-parameter dedupe test. Main refuses remove-parameter outright (UnsupportedIntentError, #66), so the test could not run. - An unused `relative` import, unused `beforeEach`/`afterEach` imports, and an unused `cwd` local. - The undefined-only guard in `pathsEqual`, which made the relation inconsistent: (null, undefined) compared false while (null, null) and (null, "") compared true. `normalizePath` already collapses every nullish and blank form to "", so the guard was both redundant and wrong. Adds coverage for the previously untested branches: cwd itself, paths outside cwd staying absolute, a sibling directory sharing a cwd prefix not being mistaken for a child, and the full nullish/blank equality matrix. Verified by mutation: reverting either normalization call site fails the two new intent tests. Full suite 614 pass / 0 fail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: document path-normalize module in ARCHITECTURE New core module from #41 was missing from the architecture doc, which the Docs Verify gate caught. Records the cwd-relative constraint that makes it unsuitable as a lock key, alongside the locking.ts section that depends on the opposite property. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Joshua Knox <Joshua.Knox@Joshuas-MacBook-Pro.local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
bigknoxy
pushed a commit
that referenced
this pull request
Aug 17, 2026
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.
Closes #16.
The bug
add-parameterwithout aparam.defaulthas no argument to pass at the call sites. The planner fabricated one — a literal/* TODO: add x */inserted at every reference. That is not a comment in Python, so a plan reported as successful wrote a syntax error to disk. In Go and Rust it is a comment, which is worse in a quieter way: it compiles, and the argument is silently dropped.Which option
The issue offered two. This PR takes option 2 — remove the placeholders entirely rather than adding a per-language
commentSyntaxtable.A placeholder in a plan step is the planner admitting it could not compute the edit. That belongs in the plan summary, not in the user's source, in any language. With placeholders gone there is no consumer for a
commentSyntaxtable, so adding it would ship dead code; it was deliberately left out.What changed
EditPlan.unresolved: UnresolvedItem[]—{file, operation, reason, resolution}, each carrying the concrete next stepimpactSummarynames the unresolved count instead of hiding itexecutePlanrefuses a partial plan up front withUNSUPPORTED_OPERATION(exit 1) rather than applying the signature edit and leaving every call site broken--yesoverrides: applies only the computable steps, call sites untouched and still listedreasonand itsresolutionUNSUPPORTED_OPERATIONis reused rather than a new code minted — the envelope schema's error enum is closed, and it already maps to exit 1 (usage), which is the right signal: supply a default or pass--yes.Measured
Real Python pair (
greeter.py+app.py), before vs after:Edit was discarded: the result does not parse (syntax error at line 5:25)py_compileThe #13 parse gate was the only thing keeping the placeholder off disk; the planner still advertised a step it could not perform and failed the intent opaquely after a partial apply. With
"default":"False"the same intent now succeeds:2 edits across 2 files,def greet(name, loud):/print(greet("world", False)),py_compileclean.Tests
tests/intent-unresolved.test.ts— 16 tests, written before the fix (red first). Per language (.ts/.py/.go/.rs): noinsert-call-argsteps emitted, blocked plan writes nothing and both files still parse,--yeswrites noTODOand leaves parseable output. Plus: a default makes call sites resolvable again, the impact summary names the gap,rename-exported-symbolis fully computable, and agrep -rn "TODO: add" srcregression guard so the string cannot come back.553 pass / 0 fail across 22 files.
Docs synced: README,
docs/ADAPTER-CONTRACT.md(newplan.unresolvedshape +--yes),docs/ARCHITECTURE.md(+ footer),docs/CLI-QUICKREF.md(regenerated),ROADMAP.md,CLAUDE.md/AGENTS.md.🤖 Generated with Claude Code