Repository navigation
docs: keep one-off PR evidence outside the source tree - #7651
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
github-actions
Bot
added
the
needs:spec-owner-review
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewNo new or modified components detected. Bundle Size SummaryNo component packages changed. Accessibility AuditStatus: No accessibility violations detected. Visual Regression40 of 478 shot(s) changed. A change here is a question, not a failure: check whether the after is the
and 20 more. Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
The screenshot removal checks out: nothing in the repository references docs/pr-assets/, and all 27 images the eight earlier pull requests embed still load, with identical bytes, from commits that GitHub's pull-request refs and the new evidence/ tags retain. Approval needs a maintainer to settle the governing spec for INV22–INV24 in architecture:knowledge-contracts, which no current spec states: adopt them as written, adopt them with edits, or reject them so the removal lands without them and without the guide section and ignore rule that project them. If they are adopted, two INV24 delegations need correcting first:
- "AST-029 owns component-audit records" gives the audit records a second owner, since INV12 owns the audit datastore and AST-029 FR13 defers to it; naming the audit pull request's receipt and eligibility report (FR5, FR10) fixes it.
- "and its artifact lifecycle" credits AST-030 with a rule FR12 never states, so the phrase should go or name the claim that owns retention.
[Automated review]
| [AST-029](../specs/AST-029/spec.md) owns component-audit records, and | ||
| [AST-030](../specs/AST-030/spec.md) owns visual-regression execution, baseline | ||
| adoption, and its artifact lifecycle. This placement rule does not add a capture |
There was a problem hiding this comment.
If this amendment is adopted, these two delegations should match their owners. AST-030 FR12 governs how the visual-regression run produces and consumes artifacts but sets no artifact lifetime or retention, so "and its artifact lifecycle" reads wider than the claim. "Component-audit records" can be read as the audit datastore, which AST-029 FR13 leaves with this record's INV12; AST-029 itself owns the audit pull request's receipt and eligibility report (FR5, FR10).
| retention anchor at that exact commit; a commit hash or a temporary branch | ||
| alone is insufficient. An immutable non-release `evidence/` tag may retain | ||
| historical Git-hosted evidence when attachment publication is unavailable. |
There was a problem hiding this comment.
Two points for the owner here. GitHub's pull-request refs already hold every commit the earlier pull requests link to, and this clause does not say whether such a ref counts as the durable anchor; if it does not, every cleanup must add tags that then need upkeep. Also, re-publishing a capture as an attachment cannot keep an existing commit-pinned link working, so "when attachment publication is unavailable" does not fit the case this clause serves, and "immutable" describes nothing the repository enforces.
| | INV21, DEC-10 | `.github/scripts/component-design-decisions.test.mjs`, `.github/scripts/review-signal-decision.test.mjs`, `.github/scripts/spec-owner-decision.test.mjs`, and `.github/scripts/lib/classify-visual.test.mjs` | A malformed/local DD escapes validation, stale approval clears a new head, owner routing loses mixed-change gates, or executable/API/unknown runtime code is misclassified as appearance-only | | ||
| | DEC-11 | `.github/scripts/review-signal-decision.test.mjs`, `.github/scripts/review-clear-workflow.test.mjs`, and `.github/scripts/review-signal-workflow.test.mjs` | An app approval clears a read-only or fork author or an earlier head; a later app request for changes or dismissal leaves the gate clear; an app approval overrides an ENGOWNER request for changes; or an unset app or unreadable permission clears the gate | | ||
|
|
||
| | Invariant | Evidence | Failure signal | |
There was a problem hiding this comment.
This row fits the existing Verification table's column widths, so it can be appended there instead of opening a second table with a duplicate header (a reader that parses the section as one table takes this header and separator as data rows).
|
/approve-spec ab68e0a |
github-actions
Bot
removed
the
needs:spec-owner-review
<!-- template: maintenance.md --> ## Maintainer impact `.github/scripts/accessibility-audit.integration.test.mjs` starts the audit CLI's static server on `40000 + (pid % 10000)`, one port per fixture. That range sits inside Linux's ephemeral port range (32768–60999 by default), where the kernel picks source ports for outgoing connections, so on a busy machine another process can already hold the chosen port. The CLI then exits with `listen EADDRINUSE` before it reaches the behavior under test, and the suite fails with no change to the code it covers. The full `pnpm test` runs reported in #7651 and #7694 both hit this, and both passed when the file was rerun alone. ## Intended invariant The fixtures exercise the CLI's readiness, routing and report paths. Which port its server binds is incidental, so a fixture run should fail only for a defect in those paths. ## Change - `accessibility-audit.js` accepts `--port 0`, the usual way to ask the OS for a free port. It builds the story URLs and the expected origin from the port its server actually holds (`server.address().port`). Every other `--port` value behaves as before, and the default stays 6007. - The fixtures pass `--port 0` and drop the PID-derived counter. - The mock browser's `goto` now fetches the URL it receives, so a fixture passes only when the audit opens the port its server holds. A new case asserts that the server reports a real port. No workflow passes `--port`, so CI audits are unchanged. ## Evidence - Failure before the change: `Error: listen EADDRINUSE: address already in use :::43064` at `accessibility-audit.js:195`, in a full local run beside parallel builds. The two affected cases pass alone. This test file run against main's script fails 11 of 11 cases with `Invalid --port value: 0`. - Success after the change: 11 of 11 pass. - Mutation check: building the story URL from the requested port instead of the bound one fails 9 of 11 cases (`fetch failed`). - Product/runtime behavior verified unchanged: no package source changes, and the default and explicit-port paths are untouched. ## Scope - [x] No intended public API, product behavior, visual direction, or policy change. - [x] Product changes discovered during the work were removed or split. - [x] Public text and artifacts contain no internal Meta context. ## Testing - `pnpm exec vitest run --project node .github/scripts/accessibility-audit.integration.test.mjs`: 11/11 pass (on main's script: 11/11 fail). - `pnpm exec vitest run --project node .github/scripts`: 64 files, 1,150 tests pass. - `pnpm -F @astryxdesign/core typecheck`, ESLint on both files, `pnpm check:repo`: pass. - Prettier: both files already differ from Prettier's output on main. This change adds no new differences, and the files are not reformatted.
Maintainer impact
One-off review screenshots remain in
docs/pr-assets/after their pull requests close. They have no product or test consumers, but they add binary material to every source checkout. Removing them without retaining the exact linked commits risks losing historical review evidence after branch cleanup.Intended invariant and approved decision
Keep disposable review evidence outside the source tree without breaking old evidence links. Extend the existing
architecture:knowledge-contractsowner with INV22–INV24: GitHub attachments are preferred for manual one-off evidence; immutable, non-release archive tags preserve historical Git-hosted evidence when needed. Reusable fixtures, accepted visual baselines, public documentation media, and owner-permitted repository receipts stay with their code.Spec approved by engineering owner
imdreamrunnerfor exact headab68e0ac4eef56214fb30976b276db1cfef71f54. The approval adopts INV22–INV24 with the corrected ownership citations below and is recorded through the repository's exact-head/approve-speccommand. No merge or auto-merge is requested.Existing authority searched
An independent audit inspected current records and all 252 open pull requests, including fully paginated changed-file lists. Searches covered the canonical owner,
docs/pr-assets, review evidence, screenshot storage, retention, receipts, attachments, PR templates, and branch pruning. No competing owner or overlapping owner/path change was found.architecture:knowledge-contractsINV2, INV10, INV13 and INV16 owns knowledge placement and evidence versus authority; extending it avoids a parallel storage spec.architecture:knowledge-contractsINV12 owns the operational audit datastore. AST-029 FR5 and FR10 own the audit PR's evidence receipt and exact-head eligibility report. AST-030 FR12 owns shared visual-regression execution and baseline adoption, not artifact retention. Their proof boundaries, stores, and artifact lifetimes are unchanged.Change
docs/pr-assets/: 1,203,690 bytes across eight historical PR directories./docs/pr-assets/as local scratch. No new check owner, schema, capture tool, receipt catalogue, or release behavior.Evidence
Exhaustive inventory and preservation
6ee9e0dfef32adf9c5431379e10afb8dcb9360eaevidence/pr-4808-review-assets920de0ae828b3ed84e2e2dfbcee472c14e40f554evidence/pr-6652-review-assets3ea4bd1171d8e202b807b394a50bb78cc57806a9evidence/pr-6708-review-assets4d397ad5360a862201a1479018ade1210d39d449evidence/pr-6856-review-assetsc740fad9caed02d133a8f1cccc7c18a80ba72b19evidence/pr-6883-review-assetsa22ae115846048111c542175cc8b560ad9a7915cevidence/pr-6898-review-assets8349dd6b7715fa2e3e1a74c4d19a423732b17619evidence/pr-7192-review-assetsbcc006e7f7c73cc9060a2fc2412db9acaf56a5b7evidence/pr-7193-review-assetsAll 32 files are also retained in the complete source snapshot, tag
evidence/assets-main-2026-10-16at base2ec83d72a1f660257e483285a40f64307ab2366d.The eight PRs' descriptions, issue comments, reviews and review comments contain 27 distinct pinned image URLs, not 32. All 27 remained HTTP 200 with identical SHA-256 bytes before and after deletion. The other five files are #6708's 320px companion captures (chat message and two before/after pairs): unembedded, but retained byte-for-byte in the full snapshot. All 32 public snapshot URLs were checked against their original Git blobs.
Tags were uploaded atomically and all nine remote refs were checked against their exact commits before source deletion. Ordinary branches are not adequate here: the existing branch cleaner deletes closed-PR branches and no-PR branches after 60 days. It does not prune tags; all push workflows are limited to
main, so these non-release tags trigger no publication. No existing remote ref was moved or deleted, no old PR was edited, and no history was rewritten.Wider reference search also found six #5623 captures at a separate non-main commit. They are outside this deletion and unchanged.
Product/runtime behavior unchanged
docs/pr-assetsconsumers or references to the removed filenames. Asset inspection and the owning PR histories classify them as review captures, not fixtures or docsite media..github/visual-baseline/, reusable fixture/ideal media, andapps/*/public/assets. Package sources, stories and tests are unchanged.Scope
ab68e0ac4eef56214fb30976b276db1cfef71f54; a new commit requires a new approval.Writing rubric results
Testing
pnpm check:knowledgeandnode scripts/check-knowledge.mjs --base 2ec83d72a1f660257e483285a40f64307ab2366d— passed.git diff --check— passed.pnpm build— passed.pnpm lint:strict— passed, including repository guardrails.pnpm test --maxWorkers=16— 24,294 passed, 52 skipped, 2 expected failures; three failures in two unchanged files. The build-kit timeout passed on focused rerun (46/46). Accessibility-audit integration failures were port collisions (EADDRINUSE) in the unchanged fixture's PID-selected port range; all 10 passed on isolated rerun. The same isolated suite on the unmodified base also reproduced the port-collision failure. No test expectations or product code were changed. The initial full run is not reported as green.lint-staged, Prettier,pnpm check:repo) — passed.