diff --git a/AGENTS.md b/AGENTS.md index 7d51e61f9..219530ba6 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -37,7 +37,7 @@ Two complementary guards, and you need both: scans `src/` for every load-time form (`import from`, `export ... from`, bare `import`, `require`) of an enforced optional peer, and fails `jest` if one appears. `await import()` is deliberately not matched - deferring resolution is the fix, not the defect. That file is also the source of truth for -*which* optional peers are enforced: `react` / `react-dom` are marked optional in `package.json` but +_which_ optional peers are enforced: `react` / `react-dom` are marked optional in `package.json` but exempted there on the record, because every component imports React at module scope and always will. **2. Built artifact, by hand.** The CI guard reads source, so it cannot speak for what the bundler @@ -79,11 +79,28 @@ of examples there, e.g. `FeedbackButton`, `NavigationItem`). Verify by building ## Repo state that looks broken but is pre-existing -`prettier --check` fails on ~82 files and `tsc --noEmit` reports errors across `src/` (including -missing `@types/jest` wiring for `src/__testing__`). Neither is a CI gate - CI runs +`prettier --check` fails on dozens of files and `tsc --noEmit` reports errors across `src/` +(including missing `@types/jest` wiring for `src/__testing__`). Neither is a CI gate - CI runs `.github/workflows/node-checks.yml` (lint + build) and `jest`. Do not assume you caused these; do not mass-reformat to "fix" them. +A type contract can still be gated, just not by a type-only assertion file: jest transforms with +`@swc/jest`, which strips types without checking them, so such a file passes no matter what it +asserts. Shell out to `tsc` over a scoped fixture, assert the fixture is in the compiled program +(`--listFiles`) before trusting an empty diagnostic list, then filter the diagnostics to that +fixture - [`src/__testing__/navigationItemTitleTypes.test.ts`](src/__testing__/navigationItemTitleTypes.test.ts) +is the worked pattern, including the checks that keep the filter from turning the guard vacuous. + +## Every commit needs a sign-off matching its own author + +DCO is a blocking required check with nothing enforcing it locally, and the trailer must match that +commit's own author identity. The rule, the enforcement, and the repair recipe live in +[`CONTRIBUTING.md`](CONTRIBUTING.md#commit-signing) - read it before rewriting history. + +The agent-specific trap: the usual offender is a follow-up commit (review fix, doc update, rebase +fixup), not the first one. Sign every commit you add, including the ones an automated pass makes +for you. + ## Maintaining this file Keep this file for knowledge useful to almost every future agent session in this project. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 51ca9ed51..03f6898e8 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -40,6 +40,11 @@ address (sorry, no pseudonyms or anonymous contributions). An example of signing $ commit -s -m “my commit message w/signoff” ``` +The trailer must match that commit's **own author identity**. `-s` copies your current +`user.name` / `user.email` into the trailer, so a commit authored under a different identity - a +repo-local override, a second machine, a co-author's commit you replayed - still fails the check +even though a `Signed-off-by:` line is present. + To ensure all your commits are signed, you may choose to add this alias to your global `.gitconfig`: _~/.gitconfig_ @@ -55,6 +60,23 @@ Or you may configure your IDE, for example, Visual Studio Code to automatically +### Repairing a failed DCO check + +DCO is a blocking required check, enforced by the [probot/dco](https://github.com/probot/dco) +GitHub App rather than by a workflow - there is no file for it under `.github/`, so it is invisible +from the repository tree until a pull request fails. `.husky/commit-msg` is deliberately empty +(commit `612608be`), so nothing catches a missing or mismatched sign-off locally either. + +To repair, rebase only the commits you authored, then force-push: + +``` +git rebase --signoff +git push --force-with-lease +``` + +Pick that starting point deliberately. `--signoff` adds your trailer to every commit it replays, so +rebasing the whole branch grafts your sign-off onto a co-author's commit as a second trailer. + ## Documentation Contribution Flow Please contribute! Layer5 documentation uses Jekyll and GitHub Pages to host docs sites. Learn more about [Layer5's documentation framework](https://docs.google.com/document/d/17guuaxb0xsfutBCzyj2CT6OZiFnMu9w4PzoILXhRXSo/edit?usp=sharing). The process of contributing follows this flow: @@ -155,10 +177,10 @@ make lint `src/custom/ResponsiveDataTable.tsx` wraps `@mui/x-data-grid` (via `@layer5/mui-datatables`) with responsive column visibility and a standardised row action menu. Key exports: -| Export | Description | -|---|---| -| `ResponsiveDataTable` | The main table component | -| `TableAction` | Type for items in the per-row action menu | +| Export | Description | +| ----------------------- | ---------------------------------------------- | +| `ResponsiveDataTable` | The main table component | +| `TableAction` | Type for items in the per-row action menu | | `getCopyDeepLinkAction` | Helper that builds a "Copy link" `TableAction` | **`getCopyDeepLinkAction`** @@ -171,7 +193,7 @@ import { getCopyDeepLinkAction, TableAction } from '@sistent/sistent'; const rowActions: TableAction[] = [ getCopyDeepLinkAction(() => copyRowDeepLink(row.id)), // or, with a custom title: - getCopyDeepLinkAction(() => copyRowDeepLink(row.id), t('Copy link')), + getCopyDeepLinkAction(() => copyRowDeepLink(row.id), t('Copy link')) ]; ``` diff --git a/src/__testing__/fixtures/navigationItemTitle.tsx b/src/__testing__/fixtures/navigationItemTitle.tsx new file mode 100644 index 000000000..fa9378ba7 --- /dev/null +++ b/src/__testing__/fixtures/navigationItemTitle.tsx @@ -0,0 +1,54 @@ +// Type-level fixture for `NavigationItem['title']`. It is compiled by +// `navigationItemTitleTypes.test.ts` via `tsc --noEmit`, never executed, and is +// deliberately outside jest's `*.test.*` glob so jest does not collect it. +// +// `NavigationNavbar` renders `title` into MUI's `ListItemText` `primary` slot, +// which is typed `React.ReactNode`. Declaring `title` as `string` therefore +// rejected composed labels that already rendered correctly - see +// https://github.com/layer5io/sistent/issues/1746. +import OpenInNewIcon from '@mui/icons-material/OpenInNew'; +import type { NavigationItem } from '../../custom/NavigationNavbar'; + +// A plain string still compiles - the widening is source-compatible. +export const plainStringTitle: NavigationItem = { + id: 'settings', + title: 'Settings', + onClick: () => {} +}; + +// A composed ReactNode title compiles: the exact shape that previously forced +// consumers into an `as unknown as string` assertion. +export const composedNodeTitle: NavigationItem = { + id: 'api-docs', + title: ( + + API Docs + + + ), + onClick: () => {} +}; + +// Sub-items share the type, so they gain the same latitude. +export const composedSubItemTitle: NavigationItem = { + id: 'docs', + title: 'Docs', + onClick: () => {}, + subItems: [ + { + id: 'docs-api', + title: API, + onClick: () => {} + } + ] +}; + +// Self-check: if the compiler ever stops type-checking this fixture, the +// suppression below goes unused and tsc reports TS2578 here, failing the test +// rather than letting it pass vacuously. A plain object is not a `ReactNode`. +export const rejectsNonNode: NavigationItem = { + id: 'bad', + // @ts-expect-error - a plain object is not assignable to React.ReactNode + title: { label: 'not a node' }, + onClick: () => {} +}; diff --git a/src/__testing__/fixtures/tsconfig.navigationItemTitle.json b/src/__testing__/fixtures/tsconfig.navigationItemTitle.json new file mode 100644 index 000000000..e84a46afd --- /dev/null +++ b/src/__testing__/fixtures/tsconfig.navigationItemTitle.json @@ -0,0 +1,7 @@ +{ + "extends": "../../../tsconfig.json", + "compilerOptions": { + "noEmit": true + }, + "include": ["./navigationItemTitle.tsx"] +} diff --git a/src/__testing__/navigationItemTitleTypes.test.ts b/src/__testing__/navigationItemTitleTypes.test.ts new file mode 100644 index 000000000..416d298b9 --- /dev/null +++ b/src/__testing__/navigationItemTitleTypes.test.ts @@ -0,0 +1,125 @@ +import { spawnSync } from 'child_process'; +import path from 'path'; + +// Type-level regression guard for https://github.com/layer5io/sistent/issues/1746. +// +// `NavigationNavbar` renders `NavigationItem['title']` into MUI's `ListItemText` +// `primary` slot, which is typed `React.ReactNode`. While `title` was declared +// `string`, composed labels rendered correctly but did not type-check, and +// consumers reached for `as unknown as string`. +// +// jest transforms with @swc/jest, which strips types without checking them, so a +// plain type-only assertion file would pass no matter what `title` is declared +// as. This test therefore shells out to `tsc` over a fixture that exercises both +// a plain string and a composed `ReactNode` title. +// +// Diagnostics are filtered to the fixture itself: type-checking it pulls in the +// component's transitive dependencies, which carry pre-existing errors unrelated +// to this contract (see CLAUDE.md, "Repo state that looks broken but is +// pre-existing"). The fixture's own `@ts-expect-error` self-check fails loudly if +// the compiler ever stops checking it, so filtering cannot make this vacuous. +// +// Everything below the filter exists so that the guard fails closed. Filtering +// is the one place a compiler check can quietly turn into a no-op: every outcome +// that is not "tsc ran, compiled the fixture, and reported on it" has to be made +// to fail rather than collapse into an empty, green list of diagnostics. + +const FIXTURE = 'src/__testing__/fixtures/navigationItemTitle.tsx'; +const PROJECT = 'src/__testing__/fixtures/tsconfig.navigationItemTitle.json'; +const repoRoot = path.resolve(__dirname, '..', '..'); + +// `(,): error TS….` The file part is what scopes a diagnostic, +// so it is parsed rather than prefix-matched: tsc prints paths relative to its +// cwd only while the fixture stays under it, and a bare `startsWith` would read +// any other emission as "the fixture is clean". +const FILE_SCOPED_DIAGNOSTIC = /^(.+?)\(\d+,\d+\): error TS\d+/; + +// `error TS…` with no file part is a config-level failure - TS18003 "No inputs +// were found in config file", TS5083 "Cannot read file" - and means the fixture +// was never compiled at all. The fixture filter drops these on the floor, and +// the fixture's `@ts-expect-error` self-check cannot see them either, because +// neither fires unless the compiler had the fixture in its program. +const CONFIG_SCOPED_DIAGNOSTIC = /^error TS\d+/; + +// Absence of diagnostics only means "nothing was wrong with what tsc compiled" - +// it says nothing about *what* tsc compiled. A project whose `include` resolves +// to some other existing file emits no diagnostic at all, so both lists above +// come back empty and the guard passes without ever having looked at the +// fixture. `--listFiles` closes that by making the program's contents an +// assertable fact instead of an assumption. +const asPosix = (value: string): string => value.replace(/\\/g, '/'); + +type Diagnostics = { fixture: string[]; config: string[]; compiledFixture: boolean }; + +// Below the 180s `beforeAll` budget. `spawnSync` blocks the jest worker outright, +// and jest's own timeout cannot preempt a synchronous child, so a wedged tsc +// would otherwise hold the worker until the whole run is killed. Node's timeout +// signals the child, which surfaces as the `status === null` throw below. +const TSC_TIMEOUT_MS = 150_000; + +const typeCheckFixture = (): Diagnostics => { + // Spawned through `process.execPath` rather than `npx`: no shell, no PATH + // lookup, and no `npx` vs `npx.cmd` divergence on Windows, where an ENOENT + // would otherwise be caught and laundered into the diagnostic output. + const tsc = spawnSync( + process.execPath, + [require.resolve('typescript/bin/tsc'), '-p', PROJECT, '--pretty', 'false', '--listFiles'], + { + cwd: repoRoot, + encoding: 'utf8', + timeout: TSC_TIMEOUT_MS, + // tsc reports on every file in the program, and `--listFiles` prints each + // one, so this runs to several MB. The 1 MB default would kill the child + // mid-stream and truncate stdout - and the fixture is a root file, so its + // diagnostics are emitted last and lost first. + maxBuffer: 32 * 1024 * 1024 + } + ); + + // A compiler that never ran is not a compiler that found nothing. + if (tsc.error) throw tsc.error; + if (tsc.status === null) { + throw new Error(`tsc was killed by ${tsc.signal ?? 'an unknown signal'} before reporting`); + } + + // tsc exits non-zero whenever any file in the program has an error, including + // the pre-existing ones outside the fixture, so the exit code is not the + // verdict here - the diagnostics are. + const lines = `${tsc.stdout}\n${tsc.stderr}`.split('\n').map((line) => line.trim()); + + const belongsToFixture = (line: string): boolean => { + const file = FILE_SCOPED_DIAGNOSTIC.exec(line)?.[1]; + return file !== undefined && asPosix(file).endsWith(FIXTURE); + }; + + return { + fixture: lines.filter(belongsToFixture), + config: lines.filter((line) => CONFIG_SCOPED_DIAGNOSTIC.test(line)), + // `--listFiles` prints one absolute path per program file, diagnostics aside. + compiledFixture: lines.some( + (line) => !FILE_SCOPED_DIAGNOSTIC.test(line) && asPosix(line).endsWith(FIXTURE) + ) + }; +}; + +describe('NavigationItem title type contract', () => { + let diagnostics: Diagnostics; + + beforeAll(() => { + diagnostics = typeCheckFixture(); + }, 180_000); + + // Ordered deliberately: the two assertions below are only meaningful once the + // fixture is known to have been compiled, so prove that first. + it('compiles the fixture', () => { + expect(diagnostics.compiledFixture).toBe(true); + }); + + it('reports no config-level failure that would skip the fixture', () => { + expect(diagnostics.config).toEqual([]); + }); + + it('accepts both a plain string and a composed ReactNode title', () => { + expect(diagnostics.fixture).toEqual([]); + }); +}); diff --git a/src/custom/NavigationNavbar/navigationNavbar.tsx b/src/custom/NavigationNavbar/navigationNavbar.tsx index 2bb024c4a..6d59537d4 100644 --- a/src/custom/NavigationNavbar/navigationNavbar.tsx +++ b/src/custom/NavigationNavbar/navigationNavbar.tsx @@ -9,7 +9,14 @@ import { IconWrapper, MenuItemList, MenuItemSubList, MenuListStyle, SubIconWrapp export type NavigationItem = { id: string; - title: string; + /** + * Label rendered into `ListItemText`'s `primary` slot. + * + * Accepts any `React.ReactNode`, not just a string, so composed labels - + * a label plus a trailing external-link glyph, a `` badge, a count - + * type-check as well as they already render. + */ + title: React.ReactNode; icon?: React.ReactNode; /** * Legacy boolean permission flag.