Skip to content

test(repo): enforce workspace contracts#2148

Merged
jrusso1020 merged 1 commit into
mainfrom
07-10-test_repo_enforce_workspace_contracts
Jul 12, 2026
Merged

test(repo): enforce workspace contracts#2148
jrusso1020 merged 1 commit into
mainfrom
07-10-test_repo_enforce_workspace_contracts

Conversation

@jrusso1020

@jrusso1020 jrusso1020 commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator

What

Require explicit build, typecheck, and test contracts for every workspace and tighten Studio smoke fixtures.

Why

Contributor-facing package scripts and SDK playground imports were inconsistent, while broad smoke filtering could hide generic failures.

How

Add a workspace contract checker, fix SDK playground public imports, and make Studio mocks schema-valid and error filtering narrow.

Test plan

  • workspace contract tests; SDK playground build/typecheck/test; Studio smoke fixture tests
  • Stack-wide lint, format, build, typecheck, and relevant integration gates

jrusso1020 commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@jrusso1020 jrusso1020 force-pushed the 07-10-test_repo_enforce_workspace_contracts branch 3 times, most recently from f6476f5 to d4e0ebc Compare July 11, 2026 08:21

@miga-heygen miga-heygen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#2148 — test(repo): enforce workspace contracts

Verdict: LGTM 🟢

Two things that make the monorepo harder to silently break:

1. Workspace contract checker. check-workspace-contracts.mjs requires every packages/* workspace to have executable build, typecheck, and test scripts — or a documented opt-out with a reason ≥20 chars. The ≥20 char requirement is a nice touch — it forces a real explanation, not "no tests" or "n/a". Runs in lint. Test coverage on the checker itself is clean: accepts all-scripts, reports missing/empty, validates opt-out specificity.

2. Studio smoke test refactoring. The 75-line inline CI heredoc becomes scripts/studio-runtime-smoke.mjs — a proper testable module with:

  • Typed API fixture map instead of a waterfall of if/else
  • unmockedApiRequests tracking — any NEW API endpoint added to studio that isn't in the fixture map fails the smoke test. This catches API drift that the old version would silently skip.
  • Critical improvement: tightened error filtering. The old version suppressed ERR_CONNECTION_REFUSED, Failed to fetch, is not iterable, Cannot read properties of undefined, Cannot read properties of null. These would hide REAL JavaScript bugs. The new version only suppresses favicon.ico 404s. This is a significant safety improvement — the old filter was a stealth correctness hole.

The smoke test's own test (studio-runtime-smoke.test.mjs) verifies: fixture schema correctness, unknown-API rejection, and that the error filter does NOT suppress generic JS errors. The "does not suppress" test at line 42-44 is the proof that the old filter's suppression was wrong.

sdk-playground improvements. Gets tsconfig.json with noUncheckedIndexedAccess: true, typecheck + test scripts, fileAdapter.test.ts covering the persist adapter contract. Import paths fixed from internal (@hyperframes/sdk/adapters/types) to public API (@hyperframes/sdk). main.ts gets ?? null and ?? "#3b82f6" guards for indexed access.

No SSOT violations. The workspace contract + tightened smoke test make this the strongest PR in the stack for preventing future regressions.

Review by Miga

miguel-heygen
miguel-heygen previously approved these changes Jul 11, 2026

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Final direct pass at current head d4e0ebc. Verified the requested stack-specific behavior and no unresolved current review threads remain. Current CI lanes are green; any displayed regression failure is from a superseded run, while the latest shard set passes. Approved on code merits; retain stack order.

@james-russo-rames-d-jusso james-russo-rames-d-jusso left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at d4e0ebc3.

The studio smoke refactor is the piece worth calling out — this is a real improvement, not just a code-tidy. The old inline heredoc in ci.yml had an error filter that dropped every one of:

  • "Cannot read properties of undefined"
  • "Cannot read properties of null"
  • "is not iterable"
  • "Failed to fetch"

Those are precisely the shapes React hooks-order violations and undefined-derefs take. Old smoke ran GREEN through them. New scripts/studio-runtime-smoke.mjs:isExpectedStudioSmokeError allows only favicon.ico — everything else surfaces as fatal. Confirmed at studio-runtime-smoke.test.mjs:53 with explicit assertions that "Cannot read properties of undefined", "is not iterable", and "Failed to fetch" are NO LONGER suppressed. This is the correct fix for feedback_illusory_defense_in_depth_same_source / feedback_regression_fixture_environment_bypass — the old filter was masking real bugs; the new one only masks favicon noise.

The mock schema tightening is the other half. Old smoke returned {} catch-all for every unmatched /api/ path. New smoke:

  • Ships per-endpoint typed responses for every path the studio actually queries (/api/projects/{id}, /api/projects/{id}/files/{path}, /api/projects/{id}/preview[/...], /gsap-animations, /lint, /storyboard, /selection, /renders, /registry/blocks, /fonts, /assets/global) via GET_RESPONSES + projectFileResponse + projectPreviewResponse + gsapAnimationsResponse.
  • Fails on unmocked /api/ requestsstudio-runtime-smoke.mjs:76-79 returns null for known-but-not-in-map paths, and the caller (:107) records them in unmockedApiRequests and throws with unmocked API request: METHOD /path in the failure list. Old smoke silently returned {} for these, letting the studio ride into "everything is empty" and pass — new smoke forces the fixture to grow when a new API surface lands.

Workspace-contract enforcement at scripts/check-workspace-contracts.mjs:

  • REQUIRED_WORKSPACE_SCRIPTS = ["build", "typecheck", "test"] is the right minimal set for the monorepo standard.
  • Opt-out mechanism at :20 is documented and specific-reason-gated (hasSpecificReason requires ≥20 chars). Reasonable social discipline — the length gate keeps drive-by escape hatches from just saying "n/a".
  • Discovery + iteration order is deterministic (readdirSync(packagesDir).sort() at :37) so failure output is stable.

The sdk-playground package upgrades are the natural payoff of the workspace-contract check: typecheck + test scripts added, tsconfig with noUncheckedIndexedAccess: true (matching #2147's core runtime discipline), fileAdapter tests exercise the real public contract (vi.stubGlobal("fetch", …) at fileAdapter.test.ts:11fetch is a public API, so mocking it is fair game, not feedback_external_dep_mock_shape_verify_runtime misuse). The @hyperframes/sdk/adapters/types@hyperframes/sdk import normalization (fileAdapter.ts:1, vite.config.ts:7) is right — importing from public entrypoints only closes off private-path drift.

Three small notes, none blocking:

  • loadPuppeteer() at studio-runtime-smoke.mjs:95 reaches into packages/producer/node_modules via createRequire(join(ROOT, "packages", "producer", "package.json")). Works today because puppeteer is a producer dep, but the coupling means "producer stops depending on puppeteer" silently breaks the studio smoke script. Consider lifting puppeteer to a top-level dev-dep (or a shared scripts/ package) if the smoke becomes load-bearing across more surfaces.
  • SMOKE_COMPOSITION_HTML is a single-line string constant. Fine now; if it grows to cover more studio surfaces (multi-composition project, block registry entries, etc.), a fixture directory (scripts/studio-smoke-fixtures/index.html etc.) will be easier to maintain than inline.
  • Workspace-contract hasSpecificReason bar is length-only (:14reason.trim().length >= 20). Someone determined to game it can write "packaging exception here" (24 chars). Not a real risk on a small team, but worth naming as a social contract rather than a technical one.

Nothing blocking. This one is a straightforward LGTM.

Review by Rames D Jusso

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: 🟢 LGTM at d4e0ebc

Endorses Miga + Rames-D. The studio-smoke error-filter tightening (studio-runtime-smoke.mjs:isExpectedStudioSmokeError now allows only favicon.ico; the old inline heredoc absorbed Cannot read properties of undefined/null, is not iterable, Failed to fetch) is the real hardening here — Rames-D flagged it correctly. Two nits on top.

Findings

F1. Workspace-contract opt-out threshold — decorative-gate flavor, nit. scripts/check-workspace-contracts.mjs:15 requires hyperframesWorkspaceContract.<script>.optOut.reason ≥ 20 chars. Currently zero packages declare hyperframesWorkspaceContract (grep clean). Twenty characters is one short phrase — "no tests, TODO ticket" (21 chars) passes. Miga read this as "a nice touch" that forces a real explanation; my lens reads it as decorative — the length gate creates the illusion of specificity without semantic content. Not blocking. If the intent is enforceable specificity, require a URL pattern (Linear issue, GH issue, PR ref) rather than a length threshold. If the intent is "make people think about it before opting out", 20 chars is fine as-is.

F2. ?? fallbacks in sdk-playground/main.ts are cosmetic-tsc — nit.

  • main.ts:258: if (m) return m[1] ?? null; — regex [^'"]+ requires 1+ chars, so m[1] is guaranteed defined when m truthy. ?? null is unreachable.
  • main.ts:350: TRACK_COLORS[index % TRACK_COLORS.length] ?? "#3b82f6" — modulo into a 6-element const tuple. Always in-bounds. Fallback is unreachable.

Both added to satisfy the new sdk-playground/tsconfig.json:10 noUncheckedIndexedAccess: true. No runtime bugs shadowed. A TRACK_COLORS as const typing (or [a, b, c, d, e, f] as const) would eliminate the fallback entirely and preserve honest strict-mode compliance. Preferred pattern for future strict-mode migrations elsewhere in the repo.

F3. PersistErrorEvent public re-export migration — verified correct. Import moves from @hyperframes/sdk/adapters/types@hyperframes/sdk (main entry). Main entry at packages/sdk/src/index.ts re-exports all three (PersistErrorEvent, PersistAdapter, PersistVersionEntry). Subpath ./adapters/types was never in packages/sdk/package.json's exports map — this migration also closes a spec violation. bundler resolution let it slip; any consumer with strict resolution would have broken. No external consumer breakage (only the two touched sdk-playground files used the subpath).

F4. Scope-drift on title — nit. test(repo): enforce workspace contracts undersells the sdk-playground vitest scaffolding, the SDK public re-export migration, and the studio-smoke extraction (which is a real tightening, not just a move). Body's "make Studio mocks schema-valid and error filtering narrow" understates the deleted-permissive-filter angle Rames-D correctly surfaced.

Adversarial pass

F1 read as decorative-gate initially but the failure mode (author writes a placeholder to bypass) is a code-review problem, not a runtime one — kept as nit. F2 initially considered ?? "#3b82f6" as a real narrowing risk; adversarial pass confirmed 6-element tuple + modulo is genuinely infallible. F3 initially thought the migration might break external consumers via package.json exports resolution; confirmed the subpath was never exported. F4 could arguably be request-changes under strict envelope reading; kept as nit because the diff is hygiene work and every sub-hunk is defensible-adjacent to the title's intent.

R1 by Via

@jrusso1020 jrusso1020 force-pushed the 07-10-fix_core_enforce_strict_runtime_safety branch from 4df6285 to d36fd6f Compare July 11, 2026 17:05
@jrusso1020 jrusso1020 force-pushed the 07-10-test_repo_enforce_workspace_contracts branch from d4e0ebc to cc4229e Compare July 11, 2026 17:05
@jrusso1020 jrusso1020 force-pushed the 07-10-fix_core_enforce_strict_runtime_safety branch from d36fd6f to 53309af Compare July 11, 2026 17:30
@jrusso1020 jrusso1020 force-pushed the 07-10-test_repo_enforce_workspace_contracts branch 2 times, most recently from 46f7f01 to a78b47e Compare July 11, 2026 17:38
@jrusso1020 jrusso1020 force-pushed the 07-10-fix_core_enforce_strict_runtime_safety branch from 53309af to 3c5a355 Compare July 11, 2026 17:38
@jrusso1020 jrusso1020 force-pushed the 07-10-test_repo_enforce_workspace_contracts branch 4 times, most recently from b765b0b to 68b3a7a Compare July 11, 2026 18:21
@jrusso1020 jrusso1020 force-pushed the 07-10-fix_core_enforce_strict_runtime_safety branch 2 times, most recently from 5634cdf to a525f05 Compare July 12, 2026 04:01
@jrusso1020 jrusso1020 force-pushed the 07-10-test_repo_enforce_workspace_contracts branch 2 times, most recently from 77827bd to b089fff Compare July 12, 2026 04:32
Base automatically changed from 07-10-fix_core_enforce_strict_runtime_safety to main July 12, 2026 04:56
@jrusso1020 jrusso1020 dismissed miguel-heygen’s stale review July 12, 2026 04:56

The base branch was changed.

@jrusso1020 jrusso1020 force-pushed the 07-10-test_repo_enforce_workspace_contracts branch from b089fff to 83db364 Compare July 12, 2026 04:58
@jrusso1020 jrusso1020 requested a review from miguel-heygen July 12, 2026 04:59

@miga-heygen miga-heygen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review — test(repo): enforce workspace contracts

Diff unchanged since my prior LGTM (one commit since — rebase only). Original review stands: workspace contract checker is clean SSOT (build/typecheck/test required or documented opt-out with ≥20-char reason), studio smoke test refactored into a proper testable script, sdk-playground gets strict TS + vitest.

LGTM — no new issues.

— Miga

@jrusso1020 jrusso1020 merged commit 9d4ef6e into main Jul 12, 2026
70 of 72 checks passed
@jrusso1020 jrusso1020 deleted the 07-10-test_repo_enforce_workspace_contracts branch July 12, 2026 08:52

@james-russo-rames-d-jusso james-russo-rames-d-jusso left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at 83db364.

R2 — substance of this PR is unchanged from d4e0ebc3 (my R1). The Graphite restacks between then and now moved the SHA but not the diff shape: still the same studio-smoke refactor (isExpectedStudioSmokeError filter narrowing to favicon.ico), the same per-endpoint mock schema tightening in studio-runtime-smoke.mjs, and the same check-workspace-contracts.mjs opt-out gate. R1's LGTM stands.

One new addition since R1: scripts/check-workspace-contracts.test.mjs locks in the hasSpecificReason ≥ 20-char gate + the "reports every missing lifecycle script" behavior. That directly addresses the R1 nit where I flagged the length-only opt-out as a "social contract, not a technical one" — adding regression tests around it is exactly the right shape. Good.

Nothing new to raise. Still an LGTM.

Review by Rames D Jusso

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants