chore(#455,#498): enforce src/core boundaries and catch openMenu extension classes - #554
Merged
Merged
Conversation
…tension classes, verify NUL fix #455: add a build/check-boundaries.mjs RULES entry forbidding src/core from importing src/workspace, src/application, src/ui, or src/net, matching the invariant src/application/main-surface.ts already documented but nothing mechanically enforced; extends tests/unit/dashboard-boundaries.test.js with a real-tree check plus a sabotage test proving the rule actually catches a violation. #498: add a dedicated 'openMenu extension classes have CSS rules' test to tests/unit/typography-contract.test.js that scans all of src/ui/**/*.ts for literal menuClass/extraClass values, plus the goClass/cancelClass values openConfirmMenu callers pass at their dash-tree-confirm*/qtab-close-confirm*/ dash-tile-confirm* call sites (confirm-menu.ts forwards these as extraClass, so the literal lives at the caller). Includes a sabotage check. The scan surfaced a real gap — dash-style-item (dashboard.ts's layout-picker row) had no CSS rule at all — so src/styles.css now gives it one. #435: tests/unit/filter-bar.test.ts's 4 raw NUL bytes were already fixed as a side effect of the #459 filter-to-variable rename (now tests/unit/variable-bar.test.ts, using one escaped GROUP_KEY constant). Verified no raw NUL bytes remain under src/ or tests/; no code change needed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GiubaoqEuBzAyo5C4P8Vqr
This was referenced Jul 29, 2026
This was referenced Jul 29, 2026
Closed
lesandie
pushed a commit
to lesandie/altinity-sql-browser
that referenced
this pull request
Aug 2, 2026
…s PR Altinity#554 added Both gates passed while the thing they guard was removed. Verified empirically, not by inspection. 1. The CSS contract counted comment prose as styling. Both class scanners (the pre-existing `class:` one and Altinity#498's new menuClass/extraClass one) ran their selector regex over RAW `src/styles.css`, so any class merely NAMED in a comment looked styled. This file already computes a comment-stripped `declarations` for exactly this reason — px values in prose must not read as declarations — but selectors never used it. The masking is real, not theoretical: each confirm menu's comment cross-references its siblings, so `.dash-tree-confirm`'s prose names "`.qtab-close-confirm*` and `.dash-tile-confirm*`". Deleting BOTH real `.qtab-close-confirm` rules while leaving that comment kept all 338 assertions in the file green. Altinity#498's own sabotage test missed it because its target, `dash-tile-menu-danger`, happens to be named in no comment. Both scanners now share `styledClassNames()`, built from `declarations`. A second sabotage test pins the specific regression: it asserts the old raw-CSS scan WOULD be fooled by the comment and the current one is not. Exposing this surfaced one genuine pre-existing gap: `dash-row` has no rule and never has — it was only ever covered by the two comments naming it. Correct by design (its grid is set inline by the flow renderer, because the column count is dynamic, and `.dash-grid` must not impose columns), so it joins ALLOWED with that rationale rather than getting a redundant rule. 2. The `src/core` boundary rule was unenforced. The spec re-implements the import walk rather than exercising `build/check-boundaries.mjs`, so deleting the production `src/core` RULES entry left this spec at 60/60 and `check:arch` reporting "OK ... 8 active rules" — Altinity#455's entire deliverable was removable with nothing red. A text-read assertion now binds the mirror to the production rule (text, not import: the checker runs its whole gate at module top level and exits non-zero, so importing it would run the gate inside the test process). Also drops the on-disk probe: it wrote a real file into `src/core` and deleted it in `finally`, which a crash between the two would have left behind. The walk now accepts virtual `[path, source]` files instead, so the working tree is never touched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GiubaoqEuBzAyo5C4P8Vqr
lesandie
pushed a commit
to lesandie/altinity-sql-browser
that referenced
this pull request
Aug 2, 2026
The e2e job was gated to tags, schedule and manual dispatch, so it reported `skipped` on every pull request and every push to `main` while the overall run still reported success. A check that never runs is not a check. The cost was concrete: two `tile-open-workbench` specs have been failing deterministically on chromium, firefox AND webkit, and nothing surfaced it. Today's `bug`/`low` batch (Altinity#554-Altinity#558, Altinity#561-Altinity#563) all merged "green"; two of those PRs changed or added e2e specs that CI never executed. The failures only appeared when the workflow was dispatched by hand (6 failed / 568 passed on `main`), and a second dispatch at f68861c showed the identical 6 failures, so they pre-date that batch. Filed as Altinity#565. Pull requests now run **Chromium only**, path-gated on anything that can move rendered geometry (`src/**` — which is where `styles.css` lives — plus `tests/e2e/**`, `playwright.config.js`, `build/**`, `schemas/**`, the manifests and the workflows). Both Altinity#565 failures reproduce on Chromium, so one engine is enough to have caught them, and it is cheap enough to sit on every relevant PR. Tags, nightly and manual dispatch keep the full three-engine matrix: engine-specific breakage is real here, and that is what a release has to clear. The `gate` job needed no change — it already fails on any `failure` among its needs and tolerates a legitimately skipped job. Altinity#565's two specs are quarantined with `test.fail()` so this gate is meaningful from the first run rather than red on arrival. Deliberately not `skip`/`fixme`: the specs keep executing, so when the underlying bug is fixed they report "expected to fail, but passed" and the quarantine has to be removed. Their assertions encode Altinity#535's intended widen semantics and must not be rewritten to match the broken output. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GiubaoqEuBzAyo5C4P8Vqr
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
Three low-priority gate/test-infra fixes with no user-facing behaviour change.
Closes #455 —
build/check-boundaries.mjshad nosrc/corerule, so the invariantsrc/application/main-surface.tsdocuments ("src/core/must never importsrc/workspace/") was not mechanically enforced. Adds theRULESentry, deliberately narrower than "no imports from any other layer" so the documented legacy type-onlycore/saved-io.ts→editor/spec-editor.types.jsdependency keeps passing.tests/unit/dashboard-boundaries.test.jsgains a real-tree check plus a sabotage probe that plants a throwaway forbidden import insidesrc/core, asserts the detector flags it, and removes it in afinally— so the clean-tree assertion cannot go vacuous.Closes #498 — the typography contract only scanned for
class: '…'hyperscript literals, so every class supplied throughopenMenu/openConfirmMenuoptions (menuClass,extraClass,goClass,cancelClass) was invisible to it. Adds a separateopenMenu extension classes have CSS rulestest that walks all ofsrc/ui/**/*.tsrecursively (not a curated list, so new menu consumers are covered automatically), splits whitespace-separated class groups, and asserts each has a rule insrc/styles.css. The existing generic scanner is untouched, so.file-menu/.fm-item/.fm-sectionstay covered. A sabotage test proves removing one such rule fails the contract.The new scanner immediately surfaced one real gap:
.dash-style-item(on every layout-picker row viabuildLayoutMenu) had no rule at all. Added one — see the note below.#435 needed no code change and is closed separately with the evidence.
tests/unit/filter-bar.test.tsno longer exists: #459 renamed it totests/unit/variable-bar.test.tsand replaced the four raw0x00bytes with a single escapedGROUP_KEYconstant matching production. Verified independently — no file undersrc/ortests/onmaincontains a NUL byte (only PNGs do, as expected).Reviewer note on the one CSS line
.dash-style-item { justify-content: space-between; }is a functional declaration added to satisfy a test gate, which deserves scrutiny. It is verifiably inert: everydash-style-itemrow setstrailingunconditionally (dashboardStyleKeyCaps(shortcut)), and.fm-trailing'smargin-left: autoalready absorbs all free space, sospace-betweenhas nothing left to distribute. It documents the row's existing intent rather than changing layout. Flagged deliberately rather than buried.Checklist
npm testpasses (the per-file coverage gate is non-negotiable)npm run buildsucceeds (single-filedist/sql.html)src/core/, network insrc/net/(injected fetch), DOM insrc/ui/CHANGELOG.md([Unreleased]) updated if behavior or the deployed surface changed🤖 Generated with Claude Code
https://claude.ai/code/session_01GiubaoqEuBzAyo5C4P8Vqr