fix(opencode): render edit/write tool calls through the shared diff views by junmo-kim · Pull Request #1644 · tiann/hapi

junmo-kim · 2026-08-19T17:18:20Z

Symptom

OpenCode edit/write tool calls render as a generic card with raw JSON input instead of the shared diff views (backlog #51):

before	after

Root causes

Three independent gaps, all on the CLI adapter side:

Name mismatch — AcpMessageHandler derives the tool name from the ACP title, so OpenCode's edit arrives as edit; the web's tool registry only knows Edit/Write, so it falls back to the generic raw-JSON presentation.
Input shape mismatch — OpenCode streams native camelCase args ({filePath, oldString, newString} / {filePath, content}); the web EditView/WriteView only read Claude-shaped keys (file_path, old_string, new_string).
Gemini diff hoist doesn't apply — hoistDiffContentIntoInput requires a leading diff block + _meta.kind; OpenCode's completed content is a text block first and carries no _meta.kind.

Wire shapes verified against opencode v1.18.x source (acp/tool.ts) and live captures.

Fix

Canonicalize at the CLI adapter boundary (no web changes), following the existing Gemini precedent:

canonicalizeDiffToolInput() (cli/src/agent/utils.ts) — shape-based: path + old/new → Edit {file_path, old_string, new_string}; path + content → Write {file_path, content}; everything else returns null so existing fallbacks are untouched.
Applied at the three ACP input sites (AcpMessageHandler initial call / running update / enrichment fallback), the local-mode part parser (opencodeLocalToolParse), and before execute-hook signature computation (opencodeLocalLauncher) so before/after pairing stays consistent.

Design note — canonicalization is gated on edit/write semantics. The
Edit shape (path + both old/new strings) is unambiguous on its own, but
Write is just {path, content} — a shape many non-edit MCP tools also
accept — so without a gate those tools would be renamed to Write and their
extra args dropped. canonicalizeDiffToolInput therefore requires a semantic
hint (tool name/kind) and only normalizes known edit/write aliases. OpenCode
maps both edit and write to kind 'edit' (verified in toToolKind), which is
in the allow-list, so the native-shape cases keep working.

Out of scope: apply_patch/patch-text shapes, generic object-input presentation (backlog #38). No migration for messages already persisted in the raw shape.

Verification

New unit tests: helper (10 cases), ACP edit/write lifecycles incl. pending→running→completed with text+diff content, local part parsing, execute-hook pairing; the existing test asserting the old camelCase output was updated to the canonical contract.
Full suite green on upstream/main (1f5602ad, release 0.29.0): cli 2431 passed (+1 skipped), hub 1196, web 2796, shared 283, relay 80; typecheck clean.
Isolated E2E (throwaway HAPI_HOME + dynamic ports, real hub + vite + headless Chromium) seeded with the exact pre/post wire shapes — before shows the raw JSON card, after shows the unified diff and the Write content view:

before-inline	after-inline	after-write-dialog

… views

… view shapes

github-actions

Findings

[Major] Preserve OpenCode's replaceAll argument — OpenCode v1.18.x edit input includes optional replaceAll, but the new canonicalizer rebuilds the object with only path/old/new. The persisted tool call therefore loses whether the operation replaced one occurrence or all occurrences. Evidence cli/src/agent/utils.ts:38.

Suggested fix:

const replaceAll = isObject(rawInput) && typeof rawInput.replaceAll === 'boolean'
    ? rawInput.replaceAll
    : undefined;

return {
    name: 'Edit',
    input: {
        file_path: filePath,
        old_string: oldString,
        new_string: newString,
        ...(replaceAll === undefined ? {} : { replace_all: replaceAll })
    }
};

[Minor] Keep id-less hook pairing keys stable across canonicalization — canonicalizing edit to Edit before computing queue signatures makes an empty/partial before use a different fallback key from the full after. That leaves stale queue entries; with usable partial input it also emits a second tool call and leaves the first without a result. Evidence cli/src/opencode/opencodeLocalLauncher.ts:382.

Suggested fix:
```
let toolInput: unknown = parseMaybeJson(...);
const signature = buildToolSignature(name, toolInput);
const fallbackSignature = buildToolSignature(name, null);

const canonical = canonicalizeDiffToolInput(toolInput);
if (canonical) {
    name = canonical.name;
    toolInput = canonical.input;
}
```

Summary

Review mode: initial

Two issues found: one Major input-data-loss regression and one Minor execute-hook pairing regression.

Testing

Not run locally (review-only; PR content was not executed). GitHub integration check passed; test was pending when inspected.

HAPI Bot

- canonicalizeDiffToolInput now carries OpenCode's optional replaceAll through to the canonical Edit input as replace_all, so persisted history keeps whether an edit replaced one occurrence or all. - opencodeLocalLauncher computes the execute-hook pairing signature from the raw name/input BEFORE canonicalizing, so an id-less 'before' with empty or partial args shares a queue key with the full 'after'.

github-actions

Findings

[Major] Preserve snake_case replace_all during canonicalization — the helper explicitly accepts canonical snake_case edit inputs, but reads only camelCase replaceAll before rebuilding the object. An input such as { file_path, old_string, new_string, replace_all: true } therefore loses the execution argument when it passes through any of the new ACP/local call sites. Evidence cli/src/agent/utils.ts:38.

Suggested fix:
```
let replaceAll: boolean | undefined;
if (isObject(rawInput)) {
    if (typeof rawInput.replaceAll === "boolean") {
        replaceAll = rawInput.replaceAll;
    } else if (typeof rawInput.replace_all === "boolean") {
        replaceAll = rawInput.replace_all;
    }
}
```

Summary

Review mode: follow-up after new commits

One Major data-loss case remains in the canonicalizer. The prior id-less hook-pairing issue is no longer present in the latest diff.

Testing

Not run (automation; PR code was not executed). GitHub integration check passed; the main test job was still in progress when reviewed. Add helper coverage for snake_case replace_all with both true and false.

HAPI Bot

canonicalizeDiffToolInput accepts already-canonical snake_case edit inputs (file_path/old_string/new_string), but only read camelCase replaceAll. Read replace_all too so the execution argument survives every call site.

github-actions

Findings

[Major] Upgrade partial local edit/write calls when the full args arrive — canonicalizeDiffToolInput() only changes the current hook snapshot. If tool.execute.before has a non-empty partial input such as { filePath }, line 387 returns null, the call is emitted and recorded in sentToolCalls; the full after snapshot is canonicalized but line 443 suppresses the replacement call. The new regression case at cli/src/opencode/utils/opencodeLocalToolParse.test.ts:317 therefore still emits name: 'edit' with only { filePath }, so the shared diff view never receives the full input. The same first-usable-input dedupe affects message.part.updated at cli/src/opencode/opencodeLocalLauncher.ts:331. Evidence cli/src/opencode/opencodeLocalLauncher.ts:387.

Suggested fix:
```
const emittedToolInputs = new Map<string, unknown>();
const previousInput = emittedToolInputs.get(callId);
const shouldEmitToolCall = previousInput === undefined
    || (canonicalizeDiffToolInput(previousInput) === null
        && canonicalizeDiffToolInput(toolInput) !== null);

if (shouldEmitToolCall) {
    emittedToolInputs.set(callId, toolInput);
    session.sendAgentMessage({ type: 'tool-call', name, callId, input: toolInput });
}
```
Apply the same upgrade rule to both local event paths, and assert that the partial-before test's first message is Edit with file_path, old_string, and new_string.

Summary

Review mode: follow-up after new commits

One Major issue remains in the local-mode lifecycle. The prior snake_case replace_all finding is resolved by the latest commit.

Testing

Not run (automation; PR code was not executed). Integration passed; the main test job was pending when reviewed. Add partial-to-full lifecycle coverage for both tool.execute.* and message.part.updated.

HAPI Bot

…arrive A partial native before ({filePath}) was emitted as-is and recorded as sent, so the later canonical full input was deduped and the web Edit/Write view never received the complete arguments. Track the emitted input per callId and, when a partial native edit/write call is superseded by a canonical full input, emit a replacement tool-call (same callId) before the result. Applies to both the message.part.updated and tool.execute.* local event paths.

github-actions

Findings

[Minor] Release completed tool inputs from upgrade tracking — emittedToolInputs retains every emitted Edit/Write input for the lifetime of the local launcher. Those values include full Write content and Edit old/new strings, so long-running sessions accumulate file payloads even though sentToolResults prevents any later upgrade after completion. Evidence cli/src/opencode/opencodeLocalLauncher.ts:245.

Suggested fix:
```
sentToolResults.add(toolResult.callId);
emittedToolInputs.delete(toolResult.callId);

// tool.execute.after path
sentToolResults.add(callId);
emittedToolInputs.delete(callId);
```

Summary

Review mode: follow-up after new commits

One Minor resource-retention issue remains. The prior partial-to-full replacement issue is no longer present in the latest diff.

Testing

Not run (automation; PR code was not executed). Integration passed; the main test job was pending when reviewed. Add lifecycle coverage that verifies completed calls are removed from upgrade tracking.

HAPI Bot

emittedToolInputs retained the full emitted Write content and Edit old/new strings for the whole launcher lifetime. Once a call is completed (sentToolResults), later upgrades are already blocked, so drop the stored payload in both result paths. Add a lifecycle test that a stale part for a completed callId neither re-emits nor emits a duplicate result.

github-actions

Findings

[Minor] Gate diff canonicalization on edit/write semantics — the helper intentionally classifies only from argument keys, while ACP and local callers invoke it without the original tool identity. ACP/custom tool schemas are arbitrary, so any non-edit tool accepting {filePath, content} is renamed to Write; extra arguments are also discarded from the stored presentation. Evidence cli/src/agent/utils.ts:24, cli/src/agent/backends/acp/AcpMessageHandler.ts:753, cli/src/opencode/utils/opencodeLocalToolParse.ts:77.

Suggested fix:

export function canonicalizeDiffToolInput(
    rawInput: unknown,
    semanticHint: string | null
): CanonicalDiffToolInput | null {
    const kind = semanticHint?.trim().toLowerCase();
    if (!kind || !['edit', 'write', 'write_file', 'replace', 'modify', 'file_edit'].includes(kind)) {
        return null;
    }
    // Existing shape mapping.
}

// ACP: canonicalizeDiffToolInput(input, asString(update.kind) ?? name)
// Local: canonicalizeDiffToolInput(input, name)

[Nit] Restore function-local indentation — the new tracking declarations are flush-left inside opencodeLocalLauncher, contrary to the repository's 4-space indentation rule. Evidence cli/src/opencode/opencodeLocalLauncher.ts:239.

Suggested fix:

    const sentTextParts = new Set<string>();
    const sentToolCalls = new Set<string>();
    const sentToolResults = new Set<string>();
    // Emitted tool input per callId...
    const emittedToolInputs = new Map<string, unknown>();

Summary

Review mode: follow-up after new commits

One Minor correctness-scope issue and one Nit formatting issue remain. The prior completed-input retention issue is addressed by the new deletion paths.

Testing

Not run (automation; security constraint prohibits executing PR code). Integration CI passed; the main test job was still running when reviewed. Add a regression case proving a non-edit custom tool with { filePath, content } retains its original name/input. The new cleanup test only checks emitted messages and would still pass without the map deletion because sentToolResults suppresses the later event; extracting the tracker policy would allow a direct state assertion.

HAPI Bot

…ng decls The per-session tracking declarations (sentTextParts, sentToolCalls, sentToolResults, emittedToolInputs) were flush-left inside opencodeLocalLauncher, violating the repo's 4-space indentation rule.

github-actions

Findings

[Minor] Gate diff canonicalization on edit/write semantics — canonicalizeDiffToolInput receives no tool identity, while all ACP and OpenCode-local tool inputs pass through it. ACP sessions accept arbitrary MCP tools, so an unrelated tool with { filePath, content, mode } is renamed to Write and mode is discarded from the persisted presentation. Evidence cli/src/agent/utils.ts:31, cli/src/agent/backends/acp/AcpMessageHandler.ts:753, cli/src/opencode/utils/opencodeLocalToolParse.ts:77.

Suggested fix:

export function canonicalizeDiffToolInput(
    rawInput: unknown,
    semanticHint: string | null
): CanonicalDiffToolInput | null {
    const kind = semanticHint?.trim().toLowerCase();
    if (!kind || !["edit", "write", "write_file", "replace", "modify", "file_edit"].includes(kind)) {
        return null;
    }
    // Existing shape mapping.
}

// ACP: pass update.kind, then the stored/original tool name as fallback.
// OpenCode local: pass the native tool name.

Summary

Review mode: follow-up after new commits

One Minor correctness-scope issue remains. The follow-up commit fixes the prior indentation nit but does not constrain shape-based canonicalization. No other high-confidence issues found in the current full diff.

Testing

Not run (automation; security constraint prohibits executing PR code). Test and integration checks were in progress when reviewed. Add a regression case proving a non-edit tool with { filePath, content, mode } retains its original name and complete input.

HAPI Bot

canonicalizeDiffToolInput now takes the tool's semantic name/kind and only normalizes known edit/write aliases (edit, write, write_file, replace, modify, file_edit). The Edit shape (path + both old/new) is unambiguous on its own, but Write is just {path, content} — a shape many non-edit MCP tools also accept — so without the gate those tools were renamed to Write and their extra args dropped. OpenCode maps both edit and write to kind 'edit', which stays in the allow-list, so the native-shape cases keep working. ACP callers pass the update kind (falling back to the derived name); local callers pass the tool name.

github-actions

Findings

[Minor] Re-emit changed canonical inputs, not only the first canonical transition — the new shouldEmit condition only handles null -> canonical. Because canonicalizeDiffToolInput accepts empty and partial string values, a running Write such as { content: "" } is already canonical; a later update containing the final content is suppressed, leaving the web diff with stale input. The same condition is repeated for execute-hook events. Evidence cli/src/opencode/opencodeLocalLauncher.ts:343, cli/src/opencode/opencodeLocalLauncher.ts:451, cli/src/opencode/opencodeLocalLauncher.ts:473.

Suggested fix:
```
const canonicalChanged = currentCanonical !== null
    && hashObject(currentCanonical) !== hashObject(previousCanonical);
const shouldEmit = previousInput === undefined || canonicalChanged;
```

Summary

Review mode: follow-up after new commits

One Minor local-streaming correctness issue remains. The prior semantic-gating finding is addressed; no other high-confidence issues found in the current full diff.

Testing

Not run (automation; security constraint prohibits executing PR code). GitHub integration passed; the main test workflow was pending when reviewed. Add regression cases for shape-complete Write/Edit inputs changing from empty or partial strings to final strings through both message.part.updated and execute-hook paths.

HAPI Bot

github-actions · 2026-08-20T03:30:59Z

+                const currentCanonical = canonicalizeDiffToolInput(toolCall.input, toolCall.name);
+                // Emit on first usable input, and replace a partial native
+                // edit/write input with the full canonical one when it arrives.
+                const shouldEmit = previousInput === undefined


[MINOR] Re-emit when canonical content changes

This only upgrades null -> canonical. Since the canonicalizer accepts empty/partial strings, a running Write with { content: "" } is already canonical; a later update with the final content is suppressed, so the web retains the stale diff. The same predicate is repeated at lines 451 and 473.

Suggested fix:

const canonicalChanged = currentCanonical !== null && hashObject(currentCanonical) !== hashObject(previousCanonical); const shouldEmit = previousInput === undefined || canonicalChanged;

Add a regression where a shape-complete Write/Edit input changes before completion.

shouldEmit only handled the null->canonical transition, so a shape-complete Write/Edit that already canonicalizes (e.g. {content: ""}) was suppressed when the final content arrived, leaving the web diff with stale input. Emit on first usable input and re-emit whenever the canonical input value changes, across both message.part.updated and tool.execute.* paths.

github-actions

Findings

None.

Summary

Review mode: follow-up after new commits

No Blocker/Major/Minor/Nit issues found in the latest full diff. The prior stale-input finding is addressed.

Residual testing gap: the changed-input behavior for message.part.updated in cli/src/opencode/opencodeLocalLauncher.ts:335 lacks a dedicated lifecycle regression; the new regression covers only the execute-hook path.

Testing

Not run (automation; security rule prohibits executing PR code). GitHub integration passed; the main test workflow was pending at review time. Suggested test: running Write with empty content to completed Write with final content via message.part.updated.

HAPI Bot

…ed path The execute-hook path already had a regression for canonical inputs changing from empty to final content; add the matching case for message.part.updated so both local emit paths are covered.

github-actions

Findings

None.

Summary

Review mode: follow-up after new commits

No Blocker/Major/Minor/Nit issues found in the latest full diff. The follow-up commit adds coverage for the previously noted changed-input message.part.updated lifecycle.

Residual testing risk: cli/src/opencode/utils/opencodeLocalToolParse.test.ts:194 exercises a test-only collectMessages mirror rather than opencodeLocalLauncher itself, so production/test emit-policy divergence would not be detected.

Testing

Not run (automation; security rule prohibits executing PR code). GitHub integration passed; the main test workflow was pending at review time. Suggested follow-up: extract the message-part emit decision into a shared helper used by the launcher and test that helper directly.

HAPI Bot

junmo-kim added 3 commits August 20, 2026 01:19

feat(cli): add diff-shape tool input canonicalization helper

92ec432

fix(cli): canonicalize ACP diff-shaped tool inputs for the Edit/Write…

f2522bf

… views

fix(opencode): normalize local-mode edit/write tool parts to the diff…

c18fe7f

… view shapes

github-actions Bot reviewed Aug 19, 2026

View reviewed changes

Comment thread cli/src/agent/utils.ts Outdated

Comment thread cli/src/opencode/opencodeLocalLauncher.ts Outdated

github-actions Bot reviewed Aug 19, 2026

View reviewed changes

Comment thread cli/src/agent/utils.ts Outdated

github-actions Bot reviewed Aug 19, 2026

View reviewed changes

Comment thread cli/src/opencode/opencodeLocalLauncher.ts Outdated

github-actions Bot reviewed Aug 20, 2026

View reviewed changes

Comment thread cli/src/opencode/opencodeLocalLauncher.ts Outdated

github-actions Bot reviewed Aug 20, 2026

View reviewed changes

Comment thread cli/src/agent/utils.ts Outdated

Comment thread cli/src/opencode/opencodeLocalLauncher.ts Outdated

style(opencode): restore 4-space indentation on local launcher tracki…

61d9b66

…ng decls The per-session tracking declarations (sentTextParts, sentToolCalls, sentToolResults, emittedToolInputs) were flush-left inside opencodeLocalLauncher, violating the repo's 4-space indentation rule.

github-actions Bot reviewed Aug 20, 2026

View reviewed changes

Comment thread cli/src/agent/utils.ts Outdated

github-actions Bot reviewed Aug 20, 2026

View reviewed changes

test(opencode): cover changed-input re-emit on the message.part.updat…

24b9258

…ed path The execute-hook path already had a regression for canonical inputs changing from empty to final content; add the matching case for message.part.updated so both local emit paths are covered.

github-actions Bot reviewed Aug 20, 2026

View reviewed changes

Uh oh!

Conversation

junmo-kim commented Aug 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Symptom

Root causes

Fix

Verification

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Aug 20, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

junmo-kim commented Aug 19, 2026 •

edited

Loading