feat: broaden E2E visual review trigger and add review-page build skill

[geojitsu]

Intent

Broaden the fleet-wide E2E visual review convention (captain-authorized amendment on top of the already-checks-passed PR #3) in two ways, applied on the same branch/PR as an amendment, not a new task: (1) Broaden the trigger from literal 'runs the project's E2E test suite' to any test run that exercises the running application - E2E suites, UI/browser-driven tests, and visual regression checks - explicitly excluding plain unit/logic tests, which have nothing meaningful to screenshot and keep reporting pass/fail as before. Updated: .agents/skills/e2e-visual-review/SKILL.md's Scope section and frontmatter description; both '# E2E Visual Review' heredoc sections in bin/fm-brief.sh (ship and scout); AGENTS.md's data//e2e-review/ layout description and its plain-language pointer sentence in section 7. Decision: kept the skill's name e2e-visual-review rather than renaming it (e.g. to something like app-visual-review) - the scope text itself is now unambiguous about covering more than literal E2E suites, and a rename would touch more surface (directory path, brief pointers, doc-audience inventory, tests) for no behavioral gain; this was a deliberate judgment call the captain explicitly left to me ('your call - a rename is not required if the scope text alone makes it clear, but flag your reasoning either way'). (2) Closed a real gap the captain hit in practice: no documented procedure existed for firstmate to actually build the captain-facing review page from evidence crewmates leave at data//e2e-review/ - the crewmate skill explicitly stops at 'producing evidence; building the page is firstmate's job' but nothing said how or when. Added a new agent-only skill .agents/skills/app-test-review/SKILL.md (same shape/frontmatter as other section-13 skills: internal:true, user-invocable:false) specifying: firstmate builds the review page whenever a task with data//e2e-review/ evidence reaches a captain-facing checkpoint (PR ready or done) - the default artifact, not optional; the page lives at .lavish//-/review.html inside firstmate's own home (captain-private, gitignored, durable across worktree teardown, unlike the source evidence which does not survive /stow-style cleanup the same way), where is feature-review or bug-detection inferred from the change; screenshots/video must be embedded directly (data: URIs or otherwise self-contained per Lavish's own constraints), never merely linked, using the crewmate's manifest.md to caption/order them; built via lavish-axi per its own conventions (playbook guidance, help output) so the captain gets the normal interactive Lavish review surface rather than a static dump. Wired this new skill into AGENTS.md: a new .lavish//... entry in section 2's layout listing, an inline pointer in section 7's 'PR ready, landing, and teardown' subsection (the exact captain-facing checkpoint where it must fire), and a section-13 trigger-index entry mirroring decision-hold-lifecycle's dual placement (both a section-13 entry and an inline pointer at its precise operational trigger). Also fixed, while already touching this exact wording: the ship brief's Rule 2 still said 'Stay inside this worktree; modify nothing outside it', directly contradicting its own later E2E Visual Review section which already declares data//e2e-review/ a permitted worktree-isolation exception; the scout brief had already been corrected for this same contradiction by the prior no-mistakes document-step fix on this branch, so I brought ship's Rule 2 wording in line with that same established pattern. Added regression-locking test assertions in tests/fm-brief.test.sh for the broadened scope wording and the corrected ship Rule 2 text in both existing ship/scout E2E-pointer tests. Updated docs/documentation-audiences.json's surface inventory for the new app-test-review skill (required by this repo's own fm-doc-audience-check.sh exact-coverage gate). Scope stayed limited to firstmate's own shared tracked material - no files under projects/ were touched, consistent with the original task.

What Changed

• Broadened the E2E Visual Review trigger from literal "runs the E2E test suite" to any test run that exercises the running application (E2E, UI/browser-driven, visual regression), explicitly excluding plain unit/logic tests; updated .agents/skills/e2e-visual-review/SKILL.md's Scope section and frontmatter description, both ship/scout # E2E Visual Review heredoc sections in bin/fm-brief.sh, and AGENTS.md's data/<id>/e2e-review/ layout description and section 7 pointer sentence.
• Added a new agent-only skill .agents/skills/app-test-review/SKILL.md (internal:true, user-invocable:false) documenting the previously-undefined procedure for firstmate to build the captain-facing review page from crewmate evidence at data/<task-id>/e2e-review/, landing it at .lavish/<project>/<type>-<task-id>/review.html with self-contained embedded media, built via lavish-axi; wired into AGENTS.md with a section 2 layout entry, an inline section 7 pointer at the PR-ready/landing/teardown checkpoint, and a section 13 trigger-index entry.
• Fixed a contradiction in the ship brief's Rule 2 ("Stay inside this worktree; modify nothing outside it") that conflicted with the already-declared data/<task-id>/e2e-review/ worktree-isolation exception, aligning it with the prior fix already applied to the scout brief.
• Added regression tests in tests/fm-brief.test.sh for the broadened scope wording and corrected ship Rule 2 text, and registered the new app-test-review skill in docs/documentation-audiences.json to satisfy the exact-coverage doc-audience gate.

Risk Assessment

✅ Low: Doc/skill/brief-text-only change with an accompanying gate script (fm-doc-audience-check.sh) that already passes; every wording change (broadened E2E scope, ship Rule 2 fix, new app-test-review skill, section 2/7/13 wiring, documentation-audiences.json entries, new regression tests) matches the stated intent exactly, no code paths or runtime behavior beyond static text are affected, and no files under projects/ were touched.

Testing

Ran the targeted fm-brief and documentation-audiences test suites (all pass, including the two new regression tests locking in the broadened scope wording and fixed ship Rule 2 text), ran the repo's exact-coverage doc-audience gate script directly (passes with the two new skills registered), and manually generated real ship/scout briefs to visually confirm the actual crewmate-facing text now carries the broadened app-facing-test scope and the corrected, non-contradictory Rule 2 wording in both modes. This is a pure documentation/agent-instruction change with no browser-rendered UI surface, so the generated-brief text transcript (the actual artifact an end-user crewmate reads) serves as the closest available product-level evidence; it's saved alongside command output for reviewer reference. No test failures, no issues, and the working tree is clean.

Evidence: Actual generated ship/scout brief text (Rules + E2E Visual Review sections)

===== SHIP BRIEF: Rules + E2E Visual Review section =====
# Rules
1. Never push to the default branch. Never merge a PR.
2. Stay inside this worktree; the only files you may write outside it are the status file below and the E2E visual-review evidence path when that section below applies.
3. Use gh-axi for GitHub operations and chrome-devtools-axi for browser operations.
4. Report status by appending one line:
   `echo "{state}: {one short line}" >> '/tmp/tmp.BKie2utvPB/state/demo-ship.status'`
   States: working, needs-decision, blocked, paused, done, failed.
   Each append wakes firstmate, so report sparingly: only phase changes a supervisor
   would act on (setup done, bug reproduced, fix implemented, validation passed) and the
   needs-decision/blocked/paused/done/failed states. No step-by-step FYI progress lines;
   firstmate reads your pane for that.
   A mid-task `working:` line (including setup complete) is nonterminal: do not end the
   turn after it; continue the same stage until a defined `done:` gate under Definition of done.
   Use `paused: {why}` - distinct from `blocked:` - ONLY when you are deliberately idling on a
   known external wait you expect to clear on its own (an upstream release, a rate-limit reset,
   a scheduled window): firstmate then leaves your idle pane alone and rechecks it on a long
   cadence instead of treating it as a possible wedge. Use `blocked:` when you are stuck and need help.
5. If you hit the same obstacle twice, append `blocked: {why}` and stop; firstmate will help.
6. If a decision belongs above the implementation worker (product choices, destructive actions, ask-user findings),
   append `needs-decision: {summary of options}` and stop. Firstmate will apply the configured authority and reply with the decision.
   When firstmate replies or a blocker clears and you resume, append `resolved: {how it was decided or unblocked}` (add the same `[key=<slug>]` if you opened it with one) so the decision or blocker is durably closed and does not keep resurfacing.
7. Never stop, restart, or update the shared `no-mistakes` daemon - it is one instance serving
   every lane/home, so restarting it kills other lanes' in-flight pipeline runs. On ANY no-mistakes
   daemon error, append `blocked: {the daemon error}` and stop; only firstmate manages the daemon.

# Project memory
If `AGENTS.md` or `CLAUDE.md` already exists, or if this task produced durable project-intrinsic knowledge, run `/home/geod/.no-mistakes/worktrees/a53c9bb8fbe9/01KYP98R61BXDA33QDN65XMRDG/bin/fm-ensure-agents-md.sh .` in the worktree.
Record only project knowledge useful to almost every future session.
For anything the codebase already shows, prefer a pointer to the authoritative file, command, or doc over copying the detail.
If you touch a project `AGENTS.md` that lacks `## Maintaining this file`, add that short self-governance section from `/home/geod/.no-mistakes/worktrees/a53c9bb8fbe9/01KYP98R61BXDA33QDN65XMRDG/bin/fm-ensure-agents-md.sh` in the same pass.
Keep it proportionate: skip `AGENTS.md` edits for trivial tasks that produced no durable project knowledge.

# Documentation
Read `/home/geod/.no-mistakes/worktrees/a53c9bb8fbe9/01KYP98R61BXDA33QDN65XMRDG/.agents/skills/project-docs/doc-routing.json` and match your change against every rule in `rules[]`.
Write or update all matching docs in the same change - not later, not in a follow-up commit.
The `directories[]` array defines the canonical project docs structure.
Load `/home/geod/.no-mistakes/worktrees/a53c9bb8fbe9/01KYP98R61BXDA33QDN65XMRDG/.agents/skills/project-docs/SKILL.md` for Docus format, docstring requirements, and file templates.
Documentation is required on every delivery tier - no-mistakes, direct-PR, and local-only alike.

# Versioning
Load `/home/geod/.no-mistakes/worktrees/a53c9bb8fbe9/01KYP98R61BXDA33QDN65XMRDG/.agents/skills/version-management/SKILL.md` for the full semantic-versioning protocol.
Bump PATCH or MINOR automatically from the branch's Conventional Commit types, in the same commit as the change; never bump MAJOR yourself - a breaking change is a `needs-decision:` finding.

# E2E Visual Review
Only if this task runs a test suite that exercises the running application - E2E, UI/browser-driven, or visual regression, never plain unit/logic tests: load `/home/geod/.no-mistakes/worktrees/a53c9bb8fbe9/01KYP98R61BXDA33QDN65XMRDG/.agents/skills/e2e-visual-review/SKILL.md` for the full capture-and-placement protocol.
`/tmp/tmp.BKie2utvPB/data/demo-ship/e2e-review/` is a permitted exception to "stay inside this worktree", alongside the status file.

# Definition of done

===== SCOUT BRIEF: Rules + E2E Visual Review section =====
# Rules
1. Never push to any remote and never open a PR.
2. Stay inside this worktree; the only files you may write outside it are the report, the status file below, and the E2E visual-review evidence path when that section below applies.
3. Use gh-axi for GitHub operations and chrome-devtools-axi for browser operations.
4. Report status by appending one line:
   `echo "{state}: {one short line}" >> '/tmp/tmp.BKie2utvPB/state/demo-scout.status'`
   States: working, needs-decision, blocked, paused, done, failed.
   Each append wakes firstmate, so report sparingly: only phase changes a supervisor
   would act on and the needs-decision/blocked/paused/done/failed states. No step-by-step
   FYI progress lines; firstmate reads your pane for that.
   Use `paused: {why}` - distinct from `blocked:` - ONLY when you are deliberately idling on a
   known external wait you expect to clear on its own (an upstream release, a rate-limit reset):
   firstmate then leaves your idle pane alone and rechecks it on a long cadence instead of
   treating it as a possible wedge. Use `blocked:` when you are stuck and need help.
5. If you hit the same obstacle twice, append `blocked: {why}` and stop; firstmate will help.
6. If a decision belongs to a human (product choices, destructive actions),
   append `needs-decision: {summary of options}` and stop. Firstmate will reply with the decision.
   When firstmate replies or a blocker clears and you resume, append `resolved: {how it was decided or unblocked}` (add the same `[key=<slug>]` if you opened it with one) so the decision or blocker is durably closed and does not keep resurfacing.
7. Never stop, restart, or update the shared `no-mistakes` daemon - it is one instance serving
   every lane/home, so restarting it kills other lanes' in-flight pipeline runs. On ANY no-mistakes
   daemon error, append `blocked: {the daemon error}` and stop; only firstmate manages the daemon.

# E2E Visual Review
Only if this task runs a test suite that exercises the running application - E2E, UI/browser-driven, or visual regression, never plain unit/logic tests: load `/home/geod/.no-mistakes/worktrees/a53c9bb8fbe9/01KYP98R61BXDA33QDN65XMRDG/.agents/skills/e2e-visual-review/SKILL.md` for the full capture-and-placement protocol.
`/tmp/tmp.BKie2utvPB/data/demo-scout/e2e-review/` is a permitted exception to "stay inside this worktree", alongside the report and the status file.

# Definition of done

Evidence: tests/fm-brief.test.sh full run (17/17 pass)

ok - fm-brief.sh: bash -n succeeds
ok - fm-brief.sh: --help renders the complete header
ok - fm-brief.sh: no-mistakes/direct-PR/local-only briefs generate cleanly
ok - fm-brief.sh: faster paths use configured authority without stacked review
ok - fm-brief.sh: no-mistakes DOD wording avoids the apostrophe regression
ok - fm-brief.sh: ship project-memory wording carries the AGENTS.md authoring bar
ok - fm-brief.sh: ship briefs point at the e2e-visual-review protocol
ok - fm-brief.sh: scout briefs point at the e2e-visual-review protocol
ok - fm-brief.sh: --herdr-lab emits the complete hard safety contract
ok - fm-brief.sh: --herdr-lab uses its quoted Firstmate-owned helper path
ok - fm-brief.sh: ship and scout scaffolds make omitted Herdr intent fail-visible
ok - fm-brief.sh: Herdr lab contract covers scouts and rejects secondmate misuse
ok - fm-brief.sh: --no-projects scaffolds a project-less charter and guards misuse
ok - fm-brief.sh: marked requests avoid generic acknowledgements and preserve material reporting
ok - fm-brief.sh: custom pause verb renders in every scaffold
ok - fm-brief.sh: investigation and visual-review completions load the shared decision policy
ok - fm-brief: scout and secondmate code paths still scaffold well-formed briefs

Evidence: fm-doc-audience-check.sh exact-coverage gate

fm-doc-audience-check: ok surfaces=59 local_links=147

Pipeline

Updates from git push no-mistakes

✅ **intent** - passed

✅ No issues found.

✅ **Rebase** - passed

✅ No issues found.

✅ **Review** - passed

✅ No issues found.

✅ **Test** - passed

✅ No issues found.

• bash tests/fm-brief.test.sh — all 17 tests pass, including new test_ship_e2e_visual_review_pointer and test_scout_e2e_visual_review_pointer regression tests
• bash tests/fm-documentation-audiences.test.sh — all 5 tests pass
• bash bin/fm-doc-audience-check.sh — exact-coverage gate passes (ok surfaces=59 local_links=147), confirming the new app-test-review and e2e-visual-review skills are correctly registered in docs/documentation-audiences.json
• Manual verification: ran bin/fm-brief.sh in both ship and scout mode against a scratch FM_HOME, and inspected the actual generated brief.md text
• Cross-checked AGENTS.md for the claimed dual placement (section 2 layout entries at lines 85/116, section 7 inline pointers at lines 283/318, section-13 trigger entry at line 487) — all present as described in the intent
• Confirmed git diff 05fba80..5c8a856 --stat touches only .agents/skills/app-test-review/SKILL.md, .agents/skills/e2e-visual-review/SKILL.md, AGENTS.md, bin/fm-brief.sh, docs/documentation-audiences.json, tests/fm-brief.test.sh — no files under projects/ touched

✅ **Document** - passed

✅ No issues found.

✅ **Lint** - passed

✅ No issues found.

✅ **Push** - passed

✅ No issues found.