feat(agent): add addTrims and addZooms, so a cut stops costing a round trip - #258
Open
EtienneLescot wants to merge 1 commit into
Open
feat(agent): add addTrims and addZooms, so a cut stops costing a round trip#258EtienneLescot wants to merge 1 commit into
EtienneLescot wants to merge 1 commit into
Conversation
…d trip
The measured cost of an auto-enhance turn is not its context, it is its shape:
19 tool calls in series, six addTrim and nine addZoom one at a time, each a full
round trip to the provider. `deep-agent/service.ts` says as much where it raises
`recursionLimit` — "one step per silence". On a half-hour recording that is
hundreds of round trips for a decision the model made in one breath.
Two batch tools, and nothing else changes:
- The element schema IS the unitary schema (`z.array(addTrimArgs)`), and the
executor REPLAYS the unitary tool per item, folding the document forward. Clip
resolution, anchoring, clamping, the wording of every refusal — identical by
construction rather than by a second implementation staying in step. A batch of
N is exactly N unitary calls minus N-1 round trips, and a test asserts that
against a document built the long way.
- Partial application, deliberately. `replaceTimeline` is the repo's other
array-taking tool and refuses in one block; that is right for rebuilding a
timeline and ruinous for adding ten independent cuts. Each item stands alone,
the result leads with requested / appliedCount / refusedCount, and `refused`
names the index and the unitary reason. `ok:false` is kept for the one case
where nothing landed — the only one where the document did not move, and the
only one where chat-service's applied-calls list may legitimately stay empty.
- No cap on the array, for the reason `getTranscript` just taught us: a number
guessed here would be the next thing to cut a recording in half in silence.
The unitary tools stay. A one-off correction should not need a one-element array,
and `setTrim` — the tool for "move that trim" — was never in scope for a batch.
Two invariants that had to be found rather than assumed:
- `diffMatches` reads an id at the TOP level of the tool result, so a batch would
have made the only check that catches a tool lying about its own writes return
vacuously true — silently, on the calls that write the most. It now verifies
every entry of `applied`. Removing that branch turns the new oracle test red.
- Overlapping zooms in one batch stay overlapping, because two `addZoom` calls
already do: no `add*` path clamps against neighbours, in the agent or the UI.
Deconflicting inside the batch would make the result depend on how the model
chose to group its calls. Pinned by a test so it stays a decision.
`MUTATING_TOOL_NAMES` carries both names, which is what puts them behind the
"project edits disabled" wall — the existing loop over that table ("no write
escapes by being added later") covers them without a new test.
📝 WalkthroughWalkthroughThe agent now supports batch trim and zoom tools. Each entry executes independently, valid entries apply, refused entries report indexed errors, and fully refused batches fail without a document. Agent prompts, documentation, tool counts, and workbench validation were updated. ChangesBatch editing tools
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Agent
participant BatchExecutor
participant UnitaryExecutor
participant Document
Agent->>BatchExecutor: submit batch trim or zoom items
BatchExecutor->>UnitaryExecutor: execute items sequentially
UnitaryExecutor->>Document: apply valid item
BatchExecutor-->>Agent: return applied and refused results
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@electron/ai-edition/agent-tools.ts`:
- Around line 364-366: The batch schemas addTrimsArgs and addZoomsArgs must
validate only their container shape, not individual elements; update
electron/ai-edition/agent-tools.ts:364-366 and 431-433 accordingly. In
ExecuteAgentTool at electron/ai-edition/agent-tools.ts:1032-1036 and 1273-1277,
validate only the batch container and let applyBatch pass raw items to the
unitary executor so each invalid sibling is reported at its refused[index].
Preserve and update the related expectations in
electron/ai-edition/agent-tools.test.ts:263-336.
In `@electron/ai-edition/deep-agent/service.test.ts`:
- Line 405: Update the test around the buildTools expectation so it reflects
that edits-disabled mode returns 21 tools, not 20. Change the test title and its
assertion consistently, while preserving the existing tool-building behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: baa3a0a3-839e-47ca-86ef-b931f98644f1
📒 Files selected for processing (7)
electron/ai-edition/agent-tools.test.tselectron/ai-edition/agent-tools.tselectron/ai-edition/deep-agent/service.test.tselectron/ai-edition/deep-agent/service.tstechnical-documentation/architecture/ai-agent.mdworkbench/l0/oracles.wb.tsworkbench/lib/oracles.ts
Comment on lines
+364
to
+366
| export const addTrimsArgs = z.object({ | ||
| ranges: z.array(addTrimArgs).min(1), | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Tracked file sizes:"
wc -l electron/ai-edition/agent-tools.ts electron/ai-edition/agent-tools.test.ts 2>/dev/null || true
echo
echo "Relevant agent-tools.ts sections:"
sed -n '330,445p' electron/ai-edition/agent-tools.ts
echo "---"
sed -n '970,1055p' electron/ai-edition/agent-tools.ts
echo "---"
sed -n '1245,1295p' electron/ai-edition/agent-tools.ts
echo
echo "Relevant test section:"
sed -n '240,355p' electron/ai-edition/agent-tools.test.ts
echo
echo "Search for addTrim/addZoom/applyBatch/buildTools references:"
rg -n "function applyBatch|const applyBatch|export .*addTrim|export .*addZoom|addTrimsArgs|addZoomsArgs|case \"addTrims\"|case \"addZooms\"|buildTools|tools\\[" electron/ai-edition/agent-tools.tsRepository: getopenscreen/openscreen
Length of output: 15785
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "applyBatch implementation:"
sed -n '680,765p' electron/ai-edition/agent-tools.ts
echo
echo "executeAgentTool signature/imports/top:"
sed -n '1,80p' electron/ai-edition/agent-tools.ts
sed -n '765,850p' electron/ai-edition/agent-tools.ts
echo
echo "buildTools references:"
rg -n "function buildTools|const buildTools|buildTools\(" electron/ai-edition -S || true
echo
echo "focused parser behavior probe:"
node - <<'JS'
const Zod = require('zod')
const secondsSchema = Zod.number().finite().nonnegative()
const depthSchema = Zod.number().int().min(1).max(6)
const addTrimArgs = Zod.object({
startSec: secondsSchema,
endSec: secondsSchema,
})
const addZoomArgs = Zod.object({
startSec: secondsSchema,
endSec: secondsSchema,
depth: depthSchema.default(3),
})
const addTrimsArgs = Zod.object({ ranges: Zod.array(addTrimArgs).min(1) })
const addZoomsArgs = Zod.object({ regions: Zod.array(addZoomArgs).min(1) })
for (const [name, schema, items, fields] of [
['trim startSec', addTrimsArgs, [{ startSec: 'bad', endSec: 2 }, { startSec: 1, endSec: 2 }], 'ranges'],
['zoom depth', addZoomsArgs, [{ startSec: 1, endSec: 3, depth: 7 }, { startSec: 4, endSec: 6 }], 'regions'],
]) {
const parsed = schema.safeParse({ [fields]: items })
console.log(JSON.stringify({ name, parsedSuccess: parsed.success, error: parsed.success ? null : parsed.error.flatten() }, null, 2))
}
JSRepository: getopenscreen/openscreen
Length of output: 12423
Keep batch element validation inside applyBatch.
addTrimsArgs.safeParse and addZoomsArgs.safeParse must not run here. They reject arrays like { ranges: [{ startSec: "bad", endSec: 2 }, { startSec: 1, endSec: 2 }] } or { regions: [{ depth: 7 }, ...] } before any sibling request reaches applyBatch, so refused[index] cannot report the invalid item. Validate only the batch container in ExecuteAgentTool, and let applyBatch pass raw items to the unitary executor as its tests and comments already require.
📍 Affects 2 files
electron/ai-edition/agent-tools.ts#L364-L366(this comment)electron/ai-edition/agent-tools.ts#L431-L433electron/ai-edition/agent-tools.ts#L1032-L1036electron/ai-edition/agent-tools.ts#L1273-L1277electron/ai-edition/agent-tools.test.ts#L263-L336
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@electron/ai-edition/agent-tools.ts` around lines 364 - 366, The batch schemas
addTrimsArgs and addZoomsArgs must validate only their container shape, not
individual elements; update electron/ai-edition/agent-tools.ts:364-366 and
431-433 accordingly. In ExecuteAgentTool at
electron/ai-edition/agent-tools.ts:1032-1036 and 1273-1277, validate only the
batch container and let applyBatch pass raw items to the unitary executor so
each invalid sibling is reported at its refused[index]. Preserve and update the
related expectations in electron/ai-edition/agent-tools.test.ts:263-336.
Source: Coding guidelines
|
|
||
| describe("the tools when the user has turned project edits off", () => { | ||
| it("still builds all 18 — the model has to be able to NAME the edit", () => { | ||
| it("still builds all 20 — the model has to be able to NAME the edit", () => { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Expect 21 tools when edits are disabled.
buildTools returns 21 entries regardless of editsAllowed: three read tools and 18 mutating tools. Line 405 uses 20, so the updated count expectation will fail. Change the title and assertion to 21.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@electron/ai-edition/deep-agent/service.test.ts` at line 405, Update the test
around the buildTools expectation so it reflects that edits-disabled mode
returns 21 tools, not 20. Change the test title and its assertion consistently,
while preserving the existing tool-building behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements the main recommendation of #217. The measured cost of an auto-enhance turn is not its context, it is its shape: 19 tool calls in series — six
addTrimand nineaddZoomone at a time — each a full round trip.deep-agent/service.tssays so where it raisesrecursionLimit: one step per silence. On a half-hour recording that is hundreds of round trips for a decision the model made in one breath.Two batch tools,
addTrims(ranges[])andaddZooms(regions[]). The unitary tools stay.A batch is N unitary calls, minus N−1 round trips. The element schema is the unitary schema (
z.array(addTrimArgs)), and the executor replays the unitary tool per item, folding the document forward. Clip resolution, anchoring, clamping, the wording of every refusal — identical by construction, not by a second implementation staying in step. A test builds the same document both ways and compares.Partial application, deliberately.
replaceTimelineis the repo's other array-taking tool and refuses in one block — right for rebuilding a timeline, ruinous for adding ten independent cuts. Here each item stands alone: the result leads withrequested/appliedCount/refusedCount, andrefusednames the index and the unitary reason, so a partial outcome is readable without re-reading the document.ok:falseis reserved for the case where nothing landed — the only one where the document did not move, and the only one wherechat-service's applied-calls list may legitimately stay empty.No cap on the array, for the reason
getTranscriptjust taught us in #256: a number guessed here would be the next thing to cut a recording in half in silence.Two invariants that had to be found rather than assumed
diffMatches(workbench/lib/oracles.ts) reads an id at the top level of a tool result. A batch puts its ids insideapplied[], so the only check that catches a tool lying about its own writes would have returned vacuously true — silently, on the calls that write the most. #217 named this risk; it is real. It now verifies every entry, and removing that branch turns the new oracle test red.Overlapping zooms in one batch stay overlapping, because two sequential
addZoomcalls already do: noadd*path clamps against neighbours, in the agent or in the UI (useTimelineappends too). Deconflicting inside the batch would make the outcome depend on how the model chose to group its calls, and would giveaddZoomsrules its unitary sibling does not have. Pinned by a test so it stays a decision rather than an accident. The bench still flags the overlap througheditorial.tszoomIssues, which is where that argument belongs.Related issue
Refs #217 — §1 bis, "RECOMMANDATION PRINCIPALE".
Type of change
Release impact
Desktop impact
Testing
npm test: 137 files, 1632 passed.npm run wb:l0: 214 passed, 44 pre-existing failures (the unversioned real fixture, unrelated).tsc --noEmit,tsc -p tsconfig.test.json,wb:typecheck, biome,docs:checkall clean.New cases: batch/unitary equivalence for trims; partial application naming the refused index and reason; a whole-batch refusal reported as an error with
Nothing was modified;addZoomsrefusing only the region that covers no clip while the others land with theirrenderedScale; the overlap decision; and thediffMatchesbatch branch, verified to fail without the fix.Three inventories that pin the tool surface failed loudly and were updated —
MUTATING_TOOL_NAMES,OPENSCREEN_TOOLS, and the two tool-count assertions (19 → 21).MUTATING_TOOL_NAMEScarrying both names is what puts them behind the "project edits disabled" wall; the existing loop over that table, named "no write escapes by being added later", covers them without a new test.Not done here
#217 also asks for a bench scenario that decides whether the model can use a batch tool correctly before generalizing — a batch is harder to call than a unitary tool, it needs a well-formed array first time. That needs a live paid run against the unversioned real take, so it is not in this PR. Nothing is generalized in the meantime: both unitary tools remain, and the system prompt points at the batch without forbidding the single call.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation