Skip to content

feat: keep raw tool input that is not valid JSON - #29988

Draft
mafredri wants to merge 3 commits into
mainfrom
mathias/edit-files-raw-input
Draft

mafredri wants to merge 3 commits into
mainfrom
mathias/edit-files-raw-input

Conversation

@mafredri

@mafredri mafredri commented Sep 26, 2026 •

Copy link
Copy Markdown
Member

Tool calls whose input is not valid JSON were stored without args, so the malformed text was lost. The tool-call part now keeps it as invalid_args, so failure shapes can be analysed from the database. The rest of this stack needs that data to evaluate the new edit_files schema.

invalid_args is internal like provider_metadata: stripped from REST responses and absent from the TypeScript types. The apidoc still lists it because swag ignores typescript:"-", the same as the other internal fields. Prompt replay reads only args, so the model still sees {}. Calls cut off mid-stream are still dropped; a complete call buffered before an interrupt keeps the field.

Stack, merge in order:

  1. feat: keep raw tool input that is not valid JSON #29988 Keep raw tool input that is not valid JSON
  2. feat(site/src/pages/AgentsPage): render flat edit_files args #29989 UI renders flat edit_files args
  3. feat(coderd/x/chatd/chattool): add edit_files grouping helpers #29990 edit_files grouping helpers
  4. feat: switch edit_files to a flat edits list #29992 Switch edit_files to a flat edits list
  5. feat: report edit_files outcomes explicitly #29993 Report edit_files outcomes explicitly
  6. feat: apply edit_files per file #29994 Apply edit_files per file
  7. feat(coderd/x/chatd/chattool): reject no-op edit_files edits #29995 Reject no-op edits
Implementation plan

edit_files: flat schema, clear results, file-level partial application

Scope of this stack: parts 1 to 5. Parts 6 (previous-call guard) and 7 (re-analysis) are not being implemented. Without the guard no file can have the already_applied status, so the already_applied entries, counts and UI note below are out of scope.

Direction (for approval)

Outcome. Fewer wasted model turns on edit_files: fewer malformed calls, no valid work thrown away, and results that say exactly what happened.

Observable end state

  • The model edits with one flat edits list; each edit carries its own path.
  • A failed file names its edits (edits[i]), the reason, and what to resend.
  • In a batch, files without errors are applied; files with any error are not.
  • Re-sending an edit applied by the previous call reports "already applied" instead of a false "not found" or a duplicated change.
  • An edit that changes nothing is rejected immediately.
  • The UI shows applied, rejected and already-applied files; no synthetic diffs for rejected files.
  • Hooks, execution and storage agree on the edits: hooks see today's grouped files form of the same edits, and execution uses exactly what the hook approved.

Recommended direction. Replace the nested files[].edits[] schema with a flat list (schema B), then build clearer results, file-level partial application, no-op rejection and a previous-call guard on top, in that order. All changes are in coderd; the workspace agent API stays as it is until the changes are validated.

Why. Evidence from dogfood (75,070 calls, 8,914 failures, 2026-03-16 to 2026-09-26; analysis in /home/coder/efa):

  • 30% of failures are files sent as a JSON string, 81% from Claude families; none parsed as-is. The nested array is the suspected cause.
  • 89% of valid entries discarded with a failed batch (1,704 of 1,910) were collateral damage from an old_text error in another file.
  • Models re-send everything after a rejection (75%); agent errors never say nothing was applied.
  • Edits with old_text == new_text (2,634 calls) are followed by rework on the same file 73% of the time vs 51%.

Shape of the work (in order)

  1. Keep raw tool input that is not valid JSON (needed to evaluate B).
  2. Schema B, as three PRs that each merge safely on their own:
    • 2a. UI renders B args (old files args keep rendering).
    • 2b. Hook conversion helpers (B to grouped, grouped override to B), tested, not yet wired.
    • 2c. Switch the tool to B: string-decoding safety net, old-shape rejection, hook conversion wired in, tool description, hooks docs note.
  3. New result format, still all-or-nothing.
  4. File-level partial application (coderd sends one agent request per file).
  5. No-op rejection.
  6. Previous-call guard.
  7. Re-run the analysis about 2 weeks after part 2 and after part 4.

Ruled out

  • A different tool or schema per model: no clear line to draw.
  • {path, edits} hybrid or top-level first edit (proposal A): two ways to express an edit; unclear what applies when.
  • Repair rules learned from the old schema: several are artifacts of the nested shape; decide from B-era data.
  • Reassembling keys split out to the top level: value escaping is ambiguous; wrote literal \" in tests.
  • fantasy property-order fix: benefit not supported by evidence.
  • Partial application within a file: later edits depend on earlier ones.
  • Whitespace-trimmed or multi-call guard: added complexity; previous call exact match covers the common case.
  • Accepting the old files shape after B: one contract for hooks and UI; after the old_text/new_text rename only 62 calls used old keys.
  • Passing a string-encoded edits raw to hooks: a policy reading edits[].path would see nothing.
  • Changing the workspace agent API (per-edit index, match line numbers, partial-apply flag): not needed to validate the direction; revisit with data after part 4.

Decisions made

  • Schema B; path repeated per edit.
  • edits sent as a JSON string is accepted only if it parses as-is; storage, UI, hooks and execution see the decoded array.
  • Hooks keep today's input shape for now: coderd presents B edits grouped by path as {"files":[{"path","edits":[...]}]} and converts input_override back. Moving hooks to B is a separate, explicit break after B is validated (chatd is experimental, so this is allowed but not needed yet).
  • Old files shape is rejected with an error showing the B shape.
  • An error tied to a file rejects that whole file; an error not tied to any file rejects the whole call.
  • No-op edits reject their file.
  • A partial result is not an error (is_error false, status: "partial").
  • Guard remembers only the previous edit_files call in the chat; exact match; no trimming.
  • Result format ships before partial application.
  • No workspace agent API changes; partial application uses one existing-API request per file.

Assumptions, constraints, tradeoffs

  • Everything ships in coderd, so there is no dependency on workspace agents updating.
  • The agent reports only the first failing edit per file and does not say which edit it was; rejected files therefore list all their edit indexes.
  • File contents are not in the data: double-apply and guard estimates are inferred from edit texts.
  • Tradeoff: repeating path costs about 3.3% more argument bytes (GPT families 8-12%).
  • Tradeoff: no-op rejection withholds real edits in about 2,012 calls that succeed today and raises the call failure rate from about 11.9% to 14.5%; accepted for a consistent file-level rule.
  • coderd/x/chatd is experimental; the hook contract could change, but this plan keeps it unchanged until B is validated.

Risks

  • B's stringified edits are broken like before. Evidence: the safety net only accepts strings that parse as-is, and none of the 2,681 historical stringified files values did; the top-level array is what gets stringified (files 2,680 times vs the nested edits array 3 times), and B still has one. Trigger: claude-sonnet-5 malformed rate stays near today's 27% two weeks after part 2. Response: analyze B-era failure shapes with the existing tooling; add repair rules only for observed shapes.
  • Models re-send with changed text. Evidence: after a rejected batch, 75% of next calls re-sent all valid files, and 21% of re-sent files had changed text, which an exact guard cannot catch (measured when models were told the whole batch failed; behaviour with the new message is unknown). Trigger: not-found errors on files the previous call applied, after part 6. Response: revisit the guard with data.
  • Extra agent round trips. Evidence: per-file requests add 18,302 requests over 73,170 historical calls (0.25 per call); 83% of calls touch one file and are unchanged; per-request latency is unmeasured. Trigger: measurable latency increase on multi-file calls. Response: send per-file requests concurrently.

User stories

Model

  1. Editing one file, I send {"edits":[{"path","old_text","new_text"}]} with no nested groups.
  2. Editing several files, I list edits with their paths; no grouping.
  3. When something fails, I learn which edit, why, which files were applied, which were not, and exactly what to resend.
  4. When I re-send edits my previous call already applied, I'm told they're already applied.
  5. When an edit changes nothing, I'm told right away instead of getting "applied" and having to check.
  6. When my edits arrives as a JSON-encoded string that is otherwise valid, the call still works.
  7. When I use the old files shape, the error shows the new shape.

User

  1. I see which files changed, which were rejected and why; rejected files never show a diff.
  2. Valid edits in a batch aren't thrown away because another file had a bad old_text.
  3. Fewer wasted turns and tokens on retries.

Admin / hook author

  1. My pre_tool_use policies keep working unchanged when the model-facing schema changes.
  2. Metrics separate applied, partial and failed calls; raw invalid input is kept for analysis.

Contracts

C1. Tool input (schema B)

{"type":"object","required":["edits"],"properties":{
  "edits":{"type":"array","minItems":1,"items":{
    "type":"object","required":["path","old_text","new_text"],"properties":{
      "path":{"type":"string","description":"Absolute path of the file to edit."},
      "old_text":{"type":"string","description":"Exact text to replace. Must match exactly one location unless replace_all is true. Must differ from new_text."},
      "new_text":{"type":"string","description":"Replacement text."},
      "replace_all":{"type":"boolean","description":"Replace every match of old_text."}}}}}}

Example: {"edits":[{"path":"/repo/a.go","old_text":"x := 1","new_text":"x := 2"},{"path":"/repo/b.go","old_text":"foo()","new_text":"bar()"}]}

C2. Input acceptance

  • edits is a JSON string that parses as-is into an array of objects: decoded where the tool call arrives (before hooks and persistence); everything downstream sees the array. Anything else in a string: whole call rejected.
  • Top-level files key: whole call rejected, error shows the C1 example.
  • Only old_text/new_text are accepted inside edits (no search/replace).
  • Validation checks every edit, not just the first bad one, and names errors by flat index edits[i].
  • Error attribution (takes effect in part 4; before that every error rejects the whole call): an edit with a path ties its error to that file (for example relative path, empty old_text). An edit without path, or malformed input, rejects the whole call. Plan-turn path restriction stays whole-call.

C3. Application semantics

  • Edits are grouped by path (as sent), keeping their order; each file's edits apply in order.
  • A file with any error (not found, ambiguous, no-op, invalid) gets none of its edits applied.
  • Files without errors are applied (from part 4; before that, all-or-nothing). From part 4, coderd sends one agent request per file, so each file's outcome comes from its own request.
  • No-op: old_text == new_text is an error for its file.
  • Guard: the previous edit_files call in the chat (most recent call before this one, with status applied or partial) defines the applied edits: (path, old_text, new_text, replace_all) of its applied files. Each edit in the new call that exactly matches one of them is skipped and reported as already applied; the rest of that file proceeds normally. Paths compare as sent by the model, not symlink-resolved.

C4. Tool result (what the model reads)

All applied (is_error: false):

{"status":"applied","message":"Applied edits to 2 files.",
 "files":[{"path":"/repo/a.go","status":"applied","diff":"..."},
          {"path":"/repo/b.go","status":"applied","diff":"..."}]}

Partial (is_error: false), rejected files first:

{"status":"partial",
 "message":"Applied 1 file. 1 file was already applied by your previous call. /repo/c.go was not applied (none of edits[3], edits[4] were applied): fix and resend only the edits for /repo/c.go.",
 "files":[
  {"path":"/repo/c.go","status":"rejected","edits":[3,4],
   "error":"old_text matches 3 occurrences (expected exactly 1). Include more surrounding context to make the match unique, or set replace_all to true"},
  {"path":"/repo/a.go","status":"applied","diff":"..."},
  {"path":"/repo/d.go","status":"already_applied","message":"edits[5] matched your previous call and was not applied again."}]}

Nothing applied (is_error: true), text. Part 3 (all-or-nothing, one agent request):

No files were applied. <agent error text>
Fix the failing edit and resend all edits.

From part 4 (per-file requests, every file failed):

No files were applied.
- /repo/c.go (edits[3], edits[4]): <agent error text>
- /repo/a.go (edits[1]): <agent error text>

Whole-call rejections (malformed input, old shape, missing path, plan restriction, infrastructure) use the same is_error: true text form, starting with what to change.

Counting: the message counts applied, already-applied and rejected files separately; an already-applied file is not counted as applied. status is applied when no file is rejected (including when some or all files are already applied), partial when at least one file is applied or already applied and at least one is rejected, and an error when no file is applied or already applied.

C5. Workspace agent API (POST /api/v0/edit-files)

Unchanged. Part 3 sends one request with all files, as today; a 400 means nothing was written (batches are atomic since 2026-03-24). Part 4 sends one request per file (edits grouped by path, in order) and derives each file's status from its own response.

C6. Hooks

  • pre_tool_use.tool_input for edit_files keeps today's shape: {"files":[{"path":"...","edits":[{"old_text":"...","new_text":"...","replace_all":false}]}]}, built from B by grouping edits by path in order (a string-encoded edits is decoded first).
  • input_override is accepted in the same shape and flattened back to B before validation and execution.
  • docs/admin/setup/chat-lifecycle-hooks.md: note that edit_files tool input is presented in this grouped form rather than the model's exact bytes.
  • Switching hooks to the B shape is out of scope; it is a separate break once B is validated.

C7. Storage and UI

  • Stored tool-call args are canonical (C1). Tool results store the C4 JSON.
  • UI renders B args and still renders old files args in existing chats.
  • UI uses per-file status: applied shows the server diff; rejected shows its error and never a synthetic diff; already_applied shows a note; header reflects partial ("Edited 1 of 3 files", counting only applied files).

C8. Observability

  • Tool input that is not valid JSON is stored as raw text on the tool-call part; it is never replayed to the model.
  • Metrics per model: applied / partial / failed outcomes, string-decoding count, guard skips, no-op rejections.

Implementation detail

Parts, surfaces, dependencies

  1. Raw invalid input. Surfaces: chatloop tool-call capture, chat message part (codersdk), persistence. No dependencies.
  2. Schema B. Depends on 1 landing first for measurement. Three PRs in order; 2a and 2b change no behaviour on their own, so main is never in a state where hooks or the UI see a shape they do not handle.
    • 2a. UI. utils.ts/Tool.tsx/EditFilesTool.tsx parse and render B args alongside old files args.
    • 2b. Hook conversion. Pure functions in coderd/x/chatd/chathooks (or next to toolinput.go): B to grouped form, grouped override to B. Unit tests only.
    • 2c. Switch. coderd/x/chatd/chattool/editfiles.go (args, validation, grouping into the agent's FileEdits, index mapping, tool description), chatloop tool-input decoding at stream arrival (prototype: RepairToolInput interface, worktree mathias/edit-files-input-repair), wiring 2b into toolinput.go/generation.go, hooks docs note.
  3. Result format. Surfaces: editfiles.go rendering (C4 applied and nothing-applied forms), UI. Depends on 2.
  4. Partial application. Surfaces: editfiles.go (one agent request per file, per-file statuses, C4 partial form), UI statuses. Depends on 3.
  5. No-op rejection. Surfaces: editfiles.go validation (coderd side, before the agent call). Depends on 4.
  6. Guard. Surfaces: generation_preparer.go (find the previous edit_files call and C4 result in the prompt rows loaded per generation; pass applied edits into EditFilesOptions), editfiles.go. Depends on 4 (needs per-file statuses in stored results).
  7. Re-analysis. Surfaces: /home/coder/efa (extract.py and shape classifier need B awareness and the raw-input field). After 2 and after 4.

References

  • Analysis: /home/coder/efa/SUMMARY.md, out/{errors,retries,models,safetynet,noop_partial,guard,guard_prev}/REPORT.md.
  • Earlier attempt (not merged): PR fix: accept string-encoded files arg in edit_files tool #28578.
  • Hook contract: docs/admin/setup/chat-lifecycle-hooks.md (lines 69-82).
  • Agent behaviour relied on (unchanged): atomic per request, first failing edit per file reported, agent/agentfiles/files.go.

Verification

  • Unit: C2 decoding (as-is only), old-shape rejection, attribution rules, grouping and index mapping, no-op, guard matching (exact, previous call only, per-edit skip).
  • Per-file requests: one failing file does not block the others; a failing file writes nothing; duplicate paths are grouped into one request.
  • UI (Vitest): B and old-shape args render; rejected files show no diff; partial header.
  • Hook conversion (2b): round trip keeps each file's edit order; interleaved paths (a, b, a) group to two files and flatten to (a, a, b).
  • Integration (chatd): string-encoded edits is stored as an array; pre_tool_use receives the grouped form; an input_override in grouped form executes as approved.
  • Result counting: applied, already-applied and rejected files are counted separately; status follows the C4 counting rule.
  • Production: re-run the analysis per model and compare against baselines: stringified rate (claude-sonnet-5 27.3% of calls), collateral withheld entries (1,910), not-found after identical re-send, no-op follow-up rework (73% vs 51%).

Invariants

  • Hooks see the stored B edits grouped by path, with each file's edits in their original order; execution uses exactly what the hook approved. Grouping does not keep the order of edits across files: for interleaved paths (a, b, a) the flattened result is (a, a, b). Per-file results are the same, but after an input_override the edits[i] indexes in results follow the flattened override, not the model's original call.
  • A rejected file is never partially written.
  • Nothing is reported as applied unless the agent wrote it.
  • The workspace agent API is unchanged.
  • The guard never applies an edit; it only skips exact repeats of the previous call's applied edits.

🤖 This PR was created with the help of Coder Agents, and will be reviewed by a human. 🏂🏻

A tool call whose input does not parse was persisted with no args, so
the malformed text was lost and failure shapes could not be analysed
from the database. The tool-call part now carries it as invalid_args,
NUL-encoded like other free-form fields. It is internal like
provider_metadata: stripped before API responses and absent from the
TypeScript types. Prompt replay still reads only args, so the model
sees the same "{}" as before.
The field comment said invalid_args is stripped before API responses,
but only StripInternal clears it and the chat stream does not call
that, so it now names StripInternal. "Non-empty" becomes "not blank"
because whitespace-only input is dropped too. The safeToolCallArgs
comment no longer repeats what the field comment already says.
The comment said the function "ensures" args are valid JSON, which
described no result. It now says what each input returns.

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.

1 participant