Skip to content

chore: make the documented verification commands work - #388

Draft
gblanc-1a wants to merge 3 commits into
AmadeusITGroup:mainfrom
gblanc-1a:docs/agents-verification-guidance
Draft

chore: make the documented verification commands work#388
gblanc-1a wants to merge 3 commits into
AmadeusITGroup:mainfrom
gblanc-1a:docs/agents-verification-guidance

Conversation

@gblanc-1a

Copy link
Copy Markdown
Contributor

Description

Working through the auth-diagnostics change surfaced several places where the documented workflow either fails outright or reports success on work it never checked. This fixes the commands, adds scripts so the correct sequence is one invocation, and records the traps in the AGENTS.md guides.

Three of the findings are correctness issues, not conveniences — each one lets a broken change look verified:

Trap Evidence
Piping a test or lint run through grep/tail replaces the runner's exit code with the pipe's a deliberately failing Vitest run piped to tail exits 0; bare, it exits 1. A Mocha run reported exit 0 while printing 2 failing.
test:unit executes test-dist/ and compiles nothing appended an always-throwing test to a compiled test file; the whole suite still passed
Lint and tsc resolve @ai-primitives-hub/* through built dist/ on a clean main checkout with a stale build, packages/infra lint reported 6 errors about a type that exists nowhere in main's source; they vanished after a rebuild, with no source change

Type of Change

  • 🐛 Bug fix (non-breaking change which fixes an issue)
  • ✨ New feature (non-breaking change which adds functionality)
  • 💥 Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • 📝 Documentation update
  • ♻️ Code refactoring (no functional changes)
  • ⚡ Performance improvement
  • 🧪 Test coverage improvement
  • 🔧 Configuration/build changes

Related Issues

None. Fallout from the work in the auth-diagnostics PR; independent of it and mergeable on its own.

Changes Made

Broken commands that were documented

  • AGENTS.md told everyone to run pnpm run lint:fix, which does not exist at the root. There is also no root eslint.config.mjs, so the npx eslint <changed-paths> --fix --quiet form (introduced by fix(infra): credential diagnostics and anonymous fallback for hub resolution #374's own AGENTS.md rewrite, and repeated in the plan I was following) fails there too.
  • pnpm -C packages -r <script> looks scoped to packages/ but -r still resolves the whole workspace, so it also runs the extension, lib/, website/ and github-actions/: 4920 lines of output where the scoped form gives 181.

New root scripts (all filtering on @ai-primitives-hub/*, the same filter CI already uses)

  • build:packages, test:packages, lint:packages, lint:extension
  • lint:fix — packages + extension, so the long-documented command now exists
  • test:extension — delegates to the extension's existing test:all (compile-tests → unit → integration). My first cut hand-rolled compile-tests && test:unit, which both duplicated test:all and silently dropped the integration layer
  • test:extension:unit — the fast path (~20s, no VS Code) for the inner loop
  • verify:packages (~30s) and verify (~132s), chaining everything in dependency order behind a single exit code

Every pre-existing script is byte-identicalbuild, watch, lint, test, compile, compile-tests, test:unit, package:vsix. I checked which ones CI invokes (build in the publish workflow, lint and test:unit in quick-check.sh, compile/compile-tests/test:integration in the secure CI) and only added alongside them.

Notably not done: a pretest:unit hook, which would make the staleness trap structurally impossible. CI runs compile → post-compile-fixes → compile-tests → post-compile-fixes-essential.js → test:unit, so a pretest hook would re-run the compile after those fixes are applied. test:unit keeps its exact definition; test:extension is the safe path instead. Closing the hole properly means pointing CI at the underlying Mocha invocation — a CI change, so it is left as a decision rather than assumed.

Lint noise

  • Set noWarnOnMultipleProjects on the shared eslint-import-resolver-typescript config. Every package legitimately has both a src and a test tsconfig, so each lint run printed a Multiple projects found warning to stderr that reads like a fault.

Guides

  • Root AGENTS.md: rewritten Commands section plus a new Verification section covering the three traps, the two different test-runner summary formats (Vitest Tests N passed vs Mocha N passing, and that ANSI codes break naive matching unless you pass --no-color), and the fact that extension tests log expected errors from negative-path cases that are not failures.
  • packages/AGENTS.md: points at the scoped scripts; notes that a cross-package type change is invisible until a build.
  • apps/vscode-extension/AGENTS.md: a table of what each test layer sees, and why a passing unit run says nothing about command registration or package.json contributions.
  • apps/vscode-extension/test/AGENTS.md: compile-tests before test:unit is mandatory, not conventional.

.gitignore

  • Ignore .gitnexus/, a local code-intelligence cache — ~111 MB of single-line JSON. One shell grep -r of mine read it and returned a six-figure-character line. It was only excluded via this clone's .git/info/exclude, so nothing protected anyone else.

Testing

No product code changes, so the testing here is that the documented commands actually run and that the composite scripts fail when they should.

Test Coverage

  • Unit tests added/updated
  • Integration tests added/updated
  • Manual testing completed
  • All existing tests pass

Manual Testing Steps

  1. Every new script run individually: build:packages, test:packages, lint:packages, lint:extension, lint:fix, test:extension:unit (23s), test:extension (39s), verify:packages, verify — all exit 0.
  2. pnpm run verifyPASS in 132s, running 2196 unit + 7 integration tests, with lint leaving no files rewritten.
  3. verify fails correctly: injected a failing Vitest test, verify:packages exited non-zero.
  4. Confirmed the eslint stderr warning is gone and lint still exits 0.
  5. Machine-checked that every pnpm run <script> named in the four guides exists in the right package.json (accounting for the one guide whose commands run from apps/vscode-extension/).
  6. Confirmed no pre-existing script definition changed: git diff package.json shows no - line for any of the eight.

Tested On

  • macOS

  • Windows

  • Linux

  • VS Code Stable

  • VS Code Insiders

VS Code Stable via @vscode/test-electron, as part of verify.

Screenshots

Not applicable.

Checklist

  • My code follows the project's style guidelines
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings or errors
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published

No automated tests: the deliverable is scripts and guidance. Each claim in the guides was verified by running the command, and each trap was reproduced deliberately before being written down — details under Manual Testing.

Documentation

  • README.md updated
  • JSDoc comments added/updated
  • No documentation changes needed

The AGENTS.md guides are the change; no other docs needed updating.

Additional Notes

verify runs lint:fix, which rewrites source files. That follows the house rule ("always run linting with its :fix option"), but if you would rather have a read-only gate I can split verify (checking) from verify:fix (mutating). Say which you prefer.

verify requires a display for the integration layer (CI wraps it in xvfb-run on Linux). verify:packages and test:extension:unit are the headless-safe subsets.

One loose end I could not explain: the AGENTS.md injected into my session context already contained this exact guidance, including the "4920 lines" figure — yet no branch, remote ref or stash in the repository contains that text, and main has the old version. Something in the local setup appears to serve a newer AGENTS.md than any committed state. Worth checking, since an agent reading guidance that is not on main will diverge from what CI and other contributors see.

Reviewer Guidelines

Please pay special attention to:

  • Whether verify should include lint:fix (mutating) or be split.
  • Whether to go further and add pretest:unit, accepting a double compile in CI, so pnpm run test:unit cannot silently pass on stale artifacts. I chose not to touch a CI-critical path unasked; the trade-off is yours.
  • The claim that no CI-invoked script changed. I verified it by diff, but a second pair of eyes on .github/workflows/ is cheap insurance.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache License 2.0.

Guillaume BLANC added 3 commits August 6, 2026 10:55
Every command below was run to confirm it succeeds, because the previous
set contained instructions that fail:

- `pnpm run lint:fix` does not exist at the root, and `npx eslint <path>`
  cannot run there either (no root eslint.config.mjs). Replaced with the
  per-package and filtered forms.
- `pnpm -C packages -r <script>` looks scoped to packages/ but resolves
  the whole workspace, so it also runs the extension, lib/, website/ and
  github-actions/: 4920 lines of output where `--filter './packages/*'`
  gives 181.

Added a Verification section for three traps that let a broken change
look verified:

- Piping a test or lint run through grep/tail replaces the runner's exit
  code with the pipe's, so a failing suite reports success.
- `test:unit` executes test-dist/ and compiles nothing (`pretest` belongs
  to `test`), so it silently re-runs the previous build. Confirmed by
  appending an always-throwing test to a compiled test file: the whole
  suite still passed.
- Lint and tsc resolve @ai-primitives-hub/* through built dist/, so a
  stale build reports errors absent from the checked-out source. On a
  clean main checkout this produced six phantom errors that disappeared
  after a rebuild, with no source change.

Also ignore .gitnexus/, a local code-intelligence cache of ~111 MB of
single-line JSON that search tools should never read.
The flow needed six commands in a specific order, and two of the obvious
ways to run them are wrong: `pnpm -C packages -r <script>` resolves the
whole workspace, and `test:unit` executes a stale test-dist/. Both are now
covered by scripts.

New root scripts:
- build:packages / test:packages / lint:packages — filter on
  @ai-primitives-hub/*, the same filter CI uses. 181 lines of output where
  the recursive form gives 4920.
- lint:extension, and lint:fix = packages + extension. AGENTS.md has been
  telling everyone to run `pnpm run lint:fix` at the root, which did not
  exist.
- test:extension = compile-tests && test:unit, so unit tests cannot pass
  against a build that predates your edit.
- verify:packages (~30s) and verify (~110s), which chain the whole thing
  in dependency order and give one exit code.

`test:unit` stays a separate script and keeps its exact definition: CI
applies post-compilation fixes between compile-tests and test:unit, so a
pretest hook would re-run the compile after those fixes. Every
pre-existing script (build, watch, lint, test, compile, compile-tests,
test:unit, package:vsix) is byte-identical, so no workflow changes.

Also set noWarnOnMultipleProjects on the shared eslint resolver config.
Every package legitimately has a src and a test tsconfig, so each lint run
printed a "Multiple projects found" warning to stderr that reads like a
fault.

AGENTS.md, packages/AGENTS.md, and the two extension guides now document
these scripts, plus the redirect-and-tail idiom for reading a failing
verify without pulling 5000 lines into context.
The first cut of `verify` stopped at unit tests, which excluded
test-dist/test/suite/** — the only layer that exercises command
registration, package.json contributions and real activation. A change to
either would have passed `verify` unnoticed.

Two corrections:

- `test:extension` now delegates to the extension's existing `test:all`
  (compile-tests && test:unit && test:integration) instead of hand-rolling
  compile-tests && test:unit. The composite already existed; duplicating it
  minus integration was both redundant and less complete.
- `test:extension:unit` keeps the fast path (~20s, no VS Code) for the
  inner loop.

`verify` is now ~132s and runs 2196 unit + 7 integration tests. The
integration layer launches a real VS Code through @vscode/test-electron and
needs a display; CI already wraps it in xvfb-run on Linux.

The guides now state which layer sees what, so "unit tests pass" is not
mistaken for "the command is registered".
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Backlog

Development

Successfully merging this pull request may close these issues.

1 participant