Document tool output conventions for file content

[yulinlina]

Records the convention from #765: read_file returns raw content without line numbers, while bounded edit-tool responses keep absolute line-numbered context headers. Also adds a lightweight source-level regression test so future tools don't drift from the split.

Addresses #765

[Copilot]

Pull request overview

Documents a convention for how model-facing tool output should represent file contents, and adds a regression test intended to prevent future drift between read_file and edit-tool output formats.

Changes:

• Add documentation describing file-content output conventions (read_file vs edit tools).
• Add an AVA “drift guard” spec for tool output conventions.
• Add a changeset entry for a patch release.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
source/tools/tool-output-conventions.spec.ts	Adds a regression test intended to enforce the documented tool output convention.
docs/features/tool-output-conventions.md	Documents the intended conventions for model-facing file content output.
.changeset/document-tool-output-conventions.md	Declares a patch changeset for the documentation/test addition.

Suppressed comments (1)

source/tools/tool-output-conventions.spec.ts:44

• This test will currently fail: none of the edit tools include the string Updated file context (lines, and the tools emit headers like Updated file contents: (string_replace/diff_edit) or File contents after write: (write_file). Also, using some() only proves one file has the header, not that the key tools follow the convention. Consider asserting headers/absolute line numbering on the specific tool source files you care about.

test('bounded edit tools keep absolute line-numbered context headers', (t) => {
  const fileOpsDir = join(toolsDir, 'file-ops');
  if (!existsSync(fileOpsDir)) {
    t.fail('source/tools/file-ops should exist');
    return;
  }

  const files = readdirSync(fileOpsDir).filter(
    (file) =>
      (file.endsWith('.tsx') || file.endsWith('.ts')) &&
      !file.includes('.spec.'),
  );

  const hasContextHeader = files.some((file) => {
    const source = readFileSync(join(fileOpsDir, file), 'utf8');
    return source.includes('Updated file context (lines');
  });

  t.true(
    hasContextHeader,
    'bounded edit tools should include an absolute line-numbered context header',
  );
});

---

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

[will-lamerton]

Hey @yulinlina - thanks for this. Worth flagging up front that the only review so far is Copilot's, and a chunk of it is now out of date, so please don't take all of it at face value.

What Copilot got wrong or is now stale

The "doc doesn't match the tools" comment was correct when it was written, but main has moved since you branched. string_replace and diff_edit now emit exactly Updated file context (lines X-Y of N) with absolute line numbers and [... lines A-B omitted ...] markers (source/tools/file-ops/string-replace.tsx:45 and diff-edit.tsx:230, landed via the bounded-string-replace-results changeset). Your doc describes current main accurately. Same goes for the suppressed comment claiming the second test will fail: I applied your three files onto a clean origin/main and ran the spec, and both tests pass. A rebase resolves both of these.

The unused-import claim is also wrong, readdirSync is used in the second test. And the Biome formatting point won't fail CI, since biome.json excludes **/*.spec.ts from files.includes. That said, every other spec in source/tools/ uses tabs and no bracket spacing, so matching that would be good for consistency.

What does still need addressing

1. The read_file guard is vacuous. %4d appears nowhere in the codebase on any ref; line numbers are formatted as String(i + 1).padStart(4, ' '). So that assertion can never fail regardless of what read_file does. Since read-file.spec.tsx already exists and tests behaviour, asserting on the actual returned string would be a real guard rather than a source-text grep for a token we never used.

2. files.some(...) in the second test only proves that one file in file-ops/ has the header. If string_replace regressed but diff_edit didn't, the guard stays green, which defeats the purpose of a drift guard. Worth asserting on string-replace.tsx and diff-edit.tsx individually.

3. The doc is missing frontmatter. All 19 other docs/features/*.md files open with a title / description / sidebar_order block, so without it this page renders untitled and lands unordered in the sidebar.

4. Please add a line on write_file. On current main it deliberately returns no file content at all (see the write-file-echo changeset). It's the most recently changed of the three conventions, so a doc on file-content output that covers read_file and the bounded edit tools but skips it feels incomplete.

Rebase on main and the first two Copilot points should drop away. Happy to take another look once it's updated.

[yulinlina]

You’re right — the %4d assertion is vacuous. It came from the original issue wording, not the current formatter. I’ll replace it with a direct check that read_file returns the fixture content verbatim, using lines that cannot accidentally look like a gutter.

Using the existing tool-test harness’s resulting string, the replacement can be:

const lines = Array.from({ length: 20 }, (_, i) => `raw-line-${i}`);
const content = lines.join('\n');
writeFileSync(fixturePath, content);

const result = await runReadFile({ path: fixturePath });

expect(result).toBe(content);
expect(result).not.toMatch(/^\s*\d+\s+raw-line-/m);
expect(result).not.toContain('Updated file context (lines');

This catches the current String(i + 1).padStart(4, ' ') numbering style and any future line-numbered wrapper, without hard-coding a dead %4d format. If the preferred home for this is read-file.spec.tsx, I’ll move it there and drop the separate source guard.

For the bounded edit tools, I’ll assert against the actual main output:

expect(result).toMatch(/Updated file context \(lines \d+-\d+ of \d+\)/);
expect(result).toMatch(/\[\.\.\. lines \d+-\d+ omitted \.\.\.\]/);

I’ll also rebase, keep the readdirSync usage, and format the specs with tabs/no bracket spacing to match the rest of source/tools.

[will-lamerton]

Thanks @yulinlina, that all sounds right. Answering your question directly: yes, move it into read-file.spec.tsx and drop the separate source guard.

I'd go a step further and retire tool-output-conventions.spec.ts entirely. Every tool in question already has a behavioural spec next to it (read-file.spec.tsx, file-ops/string-replace.spec.tsx, file-ops/diff-edit.spec.tsx, file-ops/write-file.spec.tsx), so the assertions land where a contributor changing that tool will actually see them fail. A source-text grep in a fourth file is a weaker guard and easy to overlook, and it's what led to both of the problems above: %4d: was never a real token, and some() over the directory only ever proved one file matched.

Concretely, what I'd like to see:

• read-file.spec.tsx: the verbatim round-trip plus your /^\s*\d+\s+raw-line-/m negative check. That covers the current padStart(4, ' ') numbering and any future wrapper.
• string-replace.spec.tsx and diff-edit.spec.tsx: your two regexes for the Updated file context (lines X-Y of N) header and the [... lines A-B omitted ...] marker, asserted per tool rather than across the directory.
• write-file.spec.tsx: an assertion that the response carries no file content, which pins the write-file-echo behaviour the doc will describe.

One small thing on the snippets: they're written in expect() style, but the suite is AVA, so those become t.is(result, content) and t.regex(result, /.../) with t.notRegex for the negatives. Existing tests in those files are a good template.

Then the doc just needs the frontmatter block (title / description / sidebar_order, matching the other docs/features/*.md pages) and the write_file line, and a rebase on main to clear the stale Copilot comments. Ping me when it's up and I'll take another look.

[github-actions]

Hi @yulinlina, thanks for this PR! It looks like a codeowner has left feedback
or review activity and there are still some outstanding items to wrap up.

Whenever you get a chance, could you take a look at the open comments?
If anything is unclear or you'd like a hand, just reply here and we'll help you get it across the line.