diff --git a/.github/workflows/publish-contract.yml b/.github/workflows/publish-contract.yml index 9c3154ea5..fa0de5c8b 100644 --- a/.github/workflows/publish-contract.yml +++ b/.github/workflows/publish-contract.yml @@ -126,29 +126,22 @@ jobs: echo "Unexpected file in package tarball" exit 1 fi - # Secret scan, with ONE deliberate exclusion. Unlike its sibling packages, this one SHIPS the - # redaction logic: dist/telemetry.js defines SECRET_VALUE_PATTERN, whose source text contains - # a literal `-----BEGIN [A-Z ]*PRIVATE KEY-----` alternative (plus a doc comment quoting a - # full PEM header, describing the bug that alternative fixes). Scanning it with the siblings' - # verbatim pattern is a guaranteed false positive on EVERY release -- verified against the - # 0.1.0 bootstrap tarball. Excluding the module by name keeps the check meaningful for the - # other 92 files rather than weakening the pattern for all of them; if a second module ever - # legitimately needs excluding, that is a signal to reconsider the approach, not to extend - # this list quietly. + # Secret scan over EVERY packed file, including dist/telemetry.js. + # + # This package is unusual: it SHIPS the redaction logic, so dist/telemetry.js contains + # SECRET_VALUE_PATTERN, whose source text carries a literal `-----BEGIN [A-Z ]*PRIVATE KEY-----` + # alternative. That form does NOT match the pattern below (`[A-Z ]*` sits where the pattern wants + # `(RSA |EC |OPENSSH )?`), so the module scans clean and needs no exclusion -- verified against the + # packed tarball. The module's doc comment DESCRIBES that header rather than quoting it, for + # exactly this reason (see the comment above SECRET_VALUE_PATTERN); scripts/check-contract-package.ts + # runs the same scan in test:ci, so a regression fails pre-merge rather than at release time. SCAN_DIR="$(mktemp -d)" tar -xzf "$TARBALL_PATH" -C "$SCAN_DIR" if grep -rqE '(BEGIN (RSA |EC |OPENSSH )?PRIVATE KEY|github_pat_|gh[pousr]_|gts_[0-9a-f]{64}|[A-Z0-9_]*(TOKEN|SECRET|PRIVATE_KEY)=)' \ - "$SCAN_DIR/package" \ - --exclude='telemetry.js' --exclude='telemetry.js.map' --exclude='telemetry.d.ts'; then + "$SCAN_DIR/package"; then echo "Secret-like content found in package tarball" exit 1 fi - # And prove the exclusion stayed narrow: the excluded module must still be the ONLY thing that - # would have tripped the scan. If a real secret ever lands in telemetry.js this still fires. - if grep -rqE '(github_pat_|gh[pousr]_[A-Za-z0-9]{16,}|gts_[0-9a-f]{64})' "$SCAN_DIR/package"; then - echo "Real token-shaped content found in package tarball (including the redaction module)" - exit 1 - fi TMP="$(mktemp -d)" npm --prefix "$TMP" init -y >/dev/null npm --prefix "$TMP" install "$TARBALL_PATH" --no-save >/dev/null diff --git a/.release-please-manifest.json b/.release-please-manifest.json index 44e2853ca..3c765ca97 100644 --- a/.release-please-manifest.json +++ b/.release-please-manifest.json @@ -1,4 +1,5 @@ { + "packages/loopover-contract": "0.1.0", "packages/loopover-mcp": "3.15.2", "packages/loopover-engine": "3.16.1", "packages/loopover-miner": "3.15.2", diff --git a/package.json b/package.json index ae70787bf..3be18dd49 100644 --- a/package.json +++ b/package.json @@ -41,6 +41,7 @@ "drizzle:generate": "drizzle-kit generate", "build:mcp": "npm --workspace @loopover/mcp run build", "build:miner": "turbo run build --filter=@loopover/engine && npm --workspace @loopover/miner run build", + "test:contract-pack": "tsx scripts/check-contract-package.ts", "test:mcp-pack": "tsx scripts/check-mcp-package.ts", "test:miner-pack": "tsx scripts/check-miner-package.ts", "test:engine-pack": "tsx scripts/check-engine-package.ts", @@ -136,7 +137,7 @@ "test:smoke:browser:install": "playwright install chromium", "test:smoke:browser": "node --experimental-strip-types scripts/smoke-ui-browser.ts", "pretest:ci": "npm run check-node-version", - "test:ci": "git diff --check && npm run actionlint && npm run lint:composite-actions && npm run db:migrations:check && npm run db:migrations:immutable:check && npm run db:schema-drift:check && npm run selfhost:env-reference:check && npm run miner:env-reference:check && npm run selfhost:validate-observability && npm run cf-typegen:check && npm run build --workspace @loopover/engine && npm run build --workspace @loopover/discovery-index && npm run build:mcp && npm run build:miner && npm run build --workspace @loopover/ui-kit && npm run typecheck && npm run test:coverage && npm run test:engine-parity && npm run test:live-gate-parity && npm run test:driver-parity && npm run validate:mcp && npm run test --workspace @loopover/engine && npm run test:workers && npm run test:mcp-pack && npm run test:miner-pack && npm run test:engine-pack && npm run test:ui-kit-pack && npm run test:miner-deployment-docs-audit && npm run rees:test && npm run ui:openapi:check && npm run control-plane:contract:check && npm run control-plane:openapi:check && npm run ui:version-audit && npm run docs:drift-check && npm run coverage-boltons:check && npm run import-specifiers:check && npm run ui-derived-types:check && npm run server-manifest:check && npm run dead-source-files:check && npm run regate-sort-key:check && npm run command-redelivery-guards:check && npm run dispatch-gate-reasons:check && npm run validate:no-hand-written-js && npm run replay-runner-manifest:check && npm run coco-dev-versions:check && npm run branding-drift:check && npm run manifest:drift-check && npm run engine-parity:drift-check && npm run engines-nvmrc:check && npm run release-manifest:sync:check && npm run release-linked-versions:check && npm run command-reference:check && npm run mcp:tool-reference:check && npm run mcp:client-config:check && npm run contract:api-schemas:check && npm run ui:lint && npm run ui:typecheck && npm run ui:test && npm run ui:build", + "test:ci": "git diff --check && npm run actionlint && npm run lint:composite-actions && npm run db:migrations:check && npm run db:migrations:immutable:check && npm run db:schema-drift:check && npm run selfhost:env-reference:check && npm run miner:env-reference:check && npm run selfhost:validate-observability && npm run cf-typegen:check && npm run build --workspace @loopover/engine && npm run build --workspace @loopover/discovery-index && npm run build:mcp && npm run build:miner && npm run build --workspace @loopover/ui-kit && npm run typecheck && npm run test:coverage && npm run test:engine-parity && npm run test:live-gate-parity && npm run test:driver-parity && npm run validate:mcp && npm run test --workspace @loopover/engine && npm run test:workers && npm run test:mcp-pack && npm run test:contract-pack && npm run test:miner-pack && npm run test:engine-pack && npm run test:ui-kit-pack && npm run test:miner-deployment-docs-audit && npm run rees:test && npm run ui:openapi:check && npm run control-plane:contract:check && npm run control-plane:openapi:check && npm run ui:version-audit && npm run docs:drift-check && npm run coverage-boltons:check && npm run import-specifiers:check && npm run ui-derived-types:check && npm run server-manifest:check && npm run dead-source-files:check && npm run publishable-deps:check && npm run regate-sort-key:check && npm run command-redelivery-guards:check && npm run dispatch-gate-reasons:check && npm run validate:no-hand-written-js && npm run replay-runner-manifest:check && npm run coco-dev-versions:check && npm run branding-drift:check && npm run manifest:drift-check && npm run engine-parity:drift-check && npm run engines-nvmrc:check && npm run release-manifest:sync:check && npm run release-linked-versions:check && npm run command-reference:check && npm run mcp:tool-reference:check && npm run mcp:client-config:check && npm run contract:api-schemas:check && npm run ui:lint && npm run ui:typecheck && npm run ui:test && npm run ui:build", "test:release": "npm run test:ci && npm run changelog:check", "test:release:mcp": "npm run test:ci", "test:watch": "vitest", diff --git a/packages/loopover-contract/CHANGELOG.md b/packages/loopover-contract/CHANGELOG.md new file mode 100644 index 000000000..a44659457 --- /dev/null +++ b/packages/loopover-contract/CHANGELOG.md @@ -0,0 +1,10 @@ +# Changelog + +## 0.1.0 + +Initial release. `@loopover/contract` is the single zod source of truth for LoopOver's MCP tool and API +contracts — the schemas, tool metadata, and derived projections every server and client reads. + +It is published because it is a **runtime** dependency of `@loopover/mcp` and `@loopover/miner`, which +import it from code that ships (#9749). It must be published *before* any release of those packages that +depends on a new version of it. diff --git a/packages/loopover-contract/src/telemetry.ts b/packages/loopover-contract/src/telemetry.ts index 62ebbc6c3..2cdc13745 100644 --- a/packages/loopover-contract/src/telemetry.ts +++ b/packages/loopover-contract/src/telemetry.ts @@ -152,7 +152,11 @@ const SECRET_KEY_PATTERN = /token|secret|password|passwd|dsn|credential|api[_-]? * The token prefixes carry their own `\b`; the PEM header must NOT, because a word boundary before * a leading hyphen never matches and the whole alternative would be dead. That is not hypothetical * -- it was, until the forbidden-content test in test/unit/mcp-dispatch-telemetry.test.ts caught a - * full `-----BEGIN RSA PRIVATE KEY-----` passing through untouched. + * complete RSA PEM private-key header passing through untouched. + * + * That header is described rather than quoted on purpose: this file is PUBLISHED (#9749), and a literal + * key marker in the tarball trips every secret scanner that reads it -- ours in check-contract-package.ts, + * and a consumer's own. The pattern on the next line is what must be exact; the prose need not be. */ const SECRET_VALUE_PATTERN = /\b(?:gh[pousr]_[A-Za-z0-9]{16,}|sk-[A-Za-z0-9]{16,}|phc_[A-Za-z0-9]{16,})|-----BEGIN [A-Z ]*PRIVATE KEY-----/; diff --git a/scripts/check-contract-package.ts b/scripts/check-contract-package.ts new file mode 100644 index 000000000..c1c5482c6 --- /dev/null +++ b/scripts/check-contract-package.ts @@ -0,0 +1,67 @@ +#!/usr/bin/env node +// Pack check for @loopover/contract (#9654), mirroring check-mcp-package.ts. +// +// The contract package became publishable in #9749 because it is a RUNTIME dependency of both published +// CLIs. A published tarball is immutable, so the file list and its contents are checked BEFORE the +// publish job ever sees them — the same reasoning as the MCP check, against this package's own allowlist. +import { readFileSync } from "node:fs"; +import { join } from "node:path"; +import { spawnSync } from "node:child_process"; +import { fileURLToPath } from "node:url"; +import { FORBIDDEN_CONTENT } from "./forbidden-content"; +import { CONTRACT_PACKAGE_ALLOWED_FILE_PATTERNS } from "./contract-package-allowlist"; + +const FORBIDDEN_PATH = /(^|\/)(\.dev\.vars|\.env|\.npmrc|.*\.pem|.*private.*key.*|.*secret.*)$/i; + +type PackedFile = string | { path: string }; +type ReadContentFn = (file: string) => string; + +/** PURE over an already-resolved pack listing, so the rules are unit-testable without running npm. */ +export function validateContractPackFileList(files: readonly PackedFile[], readContent: ReadContentFn): string[] { + const paths = files.map((file) => (typeof file === "string" ? file : file.path)).sort(); + if (paths.length === 0) throw new Error("Contract package tarball is empty"); + for (const file of paths) { + if (FORBIDDEN_PATH.test(file)) throw new Error(`Forbidden file in contract package: ${file}`); + if (!CONTRACT_PACKAGE_ALLOWED_FILE_PATTERNS.some((pattern) => pattern.test(file))) { + throw new Error(`Unexpected file in contract package: ${file}`); + } + if (FORBIDDEN_CONTENT.test(readContent(file))) throw new Error(`Secret-like content found in contract package file: ${file}`); + } + return paths; +} + +function loadContractPackFromNpm(): { files: PackedFile[] } { + const result = spawnSync("npm", ["pack", "--workspace", "@loopover/contract", "--dry-run", "--json"], { encoding: "utf8" }); + if (result.status !== 0) throw new Error(`npm pack failed: ${result.stderr || result.stdout}`); + const parsed = JSON.parse(result.stdout) as Array<{ files: PackedFile[] }>; + const pack = parsed[0]; + if (!pack) throw new Error("npm pack returned no package entry"); + return pack; +} + +export function runContractPackCheck( + options: { pack?: { files: PackedFile[] }; packageRoot?: string; readContent?: ReadContentFn } = {}, +): string { + const packageRoot = options.packageRoot ?? join(fileURLToPath(import.meta.url), "..", "..", "packages", "loopover-contract"); + const readContent: ReadContentFn = + options.readContent ?? + ((file) => { + try { + return readFileSync(join(packageRoot, file), "utf8"); + } catch { + // A binary or unreadable file has no text to scan; the allowlist above already bounds what may ship. + return ""; + } + }); + const paths = validateContractPackFileList((options.pack ?? loadContractPackFromNpm()).files, readContent); + return `check-contract-package: OK — ${paths.length} packed file(s), all allowlisted and clean.`; +} + +if (process.argv[1]?.endsWith("check-contract-package.ts")) { + try { + console.log(runContractPackCheck()); + } catch (error) { + console.error(String(error instanceof Error ? error.message : error)); + process.exit(1); + } +} diff --git a/scripts/check-publishable-deps.ts b/scripts/check-publishable-deps.ts new file mode 100644 index 000000000..bae0bc97f --- /dev/null +++ b/scripts/check-publishable-deps.ts @@ -0,0 +1,168 @@ +#!/usr/bin/env node +// A published package must not depend on an unpublishable one (#9749). +// +// @loopover/contract was split out by #9521 and became a RUNTIME dependency of both published CLIs +// (@loopover/mcp and @loopover/miner import it from code that ships, built with plain tsc so the specifiers +// survive into the emitted JS) -- but it had no publish workflow, so it was never on npm. Nothing caught +// that, because the failure is invisible until the NEXT release: the currently-published CLI versions +// predate the dependency and install fine. The first user to run `npm install -g @loopover/mcp@latest` +// after that release gets E404, and by then the broken version is already public and immutable. +// +// This is the check that would have failed the moment the dependency was added. It is deliberately about +// the CLASS, not about `@loopover/contract`: any future workspace package that becomes a runtime dependency +// of a published one is caught the same way, with no list to remember to update. +// +// WHAT "RELEASABLE" MEANS HERE: listed in `release-please-config.json` AND carrying a +// `.github/workflows/publish-.yml`. Both, because they fail differently and both failures ship the +// same broken tarball: a package absent from release-please never gets a version bump or a changelog entry +// (so consumers pin a range that is never satisfied), and a package with no publish workflow never reaches +// npm at all. Deriving both from the files themselves, rather than a hand-maintained list, is what keeps +// this honest when a fifth package is added. +// +// WHAT IT DOES NOT CHECK: ordering between two publishes. A contract release must go out BEFORE an mcp +// release that depends on a new version of it, and no static check can see that -- it stays a +// release-runbook step, called out in publish-contract.yml's own header. +import { readdirSync, readFileSync } from "node:fs"; +import { join } from "node:path"; +import { fileURLToPath, URL } from "node:url"; + +export type PackageManifest = { name?: string; private?: boolean; dependencies?: Record }; + +/** One violation, phrased as the failure it will cause rather than as a rule number. */ +export type PublishableDepViolation = { publishedPackage: string; dependency: string; range: string; reason: string }; + +/** + * PURE core: given every workspace manifest and the set of packages that have a publish workflow, name every + * runtime dependency of a published package that cannot itself be installed from npm. + * + * Only `dependencies` are walked. A `devDependency` is not shipped in the tarball and cannot break an + * end user's install, so flagging one would be noise -- and would push contributors toward the wrong fix. + */ +export function findPublishableDepViolations( + manifests: readonly PackageManifest[], + publishedNames: ReadonlySet, + releasePleaseNames: ReadonlySet = publishedNames, +): PublishableDepViolation[] { + const workspaceNames = new Set(manifests.map((manifest) => manifest.name).filter((name): name is string => Boolean(name))); + const privateNames = new Set(manifests.filter((manifest) => manifest.private).map((manifest) => manifest.name)); + const violations: PublishableDepViolation[] = []; + + for (const manifest of manifests) { + if (!manifest.name || !publishedNames.has(manifest.name)) continue; + for (const [dependency, range] of Object.entries(manifest.dependencies ?? {})) { + // Only WORKSPACE packages can be unpublishable in this way; a third-party dependency is on npm by + // definition of being installable at all. + if (!workspaceNames.has(dependency)) continue; + if (privateNames.has(dependency)) { + violations.push({ + publishedPackage: manifest.name, + dependency, + range, + reason: `${dependency} is marked private and can never be published`, + }); + continue; + } + if (!publishedNames.has(dependency)) { + violations.push({ + publishedPackage: manifest.name, + dependency, + range, + reason: `${dependency} has no .github/workflows/publish-*.yml, so it is not published to npm`, + }); + continue; + } + // Registered for publishing but not for RELEASING: it would never get a version bump or a changelog + // entry, so a consumer's range could never be satisfied by a new release. + if (!releasePleaseNames.has(dependency)) { + violations.push({ + publishedPackage: manifest.name, + dependency, + range, + reason: `${dependency} is missing from release-please-config.json, so it is never version-bumped or released`, + }); + } + } + } + return violations; +} + +/** + * Map `publish-.yml` to the package it actually publishes, read from its own `npm pack --workspace` + * line. + * + * Deliberately NOT "any `@loopover/*` mentioned in the file". A publish workflow legitimately references + * OTHER workspace packages in its build steps (`npx turbo run build --filter=@loopover/contract`, + * `npm run build --workspace @loopover/engine`), and treating those as published would silently mark a + * package releasable because something else happens to build it -- masking exactly the violation this + * check exists to catch. The pack line is the definitive signal: a publish workflow packs precisely the + * one package it publishes. + * + * A `publish-*.yml` that packs NOTHING contributes nothing, which is also correct rather than merely safe: + * publish-mcp-registry.yml publishes to the MCP registry, not to npm, so no npm package becomes installable + * because it ran. + * + * A publish workflow with no recognizable pack line contributes NOTHING rather than falling back to a + * looser match -- an unreadable workflow should make the check stricter, never quietly more permissive. + */ +export function publishedPackageNames(workflowFiles: ReadonlyArray<{ name: string; text: string }>): Set { + const names = new Set(); + for (const file of workflowFiles) { + if (!/^publish-.+\.ya?ml$/.test(file.name)) continue; + // Lookbehind rather than a capture group, so the package name IS the whole match. A captured group + // would be typed `string | undefined` under noUncheckedIndexedAccess, forcing a guard whose false arm + // can never run -- an unreachable branch is exactly the kind of thing that rots into noise. + for (const match of file.text.matchAll(/(?<=npm pack --workspace\s+)@[a-z0-9-]+\/[a-z0-9-]+/g)) names.add(match[0]); + } + return names; +} + +function main(): void { + const root = join(fileURLToPath(new URL(".", import.meta.url)), ".."); + const packagesDir = join(root, "packages"); + const manifests: PackageManifest[] = []; + for (const entry of readdirSync(packagesDir, { withFileTypes: true })) { + if (!entry.isDirectory()) continue; + try { + manifests.push(JSON.parse(readFileSync(join(packagesDir, entry.name, "package.json"), "utf8")) as PackageManifest); + } catch { + // A directory without a manifest is not a workspace package; skip rather than fail the whole check. + } + } + + const workflowsDir = join(root, ".github", "workflows"); + const workflowFiles = readdirSync(workflowsDir) + .filter((name) => /^publish-.+\.ya?ml$/.test(name)) + .map((name) => ({ name, text: readFileSync(join(workflowsDir, name), "utf8") })); + + const published = publishedPackageNames(workflowFiles); + const releaseConfig = JSON.parse(readFileSync(join(root, "release-please-config.json"), "utf8")) as { + packages?: Record; + }; + const releasePleaseNames = new Set( + Object.values(releaseConfig.packages ?? {}) + .map((entry) => entry["package-name"]) + .filter((name): name is string => Boolean(name)), + ); + const violations = findPublishableDepViolations(manifests, published, releasePleaseNames); + + if (violations.length > 0) { + console.error("check-publishable-deps: a PUBLISHED package depends on something users cannot install:\n"); + for (const violation of violations) { + console.error(` ${violation.publishedPackage} -> "${violation.dependency}": "${violation.range}"`); + console.error(` ${violation.reason}`); + } + console.error( + "\n The next release of that package would publish an uninstallable tarball (npm E404 for every\n" + + " external user), and a published version cannot be taken back. Fix by adding a\n" + + " .github/workflows/publish-.yml for the dependency, bundling it into the consumer, or\n" + + " demoting it to a devDependency if it is genuinely not needed at runtime. A package must be in BOTH\n" + + " release-please-config.json and a publish workflow to count as releasable.", + ); + process.exit(1); + } + console.log( + `check-publishable-deps: OK — ${published.size} published package(s), ${releasePleaseNames.size} release-please-registered, no runtime dependency on an unreleasable workspace package.`, + ); +} + +if (process.argv[1]?.endsWith("check-publishable-deps.ts")) main(); diff --git a/scripts/contract-package-allowlist.ts b/scripts/contract-package-allowlist.ts new file mode 100644 index 000000000..f479d9d46 --- /dev/null +++ b/scripts/contract-package-allowlist.ts @@ -0,0 +1,17 @@ +// Canonical @loopover/contract published-tarball allowlist (#9654). +// +// Declared SEPARATELY from MCP_PACKAGE_ALLOWED_FILE_PATTERNS on purpose: the two packages ship +// completely different trees (the CLI ships named dist/bin and dist/lib entrypoints plus a scripts/ +// helper; the contract ships a whole dist/ of schema modules), so sharing one list would either +// over-permit the CLI or under-permit the contract, and a shared list is exactly how a package +// starts shipping a file nobody reviewed. +export const CONTRACT_PACKAGE_ALLOWED_FILE_PATTERNS: RegExp[] = [ + // Every emitted module, its declarations and its source maps. The exports map has eight subpaths and + // grows with the contract, so enumerating files by name here would be a maintenance trap that fails + // the release rather than catching anything. + /^dist\/.+\.(js|d\.ts|js\.map|d\.ts\.map)$/, + /^package\.json$/, + /^README\.md$/, + /^CHANGELOG\.md$/, + /^LICENSE$/, +]; diff --git a/scripts/forbidden-content.ts b/scripts/forbidden-content.ts index 32392edc5..249e09c30 100644 --- a/scripts/forbidden-content.ts +++ b/scripts/forbidden-content.ts @@ -5,12 +5,22 @@ // the SAME pattern to assert no MCP tool response ever leaks one — importing it here rather than hand-duplicating // the regex keeps every consumer byte-for-byte in sync instead of relying on manual vigilance. // -// The concrete provider-key shapes below are hand-copied (#7433) from the exact regex bodies of the entries in +// The concrete provider-key shapes below are hand-copied (#7433) from the entries in // src/review/secret-patterns.ts's SECRET_PATTERNS that are in its HARD_SECRET_KINDS set (the "near-zero -// false-positive" subset). They are NOT imported directly: this file is a plain `.mjs` run via `node` -// (test:miner-pack / test:mcp-pack), and secret-patterns.ts is TypeScript with no runtime `.js` sibling on this -// path — a runtime `import` of it from node would fail, so the exact bodies are copied per the issue's stated -// fallback. `seed_or_mnemonic` and `bittensor_key` are deliberately NOT included: they are documented in +// false-positive" subset). +// +// WHY STILL A COPY, now that this is a `.ts` file run via `tsx` and could simply import them: the copy is +// deliberately WIDER than canonical on three shapes, and importing would silently NARROW a scanner that +// guards immutable published tarballs. Canonical `github_token`/`github_pat` carry a 20-char floor and +// `private_key_block` requires the `-----` delimiters — sensible for a review lane weighing false positives +// against a human's attention, wrong for a tarball, where a short `ghp_`-shaped fixture or a bare +// `BEGIN RSA PRIVATE KEY` should still stop the release. Two shapes here have no canonical home at all +// (`gts_*`, and the `*_TOKEN=` assignment the review lane treats as advisory-only). +// +// What keeps a copy from rotting is test/unit/forbidden-content.test.ts: it asserts a probe exists for +// EVERY member of HARD_SECRET_KINDS, and that each probe matched by the canonical pattern is matched here +// too. Adding a kind there without mirroring it here fails CI. `seed_or_mnemonic` and `bittensor_key` are +// deliberately NOT included: they are documented in // secret-patterns.ts as weak, false-positive-prone heuristics intentionally excluded from HARD_SECRET_KINDS // (an ordinary `coldkey:` / `hotkey =` line or the word "mnemonic" in Bittensor docs is not a leaked // credential). `jwt` IS in HARD_SECRET_KINDS today and is included here so packaged tarballs get the same diff --git a/scripts/lib/validate-mcp/overrides.ts b/scripts/lib/validate-mcp/overrides.ts index b32b7ff5a..f58bff88b 100644 --- a/scripts/lib/validate-mcp/overrides.ts +++ b/scripts/lib/validate-mcp/overrides.ts @@ -47,4 +47,9 @@ export const RELEASE_AUTOMATION_WATCHED_PATHS: readonly string[] = Object.freeze ".github/workflows/publish-mcp.yml", ".github/workflows/publish-engine.yml", ".github/workflows/publish-miner.yml", + // #9749/#9654: the contract package is a RUNTIME dependency of the two published CLIs, so losing any + // of these three silently returns it to the unpublished state that would break their next release. + "packages/loopover-contract/package.json", + "packages/loopover-contract/CHANGELOG.md", + ".github/workflows/publish-contract.yml", ]); diff --git a/test/unit/check-contract-package.test.ts b/test/unit/check-contract-package.test.ts new file mode 100644 index 000000000..8c88b0108 --- /dev/null +++ b/test/unit/check-contract-package.test.ts @@ -0,0 +1,81 @@ +import { describe, expect, it } from "vitest"; +import { FORBIDDEN_CONTENT } from "../../scripts/forbidden-content"; +import { validateContractPackFileList } from "../../scripts/check-contract-package"; +import { CONTRACT_PACKAGE_ALLOWED_FILE_PATTERNS } from "../../scripts/contract-package-allowlist"; +import { MCP_PACKAGE_ALLOWED_FILE_PATTERNS } from "../../scripts/mcp-package-allowlist"; + +// #9654: @loopover/contract becomes publishable, and a published tarball is immutable — so what may ship +// is checked before the publish job ever sees it, against this package's OWN allowlist. + +const clean = () => ""; + +describe("validateContractPackFileList (#9654)", () => { + it("accepts the real shape: emitted modules, declarations, maps, and the metadata files", () => { + const paths = validateContractPackFileList( + ["dist/index.js", "dist/index.d.ts", "dist/index.js.map", "dist/tools.d.ts.map", "package.json", "README.md", "CHANGELOG.md", "LICENSE"], + clean, + ); + expect(paths).toHaveLength(8); + }); + + it("REGRESSION: rejects a file nobody allowlisted, so a stray emit cannot ship unnoticed", () => { + // A plain unexpected file (the forbidden-PATH rule below catches the secret-shaped names separately). + expect(() => validateContractPackFileList(["dist/index.js", "src/index.ts"], clean)).toThrow(/Unexpected file/); + expect(() => validateContractPackFileList(["dist/index.js", "tsconfig.json"], clean)).toThrow(/Unexpected file/); + expect(() => validateContractPackFileList(["dist/index.js", ".npmrc"], clean)).toThrow(/Forbidden file/); + expect(() => validateContractPackFileList(["dist/index.js", "deploy/id_rsa.pem"], clean)).toThrow(/Forbidden file/); + }); + + it("REGRESSION: rejects secret-like CONTENT even in an allowlisted file", () => { + // This fired for real on first run: dist/telemetry.js carried a literal PEM header inside a doc + // comment. Not a leaked credential, but a string that trips every scanner a consumer might run, so it + // was removed at the source rather than excluded here. + expect(() => + validateContractPackFileList(["dist/telemetry.js"], () => "const example = '-----BEGIN RSA PRIVATE KEY-----';"), + ).toThrow(/Secret-like content/); + }); + + it("an empty tarball is a failure, not a pass — a broken build must not publish as 'clean'", () => { + expect(() => validateContractPackFileList([], clean)).toThrow(/empty/); + }); + + it("accepts npm's object-shaped pack entries as well as bare strings", () => { + expect(validateContractPackFileList([{ path: "dist/index.js" }, "package.json"], clean)).toEqual(["dist/index.js", "package.json"]); + }); + + it("REGRESSION: the contract allowlist is its OWN, not a reuse of the MCP one", () => { + // The two packages ship completely different trees; sharing a list would over-permit one and + // under-permit the other, which is how a package starts shipping a file nobody reviewed. + expect(CONTRACT_PACKAGE_ALLOWED_FILE_PATTERNS).not.toBe(MCP_PACKAGE_ALLOWED_FILE_PATTERNS); + // MCP's named CLI entrypoint is not allowlisted here... + expect(CONTRACT_PACKAGE_ALLOWED_FILE_PATTERNS.some((p) => p.test("scripts/gittensor-score-preview.mjs"))).toBe(false); + // ...and the contract's arbitrary dist modules are not allowlisted by MCP's. + expect(MCP_PACKAGE_ALLOWED_FILE_PATTERNS.some((p) => p.test("dist/agent-specs.d.ts"))).toBe(false); + expect(CONTRACT_PACKAGE_ALLOWED_FILE_PATTERNS.some((p) => p.test("dist/agent-specs.d.ts"))).toBe(true); + }); +}); + +describe("the shipped redaction module scans clean (#9654)", () => { + it("REGRESSION: telemetry.ts DESCRIBES the PEM header rather than quoting it", async () => { + // This package ships its own redaction logic, so SECRET_VALUE_PATTERN's literal + // `BEGIN [A-Z ]*PRIVATE KEY` alternative rides into dist/. That form is harmless -- it does not match + // the scanners' `BEGIN (RSA |EC |OPENSSH )?PRIVATE KEY`. A doc comment QUOTING a concrete header does + // match, and used to: it forced publish-contract.yml to exclude telemetry.js from its release scan, + // leaving the one module that handles secrets as the one module nobody scanned. The exclusion is gone + // and the release scan now covers all 97 packed files -- which holds only while this stays true. + const { readFileSync } = await import("node:fs"); + const source = readFileSync("packages/loopover-contract/src/telemetry.ts", "utf8"); + const scanner = /BEGIN (RSA |EC |OPENSSH )?PRIVATE KEY/; + expect(scanner.test(source)).toBe(false); + // ...while the alternative the pattern actually needs is still present and still unanchored. + expect(source).toContain("-----BEGIN [A-Z ]*PRIVATE KEY-----"); + }); + + it("INVARIANT: FORBIDDEN_CONTENT does not match the regex-source form, only concrete headers", () => { + // Pins WHY the above is sufficient, so a future widening of the scanner fails here with an explanation + // instead of failing an irreversible release. + expect(FORBIDDEN_CONTENT.test("-----BEGIN [A-Z ]*PRIVATE KEY-----")).toBe(false); + expect(FORBIDDEN_CONTENT.test("-----BEGIN RSA PRIVATE KEY-----")).toBe(true); + expect(FORBIDDEN_CONTENT.test("-----BEGIN PRIVATE KEY-----")).toBe(true); + }); +}); diff --git a/test/unit/check-publishable-deps.test.ts b/test/unit/check-publishable-deps.test.ts new file mode 100644 index 000000000..3382c2f18 --- /dev/null +++ b/test/unit/check-publishable-deps.test.ts @@ -0,0 +1,163 @@ +import { describe, expect, it } from "vitest"; +import { findPublishableDepViolations, publishedPackageNames } from "../../scripts/check-publishable-deps"; + +// #9749: @loopover/contract became a runtime dependency of two PUBLISHED CLIs but had no publish workflow, +// so it was never on npm. The failure is invisible until the next release — the already-published versions +// predate the dependency and install fine — and by the time a user hits E404 the broken version is public +// and immutable. These pin the check that catches it at the moment the dependency is added. + +const PUBLISHED = new Set(["@loopover/mcp", "@loopover/miner", "@loopover/engine"]); + +describe("findPublishableDepViolations (#9749)", () => { + it("REGRESSION: reproduces the real bug — a published CLI depending on an unpublished workspace package", () => { + const violations = findPublishableDepViolations( + [ + { name: "@loopover/mcp", dependencies: { "@loopover/contract": "^0.1.0", zod: "^4.0.0" } }, + { name: "@loopover/miner", dependencies: { "@loopover/contract": "^0.1.0" } }, + { name: "@loopover/contract", dependencies: {} }, + ], + PUBLISHED, + ); + expect(violations.map((violation) => violation.publishedPackage)).toEqual(["@loopover/mcp", "@loopover/miner"]); + expect(violations[0]).toMatchObject({ dependency: "@loopover/contract", range: "^0.1.0" }); + expect(violations[0]?.reason).toContain("not published to npm"); + }); + + it("passes once the dependency itself becomes published — the fix, not just the detection", () => { + const violations = findPublishableDepViolations( + [ + { name: "@loopover/mcp", dependencies: { "@loopover/contract": "^0.1.0" } }, + { name: "@loopover/contract", dependencies: {} }, + ], + new Set([...PUBLISHED, "@loopover/contract"]), + ); + expect(violations).toEqual([]); + }); + + it("REGRESSION: a PRIVATE dependency is a distinct, harder failure and says so", () => { + // Adding a publish workflow fixes the unpublished case; it cannot fix this one, so the message must + // not send someone down that path. + const violations = findPublishableDepViolations( + [ + { name: "@loopover/mcp", dependencies: { "@loopover/internal": "^1.0.0" } }, + { name: "@loopover/internal", private: true }, + ], + PUBLISHED, + ); + expect(violations).toHaveLength(1); + expect(violations[0]?.reason).toContain("private and can never be published"); + }); + + it("ignores devDependencies — they never ship in the tarball, so they cannot break an install", () => { + const violations = findPublishableDepViolations( + [{ name: "@loopover/mcp", dependencies: { zod: "^4.0.0" } }, { name: "@loopover/contract" }], + PUBLISHED, + ); + expect(violations).toEqual([]); + }); + + it("ignores third-party dependencies and UNPUBLISHED packages' own dependencies", () => { + const violations = findPublishableDepViolations( + [ + // Third-party: on npm by definition of being installable at all. + { name: "@loopover/mcp", dependencies: { zod: "^4.0.0", "posthog-node": "^5.0.0" } }, + // An unpublished package may depend on whatever it likes -- no user ever installs it directly. + { name: "@loopover/ui", dependencies: { "@loopover/contract": "^0.1.0" } }, + { name: "@loopover/contract" }, + ], + PUBLISHED, + ); + expect(violations).toEqual([]); + }); + + it("an empty workspace is not a violation", () => { + expect(findPublishableDepViolations([], PUBLISHED)).toEqual([]); + expect(findPublishableDepViolations([{ dependencies: { "@loopover/contract": "^0.1.0" } }], PUBLISHED)).toEqual([]); + }); +}); + +describe("publishedPackageNames (#9749)", () => { + it("reads the package a workflow actually PACKS, not the filename slug", () => { + // `publish-ui-kit.yml` publishes `@loopover/ui-kit`; the mapping is read from the file so a future + // workflow can name its package anything. + const names = publishedPackageNames([ + { name: "publish-ui-kit.yml", text: 'PACK_JSON="$(npm pack --workspace @loopover/ui-kit --json)"' }, + { name: "publish-engine.yml", text: 'npm pack --workspace @loopover/engine --pack-destination "$RUNNER_TEMP" --json' }, + ]); + expect([...names].sort()).toEqual(["@loopover/engine", "@loopover/ui-kit"]); + }); + + it("REGRESSION: a package merely BUILT by another workflow is not treated as published", () => { + // Review caught this: deriving from any `@loopover/*` mention meant a publish workflow's own build step + // could silently mark an unpublished package releasable, masking the exact violation this check exists + // to catch. publish-contract.yml really does contain such a line for itself, and publish workflows + // routinely build sibling packages. + const names = publishedPackageNames([ + { + name: "publish-mcp.yml", + text: [ + "npx turbo run build --filter=@loopover/contract", + "npm run build --workspace @loopover/engine", + "run: npm run test --workspace @loopover/ui-kit", + 'PACK_JSON="$(npm pack --workspace @loopover/mcp --json)"', + ].join("\n"), + }, + ]); + // ONLY the packed package. The three built/tested siblings are not published by this workflow. + expect([...names]).toEqual(["@loopover/mcp"]); + }); + + it("a publish workflow with no recognizable pack line contributes NOTHING, never a looser guess", () => { + // An unreadable workflow must make the check stricter, not quietly more permissive. + const names = publishedPackageNames([{ name: "publish-broken.yml", text: "echo @loopover/engine # no pack line" }]); + expect(names.size).toBe(0); + }); + + it("ignores workflows that are not publish-*, so a CI file mentioning a package cannot fake it published", () => { + const names = publishedPackageNames([ + { name: "ci.yml", text: "npm pack --workspace @loopover/contract" }, + { name: "release-selfhost.yml", text: "npm pack --workspace @loopover/engine" }, + ]); + expect(names.size).toBe(0); + }); + + it("accepts both .yml and .yaml, and dedupes a package packed more than once", () => { + const names = publishedPackageNames([ + { name: "publish-mcp.yaml", text: "npm pack --workspace @loopover/mcp\nnpm pack --workspace @loopover/mcp" }, + ]); + expect([...names]).toEqual(["@loopover/mcp"]); + }); + + it("INVARIANT: every real publish workflow yields at most one package, and only the npm ones yield any", async () => { + // Guards the assumption the hardening rests on: a publish workflow packs precisely the one package it + // publishes. Two ways that can rot, both caught here rather than by silently changing what the check + // considers published: + // - a workflow packing TWO packages (the pack line stops being a unique signal), or + // - a workflow that SHOULD pack but no longer matches (a reformatted pack line), which would quietly + // drop a real package out of the published set and take its dependency violations with it. + // publish-mcp-registry.yml legitimately yields zero: it publishes to the MCP *registry*, not to npm, so + // it packs no tarball. It is named explicitly so that a NEW zero-yield workflow fails instead of being + // waved through as "probably another registry one". + const { readdirSync, readFileSync } = await import("node:fs"); + const dir = ".github/workflows"; + const files = readdirSync(dir) + .filter((name) => /^publish-.+\.ya?ml$/.test(name)) + .map((name) => ({ name, text: readFileSync(`${dir}/${name}`, "utf8") })); + expect(files.length).toBeGreaterThan(0); + + const packed = files.map((file) => ({ file: file.name, packages: [...publishedPackageNames([file])] })); + for (const entry of packed) { + expect({ file: entry.file, count: entry.packages.length }).toEqual({ file: entry.file, count: entry.packages.length > 0 ? 1 : 0 }); + expect(entry.packages.length).toBeLessThanOrEqual(1); + } + expect(packed.filter((entry) => entry.packages.length === 0).map((entry) => entry.file)).toEqual(["publish-mcp-registry.yml"]); + // And the npm-publishing set is exactly the workspace packages that are actually on npm. + expect(packed.flatMap((entry) => entry.packages).sort()).toEqual([ + "@loopover/contract", + "@loopover/engine", + "@loopover/mcp", + "@loopover/miner", + "@loopover/ui-kit", + ]); + }); +}); diff --git a/test/unit/forbidden-content.test.ts b/test/unit/forbidden-content.test.ts index e0ccf1074..815b9abe7 100644 --- a/test/unit/forbidden-content.test.ts +++ b/test/unit/forbidden-content.test.ts @@ -3,6 +3,7 @@ import { readFileSync } from "node:fs"; import { join } from "node:path"; import { describe, expect, it } from "vitest"; import { FORBIDDEN_CONTENT } from "../../scripts/forbidden-content"; +import { ADVISORY_ONLY_SECRET_KINDS, HARD_SECRET_KINDS, SECRET_PATTERNS } from "../../src/review/secret-patterns"; // forbidden-content.ts calls itself the single source of truth for the packaged secret-shape detector, but // nothing enforced it: check-mcp-package.ts re-declared the regex as its own local constant and the two could @@ -111,17 +112,44 @@ describe("FORBIDDEN_CONTENT covers the concrete provider-key formats (#7433)", ( expect(FORBIDDEN_CONTENT.test(probe)).toBe(true); }); - it("still matches the pre-existing shapes (private-key block, github_pat, gh*, gts, generic assignment)", () => { - expect(FORBIDDEN_CONTENT.test("BEGIN RSA PRIVATE KEY")).toBe(true); - expect(FORBIDDEN_CONTENT.test("github_pat_" + a(20))).toBe(true); - expect(FORBIDDEN_CONTENT.test("ghp_" + a(30))).toBe(true); - expect(FORBIDDEN_CONTENT.test("gts_" + "0".repeat(64))).toBe(true); - expect(FORBIDDEN_CONTENT.test("MY" + "_TOKEN=" + "x")).toBe(true); + // Every remaining HARD_SECRET_KIND, so the probe set below can be checked for COMPLETENESS rather than + // trusted. These four predate #7433 and were previously asserted in prose-y one-off tests. + const LEGACY_PROBES: Array<[string, string]> = [ + ["private_key_block", "-----BEGIN" + " RSA PRIVATE KEY" + "-----"], + ["github_pat", "github_pat_" + a(22)], + ["github_token", "ghp_" + a(30)], + ["jwt", "eyJ" + A(20) + "." + a(20) + "." + a(20)], + ]; + const ALL_PROBES = [...NEW_FORMAT_PROBES, ...LEGACY_PROBES]; + + it("COMPLETENESS: every HARD_SECRET_KIND has a probe here — adding a 17th kind fails until it is mirrored", () => { + // THE GAP THIS CLOSES: FORBIDDEN_CONTENT hand-copies its provider-key bodies out of SECRET_PATTERNS + // (see the header of scripts/forbidden-content.ts for why the copy still exists). Nothing forced the + // copy to keep up. A 17th entry added to HARD_SECRET_KINDS and forgotten here would leave every + // PUBLISHED TARBALL unscanned for that shape while this whole file still passed green -- the exact + // silent-rot failure mode validate-no-hand-written-js guards against with its stale-entry check. + expect(new Set(ALL_PROBES.map(([kind]) => kind))).toEqual(HARD_SECRET_KINDS); }); - it("matches a jwt-shaped value (#8396)", () => { - // jwt is in HARD_SECRET_KINDS; packaged tarballs must catch the same shape. - expect(FORBIDDEN_CONTENT.test("eyJ" + A(20) + "." + a(20) + "." + a(20))).toBe(true); + it.each(ALL_PROBES)("EQUIVALENCE: the canonical %s pattern and FORBIDDEN_CONTENT agree on the same value", (kind, probe) => { + // The copy is allowed to be WIDER than canonical (it is, deliberately, for gh*/github_pat/private-key + // blocks -- a tarball scanner should not require a 20-char floor or the `-----` delimiters). It must + // never be NARROWER: a value the review lane hard-blocks must never sail into a published tarball. + const canonical = SECRET_PATTERNS.find((pattern) => pattern.name === kind); + expect(canonical, `no SECRET_PATTERNS entry named ${kind}`).toBeDefined(); + expect(canonical?.re.test(probe), `probe for ${kind} does not match its own canonical pattern`).toBe(true); + expect(FORBIDDEN_CONTENT.test(probe), `FORBIDDEN_CONTENT is NARROWER than canonical for ${kind}`).toBe(true); + }); + + it("INVARIANT: the two shapes with no canonical home stay covered", () => { + // `gts_*` (a loopover-issued token) and the `*_TOKEN=` env-assignment shape are intentionally NOT in + // SECRET_PATTERNS -- the review lane treats generic assignments as advisory-only (see + // ADVISORY_ONLY_SECRET_KINDS), but a tarball has no human to advise, so here they are hard blocks. This + // pins that asymmetry as deliberate rather than leftover. + expect(FORBIDDEN_CONTENT.test("gts_" + "0".repeat(64))).toBe(true); + expect(FORBIDDEN_CONTENT.test("MY" + "_TOKEN=" + "x")).toBe(true); + expect(SECRET_PATTERNS.some((pattern) => pattern.name === "gts_token")).toBe(false); + expect(ADVISORY_ONLY_SECRET_KINDS.has("generic_secret_assignment")).toBe(true); }); it("does NOT hard-block the deliberately-excluded weak heuristics (seed / bittensor key shapes)", () => {