docs(agent): pistes d'amélioration de l'agent d'édition, mesurées - #217
Open
EtienneLescot wants to merge 12 commits into
Open
docs(agent): pistes d'amélioration de l'agent d'édition, mesurées#217EtienneLescot wants to merge 12 commits into
EtienneLescot wants to merge 12 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe pull request adds Whisper error logging, exposes cursor-track budget overflow, adds cursor and zoom-quality regression tests, synchronizes workbench timeouts, and updates workbench documentation and improvement notes. ChangesWorkbench and cursor-track behavior
Whisper transcription diagnostics
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
4 tasks
EtienneLescot
force-pushed
the
claude/agent-improvement-leads
branch
from
July 31, 2026 23:47
9ee0ba5 to
0d2dbcb
Compare
EtienneLescot
force-pushed
the
release/v1.8.0
branch
from
August 1, 2026 01:04
4afa3d4 to
2200c7c
Compare
Document de travail: chaque piste porte la mesure qui la justifie et la contre-mesure qui la départagera. Rien n'est appliqué ici.
…pels par lot Le track n'était pas la cause des échecs: les deux tours réussis prenaient 117 s et 112 s pour un couperet à 120 s. Ce que révèle la mesure, c'est que le tour émet 19 appels d'outils en série — six addTrim et neuf addZoom un par un.
EtienneLescot
force-pushed
the
claude/agent-improvement-leads
branch
from
August 1, 2026 13:48
0d2dbcb to
64b2f74
Compare
The choice `simplifyAxis` makes — Douglas-Peucker per axis against time, not over the (x,y) path — was defended by a comment and by nothing else. Substituting a path-space simplification leaves all eleven tests of this file green: the only one that bounds the reconstruction sweeps strictly monotonically, and the two implementations agree there. The trajectory that separates them is an out-and-back. In path space it lies on its own chord, so the whole excursion collapses and interpolation then swears the pointer never moved — measured at 0.380 of the frame for a 0.02 tolerance on a real screencast. The new case keeps the apex and re-asserts the same bound; it is the only one of the fourteen that fails on the wrong implementation.
`DEFAULT_MAX_TRACK_POINTS` budgets the gap floor and the rate. The points nothing can put back — a pointer-shape change, a non-move event, the ends of a parked run — are exempt by design and stack on top, so a capture rich in them lands above the ceiling and no field said so. `truncated` could not carry it: that one means "you are seeing less than you asked for", which is the opposite claim. Charging the exempt points to the budget would mean dropping a shape change to hold a number, which is the one thing this track must never do. So the overflow is reported, not prevented: `overBudget` carries the count and the reason, and is absent when the budget held — the common payload is byte-for-byte unchanged.
`zoomPlacement` intersected the numerator and summed the denominator, under a comment claiming both were unions. The comment justified itself with a rule that does not hold on the documents this oracle exists to expose: two zooms may not overlap, but only `setZoom` goes through the clamp that enforces it — `addZoom` appends, so an agent can stack them, which is why `editorial.ts` carries an `overlap` check at all. On such a document the shared seconds are counted twice below and once above, and `precision` reads low for a reason that has nothing to do with placement. Two stacked zooms of 5 s covering 8-15 s now score 1, not 0.7.
`DEFAULT_TURN_TIMEOUT_MS` moved to 300 s when the bench's own cutoff turned out to sit three seconds above a normal turn. This config kept its own copy at 120 s, so a `.wb.ts` driving a live turn would have been killed by vitest first — the exact failure the harness comment exists to prevent, reintroduced one file away. Import the constant instead of restating it.
`recordError` stored the message in `lastError`, read by a `status` getter nothing on the transcribe path calls. The renderer does toast the failure now, but the main process left no trace at all: a packaged build without the helper gave a support thread nothing to point at. One line to the log, and the manual checklist updated — its entry still described the state before the toast existed.
…ile no clone has The README still announced 356 points and 24 238 characters for `getCursorTrack` and claimed those numbers were asserted in `l0/real-fixture.wb.ts` — which asserts 148 and 7 797 since the keyframe reduction. A reference that cites a test saying the opposite is worse than none: it is where the stale figure gets re-fetched. Four places also sent the reader to `workbench/fixtures/README.md`. That path is inside a gitignored directory and was never versioned, so it resolves in no clone at all; `check-docs` does not catch it because it is inline code, not a link. The provenance it promised is in this README, so say so there and drop the pointer. While at it, the consequence a newcomer meets first: 44 L0 tests fail on a fresh clone, all on that missing take, and nothing in CI runs the bench to say so.
…, and correct them `technical-documentation/` is reference — "describe, don't narrate", no plans, no changelogs — and a list of leads with a run table and (fixed)/(not fixed) markers is what that rule exists to keep out. It belongs next to the bench that produced the measurements, so that is where it goes; what gets settled will go to `decisions.md`, and the anti-overfitting guardrail lands in the bench README where it will actually be read — at the moment someone touches the system prompt. Corrections, from checking every claim against the code: - The customScale lead opened on a false premise. `depthIsOverridden` is emitted by the snapshot, explained by `zoomNote`, and `setZoom`'s description already says word for word that passing `depth` clears the override. The real gap is narrower and is now stated: it reaches the snapshot and the tool description, not the tool RESULTS nor the system prompt. - The whisper bullet was stale. The failure has reached a toast since the automatic transcription landed — a commit that is an ancestor of this document's own first commit. What survives is the developer-facing wording and the untested path. - The zoom-grounding numbers were filed under "Mesuré". No oracle computes the focus-to-cursor gap; it is a manual observation, and writing the missing oracle is now the first lead of that section rather than an afterthought. - 356 points became 148 two sections earlier. The lead that told the reader to reduce the noise first was asking for work already done. - Lossless became lossless within the 0.02 tolerance, the depth-to-scale "table" became the legend announced to the model, "19 round trips" became 19 calls with the note that `rounds` is the number that would prove the latency claim, and the batch-tool lead now cites `replaceTimeline` — the repo's own precedent for refusing a batch outright — instead of raising the risk hypothetically. Every figure that no versioned artefact can reproduce now says so, once, at the top.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
electron/stt/whisperServer.ts (1)
146-152: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for the new
[stt]log.
recordError()now writesconsole.error("[stt] " + message)for missing or non-executable helpers. Add a Vitest test inelectron/stt/whisperServer.test.tsthat triggers one of those paths, spies onconsole.error, and asserts the[stt]prefix.🤖 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/stt/whisperServer.ts` around lines 146 - 152, Add a Vitest regression test in whisperServer.test.ts that exercises recordError() through a missing or non-executable helper path, spies on console.error, and asserts the emitted message begins with the “[stt]” prefix. Restore the console.error spy after the test and keep existing error-state assertions intact.Source: Coding guidelines
workbench/l0/quality.wb.ts (1)
418-439: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a recall assertion for the overlap case.
The regression covers
zoomSecandprecision, butcoveredZoneSecandrecalluse the zone-report path. Assert that the covered union is 7 seconds andrecallis 1.Proposed test additions
expect(placement.zoomSec).toBeCloseTo(7, 4); expect(placement.precision).toBeCloseTo(1, 4); + expect(placement.coveredZoneSec).toBeCloseTo(7, 4); + expect(placement.recall).toBeCloseTo(1, 4);🤖 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 `@workbench/l0/quality.wb.ts` around lines 418 - 439, Extend the overlap test around zoomPlacement to assert that the zone-report results also use the merged 8–15 second union: verify coveredZoneSec is 7 and recall is 1, alongside the existing zoomSec and precision assertions.
🤖 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 `@src/lib/ai-edition/timeline/cursor-track.test.ts`:
- Around line 223-228: Correct the test comment in the “says so when the
mandatory points push it over maxPoints” case to state that the pointer changes
shape every 50ms, matching sweep’s i * 50 timing and alternating shape on each
index. Do not change the test data or sampling behavior.
In `@src/lib/ai-edition/timeline/cursor-track.ts`:
- Around line 69-75: Update the overBudget JSDoc and its associated explanation
message to include mandatory points, including the always-retained first and
last samples, alongside shape changes, non-move events, and parked-run ends.
Ensure the wording accurately explains endpoint-only overflow, such as a
two-sample track with maxPoints set to 1.
In `@vitest.workbench.config.ts`:
- Around line 19-24: Update testTimeout in the Vitest configuration to a
duration strictly greater than DEFAULT_TURN_TIMEOUT_MS, preserving
DEFAULT_TURN_TIMEOUT_MS as the harness Promise.race deadline so the harness
timeout produces the workbench diagnostic first.
In `@workbench/l0/real-fixture.wb.ts`:
- Line 78: Update the French comment near the real-fixture assertion to
reference the explicit workbench README path, workbench/README.md, while
preserving the existing section reference and meaning.
---
Nitpick comments:
In `@electron/stt/whisperServer.ts`:
- Around line 146-152: Add a Vitest regression test in whisperServer.test.ts
that exercises recordError() through a missing or non-executable helper path,
spies on console.error, and asserts the emitted message begins with the “[stt]”
prefix. Restore the console.error spy after the test and keep existing
error-state assertions intact.
In `@workbench/l0/quality.wb.ts`:
- Around line 418-439: Extend the overlap test around zoomPlacement to assert
that the zone-report results also use the merged 8–15 second union: verify
coveredZoneSec is 7 and recall is 1, alongside the existing zoomSec and
precision assertions.
🪄 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: 0a577224-37dd-4b1f-a32a-db94c9404a29
📒 Files selected for processing (11)
electron/stt/whisperServer.tssrc/lib/ai-edition/timeline/cursor-track.test.tssrc/lib/ai-edition/timeline/cursor-track.tstechnical-documentation/testing/manual-e2e-checklist.mdvitest.workbench.config.tsworkbench/README.mdworkbench/agent-improvement-leads.mdworkbench/l0/quality.wb.tsworkbench/l0/real-fixture.wb.tsworkbench/lib/quality.tsworkbench/lib/real-fixture.ts
…s correctly Four review findings, all correct. `testTimeout` was set to exactly `DEFAULT_TURN_TIMEOUT_MS`, which does not fix the race it was meant to fix — equal deadlines only make it unbiased. The harness has to win for a slow turn to be classified as TIMEOUT instead of dying as a killed worker, so the vitest deadline now sits 30 s above it. `overBudget` listed shape changes, non-move events and parked-run ends, but the mandatory set also always holds the first and last sample: a two-point track under `maxPoints: 1` overflows for a reason the message did not name, and the model was told something false. Both the JSDoc and the message now say mandatory points, and enumerate all four kinds. And two comments: the test said the shape flips every 100 ms where `sweep` samples at 50 ms and alternates on every index, and a comment in `workbench/l0/` pointed at `README.md`, which from there resolves to a file that does not exist.
…worker
The comment named `await import("deepagents")`, a package 0e53709 removed from
the dependencies, at a line number that had also drifted. A comment that
justifies a configuration is how that configuration stays open to question — this
one had stopped being checkable.
The cost survived the package, because it was never that factory: `runChat`
dynamically imports `./deep-agent/service`, and the graph underneath it costs
~1.25 s per worker, measured by timing the import inside a `.wb.ts`. About 0.38 s
of that is `langchain` itself. Seven of the nineteen `.wb.ts` files reach that
path, so isolating them would re-pay it six more times.
The two module-Map line numbers are corrected as well, and the guard is credited
to the harness rather than to `runScenario`.
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.
Pistes d'amélioration de l'agent d'édition, appuyées sur les mesures du workbench. Rien à fusionner ici — cette PR est un document de travail, à traiter plus tard.
Chaque piste porte la mesure qui la justifie et, quand elle existe, la contre-mesure qui la départagera. L'ordre est celui où je les traiterais.
1. Le poids du track fait échouer un tour sur deux
Mesuré. Sur la prise réelle de 66 s,
getCursorTrackrend 356 points pour 24 238 caractères. La requête suivante passe à ~45 000 caractères. Sur 5 répétitions du prompt wizard, 3 ont expiré à 120 s, toujours au même endroit : juste après l'appel à l'outil. Les 2 qui aboutissent produisent un montage correct.Ce n'est pas un défaut du modèle : lui donner la donnée le fait échouer.
Pistes, de la moins à la plus intrusive :
virtualSecquand il est égal àatSec. 28 % du payload, strictement redondant tant qu'aucune coupe n'existe. Un champvirtualEqualsSource: trueen tête suffirait. Gain immédiat, aucune perte d'information.Le plafond de
buildCursorTrackest par ailleurs mou :DEFAULT_MAX_TRACK_POINTSborne la grille, mais les points gardés pour un changement de forme s'ajoutent par-dessus sans quetruncatedle signale. Ici 356 pour 400, sans conséquence — une capture riche en changements de pointeur dépasserait silencieusement.2. Le modèle place ses zooms d'après le transcript, pas d'après la trajectoire
Mesuré. Il appelle bien
getCursorTrack. Mais en comparant lefocusqu'il choisit à la position réelle du curseur dans sa propre fenêtre de zoom : 7 sur 9 sont faux, trois de plus d'un tiers d'image. Le pire vise(0.33, 0.09)— haut de l'écran — quand le curseur est à(0.38, 0.60).Son récit le trahit : il annonce un zoom sur « Iceman, Views » cinq secondes avant que ces mots soient prononcés. Il raconte une lecture de la trajectoire qu'il n'a pas faite.
Rappel 6/6 zones annotées, mais précision 0,41 — il zoome 38 % de la vidéo. Toucher toutes les zones en arrosant n'est pas de la détection.
Pistes :
addZoompourrait renvoyer la position réelle du curseur sur la fenêtre demandée, à côté dufocusreçu. Le modèle apprend l'écart au premier appel, sans qu'on lui impose quoi que ce soit. C'est la piste que je préfère : elle informe au lieu de contraindre.{atSec, cx, cy}sont peut-être trop plates pour qu'il y corrèle une fenêtre temporelle. À tester en réduisant d'abord le bruit (piste 1), pas en changeant la forme.3.
customScalerenddepthinopérant en silenceMesuré.
describe-zoomsest passé de 60 % à 98 % après correction de la table depth→échelle.describe-zooms-migratedreste à 33 % : quand un zoom porte uncustomScale, ledepthne rend plus rien et aucun champ ne le dit au modèle.Piste. Le snapshot expose déjà
depthIsOverridden. Reste à vérifier qu'il atteint le modèle dans tous les chemins, et quesetZoomdit clairement que passerdepthefface lecustomScale.4. Un patron récurrent : l'absence traitée comme un non-événement
Trois occurrences rencontrées en pilotant l'app, sans rapport entre elles :
Le troisième mérite un correctif, et le patron mérite d'être nommé quelque part : distinguer « je n'ai pas trouvé » de « il n'y a rien » est la même discipline côté UI et côté agent.
5. Le banc : ce qui manque encore
noa déjà matché danscannot, accusant de mensonge une réponse honnête. Et « pas de signal » compte comme une réussite, donc une réponse en français passerait au vert sans rien vérifier. Ce qui se calcule doit rester déterministe ; ce qui demande de lire du sens doit passer à un juge, sur les tours persistés, avec verdicts conforme / fautif / indéterminé.workbench/fixtures/README.md.Summary by CodeRabbit
Bug Fixes
Improvements
Tests