diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f48704a63..abf8e1663 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -73,6 +73,9 @@ jobs: - run: bun run check:react-ui-drift - run: bun run check:react-ui-pin - run: bun run check:tool-package-pins + - run: bun run check:tool-package-freshness + env: + CHECK_BASE_REF: ${{ github.event.pull_request.base.sha }} walking-skeleton: runs-on: ubuntu-latest diff --git a/package.json b/package.json index 5cca2091a..cee30a9c1 100644 --- a/package.json +++ b/package.json @@ -24,7 +24,7 @@ "setup:memory": "bun run scripts/setup-memory.ts", "seed": "bun packages/cli/src/index.ts seed", "reset": "bun packages/cli/src/index.ts reset", - "check:structural": "bun test scripts/checks/test && bun run check:deletion && bun run check:killdates && bun run check:licenses && bun run check:no-product-tenancy && bun run check:browser-safe-subpaths && bun run check:web-utilities && bun run check:tailwind-source && bun run check:ui-vocabulary && bun run check:react-ui-drift && bun run check:react-ui-pin && bun run check:tool-package-pins", + "check:structural": "bun test scripts/checks/test && bun run check:deletion && bun run check:killdates && bun run check:licenses && bun run check:no-product-tenancy && bun run check:browser-safe-subpaths && bun run check:web-utilities && bun run check:tailwind-source && bun run check:ui-vocabulary && bun run check:react-ui-drift && bun run check:react-ui-pin && bun run check:tool-package-pins && bun run check:tool-package-freshness", "check:deletion": "bun run scripts/checks/deletion.ts", "check:killdates": "bun run scripts/checks/killdates.ts", "check:packages": "bun run scripts/checks/packages.ts", @@ -37,6 +37,7 @@ "check:react-ui-drift": "bun run scripts/checks/react-ui-drift.ts", "check:react-ui-pin": "bun run scripts/checks/react-ui-pin.ts", "check:tool-package-pins": "bun run scripts/checks/tool-package-pins.ts", + "check:tool-package-freshness": "bun run scripts/checks/tool-package-freshness.ts", "build:sidecar-image": "docker build -f apps/sidecar/Dockerfile -t corbits-sidecar:dev .", "eval": "bun run scripts/evals-run.ts" }, diff --git a/packages/github-tools/package.json b/packages/github-tools/package.json index 08b8f80fc..a3afb0709 100644 --- a/packages/github-tools/package.json +++ b/packages/github-tools/package.json @@ -2,7 +2,7 @@ "name": "@corbits/github-tools", "private": true, "description": "GitHub search integration: a minimal REST client and an @intx/agent tool bundle exposing github_activity", - "version": "0.0.5", + "version": "0.0.6", "license": "LGPL-2.1-or-later", "type": "module", "exports": { diff --git a/scripts/checks/browser-safe-subpaths.ts b/scripts/checks/browser-safe-subpaths.ts index 4129ef7ab..f7c6f2198 100644 --- a/scripts/checks/browser-safe-subpaths.ts +++ b/scripts/checks/browser-safe-subpaths.ts @@ -89,6 +89,19 @@ interface ImportSpecifier { readonly typeOnly: boolean; } +/** + * Blanks out line and block comments, preserving offsets and newlines so + * the patterns below cannot match prose. Without this, a comment + * mentioning the word `import` before a real import swallows the lines + * between them — `[^;]*?` spans newlines — and reports the file's genuine + * type-only import as a value import. + */ +export function stripComments(contents: string): string { + return contents + .replace(/\/\*[\s\S]*?\*\//g, (match) => match.replace(/[^\n]/g, " ")) + .replace(/\/\/[^\n]*/g, (match) => " ".repeat(match.length)); +} + /** * Extracts every static `from "..."` import/export specifier from a * source file, plus dynamic `import("...")` and bare side-effect @@ -97,7 +110,8 @@ interface ImportSpecifier { * `import { type X, y } from "z"` is conservatively treated as a value * import, since `y` really is one. */ -export function parseImportSpecifiers(contents: string): ImportSpecifier[] { +export function parseImportSpecifiers(source: string): ImportSpecifier[] { + const contents = stripComments(source); const results: ImportSpecifier[] = []; const fromPattern = diff --git a/scripts/checks/killdates.ts b/scripts/checks/killdates.ts index 0090d2737..efb75423b 100644 --- a/scripts/checks/killdates.ts +++ b/scripts/checks/killdates.ts @@ -80,12 +80,28 @@ export function listVendoredPaths(root: string): string[] { const bucketPath = path.join(vendorRoot, bucket.name); for (const entry of readdirSync(bucketPath, { withFileTypes: true })) { if (!entry.isDirectory()) continue; + // `vendor/intx/*` is a workspace glob, so `bun install` materializes a + // directory holding nothing but `node_modules` for every linked + // package — an install artifact, not a vendored tree. Only a + // directory carrying its own source is something the ledger owes a + // kill date. + if (!hasVendoredSource(path.join(bucketPath, entry.name))) continue; vendored.push(path.join("vendor", bucket.name, entry.name)); } } return vendored.sort(); } +/** + * A vendored tree carries hand-copied source. A directory holding only + * `node_modules` is what a workspace glob leaves behind after an install. + */ +export function hasVendoredSource(directory: string): boolean { + return readdirSync(directory, { withFileTypes: true }).some( + (entry) => entry.name !== "node_modules", + ); +} + /** Every vendored directory must carry a kill-date registry row. */ export function auditVendorCoverage( vendoredPaths: readonly string[], diff --git a/scripts/checks/test/browser-safe-subpaths.test.ts b/scripts/checks/test/browser-safe-subpaths.test.ts index b2aa3b518..471b206de 100644 --- a/scripts/checks/test/browser-safe-subpaths.test.ts +++ b/scripts/checks/test/browser-safe-subpaths.test.ts @@ -251,3 +251,26 @@ test("parseImportSpecifiers handles multi-line export-from lists", () => { ); expect(specs).toEqual([{ specifier: "./group", typeOnly: false }]); }); + +test("a comment mentioning import does not swallow a later type-only import", () => { + const parsed = parseImportSpecifiers( + [ + "// The tool modules import `defineTool` from `@intx/agent`, whose", + "// module graph reaches `node:path`, so browser-reachable callers", + "// import from here instead.", + 'import type { ToolPackagePin } from "@intx/types/tool-packages";', + ].join("\n"), + ); + expect(parsed).toEqual([ + { specifier: "@intx/types/tool-packages", typeOnly: true }, + ]); +}); + +test("a block comment cannot hide a real value import", () => { + const parsed = parseImportSpecifiers( + ['/* import x from "commented-out"; */', 'import y from "real";'].join( + "\n", + ), + ); + expect(parsed).toEqual([{ specifier: "real", typeOnly: false }]); +}); diff --git a/scripts/checks/test/killdates.test.ts b/scripts/checks/test/killdates.test.ts index 9f0b642e6..7b58407c8 100644 --- a/scripts/checks/test/killdates.test.ts +++ b/scripts/checks/test/killdates.test.ts @@ -1,11 +1,12 @@ import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import path from "node:path"; -import { expect, test } from "bun:test"; +import { describe, expect, test } from "bun:test"; import { auditKillDates, auditVendorCoverage, auditVendorDrift, + hasVendoredSource, listVendoredPaths, parseKillDates, } from "../killdates"; @@ -80,6 +81,13 @@ test("listVendoredPaths finds directories two levels under vendor/, not files", try { mkdirSync(path.join(root, "vendor", "intx", "log"), { recursive: true }); mkdirSync(path.join(root, "vendor", "intx", "agent"), { recursive: true }); + writeFileSync(path.join(root, "vendor", "intx", "log", "index.ts"), ""); + writeFileSync(path.join(root, "vendor", "intx", "agent", "index.ts"), ""); + // What `bun install` leaves behind for a workspace glob: a directory + // holding nothing but linked dependencies, and no source of its own. + mkdirSync(path.join(root, "vendor", "intx", "installed", "node_modules"), { + recursive: true, + }); writeFileSync(path.join(root, "vendor", "intx", "LICENSE"), "LGPL"); writeFileSync(path.join(root, "vendor", "stray-file"), ""); expect(listVendoredPaths(root)).toEqual([ @@ -191,3 +199,18 @@ test("a vendored row without a valid hash column is a drift violation", () => { rmSync(root, { recursive: true, force: true }); } }); + +describe("hasVendoredSource", () => { + test("a directory holding only node_modules is an install artifact", () => { + const dir = mkdtempSync(path.join(tmpdir(), "killdates-")); + mkdirSync(path.join(dir, "node_modules")); + expect(hasVendoredSource(dir)).toBe(false); + }); + + test("a directory carrying its own source is a vendored tree", () => { + const dir = mkdtempSync(path.join(tmpdir(), "killdates-")); + mkdirSync(path.join(dir, "node_modules")); + mkdirSync(path.join(dir, "src")); + expect(hasVendoredSource(dir)).toBe(true); + }); +}); diff --git a/scripts/checks/test/tool-package-freshness.test.ts b/scripts/checks/test/tool-package-freshness.test.ts new file mode 100644 index 000000000..95af0dbea --- /dev/null +++ b/scripts/checks/test/tool-package-freshness.test.ts @@ -0,0 +1,99 @@ +import { describe, expect, test } from "bun:test"; + +import { + auditFreshness, + packagesWithChangedSource, + readToolPackageNames, +} from "../tool-package-freshness"; + +const TOOL_PACKAGES = ["github-tools", "memory-tools"]; + +describe("packagesWithChangedSource", () => { + test("names a package whose src/ moved", () => { + expect( + packagesWithChangedSource( + ["packages/github-tools/src/client.ts"], + TOOL_PACKAGES, + ), + ).toEqual(["github-tools"]); + }); + + test("ignores tests — they ship no source an agent resolves", () => { + expect( + packagesWithChangedSource( + [ + "packages/github-tools/src/client.test.ts", + "packages/chat-ui/src/timeline.test.tsx", + ], + TOOL_PACKAGES, + ), + ).toEqual([]); + }); + + test("ignores everything outside a package's src/", () => { + expect( + packagesWithChangedSource( + [ + "packages/github-tools/README.md", + "apps/hub/src/index.ts", + "workflows/code-review/src/index.ts", + ], + TOOL_PACKAGES, + ), + ).toEqual([]); + }); +}); + +describe("auditFreshness", () => { + test("the recurring incident: src moved, version did not", () => { + const report = auditFreshness([ + { name: "github-tools", baseVersion: "0.0.5", headVersion: "0.0.5" }, + ]); + expect(report.violations).toHaveLength(1); + expect(report.violations[0]).toContain("packages/github-tools"); + expect(report.violations[0]).toContain("stayed at 0.0.5"); + }); + + test("a bumped package passes", () => { + const report = auditFreshness([ + { name: "github-tools", baseVersion: "0.0.5", headVersion: "0.0.6" }, + ]); + expect(report.violations).toEqual([]); + }); + + test("a package that did not exist at the base ref is new, not stale", () => { + const report = auditFreshness([ + { name: "scout-agent", baseVersion: undefined, headVersion: "0.0.1" }, + ]); + expect(report.violations).toEqual([]); + }); + + test("names every stale package, not just the first", () => { + const report = auditFreshness([ + { name: "github-tools", baseVersion: "0.0.5", headVersion: "0.0.5" }, + { name: "memory-tools", baseVersion: "0.0.4", headVersion: "0.0.4" }, + ]); + expect(report.violations).toHaveLength(2); + }); +}); + +describe("scope", () => { + test("ignores a workspace package the registry does not publish", () => { + expect( + packagesWithChangedSource( + ["packages/workflow-catalog/src/templates.ts"], + TOOL_PACKAGES, + ), + ).toEqual([]); + }); + + test("reads the publisher's own list so the two cannot disagree", () => { + const names = readToolPackageNames(` + export const CORBITS_TOOL_PACKAGE_DIRS: readonly string[] = [ + new URL("../../memory-tools", import.meta.url).pathname, + new URL("../../github-tools", import.meta.url).pathname, + ]; + `); + expect(names).toEqual(["github-tools", "memory-tools"]); + }); +}); diff --git a/scripts/checks/tool-package-freshness.ts b/scripts/checks/tool-package-freshness.ts new file mode 100644 index 000000000..b8779399d --- /dev/null +++ b/scripts/checks/tool-package-freshness.ts @@ -0,0 +1,175 @@ +// check:tool-package-freshness — a package whose `src/` changed must carry +// a version bump in the same change. +// +// Tool resolution keys on `name@version`. New source under an unchanged +// version never reaches a running or freshly-launched agent, and the hub's +// publish step rejects it at seed time — which aborts seeding partway and +// leaves the tenant without its `assistant` definition, so every workbench +// template then fails with "No default setup agent found for this workbench". +// +// `check:tool-package-pins` is the other half of this class: it compares a +// `{ name, version }` pin literal against that package's manifest. It passes +// when both sides agree at the same stale version — exactly the shape that +// keeps recurring (`@corbits/agent-directory-tools` in CL-6497, +// `@corbits/github-tools` at 0.0.5). Catching it needs the change's git +// history rather than a snapshot of the working tree, which is why it is a +// separate check. +import { spawnSync } from "node:child_process"; +import path from "node:path"; +import { + emptyReport, + reportAndExit, + rootFromArgs, + type CheckReport, +} from "./lib/repo"; + +const PACKAGE_ROOT = "packages"; + +/** + * The packages the hub publishes to its tool registry and that workflows + * pin by `{ name, version }`. Sourced from `CORBITS_TOOL_PACKAGE_DIRS` + * (`packages/tool-registry-publish/src/registry.ts`) — the same list the + * publish step walks, so this check and the publisher agree on what a + * "tool package" is. Every other workspace package is resolved by path, + * not by version, and is deliberately out of scope. + */ +const TOOL_PACKAGE_REGISTRY = "packages/tool-registry-publish/src/registry.ts"; + +export function readToolPackageNames(source: string): string[] { + const block = source.match( + /CORBITS_TOOL_PACKAGE_DIRS[^=]*=\s*\[([\s\S]*?)\]/, + ); + if (block === null) return []; + return [...(block[1] ?? "").matchAll(/\.\.\/\.\.\/([a-z0-9-]+)/g)] + .map((match) => match[1] ?? "") + .filter((name) => name.length > 0) + .sort(); +} + +export interface PackageChange { + readonly name: string; + readonly baseVersion: string | undefined; + readonly headVersion: string | undefined; +} + +function git(root: string, args: readonly string[]): string | undefined { + const result = spawnSync("git", [...args], { cwd: root, encoding: "utf8" }); + return result.status === 0 ? result.stdout.trim() : undefined; +} + +/** + * The commit this change branched from. CI supplies the base ref + * explicitly; locally the merge base with `origin/main` answers the same + * question a reviewer would ask. + */ +export function resolveBaseRef( + root: string, + explicit: string | undefined, +): string | undefined { + if (explicit !== undefined && explicit.length > 0) return explicit; + return git(root, ["merge-base", "HEAD", "origin/main"]); +} + +/** + * The package names whose non-test `src/` files appear in a changed-file + * list. Test files are excluded: they ship no source an agent resolves. + */ +export function packagesWithChangedSource( + changedFiles: readonly string[], + toolPackages: readonly string[], +): string[] { + const isToolPackage = new Set(toolPackages); + const touched = new Set(); + for (const file of changedFiles) { + const [root, name, dir] = file.split("/"); + if (root !== PACKAGE_ROOT || name === undefined || dir !== "src") continue; + if (file.endsWith(".test.ts") || file.endsWith(".test.tsx")) continue; + if (!isToolPackage.has(name)) continue; + touched.add(name); + } + return [...touched].sort(); +} + +/** + * A package is fresh when its version moved with its source. A package + * absent at the base ref is new, and its first version counts as a bump. + */ +export function auditFreshness(changes: readonly PackageChange[]): CheckReport { + const report = emptyReport(); + for (const change of changes) { + if (change.baseVersion === undefined) continue; + if (change.headVersion !== change.baseVersion) continue; + report.violations.push( + `packages/${change.name}: src/ changed but package.json stayed at ` + + `${change.baseVersion}. Tool resolution keys on name@version, so new ` + + `source under an unchanged version never reaches a running or ` + + `freshly launched agent and the hub rejects it at publish time. ` + + `Bump the version, then update every { name, version } pin that ` + + `references it.`, + ); + } + return report; +} + +function versionAtRef( + root: string, + ref: string, + name: string, +): string | undefined { + const shown = git(root, [ + "show", + `${ref}:${PACKAGE_ROOT}/${name}/package.json`, + ]); + if (shown === undefined) return undefined; + return (JSON.parse(shown) as { version?: string }).version; +} + +async function versionAtHead( + root: string, + name: string, +): Promise { + const manifest = Bun.file( + path.join(root, PACKAGE_ROOT, name, "package.json"), + ); + if (!(await manifest.exists())) return undefined; + return ((await manifest.json()) as { version?: string }).version; +} + +async function main(): Promise { + const root = rootFromArgs(Bun.argv.slice(2)); + const baseRef = resolveBaseRef(root, process.env["CHECK_BASE_REF"]); + if (baseRef === undefined) { + const report = emptyReport(); + report.notes.push( + "no base ref (no origin/main, no CHECK_BASE_REF); skipping — CI " + + "supplies the base ref for the authoritative run", + ); + reportAndExit("check:tool-package-freshness", report); + } + + const registry = Bun.file(path.join(root, TOOL_PACKAGE_REGISTRY)); + const toolPackages = (await registry.exists()) + ? readToolPackageNames(await registry.text()) + : []; + const diff = git(root, ["diff", "--name-only", `${baseRef}...HEAD`]); + const names = packagesWithChangedSource( + (diff ?? "").split("\n"), + toolPackages, + ); + const changes: PackageChange[] = []; + for (const name of names) { + changes.push({ + name, + baseVersion: versionAtRef(root, baseRef, name), + headVersion: await versionAtHead(root, name), + }); + } + + const report = auditFreshness(changes); + report.notes.push( + `${names.length} package(s) with src/ changes since ${baseRef.slice(0, 8)}`, + ); + reportAndExit("check:tool-package-freshness", report); +} + +if (import.meta.main) await main(); diff --git a/workflows/code-review/src/index.ts b/workflows/code-review/src/index.ts index acd95bc1e..62acb6afe 100644 --- a/workflows/code-review/src/index.ts +++ b/workflows/code-review/src/index.ts @@ -43,7 +43,7 @@ export const CODE_REVIEW_STEP_ID = "code-review"; /** The tool packages this definition pins: GitHub reach, nothing else. */ export const CODE_REVIEW_TOOL_PACKAGE_PINS: readonly ToolPackagePin[] = [ - { name: "@corbits/github-tools", version: "0.0.5" }, + { name: "@corbits/github-tools", version: "0.0.6" }, ]; /** Binds the pinned package's "github" handle to the tenant's connection. */ diff --git a/workflows/last-30-days-research/src/index.ts b/workflows/last-30-days-research/src/index.ts index 8436b5ad9..b7180e719 100644 --- a/workflows/last-30-days-research/src/index.ts +++ b/workflows/last-30-days-research/src/index.ts @@ -145,7 +145,7 @@ export const LAST_30_DAYS_RESEARCH_PENDING_SOURCES = [ export const LAST_30_DAYS_RESEARCH_TOOL_PACKAGE_PINS: readonly ToolPackagePin[] = [ { name: "@corbits/web-search-tools", version: "0.0.3" }, - { name: "@corbits/github-tools", version: "0.0.5" }, + { name: "@corbits/github-tools", version: "0.0.6" }, ]; const SYSTEM_PROMPT = [ diff --git a/workflows/last-30-days-research/test/definition.test.ts b/workflows/last-30-days-research/test/definition.test.ts index 7661f6784..1263a7a6c 100644 --- a/workflows/last-30-days-research/test/definition.test.ts +++ b/workflows/last-30-days-research/test/definition.test.ts @@ -58,7 +58,7 @@ test("the step carries an explicit per-turn timeout, no inline tools, and the tw expect(only.agent.capabilities).toEqual([]); expect(LAST_30_DAYS_RESEARCH_TOOL_PACKAGE_PINS).toEqual([ { name: "@corbits/web-search-tools", version: "0.0.3" }, - { name: "@corbits/github-tools", version: "0.0.5" }, + { name: "@corbits/github-tools", version: "0.0.6" }, ]); expect(only.agent.toolPackagePins).toEqual( LAST_30_DAYS_RESEARCH_TOOL_PACKAGE_PINS,