Skip to content

Commit 3ac34e4

Browse files
cliffhallclaude
andauthored
docs: post-swap branch model, PR policy, milestone rule, and Copilot review instructions (#1866)
* docs: correct post-swap branch model, PR policy, and stale gate/tree entries Part of the go-live phase-8 docs hygiene: everything that silently inverted when `main` became v2. Branch/release model — documented canonically in AGENTS.md and corrected everywhere it was restated: - AGENTS.md: replace the one-line "Base Branches" bullet with a table giving each branch its role (v2/main = develop, main = release, v1/main = maintenance), whether PRs target it, and which npm dist-tag it publishes. - README.md: the "Repo status" callout had it backwards, claiming `main` was the legacy v1 implementation. Also note that a v2 release is cut from `main` after the milestone merge, not from v2/main. - CONTRIBUTORS.md: the version table listed v2's base branch as `main`. PR policy: - Every PR must reference an issue, from anyone, no exceptions. - PRs are opened by repo maintainers only — org write access is not authorization. Everyone else files a detailed issue. - Branch names start with the target version segment (v2/…, v1/…). - Issues and PRs carry exactly one of `v1`/`v2` at creation; default `v2`. - Screenshots proving UI/TUI changes go in the gitignored pr-screenshots/. Corrections to stale content found while in there: - The TUI coverage bullet still described the Ink/App.tsx/hooks exclusion as interim pending #1501, which closed 2026-06-29; the gate now covers all of src/** with only tui-servers.ts excluded. - The completion checklist said `npm run validate` was the gate and covered "e2e tests", contradicting the mandatory-pre-push-gate section (npm run ci). - v1 PR base was given as `main`; it is `v1/main`. - Project tree omitted core/client, docs/, and scripts/ (README: core/client, docs/). core/client is under the coverage gate and imported by both App.tsx trees. Also adds the launcher and `npm run test:scripts` to the "run the tests" list and a tiers overview, since neither was mentioned. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU * docs: require a milestone on every new issue, defaulting to the current one Adds the rule alongside the version-label rule, since they fail the same way: an unmilestoned issue drops out of release planning as silently as an unlabeled one drops out of version filtering. - Set it at create time (`gh issue create --milestone <title> ...`). - If the user didn't specify one, default to the current milestone — the open milestone with the nearest due date — rather than leaving it blank pending a decision. Includes the `gh api .../milestones` one-liner that identifies it. - Milestones are release buckets, so pick by when the work ships, not by size; sub-issues normally inherit their parent's milestone. Also widens the section's opening callout: an issue is not "created" until it is labeled, milestoned, AND boarded with a Status — four distinct steps, not three. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU * docs: add .github/copilot-instructions.md and require mirroring into it GitHub Copilot reads .github/copilot-instructions.md when reviewing a PR. The repo had no such file, so Copilot reviewed without any of the conventions that actually govern this codebase — the ban on `any` and on unjustified double casts, the Mantine-first styling rules, the `.withProps()` extraction rule, the lib/utils split, test placement, and the per-file >=90% four-dimension gate. The new file is a review-focused distillation of AGENTS.md: the rules a reviewer would cite against a diff, plus a short "what to prioritize" list that puts correctness and security first (this backend spawns processes and proxies outbound requests). Deliberately omitted: board recipes and IDs, milestone and branch mechanics, release procedure, and the project tree — no reviewer cites those, and copying them would double the maintenance surface. AGENTS.md's "Keep documentation files up to date" section now requires mirroring review-relevant changes into it in the same PR, states that AGENTS.md stays the source of truth, and spells out what counts as review-relevant. There is no generation step and nothing detects drift, so the rule is the only thing keeping the two in sync. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU * docs: address Copilot review — v1 policy wording, stale V2 Go Live, typo - Align the v1 maintenance policy on "security and bug fixes only" in AGENTS.md and CONTRIBUTORS.md; both said "security fixes only" while the README callout, the branch table, and the board description said "security and bug fixes". - Drop the stale "or V2 Go Live" instruction from the work-begins step — that column no longer exists on board #28 (verified via `gh project field-list 28`), so the option id it implied would be rejected. Restore the removed-column id list the same edit dropped, now including V2 Go Live, and re-date the table to today's verification. - Fix "contains the the new version" typo. - Strip a stray trailing blank line at the end of AGENTS.md. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU * docs: restore `inspector/` as the Project Structure tree root The tree is a filesystem layout — every child is a real directory (`clients/`, `core/`, `test-servers/`). Labeling its root `v2/main/` named a branch instead, which reads as a path that does not exist in a checkout and diverged from the identical tree in README.md, which still says `inspector/`. The branch model is documented in its own table under Repository & Project Boards; the tree does not need to restate it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU * docs: settle the v1 policy wording as "security fixes only" Reverses the direction the Copilot round took in 33cc036. That commit normalized the six v1-policy sites onto "security and bug fixes" because that was the majority wording — but majority is the wrong tiebreak here. The policy is set by what we told contributors: the #1819 backlog close note, posted to 120 closed PRs, says "v1 will receive security fixes only", and the five PRs retained for 1.0.5 are all security fixes. So the two sites that already said "security fixes only" were the correct ones, and the four that said otherwise are now aligned to them: - AGENTS.md: the `v1` label description, the branch-role table, and the v1 board line. - README.md: the "Repo status" callout. CONTRIBUTORS.md and AGENTS.md's Project Status bullet already read correctly and are unchanged. This is the substance of #1813, which is why that issue's PR is now reduced to its PR-template-link half — the wording work landed here to avoid two PRs editing the same lines in opposite directions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU * docs: point the AGENTS.md policy link at CONTRIBUTING.md This PR rewrites the paragraph holding the only in-repo link to the contributing policy, so it owns that line. #1884 renames the file CONTRIBUTORS.md -> CONTRIBUTING.md; having #1884 also edit this line would guarantee a one-line conflict between the two. Writing the new filename here instead makes #1884 a pure `git mv` with no overlapping line, so neither PR conflicts with the other. Between this merging and #1884 merging, the link points at a filename that doesn't exist yet. That window is deliberate and bounded — this PR is first in the merge order (#1821) and #1884 is third. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 5d91d4a commit 3ac34e4

4 files changed

Lines changed: 221 additions & 50 deletions

File tree

‎.github/copilot-instructions.md‎

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,98 @@
1+
# Copilot review instructions — MCP Inspector
2+
3+
> **`AGENTS.md` is the source of truth.** This file is a review-focused distillation of it, kept in sync by hand — see "Keep documentation files up to date" in `AGENTS.md`. Where the two disagree, `AGENTS.md` wins, and the drift is a bug worth flagging in review.
4+
5+
The Inspector ships as one package with three clients (**Web**, **CLI**, **TUI**) over a shared `core/`, consumed via the `@inspector/core` build-time alias. v2 is **not** an npm workspace: the root and each `clients/*` keep their own `package.json` and `node_modules`.
6+
7+
## TypeScript
8+
9+
- **Never use `any`.** Not in types, not in casts, not in generics.
10+
- **Never suppress errors to satisfy the linter or compiler** — no disabling `no-unused-vars` / `no-explicit-any` in config, and no `// @ts-nocheck` or `// @ts-ignore` (`@typescript-eslint/ban-ts-comment` rejects these across every surface).
11+
- **Avoid double casts (`as unknown as T`).** They erase all type safety and usually mean the real type is being worked around. Prefer a type guard, a narrower single cast, or fixing the underlying type. If genuinely unavoidable (a documented gap in a third-party type, or bridging structurally-identical shapes TS can't relate), it **must** carry an inline comment justifying why it's safe and why nothing better exists. An unjustified `as unknown as` is not acceptable in review.
12+
- Prefer inference, type guards, and precise annotations over assertions.
13+
- An `_` prefix is the intentionally-unused marker (`argsIgnorePattern` / `varsIgnorePattern` / `caughtErrorsIgnorePattern`).
14+
15+
## React and UI (web client)
16+
17+
The web client is built from **presentational ("dumb") components** — they take data and callbacks as props and hold display logic only. No data fetching or client state inside them; state comes from the `@inspector/core` hooks wired near the top of the tree. A component that reaches for a store or fetches directly is a review finding.
18+
19+
Styling is **Mantine-first**, in this strict order of preference: component props → theme variants → CSS classes (last resort).
20+
21+
- **Never use inline styles.**
22+
- **Never use raw color literals** — no hex (`#ddd`), no `rgba()`. Use the `--inspector-*` CSS custom properties from `App.css :root` (e.g. `c: 'var(--inspector-text-primary)'`). If no token fits, add one to `:root` first.
23+
- **Avoid `div` and bare HTML for layout.** Use Mantine `Box`, `Group`, `Stack`, `Flex`, `Paper`.
24+
- **Never add a CSS class when the styles can be component props or a theme variant.** Flat CSS properties (margin, padding, background, border, color, font-size) belong in the theme (`src/theme/<Component>.ts`, via `Component.extend()`). `App.css` may contain **only** what the theme cannot express: `@keyframes`, pseudo-selectors (`:hover`, `:focus`), cross-component hover relationships, nested child selectors for third-party HTML output, and styles for native elements (`img`, `iframe`).
25+
- When a theme variant needs a class for nested/pseudo selectors, assign it via `classNames` in the theme extension — never a manual `className` in JSX for theme-styled components.
26+
27+
### The `.withProps()` rule
28+
29+
**Declare a named subcomponent constant via `.withProps()` whenever an inline Mantine element carries two or more _static_ props.** This applies to single-use elements too — "it's only used once" is not an exemption.
30+
31+
- **Static** = a literal configuring **styling, layout, or behavior**: `size="sm"`, `c="dimmed"`, `fw={500}`, `gap="xs"`, `justify="space-between"`, `variant="light"`, `withBorder`, `readOnly`, `striped`.
32+
- **Not counted:** dynamic props (`value`, `on*`, `children`, `key`, `ref`, anything whose value is a variable) — pass these at the call site; and per-instance **content/accessibility** literals (`label`, `description`, `placeholder`, `title`, `aria-label`, `role`) — these never by themselves trigger extraction.
33+
34+
```tsx
35+
const CardContent = Group.withProps({
36+
flex: 1,
37+
align: "flex-start",
38+
justify: "space-between",
39+
wrap: "nowrap",
40+
});
41+
```
42+
43+
**Legitimate exceptions** (each stays inline, with a one-line comment saying why):
44+
45+
- **`Box`** — does not support `.withProps()`. Use `Group`/`Stack`/`Flex`/`Text`/`Paper`/`UnstyledButton`/`Image` instead, chosen by purpose. A `Box` that genuinely needs a non-flex primitive (`component="iframe"`, `display="grid"`) stays inline.
46+
- **`Accordion`** — a compound, `multiple`-discriminated generic; `.withProps()` loses its JSX call signature and fails to type.
47+
- **Headless, non-`factory()` components** such as `Transition` — no Styles API, so no `.withProps` static at all.
48+
- **`data-*` attributes** — not part of a component's typed props, so excess-property-checked out of a `withProps` literal. Pass at the call site.
49+
- **Anything that isn't a Mantine factory component** — a `react-icons` glyph, another library's component, or a first-party plain `export function`.
50+
51+
### Theme files vs. element components
52+
53+
Both exist and do different jobs. Theme files (`src/theme/<Component>.ts`) customize a Mantine primitive **app-wide**. Element components (`src/components/elements/`) add **domain semantics** on top of primitives.
54+
55+
- Element components import from `@mantine/core`, **not** from `src/theme/` — the theme layer is applied transparently by the provider.
56+
- **Never push domain-specific variant logic into theme files** (annotation types, transport types, …). Domain variants belong to the element component that owns those semantics.
57+
58+
## Where code goes (web client)
59+
60+
**`utils` = functions that compute; `lib` = things that instantiate, adapt, or touch the environment.** If it does I/O or wraps a subsystem it's `lib`; if it's a pure transform it's `utils`.
61+
62+
- `src/utils/` — pure, side-effect-free. Also: pure shared domain types and their constructors; diagnostic `console.warn`/`error` does **not** count as a side effect; type-only imports from `@inspector/core`, and re-exporting pure functions/constants from core, are both fine.
63+
- `src/lib/` — infrastructure, integration, stateful adapters: composes subsystems, wraps the core **runtime**, touches DOM / `window` / `sessionStorage`, or produces side effects.
64+
- Cross-directory imports go **one way: `lib → utils`**, never the reverse.
65+
- `src/types/` is only for ambient `.d.ts` module stubs — not a home for new domain types.
66+
- ⚠️ The coverage `include` is a **whitelist** naming `components` / `hooks` / `theme` / `lib` / `utils` / `server` (plus the `core/*` runtime). A module placed outside those directories silently falls out of the ≥90% gate. Flag new top-level files or new grab-bag directories.
67+
68+
## Tests and the coverage gate
69+
70+
- **All new or modified code needs tests.** The per-file gate is **≥ 90% on all four dimensions** — lines, statements, functions, **and branches** — enforced in CI for `clients/{web,cli,tui,launcher}` and the gated `core/` runtime.
71+
- **Never lower the gate** to accommodate an unreachable branch. Annotate at the source with a justified `/* v8 ignore … -- <reason> */`. Acceptable reasons: happy-dom-inherent paths (Mantine portal mounts, `useMediaQuery` fallbacks, `typeof window` SSR guards), React StrictMode effect-replay blocks, and provably-dead defensive guards. Anything else is a missing test.
72+
- **Suppress expected error output** from the console in tests that exercise error paths.
73+
74+
### Test placement
75+
76+
- **Web:** side-by-side by default — `<Name>.test.tsx` next to the source. A web-owned test under `src/test/` instead of beside its source is a bug. `src/test/` is only for what can't be co-located: tests of the repo-root `core/` package (`src/test/core/…`, mirroring core's layout), the `integration` project (`src/test/integration/…` — placement _is_ the manifest), and shared test infrastructure.
77+
- **CLI / TUI / launcher:** **all** tests live in a top-level `__tests__/` directory. A co-located `src/**/*.test.*` lands in no tsconfig project and fails `verify:typecheck-coverage`.
78+
79+
### Rendering components in tests
80+
81+
- **Always render through `renderWithMantine`** from `src/test/renderWithMantine.tsx`. Never hand-roll a bare `MantineProvider` — that reintroduces a real failure class where a `Transition`/`Modal` timer fires after happy-dom tears down `window` and fails the entire run.
82+
- For a forced color scheme, pass the option — `renderWithMantine(ui, { colorScheme: "dark" })` — rather than a hand-rolled `defaultColorScheme` provider.
83+
- Only when asserting _mid-flight_ transition state, use `renderWithMantineTransitions`, passing `settleMs` derived from the component's real animation duration. Do **not** combine it with `vi.useFakeTimers()`, and use the `unmount()` it returns if the test unmounts the tree itself.
84+
85+
## Gates and PR hygiene
86+
87+
- `npm run format` before committing; **`npm run ci` before pushing** (`validate` → `coverage` → `verify:build-gate` → `smoke` → Storybook). `npm run validate` is the fast inner-loop check and is **not** a substitute — it runs `test`, not `test:coverage`, so it does zero coverage gating.
88+
- **Every PR references an issue**, first body line `Closes #<ISSUE_NUMBER>`.
89+
- **Every PR carries exactly one version label**, `v1` or `v2`, matching its base branch.
90+
- Update the relevant `README.md` / `AGENTS.md` when a change adds, removes, renames, or repurposes a file or folder, changes the structure or tech stack, or introduces a command, dependency, or architectural pattern.
91+
92+
## What to prioritize in review
93+
94+
1. Correctness and security — this backend spawns local processes and proxies outbound requests, so anything touching auth, origin validation, host binding, or the proxy's SSRF controls deserves close reading.
95+
2. Type-safety violations (`any`, suppressions, unjustified double casts).
96+
3. Missing or thin tests against the ≥90% four-dimension gate, and modules placed outside the gated directories.
97+
4. Mantine convention violations — inline styles, raw colors, unnecessary CSS classes, missing `.withProps()` extraction.
98+
5. Docs that contradict the change.

0 commit comments

Comments
 (0)