diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index 5d301ca1..e4b18456 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -67,3 +67,5 @@ jobs: - run: python3 -m unittest discover -s triage/scripts -p 'test_*.py' -v - run: python3 -m unittest discover -s skills/report-bug/scripts -p 'test_*.py' -v - run: python3 -m unittest discover -s design/scripts -p 'test_*.py' -v + - run: python3 -m unittest discover -s ui-design/scripts -p 'test_*.py' -v + - run: python3 -m unittest discover -s ux-design/scripts -p 'test_*.py' -v diff --git a/AGENTS.md b/AGENTS.md index 0f21d0fb..52937d53 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -97,6 +97,8 @@ _shared/ phase-override-resolution.md # Project-level phase override lookup and activation record-manual-edit.md # Tier 3 manual-edit attribution (wraps capture recipe) render-provenance-footer.md # Render durable footer into docs-repo markdown before commit + screenshot-inspection.md # Screenshot identity verification and visual inspection (used by ui-design) + screenshot-publish-stage.md # Owned-path allowlist, staging, and commit for screenshot publishes (used by ux-design) self-review-gate.md # Pre-PR self-review quality gate (used by bugfix, implement, e2e, cve-fix) validation-gate.md # Pre-commit build/test/lint discovery gate (used by bugfix) ``` diff --git a/_shared/recipes/screenshot-inspection.md b/_shared/recipes/screenshot-inspection.md new file mode 100644 index 00000000..f697ae30 --- /dev/null +++ b/_shared/recipes/screenshot-inspection.md @@ -0,0 +1,374 @@ +--- +name: screenshot-inspection +version: 0.1.0 +--- +# Recipe: Screenshot Inspection + +Inspect published screenshots referenced in a UX handoff document. +Used by `ui-design` `/ingest` to verify image identity and visually +observe the actual design assets. + +## Parameters + +| Parameter | Required | Description | +|-----------|----------|-------------| +| HANDOFF_DIR | Yes | Absolute path to the directory containing the published handoff | +| MANIFEST_PATH | Yes | Absolute path to the published `screenshot-manifest.json` | +| HANDOFF_FILE | Yes | Path to the handoff file — required for Screenshot Index ID accounting | + +## Procedure + +### 1. Read manifest + +Read the manifest at `{MANIFEST_PATH}`. It contains per-image `sha256`, +`context_revision`, `prototype_iteration`, `caption`, and `alt_text`. +If the manifest is missing, warn the user and record `manifest: unavailable` +per image. + +**Manifest ID uniqueness.** When the manifest is present and non-empty, +verify every entry's `id` field is unique among manifest entries. If +any ID appears more than once, stop before §4 identity verification — +duplicate manifest IDs make digest selection ambiguous and must not be +resolved by first-match or arbitrary choice. Direct the researcher to +repair the authoring manifest (deduplicate or re-curate). Distinct +entries with different IDs sharing the same `sha256` remain valid. +Absent or empty manifests bypass this check (existing per-image +`manifest: unavailable` rules apply). + +### 2. Account for every declared index ID + +Read `{HANDOFF_FILE}`'s **Screenshot Index** section and collect all +declared `SS-N` IDs with their declared ref paths and SHA-256 +prefixes (when the index records them). Also collect the Screenshot +Index's own `context_revision` and `prototype_iteration` from its +footer (the `**Context revision:** {R} | **Prototype iteration:** {N}` +line produced by `/handoff`). Default missing, empty, dash (`—`), or +unparseable footer fields to `unknown`. + +**Duplicate ID check.** Before matching, verify every declared SS-N +ID is unique within the index. If any ID appears more than once, +stop and require the researcher to repair the handoff — duplicate +IDs make matching ambiguous and cannot be partially overridden. +Distinct unique IDs that share the same ref path remain valid. + +Match each declared +ID to an actual image embed (`![...](...)` in the handoff body) by +**both** the ID and its declared ref path. Consume each embed at most +once — distinct IDs with the same ref may each match their own embed of +that ref, but one embed cannot satisfy two IDs. + +- **IDs with a matching embed:** pass to steps 3–4 for resolution and + inspection. One output row per declared ID. +- **IDs without an embed or whose embed was already consumed:** record + immediately as a 9-column row with `status: unavailable`, + `identity: —`, `observation: —`, `spec_match: —`, and + `notes: "declared in index but no matching embed found"`. Include + in the output and aggregate totals. + +Unmatched embeds (embeds in the body that do not correspond to any +declared ID) are not eligible for inspection or citation. + +Run this matching **before** resolution or inspection. The total is +the count of declared IDs, not the count of discovered embeds. + +Report the full accounting: +- **Total declared:** count of IDs in the Screenshot Index +- **Embeds found:** count with matching image embeds +- **Missing embeds:** count without embeds (list the IDs) + +If any IDs have missing embeds, warn the user and prompt: repair the +handoff to add the missing embeds, or explicitly authorize partial +continuation (text-only for those IDs). Never silently omit a declared +ID or claim all images were inspected when embeds are missing. + +If the user repairs the handoff (adds missing embeds or modifies +the Screenshot Index), the caller must re-enter its +published-input gate on the changed handoff/manifest before +re-entering inspection — no private authoring fallback. Repair or +wait is not partial authorization. Reload §1 manifest (including +the duplicate-ID check) on the updated inputs before rerunning +§2 matching — a changed manifest (top-level `context_revision`, +`prototype_iteration`, or per-entry digests) affects all rows, +not just repaired IDs. Rerun §2 matching on the updated inputs, +then §3–§4 for fresh index, header, and affected rows. For +affected IDs, replace previous `unavailable` rows with fresh +matched results; preserve unaffected rows only if the embed is +unchanged, still valid, and no manifest field used by that row's +§3–§4 processing changed. Recalculate all aggregate totals +before proceeding. Do not append duplicate rows — each declared +ID appears exactly once in the output. + +**Unmatched body embeds.** After matching declared IDs to embeds, +scan the handoff body for image embeds (`![...](...)`) whose `SS-N` +ID does not appear in the Screenshot Index. Report these separately +as excluded — they are not eligible for inspection, citation, or +ID assignment. Never inspect, cite, or assign fabricated +observations to excluded embeds. If any exist, warn the user and +prompt: repair the handoff (add the IDs to the Screenshot Index +and re-enter the published-input gate) or explicitly authorize +limited continuation. Record the list of excluded embed IDs and +the user's decision. + +### 3. Resolve image references + +For each matched pair from §2, determine the reference type: + +**Local references** (`screenshots/SS-N-desc.png` -- `lfs` or `git` +mode): **Path containment.** Before opening any local file, reject +paths containing `..` components or absolute paths. Resolve each +candidate against `{HANDOFF_DIR}` and verify the resolved path remains +inside `{HANDOFF_DIR}`. If the path is a symlink, verify its target is +also inside `{HANDOFF_DIR}`. Stop and report any reference that +escapes. Then locate the image file relative to `{HANDOFF_DIR}`. If the file +does not exist at the resolved path (after passing containment +checks), record the pair as unresolved with `status: unavailable` +and `notes: "file not found at resolved path"`. This is a missing- +file resolution failure, distinct from the traversal/symlink escape +hard stop. If the file is a Git LFS pointer stub (starts with +`version https://git-lfs`), attempt `git lfs smudge` or record +`inspection: lfs-unresolved`. + +**External references** (`https://...` URLs -- `external` mode): +Retrieve image bytes using the runtime's binary download capability +(save to a temporary file). If download fails, record +`inspection: fetch-failed`. + +### 4. Verify identity and inspect + +**Unresolved matched pairs.** Some matched pairs from §2 may fail +resolution in §3 (fetch-failed, lfs-unresolved, missing file, +unsupported image capability). For each unresolved pair, record a +full 9-column row immediately: + + | Field | Value | + |-------|-------| + | `id` | Screenshot ID (`SS-N`) | + | `status` | Failure status from §3 (`unavailable` / `lfs-unresolved` / `fetch-failed` / `inspection-not-supported`) | + | `identity` | `—` | + | `ref` | Published reference (local path or URL) | + | `expected_sha` | SHA-256 from manifest (or `—`) | + | `observed_sha` | `—` | + | `observation` | `—` (not observed — do **not** fabricate from caption or alt text) | + | `spec_match` | `—` | + | `notes` | Failure reason (e.g., "Git LFS pointer not resolved", "HTTP 404") | + +Report which IDs could not be resolved. Prompt the user: repair the +source so images can be resolved, wait for access, or explicitly +authorize partial continuation (text-only for unresolved IDs — +inspected observations from successfully resolved images are +retained). An unresolved status **never** becomes eligible for +citation (§5 Rule 1) even when the user authorizes partial +continuation; partial acceptance allows the workflow to proceed but +does not override status-based eligibility. + +If the user repairs the source or access is restored, the caller +must re-enter its published-input gate on the changed inputs +before re-entering inspection. Reload §1 manifest (including +the duplicate-ID check), then rerun §2 header, index, and +matching on the updated inputs — changed handoff or refs must +not reuse old matched pairs. Then rerun §3 resolution and §4 +identity/inspection (SHA, visual, bindings). If any manifest +field used by identity verification (per-entry `sha256`) or +binding checks (top-level `context_revision`, +`prototype_iteration`) changed, recheck all rows that reference +the changed fields — not just the repaired IDs. Replace +previous unresolved rows, obsolete SHA comparisons, superseded +binding/acceptance decisions, and aggregate/eligibility with +fresh results; preserve only rows whose manifest inputs are +unchanged and still valid. One row per declared ID. Do not +append duplicate rows — each declared ID appears exactly once +in the output. + +**For each resolved matched image from §3:** + +1. **Verify identity.** Always compute the SHA-256 of the actual + resolved image bytes and record it as `observed_sha`, regardless + of whether the manifest is present. If the manifest provides + `sha256`, compare the observed hash against it: match → + `identity: verified`; mismatch → `identity: mismatched`. If the + manifest is absent or has no digest for this image, identity is + `unverified` (not "impossible comparison" and never "verified"). + An absent manifest must not clear a stale observed prefix mismatch. + + **Index checksum prefix.** When the Screenshot Index declares a + SHA-256 prefix for this ID, the prefix must be exactly 12 + lowercase hex characters (matching the template `{12 hex}…`). + A trailing display ellipsis (`…`) is not part of the validated + checksum. Reject one-character, malformed, or missing prefixes — + they cannot verify identity. Handle case consistently (normalize + to lowercase before comparison). Always compare a valid 12-hex + prefix against the computed actual-bytes hash. Additionally + compare against the full manifest digest only when the manifest + is present. Matching all available digests is consistent — + identity may be `verified` only when all other gates also pass. + An observed-bytes prefix mismatch is `identity: mismatched` + regardless of manifest presence or absence. A manifest-only + prefix mismatch is also `identity: mismatched`. Absent manifest + keeps `identity: unverified` even when the observed prefix + matches — prefix alone cannot elevate to `verified`. A stale or + mismatched prefix (does not match either available digest) is an + identity exception: apply the same independent accepted-unverified + rules as byte identity. A missing or invalid prefix cannot be + silently treated as verified; preserve existing unverified + decision paths. Retain the separate full 64-character digest + comparison. + + **Identity exception decision.** After all manifest and prefix + checks above, when the final identity is `mismatched` or + `unverified`, prompt the researcher for an explicit per-image + identity exception decision, independent from the named + `context_revision`/`prototype_iteration` binding decisions in + step 5. One identity decision covers all its listed identity + reasons (byte mismatch, absent manifest, invalid prefix). Reject + or defer keeps the image ineligible; explicit acceptance records + `accepted-unverified` per existing citation rules. Do not invent + a new acceptance schema — 9 output columns unchanged. + +2. **Open and visually inspect** using the runtime's image reading + capability (the `Read` tool on the file path). Reading markdown, + metadata, captions, or alt text alone is **not** image inspection -- + observe the actual rendered layout, visual hierarchy, spacing, color, + component arrangement, and state presentation. + + If identity is `mismatched`, still record observations but flag the + image as stale/unverified. + +3. **Record** per image using this schema: + + | Field | Value | + |-------|-------| + | `id` | Screenshot ID (`SS-N`) | + | `status` | `inspected` / `unavailable` / `lfs-unresolved` / `fetch-failed` / `inspection-not-supported` | + | `identity` | `verified` / `mismatched` / `unverified` | + | `ref` | Published reference (local path or external URL) | + | `expected_sha` | SHA-256 from manifest (or `—` if unavailable) | + | `observed_sha` | SHA-256 of actual bytes (or `—` if not resolved) | + | `observation` | Brief description of actual visual content | + | `spec_match` | `Yes` / `No` — consistency with written description | + | `notes` | Visual details not in spec, or failure reason | + + When the user explicitly accepts a `mismatched` or `unverified` image, + record that decision so downstream phases can cite it as + `[Screenshot: SS-N — accepted-unverified]`. Do not silently accept + missing manifests or identity mismatches. + +4. **Inspection failure during resolved processing.** If a resolved + image cannot be visually inspected at this stage (e.g., unsupported + format after successful download), record it with status + `inspection-not-supported`. Unlike early unresolved cases + (§3 failures where no bytes were obtained), this image was + successfully resolved — §4.1 has already computed + `observed_sha` and `identity` and the `expected_sha` is + available when the manifest provides a digest for this + image (`—` when the manifest is absent or has no entry). + Preserve those actually computed + values; record `observation: —` (not observed) and + `spec_match: —` (unknown). The image remains ineligible for + citation (§5 Rule 1 — status must be `inspected`) even when + identity is `verified` and partial authorization was granted. + 9 output columns unchanged. Apply the same continuation + handling: prompt the user to repair, wait, or explicitly + authorize partial continuation for this ID. Do not fabricate + observations from captions or alt text. + +5. **Check revision binding (separate from identity).** Byte-identity + verification (step 4.1) and revision-binding validity are + independent checks — a verified SHA must not erase a binding + mismatch. + + Read the manifest's top-level `context_revision` and + `prototype_iteration` (the actual published fields). Read the + handoff's metadata header for its declared revision/iteration. + Use the Screenshot Index's own `context_revision` and + `prototype_iteration` collected in §2. For each image, compare + all three sources — header, index, and manifest: + + | Binding | Header | Index (§2) | Manifest | Match | + |---------|--------|------------|----------|-------| + | `context_revision` | {from header} | {from index footer} | {from manifest} | `matched` / `mismatched` / `unknown` | + | `prototype_iteration` | {from header} | {from index footer} | {from manifest} | `matched` / `mismatched` / `unknown` | + + A binding is `matched` only when all three sources that are + present agree. A matching header and manifest must not bypass a + conflicting index value — any disagreement among available + sources is `mismatched`. When any source is missing, empty, + dash (`—`), or otherwise invalid, default that source to + `unknown`. When available (non-unknown) sources disagree, the + binding is `mismatched` regardless of whether other sources + are absent. Only when all available sources agree but at least + one is unknown is the binding `unknown`. Never treat an absent + or unrecognizable binding as vacuously `matched`. + + Record binding status in the Notes column of the per-image schema + (keeping 9 columns) with the actual header/index/manifest values + and match status. + + When any binding is `mismatched` or `unknown`, prompt the user + **per image** with a separate named decision for each affected + binding. Persist each decision in Notes alongside its + expected/actual/status (e.g., + `context_revision: mismatched, expected=r3 actual=r2, decision=accepted; + prototype_iteration: unknown, expected=i2 actual=—, decision=accepted`). + When the manifest is absent, each binding's status follows the + rules above independently — disagreeing header and index values + are `mismatched`, not forced `unknown`; each still needs its own + explicit acceptance. One utterance explicitly + accepting both may record both decisions; an ambiguous aggregate + legacy decision must not imply all bindings accepted. Missing, + rejected, deferred, or unprompted decisions remain ineligible + (fail-closed). Accepted-unverified labels carry through to §5. + A verified SHA cannot clear a binding mismatch. See §5 + Eligibility for citation rules. + +### Aggregate status + +Compute the aggregate screenshot status after all §2–§4 processing: + +- **`inspected`** — at least one declared ID exists, every declared + ID has `status: inspected`, AND no unmatched body embeds were + excluded. +- **`partial`** — at least one declared ID has `status: inspected`, + but any declared ID has non-inspected status OR unmatched body + embeds were excluded with user authorization. Partial applies + even when every declared-ID row was inspected. +- **`unavailable`** — no declared ID has `status: inspected`, + regardless of declared row count or excluded embeds. + +Report the aggregate, the excluded unmatched embed ID list (when +any exist), and the user's continuation decision. Callers must +preserve this reported aggregate and excluded list rather than +recomputing solely from the per-image table rows. + +### 5. Eligibility and citation rules + +Callers (e.g. `/plan`) read this section for citation policy; they +must not rerun image inspection. Evaluate each image in rule order: + +**Rule 1 — Inspection gate.** `status` must be `inspected`. Any +other status (`unavailable`, `lfs-unresolved`, `fetch-failed`, +`inspection-not-supported`) → **ineligible**. Do not cite. + +**Rule 2 — Identity.** `identity` must be `verified` (no action +needed), or `mismatched`/`unverified` with explicit per-image user +acceptance. Any other value, or a rejected/unprompted/deferred +decision → **ineligible**. + +**Rule 3 — Bindings.** Stored binding status values that are missing, +empty, a dash (`—`), or otherwise invalid default to `unknown`. Each +binding (`context_revision`, `prototype_iteration`) that is `mismatched` +or `unknown` requires its own explicit acceptance, independent of +identity and independent of the other binding. Any rejected, +unprompted, or deferred binding → **ineligible**. + +**Outcome:** + +| Result | When | Citation | +|--------|------|----------| +| verified | inspected + identity `verified` + all bindings `matched` | `[Screenshot: SS-N]` | +| accepted-unverified | inspected + every applicable identity/binding exception explicitly accepted | `[Screenshot: SS-N — accepted-unverified]` | +| ineligible | any rule blocks use | Do not cite | + +Rejected, deferred, or unavailable evidence must not be used for +planning decisions — not merely given a different citation tag. +Accepted-unverified evidence stays usable but always carries the +qualified label. diff --git a/_shared/recipes/screenshot-publish-stage.md b/_shared/recipes/screenshot-publish-stage.md new file mode 100644 index 00000000..428cb89e --- /dev/null +++ b/_shared/recipes/screenshot-publish-stage.md @@ -0,0 +1,406 @@ +--- +name: screenshot-publish-stage +version: 0.1.0 +--- +# Recipe: Screenshot Publish Stage and Commit + +Build the owned-path allowlist, stage, and commit screenshot-related +changes in the docs repo. Used by `ux-design` `/publish` and `/respond` +to ensure both phases handle the staging/commit identically. + +## Parameters + +| Parameter | Required | Description | +|-----------|----------|-------------| +| DOCS_REPO_PATH | Yes | Absolute path to the docs repo working tree | +| HANDOFF_FILE_PATH | Yes | Repo-relative POSIX path to `handoff.md` | +| HANDOFF_DIR | Yes | Repo-relative POSIX path to the handoff directory (dirname of HANDOFF_FILE_PATH) | +| COPIED_FILENAMES | No | Bash array of screenshot filenames copied/updated this run (lfs/git mode) | +| REMOVED_FILENAMES | No | Bash array of screenshot filenames removed via `git rm` this run | +| MANIFEST_WRITTEN | No | Set to `true` if `screenshot-manifest.json` was written or updated this run | +| MANIFEST_REMOVED | No | Set to `true` if the consumer manifest was removed (all screenshots deleted) | +| COMMIT_MESSAGE | Yes | The commit message to use | + +## Procedure + +### 1. Build the owned-path allowlist + +Construct a Bash array of every path this run created, modified, or owns +(including tracked deletions). This array serves as the staging set, change +detector, and commit pathspec. + +```bash +owned_paths=("{HANDOFF_FILE_PATH}") + +# Consumer manifest — include when written OR removed (tracked deletion) +if [ "${MANIFEST_WRITTEN}" = "true" ] || [ "${MANIFEST_REMOVED}" = "true" ]; then + owned_paths+=("{HANDOFF_DIR}/screenshot-manifest.json") +fi + +# Each copied/updated screenshot (lfs/git mode): +for f in ${COPIED_FILENAMES[@]+"${COPIED_FILENAMES[@]}"}; do + owned_paths+=("{HANDOFF_DIR}/screenshots/$f") +done + +# Each removed screenshot (already staged by git rm): +for f in ${REMOVED_FILENAMES[@]+"${REMOVED_FILENAMES[@]}"}; do + owned_paths+=("{HANDOFF_DIR}/screenshots/$f") +done +``` + +### 2. Validate and stage owned paths + +Verify required paths exist on disk before staging: the handoff file, +each copied screenshot, and the manifest when `MANIFEST_WRITTEN` is +set. Missing required paths indicate an upstream error. + +Stage every path in the allowlist. For paths that still exist on disk, +use `git add`. For tracked deletions (removed screenshots, removed +manifest), `git rm` will have already staged them — include them in the +array so the `--only` commit sees them. + +```bash +# Required-path validation: handoff and each copied file must exist on disk. +# Written manifest must exist when MANIFEST_WRITTEN=true. +if [ ! -f "{DOCS_REPO_PATH}/{HANDOFF_FILE_PATH}" ]; then + echo "Error: required handoff missing: {HANDOFF_FILE_PATH}" >&2; exit 1 +fi +for f in ${COPIED_FILENAMES[@]+"${COPIED_FILENAMES[@]}"}; do + if [ ! -f "{DOCS_REPO_PATH}/{HANDOFF_DIR}/screenshots/$f" ]; then + echo "Error: required copied file missing: {HANDOFF_DIR}/screenshots/$f" >&2; exit 1 + fi +done +if [ "${MANIFEST_WRITTEN}" = "true" ]; then + if [ ! -f "{DOCS_REPO_PATH}/{HANDOFF_DIR}/screenshot-manifest.json" ]; then + echo "Error: MANIFEST_WRITTEN=true but manifest missing" >&2; exit 1 + fi +fi + +for p in "${owned_paths[@]}"; do + if [ -f "{DOCS_REPO_PATH}/$p" ]; then + if ! git -C "{DOCS_REPO_PATH}" add -- "$p"; then + echo "Error: git add failed for $p" >&2; exit 1 + fi + elif git -C "{DOCS_REPO_PATH}" ls-files --error-unmatch "$p" >/dev/null 2>&1; then + # Tracked file that no longer exists on disk — already staged by git rm + : + fi + # Removal paths neither on disk nor tracked are tolerated (already-committed deletions) +done +``` + +### 3. Check for changes and commit + +Use `--only` with the full allowlist so only owned paths are committed. +Unrelated staged/index and unstaged/worktree changes survive untouched. +Include paths whose deletion is staged (present in HEAD but absent from +the index after `git rm`). Filter out already-committed deletions +(absent from disk, index, and HEAD) so `commit --only` does not fail +on unknown pathspecs. + +```bash +commit_paths=() +for p in "${owned_paths[@]}"; do + if [ -f "{DOCS_REPO_PATH}/$p" ] || git -C "{DOCS_REPO_PATH}" ls-files --error-unmatch "$p" >/dev/null 2>&1; then + commit_paths+=("$p") + elif git -C "{DOCS_REPO_PATH}" rev-parse --verify "HEAD:$p" >/dev/null 2>&1; then + # Path in HEAD but not in index — staged deletion, first removal + commit_paths+=("$p") + fi + # Path absent from disk, index, and HEAD — already-committed deletion; skip +done + +if [ ${#commit_paths[@]} -eq 0 ]; then + echo "No owned paths present; skipping commit." +else + git -C "{DOCS_REPO_PATH}" diff --cached --quiet -- "${commit_paths[@]}" + rc=$? + if [ "$rc" -eq 0 ]; then + echo "No changes to owned paths; skipping commit." + elif [ "$rc" -eq 1 ]; then + if ! git -C "{DOCS_REPO_PATH}" commit --only \ + -m "{COMMIT_MESSAGE}" \ + -- "${commit_paths[@]}"; then + echo "Error: git commit failed; stopping." >&2; exit 1 + fi + else + echo "Git error (exit $rc); stopping." >&2; exit "$rc" + fi +fi +``` + +This procedure handles text-only (array contains only the handoff), +lfs/git (handoff + manifest + each image), external (handoff + manifest), +removal runs (handoff + manifest + removed paths), and final-screenshot +removal (handoff + removed manifest + removed images) identically. + +### 4. Pending attempt record (push safety) + +Callers (`/publish` Step 8, `/respond` push section) follow this +protocol to ensure push failures are recoverable without re-deriving +ownership or adopting unknown ahead commits. All Git commands in §4 +operate on the docs repo; `DOCS_REPO_PATH` must be available as the +recipe's declared input (carried from §1–§3, or explicitly supplied +on standalone snapshot/retry/post-verify paths). + +**Before push.** Write a `pending_attempt` object into +`publish-metadata.json`, preserving all existing fields: + +| Field | Value | +|-------|-------| +| `commit_sha` | Exact HEAD SHA after the commit above | +| `branch` | Feature branch name | +| `expected_base` | Remote branch SHA before push, or `null` for first push | +| `owned_paths` | Array of paths included in the commit | +| `pr_number` | PR number (existing PR) or `null` (new PR) | +| `base_branch` | Target branch name for the PR (e.g., `main`) | +| `docs_repo_remote` | Credential-free repo identity (from config) | +| `handoff_file_path` | Repo-relative handoff path | +| `candidate_screenshot_storage` | Storage mode for this attempt | +| `candidate_screenshot_assets` | Array of `{id, filename, sha256, published_path}` entries | +| `upstream_prd_path` | Repo-relative path to PRD (frozen at preparation; preserved on updates) | +| `upstream_design_path` | Repo-relative path to design doc (frozen at preparation; preserved on updates) | +| `push_destination` | Validated push target (`{host}/{owner}/{repo}` canonical form; non-default port appended as `{host}:{port}/{owner}/{repo}`) | +| `timestamp` | ISO 8601 when the attempt was recorded | + +The `pending_attempt` record is private authoring state — do not copy +it into the consumer-facing published manifest. + +**Upstream path freeze.** `upstream_prd_path` and +`upstream_design_path` are frozen at preparation: validated as +repo-relative POSIX paths to existing regular files with basenames +`prd.md` and `design.md`, sharing the same canonical parent +directory as `handoff_file_path`, inside `docs_repo_root`. On +recovery, load frozen values into Step 9 variables before use; +verify against the committed tree at `commit_sha` (not the mutable +worktree) that each path is a regular file with the expected +basename (`prd.md` / `design.md`), both share the same canonical +parent as `handoff_file_path`, and all remain inside +`docs_repo_root`. +First publication remains first publication even when `pr_number` +has since become known. Update attempts preserve established +upstream paths rather than overwrite with `null`. Legacy records +missing required path fields stop and retain `pending_attempt` +unless reconstruction has proof from frozen approved committed +state; do not fabricate paths or use placeholders. + +**Push destination validation.** Before recording `expected_base` or +pushing, resolve effective push destination(s): +`git -C "{DOCS_REPO_PATH}" remote get-url --push --all origin`. This expands `pushInsteadOf` +rules to reveal actual targets. Validate: + +1. Exactly one push URL exists (single approved destination). + Require the effective push URL to use HTTPS or SSH (including + scp-style `user@host:path`); reject `http://`, `git://`, and + unrecognized schemes before any `ls-remote` query or push — + credentials must not transit cleartext. Apply the same scheme + check on initial and retry paths. +2. Normalize the resolved push URL to credential-free canonical + identity (`{host}/{owner}/{repo}`, with non-default port: + `{host}:{port}/{owner}/{repo}`): strip userinfo, query + parameters, and fragment before extracting host/owner/repo. + Normalize `docs_repo_remote` from config the same way before + comparing — legacy configs may store raw or differently- + formatted values. SSH (port 22) and HTTPS (port 443) are + default and equivalent; an explicit non-default port difference + or unverifiable port mapping stops and retains `pending_attempt`. + Host mismatch stops without adopting. Credential-bearing URLs + must not be persisted or logged. +3. If multiple push URLs exist, the identity does not match, or + resolution fails, stop — do not push or adopt the unverified + destination. + +Record the canonical identity as `push_destination`. Use this +validated destination for the initial `expected_base` snapshot +(`git ls-remote` against the push URL, not the fetch tracking ref), +retry classification, and post-push verification. + +**Pre-push gates (initial and retry).** Before every actual +push, apply these checks in order: + +1. **PR revalidation.** When `pr_number` is known (not `null`), + verify the live PR is open, head branch matches `branch`, + base matches `base_branch`, and head repo matches the derived + PR identity. Derive the PR identity from `docs_repo_remote` + by using the already-verified GitHub instance hostname + (the same verified mapping used for `GH_HOST` — see + `/respond` Step 1) plus the same `owner/repo` from + `docs_repo_remote`: `{instance_hostname}/{owner}/{repo}`. + The derived PR identity uses the instance hostname, not the + raw Git transport host:port — SSH port 2222 is not the + GitHub instance identity. If the instance hostname mapping + is unknown, ambiguous, or unverified, stop and ask the + researcher before comparison — never assume that stripping + a non-default port yields the correct instance hostname. + Use the same derived PR identity consistently for pre-push + revalidation, metadata finalization (§4.3), and new-PR + candidate comparison. Keep `docs_repo_remote` and + `push_destination` as the canonical Git transport identity + with exact approved port for all push/query/destination + checks — the PR identity mapping does not bypass transport + validation. Closed, merged, retargeted, mismatch, + missing, or unavailable → stop; retain `pending_attempt`. Not + a cross-service atomic guarantee. Both `/publish` Step 8 and + `/respond` push must consume this gate. +2. **Owned-path verification.** For each of `handoff_file_path`, + `upstream_prd_path`, and `upstream_design_path` (when present), + verify at `commit_sha` via + `git -C "{DOCS_REPO_PATH}" ls-tree "$commit_sha" -- "$path"`: the entry must exist + with mode `100644` or `100755` (regular file — never `120000` + symlink or `040000` tree). `cat-file -e` alone is insufficient + because it accepts trees and symlinks. Verify expected basenames + (`handoff.md`, `prd.md`, `design.md`), shared canonical parent + directory as `handoff_file_path`, and containment inside + `docs_repo_root`. Missing, wrong mode, or unverifiable → stop; + preserve pending and local commit. Do not automatically stage + unrelated documents. +3. **Exact head ref query.** Query remote branch via + `git ls-remote --heads "$push_url" "refs/heads/$branch"` and + verify the returned ref name field is exactly + `refs/heads/$branch`. A same-named tag or nested branch suffix + does not satisfy the query. Empty result means absent. + Malformed or duplicate output stops. + +```bash +# Exact head ref query — canonical form for all callers. +# Checked capture: a failed query is UNKNOWN and must exit nonzero, +# never treated as empty/absent. Redact credential-bearing errors. + +# Second-rewrite guard: verify the frozen push URL is not rewritten +# again by a local insteadOf/pushInsteadOf rule before the network +# query. --get-url expands insteadOf rules WITHOUT remote contact +# (see git-scm.com/docs/git-ls-remote). If the effective query URL +# differs from the validated push URL, a second rewrite would send +# the query (and subsequent push) to a different repository. +_effective_url="" +if ! _effective_url=$(git -C "$DOCS_REPO_PATH" ls-remote --get-url "$push_url" 2>/dev/null); then + echo "Error: cannot resolve effective query URL for push destination" >&2 + exit 1 +fi +if [ "$_effective_url" != "$push_url" ]; then + echo "Error: push URL would be rewritten by insteadOf to a different destination; stopping" >&2 + exit 1 +fi + +_lr_out="" +if ! _lr_out=$(git -C "$DOCS_REPO_PATH" ls-remote --heads "$push_url" "refs/heads/{branch}" 2>/dev/null); then + echo "Error: ls-remote query failed; cannot determine remote branch state" >&2 + exit 1 +fi +exact_sha="" +while IFS=$'\t' read -r sha ref; do + [ -z "$sha" ] && continue + if [ "$ref" = "refs/heads/{branch}" ]; then + [ -n "$exact_sha" ] && { echo "Error: duplicate ref in ls-remote output" >&2; exit 1; } + if ! printf '%s' "$sha" | grep -qE '^[0-9a-f]{40}$'; then + echo "Error: malformed SHA in ls-remote output" >&2; exit 1 + fi + exact_sha="$sha" + fi +done <<< "$_lr_out" +# exact_sha empty => branch absent on remote. +``` + +**On retry (when `pending_attempt` exists).** Before any file copy, +`git rm`, or new commit: + +1. Verify local HEAD equals `pending_attempt.commit_sha`. If HEAD + differs, stop — unknown commits exist; do not force-push, reset, + or re-derive ownership. +2. Verify the current branch matches `pending_attempt.branch`. + Verify the docs repo remote identity matches `pending_attempt.docs_repo_remote`. + Re-resolve the effective push destination + (`git remote get-url --push --all origin`) and verify its + canonical identity matches `pending_attempt.push_destination`. + Changed or unverifiable push destination → stop. +3. Apply the pre-push gates above (PR revalidation, owned-path + verification, exact head ref query). +4. Classify using `exact_sha` from the exact head ref query in + Step 3 above (do not re-query with a short pattern): + - **Remote SHA equals `pending_attempt.commit_sha`:** Already + pushed. Skip push; proceed to metadata finalization. + - **Remote SHA equals `pending_attempt.expected_base` (or branch + does not exist and `expected_base` is `null` for a genuine new + branch never pushed):** normal push. Re-attempt push. + - **Any other state** (including deleted/reset branch where + `expected_base` was not `null`, or an unknown ahead SHA): stop. + Do not force-push, recreate, or guess history. + +**Initial new attempt** (no `pending_attempt` exists): resolve the +effective push destination first (same validation as above). Apply +the pre-push gates (PR revalidation when `pr_number` is known, +owned-path verification, exact ref query) before pushing. +Snapshot `expected_base` via the canonical exact head ref query +against the validated push URL before the owned commit. Require the local branch to be +at a known synced base (SHA matches the push-URL snapshot, or a +fresh branch from `{base-branch}`). If the local branch is ahead +by commits not created by this invocation, stop. + +**After successful push.** Re-run the exact head ref query (fresh +checked capture against the validated push URL — not the stale +pre-push value) and verify `exact_sha` equals +`pending_attempt.commit_sha`. Then: + +1. Finalize metadata from `pending_attempt` candidate values: + set `screenshot_storage` to `candidate_screenshot_storage`, + set `screenshot_assets` to `candidate_screenshot_assets`. +2. Remove `pending_attempt` from `publish-metadata.json`. +3. **PR identity.** For an existing PR (`pr_number` is known), + revalidate: fetch the live PR state and verify it is open with + head branch, base branch, and head repository matching the + frozen `pending_attempt` record. Compare the PR's head + repository against the derived PR identity (same + `{instance_hostname}/{owner}/{repo}` from the pre-push gate + §4 Step 1), not the raw `docs_repo_remote` transport identity. + Closed, merged, changed head, changed base, missing PR, or + unavailable tool → stop without writing final metadata; retain + `pending_attempt`. + For a new PR (`pr_number` is `null`): derive validated + `[HOST/]OWNER/REPO` using the verified GitHub instance + hostname (not API hostname or Git transport port) and + `docs_repo_remote` owner/repo, then + look up PRs in + **all states** by + canonical head repository and head `branch` only — **without** + filtering by `base_branch`. For each candidate, verify the PR's + head repository matches the derived PR identity + (`{instance_hostname}/{owner}/{repo}` from the pre-push gate + §4 Step 1) — a different fork with the same branch name is not + a match. Then check `base_branch` on each matched + candidate: + - **Retargeted base** (head repo+branch matches but base + differs from `pending_attempt.base_branch`): stop and retain + `pending_attempt` for researcher reconciliation — do not + create another PR or auto-retarget. + - **Terminal** (closed or merged): stop — do not create a + replacement; report to the researcher. + - **Open with matching base**: reuse; preserve its draft/ready + status. + - **No candidate found** and the original intent was new + publication: create as a draft. Respect `/respond` limited + permissions — direct to `/publish` when creation is needed. + A known PR identity is required before writing metadata. Record + `pr_number`, `branch`, and `handoff_file_path`. + +For first-publish recovery where path-resolution steps are +skipped, verify `upstream_prd_path` and `upstream_design_path` +from the frozen `pending_attempt` exist at `commit_sha` (not the +mutable worktree) before metadata finalization. Missing, unsafe, or +unverifiable paths → stop, retain `pending_attempt`; do not +fabricate or derive from current discovery. + +Write steps 1–3 atomically — if the write fails, retain +`pending_attempt` so the next retry can re-finalize without a new +commit. + +**Already-pushed, metadata-write failure.** If push succeeded (remote +SHA matches) but the metadata write fails, finalize from the +`pending_attempt` candidate values (`candidate_screenshot_storage`, +`candidate_screenshot_assets`) and the committed consumer state at +the verified SHA — never from the mutable authoring manifest. +Retain `pending_attempt` until the metadata write succeeds. Do not +create a new commit or PR. + +**Unexpected state.** If the remote branch HEAD is neither the +expected base nor the pending commit SHA, stop. Do not force-push, +reset, or re-derive ownership from unknown commits. diff --git a/ui-design/README.md b/ui-design/README.md index c458f0b9..32d86b09 100644 --- a/ui-design/README.md +++ b/ui-design/README.md @@ -36,6 +36,8 @@ graph TD | Docs repository (configured) | For `/ingest`, `/publish` | Load upstream PRD, design doc, UX handoff; publish UI design | | GitHub CLI (`gh`) | For `/publish`, `/respond` | Create PRs, post review comments | | Git | Yes | Branch management, commits | +| Runtime image-reading capability | For screenshot inspection | Open and inspect image files | +| Runtime binary-download capability | For external screenshot inspection | Retrieve image bytes from URLs | ## Inputs @@ -47,6 +49,7 @@ The workflow draws from multiple published sources — never from another workfl | Context path | User-provided filesystem path (non-Jira flow only) | No — used when starting from a local document | | Description | User-provided text (non-Jira flow only) | No — used when starting from a textual description | | UX handoff (`handoff.md`) | Docs repo (from `ux-design` workflow) | No — optional for purely technical work | +| UX screenshots (`screenshots/` or external URLs) | Docs repo (repo-local files or immutable external HTTPS refs alongside `handoff.md`) | No — present only when evaluation captured screenshots | | PRD (`prd.md`) | Docs repo (from `prd` workflow) | Yes | | Design document (`design.md`) | Docs repo (from `design` workflow) | Yes | | `AGENTS.md` / `UI-ARCHITECTURE.md` | Project root | If available | @@ -80,6 +83,8 @@ Jira). /ui-design:ingest EDM-1234 → fetches the [UI] story from Jira → follows references to load UX handoff, PRD, design doc from docs repo + → if UX handoff has screenshots, opens and visually inspects each image + → records screenshot observations (visual details, text/image consistency) → reads AGENTS.md, UI-ARCHITECTURE.md → explores UI codebase (components, hooks, routes, state, tests) → explores backend API (types, endpoints, OpenAPI specs) diff --git a/ui-design/SKILL.md b/ui-design/SKILL.md index c49386cd..3d82e587 100644 --- a/ui-design/SKILL.md +++ b/ui-design/SKILL.md @@ -1,6 +1,6 @@ --- name: ui-design -version: 0.1.0 +version: 0.2.0 description: >- UI design workflow that takes a [UI] Jira story and optional UX handoff artifact, produces a component decomposition with hook design, state management, diff --git a/ui-design/guidelines.md b/ui-design/guidelines.md index 2a030ec3..3f305b3e 100644 --- a/ui-design/guidelines.md +++ b/ui-design/guidelines.md @@ -11,6 +11,12 @@ - **Persona-aware decomposition is required.** When the UX handoff identifies multiple user groups with distinct views or permissions, the component architecture must address conditional rendering, permission gates, and shared vs. persona-specific components explicitly. - **Data annotations drive API review.** Every `Unknown` or `API` source type in the UX handoff's Data Annotations table must be resolved to a specific API endpoint and field — or flagged as a gap. - **Accessibility is architectural.** ARIA roles, keyboard navigation, and focus management are designed alongside components, not bolted on later. +- **Screenshots are visual evidence, not runtime verification.** When UX + handoff screenshots are available, cite them for layout and visual-state + decisions. Screenshots show rendered presentation; they do not verify + keyboard navigation, accessibility compliance, API behavior, or runtime + state management. Flag text/image discrepancies rather than silently + choosing one source or inventing behavior to reconcile them. - **Tests validate behavior, not implementation.** The testing strategy describes what user-facing behaviors to verify, using the project's existing test framework and patterns. ## Shared Content Rules @@ -21,6 +27,7 @@ artifacts and published output from this workflow. ## Hard Limits - No fabricated requirements. Every component, hook, and route must trace to a UX handoff element, design document constraint, PRD requirement, or user direction. +- No fabricated visual observations. When screenshots cannot be inspected (tool unavailability, unreadable files), report the limitation — do not invent observations from captions, alt text, or filenames. - No publishing (creating PRs, posting comments) without explicit user approval. - No Jira modifications without explicit user approval and a dry-run preview first. - **No scope reduction.** Never silently simplify, defer to "v2", use "placeholder", or say "future enhancement" to reduce scope. If scope won't fit, propose a split — don't reduce. diff --git a/ui-design/skills/ingest.md b/ui-design/skills/ingest.md index 67d2c2e6..6f6d7e96 100644 --- a/ui-design/skills/ingest.md +++ b/ui-design/skills/ingest.md @@ -26,7 +26,7 @@ to change, and what constraints apply. - **UX handoff is optional.** The `[UI]` story may not have a UX handoff artifact. If none exists, note it and proceed — the workflow must function for purely technical work. - **Explore relevant areas only.** Don't map the entire codebase. Focus on UI components, hooks, routes, and API endpoints the story will affect. - **Note unknowns.** If you can't determine something from the codebase, say so explicitly. -- **Re-invocation diffs before overwriting.** If `01-context.md` already exists, preserve it before exploring. After compiling new context, diff the key sections against the previous version and present changes to the user before overwriting (see Steps 2a and 8a). +- **Re-invocation diffs.** If `01-context.md` exists, preserve it before exploring. After compiling new context, diff against the previous version and present changes before overwriting (see Steps 2a and 8a). ## Shared Script @@ -99,12 +99,17 @@ delete or overwrite other phase artifacts during ingest. mkdir -p ".artifacts/ui-design/{workspace-id}" ``` +Resolve workspace paths once, anchored to this step's invocation +directory; retain across later `cwd` changes. No hardcoded prefix +or Git-root requirement. + +- `ARTIFACT_DIR` = `.artifacts/ui-design/{workspace-id}` +- `CONTEXT_PATH` = `{ARTIFACT_DIR}/01-context.md` +- `BACKUP_PATH` = `{ARTIFACT_DIR}/01-context.md.prev` + ### Step 2a: Check for Prior Ingest -If `.artifacts/ui-design/{workspace-id}/01-context.md` already exists, this -is a re-invocation. Copy the existing file to -`.artifacts/ui-design/{workspace-id}/01-context.md.prev` so it is preserved -for the diff in Step 8a. +If `{CONTEXT_PATH}` exists, copy it to `{BACKUP_PATH}` for Step 8a. ### Step 3: Read the [UI] Story @@ -267,36 +272,48 @@ fall back to `05-handoff.md` (the private artifact name). If multiple match, present them to the user and ask which is current. If none, proceed without a handoff (it is optional). -If found, read it. - -**Provenance freshness check.** Before extracting content, look for a -provenance footer (`