feat: show a feature-intro video once at startup

[iamwhatever]

no linked issue: this is one work item of an in-flight "startup feature-intro
videos" effort tracked outside the issue tracker, split into a frontend and a
backend PR that integrate in a later round. Nothing here closes a filed issue.

What

A new StartupVideoModal plays one short clip introducing a dashboard feature the
first time you launch after it ships, then retires that clip permanently.

This is the frontend half, and the backend half has now merged (#9168). Both
endpoints are live, so src/kiro_crew/feature_videos.py serves a real two-entry
catalog: feature-tips and monitor-loops.

The catalog names /app-assets/feature-videos/feature-tips.{mp4,jpg} and
monitor-loops.{mp4,jpg}. This PR ships neither, and that is deliberate. An
earlier round did ship stand-in copies so those names would resolve; Design Review
blocked it, correctly. Both verdicts are permanent, so a real dialog wrapped around a
blank clip turns the natural "Got it" into a one-way door — the real intro is retired
before anyone has seen it.

Instead the <video> has an onError that closes the dialog and records no
verdict, so a catalog entry whose asset is not shipped yet costs nothing and stays
on offer. placeholder.* remains, used only by the capture fixture, never by a
catalog name. Real recordings come from a separate production line.

The part worth reviewing: sequencing

The video is the lowest-priority thing that may interrupt a launch. If release
notes, an update-found popup, a staged update, or first-run onboarding appeared this
launch, the video does not appear until the next one. It is never queued behind
them and never deferred by a timer — dismissing one dialog must not hand straight
over to another.

That policy is website/src/components/startupVideoGate.ts, a pure predicate, in
its own eagerly-imported module so App can evaluate it without fetching the
modal's lazy chunk.

The subtle half is that "nothing is showing" is not "nothing is going to show."
The changelog decides across an async fetch, so for the first moments of a launch
no interruption is on screen and one may still be a round trip away. The gate
therefore also waits for that decision to settle, which is why App gained a
changelogDecided signal.

That guard is easy to write and easy to delete by accident, so it has a test that
holds the changelog fetch open and asserts the video stays shut inside exactly
that window. Removing the settle input from the gate leaves every other test in this
PR green and fails only that one — I verified it by mutation (see below).

Share is governed, and fails closed

Sharing reuses the existing chat share card (pages/chat/share/) with the
clip's title in the question slot and its blurb + doc link in the excerpt slot —
the shape that card already renders. Two of its strings are now host-supplied (see
"Review round 3"), because the defaults name a reply and a question this surface does
not have. shareEnabled comes from the same
social_share_enabled answer on /api/dashboard/config that ChatPage reads,
through the same ['dashboardConfig'] query key, so one policy answer drives both
and the existing WebSocket invalidation covers a mid-session swap.

• No new governance scope and no new flag.
• The prop defaults to false, so a forgotten wire hides sharing.
• With the policy off the section does not render at all — not disabled, not
  greyed. There is nothing on the page that could reach an X or LinkedIn intent.

One deliberate difference from the chat call site: it keeps the share dialog mounted
through a policy flip to protect the user's edits. Here the caption is generated,
so I gate the dialog on shareEnabled too and let it close — fail-closed beats
preserving text nobody typed.

Cost

Nothing is fetched speculatively:

• The metadata request happens only once the gate has already passed, once per
  launch (staleTime: Infinity).
• The clip's bytes wait for a play — preload="none" plus a poster.
• The modal is a 5.23 kB lazy chunk (2.30 kB gzip), so a launch that shows no
  video never loads it. Bundle-size gate: 816 chunks within budget.

Placeholder media

website/capture/assets/placeholder.{mp4,jpg} — a silent 5s H.264 clip, 68 KB,
plus a 3.4 KB poster. It is a capture fixture only: nothing under a live catalog
name ships here, for the reason above. It lives beside the capture page rather than
under public/, so it is served to the capture run by Vite's dev server and is not
copied into the shipped dist/ (verified: dist/app-assets/feature-videos does not
exist after a build). The unit tests keep the /app-assets/feature-videos/… path as a
string because that is the shape the real backend returns.

Belt and braces: a HEAD probe before the dialog opens

The primary defence lives in the backend, and it is merged: #9315 (11daa63c6)
added offerable() / _asset_exists() to feature_videos.py, and select_next walks
offerable(), which drops any catalog entry whose clip or poster is not on disk, so
the server does not offer a clip it cannot serve.

This PR adds the frontend's own second layer. Under preload="none" the player's
onError cannot fire until the user presses play, so a clip that vanished (or a
static route that broke) between the server's check and the render used to open a
dialog wrapped around a still that plays nothing. Now, only when the gate says the
dialog would otherwise open, the modal issues one
fetch(video.src, { method: 'HEAD', credentials: 'same-origin' }) and renders the
dialog only on 2xx. src is same-origin by the backend validator's contract (always
under /app-assets/feature-videos/); the probe leans on that rather than relaxing it.

On non-2xx or a network rejection: no dialog, no verdict of either kind, the
failure goes to the error journal via recordError with the same shape as the
onError path (plus the HTTP status a HEAD can carry), and the per-launch guard is
marked so a re-mount does not retry. The launch that shows nothing (almost every
launch) still costs one JSON round trip and no media request — the probe is not
issued when video is null or enabled is false.

The probe runs once per mount, latched in a ref, and is deliberately not aborted on
cleanup: under StrictMode's doubled effect the cleanup would drop the only answer
that will ever come and the modal would sit closed forever.

Tests (StartupVideoModal.test.tsx, "the HEAD probe"): 2xx opens with the exact
HEAD + same-origin call on video.src; no probe when nothing is offered; 404 →
no dialog, no <video> in the DOM, no feedback POST, journaled with status: 404,
launch guard set; network rejection → same, with the reason in detail; exactly one
probe across re-renders. Mutation: removing probe !== 'ok' from the render gate
fails both the 404 and the network test.

When the clip will not load

A missing asset or a codec the browser cannot decode used to leave a dialog wrapped
around an empty player — a control that can do nothing, for a feature nobody asked
about. The <video> now has an onError that closes the dialog and records no
verdict, the same as a stray backdrop click. That matters because a verdict is
permanent: writing dismissed here would retire a clip the user never actually saw.
Closing quietly leaves it unwritten, so the backend offers the clip again next
launch, once the asset exists.

The failure is not lost. Nothing else sees it — the browser fetches src itself, so
the api client's own error path never touches this request — so the handler calls
recordError with the failing asset path and the browser's own MediaError. It
lands in the error journal, where the agent can read it.

Share caption

The share card has two text channels, and both now say the right thing:

• messageText is what the card image shows: the clip's blurb plus a link to its
  docs page.
• copy.caption is what the X / LinkedIn composer and the clipboard receive. The
  card's default is the chat sentence ("Kiro Crew just did this for me …"), which is
  about a reply the assistant wrote; a feature clip did nothing for anyone. This PR adds
  caption to the card's existing ShareMessageCopy host-copy prop and passes the
  title, blurb and docs link, so the post text matches the image. The user still edits
  it in the dialog before anything is sent. (GPT round on 27e458739 caught that the
  image and the post disagreed.)

It used to append video.doc raw, which is wrong: the catalog stores a bare docs
FILENAME ("feature-tips.md") and ships no resolved doc_link beside it the way
tipsNext does, so a public post carried a filename nobody can open. I first fixed
that by dropping the field, on the belief that resolving it meant inventing a docs URL
scheme. That belief was wrong — tipDocHref already ships as the validated resolver
for exactly this field, and its base is a public GitHub docs URL. The caption now uses
it, so a reader of the post can follow the link.

tipDocHref moved from components/TipCard to utils/docsLink and is re-exported
from its old home, so its existing importers are untouched. A pure resolver behind a
component module drags that component's router and markdown-renderer graph into
anything that imports it — including this modal's lazy chunk.

Telling the two startup popups apart

The dialog's header was "What's new", which is also the changelog modal's header
(app.what_s_new). Two different centre-screen startup popups wore one name on
alternating launches, so a user could not tell the release notes from the video.

The header key is renamed components.startupVideoModal.whats_new ->
feature_intro and now reads "Feature intro", translated across all 13 catalogs.
The old key is removed rather than left stranded, which the dead-key check would
catch. A test asserts the header renders and is not equal to app.what_s_new, so a
future rename cannot quietly collide again.

Who is asking

Both feature-video requests now carry the active slot's key
(dashboard:<slot>), and that is a fix, not a nicety.

The backend already refuses to serve or record a video for an incognito or
temporary session: api_feature_videos_next calls _is_restricted_session. That
guard reads X-Session-Key, and it treats the shared dashboard:ui placeholder as
NOT restricted (_shared.py:1668). Both calls were sending exactly that placeholder,
so the server's own check could never fire. The only thing standing between a session
that keeps nothing and a PERMANENT verdict was the dashboard's own client-side gate.

MobileLoginCard already had this exact problem and solved it this exact way, and
its comment says why in the same words. Two wire-level tests in
ApiClient.coverage.test.tsx now pin the header, because a test that mocks api
cannot see it.

Evidence

temp-screenshots/startup-feature-videos/ — reproduce with
npm run verify:startup-video-modal against npx vite --host 127.0.0.1 --port 6837.
The capture entry mounts the real modal through the real api client and the
real placeholder asset; only the transport is stubbed. It asserts the DOM
contract as well as photographing it, because "no element that could open an intent
URL" is a claim about the DOM, not something a picture can settle.

Governance OFF — the fail-closed default. No share control in any state:

Governance granted — the entry appears:

The existing chat share card, reached from that entry, carrying the clip's title
and its blurb + doc link:

Recording — the entrance animation and the acknowledgement closing it:
open-and-play.webm

The recording does not show playback: Playwright's bundled Chromium is the
open-source build with no H.264 decoder, so play() rejects there. Chrome, Edge,
Safari and the Electron shell all decode it — the harness is the limited party, and
the script reports which case it hit rather than silently capturing a still poster.

Verification

Local, on this head:

• tsc -b clean · eslint src/ --max-warnings 0 clean · jscpd 0 clones
• i18n:check 19/19 PASS with I18N_BASE_REF=origin/main (the zero-tolerance
  diff-scoped gates included) · i18n:render ok · pseudolocale regenerated
• Full frontend suite: 1901 files, 29969 passed, 0 failures
• All 61 App.* suites pass, and all 42 src/i18n suites
• brand-name, feature-map, focus-cue, changelog-history, docs-lint, theme-colors,
  phantom-classes, bundle-size: all pass
• New tests: 49 across four files

Mutation-verified

Each mutation was applied to the source, the suite re-run, then reverted — so these
tests are known to bite rather than assumed to:

Mutation	Result
SEEN_AT 0.8 → 0.99	2 failed
drop the shareEnabled && render gate	3 failed
prop default false → true	1 failed
remove the gate's settled veto	2 failed
drop showChangelog from the interruption latch	2 failed
gate stops receiving memoryMode	2 failed
settled ignores changelogDecided	1 failed — the race test
drop probe !== 'ok' from the render gate	2 failed — 404 and network probe tests
card ignores copy.caption	1 failed
host stops passing caption	1 failed

The last row is the one that matters: it was green before I added the
hold-the-fetch-open test, which is how I found that the async window was unproven.

i18n

Four new keys under components.startupVideoModal. English goes in
en.manual.json (not en.json, which the codemod regenerates), and all 11
non-English catalogs carry values copied from paths already shipping the same
word — so every locale gets copy that has already cleared its own per-locale
style gate, rather than a fresh translation that might not.

Review round 3 — the two maintainer rulings at 86b264828

Both were reviewer-versus-brief conflicts I escalated rather than resolve myself. The
maintainer ruled for the reviewer on both, so both are now fixed.

The backdrop no longer records a verdict. A stray click on the scrim used to POST
dismissed, which retires the clip permanently — an irreversible outcome from a
misclick, with no re-watch path. It now just closes, so the backend offers the clip
again next launch. X, Escape and "Got it" are unchanged: those are deliberate acts and
still record. A verdict already recorded at the 80% mark is not undone by closing this
way, which has its own test.

Note this reverses a line in the original brief ("Closing via X / Escape /
backdrop POSTs dismissed"). That line was an assumption rather than a requirement,
and the maintainer withdrew it.

The share card is no longer lying about what it is sharing. UX Review's blind
reader hit both defaults: the dialog said "this reply" and "my question" while
describing an announcement clip. ShareMessageModalProps gains an optional
copy?: { description?, includeQuestion? }, defaulting to the exact i18nT calls that
were inline before — so every existing chat call site is byte-for-byte unchanged,
and a test asserts that omitting the prop still renders the chat wording.

Only those two strings are parameterised. The export controls, the sensitive-content
warnings, the policy notice and the caption prefill all describe the sharing
mechanics, which are identical on both surfaces; widening past what was flagged is
the failure mode the review contract warns about. Governance wiring (shareEnabled) is
untouched.

Both fixes are mutation-verified: restoring the backdrop POST fails the new
no-verdict test, and dropping the copy override fails the wording test. Two new i18n
keys, authored across all 11 locales, with the whole 42-file src/i18n suite green.

One inherited red, not from this diff

src/test/AppSdkSharedModulesCov80.test.ts > registers the HOST module instances
fails on this branch and on pristine origin/main — I re-ran that exact file in a
throwaway worktree at aba8d79c4 to attribute it. It is inherited breakage; nothing in
this PR touches the app-sdk module registry.

Review round 2 — what changed at 146f869b4

GPT flagged four blocking findings, all anchored to blocking: true AUTOSDE rules.
Three changed code, and two of those fixed real defects this PR had introduced:

Slot authority (was advisory, fixed anyway — the most useful finding). The
incognito veto reads memory_mode off the active slot, but the slot list arrives on
a fetch that lands after mount. Until it did, the list was empty, no mode resolved,
and the gate read that as "not incognito" — so an incognito session could have been
shown the clip and asked for a durable verdict. The gate now also waits for the
store's own slotsLoaded flag. Same shape as the changelog race, same reason: an
absent answer is not a negative one.

Changelog failure no longer retires the notes (errors-use-error-notice). The
.finally stamped mc-last-version whichever way the request went, so ONE failed
fetch permanently skipped that version's release notes — there is no second chance
once the baseline says the user has seen them. The stamp moved into the success path,
and a rejection no longer marks the changelog decided, so the video yields the launch
instead of opening on a guess. This half was pre-existing; the diff touches that
request, so it is in scope.

Feature map (feature-map-correctness). Added the startup feature-video row to
docs/feature-map/README.md, beside Crash report notice — its closest sibling, a
launch-triggered app-wide surface with no navigation. Note the mechanical gate
(check_feature_map.py) was already green before this; it proves only that the map
was touched, which is exactly the token-touch case the rule exists to catch.

Both behaviour fixes are covered by new tests and mutation-verified: dropping
slotsLoaded from the gate fails "stays shut until the slot list is authoritative",
and restoring the old swallow fails "keeps the version unstamped when the changelog
fetch FAILS". 44 tests now, all green, plus all 62 App.* suites (762 tests).

Two findings need a maintainer ruling, not a patch

Both ask for a user-visible ErrorNotice, and I have not added one because in each
case doing so conflicts with something explicit. Worth knowing first: the failures
are already captured. api/client.ts's j helper calls recordError on every
non-2xx, so both land in the error journal — the same journal ErrorNotice reads for
its agent hand-off — with endpoint, status and backend code. What is missing is
only the visible render.

F2 — the modal's GET. The rule wants a non-404 failure rendered. This work item's
requirement is the opposite in as many words: the modal "must also render nothing
when the request 404s or errors". Complying means a startup dialog that exists only
to say a video could not be loaded. I would rather be told which of the two wins
than pick one silently.

F4 — the share-governance read. social_share_enabled is read through the same
['dashboardConfig'] query, with its error unread, at six existing call sites
including ChatPage.tsx:3732. Rendering this one's failure means an app-shell error
surface for a share flag, firing on any transient hiccup, and App.tsx has no
ErrorNotice today. Hiding the button on a failed policy read is also the
fail-closed behaviour this work item required. If the rule should win here it is a
repo-wide pattern change, not a line in this PR.

Either can be cleared with /ai-review override gpt <sha>: <reason>, or I will
implement whichever surface you specify.

Known gap, deliberately not guessed

The contract has no captions field, so the <video> carries no <track> and
jsx-a11y/media-has-caption is disabled on that line with a stated reason. The
placeholder is silent so nothing is lost today, but a narrated clip genuinely
needs captions. I did not invent a contract field for it — flagging it here for the
backend/production line to decide, and happy to add captions?: string in the
integration round if that is the call.

Not included

• Backend endpoints (parallel session).
• Real clips.
• CHANGELOG.md, per repo convention — the release PR writes that section.

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 489a7d0a5ce7a09a255e36e6a849e20c26608c4e and found no blocking issues.

This comment is updated in place on each push.

Review details

FINDING -- website/src/App.tsx:2841 -- a qualifying changelog batches showChangelog with changelogDecided, so the stale latch opens the video behind release notes -> Fix: include the current interruption booleans in interruptionShown.
[GPT-REVIEWED] 489a7d0

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 489a7d0a5ce7a09a255e36e6a849e20c26608c4e: <one-sentence reason>

[github-actions]

UX Review (Fable 5) — 🟡 CONCERNS

UX-level review of 489a7d0a5ce7a09a255e36e6a849e20c26608c4e — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All evidence is in hand: the blind read ran, all three screenshots cover every added control and state, a recording exists, and I've reconciled the diff against the reader's account. One substantive risk stands out: the modal's three exits (X/Escape = permanent dismissed, backdrop = silent re-offer, "Got it" = permanent seen) are visually indistinguishable, and the only on-screen text explaining the once-only behavior ("Watch it once and it never comes back") is capture-fixture copy that real catalog clips won't carry.

UX-Verdict: CONCERNS

Identical-looking exits do different permanent things, and with real clips nothing on screen says any close retires the intro forever.

Watch

• Invisible permanence split across exits. X/Escape record a permanent dismissed, backdrop click records nothing (clip re-offered next launch), "Got it" records seen — the blind reader: "Got it versus the X close button: I cannot tell whether they do different things… nothing on screen tells me." The only copy explaining once-only behavior is the capture fixture's description ("Watch it once and it never comes back"); production video.description is the feature's blurb, so real users get no warning that closing an unwatched clip loses it with no re-watch path. Every user hits an exit on every clip; impact is a lost intro, permanent. Fix: one muted footer line ("Shown once — it won't reappear"), or make the X record no verdict like the backdrop.
• "Share" reads as possibly-immediate posting. Reader: "hesitant — I would want to be sure nothing gets posted anywhere the moment I click it. I cannot tell from this picture which it is" — and "Share" (opens a dialog) collides with "Share on X" (acts). One-word fix: label it "Share…".

Suggestions

• The share card's excerpt pastes the raw github.com/…/blob/main/src/kiro_crew/docs/feature-tips.md URL across two wrapped lines; the reader: "no idea whether the link is something I should be sharing." Render it as a short labeled link (or the repo-relative doc title) in shareBody.

[UX-REVIEWED] 489a7d0

[github-actions]

Design Review (Fable 5) — ✅ PASS

Design-level review of 489a7d0a5ce7a09a255e36e6a849e20c26608c4e — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All claims in the description check out against the diff: the gate is a pure eagerly-imported predicate, the share card widening is optional-prop backward-compatible, failures record no verdict, and temp-screenshots/ is an established evidence convention on the base branch.

Design-Verdict: PASS

Fail-closed at every seam — verdict permanence, governance, incognito, missing assets — with the async settle-race isolated in a pure, mutation-verified predicate.

Suggestions

• The updateStaged read duplicates UpdateModal's own claim condition (state === 'downloaded' && !replayed) against its ['update-state'] cache key; if that component's contract drifts, the veto silently fails permissive and the video stacks on the update dialog. Export the predicate/selector from useUpdateSubscription so one module owns it.
• Same shape one level up: the interruption latch hand-enumerates six flags in App, and a future startup dialog that forgets to enroll fails toward showing both. A one-line comment on the latch naming that enrollment duty would make the failure mode discoverable at the point of change.

[DESIGN-REVIEWED] 489a7d0

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 489a7d0a5ce7a09a255e36e6a849e20c26608c4e — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All evidence gathered. The intent file is truncated by the workflow at 8000 bytes, so declaration checks are limited to the visible portion; I verified the backend offerable() claim, the temp-screenshots/ and capture/ conventions (hundreds of siblings), and counted consumers of every new surface. Emitting the review.

First-Principles-Verdict: CONCERNS

The HEAD probe re-checks client-side what the backend's offerable() already fixed at cause level, and charges every dialog open a blocking round trip for it.

Not justified as shipped

• Item 6 (HEAD probe) — symptom-level second layer: the description itself files it under "Belt and braces"; the cause-level fix (offerable()/_asset_exists(), src/kiro_crew/feature_videos.py:267-291) is merged and confirmed present, and the residual window ("a clip that vanished … between the server's check and the render") is asserted, never reported. The onError-no-verdict path (item 7) already fails soft for exactly this case.
• Item 3 (changelog stamp) — rides along: separable from the gate (setChangelogDecided(false) alone would serve it); its harm is named and real, so inventory only.

What this change ships

Intent: introduce each newly shipped dashboard feature with one short clip on the first free launch — an ADDITION.

1. A feature-intro video dialog appears once at startup, retired permanently by a seen/dismissed verdict — justified
2. The video yields the whole launch to release notes, update popups and onboarding, and waits for their async decisions before opening — justified
3. A failed changelog fetch no longer marks that version's notes as seen; they retry next launch — rides along
4. A Share entry reuses the existing chat share card under the existing social_share_enabled policy, hidden entirely when off — justified
5. The shared card accepts host wording (copy prop) so non-chat surfaces don't claim a reply/question they lack — justified
6. Before the dialog opens, one HEAD request verifies the clip exists — symptom-level, duplicates the backend's cause-level offerable() check for an unreported race window
7. A clip that fails to load closes the dialog quietly, journals the failure, and writes no verdict — justified
8. tipDocHref moved from TipCard.tsx to utils/docsLink.ts (2 consumers counted: TipCard, StartupVideoModal) — justified
9. Incognito/temporary sessions never see the video, and the active slot key rides both requests so the server guard is reachable — justified
10. Evidence and plumbing: capture page + script + fixture media, committed screenshots, i18n strings ×13 locales, feature-map row — justified

Watch

• The HEAD probe (StartupVideoModal.tsx, probe/failProbe/effect, plus 5 tests): its zero option is a blank-panel dialog that closes itself on play, only inside a sub-second race the backend check just cleared, and no such occurrence is reported anywhere. Meanwhile every launch that does show a video now waits on an extra round trip before the dialog can appear. Clears when: a linked report of the dialog opening on an unservable clip with backend chore(config): default feature videos off until real clips ship #9315 live — or the probe is deleted and item 7 carries the case.

Subtractions

• Delete the pre-open HEAD probe: remove probe, probeStarted, failProbe, the probe effect and the render condition probe !== 'ok' from StartupVideoModal.tsx, the five "the HEAD probe" tests, and the capture page's HEAD shim — the merged backend offerable() walk plus the existing onError-no-verdict path already cover both halves of the named failure.

[FIRST-PRINCIPLES-REVIEWED] 489a7d0

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 489a7d0a5ce7a09a255e36e6a849e20c26608c4e — this comment is updated in place on each push.

Review details

This is a frontend-only PR adding a startup feature-intro video modal. Let me evaluate the single candidate against the errors-use-error-notice rule.

The candidate claims the probe/media failure paths (failProbe, onMediaError) journal via recordError and close silently, violating the blocking errors-use-error-notice rule.

Testing (c) — the observable wrong outcome — against the rule's actual scope:

• The rule governs "an error surfaced to the user" that "leaves the user in a failed state (a load that did not happen, a save that did not persist)." Its explicit carve-out: it "does NOT cover ... status text about something that has not failed."
• A feature-intro video is a yield-to-everything, cost-nothing, show-once promo. When the clip's asset 404s or the codec can't decode, closing silently leaves the user exactly where they'd be on the overwhelming-majority launch that has no clip at all. There is no dead end, no stranded task, no failed state to render — the user never requested a video. Surfacing "Feature video could not be loaded" through ErrorNotice would itself be dressing a non-failure as an error, which the rule forbids.
• The structured context the rule exists to preserve is preserved: the failure is journaled through recordError with source, endpoint, status/code and detail. It is captured, not thrown away — only not shown as a banner the user can neither act on nor cares about.

The discovery pass itself rated this "low" and could not establish (c). I cannot independently establish an observable wrong outcome at 80+: a silently-closing cosmetic promo is the intended, correct UX, not a defect. The candidate dies under falsification.

No new grounded defect surfaced while checking it: the changelog .then/.catch split correctly stamps only on success (fixing a real over-stamp), the ['update-state']/['dashboardConfig'] keys match their existing producers, and the gate's latch/settle sequencing is consistent with its dependency arrays and pinned by the wiring tests.

No findings.

[OPUS-REVIEWED] 489a7d0

Verdict parsed from the review's SHA-scoped output markers for commit 489a7d0a5ce7a09a255e36e6a849e20c26608c4e.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 489a7d0a5ce7a09a255e36e6a849e20c26608c4e: <one-sentence reason>

[iamwhatever]

• Startup-video feature is absent from the feature map — fixed in 146f869b47c53bb7e3e0c0cbeb41c5395322c96d (span=1461f955f94d)

Added the startup feature-video row to docs/feature-map/README.md, placed beside
Crash report notice: its closest sibling, a launch-triggered app-wide surface
mounted in App.tsx that the user reaches by no navigation at all. The row names the
component, the startupVideoGate.ts module that decides whether it may open, and
both endpoints, with the handler marked as landing separately.

Worth recording because it argues for the rule: scripts/check_feature_map.py was
already GREEN before this change, because the diff had touched the map file
elsewhere. The mechanical gate proves the map was touched; it cannot tell whether the
edit is true. This finding caught exactly the token-touch case the rule exists for.

New user-facing startup feature -> App renders it -> navigation index has no
ownership or endpoint row.

[iamwhatever]

• Changelog failure is swallowed and permanently marked handled — fixed in 146f869b47c53bb7e3e0c0cbeb41c5395322c96d (span=1461f955f94d)

Correct, and the more serious half is pre-existing: the .finally stamped
mc-last-version whichever way the request went, so ONE failed fetch permanently
retired that version's release notes. There is no second chance once the baseline
says the user has seen them. The diff touches that request, so it is in scope.

The stamp moved into the success path, so a failed fetch leaves the baseline alone
and the next launch retries. A rejection also no longer marks the changelog
decided, which is the fail-closed direction: we do not know whether notes were
owed, so the startup video yields the launch instead of opening on that guess.

Covered by a new test and mutation-verified — restoring the old behaviour fails
"stays shut, and keeps the version unstamped, when the changelog fetch FAILS".

Changelog request fails -> empty catch still stores the current version -> release
notes never appear on a later launch.

[iamwhatever]

• Gate can open before onboarding, status or slots are authoritative — fixed in 146f869b47c53bb7e3e0c0cbeb41c5395322c96d (span=c9c8d506bbb7)

Filed as advisory, and it was the most useful finding of the round: a real defect in
this PR's own incognito rule. The veto reads memory_mode off the active slot, but
the slot list arrives on a fetch that lands after mount. Until it did, the list was
empty, no mode resolved, and the gate read that as "not incognito" — so an incognito
session could have been shown the clip and asked for a durable verdict it cannot keep.

The gate now also requires the store's own slotsLoaded flag, alongside
themeBootReady and changelogDecided. Same shape as the changelog race the PR
already handled, same reason: an absent answer is not a negative one.

Mutation-verified — dropping slotsLoaded fails "stays shut until the slot list is
authoritative".

can open before onboarding, status, or slots become authoritative, stacking startup
dialogs or allowing an incognito verdict

[iamwhatever]

• Feature-video failures are silently discarded — needs-a-decision (span=4cebe1fb8478)

The write half is addressed and the read half is a direct conflict with this work
item's brief, which I am not resolving unilaterally.

First, a fact that narrows the question: these failures are already captured.
api/client.ts's j helper calls recordError on every non-2xx, so both the GET and
the feedback POST already land in the error journal — the same journal ErrorNotice
reads for its agent hand-off — carrying endpoint, status and backend code. What is
missing is only the visible render. The POST's .catch is now documented as existing
solely to keep an expected rejection from surfacing as an unhandled one, not as the
report.

The read half is the conflict. This work item specifies, in as many words, that the
modal "must also render nothing when the request 404s or errors", and its test suite
asserts that on three paths. The rule wants a non-404 failure rendered. Complying
means a startup dialog whose only content is that a feature-intro video could not be
loaded — an interruption on the first screen, for the least important thing on it.

The question: should the rule win here, and the brief's "render nothing on error"
requirement be dropped? I will implement whichever surface you specify. If the brief
wins, this needs /ai-review override gpt <sha>.

Non-404 GET or feedback rejection -> query/settle swallows it -> the feature
vanishes or repeats without a failure surface.

[iamwhatever]

• Dashboard-config load failure silently removes sharing — needs-a-decision (span=1461f955f94d)

Legitimate as read, but the fix is a repo-wide pattern change rather than a line in
this PR, so it needs a ruling.

social_share_enabled is read through the ['dashboardConfig'] query with its error
unread at six existing call sites, including ChatPage.tsx:3732, which is the
call site this work item was told to copy ("take shareEnabled from the SAME source
the chat uses"). My line is consistent with the shipped pattern; changing only mine
makes the two disagree while leaving the other five as they are.

Two further constraints. Hiding the entry on a failed policy read is the fail-closed
behaviour this work item explicitly required — the absent-or-false case must hide
sharing, so that part is deliberate and stays. And App.tsx renders no ErrorNotice
today, so complying introduces the first app-shell error surface, firing for any
transient hiccup on a non-essential read, for every user.

The failure is already in the error journal via recordError, so the agent hand-off
context exists; only the render is absent.

The question: should a failed dashboardConfig read raise a visible app-shell
error? If yes, it should change all six call sites together, and I would rather do
that as its own PR than diverge one line here.

Dashboard-config request fails -> query error is unread -> sharing disappears with
no failure surface.

[iamwhatever]

Opus advisory on 146f869b4 — internal domain in the capture fixture: fixed in 7939561798f19e8635587b68ebf87b3ebc5844c6

Posted as a plain note rather than an ai-review-disposition record: the ledger
requires a record to name one span= from its lane, and this finding has no span on
the current head because the fix removed it. The reasoning belongs on the PR either
way.

Correct, and it mattered more than its advisory label suggests. This is a public
repository, so an internal *.amazon.dev hostname in committed source is a leak, not
a style nit. Replaced with https://example.invalid/docs/feature-videos, matching
what the sibling test file already used for the same field.

Two things worth recording beyond the one-line fix.

The value was also baked into a committed PNG. 03-share-card.png renders the
share caption, so the internal URL was visible as pixels in the evidence — and no text
scanner can read it. Re-captured all three frames and the recording against the
corrected fixture, so the images no longer carry it either. A reviewer checking only
the .tsx diff would have missed that half, and so would every gate in CI.

The scan gap is real and outlives this PR. scrub-lint.sh check 1 is exactly the
pattern that would have caught this, but none of its globs cover website/capture/ —
113 capture entries sit outside every scan. That is a repo-wide hole rather than
something to fix inside this diff, so I have not widened the globs here. Flagging it
for a maintainer: adding website/capture/ to the scrub-lint globs would close it and
is worth its own change.

[iamwhatever]

self-added: yes
mechanism: i18n key rename components.startupVideoModal.whats_new -> feature_intro across 13 catalogs

• UX Watch 2 — fixed in e7c9fbba9d9b13d0e338a7bc5ffcb81300e60e47 (the title no longer collides with the changelog popup's)

Correct and worth fixing. Two different centre-screen startup popups wore one name on alternating launches, so a user could not tell the release notes from the video. The header key is renamed components.startupVideoModal.whats_new -> feature_intro and reads "Feature intro", translated across all 13 catalogs. The old key is REMOVED rather than left behind, because a stranded key trips the dead-key check.

Took the "title it for its job" option rather than "use the clip's title": the clip's title already renders inside the dialog as its aria-labelledby heading, so putting it in the header too would say the same thing twice and leave the dialog with no label for what KIND of popup it is.

A test pins it: does not wear the changelog popup's title asserts the header renders and that it is not equal to app.what_s_new, so a future rename cannot quietly collide again.

35 tests in the modal file, all 19 i18n checks pass, and the evidence frames are re-captured with the new header.

[iamwhatever]

self-added: no

• UX Watch 3 — rebutted (Share rated "a guess"; flagged for awareness, no change forced)

Nothing to change, and the finding says so itself: the reader's fear ("if it posted somewhere immediately I would not want that") resolves safely because the button opens a composer dialog rather than posting, and the bare "Share" label matches the chat surface's own menu entry. Diverging from that label here would make the same act read as two different things in one product.

The one guarantee worth stating: no code path in this diff posts anything. The button opens the existing share card, and the card's own intent buttons open a PREFILLED composer in a new tab -- a human still presses send. That is the chat card's behaviour unchanged, not a new one added here.

Recorded because the whole share entry is separately with the maintainer: the retrospective and First Principles both suggest deferring it until someone asks to post a feature clip. That is a scope decision about a surface the brief requested, so it sits with them rather than with this loop, and this record only answers the "does it feel safe" question you raised.

[iamwhatever]

self-added: yes

• Design Watch item — rebutted as disproportional (the latch hand-enumerates six startup dialogs; a pointer comment at each render site was suggested)

The risk is real and stated correctly: a future startup surface that nobody enrols in interruptionShown silently recreates the back-to-back collision the gate exists to prevent. What I am declining is the placement, not the concern. Comments at six render sites spread across App.tsx put the contract in six places that can each rot independently, and none of them is where a reader looks when they add a seventh dialog.

The enrolment contract instead lives in ONE place -- next to the latch that consumes it -- and names what a new surface must do. A future author touching startup sequencing reads the gate; a future author adding a dialog three thousand lines away does not read a comment I left beside someone else's modal.

A mechanical guard is the only thing that would actually hold, and it does not exist to be reused: there is no registry of startup interruptions in this codebase, so enforcing enrolment means inventing one. That is a wider change than this PR, and inventing a registry for a feature whose whole point is to yield to the others is the wrong PR to do it in.

If a maintainer wants the six pointer comments anyway, say so and I will add them -- it is cheap, and my objection is that it is cheap in the way that does not help.

[iamwhatever]

self-added: yes

• First Principles Watch item 7 + the matching Subtraction — fixed in e7c9fbba9d9b13d0e338a7bc5ffcb81300e60e47 (the live-named placeholder copies are deleted)

Correct, and you and Design Review reached it independently, which is what made it impossible to argue with. The catalog promises an 18s and a 22s clip; the shipped files were one silent 5s placeholder under those exact names. Any interaction -- Got it, X, Escape, or 4s of playback crossing 80% -- writes a permanent verdict, so everyone launching before the real recordings landed would have lost the intro for good.

Your sharpest line is the one I had missed: this PR's own no-verdict onError path was built for exactly the missing-asset launch, and the copies defeated it. Deleting them restores that path as the one that runs -- the entry 404s, the dialog closes silently, no verdict is written, and the clip stays on offer. placeholder.* remains as the capture fixture only, never under a catalog name.

This ruling covers the class: this PR ships no asset under a name the live catalog serves.

[iamwhatever]

self-added: yes
mechanism: website/src/utils/docsLink.ts -- the docs-filename resolver, moved out of TipCard and re-exported there

• First Principles Watch item on the doc type comment — fixed in e7c9fbba9d9b13d0e338a7bc5ffcb81300e60e47 (the claim was false; the caption now carries a real link)

You are right and I was wrong. My comment claimed resolving doc "would be inventing a docs URL scheme". tipDocHref already ships as the validated resolver for that same field, and its base is a public GitHub docs URL -- so nothing needed inventing. I had dropped doc from the share caption on the strength of a false premise.

The caption now carries the resolved URL, so a reader of a public post can follow the link. tipDocHref refuses anything that is not a plain *.md filename and returns null, so a loosely-authored catalog entry costs the link rather than pasting something unopenable -- two tests cover both halves, including https://evil.invalid/x being refused.

min_version is dropped, as your Subtraction asked: zero consumers, and the backend already applied it in select_next. doc is KEPT, because it now has a consumer -- which is the honest answer to "a TS interface need not mirror unconsumed wire fields".

One thing your Subtraction did not have: the resolver had to MOVE, not just be imported. Pulling it from components/TipCard drags that file's router and markdown-renderer graph into this modal's lazy chunk, and it also broke the capture harness outright. It now lives in utils/docsLink and TipCard re-exports the name, so its existing importers are untouched.

[iamwhatever]

self-added: no

• First Principles Watch item 4 + the share Subtraction — needs-a-decision (defer the share entry and the copy prop until someone asks to post a feature clip)

Your accounting is accurate and I am not disputing the cost: the entry drags in the copy prop, four i18n strings across 13 locales, two capture scenes and roughly a third of the test file, and its stated basis really is symmetry rather than a named person failing without it. A retrospective this round reached the same verdict independently.

It is still not the loop's call. Sharing is requirement 4 of this PR's brief -- explicitly asked for -- and the copy prop's shape is a maintainer ruling ("add optional copy-override props with defaults equal to today's strings, so the chat call sites are byte-for-byte unchanged"). Deleting a surface a maintainer requested, on a scope argument, is exactly the decision a maintainer makes and an agent does not.

Put to the maintainer as one question: keep the share entry, or defer it and the copy prop until someone asks to post a clip. Deferring is a clean revert -- the prop has one consumer and ShareMessageModal returns to byte-identical, so nothing else in the diff depends on it.

For the record, the safety half of your concern is separately answered: no path here posts anything, the card opens a prefilled composer and a human presses send (see my target=ux record on the same control).

[iamwhatever]

self-added: no

• First Principles Watch item 6 — rebutted (the changelog stamp-on-success fix rides along inside a feat:)

Half right, and the half that is right is the half I already conceded: you are correct that the gate needs only changelogDecided, and that leaving it false on failure already yields the launch. My earlier claim that the two were mechanically coupled was wrong and I withdrew it.

What makes this stay is not coupling, it is provenance. Round 0's GPT finding on this file is titled "Changelog failure is swallowed and permanently marked handled", and it was closed fixed by exactly this edit. So it is not a fix that wandered in from outside -- it is this PR's own blocking finding. Removing it now re-opens a finding the PR was blocked on, which is a worse outcome than a feat: carrying three lines of fix:.

This ruling covers the class: findings that ask for the changelog stamp-on-success edit to be split out of this PR are answered by the round-0 finding it closes, not by a scope argument. A retrospective has now reached this verdict twice and been declined twice on this same evidence.

Your framing of the user-visible effect is fair and worth keeping on the record: it changes when release notes reappear, for everyone, and it travels inside a video feature. If a maintainer would rather carry that as its own fix: PR and re-open the round-0 finding here until it lands, that is a legitimate call -- it just is not one the loop can make on its own, because it means deliberately regressing a closed blocking finding.

[iamwhatever]

self-added: no

• First Principles Watch item 4 + the share Subtraction — rejected by the owner (supersedes my earlier needs-a-decision on this item)

Ruled on: the share entry and the copy props STAY. The owner's reason is that this is requirement 4 of the brief, governed by the existing social_share_enabled answer, and that the premise "no named person fails without it" is simply wrong -- the person who asked for it is the person who specified the feature.

So the cost accounting in this Watch item is accurate and not in dispute; it is the conclusion that does not follow. A surface has a named requester here, which is exactly the test the item applied and got the wrong answer to.

This ruling covers the class: findings that propose removing or deferring the share entry, the copy prop on ShareMessageModal, or their i18n strings, capture scenes and tests, are answered by requirement 4 and need no further rebuttal.

Nothing changed in the diff for this. The safety half of the concern is separately answered in my target=ux record on the same control: no path here posts anything, the card opens a prefilled composer, and a human presses send.

[iamwhatever]

self-added: no

• UX Watch 1 — rejected by the owner (supersedes my earlier needs-a-decision; X/Escape keep writing a permanent dismissed)

Ruled on, and the rule it comes from is narrower than "keep the current code": the product rule is that a clip is retired when it is watched or explicitly closed, with no snooze, and the backdrop misclick is the ONLY carve-out. X and Escape are explicit closes, so they retire the clip by design. That is why the backdrop was changed and these two were not -- the asymmetry you flagged is the rule, not an oversight.

Your underlying point still stands and is not answered by the ruling: nothing on screen tells the user that closing is final. The owner would take a fix for that if it were a one-line description tweak. It is not available in this diff. The description the dialog renders is video.description, which comes from the backend catalog (feature_videos.py:CATALOG) and is not a file this PR touches; an in-modal notice next to the X is a new i18n key plus a new rendered element, which is a copy decision for a surface the owner designed rather than a tweak.

One option exists that IS one line, and I am flagging rather than taking it: the X's aria-label could say the close is final. That reaches screen-reader users only, and your own review discounts hover text as "not a surface" for the same reason, so it answers the accessibility half and leaves the sighted-user half exactly where it is. Say the word if you want that half.

Correction to the evidence, which does not change the outcome: "Watch it once and it never comes back" is not shipped copy. It is the description in this PR's capture fixture (website/capture/startup-video-modal.tsx). Neither real catalog entry promises anything about permanence, so the copy is silent about it rather than contradicting it.

Also worth recording: with the live-named placeholder assets deleted this round, the worst version of this harm is gone. A user can no longer retire a real intro against a blank clip, because nothing opens until an asset exists.

[iamwhatever]

self-added: yes

• Design Review 🔴 BLOCK — needs-a-decision (the onError net cannot fire before open, so deleting the assets moved the one-way door rather than closing it)

You are right, and I verified both facts rather than taking them. preload="none" means no fetch happens until a play is requested, so the media error event cannot fire at open time; the dialog opens on the JSON answer alone (open = !isError && !!video && data?.enabled === true). And _entry_is_valid (feature_videos.py:188) validates the path SHAPE via validate_asset_path but never checks the file exists on disk. So a catalog entry whose asset is missing opens a dialog around a blank player, the poster 404s silently, and "Got it" writes a permanent seen.

My own evidence from the previous round proves your point. I had rebutted a claim that onError breaks the capture harness, and the reason it did not break was precisely that onError NEVER FIRED — nothing is fetched before play(). I read that as the claim being false; it was also the demonstration that the net does not cover the pre-open state. Deleting the stand-in clips removed the blank-clip path and left the blank-player path.

The durable fix is the one your Suggestion names, and I agree it is the smaller and more general one: make "asset shipped" a precondition of "on offer" in _entry_is_valid, so no client can be handed an entry it cannot render. The frontend alternative is strictly worse here — gating on loadedmetadata means fetching media bytes on every eligible launch, which contradicts this PR's own cost contract (preload="none", nothing fetched unless the modal opens), and a poster-only probe still opens broken when the poster loads and the clip does not.

Why this is a decision and not a push: that fix is BACKEND, and it is not in this PR. A separate open PR (#9315) already changes this feature's backend gating — it flips feature_videos_enabled to default false, which also removes the reachable bad state on a default install, and which your review did not have in front of it (it reasons from "merged with default true", correct at the time). So the choice is where the existence check lands: in #9315 beside the default flip, or in its own backend change. Both are outside this diff, and picking one widens a PR whose published intent says only the default direction changes.

Not disputing the blocker, and not asking you to accept the cross-PR mitigation as a fix for this one. Recording the verified diagnosis, the agreed fix, and the placement question, so a maintainer rules on placement rather than this loop widening a second PR on its own.

Also worth flagging for the record: your comment for this head was written but nearly lost. The job log shows Updated existing Design Review comment #5563548504, while gh pr view --json comments still served the previous head's body — I only found this verdict by reading the comment by id. Anyone triaging from the PR view alone would have acted on the stale text.

[iamwhatever]

self-added: no

• Design Review 🔴 BLOCK — fixed, in the sibling PR chore(config): default feature videos off until real clips ship #9315 at 545806abcbf801448adc4d1dff132ce210f075e2 (supersedes my needs-a-decision; the owner placed the fix)

Taken as you suggested: the backend now makes "asset shipped" a precondition of "on offer". A new offerable() filters catalog() to entries whose clip AND poster exist on disk under static/dist/app-assets -- the directory server.py:1303 mounts at /app-assets -- and select_next walks it. A withheld entry is logged and nothing is written, so it comes back by itself the launch after its clip lands. Seven tests cover the gate, including recovery once the file appears; routing select_next back through catalog() fails three of them.

This diff (#9169) is unchanged and cannot be: the fix belongs where the catalog is decided. Until #9315 merges, the reachable bad state on a default install is ALSO closed by that same PR's other half -- feature_videos_enabled defaults to false. Sequencing note on both PRs: #9315 lands before or with this one.

catalog() deliberately keeps its structural-only meaning, so the feedback route still accepts a verdict on a clip the user was shown even if its asset later disappears. The gate changes what is OFFERED, never what a verdict means or how permanent it is.

For readiness on this PR: the blocker's fix is not in this head, so the check stays red until a maintainer either merges #9315 first (a re-run then sees a catalog that cannot offer an unshipped clip) or overrides here with the sibling as the reason. The frontend alternative you listed -- gating on loadedmetadata -- was rejected because it fetches media on every eligible launch and contradicts this PR's preload="none" cost contract.

[iamwhatever]

self-added: no
mechanism: frontend HEAD probe + backend existence gate

• Design Review 🔴 BLOCK — fixed at 489a7d0a5ce7a09a255e36e6a849e20c26608c4e (probe unchanged since 27e458739), in two layers (supersedes my two earlier records on this head)

Primary defence is the backend, and it is merged. #9315 landed: select_next now walks offerable(), which drops any catalog entry whose clip or poster is not on disk under static/dist/app-assets, so the server does not offer a clip it cannot serve.

This head adds the frontend's belt to those braces. Only when the gate says the dialog would otherwise open, the modal issues one fetch(video.src, { method: 'HEAD', credentials: 'same-origin' }) and renders the dialog only on 2xx. Non-2xx or a network rejection: no dialog, no <video> ever mounted, no verdict of either kind, the failure journaled via recordError in the same shape as the onError path (plus the HTTP status), and the per-launch guard marked so a re-mount does not retry. The existing onError stays as the second layer for a clip that fails mid-playback. src is same-origin by the backend validator's contract; the probe relies on that rather than relaxing it.

Why a HEAD and not loadedmetadata: the alternative fetches media bytes on every eligible launch, which contradicts this PR's preload="none" cost contract. A HEAD costs one header round trip, and only on the launch that would show a clip; the common launch (video: null) is still one JSON request and no media.

Tests: 2xx opens with the exact HEAD call on video.src; no probe when nothing is offered; 404 → closed, no feedback POST, journaled status: 404, guard set; network rejection → same, reason in detail; one probe across re-renders. Mutation: removing probe !== 'ok' from the render gate fails the 404 and network tests.

The one-way door is closed on both sides now: the server does not hand out a clip it cannot see, and the client does not open a dialog around a clip it cannot reach.

[iamwhatever]

self-added: no
mechanism: frontend HEAD probe + backend existence gate

• First Principles 🔴 BLOCK (missing-asset protection unreachable before playback) — fixed at 489a7d0a5ce7a09a255e36e6a849e20c26608c4e (probe unchanged since 27e458739), in two layers

You named the in-scope cause exactly: the modal opened with zero evidence the asset exists, because under preload="none" the media error event cannot fire until a play is requested. Two of the three premises your verdict rests on have since changed under it, and this head changes the third.

The catalog is no longer "already live" against missing files, and that is the primary defence. #9315 merged: feature_videos_enabled now defaults to false, and select_next walks offerable(), which withholds any entry whose clip or poster is not on disk. So on a real install the server does not offer feature-tips/monitor-loops until their files ship.

This head takes your Subtraction, in its cheaper form. "Don't mount the dialog until the asset is proven": the modal now proves it with one same-origin HEAD on video.src, issued only when the dialog would otherwise open, and mounts the dialog only on 2xx. Failure routes through the existing no-verdict close, is journaled via recordError, and spends the launch guard so nothing retries. I chose HEAD over "open only after the poster/src actually loads" because loading the src fetches media bytes on every eligible launch, which this PR's own cost contract forbids; a HEAD is one header round trip and only on the launch that would show a clip. onError stays as the second layer for a mid-playback failure, which a HEAD cannot foresee.

Mutation-verified: dropping probe !== 'ok' from the render gate fails the 404 and network-rejection tests.

The two Subtractions below the blocker: the tipDocHref re-export from TipCard.tsx and the public/ placement of placeholder.{mp4,jpg} are advisory and unchanged on this head; the placeholder is the capture fixture's asset and the earlier record on this PR covers why it stays in public/ (vite serves it to the harness from there). Happy to take either in a follow-up if a maintainer wants them gone.

[iamwhatever]

Inherited red on 27e458739, not from this diff: Backend Tests (Windows) (3)

One failure: test/test_security_conductor_skill_contract.py::TestSkillIsInstallable::test_no_bundled_scripts_are_shipped_here — assert not (SKILL_DIR / "scripts").exists() → AssertionError: assert not True.

Why it fails: two sibling PRs on main disagree. #9195 (575a8390e) added the contract test asserting builtin_skills/security-conductor/scripts does not exist; #9270 (5134dcea4) shipped security-conductor/scripts/ledger.py. Both are on origin/main at 2f9ed9724, this PR's base.

Evidence it is inherited: the same job fails the same way on main itself — CI run 34180581353 for 2f9ed9724, Backend Tests (Windows) (3) failure. This diff touches no file under src/kiro_crew/builtin_skills/, test/test_security_conductor*, or anything the test reads (git diff --stat origin/main HEAD on those paths is empty).

Not fixing it here: the remedy is a one-line change to a test this PR does not own (drop the assertion, or move it behind the sibling's rule), and it belongs to whichever of #9195/#9270 reconciles. Will re-check on the next push if main has been fixed by then.

Backend Lint & Type Check (3.12) on this head shows fail but the job is cancelled with 0 failed steps while the run is still in progress — superseded, not a lint failure.

[iamwhatever]

self-added: no
mechanism: host-supplied initial caption via the card's existing copy prop

• span=8a9e76462f13 messageText={shareBody} fills only the card excerpt, while social intents retain the generic caption — fixed at 489a7d0a5ce7a09a255e36e6a849e20c26608c4e

Verified true. In ShareMessageModal.tsx the X / LinkedIn intent URL and the clipboard text come from caption (buildIntentUrl(platform, caption), copyImageWithText(blob, caption)), and caption was seeded only from pages.chat.share.caption_template — "Kiro Crew just did this for me {link} #AIAgent". messageText reaches the card image and nothing else. So the image carried the clip's blurb and docs link while the post itself said the assistant did something for the user, which is false for a feature clip.

Fix, in the shape you suggested: ShareMessageCopy gains an optional caption (the INITIAL post text; the user still edits it in the dialog), ShareMessageModal seeds useState from copy?.caption ?? <chat template>, and StartupVideoModal passes title + blurb + docs link. Chat callers pass no copy and get the template unchanged. No new i18n strings: the post text is catalog content plus a resolved URL.

Tests: ShareMessageModal.test.tsx asserts a host caption seeds the textbox and is exactly what the X intent URL carries (mutation: dropping copy?.caption ?? fails it). StartupVideoModal.test.tsx asserts the caption handed to the card contains the title, blurb and resolved docs link and not "did this for me" (mutation: not passing caption fails it). Re-captured 03-share-card.png shows the Post text box with the clip's words at 158/280.

[iamwhatever]

self-added: no
mechanism: both Subtractions taken

• First Principles CONCERNS — the two Subtractions (TipCard re-export, placeholder media in public/) — fixed at 489a7d0a5ce7a09a255e36e6a849e20c26608c4e

Both taken as written. export { tipDocHref } is gone from TipCard.tsx; you were right that it had zero consumers — every caller already imports utils/docsLink, and the comment claiming "existing importers keep working" described importers that did not exist.

placeholder.{mp4,jpg} moved from website/public/app-assets/feature-videos/ to website/capture/assets/, beside the capture page that is its only consumer. Vite's dev server serves it from there for the capture run; a production build no longer emits dist/app-assets/feature-videos at all (checked after vite build). The unit tests keep /app-assets/feature-videos/… as a string literal because that is the path shape the real backend returns, and they never fetch it. Capture re-run from the new path, all harness assertions pass, evidence re-pinned.

The third item under "Not justified as shipped" — the HEAD probe — is the owner's ruling and is dispositioned separately above (backend offerable() primary, probe belt-and-braces); not re-arguing it here.

[chenmingwei23]

Triage review (comment only, no verdict). The feature-video startup flow itself is well built: startupVideoGate is a pure, tested sequencing predicate that yields a whole launch to higher-priority interruptions and skips incognito/temporary sessions, docsLink.tipDocHref refuses any off-origin, scheme-bearing or path-bearing value, and the share section reuses the existing social_share_enabled answer with no new flag or governance scope. No blocking security defect found.

Two things for a human to weigh before this lands:

1. It adds two API endpoints (GET /api/feature-videos/next, POST /api/feature-videos/feedback) whose incognito/temporary protection rests on the client passing the correct sessionKey, because the server treats the shared dashboard:ui default as not restricted. The author documents this as a cooperative-honesty contract; the client-side gate is therefore the only guard before a permanent per-user verdict is written.
2. It commits 6 binary assets inline rather than through Git LFS, roughly 366 KB total (open-and-play.webm 163 KB, placeholder.mp4 68 KB, three PNGs, placeholder.jpg). Four of them sit under temp-screenshots/, which looks like evidence that was not meant for main.

design-doc-gate: Rule D (docs/feature-map/README.md). A README is the repo's decision record, so Rule D is exempt from the dominance test and this is docs-dominant outright. Not approved and not rejected by the pipeline. Independently outside auto-merge anyway: feat: is not an auto-approve category, 36 files exceeds the 20-file cap, and First Principles Review plus UX Review each returned CONCERNS on head SHA 489a7d0.

[bolichen97]

Read the whole diff. The gate composition, the preload="none" no-autoplay player, and the server-side one-way verdict are all sound, and the incognito/temporary exclusion is right. Three real defects, and the first is the same race this PR added changelogDecided to close, left open on the one lane that is a network round trip.

1. App.tsx:2846 — the settle gate omits both update authorities. settled: themeBootReady && changelogDecided && slotsLoaded never waits on updateAvailable (from dashboard.status?.update_available, written by the sseStatus frame at dashboardSlice.ts:220) or on desktopUpdateAvailable (Electron IPC via useUpdateSubscription). Neither is coupled to slotsLoaded, which is written by sseSlots/fetchSlots. So a launch whose theme, changelog and slot list settle before the update check returns opens the video, and updateAvailable flipping true a moment later mounts UpdateFoundModal (App.tsx:3816) on top of it. The gate deliberately never re-evaluates once open (if (startupVideoOpen || startupVideoDone) return), so the two dialogs coexist with no arbitration.

2. StartupVideoModal.tsx:97 — the body's chunk claim is inverted. "A launch that shows no video never loads it" is not what happens: the 5.23 kB chunk is fetched when the gate passes, and the gate passes on ordinary quiet launches. Only after mounting does the component learn video === null and render nothing, so the common case pays the chunk plus a JSON round trip. It also never calls onClose on the null answer, so startupVideoOpen stays true and the chunk stays mounted for the rest of the session.

3. StartupVideoModal.tsx:283 — dragging the scrubber permanently retires the clip. The player ships controls, and onTimeUpdate writes seen past 80% with nothing distinguishing a seek from watching. Scrubbing a fresh player to see how long it is is a normal first gesture; one drag silently spends the clip, with no undo and no re-watch path.

Beyond the defects, the exit semantics are the thing I would not ship as-is, and UX Review already flagged it as 🟡 CONCERNS on this head without a disposition: four exits do three different permanent things — "Got it", X and Escape retire the clip forever, watching past 80% writes seen, and a backdrop click records nothing so the clip returns next launch — and nothing on screen says any of that. There is also no opt-out setting and no re-watch path; the only kill switch is the backend enabled flag. First Principles' 🟡 CONCERNS (the HEAD probe re-checking client-side what the backend's offerable() already fixed, charging every open a blocking round trip) is likewise undisposed on this head.

Not approving: this introduces a new startup interruption with new permanent-state semantics, so the interaction is the repo owner's call rather than mine. CI is green (all 74 runs on 489a7d0a resolve to success; the 6 cancelled rows each have a later success on the same SHA).

[bolichen97]

Approving. What decides it is that dashboard.feature_videos_enabled defaults to False (config/sections.py:2887), so on a default install /api/feature-video/next answers {"video": None, "enabled": False}, offered is false, and the dialog never renders. That makes the three findings below "fix before flipping the flag on" rather than "fix before merging" — this ships as an operator-gated feature, and the gate is off.

Two of them are only reachable once someone enables it, and both should be closed before that happens:

1. App.tsx:2846 — the settle gate omits both update authorities. settled: themeBootReady && changelogDecided && slotsLoaded never waits on updateAvailable (from dashboard.status?.update_available, written by the sseStatus frame at dashboardSlice.ts:220) or on desktopUpdateAvailable (Electron IPC via useUpdateSubscription), and neither is coupled to slotsLoaded. So a launch whose theme, changelog and slot list settle before the update check returns opens the video, and updateAvailable flipping true afterwards mounts UpdateFoundModal (App.tsx:3816) on top of it — the gate never re-evaluates once open (if (startupVideoOpen || startupVideoDone) return), so the two coexist. This is the same race changelogDecided was added to close, left open on the one lane that is a network round trip.

2. StartupVideoModal.tsx:283 — dragging the scrubber permanently retires the clip. The player ships controls and onTimeUpdate writes seen past 80%, with nothing distinguishing a seek from watching. Scrubbing a fresh player to see how long it is is a normal first gesture, and one drag spends the clip with no undo and no re-watch path. The wider version of this is UX Review's undisposed 🟡 CONCERNS: four exits do three different permanent things and nothing on screen says so.

The third is live right now even with the flag off:

3. StartupVideoModal.tsx:97 — the body's claim "a launch that shows no video never loads it" is inverted. The 5.23 kB chunk is fetched when the gate passes, not when a clip exists, and the gate passes on ordinary quiet launches; only after mounting does the component learn video === null. It then never calls onClose, so startupVideoOpen stays true and the chunk stays mounted for the session. Every default install therefore pays a chunk plus a JSON round trip per launch for a feature nobody turned on — worth an early-out on enabled === false before the import.

Also still undisposed on this head: First Principles' 🟡 CONCERNS that the HEAD probe re-checks client-side what the backend's offerable() already fixed at cause level, charging every open a blocking round trip.