Typecheck the two deployables, and hold every lockfile to what is committed - #123
Conversation
…s committed (CopilotKit#112) Root typecheck is bun run --filter '*' typecheck, and workspaces is app, server and worker, so agent-computer and supervisor ship a typecheck script nothing ever runs. agent-computer holds the only spawn in the deployment and supervisor is the only thing holding a Docker socket. Both are clean today, so this wires up a check rather than clearing a backlog. A deployables matrix job runs each package's own script after its own frozen install, and it is in verify's needs: branch protection requires verify alone, so a job outside it blocks nothing. A third deployable is one matrix entry and no change to verify. Neither package joins workspaces, because Dockerfile and tests/workspace.test.ts both depend on them staying separate. Six of the seven committed lockfiles were installed without --frozen-lockfile. Adding the flag alone would not have fixed it: bun install --frozen-lockfile exits 0 when no lockfile is present at all, and every one of these Dockerfiles copied only package.json, so the flag would have produced five checks that can never fail. Each now copies its bun.lock as well, which is what makes the flag mean anything. Verified by injection: a bad type in each package fails with TS2322, and a dependency the lockfile cannot satisfy fails with 'lockfile had changes, but lockfile is frozen'. The examples lockfiles are installed by nothing, so there is no site to add a flag to, and agent-bot and agent-langgraph have no typecheck script to run.
…#112) The new job failed on a fresh checkout with TS2688, cannot find type definition file for 'bun'. Both packages set types: ["bun"] in their tsconfig and @types/bun is a root devDependency rather than one of theirs, so tsc finds it by walking up to the root node_modules. A working copy and the image both have that already, which is why the scripts pass locally: Dockerfile installs at the root before it installs agent-computer. Reproduced by moving the root node_modules away, which fails the same way, and by restoring it, which passes. Then checked in the job's real order against a clean tree with every node_modules removed: root install, package install, typecheck, both packages green.
The comment described how the requirement was found rather than what it is. State the requirement: @types/bun is a root devDependency and both packages declare types: ["bun"], so tsc resolves it from the root node_modules.
davidmckayv
left a comment
There was a problem hiding this comment.
Bringing this in. The finding in section 2 is the reason, and I confirmed it rather than taking it on faith: with only package.json in the context, bun install --frozen-lockfile installs 73 packages and exits 0. The flag on its own would have produced five gates that cannot fail.
I have Docker here, which you did not, so I built what CI skips. All five images build from this branch: root, agent-computer, agent-bot, agent-langgraph, supervisor.
Then the part worth having on the record. Added left-pad to supervisor/package.json without touching its lockfile and built the image:
this branch: #10 error: lockfile had changes, but lockfile is frozen
ERROR: process "bun install --frozen-lockfile" did not complete successfully
exit=1
main: #10 + left-pad@1.3.0
74 packages installed
exit=0
So on main an unreviewed dependency lands in the image of the process that holds the Docker socket, and the build reports success. That is the whole argument for this change and it is not hypothetical.
The rest holds up. Both deployable typechecks pass on the merged tree, the double install is real (types: ["bun"] against a root @types/bun), deployables is in verify's needs so it actually gates, and fail-fast: false is right. All four lockfiles exist, no build context is anything but ., and bun.lock is not dockerignored.
Merges clean with main at f794e50, which has moved a long way since you cut this (#97, #124, #126, #127 all landed). Suite on the merged tree: 1138 pass, 5 skip, 0 fail.
Agreed on all three exclusions. The examples/ lockfiles in particular: a job that installs sample code so example drift can break main is a worse trade than leaving them.
No changelog is right. Nothing a deployment does differs.
#126 and #127 dropped the document index and the old connector tables, so the changelog line that said the index was "read by nothing" now understates it: the tables are gone. Say dropped, and add the one Upgrading note that matters, which is that those migrations destroy that data and cannot be rolled back. architecture.md still listed connector state among what the database holds; #127 removed it, so the line goes too. The README's database line never named those tables, so it needs nothing. #123 is CI and build hardening, not a deployment behavior, so it earns no changelog line.
* Catch the changelog and docs up to what shipped The Unreleased notes already tracked most of the recent work, since each change carried its own line in. This fills the gaps and fixes what went stale. Two merged changes had no line. The address guard's alternate-encoding refusal: it turned away the metadata and private addresses as usually written but not the same ones spelled as an IPv6-mapped or NAT64 form, an integer, or with a trailing dot, and it now canonicalises before it checks and refuses the container credential endpoints even with the private-host opt-in on. And the supervisor refusing to adopt a container it did not create, so a shared Docker host cannot hand it a stranger's container with the computer token. Docs that drifted: the README and the Cloud Run note still said one replica, which the deployment doc's own Replicas section now contradicts, so both point at the real remaining constraint instead, which is the shared browser. And AUDIT_RETENTION_DAYS and COMPUTER_SANDBOX were configurable and documented in the changelog and the README but missing from the configuration table. The knowledge back-out left one more orphan the removal missed: agents/invocation.ts routed a built-in agent to the knowledge agent that is gone, and nothing live constructs it. Deleted with its test. The changelog line that said the local index's connector "is going away" is now "has been removed", because it has been. README stays a build doc; none of this adds history to it. * Reconcile with the table drops that landed after #126 and #127 dropped the document index and the old connector tables, so the changelog line that said the index was "read by nothing" now understates it: the tables are gone. Say dropped, and add the one Upgrading note that matters, which is that those migrations destroy that data and cannot be rolled back. architecture.md still listed connector state among what the database holds; #127 removed it, so the line goes too. The README's database line never named those tables, so it needs nothing. #123 is CI and build hardening, not a deployment behavior, so it earns no changelog line.
What this changes
The two packages that run a Bot's computer are now typechecked in CI, and every committed lockfile is installed frozen.
Fixes #112. Thanks @Hotragn — the table of which package has a script against which package CI reaches, and measuring that both are already clean, is what makes this a wiring change rather than a cleanup. Heads up that you said a branch was coming: if you have one further along, say so and I will close this in favour of it.
1. The typecheck the two deployables never got
typecheckat the root isbun run --filter '*' typecheckandworkspacesis["app", "server", "worker"], so--filter '*'never reachesagent-computerorsupervisor. Both ship atypecheckscript somebody wrote deliberately, and neither has ever run in CI.agent-computerholds the onlyspawnin the deployment;supervisoris the only thing holding a Docker socket.Both are clean today, measured under the pinned bun 1.3.14:
So this adds a check rather than clearing a backlog, which is also why nobody noticed the scripts were idle.
A
deployablesmatrix job runs each package's own script after its own frozen install,fail-fast: falseso one red package does not hide the other. It is inverify'sneeds— branch protection requiresverifyalone, so a job outside it would be advisory and block nothing.Neither package joins
workspaces, as the issue asked:Dockerfiledepends on them being separate installs andtests/workspace.test.tspins the current shape on purpose. A third deployable later is one matrix entry and no change toverify, since a matrix reports as a singleneedsresult. The tradeoff, stated plainly: the matrix list is still a list, just not one inverify. A self-discovering job would remove even that, and would also silently enrol packages nobody meant to.Why the job installs twice
Both packages set
types: ["bun"]in theirtsconfig.jsonwhile@types/bunis a root devDependency (package.json:30), sotscresolves it by walking up to the rootnode_modules. Neither typecheck script is self-contained: on a fresh checkout with only the package installed, both fail witherror TS2688: Cannot find type definition file for 'bun'.The job therefore installs at the root and then in the package, which is the order
Dockerfilealready uses — root install at:46,agent-computerat:54. That dependency on the root tree is also part of why these scripts went unnoticed: anywhere anyone ran them by hand already had it.Verified against a clean tree with every
node_modulesremoved, in the job's own order: root install, package install, typecheck, both packages green. Removing the rootnode_modulesreproducesTS2688; restoring it passes.ci.ymlgained #117 twenty minutes before this branch was cut. Theimagejob'sif:, its comment, andverify's skipped-is-a-pass semantics are byte-identical here.2. The lockfiles, and why the flag alone would not have worked
This is the part that came out differently from the issue's proposal, and it is worth reading before approving.
Six of seven committed lockfiles were installed with a bare
bun install. Adding--frozen-lockfilelooks like one word per site. It is not:--frozen-lockfilewith no lockfile present is not an error. Bun resolves fresh and exits 0. Every one of these Dockerfiles copied onlypackage.json, so the flag on its own would have produced five checks that can never fail — #112's own complaint, reintroduced by its own fix.So each site copies its lockfile too:
Dockerfile:53-54COPY agent-computer/bun.lock, thenbun install --frozen-lockfileagent-computer/Dockerfile:16-17COPYsupervisor/Dockerfile:16-17agent-bot/Dockerfile:10-11--chown=bun:bunagent-langgraph/Dockerfile:10-11All five build contexts are
.indocker-compose.ymlandbun.lockis not in.dockerignore, so every path resolves. A repo-widegrep "bun install"now shows the flag at every install site in every workflow and Dockerfile; the only bare ones left areREADME.md:91anddocs/development.md:9, which are human setup instructions.The release consequence the issue raises holds: #64 signs provenance for the image digest, and provenance over a tree resolved at build time attests to less than it looks like.
Where it runs
pull_request/push: main/workflow_call, roughly two runner-minutes of cheap work besidestatic.Boundary and audit
Changelog
No entry.
CHANGELOG.mdis for when a deployment behaves differently, and #117 — the last CI-only change — added none either. The image's dependency tree is now the committed one rather than one resolved at build time, which is reproducibility rather than behaviour. Flagging rather than deciding silently: happy to add a line under### Changedif you would rather have it recorded.Proof
The new checks fail when they should. Each injection reverted afterwards:
const __ciProbe: number = "not a number"inagent-computer/src/index.tserror TS2322, exit 2supervisor/src/index.tserror TS2322, exit 2hono ^4.10.0 → ^3.0.0insupervisor/package.jsonerror: lockfile had changes, but lockfile is frozen, exit 1"left-pad": "^1.3.0"added, absent from the treepackage.jsonuntouchedOne false positive recorded so nobody repeats it: adding
"ms": "^2.1.3"tosupervisor/package.jsonpasses, becausemsis already insupervisor/bun.lockas a transitive dependency. Nothing about the resolution changed. Not a hole in the flag.git statusstayed clean through every frozen install, so no lockfile has drifted and no refresh is needed.YAML validated with the repo's own
yamlpackage: jobs parse asstatic, deployables, test, build, migrations, image, verify, andverify.needsis["static","deployables","test","build","migrations","image"].Rest of the suite, against unmodified
mainatf1701d8:bun run test— 918 pass, 8 skip, 112 fail, 1038 tests across 105 files, identical tomain. The 112 are the Postgres integration tests; there is no database and no Docker on this machine, and they fail the same way on a clean checkout, which is what the CItestsjob runs pgvector for.bunx biome lint .— 25 warnings, 1 info, identical tomain. (The lint check reports its findings and exits successfully, which is how #63 got through #75 is the separate change that makes those fail the build.)bun run format:check— clean, 385 files.bun run typecheck— clean.bun run build— clean.tests/workspace.test.tsstill passes, becauseworkspacesis unchanged.I could not build the images — no Docker here — so the Dockerfile edits are verified by running the equivalent frozen install in each directory under bun 1.3.14, confirming each
COPYsource exists, that every context is., and thatbun.lockis not dockerignored. Theimagejob builds the root Dockerfile on afull-ciPR or onmain.Not in this PR
examples/lockfiles.examples/langgraph-bot/bun.lockandexamples/mastra-bot/bun.lockare installed by nothing at all, so there is no site to add a flag to. Enforcing them means a job that installs sample code, which would let example drift break the gate onmain. Both are currently consistent (--frozen-lockfile --dry-runexits 0 in each). Worth its own issue if you want it.agent-botandagent-langgraphin the matrix. Neither has atypecheckscript, so enrolling them means writing tsconfigs first — the separate question the issue already set aside.