chore(scripts): cover PowerShell's Env: drive, and correct two stale scope summaries (#2146)
Copilot, eleventh review: - `Set-Item Env:SMOKE_BROWSER firefox` — PowerShell's Env: DRIVE, the other standard way to write the environment there — matched none of the patterns. Covered now, positional or via -Path/-Value, for Set-Item/New-Item and their -ItemProperty forms. A `Get-Item` READ is still not a finding, which has its own negative case: reading the variable is not setting it, the same distinction as round 5's `echo "SMOKE_BROWSER: firefox"`. - Two scope summaries had drifted behind the traversal, both saying the guard reads only `run` and `env`/`with`: the module header's "ONLY EXECUTABLE POSITIONS" paragraph, and the copilot-instructions mirror. Both list the real set now — `run`, the `shell` command template at all three levels, and `env`/`container.env`/`with`. The mirror one is the AGENTS.md rule this repo states about itself, so it is a rule violation rather than a typo. Signed-off-by: Cliff Hall <cliff@futurescale.com> Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
2ead041572
commit
0c0d5faac6
3 changed files with 47 additions and 6 deletions
2
.github/copilot-instructions.md
vendored
2
.github/copilot-instructions.md
vendored
|
|
@ -102,7 +102,7 @@ Both exist and do different jobs. Theme files (`src/theme/<Component>.ts`) custo
|
|||
|
||||
## Gates and PR hygiene
|
||||
|
||||
- `npm run format` before committing; **`npm run local:gate` before pushing** (`validate` → `coverage` → `verify:build-gate` → `verify:bundle-externals` → `smoke` → `smoke:web:firefox` → `local:storybook`; the Firefox step is local-only and not mirrored in GitHub CI). The gate is named `local:` rather than `ci` on purpose and has **no alias** (#2146) — `npm run ci` is gone, and a workflow that invokes a `local:*` script, a non-Chromium engine pass, `smoke:web:engine`, or sets `SMOKE_BROWSER` to a non-Chromium value fails `test:scripts` via `scripts/lib/workflow-gate.mjs` — as does naming either family through a workflow expression (`smoke:web:${{ matrix.browser }}`). It scans only `run:` scalars and `env:`/`with:` values, so metadata is not a finding. Flag any diff that adds one of those to `.github/workflows/**`, or that reintroduces a `ci`/`ci:*` root script. `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. The canonical CI-vs-local table is in the root README ("Two tiers: GitHub CI and the local gate").
|
||||
- `npm run format` before committing; **`npm run local:gate` before pushing** (`validate` → `coverage` → `verify:build-gate` → `verify:bundle-externals` → `smoke` → `smoke:web:firefox` → `local:storybook`; the Firefox step is local-only and not mirrored in GitHub CI). The gate is named `local:` rather than `ci` on purpose and has **no alias** (#2146) — `npm run ci` is gone, and a workflow that invokes a `local:*` script, a non-Chromium engine pass, `smoke:web:engine`, or sets `SMOKE_BROWSER` to a non-Chromium value fails `test:scripts` via `scripts/lib/workflow-gate.mjs` — as does naming either family through a workflow expression (`smoke:web:${{ matrix.browser }}`). It parses the workflow and scans only the executable paths — `run:` scalars, a custom `shell:` command template (step, and `defaults.run.shell` at workflow/job level, since Actions runs it *around* the script), and `env:` / `container.env:` / `with:` values, with YAML aliases resolved — so metadata is not a finding. Flag any diff that adds one of those to `.github/workflows/**`, or that reintroduces a `ci`/`ci:*` root script. `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. The canonical CI-vs-local table is in the root README ("Two tiers: GitHub CI and the local gate").
|
||||
- **A dependency bump must land in every install that declares it.** v2 is not a workspace — the root and each `clients/*` have their own `node_modules`, and a client's test project compiles `core/` and `test-servers/src` (which resolve from the **root**) alongside the client's own sources. Bumping a shared dependency in one manifest only puts two versions of it in one `tsc` program; for a recursive-generic surface like zod that exhausts the tsc heap (#1896). `verify:dep-lockstep` fails the build on this — it derives its candidates from what each `tsc` program actually resolves (`tsc --listFilesOnly`, keeping packages that reach one program from two installs), so a package reached only through another package's `.d.ts` counts too (#1965) — so a PR bumping a package the shared sources pull in should update the root **and every client that already lists it** — not every client unconditionally, since a package absent from an install can't skew and adding it there would be a spurious dependency.
|
||||
- **A test or smoke must not touch real user state.** The web smokes run against a throwaway catalog via the shared `scripts/lib/prod-web-server.mjs` helper, never the developer's `~/.mcp-inspector/mcp.json` (#1977); the cli/tui smokes drive a temp `--catalog`; `pack:verify`'s `--web` child sets its own `MCP_CATALOG_PATH` for the same reason (#2003 — its App deep link persists a server row). Anything that boots the web backend and then *navigates* it needs that isolation, not just the scripts named `smoke:*`. A new smoke spawning its own server, or teardown that removes a work dir without first awaiting `stopChild` (the #1801 race — `child-cleanup.mjs` exports both halves and both are required), should be flagged.
|
||||
- **A smoke must assert the thing survives, not that it started.** `smoke:tui` runs the Ink TUI under a pseudoterminal (`scripts/lib/pty.mjs`) and requires it to still be running `SMOKE_TUI_SURVIVE_MS` after its first frame (`scripts/lib/render-smoke.mjs`) — because it previously settled OK on first paint while the TUI was exiting 1 ~40ms later with `Raw mode is not supported`, and won that race on every machine (#2147). Flag anything that collapses the survival wait back into "resolve on the marker", and any new smoke whose only assertion is that a process printed something once. A smoke walks only its own happy path, so the branch that matters — started, then died — belongs in a `scripts/lib/*.test.mjs` driven against a stub (as with `announced-child.mjs` #2000 and `ensure-test-servers.mjs` #2111), not in the smoke.
|
||||
|
|
|
|||
|
|
@ -55,9 +55,10 @@
|
|||
* Those belong in CI and are there today — `smoke:tui` self-skips under
|
||||
* `process.env.CI` on its own, so it needs no guarding.
|
||||
*
|
||||
* ONLY EXECUTABLE POSITIONS ARE SCANNED — `run:` scalars (inline and block),
|
||||
* and the values of an `env:` or `with:` mapping, found by parsing the file
|
||||
* rather than by matching lines. Everything else in a workflow is metadata that
|
||||
* ONLY EXECUTABLE POSITIONS ARE SCANNED, found by parsing the file rather than
|
||||
* by matching lines: `run:` scalars (inline and block), a custom `shell:`
|
||||
* command template (step-level, and `defaults.run.shell` at workflow and job
|
||||
* level), and the values of an `env:`, `container.env:` or `with:` mapping. Everything else in a workflow is metadata that
|
||||
* runs nothing, so scanning it produces findings that are simply false:
|
||||
* `name: Explain why smoke:web:firefox stays local` executes nothing, and
|
||||
* failing the suite on it would contradict the "what it forbids, and nothing
|
||||
|
|
@ -67,8 +68,8 @@
|
|||
* the wrong thing.
|
||||
*
|
||||
* That is a real limit, stated rather than hidden: an execution vector outside
|
||||
* those three positions — a custom action that reads a script name from
|
||||
* somewhere else, say — is not seen. The alternative, scanning everything, was
|
||||
* those positions — a custom action that reads a script name from somewhere
|
||||
* else, say — is not seen. The alternative, scanning everything, was
|
||||
* measured against this repo's own workflow and produces false positives on
|
||||
* ordinary step names, which is the faster way to get a guard deleted.
|
||||
*/
|
||||
|
|
@ -184,6 +185,16 @@ const BROWSER_ASSIGNMENTS = [
|
|||
),
|
||||
value: 1,
|
||||
},
|
||||
// PowerShell's Env: DRIVE, the other standard way to write the environment
|
||||
// there: `Set-Item Env:SMOKE_BROWSER firefox`, or with `-Path`/`-Value`
|
||||
// (Copilot). The value may be positional or named.
|
||||
{
|
||||
re: new RegExp(
|
||||
String.raw`\b(?:Set|New)-Item(?:Property)?\b[^\n]*?\bEnv:${BROWSER_ENV_VAR}\b\s*(?:-Value\s+)?${ASSIGNED_VALUE}`,
|
||||
"gi",
|
||||
),
|
||||
value: 1,
|
||||
},
|
||||
// GitHub's MULTILINE environment-file syntax: `SMOKE_BROWSER<<EOF`, the value
|
||||
// on following lines, then the delimiter. Reported as unreadable rather than
|
||||
// parsed (Copilot): the value can arrive through any number of shapes — a
|
||||
|
|
|
|||
|
|
@ -348,6 +348,36 @@ describe("findWorkflowViolations", () => {
|
|||
text: workflow(" - shell: pwsh\n run: npm run smoke"),
|
||||
rules: [],
|
||||
},
|
||||
{
|
||||
// Copilot, eleventh review: PowerShell's Env: drive is the other standard
|
||||
// way to write the environment there.
|
||||
name: "flags a Set-Item write to the Env: drive",
|
||||
text: workflow(
|
||||
" - run: Set-Item Env:SMOKE_BROWSER firefox; npm run smoke",
|
||||
),
|
||||
rules: [VIOLATION.BROWSER_ENV],
|
||||
},
|
||||
{
|
||||
name: "flags the named -Path/-Value spelling too",
|
||||
text: workflow(
|
||||
' - run: New-Item -Path Env:SMOKE_BROWSER -Value "firefox"',
|
||||
),
|
||||
rules: [VIOLATION.BROWSER_ENV],
|
||||
},
|
||||
{
|
||||
name: "allows a Set-Item write pinning chromium",
|
||||
text: workflow(
|
||||
' - run: Set-Item -Path Env:SMOKE_BROWSER -Value "chromium"',
|
||||
),
|
||||
rules: [],
|
||||
},
|
||||
{
|
||||
// Reading the variable is not setting it — the same distinction as
|
||||
// `echo "SMOKE_BROWSER: firefox"` from round 5.
|
||||
name: "does not flag a Get-Item read of the same variable",
|
||||
text: workflow(" - run: Get-Item Env:SMOKE_BROWSER; npm run smoke"),
|
||||
rules: [],
|
||||
},
|
||||
{
|
||||
// Copilot, tenth review: GitHub's multiline environment-file syntax.
|
||||
// Reported as unreadable rather than parsed — see the comment on the
|
||||
|
|
|
|||
Loading…
Reference in a new issue