fix(diff): emit the unresolved_target slug, not the sentence - #566
Merged
Conversation
`docs/llm-format.md:29` says a target that does not exist is always `unresolved_target`, whichever command was asked. #538 unified everything except `diff.ts`, which nothing has touched since. Its `--format json` branch emitted: {"error": "No entity found matching \"decide.ts\"."} with the human sentence in the `error` field -- where every other command puts the machine slug -- and no `message` field at all. A consumer routing on `error === "unresolved_target"` gets an English sentence instead, and one routing on `message` gets nothing. There are two such sites, not one: the symbol path (`resolveEntityFull`) and the file-like path (`resolveFileOrEntity`) each carry their own copy. Both were wrong. The `--format llm` branch already emitted the right slug but with the older `No entity resolved for "..."` wording, which is now the shared text -- that string is gone from `diff.ts`, and the eight remaining copies belong to the commands #547 converts, deliberately left for that PR to avoid a conflict. `unresolvedTargetMessage` and `unresolvedTargetRecord` split out of `reportUnresolvedTarget` so a command can emit the shared record *without* the exit code, which is the whole point here. ## The exit code is deliberately not part of this Making `ix diff` exit non-zero is a breaking change for the plugins (CONTRIBUTING -> CLI Standards -> Exit codes). `ix-cursor-plugin`'s `ix_diff` tool routes on the exit status at `mcp/tools/map.ts:206`, and today it reads `raw.summary`/`raw.total` off the error object and reports a successful "0 changes" for a target that does not exist. Only the exit code fixes that, and it belongs in #547's queue behind the five plugin PRs. A test pins the current exit behaviour so nobody adds it without doing that work. Four tests, each mutation-checked against the code it covers: reverting the json shape fails two, reverting the llm wording fails the third, and adding the exit code fails the fourth. ix-cli: 1509 passed / 2 skipped. typecheck, eslint, knip clean. Note `src/cli/__tests__/watch-dedup.test.ts` ("starts both the tsx source CLI and a normal built CLI child") fails when run in isolation on unmodified main -- 4 of 4 attempts at 793c56b with this change stashed. Pre-existing and unrelated; it passes in the full suite.
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
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.
docs/llm-format.md:29says a target that does not exist is alwaysunresolved_target, whichever command was asked. #538 unified everything exceptdiff.ts, and nothing has touched it since.Found while reviewing #547: twelve commands resolve a target, #547 converts ten, #559 takes
locate, anddiffis the twelfth that nobody has.What was wrong
ix diff --format jsonon a target that does not exist emitted:{"error": "No entity found matching \"decide.ts\"."}The human sentence sits in the
errorfield — where every other command puts the machine slug — and there is nomessagefield at all. A consumer routing onerror === "unresolved_target"gets an English sentence; one readingmessagegets nothing.There are two such sites, not one. The symbol path (
resolveEntityFull,diff.ts:514) and the file-like path (resolveFileOrEntity,diff.ts:520) each carry their own copy, soix diff 3 5 Fooandix diff 3 5 Foo.tswere separately broken. Both are fixed and separately tested.The
--format llmbranch already emitted the right slug, but with the olderNo entity resolved for "…"wording. That is now the shared text, so the string is gone fromdiff.ts. The eight remaining copies belong to the commands #547 converts and are deliberately left alone to avoid conflicting with that PR.unresolvedTargetMessageandunresolvedTargetRecordare split out ofreportUnresolvedTargetso a command can emit the shared record without the exit code — which is the whole point of this PR being separable.The exit code is deliberately not in this PR
Making
ix diffexit non-zero is a breaking change for the plugins (CONTRIBUTING.md→ CLI Standards → Exit codes).ix-cursor-plugin'six_difftool routes on the exit status atmcp/tools/map.ts:206; today it readsraw.summary/raw.totaloff the error object and reports a successful "0 changes" for a target that does not exist. Only the exit code fixes that, and it belongs in #547's queue behind the five plugin PRs.A test pins the current exit behaviour so nobody adds it without doing that work.
Tests
Four, each mutation-checked against the code it covers:
errorshapeprocess.exitCode = 1Verification
typecheck, eslint and knip clean; ix-cli 1509 passed / 2 skipped.
One thing worth flagging separately:
src/cli/__tests__/watch-dedup.test.ts("starts both the tsx source CLI and a normal built CLI child") fails when run in isolation on unmodifiedmain— 4 of 4 attempts at793c56bwith this change stashed. It passes in the full suite, so it is order- or build-dependent rather than broken. Pre-existing and unrelated to this PR, but it should probably get an issue.