[codex] Harden cross-package quality checks#14
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens cross-package compatibility between the tat CLI and the VS Code extension by introducing a shared internal contract/helpers package and adding repo-wide verification gates.
Changes:
- Adds an internal
@tat/sharedworkspace package with runtime-validated result schemas and shared file-format parsing helpers. - Refactors the CLI and VS Code extension to consume the shared contract/helpers instead of duplicating parsing/types.
- Adds contract-harness tests, root workspace scripts, smoke checks, and updates CI/publish workflows to run the unified verification flow.
Reviewed changes
Copilot reviewed 38 out of 40 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
vscode-extension/tests/tatOutput.test.ts |
Adds unit coverage for parsing CLI JSON via shared runtime schema validation. |
vscode-extension/src/types.ts |
Switches result type re-exports to come from @tat/shared instead of deep CLI imports. |
vscode-extension/src/tatRunner.ts |
Validates CLI JSON output via RunResultSchema and centralizes parsing in parseRunOutput. |
vscode-extension/src/promptVariables.ts |
Reuses shared file-format detection/parsing helpers for YAML/JSON .tat.* files. |
vscode-extension/src/fileParser.ts |
Reuses shared file-format detection/parsing helpers to keep extension parsing aligned with CLI. |
vscode-extension/package.json |
Adds Vitest-based test script and wires @tat/shared into the extension dev toolchain. |
vscode-extension/CLAUDE.md |
Documents the new shared-contract boundary and test command for the extension. |
tests/contracts/fixtures/request-error.tat.yaml |
Adds a fixture for request/network error output shape validation. |
tests/contracts/fixtures/invalid.tat.json |
Adds an invalid fixture for validating tat validate behavior. |
tests/contracts/fixtures/contract-pass.tat.json |
Adds a passing JSON fixture for end-to-end contract validation. |
tests/contracts/fixtures/contract-filter.tat.yaml |
Adds a YAML fixture to validate skip/filter behavior consistency. |
tests/contracts/fixtures/contract-fail.tat.yml |
Adds a failing YAML fixture that must still emit valid RunResult JSON. |
tests/contracts/cli-extension.contract.test.ts |
Introduces a CLI↔extension contract harness that runs the built CLI and routes stdout through extension helpers. |
tat-cli/src/types.ts |
Re-exports shared result contract types from @tat/shared (CLI keeps input types locally). |
tat-cli/src/fileFormat.ts |
Re-exports file-format helpers from @tat/shared to keep parsing behavior in sync. |
tat-cli/package.json |
Adds @tat/shared as a workspace devDependency for build/type alignment. |
scripts/smoke-packaged-cli.mjs |
Adds a smoke test to validate the packed-and-installed CLI can run validate successfully. |
packages/tat-shared/tsconfig.json |
Defines strict TS settings for the shared internal package (type-check only). |
packages/tat-shared/tests/contracts.test.ts |
Adds tests for shared result schemas and file-format helper behavior. |
packages/tat-shared/src/index.ts |
Exposes shared contracts + file-format helpers as the package entrypoint. |
packages/tat-shared/src/fileFormat.ts |
Implements shared .tat.* file detection and YAML/JSON parsing helpers. |
packages/tat-shared/src/contracts.ts |
Defines Zod schemas + inferred TS types for CLI JSON result contracts. |
packages/tat-shared/package.json |
Declares the internal @tat/shared workspace package and its deps (zod, yaml). |
package.json |
Introduces root workspaces and repo-wide lint/test/verify/test:contracts/smoke scripts. |
package-lock.json |
Adds workspace lockfile for the new monorepo setup. |
Rules/project-tat-schema-compatibility.md |
Adds a repo rule documenting schema + parsing alignment expectations across packages. |
Rules/project-tat-json-result-contract.md |
Adds a repo rule documenting shared JSON result ownership and enforcement. |
Rules/project-tat-cli-exit-codes.md |
Adds a repo rule documenting stable CLI exit codes (0/1/2) and enforcement. |
Rules/project-tat-binary-resolution-and-extension-assumptions.md |
Adds a repo rule documenting extension binary resolution assumptions and enforcement. |
README.md |
Documents new root workspace install/verify/contract commands. |
AGENTS.md |
Updates agent guidance to use root verification and the new contract harness. |
.gitignore |
Ignores Vite/Vitest cache directory (.vite/). |
.github/workflows/publish-vscode.yml |
Switches marketplace publish flow to npm ci + root verify and updates lockfile bump paths. |
.github/workflows/publish-npm.yml |
Switches npm publish flow to npm ci + root verify + packaged CLI smoke test and updates lockfile bump paths. |
.github/workflows/examples-ci.yml |
Switches example CI to install via root workspace (npm ci). |
.github/workflows/ci.yml |
Replaces per-package checks with root verify on a Linux+Windows matrix and adds packaged CLI smoke on Ubuntu. |
.codex/skills/tat-release-check/SKILL.md |
Adds a checklist for publish/version-bump changes aligned with the new verification flow. |
.codex/skills/tat-harness-engineer/SKILL.md |
Adds guidance for maintaining deterministic cross-package contract fixtures/harness. |
.codex/skills/tat-contract-guard/SKILL.md |
Adds guidance for enforcing the shared result contract and exit-code stability. |
.codex/skills/tat-change-triage/SKILL.md |
Adds guidance for triaging cross-package impact and requiring contract tests. |
Comments suppressed due to low confidence (1)
vscode-extension/src/tatRunner.ts:227
- This
try/catchis now redundant becauseparseRunOutput()already wraps parse/validation errors with a detailed message. Re-wrapping here duplicates logic and can cause drift (or hide improvements toparseRunOutput’s error details). Consider removing thetry/catchand lettingparseRunOutput’s error reject the promise directly (or rethrow the caught error unchanged).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export function parseRunOutput(stdout: string, command: string): RunResult { | ||
| try { | ||
| return RunResultSchema.parse(JSON.parse(stdout.trim())); | ||
| } catch { | ||
| throw new Error( | ||
| `tat output was not valid JSON.\n` + | ||
| `Command: ${command}\n` + | ||
| `Output (first 800 chars):\n${stdout.slice(0, 800)}`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
parseRunOutput reports tat output was not valid JSON for any failure, including cases where stdout is valid JSON but fails RunResultSchema validation. This is misleading and makes debugging contract drift harder. Consider catching SyntaxError vs ZodError (or inspecting the thrown error) and emitting a message that distinguishes invalid JSON from invalid RunResult shape, ideally including schema issues in the error output.
There was a problem hiding this comment.
Fixed earlier in 97bce78. parseRunOutput() now distinguishes invalid JSON from shared-contract/schema validation failures so contract drift reports the actual cause instead of always saying the CLI output was not valid JSON.
| "test": "npm --prefix tat-cli test && npm --prefix tat-cli exec vitest run vscode-extension/tests packages/tat-shared/tests --config tat-cli/vitest.config.ts", | ||
| "test:contracts": "npm --prefix tat-cli exec vitest run tests/contracts --config tat-cli/vitest.config.ts", |
There was a problem hiding this comment.
The root test scripts use npm --prefix tat-cli exec vitest ... --config tat-cli/vitest.config.ts while passing paths like vscode-extension/tests and tests/contracts. If --prefix tat-cli sets the working directory to tat-cli/ (as it does for npm --prefix ... run), these relative paths will point at non-existent locations (e.g. tat-cli/vscode-extension/tests) and the config path will also be wrong. Consider switching to workspaces (npm exec --workspace tat-cli -- vitest ...) and/or making all paths explicitly relative to the invoked working directory (e.g. use ../vscode-extension/tests, ../tests/contracts, and vitest.config.ts when running inside tat-cli/).
| "test": "npm --prefix tat-cli test && npm --prefix tat-cli exec vitest run vscode-extension/tests packages/tat-shared/tests --config tat-cli/vitest.config.ts", | |
| "test:contracts": "npm --prefix tat-cli exec vitest run tests/contracts --config tat-cli/vitest.config.ts", | |
| "test": "npm --prefix tat-cli test && npm exec --workspace tat-cli -- vitest run ../vscode-extension/tests ../packages/tat-shared/tests --config vitest.config.ts", | |
| "test:contracts": "npm exec --workspace tat-cli -- vitest run ../tests/contracts --config vitest.config.ts", |
There was a problem hiding this comment.
I checked this suggestion against the actual Vitest behavior in this repo and did not take it. Switching to npm exec --workspace tat-cli -- vitest ... caused the cross-package paths to stop being discovered here, and because passWithNoTests is enabled that weakens the guardrail by exiting cleanly with no coverage. The current npm --prefix tat-cli exec vitest run ... --config tat-cli/vitest.config.ts form resolves correctly from the repo root in this setup, so I kept it.
| "lint": "npm --prefix tat-cli run lint && npm --prefix vscode-extension run lint && npm run lint:shared", | ||
| "lint:shared": "npm exec --workspace tat-cli -- tsc -p ../packages/tat-shared/tsconfig.json --noEmit", | ||
| "test": "npm --prefix tat-cli test && npm --prefix tat-cli exec vitest run vscode-extension/tests packages/tat-shared/tests --config tat-cli/vitest.config.ts", | ||
| "test:contracts": "npm --prefix tat-cli exec vitest run tests/contracts --config tat-cli/vitest.config.ts", |
There was a problem hiding this comment.
test:contracts runs the contract harness against tat-cli/dist/cli.js (see tests/contracts/cli-extension.contract.test.ts), but the script doesn’t build the CLI first. Running npm run test:contracts in isolation will fail unless the CLI was built previously. Consider updating the script to build tat-cli (at least) before running the harness, or adjust the harness to execute the CLI in a way that doesn’t require a pre-built artifact.
| "test:contracts": "npm --prefix tat-cli exec vitest run tests/contracts --config tat-cli/vitest.config.ts", | |
| "test:contracts": "npm --prefix tat-cli run build && npm --prefix tat-cli exec vitest run tests/contracts --config tat-cli/vitest.config.ts", |
There was a problem hiding this comment.
Fixed earlier in 97bce78. test:contracts now builds tat-cli first so the harness can run in isolation against tat-cli/dist/cli.js without depending on a previous build step.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 38 out of 40 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try { | ||
| if (isYamlFile(fileName)) { | ||
| data = parseYaml(text) as TatFile; | ||
| } else { | ||
| data = JSON.parse(text) as TatFile; | ||
| } | ||
| data = parseTatFileContent(fileName, text) as TatFile; | ||
| } catch { | ||
| const format = isYamlFile(fileName) ? 'YAML' : 'JSON'; | ||
| const format = isYamlTatFile(fileName) ? 'YAML' : 'JSON'; | ||
| throw new Error(`${format} parse error in test file`); | ||
| } |
There was a problem hiding this comment.
parseTestFile swallows the underlying parse error from parseTatFileContent and replaces it with a generic " parse error in test file" message. Since the shared parser already throws detailed errors (invalid JSON/YAML vs unsupported extension, with the original message), consider including the original error message (and propagating unsupported-format errors) so users get actionable diagnostics.
There was a problem hiding this comment.
Fixed earlier in 78a28a6. parseTestFile() now preserves the shared parser diagnostics instead of flattening them into a generic format-specific parse error, so unsupported extensions and invalid JSON/YAML surface their original actionable message.
| /** | ||
| * Parse a .tat.json / .tat.yml / .tat.yaml file and return suite/test names | ||
| * with their line positions. Uses JSON.parse or YAML.parse for structure + | ||
| * line scanning for positions. No external deps beyond the yaml package. | ||
| */ | ||
| export function parseTestFile(text: string, fileName = '.tat.json'): ParsedFile { |
There was a problem hiding this comment.
The module-level doc comment still claims parsing has "No external deps beyond the yaml package", but this file now imports parsing helpers from @tat/shared (which also brings in the shared contract/runtime deps). Update the comment to reflect the new responsibility split (shared parsing + extension-only range mapping) to avoid misleading future maintainers.
There was a problem hiding this comment.
Fixed earlier in 78a28a6. The module comment now reflects the current responsibility split: shared parsing lives in @tat/shared, while this file keeps the VS Code-specific range mapping logic.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 41 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } catch (error) { | ||
| const format = isYamlFile(filePath) ? 'YAML' : 'JSON'; | ||
| const format = isYamlTatFile(filePath) ? 'YAML' : 'JSON'; | ||
| const message = error instanceof Error ? error.message : String(error); | ||
| throw new Error(`${format} parse error in test file ${filePath}: ${message}`); |
There was a problem hiding this comment.
parseTatFileContent() can throw Unsupported file format: ..., but this catch block wraps it as a JSON/YAML parse error based only on isYamlTatFile(). That produces a misleading message for unsupported extensions (e.g., reporting a “JSON parse error” when the real issue is an unsupported format). Consider re-throwing unsupported-format errors unchanged (or branching on isTatFile() / the shared error message) so users see the correct cause.
There was a problem hiding this comment.
Fixed in 66d849f. promptVariables.ts now lets parseTatFileContent() propagate its own error, so unsupported extensions are reported as unsupported instead of being mislabeled as a JSON/YAML parse error. I also added coverage for that case in vscode-extension/tests/promptVariables.test.ts.
| import type { RunResult } from '@tat/shared'; | ||
|
|
There was a problem hiding this comment.
There’s a type-only import placed mid-file (import type { RunResult } ...). While valid, it’s easy to miss and inconsistent with typical module structure; consider moving this import to the top (or importing all shared result types once and re-exporting them) to keep the file easier to scan and avoid accidental ordering issues if tooling/lint rules are added later.
There was a problem hiding this comment.
Adjusted in 66d849f. I moved the shared RunResult type import to the top of the module so the file layout stays consistent and easier to scan.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 40 out of 42 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return JSON.parse(raw); | ||
| } catch (error) { | ||
| throw new Error(`Invalid JSON in ${filePath}: ${(error as Error).message}`); | ||
| } |
There was a problem hiding this comment.
parseTatFileContent assumes the caught value is an Error and interpolates (error as Error).message. In JS/TS, catch can receive non-Error values (strings, objects), which will produce undefined and hide the real failure. Prefer error instanceof Error ? error.message : String(error) so invalid JSON errors stay informative.
There was a problem hiding this comment.
Fixed in ec67e26. parseTatFileContent() now uses error instanceof Error ? error.message : String(error) in the JSON branch, and I added a regression test for non-Error parser failures in packages/tat-shared/tests/contracts.test.ts.
| return parseYaml(raw); | ||
| } catch (error) { | ||
| throw new Error(`Invalid YAML in ${filePath}: ${(error as Error).message}`); | ||
| } |
There was a problem hiding this comment.
Same issue for the YAML parsing branch: (error as Error).message may be undefined when non-Error values are thrown. Use an instanceof Error guard (or String(error)) to preserve the underlying parse failure details in the thrown message.
There was a problem hiding this comment.
Fixed in ec67e26. The YAML branch now applies the same non-Error fallback, and the shared tests now cover that path too so both parser branches keep the original failure detail.
| For targeted cross-package checks during development: | ||
|
|
||
| ```bash | ||
| npm --prefix tat-cli exec vitest run vscode-extension/tests --config tat-cli/vitest.config.ts | ||
| npm --prefix vscode-extension run lint | ||
| npm --prefix vscode-extension run build | ||
| npm --prefix tat-cli exec vitest run tests/contracts --config tat-cli/vitest.config.ts | ||
| ``` |
There was a problem hiding this comment.
The example contract/extension Vitest commands use npm --prefix tat-cli exec vitest run vscode-extension/tests --config tat-cli/vitest.config.ts (and tests/contracts). With --prefix tat-cli, Vitest runs in the tat-cli/ context, so these paths should be ../vscode-extension/tests / ../tests/contracts and the config should be vitest.config.ts (otherwise it resolves to non-existent paths like tat-cli/vscode-extension/tests).
There was a problem hiding this comment.
I verified the exact command from AGENTS.md in this repo: npm --prefix tat-cli exec vitest run vscode-extension/tests --config tat-cli/vitest.config.ts runs successfully from the repo root and resolves the cross-package test path correctly here. I did not change the doc because the behavior in this setup does not match the failure mode described in the comment.
| }, | ||
| "dependencies": { | ||
| "yaml": "^2.7.0" | ||
| }, | ||
| "devDependencies": { |
There was a problem hiding this comment.
vscode-extension/src/* no longer imports yaml directly (parsing is now routed through @tat/shared), so keeping yaml as a direct extension dependency appears redundant. Consider removing it from dependencies to avoid unused deps and rely on @tat/shared’s dependency instead (or add a short note if it’s intentionally kept for another runtime path).
There was a problem hiding this comment.
Fixed in ec67e26. The extension no longer keeps yaml as a direct dependency; it now relies on @tat/shared for the parser/runtime path, and I updated the extension guidance note so the manifest and docs stay aligned.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 40 out of 43 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- vscode-extension/package-lock.json: Language not supported
Comments suppressed due to low confidence (2)
.github/workflows/publish-vscode.yml:67
- The version-bump step runs
npm version patchinsidevscode-extension/, which will updatevscode-extension/package-lock.json(since it exists) rather than the rootpackage-lock.json. Because this repo now usesnpm cifrom the workspace root, the root lockfile also needs to stay in sync with workspace package versions (it currently recordsvscode-extension’s version). Either (a) remove per-package lockfiles and rely only on the root lockfile, (b) run the version bump from the root usingnpm version ... --workspace vscode-extensionso it updates the root lock, or (c) includevscode-extension/package-lock.jsoninadd-pathsas well.
add-paths: |
package-lock.json
vscode-extension/package.json
.github/workflows/publish-npm.yml:71
- The version-bump step runs
npm version patchinsidetat-cli/, which will updatetat-cli/package-lock.json(since it exists) rather than the rootpackage-lock.json. Since CI/publish now runnpm ciat the repo root, the root lockfile must remain consistent with workspace package versions. Consider removing per-package lockfiles, running the version bump from the root with--workspace tat-cliso it updates the root lock, and/or addingtat-cli/package-lock.jsontoadd-pathsif you intend to keep per-package lockfiles.
add-paths: |
package-lock.json
tat-cli/package.json
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 40 out of 43 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- vscode-extension/package-lock.json: Language not supported
Comments suppressed due to low confidence (1)
tat-cli/package.json:35
@tat/sharedis now used for runtime code by the CLI (e.g. viasrc/fileFormat.ts), but it’s only listed underdevDependenciesas a localfile:workspace package. Published installs of@nanotiny/tiny-api-testwill not have access to that private workspace package, so the build must ensure@tat/sharedis bundled intodist/cli.js(e.g. via tsupnoExternal/ bundling config) or otherwise made available at runtime. Please make the bundling/packaging expectation explicit so npm installs can’t end up with a missing module at runtime.
"devDependencies": {
"@tat/shared": "file:../packages/tat-shared",
"@types/node": "^24.5.2",
"tsup": "^8.0.0",
"typescript": "^5.4.0",
"vitest": "^2.0.0"
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { RunResultSchema } from '@tat/shared'; | ||
| import { ZodError } from 'zod'; |
There was a problem hiding this comment.
parseRunOutput imports ZodError from zod, but vscode-extension/package.json does not declare zod as a direct dependency/devDependency. This makes the extension rely on transitive/hoisted workspace deps and can also make error instanceof ZodError unreliable if multiple zod copies are ever present. Prefer using RunResultSchema.safeParse(...) (and reading result.error.issues) to avoid the direct zod import, or explicitly add zod to the extension’s dependencies if you want to keep the instanceof check.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 41 out of 44 changed files in this pull request and generated 3 comments.
Files not reviewed (1)
- vscode-extension/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| resolveOnce({ | ||
| result: JSON.parse(stdout.trim()) as RunResult, | ||
| result: parseRunOutput(stdout, `${bin} ${args.join(' ')}`), | ||
| rawOutput: stdout, |
There was a problem hiding this comment.
The error path passes ${bin} ${args.join(' ')} into parseRunOutput(), but args includes --variables key=value. If users put secrets in variables (tokens, passwords), those values will be included verbatim in thrown errors/notifications and could leak into logs. Consider redacting --variables values (or omitting variables entirely) when constructing the command string used for diagnostics.
There was a problem hiding this comment.
Fixed in 49fb1b9. tatRunner.ts now redacts --variables values when constructing the diagnostic command string, so secrets no longer leak into parse/notification errors.
| "../packages/tat-shared": { | ||
| "name": "@tat/shared", | ||
| "version": "0.0.0", | ||
| "dev": true, | ||
| "dependencies": { | ||
| "yaml": "^2.7.0", | ||
| "zod": "^3.22.0" | ||
| } | ||
| }, |
There was a problem hiding this comment.
@tat/shared declares dependencies on yaml and zod, but this lockfile doesn’t contain corresponding node_modules/yaml / node_modules/zod entries. If someone runs npm ci from vscode-extension/, the lockfile is likely out of sync and may fail or produce non-reproducible installs. Since the repo now uses a workspace root package-lock.json, consider removing this per-package lockfile or regenerating it so it fully resolves transitive deps.
There was a problem hiding this comment.
I checked this directly before deciding whether to change it. vscode-extension/package-lock.json already contains yaml and zod entries through @tat/shared, and npm --prefix vscode-extension ci succeeds from a clean install path, so I did not take this comment as an actionable lockfile bug.
| import { RunResultSchema } from '@tat/shared'; | ||
| import { ZodError } from 'zod'; | ||
| import type { RunResult } from './types'; |
There was a problem hiding this comment.
tatRunner.ts imports ZodError from zod, but vscode-extension/package.json does not declare zod as a dependency/devDependency. This can break installs/builds when zod isn’t hoisted (or when the extension is installed/tested outside the repo workspace). Either avoid importing zod directly (e.g., use RunResultSchema.safeParse and read result.error), or add an explicit zod dependency for the extension.
There was a problem hiding this comment.
Fixed in 49fb1b9. parseRunOutput() no longer imports ZodError; it now uses RunResultSchema.safeParse(...) directly, so the extension does not rely on a direct zod dependency for schema classification.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 42 out of 45 changed files in this pull request and generated 2 comments.
Files not reviewed (1)
- vscode-extension/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| throw new Error( | ||
| error instanceof Error | ||
| ? error.message | ||
| : `tat output could not be parsed.\n` + | ||
| `Command: ${command}\n` + | ||
| `Output (first 800 chars):\n${stdout.slice(0, 800)}`, | ||
| ); |
There was a problem hiding this comment.
In parseRunOutput, the catch block wraps any non-SyntaxError Error in a new Error (new Error(error.message)). That drops the original stack trace (and any future custom error subclass metadata), making contract/debug failures harder to trace. Prefer rethrowing the original error when it’s already an Error, and only constructing a new Error for non-Error throwables.
There was a problem hiding this comment.
Fixed in 199c58d. parseRunOutput() now rethrows the original Error object instead of wrapping it in new Error(error.message), so stack trace and custom error metadata are preserved. I also added a regression test that verifies a custom schema error class survives the catch path.
| "build": "node esbuild.js", | ||
| "build:watch": "node esbuild.js --watch", | ||
| "lint": "tsc --noEmit", | ||
| "test": "vitest run tests --config ../tat-cli/vitest.config.ts", |
There was a problem hiding this comment.
The extension’s test script reuses ../tat-cli/vitest.config.ts, but that config reads ./package.json relative to the current working directory to define __CLI_VERSION__. When run from vscode-extension/, it will pick up the extension version (not the CLI version), which can lead to confusing behavior if any tests/imported code rely on __CLI_VERSION__. Consider adding a small vscode-extension/vitest.config.ts (or updating the shared config to resolve package.json relative to the config file) so the define is deterministic across packages.
There was a problem hiding this comment.
I checked this one and did not change it. The reused config does read package.json from the current working directory, but in this repo __CLI_VERSION__ is only consumed by the CLI source (tat-cli/src/cli.ts), not by extension source or extension tests. So the mismatch is real in principle, but it is not currently affecting extension behavior or test outcomes.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 42 out of 45 changed files in this pull request and generated 2 comments.
Files not reviewed (1)
- vscode-extension/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| body: Automated patch bump after publishing to VS Code Marketplace. | ||
| add-paths: | | ||
| package-lock.json | ||
| vscode-extension/package.json |
There was a problem hiding this comment.
The bump PR created by this workflow only includes the root package-lock.json, but the repo still tracks vscode-extension/package-lock.json. If npm version (or any future dependency change) updates the workspace lockfile, the automated bump PR will omit it and the lockfiles can drift. Either delete the per-workspace lockfiles and rely solely on the root workspace lock, or add vscode-extension/package-lock.json to add-paths and keep it updated intentionally.
| vscode-extension/package.json | |
| vscode-extension/package.json | |
| vscode-extension/package-lock.json |
There was a problem hiding this comment.
Fixed in 0aef2b7. The VS Code release bump PR now includes vscode-extension/package-lock.json in add-paths, so if that workflow updates the per-package lockfile it will be carried in the automated bump PR instead of drifting.
| title: "chore: bump version after npm publish" | ||
| body: Automated patch bump after publishing the npm package. | ||
| add-paths: | | ||
| package-lock.json |
There was a problem hiding this comment.
The bump PR created by this workflow only includes the root package-lock.json, but the repo still tracks a package-level lockfile at tat-cli/package-lock.json. If npm version or future dependency changes update the workspace lockfile, the automated PR will omit it and lockfiles can drift. Either remove the per-workspace lockfile(s) and rely solely on the root workspace lock, or include tat-cli/package-lock.json in add-paths and keep it intentionally in sync.
| package-lock.json | |
| package-lock.json | |
| tat-cli/package-lock.json |
There was a problem hiding this comment.
Fixed in 0aef2b7. The npm release bump PR now includes tat-cli/package-lock.json in add-paths, so the package-level lockfile stays in sync with the automated version bump when that workflow touches it.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 42 out of 45 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- vscode-extension/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 42 out of 45 changed files in this pull request and generated 2 comments.
Files not reviewed (1)
- vscode-extension/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { afterEach, describe, expect, it, vi } from 'vitest'; | ||
|
|
||
| import { parseRunOutput } from '../src/tatRunner'; | ||
|
|
There was a problem hiding this comment.
This test file statically imports parseRunOutput before any vi.doMock() calls. That can prevent the @tat/shared mock in the last test from applying when the test is run in isolation (no prior vi.resetModules() has executed), because ../src/tatRunner will already be loaded with the real @tat/shared.
To make the mocking reliable, avoid the top-level import and instead await import('../src/tatRunner') within each test (or call vi.resetModules() immediately before vi.doMock() and the dynamic import).
| vi.doMock('yaml', () => ({ | ||
| parse: () => { | ||
| throw 'bad yaml payload'; | ||
| }, | ||
| })); | ||
|
|
||
| const { parseTatFileContent: parseTatFileContentWithMockedYaml } = await import('../src/fileFormat.js'); | ||
|
|
There was a problem hiding this comment.
parseTatFileContent is imported from ../src/index.js at module load time, which will also load ../src/fileFormat.js and its yaml dependency. In the last test you call vi.doMock('yaml') and then import ../src/fileFormat.js, but if this test is run alone the module will already be cached and the mock won’t take effect.
To ensure this test is isolation-safe, reset the module cache before vi.doMock('yaml') (or move the ../src/index.js import inside tests and use dynamic imports throughout).
Summary
@tat/sharedpackage for file-format helpers and runtime-validated result contractsWhy
Small CLI changes were able to break the VS Code extension, and extension fixes could drift away from the CLI contract because the repo had no single verification path or shared contract boundary. This change hardens that boundary and makes compatibility regressions block in CI.
Developer Impact
npm installonce at the repo root to install workspace dependenciesnpm run verifyfor the full repo gatenpm run test:contractsfor the CLI/extension contract harnessValidation
npm run verifynpm run smoke:packaged-cliCross-Package Notes
This change explicitly checked VS Code extension impact because it touches shared result types, file parsing behavior, CLI output assumptions, and release/CI wiring used by both packages.