feat(dashboard): add the feature-videos catalog and display-state API

[iamwhatever]

Problem / Motivation

Merge order. This is the backend half. Nothing in website/ calls these endpoints yet and the clip files are not in this repo — both ride in the parallel StartupVideoModal change. Merged alone, GET /api/feature-videos/next returns an entry whose src 404s; nothing crashes and no user sees it, because the modal that would render it does not exist yet. The API contract below was frozen in advance so both halves could be built at once.

A new install has no way to be shown a feature. Feature Tips describe one in a line of text above the composer, which works for a config toggle or a keyboard shortcut but not for anything you have to watch to understand — a monitor loop following a pull request, a tip card arriving mid-turn. There is no catalog of recorded intros, no rule for picking one, and no record of which ones a user has already seen.

This PR is the backend half: the catalog, the eligibility rule set, and the API. A parallel change builds the StartupVideoModal that plays them, and ships the clip and poster files under website/public/app-assets/feature-videos/.

Why it matters

Features nobody discovers are features nobody has. Tips already carry that job for text-shaped features and are measurably the right shape for them; the ones a still line of prose cannot convey are exactly the ones a fresh install is least likely to find on its own.

Getting the selection rule right is the part that decides whether this is welcome or annoying. An intro for something you already use every day is worse than no intro, so "have you found this yet?" has to be answered from local state rather than guessed — and it has to be answered the same way twice, or a modal flickers between clips on two polls a second apart.

What changed (motivation → approach → change)

Goal: show a fresh install one short clip for a feature it has not used, at most once per feature, ever.

Approach, and why it is not the tips engine. Tips are generated: a model picks a feature and writes prose, so their engine is a cadence gate plus a weighted-random selector over a pool that regenerates every six hours. A video is a recorded artifact — the clip either exists or it does not — so nothing about it can be produced at request time, and for a given install state there is exactly one right answer. Modelling it on tips would have bought randomness nobody wants and a six-hourly refresh with nothing to refresh.

So the rule set is deterministic:

• the catalog is a static tuple in feature_videos.py — no doc scan, no LLM;
• selection walks it in order and returns the first eligible entry, so two requests one second apart cannot disagree;
• "already used this feature?" is answered by named probes over local state.

What was built. New module src/kiro_crew/feature_videos.py, three routes registered beside the tips routes in dashboard/routes/realtime.py, and one config flag wired where tips_enabled is defined and parsed.

GET  /api/feature-videos/next      -> {"video": {...} | null, "enabled": bool}
POST /api/feature-videos/feedback  {"id", "status": "seen"|"dismissed"} -> {"ok": true}
GET  /api/feature-videos/status    -> {"enabled": bool, "state": {...}}

An entry is eligible when the kill switch is on, no status is recorded for it, min_version is satisfied, and no used_when probe fires. Decisions worth naming:

• Both statuses are permanent. seen and dismissed behave identically and there is no snooze: a feature intro that comes back is noise, not a reminder.
• An unknown id is a coded 400, not a new state key. Accepting one would let the state file accumulate unbounded client-supplied keys.
• Asset paths are validated in exactly one function. A clip src is fetched by the browser with the dashboard's own credentials, so an off-origin value there is an outbound request the user authorized without knowing it. validate_asset_path refuses any : (every scheme, plus a Windows drive letter), //, .., % (so a percent-encoded traversal cannot reconstitute one after the browser decodes it), a backslash, whitespace, and anything outside /app-assets/feature-videos/. Remote-hosted clips are a separate future change and the allowlist relaxation belongs in that function alone — the comment says so at the line it would go.
• A malformed entry is dropped, not raised. A bad path, or a doc outside the tips doc allowlist, logs a warning and withdraws that one entry rather than 500ing all three endpoints.
• Probes fail toward showing the clip. A probe that raises and a signal nobody registered both count as "not used", and both are logged. The cost of that default is one clip a user may not need; the opposite default would silently withhold every intro on a host whose audit log happens to be unreadable.
• Probes are cheap and lazy. They run at most once per /next request and only for entries no earlier check already ruled out. artifacts_nonempty stops at the first artifact directory rather than going through ArtifactStore.list(), which reads every meta.json. sel_event_seen uses the bounded tail-first sel().recent(limit=…).
• config_key_set reads the config FILES, not the effective config. Every effective key has a value, so an effective read would fire on the shipped default and withdraw the clip from someone who never touched the setting.
• A restricted session neither reads nor writes. Temporary and incognito sessions get null from /next, and feedback records nothing and still answers {"ok": true} — the write side is where the permanent row actually lands, so gating only the read would leave the trace one POST away.
• enabled: false is a body, not a 204. The settings panel and the modal both have to tell "the operator turned this off" apart from "nothing left to show", and a bodiless response cannot.

State lives in feature_videos_state.json beside tips_state.json, written through atomic_write(restrict_to_owner=True, restrict_on_error="warn") — the file records which features this user engaged with, which is a behavioural profile. record_status holds a lock across load-mutate-save so two tabs recording different videos cannot drop each other's row.

Catalog seeded with two entries (feature-tips, monitor-loops) pointing at <id>.mp4 / <id>.jpg.

Deliberately untouched: /api/dashboard/config and everything under capabilities.social_share — the frontend reuses the existing social_share_enabled for its share button.

Signal from diff_signals.py: config/infra file changed — that is config-baseline.json, regenerated by scripts/generate_config_baseline.py for the one added key, plus the dashboard.feature_videos_enabled row in sections.py / loader.py.

No CHANGELOG entry. A feature PR does not touch that file (docs/build/changelog.md), and check_changelog_history.py enforces it; the release PR writes the section.

Tests

test/test_feature_videos.py, 106 tests. What each group locks in:

• Path validation — 18 parametrized rejections (scheme, protocol-relative //, embedded //, .., ..%2f, %2e%2e, backslash, whitespace, newline, wrong prefix, bare prefix, non-string) and 3 acceptances.
• /next skips — a seen entry, a dismissed entry, an entry whose probe fires, an entry withdrawn by any one of several signals, and a version-gated entry; plus that probes are not run for an entry state already ruled out, and that five consecutive calls return the same id.
• Feedback — both statuses persist with a timestamp, a recorded video is not offered again, the state file is 0600, and 8 parametrized bad bodies each answer 400 with the right code; an oversized id is refused as unknown_video by catalog membership rather than by a length branch, and a rejected body writes no state file at all.
• Kill switch — /next and /status both report enabled: false with no video and no state.
• Restricted session — /next returns null while still reporting enabled: true; feedback writes no file; /status serves an empty state map to a read-blocking session but still serves its own map to an incognito one, so the product's read/write split cannot collapse.
• State robustness — 7 malformed-file shapes degrade to empty, a non-numeric ts is zeroed rather than dropping the row, a 400-digit integer ts (which raises OverflowError, not ValueError) does not 500 the loader, and neither does a document nested past the recursion limit (RecursionError) in any of the three files this module reads.
• Probes — each of the four in isolation, including that config_key_set does not fire on a shipped default, that tips_feedback_exists fires on an opt-out whose collections are all empty, that a corrupt tips state file is tolerated, and that the SEL read is bounded to the declared limit.
• Catalog invariants — every shipped entry validates, ids are unique, docs are in the tips allowlist, assets follow <id>.mp4 / <id>.jpg, an invalid entry is filtered rather than raised, and used_when never reaches the client payload.
• Concurrency — two threads recording different videos through a barrier both survive.
• Wiring — the three routes are registered, the config default is on, the loader reads the key, an absent key keeps the default, and a non-bool value (the string "false") keeps the default rather than reading as on.

Manual verification

N/A — unit coverage sufficient. Every route is exercised end-to-end against the real state file through the same helpers the gateway calls, and the one thing tests cannot cover is playback of clip files this PR deliberately does not ship (the parallel frontend change ships them, and the frontend session drives that verification).

Related Issues

no linked issue: one work item of a larger "startup feature-intro videos" change, tracked outside the issue tracker.

Checklist

• At most two commits (one is the norm), with a Conventional Commits title
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated — src/kiro_crew/docs/feature-videos.md (new, indexed in both docs indexes), the dashboard.feature_videos_enabled row in configuration.md, and the owning spec docs/system-specs/modules/learn-cron-dashboard.md
• No secrets, credentials, or internal references in the diff

Round 2 (head 910d98a7a)

The GPT lane returned one BLOCKING and one advisory finding on 9f4534f13. Both were legitimate one-line fixes and both are fixed; each has its own disposition comment on this PR.

json.loads raises RecursionError, which is neither OSError nor ValueError, so a state file nested past the recursion limit escaped the loader's except and 500'd every endpoint — against load_state's own documented promise to degrade. The fix catches it in all three readers of an operator-writable file, not only the line the finding named, because fixing one would have left the same 500 one probe away on the same request path. The advisory finding was bool("false") is True: a kill switch an operator wrote as a string stayed on. That line now parses through _safe_bool.

One further defect was mine, found by the full backend suite rather than by a reviewer: test_opt_out_withdraws_the_tips_video ran against the SHIPPED catalog, so its second entry's sel_event_seen probe read the host's real audit log and any box that had ever called monitor_start failed the assertion. The test now pins the catalog to the tips entry plus one signal-free control, so it cannot reach the audit log at all.

Round 3 (head 8c4d7d900)

Readiness passed on 910d98a7a with every lane green. The First Principles lane returned advisory CONCERNS naming three subtractions, and one of them was right in a way that only stays cheap before a client exists: the video_id_too_long branch shipped a second permanent error code for a case the unknown_video catalog-membership check already covers, and membership is the strictly tighter bound. That branch is gone; a 101-character id now answers unknown_video, and a new test sends 5000 characters to show one code covers every non-slug id. _VIDEO_ID_MAX_CHARS remains only as the catalog validator's own bound.

The other two subtractions — drop min_version, drop the two probes no catalog entry names — were accurate counts but are rebutted: both are the API surface this work item specified, the parallel frontend is being built against the payload as frozen, and re-adding a payload key later is the narrow-later problem the same review correctly raises about error codes. Each subtraction and both Watch items have their own reasoning in a disposition comment on this PR.

Round 4 (head 69c7b9d69)

Two things, one of them not this PR's.

The CI red on 8c4d7d900 was main's, not this branch's. test/test_security.py::TestIsSensitiveBashCommand::test_chained_cd_expansions_do_not_blow_up_the_gate patched kiro_crew.security._dir_holds_sensitive_leaf, a helper #9089 had removed, so it failed on Backend Tests shard 3 for Linux and Windows alike and took the Coverage Gate down with it (coverage-combine skips when the backend lane fails, and the gate fails closed). It reproduced on a pristine origin/main worktree with this diff nowhere in it, and it is filed as #9184. Main's own repair landed as cbdd4a569; rebasing onto it clears the red with no change to this diff, and the previously-red test now passes locally on the new base.

The GPT lane's one advisory finding was legitimate and is fixed: /status returned the full engagement-history map to any session, so a restricted session could read which features this owner had watched. It now gates on _blocks_reads_session rather than the suggested _is_restricted_session, because that is the product's own split — incognito withholds writes, a temporary session withholds reads too — and the broader predicate would also have blanked an incognito session's own settings panel. enabled stays truthful to every session, since the kill switch is configuration rather than history.

Local verification

All 47 resolved profile gates green on this head, including check_black_formatting, check_sync_io_in_async, check_loop_bound_locks, check_brand_name, check_harness_parity, check_changelog_history, docs-lint, scrub-lint --no-history, verify_vendor_manifest, isort, flake8, and mypy (1304 files clean).

Full backend suite: 90127 passed, 96 failed. All 96 are pre-existing host-environment failures on this box, unchanged in count and class from the same suite run against this branch before the review round — /local/home owned by uid 65534, xdist host-budget assertions reading the live core count, Electron symbols-manifest paths, and a gh binary probe. None are in test/test_feature_videos.py or any file this diff touches.

Both local review lanes (GPT contract, Opus contract) returned PASS with no blocking findings. Five non-blocking findings were legitimate and are fixed in this head: the tips opt-out signal, the missing write-side restricted-session gate, the OverflowError on an oversized persisted timestamp, the unsynchronized read-modify-write, and three in-function imports now hoisted. One finding was rebutted — the absent asset files are the parallel frontend change's to ship, by design. One half of another was rebutted: adding config_key_set:dashboard.tips_enabled to the tips entry would fire on presence, so a user who explicitly set that key to true would lose the clip.

[github-actions]

Design Review (Fable 5) — ✅ PASS

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

Design-Verdict: PASS

Deterministic catalog + permanent local state is the right shape for shipped-artifact intros, correctly not the tips engine; boundaries, failure directions, and the split-PR contract are all reasoned and tested.

[DESIGN-REVIEWED] 69c7b9d

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 69c7b9d6908ff4cc4f76c2bd996d884421f789da — 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 is in: the contract, the intent file, the full patch, and repo greps to verify consumer counts (min_version usage, tips' _sanitize_tip_action, app-assets validators). The change is coherent, but several pieces of shipped surface have zero consumers even within the declared two-PR plan. Final review follows.

First-Principles-Verdict: CONCERNS

min_version, two of four probes, and the feature field ship with zero consumers — scaffolding for catalog entries that don't exist yet.

What this change ships

Intent: let a fresh install be shown a short intro clip for a feature it hasn't used, at most once, ever — an ADDITION (backend half; frontend rides in a parallel PR).

1. GET /api/feature-videos/next picks the next unseen clip — justified; zero consumers until the sibling PR, declared
2. POST /api/feature-videos/feedback records permanent seen/dismissed — justified
3. GET /api/feature-videos/status returns the whole seen/dismissed map — serves a "reset" that exists nowhere; zero consumers
4. Config key dashboard.feature_videos_enabled, default on — justified operator kill switch
5. New persisted file feature_videos_state.json — justified; it is the feature's memory
6. Two-entry catalog whose clips 404 until the asset PR merges — declared
7. used_when probe registry: 4 probes shipped, 2 wired — artifacts_nonempty, config_key_set have zero consumers
8. min_version gate plus payload field — zero consumers; guards an unrepresentable state
9. feature payload field — equals id in 2 of 2 entries
10. feature-videos.md + config-doc + spec rows — justified by the same-commit spec rule

Watch

• The catalog is a static tuple compiled into the same package as kiro_crew.__version__, so an entry's presence and the running version travel together — the state min_version guards ("clip for a feature this build lacks") cannot arise; add the entry in the release that has the feature. Count: 0 of 2 entries set it; the doc's "recorded ahead of a release" scenario is a workflow nobody is constrained into.
• api_feature_videos_status justifies returning the full state map with "render (and later reset) what has been seen" — no reset route exists in this PR or the frozen contract, so the map's only named consumer is future work.
• The whole API is consumer-less until StartupVideoModal merges; declared honestly, but if that PR stalls, this is dead surface behind a live default-on config key.

Subtractions

• Drop min_version, _version_ok, the parse_version import, the payload key, and the doc rows — 0 catalog consumers (grepped min_version in CATALOG: both entries default ""); reintroduce with the first entry that needs a floor.
• Drop _probe_artifacts_nonempty and _probe_config_key_set — 0 used_when references (catalog uses only tips_feedback_exists and sel_event_seen:monitor_start); each probe can land with the entry that needs it.
• Drop the feature field from VideoEntry — identical to id in 2/2 entries; use id.
• Shrink /status's state map to nothing (or defer the route) until the settings panel that reads it exists; /next already carries enabled for the modal's off-vs-exhausted split.

[FIRST-PRINCIPLES-REVIEWED] 69c7b9d

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 69c7b9d6908ff4cc4f76c2bd996d884421f789da — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 69c7b9d

Verdict parsed from the review's SHA-scoped output markers for commit 69c7b9d6908ff4cc4f76c2bd996d884421f789da.

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

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 69c7b9d6908ff4cc4f76c2bd996d884421f789da and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 69c7b9d

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

[iamwhatever]

• span=9c325bfaec09 — src/kiro_crew/feature_videos.py:442 — Deeply nested state crashes every feature-video endpoint — fixed in 910d98a7a.

Not overridden, even though the machine-drafted override rationale is factually right about reachability. It named the reason to fix rather than the reason to waive: the module's own contract says this loader degrades.

Deeply nested valid state JSON -> load_state() -> uncaught RecursionError -> HTTP 500.
Fix: Add RecursionError to the exception tuple.

json.loads raises RecursionError, which is neither OSError nor ValueError, so the existing tuple let it through. load_state's docstring promises the opposite — "degrading to empty on anything unexpected", per-entry validation precisely so "a hand-edited file cannot 500 every endpoint". A file an operator can edit, plus a documented guarantee that editing it degrades, is a guarantee that did not hold.

RecursionError is now caught in all three JSON readers that take a file an operator can edit, not just the one the finding named — load_state, _probe_tips_feedback_exists (tips_state.json), and _probe_config_key_set (config.json / config.local.json). Fixing only line 442 would have left the same 500 one probe away, on the same request path. This is the third instance of one class in this file (OverflowError on an oversized ts, then a non-dict root, now RecursionError), so the invariant is stated once here: every JSON read of an operator-writable file in this module degrades to "no state", and no exception from json.loads or a value coercion escapes to the handler.

Three regression tests, one per reader, each writing a document nested sys.getrecursionlimit() * 3 deep.

[iamwhatever]

• span=15137de977ab — src/kiro_crew/config/loader.py:3340 — a configured string "false" is truthy in bool(...), leaving feature videos enabled — fixed in 910d98a7a.

Fix: Parse with _safe_bool(..., True).

Taken as written. The line now reads _safe_bool(dashboard_data.get("feature_videos_enabled"), True), so a non-bool value keeps the shipped default instead of being coerced. This matters more for a kill switch than for an ordinary flag: bool("false") is True, so an operator who wrote "false" in config.json intending to turn the feature off got it left on, and the config would have read as if their edit had taken effect.

Regression test asserts the string "false" resolves to the default rather than reading as "on".

Scope note: tips_enabled on the line directly above has the same bool(...) shape and the same latent behaviour. It is untouched here — it is not this PR's line, and widening the diff into a neighbouring flag would put a behaviour change to Feature Tips inside a PR about feature videos. Worth a follow-up sweep across the dashboard block's remaining bool(...) parses.

[iamwhatever]

First Principles Review — 🟡 CONCERNS — one subtraction taken, two rebutted, both Watch items answered. Head 8c4d7d900.

The premise is accepted as stated: merged alone this ships an API with no in-tree caller. That is the declared shape of the work item — the frontend StartupVideoModal and the clip assets are a parallel change, and the API contract in this PR was frozen in advance precisely so the two halves can be built at once. Merge order is a maintainer call, and I have made it explicit at the top of the PR body rather than leaving it implied.

Subtraction 3 — drop the video_id_too_long branch — fixed in 8c4d7d900.

the unknown_video catalog-membership check right below it already refuses every non-catalog id, so no unbounded key can reach the state file — and by the author's own rule a code is surface that cannot be narrowed later.

Correct, and it turned my own argument against me properly. Catalog membership is strictly tighter than a character count, so the length branch could never be the reason any id that mattered was refused, while shipping a second permanent code for a case the first one fully covers. It is removed, the parametrized case for a 101-character id now asserts unknown_video, and a new test sends a 5000-character id to show one code covers every non-slug. _VIDEO_ID_MAX_CHARS survives only as the catalog-entry validator's bound, which is its honest remaining use. Now was the only cheap moment: no client exists yet, so removing the code breaks nothing.

Subtraction 1 — drop min_version — rebutted (disproportional here, not wrong).

0 of 2 catalog entries set it. Re-add with the first clip that actually needs a floor.

The count is right and the general instinct is right. It does not apply because this field is not authored generality — it is part of the API contract this work item was given, and the payload key is consumed by the parallel frontend that is being built against it right now. Removing and re-adding a payload key is exactly the narrow-later problem the review correctly cites for a code. The floor also earns its place for this feature specifically: a clip is a shipped binary that can be recorded ahead of the code it demonstrates, so "do not play this on a build that lacks the feature" is the one eligibility rule the catalog cannot express any other way.

Subtraction 2 — drop artifacts_nonempty and config_key_set — rebutted (same grounds).

0 catalog entries name either.

Also accurate. Both probes were named in the work item as the registry's initial set, so they are the specified surface rather than speculative extension, and each has tests that pin its real behaviour — config_key_set in particular encodes a non-obvious decision (it reads the config FILES, so a shipped default cannot fire it), which is worth keeping proven rather than rediscovering. They are four lines of registry each.

Watch — feature duplicates id in both entries. True of these two clips and not of the schema: both seed features happen to have doc slugs matching their ids. feature is the feature's identity as the docs name it and id is the state key and asset basename, and the frontend renders the former. They will diverge as soon as a clip covers a feature whose id is not its slug.

Watch — merge order decides whether /next offers a 404 clip. Accepted, and worth being blunt: if this lands alone with the kill switch on, /next returns an entry whose src 404s. Nothing crashes, and the modal is the thing that would show it, so there is no user-visible defect until the frontend lands — but a maintainer merging this first should know that is the window.

[iamwhatever]

• span=3f9c2ca9f49c — src/kiro_crew/feature_videos.py:589 — restricted sessions reach load_state and receive persistent engagement history — fixed in 69c7b9d69, with a narrower gate than the one suggested.

Fix: return empty state when _is_restricted_session is true.

The defect holds. /status returned the whole id -> {status, ts} map to any session, and that map is engagement history — which features this owner watched and when. /next was gated and /status was not, so the module's own claim that a restricted session "neither sees clips nor leaves a row" was only half true.

The suggested predicate is the wrong half of the product's split, so I used the other one. _is_restricted_session covers incognito AND temporary; incognito's contract is that it withholds writes, while a temporary session withholds reads as well — and _blocks_reads_session is the existing helper for exactly that. Gating a read on the broader predicate would also blank the panel for an incognito session reading its own instance's settings, which nothing asks for. So /status now gates on _blocks_reads_session, and /next keeps _is_restricted_session because reaching it leads to a permanent write.

enabled is still reported truthfully to every session. The kill switch is instance configuration rather than history, and withholding it would make the settings panel claim the feature is off when it is on — a worse answer than the one being protected.

Two tests: a read-blocking session gets {"enabled": true, "state": {}}, and an incognito-but-not-read-blocking session still gets its own map, so the two halves of the split cannot silently collapse into one.

[iamwhatever]

First Principles Review — 🟡 CONCERNS (round 3 on this span) — one argument conceded on mechanism, four subtractions held, and one question put to the maintainer. No code change this round, deliberately.

min_version — the sharpened argument is CORRECT on mechanism, and I am not acting on it unilaterally. needs-a-decision.

The catalog is a static tuple compiled into the same package as kiro_crew.__version__, so an entry's presence and the running version travel together — the state min_version guards ("clip for a feature this build lacks") cannot arise.

This is a better argument than the count, and it defeats the defence I gave last round. I said a clip is a shipped binary that can be recorded ahead of its code; that is true of the ASSET but not of the ENTRY, and the entry is what carries the floor. Compiled into the same wheel, an old build cannot contain a new entry, so the guard is unreachable today.

I am still not removing it in this PR, and the reason is not the finding — it is who owns the call. min_version is in the payload contract that was frozen in advance so the parallel StartupVideoModal change could be built against it while this half was built. Removing a payload key mid-flight breaks the session coding against it right now, and the work item that specified this API named min_version explicitly.

Maintainer, this is the one decision I am asking for: trim the contract to only what has a consumer today (drop min_version, feature, and the two unwired probes, and coordinate the payload change with the frontend PR), or keep the frozen contract and let the first consuming entry justify each field? I will act either way. Left alone, the fields ship unused but harmless; trimmed, the two PRs must land together.

artifacts_nonempty and config_key_set — rebutted, unchanged. Both were named in the work item as the registry's initial set, so they are specified surface rather than authored generality, and each has tests pinning behaviour that is not obvious — config_key_set in particular encodes that it reads the config FILES, so a shipped default cannot fire it. That is a decision worth keeping proven rather than rediscovering. Four lines of registry each.

feature duplicating id — rebutted. Identical in 2 of 2 entries, as observed, and that is a property of these two clips rather than of the schema: both seed features have doc slugs matching their ids. feature is the feature's identity as the docs name it, id is the state key and the asset basename. They diverge as soon as one clip covers a feature whose id is not its slug, and collapsing them now would put a rename of either concept into the state file's key space.

Shrink /status's state map — rebutted, and it was just narrowed for a different reason. The map is /status's only substantive content, and the route is in the frozen contract for the settings panel. It also stopped being unconditional this round: the GPT lane found that a restricted session could read it, so it now returns {} to a read-blocking session while enabled stays truthful. Emptying it for everyone would leave a route that answers only what /next already carries, which is the argument for deleting the route rather than hollowing it — and the contract keeps it.

On the round count. This is the third round in which this lane has proposed the same subtractions, and each one now trades against a contract another in-flight change depends on rather than against code I can freely edit. Per the loop's same-span rule I am not patching further here; the invariant that answers all of them at once is the maintainer decision above.

[iamwhatever]

Review-ready — final summary

Head 69c7b9d6908ff4cc4f76c2bd996d884421f789da, one commit on main, all 64 checks green, PR Readiness passed, mergeable, 0 unresolved threads. Not merged and auto-merge not armed.

What this is. The backend half of startup feature-intro videos: a static catalog, a deterministic eligibility rule set, and three routes registered beside the tips routes. The frontend StartupVideoModal and the clip files ride in a parallel change, so nothing in website/ calls these endpoints yet — the merge-order note is at the top of the description.

Reviewer rounds, and what each changed.

Lane	Verdict on this head	Rounds it changed the code
GPT 5.6	✅ no blocking findings	3 findings fixed across rounds 2 and 4
Opus 4.8	✅ no blocking findings	5 findings fixed pre-push, from the local pass
Design	✅ PASS	—
First Principles	🟡 CONCERNS (advisory)	1 subtraction taken, 4 held, 1 question open

Every finding has its own disposition comment. Fixed, in order: RecursionError escaping three JSON readers of operator-writable files (a documented degrade that did not hold); bool("false") leaving the kill switch on; the redundant video_id_too_long code that catalog membership already covered; /status serving engagement history to a restricted session; and, from the local pass before the first push, the tips opt-out signal, a missing write-side session gate, an OverflowError on an oversized timestamp, an unsynchronized read-modify-write, and three in-function imports.

One decision is open for a maintainer, in the round-3 First Principles disposition: keep the frozen cross-session API contract, or trim min_version, feature and the two unwired probes to only what has a consumer today. The lane's mechanism argument against min_version is correct — the catalog is compiled into the same wheel as the version, so the floor it guards is unreachable — but removing a payload key breaks the frontend change being built against the frozen contract right now. Readiness does not depend on it; both fields ship unused and harmless if left alone.

Two reds on this PR were never this PR's. test/test_security.py::TestIsSensitiveBashCommand::test_chained_cd_expansions_do_not_blow_up_the_gate patched a helper #9089 had removed, failing Backend Tests shard 3 on Linux and Windows and taking the Coverage Gate with it; it reproduced on a pristine origin/main worktree, is filed as #9184, and was cleared by rebasing onto main's own repair (cbdd4a569). The local full backend suite also reports ~96–269 failures on this box across session_storage, trash, acp_client and host_isolation_floor; an identical run on pristine origin/main produced the same set, so none of it is attributable here. No failure in test/test_feature_videos.py or any file this diff touches at any point.

106 tests in test/test_feature_videos.py; every resolved profile gate green on this head.

[bolichen97]

Tech Lead review — approve.

Per-user scoping of display state: correct. State is config_dir()/feature_videos_state.json, written through atomic_write(restrict_to_owner=True) (0600 on POSIX, owner-only DACL on Windows), sitting beside tips_state.json and reached through the same config_dir() helper — so it inherits KIROCREW_HOME expansion and unsafe-system-directory rejection rather than a raw environ read. That is per-install owner state, which is the only user boundary KiroCrew's owner-gated dashboard has; there is no global or shared 'has seen' flag and no cross-account surface to leak into. The restricted-session split is the right half on each side: /next and feedback gate on _is_restricted_session (temporary and incognito neither see a clip nor leave a row), while /status gates on the narrower _blocks_reads_session so an incognito session still renders its own settings panel. Gating the write side matters more than the read side here, since the write is what creates the permanent row.

Scope: proportionate. 11 files, +1639/-0, no deletions, one commit. The 659-line module is one new concern; 866 of the added lines are tests. Nothing existing is refactored — three routes registered beside the tips routes, one config key added where tips_enabled is defined and parsed, and a regenerated config-baseline.json row.

Blocking findings: none unresolved. GPT 5.6 and Opus 4.8 both report no blocking findings on 69c7b9d69; Design is PASS; First Principles is advisory CONCERNS with every subtraction dispositioned in a per-finding comment. Zero human reviews requesting changes, zero unresolved threads. The two GPT findings that were real got fixed rather than waived — RecursionError escaping load_state's documented degrade-to-empty contract (fixed in all three readers of an operator-writable file, not just the flagged line), and bool("false") is True leaving a kill switch on (now through _safe_bool). The /status engagement-history leak to restricted sessions was also a genuine find and is fixed. I accept the min_version needs-a-decision item as a maintainer call: keep it. The counter-argument that catalog entry and running version travel together is correct on mechanism, but this is a frozen contract the parallel frontend is being built against, and re-adding a payload key later is the same narrow-later cost that review rightly flags about error codes.

i18n: nothing owed. The diff touches zero website/ files, so no locale catalog obligation and the ux-review-required ruleset does not apply. The catalog's English title/description are backend-supplied display copy, which is exactly what tips.py already serves through its title/body fields — a pre-existing repo-wide pattern, not a regression this PR introduces.

Sequencing vs #9169: no base dependency, but an order. #9169's base is main, not this branch, and the two diffs share zero files — so nothing is stacked and neither blocks the other mechanically. The logical order is still backend-first: #9169 is the half that calls these endpoints and ships the clip files. Merged alone, this PR's /next would return an entry whose src 404s, but no in-tree caller exists yet, so no user reaches it. The reverse order would ship a modal calling routes that do not exist. Merging this first is the safe direction.

CI. 56 success / 9 skipped on the head. The six cancelled check-runs (Automated Rule Check, GPT 5.6 Review, Inclusive Language, PR Hygiene, SAST, Screenshot Evidence) each have a later success run on the same SHA — superseded, not an unresolved gap, so the readiness signal is honest.