Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
…successful retry as origin best result 35a9e81 (#11514) records completions on the task_complete early returns so the spawn cap can relay them instead of re-spawning. but the verify-retry handoff records a completion the router itself just judged a failed build: the dead-url verification annotation makes that text systematically longer than a clean success, so longest-wins recordOriginResult keeps the failure and the successful retry's shorter deliverable can never displace it. at the per-origin spawn cap tasks.ts then relays the dead-url completion — planner-only verification directive included — verbatim to the user as the final answer. gate verify-failed completions (deadUrls > 0) out of origin-result capture at every site: a known-failed build is not a relayable deliverable, and the cap's honest "attempted N times" fallback covers the nothing-clean case. the main-path record now routes through the same helper so the key contract (#8875) and the gate live in one place.
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
3 participants
Defect
35a9e81 (#11514) added
captureOriginResultForCompletionon the router'stask_completeearly returns so a finished deliverable survives to the per-origin spawn cap. But the verify-retry handoff records a completion the router itself just judged a failed build (deadUrls.length > 0), andrecordOriginResultis pure longest-wins — no verification-status gate. The #11514 justification ("recordOriginResult is monotonic so recording here is always safe") is wrong for known-failed results: longest-wins is not correctness-wins.Failure path (refs on pre-fix
develop):task_completereferencing a URL that fails liveness verification.annotateUnverifiedUrlsstamps the narration with the dead-URL failure annotation (sub-agent-router.ts~2988) — which systematically makes the failed text longer than a clean success text.retryIncompleteBuildspawns attempt 2, and the verify-retry handoff records the failed completion for the origin (sub-agent-router.ts:1135; same-shaped captures at 1150/1196).sanitizeSuccessorMetadatapreserves the origin keys byte-for-byte, so both attempts record under the SAME origin-result key.recordOriginResult(if (prev && candidate.length <= prevLen) return;) keeps the longer verify-FAILED text — the successful retry can never displace it.tasks.ts(~1199-1211) relaysbest.deliverable ?? best.textverbatim: the user receives the dead-URL completion the router judged a failed build — including the planner-only[verification: … do NOT tell the user the app is live …]directive — as the final answer.Fix
Gate verify-FAILED completions (
deadUrls.length > 0) out of origin-result capture at every site. A known-failed build is not a relayable deliverable; with nothing clean captured, the spawn cap's honest "attempted N times" fallback covers the case where no clean completion ever lands. The main-path record now routes through the same helper, so the #8875 key contract and the gate live in one place.recordOriginResultstays longest-wins for the results that are actually relayable.Tests (real path, no mocks of the thing under test)
New
origin-result-verify-failed-shadow.test.tsdrives the realhandleEventand the realTASKSspawn-cap branch through the exact two-attempt lineage against a real local HTTP app host whose deploy state transitions 404 → 200 (attempt 1's dead URL, then the retry's verified-live URL; realsafeFetchprobes both ways). The retry successor session uses the byte-identical metadata shaperetryIncompleteBuildproduces (sanitizeSuccessorMetadata+buildVerifyRetryCount/retryOfSessionId).RED on pre-fix develop — reproduced at the exact user-facing symptom:
GREEN after the fix. The #11514 test that pinned the old behavior (verify-retry handoff records the failed completion) is updated to pin the corrected semantics, with the rationale in-line; its other pins (lineage-dedupe longest-wins for clean completions, honest cap fallback, deliverable relay at the cap) are unchanged and still pass.
bun run typecheckexit 0;bun run lint:checkclean (plugin scope).Evidence
[verify] probe … → HTTP 404/re-dispatched sub-agent after failed verificationrouter log path.bestResultFor()state inspected before/after each attempt in the tests — failed capture absent, clean retry result retained, spawn-cap reply free of the failure annotation.