refactor(cli): one selector for bun cells, one output mode for the matrix - #247
Merged
Conversation
…trix Quality pass over the previous two commits. Four independent reviews converged on the same defect, which is the substantive fix here. **The by-name selection survived in the file that owns the table.** The earlier commit removed `matrix.combo == 'bun'` from the workflow but left `COMBOS.filter(c => c.name === "bun")` in the script, so `scaffold-check --runtime bun` ran one of the two bun cells while CI ran both -- `bun-pm` was reachable locally only by naming it explicitly. That breaks the guarantee the file's own header makes, that a green local run means a green CI run by construction. Both consumers now select on `needsBun` through a single derived `BUN_COMBOS`, and the singular wording in the help and error text is fixed. **`--list` emits descriptors instead of names**, which removes more code than it adds: the `--list-needs-bun` mode, its flag, its help line, the second job output, the second echo, and the `contains(fromJSON(...))` expression all go away, and the condition becomes a plain `if: matrix.combo.needsBun`. It also removes a latent trap -- had the `fromJSON` ever been dropped, `contains` would have degraded to a substring match where `bun` also matches `bun-pm`. Job names are unchanged, so branch protection is unaffected. **The new README test guarded the wrong invariant.** It passed a literal to `transformBase`, which never calls `resolveConfig`, so it pinned the *type union* while `--package-manager bun` actually depends on the *runtime array*. Removing "bun" from `PACKAGE_MANAGERS` would have left it green. Replaced with an assertion next to the existing enum-rejection test, and verified by deleting the value: the new assertion fails, the old test would not have. Also: added the `concurrency` group every other workflow in the repo has (this one let superseded runs finish, ~370 job-seconds each); trimmed the buf-packaging archaeology out of the `pnpm-workspace.yaml` comment, since that detail goes stale exactly like the wrong comment it replaced; and dropped the separate changeset -- `connectum init` is unreleased and its own changeset already enumerates bun, so a second entry would read as if bun were added to a shipped feature. Deliberately not done: deriving `PackageManager` from a const tuple. It closes a real gap, but the same gap exists in all five option lists and the current shape is the established pattern -- changing one of five would trade a small hole for an inconsistency. Worth its own change.
github-actions
Bot
added
type:chore
📝 WalkthroughWalkthroughThe scaffold checker now reports Bun requirements with each combination. The GitHub Actions matrix consumes this metadata for conditional setup and validation. Scaffold tests accept Bun, and pnpm build approval documentation is updated. ChangesScaffold matrix Bun selection
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Discovery as scaffold-check.mjs
participant Matrix as GitHub Actions matrix
participant Bun as Bun setup
participant Checker as scaffold checker
Discovery->>Matrix: Emit combination name and needsBun
Matrix->>Bun: Set up Bun when needsBun is true
Matrix->>Checker: Pass matrix.combo.name for validation
Possibly related PRs
🚥 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 |
@connectum/auth
@connectum/cli
@connectum/core
@connectum/events
@connectum/events-amqp
@connectum/events-kafka
@connectum/events-nats
@connectum/events-redis
@connectum/healthcheck
@connectum/interceptors
@connectum/otel
@connectum/protoc-gen-catalog
@connectum/reflection
@connectum/test-fixtures
@connectum/testing
commit: |
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
1 participant
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.
Summary
Quality pass over #246, which merged while this was being prepared. Four independent reviews (reuse / simplification / efficiency / altitude) converged on the same defect: the by-name selection of bun cells survived in the script that owns the combination table. The diff removes more than it adds — 30 insertions, 64 deletions.
Type of change
Test plan
pnpm build && pnpm typecheck && pnpm testpass locallypnpm lintpasses locallyexamples/exercised (if applicable) — N/A, no example touched; instead both bun cells were run locally vianode scripts/scaffold-check.mjs --combo bun,bun-pm(bun19.6s ok,bun-pm7.8s ok), which is the path this PR changes."bun"fromPACKAGE_MANAGERSmakes the new test fail (137 tests, 1 failure); restoring it passes. The test it replaces stayed green through that deletion.node scripts/scaffold-check.mjs --listoutput inspected — descriptors{name, needsBun}for all 10 combinations.init bun-pm, …) so required checks in branch protection are unaffected.Parity coverage
scripts/scaffold-check.mjs, the scaffold-matrix workflow), one code comment, and unit tests. No server, transport, interceptor or protocol code is changed, so no observable RPC behaviour differs on either transport.Related issues / changes
--package-manager bun#246 (--package-manager bun), which merged before this pass was ready.--package-manageraccepts bun docs#73.Detail
The by-name selection survived in the file that owns the table
#246 removed
matrix.combo == 'bun'from the workflow but leftCOMBOS.filter(c => c.name === "bun")in the script. Soscaffold-check --runtime bunran one of the two bun cells while CI ran both;bun-pmwas reachable locally only by naming it explicitly.That breaks the guarantee the file's own header makes — that a green local run means a green CI run by construction. Both consumers now select on
needsBunthrough a single derivedBUN_COMBOS, and the singular wording in the help and error text is corrected.--listemits descriptors instead of namesThis removes more code than it adds: the
--list-needs-bunmode, its flag, its help line, the second job output, the secondecho, and thecontains(fromJSON(...))expression all disappear. The condition becomes a plainif: matrix.combo.needsBun.It also closes a latent trap: had the
fromJSONever been dropped in an edit,containswould have degraded to a substring match, wherebunalso matchesbun-pm.Measured, so nobody re-optimises it later: the two Node spawns this replaces cost 0.089s combined — 0.4% of the
discoverjob. This is a simplification, not a speed-up.The new test guarded the wrong invariant
The README test passed a literal to
transformBase, which never callsresolveConfig— so it pinned the type union while--package-manager bunactually depends on the runtime array.Also
concurrencygroup every other workflow in this repo has — this one let superseded runs finish, ~370 job-seconds each.pnpm-workspace.yamlcomment (11 lines → 6). That detail goes stale exactly like the wrong comment feat(cli): accept--package-manager bun#246 replaced.connectum initis unreleased and its own changeset already enumeratesbun.Deliberately not done
Deriving
PackageManagerfrom a const tuple. It closes a real gap — a union member forgotten in the validation array is accepted silently — but the same gap exists in all five option lists, and the current shape is the established pattern. Changing one of five trades a small hole for an inconsistency. Worth its own change.Two further recommendations were declined with reasons: folding
bun-pminto the neighbouring cell (would reintroduce by-name selection to save 21 job-seconds) and turningif (packageManager === "pnpm")into a capability table (the altitude review itself concluded the current depth is right).