mirror of
https://github.com/nexu-io/open-design.git
synced 2026-10-27 09:51:12 +01:00
main
1242
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
|
3af55e9f22 |
fix(daemon): stop blocking a strategy task whose clarification form renders (#7509)
The clarification form has two parsers by design. The web renderer
(`splitOnQuestionForms`) decides what the user sees; the daemon detector
(`scanQuestionForms`) decides what the host believes was asked. The app
boundary forbids `apps/daemon` importing `apps/web/src`, so the daemon side
is a deliberate mirror — and its own header says to keep it in sync or
promote a shared parser once the two drift.
They drifted, and the two answers are load-bearing against each other. The
OD Next coordinator blocks a `clarification_required` turn that rendered no
form (`od_next_clarification_form_missing`), settling the strategy task
terminal + blocked. So when the web renders a form the daemon scored as
absent, the user is left filling in a live form belonging to an
already-terminal task, and submitting it returns 409
STRATEGY_TASK_STATE_MISMATCH.
That is the production report: an agent wrapped its form in a duplicate of
its own open tag. The web treats an outer block that fails to parse but
holds another open marker as a false positive, unwinds to the inner marker
and renders the form. The daemon had no such unwind, charged the outer block
as unrenderable, and blocked the task underneath a form the user could still
fill in.
Mirror both of the renderer's recovery paths — a failed block whose body
holds another marker, and an unterminated marker with another marker after
it — so a marker is only charged once the renderer would also have given up
on it. Recovery is not permissiveness: a wrapper with nothing left to retry
from is still the unrenderable block it always was, and two forms the chat
renders separately stay two, so the one-round protocol still catches the
ambiguity.
Also accept a bare top-level questions array. `parseForm` has always read
`[{…}]` as the questions list, so requiring the `questions` key made another
displayed form invisible to every daemon consumer.
Both changes move the daemon in one direction only: toward counting what the
UI already shows. A 20k-case differential run over randomly assembled markup
now finds no input where the two disagree on either the renderable or the
unrenderable count.
Left deliberately out of scope, as backend-only: the UI hardening the report
also asks for — writing the blocked verdict back during state restoration,
and re-checking task state before submitting. With the parse contract
restored the task is no longer blocked, so the reported 409 does not occur;
those remain worth doing as defence in depth for a task blocked for any
other reason.
Plane: OPEND-2364
Co-authored-by: Cheems <94773058+itscheems@users.noreply.github.com>
|
||
|
lefarcenandGitHub
|
1e34ca4059 |
fix(daemon,web): report analytics for daemon-started Runs and mid-write Labs toggles (#7514)
* fix(daemon,web): report analytics for daemon-started Runs and mid-write Labs toggles An OD Next task runs as several physical Runs — the user's request, then the repair and production stages the daemon starts itself. The Run analytics lifecycle was installed inline in POST /api/runs, so only the stage a client asked for ever reported: volume, success, failure, cancellation, duration and artifact counts were all computed on the request stage alone. Walking every Run-creation site rather than only the one the report named found three more callers in the same position — scheduled Automations, live-artifact refreshes, and POST /api/chat. Extract the lifecycle into routes/run-analytics-lifecycle.ts and install it from internalRunCreation.start, which is now the only way a physical Run starts. Its analytics argument is required, so a new caller has to state its identity — including stating that it has none — instead of silently dropping the Run, and a guard check keeps the bypass from coming back. A daemon-created Run inherits its caller's identity and its task lineage from the Run that caused it, resolved through the same deterministic fallback the lifecycle itself applies. Reading the source Run's recovery record instead would race: the lifecycle re-reads host facts before capturing, so a short Run hands off before its own record exists. Also split the Labs mounted guard. It sat in front of the toggle event and the dialog's save pill as well as this component's state, so leaving the section while the write was in flight lost the event and left the pill on "Saving" for a preference the machine already held. * refactor(daemon): move the run analytics lifecycle to its owning domains Review follow-up. `createRunAnalyticsLifecycle` takes no Express request or response, so by `apps/daemon/AGENTS.md` it is a reusable daemon service and belongs under `src/services/`, not `src/routes/`. The run/project record shapes it shares with the run routes describe a Run, not an HTTP exchange, so they move to the runs domain. The seam also carried a second copy of the analytics facts, with its context, snapshot and rollout fields widened to `unknown` so the two definitions could drift. `RunAnalyticsFacts` is now declared once by the lifecycle and reused by `InternalRunCreationService.start`. Consolidating immediately surfaced what the weak copy had been hiding: the route was passing a snapshot shape the lifecycle did not actually accept, and the test was passing an analytics context missing four required fields. Both are now typed at the choke point. * fix(web): keep a late Labs save from taking the shared indicator Review follow-up. Letting the Labs write settle after unmount fixed the stuck "Saving" pill but handed an older request the ability to relabel a newer one: hold the Labs PUT, switch section, start another save, release. The old completion wrote `saved` over the newer save and its timer then forced `idle`, which is worse than the pill it was fixing. `SettingsDialog` owns one indicator for every section and already refuses to let a superseded save win — its own path checks `autosaveLatestRef` and its idle timer is a compare-and-set. Labs bypassed both, and its timer wrote `idle` unconditionally. The dialog now hands out a claim when a writer takes the indicator, and only the current claim may settle it. Labs captures its claim when the write starts, not when it finishes. Comparing the displayed value would not do: a newer section can legitimately be showing the same `saving` the older write left behind. The analytics event still fires after unmount — that half was never about the indicator. * fix(daemon): make the run analytics seam impossible to opt out of by omission Review follow-up. The choke point documented an invariant it did not enforce: `analyticsLifecycle` was optional and invoked with `?.`, so a future factory could build the service, satisfy the new `.runs.start` guard, and still emit neither `run_created` nor `run_finished` — the exact failure this PR exists to close. `RunAnalyticsFacts.requestAnalyticsContext` had the same gap: the contract says a caller with no attributable identity passes `null` explicitly, while the type let it drop the key instead. Both are required now. A harness that wants silence injects a no-op lifecycle and says so. Making the context required immediately caught a service spec that had been omitting it. * fix(web): preserve autosave ownership across async saves |
||
|
CaprikaandGitHub
|
35edb37d60 | fix(media): preserve bound Workspace billing scope (#7504) | ||
|
|
2828a086c8 |
feat(web): add an Open Design Labs settings page with the Design Harness switch (#7445)
* feat(web): add an Open Design Labs settings page with the Design Harness switch OD Next has been opt-in through `odNextStrategyMode` since #7362, but the only way to reach it was `od config set` or an environment variable — and `PACKAGED_CHILD_ENV_ALLOWLIST` strips that variable before the packaged daemon ever sees it. A packaged user had no way to choose either direction, so the strategy was effectively invisible to everyone outside a terminal. Add the missing surface: a Labs page in Settings, between General and Instructions, carrying one row for Design Harness. The section reads and writes on its own rather than threading through `cfg` / `setCfg`. `apps/web/src/types.ts`'s `AppConfig` is a web-local projection that does not carry `odNextStrategyMode`, and `syncConfigToDaemon` serialises an explicit allow-list, so the dialog route costs three edits and fails silently if the allow-list one is missed. One `GET /api/strategies/od-next/rollout` answers the saved mode, who set it, and whether the local safety latch has tripped; one single-field `PUT /api/app-config` writes it back, which the daemon merges per key. Three states lock the switch, each with its own sentence: an environment variable owns the mode, the local latch has tripped (it overrides the saved mode, so the switch would otherwise read as on while every run took the ordinary route), or the status could not be read at all. The lock uses `aria-disabled` rather than `disabled` so the explanation stays reachable by keyboard. Turning the switch off writes `'off'` rather than clearing the key: an absent value means "never touched", which is worth telling apart from a deliberate opt-out. `observe` renders as off and is never rewritten — it is a developer diagnostic mode, and clearing it silently would break a debugging session. "Applies to your next generation" is a permanent line under the item name, not a confirmation that appears after the fact. The switch has no visible effect until the next run, so the user should read that before flipping it. Save outcome reports on the dialog's existing autosave pill, the same surface every other Settings edit uses. `settingsSectionToTracking` also gains a `labs` case. Its default returns `configure_execution_mode`, so without this a Labs page view would have been counted as an execution-mode view and inflated that funnel. * fix(web): start one harness write per click burst Found by clicking the switch three times in a row against a live daemon: all three writes went out. `busy` is state, so the second and third clicks in the same tick still read the pre-render closure, where `busy` is false and `on` still holds the old value — each one starts its own write from a stale baseline. Gate re-entry on a ref that flips synchronously instead. `busy` keeps driving the dimmed styling, which is all it was ever able to do reliably. * feat(analytics): report which harness produced a run, and Labs opt-in The Labs switch exists so a new generation strategy can be compared against the old one. That comparison needs two things nothing was reporting: which side of it a given run landed on, and whether anyone is turning the switch on at all. `run_created` / `run_finished` gain `harness` plus `harness_fallback_reason`. The reason is the more useful half: the switch can be on and the run still take the ordinary route — the project's task type, the selected agent, an explicitly chosen plugin, or an unverified runtime capability each route back, silently. Without the reason code, "I enabled it and nothing changed" is unanswerable from the data; with it, the size of each silent-fallback bucket is a query. Stamped once, on the run-created base props. `run_finished` spreads those, and the crash-recovery replay stores them verbatim, so the dimension reaches all three emitters from a single assignment rather than three that can drift. `labs_item_toggled` is deliberately generic: the experiment is a property (`item_id`), not part of the event name, so a second experiment reuses the event and every dashboard built on it keeps working. Same for `harness` — a third harness is one more enum member, not a new field. The event fires after the preference is persisted, not on click. A failed write rolls the switch back, and an event asserting a preference the machine does not hold is worse than a missing one. Nothing new is built for delivery: `$insert_id` already dedupes (analytics.ts), identity already rides as a PostHog super-property (identity.ts), and both sinks already gate on consent. Best-effort throughout — no local queue, no retry, no compensation path. * feat(web): collect why users turn a Labs experiment off The opt-out event carried no reason, so PostHog could show how many people turned Design Harness off and nothing about why — and the `reason` fields the contract already reserved stayed permanently empty. Ask inline, right after the switch moves. Not a dialog: the user has just declined something, and a modal asking them to justify it reads as friction. The panel resolves exactly once, and every way out of it reports — a chosen reason, an explicit skip, the 120-second timeout, or leaving the page with the question still open. That last one matters: without it an abandoned question is a silent gap, and the share of people who declined to answer reads lower than it is. `skipped` is not a reason, it is the recorded absence of one, which is why a timeout and a deliberate skip store the same value. Picking "other" stops the clock. It is an explicit intent to write something, and taking the panel away mid-sentence would discard it. The opt-out still emits immediately without a reason, and the answer arrives as a second event. Opt-out counts stay whole even when nobody answers; reason distribution reads off the follow-up. Also aligns the switch itself with the toggle the rest of Settings already uses (`.toggle-slider` — 36x20, `--brand`); it had been drawn larger and in a different green. Hover and focus-visible states are new: the shared toggles only lift on hover in dark mode, so in the default theme this control gave no pointer affordance at all and keyboard focus was invisible. * fix(web): match the Labs reason panel to the approved mock The chips shipped as full pills copied from the older assistant-feedback panel; the mock asks for rounded rectangles. Take the geometry from the mock directly — 8px radius, 6x11 padding, 7px gaps, 9px on the input and submit — with tokens standing in for its literal greys. Also stop the switch drifting. The row was centre-aligned, so opening the reason panel pushed the switch down to the middle of a block it does not belong to. Top-align the row and offset the switch to sit level with the name and hint instead, which is where the mock puts it and where it stays whether or not the panel is open. * fix(daemon): keep the harness decision across a clarification continuation Answering a clarification creates a second physical Run, and the rollout is only evaluated on the branch that resolves a project — which that path does not take. So a continuation carried no decision at all, and its `run_created`, `run_finished` and recovery replay reported no harness. The runs that hit this are exactly the OD Next runs that stopped to ask a question. Dropping them leaves the comparison the dimension exists for biased toward tasks that never needed clarifying, which is the opposite of a neutral sample. Inherit from the source Run rather than re-reading the setting: the user may have flipped the switch while the question was on screen, and this Run belongs to the decision its task started under. `resolveClarificationContinuation` already validates that source against the locked task, so it is the trustworthy copy — the same reasoning, and the same seam, as the task input snapshot the continuation already inherits beside it. An idempotent strategy retry skips the evaluation branch for the same reason and gets the same treatment, reading the decision off the run it is retrying. Regression coverage asserts inheritance through both `/api/runs` and `/api/chat`, and that a source Run with no decision still yields none — absent and "took the ordinary route" are different facts, and a task that predates the strategy must not be back-filled into either arm. Reported in QA of #7445. * feat(web): say what the Design Harness switch does, not when it applies The line under the item explained timing to someone who had already decided. It now says what turning it on gets you — the latest OpenDesign generation strategy, better-looking output — and marks it as beta, which is the part that sets expectations for something shipped through Labs. * fix(web): three defects in the Labs switch found in QA **Double-counted opt-outs.** Turning the switch off opens the reason question; turning it back on left that question open, still asking about a switch that was now on. The stale question settled later as a skip, so a fumbled off/on reported two opt-outs against a single reason row. Re-enabling now retracts the question, restoring the invariant every count depends on: one reported opt-out, one reason row. **The saved confirmation never went away.** The dialog's autosave pill is a shared surface with no timer of its own — whoever sets it owns clearing it, and this switch only ever set it. The confirmation sat there for the rest of the session and followed the user into every other settings section. It now returns to idle after three seconds, and immediately if the user leaves the section first, since by then it describes an edit they can no longer see. **The disabled submit button read as blank.** The mock draws white-on-grey there, which against this palette lands at roughly 1.9:1 — the label was effectively invisible rather than merely dimmed. Mute the surface instead of the text so the word stays legible while the button is unavailable. All three verified against the running client, and each has a regression test. * style(web): sit autosave status on the chrome row and soften the settings card edge Two visual corrections to the Settings page. The autosave pill moves from the bottom of the viewport to the app's own top chrome row, centred on it. OPEND-2148 parked it at the bottom to get it off the Local CLI pickers; the chrome row clears them just as well and is the better home — on the full page that band is empty, and in the dialog it sits above the panel rather than over any of its content. What that ticket guarantees is unchanged: a viewport-level layer, out of the controls' band, and under the body-portaled model picker. Its spec is updated to pin that guarantee rather than the edge it hangs from. The content card's hairline steps down from `--border` to `--border-soft`. At that size the card is a large, quiet plane and the standard #dbdbdb rule read as a hard box drawn around it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Cheems <94773058+itscheems@users.noreply.github.com> Co-authored-by: wangchenglong <honam884844@gmail.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Gökhan KöseandGitHub
|
586469938e |
fix(daemon): add Fable and the Claude 5 ids to the model picker (#7401)
* fix(daemon): add Fable and the Claude 5 ids to the model picker The Claude runtime's fallback model list still ends at the 4.5 generation and has no entry for Fable at all, so the picker cannot offer a model the CLI has supported for a while. `claude --help` documents the alias explicitly. Adds the `fable` alias plus pinned `claude-fable-5`, `claude-opus-5` and `claude-sonnet-5`, and orders aliases before pinned ids since aliases stay correct across generations. The 4.5 ids are kept — they still serve. * fix(daemon): keep the existing fallback order, add fable purely additively The first revision moved the aliases ahead of the pinned ids, which broke the fallback-order contract that apps/daemon/tests/runtimes/mmd-routes.test.ts pins at lines 148, 183 and 270 — those compare the leading entries and expect `sonnet` as the first alias. Reordering was never the point of this change, so this restores the original order and inserts the new entries additively: `fable` after the existing aliases, and the pinned 5-generation ids in the same order PR #6535 uses so a merge of both stays trivial. No existing entry moves. |
||
|
Ray XiandGitHub
|
e13346decf | fix(daemon): stop AMR prompt after model rejection (#7458) | ||
|
lefarcenandGitHub
|
7f65c795d9 |
refactor: share one structural HTML injection-point locator (#7422)
* refactor: share one structural HTML injection-point locator #7410's root cause was two copies of the same lookup drifting apart: the srcDoc path anchored on the real `</body>` while the daemon copy still matched the first one in the text. This puts the locator in `packages/contracts` and points every injector at it — daemon preview bridges and containment base, web srcDoc and export paths, desktop PDF/artifact/deck capture, and the brand build. Rebuilt on `main` rather than rebased onto it. This branch was cut before #7421 and carried its 28 commits, so a rebase replayed them against a main that already had them and conflicted on the first one; replaying 17 commits through that would have been the easy way to silently drop one of the fixes. The net change is the same 26 files, and only four of them had also moved on main. The shared module is the daemon implementation as merged, not a reconstruction. Verified as equivalence before the daemon's copy was deleted: against `main`'s `apps/daemon/src/routes/project/index.ts` on identical inputs, 2,544 repository HTML files × 7 lookups and 15,000 generated adversarial documents × 6 lookups produce byte-identical answers — 0 divergences. The generated corpus is built from every hazard #7421's review rounds turned up, which is the comparison that would have caught the drift this PR exists to prevent. Three call sites finished their migration on the way: `daemonSanitizeTitleInDoc`, `assembleExample` and the collab-sync title read all located `<title>` structurally and then searched for its close as text, so `</title >` and `</title\n>` were missed and a `</title` inside the open tag's own attributes could match first. All three go through `findRealElementRange` now. `findRealElementRange` itself built its shadow with `toLowerCase()`, which is not length-preserving; it uses `asciiLower` like the rest of the module. `pnpm guard` gains an "HTML structural boundaries" check that fails any new hand-rolled boundary lookup outside the shared module — the safeguard #7421's rationale comment once claimed and did not have. Validation: 100 daemon route/containment specs (#7421's full set, now running against the shared locator), 45 contracts specs, 817 web runtime specs including the jsdom differential oracle, guard exit 0 with the new check reporting, daemon and contracts typecheck clean. * fix: finish the title migration in srcdoc and the prefetch reader Two from review, both the same leftover: the open tag was located structurally and then the close was searched for as text. I had claimed this migration finished in three call sites and it was five — a sweep of the repository for `indexOf('</…')` found exactly these two remaining, and nothing else (the other hits are `<artifact>` / `<PANELIST>` marker streams, which are not HTML). `sanitizeTitleInDoc` used `indexOf('</title>')`, which accepts one spelling. A document titled with `</title >` fell through to the next `</title>` in the source — here one inside an authored script string — and the rewrite spanned everything between, splicing into author content exactly as #7410 did. `readRealTitleText` used `indexOf('</title')` with no boundary check, so `</title-page>` ended the title. Both go through `findRealElementRange` now, and with it the `toLowerCase()` shadow both were building, which is not length-preserving. Two specs. The `</title >` one is red on the previous commit. The `</title-page>` one is green on both — `indexOf('</title>')` happened to skip it too, since it demands the full spelling — so it is regression cover rather than a red spec, and it is worth having because the new code reaches that case through a different rule. Writing them cost three wrong assertions before one held, all mine rather than the code's: the title text is rewritten by `sanitizePreviewTitle`, so the raw authored bytes are not observable in the output, and `b</title>` — my second attempt at something that would be — is a substring of the sanitized `a-title-page-b</title>`. The spec asserts the sanitized title itself now. Validation: 819 web runtime specs, 100 daemon route/containment specs, `pnpm guard` exit 0 with the structural-boundary check reporting, web and daemon typecheck clean. |
||
|
|
1c2e911634 |
fix(daemon): pass noninteractive permission bypass to Antigravity (#7316)
* fix(daemon): pass noninteractive permission bypass to Antigravity * test(daemon): harden Antigravity argv ordering --------- Co-authored-by: helson <helson@helsondeMac-mini.local> |
||
|
Ray XiandGitHub
|
b7465aa57a |
fix(amr): classify membership concurrency limits (#7429)
* fix(amr): classify membership concurrency limits * fix(daemon): scope membership concurrency to AMR Generated-By: looper 0.12.0 (runner=fixer, agent=codex) |
||
|
|
cd6920f963 |
feat(prompts): require real imagery for factual referents (#7436)
* feat(prompts): require real imagery for factual referents * fix(prompts): preserve content image aspect ratios * fix(prompts): resolve slim charter imagery rule conflicts - Move the factual-integrity carve-out into instruction-priority item 2 so it can actually win a conflict against a skill, instead of sitting after the list as charter prose at priority 4. - Qualify the Conduct copyright rule so it composes with the requirement to acquire real brand marks, covers, and artwork, closing the documented failure where the agent silently substitutes a look-alike. - Restore the charter byte budget at 29_696 (was deleted, previously 25_600) so future growth stays a deliberate, explained decision. --------- Co-authored-by: Mason6660201 <13073456345@163.com> Co-authored-by: bone3deep1962-collab <bone3deep1962@gmail.com> Co-authored-by: NJUHua <113895241+NJUHua@users.noreply.github.com> |
||
|
CaprikaandGitHub
|
3c05f82c79 |
fix(daemon): report terminal AMR Runs to Vela (#7424)
* feat(daemon): persist AMR terminal reports * feat(daemon): deliver AMR terminal reports through Vela * fix(daemon): address terminal reporting review gaps * fix(daemon): align terminal receipt validation * fix(daemon): retry terminal report enqueue * fix(daemon): isolate terminal delivery storage failures * fix(daemon): expose terminal reporter health counters |
||
|
lefarcenandGitHub
|
7e365acc4d |
fix(daemon): locate URL preview injection points structurally (#7421)
* fix(daemon): locate URL preview injection points structurally Three injectors on the URL-load preview path found their insertion point with a raw text match: the first `</body>` for the scroll / selection / snapshot bridges, the first `<head>` for the sandbox / focus / redirect / observability guards, and the first `<head>` again for the containment `<base>`. Those tags are also perfectly ordinary *content* — any prototype that builds an HTML document (a print window, an email template, a `srcdoc` payload) writes them into a script string or a `data-` attribute. So the injected markup landed inside the author's string, which ended their script early — or closed their attribute early, since the injection carries quotes — and rendered the remainder as page text, with no console error to explain it. All three now share `findRealTagOffset`, which walks tag by tag instead of character by character and skips the places a tag-looking run of text is not a tag: comments and markup declarations, raw-text element content (the full HTML5 set, `noscript` included since the preview iframe always runs with scripting enabled), attribute values, and `<template>` content, which the tree builder keeps out of the document so an injection placed there would silently never run. `endOfTag` gives the tag's real extent, so a `>` inside a quoted value cannot split it, and the `<head>` / `<html>` / `<base>` patterns carry a tag-name boundary so `<head[^>]*>` no longer matches `<header>`. This became reachable in 0.21.0: before #7336 an artifact needing the sandbox shim or focus guard was forced onto the srcDoc path, whose `injectBeforeBodyEnd` already anchored on the real `</body>`. Serving those guards over URL-load instead exposed the daemon copies, which never got the same treatment. Fixes #7410 * fix(daemon): require a real end-tag boundary to leave raw text Review catch on #7421. The raw-text skip resumed at the first `</name` substring, but the tokenizer only leaves raw text when that name is followed by whitespace, `/`, or `>`. A longer name starting with it is still character data, so: <script>const doc = "</script-template><body>slip</body>";</script> put the scan back into the author's string at `</script-template>`, and the very next `</body>` it handed back came from inside that string — recreating the corruption this branch exists to prevent. Measured against parse5: the scan returned offset 49, the parser says 77. `findRawTextClose` now walks candidates until one is followed by a real boundary character, and reports none when the element never closes, which keeps an unterminated end tag from closing it. The route fixture covers both a `<script>` prefix and an RCDATA (`<textarea>`) one. * fix(daemon): follow the tokenizer's template and script-escape states Review catch from @Siri-Ray. Two state gaps let the scan resume inside author content and hand back a boundary from there. Template depth was counted with a regex, so a `</template>` written inside a nested script string ended the template early: <template><script>const doc = "</template><body>slip</body>";</script></template><main>real</main></body> scanner 64, parse5 110 `skipTemplateContent` now walks the content with the same skip rules the main scan uses. This also corrects what the previous comment claimed: being fooled did *not* degrade to -1, it returned a wrong offset. Script data has escape states the other raw-text elements do not. After `<!--` a nested `<script` moves the tokenizer to double-escaped, where `</script>` steps back out instead of closing: <script><!-- const open = "<script>"; const doc = "</script><body>slip</body>"; //--></script> scanner 82, parse5 123 `findScriptClose` tracks those states; other raw-text elements keep the simple boundary walk. Route fixture covers both shapes. * fix(daemon): treat CDATA as markup only in foreign content Both reviewers landed on the same gap. Declarations were all skipped to the next `>`, but foreign content is the one place `<![CDATA[ … ]]>` is real markup, so that `>` can still be inside the section: <svg><![CDATA[label > </body>]]></svg><main>real</main></body> scanner 34, parse5 67 The scan resumed inside the CDATA and returned the `</body>` it holds, which put the bridge in the SVG as inert text and mutated the authored content — the failure this locator exists to prevent. `skipForeignContent` walks an `<svg>`/`<math>` subtree with the same skip rules and consumes `<![CDATA[` through `]]>`, tracking HTML integration points (`<foreignObject>`, `<desc>`, …) where the parser is back in HTML and `<![CDATA[` is a bogus comment again. The whole subtree is skipped either way: no document-level boundary can live inside one. Checked against parse5 on six shapes — CDATA in svg and in math, an integration point, nested svg, a self-closing `<svg/>`, and plain svg with no CDATA — all matching. Route fixture covers the first three. * fix(daemon): PLAINTEXT has no way out Found by sweeping the scanner against parse5 across tokenizer states rather than waiting for the next review round. `<plaintext>` switches the tokenizer to PLAINTEXT, and unlike every other raw-text element there is no exit: all input after it is character data, `</plaintext>` included. The scan honoured that close tag and resumed, so in `<body><plaintext></body></plaintext><main>x</main></body></html>` it handed back the trailing `</body>` (offset 56) where the parser reports none. The bridge was then spliced into inert text and never ran at all. Reporting no close is what keeps the scan out; the caller appends instead, where the bridge still runs. The same sweep found three more offset differences from parse5 that are *not* defects — `<!-->`, `<!--->`, and a `<![CDATA[` outside foreign content all place the injection correctly and leave the document unchanged, so the offset difference is the parser's own bookkeeping. One genuine limitation stands: a doctype with an internal subset (`<!DOCTYPE html [ … ]>`) puts the bridge in `<head>` rather than at the end of `<body>`. It still runs and nothing is corrupted, and matching it would take insertion-mode tracking this scanner deliberately does not do. * fix(daemon): refuse to guess a boundary through CDATA in foreign content Three review findings converge on the same construct, and the honest answer is to stop modelling rather than model harder. Whether `<![CDATA[` opens a section or is a bogus comment depends on the adjusted current node's namespace: an integration-point element is itself still SVG/MathML so CDATA directly inside it is a section, `annotation-xml` is an integration point only when its `encoding` is `text/html` or `application/xhtml+xml`, and foreign content nested in a `<template>` follows the same rules. Modelling that correctly is the HTML tree-construction algorithm, which this scanner deliberately is not. So it refuses: any `<![CDATA[` inside `<svg>`/`<math>` ends the scan with "no boundary", the caller appends, and the injection still runs with every authored byte intact. Guessing is the only outcome that corrupts, which makes it the one worth giving up precision for. `skipTemplateContent` now routes foreign content through the same skip so a template cannot slip past it. Measured before choosing: across the 2544 HTML files in this repo, `<svg>` appears in 178 and `<![CDATA[` in **none**. Precise placement is kept for every one of those 178; the conservative path is reserved for a construct the product's own content never produces. Route fixture now carries all three reviewed shapes — CDATA in a `<foreignObject>`, in a template's `<svg>`, and in an unencoded `annotation-xml` — asserting each section survives byte for byte and the bridge lands after the document. * fix(daemon): keep double-escaped script state across `-->` Review catch. `-->` returns *escaped* script data to normal script data, but while double-escaped the tokenizer stays where it is. Clearing both flags there accepted the next `</script>` as the element close: <script><!--<script>--></script><body>slip</body></script><main>real</main></body> scanner 42 (the `</body>` still inside the script), parse5 75 Checked against parse5 on all three transitions — double-escaped across `-->`, plain escaped across `-->`, and double-escaped leaving through its own `</script>` — all matching. Route fixture carries the first. * fix(daemon): close the token-shape gaps an audit found, and revert |
||
|
Ray XiandGitHub
|
0dc31fef63 |
fix(daemon): silence ACP process trees before terminal (#7432)
* fix(daemon): silence ACP process trees before terminal * fix(daemon): preserve child fallback during tree teardown Keep PID-less child handles on the direct signal path while retaining process-group and platform PID-tree quiescence for real agent processes. Preserve ACP abort grace before closing stdin and escalating signals. Generated-By: looper 0.12.0 (runner=fixer, agent=codex) * fix(daemon): verify no-pgid teardown snapshots Refresh and merge the owned process tree after ACP abort grace, verify quiescence after escalation, and surface failed enumeration as termination_failed evidence. Cover late descendants and unverifiable enumeration in focused run-service regressions. Generated-By: looper 0.12.0 (runner=fixer, agent=codex) |
||
|
open-design-release-bot[bot]GitHubopen-design-release-bot[bot] <open-design-release-bot[bot]@users.noreply.github.com> |
d88941ba0e |
chore(release): bump main to 0.21.1 ahead of stable 0.21.0 (#7407)
Co-authored-by: open-design-release-bot[bot] <open-design-release-bot[bot]@users.noreply.github.com> |
||
|
netteeandGitHub
|
ebb5b24c07 |
feat(web): show targeted Go Plan sunset notice (#7430)
* feat(web): show targeted Go Plan sunset notice * style(web): match final Go sunset modal * chore: remove temporary design QA notes * fix(web): keep sunset notice actions reachable on short viewports Preserve the announcement copy panel's scroll container on mobile and allow the bounded grid item to shrink. Add responsive CSS coverage for narrow and short landscape dialogs. Generated-By: looper 0.11.8 (runner=fixer, agent=codex) |
||
|
lefarcenandGitHub
|
53e78ac16f |
perf(collab): make background materialization volume observable (#7433)
The cumulative ceiling for background shared-project pulls (`OD_COLLAB_BACKGROUND_PULL_MAX_CUMULATIVE_ENTRIES`) shipped disabled in #7403, because choosing that number is a capacity decision and there was no production data on real team sizes to choose it from. Getting that data turns out to be blocked on a one-line self-lock in the same file: if (cumulativeLimit > 0) cumulativeEntries += inspection.entryCount; The counter only ran once the ceiling was already set. Every default-configured client measured nothing, so the data needed to set the ceiling could not exist until the ceiling was set. Counting always — and enforcing only when a ceiling is present — is what breaks that loop. The accumulation changes no decision while the ceiling is unset: the budget branch is already guarded by `cumulativeLimit > 0`. Reporting it needs a sink, which is the other half of the same mistake. The catch-up completion diagnostic ends with `candidates=`, which counts projects CONSIDERED — a sweep logging `candidates=25` says nothing about whether 25 files or 25,000 landed on that member's disk. `volume()` now feeds three `process*` totals into that line, so a diagnostics bundle from a real workspace answers the capacity question directly, instead of it needing a synthetic several-hundred-project team to reproduce. `uncountedProjects` is tracked apart from `entries` on purpose. An old server returns no manifest count and the guard fails open, so those pulls are real but uncounted; folded together, a fleet that counts nothing would be indistinguishable from a fleet that pulls nothing, and reading the first as the second would set the ceiling far below what teams actually need. No behaviour change: no decision, threshold or default moves. This is measurement only, and the ceiling stays unset until the numbers justify one. Tests: four specs verified red against the unfixed source (`entries: 0` vs `24`/`12`, not merely a missing API), one of them driving the real `createProactiveContentPull` lane so that a future change which stops consulting the guard fails here. `workspace-projects-hub-wiring` gains the sink assertions; its existing catch-up-line check is matched without the closing backtick so appending a field no longer breaks it. `@open-design/daemon`: 9,533 passed. The one remaining failure in `od-next-automatic-simple-server.test.ts` is a pre-existing 20s-plus timeout in that file — it fails on `main` too, with a different test each run. `pnpm guard` and `pnpm typecheck` clean. |
||
|
mehmet turacandGitHub
|
2642b50a8a |
fix(classification): empty_output close reason must not outrank RATE_LIMITED or upstream signals (#7248)
* fix(classification): prevent empty_output from being misclassified as hard_quota BYOK OpenCode runs that end with rpc_close_reason=empty_output were incorrectly classified as non-retryable hard_quota/rate_limit because the generic empty-output fallback message emitted by the daemon contained the advisory phrase "checking quota", which matched the loose \bquota\b pattern in isHardQuotaText(). Changes: - Move readRuntimeCloseReason() call earlier in classifyRunFailureBase so it is computed once and reused across both the new empty_output guard and the existing fatal_rpc_error promotion. - Add an early return before the quota/rate-limit heuristics: when rpc_close_reason=empty_output is present, classify the run as empty_output (retryable) without consulting text patterns. - Tighten isHardQuotaText by removing the standalone \bquota\b term and replacing it with the specific exhaustion phrase "quota exceeded". Advisory occurrences of "quota" (e.g. "checking quota") no longer match. - Remove "checking quota, or" from the daemon's generic empty-output fallback message so future text-matching is not confused either. - Add three regression tests: advisory "checking quota" text with rpc_close_reason=empty_output must not be hard_quota; clean empty_output close reason must classify correctly; genuine quota-exhaustion text still classifies as hard_quota. Fixes #7214 * fix(classification): empty_output close reason must not outrank RATE_LIMITED or upstream signals The `rpc_close_reason=empty_output` early return was inserted before the RATE_LIMITED and UPSTREAM_UNAVAILABLE branches, so a run whose provider returned a hard-quota rejection — and whose child then exited cleanly, causing the daemon to stamp `empty_output` — was misclassified as a retryable `empty_output` instead of a non-retryable `hard_quota`. Move the `rpc_close_reason=empty_output` guard to after both the RATE_LIMITED/isHardQuotaText/isRateLimitText and UPSTREAM_UNAVAILABLE branches so structured error codes always win over the close-reason signal. Also pin that dropping bare `\bquota\b` from isHardQuotaText correctly preserves real exhaustion phrases ("exceeded your current quota") while still preventing advisory phrases ("checking quota") from matching. Regression tests added for: - RATE_LIMITED code + empty_output close reason → rate_limit (not empty_output) - Hard quota text + empty_output close reason → hard_quota (not empty_output) - UPSTREAM_UNAVAILABLE code + empty_output close reason → upstream_unavailable - "exceeded your current quota" phrase without bare \bquota\b → hard_quota * fix(classification): add quota reached and RESOURCE_EXHAUSTED to hard quota matcher Antigravity's upstream log emits: RESOURCE_EXHAUSTED (code 429): Individual quota reached. Neither phrase was in isHardQuotaText after the bare \bquota\b term was removed in the previous commit. Add `quota reached` (covers the full log line) and `RESOURCE_EXHAUSTED` (covers the status code alone when the message portion is stripped) to the exhaustion alternation. Advisory phrases from antigravityQuotaGuidance — "has its own quota", "available quota" — do not contain either new token and continue to not match. Regression tests added for the full Antigravity log line, the bare phrase, the bare status code, and the two advisory non-matching cases. Refs #7214 |
||
|
lefarcenandGitHub
|
2856f87e8c |
feat(collab): add an opt-in cumulative ceiling for background content pulls (#7403)
* feat(collab): add an opt-in cumulative ceiling for background content pulls `background-pull-size-guard` (incident #6512) asks one question per project: is THIS version too big to push onto every member's disk. It cannot see accumulation, and nothing else in the path can either. Measured on a live Team workspace — 12 shared projects of ~2.9 MB each, 12 files apiece, every one an order of magnitude below the 2000-entry threshold. A member who did nothing but open the client pulled 23 MB across 8 projects within 50s, and the sweep log shows why: catch-up completed ... scanned=25 candidates=25 headChecks=4 suppressed=0 Every project cleared the gate individually. `headChecks=4` is a per-round batch size, not a ceiling — later rounds keep going. So member onboarding cost grows linearly with team size, with no bound. This adds the missing second dimension: a per-process budget of entries the BACKGROUND lanes may materialize before deferring the rest. Deferring is not denying — the foreground open-project lane never consults this guard, so anything skipped still materializes the moment the user opens it. Deliberately inert by default. `OD_COLLAB_BACKGROUND_PULL_MAX_CUMULATIVE_ENTRIES` defaults to 0 (disabled) and a malformed value disables rather than guesses, mirroring how `maxEntries` refuses to be silently misconfigured. Picking a real ceiling is a capacity decision that needs production data on team sizes and pre-pull hit rate; shipping a guessed default would silently change what every existing client downloads. Budget is only consumed by decisions this guard actually allowed, and an exhausted budget is NOT recorded in `deferredVersions`: that map means "this version is permanently too big", whereas the budget is a transient per-process limit that a restart resets. Verified by unit spec, NOT end to end. The local vela (0.0.27-test.3) has no `team-projects pull --authorize-only` flag, so `probeUnavailable` latches on first use and every size decision short-circuits to 'pull' — see the follow-up note on the PR, since that also disables the pre-existing #6512 protection on any client shipping a CLI that old. * fix(collab): say which ceiling deferred a background pull Verifying the cumulative budget end to end (vela-cli 0.0.33, live Team workspace) produced this line: background shared-project pull deferred (oversized): projectId=many204328n11 version=2 entries=12 maxEntries=2000 Which reads as a contradiction — 12 entries against a 2000 threshold is not oversized. The project was deferred because the session budget was spent, but both ceilings logged the same word. The two need different responses: one says raise the per-project threshold, the other says raise the session budget. Carry the reason on the observation so the log points at the knob that actually applied. * fix(collab): decide the per-project ceiling first and remember budget deferrals Two review findings on #7403, both correct. **Order.** The cumulative ceiling was evaluated before the per-project one, so a project that independently exceeds `maxEntries` was labelled `budget-exhausted` whenever the remaining budget happened to be smaller than it. That points a reader at the wrong knob, and worse, it never reached `deferredVersions` — so an oversized project was re-probed on every later round. Exceeding `maxEntries` is a permanent property of the version and is now decided, and cached, first. **Memoization.** A deferral is terminal only for the current work item; later catch-up rounds revisit a head that is still unmaterialized. The budget branch recorded nothing, so every round repeated the remote authorize-only probe and emitted another deferral log for every project past the budget — unbounded remote calls for a decision that cannot change until the process restarts. My comment justified leaving it uncached by worrying about a restart still suppressing the project. That reasoning does not hold: every map in this guard is already process-local, so a restart clears the budget and these entries together. Budget deferrals are now remembered in their own map, kept separate from `deferredVersions` so "too big" and "arrived after the budget was spent" stay distinguishable. Verified live (vela-cli 0.0.33, budget=30): unchanged capping behaviour — 2 projects / 5.8 MB versus 8 / 24 MB unbudgeted — and the deferral log now stays at 5 lines across 100s instead of growing every sweep round, one per project. |
||
|
lefarcenandGitHub
|
d5aa100291 |
perf(collab): serve the team-project display read from its own cache (#7398)
* fix(collab): say a team project is downloading during its first open A member's first open of a team-shared project spends its longest stretch on the project route BEFORE any local project row exists: `PUT /collab/bootstrap` has to discover the shared owner and start the content pull first. ProjectView owns every download indicator we have — the design-files tab badge, the "syncing" empty state, the composer placeholder — but it is deliberately not mounted around the synthetic "Untitled" placeholder, so none of them can run during that window. The route therefore fell through to `resolving-deep-link` (or `loading-projects`) and rendered the shell's generic "Loading workspace…" spinner, which is indistinguishable from a wedged app. Reported from dogfooding as "no download status on first open, looks stuck" (OPEND-2095). Name that lane instead. `projectRouteSurfaceState` gains a `firstOpenTeamMaterializing` input, set only after a verified active Team context named the project — never inferred from a missing row, which is also how an unknown id looks — and refines ONLY the two loader outcomes, so every terminal outcome (dead daemon, bounded deep-link failure, resolved project) still decides exactly as before and a stuck flag cannot outlive them. The new surface reuses the same sentence ProjectView shows once it mounts, so the hand-off reads as one continuous download instead of two unrelated waits. It stays indeterminate on purpose: `vela resource pull` is awaited as a whole and reports no byte progress, so a percentage here would be invented rather than measured. * fix(collab): key the first-open indicator on the run, not the project id Review finding on #7350: the deep-link effect that owns this indicator also depends on `projects`, `projectsLoading` and `daemonLive`. Any of those changing while `bootstrapFirstOpenTeamProjectRoute` is still pending cancels that run and starts a fresh one FOR THE SAME ROUTE, which re-claims the same project id. Keyed on the id alone, the cancelled run's `finally` then retracted the LIVE run's claim while its bootstrap was still downloading — putting the anonymous "Loading workspace…" spinner back during exactly the slow path this indicator exists to explain. Give each effect run a monotonic id and let a run retract only its OWN claim. `releaseFirstOpenTeamMaterializationClaim` is exported so that rule is unit tested without mounting App, matching how `projectRouteSurfaceState` already makes the route's other decisions testable. * fix(collab): claim the first-open download only for a named project Second review finding on #7350: `firstOpenTeamContext` is `exactOpenContext ?? deepLinkContext`, and on the ambient path that is just the shell's Workspace context. It proves the caller is in an active Team; it does NOT name or authorize the requested project id. This lane is entered precisely because the local scope lookup returned not-found, so a missing, revoked, or unauthorized project URL was shown "Syncing files from the team…" right up until it became "project missing" — presenting an unverified download as fact, and changing a failure surface this PR states it leaves untouched. Gate the claim on positive evidence that something named THIS project: - a project-scoped opening witness (`exactOpenContext`) warrants it at once; - the ambient path waits for `PUT /collab/bootstrap` to answer 2xx, which is the hub confirming the project exists and is shared to this Team. `bootstrapFirstOpenTeamProjectRoute` gains an `onSharedProjectConfirmed` callback for that moment, and `firstOpenDownloadClaimIsWarranted` states the rule as a pure predicate so the unknown-project-id case is unit tested rather than argued. * fix(collab): stop reporting cancelled team pulls as failures Revert the first-open download indicator and keep the one verified fix the investigation produced. The indicator is gone because it was measured, repeatedly, not to work. Against a live vela Team workspace with two distinct accounts and two isolated daemons it never rendered once — empty project or 3 MB one, warm data root or cold, even racing the share. `proactiveContentPull` materializes the content before the user can open it and wins that race every time, so the window the indicator was built for does not exist. Its unit specs were green throughout, which is exactly why they are not kept: they would have locked in coverage for a surface nobody can reach. What the investigation did find is that a deliberately cancelled `vela` child was reported as a failure. `runVelaCommand` marks an abort with `name: 'AbortError'` / `code: 'ABORT_ERR'` and keeps a separate `timeout` termination, but nothing downstream read those markers. The proactive pull scheduler aborts as ordinary control flow — `mergeIntentUpdate` when a higher published version supersedes the fetch, `clearIntent` when an intent is superseded or satisfied — so routine version churn logged `authorized proactive team pull failed closed`, and sent at least one investigation chasing a fault that never happened. `isAbortedOperationError` now draws that line at both sinks: the `failed closed` warning in collab-sync, and the single `onError` consumer that backs proactive-content-pull's three raise sites (that module is deliberately import-free, so the distinction belongs at its consumer). Behaviour is unchanged; this is log correctness only. An aborted intent already bumped `revision` and re-looped, or was removed outright, so it never reached the retry/backoff accounting. The predicate lives in a dependency-free module on purpose. Importing it from `vela-command.ts` pulled `runtimes/env` + `runtimes/registry` into the route module graph, where `collab-sync-routes.test.ts` mocks `integrations/vela.js` with a single export — turning a passing pull into `register_failed`. That regression was caught only by running the neighbouring suite. * perf(collab): serve the team-project display read from its own cache `GET /api/workspace/projects/team` is the display read: the Home team-project grid and the deep-link "is this project shared to my team?" check both go through it. Measured against a live vela Team workspace it cost ~1.1s on EVERY call, cold and warm alike, because each one spawned `vela team-projects list` and waited on a round trip to the API. server.ts already builds `teamProjectsDisplayCache` for precisely this, and its doc comment names the route it was written for: Short-TTL, single-flight cache for the read-only DISPLAY path (GET /api/workspace/projects/team) ... Deliberately NOT used by resolveSharedProject below: the pull gate and comment/presence relays must observe an unshare immediately, so those use the uncached exact lookup. The route was wired to the uncached lister anyway, so the cache built for it went unused by it while the low-frequency invalidation poller got the cached one. This swaps them back to what that comment describes. `listTeamProjects` has exactly one consumer inside `registerCollabContextRoutes`, so the change is contained to that read. Measured on the same live workspace after the change: before 1094ms, every call after 3746ms first (daemon cold, includes the CLI capability probe) 49ms / 57ms / 52ms / 64ms thereafter Freshness was verified rather than assumed, since immediate unshare visibility is the whole reason the uncached path exists: sharing a new project from the owner client and polling the member's catalog, the project appeared within 1-2s (12 -> 13 entries) — inside the 3s TTL, and share/unshare/workspace-change all invalidate the cache explicitly on top of that. The accompanying spec pins what the display path needs from the primitive: repeat reads inside the window cost one upstream call, an explicit invalidation forces a re-read so an unshare is never hidden, and entries never cross scopes. * fix(collab): drop the helper this swap orphaned, and correct its comment Self-review after merging main, applying what #7408's review rounds taught about comments that claim more than they check. The comment said the uncached lookup "stays where its doc says it belongs: the pull gate and comment/presence relays". That is false: this registration was `teamProjectsForRequest`'s only caller anywhere in the daemon, so the swap left it with zero references. The pull gate and the relays reach `teamProjectsLister` directly and are unaffected — the claim was right about them and wrong about the wrapper. Remove the orphaned wrapper rather than leave dead code behind a swap, and say what the invalidation actually rests on: `invalidateTeamProjectCatalog` from collab-sync and the project routes, plus the per-scope invalidations beside the cache, not the 3s TTL alone. Verified while checking, since the swap is only safe if these hold: - the display cache is keyed on `[workspaceId, workspaceMemberId]`, so one client cannot be served another's list; - the swapped registration is `registerCollabContextRoutes` — display reads, not a data-access gate; - `teamProjectsLister` and `withoutLocallyUnsharedProjects` keep other callers, so removing the wrapper orphans nothing further. * fix(collab): invalidate the team catalog on a sync-intent share/unshare Review catch, and a regression this PR introduces rather than an old gap: before the swap this read was uncached, so no caller had to announce a mutation. Moving `/api/workspace/projects/team` onto the display cache makes invalidation part of the mutation contract. The `/api/workspaces/:id/projects` move path already calls `invalidateTeamProjectCatalog`. The sync-intent route — the one `od project share` and CollabDemoView drive — did not, so a catalog GET issued right after sharing could answer with the pre-share list until the freshness window expired or a hub event happened to arrive. That also made the comment added next to the swap false, since it claimed share and unshare invalidate. Fixed at the seam rather than at the two call sites. `onTeamShareStateChanged` is announced from exactly the share and unshare handlers, so `announceTeamShareStateChange` now fires it together with the catalog invalidation: persisting the visibility record and dropping the catalog belong to the same event, and pairing them means a mutation added to this module later cannot pick up one and forget the other. (#7408's review found precisely that failure — a tenth mutating export missed because the others were enumerated by hand.) Red spec is route-level, as asked: share through sync-intent, then unshare, and assert the injected `invalidateTeamProjectCatalog` fires each time. It fired zero times before this change. |
||
|
|
aa424df27d |
feat(analytics): attribute project editing events (#7335)
* feat(analytics): add project context to edit events * fix(analytics): make edit attribution consistent * fix(analytics): preserve project edit attribution --------- Co-authored-by: free666799 <293857035+free666799@users.noreply.github.com> |
||
|
lefarcenandGitHub
|
1246ca3d67 |
test(daemon): stop the telemetry-sink spec reading a real AMR login (#7414)
`describes relay, direct, and disabled sinks through the same allowlist` fails on any developer machine that has signed into AMR, and passes on CI. It is not flaky and it is not a broken build — the spec simply is not hermetic. Its langfuse case passes only `LANGFUSE_*` keys. With no `OPEN_DESIGN_VELA_TELEMETRY: 'off'`, the vela branch of `readRunTelemetrySinkConfig` is enabled by default, and `readVelaControlApiContext` falls through `readVelaProfileConfigSnapshot` to the real `~/.amr/config.json` on disk. Where that file exists with a control key, the sink resolves to `kind: 'vela'` and the assertion fails; a CI runner has no such file, so the branch is never taken there. The relay case immediately above it already passes that opt-out. Add the same one to the langfuse case: the spec is about the allowlist's shape, not about whichever machine happens to be running it. Found while triaging the daemon suite for an unrelated PR, where it was the only persistent failure and cost real time to attribute — worth removing rather than re-explaining on every branch. |
||
|
lefarcenandGitHub
|
9cac9d6fc7 |
fix(collab): stop reporting cancelled team pulls as failures (#7405)
Revert the first-open download indicator and keep the one verified fix the investigation produced. The indicator is gone because it was measured, repeatedly, not to work. Against a live vela Team workspace with two distinct accounts and two isolated daemons it never rendered once — empty project or 3 MB one, warm data root or cold, even racing the share. `proactiveContentPull` materializes the content before the user can open it and wins that race every time, so the window the indicator was built for does not exist. Its unit specs were green throughout, which is exactly why they are not kept: they would have locked in coverage for a surface nobody can reach. What the investigation did find is that a deliberately cancelled `vela` child was reported as a failure. `runVelaCommand` marks an abort with `name: 'AbortError'` / `code: 'ABORT_ERR'` and keeps a separate `timeout` termination, but nothing downstream read those markers. The proactive pull scheduler aborts as ordinary control flow — `mergeIntentUpdate` when a higher published version supersedes the fetch, `clearIntent` when an intent is superseded or satisfied — so routine version churn logged `authorized proactive team pull failed closed`, and sent at least one investigation chasing a fault that never happened. `isAbortedOperationError` now draws that line at both sinks: the `failed closed` warning in collab-sync, and the single `onError` consumer that backs proactive-content-pull's three raise sites (that module is deliberately import-free, so the distinction belongs at its consumer). Behaviour is unchanged; this is log correctness only. An aborted intent already bumped `revision` and re-looped, or was removed outright, so it never reached the retry/backoff accounting. The predicate lives in a dependency-free module on purpose. Importing it from `vela-command.ts` pulled `runtimes/env` + `runtimes/registry` into the route module graph, where `collab-sync-routes.test.ts` mocks `integrations/vela.js` with a single export — turning a passing pull into `register_failed`. That regression was caught only by running the neighbouring suite. |
||
|
|
0944e176ce |
feat(daemon): make OD Next opt-in through an app-config switch (#7362)
* feat(daemon): make OD Next opt-in through an app-config switch OD Next shipped switched on: `readOdNextRolloutPolicy` read `active` when `OD_NEXT_STRATEGY_ROLLOUT` was unset, so merging the strategy changed how every design run behaved on every install. The only way back to the ordinary route was an environment variable, which `PACKAGED_CHILD_ENV_ALLOWLIST` strips before the packaged daemon ever sees it — a packaged user had no way to choose either direction. Turn the switch around. An installation that configured nothing runs `off` and takes the ordinary route, and `odNextStrategyMode` in app config is how it opts in. The run route reads that field per request, so configuring it takes effect on the next run rather than the next daemon restart, and it persists across restarts and upgrades because it lives in app-config.json rather than in one process's environment. `OD_NEXT_STRATEGY_ROLLOUT` still outranks the saved mode, so a pinned process stays pinned: an operator debugging one daemon, a packaged smoke run, and a test each set the mode for one process without overwriting what a user saved, and without a user's saved choice overriding theirs. `od strategy rollout status` now reports which authority decided (env / app_config / default), so whoever just configured the mode can confirm theirs is the one in effect instead of inferring it from the resulting mode. * test(daemon): opt the failure-telemetry smoke into OD Next Its `task_hierarchy` cases assert how an ADMITTED Run reports — one `strategy-task:` trace per task execution with a `strategy-stage:` span per Run inside it. That representation only exists for a Run OD Next owns, so with the strategy now opt-in these three cases waited 3s for an observation the daemon had no reason to emit and timed out. Opt in through the same `putConfig` the cases already use. The `single_run` case keeps its representation on top of the same opt-in, because deepseek sits outside the capability gate — which is the contrast the table exists to draw, and it now draws it without depending on an install-wide default. * fix(daemon): refuse an odNextStrategyMode write that is not a mode Review catch (#7362): `doWrite` copies the existing config before applying a partial, so the sanitizer's `delete` did not "leave the previous choice alone" — it removed an already-saved `active`. `od config set odNextStrategyMode acive` therefore switched the installation off, printed success, and left the person who typed it no way to tell that from an opt-out they never asked for. Dropping a bad value is the right trade for a preference, where the cost is one setting falling back to its default. It is the wrong trade for a switch that decides whether OD Next runs at all, because there "dropped" and "deliberately off" are the same state. So a write that names the key with anything other than a mode (or `null`, which stays the deliberate way to opt back out) is refused whole: 400 from `PUT /api/app-config`, non-zero exit from `od config set`, nothing written — neighbouring keys in the same body included. The read path stays fail-soft: a hand-edited or truncated file drops the value and reads as unconfigured, which is `off`, rather than taking the daemon down. The 400 carries the nested error envelope. `od` only finds a code in that shape; from a flat body it fell back to `daemon-not-running` and exited 64, telling a caller to go start a daemon that had just answered. * fix(daemon): stop reading an unreadable app config as an unconfigured one Review catch (#7362), and the same defect class as the write-side fix one commit earlier: an error was being made indistinguishable from a deliberate choice. `readAppConfig` already answers `{}` for both states that legitimately mean "nothing configured" — no file, unparseable file — and throws only when the daemon genuinely cannot read its own config. The `.catch(() => ({}))` on the rollout reads flattened that throw into the same answer, so an installation whose saved `active` became unreadable silently took the ordinary route, and `od strategy rollout status` reported `requestedMode: off` / `requestedModeSource: default` — a claim about the operator's choice made from a fact about the disk. The run route and the status route now let the read fail. Express 5 forwards the rejection, so the operator sees an error instead of a confident wrong answer, and the run fails rather than quietly running unconfigured (it would have had no `agentCliEnv` either). The safety latch is the one place that must not depend on the read, so it takes a thunk now: the latch lands first, and a failed read skips the control-change event rather than reporting a mode the installation never chose. Also gives the opt-in acceptance case an explicit 60s timeout. It is the only case in that file driving two full runs against a real server, and the suite default of 20s leaves it no headroom on a slow runner. --------- Co-authored-by: Cheems <94773058+itscheems@users.noreply.github.com> |
||
|
Ray XiandGitHub
|
a8b571ccd5 |
feat(amr): load Coding Plan models dynamically (#7289)
* feat(amr): load coding plan models from Vela * test(web): mark wallet gate fixtures authoritative Generated-By: looper 0.11.8 (runner=fixer, agent=codex) * fix(web): keep personal plan badges out of team workspaces Generated-By: looper 0.11.9 (runner=fixer, agent=codex) |
||
|
lefarcenandGitHub
|
9810fc08fe |
fix(daemon): stamp first_visible_output when output is emitted, not decoded (#7364)
* fix(daemon): stamp first_visible_output when output is emitted, not decoded `time_to_first_visible_output_ms` has been a byte-for-byte copy of `time_to_first_token_ms` since it shipped: `noteFirstTokenAt()` marked both `first_token` and `first_visible_output` from one call with one timestamp, and marks are first-write-wins, so the real mark inside `send()` could never fire. Across 205,795 `run_finished` events in the last 7 days the difference was 0 for every single run — p50, p90, p99 and max all 0. The daemon does not emit every token it decodes. The `<od-title>` marker stripper, the fabricated-role-marker guard (#3247) and close-time stdout buffering can all hold bytes back after a token has been recognised, and that withheld window is exactly what this field exists to measure. `send()` is now the sole owner of the mark: it is the last thing every stream handler passes through, so a mark there means the bytes really left the daemon. `stdout` now counts as visible output too — the plain / BYOK / antigravity family has no structured `agent` stream, and antigravity buffers stdout until close, so its first token and its first visible byte can be a whole run apart. No user-visible change. `first_token` keeps its current meaning, the `?? firstTokenAt` fallback still covers runs that never emit anything, and `last_observed_phase` keeps the same labels — a later `first_visible_output` can only move a run from `tool_execution` to `stream_output`, which is the more accurate answer. * fix(daemon): read the decode clock before emitting the first text delta Splitting `first_visible_output` off `first_token` exposed an ordering flaw the shared timestamp had been hiding. On the qoder / json-event- stream / pi-rpc, Copilot and ACP paths the gate reads if (emitTitleFilteredGuardedTextDelta(delta)) noteFirstTokenAt(); so the emit — which fans the delta out to every SSE client and stamps `first_visible_output` on the way — completes BEFORE the clock is read for the token. Two consequences: `time_to_first_token_ms` silently included our own write latency, and on a loaded box the visible-output mark could land a millisecond AHEAD of the token that produced it, making the gap negative. Capture the decode timestamp before the gate and hand it to `noteFirstTokenAt`. The Claude and plain-stdout paths already stamped the token first and are unchanged. With all five sites token-first, `firstVisibleOutputAt >= firstTokenAt` now holds by construction rather than by both marks sharing one call, and the straight-through spec asserts it. * fix(daemon): mark visible output on the strategy close-time tail The OD Next machine protocol withholds any text that might still complete a reserved `<open-design-...>` block. `finish()` is what finally rules that out, at child close, and that release is persisted and broadcast directly instead of going through `send()`. So a reply whose visible text was withheld in its entirety recorded no visible output at all, and the analytics fallback collapsed a real close-time wait back to `firstTokenAt` -- reproducing the exact dead-field symptom this change exists to end. Both emission paths now apply one shared visible-output rule; `first_model_event` stays on the live path only, so a withheld reply cannot drag the phase anchor to run end. Telemetry also stops being able to break the product. Every lifecycle mark in this file rides the path the model's bytes travel on, so a throw from instrumentation cost the user the whole reply rather than one analytics field: a red test that injects a throwing tracer failed the run outright before this. The marks now run inside a failure-isolated helper scoped to the marks alone, logging once per run instead of swallowing silently. The regression suite no longer inherits the OD Next rollout default -- that default has been `active`, is expected to flip after 0.21.0, and is a user-facing switch besides. Each case names the mode it runs under and the strategy case asserts admission actually happened. `run_finished` delivery is awaited through the daemon's own analytics flush instead of an arbitrary 15-second cutoff, while still failing when the event never arrives. |
||
|
netteeandGitHub
|
a8518fb861 | fix(daemon): isolate task observation manifest env (#7373) | ||
|
lefarcenandGitHub
|
204ee7a06d |
fix(daemon): explain an ACP handshake rejection instead of retrying it (#7303)
* fix(daemon): explain an ACP handshake rejection instead of retrying it
An agent CLI that answers `initialize` and then refuses `session/new` /
`session/load` surfaced as a bare `json-rpc id 2: Internal error`, and the
retry policy treated it as transient — so the run failed, retried, and failed
identically while the user read a JSON-RPC frame.
Classify a handshake-numbered protocol error as `session_init` /
`install_cli`, keep every later protocol error on `child_close`, and lead the
user-facing message with the CLI and its detected version. The raw line stays
appended because `run.error` is also the classifier's input.
* fix(daemon): route the ACP handshake guidance onto the path users hit
The guidance copy was only reachable through `rewriteKnownAgentStreamError`,
which the ACP failure path never calls. An ACP agent's error goes
`attachAcpSession` -> `fail()` -> the bridge's `send('error', payload)`, and
that callback forwarded the payload verbatim; the close handler then returned
on `acpFatalErrorObservedBeforeCancellation && hasFatalError()` before the
stderr-tail rewrite further down. So a Kimi user still read
`json-rpc id 2: Internal error`.
Rewrite the payload at the bridge instead. That one object is the whole
user-visible surface of an ACP failure: `send` streams it to SSE clients and
`design.runs.emit` reads `run.error` out of it, so correcting it there fixes
both surfaces at once. `withAcpHandshakeFailureGuidance` rewrites only the
message fields, and only for a handshake-numbered JSON-RPC error, so
`AMR_MODEL_UNAVAILABLE`, promoted opencode errors, and post-session protocol
errors keep the shape their own handling depends on. The raw line stays
embedded because `run.error` is also the classifier's input.
Cover it through the full server run cycle with a fake ACP CLI that answers
`initialize` and rejects `session/new`, asserting on what a client and the
telemetry pipeline actually observe. Against the pre-fix tree the run error is
the bare `json-rpc id 2: Internal error`; against the pre-PR tree the daemon
also opened `session/new` twice.
* fix(daemon): read the handshake failure's cause, not just its id
Running a real Kimi CLI while signed out produces `json-rpc id 2:
Authentication required`. The handshake reading keyed on the JSON-RPC id
alone, so that user was told their CLI version was incompatible and they
should update or reinstall it — a prescription for a problem they did not
have, in place of the one sentence that pointed at the fix.
The id answers *when* a failure happened; it can never answer *why*. Split
the two questions: `isAcpHandshakeRpcErrorText` stays structural, and
`isAcpCliSessionRefusalText` is what the copy and the `install_cli` verdict
now hang off — handshake numbering AND no cause with a remedy of its own.
The "has its own remedy" half reuses `classifyAgentServiceFailure` rather
than growing a second signature list: it is agent-agnostic, already covers
auth / quota / upstream, and carries its own suite. It is deliberately
unrelated to, and narrower than, `isCliNotInstalledText` — that predicate
answers "was the binary even there", which a CLI that already answered
`initialize` has plainly settled.
A handshake failure that names its cause now keeps its own text and falls
through to the branch that owns that cause, so telemetry and copy agree: an
unauthenticated handshake is `auth` / `login`, a throttled one is
`rate_limit`. A bare refusal (`Internal error`, `Method not found`) is
unchanged and still gets the CLI guidance and the retry suppression.
No pure-function test could have caught this: the helpers were consistent
with themselves. The wiring test drives a fake ACP CLI that rejects
`session/new` with `Authentication required` and asserts on the text a
signed-out user actually reads.
* test(daemon): pin the category each named handshake cause is filed under
Verifies the routing claimed in the PR body rather than asserting it. Caught
one: `insufficient balance` has its own `insufficient_balance` category, not
`rate_limit`.
* fix(chat): name the ACP session refusal instead of writing its sentence
The guidance this PR added reached the user as a paragraph the daemon
composed: "The Kimi CLI (0.38.0) accepted the connection but refused to start
a session. … Details: json-rpc id 2: Internal error". That is the wrong layer,
and it showed:
- The daemon has no locale. The string went straight into `run.error` and
the chat rendered it verbatim, so a Chinese UI put a Chinese title over an
English body. No amount of rewording fixes that; only moving the sentence
to the client does.
- The paragraph restated the raw agent line, which the card's details block
already prints — the same text twice in one card.
So the daemon now NAMES the failure and stops there: `AGENT_CLI_SESSION_REFUSED`
plus `details: { kind: 'agent_cli', action: 'update_cli', agent, agentCliVersion }`,
with `retryable: false` because a build that refuses `session/new` refuses the
identical request identically. `withAcpHandshakeFailureGuidance` stays the single
invariant point (the ACP bridge payload feeds SSE and `run.error` at once) and now
stamps only the structured half; the stderr-tail fallbacks wrap their payloads in
it too, so no path is left composing prose.
`run.error` keeps the agent's `json-rpc id N: …` line byte for byte. It is the
classifier's input — rewriting it degrades this failure class to `unknown` in
telemetry — and it is what the details block shows, so leaving it alone is also
what removes the duplicate.
The web maps the code to localized copy in `resolveRunFailureUi`, resolved before
every other branch because the copy is not a fixed sentence: it names the CLI
build the daemon detected, and falls back to a version-less wording when there
was none (the same rule the rolling model-window copy follows). The version rides
to the client as data, on the status event next to `code` / `failureDetail`.
Three keys in all 19 locales.
Red first, on the path users hit: the wiring suite (real `startServer` + a fake
ACP CLI) went red on `run.error` being the English paragraph, and a new ChatPane
render spec went red on the raw line appearing twice in one card. Both green here.
Verified in a real browser against a local runtime: the same failed run renders in
zh-CN and in en from one persisted event.
* test(daemon): stop the wiring suite pinning a racy CLI-version probe
CI run 32683047377 caught this on the previous head: the wiring test demanded
`0.38.0` inside `run.error` and got a version-less sentence, because
`getDetectedRuntimeVersions` is a process-wide cache that `probe()` clears
before each probe and refills after — a concurrent `/api/agents` refresh can
leave it empty exactly while a run is failing. Locally it always won the race;
on CI it did not.
The new assertions inherited that: `details.agentCliVersion` was pinned to an
exact string on both wiring cases. Assert the property that actually holds
instead — the version is either the one the daemon detected or absent, never
some other value — and say in the helper why presence is not asserted here.
Both copy paths (version present, version absent) stay covered
deterministically at the layers that own them: the payload unit suite and the
two web suites.
Making detection itself deterministic is a separate finding, still open on
this PR, and deliberately not fixed here.
* fix(daemon): let the classifier, not the wording, name a handshake refusal
Three review findings, one shape: the ACP handshake verdict was being decided
in more than one place, so the places disagreed.
1. The handshake reading sat inside `isAgentProtocolErrorText`, whose JSON-RPC
arm matches only `json-rpc id N: Internal error`. A CLI that refuses
`session/new` with `Method not found` or `Invalid params` skipped it
entirely and was promoted to a retryable `fatal_rpc_error` on `child_close`
— the no-auto-retry behaviour this PR claims never applied to it — or, once
the payload rewrite had stamped its code, fell all the way through to
`unknown` / `finalize`, which is exactly the untriageable bucket the module
docblock says the rewrite must never produce.
The wording an agent picks for its own rejection is not a signal. The
handshake branch now sits AFTER every cause branch instead of inside the
protocol one, so precedence is a fact of the control flow rather than a
promise: whatever auth, rate limit, balance, upstream, prompt size, or CPU
already claimed keeps it, and only an unexplained refusal reaches the last
line. Ids 3 and up never enter it and keep the transient treatment they
have always had.
2. `handshakeFailureNamesItsOwnRemedy` knew only the three
`classifyAgentServiceFailure` classes, so every other cause the run
classifier can already advise on was rewritten into the CLI-upgrade copy.
A user whose content was too long read "your CLI version is incompatible"
while the same run was filed as `prompt_too_large` / `reduce_context`.
`isAcpCliSessionRefusalText` now IS the classifier — it runs
`classifyRunFailure` over the text and reports whether that reading is the
handshake refusal — so the card the user gets and the bucket the run lands
in cannot prescribe two different fixes. The id-only helpers moved to
`runtimes/acp-handshake-id.ts` so the classifier can read the id without the
id-reader having to reach back into the classifier.
3. The guidance read the process-wide detected-version cache at FAILURE time,
while `probe()` cleared each entry before re-reading it. Any `/api/agents`
refresh overlapping a failing run decided whether the user was told which
build refused them; CI caught it (run 32683047377) and the assertion was
loosened instead of the race closed.
Both halves are closed. A probe now publishes its reading once, at the end,
and only when no later probe has already published, so the cache holds the
newest COMPLETED reading rather than the state of one in flight. And the run
freezes the version it is spawning with, so failure-time guidance reads the
run and never the live cache.
Coverage is at the wiring layer, not just the predicates: the fake ACP CLI
gained named gates so a run's handshake and a concurrent `/api/agents` refresh
can be held open together and the failure built at the exact moment the cache
is mid-probe — deterministic, no sleeping on wall-clock timing. The wiring
suite's CLI-version assertion goes back to strict.
One deferral found by the existing suite and now pinned: a handshake-numbered
frame carrying an OS-level crash banner (Windows STATUS_ILLEGAL_INSTRUCTION
from the bundled opencode) reports a child that died, not one that refused, and
stays with the crash reading.
* test(daemon): describe the overlap the gate test actually constructs
The comment said the failure is built "at the precise moment the cache is
blank". That was the pre-fix window; the cache no longer goes blank mid-probe,
which is half of what the test proves. Say what it now holds open — a probe in
flight — and name both halves it exercises.
* fix(daemon): drop the duplicate detection import that blocks startup
`getDetectedRuntimeVersions` was imported twice — once standalone and
once inside the `./runtimes/detection.js` group — so the module has two
declarations of the same lexical binding. Node's ESM loader rejects that
outright with `Identifier 'getDetectedRuntimeVersions' has already been
declared`, i.e. the daemon could not boot.
The suite never caught it: vitest transforms through esbuild, which
tolerates the duplicate, so 179 tests passed against a module real Node
refuses to load. Verified by importing server.ts under Node directly —
SyntaxError before, gone after.
* fix(daemon): report the CLI build this run actually started, on both surfaces
A version-specific refusal (`AGENT_CLI_SESSION_REFUSED`) names the CLI build
that refused, so the user knows which one to change. Two things could make
that name wrong.
**The persisted event dropped it.** `send()` stores every run event on the
assistant message before emitting it, and `runSseEventToPersistedAgentEvent`
serialized only `detail` and `code`. The live card named the build; a reloaded
one fell back to the version-less sentence — exactly when the user comes back
to act on it. The mapper now lifts `error.details.agentCliVersion` out of the
frame, validated as a non-blank string, because `details` is a pass-through bag
that also carries the agent's own JSON-RPC `error.data`.
**The spawn-time capture could name a different executable.**
`getDetectedRuntimeVersions` answers per agent id and says nothing about which
binary the reading came from. A user who repointed the CLI after Settings last
listed agents (Settings writes `KIMI_BIN`; PATH changes) ran one build and was
told to change another. The capture now resolves through
`ensureDetectedRuntimeVersions` with the same `configuredAgentEnv` that
produced this run's `agentLaunch` — cached when the launch identity is
unchanged, re-probed through the same bounded `--version` read when it is not,
and taken before the spawn marks so preflight work is accounted to preflight.
Both are pinned end to end in `acp-handshake-failure-wiring.test.ts`: one test
reads the conversation back through the messages route a reloading client
calls, the other repoints `KIMI_BIN` between detection and the run and reads
the two fake CLIs' invocation logs to establish which child actually ran before
asserting what the failure reports.
* fix(daemon): keep a runtime that never started out of the CLI-refusal verdict
An ACP handshake rejection was decided on the JSON-RPC request id alone, so any
failure numbered 1 or 2 became "this CLI build cannot open a session; change
it". AMR is the population that reaches this path, and what it runs underneath
is OpenCode: when vela's bundled OpenCode child fails to come up it reports
`start opencode server: opencode exited before readiness: exit status 3` from
inside `session/new`, which arrives handshake-numbered while saying nothing
about the agent CLI's own build.
Two things went wrong at once. The user was told to replace a perfectly healthy
CLI, and the automatic retry that actually recovers a startup race was
withdrawn — `process_exit / agent_protocol_error / session_init`,
`retryable: false`, `install_cli`, with the ACP wrapper stamping
`AGENT_CLI_SESSION_REFUSED` on top. A startup race clears on the second
attempt; a CLI-build verdict never does, so the two must not collapse.
`isManagedRuntimeStartupFailureText` names the distinction: a CLI reporting
that a runtime IT manages never came up has named the moving part itself, so
its build is not the only variable left. The markers are vela's own wrapper
text, already trusted by `isCpuUnsupportedCrashText` for the crash-banner
sibling of this shape and now shared with it. Both deferrals now read the same
way: the handshake frame is the envelope, not the evidence — a child that DIED
(crash banner) or one that never STARTED (readiness) is not a refusal.
Also stops this suite's runtime from being a property of the host. The wiring
cases drive `GET /api/agents`, which probes every shipped runtime and resolves
binaries from PATH plus the machine's toolchain directories, so a host with
real agent CLIs installed turns each refresh into a dozen real spawns: ~2.8s
here, ~28s on a reviewer's machine — past the 20s test timeout for the
mid-refresh case, which waits on two. `isolateAgentDetection` points
`OD_AGENT_HOME` at an empty per-test directory, detection's own mechanism for
scoping the search, so the only CLI anything can find is the fixture's. Same
route, same probe path, same deterministic gates and overlap assertions; the
file drops from 109s to 14s and the mid-refresh case from 30s (timing out) to
1.2s.
* revert(daemon): drop the CLI-version half of the handshake-refusal card
The copy wanted to name the build that refused, and the chain that
followed ended in the launch path: the version had to be frozen before
the spawn, freezing meant awaiting a `--version` probe there, and an
await before `spawn()` sits after the run's only pre-spawn cancellation
guard. Stop clicked during that window finishes the run as canceled,
the probe then resolves, and the child is spawned onto an already
terminal run that nothing will signal.
The whole sub-branch goes rather than a second guard. What it bought
was `(0.38.0)` in a sentence that already read correctly without it;
what it cost was a run the user asked to stop starting anyway.
Removed: the pre-spawn `ensureDetectedRuntimeVersions` await,
`run.spawnedAgentCliVersion`, and the version half of
`agentFailureIdentity`; detection's probe generation guard;
`agentCliVersion` persistence, its `PersistedAgentEvent` field, its web
pass-through, and the `{version}` copy variant in all 19 locales.
`server.ts` no longer touches the run lifecycle at all — its diff is
four `send('error', ...)` call sites and one helper.
Detection's generation guard fixes a real, independent daemon bug (for
the length of every `/api/agents` refresh the daemon cannot report the
version it detected). It ships as its own PR, and naming the build in
the copy follows it.
Kept: the localized `AGENT_CLI_SESSION_REFUSED` card in 19 languages,
no auto-retry for a handshake-stage refusal, `run.error` exactly equal
to the agent's own line, auth/quota/upstream/managed-runtime-startup
each keeping their own verdict, and ids 3+ staying out of the branch.
The cases that asserted a version number now assert the same behaviour
without one, plus `not.toHaveProperty('agentCliVersion')` and a locale
guard against a stray `{version}` placeholder, so a partial re-land
cannot slip half of this back in.
|
||
|
|
f1a73f0d89 |
feat(od-next): structure-only layout primitives for the prototype task profile (#7327)
* feat(od-next): ship structure-only layout primitives with the prototype task profile Blind evals and the pet-grooming runs showed the same layout defects over and over — stacked spans that never became blocks, chrome height constants reused as padding, authored copy crammed until it wrapped — while the prose rules that forbid them sat unchanged in the prompt. The rules are now backed by something the Build can copy instead of re-derive: - `assets/task-profiles/prototype/layout.css`: a ~50-line, structure-only stylesheet (`@layer od-layout`, `:where()`, `od-` prefix) for stacked information, truncation/clamp of data text, media ratio, rails, and an optional screen skeleton. Declared as a prototype resource, staged into `.od-frames/` under the existing ownership manifest, and quoted into the stable request context as the `layout-primitives` fact on every prototype run. - Prototype rule card 2.2.0: a "Variable-length text and stacked information" section that separates authored copy (rewritten to a budget, never truncated) from data text (truncate/clamp in lists, wrap in full on confirmation and detail screens), keyed to the real class names; the quality floor's layout bullet now points at the primitives. - Core 2.1.0: the variable-length-text paragraph rewritten as actions (three fates before the text is written) instead of prohibitions. - Run-finish observation `od_next_layout_primitives` (verbatim / modified / linked / absent) so the rollout can tell whether a model tier reproduces the block faithfully enough for inlining. * fix(od-next): let aspect-ratio win on .od-media and pin two rule-card gaps The first verification runs exposed one primitive bug and two recurring model slips: `.od-media` lacked `height: auto`, so an `<img height=…>` attribute kept its pixel height and the ratio never applied (tall cropped thumbnails); cards built on `<a>` shipped with browser-default link styling; and product tokens declared on an inner wrapper left sheet/dialog overlays mounted in the shell's screen without a surface. The stylesheet and the rule card now say so. * fix(od-next): keep .od-media at its intrinsic ratio; make cropping an explicit opt-in Review: the fallback aspect-ratio of 1 plus an unconditional object-fit: cover turned every unset .od-media into a square crop — contradicting the core image-geometry rule (content-bearing images render their full frame; cover is reserved for deliberate crops) and the primitive's own comment, exactly on the media the rule card points at product shots. The primitive now defaults to aspect-ratio: var(--od-ratio, auto) with no object-fit; a uniform tile crop sets --od-ratio and adds .od-media-cover together, and the commerce-card row of the rule card says so. * fix(od-next): carry finish-time observations on run_finished, not the frozen baseProps Review: od_next_device_platform*/od_next_layout_primitives were spread into baseProps, which is built at run creation — before any deliverable exists — and run_finished only spreads that frozen object. The daemon log printed the observations while the analytics event never carried them, which defeats the rollout signal these fields exist for. They now join finishedProperties, read live from the run after the physical run resolves. (The device-shell fields shipped with the same trap in the shells PR; fixed here on the same lines.) --------- Co-authored-by: NJUHua <113895241+NJUHua@users.noreply.github.com> |
||
|
mehmet turacandGitHub
|
ba3e8ef2c1 |
fix(runtimes): pass Antigravity prompt via -p argument (#7205)
* fix(runtimes): pass Antigravity prompt via -p argument agy -p - treats "-" as the literal prompt and ignores stdin, so print mode never receives the user message. * chore: retrigger CI after flaky workspace-restoration P0 * chore: retrigger CI after repeated workspace-restoration flake |
||
|
Trần Đình HuyandGitHub
|
bc93dfe680 | feat(mcp): make stdio idle exit configurable (#7288) | ||
|
|
0918d79fbd |
fix(daemon): make an ACP stage-watchdog stall attributable (#7310)
* fix(daemon): make an ACP stage-watchdog stall attributable
An ACP turn that goes silent is ended by attachAcpSession's own stage
watchdog, which fails the turn and then kills the child. Two telemetry
fields that exist to explain that terminal were wrong:
- `last_progress_age_ms` is contractually "near the inactivity ceiling on
a stall". The daemon's ACP `send` wrapper stamped
run.lastAgentActivityAt for every emission, including the watchdog's own
error, so the daemon's timeout event became the last recorded progress
and a 30-minute silence reported 664ms.
- `terminal_trigger` was unset, so the run surfaced only the child's exit
code (AGENT_EXIT_130) - indistinguishable from a user interrupt.
Field case: run 14b04dd3-56b0-4d44-926b-db6cee3017ab (2026-07-28, OD
0.16.1, amr_cloud). 37.2 minutes, killed after ~30 minutes of silence,
reported last_progress_age_ms=664, no terminal_trigger. Triage read that
as "still working when killed" and chased the wrong window.
* fix(daemon): exclude host-synthesized ACP tool pairs from the progress clock
The previous commit stopped the ACP bridge's terminal `error` from stamping
`run.lastAgentActivityAt`, but that is only half the failure path. Every ACP
fatal path calls `flushOpenAcpTools(true)` *before* `send('error')`, and that
helper emits a synthetic `tool_use` + errored `tool_result` pair for each tool
the agent left open. Those ride the ordinary `agent` channel, so they stamped
the progress clock microseconds before the run finalized — a stall that died
with a tool in flight still reported a near-zero `last_progress_age_ms`.
That is the common stall shape, not an edge case: the new regression fixture
(open tool, then a silent stage timeout) reported 605ms against a 2000ms
silence, which is the same magnitude as the field incident's 664ms.
Carry the provenance out-of-band instead of in the payload: `send` takes an
optional `AcpEmissionMeta`, `flushOpenAcpTools` tags its pairs
`hostSynthesized`, and `runtimeEmissionCountsAsAgentProgress` excludes them
alongside the terminal error. The transcript, SSE wire, and Langfuse metadata
are unchanged — the pairs are still emitted, they just no longer count as the
agent making progress.
* test(daemon): name the emission that re-stamps the ACP stall progress clock
The progress-age assertion is green locally and red on CI, and a bare
"expected 16 to be >= 1600" cannot say which emission moved the clock.
Attach the run_finished summary and the persisted run event tail to the
failure so the next CI run identifies the culprit directly.
* fix(daemon): stop the progress clock when the daemon gives up on the turn
CI proved the previous two fixes were still partial. With the failure path's
synthetic tool pairs suppressed, an ACP stall on CI still reported
`last_progress_age_ms` of 17ms and 12ms, and the persisted event tail named the
culprit:
... agent:text_delta, error, agent:diagnostic, diagnostic:runtime_close
Emissions keep arriving AFTER the terminal error. The child's shutdown line on
stderr (`child.stderr.on('data')` stamps every byte), and the diagnostic
promoted from it, both land after the daemon has already decided to kill the
turn — and each one re-stamped the clock, so the reported age measured our own
teardown instead of the agent's silence. That ordering is timing-dependent,
which is why these specs were green locally and red on CI.
Freeze the clock at the verdict instead of hunting emissions one at a time.
`noteAgentActivity` stops advancing `run.lastAgentActivityAt` once this attempt
has a terminal verdict; everything else it does (tool-token TTL, watchdog
re-arm) is unchanged. The freeze is attempt-scoped: retries and the
resume-failed reseed re-enter `startChatRun` with a fresh closure, so the next
attempt measures its own silence. User cancel returns before the freeze and
keeps its own terminal path.
The flushed-tool marker is still required and not redundant: `fail()` flushes
open tools BEFORE it sends the error, so those pairs precede the freeze.
Adds a third regression case: the agent logs to stderr while shutting down.
Without the freeze it reports ~400ms against a 2000ms silence, deterministically.
* docs(daemon): tie the progress-clock predicate and freeze into one rule
* fix(daemon): let the ACP verdict own the attempt's terminal attribution
`freezeProgressClock` stopped the timestamp but left the outer chat inactivity
watchdog armed. `attachAcpSession.fail()` issues one direct SIGTERM and nothing
escalates it, so a child that lingers past the inactivity ceiling lets
`failForInactivity` fire on a run it does not yet consider terminal — the close
handler is what terminalizes — overwriting `terminal_trigger` with
`inactivity_watchdog` and emitting a second failure. The stall then reads as the
wrong clock, which is exactly the confusion `acp_stage_timeout` exists to
remove.
Red spec (a fixture that swallows SIGTERM, with the outer ceiling set just above
the stage timeout) reproduced it deterministically:
AssertionError: expected 'inactivity_watchdog' to be 'acp_stage_timeout'
run event tail: [... "agent:text_delta", "error", "error",
"diagnostic:runtime_close", ...]
Two `error` events, and the run finished under AGENT_EXECUTION_FAILED instead of
the child's own terminal code.
`retireAttemptOnAcpVerdict` now hands the attempt to the close handler under the
verdict the ACP bridge already reached: freeze the clock, retire the outer
watchdog, and escalate the teardown. The escalation is the necessary other half
— retiring the watchdog alone would leave a SIGTERM-ignoring child with nothing
to reap it. `scheduleForcedChildShutdown` captures this attempt's child, so a
retry that swaps `run.child` inside the grace window is unaffected, and user
cancel still returns before any of this.
* fix(daemon): stop every clock of an attempt that already has a verdict
Retiring the outer watchdog at the ACP verdict was not enough on its own.
`noteAgentActivity` only skipped the timestamp when frozen and still fell
through to the timer block, which clears and re-arms `inactivityTimer` — and
the raw `child.stderr` handler routes every late byte through that helper. So a
child that logs its shutdown line while ignoring SIGTERM re-armed the very
watchdog `retireAttemptOnAcpVerdict` had just cleared, and that timer firing
before forced shutdown reaped the child terminalized the run a second time.
The two existing fixtures missed it: one ignores SIGTERM without logging, the
other logs and exits. The gap is a child that does both. With the fixture logging
once (as a real CLI announces shutdown) rather than per signal — repeating it
would keep pushing the timers out and hide the race:
AssertionError: expected 'inactivity_watchdog' to be 'acp_stage_timeout'
run event tail: [... "agent:text_delta", "error", "stderr", "error",
"diagnostic:runtime_close", ...]
`error, stderr, error` is the whole story: the late stderr between the two
verdicts is what re-armed the clock.
`noteAgentActivity` now returns as soon as the attempt is frozen, so neither the
timestamp, the tool-token TTL, nor the watchdog can be restarted by a child that
is already being torn down. The new case also asserts exactly one terminal
`error` reaches the transcript, which is the property the trigger field depends
on.
---------
Co-authored-by: lefarcen <ontf116@gmail.com>
|
||
|
lefarcenandGitHub
|
fb26b5cb8a |
fix(runtimes): support the dsh release line users are actually served (#7339)
* fix(runtimes): support the dsh release line users are actually served Two days after #7153 widened the version policy from one release candidate to the `0.1.0-rc.N` line, upstream published `0.1.1-rc.1` and then `0.1.1-rc.2`, and made the latter the `latest` tag. Scoping to `0.1.0-rc.N` was still a pin, just a longer one: every user on what npm now serves is told their CLI is untested, our installer hands out a version two releases behind, and the connection component's peers do not resolve against the host they land on. That last one is not a warning. semver only lets a prerelease satisfy a range when some comparator carries the SAME major.minor.patch tuple AND a prerelease tag, so `>=0.1.0-rc.6` matches nothing on the 0.1.1 line — a verified `satisfies('0.1.1-rc.2', '>=0.1.0-rc.6') === false`, the same rule that made the original pin uninstallable. Move all three to the 0.1.x line: the policy accepts any `0.1.N`, release candidate or stable; both installers pin `0.1.1-rc.2` with the resolution window reset to just after its publish time; the peers gain a comparator for the new line. This will expire again. A comparator per prerelease line is what semver allows, so `0.1.2-rc.1` will not resolve until someone adds one, and nothing in the repo notices when upstream moves. The check that catches installer/policy drift only compares the two against each other. * ci: watch upstream dsh releases instead of waiting for a bug report Twice in one week `@deepseek-ai/dsh` published a new release line while this repo kept pinning the old one, and both times the detection mechanism was a person reporting that their install did not work. The check added with the first fix compares our installers against our agent def. Both are our own config, so it stays green precisely when they are equally out of date — which is the state that shipped, twice. Nothing here looks at the registry. Add a daily scheduled comparison against npm's `latest` that separates the two failures worth telling apart: `behind` means our installer hands out an older version than the registry serves, everything still works; `unsupported` means a user who has what npm serves is outside what we claim to support, which is an "untested" warning in Settings and peer ranges that do not resolve. The card names all three files that must move together, because forgetting the peer ranges is the one that breaks an install rather than warning about it. Standalone and outside the merge gate, like nix.yml and docker-image.yml: an upstream release is not a reason to fail someone's PR. The workflow runs `self-check` before touching the registry so a broken checker is loud rather than quietly in-sync. * fix(ci,runtimes): stop the drift watch losing what it exists to report Four defects from review, each of which would have made this quieter than having built nothing. The widened policy over-claimed. `/^0\.1\.\d+(?:-rc\.\d+)?$/` accepts `0.1.2-rc.1`, which the peer ranges cannot install — so the daemon would suppress the untested warning for a version whose companion fails to resolve, which is worse than the warning and silent. Narrow prerelease acceptance to the tuples and floors the peers actually carry; stable releases on the line need no comparator and stay accepted. This also retires an over-claim that predates the branch: `0.1.0-rc.2` and `rc.3` are published and below the `>=0.1.0-rc.6` floor. `e2e/tests/dsh-installer-version-policy.test.ts` now runs both authorities over a version matrix and fails when the def accepts anything the peers reject, so the two cannot drift apart again. Importing the script ran the CLI. Collecting the new test fetched the live registry — measured at 354ms with the verdict printed — and once drift existed would have attempted a real Feishu post before any assertion. Move the dispatch behind a direct-entry check and prove it with a test that stubs fetch and re-imports. Feishu answers a rejected webhook with HTTP 200 and a nonzero code, so treating every 2xx as delivered discards the alert and logs success doing it. Mirror the landing notifier's contract: code 0 or absent is delivered, 429/5xx/9499 retry, anything else raises. The workflow chose the landing webhook but signed with an unpaired secret, so a bot that verifies signatures would reject the card. Pair them the way the landing workflow does, and pin the mapping in the topology test. * test(e2e): check each dsh peer range on its own The compatibility guard scanned the whole manifest and merged every comparator into one tuple table, so a single updated peer answered for the other four. A bump that updates one range and forgets the rest — the realistic way this goes wrong — read as compatible, which is the partial state the guard exists to catch. Parse `peerDependencies`, keep the `@deepseek-ai/dsh-*` entries, and require every one of them to admit a version before calling it installable. `cordis` is excluded: it has its own version line and never moves with dsh. Two cases cover it: a fixture where one peer carries the new comparator and the others do not, and the floor itself, since a comparator for the right tuple still rejects a candidate below it. Verified against the real manifest by downgrading four of the five ranges, which reds the matrix with `expected [ '0.1.1-rc.1', '0.1.1-rc.2' ] to deeply equal []`. |
||
|
lefarcenandGitHub
|
f92c2d957c | fix(viewer): stabilize Electron HTML previews (#7336) | ||
|
|
fe94564a71 |
fix(daemon): stop sending Kimi 0.37+ an MCP transport it rejects (#7313)
Kimi Code CLI 0.37.0 replaced the stdio branch of its ACP `session/new`
MCP handler with a throw:
{"code":-32603,"message":"Internal error","data":{"details":
"ACP stdio MCP server open-design-live-artifacts does not declare
a runtime identity"}}
OD attaches a stdio MCP server (`open-design-live-artifacts`) to every
`mature-acp` runtime, so every Kimi run on 0.37+ failed at the handshake
— which is why the observed failure rate is 84–92% rather than flaky.
The message names a "runtime identity" but no accepted value exists.
Kimi validates `session/new` against a zod union whose stdio member
declares no `type` key, so an explicit `type: "stdio"` is stripped by
validation and the handler then sees no `type` and throws. Verified
against the published tarballs by driving a real ACP handshake: 0.35.0
and 0.36.1 accept a stdio entry with or without `type`; 0.37.0, 0.37.1,
0.37.2 and 0.38.0 reject both spellings. Only http/sse survive.
So the session now withholds stdio MCP servers from builds that reject
them, judged against the version the agent reports in its own
`initialize` result — the build that is actually about to parse
`session/new`, and therefore more current than any earlier `--version`
probe. Which builds reject is runtime-def data
(`acpStdioMcpRemovedInVersion`), following the `acpMcpEnvFormat`
precedent, so hermes / trae-cli / reasonix declare nothing and keep a
byte-identical payload.
Kimi 0.37+ runs lose live-artifact tooling, which is unreachable on
those builds either way; a session that opens without MCP beats one that
cannot open at all. 0.36.x keeps the capability.
Co-authored-by: lefarcen <ontf116@gmail.com>
|
||
|
lefarcenandGitHub
|
d85e78ab7d |
fix(runtimes): reach a working agent CLI past a broken shim (#7153)
* fix(runtimes): reach a working agent CLI past a broken shim
Resolving an agent's name on PATH only proves a file exists there, never
that it runs. Detection stopped at the first hit, so a wrapper orphaned by
a half-finished `npm i -g` shadowed a perfectly good CLI of the same name
in a later search directory — and because the not-invocable branch dropped
the resolved path, the picker (which renders an unavailable agent only when
it carries one) hid the agent entirely. The user saw "scan found nothing"
with no way to act.
Detection now walks past candidates that cannot be executed, and reports
the path it tried when every candidate fails, so the row stays actionable.
Also on this chain:
- `@open-design/dsh-runtime` pinned its DeepSeek Harness peers to one exact
release candidate, so it became uninstallable the moment upstream shipped
the next one. Peers now take ranges. `scripts/guard.ts` gains a narrow,
logged exemption for external-host plugin manifests — their peers describe
a host this repo neither installs nor controls, which is the one case
where an exact pin is wrong rather than reproducible.
- The DSH installers pinned only the entry package while its ~190 siblings
used caret ranges over prerelease floors. Once a newer release candidate
existed, the transitive tree floated onto it and its mutually exclusive
peer sets sent npm into an ERESOLVE backtrack that never converged. Both
installers now freeze the resolution window alongside the pin.
- The rescan notice counted agents the list never renders, so it announced
"3 available" above two rows.
* fix(runtimes): make detection's chosen executable the one spawn sites use
The candidate walk taught detection to skip a binary it cannot execute, but
every other resolution — chat, the connection test, the memory summariser,
companion install — called `resolveAgentLaunch` with no skip list and got
back the first file that merely exists on PATH: the broken shim detection
had just rejected. Settings advertised the agent as installed while each
turn exec'd the wrapper, so the fix only ever reached the picker.
Detection is the only stage that spawns anything, so it is the only one that
learns which candidate works. It now publishes that winner, and resolution
prefers it over a fresh first-hit walk. An explicit `*_BIN` override and a
packaged built-in still outrank it, a winner whose file has since vanished
is ignored, and detection clears the entry before each pass so a rescan
after a repair never resolves against a stale one.
This restores the invariant the file already documented — detection probes
the exact path the runtime will spawn — which the walk had broken.
Reported by @mrcfps in review.
* fix(runtimes): keep the remembered executable inside the live candidate set
The winner map made resolution stop being a pure function of the current
environment. It only checked that the remembered file still existed, so a
path learned in a richer environment survived an emptied PATH, a sandboxed
OD_AGENT_HOME, and any other narrowing of the search — resurrecting a binary
the caller could no longer see.
That is not just a test artifact: `GET /api/integrations/vela/status` must
answer `amr-runtime-unavailable` when the vela binary cannot be resolved, and
it started answering "ready" instead, because an earlier detection in the
same process had remembered a path.
The memory now reorders candidates rather than introducing them: it applies
only when the live search already offers the remembered path. An emptied
PATH, a sandbox, and an uninstalled CLI all keep meaning "not found", while
a broken shim shadowing a working binary is still resolved to the winner.
This also settles the scope question raised in review — no separate cache key
is needed, because the candidate set already carries the resolution scope.
* fix(runtimes): treat a launcher whose target is gone as not invocable
A stale global npm wrapper on Windows is a `.CMD`, which OpenDesign runs
through `cmd.exe /d /s /c`. When the package behind it has been
uninstalled, cmd.exe starts, node starts, and only then does node fail to
load the script the wrapper names — a plain exit 1 with MODULE_NOT_FOUND
on stderr. None of the spawn-level signals the classifier looked for
(ENOENT / EACCES / ENOTDIR, exits 126 and 127) can occur along that
chain, so detection read the dead wrapper as a healthy CLI that merely
disliked `--version`, stopped the candidate walk on it, and never reached
the working `dsh.CMD` later on PATH.
Classify invocability from the launcher's own vocabulary as well: cmd.exe's
9009 (the Windows analogue of POSIX 127), node's missing-module report, and
cmd.exe / PowerShell's missing-command and missing-path reports. Exit status
alone still means nothing — a CLI that runs and exits non-zero is a real
answer from the right binary and keeps its place in the walk.
Drop the win32 early return from every case in the fallback suite and write
fixtures per platform, so `dsh.CMD` resolution through PATHEXT and the
cmd.exe wrapping are exercised natively. The orphaned-wrapper regression is
reproduced on every platform — the defect is a classification bug, not an OS
API one, and the daemon suite only runs on Linux in CI.
* fix(runtimes): carry forward the dead paths, not detection's winner
Detection published the candidate it settled on and resolution preferred
it over the first live PATH hit. That pinned the daemon to one binary for
its whole lifetime and let it silently outrank the caller's own PATH: a
CLI that became visible earlier afterwards — a fresh install, a version
manager swapping shims, a caller resolving under its own environment —
could no longer be reached.
The daemon suite caught it on a machine that has a real `vela` installed:
`chat-route` and `connection-test` inject a fake agent CLI ahead of PATH
per case, and the remembered winner outranked it, so 15 cases spawned the
machine's real CLI instead of the fixture. CI never saw it because its
runners have no such CLI to remember.
Carry the proven-dead paths forward instead. Skipping them keeps every
candidate still standing in plain PATH order, while chat, the connection
test, the memory summariser, and companion setup still avoid the shim
detection rejected — which is the whole point of publishing anything.
* docs(runtimes): say why detection keeps its own launcher-error list
`isCliNotInstalledText` in run-failure-classification.ts reads similar
output, and the next reader will reasonably ask why the two are not one.
They answer different questions: that one buckets a finished run for
telemetry, where over-matching costs a mislabelled event, while this one
decides whether to abandon a binary mid-resolution, where over-matching
sends a working CLI's user to a different install.
* fix(runtimes): accept the dsh release line, not one release candidate
This branch moved the installers to 0.1.0-rc.8 and left the agent def
naming 0.1.0-rc.6 as the only supported version, so following our own
one-line install produced "0.1.0-rc.8 has not been tested with this
OpenDesign build" in Settings on a clean machine. That is the same
pinning defect the installers had, one layer up.
`dsh` publishes a stream of release candidates whose sibling packages
peer-require each other, so the version our installer hands the user
moves with upstream. Version policies can now declare the release line
they accept alongside the exact versions exercised, and DeepSeek Harness
declares its own. A version off that line still warns — the point is to
stop pinning, not to stop checking.
Add a cross-boundary guard for the drift itself: the installers and the
agent def cannot import each other, so `e2e/tests` reads both and fails
when an installer pins a version the daemon would not accept.
* docs(contracts): correct what shim-broken now means
The reason's docstring said "exit 126/127", which stopped being the whole
story once detection learned to read a Windows wrapper's stderr: a `.cmd`
starts an interpreter successfully and reports the missing target there,
so no POSIX exit code appears at all.
* test(runtimes): cover the launcher signatures no fixture can produce
`launcherTargetMissing` recognises six ways a launcher says it never
reached its target, and until now two of them had any coverage — the two
a POSIX shell can print. cmd.exe's wording, PowerShell's, and exit 9009
were load-bearing for the Windows fix and untested anywhere.
A fixture cannot close that gap. POSIX masks exit statuses to eight bits,
so a shell asked for 9009 reports 49, and /bin/sh does not speak cmd.exe.
The daemon suite runs on Linux in CI, so driving the classifier directly
is the only way the merge gate can catch a typo in a signature the fix
depends on for the platform the bug came from.
Extract the catch arm verbatim as `classifyVersionProbeFailure` — same
branches, same order, no behaviour change — and feed it the rejection
shape `promisify(execFile)` actually produces. Verified by mutation:
deleting the 9009 branch reds one case, deleting the stderr branch reds
six.
The suite also pins the constraint from the review by name: exit 1 with an
ordinary argument complaint, with no stderr, or with wording that belongs
to run-failure telemetry ("not installed") all stay `spawned`. That last
one is why detection keeps a narrower list than `isCliNotInstalledText`.
|
||
|
Marc ChanandGitHub
|
18273c01b8 |
fix(packaged): recover from stale web sidecars (#7279)
* fix(packaged): recover from stale web sidecars * fix(packaged): stop stamped web owner before socket takeover Unlinking only the unresponsive web socket left the old Next process alive. Its later JsonIpcServer.close() could delete the replacement pathname. Stop the stamped owner through platform primitives first. Generated-By: looper 0.12.0 (runner=fixer, agent=opencode) * fix(packaged): strip handoff parent pid and skip unsafe web unlink Strip OD_TOOLS_DEV_PARENT_PID from the legacy payload desktop spawn environment so the replacement does not exit after waiting for the outer PID. Skip web socket unlink when a stamped owner is still running after stop. Generated-By: looper 0.12.0 (runner=fixer, agent=opencode) * fix(packaged): skip web unlink when process discovery fails listProcessSnapshots returns [] on enumeration failure, so an empty table is not proof that no stamped owner exists. Leave the live socket in place unless discovery succeeded and every matched owner is gone. Generated-By: looper 0.12.0 (runner=fixer, agent=opencode) * fix(daemon): hold parent monitor until handoff journal commits Packaged daemons now exit when the outer Electron dies. Desktop SHUTDOWN acks before asynchronously exiting, so the parent can disappear while armed handoff, attempt, and runtime writes are still pending. Hold the parent-death exit across that window. Generated-By: looper 0.12.0 (runner=fixer, agent=opencode) * fix(daemon): defer SHUTDOWN until handoff journal commits Packaged beforeShutdown still sends sidecar SHUTDOWN after desktop acks, and that path ignored the parent-monitor hold. Wait for the same release before exiting so handoff.json, attempts.json, and runtime.json can finish. Generated-By: looper 0.12.0 (runner=fixer, agent=opencode) * fix(daemon): defer SIGTERM until handoff journal commits Packaged closeManagedChild waits five seconds then stopProcesses() sends SIGTERM. That handler still exited immediately and could cut off the held journal writes. Route signal shutdown through the same hold as SHUTDOWN. Generated-By: looper 0.12.0 (runner=fixer, agent=opencode) * fix(daemon): hold parent monitor before payload spawn A crash after the replacement emits spawn but before the journal hold lets the parent monitor exit the daemon while handoff.json is still prepared. Acquire the hold before spawn and release it on failure. Generated-By: looper 0.12.0 (runner=fixer, agent=opencode) * fix(packaged): wait for deferred handoff shutdown before SIGKILL Daemon SHUTDOWN now reports deferred:true while the handoff journal hold is active. closeManagedChild then waits a bounded 30s for self-exit before stopProcesses, so SIGKILL cannot cut off the three sequential journal writes. A POSIX regression exercises the real stopProcesses path and checks all three files commit before forcedPids. |
||
|
open-design-crew[bot]GitHubCheemslefarcenCheems黄桃open-design-crew[bot] <299007234+open-design-crew[bot]@users.noreply.github.com>NJUHuahihijack111 |
7320f9b8ab |
feat(daemon): make OD Next the default design strategy (#7016)
* refactor(observability): isolate legacy run exporter
* refactor(daemon): extract internal run creation service
Route /api/runs and compatibility /api/chat through one daemon-owned create, claim,
resume, and start seam. Preserve HTTP responses, SSE timing, session bookkeeping,
and durable Run formats.
Validated with daemon tests/build/typecheck, workspace guard, and workspace typecheck.
* test(plugin-runtime): freeze OD Next strategy fixture boundaries
Add exact pre-Build route and stage cases plus deterministic deletion-boundary mutations without shipping an evaluation harness.
Preserve the current official and community fallback pipeline and composed verification/critique prompt sections as golden witnesses.
Validated with plugin-runtime tests and types, focused daemon prompt composition, daemon types, guard, and workspace typecheck.
* fix(daemon): finalize terminal telemetry delivery
Persist a versioned delivery record so normal failures stop after bounded retries
and only an unfinished crash window resumes on restart.
Reuse the persisted identity across Vela and Relay transport, isolate stable
direct Langfuse event ids by delivery purpose, and log only redacted sink data.
Validated with focused daemon and contracts tests, workspace typechecks, and
guard. The full daemon suite had one timeout; its four-test file passed alone.
* feat(plugins): add OD Next strategy scenario and contracts
Ship the V2 strategy package as inactive bundled content and validate its contracts without changing ordinary plugin manifest compatibility.
Keep bundled strategy bytes out of the catalog and generic apply path until a later hash-gated activation owner binds them.
* feat(observability): add normalized agent observation contract
Define a pure versioned contract for identity, Prompt, usage, timing, and status evidence with explicit availability and per-value provenance.
Add a structured main-Run compatibility adapter that preserves unknown facts, separates clocks, and omits provenance-ambiguous derived totals without changing legacy export behavior.
* feat(plugins): bind OD Next strategy content identity
* feat(daemon): persist OD Next strategy task chains
Add the daemon-owned SQLite truth for immutable strategy task identity, ordered physical Run mappings, canonical Plan Contracts, revision-CAS transitions, and terminal reconciliation.
Pin task-owned Snapshots and exclude their durable references from pruning so mixed GC sweeps remain safe.
Validated with 103 focused daemon tests, contracts tests/typecheck, daemon and workspace typechecks, and guard.
* feat(daemon): gate OD Next runtime capabilities by fixtures
Add exact CLI/adapter/fixture tuple resolution, stable capability snapshots, and offline
replay graph invariants for the four initial runtime paths.
Keep production registry empty until sanitized real fixtures exist; synthetic evidence and
version drift remain unknown.
Validated with contracts tests, focused daemon and e2e boundary suites, guard, and
workspace typecheck.
* feat(daemon): add hash-gated OD Next prompt recipe
Compose the bundled strategy from validated content, one Task Skill, and the exact stage policy.
Fail closed on package, profile, pipeline, or atom drift while preserving ordinary quality paths.
Validated with focused tests, package typechecks, workspace typecheck, and guard.
* feat(observability): aggregate OD Next task observations
Build stable task, Run, Child, model, and tool hierarchies from durable StrategyTaskExecution mappings and normalized facts while keeping missing coverage explicit.
Map task aggregates to legacy ingestion behind the consent gate. Preserve owner/exclude hierarchy while accounting usage once, enforce kind-specific Prompt boundaries, and accept absolute time only from one Unix-epoch evidence record.
Validated with 228 focused daemon tests, 353 contracts tests, daemon/contracts and workspace typechecks, guard, and the task harness.
* feat(daemon): collect bounded Codex child evidence
Read declared Codex rollout sessions without scanning arbitrary homes.
Normalize verified child facts while redacting prompts and preserving Run semantics.
* feat(daemon): add OD Next planning coordinator
Add a daemon-owned planning reducer, deterministic Resolver and Preflight facts, and a stream-safe machine protocol boundary backed by the durable task store.
Lock one clarification and one serialization-only repair across restart, preserve the Plan hash, and keep ordinary Run text unchanged.
* feat(daemon): collect native OpenCode child evidence
* feat(observability): add Langfuse OTLP task exporter
Map normalized task observations to complete OTLP/JSON v4 spans with stable parent context, allowlisted metadata, usage accounting, and bounded terminal delivery.
Fail closed on drift across root and operation input, model, timing, type, hierarchy, status, and complete usage provenance while dual remains a local-only OTLP shadow.
Keep legacy rollback, consent gates, and X5 live self-host verification boundaries intact.
* feat(daemon): normalize Claude native child evidence
Collect matched Claude Task sidechain lifecycle facts without changing main stream turn, stdin, or error bookkeeping, and adapt them to provider-neutral observations with unavailable Prompt and usage evidence.
Bind session, Task id, and parent identity immutably; poison rebinding, session drift, and conflicting terminals so ambiguous evidence cannot reach L2. Keep production capability unknown until X1 supplies an exact-version sanitized real fixture.
* feat(daemon): consume versioned Vela child evidence
Negotiate schema v1 only on AMR and enforce run-scoped start-to-terminal graph, status/source coherence, and exact usage source/completeness bindings. Emit allowlisted Normalized child facts without Vela private I/O.
Pin the unpublished producer golden for offline compatibility while keeping production capability unknown until the X1/X3 evidence gates are satisfied.
Validated with 147 focused daemon tests, 353 contract tests, daemon/workspace typechecks, guard, and the task harness.
* feat(daemon): automate OD Next simple production
Connect parsed OD Next terminal output to the durable coordinator, claim clarification, repair, and production Runs atomically, and start continuations only after the source Run ends.
Converge task failure and cancellation before physical end, require daemon-owned execution facts and canonical delivery, and keep ordinary Runs compatible.
Validated with focused daemon route and coordinator tests, contracts tests, daemon and workspace typechecks, guard, and the task harness.
* feat(daemon): enforce verified complex production
* feat(web): follow OD Next task chains across Web and CLI
Persist daemon-issued task handles and visible predecessor-prefix boundaries, then follow projected active Runs through one terminal outcome in Web and od run.
Normalize reloads to daemon activeRunId before subscribing, transfer controller ownership atomically across successors, discard stale cursors, and preserve visible prefixes without replaying predecessor machine payloads.
Validated with focused Web and daemon tests, contracts tests, workspace typecheck, guard, diff checks, and the task harness.
* feat(daemon): roll out OD Next task observability
* feat(daemon): add controlled OD Next rollout
* fix(daemon): align OpenCode and Vela evidence tuples
Pass GPT-5.6 Sol's verified high variant to OpenCode, pin native evidence to 1.18.18, retain locally recorded success and failure seeds as explicitly unattested, and align the Vela consumer golden to the local versioned producer.
Validated with 5 focused daemon files (175 tests), daemon and workspace typechecks, guard, the real local Vela ACP Sol smoke, and a minimal test-relay telemetry write.
* fix(daemon): honor OpenCode model variant capabilities
Parse the live verbose OpenCode catalog per model and forward only variants advertised by the exact detected binary. Keep fallback and one-off OPENCODE_BIN connection tests on the base model when variant support is unknown.
Record machine-checkable OpenCode 1.18.18 root, tool, child, recovery, cancel, timeout, and resume evidence without promoting the unattested seed into the production capability registry.
Validated with focused runtime and connection tests, host process-group and watchdog tests, contracts tests, daemon/workspace typechecks, guard, and an isolated SIGKILL delivery-recovery exercise.
* feat(daemon): add OD Next analytics operations report
Reduce normalized task aggregates into source-aware operations metrics without
copying the Langfuse dashboard or exposing user trace links.
Preserve main/Child accounting, stage-scoped coverage, unavailable facts, and
external-signal-only quality inputs.
Validated with focused and contract tests, the full daemon suite, guard,
workspace typecheck, and exact-trace Langfuse readback.
* feat(observability): complete child prompt and runtime provenance
Export bounded redacted child-injected Prompt content and exact CLI, companion, and
adapter versions through task-level legacy and OTLP mappings.
Register the OpenCode 1.18.18 best-effort replay tuple while keeping unknown versions
and incomplete Codex evidence fail closed.
Validated with 353 contracts tests, 196 focused daemon tests, daemon/workspace
typechecks, and guard.
* fix(daemon): recognize Codex child failure terminals
Parse Codex 0.147.0 Child task_complete.error as the runtime-owned failed terminal, honor session_meta id over its legacy root session_id, and map interrupt_agent aborts to canceled.
Add a redacted seven-path best-effort replay and exact Codex capability tuple. Unknown versions remain fail-closed and ordinary Codex runs are unchanged.
Validated with 92 focused daemon tests, 353 contracts tests, daemon and workspace typechecks, guard, and the 25-unit task harness.
* fix(daemon): report verified child runtime versions
Use versions observed from resolved CLI probes or Vela's ACP handshake when annotating Child observations. Correct Native OpenCode's allowlist key, add Claude and Vela/OpenCode companion provenance, reject Vela probe/handshake disagreement, and ignore the legacy 0.0.0 placeholder.
Validated with 252 focused daemon tests, daemon and workspace typechecks, guard, the task harness, and a real local Vela --version/-v/ACP initialize build smoke.
* test(observability): record Vela child evidence matrix
Add a sanitized provider-backed Terra replay for main, tool, child success, child failure with parent recovery, cancel, timeout, and cross-process resume. Keep the unpublished Vela tuple outside the production registry while pinning its exact candidate and OpenCode companion evidence.
Validated with focused Vela consumer/capability tests, daemon typecheck, workspace typecheck, guard, and the task harness.
* feat(daemon): enable OD Next production profiles
Make prototype, presentation, marketing, and HyperFrames the exact global OD Next production set while preserving explicit off/observe, user plugin authority, and fail-closed runtime capability gates. Feed runtime-owned profile, route, output, input, and capability facts into the planning contract and keep all other artifact kinds on ordinary Open Design.
Use bounded executable-scoped CLI version probes so headless and restarted daemons can resolve exact tuples without a prior agent-list request or cross-binary cache contamination.
Validated with four provider-backed Terra CLI chains, focused daemon/contracts/plugin-runtime suites, daemon and workspace typechecks, guard, and the 25-unit task harness.
* feat(daemon): classify OD Next exact agent inputs
Route prompt contributors through one daemon-owned choke point and version their final-text, semantic-fact, transport, and exclusion ownership for the forthcoming canonical bundle. Send OD Next continuation stages directly through native sessions without rewrapping or re-seeding stable instructions.
Validated with focused daemon and contracts tests, daemon/workspace typechecks, and guard.
* fix(daemon): close OD Next input ownership gaps
Bind semantic request facts to their real daemon-system or request producers, cover the DSH profile exact-text transport, and reject lookalike prompt-bundle root names.
Validated with focused exact-input, production-server and DSH tests, daemon typecheck, task harness, and guard.
* feat(daemon): emit canonical OD Next prompt bundles
Add strict pure TypeScript Bundle and request-Turn serializers, then route OD Next request and continuation text through their versioned XML contracts.
Split Web prior transcript from the current turn without text subtraction, keep mutable Skill and attachment inputs explicitly pending, and preserve ordinary prompt behavior.
Validated with focused contracts, daemon and web tests, the real server fixture, package and workspace typechecks, and guard.
* fix(daemon): align OD Next bundle input ownership
Register the real canonical Bundle contributors and keep current-turn, prior-transcript, pending-fact, and research-query ownership explicit at the production seam.
Reject continuation task_run_index zero and cover Web framing, ordinary compatibility, fail-closed contributor drift, and real server prompt capture.
* feat(daemon): freeze OD Next user skills
Capture explicit Skill bodies and audited side files before task claim, persist verified task-owned bytes, and replay only the frozen package for OD Next runs.
Keep legacy single-Skill CLI requests and ordinary live staging compatible while adding Web/CLI multi-Skill coverage and fail-closed snapshot checks. Validated with focused server, contract, CLI, Web, typecheck, and guard suites.
* fix(daemon): harden OD Next skill snapshots
Require an explicit frozen package for every mapped OD Next task and fail corrupt task state before any live Skill staging.
Use bounded no-follow descriptor reads with race checks, preserve legacy CLI compatibility, and make materialized roots collision-resistant. Validated with focused, real-server, workspace typecheck, and guard suites.
* feat(daemon): freeze OD Next task input snapshots
Freeze OD Next attachments and canonical configuration before the atomic run/task claim, then route runtime access through durable task-scoped snapshots while preserving ordinary attachment behavior.
Validate immutable identities, transport references, continuation propagation, tamper failure, and the four production task types with focused contract and daemon coverage.
* fix(daemon): isolate OD Next runtime input projections
Keep canonical task snapshots daemon-only, rebuild a read-only per-Run projection, and deliver frozen document resources through ACP or OD_TASK_INPUT_DIR.
Harden source and snapshot reads with no-follow same-fd identity checks and bounded caps. Validated with focused runtime tests, contracts, daemon/workspace typechecks, guard, and the task harness.
* fix(daemon): harden OD Next snapshot transport boundaries
Reject intermediate symlinks and realpath escapes while loading canonical snapshot files, including identical-byte replacement targets.
Preserve ordinary ACP duplicate image ordering while suppressing only OD Next resource/image cross-duplicates. Validated with focused tests, daemon typecheck, guard, and the task harness.
* fix(daemon): preserve structured OD Next snapshot failures
Defer frozen Skill package errors from the Task 04 preclaim lookup to the existing post-claim failure boundary so mapped Runs remain durable JSON failures without orphan input snapshots.
Validated with the combined real-server repair, missing-package, and tampered-package regressions.
* fix(daemon): keep OD Next continuation cwd stable
Separate authoritative project cwd resolution from best-effort file inventory and tolerate entries deleted between directory enumeration and stat.
This keeps native-session identity stable while frozen task inputs replace live attachments during continuation.
* feat(daemon): persist exact OD Next prompt inputs
Persist the canonical first-turn Bundle and every deterministic continuation Turn with UTF-8 and SHA-256 identities, then send only those validated texts through runtime transports.
Fail closed on mapped task drift, tamper, legacy NULL identities, missing mappings, and cold continuation while leaving ordinary Runs on the existing live composer.
Validated with the 15-case automatic server acceptance twice, 223 focused daemon tests, daemon typecheck, guard, and the task harness.
* feat(daemon): observe exact OD Next prompt inputs
* feat(daemon): bind and observe Claude native subagents
Register the replay-verified Claude 2.1.233 tuple, bind complex Build Packages to daemon-issued native Agent handles, and derive completion only from durable child lifecycle evidence.
Export bounded parent-tool, Child Agent, and Child-tool spans through the existing legacy and OTLP task hierarchy without exposing raw tool inputs, outputs, provider ids, paths, or credentials.
Validated with focused contracts and daemon suites, the 17-case real-server chain, workspace typecheck, guard, and the 26-unit task harness.
* fix(daemon): repair OD Next rollout authority and controls
* fix(daemon): harden OD Next public canaries
* refactor(daemon): extract OD Next prompt preparation
* test(daemon): align plugin fixture with project schema
* test(e2e): assert daemon-owned prototype routing
* fix(daemon): restore OD Next task observability
Default eligible OD Next tasks to privacy-aware Langfuse hierarchy delivery through relay or direct sinks, with a versioned safe quality projection.
Persist lightweight representation ownership before suppressing mapped Runs, retain best-effort startup retry semantics, and preserve single-Run compatibility when Task delivery is not eligible.
Adopt the active payload relay for historical packaged launchers and cover the real sidecar environment handoff.
Validated with the full daemon suite, focused contracts/packaged/e2e/tools-pack suites, workspace guard, and workspace typecheck.
* fix(daemon): make OD Next capability fallback invisible
Admit capability-certified runtime adapters across CLI version drift while keeping recorded fixture versions as auditable evidence and reserving a narrow incompatibility denylist.
Fall back before Run claim on preparation failures, keep continuation bound to the frozen capability snapshot, and report truthful child coverage across Langfuse legacy and OTLP without replaying an active task.
Validated with focused contracts and daemon suites, the automatic server suite, pnpm guard, pnpm typecheck, daemon typecheck, LSP diagnostics, and git diff --check.
* fix(strategy): isolate OD Next automatic task routing
* fix(daemon): persist blocked strategy-task attribution
Every blocked strategy task now stores its gate reason codes and the
agent-visible turn text (blocked_reason_codes_json / blocked_visible_text)
so operators can diagnose a blocked task from the store alone. Covers the
coordinator blockTask gate, the automatic-continuation fail-closed path,
and startup run-terminal reconciliation.
* fix(daemon): accept form-only clarification turns by deterministic inference
A first Full-Plan request turn that renders exactly one question form and no
machine-protocol block has exactly one valid protocol meaning: outcome
clarification_required with an unlocked execution mode. The coordinator now
recovers that state deterministically instead of dead-ending the task as
blocked, stamps od_next_protocol_runtime_state_inferred into the accepted
state's reasonCodes for attribution, and keeps every ambiguous shape (extra
forms, recovered plan blocks, later stages, spent clarification budget,
any other protocol issue) fail-closed. This closes the observed field
failure where the agent's first reply produced a discovery form, the task
went blocked, and the user's form answer then failed with 409
STRATEGY_TASK_STATE_MISMATCH.
* feat(contracts,daemon): project blocked strategy-task attribution to clients
A task the OD Next protocol gate settles as blocked persists its
attribution (reasonCodes + agent-visible text) since
|
||
|
|
e34d823160 |
fix(web): gate PPTX export on a daemon-advertised renderer capability (#7224)
* fix(web): gate PPTX export on a daemon-advertised renderer capability
The export entry was offered whenever the artifact looked like a deck, so
in a deployment without an off-screen renderer (headless / container) the
button was always shown and always failed: `POST /export/pptx` answers 501
whenever `desktopSlideRenderer` is absent, and the check sits *before* the
editable/screenshot branch, so the mode picker in the modal was a choice
that could not affect the outcome either.
Ask the daemon instead of guessing. `/api/version` now carries an optional
`capabilities.slideRenderer`, computed from the very binding the export
routes guard on, so the advertisement cannot drift from the 501 they
return. It rides on the version payload because that is already the
"what runtime am I talking to" channel (packaged/channel/platform/arch),
already fetched once at boot, and — unlike `/api/app-config` — never
persisted: the same data dir can be opened by a desktop daemon today and a
headless one tomorrow, so a stored answer would be a stale claim.
The flag deliberately cannot be derived from `packaged`: a packaged binary
started as a plain daemon has no renderer.
Mixed versions are the delicate part. The field is optional and the client
treats absent as *unknown*, not false, so a newer web bundle keeps showing
PPTX against an older daemon that supports it but does not advertise it
yet. Only an explicit `false` hides the entry. A present-but-malformed
block is dropped rather than trusted, and an unresolved answer stays
retryable so one early fetch against a not-yet-ready daemon cannot pin
"unknown" for the session.
Client-side `isOpenDesignHostAvailable()` was not used for this even though
it is the existing desktop-only gate: it reports whether the web UI runs
inside the Electron shell, which is wrong in both directions here — a
browser pointed at a desktop-hosted daemon does have a renderer, and a
packaged daemon may not.
Also splits the two states that `unavailable: true` collapsed. A dead
daemon and a runtime with no renderer both produced "PPTX export is not
available here", which sends someone whose daemon is down to look at the
wrong problem. Callers keep the existing `'unavailable' in res` fallback
behaviour; only the message differs.
* test(web): pin the two unavailable reasons in the export specs
The three specs that asserted the old flat `{ ok: false, unavailable: true }`
now pin which reason they exercise: the two 501 cases as `no-renderer` and
the transport failure as `unreachable`. Without this the split is only
enforced by types, so a later edit could collapse both back onto one value
and every assertion would still pass.
* fix(web): let a late capability answer reach an already-mounted viewer
Review catch (thanks @nettee): the first shape probed once on mount and
ignored an unresolved result, so a probe that raced daemon startup or
blipped left `available` at null for the life of that mount. The gate then
stayed permissive and the entry could still be clicked into the very 501
this change exists to prevent. Worse, the module raced the app's own boot
fetch with a second request, so a failed probe here could keep the entry
showing even when the boot response had already reported `false`.
Three changes, all aimed at that gap:
- The boot fetch now feeds the cache. `fetchAppVersionInfo()` hands its
parsed payload to `recordAppVersionInfo()`, so the common path costs no
extra request and the two paths cannot disagree. The dependency runs
registry -> runtime-capabilities only, so there is no cycle; this module
no longer imports the registry and does its own minimal probe when it
still has to.
- Consumers subscribe. An answer arriving from boot, from a retry, or from
another mounted consumer now reaches every mounted viewer instead of
leaving each one pinned on its own failed probe.
- Unresolved probes retry with a bounded backoff (500ms / 1.5s / 4s) and
then stop. An unreachable daemon is its own visible problem and should
not be polled forever; a startup race is exactly what needs one more look.
Covered at the level the review asked for: a FileViewer spec where the
first probe cannot resolve, the entry is still offered (unknown means
assume available), and a later explicit `false` removes it. Verified red by
restoring the old single-shot behaviour — the late answer reaches nobody
and the entry stays.
* fix(web): recognise proxy-answered daemon outages as unreachable
Review catch (thanks @nettee): the daemon-down diagnostic did not actually
fire on the packaged / sidecar path, which is the common one. The proxies
in front of the daemon do not reject `fetch` when it is down — they answer.
The web sidecar ends a connection error as a plain-text 502, and Next's dev
rewrite as a plain-text 500 when OD_PORT points at a dead port. Both landed
in the generic `!resp.ok` branch and surfaced as "export request failed
(502)", so the new message this PR added never appeared where it was most
needed.
`state/projects.ts` already had exactly the right detector for this, kept
private. Extracted it to `runtime/daemon-proxy-failure.ts` and used it from
both places rather than growing a second copy.
Kept deliberately narrow: only a 500/502 that is `text/plain` AND carries a
connection errno counts. Widening to any 5xx would relabel genuine upstream
failures as an outage — the same misleading move in the opposite direction,
which is why the original detector was written this way. A JSON 502 stays a
semantic error, and there is a spec pinning that.
* fix(web): honour the unknown contract on both capability cache edges
Two compatibility edges from review (thanks @nettee), both of which broke
the same rule this PR is built on: absent or malformed means unknown, and
unknown must keep the entry visible.
- A resolved `false` outlived the daemon that gave it. If a page cached
`false` from a headless daemon and then talked to one that stops
advertising the field — a restart or a downgrade — the stale `false` kept
hiding the entry against a daemon we are supposed to know nothing about.
A fresh, valid response with no capability now drops the cache back to
unknown and tells mounted consumers, so listeners take `boolean | null`.
- The module's own probe trusted the capability without validating the
envelope it arrived in, so `{version:{capabilities:{slideRenderer:false}}}`
could publish `false` and hide the entry. That was self-inflicted: the
validation went missing when this module stopped importing the registry to
avoid a cycle, and nothing replaced it.
`isAppVersionInfo` now lives here and the registry imports it, so there is
one validator instead of two and the dependency still runs one way only.
Regression specs for both: `false` followed by an absent field returns to
unknown, and a capability in a malformed envelope is refused.
* fix(web): subscribe mounted viewers even when the cache is already resolved
Follow-on from the invalidation added in the previous commit, and the same
mistake one layer out: a viewer that mounted while the answer was already
`false` took the early-return path and never registered a listener, so when
a later valid response omitted the capability and invalidated the cache to
unknown, that viewer never heard it. Its entry stayed hidden against a
daemon the compatibility contract says we know nothing about — exactly the
bug the invalidation was added to prevent, just moved from the cache to the
consumer.
Subscribe unconditionally and only skip the probe when the cache is already
resolved. Spec covers the mounted case the loader-level tests could not:
render with `false` cached and the entry absent, invalidate, entry returns.
Verified red by restoring the early return.
* refactor(web): collapse the capability store into one atom with one writer
Four review rounds found six compatibility edges in this module, all the
same shape: `cached`, `inFlight` and the listener set were separate mutable
pieces, and each fix exposed another pair that could disagree — an
invalidation that forgot the in-flight promise, a validation path that
forgot the invalidation, a subscription that forgot to register. Patching
the seventh instance was not going to be different from the sixth.
So the state is now one snapshot behind one writer. `setSnapshot` is the
only thing that mutates it and always drops any in-flight probe, because a
promise created under a superseded answer can only report something this
state has already replaced. `loadSlideRendererAvailable` clears the promise
on settle rather than only on success, so an unanswered probe stays
retryable. Consumers read through `useSyncExternalStore`, so every mounted
viewer sees the same value including a later invalidation — there is no
longer a subscribe-or-not branch to get wrong.
That makes the two edges from this round unrepresentable rather than merely
fixed: a settled probe cannot be served after an invalidation, and a
malformed payload now routes through the same invalidation path as an
absent capability instead of early-returning past it.
One deliberate asymmetry: a transport failure does not invalidate. It is
not an answer, unlike a malformed body, so it must not erase what we know.
Covered by its own spec.
* fix(web): version probes and treat an unusable answer as unknown
Blocking catch (thanks @nettee): clearing `inFlight` stopped a stale promise
being *served*, but not a request already in the air from writing when it
lands. A probe issued before the boot fetch could resume afterwards and put
its `true` back over the boot answer's `false` — the entry returns on a
headless daemon and the next export hits the 501 this change exists to
gate. That was the half of "collapse the state" I had not done: the writes
were unified, but time was not.
Each probe now carries the generation it was issued under and drops its
result once a write has superseded it, so a request from the old world
cannot write into the new one.
Second, transport and parse failures are no longer the same branch. A 200
whose body will not parse IS an answer, just an unusable one, and the
contract says that means unknown — collapsing it with a dead socket let a
stale `false` survive it and keep hiding the entry. Both the boot fetch and
the direct probe now invalidate on an unparseable OK response while leaving
a known answer alone on a transport failure.
Specs: a deferred probe that resolves after a `false` is recorded, an
unparseable 200 through the boot path, and a transport failure that must
not erase what we know. The first is verified red by removing the guard.
Also replaced a spec that passed without exercising its path — a cached
answer short-circuits the direct probe, so the parse-failure case only
reaches the code through the boot fetch.
* refactor(web): read the capability from app state instead of a client store
Seven review rounds produced eleven findings, and ten of them were in the
client-side capability store rather than in what this PR is actually about.
The pattern was not bad luck: each fix added a variable to reconcile the
existing ones (`cached` -> `+inFlight` -> `+listeners` -> `+generation`),
which widened the interaction surface that produced the next finding. The
latest pair — a parsed JSON `null` throwing past the invalidation, and a
probe's `finally` clearing a newer probe's promise — are both interactions
between those variables, not defects in the gate.
The store existed to avoid prop-drilling a boolean that `App` already holds
in React state from its boot `/api/version` fetch. That trade bought a
tidier diff and paid for it with cache coherence, staleness, concurrency,
retry and subscription lifecycle — a distributed-state problem invented for
a value that was never distributed.
So the store is gone. `App` passes `appVersionInfo?.capabilities
?.slideRenderer ?? null` down the same path `metricsConsent` already takes,
and the gate reads a prop. No cache, no probe, no retry, no generations, no
subscription: every failure mode those rounds found is now absent rather
than fixed.
The three cases the contract cares about — `false` hides, `true` offers,
`null` (unknown) offers — are three renders in `FileViewer.test.tsx`. The
specs that exercised probe lifecycle went with the probe; they were testing
a mechanism, and the mechanism is what turned out to be wrong.
* fix(web): close the two paths that could still reach the 501
Both blocking notes from review, and both are consequences of how I threaded
the capability rather than of the gate itself.
- The design-system detail route mounts `FileWorkspace` directly, so it never
received the prop and `FileViewer` fell back to its `null` default. Since
unknown deliberately fails open, a headless daemon kept offering PPTX for
`system/artifacts/deck.html`. Threaded from the same App-level value the
project route uses; one source, two callers, rather than a second mechanism
for the second route.
- `appVersionInfo` came only from the one-shot boot fetch, and that helper
yields null on any failed or non-OK response. A daemon merely slow to come
up therefore left the capability unknown for the life of the page — and
unknown fails open, so the gate was defeated by a startup blip. It is now
re-asked once the daemon is reachable, which is the narrowest retry that
fixes it: keyed on the daemon becoming live rather than on a timer, and it
stops as soon as an answer arrives.
* fix(web): make the capability refetch schedule its own retry
Third incarnation of the same mistake, so worth naming: retrying on a
dependency change cannot recover from a failed probe, because a failure
changes nothing — `appVersionInfo` stays null, `daemonLive` was already
true, neither dependency moves, and no further attempt is ever issued. The
failure is sticky, `slideRendererAvailable` stays unknown, and unknown
deliberately fails open, so a headless daemon keeps offering an export it
can only answer with 501.
The retry now schedules its own next attempt instead of waiting to be
re-triggered: bounded with backoff (500ms / 1.5s / 4s / 10s), stopping the
moment an answer arrives. Bounded because an unreachable daemon is its own
visible problem and must not be polled for the life of the page.
* fix(web): cap the capability retry interval instead of the attempt count
Closing the residual case I flagged in my own audit rather than waiting for
it to be found: with a bounded number of attempts, once they were spent
`daemonLive` never flips again and nothing could ever ask, so the capability
stayed unknown for the life of the page — and unknown deliberately fails
open. That is the same hole the retry was added to close, just with a
smaller window (four consecutive failures against a daemon that had already
health-checked as live).
I argued for bounding attempts to avoid polling for the life of the page. On
reflection that traded a rare slow poll for a permanent fail-open hole,
which is the worse side of the trade. The backoff now settles into a steady
30s interval instead of stopping: it only runs while the answer is unknown,
it stops on the first answer, and it leaves no state the page cannot recover
from.
* fix: forbid caching /api/version and stop the two writers clobbering
Both blocking notes, and the first is a consequence of the change itself
that I should have caught: adding a capability turned `/api/version` from a
near-static fact about the build into an answer that differs by which daemon
is behind the URL. It was still served with no `Cache-Control`, so a browser
or proxy could reuse a pre-swap response and a stale `slideRenderer: true`
would be consumed as authoritative — reopening the export the gate exists to
hide. Now `no-store`, with a spec pinning it.
Second, `appVersionInfo` had two unsynchronised writers: the boot pass and
the capability retry. A boot response that resolved `null` after the retry
had obtained an explicit `false` would erase it and reopen the fail-open
entry; a slow but valid boot response could likewise overwrite a newer
answer. The boot pass now never writes `null` and never overwrites an answer
that is already present — a late boot response is by definition the older of
the two.
* refactor(web): ask for the capability at the point of use
Fourteen findings on this PR, and nearly all of them asserted one property:
the capability must not get stale or stuck, because unknown fails open. That
is a global property, and global properties cannot be established by local
patches — which is why each fix produced the next finding rather than
ending the series.
The property is global because the gate was reading a client-side replica of
remote state. A replica needs an initial fetch, a retry, invalidation, write
ordering, cache directives, multi-writer synchronisation, distribution to
consumers, and a refresh when liveness changes. Every one of those was a
finding here.
Deleting `runtime-capabilities.ts` last time did not end it because it moved
the replica into `App` rather than removing it. This removes it: the export
surface asks the daemon when it opens, uses the answer, and discards it with
the menu. If the daemon is swapped, the next open simply asks again.
Gone with it: the prop threaded through four components, the boot-fetch
coupling, the retry with its backoff, and the two-writer synchronisation.
Kept: the daemon-side capability, `Cache-Control: no-store`, the optional
field tolerance, and the export-reason split.
The pending window still fails open, and that is deliberate — but it is one
round trip inside a single menu opening and nothing carries it forward, so
unlike the replica it cannot get stuck.
* fix(web): start each export-menu opening from an unknown capability
The probe state outlives the menu — the component stays mounted — so the
previous opening's answer was still present on reopen. If the daemon was
replaced in between, the gate decided on an answer describing a daemon that
is gone: a stale `true` offers an export the new daemon can only 501.
My previous commit message claimed the answer was "discarded with the menu".
It was not; only the fetch was tied to the opening. Now the state is reset
when the menu opens, so the claim and the code agree.
The regression spec is not in this commit, deliberately. My first attempt
passed with and without the fix — the second response landed too quickly for
the stale window to be observable, so it asserted nothing. The second
attempt, holding that response open, fails in both directions, which means
my assumptions about the close/reopen path are wrong rather than the code
being wrong. Shipping either would be worse than shipping neither, so the
fix goes in now and the spec follows once I understand that path.
* fix(web): revalidate at the menu-to-modal handoff and state the contract
The blocking finding: clicking the PPTX row closes the menu and opens the
modal in one batched update, and the modal's confirm reads the same
capability value — so a `true` fetched at menu-open outlived its surface and
kept the confirm enabled against a daemon that may since only answer 501.
Because the two booleans flip in one batch, an OR of them never changes
across the handoff; the effect now depends on the booleans themselves, so
the handoff resets to unknown and asks again.
The capability contract is now stated once, at the probe:
1. An answer never outlives the surface opening that fetched it — every
opening, including the handoff, starts from unknown and re-asks.
2. Unknown fails open; only a resolved `false` hides the entry.
3. The daemon is the authority of last resort: a swap after the probe
resolves is unclosable client-side, and is caught by the export POST's
501, which now surfaces the no-renderer message.
Tests: the two vacuous positive cases now hold the response open and assert
against the resolved answer, and the reopen regression I owed exists. Its
three earlier failures had one cause worth recording: a stale `true` is
observationally identical to a pending probe — both fail open — so the
true-then-swap direction asserts nothing. The observable stale direction is
a lingering `false` wrongly hiding the entry on reopen, and that is what
the spec pins. Red-check matrix: removing the reset turns the reopen spec
red; reverting the deps to the OR turns the handoff spec red; mapping an
absent field to false turns the absent spec red.
* fix(web): recognise EPIPE as a proxy connection failure; drop a stray file
Both follow-ups from review.
EPIPE belongs in the errno set: the sidecar's own replay check classifies
exactly ECONNRESET and EPIPE as connection-level, and when a replay is not
applicable the EPIPE surfaces as a plain-text 502 containing `write EPIPE` —
which fell through to the generic "export request failed (502)" instead of
the daemon-down message. The classifier also had no direct tests since its
earlier ones were deleted along with the capability store, so it now has a
dedicated spec pinning both halves of its contract: every connection shape
the proxies actually emit is recognised, and JSON 5xx / errno-less text /
other statuses are not. Removing EPIPE turns exactly its case red.
`__diag.test.tsx` should never have existed in the repo: it was a local
scratch file for probing menu toggle mechanics, swept in by a `git add -A`
in the previous commit. It logged instead of asserting and leaked its fetch
stub into the rest of the file. Deleted.
* fix(web): key the capability probe to the export tab, not the popover shell
The popover shell can open on Share and sit there across a daemon swap;
switching to Export then presented the PPTX entry against the answer from
shell-open time. The export surface's opening is the tab becoming visible,
not the shell opening.
This is the terminating fix for the transition family, and the reason can
be stated as an equivalence rather than a hope: the probe's guard is now
the same predicate the consumers render under — the PPTX row mounts inside
`unifiedActionTab === 'export'` within the open popover, the modal confirm
under `pptxExportModalOpen` — and the effect's dependencies are exactly the
free variables of that predicate. A transition into visibility is by
definition a change of one of those inputs, so it necessarily re-runs the
probe. "A transition we forgot" is no longer a findable category: the
guard cannot be coarser than the render condition, because it is the
render condition.
Regression: false from the first daemon, switch to Share, swap, switch
back to Export with the new answer held open — the entry must show
(unknown fails open) and a second probe must have fired. Reverting the
guard to the popover shell turns exactly that spec red.
* fix(web): retry a failed probe within the opening; stop at a fieldless one
A probe that fails (daemon still starting, transient network) left the
whole opening at unknown, and unknown fails open — so a headless runtime
kept the entry clickable for the lifetime of that opening. Retried now with
a short bounded backoff that is cancelled by any surface transition:
nothing crosses surfaces, so the contract's first clause still holds for
the retry itself.
The distinction that keeps this from over-polling: a FAILED probe is worth
asking again, but a probe that SUCCEEDS without the capability field is an
old daemon — asking again cannot teach us more — so it stops at unknown on
the first valid answer. The two cases were previously collapsed into one
`null`, which is exactly why the earlier background-copy design kept
tripping over them; here they are separated at the only point that knows
the difference (did `fetchAppVersionInfo` return a payload or not).
Regression: first probe answers 500, the entry stays visible (unknown
fails open), the in-opening retry fires, and its held `false` then hides
the entry. Reverting to a single-shot probe turns exactly that spec red.
---------
Co-authored-by: Jason <>
|
||
|
|
e13a4c385f |
fix(daemon): hide ACP metadata updates from transcript (#7309)
Co-authored-by: helson <helson@helsondeMac-mini.local> |
||
|
lefarcenandGitHub
|
7a4175c86f |
feat(analytics): report why an installed agent CLI cannot be used (#7285)
* feat(analytics): report why an installed agent CLI cannot be used A user installed DeepSeek Harness correctly and OpenDesign showed nothing. We only learned about it because they said so and sent a diagnostics zip: detection is the one stage that knows an installed CLI is unusable, and it told nobody. There is no way today to ask how many installs are in that state, or which CLI, or why. Report each diagnostic detection produces, once per (agent, reason, version) per page session. Detection runs on cold start and on every rescan, so counting passes would turn one broken install into dozens of events; deduped, the number means "installs affected", which is the denominator a decision here actually needs. Warnings count too — an `untested-version` spike across many installs is how we would learn our supported-version list had fallen behind our own installer, which is precisely what went unnoticed. The resolved path is deliberately absent: an agent binary path contains the OS username. `has_path` carries the only part that matters, since the picker hides a row that has none. Half the registry was invisible to this before it started. `TrackingCliProviderId` covered 14 ids while the daemon ships 28, so DeepSeek Harness and a dozen others collapsed into `other` — a breakdown that cannot name the failing CLI answers nothing. Every shipped agent now has an id, and a test fails when a new runtime def arrives without one. * test(runtimes): assert the shipped agents, not the machine's The tracking-id guard walked AGENT_DEFS, which appends whatever local profiles the developer declared in their own agents.local.json. Those ids are required not to collide with a shipped one, so they correctly collapse to `other` — and turned anyone's personal config into a failure reading `expected [ 'my-custom-agent' ] to deeply equal []`, indistinguishable from the missing-mapper regression the guard exists to catch. Export the shipped list under a name that says which question it answers, and assert on that. The unknown-id case stays: a locally declared agent is not ours to name, and PostHog should never learn an id out of someone's config file. * test(runtimes): pin the local-profile case in the repo, not in my shell The isolation fix was verified by pointing OD_AGENT_PROFILES_CONFIG at a throwaway file by hand, which proves it once and protects nothing. Build the profile inside the test instead, so the machine-dependent failure the guard used to produce cannot come back unnoticed. Registry lists are built at module load, so the profile is written before a fresh dynamic import. The case asserts the profile actually reached AGENT_DEFS before asserting it stayed out of SHIPPED_AGENT_DEFS — without that, it would pass just as happily if the profile never loaded at all. |
||
|
lefarcenandGitHub
|
35a38abf34 |
fix(daemon): admit one design-system enrichment run per conversation (#7271)
A double-triggered "AI Optimize" affordance created two concurrent design-system enrichment runs 383 ms apart in one conversation and both were billed (2026-07-28 incident). Reject the newcomer on the daemon with HTTP 409 DESIGN_SYSTEM_ENRICHMENT_IN_PROGRESS (details name the owning run), keep ordinary chat turns ungated, and make the web trigger a synchronous single-flight call so the duplicate never leaves the client. |
||
|
mehmet turacandGitHub
|
a588cefd2b |
test(daemon): add regression tests for headerless /raw/ reads of workspace-bound projects (#7206)
* test(daemon): add regression tests for headerless raw reads of workspace-bound projects Closes #7072. The /raw/ and /preview/ routes call authorizeProjectRequest with { mode: 'read', allowNavigationQuery: true }. Before the enforceLocalProjectDataPlaneRequest refactor, these routes delegated to enforceVerifiedWorkspaceResourceRead, which required an explicit workspace context for any workspace-bound project and returned WORKSPACE_CONTEXT_REQUIRED (400) when headers were absent — breaking local/desktop preview iframes and MCP preview URLs for projects with a personal workspace_projects binding. The two new test cases pin the corrected behaviour: a headerless request to authorizeProjectRequest with allowNavigationQuery: true must succeed (return true, not call any remote verifier) for both personal and team workspace-bound projects, matching exactly what an iframe src or a bare loopback fetch sends. * chore: retrigger CI after flaky workspace-restoration P0 * chore: retrigger CI after repeated workspace-restoration flake |
||
|
open-design-release-bot[bot]GitHubopen-design-release-bot[bot] <open-design-release-bot[bot]@users.noreply.github.com> |
05f5b33ef5 |
chore(release): bump main to 0.20.3 ahead of stable 0.20.2 (#7253)
Co-authored-by: open-design-release-bot[bot] <open-design-release-bot[bot]@users.noreply.github.com> |
||
|
Alex LuceroandGitHub
|
9caec8056d | Normalize agent resume fallback policy (#5269) | ||
|
lefarcenandGitHub
|
c9eb23027f |
fix(viewer): preserve live HTML previews (#7156)
* fix(viewer): preserve live HTML previews * fix(viewer): sync saved text to retained preview * fix(viewer): prewarm retained file revisions * fix(viewer): expire saved preview adoption * fix(viewer): repaint retained previews on activation * fix(viewer): reconcile retained style history * fix(viewer): renew preview asset scopes without reloads * test(viewer): cover scope renewal in team previews * test(exports): expect managed preview bases |
||
|
open-design-release-bot[bot]GitHubopen-design-release-bot[bot] <open-design-release-bot[bot]@users.noreply.github.com> |
8b18210103 |
chore(release): bump main to 0.20.2 ahead of stable 0.20.1 (#7201)
Co-authored-by: open-design-release-bot[bot] <open-design-release-bot[bot]@users.noreply.github.com> |
||
|
lefarcenandGitHub
|
a81d114b1d |
fix(daemon): anchor run phases on first model event (#7155)
* fix(daemon): anchor run phases on first model event Phase boundaries anchored on `firstTokenAt`, the first *text* token. A run that opens with a tool call and stays silent until the end therefore had its entire tool loop billed to `runtime_init`, leaving the generation phase holding only the closing sentence. Tool-first is the dominant opening shape for the OpenCode/OpenAI families, so `bottleneck_phase` reported startup for runs that were working the whole time. The decomposition was also not a partition: on the new fixture's 24.2s run the old boundaries sum to 37.8s, because the tool loop is counted inside both `runtime_init` and `tool_execution`. Anchor phases on `telemetry.firstModelEventAt ?? telemetry.firstTokenAt` -- tracer marks only. Deliberately not the nearby `firstModelEventAt` composite, whose `firstToolUseAt` fallback is scanned off event records on the daemon's clock and is never reset per retry attempt. Every published metric keeps its current value; a spec pins the four first-token metrics so a future edit cannot silently redefine them. Adds `runtime_init_to_first_model_event_ms`, `model_active_duration_ms`, and `phase_schema_version` so old and new rows can be filtered apart. The `stream_output` phase now subtracts tool time to stay mutually exclusive with `tool_execution`. Trace side gains a `model-active` phase-diagnostics entry rather than a second span, to avoid an extra observation row per run. * fix(daemon): measure the tool phase as wall-clock occupancy Review found two ways the tool phase could exceed the window it sits inside, both because it was built from `tool_duration_ms` -- the sum of paired tool_use -> tool_result spans. That sum is the right published answer to "how much tool work happened" but it is not elapsed time, so it cannot partition a window: - Parallel tool calls sum to more than the clock they occupy. Two overlapping 4s and 5s tools inside a 6s active window reported 9s of tool execution. - `run.events` is never cleared between retry attempts, while the phase anchor comes from the current attempt's lifecycle marks. A retried run charged the previous attempt's tool spans to this attempt's window. - ACP producers supply their own `startedAt`, which can predate the anchor when their clock differs from ours. In each case the inflated phase won `bottleneck_phase` with a duration longer than the run had been active, and the previous `Math.max(0, ...)` floor on the residual hid the symptom rather than fixing the partition. Derive both `stream_output` and `tool_execution` from the union of tool intervals clipped to [phaseAnchorAt, runEndAt]. The two are then halves of the model-active window by construction. `tool_duration_ms` keeps summing paired spans and is unchanged. Three fixtures cover the parallel, retry, and producer-clock cases; all three reported `tool_execution` as the bottleneck before this change. * fix(daemon): keep the phase anchor on real model output Second review round found three more ways the new boundaries could publish wrong attribution. A daemon-persisted artifact was being marked as the first model event. Agent `artifact` events are emitted from exactly one place -- the daemon's close-time persistence of plain-stream stdout -- and never by a runtime relaying model output. On a plain-stream run that stamps first-token from its first buffered stdout chunk, that late mark won the anchor and reported a 1s active window and a 16s runtime init for a run that had been streaming since second 5. That is the opposite of this PR's intent, and it falsified the claim that the plain/BYOK family keeps its current behaviour through the first-token fallback: the fallback was never reached, because the field was set -- just set by the daemon, at the end of the run. Fixed at the source (the tracer no longer treats an artifact event as a model event, and neither does the scan-based fallback that feeds `first_model_event_type`), and again as an invariant: the anchor is the EARLIEST of `firstModelEventAt` and `firstTokenAt`, because the model cannot have started responding after it emitted its first token. A late producer mark can no longer drag the boundaries to the end of the run. `phase_timing_status` still required `firstTokenAt`, so a fully instrumented tool-only turn -- every boundary the phases are measured from present, no text ever emitted -- was stamped `partial` and dropped by dashboards filtering on complete timings. It now follows the anchor the phases actually use. The first-token fields are unchanged. Tool occupancy ignored a tool still outstanding at run end, handing the whole active window to `stream_output` while `last_observed_phase` said `tool_execution`. Outstanding spans are now closed at run end for phase purposes; `tool_duration_ms` still sums completed pairs only. Five fixtures, red before the change, including one in the tracer suite pinning that an artifact event is not a model event. * fix(daemon): scope outstanding tool spans to the current attempt Closing every still-open tool span at run end interacted badly with the retry path. `run.events` survives a retry while lifecycle telemetry does not, so an attempt whose child was killed mid-tool leaves an unpaired tool_use behind. Closing that at run end stretched it across the retry boundary: a tool_use at 3s, an attempt-2 anchor at 20s, and a run ending at 25s clipped to a 5s `tool_execution` phase for an attempt that never called a tool. Gate the close on the daemon-clock timestamp the tool_use was OBSERVED at, which shares a clock with our own marks, rather than on its start. A producer-supplied `startedAt` can legitimately predate the anchor on a tool that is genuinely running, and clipping already handles that case -- gating on the start would discard real occupancy instead. Two fixtures: the killed-prior-attempt case (red before this change), and a guard that an outstanding tool with a skewed producer start is still counted, so the gate cannot be tightened into discarding live work. * fix(daemon): give the summarizer a real attempt boundary Both remaining review findings came from the same gap: the outstanding-tool gate used `phaseAnchorAt` as a stand-in for "when this attempt started", and those are different instants. `phaseAnchorAt` falls back to `firstTokenAt` when no model-event mark exists. On a legacy or recovery tool-first run -- tool_use at 4s, first text at 10s, run ending at 30s -- the gate then discarded a tool this attempt had issued and never finished, reported zero tool occupancy, and handed the whole active window to `stream_output`. That is the exact shape this change exists to measure. The gate now uses the attempt boundary; `phaseAnchorAt` is only used for clipping. For that boundary to exist, two producers had to stop destroying it: - The retry path called `resetForAttempt`, which stamps `attemptStartedAt` and `attemptIndex`, and then replaced `run.analyticsTelemetry` wholesale, so both were dropped every time. It now spreads, keeping the `run.createdAt` fallback for the logical run start. This also means `attempt_index` and `attempt_duration_ms` -- already-published fields -- start reporting across retries instead of silently vanishing. - `prepareRestart` reused the run for a recharge resume and cleared its terminal state but left `analyticsTelemetry` untouched, so a resumed attempt inherited attempt 1's `firstModelEventAt`/`firstTokenAt`. Every boundary then measured from before the recharge pause, putting the wait inside the new attempt's model-active window. It now resets the marks, preserving only `startRequestedAt` so queue time is still measured from the user's request. Two fixtures, red before the change: a no-model-event outstanding tool, and a recharge resume asserting the stale marks are gone while the logical run start survives. * fix(daemon): anchor ACP tool-first turns at the tool's start Three findings from the fifth review round. ACP accumulates `tool_call` frames and emits the canonical `tool_use` only once the call reaches terminal status, carrying `startedAt` set to the first frame's arrival. The universal `send()` path stamped the anchor with `Date.now()` when that delayed event arrived, so a tool-only ACP turn put its anchor at tool COMPLETION -- measuring the whole tool loop as runtime init. That is precisely the case this PR exists to fix, still fully present for the ACP family. The stream-event markers now carry the producer's `startedAt` (daemon clock, same domain as our own marks) and the tracer stamps that instead of arrival time. Tool ids are reused across a retry: sequential ids like `call_0` restart in the retry's fresh session, and `openTools` is keyed by id alone. A killed attempt's still-open `call_0` therefore merged with the retry's genuinely new `call_0`, pairing attempt one's opening with attempt two's close and reporting a 19s tool that never ran -- inflating `tool_duration_ms` as well as phase occupancy. A still-open entry observed before the current attempt began is now replaced rather than merged. The Langfuse `model-active` diagnostic used different boundaries from the metric it exists to be compared against: it preferred `firstModelEventAt` over the earliest mark, and ended at `finalizeStartAt` rather than run end. On a text-first stream those differ, so the same run reported two windows. It now mirrors the canonical rules; the shared "earliest finite mark" logic is a named helper. * fix(daemon): stop reporting phase attribution we cannot support Two findings from the sixth review round. The first-model-event mark was first-write-wins, which is wrong for ACP. It holds each toolCallId until terminal status, so two parallel calls can complete in the opposite order they started: a call that began at 200ms can be the first one we hear about, and the call that began at 100ms is then discarded. Every phase boundary shifted later by the difference. The mark now keeps the earliest finite timestamp with its type. Marks without a producer timestamp default to arrival, which is monotonic, so this only ever moves the anchor earlier. Phase occupancy is reconstructed from tool frames in `run.events`, which is a bounded ring buffer. A run that opens with a 20s tool and then emits enough output to evict it reconstructs zero occupancy, hands the whole active window to `stream_output`, and -- because lifecycle marks live on the run and survive eviction -- still stamped the bundle `complete`. The summarizer now reads the same truncation signal the execution diagnostics in `runtimes/runs.ts` already use (first surviving event id is no longer 1), downgrades a `complete` phase bundle to `partial`, and withholds `bottleneck_phase` entirely rather than naming whichever phase the buffer happened to keep. Timings that come from lifecycle marks are unaffected and keep reporting. Five fixtures. Two of them were green before their change and exist to pin the boundary from the other side: a later event must not move the anchor forward, and a truncated run must still report its mark-derived timings. * fix(daemon): gate every tool interval on the opening attempt The pairing path never checked which attempt opened the tool. A `tool_result` that arrives after a retry has started -- a buffered stdout flush racing the abort, which the ACP session code already documents as a real sequence -- was paired with its attempt-1 opener into a single interval spanning the retry boundary. Clipping then charged the new attempt 8.9s of tool execution inside a 9s window for an attempt that issued no tool at all. Earlier rounds gated the two other ways a stale entry reaches an interval (id reuse on open, close at run end) but did so at each site, which is why this third path was missed. The rule is now stated once as `openedInCurrentAttempt` and every append to `toolIntervals` goes through it, so a fourth path cannot quietly skip the check. `tool_duration_ms` deliberately does not pass through the gate and keeps its whole-run paired-sum definition; the fixture asserts that value explicitly so the distinction stays deliberate. * fix(daemon): stop moving a published metric, and abstain when tools are unattributable Two findings from the seventh review round. Passing the producer's start into `markFirstModelEvent` moved `time_to_first_model_event_ms`, which this PR promised not to touch. That field means "when we first saw a model event"; for an ACP tool starting at 4s and emitted at terminal 20s it began reporting 4s instead of 20s. My own invariance fixture did not catch it because it only pins the four first-token metrics. The mark is now split. `firstModelEventAt` goes back to first-write-wins on arrival and keeps feeding the published metric unchanged. A new `firstModelResponseAt` holds the earliest producer start (clamped to arrival, so a fast producer clock cannot claim the model responded in the future) and is what phase boundaries anchor on. The new phase field is renamed to `runtime_init_to_first_model_response_ms` to match what it measures; it is introduced by this PR and has never shipped. Second, a tool id shared by two attempts cannot be attributed. When a retry reuses an id whose previous opener is still outstanding, a later `tool_result` could belong to either, and the two readings differ by seconds of occupancy. Earlier rounds tried to pick a winner; nothing in the event log supports one. The summarizer now records that an opener was displaced, marks the ledger ambiguous when a result arrives for such an id, and withholds `bottleneck_phase` and a `complete` phase status -- the same treatment truncation already gets, and for the same reason. Timings derived from lifecycle marks keep reporting. Fixing this properly rather than abstaining needs an attempt or generation id carried on the tool events themselves, which is a producer and event-schema change; noted in the review thread rather than smuggled in here. * fix(daemon): share one phase-anchor rule between both sinks The Langfuse `model-active` diagnostic listed its own marks, so adding `firstModelResponseAt` to the analytics anchor last commit left the mirror behind again. On an ACP tool-first run with no text token it reported 10s against the analytics value of 26s for the same run -- on precisely the runtime shape this change targets, and in the one entry whose whole purpose is to be compared across sinks. This is the second time the two drifted, so the fix is structural rather than another manual sync: the rule is exported as `phaseAnchorFromMarks` and both call sites use it. Re-listing marks per sink is what allowed the drift, and a third mark would have caused it again. Validity rules are now identical by construction too, which resolves the separate half of the finding. |
||
|
CaprikaandGitHub
|
3662637d38 |
fix(media): surface concrete image-generation failure reasons (#7118)
* fix(media): surface concrete generation failures * fix(media): preserve error code underscores in chat * test(contracts): expect concrete media failures * fix(media): flush structured CLI failures |
||
|
mehmet turacandGitHub
|
8d62fda2b5 |
fix(daemon): preserve scripts during preview URL rewriting (#6938)
* fix(daemon): preserve scripts during preview URL rewriting * fix(daemon): preserve inline script contents during rewrites * fix(daemon): preserve executable HTML attributes * fix(daemon): preserve scoped link rewriting * fix(daemon): harden protected HTML rewriting * fix(daemon): normalize executable URL schemes * fix(daemon): preserve namespaced executable URLs |
||
|
CaprikaandGitHub
|
13c3be557f | fix(daemon): hide media models from chat picker (#7123) |