fix(ci)!: build the bundle before packing, and make the caches stop lying - #13
Merged
Conversation
CI ran `npx tsc` as its build step and then packed that tree. tsc emits the
per-module dist; it does not run esbuild, so dist/krm-stream.js was never
built — and release.yml publishes CI's exact bytes rather than rebuilding,
by design.
The next release would therefore have published a package whose exports map
advertises
"./bundle": { "default": "./dist/krm-stream.js" }
pointing at a file that is not in the tarball. Every
`import "@configbutler/krm-stream/bundle"` fails with ERR_MODULE_NOT_FOUND,
in a release that passed CI green — because nothing in the pipeline imports
the published artifact. The whole point of the ./bundle work, silently
absent, plus a broken exports entry that is worse than not shipping it at
all.
Reproduced locally against the real sequence (npm ci → npx tsc → npm pack):
the tarball contains dist/index.js and no dist/krm-stream.js.
Two changes:
- the build step is `npm run build`, which is tsc AND the esbuild flatten.
One source of truth with package.json.
- a guard after `npm pack` asserts that every entry point in the exports
map is actually IN the tarball. A comment saying "keep these in step" rots;
this fails the build. Verified it rejects the bundle-less tarball and
passes the correct one.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe change expands local and CI linting, centralizes Task-based verification and client packaging, adds CodeQL analysis, and documents security reporting, vulnerability scope, supported versions, and workflow credential handling. ChangesCI quality and packaging
Security analysis and policy
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…ying
Three bugs, one disease: a step that cannot tell whether its own output
exists.
# 1. CI packed a bundle it never built
CI built with `npx tsc` and packed that tree. tsc emits the per-module dist;
it does not run esbuild, so dist/krm-stream.js was never built — and
release.yml publishes CI's exact bytes rather than rebuilding, by design.
The next release would have published a package whose exports map advertises
"./bundle": "./dist/krm-stream.js" pointing at a file NOT IN THE TARBALL.
Every `import "@configbutler/krm-stream/bundle"` fails with
ERR_MODULE_NOT_FOUND, in a release that passed CI green — because nothing in
the pipeline ever imports the published artifact.
# 2. `task build-client` could not see its own outputs
rm dist/krm-stream.js && task build-client
→ "Task is up to date" (nothing rebuilt)
`generates: ["dist/**/*.js"]` is only a checksum key; Task never asks whether
the files exist. Outputs are now named individually AND guarded by a `status:`
that probes the filesystem, so a missing artifact rebuilds.
# 3. npm install prunes node_modules across branches
node_modules is shared mutable state that git does not track, and `npm install`
reconciles it to the CURRENT branch's package.json. Running any task from a
branch predating a devDependency DELETES it; switching back leaves a build
dying on `sh: 1: esbuild: not found`, a message that points at nothing.
_client-deps/_example-deps now use `npm ci` — the tree is a pure function of
two committed files — fingerprinted on those files and status-guarded on the
binaries actually being present.
# One definition of a thing
`task pack-client` builds, packs, and ASSERTS every entry point in the exports
map is in the tarball. CI runs that same task instead of a hand-copied shell
line, so the local gate and the release gate cannot drift again. `task verify`
is the whole gate in CI's order.
The guard caught a bug in itself on its first run: `tar | grep -q` under
pipefail reports FAILURE on a match, because grep exits early and tar takes
SIGPIPE. It lists once into a variable now. Verified in both directions — it
passes a good tarball and rejects a tsc-only one.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
sunib
changed the title
…, and lint the two unlinted languages
Scorecard was already wired correctly — pinned actions, least-privilege
tokens, OIDC, publish_results. The workflow was never the problem. The repo's
posture was: 4.8/10, with `main` completely unprotected.
# No PAT, on purpose
scorecard-action offers `repo_token`, and it is the thing people reflexively
add. It is needed for exactly one check: reading CLASSIC branch protection.
`main` is now protected by a repository RULESET, which the default GITHUB_TOKEN
can read — so Branch-Protection scores without one.
That is the better posture, not a shortcut. A PAT with repo scope is a
long-lived write credential, stored as a secret, in a workflow whose whole job
is to attest that the supply chain is sound. Not creating it is worth more than
the check it would buy.
The ruleset (applied via the API, id 18924161): no deletion, no force-push,
PRs required, CI required. Zero required approvals — a solo maintainer cannot
approve their own PR, and a bypass actor would have scored worse than the rule
is worth. Code-Review stays 0 until there is a second contributor, which is
honest rather than papered over.
# The two languages nobody counts as languages
Half this repo's supply-chain surface is a workflow YAML and a Dockerfile, and
they were the only files here with no linter pointed at them. That is not a
coincidence: the bug that packed a bundle it had never built lived in a
workflow, and every Go and TypeScript check in the pipeline was blind to it.
actionlint also runs shellcheck over every `run:` block — which is exactly where
the `tar | grep -q` under pipefail bug hid.
- actionlint + hadolint in the devcontainer and in CI (new `hygiene` job,
now a required check). Versions are read FROM the Dockerfile's ENV block in
CI, so there is one definition and they cannot drift.
- .hadolint.yaml ignores DL3008 with the reason written down: the devcontainer
is a tooling image, distro packages are deliberately unpinned, and the
versions that matter are pinned in ENV. The `# hadolint ignore=` comments
already in that Dockerfile have been talking to a linter that was not there.
# The rest
- SECURITY.md: private reporting, and — more usefully — what counts as a
vulnerability HERE. Every interesting failure in this library is a
disclosure failure: a redacted value reaching a browser, a subscriber served
another tenant's object from the shared cache, a patch writing a field the
browser was never shown.
- CodeQL (Go + TypeScript, security-extended). The gateway parses untrusted
input from a browser and hands back objects it has projected; that is the
shape where taint tracking earns its runtime.
Deliberately NOT done: pinning the golangci-lint installer in the devcontainer.
It is a dev-only image from a source we already trust, and it is not in the
supply chain of anything this repo publishes. Pinned-Dependencies stays at 7.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
…ms to
The hygiene job I added in the previous commit failed on its first CI run,
having passed locally on the same tree. That gap is the point.
actionlint SHELLS OUT to shellcheck to check every `run:` block. If shellcheck
is absent it silently skips that half of its job and still exits 0. The
devcontainer had no shellcheck, so `task lint` was green on a workflow CI
rejected — a local gate that is not weaker than CI, but LYING to you. Which is
the same disease as everything else in this PR: a check that reports success
about a thing it never looked at.
shellcheck is now installed in the devcontainer. Verified by reintroducing the
bug: local actionlint now reports the same SC2024 CI did.
The two findings it was hiding:
- SC2024 in my own install step: `sudo tar … > /tmp/actionlint`. sudo does
not affect a redirect — the shell opens the file, unprivileged. Harmless
here, wrong everywhere it matters.
- SC2016 in the `no module may carry a replace` check: backticks inside single
quotes, which are literal prose in an error message and exactly right.
Disabled inline with the reason, rather than by loosening the linter.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
They explained the why at three times the length it needed. Kept the reason, cut the sermon, dropped the em-dashes. The Taskfile alone goes from 98 added comment lines to 59. The one quoted back at me is the example: a two-line explanation of why backticks inside single quotes are correct is now one line that says so. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
272-281: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider delegating the CI test and lint steps entirely to Task in the future.
Since
task pack-clientinternally relies on_client-deps(which executesnpm ci), thenode_modulesdirectory gets seamlessly recreated during the Task execution. This makes the earlier- run: npm cistep inside this job slightly redundant, though completely harmless.While delegating the test and lint steps directly to Task (e.g.,
task test-client lint-client pack-client) would remove this duplication and align CI identically with the local environment, doing so today would fail:task test-clientdepends onfixtures. Without a warmed.task/cache available in CI, Task will attempt to regenerate the fixtures (which requires Go) within this Node-only runner.The current workflow correctly and safely bypasses the Go dependency by invoking the underlying Node tools directly. You might optionally refactor the Taskfile later (e.g., by adding a filesystem
statuscheck tofixturesor splitting the testing dependency graph) to allow CI to rely exclusively on Task without needing a multi-language runner.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 272 - 281, Keep the current CI workflow’s direct Node-based test and lint commands and do not delegate them to Task yet, because task test-client transitively requires fixtures and may invoke Go generation on this Node-only runner. Leave the existing npm ci step unchanged for now; any future Taskfile changes to make fixtures safely reusable in CI are outside this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 272-281: Keep the current CI workflow’s direct Node-based test and
lint commands and do not delegate them to Task yet, because task test-client
transitively requires fixtures and may invoke Go generation on this Node-only
runner. Leave the existing npm ci step unchanged for now; any future Taskfile
changes to make fixtures safely reusable in CI are outside this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1a92c807-9377-4a3f-82f9-9a4034f1971f
📒 Files selected for processing (7)
.devcontainer/Dockerfile.github/workflows/ci.yml.github/workflows/codeql.yml.github/workflows/scorecard.yml.hadolint.yamlSECURITY.mdTaskfile.yml
The README opened with two paragraphs of KRM theory before the reader could
tell whether the library was for them. For the audience this project is
actually trying to reach, a frontend or backend developer who is not a
Kubernetes expert, that is the wrong first page.
- "Is this for you?" up top: yes-if / probably-not-if, in plain terms. It
says out loud that a generic three-way merge library is a different thing,
that Headlamp is the answer if you want a dashboard, and that the write
path belongs to your application.
- KRM in one line, then a link to the frontend glossary. The theory did not
need two paragraphs to earn its place.
- "Start here" now leads with the BROWSER half, because that is the half a
frontend developer owns and the old order made them read Go first. The
snippet is real: it typechecks against the actual store API, including the
StreamChange uid.
- Badges: CI, Scorecard, CodeQL, npm version, zero runtime deps, Go,
TypeScript, license, issues. All verified to render.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Shorter tagline, "What is KRM?" as the heading, and the glossary pointer cut to one line.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Started as "CI would publish a broken package." Ended as three bugs with one disease: a step that cannot tell whether its own output exists.
1. CI packed a bundle it never built
CI built with
npx tscand packed that tree.tscemits the per-moduledist/; it does not run esbuild, sodist/krm-stream.jswas never built — and release.yml publishes CI's exact bytes rather than rebuilding, deliberately.So the next release publishes a package whose exports map says:
Every
import "@configbutler/krm-stream/bundle"fails withERR_MODULE_NOT_FOUND, in a release that passed CI green — because nothing in the pipeline ever imports the published artifact. Worse than not shipping the bundle: a missing feature is invisible, a broken exports entry is a runtime failure for the first person who trusts the README.Reproduced against the real sequence (
npm ci→npx tsc→npm pack): tarball hasdist/index.js, no bundle. Mynpm pack --dry-runin #8 passed only because I'd built locally first — a clean checkout never would have.2.
task build-clientcould not see its own outputsgenerates: ["dist/**/*.js"]is only a checksum key. Task never asks whether the files exist, so a half-deleteddist/survives a build. Same bug as CI, one layer down.3.
npm installprunesnode_modulesacross branchesnode_modulesis shared mutable state that git does not track, andnpm installreconciles it to the current branch'spackage.json. Run any task from a branch that predates a devDependency and it is silently deleted; switch back to main and the build dies onsh: 1: esbuild: not found— a message that points at nothing. This is what happened during the final check on main, and it is exactly the "costs tons of tokens and frustration" failure.The fixes (Taskfile refactored on the gitops-reverser model)
generates— never a glob.status:that probes the filesystem. This is the check a checksum cannot do, and it makes a deleted artifact or a pruned dependency self-heal instead of being skipped.npm ci, nevernpm install(_client-deps,_example-deps): the tree becomes a pure function of two committed files and cannot drift from the branch you are on.task pack-clientbuilds, packs, and asserts every entry point in the exports map is in the tarball. CI now runs that same task instead of a hand-copied shell line.task verifyis the whole gate in CI's order — if it passes, CI passes.task cleanfor when a cache does lie. It deliberately does not touch Go's build cache, which is content-addressed and sound._-prefixed andinternal: true, sotask --liststays the commands people type.Verified, not asserted
node_modulesnpm citsc-only tarballtask verifyfrom a cold cacheThe guard found a bug in itself on its first run.
tar … | grep -qunderset -o pipefailreports failure on a match:grep -qexits the moment it finds the line,tartakes SIGPIPE, and the pipeline inherits tar's status. It failed a perfectly good tarball. It lists once into a variable now — and that is the argument for running a new check against a known-good input before trusting it to gate a release.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Quality Improvements