From ae87caeb2a829ceb4fa00d00b1e64ff559b72733 Mon Sep 17 00:00:00 2001 From: roboomp Date: Sat, 27 Jun 2026 11:22:32 +0000 Subject: [PATCH] fix(coding-agent): split per-file TTSR digests for multi-file edits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #3648 review (codex): the first iteration emitted every envelope path alongside one combined digest, so a multi-file payload that added `: any` to a README.md hunk and merely touched src/ok.ts would surface a *.ts path in the TTSR match context and trip the bundled tool:edit(*.ts) ts-no-any rule on text that belonged to the Markdown hunk — aborting valid edits under interruptMode:always. Add a per-file matcherEntries(args) hook on AgentTool / EditStreaming- Strategy returning [{ path, digest }] entries, one per touched file (same-path sections/hunks merged): - replace / patch: one entry from the top-level path + matcherDigest - hashline: regex-split by [path#TAG] section, body added-lines per entry (tolerant of streaming partial payloads) - apply_patch: expandApplyPatchToPreviewEntries grouped by path AgentSession.#checkTtsrStream / #checkTtsrAstStream now prefer matcherEntries and iterate per-file with isolated filePaths + streamKey, so each file's buffer and repeat-tracking are independent. Tools without matcherEntries keep the existing combined matcherDigest + matcherPaths path. --- packages/agent/CHANGELOG.md | 1 + packages/agent/src/types.ts | 11 ++ packages/coding-agent/CHANGELOG.md | 1 + packages/coding-agent/src/edit/index.ts | 11 ++ packages/coding-agent/src/edit/streaming.ts | 106 +++++++++++++ .../coding-agent/src/session/agent-session.ts | 60 ++++++- .../test/edit/streaming-matcher-paths.test.ts | 149 ++++++++++++++++++ 7 files changed, 333 insertions(+), 6 deletions(-) diff --git a/packages/agent/CHANGELOG.md b/packages/agent/CHANGELOG.md index d988b6753..20c65dd7b 100644 --- a/packages/agent/CHANGELOG.md +++ b/packages/agent/CHANGELOG.md @@ -5,6 +5,7 @@ ### Added - Added optional `AgentTool.matcherPaths(args)` hook: tools whose streamed arguments embed target file paths inside the wire payload (patch envelope markers, hashline section headers) can surface those paths so path-scoped stream matchers (e.g. TTSR `tool:edit(*.ts)` globs) match without a top-level `path` / `paths` argument. Returns `undefined` (or an empty list) to fall back to the caller's existing top-level argument scan. +- Added optional `AgentTool.matcherEntries(args)` hook: returns one `{ path, digest }` entry per file a (potentially partial) streamed call touches, so path-scoped stream matchers evaluate each file in isolation. Multi-file payloads (multi-section hashline, multi-hunk apply_patch) MUST split here — otherwise a combined digest from a sibling Markdown hunk would leak into the `.ts` path scope and trip rules like `tool:edit(*.ts)` `ts-no-any` on text that does not belong to a TypeScript file. Takes precedence over `matcherDigest` + `matcherPaths` when present; returns `undefined` (or empty) to fall back to the combined hooks. ### Removed diff --git a/packages/agent/src/types.ts b/packages/agent/src/types.ts index 05760dd93..71046a90e 100644 --- a/packages/agent/src/types.ts +++ b/packages/agent/src/types.ts @@ -643,6 +643,17 @@ export interface AgentTool readonly string[] | undefined; + /** + * Per-file projection of a (potentially partial) streamed call, pairing each + * touched file path with the digest of only the lines added to that file. + * Path-scoped stream matchers (TTSR) evaluate each entry in isolation, so a + * scoped rule like `tool:edit(*.ts)` never fires on text that actually + * belongs to a sibling Markdown hunk in a multi-file payload. Takes + * precedence over {@link matcherDigest} + {@link matcherPaths} when present; + * returns `undefined` (or empty) to fall back to the combined hooks. + */ + matcherEntries?: (args: unknown) => readonly { path: string; digest: string }[] | undefined; + /** Capability tier declaration used by approval gates. Omitted means "exec". */ approval?: ToolApproval; diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 51636a76a..3e9ebbe17 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -27,6 +27,7 @@ - Fixed marketplace-installed plugins appearing in both the npm plugin list and the OMP extension-package status provider. ([#3628](https://github.com/can1357/oh-my-pi/issues/3628)) - Fixed inconsistent OpenRouter prompt-cache hits on `/advisor` turns: `AgentSession.#buildAdvisorRuntime` (`packages/coding-agent/src/session/agent-session.ts`) constructed the advisor `Agent` without the provider-shaping options the SDK installs on the main agent — the `streamFn` wrapper that applies `providers.openrouterVariant`, `providers.antigravityEndpoint`, `providers.maxInFlightRequests`, and `model.loopGuard.*`; the `onPayload`/`onResponse`/`onSseEvent` hooks; the shared `providerSessionState` map; `transformProviderContext` (snapcompact, secret obfuscation, image clamping); and a stable `promptCacheKey`. Advisor turns therefore dropped the OpenRouter sticky-routing variant suffix, used a different `prompt_cache_key` than the main turn, and skipped the per-session provider hooks — producing intermittent OpenRouter response-cache misses across consecutive advisor calls. The advisor now reuses the same wrapper through a new shared `createSettingsAwareStreamFn` helper (`packages/coding-agent/src/session/settings-stream-fn.ts`) and inherits every provider hook the main agent does. ([#3639](https://github.com/can1357/oh-my-pi/issues/3639)) - Fixed hashline (and `apply_patch`) edit streams skipping path-scoped TTSR rules (e.g. the bundled `ts-no-any` rule scoped to `tool:edit(*.ts)`). `AgentSession.#getTtsrToolMatchContext` only scanned top-level `path` / `paths` arguments, so for an `edit` call whose only path lived inside the hashline `[demo.ts#TAG]` section header (or an `apply_patch` `*** Update File: …` marker), the TTSR match context arrived without any `filePaths` and the path glob never matched. The edit tool now exposes a mode-aware `matcherPaths(args)` companion to `matcherDigest(args)`: `replace` / `patch` reuse the top-level `path`, `hashline` parses `[path#TAG]` section headers, and `apply_patch` parses `*** Add/Update/Delete File:` envelope markers (all tolerant of streaming partial payloads). The session consults `tool.matcherPaths` first and falls back to the generic top-level argument scan for tools that don't implement it. ([#3646](https://github.com/can1357/oh-my-pi/issues/3646)) +- Fixed multi-file hashline / `apply_patch` edit streams leaking added lines across file scopes when evaluated against path-scoped TTSR rules. The first iteration of #3646's fix combined every section/hunk into one digest while emitting every envelope path, so a multi-file payload that added `: any` to a `README.md` hunk and merely touched `src/ok.ts` would surface a `*.ts` path alongside the combined digest and trip the bundled `tool:edit(*.ts)` `ts-no-any` rule on text that actually belonged to the Markdown hunk — aborting/retrying valid edits under `interruptMode: always`. The edit tool now exposes a per-file `matcherEntries(args)` hook (companion to `matcherDigest` + `matcherPaths`): hashline splits by `[path#TAG]` section, `apply_patch` splits by envelope marker via `expandApplyPatchToPreviewEntries`, same-path sections/hunks merge into one entry, and `AgentSession.#checkTtsrStream` / `#checkTtsrAstStream` iterate per file with isolated `filePaths` + `streamKey`. ([#3648](https://github.com/can1357/oh-my-pi/pull/3648)) ## [16.2.1] - 2026-06-27 diff --git a/packages/coding-agent/src/edit/index.ts b/packages/coding-agent/src/edit/index.ts index 1567f9f37..420206e57 100644 --- a/packages/coding-agent/src/edit/index.ts +++ b/packages/coding-agent/src/edit/index.ts @@ -407,6 +407,17 @@ export class EditTool implements AgentTool { return EDIT_MODE_STRATEGIES[this.mode].matcherPaths(args); } + /** + * Per-file projection of the streamed args, splitting multi-section + * hashline / multi-hunk apply_patch payloads into one (path, digest) entry + * per touched file. Path-scoped stream matchers (TTSR) then evaluate each + * file in isolation, so a `tool:edit(*.ts)` rule never fires on text that + * actually belongs to a sibling Markdown hunk. + */ + matcherEntries(args: unknown): readonly { path: string; digest: string }[] | undefined { + return EDIT_MODE_STRATEGIES[this.mode].matcherEntries(args); + } + async execute( _toolCallId: string, params: EditParams, diff --git a/packages/coding-agent/src/edit/streaming.ts b/packages/coding-agent/src/edit/streaming.ts index 61f3cf5a1..4a1f992cf 100644 --- a/packages/coding-agent/src/edit/streaming.ts +++ b/packages/coding-agent/src/edit/streaming.ts @@ -52,6 +52,17 @@ export interface StreamingDiffContext { isStreaming?: boolean; } +/** + * Per-file projection of a streamed edit payload. Pairs one target file path + * with the digest of only the lines added to that file, so path-scoped stream + * matchers (TTSR) evaluate each file in isolation — a `tool:edit(*.ts)` rule + * never fires on text that actually belongs to a sibling `README.md` hunk. + */ +export interface EditMatcherEntry { + readonly path: string; + readonly digest: string; +} + export interface EditStreamingStrategy { /** * Return the args restricted to edits that are "complete enough" to @@ -85,6 +96,18 @@ export interface EditStreamingStrategy { * Returns `undefined` (or an empty list) when no paths are recoverable. */ matcherPaths(args: Args): readonly string[] | undefined; + /** + * Per-file projection of the (potentially partial) args: one entry per + * touched file pairing the path with the digest of only the lines added to + * that file. Multi-file payloads (multi-section hashline / multi-hunk + * apply_patch) MUST split here so callers can evaluate each file under its + * own path scope instead of leaking added lines from one file into the + * other's match context. Same-path sections / hunks are merged into one + * entry. Returns `undefined` (or empty) when no per-file split is + * recoverable yet — the caller falls back to {@link matcherDigest} + + * {@link matcherPaths}. + */ + matcherEntries(args: Args): readonly EditMatcherEntry[] | undefined; } // ----------------------------------------------------------------------------- @@ -237,6 +260,65 @@ function extractApplyPatchEnvelopePaths(input: string): string[] { return paths; } +/** + * Split a (possibly partial) hashline buffer into one matcher entry per + * touched file: pair the section header path with the added lines from that + * section's body, merging sections that target the same file into one entry. + * Header-line regex (not `Patch.parse`) so a mid-typed trailing op still + * yields entries for completed sections. + */ +function splitHashlinePerFile(input: string): EditMatcherEntry[] { + const headerRe = /^\s*\[([^\]\r\n]+?)(?:#[0-9a-fA-F]{4})?\]\s*$/gm; + const sections: { path: string; headerStart: number; bodyStart: number }[] = []; + let match: RegExpExecArray | null = headerRe.exec(input); + while (match !== null) { + const candidate = stripApplyPatchPathNoise(match[1]).trim(); + if (candidate.length > 0) { + sections.push({ path: candidate, headerStart: match.index, bodyStart: headerRe.lastIndex }); + } + match = headerRe.exec(input); + } + if (sections.length === 0) return []; + + const byPath = new Map(); + for (let i = 0; i < sections.length; i++) { + const { path: sectionPath, bodyStart } = sections[i]; + const bodyEnd = i + 1 < sections.length ? sections[i + 1].headerStart : input.length; + const added = extractAddedLines(input.slice(bodyStart, bodyEnd), false); + if (added.length === 0) continue; + const existing = byPath.get(sectionPath); + byPath.set(sectionPath, existing === undefined ? added : `${existing}\n${added}`); + } + return Array.from(byPath, ([path, digest]) => ({ path, digest })); +} + +/** + * Split a (possibly partial) apply_patch envelope into one matcher entry per + * touched file. Same-path hunks are merged into one entry. Falls back to the + * streaming-tolerant parser when the envelope hasn't reached `*** End Patch`. + */ +function splitApplyPatchPerFile(input: string): EditMatcherEntry[] { + let entries: ApplyPatchEntry[]; + try { + entries = expandApplyPatchToEntries({ input }); + } catch { + try { + entries = expandApplyPatchToPreviewEntries({ input }); + } catch { + return []; + } + } + const byPath = new Map(); + for (const entry of entries) { + if (typeof entry.diff !== "string") continue; + const added = extractAddedLines(entry.diff, false); + if (added.length === 0) continue; + const existing = byPath.get(entry.path); + byPath.set(entry.path, existing === undefined ? added : `${existing}\n${added}`); + } + return Array.from(byPath, ([path, digest]) => ({ path, digest })); +} + // ----------------------------------------------------------------------------- // Strategies // ----------------------------------------------------------------------------- @@ -285,6 +367,12 @@ const replaceStrategy: EditStreamingStrategy = { matcherPaths(args) { return typeof args?.path === "string" && args.path.length > 0 ? [args.path] : undefined; }, + matcherEntries(args) { + const path = args?.path; + if (typeof path !== "string" || path.length === 0) return undefined; + const digest = replaceStrategy.matcherDigest(args); + return digest === undefined ? undefined : [{ path, digest }]; + }, }; interface PatchArgs { @@ -330,6 +418,12 @@ const patchStrategy: EditStreamingStrategy = { matcherPaths(args) { return typeof args?.path === "string" && args.path.length > 0 ? [args.path] : undefined; }, + matcherEntries(args) { + const path = args?.path; + if (typeof path !== "string" || path.length === 0) return undefined; + const digest = patchStrategy.matcherDigest(args); + return digest === undefined ? undefined : [{ path, digest }]; + }, }; interface HashlineArgs { @@ -495,6 +589,12 @@ const hashlineStrategy: EditStreamingStrategy = { const paths = extractHashlineHeaderPaths(input); return paths.length > 0 ? paths : undefined; }, + matcherEntries(args) { + const input = args?.input; + if (typeof input !== "string" || input.length === 0) return undefined; + const entries = splitHashlinePerFile(input); + return entries.length > 0 ? entries : undefined; + }, }; interface ApplyPatchArgs { @@ -559,6 +659,12 @@ const applyPatchStrategy: EditStreamingStrategy = { const paths = extractApplyPatchEnvelopePaths(input); return paths.length > 0 ? paths : undefined; }, + matcherEntries(args) { + const input = args?.input; + if (typeof input !== "string" || input.length === 0) return undefined; + const entries = splitApplyPatchPerFile(input); + return entries.length > 0 ? entries : undefined; + }, }; export const EDIT_MODE_STRATEGIES: Record> = { replace: replaceStrategy as EditStreamingStrategy, diff --git a/packages/coding-agent/src/session/agent-session.ts b/packages/coding-agent/src/session/agent-session.ts index 2a855dec8..efdaadab2 100644 --- a/packages/coding-agent/src/session/agent-session.ts +++ b/packages/coding-agent/src/session/agent-session.ts @@ -3808,6 +3808,14 @@ export class AgentSession { if (!manager) { return []; } + const entries = this.#resolveTtsrMatcherEntries(toolCall); + if (entries) { + const matches: Rule[] = []; + for (const entry of entries) { + matches.push(...manager.checkSnapshot(entry.digest, this.#perFileTtsrContext(matchContext, entry.path))); + } + return matches; + } const digest = this.#resolveTtsrMatcherDigest(toolCall); if (digest !== undefined) { return manager.checkSnapshot(digest, matchContext); @@ -3817,14 +3825,44 @@ export class AgentSession { /** Reconstruct the tool's normalized source snapshot via its `matcherDigest`, if any. */ #resolveTtsrMatcherDigest(toolCall: ToolCall | undefined): string | undefined { - if (!toolCall) { - return undefined; - } + const tool = this.#resolveTtsrTool(toolCall); + return tool?.matcherDigest?.(toolCall?.arguments ?? {}); + } + + /** + * Per-file split of a streamed call (one entry per touched file paired with + * the digest of only that file's added lines). Lets {@link #checkTtsrStream} + * and {@link #checkTtsrAstStream} evaluate each file in isolation so a + * path-scoped rule like `tool:edit(*.ts)` never fires on text that belongs + * to a sibling Markdown hunk in a multi-file payload. + */ + #resolveTtsrMatcherEntries(toolCall: ToolCall | undefined): readonly { path: string; digest: string }[] | undefined { + const tool = this.#resolveTtsrTool(toolCall); + const entries = tool?.matcherEntries?.(toolCall?.arguments ?? {}); + return entries && entries.length > 0 ? entries : undefined; + } + + #resolveTtsrTool(toolCall: ToolCall | undefined) { + if (!toolCall) return undefined; const tools = this.agent.state.tools; - const tool = + return ( tools.find(t => t.name === toolCall.name) ?? - tools.find(t => t.customWireName !== undefined && t.customWireName === toolCall.name); - return tool?.matcherDigest?.(toolCall.arguments ?? {}); + tools.find(t => t.customWireName !== undefined && t.customWireName === toolCall.name) + ); + } + + /** + * Replace `matchContext`'s `filePaths` + `streamKey` so a per-file entry + * gets its own glob-eligible path and its own TTSR buffer/repeat tracking + * (each file's stream is independent inside the same tool call). + */ + #perFileTtsrContext(base: TtsrMatchContext, filePath: string): TtsrMatchContext { + const filePaths = this.#normalizeTtsrPathCandidates(filePath); + return { + ...base, + filePaths: filePaths.length > 0 ? filePaths : [filePath], + streamKey: base.streamKey ? `${base.streamKey}#${filePath}` : undefined, + }; } /** @@ -3839,6 +3877,16 @@ export class AgentSession { if (!manager) { return []; } + const entries = this.#resolveTtsrMatcherEntries(toolCall); + if (entries) { + const matches: Rule[] = []; + for (const entry of entries) { + matches.push( + ...(await manager.checkAstSnapshot(entry.digest, this.#perFileTtsrContext(matchContext, entry.path))), + ); + } + return matches; + } const digest = this.#resolveTtsrMatcherDigest(toolCall); if (digest === undefined) { return []; diff --git a/packages/coding-agent/test/edit/streaming-matcher-paths.test.ts b/packages/coding-agent/test/edit/streaming-matcher-paths.test.ts index 7d97a0ffe..617069248 100644 --- a/packages/coding-agent/test/edit/streaming-matcher-paths.test.ts +++ b/packages/coding-agent/test/edit/streaming-matcher-paths.test.ts @@ -109,6 +109,64 @@ describe("EDIT_MODE_STRATEGIES.matcherPaths", () => { }); }); +describe("EDIT_MODE_STRATEGIES.matcherEntries", () => { + it("replace + patch return one (path, digest) entry from the top-level path", () => { + expect( + EDIT_MODE_STRATEGIES.replace.matcherEntries({ path: "src/foo.ts", edits: [{ new_text: "x = 1" }] }), + ).toEqual([{ path: "src/foo.ts", digest: "x = 1" }]); + expect( + EDIT_MODE_STRATEGIES.patch.matcherEntries({ path: "src/bar.ts", edits: [{ op: "update", diff: "@@\n+y" }] }), + ).toEqual([{ path: "src/bar.ts", digest: "y" }]); + }); + + it("hashline splits multi-section payloads into one entry per file", () => { + const input = [ + "[src/a.ts#ABCD]", + "SWAP 1.=1:", + "+const a = 1;", + "[README.md#EF01]", + "SWAP 1.=1:", + "+# Heading", + "[src/a.ts#1234]", + "SWAP 2.=2:", + "+const c = 3;", + "", + ].join("\n"); + expect(EDIT_MODE_STRATEGIES.hashline.matcherEntries({ input })).toEqual([ + // Same-path sections are merged into one entry, preserving order. + { path: "src/a.ts", digest: "const a = 1;\nconst c = 3;" }, + { path: "README.md", digest: "# Heading" }, + ]); + }); + + it("apply_patch splits multi-hunk payloads into one entry per file", () => { + const input = [ + "*** Begin Patch", + "*** Update File: src/a.ts", + "@@", + "-foo", + "+const a = 1;", + "*** Update File: README.md", + "@@", + "-old", + "+# Heading", + "*** End Patch", + "", + ].join("\n"); + const entries = EDIT_MODE_STRATEGIES.apply_patch.matcherEntries({ input }); + expect(entries).toEqual([ + { path: "src/a.ts", digest: "const a = 1;" }, + { path: "README.md", digest: "# Heading" }, + ]); + }); + + it("returns undefined when no entries are recoverable yet", () => { + expect(EDIT_MODE_STRATEGIES.hashline.matcherEntries({ input: "" })).toBeUndefined(); + expect(EDIT_MODE_STRATEGIES.apply_patch.matcherEntries({ input: "*** Begin Patch\n" })).toBeUndefined(); + expect(EDIT_MODE_STRATEGIES.replace.matcherEntries({})).toBeUndefined(); + }); +}); + /** * Integration: a hashline edit payload whose only path lives in the * `[demo.ts#TAG]` section header must trigger the bundled `ts-no-any` rule @@ -184,4 +242,95 @@ describe("hashline edit + path-scoped TTSR (regression: #3646)", () => { }); expect(matches).toEqual([]); }); + + it("multi-file hashline isolates a .md hunk's `: any` from a sibling .ts entry", async () => { + // PR review (#3648): a multi-file payload that adds `: any` only to a + // Markdown hunk MUST NOT trip the TS-only `tool:edit(*.ts)` rule. Per-file + // matchers pair each path with its own digest. + const manager = await makeManager(); + const input = [ + "[README.md#ABCD]", + "SWAP 1.=1:", + `+${VIOLATING_LINE}`, + "[src/ok.ts#EF01]", + "SWAP 1.=1:", + "+export const ok = 1;", + "", + ].join("\n"); + + const entries = EDIT_MODE_STRATEGIES.hashline.matcherEntries({ input }); + expect(entries?.map(e => e.path)).toEqual(["README.md", "src/ok.ts"]); + + const allMatches: string[] = []; + for (const entry of entries ?? []) { + const matches = manager.checkSnapshot(entry.digest, { + source: "tool", + toolName: "edit", + filePaths: [entry.path], + streamKey: `toolcall:test#${entry.path}`, + }); + allMatches.push(...matches.map(r => r.name)); + } + expect(allMatches).toEqual([]); + }); + + it("multi-file hashline fires only on the .ts entry when the .ts entry carries `: any`", async () => { + const manager = await makeManager(); + const input = [ + "[README.md#ABCD]", + "SWAP 1.=1:", + "+# Heading", + "[src/bad.ts#EF01]", + "SWAP 1.=1:", + `+${VIOLATING_LINE}`, + "", + ].join("\n"); + + const entries = EDIT_MODE_STRATEGIES.hashline.matcherEntries({ input }); + const matchesByPath = new Map(); + for (const entry of entries ?? []) { + const matches = manager.checkSnapshot(entry.digest, { + source: "tool", + toolName: "edit", + filePaths: [entry.path], + streamKey: `toolcall:test2#${entry.path}`, + }); + matchesByPath.set( + entry.path, + matches.map(r => r.name), + ); + } + expect(matchesByPath.get("README.md")).toEqual([]); + expect(matchesByPath.get("src/bad.ts")).toEqual(["ts-no-any"]); + }); + + it("multi-file apply_patch isolates a .md hunk's `: any` from a sibling .ts hunk", async () => { + const manager = await makeManager(); + const input = [ + "*** Begin Patch", + "*** Update File: README.md", + "@@", + "-old", + `+${VIOLATING_LINE}`, + "*** Update File: src/ok.ts", + "@@", + "-old", + "+export const ok = 1;", + "*** End Patch", + "", + ].join("\n"); + + const entries = EDIT_MODE_STRATEGIES.apply_patch.matcherEntries({ input }); + const allMatches: string[] = []; + for (const entry of entries ?? []) { + const matches = manager.checkSnapshot(entry.digest, { + source: "tool", + toolName: "edit", + filePaths: [entry.path], + streamKey: `toolcall:test3#${entry.path}`, + }); + allMatches.push(...matches.map(r => r.name)); + } + expect(allMatches).toEqual([]); + }); });