Skip to content

[#1352] Derive codev's build closure from the pnpm graph, not a hand-list - #1355

Merged
amrmelsayed merged 7 commits into
mainfrom
builder/air-1352
Aug 5, 2026
Merged

[#1352] Derive codev's build closure from the pnpm graph, not a hand-list#1355
amrmelsayed merged 7 commits into
mainfrom
builder/air-1352

Conversation

@amrmelsayed

@amrmelsayed amrmelsayed commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Closes #1352

Summary

After the sdk split, building from packages/codev (or via pnpm --filter @cluesmith/codev build) failed with convincing false TS errors (TS2339 on TowerClient, TS2307 on @cluesmith/codev-sdk/constants) whenever workspace deps had no fresh dist. The root chain papered over this with a hand-maintained package list that had already drifted from the dependency graph.

This PR implements the issue's fix A + D:

  • A (graph-derived closure): packages/codev's build now starts with pnpm --filter "@cluesmith/codev^..." build, so its workspace deps (types, sdk, core, apps/web) are built in topological order straight from the pnpm graph. The root chain drops the hand-list entirely and calls the now self-sufficient codev build.
  • D (docs): the stale Local Build Testing bullet and Directory Map line in CLAUDE.md and AGENTS.md are updated; the two files remain byte-identical (enforced by the existing governance-sweep test).

Key decisions

  • build:dashboard collapsed to copy-dashboard. The issue flagged this nuance for reconciliation: the closure now builds apps/web (its package build is tsc -b && vite build, exactly what build:dashboard ran via cd ../../apps/web && pnpm build). Keeping the old step would double-build the dashboard, so the codev-side step is now copy-only (apps/web/dist to dashboard-dist). build:dashboard had no other callers (verified by grep).
  • artifact-canvas stays as one explicit root entry. It has zero workspace deps and is consumed by apps/vscode, so it is not part of codev's closure and not dep-closure drift; the root builds it for the extension dev flow. This is documented in the updated CLAUDE.md/AGENTS.md bullet.
  • check-main-fresh.sh untouched. It is a no-op off main, so worktree and builder-branch root builds are unaffected.
  • local-install path unaffected. scripts/local-install.sh packs core, sdk, and codev; packing does not trigger prepublishOnly, and prepublishOnly (pnpm build) still works from packages/codev since pnpm resolves the workspace from any member directory.

Test plan

  • New packages/codev/src/__tests__/build-scripts.test.ts: asserts codev's build script carries the graph-derived closure prefix, and that the root build script no longer hand-lists any codev workspace dep (types, sdk, core, web). This encodes the invariant "dep closure comes from the graph, not a hand-list".
  • Existing governance-sweep.test.ts continues to enforce CLAUDE.md and AGENTS.md byte-identity over the doc edits.
  • Acceptance (all run from a clean tree, rm -rf packages/*/dist apps/web/dist packages/codev/dashboard-dist):
    1. pnpm --filter @cluesmith/codev build from the repo root: PASS
    2. pnpm build from packages/codev/: PASS
    3. Root pnpm build on the builder branch: PASS
  • porch check 1352: build and unit tests green.

Follow-up (out of scope per architect guidance)

TypeScript project references (composite + tsc -b) would additionally catch the STALE-dist case (deps built but outdated), which neither the closure prefix nor a preflight guard catches. The closure prefix mitigates it in practice (every codev build re-runs the deps' builds, so their dist is refreshed), but incremental correctness across packages without full rebuilds would need project references. Worth weighing against the build-tooling churn in a separate issue if stale-dist bites again.

Consultation (AIR 3-way at PR, consult --protocol air --type pr --issue 1352)

Lane Verdict Confidence Key issues
gemini APPROVE HIGH None
codex APPROVE HIGH None
claude REQUEST_CHANGES HIGH Two stale build-wiring lines in codev/resources/arch.md (819, 1143)

Dispositions:

  • arch.md:819 (build:dashboard reference) and arch.md:1143 (deleted hand-list build order): fixed in commit 1db87032. This is the same doc-misdirection class the issue was filed about, so it belongs in this PR. The types-first rationale for the VS Code extension's esbuild bundle still holds and is now stated as a guarantee of the closure rather than of hand-ordering. A repo-wide grep confirmed the only remaining build:dashboard mentions are historical artifacts (past plans, reviews, release notes), left untouched as history.
  • Non-blocking (claude): CI workflows still hand-build sdk/core before codev. Redundant now but harmless, and in test.yml those builds also feed their own unit-test steps, so they cannot be deleted blindly. Flagged as a follow-up candidate rather than expanded here (AIR scope).
  • Non-blocking (claude): root guard test is absence-only (a hand-list re-expressed as --filter ./packages/sdk would slip through). Accepted as minor; the positive closure-prefix assertion is the load-bearing guard.

Note: the consult ran at the architect's direction at the PR gate; the first attempt failed on project auto-detection ("Multiple projects found") and produced no verdicts, re-run with --issue 1352.

…hand-list

packages/codev's build never built its workspace deps, so a missing dep
dist/ surfaced as convincing false TS errors (TS2339/TS2307) in codev's
own sources. The root chain papered over it with a hand-maintained list
that had already drifted (built artifact-canvas, not in the closure;
omitted apps/web, which is).

- packages/codev build now starts with the graph-derived closure:
  pnpm --filter "@cluesmith/codev^..." build. build:dashboard collapses
  to copy-dashboard (the closure builds apps/web in topo order, so the
  old step would double-build the dashboard).
- Root build drops the hand-list: check-main-fresh + artifact-canvas
  (kept explicitly for the VS Code extension; zero workspace deps) +
  the now self-sufficient codev build.
- CLAUDE.md/AGENTS.md Local Build Testing + Directory Map lines updated
  (byte-identical, enforced by governance-sweep test).
- New build-scripts.test.ts guards the closure prefix and the absence
  of hand-listed deps at root.
Consult (claude lane, REQUEST_CHANGES) caught two stale build-wiring
lines in arch.md: the build:dashboard reference and the deleted
hand-list build order with its types-first rationale. Same
doc-misdirection class the issue targets. The types/dist rationale for
the VS Code extension still holds; it is now guaranteed by the closure
rather than by hand-ordering. Thread records verdicts + dispositions.
@amrmelsayed

Copy link
Copy Markdown
Collaborator Author

Architect Review

APPROVE. Implementation verified: the graph-derived ^... closure with the clean copy-dashboard reconciliation (the closure builds apps/web; codev's script only copies), the root chain reduced to the one deliberate-and-documented entry (artifact-canvas, extension-consumed, outside codev's closure), doc twins byte-identical, and the guard test pinning the scripts against hand-list drift returning.

Process record, worth its weight: the gate initially arrived with the AIR consult skipped (builder misread the 'optional' clause as discretion — honestly confessed, nothing to reconstruct). Held the gate; the consult then earned its keep on the spot — claude's REQUEST_CHANGES caught two further stale build-wiring lines in arch.md (the build:dashboard reference and the old hand-list order) that both the builder's sweep and my review had missed. Fixed in 1db8703, verified. That's the trust-the-protocol lesson demonstrated end-to-end on a 'trivial' build-tooling change.

Non-blocking follow-up candidates recorded in the PR body, correctly not expanded into: CI workflows still hand-build sdk/core (not blindly deletable — test.yml's dep builds feed unit-test steps), and the root guard test is absence-only. Consult UX note: auto-detect failed with 'Multiple projects found' from the worktree, needing --issue 1352 — same friction bugfix-1224's thread hit; if it bites a third time it earns an issue.


Architect review

@amrmelsayed
amrmelsayed merged commit 2c6f7b6 into main Aug 5, 2026
6 checks passed
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.

packages/codev build doesn't build its workspace deps — false TS failures after the sdk split; Directory Map doc points at the broken path

1 participant