Skip to content

feat!: AIR tool call contract, exact diff patches, and fixes for every client - #530

Open
nikita-ashihmin wants to merge 41 commits into
mainfrom
nikita.ashikhmin/acp-patch-content
Open

nikita-ashihmin wants to merge 41 commits into
mainfrom
nikita.ashikhmin/acp-patch-content

Conversation

@nikita-ashihmin

@nikita-ashihmin nikita-ashihmin commented Sep 22, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

One PR with separate commits (the earlier stack #544 and #545 is folded back in).

Diff patch

  • Advertise the diffPatch AIR capability and send a file change as one compact Git patch only to a client that declares it.
  • src/GitPatch.ts builds valid patches: paths and quoting, rename headers, file modes, CRLF, the exact \ No newline at end of file marker, and hunks whose starts and counts are in order without overlap. Anything else falls back to the standard ACP diff.

For every client

Clients that do not declare _meta.jetbrains.air (Zed, plain ACP clients) get the same information as on main.

  • A tool_call_update omits top-level fields that did not change since the last report of the same tool call (ACP merge semantics). The _meta of each report stays complete.
  • Bug fixes: replayed command output sent once, replayed tool calls end with a terminal status, generated images sent once, unique MCP startup ids, dynamic tool results forwarded, no output after a tool call ended, permission requests keep the status and kind of a started tool call, an empty fuzzy search clears old locations, the subagent buffer keeps every notification and counts its cap in serialized bytes.

For AIR only

  • One fact in one field: input in rawInput, the file text of an edit only in the diff, command output only through terminal_output_delta, stdin in terminal_input, no display copies of the input (rawInputRendering), no MCP progress lines.
  • The Markdown plan streams as plan_update with _meta.jetbrains.air.contentDelta (planContentDelta).
  • AIR-only keys move under _meta.jetbrains.air.* and are no longer sent to other clients (hence feat!; only AIR reads them).

Structure and docs

  • src/tool-calls/: one reporter per tool kind, one ClientCapabilities, one AcpToolCallRenderer, the changed-field filter.
  • docs/air-extensions.md replaces the per-extension AIR docs.

Tests

  • npm run typecheck, npm test: 976 passed.
  • Scenario tests for plain, Zed, and AIR clients: every outbound message is recorded (including _auth/status_update); standard ACP messages are validated against the ACP JSON schema, AIR extension updates get an envelope check; plain and Zed are compared with a recorded main baseline in code; AIR keeps compact golden recordings.

@nikita-ashihmin nikita-ashihmin changed the title feat: add ACP patch content capability feat: AIR tool call contract, exact diff patches, and no duplicated tool call data Sep 23, 2026
@nikita-ashihmin
nikita-ashihmin requested a balanced review from Copilot September 23, 2026 10:51

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

One critical and six moderate correctness issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity · 1 Medium severity

Open (2)
What changed in this PR

Implements the AIR tool-call contract, sparse field reporting, exact Git patches, and improved tool/subagent lifecycle handling while preserving non-AIR compatibility.

Changes:

  • Adds capability-aware tool reporters and AIR-specific metadata/rendering.
  • Adds exact Git patch and plan-delta support.
  • Expands protocol, lifecycle, replay, buffering, and scenario coverage.

Unresolved findings:

  • Critical (1 vote): src/CodexAcpServer.ts:2297 must force terminal status when replaying history.
  • Moderate (2 votes): src/GitPatch.ts:103 accepts malformed no-newline markers.
  • Moderate (1 vote): src/ToolCallReportingConnection.ts:49 does not restore full permission-call fields after cancellation.
  • Moderate (1 vote): src/ToolCallReportingConnection.ts:41 repeats unchanged permission-call fields.
  • Moderate (1 vote): src/subagents/PendingNotificationBuffer.ts:34 undercounts JSON-escaped buffered bytes.
  • Moderate (1 vote): src/tool-calls/reporters/FileChangeReporter.ts:110 drops pure renames with empty patches.
  • Moderate (1 vote): src/tool-calls/reporters/ImageGenerationReporter.ts:61 duplicates images for non-AIR clients.
  • Nit (1 vote): README.md:17 incorrectly presents the AIR-only shape as universal.
File Reviewed change
src/​ToolCallReportingConnection.ts Filters changed fields; two permission-flow issues remain.
src/​tool-calls/​ToolFacts.ts Defines normalized tool facts.
src/​tool-calls/​reporters/​WebSearchReporter.ts Reports web searches.
src/​tool-calls/​reporters/​ToolStatus.ts Maps tool statuses.
src/​tool-calls/​reporters/​SubagentActivityReporter.ts Reports subagent activity.
src/​tool-calls/​reporters/​SandboxPermissionReporter.ts Reports sandbox permissions.
src/​tool-calls/​reporters/​PlanReviewReporter.ts Reports plan reviews.
src/​tool-calls/​reporters/​McpToolReporter.ts Reports MCP tools.
src/​tool-calls/​reporters/​McpStartupReporter.ts Reports MCP startup.
src/​tool-calls/​reporters/​ImageViewReporter.ts Reports viewed images.
src/​tool-calls/​reporters/​ImageGenerationReporter.ts Reports generated images; non-AIR duplication remains.
src/​tool-calls/​reporters/​GuardianReporter.ts Reports guardian reviews.
src/​tool-calls/​reporters/​FuzzySearchReporter.ts Reports fuzzy searches.
src/​tool-calls/​reporters/​ElicitationReporter.ts Reports elicitation requests.
src/​tool-calls/​reporters/​DynamicToolReporter.ts Reports dynamic tools.
src/​tool-calls/​reporters/​CompactionReporter.ts Reports context compaction.
src/​tool-calls/​reporters/​CollabAgentReporter.ts Reports collaborative agents.
src/​tool-calls/​ClientCapabilities.ts Centralizes client capabilities.
src/​TerminalOutputMode.ts Removes superseded output capability handling.
src/​subagents/​PendingNotificationBuffer.ts Buffers notifications; encoded-size accounting remains incorrect.
src/​subagents/​CodexSubagentEventRouter.ts Routes and cleans up subagent events.
src/​PlanCapabilities.ts Removes superseded plan capabilities.
src/​permissions/​presentation.ts Updates permission presentation.
src/​permissions/​plan-review.ts Updates plan-review permissions.
src/​permissions/​metadata.ts Namespaces permission metadata.
src/​permissions/​mcp.ts Updates MCP permissions.
src/​permissions/​lifecycle.ts Updates permission lifecycle handling.
src/​permissions/​CodexApprovalHandler.ts Integrates approval reporting.
src/​DiffStats.ts Removes replaced diff statistics.
src/​ContextCompactionMeta.ts Reshapes compaction metadata.
src/​ContentChunks.ts Gates AIR content metadata.
src/​CodexPlanStream.ts Streams capability-aware plan deltas.
src/​CodexElicitationHandler.ts Integrates elicitation reporting.
src/​CodexCommands.ts Namespaces AIR command actions.
src/​AirExtension.ts Defines AIR negotiation and metadata.
src/​AgentMode.ts Gates AIR mode metadata.
src/​__tests__/​ToolCallReportingConnection.test.ts Tests filtering and cancellation.
src/​__tests__/​TerminalOutputMode.test.ts Updates terminal-output tests.
src/​__tests__/​scenarios/​data/​zed/​mcp-elicitation.json Updates Zed elicitation snapshot.
src/​__tests__/​scenarios/​data/​zed/​goal-update.json Updates Zed goal snapshot.
src/​__tests__/​scenarios/​data/​plain/​plan-stream.json Updates plain plan snapshot.
src/​__tests__/​scenarios/​data/​plain/​mcp-elicitation.json Updates plain elicitation snapshot.
src/​__tests__/​scenarios/​data/​plain/​goal-update.json Updates plain goal snapshot.
src/​__tests__/​scenarios/​acp-schema.ts Validates outbound ACP messages.
src/​__tests__/​PendingNotificationBuffer.test.ts Tests notification buffering.
src/​__tests__/​GitPatch.test.ts Tests Git patch generation.
src/​__tests__/​DiffStats.test.ts Updates diff-related tests.
src/​__tests__/​CodexPlanStream.test.ts Tests plan streaming profiles.
src/​__tests__/​CodexACPAgent/​turn-diff-events.test.ts Tests turn diff events.
src/​__tests__/​CodexACPAgent/​thread-goal-events.test.ts Tests goal events.
src/​__tests__/​CodexACPAgent/​terminal-output-events.test.ts Tests terminal output events.
src/​__tests__/​CodexACPAgent/​session-compaction.test.ts Tests session compaction.
src/​__tests__/​CodexACPAgent/​response-item-history-fallback.test.ts Tests history fallback.
src/​__tests__/​CodexACPAgent/​plan-review-events.test.ts Tests plan-review events.
src/​__tests__/​CodexACPAgent/​plan-events.test.ts Tests plan events.
src/​__tests__/​CodexACPAgent/​load-session.test.ts Tests session loading.
src/​__tests__/​CodexACPAgent/​initialize.test.ts Tests capability initialization.
src/​__tests__/​CodexACPAgent/​fuzzy-file-search-events.test.ts Tests fuzzy-search events.
src/​__tests__/​CodexACPAgent/​elicitation-events.test.ts Tests elicitation events.
src/​__tests__/​CodexACPAgent/​data/​web-search-start-and-complete.json Covers web-search lifecycle.
src/​__tests__/​CodexACPAgent/​data/​web-search-action-titles.json Covers web-search titles.
src/​__tests__/​CodexACPAgent/​data/​view-image-flow.json Covers image viewing.
src/​__tests__/​CodexACPAgent/​data/​tool-call-dynamic-names.json Covers dynamic tool names.
src/​__tests__/​CodexACPAgent/​data/​tool-call-completed-name.json Covers completed tool names.
src/​__tests__/​CodexACPAgent/​data/​tool-call-command-names.json Covers command names.
src/​__tests__/​CodexACPAgent/​data/​thread-goal-updated.json Covers goal updates.
src/​__tests__/​CodexACPAgent/​data/​thread-goal-updated-multiline.json Covers multiline goals.
src/​__tests__/​CodexACPAgent/​data/​thread-goal-cleared.json Covers cleared goals.
src/​__tests__/​CodexACPAgent/​data/​terminal-output-parsed-command-legacy-delta.json Covers legacy terminal deltas.
src/​__tests__/​CodexACPAgent/​data/​terminal-interaction-stdin.json Covers terminal input.
src/​__tests__/​CodexACPAgent/​data/​terminal-command-failed.json Covers failed commands.
src/​__tests__/​CodexACPAgent/​data/​terminal-command-completed.json Covers completed commands.
src/​__tests__/​CodexACPAgent/​data/​session-notices-replay.json Covers notice replay.
src/​__tests__/​CodexACPAgent/​data/​session-compaction-legacy.json Covers legacy compaction.
src/​__tests__/​CodexACPAgent/​data/​response-item-history-tool-names.json Covers history tool names.
src/​__tests__/​CodexACPAgent/​data/​plan-deltas.json Covers plan deltas.
src/​__tests__/​CodexACPAgent/​data/​plan-delta-fallback.json Covers plan-delta fallback.
src/​__tests__/​CodexACPAgent/​data/​plan-completed-fallback.json Covers completed-plan fallback.
src/​__tests__/​CodexACPAgent/​data/​mcp-tool-repeated-progress.json Covers repeated MCP progress.
src/​__tests__/​CodexACPAgent/​data/​mcp-tool-completed-with-logs.json Covers MCP completion logs.
src/​__tests__/​CodexACPAgent/​data/​load-session-response-item-history-fallback.json Covers loaded history fallback.
src/​__tests__/​CodexACPAgent/​data/​load-session-history.json Covers loaded session history.
src/​__tests__/​CodexACPAgent/​data/​image-generation-flow.json Covers image-generation lifecycle.
src/​__tests__/​CodexACPAgent/​data/​image-generation-completed-only.json Covers completion-only image generation.
src/​__tests__/​CodexACPAgent/​data/​guardian-approval-review-flow.json Covers guardian review flow.
src/​__tests__/​CodexACPAgent/​data/​guardian-approval-review-completed-without-start.json Covers completion-only guardian review.
src/​__tests__/​CodexACPAgent/​data/​file-change-delete-raw-content.json Covers deletion from raw content.
src/​__tests__/​CodexACPAgent/​data/​file-change-delete-file.json Covers file deletion.
src/​__tests__/​CodexACPAgent/​data/​file-change-add-raw-content.json Covers addition from raw content.
src/​__tests__/​CodexACPAgent/​data/​file-change-add-new-file.json Covers new files.
src/​__tests__/​CodexACPAgent/​data/​file-change-add-multiple-files.json Covers multiple file additions.
src/​__tests__/​CodexACPAgent/​data/​elicitation-url-accept.json Covers accepted URL elicitation.
src/​__tests__/​CodexACPAgent/​data/​elicitation-tool-approval-session-only.json Covers session-only approval.
src/​__tests__/​CodexACPAgent/​data/​elicitation-tool-approval-no-persist.json Covers non-persisted approval.
src/​__tests__/​CodexACPAgent/​data/​elicitation-tool-approval-all-persist.json Covers persisted approval.
src/​__tests__/​CodexACPAgent/​data/​dynamic-tool-in-progress.json Covers active dynamic tools.
src/​__tests__/​CodexACPAgent/​data/​dynamic-tool-completed.json Covers completed dynamic tools.
src/​__tests__/​CodexACPAgent/​data/​context-compaction-lifecycle.json Covers compaction lifecycle.
src/​__tests__/​CodexACPAgent/​data/​command-search-with-query-only.json Covers query-only search.
src/​__tests__/​CodexACPAgent/​data/​command-search-with-query-and-path.json Covers scoped query search.
src/​__tests__/​CodexACPAgent/​data/​command-search-with-path-only.json Covers path-only search.
src/​__tests__/​CodexACPAgent/​data/​command-search-no-query-no-path.json Covers empty search parameters.
src/​__tests__/​CodexACPAgent/​data/​command-read-file-with-path.json Covers file reads.
src/​__tests__/​CodexACPAgent/​data/​command-list-files-without-path.json Covers unscoped file listing.
src/​__tests__/​CodexACPAgent/​data/​command-list-files-with-path.json Covers scoped file listing.
src/​__tests__/​CodexACPAgent/​data/​available-commands-skills.json Covers skill commands.
src/​__tests__/​CodexACPAgent/​data/​available-commands-build-in.json Covers built-in commands.
src/​__tests__/​CodexACPAgent/​data/​agent-message-phases.json Covers message phases.
src/​__tests__/​CodexACPAgent/​collab-agent-events.test.ts Tests collaborative-agent events.
src/​__tests__/​CodexACPAgent/​CodexAcpClient.test.ts Tests ACP client behavior.
src/​__tests__/​CodexACPAgent/​auth-status.test.ts Tests authentication status.
src/​__tests__/​CodexACPAgent/​auth-error-events.test.ts Tests authentication errors.
src/​__tests__/​CodexACPAgent/​approval-events.test.ts Tests approval events.
src/​__tests__/​CodexACPAgent/​agent-file-change-report.test.ts Tests file-change reports.
src/​__tests__/​AirExtension.test.ts Tests AIR negotiation.
src/​__tests__/​acp-test-utils.ts Updates shared test utilities.
README.md Documents AIR features; wording overstates their scope.
readme-dev.md Removes obsolete diff-statistics guidance.
package.json Adds schema-validation support.
package-lock.json Locks dependency changes.
docs/​subagent-sessions.md Updates subagent documentation.
docs/​session-compaction.md Updates compaction documentation.
docs/​recommended-config-values-extension.md Removes superseded documentation.
docs/​goal-extension.md Removes superseded documentation.
docs/​diff-statistics-extension.md Removes replaced documentation.
docs/​async-tasks.md Removes superseded documentation.
docs/​agent-file-change-report.md Removes superseded documentation.
CHANGELOG.md Removes the obsolete diff-statistics entry.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/CodexAcpServer.ts
return [renderer.render(ImageViewReporter.viewed(item))];
case "imageGeneration":
return [createImageGenerationUpdate(item)];
return [renderer.render(ImageGenerationReporter.whole(item))];
Comment thread src/GitPatch.ts
Send compact Git patches when both the client and the adapter advertise
diffPatch. Keep the standard diff text fields for a client without the
capability.

The patch builders:
- strip the leading slash of an absolute path, so a header reads
  a/workspace/App.ts and not a//workspace/App.ts;
- quote a path that Git quotes;
- add rename from and rename to headers and use the target path;
- replace the file headers that Codex supplied, so that all headers agree;
- add new file mode and deleted file mode headers;
- keep carriage returns and mark a missing final newline.

A patch is built only when it has at least one valid hunk, holds no
binary content and is at most 1 MiB. Otherwise the adapter sends the
standard diff with the file texts. This covers an empty file, a pure
rename and malformed Codex hunks. Before, the adapter dropped the change
when the diff did not parse. The legacy already-patched branch now
reports the move target path.

Stop sending the ACP diff statistics. A client can derive the line
counts from a negotiated patch or from the standard diff texts. The
diff statistics contract and its documentation are removed.

docs/diff-patch-extension.md describes the negotiation, the patch
metadata, the placeholder text fields, the header paths and quoting,
the file mode and rename headers, the kept bytes, the final newline
marker and every fallback case.
@nikita-ashihmin
nikita-ashihmin force-pushed the nikita.ashikhmin/acp-patch-content branch from 21b4f35 to 34a3d5c Compare September 23, 2026 11:48
@nikita-ashihmin nikita-ashihmin changed the title feat: AIR tool call contract, exact diff patches, and no duplicated tool call data feat: add the ACP diff patch capability Sep 23, 2026
The hunk validator accepted every line that starts with a backslash.
A hunk with such a line then went to the client as a valid patch.

Now the validator accepts only the exact "\ No newline at end of file"
line that Git writes. Any other backslash line makes the hunks invalid.
The adapter then sends the standard ACP diff.
…r every client

Report tool calls through reporters and one renderer. The tool call
mapping moves to src/tool-calls/. A ToolReporter per tool kind reads
the Codex item once and returns tool facts. One AcpToolCallRenderer
puts each fact into one ACP field. It reads the capability choices
from one ClientCapabilities object. CodexToolCallMapper,
TerminalOutputMode, PlanCapabilities and permissions/presentation.ts
are removed.

Only AIR gets the AIR shape. A client is AIR when it declares
clientCapabilities._meta.jetbrains.air. A client that is not AIR, for
example Zed or a plain ACP client, gets the fields that origin/main
sent. ToolFacts.standard holds these fields where they differ.

Fixes for every client:
- Send only changed tool call fields. A new ToolCallReportingConnection
  wraps the ACP client connection and runs every session update through
  the ToolCallReports filter: live events, history replay, MCP startup,
  permission and elicitation updates, plan review and async tasks. The
  permission request tool call counts as a report. Appended output
  chunks are never compared. ACP defines no merge for the keys of
  _meta, so a client that is not AIR gets the whole _meta of each
  report.
- Release the reported fields when a turn ends, when a native child
  session ends, and after a cancelled or failed permission request.
  Keep the small fields of a finished tool call, so a replayed tool
  call does not repeat its status.
- Drop terminal and MCP output deltas that arrive after the tool call
  finished.
- Send replayed command output once, and generated images once.
- Make the MCP startup tool call ids unique.
- Forward the result of a dynamic tool in content.
- Keep the shown title, status and kind of a started tool call in its
  permission request.
- Send empty locations when a fuzzy search finds no file.
- Keep every notification of a pending subagent. The buffer merges
  adjacent text deltas and is bounded by bytes, not by count. It stores
  a copy of each notification.
- Send terminal_output_delta chunks to a client that declares no
  terminal channel. Zed gets terminal_output for a command with a
  terminal.

The AIR profile:
- Move the AIR metadata keys to _meta.jetbrains.air: the message phase,
  the goal, the mode kind, the command action, the permission
  presentation and the compaction record. AIR is not released, so the
  adapter sends only the new keys.
- One fact goes into one field. A command sends its output in
  terminal_output_delta, the raw stdin in _meta.terminal_input and its
  end in terminal_exit, without rawOutput. Read, search and list output
  is a result in content.
- rawInputRendering: AIR gets no display copy of readable input.
- planContentDelta: a streamed plan goes out as plan_update snapshots
  and _meta.jetbrains.air.contentDelta appends (CodexPlanStream).
- The MCP result goes to rawOutput = {result, error}. AIR gets no MCP
  progress. Other clients get the trimmed progress text.
- rawInput keeps the collaboration keys. Only spawnAgent gets
  _meta.jetbrains.air.subagent.
- The plan review sends the plan text in rawInput.plan.
…air-extensions.md

Merge the AIR extension docs into one file: the diff patch, the agent
file change report, async tasks, goals, permissions, recommended config
values, and the tool call contract. The old files are removed, and the
README and the other docs link to the new sections.

The compatibility rule limits the AIR extensions to AIR. A client that
is not AIR gets the fields that the adapter sent before these
extensions, with only the listed differences. The docs state the
title and commandTitle exceptions of the contract, the terminal channel
and the _meta filter of each client, and the Zed conventions.

The plan of codex-acp is always streamed text. AIR also accepts a plan
as a path to a file that it follows, but Codex keeps the plan only as
text, so this adapter does not declare the planFile capability.
…call

The tool call contract sent only the status in the permission request of
a started MCP tool call. Before, the request also had kind execute.
A client that reads the kind of the request, such as the MCP approval
e2e test, no longer found it.

Now the request has kind execute again for every client. The
compatibility rule in docs/air-extensions.md now says that a permission
request omits no field that it had before.
A scenario harness drives the adapter with scripted app-server traffic
for every tool kind: commands with output, stdin, and failure; read,
search, and list; file changes; MCP tools, approvals, and elicitations;
dynamic tools; web search; images; subagents with native, nested and
late child sessions and the collaboration controls; fuzzy search;
guardian reviews; compaction; plans and plan review; goals; permission
requests; MCP startup failures; background terminals; messages and
reasoning; and a history replay.

The harness records every outbound ACP message for a plain ACP client,
Zed, and AIR. The tests:
- validate each message against the ACP JSON schema of
  @agentclientprotocol/sdk;
- keep the AIR messages as golden snapshots, one message per line, with
  the keys in sorted order;
- compare the plain and Zed messages with the baseline, the recorded
  messages of origin/main at 1cc6223. The comparison applies only the
  allowed differences of the compatibility rule in
  docs/air-extensions.md;
- check that no client gets a tool call field twice with the same
  value, the Zed conventions, the plain client output, the subagent
  paths, and the AIR keys.

The SDK does not export its zod schemas, and zod cannot read the ACP
schema, so Ajv validates it. Ajv is a new dev dependency.

Each of these regressions was injected into the adapter and then
reverted, to prove that the baseline comparison catches it:
- no is_mcp_tool_call for a client that is not AIR: 6 comparisons fail;
- Zed gets the command output in content, not in rawOutput: 6 fail;
- the plain client loses its first terminal_output_delta chunk: 1 fails;
- a client that is not AIR gets every unchanged field again: the
  repeated-field check fails for plain and Zed.

To record the baseline again, see src/__tests__/scenarios/baseline.ts.
The baseline comparison merged the tool call of a permission request
into the stored tool call. A request that lost a field, such as the
kind, then still equaled the baseline.

Now the comparison keeps the tool call of a permission request as it
is. Only a tool_call_update is merged. The fields of the request still
go into the stored tool call. A new test checks that a request without
the kind fails the comparison.

The AIR recording of mcp-tool-approval now has kind execute in the
request, after the fix of the approval request in the tool call
contract.
…nses

The scenario harness removed models from the session/new and
session/load responses. A change of the model list then passed every
scenario test, although the tests claimed to record every outbound
message.

Now the harness keeps models. The baseline of the plain client and of
Zed was recorded again at 1cc6223, and the AIR snapshots were updated.
Each recording changes only in the session/new or session/load line,
which now has models. The model list of the current adapter equals the
baseline. A new test checks the model list of each profile.

docs/air-extensions.md now names the one message that the harness does
not record: the _auth/status_update notification.
@nikita-ashihmin
nikita-ashihmin requested a balanced review from Copilot September 23, 2026 12:32
…ording

The harness replaced every UUID-shaped string with <uuid>, every string
under a version key with <version>, and every large number under a key
that ends with At, AtMs, Time, or timestamp with <time>. A wrong id,
version, or time of the same shape then passed the comparison.

Now the harness replaces only these exact values:
- the temporary workspace path, as before;
- the package version in initialize.agentInfo.version;
- the random id of each MCP startup tool call, wherever that id occurs.

The time rule is removed. The only times in the recordings are the goal
times, and the scenario sets them. The AIR goal-update snapshot and the
plain and Zed goal-update baselines, recorded again at 1cc6223, now show
createdAt 1710000000000 and updatedAt 1710000001000.

With the old rules, a wrong goal time conversion or a wrong agent
version passed every scenario test. Now the first fails the AIR
goal-update snapshot, and the second fails 99 recordings. New tests
check that normalization keeps every other id, version, path, and time.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Multiple moderate patch-validation and pure-rename handling issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity · 2 Medium severity

Open (3)
Resolved since last review (1)

Comment thread src/CodexToolCallMapper.ts Outdated
Comment on lines +882 to +885
const patch = parseSinglePatch(unifiedDiff);
if (patch === null) {
logger.log("Skipped a file change whose diff has no single valid patch", {path: change.path});
return null;
Comment thread src/GitPatch.ts Outdated
Comment on lines +83 to +86
const header = HUNK_HEADER.exec(hunks[index]!);
if (header === null) return null;
let oldLines = header[1] === undefined ? 1 : Number(header[1]);
let newLines = header[2] === undefined ? 1 : Number(header[2]);
…enario recordings

The step-3 commit said that the scenario tests record every outbound ACP
message. The harness still dropped two kinds of message:
- the _auth/status_update notification, by a filter;
- every message before the first turn, because it cleared the
  connection dump after session/new. These were the first
  _auth/status_update and the available_commands_update of the session.

Now the harness drops no message. After initialize, it waits for the
first _auth/status_update, because the adapter sends it from a later
event loop phase. After session/new, it waits for the
available_commands_update. Both waits keep the order of the recording
stable.

The ACP schema defines no _auth/status_update, so the tests validate it
against the payload of AuthStatusUpdateNotification in
src/AuthStatusMeta.ts. An extension notification without a schema fails
the validation.

The recordings changed only by added lines:
- each of the 99 recordings has the _auth/status_update line after the
  initialize response;
- the 93 recordings of a scenario with a prompt turn have the
  available_commands_update line after the session/new response. The
  history and MCP startup scenarios recorded it already.
The plain and Zed baseline was recorded again at 1cc6223 with the new
harness, as baseline.ts describes. The same procedure with the old
harness gives the old baseline byte for byte, so the baseline changes
only by the new lines. The AIR snapshots were updated.

A new test checks that each recording starts with the initialize
response and the auth status, and has one available_commands_update.
The timeline helper and one guardian review test now skip the messages
of the session start.
…hema coverage

docs/air-extensions.md said that the scenario tests validate each
message against the ACP schema. The ACP schema does not define the AIR
session updates of subagents and async tasks: subagent_spawned,
subagent_state_update, async_task_spawned, and async_task_state_update.
The tests skipped these updates, so an update without a session id
passed.

Now the tests check the SessionNotification envelope of each AIR
session update: a valid ACP sessionId, an update with an AIR kind, and
_meta as an object or null. The payload of the update stays unchecked,
because no schema defines it. The docs now say that only the standard
ACP messages are validated against the ACP schema, and name the checks
for the other messages.

A new test checks that the envelope check rejects an AIR session update
without a sessionId, with a sessionId that is not a string, and with a
_meta that is not an object. The recordings do not change.

This answers the Copilot review comment on docs/air-extensions.md in
PR #545.
A history item is finished. The history replay rendered an image
generation with ImageGenerationReporter.whole(item), which mapped a
provider status such as generating to in_progress. The terminalStatus
option was set only for a live item/completed. A persisted item that
Codex saved while it was generating then stayed an active tool call
that never completed. origin/main had the same bug, so the fix applies
to every client.

Now whole() always maps the status to completed or failed, because both
callers report a finished item. The terminalStatus option and the
non-terminal status mapping are removed. docs/air-extensions.md lists
the fix with the other bug fixes of the compatibility rule.

A new test replays a generating image generation for each client
profile and expects one report with status completed. The scenario
recordings do not change, because the history-replay scenario has only
a completed image generation.

This answers the Copilot review comment on src/CodexAcpServer.ts in
PR #530.
…built

The hunk validator read only the line counts of a hunk header and
ignored the start lines. Hunks out of order, overlapping hunks, such as
two identical "@@ -1 +1 @@" hunks, and a hunk with a wrong new start
line then went to the client as a valid patch. A start or a count that
is larger than a safe integer was also accepted.

Now the validator reads the start and the count of both sides as safe
integers. Start 0 is valid only for an empty side. An empty side starts
after the line that the header names, as Git writes it. Each hunk must
start at or after the end of the previous hunk in the old file. Its new
start must equal its old start plus the line count change of the hunks
before it. Otherwise the adapter sends the standard ACP diff.

New tests check valid hunks with an insertion and a deletion, and
reject hunks out of order, overlapping hunks, wrong new start lines,
start 0 with lines, and numbers that are not safe integers.

This answers the Copilot review comment on src/GitPatch.ts ~86 in
PR #530.
…ffer cap

The pending notification buffer of a subagent stores serialized JSON
notifications and is bounded by bytes. For a new notification, it
reserved the size of the serialized notification. For a text delta that
it merged into the previous notification, it reserved only the UTF-8
size of the raw delta. A quote, a backslash, a newline, or a control
character takes more bytes in JSON, so the stored notifications could
exceed maxBytes.

Now a merged delta reserves the growth of the serialized notification:
the delta as a JSON string without its two quotes. A surrogate pair
that a stream splits between two deltas counts with its escapes, so the
count can be too large but never too small.

A new test fills the buffer to 10 bytes under the cap and pushes a
delta of three quotes and three newlines. The delta is 6 bytes of text
and 12 bytes in JSON. Before the fix, the buffer kept it and exceeded
the cap by 2 bytes. Now it drops the delta and keeps the next delta
that fits.

This answers the Copilot review comment on
src/subagents/PendingNotificationBuffer.ts in PR #544.
@nikita-ashihmin nikita-ashihmin changed the title feat: add the ACP diff patch capability feat!: AIR tool call contract, exact diff patches, and fixes for every client Sep 23, 2026
The reader joined each 64 KB chunk to the partial line and searched the
whole partial line for a line break again. One line of M bytes therefore
took O(M^2 / 64 KB) time. A 20 MB line took about 6 s, and a 40 MB line
took 21 s. Codex sends lines of that size, for example an item/completed
with the aggregated command output or a turn/diff/updated.

The reader also decoded each chunk alone. A UTF-8 character on a chunk
boundary became two replacement characters.

readline decodes UTF-8 across the chunks and searches only the new chunk.
A 40 MB line now takes about 20 ms. The reader still adds the missing
jsonrpc field, and it still skips blank and malformed lines.
session/load for a client without subagent sessions also read the whole
rollout file and rebuilt tool calls from its raw function_call records. It
then merged them into the thread history. The rollout parse is O(file size)
on each load. The merge scanned the fallback list from the start for each
thread update, which is O(U * F): 12,000 updates on each side took 16.5 s,
all synchronous.

The fallback was added when thread/read returned only the text of a
session. The app-server now returns the tool items as well. A check of 532
local sessions with thread/read found no command, file change or MCP call
that only the fallback recovered. The fallback added only calls that the
live session did not show as tool calls: 1167 send_message, 339
followup_task, 302 spawn_agent with an encrypted message, 283 sleep that
duplicate the sleep items, 180 list_agents, 124 wait, 41 write_stdin and
30 request_user_input.

The adapter now replays only the thread items and sends each update when
it is built. A session from an old app-server that has no tool items
replays its text only.
session/load read every turn of the thread into one array, and only then
sent the history. For native subagents it also read the whole thread of
each child and kept it in a cache until the load ended, although the replay
used only one turn of the child. A local session of 171 turns kept 178 MB
of heap during the load. A session of 4 turns with 7457 items kept 160 MB,
because one turn can hold thousands of items.

The adapter now reads the history with thread/items/list, oldest first,
100 items per page, and sends each page before it reads the next one. The
pages end at the itemsBackwardsCursor of thread/resume, so an item that
arrives after the resume is not replayed twice. A child replay reads only
the items of its turn. The same two sessions now keep 10 MB and 6 MB.

A legacy store still reads the whole history in one request. A full read
for a fork still reads every turn.
For AIR, the adapter collected the output chunks of each read, search and
list command in memory and sent the output once in content at completion.
After 1 MiB, each chunk copied the last 1 MiB of the collected text, so
50 MB of output in 1 KB chunks cost about 5 s of CPU. The collected text
was used only when item/completed had no aggregatedOutput, and none of
14,812 local completed commands lacked it. AIR also drops the result text
of a read: it does not need the file text.

AIR now gets no output of a file read. The output of a search or a list
streams to _meta.terminal_output_delta, and output that did not stream
goes there once at completion. The adapter collects nothing. The other
clients keep the output as on main.
The standard ACP diff of an updated file held the whole file before and
after the change. To build it, the adapter read the file from disk,
applied the Codex hunks, and on a failure applied the reversed hunks. For
a 50 MB file that took about 1 to 2 s of synchronous work and about
350 MB at the peak. The history replay did the same for each file change
of the session, against today's file, so the patch usually failed.

The adapter now builds the standard diff from the Codex diff alone. An
update gets one diff block per hunk, with the context lines and the
changed lines of the hunk. ACP asks for the original and the new content
of the change, not of the whole file. The adapter reads no file, for a
live change and for a replay. A pure rename gets one block without
changed lines. An added or a deleted file keeps its whole text.

A Codex diff larger than 1 MiB gets no block, for every client. The Git
patch builder now checks the size of its source text before it copies
it: a 50 MB added file took 356 ms and 151 MB before it returned null.
The output of a command could reach a client that is not AIR twice: in the
chunks, and again in rawOutput.formatted_output at the end. Zed and plain
clients got both for a terminal command, and a replayed command carried
both for every client. Zed also got read, search and list chunks in
terminal_output_delta, which it does not read, and then the output in
rawOutput, which it shows only as a JSON fallback when content is empty.

The rule is now one channel per command and client:
- With a chunk channel, the output goes only to the chunks, and output
  that did not stream goes in one chunk at the end, also on a replay.
- Without a chunk channel, the output goes once to content as text. Zed has
  no chunk channel for a read, search or list command.
- A command without a terminal keeps rawOutput = {exit_code}.

AIR gets the output of a read, search or list command once, as rawOutput
text at the end, from aggregatedOutput. AIR stores it as an output stream
and shows it on the generic card. The file read is not dropped any more.

Stdin no longer marks a command as streamed, so a command that got stdin
and no output chunk still sends its output at the end.

The baseline comparison accepts the output once, in any channel, as an
allowed difference.
@nikita-ashihmin
nikita-ashihmin force-pushed the nikita.ashikhmin/acp-patch-content branch from 7aab80c to 2168cb0 Compare September 24, 2026 11:41
Before each turn/start, the prompt waited for
skills/list {forceReload: true} and dropped the answer. That was one more
request and a scan of the skill directories on each turn, before the model
could start.

A check with Codex CLI 0.156.1 showed that the reload is not needed: a
skill file that appears during a session is in skills/list without
forceReload, and the model of the next turn sees it without any request.
Codex sent no skills/changed notification in that check.

The prompt now only sets the extra skill roots when they change. After a
turn, the adapter lists the skills in the background and sends a new
available_commands_update when the commands changed. A skill that appears
during a session now becomes a slash command after the next turn. Before,
it did not appear at all.
A spawned child is announced to the client only when Codex reports its
subAgentActivity. Until then, the router buffers the updates of the child,
up to 32 MiB. A child that ended first, for example a fast child, a failed
spawn or a child of an interrupted root turn, moved to
terminalPendingSpawns together with its buffer. That map lives until the
session closes, so the buffer stayed in memory for the whole session,
although nothing read it again.

The ended spawn now keeps only its parent and its task, which a reopen
needs. Its buffered updates are dropped, as the client never saw the child.

The replay of a materialized child also pushed the whole buffer with a
spread, push(...buffered). About 150,000 updates exceed the argument limit
and throw a RangeError after the buffer is already taken, so the updates
were lost and the approvals that wait for the child session hung. The
replay now uses a loop. A test replays 200,000 updates.
…ended tasks

Codex sends no event when a command moves to the background or its
background process exits, so the adapter lists the background terminals
of a thread. It sent thread/backgroundTerminals/list on every item/started
of any type, also for a message or a reasoning item, and it waited for one
more list at turn/completed inside the notification queue of the session.

A task also stayed in memory for the whole session. The adapter remembers
every command with a process id, and finish() only changed the state, so a
normal command that never went to the background stayed as well. Each
sync walked all tasks of the session.

Now:
- The adapter lists the terminals on item/started only while a command of
  the thread runs, and at the turn end without waiting. A background task
  outlives the turn, so the prompt response does not depend on the list.
- A task leaves the map when it ends: at once when the client never saw
  it, and after its terminal state reached the client otherwise.
- A small map keeps the ended tasks that a list can still name, for
  example a list that started before the end. A task leaves it when a list
  of its thread no longer names it, so an ended task never comes back.
Since the history replay reads pages lazily, a history request runs after
the session is installed. A failed page request failed session/load, but
the session state and the thread subscription stayed: the client got part
of the history, an error, and a half-open session. Before, the history was
read inside the open, and a failure closed the session.

A failed read of a subagent turn now also stays local to that child, as
before: the child pages are read lazily too, so the error came from the
nested replay and failed the whole load.

- A failed root history read now closes the session and rethrows.
- A failed child history read marks the child as unreadable, logs, and
  the replay goes on.
0b1437c read the Codex stdout with readline. readline also ends a line at
U+2028 and U+2029, and JSON allows both unescaped in a string. Codex
writes them so. A message that held one was split into parts that are not
JSON, and the reader dropped them in silence. A lost response left its
request waiting forever: session/load of a real session hung at the page
whose text held U+2028. A lost notification dropped a message chunk or a
command output chunk.

The reader now splits at \n only. It decodes UTF-8 across chunks with a
StringDecoder and searches only the new text, so a long line still costs
linear time. A final line without a line break is read at the end of the
stream. A line that is not JSON is now logged, so a lost message is
visible. The session that hung now loads its 1753 items in 88 ms.
A history page request had no timeout, so a lost response made
session/load wait forever. A page takes milliseconds. A page request now
fails after 60 s with an error that names the method and the thread. The
load then fails and closes the session, instead of hanging.
session/list called thread/list without useStateDbOnly, so Codex scanned
and repaired every rollout file on each call. On a local store of 92
sessions that took 3.0 to 3.7 s for 4 pages. From the state DB, the same
list takes 9 to 11 ms.

A comparison of the two answers found the same titles, previews and
update times. The state DB also returned 3 VS Code sessions that the scan
dropped, and it returns the real path of a cwd behind a symbolic link,
for example /private/var instead of /var.

An empty page also ran three more thread/list calls for diagnostics, each
with a full scan. They only logged counts, so they are removed.
A close during session/load did not stop the replay. The adapter read
every remaining history page, also of the subagents, and sent updates
for the closed session. For a session of 10k items that is about 100
extra thread/items/list requests.

The replay now checks the session generation at each page and at each
item. A close stops the read and the load fails with "is closing". The
load does not close the session a second time.
…closed thread settings

The adapter kept the settings of each thread from thread/settings/updated
and never removed them. An entry holds the developer instructions of the
mode, about 10 KB for Plan mode, and subagent threads add entries too.

A resume or a load read the mode from this map, but Codex sends no
thread/settings/updated on thread/resume (checked live on Codex 0.156.1).
After a restart of the adapter, a loaded Plan session showed the default
mode. The resume response holds the mode, so the adapter now reads it
there. The close of a thread now removes its settings.
Only tests called it. The tests now call threadReadWithHistory of the
app-server client directly.
The title generation sent the whole first message to the title model. A
pasted log of 200 KB cost about 50k tokens, and a message larger than
the context of the model failed the generation without a title.

The adapter now sends the first 4000 characters. It does not split a
surrogate pair.
The README said that every client gets the tool call contract and that
the goal extension is provider-neutral. Only AIR gets both. Other clients
keep the earlier tool call fields.

The subagent document said that the adapter always advertises
nativeSubagentSessions. It advertises the key to AIR only.

The comment of DIFF_PATCH_MAX_BYTES said that a larger change uses the
standard ACP diff. The file change reporter sends no diff for it. The
comment of commandActionFacts named the deleted history fallback.

The history page constants now sit above the class comment, so the
comment documents the class again.
CommandEnd.replay was set but never read. FileChangeReporter.started is
synchronous, so the callers do not await it. renderFacts accepted null,
but no caller passes null, so the handler calls the renderer directly.

The Git patch builders no longer check the source size. The file change
reporter drops a Codex diff over the limit before it calls a builder, and
the builder still checks the size of the patch with its headers. The
patch test no longer measures time.

The MCP progress comment said that the empty report stops the progress
for AIR. The event handler stops it, so the comment now says so.

Nine exports had no importer outside their module. They are now
module-private.
The guardian reporter built the verdict text in two functions that
differ only in the Action line. One function now builds both texts.

The event handler and the server built the goal session_info_update in
the same way. goalSessionInfoUpdate in ThreadGoalSnapshot.ts now builds
it for both.

Four reporters mapped a finished item status to completed or failed in
place. toTerminalToolStatus in ToolStatus.ts now does it.

Four AIR key constants lived in the modules that use them. They now live
in AirExtension.ts with the other AIR keys. ToolCallReports now imports
isRecord instead of a copy.
ClientCapabilities.with was called only from tests. The tests now build
the capabilities with ClientCapabilities.from, as the server does.

The approval handler, the elicitation handler, the event handler and the
subagent router had default parameters that only tests used. The server
passes every argument, so the defaults are removed. The tests now pass
the arguments. createTestEventHandler builds an event handler for a test.
Three loops read a cursor-paged history: threadItemPages, the turn pages
of threadReadWithHistory and readSessionTurnItems. Only the first had a
timeout. A lost response made the other two wait forever, so a
session/load or a subagent replay could hang.

historyPages now holds the loop for all three. It applies the page
timeout and the repeated-cursor check. threadTurnPages exposes it for
thread/turns/list. The first request of threadReadWithHistory also has
the timeout now.
Two history fallback fixtures lost their last reader with the deleted
history fallback. The MCP log fixture named a home directory of a
developer. It now names /workspace.
Six test files of this branch had their own copy of deferred. They now
import one copy from acp-test-utils.
A child that ends before Codex reports its activity drops its buffered
updates. No test checked this. The new test ends such a child, reopens
it, and checks that the ended spawn keeps no buffer and that nothing is
replayed.

The process exit test starts a real process. In a full serial run it
once took more than the default 5 s. It now has 15 s.
AIR shows a command that reads one file as the viewed file. That view does
not show the text, and AIR stored the rawOutput in the session log in full.
The completion for AIR now leaves the output of a file read out. A search
or a list has no path and keeps its output. Other clients keep the output.
The plain and the Zed baselines of 27 of the 33 scenarios were the same
file. The Zed baseline now exists only where its messages differ, and the
test reads the plain baseline otherwise. The baseline writer applies the
same rule.

.gitattributes marks the scenario recordings as generated, so a pull
request diff folds them.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants