build: derive libc verifier scope from optionalDependencies#1172
Conversation
EXPECTED_LIBC was a hand-maintained map duplicating knowledge already in package.json's optionalDependencies. Adding a new linux-* platform required updating both, and #1168 exists because they drifted (arm64-musl is mapped in the CI parity job but absent here). Derive the package list from optionalDependencies and infer expected libc from the napi-rs -gnu/-musl suffix. New linux platforms added to optionalDependencies are now covered automatically. An unknown suffix fails loudly so silent gaps cannot reappear.
|
Claude finished @carlos-alm's task —— View job Reviewing PR: build: derive libc verifier scope from optionalDependenciesTodo List
Root Cause AnalysisProblem Being Solved: Issue #1168 - The Why This Approach: The solution derives the package list dynamically from Risk Assessment: Low risk. This is a build-time validation script with clear error handling and explicit testing in the PR description. Backlog Compliance
Code Quality AnalysisStrengths ⭐⭐⭐⭐
Technical ImplementationThe regex const expectedLibc = LIBC_BY_SUFFIX[match[1]]; // gnu → glibc, musl → muslPotential Concerns (Minor)
Test Integrity ✅The PR author manually validated:
Final Recommendation
Minor suggestions (non-blocking):
|
Greptile SummaryThis PR eliminates the manual
Confidence Score: 5/5Safe to merge — a focused script refactor that removes a maintenance footgun without changing the lockfile validation logic. The change replaces a static map with a derivation from optionalDependencies, keeping all the same lockfile checks intact. The regex is constructed from the same keys as the lookup map, so they are structurally coupled. Edge cases (empty package list, unknown suffix) are handled explicitly and exit non-zero. No data is written, no external systems are touched, and the test plan confirms the happy path and both failure modes. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Start] --> B[Read package.json]
B --> C[Read package-lock.json]
C --> D[Filter optionalDependencies\nfor @optave/codegraph-linux-*]
D --> E{linuxPackages\nempty?}
E -- Yes --> F[Log: skipping\nexit 0]
E -- No --> G[For each package:\nmatch LINUX_PKG_PATTERN]
G --> H{Regex\nmatch?}
H -- No --> I[Collect into\nunknownSuffixes]
H -- Yes --> J[Look up expectedLibc\nfrom LIBC_BY_SUFFIX]
J --> K{Entry in\nlockfile?}
K -- No --> L[Collect into failures:\nmissing from lockfile]
K -- Yes --> M{libc field\ncorrect?}
M -- No --> N[Collect into failures:\nwrong libc value]
M -- Yes --> O[OK]
I --> P{unknownSuffixes\nnon-empty?}
L --> P
N --> P
O --> P
P -- Yes --> Q[Print unknown suffix error\nwith fix instructions\nexit 1]
P -- No --> R{failures\nnon-empty?}
R -- Yes --> S[Print libc failures\nexit 1]
R -- No --> T[Log: OK N packages checked\nexit 0]
Reviews (8): Last reviewed commit: "Merge branch 'main' into fix/1168-libc-v..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (unknownSuffixes.length > 0) { | ||
| console.error( | ||
| 'package-lock.json libc discriminator check cannot infer expected libc for:\n', | ||
| ); | ||
| for (const name of unknownSuffixes) console.error(` - ${name}`); | ||
| console.error( | ||
| `\nExtend LIBC_BY_SUFFIX in ${import.meta.url.replace('file://', '')}\n` + | ||
| 'to cover the new suffix (current rule: -gnu → glibc, -musl → musl).', | ||
| ); | ||
| process.exit(1); | ||
| } |
There was a problem hiding this comment.
The error message tells the developer to "Extend LIBC_BY_SUFFIX", but that alone is not sufficient to fix the problem. A future suffix like gnueabihf would also require updating LINUX_PKG_PATTERN (specifically the (gnu|musl) capture group), because the regex is the gatekeeper that determines what reaches the map lookup — extending the map alone leaves the package permanently matched as an unknown suffix and exiting 1. A developer following only the printed instructions would be confused when adding a new map entry doesn't resolve the failure.
There was a problem hiding this comment.
Fixed in 4af75ec — derived LINUX_PKG_PATTERN from Object.keys(LIBC_BY_SUFFIX) so the regex and the map can never drift apart. Adding a new suffix to the map is now genuinely the only required edit (the regex picks it up automatically). The error message has been updated to reflect this and to list the current rules from the map dynamically.
Greptile flagged that the unknown-suffix error message told developers to extend LIBC_BY_SUFFIX, but the regex (gnu|musl) capture group is the actual gatekeeper — adding only a map entry would leave the package permanently unmatched. Derive LINUX_PKG_PATTERN from Object.keys(LIBC_BY_SUFFIX) so the regex and map can never drift apart, and update the error to reflect that adding a map entry is now genuinely the only required change. Also switch the script path interpolation to use fileURLToPath(import.meta.url) instead of string-replacing the file:// scheme, per Claude's review note.
|
Thanks for the review! Addressed both points in 4af75ec:
|
1 participant
Summary
scripts/verify-lockfile-libc.mjspreviously hand-maintainedEXPECTED_LIBC, duplicating the list of linux native packages already declared inpackage.json'soptionalDependencies. Adding a new platform required updating both — and follow-up: extend lockfile libc verifier when linux-arm64-musl is added to optionalDependencies #1168 exists precisely because they drifted (@optave/codegraph-linux-arm64-muslis mapped inci-install-native.mjsand the CI parity job, but was absent fromEXPECTED_LIBC).optionalDependenciesand infers the expected libc value from the napi-rs-gnu/-muslsuffix. New linux platforms added tooptionalDependenciesare now covered automatically with no script edit.linux-armhf-gnueabihf) fails loudly with a pointer to the inference rule, so silent gaps cannot reappear.Test plan
node scripts/verify-lockfile-libc.mjson the current lockfile →package-lock.json libc discriminators OK (3 linux package(s) checked).@optave/codegraph-linux-arm64-musltooptionalDependenciesand re-ran — verifier reportsmissing from package-lock.jsonand exits 1 (proves the script picks up new entries without code changes).@optave/codegraph-linux-armhf-gnueabihf— verifier reports the suffix is unknown, points atLIBC_BY_SUFFIX, and exits 1.lintjob remains green on this branch.Closes #1168