Repository navigation
tooling(knowledge): records route the claims of shared concerns a change touches - #9145
Conversation
…nge touches A record that owns a shared concern lists it in its review-applicability block's new concerns map: the shared utility's names, the code patterns that re-implement it, and the claims it routes. pnpm specs:for --diff and scripts/review-global-baselines.mjs read a change's added shipped-source lines and route every matching concern's claims, whatever the component record's review_triggers list (knowledge-contracts INV20). - check-knowledge validates concerns (identifiers, compilable patterns, existing claims; triggers optional when concerns are present) and its duplicate-key check now works per JSON object. - specs:for --diff <base> (merge base to the working tree) or <base>..<head> lists the change's files when none are named and puts the concern claims first. Its default view is short: claims, then a count of linked records; --records lists them (Dialog.tsx: 166 lines -> 17). - review-global-baselines lists concernMatches from authority..head. - Seeded concerns: effects/listeners/observers -> react-component-runtime INV2, INV3, INV5, INV6; modality tracking -> interaction-modality INV1-3; press tracking -> interaction-modality INV13; focus return -> AST-085 FR4; announcements -> AST-086 FR1-3, FR12. - AGENTS.md: run pnpm specs:for --diff origin/main before editing. - Tests: validation cases; #8381's diff routes the runtime and modality claims, #8513's listeners the runtime claims; a CSS-only diff and a test file's listeners route nothing; the CLI's concernMatches in a scratch repository.
|
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🔎 Preview ·
|
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approval waits on an engineering owner's decision on the amended architecture:knowledge-contracts INV20 and the four seeded concern blocks: an engineering owner records it by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Separately from that decision, a few fixes are needed:
- The new default of
pnpm specs:for <path>only counts linked records, while INV26, DEC-12, the lookup sentence in knowledge-contracts,AGENTS.mdand the usage text still say it lists them; keep listing by default or amend those texts here. - The new AGENTS.md step runs
--diff origin/mainbefore editing, where it prints usage and exits 2, and untracked new files or runs from a subdirectory route nothing. - The press-tracking concern routes INV13 from
usePressFeedbackanduseLongPressinstead ofcreateActionGroupTracker, the owner INV13 names.
[Automated review]
| route exposes only the listed claim; it never makes the whole record govern | ||
| the component or change. Preserve each matched record, claim, trigger, and | ||
| match reason in the review receipt. | ||
| - Before editing, run `pnpm specs:for --diff origin/main` on your changed |
There was a problem hiding this comment.
Before any edit there is no diff, so pnpm specs:for --diff origin/main prints the usage text and exits 2; concern routing needs added lines. Suggest "After editing, before you commit or push, run …" (and pnpm specs:for <path> before editing). An unstaged new file is also left out of git diff <merge-base>.
| } | ||
|
|
||
| function formatResult(result) { | ||
| export function formatResult(result, {records: listRecords = false} = {}) { |
There was a problem hiding this comment.
With the short view as the default, pnpm specs:for <path> counts linked records instead of listing them. INV26 ("A matching path lists the record"), DEC-12, the lookup sentence in knowledge-contracts.md, AGENTS.md:35, docs/README.md:125 and the USAGE text still say it lists them. For Dialog.tsx the React runtime and interaction-modality records no longer appear by default.
| } | ||
| const diff = | ||
| diffRange == null ? null : readDiff(diffText(DEFAULT_ROOT, diffRange)); | ||
| if (files.length === 0 && diff != null) files.push(...diff.files); |
There was a problem hiding this comment.
diff.files are repository-relative, but they are later resolved against INIT_CWD. Run from packages/core, a staged packages/core/src/X/X.tsx becomes packages/core/packages/core/src/X/X.tsx (not in the working tree) and no concern claim prints. Resolving these paths against the repository root fixes it.
| "claims": ["INV1", "INV2", "INV3"] | ||
| }, | ||
| "press-tracking": { | ||
| "names": ["usePressFeedback", "useMenuPress", "useLongPress"], |
There was a problem hiding this comment.
INV13 is about group press tracking, owned by the action-group tracker (createActionGroupTracker) and useMenuPress. usePressFeedback is the press-paint hook that every pressable surface calls, and useLongPress is a public gesture hook, so this routes INV13 for every Button-style press change and not for a change that calls or copies the tracker. Suggest ["createActionGroupTracker", "useMenuPress"].
…y directory, and the AGENTS step runs after editing - The short view lists every linked record by id (INV26); --records shows how each links, with cited claims and deciding specs. - --diff paths are repository-relative, so a run from packages/core routes its concerns; untracked new files count as added; an empty diff prints that there are no changes instead of the usage text. - AGENTS.md: specs:for <path> before editing, specs:for --diff origin/main after editing, before commit or push. - press-tracking names INV13's owners: createActionGroupTracker and useMenuPress.
There was a problem hiding this comment.
Approval waits on an engineering owner's decision on the amended architecture:knowledge-contracts INV20 and its format paragraph, together with the four seeded concern blocks. An engineering owner records it by commenting /approve-spec <full head SHA> on this pull request, and the author may do so if they are an engineering owner. The follow-up commit fixes the earlier lookup and --diff issues, but these still need changes:
- The
focus-returnconcern in AST-085 routes FR4 fromfocusForLastInput,isFocusDetachedanduseFocusReturnVisibility, whichfocusReturn.tsand interaction-modality'sapplies_totie to interaction-modality INV1, so route them from that record instead. - When no whole record links to a path, the default view says "it cites the claims above" but never prints the cited claims, and USAGE,
AGENTS.mdand the lookup sentence still describe the full view. - The description says
specs:for Dialog.tsxreturned none of the runtime rules, though it listed the runtime record and only routed none of its claims; also,--diff a...bruns a two-dot diff.
[Automated review]
| "focus-return": { | ||
| "names": [ | ||
| "focusForLastInput", | ||
| "isFocusDetached", | ||
| "useFocusReturnVisibility" | ||
| ], | ||
| "patterns": ["document\\.activeElement"], | ||
| "claims": ["FR4"] |
There was a problem hiding this comment.
These utilities belong to interaction-modality: focusForLastInput says it implements architecture:interaction-modality INV1 (packages/core/src/utils/focusReturn.ts:42-43), useFocusReturnVisibility.ts is in that record's applies_to, and this record never mentions focus return. As seeded, a change that adds or copies a programmatic focus return is told to check FR4 ("Becoming disabled keeps focus") rather than INV1, and every one of the 83 shipped document.activeElement lines routes FR4. Suggest moving these names to interaction-modality's block, routing INV1, and keeping an FR4 concern only with a matcher for disabling or removing a focused control.
| if (result.records.length === 0) { | ||
| lines.push( | ||
| result.decisions.length > 0 | ||
| ? ' No whole record links to this path; it cites the claims above.' |
There was a problem hiding this comment.
The short view never prints cited claims, so "the claims above" points at nothing: for .changeset/announce-has-replace.md the only governing claim, spec:AST-086/FR9, appears only with --records. Suggest printing cited-claim and deciding-spec ids on one line each (as Records: is), or pointing this message at --records. USAGE (:890-891), AGENTS.md:36-37 and the docstring at :826 also still describe the full view.
| maxBuffer: 256 * 1024 * 1024, | ||
| }); | ||
| if (range.includes('..')) { | ||
| const [base, head] = range.split(/\.\.\.?/); |
There was a problem hiding this comment.
This accepts a...b but runs git diff a b, not git's merge-base form, so --diff origin/main...HEAD counts lines main removed since the branch point as added. Suggest rejecting ... or diffing from git merge-base a b.
…ort view prints cited claims; --diff a...b diffs from the merge base - The focus-return utilities cite interaction-modality INV1 and sit in its applies_to, so their concern moves into that record and routes INV1; the AST-085 block (FR4 is about a control becoming disabled) is withdrawn, and the 83-line document.activeElement pattern goes with it. - The short view lists cited claims and deciding specs by id beside the records; --records shows how each one links. - --diff <base>...<head> diffs from their merge base, as git does.
…ce and modality tracking With #9139's INV16 on main, the runtime record owns two more concerns: slot-presence (isRenderable, or a typeof ... !== 'boolean' check) and modality-tracking (useInteractionModalityTracking, or a pointerdown or keydown listener) both route INV16, beside interaction-modality's own INV1-INV3 for modality. Tests: #8236's Badge check routes INV16 naming isRenderable; #8381's diff now also routes INV16.
There was a problem hiding this comment.
Approval waits on an engineering owner's decision on the amended architecture:knowledge-contracts INV20 and its format paragraph, together with the seeded concern blocks in react-component-runtime, interaction-modality and AST-086; an engineering owner records it by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. The latest commit fixes the focus-return routing, the short view and --diff a...b, but two items still need changes:
- Add
trackInteractionModalityInto the modality-tracking concern'snames: an Effect that calls it, asDialog.tsxdoes, currently routes no interaction-modality claim. - The description says
specs:for Dialog.tsxreturned none of the React runtime rules, but it listed the runtime record and only routed none of its claims.
[Automated review]
| }, | ||
| "concerns": { | ||
| "modality-tracking": { | ||
| "names": ["useInteractionModalityTracking", "getInteractionModality"], |
There was a problem hiding this comment.
trackInteractionModalityIn (packages/core/src/utils/interactionModality.ts:126) is the shared second-document tracker, and packages/core/src/Dialog/Dialog.tsx:548-554 calls it from its own Effect. It is not in these names, so a change whose only modality line is an Effect calling it routes the runtime claims but no interaction-modality claim. Suggest adding it here.
There was a problem hiding this comment.
Approval waits on an engineering owner's decision on the amended architecture:knowledge-contracts INV20 and its format paragraph, together with every seeded concern block, including this push's slot-presence and modality-tracking routes to react-component-runtime INV16; an engineering owner records it by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. The earlier note about the description's base gap is withdrawn, but these still need changes:
- Add
trackInteractionModalityInto both modality-tracking blocks'names: an Effect that calls it, asDialog.tsxdoes, routes neither INV16 norinteraction-modalityINV1–INV3. - Narrow the slot-presence pattern: 8 of the 11 shipped lines it matches are data checks such as
Markdown/remark.ts:1045, which are then told to follow INV16 withisRenderable; anchoring it as!= null && typeof …keeps the three slot checks. - The amended INV20 still says every routed match records a trigger, but
concernMatchesrows carry a concern instead; say "trigger or concern".
[Automated review]
| }, | ||
| "slot-presence": { | ||
| "names": ["isRenderable"], | ||
| "patterns": ["typeof [\\w.?]+ !==? ['\"]boolean['\"]"], |
There was a problem hiding this comment.
This matches 11 shipped lines today, but only three are slot checks (Badge.tsx:252, RichTextEditor.tsx:602, isRenderable.ts:32). The rest validate data, such as Markdown/remark.ts:1045 (typeof checked !== 'boolean'), and five are outside @astryxdesign/core, so a parser edit is told to follow INV16 with "shared: isRenderable". Anchoring it as != null && typeof [\w.?]+ !==? ['"]boolean['"] keeps the three slot checks (plus one docsite line) and still routes the #8236 fixture.
| "claims": ["INV16"] | ||
| }, | ||
| "modality-tracking": { | ||
| "names": ["useInteractionModalityTracking"], |
There was a problem hiding this comment.
trackInteractionModalityIn is missing here too. Dialog.tsx:548-554 calls it from its own Effect, which is the gap INV16 names, but a change whose only modality line calls it routes no INV16 from this concern. Suggest adding it to names here and in interaction-modality's modality-tracking block.
| the whole record, adjacent claims, drafts, or proposed-head authority govern. | ||
| Every routed match records the base authority commit, record digest, record id, | ||
| path, trigger, claim id, claim title, and deterministic match reason. | ||
| path, trigger, claim id, claim title, and deterministic match reason. A record |
There was a problem hiding this comment.
This sentence still says every routed match records a trigger, but concernMatches rows carry concern and no trigger. Suggest "trigger or concern" so the provenance list covers both routes.
…ck; modality names trackInteractionModalityIn; INV20 provenance covers concerns - slot-presence's pattern is anchored on != null && typeof ... !== 'boolean', so data validation (typeof checked !== 'boolean') routes no INV16; #8236's Badge check still does. - Both modality-tracking blocks name trackInteractionModalityIn, which Dialog calls from its own effect. - INV20: every routed match records its trigger or concern.
With #9142's FR9 (focus returns in the opener's document) on main, the overlay-dismissal family owns a focus-return concern too: the same utilities route FR9 beside interaction-modality INV1.
There was a problem hiding this comment.
Approval waits on an engineering owner's decision on the amended architecture:knowledge-contracts INV20 and its format paragraph, together with every seeded concern block, including this push's family:overlay-dismissal focus-return → FR9 route; an engineering owner records it by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. The earlier modality, slot-presence and provenance notes are resolved at this head, but three things still need changes:
- INV20's first sentence still says a block participates only when it maps a semantic trigger, which
spec:AST-086's concerns-only block does not; say "a semantic trigger or a concern", drop "also" from "MAY also list", and use the seeded slot pattern in the format example. specs:for --diffandreview-global-baselines.mjsread no change when a contributor's git setsdiff.noprefixordiff.mnemonicPrefix; pass--src-prefix=a/ --dst-prefix=b/to eachgit diff.- Files such as
internal/a11y-spec/src/patterns/*.fixtures.tscount as shipped source, so a fixture's keydown listener routes Core modality claims; skip*.fixtures.*andfixtures/, or narrow the "fixtures are skipped" wording.
The build-storybook failure comes from the branch predating main's .github/scripts/package-size.mjs, so merging main should clear it.
[Automated review]
| path, trigger, claim id, claim title, and deterministic match reason. | ||
| path, trigger or concern, claim id, claim title, and deterministic match | ||
| reason. A record | ||
| that owns a shared concern MAY also list it in the same block's `concerns` |
There was a problem hiding this comment.
INV20's first sentence (lines 247-249) still says a block participates only when it "maps a semantic trigger", and "MAY also list" here implies triggers stay required, but the format paragraph lets a block carry concerns instead of triggers, and spec:AST-086's block does. Suggest "maps a semantic trigger or a concern to exact claim ids" and dropping "also".
| return git(['diff', '--no-color', '--no-ext-diff', base, head]); | ||
| } | ||
| const base = git(['merge-base', 'HEAD', range]).trim(); | ||
| const tracked = git(['diff', '--no-color', '--no-ext-diff', base]); |
There was a problem hiding this comment.
With diff.noprefix=true or diff.mnemonicPrefix=true in the user's git config, these headers lose their a/ and b/ prefixes, readDiff finds no file, and --diff prints No changes against … even for a change that routes claims. Passing --src-prefix=a/ --dst-prefix=b/ here, at lines 920 and 924, and in review-global-baselines.mjs:76 fixes it.
| * listener a test adds re-implements nothing. | ||
| */ | ||
| const NOT_SHIPPED = | ||
| /(?:^|\/)(?:__tests__|__fixtures__|__mocks__)\/|\.(?:test|spec|stories)\.[cm]?[jt]sx?$/; |
There was a problem hiding this comment.
This skips __fixtures__ directories but not *.fixtures.ts files or fixtures/ directories: 41 such files are read as shipped, and a keydown listener added to internal/a11y-spec/src/patterns/accordion.fixtures.ts (test data for its jsdom and chromium tests) routes interaction-modality INV1–INV3 and react-component-runtime INV16. Suggest adding \.fixtures?\.[cm]?[jt]sx?$ and (?:^|/)fixtures/, with a test.
…/ prefixes; fixture files are not shipped - INV20: a block maps a trigger or a concern to exact claims, as AST-086's concerns-only block does. - Every diff passes --src-prefix=a/ --dst-prefix=b/, so diff.noprefix or diff.mnemonicPrefix in a user's git config no longer hides the files. - *.fixtures.ts files and fixtures/ directories are test data and route nothing.
There was a problem hiding this comment.
Approval waits on an engineering owner's decision on the amended architecture:knowledge-contracts INV20 and its format paragraph, together with every seeded concern block; an engineering owner records it by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. The earlier fixes (INV20's first sentence, the a//b/ diff prefixes and the fixture filter) are in at this head. One change is still needed: pnpm specs:for --diff … --records prints none of the concern-routed claims that the default view and --json carry, because the long view in formatResult never reads concernClaims, so print the same "Follow first" block there and cover it in the --records test.
[Automated review]
|
|
||
| function formatResult(result) { | ||
| export function formatResult(result, {records: listRecords = false} = {}) { | ||
| if (!listRecords) return formatShortResult(result); |
There was a problem hiding this comment.
With --records, the long view below never prints result.concernClaims, so pnpm specs:for --diff HEAD --records shows none of the concern-routed claims the default view lists under "Follow first" (--json still carries them). Suggest moving the Follow-first block into a helper both formatters call, and asserting it in the --records test.
Both views now share one formatter for the Follow-first block, so `--diff … --records` no longer drops the claims the default view lists.
There was a problem hiding this comment.
Approval waits on an engineering owner's decision on the amended architecture:knowledge-contracts INV20 and its format paragraph, together with every seeded concern block; an engineering owner records it by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Separately, main now has useInteractionModalityTrackingFor (#9148): please merge main and add it to both modality-tracking names lists, since today #9148's own edits to useTouchTrigger and useMenuPress route no modality claim. The earlier --records issue is fixed.
[Automated review]
| "names": [ | ||
| "useInteractionModalityTracking", | ||
| "trackInteractionModalityIn", | ||
| "getInteractionModality" |
There was a problem hiding this comment.
main now exports useInteractionModalityTrackingFor (#9148), the modality hook useTouchTrigger and useMenuPress call, and the name matcher's (?![\w$]) keeps useInteractionModalityTracking from matching it. On this head merged with main, #9148's own edits to those two hooks route no claim, while the same lines calling useInteractionModalityTracking() route INV1–INV3 and INV16. Suggest merging main and adding useInteractionModalityTrackingFor here and in react-component-runtime's modality-tracking block.
| "claims": ["INV16"] | ||
| }, | ||
| "modality-tracking": { | ||
| "names": ["useInteractionModalityTracking", "trackInteractionModalityIn"], |
There was a problem hiding this comment.
Same gap as the interaction-modality block: add useInteractionModalityTrackingFor (on main since #9148) so a hook that switches to it routes INV16.
…dalityTrackingFor #9148 added the document-scoped hook that useTouchTrigger and useMenuPress call; a change to those calls now routes interaction-modality INV1-INV3 and react-component-runtime INV16 like the other modality names.
There was a problem hiding this comment.
Approval waits on an engineering owner's decision on the amended architecture:knowledge-contracts INV20 and its format paragraph, together with every seeded concern block in react-component-runtime, interaction-modality, overlay-dismissal and AST-086. An engineering owner records it by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. The earlier useInteractionModalityTrackingFor gap is fixed, and no other change is needed.
[Automated review]
|
/approve-spec 7267e68 |
github-actions
Bot
removed
the
needs:spec-owner-review
There was a problem hiding this comment.
An engineering owner has approved the amended architecture:knowledge-contracts INV20 and every seeded concern block at this head, and the review found no defects: concern routing, pnpm specs:for --diff and the baseline helper's concernMatches behave as described, including on #8381's real Dialog diff. Nothing else is needed from the contributor.
[Automated review]
Maintainer impact
A contributor or reviewer who changes how a component handles a shared concern, such as an effect, a modality listener, focus return or an announcement, is shown the rules that govern it only when someone names the right trigger.
pnpm specs:forroutes global claims from the component record'sreview_triggers, andreview-global-baselines.mjsfrom the caller's--triggers.Dialog.spec.mddoesn't listreact-runtime, sopnpm specs:for Dialog.tsxreturned none of the React runtime rules for a Dialog effect change. The default output is also about 166 lines for Dialog.tsx, so few people read it.Intended invariant
architecture:knowledge-contractsINV20 (amended here): global review routing stays explicit and claim-scoped. A record that owns a shared concern now declares it beside its triggers, so a change that touches the concern routes those claims from the change itself.Change
review-applicability:v1block can carry aconcernsmap. Each concern lists the shared utility'snames, thepatternsthat re-implement it, and theclaimsit routes. There's no central catalog: a claim is routed from the one record that owns it.scripts/check-knowledge.mjsvalidates the map: identifiers, patterns that compile, claims the record defines, and no unknown keys.triggersbecomes optional whenconcernsis present. It also collects concern matches. Its duplicate-key check now works per JSON object, so two concerns can both listnames.scripts/lib/review-diff.mjsreads a diff's changed files and its added shipped-source lines. Tests, stories and fixtures are skipped.pnpm specs:for --diff <base>reads from the merge base to the working tree (untracked new files included), and--diff <base>..<head>reads between two commits. It lists the change's files when none are named (repository-relative, so it works from any directory) and prints the concern-routed claims first, each with the line that matched. An empty diff says there are no changes.--recordsshows how each one links. Dialog.tsx goes from 166 lines to 18.scripts/review-global-baselines.mjsaddsconcernMatchesfor the head's added lines (authority..head). Each match carries its provenance and the matching lines.architecture:react-component-runtimeINV2, INV3, INV5, INV6isRenderable, or a!= null && typeof … !== 'boolean'check) →architecture:react-component-runtimeINV16architecture:react-component-runtimeINV16useInteractionModalityTracking,trackInteractionModalityIn,getInteractionModality, or a pointerdown/keydown listener) →architecture:interaction-modalityINV1–INV3createActionGroupTracker,useMenuPress) →architecture:interaction-modalityINV13focusForLastInput,isFocusDetached,useFocusReturnVisibility) →architecture:interaction-modalityINV1 andfamily:overlay-dismissalFR9spec:AST-086FR1–FR3, FR12AGENTS.md: runpnpm specs:for <path>before editing a path. After editing, before commit or push, runpnpm specs:for --diff origin/mainand follow every concern-routed claim.Not in this PR: a scroll-lock concern (it routes
spec:AST-094FR19 once #8098 lands).Evidence
pnpm specs:for packages/core/src/Dialog/Dialog.tsxroutes accessibility, behavior and public-api claims only, with no React runtime rule, even for a diff that adds an effect.pnpm specs:for --diff <base>..<#8381 head>on Dialog.tsx prints:architecture:react-component-runtimeINV16 for the modality hook. fix(Badge): a badge grows with its text #8236's Badge diff printsarchitecture:react-component-runtime INV16 ← slot-presence (line 252: typeof label !== 'boolean'; shared: isRenderable). fix(DateInput, DateTimeInput): after a month arrow, Safari's calendar rests on the month the title names #8513's MonthScroller listeners route the runtime claims, and a CSS-only diff routes nothing.Scope
/approve-spec.Testing
scripts/check-knowledge.test.mjs: concern validation (each rejection, triggers optional, the same key in two objects) and concern routing with evidence.scripts/lib/review-diff.test.mjs:review-global-baselines.mjslistsconcernMatchesin a scratch repository.scripts/specs-for-file.test.mjs: concern claims come from the file's own added lines; the short view prints them first and still lists every linked record by id;--recordsshows the routes.pnpm check:knowledge: aligned. The four suites pass (188 tests).Writing rubric results
Run over the records this PR edits (
knowledge-contracts,react-component-runtime,interaction-modality,AST-086,overlay-dismissal):