chore(config): default feature videos off until real clips ship

[iamwhatever]

Problem / Motivation

Two ways the feature-video dialog can show a user something nobody should see yet.

It ships on. The backend landed in #9168 with dashboard.feature_videos_enabled
defaulting to true. Nothing shows yet only because no frontend mounts the dialog.
The moment #9169 merges, every install starts playing a clip at startup — a feature
nobody has used end to end, with placeholder media rather than real recordings.

It offers clips that are not on disk. _entry_is_valid checks the shape of an
entry's src and poster but never whether the file exists. The catalog already
names feature-tips.mp4 and monitor-loops.mp4, and neither has been recorded.

Why it matters

A startup dialog is the most intrusive surface this app has, and its verdict is
permanent: watch or close a clip and that clip never comes back.

The second problem is the sharper one, because the frontend cannot defend against
it. The dialog opens on the JSON answer alone, and its <video> is preload="none"
— nothing is fetched, and no media error can fire, until the user presses play. So an
entry whose clip is not shipped opens a real dialog titled "Feature tips above the
composer" around a blank player. The natural "Got it" writes a permanent seen, and
the real intro is retired before anyone could see it. Design Review on #9169 found
this and named the backend check as the smaller, more general fix; it is.

What changed (motivation → approach → change)

Both changes sit in the same gate and change nothing about selection order, the
probes, or what a verdict means.

The default flips to false in the two places that decide it: the dataclass
field (config/sections.py) and the loader's fallback for a config that does not
mention the key (config/loader.py). config-baseline.json is regenerated from the
dataclass registry — it records the default in two spots, so a hand-edit would have
missed one. To turn it on: { "dashboard": { "feature_videos_enabled": true } } in
~/.kiro/crew/config.json.

"Asset shipped" becomes a precondition of "on offer". A new offerable() is
catalog() filtered to entries whose clip AND poster exist on disk under
static/dist/app-assets — the directory server.py mounts at /app-assets.
select_next walks offerable(). An entry it withholds is logged and left alone:
nothing is written, so it comes back on its own the launch after its clip lands.

catalog() deliberately keeps its structural-only meaning. The feedback route checks
membership against it, and a user who was shown a clip must still be able to record a
verdict on it even if its asset later goes missing. The two sets differing is pinned
by a test.

flowchart LR
  subgraph Before
    A1[CATALOG]:::ctx --> B1[_entry_is_valid<br/>shape only]:::ctx --> C1[select_next]:::ctx --> D1[dialog opens<br/>blank player]:::removed
  end
  subgraph After
    A2[CATALOG]:::ctx --> B2[_entry_is_valid<br/>shape only]:::ctx --> E2[offerable<br/>clip + poster on disk?]:::added --> C2[select_next]:::ctx
    E2 -. withheld, nothing written .-> F2[offered again<br/>once clip lands]:::added
  end
  classDef added fill:#DCFCE7,stroke:#16A34A,color:#14532D,stroke-width:2px
  classDef changed fill:#FEF3C7,stroke:#D97706,color:#78350F,stroke-width:2px
  classDef removed fill:#FEE2E2,stroke:#DC2626,color:#7F1D1D,stroke-dasharray:4 3
  classDef ctx fill:#E0F2FE,stroke:#0284C7,color:#0C4A6E
  linkStyle 2 stroke:#DC2626,stroke-dasharray:4 3
  linkStyle 4,5,6 stroke:#16A34A,stroke-width:2px

Loading

🟩 added · 🟨 changed · 🟥 removed · 🟦 unchanged

A clip with no file on disk is now withheld by the backend, so the dialog never
opens around a blank player.

Both docs pages say false is the default, and the feature-videos page's selection
rule now names the shipped-asset precondition.

This is a pause, not a retreat: land this, turn it on locally, record real clips,
then flip the default back to true in its own change. The existence gate stays.

Tests

test/test_feature_videos.py::TestConfigFlag pinned the old default in three places;
all three now pin the new one. test_a_non_bool_value_keeps_the_default is more
load-bearing now: bool("false") is True, so without _safe_bool that config would
turn the feature ON against the operator's wishes.

An autouse fixture treats every asset as shipped, so the 100+ existing selection,
probe and route tests keep testing what they were written for instead of all
exercising the withholding branch. TestAssetExistenceGate (7 cases) replaces that
default with a real temp directory and covers: the /app-assets/... → disk mapping
(and that an off-prefix path is never "present"); withholding on a missing clip;
withholding on a missing poster alone; a shipped entry being offered; selection
skipping past an unshipped entry to a shipped one; the entry coming back once its
clip lands; and catalog() still containing what offerable() withholds.

Mutation-verified: routing select_next back through catalog() fails 3 of the 7.
123 tests pass across this file and test_config_baseline.py.

Manual verification

N/A — unit coverage is sufficient. The default is read through the real loader from
a real config file, and the existence gate is exercised against a real directory.

Blast radius, checked: _enabled() (feature_videos.py:555) is the flag's only
consumer in src/; select_next is the only caller that moved to offerable(), and
the feedback route's membership check stays on catalog() by design.

Related Issues

no linked issue: pre-launch gating, not a tracked defect.

Sequencing note: this should land before or with #9169. Merging #9169 first opens
a window where an unvalidated feature is on for everyone, offering clips that do not
exist.

Checklist

• At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable)
• No secrets, credentials, or internal references in the diff

[iamwhatever]

Intent: Stop feature videos from switching themselves on for every install the moment the frontend merges. The kill switch already works; only its default direction changes, so turning the feature on becomes a decision someone makes after looking at it.

Not a goal: Removing or weakening the feature, the catalog, or the switch itself. Recording real clips. Any change to how selection, the probes, or the permanence of a verdict behave. Flipping the default back — that is a later change, once real clips exist.

[github-actions]

Design Review (Fable 5) — ✅ PASS

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

Design-Verdict: PASS

Backend existence gate is the right layer — the only place that can know — and withholding-without-writing makes every failure mode recoverable.

Suggestions

• _asset_root() re-derives server.py's mount by convention; a test pinning it against the path server.py actually mounts would catch a silent drift where every clip gets withheld.

[DESIGN-REVIEWED] f73ab14

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] f73ab14

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

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

[github-actions]

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of f73ab1464ed3efc4dd0463b6cf9e1dcf92585716 — 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.

Both counts from the description verified: the flag has one consumer (_enabled, feature_videos.py:605), select_next (line 590) is offerable()'s only caller, and the feedback route (line 696) stays on catalog(). I checked for a pre-existing runtime asset-existence mechanism and found none — the only existence checks for /app-assets/ paths anywhere in src/ are test-time manifest pins (apps/builtins/*/tests/test_manifest.py); the static route in server.py:1303 404s a missing file but cannot stop the dialog opening, since the <video> is preload="none". The gate sits at cause level: the divergence class is "catalog is code, assets are build files", and deleting the two unshipped entries instead would only remove today's two instances while every future entry re-creates the risk.

First-Principles-Verdict: PASS

Both items are derived from one nameable defect — a blank-player dialog writes a permanent seen — and no existing mechanism could catch it.

What this change ships

Intent: stop an unfinished startup-dialog feature from reaching users before its clips exist — a FIX (pre-launch gating).

1. Feature videos now off by default; opt-in via config — justified (would auto-enable for every install the moment feat: show a feature-intro video once at startup #9169 merges)
2. A clip not on disk is never offered, and returns by itself once shipped — justified (frontend provably cannot detect it; verdicts are permanent)
3. New public offerable() beside catalog(); feedback route deliberately stays on catalog() — declared; 1 consumer each, counted
4. Withheld entries logged with the missing paths — declared; silent withholding would be undiagnosable
5. Both docs pages restate the default and the shipped-asset precondition — mandated (AGENTS.md same-commit doc rule)

Verification notes: flag consumers in src/ = 1 (_enabled); offerable() consumers = 1 (select_next) — singular already, nothing generalized; no duplicate mechanism found (grep app-assets across src/: only test-time manifest existence checks). The delete alternative (drop the two unshipped CATALOG entries) was considered and rejected here: it is the point patch — the gate removes the whole catalog/asset-divergence class, which is the cause.

[FIRST-PRINCIPLES-REVIEWED] f73ab14

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] f73ab14

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

[iamwhatever]

CI note: the three reds on this head are one inherited failure, not this diff

Backend Tests (3.12, 4), Backend Tests (Windows) (4) and Coverage Gate all trace
to the same two test cases, and they fail the same way on main with this PR nowhere
near them.

The failure

test/test_usage.py::TestParseSessions::test_empty_session_stats[True]  - AssertionError
test/test_usage.py::TestParseSessions::test_empty_session_stats[False] - AssertionError

It is on main, verified by running it there. I checked out pristine
origin/main (678fc3266) in a throwaway worktree and ran those two cases alone:

2 failed  --  test/test_usage.py::TestParseSessions::test_empty_session_stats[True]
                                                                          [False]

Identical assertion, no changes from this branch present. That is stronger evidence
than comparing CI runs, because shard composition moves between runs and this does
not depend on it.

Root cause, since the diff makes it easy to name. _parse_sessions() returns a
key the test's expected dict does not list:

+     'refused_transcripts': 0,

The dicts are otherwise equal, so the extra key is the whole failure. (The
avg_msgs_per_session: 0 vs 0.0 lines in the diff output are a red herring —
0 == 0.0 is true in Python and does not fail a dict comparison.) So production
gained refused_transcripts and test_empty_session_stats was not updated with it.
The fix belongs to whoever added that key, in its own change.

Why Coverage Gate is not a separate red. Its log ends
##[error]backend-test=failure -- failing closed. — it gates on the backend test job
rather than measuring anything itself, so it goes green when that job does.

Why this diff cannot reach it. This PR changes the feature_videos_enabled
default (config/sections.py, config/loader.py), the regenerated
config-baseline.json, two docs pages, and test/test_feature_videos.py.
_parse_sessions() reads a sessions directory the test patches to a tmp_path and
touches no config dataclass, no loader, and nothing about feature videos —
test/test_usage.py contains zero references to them.

I did try the re-run first, as asked. gh run rerun --job answered
job ... cannot be rerun and --failed answered This workflow is already running,
so I ran the test against main directly instead, which settles it either way.

The gates that DO cover this diff are green: 116 tests across
test/test_feature_videos.py + test/test_config_baseline.py, plus black, isort,
flake8, mypy (1326 files), docs-lint and the changelog-history gate.

[iamwhatever]

Scope change, by owner decision — not an edit to the intent above.

The intent comment said "no change to how selection behaves". This head (545806abcbf8) adds one: select_next now walks offerable(), which withholds a catalog entry whose clip or poster is not on disk. The owner ruled that this backend gate lands here rather than in a third PR, after Design Review on #9169 showed the frontend cannot defend against an unshipped asset (preload="none" means no media error can fire before the dialog opens, and _entry_is_valid never checked file existence).

Left as posted so later retrospectives measure against the original; this note is the record of when and why the scope grew. Selection ORDER, the probes, and verdict permanence are still untouched — the gate only removes entries from consideration, and catalog() keeps its structural-only meaning so the feedback route still accepts a verdict on a clip whose asset later vanished.

[chenmingwei23]

Tier 1 auto-approve: chore/config (7 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: chore(config) default-off config flag for feature videos, config loader/sections + docs + tests, no runtime security impact.