fix: resilient protoc resolution + add CI workflow#33
Merged
Conversation
Context: build-proto.mjs hard-coded the grpc-tools bundled protoc path. When the grpc-tools postinstall download fails, that binary is absent and `npm run protos` exits with status 127, leaving the proto TS modules ungenerated and ~431 tsc errors. Approach: replace the rigid constant with resolveProtoc(), a fallback chain. Changes: - Try the bundled grpc-tools/bin/protoc(.exe) first, gated on an executable-access check, to keep version consistency. - Fall back to a protoc found on PATH via which/where. - Fall back to a bare protoc if it answers --version. - Otherwise print a clear remediation message and exit. Impact: npm run protos now succeeds with a system protoc; no other proto pipeline behaviour changes.
Context: .github/workflows only had pack-cli.yml, triggered on v*-cli tags. No workflow validated push or pull requests. Changes: - Add ci.yml triggered on push to master and on pull_request. - build job: install, generate protos, check-types, lint (blocking). - test job: run the mocha unit suite with continue-on-error, since it is flaky in CI and should signal without gating. Impact: every push and PR now gets typecheck and lint coverage.
There was a problem hiding this comment.
Pull request overview
This PR makes proto code generation more resilient when grpc-tools’ bundled protoc binary is missing, and introduces a new CI workflow to run proto generation, typechecking, and linting on pushes/PRs.
Changes:
- Add
resolveProtoc()fallback chain inscripts/build-proto.mjs(bundledgrpc-tools→ PATH lookup →protoc --version). - Add
.github/workflows/ci.ymlto runnpm run protos,check-types,lint, plus non-blocking unit tests on push/PR.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| scripts/build-proto.mjs | Adds fallback resolution for protoc to avoid hard failures when the bundled binary is missing. |
| .github/workflows/ci.yml | Adds CI workflow for proto generation, typecheck, lint, and non-blocking unit tests on push/PR. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+67
to
70
| const PROTOC = resolveProtoc() | ||
|
|
||
| const PROTO_DIR = path.resolve("proto") | ||
| const TS_OUT_DIR = path.resolve("src/shared/proto") |
Comment on lines
+28
to
+30
| - name: Generate Protocol Buffers | ||
| run: npm run protos | ||
|
|
Patches from the critic review of PR #33: - ci.yml: drop the redundant "Generate Protocol Buffers" step in the build job — `check-types` already runs `npm run protos`. - ci.yml: add a concurrency group (cancel superseded runs), `permissions: contents: read`, and `timeout-minutes` on both jobs. - ci.yml: rename the test job "Unit Tests (non-blocking)" so the continue-on-error behaviour is explicit rather than implied. - build-proto.mjs: warn when falling back to a system protoc so a version divergence from grpc-tools is visible in the log.
The first CI run exposed a pre-existing bootstrap defect: the `cli` workspace has a `prepare` script that builds (and runs tsc) during `npm install`, which fails before protos exist. Approach: install with `--ignore-scripts` so the cli prepare build does not run at install time, install a system protoc (grpc-tools' postinstall is skipped too), then generate protos and run the checks in the correct order. Changes: - Both jobs: `npm install --ignore-scripts`. - Both jobs: add an "Install protoc" step (apt protobuf-compiler) so build-proto.mjs has a protoc to fall back to.
Context: npm --ignore-scripts does not suppress prepare for local workspace packages (known npm v7+ behaviour). The cli workspace prepare runs tsc which fails before protos are generated. Approach: split install into phases so protos exist before cli prepare runs. Changes: - Phase 1: npm install --workspaces=false --ignore-scripts (root deps only, cli workspace not linked, no prepare triggered) - Phase 2: sudo apt-get install protobuf-compiler + npm run protos - Phase 3: npm install --workspace=cli (prepare now succeeds) - Phase 4: cd webview-ui && npm install (needed for webview-ui tsc) - Same fix applied to the test job for consistency Impact: Typecheck & Lint job can now install and typecheck without hitting the chicken-and-egg proto/prepare deadlock.
Remove the unused `os` import in ailiance-memory.test.ts (dead code left from a refactor that replaced os.homedir() mocking with vi.mock). Replace the Logger.warn/console.warn fallback pattern in ailiance-memory.ts with a direct Logger.warn call. The fallback was unnecessary: Logger is always available in this context, and biome's no-console rule flags the console.warn at error level.
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
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.
Problèmes
1 —
npm run protoscasséscripts/build-proto.mjsfigeait le chemin deprotocsur le binaire bundlénode_modules/grpc-tools/bin/protoc. Ce binaire est téléchargé en postinstall pargrpc-tools; quand le téléchargement échoue, le binaire est absent etnpm run protossort enstatus: 127. Aucun fichier proto TS n'est généré, ce qui provoquait ~431 erreurstscCannot find module '@shared/proto/...'/@generated/....2 — pas de CI sur push/PR
.github/workflows/ne contenait quepack-cli.yml, déclenché sur les tagsv*-cli. Aucun workflow ne vérifiait typecheck/lint/tests sur les push et pull requests.Corrections
build-proto.mjs— résolutionprotocavec fallbackRemplacement de la constante rigide par
resolveProtoc():grpc-tools/bin/protoc(.exe)s'il existe et est exécutable (priorité, cohérence de version) ;protoctrouvé sur lePATH(which/where) ;protocqui répond à--version;protocou réinstallergrpc-tools) + exit.Aucune autre étape du pipeline proto n'est modifiée. Vérifié :
npm run protosréussit avec leprotocsystème, et les erreurstscpassent de 431 à 0..github/workflows/ci.ymlNouveau workflow déclenché sur
push(master) etpull_request:build(bloquant) :npm install→npm run protos→npm run check-types→npm run lint;test: suite unitaire mocha (test:unit) encontinue-on-errorcar fragile en CI — elle remonte un signal sans bloquer le typecheck/lint.Runner
ubuntu-latest, Node 20,actions/setup-node@v4aveccache: npm, cohérent avecpack-cli.yml.