From b361644f693a7fef00b77bccf0a11d9193d6ec71 Mon Sep 17 00:00:00 2001 From: Juha Litola Date: Mon, 17 Aug 2026 14:36:15 +0300 Subject: [PATCH 01/19] docs: replan CodeDiff silent CLI dogfood Split the merged public CLI/MCP phase into a CLI-only dogfood launch and a later agent rollout. Pin the Git-like command contract, validation boundaries, and release impact for the next increment. --- docs/plans/code-diff-cli-mcp.md | 1065 +++++++++++++++---------------- 1 file changed, 531 insertions(+), 534 deletions(-) diff --git a/docs/plans/code-diff-cli-mcp.md b/docs/plans/code-diff-cli-mcp.md index f84ef9f..b9cc10d 100644 --- a/docs/plans/code-diff-cli-mcp.md +++ b/docs/plans/code-diff-cli-mcp.md @@ -1,597 +1,594 @@ -# Plan: Raw code diff CLI and MCP surface +# Plan: CodeDiff CLI dogfood and agent rollout -> Overall status: Phase 1 complete; Phase 2 ergonomics require focused design -> against the accepted Git-like direction and the narrower backend glob -> contract. +> Overall status: Phase 1 is merged. Phase 2 is implementation-ready as a +> silent CLI-only dogfood launch. MCP and agent-facing exposure are deferred +> until the CLI contract has been exercised. > -> Reoriented: 2026-08-17 against `githits` `origin/main` at `fa27604` and -> the PkgSeer backend `main` GraphQL schema and implementation. +> Reoriented: 2026-08-17 against `githits` `origin/main` at `77417aa` and +> PkgSeer backend `origin/main` at `c0cf92e` with GraphQL schema hash +> `sha256:28413c4e9b31`. -## Problem and expected outcome +## Problem and overall expected outcome Package changelogs are often missing or too abstract to explain an upgrade. -GitHits should expose PkgSeer's authoritative raw `codeDiff` evidence through: - -- `githits code diff` for humans and shell automation; and -- `code_diff` for agents. - -Both surfaces must compare the exact resolved `from` and `to` trees, return -bounded source-file evidence, distinguish missing content from an empty diff, -and preserve enough exact identity for `code_read` follow-ups. They must not -claim compatibility, safety, or semantic impact that the raw patch does not -prove. - -The completed effort also lets package-changelog responses steer callers to -the stable diff operation through typed data. Structural/symbol diff remains a -separate, unexposed backend capability. - -## Verified current state - -### Repository state - -- The working branch was rebased on 2026-08-17 against `origin/main` at - `fa27604`. -- The CodeDiff client adapter now exists; no CLI command, MCP tool, response - formatter, or changelog action consumer exists in the current GitHits tree. -- `CodeNavigationService` owns source navigation and is the verified service - boundary for the new operation. CLI commands live under `src/commands/code/`; - MCP tools and shared request/response modules live under - `packages/mcp/src/tools/` and `packages/mcp/src/shared/`. +GitHits needs a bounded source diff that compares the exact resolved `from` +and `to` trees and remains truthful about package scope, truncation, omitted +content, and failures. + +The rollout is deliberately staged: + +1. land the transport-neutral CodeDiff client; +2. expose `githits code diff` as a normally registered but unpromoted CLI + dogfood surface; +3. use dogfood evidence to stabilize the contract before adding `code_diff`, + MCP instructions, or agent guidance; and +4. later consume typed changelog steering when PkgSeer publishes that + contract. + +The completed effort gives humans and agents the same authoritative raw diff +facts without claiming compatibility, upgrade safety, semantic impact, rename +identity, or any other conclusion the raw evidence cannot prove. Structural +CodeDiff remains a separate, unexposed backend evaluation surface. + +## Verified current state and evidence + +### GitHits repository + +- Phase 1 merged through PR #287 at `origin/main` `77417aa`. The merged delta + adds `CodeNavigationService.codeDiff`, exact request/result/error types, + mode-minimal GraphQL selections, runtime validation, fixtures, public client + exports, and `docs/implementation/code-diff.md`. +- The declaration-build pipeline failure was fixed before merge. Phase 1's + focused tests, full tests, build, and public-package validation passed. +- No `githits code diff` command, `code_diff` MCP tool, response formatter, + MCP instruction, Agent Skill guidance, plugin asset, or changelog action + consumer exists on `origin/main`. +- `CodeNavigationService` is already available through the CLI container and + its mock factory. Existing indexed `code` commands keep Commander and + terminal adaptation under `src/commands/code/` while reusing pure request, + response, text, and error helpers through the workspace-only + `@githits/mcp/internal` boundary. +- `githits resolve` is the verified silent-rollout precedent: it is a normally + registered, documented CLI command backed by workspace-internal pure + helpers, but it has no MCP tool or Agent Skill promotion. - Root `githits` is version `0.9.2`; public `@githits/mcp` is version `0.9.1`. - User-visible implementation will require one independent changelog fragment - with pending impacts for both artifacts. Version bumps happen during release - preparation, not feature implementation. -- `CodeNavigationService` and `CodeNavigationServiceImpl` are publicly - re-exported through `@githits/mcp/client`, and `CodeNavigationService` is part - of public `McpToolServices`. Phase 1 therefore adds public client API even - though it adds no MCP tool or CLI command. Custom service implementations - must add the required method when adopting that package version. + A new CLI command affects `githits` only unless implementation changes a + public MCP export or behavior. +- This worktree remains at the merged PR head `f263cf8` while `origin/main` + points at the merge commit. Phase 2 implementation must begin from current + `origin/main`; this replan does not itself rewrite branch history. ### Backend contract -PkgSeer backend `main` replaced GitHub Compare with credential-free aigrep -RawDiff and transferred the lasting contract into its repository-internal -CodeDiff implementation guide. - -The committed GraphQL schema and implementation now prove: - -- `codeDiff` resolves package versions or repository refs to full immutable - commit SHAs before raw execution. -- Raw compares the two exact trees directly. Forward, reverse, and diverged - pairs share the same directional contract; identical SHAs perform no raw - subprocess work. -- A broad authoritative inventory is filtered by verified publication scope, - optional `pathPrefix` and `pathGlob`, then relevance-sorted before - `maxFiles`. The old provider-order and 300-file upstream cap blockers are - gone. -- `RawCodeDiffSummary` reports scoped counts before projectability and - `maxFiles`: `filesChanged`, added/deleted/modified, mode/type changes, - `inventoryComplete`, and `unprojectableFiles`. -- `hasMoreFiles` reports additional projectable files after `maxFiles`; it does - not include overlong unprojectable paths. -- Selection controls work. Inventory requires one raw call; selecting stats or - patches performs one additional exact-path call only for retained files. -- Content truth is typed independently through `contentCoverage`, optional - `contentFailure`, and per-file `contentStatus` / - `contentOmissionReason`. Paths also expose `pathEncoding` and - `contentSafety`. -- Backend bounds/defaults are `maxFiles=50` within `1..300` and - `maxPatchBytes=262144` within `1024..2097152`, with a fixed 131072-byte - per-file patch ceiling. Numeric values are clamped by the backend; the client - must reject out-of-range values instead of silently changing agent input. -- `pathPrefix` and `pathGlob` are independently optional, compose by - intersection, reject explicit empty values, and have 1024-byte limits. -- First-inventory failures are GraphQL field errors. Failures after an - authoritative inventory remain data under `contentFailure`, preserving the - inventory and selected file identities. -- Raw file status is only `ADDED`, `DELETED`, or `MODIFIED`. The contract does - not expose rename identity, so the client must not infer or advertise - renames. - -The local backend checkout matches its `origin/main`, but deployment of this -exact schema to the development and production GraphQL endpoints has not been -verified in this reorientation. The schema changelog still labels the -replacement change as pending. The user reported on 2026-08-17 that a backend -GraphQL documentation round is still needed. Phase 1 can use the committed SDL -and implementation contract; if that round changes fields or semantics rather -than descriptions, reorient before implementation continues. - -### Contradictions retired from the old proposal - -| Old proposal statement | Current verified contract | -|---|---| -| GitHub `BASE...HEAD` cannot produce reverse patches | Exact-tree RawDiff supports forward, reverse, and diverged transformations | -| GitHub exposes at most 300 upstream files | Authoritative inventory has no GitHub Compare file window | -| Upstream ordering can consume the file/patch budget | PkgSeer applies publication scope and relevance before caller limits | -| Counts are repository-wide before caller scope | Summary counts are recomputed after publication and caller scope | -| `compareUrl`, direction counts, previous rename path, and patch-omission enums are available | Those fields were removed; use exact resolutions, content coverage/failure, content status, and omission strings | -| Only patch/stat views need consideration | Backend has distinct Inventory, Stats, and Patches execution modes | +PkgSeer `origin/main` now contains the completed GraphQL documentation round. +The SDL and implementation documentation prove: + +- `codeDiff` accepts exactly one complete addressing form: package + `registry/name/fromVersion/toVersion` or public GitHub repository + `repoUrl/fromRef/toRef`. +- Both endpoints resolve to full immutable commit SHAs before raw work. Forward, + reverse, diverged, and identical pairs use the same direct tree-to-tree + contract; identical SHAs perform no raw subprocess work. +- GraphQL selection determines Inventory, Stats, or Patches work. Phase 1 maps + those shapes to `inventory`, `stats`, and `patches` service modes and proves + their selected fields at the wire boundary. +- `pathGlob` and `pathPrefix` are repository-relative even for package scope. + Package publication evidence restricts the inventory but does not rewrite + returned paths or caller filters to package-relative values. +- `pathGlob` supports a bounded subset: `*` and `?` stay within a component and + an exact `**` component spans components. Unsupported shell/pathspec syntax + is rejected. Only one glob is accepted per request. +- Server defaults are `maxFiles=50` and `maxPatchBytes=262144`; each patch also + has a fixed 131072-byte ceiling. The server clamps numeric options, but the + CLI must reject out-of-range user values rather than silently altering them. +- `summary`, `hasMoreFiles`, and `contentCoverage` answer different questions. + `summary` covers the full filtered inventory before projectability and + `maxFiles`; `hasMoreFiles` covers omitted projectable paths; content coverage + covers only returned file evidence. +- Content outcomes distinguish inventory-only, stats, patch, binary, + metadata-only, omitted, and unavailable rows. A failed post-inventory content + phase remains data with an authoritative inventory; a first-inventory or + resolution failure is a typed GraphQL error. +- The documentation round was description/contract clarification plus tests; + it did not replace Phase 1's GraphQL shapes. The later correction at + `1388b42` clarified repository-relative path identity and is consistent with + this plan. + +Deployment of schema hash `sha256:28413c4e9b31` to the development endpoint has +not been authenticated or introspected during this replan. That is an external +readiness check due before Phase 2 is represented as live-validated, not a +blocker for isolated implementation and fixtures. + +### Reorientation contradiction and resolution + +The merged plan still grouped CLI, MCP, instructions, plugin assets, parity, +and agent evaluation into one public Phase 2. That conflicts with the user's +2026-08-17 decision to launch the CLI silently, dogfood it like `resolve`, and +leave the MCP tool and instructions out. This revision splits those outcomes +into separate phases. Do not implement the old combined Phase 2. ## Scope -### In scope +### Overall scope -- Package addressing for all registries accepted by the existing code-target - parser, when PkgSeer can resolve both published versions to public GitHub +- Package comparisons for registries already accepted by the code-target + parser when PkgSeer can resolve both published versions to public GitHub source. -- Public GitHub repository addressing with explicit `from` and `to` refs. -- Raw inventory, stats, and patch projections, subject to the Phase 2 view - decision. -- Prefix/glob filtering, file and patch budgets, compact text, structured JSON, - typed errors, CLI/MCP parity, smoke coverage, and agent evaluation. -- Later consumption of a typed package-changelog diff action once PkgSeer - publishes that action contract. +- Public GitHub repository comparisons with explicit base and target refs. +- Authoritative raw Inventory, Stats, and Patches projections. +- Repository-relative glob filtering, file and patch budgets, exact resolved + identity, pipe-friendly terminal output, structured JSON, typed errors, and + truthful completeness signals. +- CLI-only dogfooding before MCP/agent exposure. +- Later MCP parity and typed changelog steering after their dependencies are + verified. -### Non-goals +### Overall non-goals - Public structural/symbol diff. -- Compatibility, upgrade-safety, API-stability, or semantic-version verdicts. -- Private repositories, local worktrees, proprietary code, or uncommitted - changes. -- Client-side ranking, patch inversion, rename detection, patch synthesis, or - fallback to a hosted compare API. -- A client cache, queue, lock, concurrency gate, retry layer, or feature flag. -- Reproducing Git's full revision or pathspec language. - -## Target architecture +- Compatibility, safety, API-stability, or semantic-version verdicts. +- Rename/copy detection or compare-era ahead/behind/diverged relationships. +- Working-tree, staged, local-path, private-repository, or arbitrary Git host + diff. +- Full Git revision grammar, three-dot merge-base semantics, multiple + pathspecs, pathspec magic, exclusions, attributes, or unbounded output. +- Automatic indexing, background rollout machinery, feature flags, telemetry + infrastructure, or persistent storage. +- Changelog action parsing before PkgSeer commits the typed action contract. + +## Target architecture and end state ### Boundaries and responsibilities -`packages/core-internal` owns the transport-neutral contract: - -- Add explicit CodeDiff request/result/error types and - `CodeNavigationService.codeDiff(params)`. -- Build the GraphQL variables from one normalized package or repository target. -- Use a mode-specific selection for inventory, stats, and patches. Never select - structural fields. Patch mode selects patch fields; stats omits patch and - patch-only fields; inventory omits per-file `additions`, `deletions`, and - patch content. The authoritative summary's added/deleted file counts remain - selected in every mode. -- Validate the external response with Zod and preserve backend enum/string - values needed to represent content truth. -- Handle root errors separately from `raw` field-local errors. A field-local - raw error must carry parsed GraphQL extensions plus the successfully returned - package/from/to resolutions; ordinary CodeNavigation methods may keep their - existing fail-on-any-error behavior. - -`packages/mcp` owns public request and response behavior: - -- A dedicated diff-target builder reuses existing package/repository parsers - but rejects target-embedded package versions and repository refs because the - comparison endpoints own both identities. -- Shared pure modules normalize view, filters, explicit defaults, and error - envelopes once for CLI/MCP parity. -- A data-first camelCase JSON envelope exposes exact resolutions, scoped - summary, scope, content coverage/failure, files, `hasMoreFiles`, and only the - effective/caller-supplied filters needed to interpret truncation. -- Compact text shows the requested range and exact resolved SHAs once, then - scoped summary, scope warning if unknown, relevance-ordered files, selected - stats/patches, and only actionable recovery guidance. -- MCP validation failures and service errors remain JSON error envelopes in - every requested format. - -The root CLI owns Commander syntax and terminal error adaptation. The MCP -server owns tool registration, annotations, descriptions, instructions, and -public package exports. Both call the same shared builders and formatters. - -### Data flow +- `CodeNavigationService.codeDiff` remains the only network/service boundary. + It owns authenticated GraphQL execution, exact variable construction, + mode-minimal selections, runtime validation, and typed CodeDiff errors. +- Small pure CodeDiff request, response, text, and error helpers live under + `packages/mcp/src/shared/` and are exported only through + `packages/mcp/src/internal.ts` during CLI dogfooding. This follows the + existing `resolve` pattern and lets a later MCP adapter reuse an exercised + contract without exposing a public `@githits/mcp` API prematurely. +- `src/commands/code/diff.ts` owns Commander syntax, CLI-native validation + wording, dependency acquisition, spinner lifecycle, stdout/stderr routing, + JSON serialization, and process exit behavior. It does not construct + GraphQL or duplicate backend response mapping. +- Phase 3 adds a thin MCP `code_diff` adapter only after Phase 2 evidence is + reviewed. It reuses the pure contract, adds agent-native descriptions and + errors, and introduces no second CodeDiff service path. +- Changelog steering later produces typed calls to the stable CLI/MCP + invocation; it never parses human changelog prose for suggestions. + +The root CLI may import `@githits/mcp/internal`. Public packages and future +remote MCP servers must not. No new module from Phase 2 is exported by +`@githits/mcp`, `@githits/mcp/client`, or `@githits/mcp/smoke-test`. + +### Data flow during silent dogfood ```text -CLI args / MCP input - -> shared target, endpoint, view, and option normalization +CLI target + from..to + view/filter/bounds + -> pure CLI request normalization -> CodeNavigationService.codeDiff - -> mode-minimal GraphQL selection of codeDiff.raw - -> Zod-validated transport result or typed partial/root error - -> shared data-first envelope - -> CLI text/JSON or MCP text-v1/JSON + -> mode-minimal codeDiff.raw GraphQL selection + -> typed CodeDiff result or error + -> pure selected-view JSON/text projection + -> primary view on stdout + diagnostics on stderr +``` + +Phase 3 later adds MCP input/output adapters on either side of the same pure +normalization and projection modules. + +### CLI dogfood contract + +The initial syntax is: + +```text +githits code diff .. [options] [-- ] +githits code diff --repo-url .. [options] [-- ] ``` -### Failure behavior - -- Invalid target/range/filter/budget input returns `INVALID_ARGUMENT` without a - network request. -- Shared resolution errors retain backend `code`, `retryable`, side, retry - delay, publication/ref candidates, and ambiguity kinds when supplied. -- Raw first-call failures preserve partial root resolution data when GraphQL - returned it. Codes such as `RAW_DIFF_LIMIT_EXCEEDED`, - `RAW_DIFF_UNAVAILABLE`, `RATE_LIMITED`, `TIMEOUT`, and `UPSTREAM_ERROR` must - not collapse when they imply different recovery. -- A post-inventory `contentFailure` is a successful partial evidence result, - not a thrown request error. Text and JSON must show that inventory is - authoritative while requested content is partial or failed. -- `UNKNOWN` package scope is visibly repository-wide. Empty scoped inventory - is not described as missing evidence when `inventoryComplete=true`. -- `BYTE_ESCAPED` paths remain evidence but cannot be reused as if they were an - exact UTF-8 path. Follow-up hints are emitted only for stable UTF-8 paths. - -## Assumptions and open decisions - -### Verified assumptions - -1. `code` remains the correct command/tool family because the output is source - evidence and composes with `code_read`. -2. Raw remains read-only from the caller's perspective and does not enqueue - indexing. MCP annotations can therefore use the existing read-only shape. -3. The exact-tree schema on backend `main` is the implementation target; no - compatibility adapter for the removed GitHub Compare schema will be added. -4. Structural exposure remains independently blocked by backend quality and - uncertainty work documented in PkgSeer's implementation guide. - -### Resolved product direction (2026-08-17) - -1. **CLI addressing:** use the compact - `githits code diff ..` form for packages and repositories, - with paired `--from` / `--to` only as the delimiter escape hatch. -2. **Public views:** expose Inventory, Stats, and Patches behavior. Design the - names, defaults, flags, output, and option interactions to match `git diff` - expectations as closely as the backend can truthfully support; validate the - ergonomics with agents rather than treating the provisional - `inventory | stat | patch` names as final. -3. **Path scoping:** when a caller supplies path scope, make backend `pathGlob` - the primary public mechanism. An omitted path filter still means the whole - resolved package/repository scope. Retain `pathPrefix` only when it provides - a materially clearer exact-subtree escape hatch. - -The backend glob is not a Git pathspec. It accepts one pattern, keeps `*` and -`?` within one component, and lets an exact `**` component span directories. -It rejects character classes, brace alternatives, negation, empty components, -and multiple patterns. CLI design should investigate a familiar -`-- ` position after the range, but help/tool descriptions must call -the value a bounded glob and document the difference rather than claiming full -Git pathspec compatibility. MCP should use `path_glob` as the primary field. - -The exact client defaults for returned files and cumulative patch bytes are -not a product decision yet. Start with 20 files and 32 KiB as evaluation -hypotheses only. Phase 2 must measure them against the backend's relevance -ordering and select the smallest useful defaults before public descriptions or -tests pin them. - -### External unknowns - -- Whether the exact schema is deployed to the development endpoint. Evidence: - authenticated schema/introspection or representative dev calls. Due before - Phase 2 live smoke; lack of deployment does not block the isolated Phase 1 - adapter and fixtures. -- Whether the backend GraphQL documentation round is description-only. - Evidence: inspect the resulting SDL hash/changelog and implementation diff. - Due before Phase 1 merges if the round lands while Phase 1 is open; any shape - or semantic change requires immediate reorientation. -- The future PkgSeer changelog action discriminator and argument fields. Due - before Phase 3; resolve from the committed backend schema, not prose. - -### Overall dependencies - -- The exact-tree PkgSeer GraphQL contract remains stable through Phase 1 and is - deployed before Phase 2 live validation. -- Phase 2 follows the transport-neutral Phase 1 API and the resolved Git-like - product direction; Phase 3 follows the stable Phase 2 invocation and a - committed backend changelog-action contract. -- Existing CodeNavigation authentication, request-header, content-safety, - smoke, plugin-generation, and package-boundary workflows remain the shared - infrastructure. This effort does not replace them. +- `` accepts an unversioned package target such as `npm:express` or an + existing compact public GitHub target. `--repo-url` remains available for + consistency with the other `code` commands. +- Versions/refs belong only in `..`. A version embedded in a package + target, a ref embedded in a repository target, `--git-ref`, an empty side, + more than one `..` separator, or `...` is rejected before network I/O. +- Direction is always left-to-right. Reverse comparison is expressed by + swapping the endpoints; no compare relationship or merge-base behavior is + inferred. +- The default view is patch output, matching ordinary `git diff`. Explicit + `-p`/`--patch`, `--stat`, `--name-only`, and `--name-status` are supported and + mutually exclusive. `--name-only` and `--name-status` both use the cheap + Inventory service mode; they differ only in the selected output projection. +- One value after `--` maps to backend `pathGlob` and is the primary public + path filter. It is called a repository-relative glob in help and errors, not + a full Git pathspec. Multiple values and unsupported pathspec syntax are + rejected. `pathPrefix` is not exposed during dogfooding; evidence must show a + distinct need before adding a second path control. +- `--max-files` is valid for every view. `--max-patch-bytes` is valid only for + patch view. Omitted values are omitted from the service request so dogfooding + measures the documented backend defaults rather than pinning a separate + client default. +- `--json` returns a data-first envelope containing normalized target identity, + public view, exact `from`/`to` resolutions, scoped summary, scope, + `contentCoverage`, optional `contentFailure`, selected file facts, and + `hasMoreFiles`. File objects contain only facts selected for the view: + path-only for `name-only`, path/status for `name-status`, line counts and + content status for `stat`, and bounded patch/omission/safety facts for patch. +- Default text keeps stdout compatible with the chosen Git-style view: + patches for patch mode, bare paths for `--name-only`, status plus path for + `--name-status`, and line-count rows plus a total for `--stat`. Resolution, + scope, truncation, unprojectable-path, safety, and incomplete-content + diagnostics go to stderr so piping the primary output remains useful. +- `--verbose` adds requested/resolved identities, full summary, effective + scope/filter, returned count, and content coverage to terminal output. JSON + already carries these facts and does not change shape under `--verbose`. +- No rename wording is emitted. Binary and metadata-only changes explicitly + state that content differs without claiming a textual patch. Byte-escaped + paths are marked display-only and are never suggested as exact follow-up + identities. + +### Failure and exit behavior + +- Empty authoritative diffs exit 0, matching normal `git diff` behavior. +- Client validation, authentication, root/shared-resolution errors, and raw + field errors exit 1 and use the existing CLI JSON error envelope on stderr. +- CodeDiff-specific backend details are mapped into a bounded CLI error shape; + arbitrary GraphQL extensions and raw backend codes are not passed through. + Preserve safe side, published-version/ref candidates, retry timing, stage, + limit kind, repository identity, and any partial exact resolutions. +- `PARTIAL` or `FAILED` content coverage inside a successful result remains a + successful evidence envelope and exits 0 because the authoritative inventory + is usable. Text emits an unmistakable stderr warning; JSON retains structured + coverage/failure and per-file statuses. Dogfooding must revisit whether this + is suitable for agent automation before Phase 3. +- Terminal-visible backend text and paths use existing sanitization. Patches + use the backend's content-safety projection; the CLI does not restore removed + content or log patches. + +## Overall assumptions and unknowns + +### Assumptions + +1. Phase 1's public service signatures are sufficient for the CLI; dogfooding + should change normalization and presentation before changing that boundary. +2. Git-like syntax means matching familiar behavior where backend semantics + are equivalent, not accepting Git flags or pathspecs that would be ignored + or approximated. +3. Silent rollout means normal CLI registration, help, implementation docs, + smoke coverage, and a release fragment, while omitting MCP registration, + MCP instructions, Agent Skills, plugin guidance, and proactive agent + evaluation. +4. Manual dogfood notes and existing debug facilities are sufficient for this + phase. No feature flag, telemetry schema, counter, or rollout service is + authorized. + +### Unknowns and product decisions + +- **Development deployment:** whether the dev endpoint serves schema hash + `sha256:28413c4e9b31`. Resolve with authenticated introspection or a + representative CLI call before claiming authenticated live smoke passed. +- **Client defaults:** whether later public agent use needs file/patch limits + smaller than backend defaults. Resolve from Phase 2 dogfood evidence before + Phase 3 schema/descriptions pin defaults. +- **Agent view schema:** whether MCP should expose Git-like view names or the + service's Inventory/Stats/Patches names. Resolve after CLI dogfood and before + Phase 3 implementation. +- **Content-failure exit semantics:** Phase 2 deliberately keeps successful + authoritative inventory at exit 0. Reassess from shell and automation + dogfooding before agent exposure. +- **Changelog action shape:** discriminator, fields, and placement do not yet + exist as a committed client contract. Resolve from future PkgSeer SDL and + implementation before the changelog-steering phase. + +There is no blocking product decision for Phase 2. ## Cross-cutting considerations ### Security and privacy -- Never print, persist, or place credentials in command output, fixtures, - plans, telemetry, or tool-call evidence. Live validation may use existing - process-local authentication only. -- Keep public-Git scope explicit. Do not accept credential-bearing repository - URLs or weaken the existing repository parser. -- Treat paths and patches as untrusted. Respect backend `contentSafety`, keep - formatter-owned layout separate, and do not invent exact-path hints after - identity-changing normalization. - -### Performance and data minimization - -- Mode-minimal GraphQL selections are part of the contract and require wire - tests. Inventory must not request stats or patches; stat must not request - patches. -- Evaluate response bytes, model tokens, materially relevant patched files, - end-to-end time, and agent follow-up quality before fixing client defaults. - Use release builds for any reported performance numbers. -- Do not add client-side ranking. Backend relevance runs over authoritative - scoped inventory before bounds and is the correct ownership boundary. +- Only already-supported public package and public GitHub addressing is + accepted. Existing URL parsing must continue rejecting credential-bearing or + path-bearing repository URLs rather than silently canonicalizing them. +- Do not expose, print, persist, fixture, or record credentials. Authenticated + smoke may use an existing credential only through the normal client path and + must not print its value. +- Dogfood notes may record target/version pairs, aggregate sizes, latency, and + outcome categories, but never patches, file bodies, tokens, or arbitrary + backend failure prose. +- Preserve content-safety and terminal-sanitization behavior in every text and + JSON path. + +### Performance and data fetching + +- Each CLI call makes one CodeDiff GraphQL request through the existing + service. No local Git checkout, GitHub Compare fallback, cache, queue, or + background indexing is introduced. +- View choice must reach the service unchanged so Inventory never selects line + counts and Stats never selects patches or omission reasons. Existing Phase 1 + wire tests remain the authority for this boundary. +- Omitted limits stay omitted. Dogfood observation is not a performance claim + or optimization benchmark; any later optimization requires the repository's + benchmark-first workflow. ### Compatibility, release, and rollback -- Phase 2 changes both public artifacts and therefore adds one changelog - fragment with pending SemVer impacts for `githits` and `@githits/mcp`. -- Use the repository-internal `githits-plugin-maintenance` skill for MCP - instructions, root Agent Skill, plugin/extension manifests, and generated - assets. Edit canonical inputs, run `bun run plugins:generate`, then - `bun run plugins:check`. -- Rollback is the previous CLI/MCP package version; this feature writes no - persistent data and needs no cleanup or migration. +- Phase 2 adds a root CLI command and therefore adds one independent fragment + with pending impacts `githits: minor` and `@githits/mcp: none`. +- Phase 2 does not bump versions directly. Release preparation owns package and + generated-manifest versions. +- Workspace-internal shared modules must not appear in public export maps or + declarations reachable through `@githits/mcp`, `@githits/mcp/client`, or + `@githits/mcp/smoke-test`. +- The feature writes no state and needs no migration. Rollback is the previous + root CLI release; no cleanup is required. ### Durable documentation -Implementation starts a focused CodeDiff document in Phase 1 and updates -`docs/implementation/tools.md`, -`docs/implementation/cli-commands.md`, MCP/CLI parity documentation where the -new shared contract matters, and a focused CodeDiff implementation document. -The durable document must explain exact-tree identity, scope/summary semantics, -content coverage/failure, view-minimal fetching, and non-goals. It must not -copy transient rollout detail from this plan. +- Update `docs/implementation/code-diff.md` with the exercised CLI projection, + exact-tree identity, repository-relative glob semantics, output/failure + contract, and current absence of an MCP tool. +- Update `docs/implementation/cli-commands.md` with command examples, piping, + view flags, JSON shape, warnings, exit behavior, and the silent dogfood + posture. +- Do not edit MCP instructions, `docs/implementation/tools.md`, root Agent + Skills/guidance, generated plugin assets, or `CHANGELOG.md` in Phase 2. ## Phase map | Phase | Status | Outcome | |---|---|---| -| 1. Transport-neutral CodeDiff adapter | Complete | Typed exact-tree request/result/error support in `CodeNavigationService`, with no public command/tool yet | -| 2. Public CLI/MCP raw diff | Design refinement pending; no product blocker | Git-like shared request/response rendering, validated defaults, `githits code diff`, and `code_diff` with tests, smoke, docs, assets, and release fragment | -| 3. Changelog steering | Blocked on backend action contract and Phase 2 | Consume typed PkgSeer changelog actions and render context-appropriate diff calls | +| 1. Transport-neutral CodeDiff adapter | Complete and merged | Typed exact-tree request/result/error support with no CLI command or MCP tool | +| 2. Silent CLI dogfood | Implementation-ready | Git-like `githits code diff`, CLI-only tests/smoke/docs/release fragment, and dogfood evidence with no agent exposure | +| 3. MCP and agent rollout | Blocked on Phase 2 evidence | Stable `code_diff`, CLI/MCP parity, instructions, assets, smoke, and agent evaluation | +| 4. Changelog steering | Blocked on backend action contract and stable Phase 3 invocation | Typed sparse-changelog actions that point to valid CLI/MCP diff calls | | Structural track | Out of scope / backend-blocked | Separate future proposal only after backend says structural evidence is externally safe | ## Phase 1: transport-neutral CodeDiff adapter -**Status:** complete. - -**Expected outcome:** `packages/core-internal` can request and validate all -three raw modes through `CodeNavigationService.codeDiff`, represent partial -content separately from field errors, and preserve exact root identity on raw -field errors. The method and types become public through -`@githits/mcp/client`; no MCP tool, CLI command, or user-facing default is -introduced. - -**Assumptions:** backend `main` exact-tree schema is the intended final client -contract; service callers choose an explicit mode; no compatibility with the -removed compare schema is required. - -**Unknowns/product decisions:** none for this client adapter. The deployment -state is not needed for isolated tests. - -**Dependencies:** current `CodeNavigationService`, shared request-header/token -provider behavior, the inspected PkgSeer backend `main` SDL, and existing -service mock factories. - -**Likely files:** - -- `packages/core-internal/src/services/code-navigation-service.ts` -- `packages/core-internal/src/services/code-navigation-service.test.ts` -- `packages/core-internal/src/index.ts` if public workspace exports require it -- `packages/mcp/src/client.ts` -- `packages/mcp/src/mcp/server.ts` for the descriptor-only service stub -- `packages/mcp/src/services/test-helpers.ts` -- `src/services/test-helpers.ts` -- `scripts/validate-public-packages.ts` -- `docs/implementation/code-diff.md` -- one independent `changes/*.added.md` fragment - -**Contracts and edge cases:** - -- Params use a normalized unversioned package/repository target plus explicit - `from`, `to`, raw mode, and optional raw bounds/filters. -- Package variables populate only `registry/name/fromVersion/toVersion`; - repository variables populate only `repoUrl/fromRef/toRef`. Empty opposite - fields are omitted, not sent as empty strings. -- Each mode has a test-visible minimal selection. Shared identity, summary, - scope, files, `hasMoreFiles`, and `contentSafety` remain selected where used; - content fields appear only in the mode that consumes them. -- Runtime schemas cover every committed enum and nullable field. Unknown - omission/failure strings remain bounded backend facts rather than closed - client enums unless the SDL defines an enum. -- Identical SHAs, empty authoritative inventory, unknown scope, moved package - roots, unprojectable paths, binary/metadata-only files, byte-escaped paths, - partial/failed content, and field-local raw errors all have fixtures. -- Field-local errors preserve root data; root/shared-resolution errors do not - fabricate a result. - -**Ordered implementation:** - -1. Add service-level request/result/error interfaces and update both mock - factories first so compilation exposes every consumer. -2. Add Zod schemas for CodeDiff root, raw modes, GraphQL extensions, and partial - responses. Keep them local to the service until a second consumer proves a - shared schema is needed. -3. Add mode-minimal query selections and exact variable construction. Prefer - the smallest explicit query construction that lets tests assert selected - fields; do not introduce a general GraphQL query builder. -4. Implement `codeDiff` using the existing authenticated request boundary and - a dedicated mapper for CodeDiff-specific partial/root errors. -5. Add focused async service tests for variables, selections, normalization, - malformed responses, root errors, raw field errors with root data, and - post-inventory failure-as-data. -6. Document the client adapter's exact-tree identity, mode-minimal fetching, - partial/error contract, public export boundary, and current lack of a CLI or - MCP tool. Add a changelog fragment with `githits: none` and - `@githits/mcp: minor`; the required interface method is a public client API - addition and custom service implementations must adopt it. -7. Run focused tests, the full `bun test`, `bun run build`, and - `bun run validate:packages`. Inspect built declarations and packed artifacts - to ensure private `@githits/core-internal` names do not leak. - -**Acceptance criteria:** - -- All three modes produce exact variables and minimal selections proven by - wire-level tests. -- Every schema field needed by later text/JSON consumers survives typed - normalization without compare-era fields. -- Partial content remains successful evidence; field-local and root errors are - distinguishable and retain all safe actionable metadata the backend sent. -- Existing navigation behavior and error mapping are unchanged. -- Durable documentation and the changelog fragment state the public client API - impact without advertising an unimplemented tool or command. -- Focused tests, `bun test`, `bun run build`, and - `bun run validate:packages` pass. - -## Phase 2: public CLI/MCP raw diff - -**Status:** product direction resolved; detailed ergonomics must be validated -after Phase 1 and before the public schema/help is pinned. - -**Expected outcome:** users and agents can invoke the same exact-tree raw diff -through `githits code diff` and `code_diff`, with compact truthful output, -lossless selected JSON, useful defaults, and complete local/built/live -validation. - -**Assumptions:** Phase 1 result types remain sufficient; the deployed dev -schema matches the committed SDL before live smoke; the selected public modes -map directly to backend selection modes. - -**Unknowns/product decisions:** no remaining product blocker. Exact Git-like -view names/flags and the single-glob CLI spelling require usability design; -client file/patch defaults require workload evidence. Both are due before the -public schema, descriptions, and tests are finalized. - -**Dependencies:** completed Phase 1; deployed development backend for final -smoke; plugin-maintenance workflow; current smoke/eval harnesses. - -**Likely files/components:** - -- shared `code-diff-request.ts`, `code-diff-response.ts`, and - `code-diff-text.ts` modules with focused tests; -- `packages/mcp/src/shared/code-navigation-error-map.ts` for CodeDiff error - envelope mapping; -- `packages/mcp/src/tools/code-diff.ts`, tool index/server registration, - instructions, public types/exports, and test helpers; -- `src/commands/code/diff.ts`, command registration/help, CLI helpers, and - parity tests; -- smoke-test tool inventories, CLI/MCP smoke scripts, targeted agent workloads, - canonical Agent Skill/guidance, generated plugin assets, implementation - docs, and one `changes/*.added.md` fragment. - -**Behavior and constraints:** - -- MCP input is `target + from + to`, not the backend's coupled flat XOR groups. - The target may be compact or structured but must not embed an endpoint. -- The normal CLI begins with `githits code diff ..`. View and - path controls should borrow familiar Git spelling where semantics align. - Familiar spelling must not imply unsupported Git pathspec, revision, rename, - or unbounded-output behavior. -- `path_glob` is the primary MCP path filter. Evaluate one CLI glob after `--` - as the closest honest match to `git diff ... -- `; do not accept - multiple values or unsupported pathspec magic unless the backend contract - gains equivalent semantics. -- CLI and MCP reject empty endpoints, mixed/partial range forms, explicit empty - filters, invalid glob syntax, out-of-range numeric values, and no-op option - combinations before network I/O. `maxPatchBytes` is valid only for the patch - view; other views reject it instead of accepting an ignored coupled option. -- JSON is a stable data-first projection, not a raw GraphQL wrapper. It retains - selected content truth and exact full SHAs without prose-only facts. -- Text shows exact range direction as `from -> to`; it does not recreate the - removed AHEAD/BEHIND/DIVERGED model. Equal full SHAs are labeled identical. -- Summary counts describe the complete scoped inventory. Returned-file counts, - `hasMoreFiles`, and `unprojectableFiles` remain separate. -- Patch omission actions depend on `contentStatus`, omission reason, and - coverage. Raising total bytes is never suggested for a fixed per-file, - binary, metadata-only, invalid-UTF-8, or transport failure. -- No rename wording appears because the backend does not preserve rename - identity. - -**Ordered implementation:** - -1. Reorient against Phase 1 types and the completed backend documentation - round. Build small CLI/MCP ergonomics fixtures for common `git diff` - expectations, then pin only the spellings whose semantics match. -2. Add pure target/range/view/filter normalization tests, then implement the - shared request builder. Reuse existing parsers without loosening other code - tools. -3. Add data-first response and text fixtures covering every scope, summary, - coverage, failure, content status, path encoding, truncation, identity, and - error state; then implement the smallest formatter modules that satisfy - them. -4. Add MCP and CLI adapters, registrations, read-only annotations, JSON parity, - and surface-native hints. Extend the internal closed `MappedErrorCode` union - and exhaustive mapping switch for `CodeDiffError`: map backend details to a - closed recovery category rather than passing `details.code` through, while - preserving publication/ref candidates and retry timing in the bounded error - details. This is internal error mapping, not a public API change. -5. Before pinning defaults, run representative small patch, medium minor, - large major, non-root monorepo, root-workspace, unknown-scope, +**Status:** complete and merged through PR #287. + +**Expected outcome:** the transport-neutral service can request and validate +exact-tree raw CodeDiff Inventory, Stats, and Patches without exposing a CLI or +MCP surface. + +**Assumptions:** the committed exact-tree backend contract is the client +target; no compatibility adapter for the removed GitHub Compare shape is +needed. + +**Unknowns/product decisions:** none remaining for this completed phase. + +**Dependencies:** completed backend exact-tree SDL and existing authenticated +CodeNavigation request boundary. + +**Observed outcome and acceptance:** mode-minimal variables/selections, runtime +schemas, partial/root error separation, fixtures, mock factories, public client +exports, durable documentation, release fragment, focused tests, full tests, +build, and public-package validation merged. No user command/tool or agent +guidance was added. + +## Phase 2: silent CLI dogfood + +**Status:** implementation-ready. + +**Expected outcome:** developers and deliberate CLI users can run an +authoritative exact-tree raw diff through `githits code diff` with familiar +Git-style views, pipe-friendly text, structured JSON, and truthful bounded +evidence. Agents receive no new tool, instructions, suggestions, or packaged +guidance. + +**Assumptions:** Phase 1 types remain sufficient; existing code-target parsing, +container injection, auth handling, sanitization, spinner, and CLI error +envelopes can be reused without changing their unrelated behavior; backend +defaults are acceptable evaluation defaults but are not yet endorsed for agent +use. + +**Unknowns/product decisions:** none blocking implementation. The development +deployment, later client defaults, Phase 3 MCP view names, and final +content-failure automation semantics remain evidence questions described +above. + +**Dependencies:** current `origin/main`; merged Phase 1; committed backend SDL +and CodeDiff guide; existing CLI container and smoke harness. Authenticated dev +deployment is required only for authenticated live validation. + +### Likely files and components + +- `packages/mcp/src/shared/code-diff-request.ts` and focused tests for target, + range, view, glob, and numeric normalization; +- `packages/mcp/src/shared/code-diff-response.ts` and focused tests for + selected-view success/error envelopes; +- `packages/mcp/src/shared/code-diff-text.ts` and focused tests for + pipe-friendly views and diagnostic facts; +- a small CodeDiff-specific error mapper beside those modules if the existing + generic code-navigation mapper cannot preserve the required bounded details + without broadening unrelated error behavior; +- `packages/mcp/src/internal.ts` for workspace-only exports, with no public + export-map or package-index change; +- `src/commands/code/diff.ts` and `src/commands/code/diff.test.ts`; +- `src/commands/code/index.ts` registration and code-group help; +- `scripts/cli-smoke.ts` structural command/help/auth/JSON coverage; +- `docs/implementation/code-diff.md`, + `docs/implementation/cli-commands.md`, and one new + `changes/*.added.md` fragment. + +Do not add or edit an MCP tool, MCP server registration, MCP instruction, +smoke-test tool inventory, Agent Skill, `AGENTS.md`, plugin/marketplace manifest, +plugin generator, or generated agent asset in this phase. + +### Contracts and edge cases + +- Parse package and repository forms into the Phase 1 target union without + own-key leakage from the opposite target shape. +- Accept exactly one non-empty `from..to` pair. Preserve endpoint spelling in + the request and exact resolved identity in the result. Identical endpoints + and equal resolved SHAs are valid; empty sides, whitespace-only sides, + `...`, or ambiguous separators are invalid. +- Reject versions/refs embedded in target syntax because comparison endpoints + own both identities. Reject `--git-ref` for the same reason. +- Map patch, stat, name-only, and name-status views to the minimal Phase 1 + service mode. Explicit conflicting view flags are invalid instead of relying + on Commander option order. +- Accept at most one non-empty repository-relative glob after `--`. Reject + multiple values, absolute paths, invalid encoding/length, and unsupported + backend glob syntax before network I/O. +- Validate `--max-files` against 1..300 and `--max-patch-bytes` against + 1024..2097152. Reject patch bytes outside patch view. Do not populate omitted + option keys. +- Cover package scope, repository scope, unknown scope, moved package roots, + empty inventory, identical SHA, reverse/diverged pairs, truncation, + unprojectable and byte-escaped paths, binary and metadata-only files, + content omissions, partial/failed content, safety modifications, root errors, + and raw field errors with partial resolutions. +- Keep summary counts separate from returned file count. Never infer that + `contentCoverage: COMPLETE` means the full inventory was returned. +- Preserve pipe-friendly stdout in every view and send non-primary diagnostics + to stderr. Tests must assert the two streams separately. + +### Ordered implementation + +1. Start from current `origin/main`. Add focused behavior fixtures for the + accepted CLI grammar and the four Git-style views, including rejected Git + syntax the backend cannot honor. +2. Write request-normalization tests, then implement the smallest pure builder + that produces `CodeDiffParams`. Reuse existing package/repository parsing + without loosening other `code` commands. +3. Write data-first response fixtures for all scope, summary, coverage, + content-status, safety, truncation, identity, and error states. Implement + selected-view envelopes and the bounded CodeDiff error mapping. +4. Write stdout/stderr formatter fixtures, then implement patch, stat, + name-only, name-status, verbose context, and actionable warnings. Do not + synthesize rename or compare-relationship facts. +5. Add the thin Commander action and registration. Use the existing container, + auth gate, spinner, color/sanitization, JSON error, and exit paths. +6. Extend CLI smoke with command/help registration, unauthenticated auth + handling, invalid local grammar, and built-product coverage. If an existing + credential is available without exposing it and dev serves the schema, run + representative package and repository calls; otherwise record the live + check as unavailable. +7. Dogfood representative small patch, medium release, large release, + non-root monorepo package, root-workspace package, unknown package scope, generated/lockfile-heavy, binary, reverse, diverged, and identical cases. - Compare 16/32/64 KiB and practical file caps using release builds. Record - aggregate counts, bytes/tokens, latency, content status, and agent follow-up - quality without persisting patches or credentials. -6. Pin defaults and descriptions from the evidence. Run targeted Claude and - Codex `agent:e2e` workloads when practical; inspect tool calls, - `toolIssues`, `instructionIssues`, follow-ups, and unsupported conclusions. -7. Update smoke inventories, durable docs, canonical guidance, and changelog - fragment. Use `githits-plugin-maintenance`, generate assets, and inspect the - complete diff. -8. Run `bun test`, `bun run build`, `bun run smoke:cli`, - `bun run smoke:mcp`, `bun run smoke:cli:built`, - `bun run smoke:mcp:built`, `bun run plugins:check`, and external package - export/declaration validation. Run authenticated dev smoke only if an - existing credential is available without exposing it; unauthenticated smoke - must still pass its auth-handling contract. - -**Acceptance criteria:** - -- Equivalent CLI/MCP calls send equivalent normalized service params and - produce the same JSON success/error facts. -- Inventory/stat/patch behavior matches the accepted public view set and each - view fetches no unused GraphQL fields. -- CLI addressing, view flags, `--` path placement, help, and errors feel - familiar to `git diff` users while explicitly rejecting unsupported - pathspec/revision behavior; agents select the intended view and filter in - targeted evaluations. -- Ordinary package upgrades return materially relevant bounded evidence under - the measured defaults; incomplete evidence is unmistakable and leads to a - valid focused follow-up. -- Reverse and diverged calls are direct exact-tree transformations, not - compare-era relationship claims. -- Scope, authoritative summary, returned/projectable counts, content coverage, - partial content failure, and per-file status are impossible to confuse in - text or JSON. -- All required unit, parity, smoke, built-product, plugin, package-boundary, - and agent evaluations pass. Any unavailable authenticated dev check is - reported explicitly rather than represented as passed. - -## Phase 3: changelog steering - -**Status:** blocked on Phase 2 and a committed backend action contract. - -**Expected outcome:** sparse/missing range changelogs carry typed package-only -diff arguments that GitHits maps to the active MCP or CLI syntax without -matching human prose. - -**Assumptions:** the action remains transport-neutral and package-addressed; -Phase 2 invocation is stable. - -**Unknowns/product decisions:** exact discriminator, field names, and success / -error placement. Resolve from the future SDL and backend implementation during -phase-boundary reorientation. - -**Dependencies:** deployed Phase 2 and committed/deployed PkgSeer action schema. + Record only target/version pairs and aggregate outcome/size/latency notes. + Evaluate view spelling, stdout piping, one-glob sufficiency, backend default + usefulness, warnings, and recovery behavior; add no instrumentation. +8. Update durable CLI/CodeDiff documentation and add the CLI-only release + fragment. Inspect the complete diff to confirm MCP instructions, tool + inventories, Agent Skills, and generated assets are unchanged. +9. Run focused tests, `bun test`, `bun run build`, `bun run smoke:cli`, + `bun run smoke:cli:built`, `bun run validate:packages`, formatting/lint + checks required by the changed files, and package-artifact inspection. Do + not report an unavailable authenticated smoke as passed. + +### Acceptance criteria + +- The documented package and repository examples normalize to exact Phase 1 + service params; invalid/mixed/empty forms fail before service invocation. +- Default patch, explicit patch, stat, name-only, and name-status output use the + intended minimal service modes and preserve Git-like stdout discipline. +- Omitted bounds are absent on the wire; explicit bounds and the single glob + are validated and forwarded exactly. +- Text and JSON cannot confuse full scoped summary, returned/projectable + files, content coverage, content failure, or per-file content status. +- Exact resolutions and safe recovery facts survive success and typed error + projections without arbitrary GraphQL extensions or unsafe terminal text. +- Empty and identical diffs succeed; real errors fail; successful partial or + failed content evidence succeeds with unmistakable structured/text warnings. +- CLI help states the bounded glob and revision limitations without claiming + full Git pathspec or revision compatibility. +- No `code_diff` tool, MCP instruction, agent guidance, plugin asset, or public + `@githits/mcp` export is added or changed. +- Durable docs describe the dogfood posture and one release fragment records + `githits: minor`, `@githits/mcp: none`. +- Focused tests, full tests, build, CLI smoke, built CLI smoke, package + validation, and relevant format/lint checks pass. Authenticated dev results + are recorded accurately. + +## Phase 3: MCP and agent rollout + +**Status:** blocked on Phase 2 merge, dogfood evidence, and phase-boundary +reorientation. + +**Expected outcome:** the exercised raw-diff contract becomes a public +`code_diff` MCP tool with CLI/MCP JSON parity, agent-native errors and +descriptions, minimal instructions/guidance, generated assets, complete smoke +coverage, and targeted agent evaluation. + +**Assumptions:** dogfooding identifies a stable view/filter contract and useful +bounded defaults; no Phase 1 service redesign is required; structural remains +unexposed. + +**Unknowns/product decisions:** MCP view names, public defaults, content-failure +automation semantics, whether `pathPrefix` has proven necessary, and any CLI +signature corrections found during dogfooding. Resolve from Phase 2 evidence +before adding tactical Phase 3 steps. + +**Dependencies:** merged Phase 2; verified deployed backend; accepted dogfood +findings; MCP/package release workflow; plugin-maintenance workflow; smoke and +agent-evaluation harnesses. + +**Acceptance criteria:** equivalent CLI/MCP requests produce equivalent +normalized service params and JSON facts; every view remains mode-minimal; +agents choose correct views/filters and avoid unsupported conclusions in +targeted Claude and Codex evaluations; MCP/CLI smoke, built smoke, plugin +generation/checks, package validation, and required release fragments pass; +durable instructions remain concise and truthful. Tactical files and steps are +intentionally deferred until Phase 2 reorientation supplies evidence. + +## Phase 4: changelog steering + +**Status:** blocked on a stable Phase 3 invocation and a committed/deployed +PkgSeer changelog-action contract. + +**Expected outcome:** sparse or missing package range changelogs carry typed +package-only diff arguments that GitHits maps to the active CLI or MCP syntax +without matching human prose. + +**Assumptions:** the future action remains transport-neutral and +package-addressed; the stabilized diff invocation can represent it directly. + +**Unknowns/product decisions:** exact discriminator, field names, success/error +placement, and fallback behavior. Resolve from the future SDL and backend +implementation during phase-boundary reorientation. + +**Dependencies:** merged Phase 3 and committed/deployed PkgSeer action schema. **Acceptance criteria:** empty, partial, and missing-content range outcomes -steer to a valid CodeDiff call in both contexts; latest/repository outcomes and -unknown actions remain truthful; structural stays unexposed. Tactical files -and steps are intentionally deferred until the backend contract exists. +steer to valid CodeDiff calls in both contexts; latest/repository outcomes and +unknown actions remain truthful; structural stays unexposed. Tactical detail is +intentionally deferred until the backend contract exists. ## Phase-boundary reorientation -After each phase merges, fetch and rebase on current `origin/main`, inspect the -merged delta and current PkgSeer SDL/changelog/implementation guide, then update -this same plan before detailing or implementing the next phase. Record: +After each phase merges and before detailing or implementing the next phase: -- the merged outcome and tests actually passed; -- changed schema fields, assumptions, decisions, dependencies, or release - boundaries; -- contradictions between this plan, implementation, backend behavior, or user - comments; and -- whether the next phase is `READY`, `REPLAN`, or `PRODUCT INPUT NEEDED`. +1. run `$next-steps` against current `origin/main` and fetch the backend's + current `origin/main`; +2. inspect the merged delta, tests actually passed, SDL hash/changelog, + deployment evidence, and dogfood findings; +3. record changed assumptions, decisions, dependencies, release boundaries, + and contradictions in this same plan; +4. classify the next phase as `READY`, `REPLAN`, or `PRODUCT INPUT NEEDED`; and +5. add tactical detail only when its blocking evidence and product decisions + are resolved. -Do not carry forward compare-era fields, an unmeasured client default, or an -unverified changelog action shape. +Do not rebase or rewrite branch history unless explicitly requested. Do not +carry forward compare-era fields, unmeasured client defaults, or an unverified +changelog action shape. ## Overall acceptance, completion, and plan cleanup The effort is complete only when: - raw CodeDiff is released through CLI and MCP with exact-tree identity, - truthful scoped inventory/content states, and measured useful defaults; + truthful scope/inventory/content states, and evidence-backed defaults; - typed changelog steering is consumed in both CLI and MCP contexts; - structural remains honestly unexposed; -- durable docs match the implementation and no compare-era contract remains in - active guidance; and +- durable implementation docs match shipped behavior and no compare-era + contract remains in active guidance; and - all required unit, build, parity, smoke, built-product, package-boundary, - plugin, live-when-authenticated, and targeted agent validation has passed or - an unavailable authenticated check is explicitly reported. + plugin, and agent validations for the affected phases pass. -Keep this plan through final implementation review. Then transfer remaining -lasting decisions to `docs/implementation/`, delete this plan as the final -substantive cleanup, and search the repository for stale compare-era or plan -references. +Keep this plan through implementation review. After the final phase merges, +transfer all lasting architecture, contracts, operational findings, and +rollout decisions to `docs/implementation/`, then delete this temporary plan so +it cannot become stale guidance. From 568c0a912c0c7bae9f5dece48f796afa6c0ebd0f Mon Sep 17 00:00:00 2001 From: Juha Litola Date: Mon, 17 Aug 2026 14:48:08 +0300 Subject: [PATCH 02/19] feat: normalize CodeDiff CLI requests Add the workspace-internal request contract for Git-like diff views, exact range parsing, bounded repository-relative globs, and raw option validation without exposing an MCP tool. --- packages/mcp/src/internal.ts | 1 + .../mcp/src/shared/code-diff-request.test.ts | 283 +++++++++++++++ packages/mcp/src/shared/code-diff-request.ts | 343 ++++++++++++++++++ 3 files changed, 627 insertions(+) create mode 100644 packages/mcp/src/shared/code-diff-request.test.ts create mode 100644 packages/mcp/src/shared/code-diff-request.ts diff --git a/packages/mcp/src/internal.ts b/packages/mcp/src/internal.ts index feea71b..cebb16d 100644 --- a/packages/mcp/src/internal.ts +++ b/packages/mcp/src/internal.ts @@ -1,5 +1,6 @@ export * from "./index.js"; export { getMcpToolDefinitions } from "./mcp/server.js"; +export * from "./shared/code-diff-request.js"; export * from "./shared/code-navigation.js"; export * from "./shared/code-navigation-defaults.js"; export * from "./shared/code-navigation-error-map.js"; diff --git a/packages/mcp/src/shared/code-diff-request.test.ts b/packages/mcp/src/shared/code-diff-request.test.ts new file mode 100644 index 0000000..bb897b6 --- /dev/null +++ b/packages/mcp/src/shared/code-diff-request.test.ts @@ -0,0 +1,283 @@ +import { describe, expect, it } from "bun:test"; +import { + buildCodeDiffParams, + type CodeDiffRequestInput, + type CodeDiffView, +} from "./code-diff-request.js"; +import { InvalidPackageSpecError } from "./package-spec.js"; + +function invalid(input: CodeDiffRequestInput, message?: string): void { + expect(() => buildCodeDiffParams(input)).toThrow(InvalidPackageSpecError); + if (message !== undefined) { + expect(() => buildCodeDiffParams(input)).toThrow(message); + } +} + +describe("buildCodeDiffParams", () => { + it("builds an unversioned package target and preserves trimmed endpoints", () => { + const result = buildCodeDiffParams({ + target: " npm:express ", + range: " v4.18.1 .. v4.18.2 ", + }); + + expect(result).toEqual({ + params: { + target: { registry: "NPM", packageName: "express" }, + from: "v4.18.1", + to: "v4.18.2", + mode: "patches", + }, + view: "patch", + }); + expect(Object.hasOwn(result.params.target, "version")).toBe(false); + expect(Object.hasOwn(result.params.target, "repoUrl")).toBe(false); + expect(Object.hasOwn(result.params, "options")).toBe(false); + }); + + it("builds a compact repository target without the parser's ref key", () => { + const result = buildCodeDiffParams({ + target: "github:expressjs/express", + range: "main..release", + }); + + expect(result.params.target).toEqual({ + repoUrl: "https://github.com/expressjs/express", + }); + expect(Object.hasOwn(result.params.target, "registry")).toBe(false); + expect(Object.hasOwn(result.params.target, "packageName")).toBe(false); + expect(Object.hasOwn(result.params.target, "gitRef")).toBe(false); + }); + + it("builds an explicit repository URL", () => { + const { params } = buildCodeDiffParams({ + repoUrl: "https://github.com/expressjs/express", + range: "v4.18.1..v4.18.2", + }); + + expect(params.target).toEqual({ + repoUrl: "https://github.com/expressjs/express", + }); + }); + + it.each([ + ["patch", "patches"], + ["stat", "stats"], + ["name-only", "inventory"], + ["name-status", "inventory"], + ] as const)("maps %s to service mode %s", (view, mode) => { + const result = buildCodeDiffParams({ + target: "npm:express", + range: "1..2", + view, + }); + + expect(result.view).toBe(view); + expect(result.params.mode).toBe(mode); + }); + + it("forwards only explicitly supplied options", () => { + const result = buildCodeDiffParams({ + target: "npm:express", + range: "1..2", + pathGlob: "src/**/*.ts", + maxFiles: 300, + maxPatchBytes: 2_097_152, + }); + + expect(result.params.options).toEqual({ + pathGlob: "src/**/*.ts", + maxFiles: 300, + maxPatchBytes: 2_097_152, + }); + expect(Object.hasOwn(result.params.options ?? {}, "pathPrefix")).toBe( + false, + ); + }); + + it("omits options when every bound and glob is omitted", () => { + const { params } = buildCodeDiffParams({ + target: "npm:express", + range: "1..2", + maxFiles: undefined, + maxPatchBytes: undefined, + pathGlob: undefined, + }); + + expect(Object.hasOwn(params, "options")).toBe(false); + }); + + it("accepts representative bounded repository-relative globs", () => { + for (const pathGlob of [ + "src/**/*.ts", + "**/*.ts", + "src/?odule/file?.ts", + "src/\\[generated\\]/literal\\*", + "docs/guide\\!/*.md", + "dir with spaces/*.ts", + ]) { + expect( + buildCodeDiffParams({ + target: "npm:express", + range: "1..2", + pathGlob, + }).params.options?.pathGlob, + ).toBe(pathGlob); + } + }); + + it("rejects absent, mixed, and incomplete addressing", () => { + invalid({ range: "1..2" }, "exactly one"); + invalid( + { + target: "npm:express", + repoUrl: "https://github.com/a/b", + range: "1..2", + }, + "either", + ); + invalid({ target: " ", range: "1..2" }); + invalid({ repoUrl: " ", range: "1..2" }); + invalid({ repoUrl: "npm:express", range: "1..2" }, "repository target"); + }); + + it("rejects versions and refs embedded in target addressing", () => { + invalid( + { target: "npm:express@4.18.1", range: "1..2" }, + "must not include a version", + ); + invalid( + { target: "github:expressjs/express#main", range: "1..2" }, + "must not include a ref", + ); + invalid( + { + repoUrl: "https://github.com/expressjs/express#main", + range: "1..2", + }, + "must not include a ref", + ); + invalid( + { + target: "https://github.com/expressjs/express@main", + range: "1..2", + }, + "must not include a ref", + ); + }); + + it("rejects invalid ranges and accepts identical endpoints", () => { + for (const range of [ + "1", + "..2", + "1..", + "..", + "1...2", + "1..2..3", + "1....2", + "1..2..", + "1.. ..2", + ]) { + invalid({ target: "npm:express", range }); + } + + expect( + buildCodeDiffParams({ target: "npm:express", range: " same .. same " }) + .params, + ).toMatchObject({ from: "same", to: "same" }); + }); + + it("rejects invalid path glob forms", () => { + for (const pathGlob of [ + "", + "/src/*.ts", + "src//*.ts", + "src/./*.ts", + "src/../*.ts", + "src/", + "src/*.ts/", + "src/**.ts", + "src/a***.ts", + "src/[test].ts", + "src/{test}.ts", + "src/!test.ts", + "src/foo\\", + "src/foo\\/*.ts", + ]) { + invalid({ target: "npm:express", range: "1..2", pathGlob }); + } + + invalid( + { + target: "npm:express", + range: "1..2", + pathGlob: "a".repeat(1025), + }, + "1024", + ); + invalid( + { + target: "npm:express", + range: "1..2", + pathGlob: "é".repeat(513), + }, + "1024", + ); + invalid( + { + target: "npm:express", + range: "1..2", + pathGlob: "bad\ud800", + }, + "valid UTF-8", + ); + }); + + it("validates numeric bounds and patch-byte view compatibility", () => { + for (const maxFiles of [ + 0, + 301, + 1.5, + Number.NaN, + Number.POSITIVE_INFINITY, + ]) { + invalid({ target: "npm:express", range: "1..2", maxFiles }, "maxFiles"); + } + for (const maxPatchBytes of [1023, 2_097_153, 1.5, Number.NaN]) { + invalid( + { target: "npm:express", range: "1..2", maxPatchBytes }, + "maxPatchBytes", + ); + } + invalid( + { + target: "npm:express", + range: "1..2", + view: "stat", + maxPatchBytes: 1024, + }, + "only", + ); + + expect( + buildCodeDiffParams({ + target: "npm:express", + range: "1..2", + maxFiles: 1, + maxPatchBytes: 1024, + }).params.options, + ).toEqual({ maxFiles: 1, maxPatchBytes: 1024 }); + }); + + it("rejects unknown runtime view values, including inherited keys", () => { + for (const view of ["summary", "toString"]) { + invalid( + { + target: "npm:express", + range: "1..2", + view: view as CodeDiffView, + }, + "view", + ); + } + }); +}); diff --git a/packages/mcp/src/shared/code-diff-request.ts b/packages/mcp/src/shared/code-diff-request.ts new file mode 100644 index 0000000..ff6b639 --- /dev/null +++ b/packages/mcp/src/shared/code-diff-request.ts @@ -0,0 +1,343 @@ +import type { + CodeDiffParams, + CodeNavigationTarget, +} from "@githits/core-internal"; +import { parseCodeNavigationTargetSpec } from "./code-navigation-target.js"; +import { InvalidPackageSpecError } from "./package-spec.js"; + +export type CodeDiffView = "patch" | "stat" | "name-only" | "name-status"; + +export interface CodeDiffRequestInput { + target?: string; + repoUrl?: string; + range: string; + view?: CodeDiffView; + pathGlob?: string; + maxFiles?: number; + maxPatchBytes?: number; +} + +export interface CodeDiffRequestBuildResult { + params: CodeDiffParams; + view: CodeDiffView; +} + +const VIEW_TO_MODE: Record = { + patch: "patches", + stat: "stats", + "name-only": "inventory", + "name-status": "inventory", +}; + +const MAX_PATH_GLOB_BYTES = 1024; +const MAX_FILES_MIN = 1; +const MAX_FILES_MAX = 300; +const MAX_PATCH_BYTES_MIN = 1024; +const MAX_PATCH_BYTES_MAX = 2_097_152; + +export function buildCodeDiffParams( + input: CodeDiffRequestInput, +): CodeDiffRequestBuildResult { + const target = buildTarget(input); + const { from, to } = parseRange(input.range); + const view = normaliseView(input.view); + const pathGlob = normalisePathGlob(input.pathGlob); + const maxFiles = normaliseIntegerOption( + input.maxFiles, + "maxFiles", + MAX_FILES_MIN, + MAX_FILES_MAX, + ); + const maxPatchBytes = normaliseIntegerOption( + input.maxPatchBytes, + "maxPatchBytes", + MAX_PATCH_BYTES_MIN, + MAX_PATCH_BYTES_MAX, + ); + + if (maxPatchBytes !== undefined && view !== "patch") { + throw invalid( + "`maxPatchBytes` is valid only when the CodeDiff view is `patch`.", + ); + } + + const options = buildOptions({ maxFiles, maxPatchBytes, pathGlob }); + const params: CodeDiffParams = { + target, + from, + to, + mode: VIEW_TO_MODE[view], + }; + if (options !== undefined) params.options = options; + + return { params, view }; +} + +function buildTarget(input: CodeDiffRequestInput): CodeDiffParams["target"] { + const hasTarget = input.target !== undefined; + const hasRepoUrl = input.repoUrl !== undefined; + + if (hasTarget && hasRepoUrl) { + throw invalid("Provide either `target` or `repoUrl`, not both."); + } + if (!hasTarget && !hasRepoUrl) { + throw invalid("Provide exactly one of `target` or `repoUrl`."); + } + + const raw = hasTarget ? input.target : input.repoUrl; + if (typeof raw !== "string") { + throw invalid("CodeDiff target must be a string."); + } + + const parsed = parseTarget(raw); + if (hasTarget && parsed.version !== undefined) { + throw invalid( + "Package targets must not include a version; put both versions in `range`.", + ); + } + if (parsed.gitRef !== undefined) { + throw invalid( + "Repository targets must not include a ref; put both refs in `range`.", + ); + } + + const hasPackageKeys = + Object.hasOwn(parsed, "registry") || Object.hasOwn(parsed, "packageName"); + const hasRepoKey = Object.hasOwn(parsed, "repoUrl"); + if (hasRepoUrl && hasPackageKeys) { + throw invalid("`repoUrl` must identify a repository target."); + } + if (hasPackageKeys && hasRepoKey) { + throw invalid( + "CodeDiff target cannot combine package and repository keys.", + ); + } + if (hasPackageKeys) { + if ( + !Object.hasOwn(parsed, "registry") || + !Object.hasOwn(parsed, "packageName") || + parsed.registry === undefined || + parsed.packageName === undefined + ) { + throw invalid( + "CodeDiff package target must include a registry and name.", + ); + } + return { + registry: parsed.registry, + packageName: parsed.packageName, + }; + } + if (hasRepoKey && parsed.repoUrl !== undefined) { + return { repoUrl: parsed.repoUrl }; + } + throw invalid("CodeDiff target must be a package or repository target."); +} + +function parseTarget(raw: string): CodeNavigationTarget { + try { + return parseCodeNavigationTargetSpec(raw); + } catch (error) { + if (error instanceof InvalidPackageSpecError) throw error; + const message = error instanceof Error ? error.message : "Invalid target."; + throw invalid(message); + } +} + +function parseRange(raw: string): { from: string; to: string } { + if (typeof raw !== "string") { + throw invalid("CodeDiff range must be a string in the form `from..to`."); + } + const range = raw.trim(); + if (range.includes("...")) { + throw invalid( + "CodeDiff range must use one `from..to` separator, not `...`.", + ); + } + + const separator = range.indexOf(".."); + if (separator === -1 || range.indexOf("..", separator + 2) !== -1) { + throw invalid( + "CodeDiff range must contain exactly one `from..to` separator.", + ); + } + + const from = range.slice(0, separator).trim(); + const to = range.slice(separator + 2).trim(); + if (!from || !to) { + throw invalid("CodeDiff range endpoints must not be empty."); + } + return { from, to }; +} + +function normaliseView(raw: CodeDiffView | undefined): CodeDiffView { + if (raw === undefined) return "patch"; + if (typeof raw === "string" && Object.hasOwn(VIEW_TO_MODE, raw)) { + return raw; + } + throw invalid( + "CodeDiff view must be patch, stat, name-only, or name-status.", + ); +} + +function normalisePathGlob(raw: string | undefined): string | undefined { + if (raw === undefined) return undefined; + if (typeof raw !== "string") { + throw invalid("`pathGlob` must be a string when supplied."); + } + + const pathGlob = raw; + if (pathGlob.length === 0) { + throw invalid("`pathGlob` must not be empty when supplied."); + } + if (hasInvalidUtf16(pathGlob)) { + throw invalid("`pathGlob` must be valid UTF-8."); + } + if (new TextEncoder().encode(pathGlob).byteLength > MAX_PATH_GLOB_BYTES) { + throw invalid( + `\`pathGlob\` must be at most ${MAX_PATH_GLOB_BYTES} UTF-8 bytes.`, + ); + } + + validatePathGlobGrammar(pathGlob); + return pathGlob; +} + +interface GlobToken { + char: string; + escaped: boolean; +} + +function validatePathGlobGrammar(pathGlob: string): void { + const components: GlobToken[][] = []; + let component: GlobToken[] = []; + const characters = Array.from(pathGlob); + + for (let index = 0; index < characters.length; index += 1) { + const character = characters[index]; + if (character === undefined) break; + if (character === "/") { + addGlobComponent(components, component); + component = []; + continue; + } + if (character === "\\") { + const escaped = characters[index + 1]; + if (escaped === undefined || escaped === "/") { + throw invalid( + "`pathGlob` backslashes must escape one following non-slash character.", + ); + } + component.push({ char: escaped, escaped: true }); + index += 1; + continue; + } + if ( + character === "[" || + character === "]" || + character === "{" || + character === "}" || + character === "!" + ) { + throw invalid( + "`pathGlob` does not support unescaped brackets, braces, or `!`.", + ); + } + component.push({ char: character, escaped: false }); + } + addGlobComponent(components, component); + + for (const tokens of components) { + const isGlobstar = + tokens.length === 2 && + tokens.every(({ char, escaped }) => char === "*" && !escaped); + if (isGlobstar) continue; + + let previous: GlobToken | undefined; + for (const token of tokens) { + if ( + previous?.char === "*" && + !previous.escaped && + token.char === "*" && + !token.escaped + ) { + throw invalid( + "`pathGlob` allows adjacent stars only as an exact `**` component.", + ); + } + previous = token; + } + } +} + +function addGlobComponent( + components: GlobToken[][], + component: GlobToken[], +): void { + if (component.length === 0) { + throw invalid( + "`pathGlob` must use non-empty repository-relative components.", + ); + } + const literal = component.map(({ char }) => char).join(""); + if (literal === "." || literal === "..") { + throw invalid("`pathGlob` must not contain `.` or `..` components."); + } + components.push(component); +} + +function hasInvalidUtf16(value: string): boolean { + for (let index = 0; index < value.length; index += 1) { + const code = value.charCodeAt(index); + if (code >= 0xd800 && code <= 0xdbff) { + const next = value.charCodeAt(index + 1); + if (next >= 0xdc00 && next <= 0xdfff) { + index += 1; + continue; + } + return true; + } + if (code >= 0xdc00 && code <= 0xdfff) return true; + } + return false; +} + +function normaliseIntegerOption( + value: number | undefined, + name: string, + minimum: number, + maximum: number, +): number | undefined { + if (value === undefined) return undefined; + if (!Number.isInteger(value) || value < minimum || value > maximum) { + throw invalid( + `\`${name}\` must be an integer from ${minimum} through ${maximum}.`, + ); + } + return value; +} + +function buildOptions(input: { + maxFiles: number | undefined; + maxPatchBytes: number | undefined; + pathGlob: string | undefined; +}): CodeDiffParams["options"] { + if ( + input.maxFiles === undefined && + input.maxPatchBytes === undefined && + input.pathGlob === undefined + ) { + return undefined; + } + const options: NonNullable = {}; + if (input.maxFiles !== undefined) options.maxFiles = input.maxFiles; + if (input.maxPatchBytes !== undefined) { + options.maxPatchBytes = input.maxPatchBytes; + } + if (input.pathGlob !== undefined) options.pathGlob = input.pathGlob; + return options; +} + +function invalid(message: string): InvalidPackageSpecError { + return new InvalidPackageSpecError(message); +} From e57867d335d77aad0d74fb20623108bce59d3698 Mon Sep 17 00:00:00 2001 From: Juha Litola Date: Mon, 17 Aug 2026 14:58:32 +0300 Subject: [PATCH 03/19] feat: project CodeDiff CLI results Add the workspace-internal data-first success envelope with view-minimal file facts, exact resolution identity, and mandatory path-encoding safety across every CLI view. --- docs/plans/code-diff-cli-mcp.md | 8 +- packages/mcp/src/internal.ts | 1 + .../mcp/src/shared/code-diff-response.test.ts | 400 ++++++++++++++++++ packages/mcp/src/shared/code-diff-response.ts | 281 ++++++++++++ 4 files changed, 687 insertions(+), 3 deletions(-) create mode 100644 packages/mcp/src/shared/code-diff-response.test.ts create mode 100644 packages/mcp/src/shared/code-diff-response.ts diff --git a/docs/plans/code-diff-cli-mcp.md b/docs/plans/code-diff-cli-mcp.md index b9cc10d..018afdb 100644 --- a/docs/plans/code-diff-cli-mcp.md +++ b/docs/plans/code-diff-cli-mcp.md @@ -211,9 +211,11 @@ githits code diff --repo-url .. [options] [-- ] - `--json` returns a data-first envelope containing normalized target identity, public view, exact `from`/`to` resolutions, scoped summary, scope, `contentCoverage`, optional `contentFailure`, selected file facts, and - `hasMoreFiles`. File objects contain only facts selected for the view: - path-only for `name-only`, path/status for `name-status`, line counts and - content status for `stat`, and bounded patch/omission/safety facts for patch. + `hasMoreFiles`. Every file keeps `pathEncoding` so byte-escaped display paths + cannot be mistaken for reusable identities. Beyond that safety fact, file + objects contain only facts selected for the view: path-only for `name-only`, + path/status for `name-status`, line counts and content status for `stat`, and + bounded patch/omission/safety facts for patch. - Default text keeps stdout compatible with the chosen Git-style view: patches for patch mode, bare paths for `--name-only`, status plus path for `--name-status`, and line-count rows plus a total for `--stat`. Resolution, diff --git a/packages/mcp/src/internal.ts b/packages/mcp/src/internal.ts index cebb16d..7f056b8 100644 --- a/packages/mcp/src/internal.ts +++ b/packages/mcp/src/internal.ts @@ -1,6 +1,7 @@ export * from "./index.js"; export { getMcpToolDefinitions } from "./mcp/server.js"; export * from "./shared/code-diff-request.js"; +export * from "./shared/code-diff-response.js"; export * from "./shared/code-navigation.js"; export * from "./shared/code-navigation-defaults.js"; export * from "./shared/code-navigation-error-map.js"; diff --git a/packages/mcp/src/shared/code-diff-response.test.ts b/packages/mcp/src/shared/code-diff-response.test.ts new file mode 100644 index 0000000..13b2726 --- /dev/null +++ b/packages/mcp/src/shared/code-diff-response.test.ts @@ -0,0 +1,400 @@ +import { describe, expect, it } from "bun:test"; +import type { CodeDiffParams, CodeDiffResult } from "@githits/core-internal"; +import { + type BuildCodeDiffPayloadOptions, + buildCodeDiffSuccessPayload, +} from "./code-diff-response.js"; + +const packageTarget: CodeDiffParams["target"] = { + registry: "NPM", + packageName: "request-package", +}; + +const repositoryTarget: CodeDiffParams["target"] = { + repoUrl: "https://github.com/example/repository", +}; + +function makeResult(): CodeDiffResult { + return { + package: { + registry: "PYPI", + name: "canonical-package", + repoUrl: "https://github.com/example/canonical-package", + }, + fromResolution: { + requested: " from ", + resolvedVersion: "1.0.0", + ref: "refs/tags/v1.0.0", + commitSha: "0123456789abcdef0123456789abcdef01234567", + refKind: "TAG", + versionSource: "REGISTRY", + }, + toResolution: { + requested: "to-ref", + resolvedVersion: undefined, + ref: "to-ref", + commitSha: "fedcba9876543210fedcba9876543210fedcba98", + refKind: "HEAD", + versionSource: "GIT_HEAD", + }, + raw: { + summary: { + filesChanged: 8, + added: 3, + deleted: 2, + modified: 3, + modeChanged: 1, + typeChanged: 2, + inventoryComplete: false, + unprojectableFiles: 1, + }, + scope: { + status: "PACKAGE", + fromSubpath: "", + toSubpath: "", + pathPrefix: "src", + pathGlob: "src/**/*.ts", + }, + contentCoverage: "PARTIAL", + contentFailure: undefined, + files: [ + { + path: "first\\x80.ts", + pathEncoding: "BYTE_ESCAPED", + status: "ADDED", + modeChanged: true, + typeChanged: true, + additions: 0, + deletions: 2, + contentStatus: "OMITTED", + contentOmissionReason: "invalid_utf8", + contentSafety: { + filtered: true, + modifications: ["HTML_COMMENTS_STRIPPED", "IMAGES_REPLACED"], + }, + }, + { + path: "second.ts", + pathEncoding: "UTF8", + status: "MODIFIED", + modeChanged: false, + typeChanged: false, + additions: 4, + deletions: 1, + patch: "@@ -1 +1 @@\n-old\n+new", + contentStatus: "PATCH", + contentSafety: { filtered: false, modifications: [] }, + }, + ], + hasMoreFiles: true, + }, + }; +} + +function options( + target: CodeDiffParams["target"] = packageTarget, + view: BuildCodeDiffPayloadOptions["view"] = "patch", +): BuildCodeDiffPayloadOptions { + return { target, view }; +} + +describe("buildCodeDiffSuccessPayload", () => { + it("uses canonical package facts and omits opposite target keys", () => { + const payload = buildCodeDiffSuccessPayload( + makeResult(), + options(packageTarget, "name-only"), + ); + + expect(payload.target).toEqual({ + kind: "package", + registry: "pypi", + name: "canonical-package", + repoUrl: "https://github.com/example/canonical-package", + }); + expect(Object.keys(payload.target)).toEqual([ + "kind", + "registry", + "name", + "repoUrl", + ]); + expect(Object.hasOwn(payload.target, "repoUrl")).toBe(true); + expect(Object.hasOwn(payload.target, "gitRef")).toBe(false); + }); + + it("uses normalized package facts when the result has no package", () => { + const result = makeResult(); + result.package = undefined; + const payload = buildCodeDiffSuccessPayload(result, options()); + + expect(payload.target).toEqual({ + kind: "package", + registry: "npm", + name: "request-package", + }); + expect(Object.keys(payload.target)).toEqual(["kind", "registry", "name"]); + expect(Object.hasOwn(payload.target, "repoUrl")).toBe(false); + }); + + it("projects repository targets without package-shaped keys", () => { + const payload = buildCodeDiffSuccessPayload( + makeResult(), + options(repositoryTarget, "name-status"), + ); + + expect(payload.target).toEqual({ + kind: "repository", + repoUrl: "https://github.com/example/repository", + }); + expect(Object.keys(payload.target)).toEqual(["kind", "repoUrl"]); + expect(Object.hasOwn(payload.target, "registry")).toBe(false); + expect(Object.hasOwn(payload.target, "name")).toBe(false); + }); + + it("preserves full resolution identity and lowercases enum values", () => { + const payload = buildCodeDiffSuccessPayload( + makeResult(), + options(packageTarget, "name-only"), + ); + + expect(payload.from).toEqual({ + requested: " from ", + resolvedVersion: "1.0.0", + ref: "refs/tags/v1.0.0", + commitSha: "0123456789abcdef0123456789abcdef01234567", + refKind: "tag", + versionSource: "registry", + }); + expect(payload.to).toEqual({ + requested: "to-ref", + ref: "to-ref", + commitSha: "fedcba9876543210fedcba9876543210fedcba98", + refKind: "head", + versionSource: "git_head", + }); + expect(Object.hasOwn(payload.to, "resolvedVersion")).toBe(false); + }); + + it("preserves summary, root package scope, truncation, and coverage facts", () => { + const payload = buildCodeDiffSuccessPayload( + makeResult(), + options(packageTarget, "name-only"), + ); + + expect(payload.summary).toEqual({ + filesChanged: 8, + added: 3, + deleted: 2, + modified: 3, + modeChanged: 1, + typeChanged: 2, + inventoryComplete: false, + unprojectableFiles: 1, + }); + expect(payload.scope).toEqual({ + status: "package", + fromSubpath: "", + toSubpath: "", + pathPrefix: "src", + pathGlob: "src/**/*.ts", + }); + expect(payload.contentCoverage).toBe("partial"); + expect(payload.hasMoreFiles).toBe(true); + expect(payload.files.map((file) => file.path)).toEqual([ + "first\\x80.ts", + "second.ts", + ]); + }); + + it("preserves different package roots on each comparison side", () => { + const result = makeResult(); + result.raw.scope = { + status: "PACKAGE", + fromSubpath: "packages/old-name", + toSubpath: "packages/new-name", + }; + + const payload = buildCodeDiffSuccessPayload( + result, + options(packageTarget, "name-only"), + ); + + expect(payload.scope).toEqual({ + status: "package", + fromSubpath: "packages/old-name", + toSubpath: "packages/new-name", + }); + }); + + it("projects name-only with mandatory path encoding and no other fields", () => { + const payload = buildCodeDiffSuccessPayload( + makeResult(), + options(packageTarget, "name-only"), + ); + + expect(payload.files).toEqual([ + { path: "first\\x80.ts", pathEncoding: "byte_escaped" }, + { path: "second.ts", pathEncoding: "utf8" }, + ]); + }); + + it("projects name-status with lowercase status only", () => { + const payload = buildCodeDiffSuccessPayload( + makeResult(), + options(packageTarget, "name-status"), + ); + + expect(payload.files).toEqual([ + { + path: "first\\x80.ts", + pathEncoding: "byte_escaped", + status: "added", + }, + { path: "second.ts", pathEncoding: "utf8", status: "modified" }, + ]); + }); + + it("projects stat facts and omits patch-only fields", () => { + const result = makeResult(); + for (const file of result.raw.files) { + file.contentStatus = "STATS"; + file.patch = undefined; + file.contentOmissionReason = undefined; + } + const payload = buildCodeDiffSuccessPayload( + result, + options(packageTarget, "stat"), + ); + + expect(payload.files).toEqual([ + { + path: "first\\x80.ts", + pathEncoding: "byte_escaped", + status: "added", + modeChanged: true, + typeChanged: true, + additions: 0, + deletions: 2, + contentStatus: "stats", + }, + { + path: "second.ts", + pathEncoding: "utf8", + status: "modified", + modeChanged: false, + typeChanged: false, + additions: 4, + deletions: 1, + contentStatus: "stats", + }, + ]); + expect(Object.hasOwn(payload.files[0]!, "patch")).toBe(false); + expect(Object.hasOwn(payload.files[0]!, "contentSafety")).toBe(false); + }); + + it("projects realistic patch and omission facts with safety", () => { + const payload = buildCodeDiffSuccessPayload( + makeResult(), + options(packageTarget, "patch"), + ); + + expect(payload.files).toEqual([ + { + path: "first\\x80.ts", + pathEncoding: "byte_escaped", + status: "added", + modeChanged: true, + typeChanged: true, + additions: 0, + deletions: 2, + contentStatus: "omitted", + contentOmissionReason: "invalid_utf8", + contentSafety: { + filtered: true, + modifications: ["html_comments_stripped", "images_replaced"], + }, + }, + { + path: "second.ts", + pathEncoding: "utf8", + status: "modified", + modeChanged: false, + typeChanged: false, + additions: 4, + deletions: 1, + contentStatus: "patch", + patch: "@@ -1 +1 @@\n-old\n+new", + contentSafety: { filtered: false, modifications: [] }, + }, + ]); + }); + + it("preserves content failure fields for failed content coverage", () => { + const result = makeResult(); + result.raw.contentCoverage = "FAILED"; + result.raw.contentFailure = { + code: "RAW_DIFF_LIMIT_EXCEEDED", + retryable: false, + retryAfterMs: 0, + stage: "content", + limitKind: "max_content_entries", + }; + const unavailable = result.raw.files[1]; + if (!unavailable) throw new Error("Expected second fixture file."); + unavailable.contentStatus = "UNAVAILABLE"; + unavailable.additions = undefined; + unavailable.deletions = undefined; + unavailable.patch = undefined; + const payload = buildCodeDiffSuccessPayload(result, options()); + + expect(payload.contentFailure).toEqual({ + code: "RAW_DIFF_LIMIT_EXCEEDED", + retryable: false, + retryAfterMs: 0, + stage: "content", + limitKind: "max_content_entries", + }); + expect(payload.contentCoverage).toBe("failed"); + expect(payload.files[1]).toMatchObject({ + path: "second.ts", + contentStatus: "unavailable", + }); + }); + + it("supports empty identical inventory results and unknown scope", () => { + const result = makeResult(); + result.raw = { + ...result.raw, + summary: { + filesChanged: 0, + added: 0, + deleted: 0, + modified: 0, + modeChanged: 0, + typeChanged: 0, + inventoryComplete: true, + unprojectableFiles: 0, + }, + scope: { status: "UNKNOWN" }, + contentCoverage: "NOT_REQUESTED", + files: [], + hasMoreFiles: false, + }; + result.toResolution = { + ...result.fromResolution, + requested: "same", + }; + result.fromResolution = { ...result.toResolution }; + + const payload = buildCodeDiffSuccessPayload( + result, + options(repositoryTarget, "name-only"), + ); + + expect(payload.from.commitSha).toBe(payload.to.commitSha); + expect(payload.from.requested).toBe("same"); + expect(payload.to.requested).toBe("same"); + expect(payload.scope).toEqual({ status: "unknown" }); + expect(payload.contentCoverage).toBe("not_requested"); + expect(payload.files).toEqual([]); + }); +}); diff --git a/packages/mcp/src/shared/code-diff-response.ts b/packages/mcp/src/shared/code-diff-response.ts new file mode 100644 index 0000000..db70382 --- /dev/null +++ b/packages/mcp/src/shared/code-diff-response.ts @@ -0,0 +1,281 @@ +import type { + CodeDiffParams, + CodeDiffResult, + RawCodeDiffFile, +} from "@githits/core-internal"; +import type { CodeDiffView } from "./code-diff-request.js"; + +export type CodeDiffEnvelopeRefKind = + | "sha" + | "tag" + | "branch" + | "head" + | "unknown"; +export type CodeDiffEnvelopeVersionSource = + | "registry" + | "git_head" + | "tag" + | "release"; +export type CodeDiffEnvelopeScopeStatus = "package" | "repository" | "unknown"; +export type CodeDiffEnvelopeFileStatus = "added" | "deleted" | "modified"; +export type CodeDiffEnvelopePathEncoding = "utf8" | "byte_escaped"; +export type CodeDiffEnvelopeContentStatus = + | "not_requested" + | "stats" + | "patch" + | "binary" + | "metadata_only" + | "omitted" + | "unavailable"; +export type CodeDiffEnvelopeContentCoverage = + | "not_requested" + | "complete" + | "partial" + | "failed"; + +export interface LeanCodeDiffPackageTarget { + kind: "package"; + registry: string; + name: string; + repoUrl?: string; +} + +export interface LeanCodeDiffRepositoryTarget { + kind: "repository"; + repoUrl: string; +} + +export type LeanCodeDiffTarget = + | LeanCodeDiffPackageTarget + | LeanCodeDiffRepositoryTarget; + +export interface LeanCodeDiffResolution { + requested: string; + resolvedVersion?: string; + ref: string; + commitSha: string; + refKind: CodeDiffEnvelopeRefKind; + versionSource?: CodeDiffEnvelopeVersionSource; +} + +export interface LeanCodeDiffSummary { + filesChanged: number; + added: number; + deleted: number; + modified: number; + modeChanged: number; + typeChanged: number; + inventoryComplete: boolean; + unprojectableFiles: number; +} + +export interface LeanCodeDiffScope { + status: CodeDiffEnvelopeScopeStatus; + fromSubpath?: string; + toSubpath?: string; + pathPrefix?: string; + pathGlob?: string; +} + +export interface LeanCodeDiffContentFailure { + code: string; + retryable: boolean; + retryAfterMs?: number; + stage?: string; + limitKind?: string; +} + +export interface LeanCodeDiffContentSafety { + filtered: boolean; + modifications: string[]; +} + +export interface LeanCodeDiffFileBase { + path: string; + pathEncoding: CodeDiffEnvelopePathEncoding; +} + +export interface LeanCodeDiffNameOnlyFile extends LeanCodeDiffFileBase {} + +export interface LeanCodeDiffNameStatusFile extends LeanCodeDiffFileBase { + status: CodeDiffEnvelopeFileStatus; +} + +export interface LeanCodeDiffStatFile extends LeanCodeDiffNameStatusFile { + modeChanged: boolean; + typeChanged: boolean; + additions?: number; + deletions?: number; + contentStatus: CodeDiffEnvelopeContentStatus; +} + +export interface LeanCodeDiffPatchFile extends LeanCodeDiffStatFile { + patch?: string; + contentOmissionReason?: string; + contentSafety: LeanCodeDiffContentSafety; +} + +export type LeanCodeDiffFile = + | LeanCodeDiffNameOnlyFile + | LeanCodeDiffNameStatusFile + | LeanCodeDiffStatFile + | LeanCodeDiffPatchFile; + +export interface LeanCodeDiffEnvelope { + target: LeanCodeDiffTarget; + view: CodeDiffView; + from: LeanCodeDiffResolution; + to: LeanCodeDiffResolution; + summary: LeanCodeDiffSummary; + scope: LeanCodeDiffScope; + contentCoverage: CodeDiffEnvelopeContentCoverage; + contentFailure?: LeanCodeDiffContentFailure; + files: LeanCodeDiffFile[]; + hasMoreFiles: boolean; +} + +export interface BuildCodeDiffPayloadOptions { + target: CodeDiffParams["target"]; + view: CodeDiffView; +} + +export function buildCodeDiffSuccessPayload( + result: CodeDiffResult, + options: BuildCodeDiffPayloadOptions, +): LeanCodeDiffEnvelope { + const envelope: LeanCodeDiffEnvelope = { + target: buildTarget(result, options.target), + view: options.view, + from: projectResolution(result.fromResolution), + to: projectResolution(result.toResolution), + summary: projectSummary(result.raw.summary), + scope: projectScope(result.raw.scope), + contentCoverage: lower(result.raw.contentCoverage), + files: result.raw.files.map((file) => projectFile(file, options.view)), + hasMoreFiles: result.raw.hasMoreFiles, + }; + + if (result.raw.contentFailure != null) { + envelope.contentFailure = projectContentFailure(result.raw.contentFailure); + } + return envelope; +} + +function buildTarget( + result: CodeDiffResult, + target: CodeDiffParams["target"], +): LeanCodeDiffTarget { + if ("registry" in target) { + const packageInfo = result.package; + return { + kind: "package", + registry: lower(packageInfo?.registry ?? target.registry), + name: packageInfo?.name ?? target.packageName, + ...(packageInfo?.repoUrl != null ? { repoUrl: packageInfo.repoUrl } : {}), + }; + } + return { kind: "repository", repoUrl: target.repoUrl }; +} + +function projectResolution( + resolution: CodeDiffResult["fromResolution"], +): LeanCodeDiffResolution { + const projected: LeanCodeDiffResolution = { + requested: resolution.requested, + ref: resolution.ref, + commitSha: resolution.commitSha, + refKind: lower(resolution.refKind), + }; + if (resolution.resolvedVersion != null) { + projected.resolvedVersion = resolution.resolvedVersion; + } + if (resolution.versionSource != null) { + projected.versionSource = lower(resolution.versionSource); + } + return projected; +} + +function projectSummary( + summary: CodeDiffResult["raw"]["summary"], +): LeanCodeDiffSummary { + return { + filesChanged: summary.filesChanged, + added: summary.added, + deleted: summary.deleted, + modified: summary.modified, + modeChanged: summary.modeChanged, + typeChanged: summary.typeChanged, + inventoryComplete: summary.inventoryComplete, + unprojectableFiles: summary.unprojectableFiles, + }; +} + +function projectScope( + scope: CodeDiffResult["raw"]["scope"], +): LeanCodeDiffScope { + const projected: LeanCodeDiffScope = { status: lower(scope.status) }; + if (scope.fromSubpath != null) projected.fromSubpath = scope.fromSubpath; + if (scope.toSubpath != null) projected.toSubpath = scope.toSubpath; + if (scope.pathPrefix != null) projected.pathPrefix = scope.pathPrefix; + if (scope.pathGlob != null) projected.pathGlob = scope.pathGlob; + return projected; +} + +function projectContentFailure( + failure: NonNullable, +): LeanCodeDiffContentFailure { + const projected: LeanCodeDiffContentFailure = { + code: failure.code, + retryable: failure.retryable, + }; + if (failure.retryAfterMs != null) { + projected.retryAfterMs = failure.retryAfterMs; + } + if (failure.stage != null) projected.stage = failure.stage; + if (failure.limitKind != null) projected.limitKind = failure.limitKind; + return projected; +} + +function projectFile( + file: RawCodeDiffFile, + view: CodeDiffView, +): LeanCodeDiffFile { + const base: LeanCodeDiffFileBase = { + path: file.path, + pathEncoding: lower(file.pathEncoding), + }; + if (view === "name-only") return base; + + const nameStatus: LeanCodeDiffNameStatusFile = { + ...base, + status: lower(file.status), + }; + if (view === "name-status") return nameStatus; + + const stat: LeanCodeDiffStatFile = { + ...nameStatus, + modeChanged: file.modeChanged, + typeChanged: file.typeChanged, + contentStatus: lower(file.contentStatus), + }; + if (file.additions != null) stat.additions = file.additions; + if (file.deletions != null) stat.deletions = file.deletions; + if (view === "stat") return stat; + + const patch: LeanCodeDiffPatchFile = { + ...stat, + contentSafety: { + filtered: file.contentSafety.filtered, + modifications: file.contentSafety.modifications.map(lower), + }, + }; + if (file.patch != null) patch.patch = file.patch; + if (file.contentOmissionReason != null) { + patch.contentOmissionReason = file.contentOmissionReason; + } + return patch; +} + +function lower(value: T): Lowercase { + return value.toLowerCase() as Lowercase; +} From c3c5981e32ebf5497ded05211c0efb3bba9658e2 Mon Sep 17 00:00:00 2001 From: Juha Litola Date: Mon, 17 Aug 2026 15:00:17 +0300 Subject: [PATCH 04/19] feat: map CodeDiff CLI errors Classify bounded CodeDiff resolver failures into the existing CLI error contract while retaining safe recovery metadata and exact partial resolutions. --- .../shared/code-navigation-error-map.test.ts | 123 ++++++++++++++++++ .../src/shared/code-navigation-error-map.ts | 92 +++++++++++++ 2 files changed, 215 insertions(+) diff --git a/packages/mcp/src/shared/code-navigation-error-map.test.ts b/packages/mcp/src/shared/code-navigation-error-map.test.ts index 6e15834..9b64bc2 100644 --- a/packages/mcp/src/shared/code-navigation-error-map.test.ts +++ b/packages/mcp/src/shared/code-navigation-error-map.test.ts @@ -2,6 +2,7 @@ import { afterEach, beforeEach, describe, expect, it, spyOn } from "bun:test"; import { AuthenticationError, ClientUpdateRequiredError, + CodeDiffError, CodeNavigationAccessError, CodeNavigationBackendError, CodeNavigationFeatureFlagRequiredError, @@ -64,6 +65,127 @@ describe("mapCodeNavigationError", () => { }); }); + it("classifies CodeDiff version failures with bounded recovery details", () => { + const error = new CodeDiffError("Version was not published.", { + code: "VERSION_NOT_FOUND", + retryable: false, + side: "from", + registry: "NPM", + publishedVersions: ["2.0.0", "1.0.0"], + publishedVersionsTruncated: true, + }); + + expect(mapCodeNavigationError(error)).toEqual({ + code: "VERSION_NOT_FOUND", + message: "Version was not published.", + retryable: false, + details: { + side: "from", + registry: "NPM", + publishedVersions: ["2.0.0", "1.0.0"], + publishedVersionsTruncated: true, + }, + }); + }); + + it("classifies ambiguous CodeDiff refs without exposing the raw code", () => { + const error = new CodeDiffError("Ref is ambiguous.", { + code: "AMBIGUOUS_REF", + repoUrl: "https://github.com/example/repo", + gitRef: "release", + availableRefs: [{ ref: "refs/heads/release" }], + suggestedRefs: [{ ref: "refs/tags/release", version: "1.0.0" }], + refKinds: ["BRANCH", "TAG"], + }); + + expect(mapCodeNavigationError(error)).toEqual({ + code: "REF_NOT_FOUND", + message: "Ref is ambiguous.", + retryable: false, + details: { + repoUrl: "https://github.com/example/repo", + gitRef: "release", + availableRefs: [{ ref: "refs/heads/release" }], + suggestedRefs: [{ ref: "refs/tags/release", version: "1.0.0" }], + refKinds: ["BRANCH", "TAG"], + }, + }); + }); + + it("preserves exact root identity for a CodeDiff raw-field failure", () => { + const error = new CodeDiffError( + "Raw diff limit exceeded.", + { + code: "RAW_DIFF_LIMIT_EXCEEDED", + retryable: false, + stage: "content", + limitKind: "max_content_entries", + }, + { + package: { + registry: "NPM", + name: "example", + repoUrl: "https://github.com/example/repo", + }, + fromResolution: { + requested: "1.0.0", + resolvedVersion: "1.0.0", + ref: "v1.0.0", + commitSha: "0123456789abcdef0123456789abcdef01234567", + refKind: "TAG", + versionSource: "REGISTRY", + }, + toResolution: { + requested: "2.0.0", + resolvedVersion: "2.0.0", + ref: "v2.0.0", + commitSha: "fedcba9876543210fedcba9876543210fedcba98", + refKind: "TAG", + versionSource: "REGISTRY", + }, + }, + ); + + const mapped = mapCodeNavigationError(error); + expect(mapped.code).toBe("BACKEND_ERROR"); + expect(mapped.retryable).toBe(false); + expect(mapped.details).toMatchObject({ + stage: "content", + limitKind: "max_content_entries", + codeDiffResolution: { + package: { registry: "NPM", name: "example" }, + from: { commitSha: "0123456789abcdef0123456789abcdef01234567" }, + to: { commitSha: "fedcba9876543210fedcba9876543210fedcba98" }, + }, + }); + expect(mapped.details).not.toHaveProperty("graphqlCode"); + }); + + it.each([ + ["VALIDATION_ERROR", "INVALID_ARGUMENT", false], + ["REF_NOT_FOUND", "REF_NOT_FOUND", false], + ["REPOSITORY_NOT_FOUND", "NOT_FOUND", false], + ["TIMEOUT", "TIMEOUT", true], + ["RATE_LIMITED", "RATE_LIMITED", true], + ["RAW_DIFF_UNAVAILABLE", "BACKEND_ERROR", false], + [undefined, "BACKEND_ERROR", false], + ] as const)( + "maps CodeDiff code %s to %s", + (graphqlCode, expectedCode, retryable) => { + const mapped = mapCodeNavigationError( + new CodeDiffError("failure", { + code: graphqlCode, + retryAfterMs: 250, + }), + ); + expect(mapped).toMatchObject({ + code: expectedCode, + retryable, + details: { retryAfterMs: 250 }, + }); + }, + ); + it("classifies CodeNavigationTargetNotFoundError as NOT_FOUND", () => { const err = new CodeNavigationTargetNotFoundError("Package not found", [ { version: "5.2.1", ref: "v5.2.1" }, @@ -590,6 +712,7 @@ describe("mapCodeNavigationError debug instrumentation", () => { process.env.GITHITS_DEBUG = "*"; const errors: unknown[] = [ new CodeNavigationTargetNotFoundError("x"), + new CodeDiffError("x", { code: "RAW_DIFF_UNAVAILABLE" }), new CodeNavigationFileNotFoundError("x", "some/path"), new CodeNavigationIndexingError("x"), new CodeNavigationRefNotFoundError( diff --git a/packages/mcp/src/shared/code-navigation-error-map.ts b/packages/mcp/src/shared/code-navigation-error-map.ts index 11ac24f..06d3b49 100644 --- a/packages/mcp/src/shared/code-navigation-error-map.ts +++ b/packages/mcp/src/shared/code-navigation-error-map.ts @@ -5,6 +5,9 @@ import { type AvailableVersion, CLIENT_UPDATE_REQUIRED_REASON, ClientUpdateRequiredError, + CodeDiffError, + type CodeDiffPackageInfo, + type CodeDiffRefResolution, CodeNavigationAccessError, CodeNavigationBackendError, type CodeNavigationErrorMetadata, @@ -91,6 +94,28 @@ export interface MappedErrorDetails { termsUrl?: string; /** Authenticated web UI where the user can accept the current terms. */ acceptanceUrl?: string; + /** CodeDiff comparison side involved in a bounded resolver failure. */ + side?: string; + /** Published package versions supplied for CodeDiff recovery. */ + publishedVersions?: string[]; + publishedVersionsTruncated?: boolean; + /** Canonical registry supplied by a CodeDiff resolver failure. */ + registry?: string; + /** Retry delay retained at millisecond precision for CodeDiff. */ + retryAfterMs?: number; + /** Bounded raw-diff failure stage and limit identifier. */ + stage?: string; + limitKind?: string; + /** Git ref supplied by a CodeDiff resolver failure. */ + gitRef?: string; + /** Ref classifications retained for ambiguous-ref recovery. */ + refKinds?: string[]; + /** Immutable root identity retained when only the raw field failed. */ + codeDiffResolution?: { + package?: CodeDiffPackageInfo; + from: CodeDiffRefResolution; + to: CodeDiffRefResolution; + }; } export interface MappedError { @@ -155,6 +180,9 @@ function classify(error: unknown): MappedError { if (error instanceof ClientUpdateRequiredError) { return buildUpdateRequiredError(error.reason, error.currentVersion); } + if (error instanceof CodeDiffError) { + return classifyCodeDiffError(error); + } if (error instanceof CodeNavigationVersionNotFoundError) { const details: MappedErrorDetails = {}; preserveBackendMetadata(details, error.metadata); @@ -311,6 +339,70 @@ function classify(error: unknown): MappedError { return { code: "UNKNOWN", message: "Unknown error", retryable: false }; } +function classifyCodeDiffError(error: CodeDiffError): MappedError { + const details: MappedErrorDetails = {}; + const source = error.details; + + if (source?.side !== undefined) details.side = source.side; + if (source?.publishedVersions !== undefined) { + details.publishedVersions = source.publishedVersions; + } + if (source?.publishedVersionsTruncated !== undefined) { + details.publishedVersionsTruncated = source.publishedVersionsTruncated; + } + if (source?.registry !== undefined) details.registry = source.registry; + if (source?.retryAfterMs !== undefined) { + details.retryAfterMs = source.retryAfterMs; + } + if (source?.stage !== undefined) details.stage = source.stage; + if (source?.limitKind !== undefined) details.limitKind = source.limitKind; + if (source?.repoUrl !== undefined) details.repoUrl = source.repoUrl; + if (source?.gitRef !== undefined) details.gitRef = source.gitRef; + if (source?.availableRefs !== undefined) { + details.availableRefs = source.availableRefs.map((entry) => ({ ...entry })); + } + if (source?.suggestedRefs !== undefined) { + details.suggestedRefs = source.suggestedRefs.map((entry) => ({ ...entry })); + } + if (source?.refKinds !== undefined) details.refKinds = source.refKinds; + if (error.partial !== undefined) { + details.codeDiffResolution = { + package: error.partial.package, + from: error.partial.fromResolution, + to: error.partial.toResolution, + }; + } + + const build = ( + code: MappedErrorCode, + defaultRetryable: boolean, + ): MappedError => ({ + code, + message: error.message, + retryable: source?.retryable ?? defaultRetryable, + details: Object.keys(details).length > 0 ? details : undefined, + }); + + switch (source?.code) { + case "VALIDATION_ERROR": + return build("INVALID_ARGUMENT", false); + case "VERSION_NOT_FOUND": + return build("VERSION_NOT_FOUND", false); + case "REF_NOT_FOUND": + case "AMBIGUOUS_REF": + return build("REF_NOT_FOUND", false); + case "REPOSITORY_NOT_FOUND": + case "PACKAGE_NOT_FOUND": + return build("NOT_FOUND", false); + case "TIMEOUT": + return build("TIMEOUT", true); + case "RATE_LIMITED": + return build("RATE_LIMITED", true); + default: + return build("BACKEND_ERROR", false); + } +} + export function buildUpdateRequiredError( reason: string = CLIENT_UPDATE_REQUIRED_REASON, currentVersion?: string, From d5838606aced9184c9bdfbeea10ce4566b62af0a Mon Sep 17 00:00:00 2001 From: Juha Litola Date: Mon, 17 Aug 2026 15:04:02 +0300 Subject: [PATCH 05/19] feat: format CodeDiff CLI output Render Git-like diff views while keeping completeness and safety diagnostics on stderr. --- packages/mcp/src/internal.ts | 1 + .../mcp/src/shared/code-diff-text.test.ts | 244 ++++++++++++++++ packages/mcp/src/shared/code-diff-text.ts | 266 ++++++++++++++++++ 3 files changed, 511 insertions(+) create mode 100644 packages/mcp/src/shared/code-diff-text.test.ts create mode 100644 packages/mcp/src/shared/code-diff-text.ts diff --git a/packages/mcp/src/internal.ts b/packages/mcp/src/internal.ts index 7f056b8..747fffa 100644 --- a/packages/mcp/src/internal.ts +++ b/packages/mcp/src/internal.ts @@ -2,6 +2,7 @@ export * from "./index.js"; export { getMcpToolDefinitions } from "./mcp/server.js"; export * from "./shared/code-diff-request.js"; export * from "./shared/code-diff-response.js"; +export * from "./shared/code-diff-text.js"; export * from "./shared/code-navigation.js"; export * from "./shared/code-navigation-defaults.js"; export * from "./shared/code-navigation-error-map.js"; diff --git a/packages/mcp/src/shared/code-diff-text.test.ts b/packages/mcp/src/shared/code-diff-text.test.ts new file mode 100644 index 0000000..ec52d09 --- /dev/null +++ b/packages/mcp/src/shared/code-diff-text.test.ts @@ -0,0 +1,244 @@ +import { describe, expect, it } from "bun:test"; +import type { LeanCodeDiffEnvelope } from "./code-diff-response.js"; +import { formatCodeDiffTerminal } from "./code-diff-text.js"; + +function envelope( + overrides: Partial = {}, +): LeanCodeDiffEnvelope { + return { + target: { kind: "package", registry: "npm", name: "example" }, + view: "patch", + from: { + requested: "1.0.0", + resolvedVersion: "1.0.0", + ref: "v1.0.0", + commitSha: "0123456789abcdef0123456789abcdef01234567", + refKind: "tag", + versionSource: "registry", + }, + to: { + requested: "2.0.0", + resolvedVersion: "2.0.0", + ref: "v2.0.0", + commitSha: "fedcba9876543210fedcba9876543210fedcba98", + refKind: "tag", + versionSource: "registry", + }, + summary: { + filesChanged: 2, + added: 1, + deleted: 0, + modified: 1, + modeChanged: 0, + typeChanged: 0, + inventoryComplete: true, + unprojectableFiles: 0, + }, + scope: { status: "package", fromSubpath: "", toSubpath: "" }, + contentCoverage: "complete", + files: [], + hasMoreFiles: false, + ...overrides, + }; +} + +const options = { useColors: false } as const; + +describe("formatCodeDiffTerminal", () => { + it("renders name-only as one sanitized path per line", () => { + const result = formatCodeDiffTerminal( + envelope({ + view: "name-only", + contentCoverage: "not_requested", + files: [ + { path: "src/a.ts", pathEncoding: "utf8" }, + { path: "bad\u001b[31m.ts", pathEncoding: "utf8" }, + ], + }), + options, + ); + + expect(result).toEqual({ stdout: "src/a.ts\nbad.ts\n", stderr: undefined }); + }); + + it("renders name-status with Git status letters", () => { + const result = formatCodeDiffTerminal( + envelope({ + view: "name-status", + contentCoverage: "not_requested", + files: [ + { path: "a.ts", pathEncoding: "utf8", status: "added" }, + { path: "d.ts", pathEncoding: "utf8", status: "deleted" }, + { path: "m.ts", pathEncoding: "utf8", status: "modified" }, + ], + }), + options, + ); + + expect(result.stdout).toBe("A\ta.ts\nD\td.ts\nM\tm.ts\n"); + }); + + it("renders returned stat rows without claiming full-inventory line totals", () => { + const result = formatCodeDiffTerminal( + envelope({ + view: "stat", + files: [ + { + path: "text.ts", + pathEncoding: "utf8", + status: "modified", + modeChanged: false, + typeChanged: false, + additions: 4, + deletions: 2, + contentStatus: "stats", + }, + { + path: "image.png", + pathEncoding: "utf8", + status: "modified", + modeChanged: false, + typeChanged: false, + contentStatus: "binary", + }, + ], + }), + options, + ); + + expect(result.stdout).toBe( + "text.ts | +4 -2\nimage.png | binary content differs\n2 returned files, +4 -2\n", + ); + }); + + it("preserves served patches and renders truthful non-text outcomes", () => { + const result = formatCodeDiffTerminal( + envelope({ + files: [ + { + path: "a.ts", + pathEncoding: "utf8", + status: "modified", + modeChanged: false, + typeChanged: false, + additions: 1, + deletions: 1, + patch: "@@ -1 +1 @@\n-old\n+new\n", + contentStatus: "patch", + contentSafety: { filtered: false, modifications: [] }, + }, + { + path: "image.png", + pathEncoding: "utf8", + status: "modified", + modeChanged: false, + typeChanged: false, + contentStatus: "binary", + contentSafety: { filtered: false, modifications: [] }, + }, + { + path: "mode.sh", + pathEncoding: "utf8", + status: "modified", + modeChanged: true, + typeChanged: false, + contentStatus: "metadata_only", + contentSafety: { filtered: false, modifications: [] }, + }, + { + path: "large.ts", + pathEncoding: "utf8", + status: "modified", + modeChanged: false, + typeChanged: false, + contentStatus: "omitted", + contentOmissionReason: "content_budget\u001b[31m", + contentSafety: { filtered: false, modifications: [] }, + }, + ], + }), + options, + ); + + expect(result.stdout).toBe( + "@@ -1 +1 @@\n-old\n+new\nBinary file image.png differs\nMetadata changed: mode.sh\nPatch omitted: large.ts (content_budget)\n", + ); + }); + + it("keeps an empty authoritative diff silent in plain mode", () => { + expect(formatCodeDiffTerminal(envelope({ files: [] }), options)).toEqual({ + stdout: "", + stderr: undefined, + }); + }); + + it("renders completeness, scope, encoding, safety, and content warnings", () => { + const result = formatCodeDiffTerminal( + envelope({ + summary: { + filesChanged: 8, + added: 2, + deleted: 2, + modified: 4, + modeChanged: 1, + typeChanged: 1, + inventoryComplete: false, + unprojectableFiles: 2, + }, + scope: { status: "unknown" }, + contentCoverage: "failed", + contentFailure: { + code: "RAW_DIFF_LIMIT_EXCEEDED", + retryable: false, + stage: "content", + limitKind: "max_content_entries", + }, + files: [ + { + path: "bad\\x80.ts", + pathEncoding: "byte_escaped", + status: "modified", + modeChanged: false, + typeChanged: false, + contentStatus: "unavailable", + contentSafety: { + filtered: true, + modifications: ["invisible_controls_stripped"], + }, + }, + ], + hasMoreFiles: true, + }), + options, + ); + + expect(result.stderr).toContain("repository-wide"); + expect(result.stderr).toContain("inventory is incomplete"); + expect(result.stderr).toContain("More matching files"); + expect(result.stderr).toContain("2 matching path(s)"); + expect(result.stderr).toContain("Requested content failed"); + expect(result.stderr).toContain("display-only byte escapes"); + expect(result.stderr).toContain("modified for content safety"); + }); + + it("adds full exact identity and scope facts only in verbose diagnostics", () => { + const result = formatCodeDiffTerminal( + envelope({ + scope: { + status: "package", + fromSubpath: "packages/old", + toSubpath: "packages/new", + pathGlob: "packages/**/*.ts", + }, + }), + { useColors: false, verbose: true }, + ); + + expect(result.stdout).toBe(""); + expect(result.stderr).toContain("target: npm:example"); + expect(result.stderr).toContain("0123456789abcdef0123456789abcdef01234567"); + expect(result.stderr).toContain("fedcba9876543210fedcba9876543210fedcba98"); + expect(result.stderr).toContain('roots "packages/old" -> "packages/new"'); + expect(result.stderr).toContain('glob "packages/**/*.ts"'); + }); +}); diff --git a/packages/mcp/src/shared/code-diff-text.ts b/packages/mcp/src/shared/code-diff-text.ts new file mode 100644 index 0000000..74058fa --- /dev/null +++ b/packages/mcp/src/shared/code-diff-text.ts @@ -0,0 +1,266 @@ +import type { + LeanCodeDiffEnvelope, + LeanCodeDiffFile, + LeanCodeDiffPatchFile, + LeanCodeDiffStatFile, +} from "./code-diff-response.js"; +import { warning } from "./colors.js"; +import { sanitizeTerminalText } from "./resolve-target-response.js"; + +export interface FormatCodeDiffTerminalOptions { + useColors: boolean; + verbose?: boolean; +} + +export interface FormattedCodeDiffTerminal { + stdout: string; + stderr?: string; +} + +/** Render a Git-like primary stream plus truthful bounded-evidence diagnostics. */ +export function formatCodeDiffTerminal( + envelope: LeanCodeDiffEnvelope, + options: FormatCodeDiffTerminalOptions, +): FormattedCodeDiffTerminal { + const stdout = formatPrimaryOutput(envelope); + const diagnostics = buildDiagnostics(envelope, options); + return { + stdout, + stderr: diagnostics.length > 0 ? `${diagnostics.join("\n")}\n` : undefined, + }; +} + +function formatPrimaryOutput(envelope: LeanCodeDiffEnvelope): string { + switch (envelope.view) { + case "name-only": + return formatLines(envelope.files.map((file) => safe(file.path))); + case "name-status": + return formatLines( + envelope.files.map( + (file) => `${statusLetter(requireStatus(file))}\t${safe(file.path)}`, + ), + ); + case "stat": + return formatStat(envelope); + case "patch": + return formatPatches(envelope.files); + } +} + +function formatStat(envelope: LeanCodeDiffEnvelope): string { + const lines: string[] = []; + let additions = 0; + let deletions = 0; + + for (const file of envelope.files) { + const stat = requireStat(file); + const path = safe(stat.path); + if (stat.additions !== undefined && stat.deletions !== undefined) { + lines.push(`${path} | +${stat.additions} -${stat.deletions}`); + additions += stat.additions; + deletions += stat.deletions; + continue; + } + lines.push(`${path} | ${contentLabel(stat.contentStatus)}`); + } + + if (lines.length > 0) { + const noun = lines.length === 1 ? "file" : "files"; + lines.push(`${lines.length} returned ${noun}, +${additions} -${deletions}`); + } + return formatLines(lines); +} + +function formatPatches(files: LeanCodeDiffFile[]): string { + let output = ""; + for (const file of files) { + const patchFile = requirePatch(file); + if (patchFile.patch !== undefined) { + output += patchFile.patch; + if (!patchFile.patch.endsWith("\n")) output += "\n"; + continue; + } + output += `${patchFallback(patchFile)}\n`; + } + return output; +} + +function patchFallback(file: LeanCodeDiffPatchFile): string { + const path = safe(file.path); + switch (file.contentStatus) { + case "binary": + return `Binary file ${path} differs`; + case "metadata_only": + return `Metadata changed: ${path}`; + case "omitted": { + const reason = file.contentOmissionReason + ? ` (${safe(file.contentOmissionReason)})` + : ""; + return `Patch omitted: ${path}${reason}`; + } + case "unavailable": + return `Patch unavailable: ${path}`; + default: + return `No textual patch: ${path}`; + } +} + +function buildDiagnostics( + envelope: LeanCodeDiffEnvelope, + options: FormatCodeDiffTerminalOptions, +): string[] { + const lines: string[] = []; + if (options.verbose) appendVerboseContext(lines, envelope); + + if (envelope.scope.status === "unknown") { + lines.push( + warn( + "Package ownership could not be proved; this evidence is repository-wide.", + options, + ), + ); + } + if (!envelope.summary.inventoryComplete) { + lines.push( + warn("The authoritative file inventory is incomplete.", options), + ); + } + if (envelope.hasMoreFiles) { + lines.push( + warn( + `More matching files exist than the ${envelope.files.length} returned. Narrow the glob or raise --max-files.`, + options, + ), + ); + } + if (envelope.summary.unprojectableFiles > 0) { + lines.push( + warn( + `${envelope.summary.unprojectableFiles} matching path(s) could not be projected safely.`, + options, + ), + ); + } + if (envelope.contentCoverage === "partial") { + lines.push( + warn("Requested content is partial; inspect per-file status.", options), + ); + } else if (envelope.contentCoverage === "failed") { + const failure = envelope.contentFailure; + const detail = failure + ? ` (${[failure.code, failure.stage, failure.limitKind] + .filter((value): value is string => Boolean(value)) + .map(safe) + .join(", ")})` + : ""; + lines.push( + warn( + `Requested content failed after the file inventory completed${detail}.`, + options, + ), + ); + } + + const byteEscaped = envelope.files.filter( + (file) => file.pathEncoding === "byte_escaped", + ).length; + if (byteEscaped > 0) { + lines.push( + warn( + `${byteEscaped} path(s) are display-only byte escapes and cannot be reused as exact identities.`, + options, + ), + ); + } + const filtered = envelope.files.filter( + (file) => "contentSafety" in file && file.contentSafety.filtered, + ).length; + if (filtered > 0) { + lines.push( + warn(`${filtered} patch(es) were modified for content safety.`, options), + ); + } + return lines; +} + +function appendVerboseContext( + lines: string[], + envelope: LeanCodeDiffEnvelope, +): void { + const target = + envelope.target.kind === "package" + ? `${envelope.target.registry}:${safe(envelope.target.name)}` + : safe(envelope.target.repoUrl); + lines.push(`target: ${target}`); + lines.push( + `range: ${safe(envelope.from.requested)} (${envelope.from.commitSha}) -> ${safe(envelope.to.requested)} (${envelope.to.commitSha})`, + ); + lines.push( + `summary: ${envelope.summary.filesChanged} changed, ${envelope.summary.added} added, ${envelope.summary.deleted} deleted, ${envelope.summary.modified} modified`, + ); + const roots = + envelope.scope.fromSubpath !== undefined || + envelope.scope.toSubpath !== undefined + ? `, roots ${JSON.stringify(envelope.scope.fromSubpath ?? "?")} -> ${JSON.stringify(envelope.scope.toSubpath ?? "?")}` + : ""; + const filter = envelope.scope.pathGlob + ? `, glob ${JSON.stringify(safe(envelope.scope.pathGlob))}` + : ""; + lines.push(`scope: ${envelope.scope.status}${roots}${filter}`); + lines.push( + `returned: ${envelope.files.length}, content: ${envelope.contentCoverage}`, + ); +} + +function requireStatus( + file: LeanCodeDiffFile, +): "added" | "deleted" | "modified" { + if (!("status" in file)) + throw new Error("CodeDiff status view lacks status."); + return file.status; +} + +function requireStat(file: LeanCodeDiffFile): LeanCodeDiffStatFile { + if (!("contentStatus" in file)) { + throw new Error("CodeDiff stat view lacks content status."); + } + return file; +} + +function requirePatch(file: LeanCodeDiffFile): LeanCodeDiffPatchFile { + if (!("contentSafety" in file)) { + throw new Error("CodeDiff patch view lacks content safety."); + } + return file; +} + +function statusLetter(status: "added" | "deleted" | "modified"): string { + return status === "added" ? "A" : status === "deleted" ? "D" : "M"; +} + +function contentLabel(status: LeanCodeDiffStatFile["contentStatus"]): string { + switch (status) { + case "binary": + return "binary content differs"; + case "metadata_only": + return "metadata differs"; + case "omitted": + return "content omitted"; + case "unavailable": + return "content unavailable"; + default: + return "line statistics unavailable"; + } +} + +function formatLines(lines: string[]): string { + return lines.length > 0 ? `${lines.join("\n")}\n` : ""; +} + +function warn(text: string, options: FormatCodeDiffTerminalOptions): string { + return warning(text, options.useColors); +} + +function safe(value: string): string { + return sanitizeTerminalText(value); +} From 721ecf073126b5aab7f3a3eafadb9949fd725268 Mon Sep 17 00:00:00 2001 From: Juha Litola Date: Mon, 17 Aug 2026 15:07:48 +0300 Subject: [PATCH 06/19] feat: add silent CodeDiff CLI command Register a Git-like code diff surface for CLI dogfooding without adding MCP or agent-facing guidance. --- src/commands/code/diff.test.ts | 237 ++++++++++++++++++++++++++++ src/commands/code/diff.ts | 263 ++++++++++++++++++++++++++++++++ src/commands/code/index.test.ts | 3 + src/commands/code/index.ts | 4 +- 4 files changed, 506 insertions(+), 1 deletion(-) create mode 100644 src/commands/code/diff.test.ts create mode 100644 src/commands/code/diff.ts diff --git a/src/commands/code/diff.test.ts b/src/commands/code/diff.test.ts new file mode 100644 index 0000000..7f014d6 --- /dev/null +++ b/src/commands/code/diff.test.ts @@ -0,0 +1,237 @@ +import { describe, expect, it, mock, spyOn } from "bun:test"; +import { Command } from "commander"; +import { + createMockCodeNavigationService, + defaultCodeDiffResult, +} from "../../services/test-helpers.js"; +import { + type CodeDiffCommandDependencies, + codeDiffAction, + formatCodeDiffError, + registerCodeDiffCommand, +} from "./diff.js"; + +function dependencies( + overrides: Partial = {}, +): CodeDiffCommandDependencies { + return { + codeNavigationService: createMockCodeNavigationService(), + codeNavigationUrl: "https://pkgseer.dev/graphql", + hasValidToken: true, + mcpUrl: "https://mcp.githits.com", + ...overrides, + }; +} + +describe("codeDiffAction", () => { + it("uses patch mode by default and omits backend defaults", async () => { + const codeDiff = mock(() => Promise.resolve(defaultCodeDiffResult)); + const stdout = spyOn(process.stdout, "write").mockImplementation( + (() => true) as typeof process.stdout.write, + ); + + await codeDiffAction( + "npm:express", + "4.18.1..4.18.2", + undefined, + {}, + dependencies({ + codeNavigationService: createMockCodeNavigationService({ codeDiff }), + }), + ); + + expect(codeDiff).toHaveBeenCalledWith({ + target: { registry: "NPM", packageName: "express" }, + from: "4.18.1", + to: "4.18.2", + mode: "patches", + }); + expect(String(stdout.mock.calls[0]?.[0])).toContain("@@ -1 +1 @@"); + stdout.mockRestore(); + }); + + it("maps repo mode, inventory view, bounds, and one positional glob", async () => { + const codeDiff = mock(() => Promise.resolve(defaultCodeDiffResult)); + const stdout = spyOn(process.stdout, "write").mockImplementation( + (() => true) as typeof process.stdout.write, + ); + + await codeDiffAction( + "v1..v2", + "src/**/*.ts", + undefined, + { + repoUrl: "https://github.com/expressjs/express", + nameStatus: true, + maxFiles: "12", + }, + dependencies({ + codeNavigationService: createMockCodeNavigationService({ codeDiff }), + }), + ); + + expect(codeDiff).toHaveBeenCalledWith({ + target: { repoUrl: "https://github.com/expressjs/express" }, + from: "v1", + to: "v2", + mode: "inventory", + options: { maxFiles: 12, pathGlob: "src/**/*.ts" }, + }); + expect(String(stdout.mock.calls[0]?.[0])).toBe("M\tlib/express.js\n"); + stdout.mockRestore(); + }); + + it("emits the selected-view JSON envelope", async () => { + const log = spyOn(console, "log").mockImplementation(() => {}); + + await codeDiffAction( + "npm:express", + "4.18.1..4.18.2", + undefined, + { nameOnly: true, json: true }, + dependencies(), + ); + + const payload = JSON.parse(log.mock.calls[0]?.[0] as string); + expect(payload.view).toBe("name-only"); + expect(payload.from.commitSha).toBe("from-sha"); + expect(payload.files).toEqual([ + { path: "lib/express.js", pathEncoding: "utf8" }, + ]); + log.mockRestore(); + }); + + it("keeps failed post-inventory content as successful evidence", async () => { + const stdout = spyOn(process.stdout, "write").mockImplementation( + (() => true) as typeof process.stdout.write, + ); + const stderr = spyOn(process.stderr, "write").mockImplementation( + (() => true) as typeof process.stderr.write, + ); + const result = structuredClone(defaultCodeDiffResult); + result.raw.contentCoverage = "FAILED"; + result.raw.contentFailure = { + code: "RAW_DIFF_LIMIT_EXCEEDED", + retryable: false, + stage: "content", + limitKind: "max_content_entries", + }; + result.raw.files[0] = { + ...result.raw.files[0]!, + patch: undefined, + additions: undefined, + deletions: undefined, + contentStatus: "UNAVAILABLE", + }; + + await codeDiffAction( + "npm:express", + "4.18.1..4.18.2", + undefined, + {}, + dependencies({ + codeNavigationService: createMockCodeNavigationService({ + codeDiff: mock(() => Promise.resolve(result)), + }), + }), + ); + + expect(String(stderr.mock.calls[0]?.[0])).toContain( + "Requested content failed", + ); + stdout.mockRestore(); + stderr.mockRestore(); + }); + + it.each([ + [{ patch: true, stat: true }, "Choose only one diff view"], + [{ stat: true, maxPatchBytes: "2048" }, "valid only"], + [{ maxFiles: "0" }, "--max-files expects"], + ] as const)( + "rejects invalid options before network I/O", + async (options, text) => { + const codeDiff = mock(() => Promise.resolve(defaultCodeDiffResult)); + const error = spyOn(console, "error").mockImplementation(() => {}); + const exit = spyOn(process, "exit").mockImplementation(() => { + throw new Error("process.exit"); + }); + + try { + await codeDiffAction( + "npm:express", + "1.0.0..2.0.0", + undefined, + options, + dependencies({ + codeNavigationService: createMockCodeNavigationService({ + codeDiff, + }), + }), + ); + } catch { + // process.exit is mocked as a throw. + } + + expect(error.mock.calls[0]?.[0]).toContain(text); + expect(codeDiff).not.toHaveBeenCalled(); + error.mockRestore(); + exit.mockRestore(); + }, + ); + + it("rejects a third positional in repo mode", async () => { + const error = spyOn(console, "error").mockImplementation(() => {}); + const exit = spyOn(process, "exit").mockImplementation(() => { + throw new Error("process.exit"); + }); + try { + await codeDiffAction( + "v1..v2", + "src/**", + "extra", + { repoUrl: "https://github.com/x/y" }, + dependencies(), + ); + } catch { + // process.exit is mocked as a throw. + } + expect(error.mock.calls[0]?.[0]).toContain("at most one"); + error.mockRestore(); + exit.mockRestore(); + }); +}); + +describe("formatCodeDiffError", () => { + it("renders bounded recovery fields", () => { + const output = formatCodeDiffError({ + code: "VERSION_NOT_FOUND", + message: "Version was not found.", + retryable: false, + details: { + side: "from", + publishedVersions: ["2.0.0", "1.0.0"], + publishedVersionsTruncated: true, + stage: "resolution", + }, + }); + + expect(output).toContain("side: from"); + expect(output).toContain("stage: resolution"); + expect(output).toContain("2.0.0, 1.0.0, …"); + }); +}); + +describe("registerCodeDiffCommand", () => { + it("registers the dogfood surface without --git-ref", () => { + const parent = new Command("code"); + const command = registerCodeDiffCommand(parent); + + expect(command.name()).toBe("diff"); + expect( + command.options.some((option) => option.long === "--name-status"), + ).toBe(true); + expect(command.options.some((option) => option.long === "--git-ref")).toBe( + false, + ); + }); +}); diff --git a/src/commands/code/diff.ts b/src/commands/code/diff.ts new file mode 100644 index 0000000..83b6f4b --- /dev/null +++ b/src/commands/code/diff.ts @@ -0,0 +1,263 @@ +import type { CodeNavigationService } from "@githits/core-internal"; +import { + buildCodeDiffParams, + buildCodeDiffSuccessPayload, + type CodeDiffView, + formatCodeDiffTerminal, + InvalidPackageSpecError, + type MappedError, + requireAuth, + sanitizeTerminalText, + shouldUseColors, +} from "@githits/mcp/internal"; +import type { Command } from "commander"; +import { createContainer } from "../../container.js"; +import { startSpinner } from "../../shared/spinner.js"; +import { SPINNER_MESSAGES } from "../../shared/spinner-messages.js"; +import { formatMappedErrorForTerminal } from "../format-mapped-error.js"; +import { + handleCodeNavCommandError, + parseIntCliOption, +} from "./code-nav-cli-helpers.js"; + +export interface CodeDiffCommandOptions { + repoUrl?: string; + patch?: boolean; + stat?: boolean; + nameOnly?: boolean; + nameStatus?: boolean; + maxFiles?: string; + maxPatchBytes?: string; + verbose?: boolean; + json?: boolean; +} + +export interface CodeDiffCommandDependencies { + codeNavigationService: CodeNavigationService | undefined; + codeNavigationUrl: string | undefined; + hasValidToken: boolean; + mcpUrl: string; +} + +/** Execute the silent-dogfood CodeDiff CLI adapter. */ +export async function codeDiffAction( + arg1: string | undefined, + arg2: string | undefined, + arg3: string | undefined, + options: CodeDiffCommandOptions, + deps: CodeDiffCommandDependencies, +): Promise { + try { + requireAuth(deps); + } catch (error) { + if (options.json) { + handleCodeNavCommandError(error, true, formatCodeDiffError); + } + throw error; + } + + try { + if (!deps.codeNavigationUrl || !deps.codeNavigationService) { + throw new InvalidPackageSpecError( + "Code navigation is not configured for this environment.", + ); + } + + const positionals = resolvePositionals( + arg1, + arg2, + arg3, + options.repoUrl !== undefined, + ); + const view = resolveView(options); + const build = buildCodeDiffParams({ + target: positionals.target, + repoUrl: options.repoUrl, + range: positionals.range, + view, + pathGlob: positionals.pathGlob, + maxFiles: parseIntCliOption(options.maxFiles, "--max-files", 1, 300), + maxPatchBytes: parseIntCliOption( + options.maxPatchBytes, + "--max-patch-bytes", + 1024, + 2_097_152, + ), + }); + + const spinner = startSpinner(SPINNER_MESSAGES.code, !options.json); + const result = await deps.codeNavigationService + .codeDiff(build.params) + .finally(() => spinner.stop()); + const payload = buildCodeDiffSuccessPayload(result, { + target: build.params.target, + view: build.view, + }); + + if (options.json) { + console.log(JSON.stringify(payload)); + return; + } + + const formatted = formatCodeDiffTerminal(payload, { + useColors: shouldUseColors(), + verbose: options.verbose ?? false, + }); + if (formatted.stdout) process.stdout.write(formatted.stdout); + if (formatted.stderr) process.stderr.write(formatted.stderr); + } catch (error) { + handleCodeNavCommandError( + error, + options.json ?? false, + formatCodeDiffError, + ); + } +} + +interface ResolvedPositionals { + target?: string; + range: string; + pathGlob?: string; +} + +function resolvePositionals( + arg1: string | undefined, + arg2: string | undefined, + arg3: string | undefined, + repoMode: boolean, +): ResolvedPositionals { + if (repoMode) { + if (arg3 !== undefined) { + throw new InvalidPackageSpecError( + "Pass at most one repository-relative after `--`.", + ); + } + if (arg1 === undefined) { + throw new InvalidPackageSpecError( + "A .. range is required after --repo-url.", + ); + } + return { range: arg1, pathGlob: arg2 }; + } + + if (arg1 === undefined) { + throw new InvalidPackageSpecError( + "An unversioned and .. range are required.", + ); + } + if (arg2 === undefined) { + throw new InvalidPackageSpecError( + "A .. range is required after the target.", + ); + } + return { target: arg1, range: arg2, pathGlob: arg3 }; +} + +function resolveView(options: CodeDiffCommandOptions): CodeDiffView { + const selected: CodeDiffView[] = []; + if (options.patch) selected.push("patch"); + if (options.stat) selected.push("stat"); + if (options.nameOnly) selected.push("name-only"); + if (options.nameStatus) selected.push("name-status"); + if (selected.length > 1) { + throw new InvalidPackageSpecError( + "Choose only one diff view: --patch, --stat, --name-only, or --name-status.", + ); + } + return selected[0] ?? "patch"; +} + +/** Render bounded CodeDiff diagnostics without exposing raw GraphQL details. */ +export function formatCodeDiffError(mapped: MappedError): string { + const safe = (value: string): string => sanitizeTerminalText(value); + const lines = [safe(formatMappedErrorForTerminal(mapped))]; + const details = mapped.details; + if (!details) return lines[0] as string; + + if (details.side) lines.push(` side: ${safe(details.side)}`); + if (details.stage) lines.push(` stage: ${safe(details.stage)}`); + if (details.limitKind) lines.push(` limit: ${safe(details.limitKind)}`); + if (details.publishedVersions?.length) { + lines.push( + ` published versions: ${details.publishedVersions.slice(0, 8).map(safe).join(", ")}${details.publishedVersionsTruncated ? ", …" : ""}`, + ); + } + if (details.availableRefs?.length) { + lines.push( + ` available refs: ${details.availableRefs + .slice(0, 8) + .map((entry) => safe(entry.version ?? entry.ref)) + .join(", ")}`, + ); + } + if (details.suggestedRefs?.length) { + lines.push( + ` suggested refs: ${details.suggestedRefs + .slice(0, 8) + .map((entry) => safe(entry.version ?? entry.ref)) + .join(", ")}`, + ); + } + return lines.join("\n"); +} + +const CODE_DIFF_DESCRIPTION = `Compare two exact dependency source trees. + +The default output is a bounded patch, matching ordinary \`git diff\` where the +backend contract permits it. Select --stat, --name-only, or --name-status for +cheaper projections. Exactly one view may be selected. + +Addressing: an unversioned plus .., or --repo-url plus +... Versions and refs belong in the range, not the target. Direction +is always left-to-right; three-dot merge-base syntax is not supported. + +After \`--\`, one optional applies a repository-relative bounded +glob. It supports *, ?, and an exact ** path component; it is not a full Git +pathspec.`; + +export function registerCodeDiffCommand(codeCommand: Command): Command { + return codeCommand + .command("diff") + .summary("Compare two dependency source trees") + .description(CODE_DIFF_DESCRIPTION) + .argument( + "[target-or-range]", + "Unversioned target, or the range when using --repo-url.", + ) + .argument( + "[range-or-path-glob]", + "from..to range, or the path glob when using --repo-url.", + ) + .argument( + "[path-glob]", + "One repository-relative glob, preferably passed after `--`.", + ) + .option("--repo-url ", "Public GitHub repository URL addressing") + .option("-p, --patch", "Emit bounded patches (default)") + .option("--stat", "Emit per-file line statistics") + .option("--name-only", "Emit changed paths only") + .option("--name-status", "Emit change status and path") + .option("--max-files ", "Maximum returned files (1-300)") + .option( + "--max-patch-bytes ", + "Maximum aggregate patch bytes (1024-2097152; patch view only)", + ) + .option("-v, --verbose", "Emit exact resolution and scope diagnostics") + .option("--json", "Emit the JSON envelope") + .action( + async ( + arg1: string | undefined, + arg2: string | undefined, + arg3: string | undefined, + options: CodeDiffCommandOptions, + ) => { + const deps = await createContainer(); + await codeDiffAction(arg1, arg2, arg3, options, { + codeNavigationService: deps.codeNavigationService, + codeNavigationUrl: deps.codeNavigationUrl, + hasValidToken: deps.hasValidToken, + mcpUrl: deps.mcpUrl, + }); + }, + ); +} diff --git a/src/commands/code/index.test.ts b/src/commands/code/index.test.ts index 616f3d8..855150f 100644 --- a/src/commands/code/index.test.ts +++ b/src/commands/code/index.test.ts @@ -14,5 +14,8 @@ describe("registerCodeCommandGroup", () => { expect( codeCommand?.commands.some((command) => command.name() === "files"), ).toBe(true); + expect( + codeCommand?.commands.some((command) => command.name() === "diff"), + ).toBe(true); }); }); diff --git a/src/commands/code/index.ts b/src/commands/code/index.ts index 0038001..8e0f31c 100644 --- a/src/commands/code/index.ts +++ b/src/commands/code/index.ts @@ -1,4 +1,5 @@ import type { Command } from "commander"; +import { registerCodeDiffCommand } from "./diff.js"; import { registerCodeFilesCommand } from "./files.js"; import { registerCodeGrepCommand } from "./grep.js"; import { registerCodeReadCommand } from "./read.js"; @@ -13,10 +14,11 @@ export async function registerCodeCommandGroup( .command("code") .summary("Inspect dependency source code and symbols") .description( - "List files, read files, and grep substrings inside indexed dependency source. Every command accepts either `` (registry:name[@version]) or `--repo-url [--git-ref ]`. Omitted package versions use the latest release; omitted repo refs use the default-branch intent. For package-level metadata use `githits pkg`.", + "List files, read files, grep substrings, and compare exact trees inside indexed dependency source. Read/list/grep accept either `` (registry:name[@version]) or `--repo-url [--git-ref ]`; diff keeps both versions or refs in its required `..` range. For package-level metadata use `githits pkg`.", ); registerCodeFilesCommand(codeCommand); registerCodeReadCommand(codeCommand); registerCodeGrepCommand(codeCommand); + registerCodeDiffCommand(codeCommand); } From 1bcc7739d75b354a922aaa182009a32f896e308c Mon Sep 17 00:00:00 2001 From: Juha Litola Date: Mon, 17 Aug 2026 15:11:23 +0300 Subject: [PATCH 07/19] fix: align CodeDiff stat output with Git Render familiar per-file bars and totals while identifying returned-only totals when projection is incomplete. --- .../mcp/src/shared/code-diff-text.test.ts | 28 ++++++++++++- packages/mcp/src/shared/code-diff-text.ts | 39 +++++++++++++++---- 2 files changed, 59 insertions(+), 8 deletions(-) diff --git a/packages/mcp/src/shared/code-diff-text.test.ts b/packages/mcp/src/shared/code-diff-text.test.ts index ec52d09..d6d7808 100644 --- a/packages/mcp/src/shared/code-diff-text.test.ts +++ b/packages/mcp/src/shared/code-diff-text.test.ts @@ -107,7 +107,33 @@ describe("formatCodeDiffTerminal", () => { ); expect(result.stdout).toBe( - "text.ts | +4 -2\nimage.png | binary content differs\n2 returned files, +4 -2\n", + " text.ts | 6 ++++--\n image.png | binary content differs\n 2 files changed, 4 insertions(+), 2 deletions(-)\n", + ); + }); + + it("marks stat totals as returned-only when file projection is truncated", () => { + const result = formatCodeDiffTerminal( + envelope({ + view: "stat", + hasMoreFiles: true, + files: [ + { + path: "one.ts", + pathEncoding: "utf8", + status: "modified", + modeChanged: false, + typeChanged: false, + additions: 1, + deletions: 0, + contentStatus: "stats", + }, + ], + }), + options, + ); + + expect(result.stdout).toContain( + "1 returned file changed, 1 insertion(+), 0 deletions(-)", ); }); diff --git a/packages/mcp/src/shared/code-diff-text.ts b/packages/mcp/src/shared/code-diff-text.ts index 74058fa..8863e45 100644 --- a/packages/mcp/src/shared/code-diff-text.ts +++ b/packages/mcp/src/shared/code-diff-text.ts @@ -48,7 +48,7 @@ function formatPrimaryOutput(envelope: LeanCodeDiffEnvelope): string { } function formatStat(envelope: LeanCodeDiffEnvelope): string { - const lines: string[] = []; + const rows: Array<{ path: string; detail: string }> = []; let additions = 0; let deletions = 0; @@ -56,21 +56,46 @@ function formatStat(envelope: LeanCodeDiffEnvelope): string { const stat = requireStat(file); const path = safe(stat.path); if (stat.additions !== undefined && stat.deletions !== undefined) { - lines.push(`${path} | +${stat.additions} -${stat.deletions}`); + rows.push({ + path, + detail: formatStatCounts(stat.additions, stat.deletions), + }); additions += stat.additions; deletions += stat.deletions; continue; } - lines.push(`${path} | ${contentLabel(stat.contentStatus)}`); + rows.push({ path, detail: contentLabel(stat.contentStatus) }); } - if (lines.length > 0) { - const noun = lines.length === 1 ? "file" : "files"; - lines.push(`${lines.length} returned ${noun}, +${additions} -${deletions}`); - } + if (rows.length === 0) return ""; + const width = Math.max(...rows.map(({ path }) => path.length)); + const lines = rows.map( + ({ path, detail }) => ` ${path.padStart(width)} | ${detail}`, + ); + const noun = rows.length === 1 ? "file" : "files"; + const inventoryFullyRepresented = + envelope.summary.inventoryComplete && + envelope.summary.unprojectableFiles === 0 && + !envelope.hasMoreFiles && + rows.length === envelope.summary.filesChanged; + const qualifier = inventoryFullyRepresented ? "" : "returned "; + const totals = [ + `${rows.length} ${qualifier}${noun} changed`, + `${additions} ${additions === 1 ? "insertion" : "insertions"}(+)`, + `${deletions} ${deletions === 1 ? "deletion" : "deletions"}(-)`, + ]; + lines.push(` ${totals.join(", ")}`); return formatLines(lines); } +function formatStatCounts(additions: number, deletions: number): string { + const total = additions + deletions; + if (total === 0) return "0"; + const barWidth = Math.min(total, 40); + const pluses = Math.round((additions / total) * barWidth); + return `${total} ${"+".repeat(pluses)}${"-".repeat(barWidth - pluses)}`; +} + function formatPatches(files: LeanCodeDiffFile[]): string { let output = ""; for (const file of files) { From 2f40e864bb6b571e585a89bce06ea6e8524fdf31 Mon Sep 17 00:00:00 2001 From: Juha Litola Date: Mon, 17 Aug 2026 15:11:30 +0300 Subject: [PATCH 08/19] docs: record silent CodeDiff dogfood surface Document the CLI-only rollout, add release impact, and cover help, auth, validation, and live JSON behavior in CLI smoke. --- changes/code-diff-cli-dogfood.added.md | 6 +++ docs/implementation/cli-commands.md | 32 ++++++++++++ docs/implementation/code-diff.md | 49 ++++++++++++++++--- scripts/cli-smoke.ts | 67 ++++++++++++++++++++++++++ 4 files changed, 147 insertions(+), 7 deletions(-) create mode 100644 changes/code-diff-cli-dogfood.added.md diff --git a/changes/code-diff-cli-dogfood.added.md b/changes/code-diff-cli-dogfood.added.md new file mode 100644 index 0000000..660c29b --- /dev/null +++ b/changes/code-diff-cli-dogfood.added.md @@ -0,0 +1,6 @@ +--- +"githits": minor +"@githits/mcp": none +--- + +- **Add silent CodeDiff CLI dogfooding** - Adds `githits code diff` with bounded Git-like views, repository-relative glob filtering, structured completeness diagnostics, and JSON output without exposing an MCP tool or agent guidance yet. diff --git a/docs/implementation/cli-commands.md b/docs/implementation/cli-commands.md index f9f0a04..f1ec97e 100644 --- a/docs/implementation/cli-commands.md +++ b/docs/implementation/cli-commands.md @@ -32,6 +32,7 @@ The CLI exposes setup/auth commands, `doctor`, `example`, `languages`, `feedback | `pkg upgrade-review [spec]` | single package spec with current version plus `--to`, OR repeatable `--package` ranges | `--to`, repeatable `--package`, `--no-transitive-security`, `--dependency-issues`, `--min-severity`, `--verbose`, `--json` | Compare current and target versions for upgrade evidence: vulnerabilities, changelog entries, deprecation metadata, peer changes, dependency changes, and transitive security evidence by default. Reports facts only. | | `docs list ` | package spec (optional `@version`) | `--limit`, `--after`, `--verbose`, `--json` | List hosted/crawled and repository-backed documentation pages for a package. Entries include page IDs for `docs read`; JSON includes exact repo-file follow-up metadata when available. | | `docs read ` | page ID from `docs list` or search results | `--lines`, `--verbose`, `--json` | Read a documentation page by page ID. Default output is content-only; `--lines` fetches a bounded range for long pages. | +| `code diff ..` | unversioned package/repository target and exact range, or `--repo-url` and range | `--patch`, `--stat`, `--name-only`, `--name-status`, `--max-files`, `--max-patch-bytes`, `--verbose`, `--json`, one glob after `--` | Silently dogfood a bounded exact-tree diff; not exposed through MCP or agent guidance | | `code files [spec] [path-prefix]` | package spec OR `--repo-url` with optional `--git-ref`; optional `[path-prefix]` | `--path`, repeatable `--glob`, repeatable `--ext`, repeatable `--file-type`, repeatable `--language`, repeatable `--file-intent`, repeatable `--exclude-intent`, `--exclude-docs`, `--exclude-tests`, `--hidden`, `--limit`, `--wait`, `--verbose`, `--json` | List files in an indexed dependency. Selectors (`[path-prefix]`, `--path`, `--glob`) are OR-ed; the other flags filter that scope down further. Plain output is one path per line; `--verbose` adds language / type / size annotations. Indexing errors include elapsed/expected duration when available plus retry via `--wait` or indexed refs/versions from the error detail. | | `code read ` | package spec OR `--repo-url` with optional `--git-ref`; plus `` | `--lines`, `--start`, `--end`, `--wait`, `--verbose`, `--json` | Read a file's contents. Plain output is the raw file bytes (pipe-friendly); `--verbose` adds a header and a line-number gutter. `--lines 10-40` concise form; `--start`/`--end` equivalent. Binary files show a sentinel line. | | `code grep [spec] [path-prefix]` | package spec OR `--repo-url` with optional `--git-ref`; plus `` and optional `[path-prefix]` | `--path`, repeatable `--glob`, repeatable `--ext`, `--regex`, `--case-sensitive`, `-C/-A/-B`, `--exclude-docs`, `--exclude-tests`, `--limit`, `--per-file-limit`, `--cursor`, `--symbol-field`, `--wait`, `--verbose`, `--json` | Deterministic text grep over indexed dependency or repository source. Defaults to whole-target, literal, ASCII case-insensitive matching; non-ASCII letters match case-sensitively. Narrow with `[path-prefix]`, `--path`, `--glob`, or `--ext`. Plain output is `file:line:text`; `--verbose` groups matches by file. | @@ -502,6 +503,37 @@ Reads a documentation page returned by `docs list` or search results. Default ou **Troubleshooting.** Same debug areas as the `pkg` family. +### `githits code diff` + +```sh +githits code diff npm:express 4.18.1..4.18.2 +githits code diff npm:express 4.18.1..4.18.2 --stat +githits code diff npm:express 4.18.1..4.18.2 --name-status -- 'lib/**/*.js' +githits code diff --repo-url https://github.com/expressjs/express v4.18.1..v4.18.2 --name-only +``` + +Compares two exact source trees left-to-right. Package targets must omit a +version and repository targets must omit a ref because both endpoints belong +in the required two-dot range. Three-dot merge-base syntax and `--git-ref` are +rejected. The optional value after `--` is one repository-relative bounded +glob, not a full Git pathspec. + +Patch output is the default. `--stat`, `--name-only`, and `--name-status` +select cheaper views and are mutually exclusive with `--patch` and each other. +`--max-files` applies to every view; `--max-patch-bytes` is patch-only. The CLI +does not send client defaults for either bound. + +The selected Git-like view stays on stdout. Completeness, truncation, scope, +content-safety, and display-only path warnings stay on stderr; `--verbose` adds +exact resolutions and scope facts there. JSON keeps the same evidence as a +lean selected-view envelope. Empty authoritative diffs and successful partial +post-inventory evidence exit 0; request, auth, resolution, and backend errors +exit 1. + +This is a silent dogfood surface. It is normally registered and documented for +maintainers, but no MCP tool, MCP instruction, Agent Skill, or plugin guidance +promotes it yet. + ### `githits code files` ``` diff --git a/docs/implementation/code-diff.md b/docs/implementation/code-diff.md index 6a103a5..2ddddcd 100644 --- a/docs/implementation/code-diff.md +++ b/docs/implementation/code-diff.md @@ -3,13 +3,13 @@ ## Purpose The transport-neutral adapter exposes PkgSeer's exact-tree `codeDiff` GraphQL -operation to the public `@githits/mcp/client` runtime. It provides the typed -boundary needed by the later CLI and MCP surfaces without choosing their -ergonomics or claiming that a patch proves compatibility. +operation to the public `@githits/mcp/client` runtime. The root package also +registers `githits code diff` as an intentionally unpromoted CLI dogfood +surface. Neither layer claims that a patch proves compatibility. -Phase 1 deliberately adds no CLI command or MCP tool. Those surfaces must first -settle Git-like view names, path-glob behavior, output defaults, and parity -tests. +The CLI is being exercised before any MCP tool or agent instruction is added. +This keeps agent-facing signatures out of the public surface until the Git-like +ergonomics and evidence envelope have been dogfooded. ## Addressing and modes @@ -75,6 +75,39 @@ when adopting the package version containing this adapter. The existing test factories provide deterministic default results so current tool tests remain focused on their own behavior. +## Silent CLI dogfood contract + +The CLI accepts either an unversioned package/repository target followed by an +explicit `from..to` range, or `--repo-url ` followed by that range: + +```sh +githits code diff npm:express 4.18.1..4.18.2 +githits code diff npm:express 4.18.1..4.18.2 --name-status +githits code diff --repo-url https://github.com/expressjs/express v4.18.1..v4.18.2 -- 'lib/**/*.js' +``` + +The default is bounded patch output. `--patch`, `--stat`, `--name-only`, and +`--name-status` are mutually exclusive; the inventory-backed name views avoid +requesting stats or patches. One optional repository-relative glob follows +`--`. It is the backend's bounded `*`/`?`/exact-`**` grammar, not a Git +pathspec. `--max-files` applies to every view and `--max-patch-bytes` applies +only to patch output. Omitted bounds remain absent on the wire so the backend +owns its defaults. + +Plain stdout contains only the selected Git-like projection. Resolution, +scope, truncation, unprojectable-file, content-coverage, path-encoding, and +content-safety diagnostics go to stderr. `--verbose` adds exact identity and +scope diagnostics without changing the primary stream. `--json` emits a lean +camel-case data envelope whose file objects include only fields relevant to the +selected view, except that `pathEncoding` is always retained to distinguish +display-only byte escapes. + +An empty authoritative diff exits 0. `PARTIAL` or `FAILED` post-inventory +content also exits 0 with explicit diagnostics because the inventory remains +usable evidence. Validation, authentication, resolution, and raw-field errors +exit 1 through the shared CLI error envelope. No `code_diff` MCP tool, +instruction, skill, or plugin promotion exists during this phase. + ## Key reference files | File | Responsibility | @@ -82,5 +115,7 @@ focused on their own behavior. | `packages/core-internal/src/services/code-navigation-service.ts` | GraphQL query, validation, schemas, normalization, and errors | | `packages/core-internal/src/services/code-navigation-service.test.ts` | Wire-selection, variables, normalization, and failure fixtures | | `packages/mcp/src/client.ts` | Public client type/value re-exports | +| `packages/mcp/src/shared/code-diff-{request,response,text}.ts` | CLI-internal normalization, lean projection, and Git-like rendering | +| `src/commands/code/diff.ts` | Commander syntax, service call, stream routing, and CLI errors | | `scripts/validate-public-packages.ts` | Packed-package runtime and no-network TypeScript consumer checks | -| `docs/plans/code-diff-cli-mcp.md` | Phase 2 CLI/MCP ergonomics and scope decisions | +| `docs/plans/code-diff-cli-mcp.md` | Remaining rollout phases and dogfood acceptance evidence | diff --git a/scripts/cli-smoke.ts b/scripts/cli-smoke.ts index fe0e54d..2ad4a69 100644 --- a/scripts/cli-smoke.ts +++ b/scripts/cli-smoke.ts @@ -492,6 +492,16 @@ async function assertUnauthenticatedBehavior(): Promise { "resolve help should disclose query privacy guidance", ); + const codeDiffHelp = await runCliWithEnv(["code", "diff", "--help"], env); + assert(codeDiffHelp.exitCode === 0, "code diff help should succeed"); + assert( + codeDiffHelp.stdout.includes("..") && + codeDiffHelp.stdout.includes("repository-relative") && + codeDiffHelp.stdout.includes("--name-status") && + !codeDiffHelp.stdout.includes("--git-ref"), + "code diff help should expose the bounded dogfood contract", + ); + for (const command of ["init", "login"] as const) { const commandHelp = await runCliWithEnv([command, "--help"], env); assert(commandHelp.exitCode === 0, `${command} help should succeed`); @@ -541,6 +551,23 @@ async function assertUnauthenticatedBehavior(): Promise { "unauthenticated languages JSON envelope", ); + const codeDiffAuth = await runCliWithEnv( + ["code", "diff", "npm:express", "5.2.0..5.2.1", "--json"], + env, + ); + assert( + codeDiffAuth.exitCode !== 0 && codeDiffAuth.stdout.trim() === "", + "unauthenticated code diff JSON should fail with clean stdout", + ); + const codeDiffAuthPayload = assertCleanErrorEnvelope( + codeDiffAuth.stderr, + "unauthenticated code diff", + ); + assert( + codeDiffAuthPayload.code === "AUTH_REQUIRED", + "unauthenticated code diff should preserve the shared auth envelope", + ); + const resolveJson = await runCliWithEnv( ["resolve", "express", "--json"], env, @@ -1165,6 +1192,46 @@ async function runLiveSmoke(): Promise { "code grep invalid json missing CLI-native recovery", ); + const codeDiffJson = assertJsonOutput( + await runCli([ + "code", + "diff", + "npm:express", + "5.2.0..5.2.1", + "--name-only", + "--max-files", + "2", + "--json", + ]), + "code diff json", + ); + assertRecord(codeDiffJson, "code diff json"); + assert( + codeDiffJson.view === "name-only" && + Array.isArray(codeDiffJson.files) && + typeof codeDiffJson.from === "object" && + typeof codeDiffJson.to === "object", + "code diff json missing selected view or exact resolutions", + ); + + const codeDiffInvalid = await runCli([ + "code", + "diff", + "npm:express", + "5.2.0...5.2.1", + "--json", + ]); + const codeDiffInvalidEnvelope = assertCleanErrorEnvelope( + codeDiffInvalid.stderr, + "code diff invalid json", + ); + assert( + codeDiffInvalid.exitCode !== 0 && + codeDiffInvalidEnvelope.code === "INVALID_ARGUMENT" && + codeDiffInvalidEnvelope.error.includes("not `...`"), + "code diff invalid JSON should explain the two-dot range", + ); + const searchText = assertTerminalOutput( await runCli([ "search", From 2cfd7964a94a24fcd0d95ce07c76e9bf162f868d Mon Sep 17 00:00:00 2001 From: Juha Litola Date: Mon, 17 Aug 2026 15:22:33 +0300 Subject: [PATCH 09/19] fix: enforce CodeDiff glob delimiter Require the Git-style double-dash boundary for the repository-relative glob and refresh Phase 2 plan status after implementation. --- docs/plans/code-diff-cli-mcp.md | 18 +++++++++------- src/commands/code/diff.test.ts | 26 ++++++++++++++++++++++ src/commands/code/diff.ts | 38 +++++++++++++++++++++++++++------ 3 files changed, 67 insertions(+), 15 deletions(-) diff --git a/docs/plans/code-diff-cli-mcp.md b/docs/plans/code-diff-cli-mcp.md index 018afdb..ce0fb3a 100644 --- a/docs/plans/code-diff-cli-mcp.md +++ b/docs/plans/code-diff-cli-mcp.md @@ -1,8 +1,9 @@ # Plan: CodeDiff CLI dogfood and agent rollout -> Overall status: Phase 1 is merged. Phase 2 is implementation-ready as a -> silent CLI-only dogfood launch. MCP and agent-facing exposure are deferred -> until the CLI contract has been exercised. +> Overall status: Phase 1 is merged. Phase 2 is implemented and locally/live +> verified as a silent CLI-only dogfood launch; review and merge are pending. +> MCP and agent-facing exposure remain deferred until the CLI contract has +> been exercised after merge. > > Reoriented: 2026-08-17 against `githits` `origin/main` at `77417aa` and > PkgSeer backend `origin/main` at `c0cf92e` with GraphQL schema hash @@ -54,9 +55,8 @@ CodeDiff remains a separate, unexposed backend evaluation surface. - Root `githits` is version `0.9.2`; public `@githits/mcp` is version `0.9.1`. A new CLI command affects `githits` only unless implementation changes a public MCP export or behavior. -- This worktree remains at the merged PR head `f263cf8` while `origin/main` - points at the merge commit. Phase 2 implementation must begin from current - `origin/main`; this replan does not itself rewrite branch history. +- Phase 2 implementation began from `origin/main` at `77417aa`; subsequent + branch commits contain only the silent CLI dogfood delta described here. ### Backend contract @@ -340,7 +340,7 @@ There is no blocking product decision for Phase 2. | Phase | Status | Outcome | |---|---|---| | 1. Transport-neutral CodeDiff adapter | Complete and merged | Typed exact-tree request/result/error support with no CLI command or MCP tool | -| 2. Silent CLI dogfood | Implementation-ready | Git-like `githits code diff`, CLI-only tests/smoke/docs/release fragment, and dogfood evidence with no agent exposure | +| 2. Silent CLI dogfood | Implemented and verified; review pending | Git-like `githits code diff`, CLI-only tests/smoke/docs/release fragment, and initial live dogfood evidence with no agent exposure | | 3. MCP and agent rollout | Blocked on Phase 2 evidence | Stable `code_diff`, CLI/MCP parity, instructions, assets, smoke, and agent evaluation | | 4. Changelog steering | Blocked on backend action contract and stable Phase 3 invocation | Typed sparse-changelog actions that point to valid CLI/MCP diff calls | | Structural track | Out of scope / backend-blocked | Separate future proposal only after backend says structural evidence is externally safe | @@ -370,7 +370,9 @@ guidance was added. ## Phase 2: silent CLI dogfood -**Status:** implementation-ready. +**Status:** implemented and verified; review and merge pending. Authenticated +package smoke against `npm:express` `5.2.0..5.2.1` succeeded; broader +representative dogfooding remains the post-merge gate before Phase 3. **Expected outcome:** developers and deliberate CLI users can run an authoritative exact-tree raw diff through `githits code diff` with familiar diff --git a/src/commands/code/diff.test.ts b/src/commands/code/diff.test.ts index 7f014d6..9237e8f 100644 --- a/src/commands/code/diff.test.ts +++ b/src/commands/code/diff.test.ts @@ -68,6 +68,7 @@ describe("codeDiffAction", () => { dependencies({ codeNavigationService: createMockCodeNavigationService({ codeDiff }), }), + true, ); expect(codeDiff).toHaveBeenCalledWith({ @@ -199,6 +200,31 @@ describe("codeDiffAction", () => { error.mockRestore(); exit.mockRestore(); }); + + it("requires the Git-style -- delimiter before a path glob", async () => { + const codeDiff = mock(() => Promise.resolve(defaultCodeDiffResult)); + const error = spyOn(console, "error").mockImplementation(() => {}); + const exit = spyOn(process, "exit").mockImplementation(() => { + throw new Error("process.exit"); + }); + try { + await codeDiffAction( + "npm:express", + "1.0.0..2.0.0", + "src/**/*.ts", + {}, + dependencies({ + codeNavigationService: createMockCodeNavigationService({ codeDiff }), + }), + ); + } catch { + // process.exit is mocked as a throw. + } + expect(error.mock.calls[0]?.[0]).toContain("after `--`"); + expect(codeDiff).not.toHaveBeenCalled(); + error.mockRestore(); + exit.mockRestore(); + }); }); describe("formatCodeDiffError", () => { diff --git a/src/commands/code/diff.ts b/src/commands/code/diff.ts index 83b6f4b..215d72c 100644 --- a/src/commands/code/diff.ts +++ b/src/commands/code/diff.ts @@ -39,6 +39,10 @@ export interface CodeDiffCommandDependencies { mcpUrl: string; } +interface RootCommandWithRawArgs extends Command { + rawArgs?: string[]; +} + /** Execute the silent-dogfood CodeDiff CLI adapter. */ export async function codeDiffAction( arg1: string | undefined, @@ -46,6 +50,7 @@ export async function codeDiffAction( arg3: string | undefined, options: CodeDiffCommandOptions, deps: CodeDiffCommandDependencies, + pathGlobAfterDoubleDash = false, ): Promise { try { requireAuth(deps); @@ -69,6 +74,11 @@ export async function codeDiffAction( arg3, options.repoUrl !== undefined, ); + if (positionals.pathGlob !== undefined && !pathGlobAfterDoubleDash) { + throw new InvalidPackageSpecError( + "Pass the repository-relative after `--`.", + ); + } const view = resolveView(options); const build = buildCodeDiffParams({ target: positionals.target, @@ -230,7 +240,7 @@ export function registerCodeDiffCommand(codeCommand: Command): Command { ) .argument( "[path-glob]", - "One repository-relative glob, preferably passed after `--`.", + "One repository-relative glob; must be passed after `--`.", ) .option("--repo-url ", "Public GitHub repository URL addressing") .option("-p, --patch", "Emit bounded patches (default)") @@ -250,14 +260,28 @@ export function registerCodeDiffCommand(codeCommand: Command): Command { arg2: string | undefined, arg3: string | undefined, options: CodeDiffCommandOptions, + command: Command, ) => { const deps = await createContainer(); - await codeDiffAction(arg1, arg2, arg3, options, { - codeNavigationService: deps.codeNavigationService, - codeNavigationUrl: deps.codeNavigationUrl, - hasValidToken: deps.hasValidToken, - mcpUrl: deps.mcpUrl, - }); + await codeDiffAction( + arg1, + arg2, + arg3, + options, + { + codeNavigationService: deps.codeNavigationService, + codeNavigationUrl: deps.codeNavigationUrl, + hasValidToken: deps.hasValidToken, + mcpUrl: deps.mcpUrl, + }, + rootCommandUsedDoubleDash(command), + ); }, ); } + +function rootCommandUsedDoubleDash(command: Command): boolean { + let root = command; + while (root.parent) root = root.parent; + return (root as RootCommandWithRawArgs).rawArgs?.includes("--") ?? false; +} From 77577283eabfa73ea4c68c074fe67156c0ac0596 Mon Sep 17 00:00:00 2001 From: Juha Litola Date: Mon, 17 Aug 2026 15:28:40 +0300 Subject: [PATCH 10/19] fix: bind CodeDiff glob to delimiter suffix Validate the actual Commander argv suffix and cover valid, trailing-delimiter, and root-delimiter parsing. --- src/commands/code/diff.test.ts | 84 ++++++++++++++++++++++++++++++++++ src/commands/code/diff.ts | 47 ++++++++++++++----- 2 files changed, 120 insertions(+), 11 deletions(-) diff --git a/src/commands/code/diff.test.ts b/src/commands/code/diff.test.ts index 9237e8f..93cd93a 100644 --- a/src/commands/code/diff.test.ts +++ b/src/commands/code/diff.test.ts @@ -260,4 +260,88 @@ describe("registerCodeDiffCommand", () => { false, ); }); + + it("accepts a path glob only when the raw argv suffix is -- ", async () => { + const codeDiff = mock(() => Promise.resolve(defaultCodeDiffResult)); + const stdout = spyOn(process.stdout, "write").mockImplementation( + (() => true) as typeof process.stdout.write, + ); + const program = new Command("githits"); + const code = program.command("code"); + registerCodeDiffCommand(code, async () => + dependencies({ + codeNavigationService: createMockCodeNavigationService({ codeDiff }), + }), + ); + + await program.parseAsync([ + "node", + "githits", + "code", + "diff", + "npm:express", + "1.0.0..2.0.0", + "--", + "src/**/*.ts", + ]); + + const calls = codeDiff.mock.calls as unknown as Array< + [{ options?: { pathGlob?: string } }] + >; + expect(calls[0]?.[0].options?.pathGlob).toBe("src/**/*.ts"); + stdout.mockRestore(); + }); + + it.each([ + [ + "glob before trailing delimiter", + [ + "node", + "githits", + "code", + "diff", + "npm:express", + "1.0.0..2.0.0", + "src/**/*.ts", + "--", + ], + ], + [ + "root delimiter", + [ + "node", + "githits", + "--", + "code", + "diff", + "npm:express", + "1.0.0..2.0.0", + "src/**/*.ts", + ], + ], + ] as const)("rejects a path glob with a %s", async (_label, argv) => { + const codeDiff = mock(() => Promise.resolve(defaultCodeDiffResult)); + const error = spyOn(console, "error").mockImplementation(() => {}); + const exit = spyOn(process, "exit").mockImplementation(() => { + throw new Error("process.exit"); + }); + const program = new Command("githits"); + const code = program.command("code"); + registerCodeDiffCommand(code, async () => + dependencies({ + codeNavigationService: createMockCodeNavigationService({ codeDiff }), + }), + ); + + try { + await program.parseAsync([...argv]); + } catch { + // process.exit is mocked as a throw. + } + + expect(error.mock.calls[0]?.[0]).toContain("after `--`"); + expect(codeDiff).not.toHaveBeenCalled(); + error.mockRestore(); + exit.mockRestore(); + }); }); diff --git a/src/commands/code/diff.ts b/src/commands/code/diff.ts index 215d72c..9711d24 100644 --- a/src/commands/code/diff.ts +++ b/src/commands/code/diff.ts @@ -39,6 +39,9 @@ export interface CodeDiffCommandDependencies { mcpUrl: string; } +export type CodeDiffCommandDependencyFactory = + () => Promise; + interface RootCommandWithRawArgs extends Command { rawArgs?: string[]; } @@ -225,7 +228,10 @@ After \`--\`, one optional applies a repository-relative bounded glob. It supports *, ?, and an exact ** path component; it is not a full Git pathspec.`; -export function registerCodeDiffCommand(codeCommand: Command): Command { +export function registerCodeDiffCommand( + codeCommand: Command, + createDependencies: CodeDiffCommandDependencyFactory = createCodeDiffCommandDependencies, +): Command { return codeCommand .command("diff") .summary("Compare two dependency source trees") @@ -262,26 +268,45 @@ export function registerCodeDiffCommand(codeCommand: Command): Command { options: CodeDiffCommandOptions, command: Command, ) => { - const deps = await createContainer(); + const deps = await createDependencies(); await codeDiffAction( arg1, arg2, arg3, options, - { - codeNavigationService: deps.codeNavigationService, - codeNavigationUrl: deps.codeNavigationUrl, - hasValidToken: deps.hasValidToken, - mcpUrl: deps.mcpUrl, - }, - rootCommandUsedDoubleDash(command), + deps, + pathGlobFollowsDoubleDash( + command, + options.repoUrl !== undefined ? arg2 : arg3, + ), ); }, ); } -function rootCommandUsedDoubleDash(command: Command): boolean { +function pathGlobFollowsDoubleDash( + command: Command, + pathGlob: string | undefined, +): boolean { + if (pathGlob === undefined) return false; let root = command; while (root.parent) root = root.parent; - return (root as RootCommandWithRawArgs).rawArgs?.includes("--") ?? false; + const rawArgs = (root as RootCommandWithRawArgs).rawArgs; + if (!rawArgs) return false; + const delimiter = rawArgs.lastIndexOf("--"); + return ( + delimiter >= 0 && + delimiter === rawArgs.length - 2 && + rawArgs[delimiter + 1] === pathGlob + ); +} + +async function createCodeDiffCommandDependencies(): Promise { + const deps = await createContainer(); + return { + codeNavigationService: deps.codeNavigationService, + codeNavigationUrl: deps.codeNavigationUrl, + hasValidToken: deps.hasValidToken, + mcpUrl: deps.mcpUrl, + }; } From 0e8413e54e2ee7a10edfef3e93eeabb3d21cb5a6 Mon Sep 17 00:00:00 2001 From: Juha Litola Date: Mon, 17 Aug 2026 15:29:40 +0300 Subject: [PATCH 11/19] fix: accept double-dash CodeDiff glob Compare the final argv tokens directly so a literal double-dash glob remains valid after the separator. --- src/commands/code/diff.test.ts | 59 ++++++++++++++++++---------------- src/commands/code/diff.ts | 7 +--- 2 files changed, 32 insertions(+), 34 deletions(-) diff --git a/src/commands/code/diff.test.ts b/src/commands/code/diff.test.ts index 93cd93a..436520b 100644 --- a/src/commands/code/diff.test.ts +++ b/src/commands/code/diff.test.ts @@ -261,36 +261,39 @@ describe("registerCodeDiffCommand", () => { ); }); - it("accepts a path glob only when the raw argv suffix is -- ", async () => { - const codeDiff = mock(() => Promise.resolve(defaultCodeDiffResult)); - const stdout = spyOn(process.stdout, "write").mockImplementation( - (() => true) as typeof process.stdout.write, - ); - const program = new Command("githits"); - const code = program.command("code"); - registerCodeDiffCommand(code, async () => - dependencies({ - codeNavigationService: createMockCodeNavigationService({ codeDiff }), - }), - ); + it.each(["src/**/*.ts", "--"])( + "accepts path glob %s when the raw argv suffix is -- ", + async (pathGlob) => { + const codeDiff = mock(() => Promise.resolve(defaultCodeDiffResult)); + const stdout = spyOn(process.stdout, "write").mockImplementation( + (() => true) as typeof process.stdout.write, + ); + const program = new Command("githits"); + const code = program.command("code"); + registerCodeDiffCommand(code, async () => + dependencies({ + codeNavigationService: createMockCodeNavigationService({ codeDiff }), + }), + ); - await program.parseAsync([ - "node", - "githits", - "code", - "diff", - "npm:express", - "1.0.0..2.0.0", - "--", - "src/**/*.ts", - ]); + await program.parseAsync([ + "node", + "githits", + "code", + "diff", + "npm:express", + "1.0.0..2.0.0", + "--", + pathGlob, + ]); - const calls = codeDiff.mock.calls as unknown as Array< - [{ options?: { pathGlob?: string } }] - >; - expect(calls[0]?.[0].options?.pathGlob).toBe("src/**/*.ts"); - stdout.mockRestore(); - }); + const calls = codeDiff.mock.calls as unknown as Array< + [{ options?: { pathGlob?: string } }] + >; + expect(calls[0]?.[0].options?.pathGlob).toBe(pathGlob); + stdout.mockRestore(); + }, + ); it.each([ [ diff --git a/src/commands/code/diff.ts b/src/commands/code/diff.ts index 9711d24..238b2b4 100644 --- a/src/commands/code/diff.ts +++ b/src/commands/code/diff.ts @@ -293,12 +293,7 @@ function pathGlobFollowsDoubleDash( while (root.parent) root = root.parent; const rawArgs = (root as RootCommandWithRawArgs).rawArgs; if (!rawArgs) return false; - const delimiter = rawArgs.lastIndexOf("--"); - return ( - delimiter >= 0 && - delimiter === rawArgs.length - 2 && - rawArgs[delimiter + 1] === pathGlob - ); + return rawArgs.at(-2) === "--" && rawArgs.at(-1) === pathGlob; } async function createCodeDiffCommandDependencies(): Promise { From de9205214cd1488938e579a7895e5a035fbac8b3 Mon Sep 17 00:00:00 2001 From: Juha Litola Date: Mon, 17 Aug 2026 15:52:53 +0300 Subject: [PATCH 12/19] fix: refine CodeDiff CLI ergonomics Use CLI-native validation names, reject Git pathspec magic, align stat formatting, share numeric bounds, mark truncated recovery lists, document backend escaping, and smoke the delimiter contract. --- docs/implementation/cli-commands.md | 3 +- docs/implementation/code-diff.md | 4 +- docs/plans/code-diff-cli-mcp.md | 5 +- .../mcp/src/shared/code-diff-request.test.ts | 6 ++ packages/mcp/src/shared/code-diff-request.ts | 28 +++++-- .../mcp/src/shared/code-diff-text.test.ts | 38 ++++++++- packages/mcp/src/shared/code-diff-text.ts | 61 ++++++++++---- scripts/cli-smoke.ts | 21 +++++ src/commands/code/diff.test.ts | 59 ++++++++++++-- src/commands/code/diff.ts | 79 +++++++++++++++---- 10 files changed, 254 insertions(+), 50 deletions(-) diff --git a/docs/implementation/cli-commands.md b/docs/implementation/cli-commands.md index f1ec97e..0645cff 100644 --- a/docs/implementation/cli-commands.md +++ b/docs/implementation/cli-commands.md @@ -516,7 +516,8 @@ Compares two exact source trees left-to-right. Package targets must omit a version and repository targets must omit a ref because both endpoints belong in the required two-dot range. Three-dot merge-base syntax and `--git-ref` are rejected. The optional value after `--` is one repository-relative bounded -glob, not a full Git pathspec. +glob, not a full Git pathspec. A backslash escapes one following non-slash +character according to the backend grammar. Patch output is the default. `--stat`, `--name-only`, and `--name-status` select cheaper views and are mutually exclusive with `--patch` and each other. diff --git a/docs/implementation/code-diff.md b/docs/implementation/code-diff.md index 2ddddcd..c9d6ab7 100644 --- a/docs/implementation/code-diff.md +++ b/docs/implementation/code-diff.md @@ -90,7 +90,9 @@ The default is bounded patch output. `--patch`, `--stat`, `--name-only`, and `--name-status` are mutually exclusive; the inventory-backed name views avoid requesting stats or patches. One optional repository-relative glob follows `--`. It is the backend's bounded `*`/`?`/exact-`**` grammar, not a Git -pathspec. `--max-files` applies to every view and `--max-patch-bytes` applies +pathspec. A backslash escapes exactly one following non-slash character; this +mirrors PkgSeer's `CodeDiff.Raw.PathGlob` compiler rather than shell or Git +escaping. `--max-files` applies to every view and `--max-patch-bytes` applies only to patch output. Omitted bounds remain absent on the wire so the backend owns its defaults. diff --git a/docs/plans/code-diff-cli-mcp.md b/docs/plans/code-diff-cli-mcp.md index ce0fb3a..597ef6d 100644 --- a/docs/plans/code-diff-cli-mcp.md +++ b/docs/plans/code-diff-cli-mcp.md @@ -76,8 +76,9 @@ The SDL and implementation documentation prove: Package publication evidence restricts the inventory but does not rewrite returned paths or caller filters to package-relative values. - `pathGlob` supports a bounded subset: `*` and `?` stay within a component and - an exact `**` component spans components. Unsupported shell/pathspec syntax - is rejected. Only one glob is accepted per request. + an exact `**` component spans components. A backslash escapes one following + non-slash character. Unsupported shell/pathspec syntax is rejected. Only one + glob is accepted per request. - Server defaults are `maxFiles=50` and `maxPatchBytes=262144`; each patch also has a fixed 131072-byte ceiling. The server clamps numeric options, but the CLI must reject out-of-range user values rather than silently altering them. diff --git a/packages/mcp/src/shared/code-diff-request.test.ts b/packages/mcp/src/shared/code-diff-request.test.ts index bb897b6..12afe8c 100644 --- a/packages/mcp/src/shared/code-diff-request.test.ts +++ b/packages/mcp/src/shared/code-diff-request.test.ts @@ -202,6 +202,12 @@ describe("buildCodeDiffParams", () => { "src/!test.ts", "src/foo\\", "src/foo\\/*.ts", + ":", + ":(exclude)lib/**", + ":(glob)src/*", + ":/src/**", + ":!lib/**", + ":^lib/**", ]) { invalid({ target: "npm:express", range: "1..2", pathGlob }); } diff --git a/packages/mcp/src/shared/code-diff-request.ts b/packages/mcp/src/shared/code-diff-request.ts index ff6b639..49c28f9 100644 --- a/packages/mcp/src/shared/code-diff-request.ts +++ b/packages/mcp/src/shared/code-diff-request.ts @@ -30,10 +30,10 @@ const VIEW_TO_MODE: Record = { }; const MAX_PATH_GLOB_BYTES = 1024; -const MAX_FILES_MIN = 1; -const MAX_FILES_MAX = 300; -const MAX_PATCH_BYTES_MIN = 1024; -const MAX_PATCH_BYTES_MAX = 2_097_152; +export const CODE_DIFF_MAX_FILES_MIN = 1; +export const CODE_DIFF_MAX_FILES_MAX = 300; +export const CODE_DIFF_MAX_PATCH_BYTES_MIN = 1024; +export const CODE_DIFF_MAX_PATCH_BYTES_MAX = 2_097_152; export function buildCodeDiffParams( input: CodeDiffRequestInput, @@ -45,14 +45,14 @@ export function buildCodeDiffParams( const maxFiles = normaliseIntegerOption( input.maxFiles, "maxFiles", - MAX_FILES_MIN, - MAX_FILES_MAX, + CODE_DIFF_MAX_FILES_MIN, + CODE_DIFF_MAX_FILES_MAX, ); const maxPatchBytes = normaliseIntegerOption( input.maxPatchBytes, "maxPatchBytes", - MAX_PATCH_BYTES_MIN, - MAX_PATCH_BYTES_MAX, + CODE_DIFF_MAX_PATCH_BYTES_MIN, + CODE_DIFF_MAX_PATCH_BYTES_MAX, ); if (maxPatchBytes !== undefined && view !== "patch") { @@ -209,6 +209,18 @@ interface GlobToken { } function validatePathGlobGrammar(pathGlob: string): void { + if ( + pathGlob === ":" || + pathGlob.startsWith(":(") || + pathGlob.startsWith(":/") || + pathGlob.startsWith(":!") || + pathGlob.startsWith(":^") + ) { + throw invalid( + "`pathGlob` does not support Git pathspec magic; pass one bounded glob.", + ); + } + const components: GlobToken[][] = []; let component: GlobToken[] = []; const characters = Array.from(pathGlob); diff --git a/packages/mcp/src/shared/code-diff-text.test.ts b/packages/mcp/src/shared/code-diff-text.test.ts index d6d7808..0e64600 100644 --- a/packages/mcp/src/shared/code-diff-text.test.ts +++ b/packages/mcp/src/shared/code-diff-text.test.ts @@ -107,7 +107,7 @@ describe("formatCodeDiffTerminal", () => { ); expect(result.stdout).toBe( - " text.ts | 6 ++++--\n image.png | binary content differs\n 2 files changed, 4 insertions(+), 2 deletions(-)\n", + " text.ts | 6 ++++--\n image.png | binary content differs\n 2 files changed, 4 insertions(+), 2 deletions(-)\n", ); }); @@ -132,9 +132,41 @@ describe("formatCodeDiffTerminal", () => { options, ); - expect(result.stdout).toContain( - "1 returned file changed, 1 insertion(+), 0 deletions(-)", + expect(result.stdout).toContain("1 returned file changed, 1 insertion(+)"); + }); + + it("keeps a visible sign for every non-zero stat direction", () => { + const result = formatCodeDiffTerminal( + envelope({ + view: "stat", + summary: { + filesChanged: 1, + added: 0, + deleted: 0, + modified: 1, + modeChanged: 0, + typeChanged: 0, + inventoryComplete: true, + unprojectableFiles: 0, + }, + files: [ + { + path: "skewed.ts", + pathEncoding: "utf8", + status: "modified", + modeChanged: false, + typeChanged: false, + additions: 1, + deletions: 100, + contentStatus: "stats", + }, + ], + }), + options, ); + + expect(result.stdout.split("\n")[0]).toContain("+"); + expect(result.stdout.split("\n")[0]).toContain("-"); }); it("preserves served patches and renders truthful non-text outcomes", () => { diff --git a/packages/mcp/src/shared/code-diff-text.ts b/packages/mcp/src/shared/code-diff-text.ts index 8863e45..9583909 100644 --- a/packages/mcp/src/shared/code-diff-text.ts +++ b/packages/mcp/src/shared/code-diff-text.ts @@ -48,7 +48,12 @@ function formatPrimaryOutput(envelope: LeanCodeDiffEnvelope): string { } function formatStat(envelope: LeanCodeDiffEnvelope): string { - const rows: Array<{ path: string; detail: string }> = []; + const rows: Array<{ + path: string; + additions?: number; + deletions?: number; + contentStatus: LeanCodeDiffStatFile["contentStatus"]; + }> = []; let additions = 0; let deletions = 0; @@ -58,20 +63,34 @@ function formatStat(envelope: LeanCodeDiffEnvelope): string { if (stat.additions !== undefined && stat.deletions !== undefined) { rows.push({ path, - detail: formatStatCounts(stat.additions, stat.deletions), + additions: stat.additions, + deletions: stat.deletions, + contentStatus: stat.contentStatus, }); additions += stat.additions; deletions += stat.deletions; continue; } - rows.push({ path, detail: contentLabel(stat.contentStatus) }); + rows.push({ path, contentStatus: stat.contentStatus }); } if (rows.length === 0) return ""; - const width = Math.max(...rows.map(({ path }) => path.length)); - const lines = rows.map( - ({ path, detail }) => ` ${path.padStart(width)} | ${detail}`, + const pathWidth = Math.max(...rows.map(({ path }) => path.length)); + const countWidth = Math.max( + 1, + ...rows.map(({ additions, deletions }) => + additions !== undefined && deletions !== undefined + ? String(additions + deletions).length + : 0, + ), ); + const lines = rows.map(({ path, additions, deletions, contentStatus }) => { + const detail = + additions !== undefined && deletions !== undefined + ? formatStatCounts(additions, deletions, countWidth) + : contentLabel(contentStatus); + return ` ${path.padEnd(pathWidth)} | ${detail}`; + }); const noun = rows.length === 1 ? "file" : "files"; const inventoryFullyRepresented = envelope.summary.inventoryComplete && @@ -79,21 +98,33 @@ function formatStat(envelope: LeanCodeDiffEnvelope): string { !envelope.hasMoreFiles && rows.length === envelope.summary.filesChanged; const qualifier = inventoryFullyRepresented ? "" : "returned "; - const totals = [ - `${rows.length} ${qualifier}${noun} changed`, - `${additions} ${additions === 1 ? "insertion" : "insertions"}(+)`, - `${deletions} ${deletions === 1 ? "deletion" : "deletions"}(-)`, - ]; + const totals = [`${rows.length} ${qualifier}${noun} changed`]; + if (additions > 0) { + totals.push( + `${additions} ${additions === 1 ? "insertion" : "insertions"}(+)`, + ); + } + if (deletions > 0) { + totals.push( + `${deletions} ${deletions === 1 ? "deletion" : "deletions"}(-)`, + ); + } lines.push(` ${totals.join(", ")}`); return formatLines(lines); } -function formatStatCounts(additions: number, deletions: number): string { +function formatStatCounts( + additions: number, + deletions: number, + countWidth: number, +): string { const total = additions + deletions; - if (total === 0) return "0"; + if (total === 0) return "0".padStart(countWidth); const barWidth = Math.min(total, 40); - const pluses = Math.round((additions / total) * barWidth); - return `${total} ${"+".repeat(pluses)}${"-".repeat(barWidth - pluses)}`; + let pluses = Math.round((additions / total) * barWidth); + if (additions > 0) pluses = Math.max(1, pluses); + if (deletions > 0) pluses = Math.min(barWidth - 1, pluses); + return `${String(total).padStart(countWidth)} ${"+".repeat(pluses)}${"-".repeat(barWidth - pluses)}`; } function formatPatches(files: LeanCodeDiffFile[]): string { diff --git a/scripts/cli-smoke.ts b/scripts/cli-smoke.ts index 2ad4a69..3eb8f89 100644 --- a/scripts/cli-smoke.ts +++ b/scripts/cli-smoke.ts @@ -1202,6 +1202,8 @@ async function runLiveSmoke(): Promise { "--max-files", "2", "--json", + "--", + "**/*.js", ]), "code diff json", ); @@ -1232,6 +1234,25 @@ async function runLiveSmoke(): Promise { "code diff invalid JSON should explain the two-dot range", ); + const codeDiffBareGlob = await runCli([ + "code", + "diff", + "npm:express", + "5.2.0..5.2.1", + "**/*.js", + "--json", + ]); + const codeDiffBareGlobEnvelope = assertCleanErrorEnvelope( + codeDiffBareGlob.stderr, + "code diff bare glob JSON", + ); + assert( + codeDiffBareGlob.exitCode !== 0 && + codeDiffBareGlobEnvelope.code === "INVALID_ARGUMENT" && + codeDiffBareGlobEnvelope.error.includes("after `--`"), + "code diff should require the Git-style glob delimiter", + ); + const searchText = assertTerminalOutput( await runCli([ "search", diff --git a/src/commands/code/diff.test.ts b/src/commands/code/diff.test.ts index 436520b..8988bde 100644 --- a/src/commands/code/diff.test.ts +++ b/src/commands/code/diff.test.ts @@ -145,12 +145,16 @@ describe("codeDiffAction", () => { }); it.each([ - [{ patch: true, stat: true }, "Choose only one diff view"], - [{ stat: true, maxPatchBytes: "2048" }, "valid only"], - [{ maxFiles: "0" }, "--max-files expects"], + [{ patch: true, stat: true }, "Choose only one diff view", undefined], + [ + { stat: true, maxPatchBytes: "2048" }, + "`--max-patch-bytes` is valid only", + "maxPatchBytes", + ], + [{ maxFiles: "0" }, "--max-files expects", undefined], ] as const)( "rejects invalid options before network I/O", - async (options, text) => { + async (options, text, forbidden) => { const codeDiff = mock(() => Promise.resolve(defaultCodeDiffResult)); const error = spyOn(console, "error").mockImplementation(() => {}); const exit = spyOn(process, "exit").mockImplementation(() => { @@ -174,6 +178,7 @@ describe("codeDiffAction", () => { } expect(error.mock.calls[0]?.[0]).toContain(text); + if (forbidden) expect(error.mock.calls[0]?.[0]).not.toContain(forbidden); expect(codeDiff).not.toHaveBeenCalled(); error.mockRestore(); exit.mockRestore(); @@ -201,6 +206,34 @@ describe("codeDiffAction", () => { exit.mockRestore(); }); + it("uses CLI-native wording for invalid path globs", async () => { + const codeDiff = mock(() => Promise.resolve(defaultCodeDiffResult)); + const error = spyOn(console, "error").mockImplementation(() => {}); + const exit = spyOn(process, "exit").mockImplementation(() => { + throw new Error("process.exit"); + }); + try { + await codeDiffAction( + "npm:express", + "1.0.0..2.0.0", + ":(exclude)lib/**", + {}, + dependencies({ + codeNavigationService: createMockCodeNavigationService({ codeDiff }), + }), + true, + ); + } catch { + // process.exit is mocked as a throw. + } + expect(error.mock.calls[0]?.[0]).toContain("``"); + expect(error.mock.calls[0]?.[0]).toContain("pathspec magic"); + expect(error.mock.calls[0]?.[0]).not.toContain("pathGlob"); + expect(codeDiff).not.toHaveBeenCalled(); + error.mockRestore(); + exit.mockRestore(); + }); + it("requires the Git-style -- delimiter before a path glob", async () => { const codeDiff = mock(() => Promise.resolve(defaultCodeDiffResult)); const error = spyOn(console, "error").mockImplementation(() => {}); @@ -243,7 +276,23 @@ describe("formatCodeDiffError", () => { expect(output).toContain("side: from"); expect(output).toContain("stage: resolution"); - expect(output).toContain("2.0.0, 1.0.0, …"); + expect(output).toContain("2.0.0, 1.0.0 (+more)"); + }); + + it("marks locally truncated recovery lists", () => { + const output = formatCodeDiffError({ + code: "REF_NOT_FOUND", + message: "Ref was not found.", + retryable: false, + details: { + availableRefs: Array.from({ length: 10 }, (_, index) => ({ + ref: `ref-${index}`, + })), + }, + }); + + expect(output).toContain("ref-0, ref-1"); + expect(output).toContain("(+2 more)"); }); }); diff --git a/src/commands/code/diff.ts b/src/commands/code/diff.ts index 238b2b4..3b8e599 100644 --- a/src/commands/code/diff.ts +++ b/src/commands/code/diff.ts @@ -2,6 +2,12 @@ import type { CodeNavigationService } from "@githits/core-internal"; import { buildCodeDiffParams, buildCodeDiffSuccessPayload, + CODE_DIFF_MAX_FILES_MAX, + CODE_DIFF_MAX_FILES_MIN, + CODE_DIFF_MAX_PATCH_BYTES_MAX, + CODE_DIFF_MAX_PATCH_BYTES_MIN, + type CodeDiffRequestBuildResult, + type CodeDiffRequestInput, type CodeDiffView, formatCodeDiffTerminal, InvalidPackageSpecError, @@ -83,18 +89,23 @@ export async function codeDiffAction( ); } const view = resolveView(options); - const build = buildCodeDiffParams({ + const build = buildCliCodeDiffParams({ target: positionals.target, repoUrl: options.repoUrl, range: positionals.range, view, pathGlob: positionals.pathGlob, - maxFiles: parseIntCliOption(options.maxFiles, "--max-files", 1, 300), + maxFiles: parseIntCliOption( + options.maxFiles, + "--max-files", + CODE_DIFF_MAX_FILES_MIN, + CODE_DIFF_MAX_FILES_MAX, + ), maxPatchBytes: parseIntCliOption( options.maxPatchBytes, "--max-patch-bytes", - 1024, - 2_097_152, + CODE_DIFF_MAX_PATCH_BYTES_MIN, + CODE_DIFF_MAX_PATCH_BYTES_MAX, ), }); @@ -127,6 +138,23 @@ export async function codeDiffAction( } } +function buildCliCodeDiffParams( + input: CodeDiffRequestInput, +): CodeDiffRequestBuildResult { + try { + return buildCodeDiffParams(input); + } catch (error) { + if (!(error instanceof InvalidPackageSpecError)) throw error; + const rewritten = error.message + .replace(/`maxPatchBytes`/g, "`--max-patch-bytes`") + .replace(/`maxFiles`/g, "`--max-files`") + .replace(/`pathGlob`/g, "``") + .replace(/CodeDiff view/g, "Diff view"); + if (rewritten === error.message) throw error; + throw new InvalidPackageSpecError(rewritten); + } +} + interface ResolvedPositionals { target?: string; range: string; @@ -183,7 +211,12 @@ function resolveView(options: CodeDiffCommandOptions): CodeDiffView { /** Render bounded CodeDiff diagnostics without exposing raw GraphQL details. */ export function formatCodeDiffError(mapped: MappedError): string { const safe = (value: string): string => sanitizeTerminalText(value); - const lines = [safe(formatMappedErrorForTerminal(mapped))]; + const safeBlock = (value: string): string => + value + .split("\n") + .map((line) => safe(line)) + .join("\n"); + const lines = [safeBlock(formatMappedErrorForTerminal(mapped))]; const details = mapped.details; if (!details) return lines[0] as string; @@ -192,28 +225,44 @@ export function formatCodeDiffError(mapped: MappedError): string { if (details.limitKind) lines.push(` limit: ${safe(details.limitKind)}`); if (details.publishedVersions?.length) { lines.push( - ` published versions: ${details.publishedVersions.slice(0, 8).map(safe).join(", ")}${details.publishedVersionsTruncated ? ", …" : ""}`, + ` published versions: ${formatRecoveryList( + details.publishedVersions, + safe, + details.publishedVersionsTruncated, + )}`, ); } if (details.availableRefs?.length) { lines.push( - ` available refs: ${details.availableRefs - .slice(0, 8) - .map((entry) => safe(entry.version ?? entry.ref)) - .join(", ")}`, + ` available refs: ${formatRecoveryList( + details.availableRefs.map((entry) => entry.version ?? entry.ref), + safe, + )}`, ); } if (details.suggestedRefs?.length) { lines.push( - ` suggested refs: ${details.suggestedRefs - .slice(0, 8) - .map((entry) => safe(entry.version ?? entry.ref)) - .join(", ")}`, + ` suggested refs: ${formatRecoveryList( + details.suggestedRefs.map((entry) => entry.version ?? entry.ref), + safe, + )}`, ); } return lines.join("\n"); } +function formatRecoveryList( + values: string[], + safe: (value: string) => string, + backendTruncated = false, +): string { + const limit = 8; + const shown = values.slice(0, limit).map(safe).join(", "); + if (backendTruncated) return `${shown} (+more)`; + const omitted = values.length - limit; + return omitted > 0 ? `${shown} (+${omitted} more)` : shown; +} + const CODE_DIFF_DESCRIPTION = `Compare two exact dependency source trees. The default output is a bounded patch, matching ordinary \`git diff\` where the @@ -226,7 +275,7 @@ is always left-to-right; three-dot merge-base syntax is not supported. After \`--\`, one optional applies a repository-relative bounded glob. It supports *, ?, and an exact ** path component; it is not a full Git -pathspec.`; +pathspec. A backslash escapes one following non-slash character.`; export function registerCodeDiffCommand( codeCommand: Command, From 11d936eb027670959e4dac352dd8a9ceb3663e5a Mon Sep 17 00:00:00 2001 From: Juha Litola Date: Mon, 17 Aug 2026 15:57:16 +0300 Subject: [PATCH 13/19] fix: complete CodeDiff CLI error adaptation Translate remaining target fields and retain a lower bound when recovery candidates are truncated twice. --- src/commands/code/diff.test.ts | 54 ++++++++++++++++++++++++++++++++++ src/commands/code/diff.ts | 6 +++- 2 files changed, 59 insertions(+), 1 deletion(-) diff --git a/src/commands/code/diff.test.ts b/src/commands/code/diff.test.ts index 8988bde..1393f5d 100644 --- a/src/commands/code/diff.test.ts +++ b/src/commands/code/diff.test.ts @@ -234,6 +234,43 @@ describe("codeDiffAction", () => { exit.mockRestore(); }); + it.each([ + { + arg1: "npm:express@5.0.0", + arg2: "1.0.0..2.0.0", + options: {}, + expected: "`..`", + forbidden: "`range`", + }, + { + arg1: "1.0.0..2.0.0", + arg2: undefined, + options: { repoUrl: "npm:express" }, + expected: "`--repo-url`", + forbidden: "`repoUrl`", + }, + ])("uses CLI names for target validation", async (testCase) => { + const error = spyOn(console, "error").mockImplementation(() => {}); + const exit = spyOn(process, "exit").mockImplementation(() => { + throw new Error("process.exit"); + }); + try { + await codeDiffAction( + testCase.arg1, + testCase.arg2, + undefined, + testCase.options, + dependencies(), + ); + } catch { + // process.exit is mocked as a throw. + } + expect(error.mock.calls[0]?.[0]).toContain(testCase.expected); + expect(error.mock.calls[0]?.[0]).not.toContain(testCase.forbidden); + error.mockRestore(); + exit.mockRestore(); + }); + it("requires the Git-style -- delimiter before a path glob", async () => { const codeDiff = mock(() => Promise.resolve(defaultCodeDiffResult)); const error = spyOn(console, "error").mockImplementation(() => {}); @@ -294,6 +331,23 @@ describe("formatCodeDiffError", () => { expect(output).toContain("ref-0, ref-1"); expect(output).toContain("(+2 more)"); }); + + it("preserves a local lower bound when the backend also truncated", () => { + const output = formatCodeDiffError({ + code: "VERSION_NOT_FOUND", + message: "Version was not found.", + retryable: false, + details: { + publishedVersions: Array.from( + { length: 10 }, + (_, index) => `${index}.0.0`, + ), + publishedVersionsTruncated: true, + }, + }); + + expect(output).toContain("(+2+ more)"); + }); }); describe("registerCodeDiffCommand", () => { diff --git a/src/commands/code/diff.ts b/src/commands/code/diff.ts index 3b8e599..3cce148 100644 --- a/src/commands/code/diff.ts +++ b/src/commands/code/diff.ts @@ -149,6 +149,8 @@ function buildCliCodeDiffParams( .replace(/`maxPatchBytes`/g, "`--max-patch-bytes`") .replace(/`maxFiles`/g, "`--max-files`") .replace(/`pathGlob`/g, "``") + .replace(/`repoUrl`/g, "`--repo-url`") + .replace(/`range`/g, "`..`") .replace(/CodeDiff view/g, "Diff view"); if (rewritten === error.message) throw error; throw new InvalidPackageSpecError(rewritten); @@ -258,8 +260,10 @@ function formatRecoveryList( ): string { const limit = 8; const shown = values.slice(0, limit).map(safe).join(", "); - if (backendTruncated) return `${shown} (+more)`; const omitted = values.length - limit; + if (backendTruncated) { + return omitted > 0 ? `${shown} (+${omitted}+ more)` : `${shown} (+more)`; + } return omitted > 0 ? `${shown} (+${omitted} more)` : shown; } From 2faef4ca6d10362a95c3baa0c3d2fb48fbbd7732 Mon Sep 17 00:00:00 2001 From: Juha Litola Date: Mon, 17 Aug 2026 16:00:17 +0300 Subject: [PATCH 14/19] docs: record CodeDiff CLI review completion Capture the final verification and review evidence while keeping broader post-merge dogfooding as the Phase 3 gate. --- docs/plans/code-diff-cli-mcp.md | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/docs/plans/code-diff-cli-mcp.md b/docs/plans/code-diff-cli-mcp.md index 597ef6d..4522fb7 100644 --- a/docs/plans/code-diff-cli-mcp.md +++ b/docs/plans/code-diff-cli-mcp.md @@ -1,7 +1,7 @@ # Plan: CodeDiff CLI dogfood and agent rollout > Overall status: Phase 1 is merged. Phase 2 is implemented and locally/live -> verified as a silent CLI-only dogfood launch; review and merge are pending. +> verified and reviewed as a silent CLI-only dogfood launch; merge is pending. > MCP and agent-facing exposure remain deferred until the CLI contract has > been exercised after merge. > @@ -341,7 +341,7 @@ There is no blocking product decision for Phase 2. | Phase | Status | Outcome | |---|---|---| | 1. Transport-neutral CodeDiff adapter | Complete and merged | Typed exact-tree request/result/error support with no CLI command or MCP tool | -| 2. Silent CLI dogfood | Implemented and verified; review pending | Git-like `githits code diff`, CLI-only tests/smoke/docs/release fragment, and initial live dogfood evidence with no agent exposure | +| 2. Silent CLI dogfood | Implemented, verified, and reviewed; merge pending | Git-like `githits code diff`, CLI-only tests/smoke/docs/release fragment, and initial live dogfood evidence with no agent exposure | | 3. MCP and agent rollout | Blocked on Phase 2 evidence | Stable `code_diff`, CLI/MCP parity, instructions, assets, smoke, and agent evaluation | | 4. Changelog steering | Blocked on backend action contract and stable Phase 3 invocation | Typed sparse-changelog actions that point to valid CLI/MCP diff calls | | Structural track | Out of scope / backend-blocked | Separate future proposal only after backend says structural evidence is externally safe | @@ -371,7 +371,7 @@ guidance was added. ## Phase 2: silent CLI dogfood -**Status:** implemented and verified; review and merge pending. Authenticated +**Status:** implemented, verified, and reviewed; merge pending. Authenticated package smoke against `npm:express` `5.2.0..5.2.1` succeeded; broader representative dogfooding remains the post-merge gate before Phase 3. @@ -508,6 +508,15 @@ plugin generator, or generated agent asset in this phase. validation, and relevant format/lint checks pass. Authenticated dev results are recorded accurately. +**Observed outcome and acceptance:** the command, pure request/result/error +adapters, Git-like text formatter, smoke coverage, durable documentation, and +release fragment are complete without MCP or agent-facing exposure. The final +full suite passed 2,888 tests, build, built CLI smoke, format, lint, and public +package validation. Authenticated source smoke exercised a live package diff. +Preflight, internal runtime review, and the retained external Claude review are +clean after correcting delimiter enforcement, Git-like stat presentation, glob +grammar, bounded recovery lists, and CLI-native validation messages. + ## Phase 3: MCP and agent rollout **Status:** blocked on Phase 2 merge, dogfood evidence, and phase-boundary From 1edd009272bc56d531ecf81c02475b85846edc2f Mon Sep 17 00:00:00 2001 From: Juha Litola Date: Tue, 18 Aug 2026 09:11:50 +0300 Subject: [PATCH 15/19] fix: show paths in CodeDiff patch headers Bind raw diff placeholder headers to each authoritative file path, including Git-style /dev/null sides for additions and deletions. Add unit and authenticated smoke regression coverage. --- docs/implementation/code-diff.md | 4 +- docs/plans/code-diff-cli-mcp.md | 12 +++-- .../mcp/src/shared/code-diff-text.test.ts | 54 +++++++++++++++++-- packages/mcp/src/shared/code-diff-text.ts | 20 ++++++- scripts/cli-smoke.ts | 17 ++++++ 5 files changed, 97 insertions(+), 10 deletions(-) diff --git a/docs/implementation/code-diff.md b/docs/implementation/code-diff.md index c9d6ab7..572203b 100644 --- a/docs/implementation/code-diff.md +++ b/docs/implementation/code-diff.md @@ -102,7 +102,9 @@ content-safety diagnostics go to stderr. `--verbose` adds exact identity and scope diagnostics without changing the primary stream. `--json` emits a lean camel-case data envelope whose file objects include only fields relevant to the selected view, except that `pathEncoding` is always retained to distinguish -display-only byte escapes. +display-only byte escapes. For text patches, the formatter replaces the raw +content service's `a/file` and `b/file` placeholders with the authoritative +file path; added and deleted sides use `/dev/null` like Git. An empty authoritative diff exits 0. `PARTIAL` or `FAILED` post-inventory content also exits 0 with explicit diagnostics because the inventory remains diff --git a/docs/plans/code-diff-cli-mcp.md b/docs/plans/code-diff-cli-mcp.md index 4522fb7..d94f8ac 100644 --- a/docs/plans/code-diff-cli-mcp.md +++ b/docs/plans/code-diff-cli-mcp.md @@ -510,12 +510,16 @@ plugin generator, or generated agent asset in this phase. **Observed outcome and acceptance:** the command, pure request/result/error adapters, Git-like text formatter, smoke coverage, durable documentation, and -release fragment are complete without MCP or agent-facing exposure. The final -full suite passed 2,888 tests, build, built CLI smoke, format, lint, and public -package validation. Authenticated source smoke exercised a live package diff. +release fragment are complete without MCP or agent-facing exposure. The full +suite passed 2,889 tests, build, built CLI smoke, format, lint, and public +package validation. Authenticated source smoke exercises a live package diff +and asserts that text patch headers contain the authoritative file path. Preflight, internal runtime review, and the retained external Claude review are clean after correcting delimiter enforcement, Git-like stat presentation, glob -grammar, bounded recovery lists, and CLI-native validation messages. +grammar, bounded recovery lists, and CLI-native validation messages. Initial +post-review dogfooding exposed backend `a/file` and `b/file` patch placeholders; +the text formatter now binds them to the returned path and uses `/dev/null` for +added or deleted sides. ## Phase 3: MCP and agent rollout diff --git a/packages/mcp/src/shared/code-diff-text.test.ts b/packages/mcp/src/shared/code-diff-text.test.ts index 0e64600..0c5a03a 100644 --- a/packages/mcp/src/shared/code-diff-text.test.ts +++ b/packages/mcp/src/shared/code-diff-text.test.ts @@ -169,7 +169,7 @@ describe("formatCodeDiffTerminal", () => { expect(result.stdout.split("\n")[0]).toContain("-"); }); - it("preserves served patches and renders truthful non-text outcomes", () => { + it("binds served patch headers to the authoritative file path", () => { const result = formatCodeDiffTerminal( envelope({ files: [ @@ -181,7 +181,31 @@ describe("formatCodeDiffTerminal", () => { typeChanged: false, additions: 1, deletions: 1, - patch: "@@ -1 +1 @@\n-old\n+new\n", + patch: "--- a/file\n+++ b/file\n@@ -1 +1 @@\n-old\n+new\n", + contentStatus: "patch", + contentSafety: { filtered: false, modifications: [] }, + }, + { + path: "added file.ts", + pathEncoding: "utf8", + status: "added", + modeChanged: false, + typeChanged: false, + additions: 1, + deletions: 0, + patch: "--- a/file\n+++ b/file\n@@ -0,0 +1 @@\n+new\n", + contentStatus: "patch", + contentSafety: { filtered: false, modifications: [] }, + }, + { + path: "deleted.ts", + pathEncoding: "utf8", + status: "deleted", + modeChanged: false, + typeChanged: false, + additions: 0, + deletions: 1, + patch: "--- a/file\n+++ b/file\n@@ -1 +0,0 @@\n-old\n", contentStatus: "patch", contentSafety: { filtered: false, modifications: [] }, }, @@ -219,8 +243,32 @@ describe("formatCodeDiffTerminal", () => { ); expect(result.stdout).toBe( - "@@ -1 +1 @@\n-old\n+new\nBinary file image.png differs\nMetadata changed: mode.sh\nPatch omitted: large.ts (content_budget)\n", + "--- a/a.ts\n+++ b/a.ts\n@@ -1 +1 @@\n-old\n+new\n--- /dev/null\n+++ b/added file.ts\n@@ -0,0 +1 @@\n+new\n--- a/deleted.ts\n+++ /dev/null\n@@ -1 +0,0 @@\n-old\nBinary file image.png differs\nMetadata changed: mode.sh\nPatch omitted: large.ts (content_budget)\n", + ); + }); + + it("preserves patches without backend placeholder headers", () => { + const result = formatCodeDiffTerminal( + envelope({ + files: [ + { + path: "a.ts", + pathEncoding: "utf8", + status: "modified", + modeChanged: false, + typeChanged: false, + additions: 1, + deletions: 1, + patch: "@@ -1 +1 @@\n-old\n+new\n", + contentStatus: "patch", + contentSafety: { filtered: false, modifications: [] }, + }, + ], + }), + options, ); + + expect(result.stdout).toBe("@@ -1 +1 @@\n-old\n+new\n"); }); it("keeps an empty authoritative diff silent in plain mode", () => { diff --git a/packages/mcp/src/shared/code-diff-text.ts b/packages/mcp/src/shared/code-diff-text.ts index 9583909..a88fd6f 100644 --- a/packages/mcp/src/shared/code-diff-text.ts +++ b/packages/mcp/src/shared/code-diff-text.ts @@ -132,8 +132,9 @@ function formatPatches(files: LeanCodeDiffFile[]): string { for (const file of files) { const patchFile = requirePatch(file); if (patchFile.patch !== undefined) { - output += patchFile.patch; - if (!patchFile.patch.endsWith("\n")) output += "\n"; + const patch = bindPatchHeaders(patchFile); + output += patch; + if (!patch.endsWith("\n")) output += "\n"; continue; } output += `${patchFallback(patchFile)}\n`; @@ -141,6 +142,21 @@ function formatPatches(files: LeanCodeDiffFile[]): string { return output; } +const RAW_DIFF_PLACEHOLDER_HEADERS = "--- a/file\n+++ b/file\n"; + +/** Bind the raw diff service's content-only placeholders to its owning file. */ +function bindPatchHeaders(file: LeanCodeDiffPatchFile): string { + const patch = file.patch; + if (patch === undefined || !patch.startsWith(RAW_DIFF_PLACEHOLDER_HEADERS)) { + return patch ?? ""; + } + + const path = safe(file.path); + const fromPath = file.status === "added" ? "/dev/null" : `a/${path}`; + const toPath = file.status === "deleted" ? "/dev/null" : `b/${path}`; + return `--- ${fromPath}\n+++ ${toPath}\n${patch.slice(RAW_DIFF_PLACEHOLDER_HEADERS.length)}`; +} + function patchFallback(file: LeanCodeDiffPatchFile): string { const path = safe(file.path); switch (file.contentStatus) { diff --git a/scripts/cli-smoke.ts b/scripts/cli-smoke.ts index 3eb8f89..12f5f71 100644 --- a/scripts/cli-smoke.ts +++ b/scripts/cli-smoke.ts @@ -1216,6 +1216,23 @@ async function runLiveSmoke(): Promise { "code diff json missing selected view or exact resolutions", ); + const codeDiffPatchText = assertTerminalOutput( + await runCli([ + "code", + "diff", + "npm:express", + "5.2.0..5.2.1", + "--", + "lib/utils.js", + ]), + "code diff patch", + ); + assert( + codeDiffPatchText.startsWith("--- a/lib/utils.js\n+++ b/lib/utils.js\n") && + !codeDiffPatchText.includes("--- a/file\n+++ b/file\n"), + "code diff patch should bind headers to the authoritative file path", + ); + const codeDiffInvalid = await runCli([ "code", "diff", From a3ba4b1ce5c0a7a60d25ca9d29e785486bc4628d Mon Sep 17 00:00:00 2001 From: Juha Litola Date: Tue, 18 Aug 2026 11:32:51 +0300 Subject: [PATCH 16/19] fix: harden CodeDiff patch output Normalize JSON and text patch headers to authoritative paths, preserve unusual path identities with Git quoting, and suppress unexpectedly non-applicable patch streams while retaining caller-selected bounds. --- changes/code-diff-cli-dogfood.added.md | 2 +- docs/implementation/cli-commands.md | 11 +- docs/implementation/code-diff.md | 25 +-- packages/mcp/src/shared/code-diff-path.ts | 30 ++++ .../mcp/src/shared/code-diff-response.test.ts | 63 ++++++++ packages/mcp/src/shared/code-diff-response.ts | 30 +++- .../mcp/src/shared/code-diff-text.test.ts | 153 +++++++++++++++--- packages/mcp/src/shared/code-diff-text.ts | 76 ++++++--- src/commands/code/diff.test.ts | 78 +++++++-- src/commands/code/diff.ts | 13 +- 10 files changed, 412 insertions(+), 69 deletions(-) create mode 100644 packages/mcp/src/shared/code-diff-path.ts diff --git a/changes/code-diff-cli-dogfood.added.md b/changes/code-diff-cli-dogfood.added.md index 660c29b..7d1ad43 100644 --- a/changes/code-diff-cli-dogfood.added.md +++ b/changes/code-diff-cli-dogfood.added.md @@ -3,4 +3,4 @@ "@githits/mcp": none --- -- **Add silent CodeDiff CLI dogfooding** - Adds `githits code diff` with bounded Git-like views, repository-relative glob filtering, structured completeness diagnostics, and JSON output without exposing an MCP tool or agent guidance yet. +- **Add silent CodeDiff CLI dogfooding** - Adds `githits code diff` with bounded Git-like views, repository-relative glob filtering, reversible path quoting, apply-safe patch handling, structured completeness diagnostics, and JSON output without exposing an MCP tool or agent guidance yet. diff --git a/docs/implementation/cli-commands.md b/docs/implementation/cli-commands.md index 0645cff..2e7bbca 100644 --- a/docs/implementation/cli-commands.md +++ b/docs/implementation/cli-commands.md @@ -527,9 +527,14 @@ does not send client defaults for either bound. The selected Git-like view stays on stdout. Completeness, truncation, scope, content-safety, and display-only path warnings stay on stderr; `--verbose` adds exact resolutions and scope facts there. JSON keeps the same evidence as a -lean selected-view envelope. Empty authoritative diffs and successful partial -post-inventory evidence exit 0; request, auth, resolution, and backend errors -exit 1. +lean selected-view envelope, including authoritative paths in normalized patch +headers. Text paths use reversible Git-style quoting rather than deleting +control characters. Empty authoritative diffs exit 0. Caller-selected +`--max-files` and `--max-patch-bytes` bounds may intentionally produce partial +patches and still exit 0 with warnings. Unexpectedly incomplete or non-applicable +plain patches are suppressed and exit 1; name, stat, and JSON views preserve +their structured partial evidence. Request, auth, resolution, and backend +errors also exit 1. This is a silent dogfood surface. It is normally registered and documented for maintainers, but no MCP tool, MCP instruction, Agent Skill, or plugin guidance diff --git a/docs/implementation/code-diff.md b/docs/implementation/code-diff.md index 572203b..68f4296 100644 --- a/docs/implementation/code-diff.md +++ b/docs/implementation/code-diff.md @@ -102,15 +102,22 @@ content-safety diagnostics go to stderr. `--verbose` adds exact identity and scope diagnostics without changing the primary stream. `--json` emits a lean camel-case data envelope whose file objects include only fields relevant to the selected view, except that `pathEncoding` is always retained to distinguish -display-only byte escapes. For text patches, the formatter replaces the raw -content service's `a/file` and `b/file` placeholders with the authoritative -file path; added and deleted sides use `/dev/null` like Git. - -An empty authoritative diff exits 0. `PARTIAL` or `FAILED` post-inventory -content also exits 0 with explicit diagnostics because the inventory remains -usable evidence. Validation, authentication, resolution, and raw-field errors -exit 1 through the shared CLI error envelope. No `code_diff` MCP tool, -instruction, skill, or plugin promotion exists during this phase. +display-only byte escapes. Text views use reversible Git-style quoting for +control characters, quotes, and backslashes instead of changing path identity. +The response projector replaces the raw content service's `a/file` and +`b/file` patch placeholders with the authoritative Git-quoted file path, so +plain and JSON patches agree; added and deleted sides use `/dev/null` like Git. + +An empty authoritative diff exits 0. Name, stat, and JSON views retain partial +evidence with explicit completeness fields and diagnostics. Plain patch mode +suppresses stdout and exits 1 when unexpected truncation, failed or unavailable +content, binary/metadata-only changes, display-only paths, unprojectable files, +or content-safety changes would make the stream unsafe to apply. An explicit +`--max-files` authorizes file-count truncation, and an explicit +`--max-patch-bytes` authorizes `content_budget` omissions; neither authorizes +unrelated failure classes. Validation, authentication, resolution, and +raw-field errors exit 1 through the shared CLI error envelope. No `code_diff` +MCP tool, instruction, skill, or plugin promotion exists during this phase. ## Key reference files diff --git a/packages/mcp/src/shared/code-diff-path.ts b/packages/mcp/src/shared/code-diff-path.ts new file mode 100644 index 0000000..6b260cb --- /dev/null +++ b/packages/mcp/src/shared/code-diff-path.ts @@ -0,0 +1,30 @@ +/** Quote a UTF-8 path using Git's reversible double-quoted path syntax. */ +export function quoteGitPath(path: string): string { + let quoted = false; + let output = ""; + + for (const character of path) { + const codePoint = character.codePointAt(0); + if (character === '"' || character === "\\") { + quoted = true; + output += `\\${character}`; + continue; + } + if ( + codePoint !== undefined && + (codePoint <= 0x1f || + (codePoint >= 0x7f && codePoint <= 0x9f) || + codePoint === 0x2028 || + codePoint === 0x2029) + ) { + quoted = true; + for (const byte of new TextEncoder().encode(character)) { + output += `\\${byte.toString(8).padStart(3, "0")}`; + } + continue; + } + output += character; + } + + return quoted ? `"${output}"` : output; +} diff --git a/packages/mcp/src/shared/code-diff-response.test.ts b/packages/mcp/src/shared/code-diff-response.test.ts index 13b2726..2214af9 100644 --- a/packages/mcp/src/shared/code-diff-response.test.ts +++ b/packages/mcp/src/shared/code-diff-response.test.ts @@ -328,6 +328,69 @@ describe("buildCodeDiffSuccessPayload", () => { ]); }); + it("binds placeholder patch headers to Git-quoted authoritative paths", () => { + const result = makeResult(); + result.raw.files = [ + { + path: "src/line\nname.ts", + pathEncoding: "UTF8", + status: "MODIFIED", + modeChanged: false, + typeChanged: false, + additions: 1, + deletions: 1, + patch: "--- a/file\n+++ b/file\n@@ -1 +1 @@\n-old\n+new\n", + contentStatus: "PATCH", + contentSafety: { filtered: false, modifications: [] }, + }, + { + path: "added file.ts", + pathEncoding: "UTF8", + status: "ADDED", + modeChanged: false, + typeChanged: false, + additions: 1, + deletions: 0, + patch: "--- /dev/null\n+++ b/file\n@@ -0,0 +1 @@\n+new\n", + contentStatus: "PATCH", + contentSafety: { filtered: false, modifications: [] }, + }, + { + path: "deleted.ts", + pathEncoding: "UTF8", + status: "DELETED", + modeChanged: false, + typeChanged: false, + additions: 0, + deletions: 1, + patch: "--- a/file\n+++ /dev/null\n@@ -1 +0,0 @@\n-old\n", + contentStatus: "PATCH", + contentSafety: { filtered: false, modifications: [] }, + }, + ]; + + const payload = buildCodeDiffSuccessPayload(result, options()); + + expect( + payload.files.map((file) => ("patch" in file ? file.patch : null)), + ).toEqual([ + '--- "a/src/line\\012name.ts"\n+++ "b/src/line\\012name.ts"\n@@ -1 +1 @@\n-old\n+new\n', + "--- /dev/null\n+++ b/added file.ts\n@@ -0,0 +1 @@\n+new\n", + "--- a/deleted.ts\n+++ /dev/null\n@@ -1 +0,0 @@\n-old\n", + ]); + }); + + it("preserves upstream patches whose headers are already authoritative", () => { + const result = makeResult(); + const file = result.raw.files[1]; + if (!file) throw new Error("Expected second fixture file."); + file.patch = "--- a/second.ts\n+++ b/second.ts\n@@ -1 +1 @@\n-old\n+new\n"; + + const payload = buildCodeDiffSuccessPayload(result, options()); + + expect(payload.files[1]).toMatchObject({ patch: file.patch }); + }); + it("preserves content failure fields for failed content coverage", () => { const result = makeResult(); result.raw.contentCoverage = "FAILED"; diff --git a/packages/mcp/src/shared/code-diff-response.ts b/packages/mcp/src/shared/code-diff-response.ts index db70382..165bacc 100644 --- a/packages/mcp/src/shared/code-diff-response.ts +++ b/packages/mcp/src/shared/code-diff-response.ts @@ -3,6 +3,7 @@ import type { CodeDiffResult, RawCodeDiffFile, } from "@githits/core-internal"; +import { quoteGitPath } from "./code-diff-path.js"; import type { CodeDiffView } from "./code-diff-request.js"; export type CodeDiffEnvelopeRefKind = @@ -269,13 +270,40 @@ function projectFile( modifications: file.contentSafety.modifications.map(lower), }, }; - if (file.patch != null) patch.patch = file.patch; + if (file.patch != null) { + patch.patch = bindPatchHeaders(file.patch, file.path, patch.status); + } if (file.contentOmissionReason != null) { patch.contentOmissionReason = file.contentOmissionReason; } return patch; } +/** Bind the raw diff service's content-only placeholders to its owning file. */ +function bindPatchHeaders( + patch: string, + path: string, + status: CodeDiffEnvelopeFileStatus, +): string { + const firstEnd = patch.indexOf("\n"); + if (firstEnd < 0) return patch; + const secondStart = firstEnd + 1; + const secondEnd = patch.indexOf("\n", secondStart); + if (secondEnd < 0) return patch; + + const originalFrom = patch.slice(0, firstEnd); + const originalTo = patch.slice(secondStart, secondEnd); + if (originalFrom !== "--- a/file" && originalTo !== "+++ b/file") { + return patch; + } + + const fromPath = status === "added" ? "/dev/null" : quoteGitPath(`a/${path}`); + const toPath = status === "deleted" ? "/dev/null" : quoteGitPath(`b/${path}`); + const from = originalFrom === "--- a/file" ? `--- ${fromPath}` : originalFrom; + const to = originalTo === "+++ b/file" ? `+++ ${toPath}` : originalTo; + return `${from}\n${to}\n${patch.slice(secondEnd + 1)}`; +} + function lower(value: T): Lowercase { return value.toLowerCase() as Lowercase; } diff --git a/packages/mcp/src/shared/code-diff-text.test.ts b/packages/mcp/src/shared/code-diff-text.test.ts index 0c5a03a..34cfa09 100644 --- a/packages/mcp/src/shared/code-diff-text.test.ts +++ b/packages/mcp/src/shared/code-diff-text.test.ts @@ -45,7 +45,7 @@ function envelope( const options = { useColors: false } as const; describe("formatCodeDiffTerminal", () => { - it("renders name-only as one sanitized path per line", () => { + it("renders name-only with reversible Git quoting", () => { const result = formatCodeDiffTerminal( envelope({ view: "name-only", @@ -53,12 +53,17 @@ describe("formatCodeDiffTerminal", () => { files: [ { path: "src/a.ts", pathEncoding: "utf8" }, { path: "bad\u001b[31m.ts", pathEncoding: "utf8" }, + { path: "line\u2028separator.ts", pathEncoding: "utf8" }, ], }), options, ); - expect(result).toEqual({ stdout: "src/a.ts\nbad.ts\n", stderr: undefined }); + expect(result).toEqual({ + stdout: + 'src/a.ts\n"bad\\033[31m.ts"\n"line\\342\\200\\250separator.ts"\n', + stderr: undefined, + }); }); it("renders name-status with Git status letters", () => { @@ -169,7 +174,7 @@ describe("formatCodeDiffTerminal", () => { expect(result.stdout.split("\n")[0]).toContain("-"); }); - it("binds served patch headers to the authoritative file path", () => { + it("renders normalized patches and an explicitly budgeted omission", () => { const result = formatCodeDiffTerminal( envelope({ files: [ @@ -181,7 +186,7 @@ describe("formatCodeDiffTerminal", () => { typeChanged: false, additions: 1, deletions: 1, - patch: "--- a/file\n+++ b/file\n@@ -1 +1 @@\n-old\n+new\n", + patch: "--- a/a.ts\n+++ b/a.ts\n@@ -1 +1 @@\n-old\n+new\n", contentStatus: "patch", contentSafety: { filtered: false, modifications: [] }, }, @@ -193,7 +198,7 @@ describe("formatCodeDiffTerminal", () => { typeChanged: false, additions: 1, deletions: 0, - patch: "--- a/file\n+++ b/file\n@@ -0,0 +1 @@\n+new\n", + patch: "--- /dev/null\n+++ b/added file.ts\n@@ -0,0 +1 @@\n+new\n", contentStatus: "patch", contentSafety: { filtered: false, modifications: [] }, }, @@ -205,10 +210,34 @@ describe("formatCodeDiffTerminal", () => { typeChanged: false, additions: 0, deletions: 1, - patch: "--- a/file\n+++ b/file\n@@ -1 +0,0 @@\n-old\n", + patch: "--- a/deleted.ts\n+++ /dev/null\n@@ -1 +0,0 @@\n-old\n", contentStatus: "patch", contentSafety: { filtered: false, modifications: [] }, }, + { + path: "large.ts", + pathEncoding: "utf8", + status: "modified", + modeChanged: false, + typeChanged: false, + contentStatus: "omitted", + contentOmissionReason: "content_budget", + contentSafety: { filtered: false, modifications: [] }, + }, + ], + }), + { ...options, explicitMaxPatchBytes: true }, + ); + + expect(result.stdout).toBe( + "--- a/a.ts\n+++ b/a.ts\n@@ -1 +1 @@\n-old\n+new\n--- /dev/null\n+++ b/added file.ts\n@@ -0,0 +1 @@\n+new\n--- a/deleted.ts\n+++ /dev/null\n@@ -1 +0,0 @@\n-old\nPatch omitted: large.ts (content_budget)\n", + ); + }); + + it("suppresses patch streams that cannot represent binary changes", () => { + const result = formatCodeDiffTerminal( + envelope({ + files: [ { path: "image.png", pathEncoding: "utf8", @@ -218,23 +247,54 @@ describe("formatCodeDiffTerminal", () => { contentStatus: "binary", contentSafety: { filtered: false, modifications: [] }, }, + ], + }), + options, + ); + + expect(result.stdout).toBe(""); + expect(result.exitCode).toBe(1); + }); + + it("preserves patches without backend placeholder headers", () => { + const result = formatCodeDiffTerminal( + envelope({ + files: [ { - path: "mode.sh", + path: "a.ts", pathEncoding: "utf8", status: "modified", - modeChanged: true, + modeChanged: false, typeChanged: false, - contentStatus: "metadata_only", + additions: 1, + deletions: 1, + patch: "@@ -1 +1 @@\n-old\n+new\n", + contentStatus: "patch", contentSafety: { filtered: false, modifications: [] }, }, + ], + }), + options, + ); + + expect(result.stdout).toBe("@@ -1 +1 @@\n-old\n+new\n"); + }); + + it("suppresses unexpectedly incomplete patch streams", () => { + const result = formatCodeDiffTerminal( + envelope({ + hasMoreFiles: true, + files: [ { - path: "large.ts", + path: "one.ts", pathEncoding: "utf8", status: "modified", modeChanged: false, typeChanged: false, - contentStatus: "omitted", - contentOmissionReason: "content_budget\u001b[31m", + additions: 1, + deletions: 1, + patch: "--- a/one.ts\n+++ b/one.ts\n@@ -1 +1 @@\n-old\n+new\n", + contentStatus: "patch", contentSafety: { filtered: false, modifications: [] }, }, ], @@ -242,33 +302,62 @@ describe("formatCodeDiffTerminal", () => { options, ); - expect(result.stdout).toBe( - "--- a/a.ts\n+++ b/a.ts\n@@ -1 +1 @@\n-old\n+new\n--- /dev/null\n+++ b/added file.ts\n@@ -0,0 +1 @@\n+new\n--- a/deleted.ts\n+++ /dev/null\n@@ -1 +0,0 @@\n-old\nBinary file image.png differs\nMetadata changed: mode.sh\nPatch omitted: large.ts (content_budget)\n", - ); + expect(result.stdout).toBe(""); + expect(result.exitCode).toBe(1); + expect(result.stderr).toContain("Patch output was suppressed"); }); - it("preserves patches without backend placeholder headers", () => { + it("keeps explicitly file-limited patch streams successful", () => { const result = formatCodeDiffTerminal( envelope({ + hasMoreFiles: true, files: [ { - path: "a.ts", + path: "one.ts", pathEncoding: "utf8", status: "modified", modeChanged: false, typeChanged: false, additions: 1, deletions: 1, - patch: "@@ -1 +1 @@\n-old\n+new\n", + patch: "--- a/one.ts\n+++ b/one.ts\n@@ -1 +1 @@\n-old\n+new\n", contentStatus: "patch", contentSafety: { filtered: false, modifications: [] }, }, ], }), - options, + { ...options, explicitMaxFiles: true }, ); - expect(result.stdout).toBe("@@ -1 +1 @@\n-old\n+new\n"); + expect(result.stdout).toContain("--- a/one.ts"); + expect(result.exitCode).toBeUndefined(); + }); + + it("does not let explicit limits authorize unrelated patch failures", () => { + const result = formatCodeDiffTerminal( + envelope({ + contentCoverage: "failed", + files: [ + { + path: "one.ts", + pathEncoding: "utf8", + status: "modified", + modeChanged: false, + typeChanged: false, + contentStatus: "unavailable", + contentSafety: { filtered: false, modifications: [] }, + }, + ], + }), + { + ...options, + explicitMaxFiles: true, + explicitMaxPatchBytes: true, + }, + ); + + expect(result.stdout).toBe(""); + expect(result.exitCode).toBe(1); }); it("keeps an empty authoritative diff silent in plain mode", () => { @@ -278,6 +367,30 @@ describe("formatCodeDiffTerminal", () => { }); }); + it("does not treat an all-unprojectable result as an empty diff", () => { + const result = formatCodeDiffTerminal( + envelope({ + summary: { + filesChanged: 1, + added: 0, + deleted: 0, + modified: 1, + modeChanged: 0, + typeChanged: 0, + inventoryComplete: true, + unprojectableFiles: 1, + }, + files: [], + }), + options, + ); + + expect(result.stdout).toBe(""); + expect(result.exitCode).toBe(1); + expect(result.stderr).toContain("1 matching path(s)"); + expect(result.stderr).toContain("Patch output was suppressed"); + }); + it("renders completeness, scope, encoding, safety, and content warnings", () => { const result = formatCodeDiffTerminal( envelope({ diff --git a/packages/mcp/src/shared/code-diff-text.ts b/packages/mcp/src/shared/code-diff-text.ts index a88fd6f..13ff4b2 100644 --- a/packages/mcp/src/shared/code-diff-text.ts +++ b/packages/mcp/src/shared/code-diff-text.ts @@ -1,3 +1,4 @@ +import { quoteGitPath } from "./code-diff-path.js"; import type { LeanCodeDiffEnvelope, LeanCodeDiffFile, @@ -10,11 +11,14 @@ import { sanitizeTerminalText } from "./resolve-target-response.js"; export interface FormatCodeDiffTerminalOptions { useColors: boolean; verbose?: boolean; + explicitMaxFiles?: boolean; + explicitMaxPatchBytes?: boolean; } export interface FormattedCodeDiffTerminal { stdout: string; stderr?: string; + exitCode?: 1; } /** Render a Git-like primary stream plus truthful bounded-evidence diagnostics. */ @@ -22,22 +26,32 @@ export function formatCodeDiffTerminal( envelope: LeanCodeDiffEnvelope, options: FormatCodeDiffTerminalOptions, ): FormattedCodeDiffTerminal { - const stdout = formatPrimaryOutput(envelope); const diagnostics = buildDiagnostics(envelope, options); + const suppressPatch = shouldSuppressPatch(envelope, options); + if (suppressPatch) { + diagnostics.push( + warn( + "Patch output was suppressed because the result is not safely applicable. Use --json to inspect the partial evidence.", + options, + ), + ); + } return { - stdout, + stdout: suppressPatch ? "" : formatPrimaryOutput(envelope), stderr: diagnostics.length > 0 ? `${diagnostics.join("\n")}\n` : undefined, + ...(suppressPatch ? { exitCode: 1 as const } : {}), }; } function formatPrimaryOutput(envelope: LeanCodeDiffEnvelope): string { switch (envelope.view) { case "name-only": - return formatLines(envelope.files.map((file) => safe(file.path))); + return formatLines(envelope.files.map((file) => quoteGitPath(file.path))); case "name-status": return formatLines( envelope.files.map( - (file) => `${statusLetter(requireStatus(file))}\t${safe(file.path)}`, + (file) => + `${statusLetter(requireStatus(file))}\t${quoteGitPath(file.path)}`, ), ); case "stat": @@ -59,7 +73,7 @@ function formatStat(envelope: LeanCodeDiffEnvelope): string { for (const file of envelope.files) { const stat = requireStat(file); - const path = safe(stat.path); + const path = quoteGitPath(stat.path); if (stat.additions !== undefined && stat.deletions !== undefined) { rows.push({ path, @@ -132,7 +146,7 @@ function formatPatches(files: LeanCodeDiffFile[]): string { for (const file of files) { const patchFile = requirePatch(file); if (patchFile.patch !== undefined) { - const patch = bindPatchHeaders(patchFile); + const patch = patchFile.patch; output += patch; if (!patch.endsWith("\n")) output += "\n"; continue; @@ -142,23 +156,8 @@ function formatPatches(files: LeanCodeDiffFile[]): string { return output; } -const RAW_DIFF_PLACEHOLDER_HEADERS = "--- a/file\n+++ b/file\n"; - -/** Bind the raw diff service's content-only placeholders to its owning file. */ -function bindPatchHeaders(file: LeanCodeDiffPatchFile): string { - const patch = file.patch; - if (patch === undefined || !patch.startsWith(RAW_DIFF_PLACEHOLDER_HEADERS)) { - return patch ?? ""; - } - - const path = safe(file.path); - const fromPath = file.status === "added" ? "/dev/null" : `a/${path}`; - const toPath = file.status === "deleted" ? "/dev/null" : `b/${path}`; - return `--- ${fromPath}\n+++ ${toPath}\n${patch.slice(RAW_DIFF_PLACEHOLDER_HEADERS.length)}`; -} - function patchFallback(file: LeanCodeDiffPatchFile): string { - const path = safe(file.path); + const path = quoteGitPath(file.path); switch (file.contentStatus) { case "binary": return `Binary file ${path} differs`; @@ -177,6 +176,39 @@ function patchFallback(file: LeanCodeDiffPatchFile): string { } } +function shouldSuppressPatch( + envelope: LeanCodeDiffEnvelope, + options: FormatCodeDiffTerminalOptions, +): boolean { + if (envelope.view !== "patch") return false; + if ( + !envelope.summary.inventoryComplete || + envelope.summary.unprojectableFiles > 0 || + (envelope.hasMoreFiles && !options.explicitMaxFiles) || + envelope.contentCoverage === "failed" || + (envelope.contentCoverage === "partial" && !options.explicitMaxPatchBytes) + ) { + return true; + } + if (envelope.files.length === 0) return false; + + return envelope.files.some((file) => { + const patchFile = requirePatch(file); + if ( + patchFile.pathEncoding === "byte_escaped" || + patchFile.contentSafety.filtered + ) { + return true; + } + if (patchFile.patch !== undefined) return false; + return !( + options.explicitMaxPatchBytes && + patchFile.contentStatus === "omitted" && + patchFile.contentOmissionReason === "content_budget" + ); + }); +} + function buildDiagnostics( envelope: LeanCodeDiffEnvelope, options: FormatCodeDiffTerminalOptions, diff --git a/src/commands/code/diff.test.ts b/src/commands/code/diff.test.ts index 1393f5d..09f4174 100644 --- a/src/commands/code/diff.test.ts +++ b/src/commands/code/diff.test.ts @@ -102,7 +102,35 @@ describe("codeDiffAction", () => { log.mockRestore(); }); - it("keeps failed post-inventory content as successful evidence", async () => { + it("emits authoritative patch headers in JSON", async () => { + const log = spyOn(console, "log").mockImplementation(() => {}); + const result = structuredClone(defaultCodeDiffResult); + result.raw.files[0] = { + ...result.raw.files[0]!, + patch: "--- a/file\n+++ b/file\n@@ -1 +1 @@\n-old\n+new\n", + }; + + await codeDiffAction( + "npm:express", + "4.18.1..4.18.2", + undefined, + { json: true }, + dependencies({ + codeNavigationService: createMockCodeNavigationService({ + codeDiff: mock(() => Promise.resolve(result)), + }), + }), + ); + + const payload = JSON.parse(log.mock.calls[0]?.[0] as string); + expect(payload.files[0].path).toBe("lib/express.js"); + expect(payload.files[0].patch).toStartWith( + "--- a/lib/express.js\n+++ b/lib/express.js\n", + ); + log.mockRestore(); + }); + + it("suppresses failed post-inventory patch content and exits nonzero", async () => { const stdout = spyOn(process.stdout, "write").mockImplementation( (() => true) as typeof process.stdout.write, ); @@ -125,23 +153,37 @@ describe("codeDiffAction", () => { contentStatus: "UNAVAILABLE", }; - await codeDiffAction( - "npm:express", - "4.18.1..4.18.2", - undefined, - {}, - dependencies({ - codeNavigationService: createMockCodeNavigationService({ - codeDiff: mock(() => Promise.resolve(result)), + const exit = spyOn(process, "exit").mockImplementation(() => { + throw new Error("process.exit"); + }); + + try { + await codeDiffAction( + "npm:express", + "4.18.1..4.18.2", + undefined, + {}, + dependencies({ + codeNavigationService: createMockCodeNavigationService({ + codeDiff: mock(() => Promise.resolve(result)), + }), }), - }), - ); + ); + } catch { + // process.exit is mocked as a throw. + } expect(String(stderr.mock.calls[0]?.[0])).toContain( "Requested content failed", ); + expect(String(stderr.mock.calls[0]?.[0])).toContain( + "Patch output was suppressed", + ); + expect(stdout).not.toHaveBeenCalled(); + expect(exit).toHaveBeenCalledWith(1); stdout.mockRestore(); stderr.mockRestore(); + exit.mockRestore(); }); it.each([ @@ -364,6 +406,20 @@ describe("registerCodeDiffCommand", () => { ); }); + it("shows the two concrete invocation forms in help", () => { + const program = new Command("githits"); + const command = registerCodeDiffCommand(program.command("code")); + const help = command.helpInformation(); + + expect(help).toContain( + "githits code diff [options] .. [-- ]", + ); + expect(help).toContain( + "githits code diff [options] --repo-url .. [-- ]", + ); + expect(help).not.toContain("[target-or-range] [range-or-path-glob]"); + }); + it.each(["src/**/*.ts", "--"])( "accepts path glob %s when the raw argv suffix is -- ", async (pathGlob) => { diff --git a/src/commands/code/diff.ts b/src/commands/code/diff.ts index 3cce148..18594e5 100644 --- a/src/commands/code/diff.ts +++ b/src/commands/code/diff.ts @@ -61,6 +61,7 @@ export async function codeDiffAction( deps: CodeDiffCommandDependencies, pathGlobAfterDoubleDash = false, ): Promise { + let terminalExitCode: 1 | undefined; try { requireAuth(deps); } catch (error) { @@ -126,9 +127,12 @@ export async function codeDiffAction( const formatted = formatCodeDiffTerminal(payload, { useColors: shouldUseColors(), verbose: options.verbose ?? false, + explicitMaxFiles: options.maxFiles !== undefined, + explicitMaxPatchBytes: options.maxPatchBytes !== undefined, }); if (formatted.stdout) process.stdout.write(formatted.stdout); if (formatted.stderr) process.stderr.write(formatted.stderr); + terminalExitCode = formatted.exitCode; } catch (error) { handleCodeNavCommandError( error, @@ -136,6 +140,8 @@ export async function codeDiffAction( formatCodeDiffError, ); } + + if (terminalExitCode !== undefined) process.exit(terminalExitCode); } function buildCliCodeDiffParams( @@ -289,13 +295,16 @@ export function registerCodeDiffCommand( .command("diff") .summary("Compare two dependency source trees") .description(CODE_DIFF_DESCRIPTION) + .usage( + "[options] .. [-- ]\n githits code diff [options] --repo-url .. [-- ]", + ) .argument( "[target-or-range]", - "Unversioned target, or the range when using --repo-url.", + "Package mode: target. Repository mode: from..to range.", ) .argument( "[range-or-path-glob]", - "from..to range, or the path glob when using --repo-url.", + "Package mode: from..to range. Repository mode: path glob.", ) .argument( "[path-glob]", From 91076f94ae52eaf9388581a411abfd05d109585d Mon Sep 17 00:00:00 2001 From: Juha Litola Date: Tue, 18 Aug 2026 12:01:45 +0300 Subject: [PATCH 17/19] fix: refine CodeDiff CLI ergonomics Accept the live backend patch-budget omission reason and improve diff-specific help, suppression diagnostics, and recovery guidance. --- docs/implementation/cli-commands.md | 6 ++- docs/implementation/code-diff.md | 12 +++-- packages/mcp/src/shared/code-diff-request.ts | 8 +-- .../mcp/src/shared/code-diff-text.test.ts | 37 +++++++++++++- packages/mcp/src/shared/code-diff-text.ts | 49 +++++++++++++++++-- src/commands/code/diff.test.ts | 33 +++++++++++++ src/commands/code/diff.ts | 7 ++- 7 files changed, 135 insertions(+), 17 deletions(-) diff --git a/docs/implementation/cli-commands.md b/docs/implementation/cli-commands.md index 2e7bbca..59755c8 100644 --- a/docs/implementation/cli-commands.md +++ b/docs/implementation/cli-commands.md @@ -533,8 +533,10 @@ control characters. Empty authoritative diffs exit 0. Caller-selected `--max-files` and `--max-patch-bytes` bounds may intentionally produce partial patches and still exit 0 with warnings. Unexpectedly incomplete or non-applicable plain patches are suppressed and exit 1; name, stat, and JSON views preserve -their structured partial evidence. Request, auth, resolution, and backend -errors also exit 1. +their structured partial evidence. Suppression diagnostics name binary and +metadata-only causes and direct terminal users to stat/name views. Patch output +is applicable unified-diff content but may omit Git metadata such as index and +mode headers. Request, auth, resolution, and backend errors also exit 1. This is a silent dogfood surface. It is normally registered and documented for maintainers, but no MCP tool, MCP instruction, Agent Skill, or plugin guidance diff --git a/docs/implementation/code-diff.md b/docs/implementation/code-diff.md index 68f4296..054175b 100644 --- a/docs/implementation/code-diff.md +++ b/docs/implementation/code-diff.md @@ -114,10 +114,14 @@ suppresses stdout and exits 1 when unexpected truncation, failed or unavailable content, binary/metadata-only changes, display-only paths, unprojectable files, or content-safety changes would make the stream unsafe to apply. An explicit `--max-files` authorizes file-count truncation, and an explicit -`--max-patch-bytes` authorizes `content_budget` omissions; neither authorizes -unrelated failure classes. Validation, authentication, resolution, and -raw-field errors exit 1 through the shared CLI error envelope. No `code_diff` -MCP tool, instruction, skill, or plugin promotion exists during this phase. +`--max-patch-bytes` authorizes aggregate patch-budget omissions; neither +authorizes unrelated failure classes. Suppression diagnostics name +binary/metadata-only causes and direct humans to stat/name views while JSON +retains structured partial evidence. The applicable patch stream is unified +diff content; the backend does not provide Git index or mode headers. +Validation, authentication, resolution, and raw-field errors exit 1 through +the shared CLI error envelope. No `code_diff` MCP tool, instruction, skill, or +plugin promotion exists during this phase. ## Key reference files diff --git a/packages/mcp/src/shared/code-diff-request.ts b/packages/mcp/src/shared/code-diff-request.ts index 49c28f9..852da5b 100644 --- a/packages/mcp/src/shared/code-diff-request.ts +++ b/packages/mcp/src/shared/code-diff-request.ts @@ -137,10 +137,10 @@ function buildTarget(input: CodeDiffRequestInput): CodeDiffParams["target"] { function parseTarget(raw: string): CodeNavigationTarget { try { return parseCodeNavigationTargetSpec(raw); - } catch (error) { - if (error instanceof InvalidPackageSpecError) throw error; - const message = error instanceof Error ? error.message : "Invalid target."; - throw invalid(message); + } catch { + throw invalid( + "Invalid CodeDiff target. Expected an unversioned package target `:` (for example `npm:express`) or an unversioned repository target (for example `github:expressjs/express`).", + ); } } diff --git a/packages/mcp/src/shared/code-diff-text.test.ts b/packages/mcp/src/shared/code-diff-text.test.ts index 34cfa09..bb93391 100644 --- a/packages/mcp/src/shared/code-diff-text.test.ts +++ b/packages/mcp/src/shared/code-diff-text.test.ts @@ -221,7 +221,7 @@ describe("formatCodeDiffTerminal", () => { modeChanged: false, typeChanged: false, contentStatus: "omitted", - contentOmissionReason: "content_budget", + contentOmissionReason: "total_patch_bytes", contentSafety: { filtered: false, modifications: [] }, }, ], @@ -230,10 +230,36 @@ describe("formatCodeDiffTerminal", () => { ); expect(result.stdout).toBe( - "--- a/a.ts\n+++ b/a.ts\n@@ -1 +1 @@\n-old\n+new\n--- /dev/null\n+++ b/added file.ts\n@@ -0,0 +1 @@\n+new\n--- a/deleted.ts\n+++ /dev/null\n@@ -1 +0,0 @@\n-old\nPatch omitted: large.ts (content_budget)\n", + "--- a/a.ts\n+++ b/a.ts\n@@ -1 +1 @@\n-old\n+new\n--- /dev/null\n+++ b/added file.ts\n@@ -0,0 +1 @@\n+new\n--- a/deleted.ts\n+++ /dev/null\n@@ -1 +0,0 @@\n-old\nPatch omitted: large.ts (total_patch_bytes)\n", ); }); + it("accepts both backend names for an explicit patch-budget omission", () => { + for (const reason of ["content_budget", "total_patch_bytes"]) { + const result = formatCodeDiffTerminal( + envelope({ + contentCoverage: "partial", + files: [ + { + path: "large.ts", + pathEncoding: "utf8", + status: "modified", + modeChanged: false, + typeChanged: false, + contentStatus: "omitted", + contentOmissionReason: reason, + contentSafety: { filtered: false, modifications: [] }, + }, + ], + }), + { ...options, explicitMaxPatchBytes: true }, + ); + + expect(result.stdout).toContain(`Patch omitted: large.ts (${reason})`); + expect(result.exitCode).toBeUndefined(); + } + }); + it("suppresses patch streams that cannot represent binary changes", () => { const result = formatCodeDiffTerminal( envelope({ @@ -254,6 +280,10 @@ describe("formatCodeDiffTerminal", () => { expect(result.stdout).toBe(""); expect(result.exitCode).toBe(1); + expect(result.stderr).toContain( + "1 binary change cannot be represented as an applicable text patch", + ); + expect(result.stderr).toContain("Use --stat or --name-status"); }); it("preserves patches without backend placeholder headers", () => { @@ -434,8 +464,11 @@ describe("formatCodeDiffTerminal", () => { expect(result.stderr).toContain("repository-wide"); expect(result.stderr).toContain("inventory is incomplete"); expect(result.stderr).toContain("More matching files"); + expect(result.stderr).toContain("Add a path glob after `--`"); expect(result.stderr).toContain("2 matching path(s)"); expect(result.stderr).toContain("Requested content failed"); + expect(result.stderr).toContain("Use --stat or --name-status"); + expect(result.stderr).toContain("--json"); expect(result.stderr).toContain("display-only byte escapes"); expect(result.stderr).toContain("modified for content safety"); }); diff --git a/packages/mcp/src/shared/code-diff-text.ts b/packages/mcp/src/shared/code-diff-text.ts index 13ff4b2..53647a8 100644 --- a/packages/mcp/src/shared/code-diff-text.ts +++ b/packages/mcp/src/shared/code-diff-text.ts @@ -21,6 +21,11 @@ export interface FormattedCodeDiffTerminal { exitCode?: 1; } +const EXPLICIT_PATCH_BUDGET_OMISSION_REASONS = new Set([ + "content_budget", + "total_patch_bytes", +]); + /** Render a Git-like primary stream plus truthful bounded-evidence diagnostics. */ export function formatCodeDiffTerminal( envelope: LeanCodeDiffEnvelope, @@ -31,7 +36,7 @@ export function formatCodeDiffTerminal( if (suppressPatch) { diagnostics.push( warn( - "Patch output was suppressed because the result is not safely applicable. Use --json to inspect the partial evidence.", + "Patch output was suppressed because the result is not safely applicable. Use --stat or --name-status to inspect changes, or --json for structured partial evidence.", options, ), ); @@ -204,7 +209,10 @@ function shouldSuppressPatch( return !( options.explicitMaxPatchBytes && patchFile.contentStatus === "omitted" && - patchFile.contentOmissionReason === "content_budget" + patchFile.contentOmissionReason !== undefined && + EXPLICIT_PATCH_BUDGET_OMISSION_REASONS.has( + patchFile.contentOmissionReason, + ) ); }); } @@ -230,9 +238,12 @@ function buildDiagnostics( ); } if (envelope.hasMoreFiles) { + const recovery = envelope.scope.pathGlob + ? "Narrow the path glob or raise --max-files (up to 300)." + : "Add a path glob after `--` or raise --max-files (up to 300)."; lines.push( warn( - `More matching files exist than the ${envelope.files.length} returned. Narrow the glob or raise --max-files.`, + `More matching files exist than the ${envelope.files.length} returned. ${recovery}`, options, ), ); @@ -247,7 +258,12 @@ function buildDiagnostics( } if (envelope.contentCoverage === "partial") { lines.push( - warn("Requested content is partial; inspect per-file status.", options), + warn( + envelope.view === "patch" + ? "Requested content is partial; inspect per-file status with --stat or --json." + : "Requested content is partial; inspect the returned rows or use --json.", + options, + ), ); } else if (envelope.contentCoverage === "failed") { const failure = envelope.contentFailure; @@ -284,6 +300,31 @@ function buildDiagnostics( warn(`${filtered} patch(es) were modified for content safety.`, options), ); } + if (envelope.view === "patch") { + const binary = envelope.files.filter( + (file) => "contentStatus" in file && file.contentStatus === "binary", + ).length; + if (binary > 0) { + lines.push( + warn( + `${binary} binary ${binary === 1 ? "change" : "changes"} cannot be represented as an applicable text patch.`, + options, + ), + ); + } + const metadataOnly = envelope.files.filter( + (file) => + "contentStatus" in file && file.contentStatus === "metadata_only", + ).length; + if (metadataOnly > 0) { + lines.push( + warn( + `${metadataOnly} metadata-only ${metadataOnly === 1 ? "change" : "changes"} cannot be represented as an applicable text patch.`, + options, + ), + ); + } + } return lines; } diff --git a/src/commands/code/diff.test.ts b/src/commands/code/diff.test.ts index 09f4174..019e4c1 100644 --- a/src/commands/code/diff.test.ts +++ b/src/commands/code/diff.test.ts @@ -337,6 +337,37 @@ describe("codeDiffAction", () => { error.mockRestore(); exit.mockRestore(); }); + + it("uses diff-specific target guidance", async () => { + const codeDiff = mock(() => Promise.resolve(defaultCodeDiffResult)); + const error = spyOn(console, "error").mockImplementation(() => {}); + const exit = spyOn(process, "exit").mockImplementation(() => { + throw new Error("process.exit"); + }); + + try { + await codeDiffAction( + "express", + "1.0.0..2.0.0", + undefined, + {}, + dependencies({ + codeNavigationService: createMockCodeNavigationService({ codeDiff }), + }), + ); + } catch { + // process.exit is mocked as a throw. + } + + expect(error.mock.calls[0]?.[0]).toContain( + "unversioned package target `:`", + ); + expect(error.mock.calls[0]?.[0]).not.toContain("[@]"); + expect(error.mock.calls[0]?.[0]).not.toContain("[#ref|@ref]"); + expect(codeDiff).not.toHaveBeenCalled(); + error.mockRestore(); + exit.mockRestore(); + }); }); describe("formatCodeDiffError", () => { @@ -418,6 +449,8 @@ describe("registerCodeDiffCommand", () => { "githits code diff [options] --repo-url .. [-- ]", ); expect(help).not.toContain("[target-or-range] [range-or-path-glob]"); + expect(help).toContain("Target examples: `npm:express`"); + expect(help).toContain("suppressed patch output exits 1"); }); it.each(["src/**/*.ts", "--"])( diff --git a/src/commands/code/diff.ts b/src/commands/code/diff.ts index 18594e5..94daaf3 100644 --- a/src/commands/code/diff.ts +++ b/src/commands/code/diff.ts @@ -285,7 +285,12 @@ is always left-to-right; three-dot merge-base syntax is not supported. After \`--\`, one optional applies a repository-relative bounded glob. It supports *, ?, and an exact ** path component; it is not a full Git -pathspec. A backslash escapes one following non-slash character.`; +pathspec. A backslash escapes one following non-slash character. + +Target examples: \`npm:express\` or \`github:expressjs/express\`; keep versions +and refs in ... Empty diffs exit 0; suppressed patch output exits 1. +Patch output is applicable unified-diff content and may omit Git metadata such +as index and mode headers.`; export function registerCodeDiffCommand( codeCommand: Command, From 8ecdab7f6fc6b6d6c02ba9e5cbdf5af6be6eb8cb Mon Sep 17 00:00:00 2001 From: Juha Litola Date: Tue, 18 Aug 2026 12:10:24 +0300 Subject: [PATCH 18/19] fix: restore CodeDiff registry guidance Keep invalid-target help diff-specific while listing the package registries accepted by the target parser. --- packages/mcp/src/shared/code-diff-request.test.ts | 12 ++++++++++++ packages/mcp/src/shared/code-diff-request.ts | 4 ++-- src/commands/code/diff.test.ts | 2 ++ 3 files changed, 16 insertions(+), 2 deletions(-) diff --git a/packages/mcp/src/shared/code-diff-request.test.ts b/packages/mcp/src/shared/code-diff-request.test.ts index 12afe8c..15d75b3 100644 --- a/packages/mcp/src/shared/code-diff-request.test.ts +++ b/packages/mcp/src/shared/code-diff-request.test.ts @@ -140,6 +140,18 @@ describe("buildCodeDiffParams", () => { invalid({ repoUrl: "npm:express", range: "1..2" }, "repository target"); }); + it("gives supported registries without suggesting embedded versions", () => { + try { + buildCodeDiffParams({ target: "nppm:express", range: "1..2" }); + throw new Error("Expected target validation to fail."); + } catch (error) { + expect(error).toBeInstanceOf(InvalidPackageSpecError); + expect((error as Error).message).toContain("supported registries"); + expect((error as Error).message).toContain("npm"); + expect((error as Error).message).not.toContain("[@]"); + } + }); + it("rejects versions and refs embedded in target addressing", () => { invalid( { target: "npm:express@4.18.1", range: "1..2" }, diff --git a/packages/mcp/src/shared/code-diff-request.ts b/packages/mcp/src/shared/code-diff-request.ts index 852da5b..8ff6fec 100644 --- a/packages/mcp/src/shared/code-diff-request.ts +++ b/packages/mcp/src/shared/code-diff-request.ts @@ -3,7 +3,7 @@ import type { CodeNavigationTarget, } from "@githits/core-internal"; import { parseCodeNavigationTargetSpec } from "./code-navigation-target.js"; -import { InvalidPackageSpecError } from "./package-spec.js"; +import { InvalidPackageSpecError, KNOWN_REGISTRIES } from "./package-spec.js"; export type CodeDiffView = "patch" | "stat" | "name-only" | "name-status"; @@ -139,7 +139,7 @@ function parseTarget(raw: string): CodeNavigationTarget { return parseCodeNavigationTargetSpec(raw); } catch { throw invalid( - "Invalid CodeDiff target. Expected an unversioned package target `:` (for example `npm:express`) or an unversioned repository target (for example `github:expressjs/express`).", + `Invalid CodeDiff target. Expected an unversioned package target \`:\` (for example \`npm:express\`; supported registries: ${KNOWN_REGISTRIES.join(", ")}) or an unversioned repository target (for example \`github:expressjs/express\`).`, ); } } diff --git a/src/commands/code/diff.test.ts b/src/commands/code/diff.test.ts index 019e4c1..59b0051 100644 --- a/src/commands/code/diff.test.ts +++ b/src/commands/code/diff.test.ts @@ -364,6 +364,8 @@ describe("codeDiffAction", () => { ); expect(error.mock.calls[0]?.[0]).not.toContain("[@]"); expect(error.mock.calls[0]?.[0]).not.toContain("[#ref|@ref]"); + expect(error.mock.calls[0]?.[0]).toContain("supported registries"); + expect(error.mock.calls[0]?.[0]).toContain("npm"); expect(codeDiff).not.toHaveBeenCalled(); error.mockRestore(); exit.mockRestore(); From 4da1ed85281ff0f452a1b8074571e11d1ce43c5c Mon Sep 17 00:00:00 2001 From: Juha Litola Date: Tue, 18 Aug 2026 12:27:57 +0300 Subject: [PATCH 19/19] fix: clarify repository-wide CodeDiff scope Explain unknown package scope in user terms and provide context-aware path-glob recovery without exposing backend proof terminology. --- .../mcp/src/shared/code-diff-text.test.ts | 37 ++++++++++++++++++- packages/mcp/src/shared/code-diff-text.ts | 15 +++++--- 2 files changed, 45 insertions(+), 7 deletions(-) diff --git a/packages/mcp/src/shared/code-diff-text.test.ts b/packages/mcp/src/shared/code-diff-text.test.ts index bb93391..31d8def 100644 --- a/packages/mcp/src/shared/code-diff-text.test.ts +++ b/packages/mcp/src/shared/code-diff-text.test.ts @@ -461,7 +461,10 @@ describe("formatCodeDiffTerminal", () => { options, ); - expect(result.stderr).toContain("repository-wide"); + expect(result.stderr).toContain( + "Showing changes for the entire repository", + ); + expect(result.stderr).toContain("Unrelated files may be included"); expect(result.stderr).toContain("inventory is incomplete"); expect(result.stderr).toContain("More matching files"); expect(result.stderr).toContain("Add a path glob after `--`"); @@ -473,6 +476,38 @@ describe("formatCodeDiffTerminal", () => { expect(result.stderr).toContain("modified for content safety"); }); + it("explains that an unknown package scope applies the glob repository-wide", () => { + const result = formatCodeDiffTerminal( + envelope({ + scope: { status: "unknown", pathGlob: "packages/**/*.ts" }, + }), + options, + ); + + expect(result.stderr).toContain( + "the path glob was applied across the entire repository", + ); + expect(result.stderr).toContain( + "Matching files from other packages may be included", + ); + expect(result.stderr).toContain("narrow the path glob if needed"); + expect(result.stderr).not.toContain("add a path glob"); + }); + + it("offers a path glob when unknown package scope is otherwise complete", () => { + const result = formatCodeDiffTerminal( + envelope({ scope: { status: "unknown" } }), + options, + ); + + expect(result.stderr).toContain( + "Showing changes for the entire repository", + ); + expect(result.stderr).toContain( + "Add a path glob after `--` to narrow the diff", + ); + }); + it("adds full exact identity and scope facts only in verbose diagnostics", () => { const result = formatCodeDiffTerminal( envelope({ diff --git a/packages/mcp/src/shared/code-diff-text.ts b/packages/mcp/src/shared/code-diff-text.ts index 53647a8..2b6135a 100644 --- a/packages/mcp/src/shared/code-diff-text.ts +++ b/packages/mcp/src/shared/code-diff-text.ts @@ -225,12 +225,15 @@ function buildDiagnostics( if (options.verbose) appendVerboseContext(lines, envelope); if (envelope.scope.status === "unknown") { - lines.push( - warn( - "Package ownership could not be proved; this evidence is repository-wide.", - options, - ), - ); + let message = + "Showing changes for the entire repository because GitHits could not identify this package's directory. Unrelated files may be included."; + if (envelope.scope.pathGlob) { + message = + "GitHits could not identify this package's directory, so the path glob was applied across the entire repository. Matching files from other packages may be included; narrow the path glob if needed."; + } else if (!envelope.hasMoreFiles) { + message += " Add a path glob after `--` to narrow the diff."; + } + lines.push(warn(message, options)); } if (!envelope.summary.inventoryComplete) { lines.push(