Enforce quality-tools release size growth

[feng-shiplight]

Summary

• compare the exact pnpm pack artifact against the current published npm release
• cap packed and unpacked growth at 1%
• require an exact, version-specific human approval record for larger growth
• run the quality-tools size gate in CI as well as before publish

Verification

• pnpm test — 374 tests passed across 68 files
• pnpm typecheck
• pnpm --filter @shiplightai/quality-tools build
• pnpm --filter @shiplightai/quality-tools check:size — packed 42,999 bytes and unpacked 154,252 bytes, both 0.00% versus 0.3.2

[claude]

Review: Enforce quality-tools release size growth

The core design is sound and well-executed: splitting policy from runner, using the actual pnpm-produced artifact rather than npm pack --dry-run, dynamically comparing against the live published release, and the human-approval mechanism that explicitly blocks agents from self-approving. The test suite covers the boundary conditions clearly.

That said, there are two MEDIUM issues and a handful of LOWs.

---

MEDIUM — Three uncached network calls in CI, no retry or fallback

Files: packages/quality-tools/scripts/check-package-size.mjs (lines 19–43)

The script now makes three external calls at CI time:

1. npm view @shiplightai/quality-tools@latest ... --json (metadata)
2. npm view @shiplightai/quality-tools versions --json (full version list)
3. fetch(baselineTarball) (downloads the full tarball)

The old approach read package-size.json with zero network calls. A transient registry outage or rate-limit now fails the build job with an error that looks identical to a genuine size violation — there is no retry path, no cache, and no way for the developer to tell the difference. The quality-ui gate already carries this fragility; this PR doubles the registry exposure in the same CI job.

Suggestion: either combine calls 1 and 2 into a single npm view invocation, or add a brief comment at the top of the script noting that the gate requires registry access, so a CI failure here could be a registry flap.

---

MEDIUM — pack.filename assumed absolute; statSync and tar will silently fail if pnpm returns a relative name

File: packages/quality-tools/scripts/check-package-size.mjs (lines 67–75)

const archivePath = String(pack.filename ?? "");
// ...
currentPackedBytes = statSync(archivePath).size;
// ...
execFileSync("tar", ["-xzf", archivePath, "-C", packRoot], ...);

statSync and tar -xzf both require an absolute or correctly relative path. The cwd for the execFileSync("pnpm", ...) call is packageRoot, not packRoot. If pnpm's JSON output for --pack-destination ever returns just the filename (e.g. shiplightai-quality-tools-0.3.2.tgz) rather than the full path, both calls fail with an opaque ENOENT unrelated to a size issue.

The PR verification shows it works today, but pnpm's --json output format is not part of its stable API contract.

Fix:

import { isAbsolute, basename } from "node:path";
// ...
const archivePath = isAbsolute(pack.filename ?? "")
  ? String(pack.filename)
  : join(packRoot, String(pack.filename ?? ""));

Or add a guard:

if (archivePath.length === 0 || !Array.isArray(pack.files) || !isAbsolute(archivePath)) {
  throw new Error("pnpm pack did not return an absolute archive path.");
}

---

LOW — Two npm view calls where one would do

File: packages/quality-tools/scripts/check-package-size.mjs (lines 19–36)

Both calls target the same registry and package. They can be merged:

npm view @shiplightai/quality-tools@latest version dist.tarball dist.unpackedSize versions --json

npm view ... versions on a @latest-scoped name still returns the complete version list. Eliminating the second round-trip halves the registry exposure and speeds up the step.

---

LOW — extractRoot convention is implicit

File: packages/quality-tools/scripts/check-package-size.mjs (line 77)

const extractRoot = resolve(packRoot, "package");
execFileSync("tar", ["-xzf", archivePath, "-C", packRoot], ...);
// ...
currentUnpackedBytes = directorySize(extractRoot);

The assumption that tar extracts to a package/ subdirectory is correct (npm/pnpm tarball convention) but implicit. If extractRoot ends up not existing, readdirSync throws an unhelpful ENOENT. A one-line comment or a statSync(extractRoot).isDirectory() guard would make the assumption explicit.

---

LOW — No comment on the quality-tools CI step explaining why it's there

File: .github/workflows/ci.yml (new step, after Build)

The quality-ui step immediately below has:

# The 1% size gate is otherwise only wired into quality-ui's
# prepublishOnly, so a regression would surface at publish time rather
# than on the PR that caused it.

The quality-tools step was added above without an equivalent comment. Since the prepublishOnly for quality-tools also runs check:size, the same rationale applies. Without the comment, it's unclear to a future reader why this step is in the build job rather than only in prepublishOnly.

---

LOW — No canonical quality observation for quality-tools size gate

File: .github/workflows/ci.yml

The quality-ui size gate emits a canonical observation (pass or fail) and uploads it for quality scoring. If the quality-tools gate fails, the build job fails but no observation is recorded. This may be intentional (hard gate vs. tracked metric), but the asymmetry is unexplained. Worth a comment or a follow-up issue if this gap is intentional.

---

What is done well

• Policy/runner split (package-size-policy.mjs + check-package-size.mjs): pure policy logic is unit-testable without network or filesystem; tests cover the exact threshold, just-over, valid approval, wrong-version approval, and undersized approval.
• Human-approval guard is properly tight: version-pinned to the local packageVersion, byte-capped at the measured artifact, requires both approvedBy and reason, and the error message explicitly tells agents not to self-approve. AGENTS.md is updated consistently with CLAUDE.md.
• Real artifact vs. dry-run: using pnpm pack produces the exact bytes that pnpm publish would upload, which npm pack --dry-run does not.
• Cleanup logic (two-stage try/finally): packRoot is removed on pack failure in the first finally, and always removed after policy evaluation in the second. Correct.
• Older-than-published guard prevents the gate from running against an already-superseded local version.
• approvedIncrease: null in package-size.json is correctly handled: null only becomes an error when the limit is actually exceeded, and the error message makes clear what a human maintainer must do (and what an agent must not).

[claude]

Review: Enforce quality-tools release size growth

Overall: The PR is well-structured. It moves from a static committed baseline to a live npm-registry comparison, correctly adds CI enforcement before publish, and factors the policy into a separately-tested module. The evaluatePackageSize logic and its test suite are solid. Two issues warrant changes.

---

MEDIUM — No tarball integrity verification

File: packages/quality-tools/scripts/check-package-size.mjs, lines ~58-59

const baselineResponse = await fetch(baselineTarball);
const baselinePackedBytes = (await baselineResponse.arrayBuffer()).byteLength;

The script downloads the published tarball from npm's CDN to measure the baseline packed size, but it never verifies the download against the dist.integrity SRI hash that npm view already returns. If the CDN serves a different byte stream (corruption, compromise, or a cache-poisoning event), the measured baseline could be inflated, allowing an artifact that exceeds the 1% limit to slip through.

The fix is straightforward: add "dist.integrity" to the npm view field list, then verify the downloaded bytes with Node's built-in crypto.createHash('sha512') or SubtleCrypto. This project's stated invariant is that scores/gates "nobody can fake" — the gate's own comparison baseline should carry the same rigor.

---

MEDIUM — Approved-increase path leaves no audit trail in CI output

File: packages/quality-tools/scripts/check-package-size.mjs (the evaluatePackageSize call block); packages/quality-tools/scripts/package-size-policy.mjs, line 76

When evaluatePackageSize returns without throwing (because a valid approvedIncrease is recorded), the caller logs only the raw size numbers:

console.log(`${packageName} ${measurement.label} size: ${measurement.current} bytes ...`);

An operator reading CI output for a build that relied on an approved override has no indication the gate was waived. The approval is visible in the committed package-size.json, but the CI log — the artefact that gets shared when someone asks "did the size gate pass?" — is silent about it.

Suggest either: (a) have evaluatePackageSize return a usedApproval: boolean flag alongside measurements, or (b) log a distinct console.warn in the caller when policy.approvedIncrease !== null and the call succeeds. This makes the override visible at the same level as the violation it replaced.

---

LOW — directorySize does not account for symlinks

File: packages/quality-tools/scripts/check-package-size.mjs, lines ~96-101

function directorySize(path) {
  return readdirSync(path, { withFileTypes: true }).reduce((total, entry) => {
    const entryPath = resolve(path, entry.name);
    return total + (entry.isDirectory() ? directorySize(entryPath) : statSync(entryPath).size);
  }, 0);
}

Dirent.isDirectory() returns false for symlinks to directories, so statSync(entryPath).size is called on them. On Linux statSync follows symlinks; a symlink-to-directory returns the target directory's inode size (typically 4096), not its contents. npm tarballs from pnpm pack should not contain symlinks, so this is unlikely to fire in practice, but the silent mis-measurement is worth documenting or guarding.

---

LOW — versions array fetched but only used for an older-version guard

File: packages/quality-tools/scripts/check-package-size.mjs, line ~30

npm view is called with versions (all published version strings), but the only use is an Array.includes check to detect a downgraded local version. Fetching dist-tags instead (or just the latest dist-tag URL) would yield a smaller payload. Not a correctness issue, but the pattern will amplify if the package ever accumulates many releases.

---

LOW — Missing boundary test: current === limit should pass

File: packages/quality-tools/scripts/package-size-policy.test.ts

The tests cover growth above and below 1%, but not the exact boundary (currentPackedBytes === limitFor(baseline, 1)). The policy uses current > measurement.limit (strict greater-than), so a byte-equal artifact at the limit should pass — worth a one-line assertion to pin that inequality direction.

---

What works well

• Fails closed: if npm is unreachable, execFileSync throws and the script aborts rather than silently passing.
• Deterministic comparison: the baseline comes from the live registry, not a stale committed value, so the gate always reflects the actual published artifact.
• Approval structure is tight: approvalProblem requires version, exact byte bounds, a named approvedBy, and a non-empty reason. An approval for a different version or a smaller artifact is rejected. The AGENTS.md prohibition ("Agents must not add, modify, or claim this approval") correctly documents the human-only constraint.
• Temp-dir cleanup is correct: the asymmetric first-finally pattern (clean up only on measurement failure, leave it for the second finally on success) works correctly across all paths, even if the reasoning requires two reads.
• prepublishOnly is already wired: the CI step is genuinely additive — it surfaces failures at PR time rather than only at publish time, without replacing the publish-time guard.

[claude]

PR #12 — Enforce quality-tools release size growth

Overall: Strong improvement — moving the baseline from a static JSON file to the live published tarball is strictly more correct, and extracting evaluatePackageSize into an isolated, testable module is good engineering. The human-approval gate and its error message ("Agents must not create or claim that approval") correctly enforce the governance invariant. No CRITICAL or HIGH issues found. Two MEDIUM issues require attention before merge.

---

Findings

MEDIUM — No timeout on either network call

packages/quality-tools/scripts/check-package-size.mjs lines 30–43 and 74–80

execFileSync("npm", ["view", ...]) and await fetch(baselineTarball) have no timeout. A registry that is reachable but stalled (e.g., a slow CDN) will hang the CI job until GitHub Actions' 6-hour job wall-clock expires. A transient hang on the PR that introduces the feature would be especially confusing.

Suggested fix:

// execFileSync
execFileSync("npm", ["view", ...], { timeout: 30_000, ... })

// fetch — Node 18+ supports AbortSignal.timeout
const baselineResponse = await fetch(baselineTarball, {
  signal: AbortSignal.timeout(60_000)
});

---

MEDIUM — Test coverage gaps in approvalProblem

packages/quality-tools/scripts/package-size-policy.test.ts

approvalProblem has four distinct validation branches after the version check; the tests only exercise two of them:

Branch	Tested?
approval === null	✓ (implicit, "rejects above 1%")
approval.version !== packageVersion	✓
bounds check (packedBytes < current or unpackedBytes < current)	✓ (packed-only; unpacked variant is not)
approvedBy empty or whitespace	✗
reason empty or whitespace	✗
both packed and unpacked exceed simultaneously	✗

The approvedBy / reason branches are the only technical enforcement of the human-ratification requirement, so they deserve explicit coverage.

---

LOW — evaluatePackageSize divides by baseline without validating it

packages/quality-tools/scripts/package-size-policy.mjs line 48

percent: ((measurement.current - measurement.baseline) / measurement.baseline) * 100

evaluatePackageSize validates maxIncreasePercent but not baseline.packedBytes / baseline.unpackedBytes. A zero baseline produces Infinity / NaN in the returned measurements. The caller currently validates these values, but the function's contract is silent on the requirement. A guard at the top of evaluatePackageSize would make it safe to call from future callers too.

---

LOW — baselineTarball URL fetched without registry-domain validation

check-package-size.mjs line 74

const baselineResponse = await fetch(baselineTarball);

baselineTarball comes from npm's JSON response and is trusted as-is. The subsequent integrity check (line 82) prevents accepting a tampered payload, so the blast radius is limited — but a stale or reconfigured publishConfig pointing at a private registry could redirect the download to an unintended host. A one-line new URL(baselineTarball).hostname guard against the expected registry domain would eliminate the ambiguity.

---

LOW — Cleanup split across two try/finally blocks is correct but opaque

check-package-size.mjs lines 87–187

The measurementComplete flag defers cleanup of packRoot from the first finally to the second one so the archive is available during evaluatePackageSize. The logic is correct, but anyone reading it has to mentally trace the flag to see why cleanup is conditional. A short comment above the first finally block (explaining why cleanup is deferred, not just that it is) would prevent future readers from "fixing" the pattern and introducing a double-delete or a cleanup-before-use bug.

---

Positive observations

• Downloading and integrity-verifying the actual published tarball (rather than npm pack --dry-run) is strictly more accurate for measuring baselinePackedBytes.
• Extracting policy logic into package-size-policy.mjs makes the gate testable in isolation — the existing four tests exercise the critical paths cleanly.
• The error message explicitly says "Agents must not create or claim that approval", reinforcing the governance invariant at the point of failure.
• Adding LICENSE to allowedFilePatterns is correct — npm auto-includes LICENSE even when it is absent from files.
• The stale-version guard (lines 63–72) prevents a developer on an old branch from silently comparing against a newer release.
• AGENTS.md and CLAUDE.md are in sync; the gate description added to AGENTS.md matches the CLAUDE.md invariant exactly.

[claude]

Review: Enforce quality-tools release size growth

Overall: Solid improvement over the hardcoded-baseline approach. The script now fetches the actual published tarball from npm, verifies its SRI integrity, validates the tarball origin against publishConfig.registry, and separates deterministic policy logic into a unit-testable module. The human-approval path is well-guarded. All issues below are LOW severity.

INVARIANT CHECKS

• Score independence: NOT APPLICABLE (release tooling only, not on the scoring path)
• Human-gated fields: PASS. approvedIncrease validation (package-size-policy.mjs) requires exact version, measured byte bounds, non-blank approvedBy and reason. AGENTS.md explicitly warns agents not to write or claim this approval.
• Determinism: PASS. No Date.now(), Math.random(), or unstable iteration anywhere on the scoring path.
• Dependency direction: PASS. Scripts are internal tooling, not importable by any package.
• Saved-artifact compatibility: PASS. package-size.json is an internal config file; the reader and writer change atomically in this PR.
• Published surface: PASS. check:size lives in scripts/, not dist/.
• Agent-skill safety: PASS. AGENTS.md is explicit and specific about what agents must not do.

ISSUES

[LOW] check-package-size.mjs - unhelpful crash when publishConfig.registry is absent

const publishRegistry = new URL(String(packageManifest.publishConfig?.registry ?? ""));

If publishConfig.registry is not set in package.json, new URL("") throws TypeError: Invalid URL with no actionable message. An explicit guard with a clear error (e.g. "package.json must set publishConfig.registry for the size gate to validate the download origin") would save debugging time in CI.

[LOW] check-package-size.mjs - createHash algorithm taken verbatim from npm metadata

const baselineDigest = createHash(integrityMatch[1]).update(baselineBytes).digest("base64");

The algorithm string comes directly from the npm integrity field. npm always uses sha512 today, but accepting any algorithm means a spoofed or mutated registry response could quietly downgrade to a weaker hash. Adding an explicit check that integrityMatch[1] === "sha512" would lock the gate to the algorithm npm actually uses.

[LOW] check-package-size.mjs - backwards-version guard silently skips when versions is not an array

if (Array.isArray(publishedVersions) && publishedVersions.includes(packageVersion) ...

If npm view changes the shape of the versions field, the guard is silently bypassed with no log entry. A console.warn or explicit throw when publishedVersions is present but not an array would surface the skip in CI output.

[LOW] package-size-policy.mjs - current-build metrics not validated

evaluatePackageSize validates baseline.packedBytes and baseline.unpackedBytes but not input.currentPackedBytes or input.currentUnpackedBytes. In practice these come from statSync().size and directorySize() which are always non-negative integers, but an explicit guard would make the contract self-documenting and protect future callers.

[LOW] package-size-policy.test.ts - no test for invalid current-build metrics

A test case passing currentPackedBytes: 0 or NaN would close the coverage gap that mirrors the validation gap above and document the expected behavior.

MINOR OBSERVATIONS (not blocking)

• The uninitialized let currentPackedBytes; let currentUnpackedBytes; declarations are safe because a throw in the first try block propagates before the second try runs. A brief comment would clarify this control-flow dependency for future readers.
• LICENSE was correctly added to allowedFilePatterns.
• The two-phase finally cleanup is correct and well-commented.