Repository navigation
Conversation
The EQL workspace depends by path on root-workspace crates: tests/encryption
on stack-kms and stack-encrypt, eql-bindings on stack-encrypt. Those crates
declare recipher, cllw-ore and vitaminc as `{ workspace = true }`, so their
requirements come from the ROOT Cargo.toml even inside the EQL workspace.
The /packages/eql Dependabot entry ignored none of them, so its monthly PR
(#1205) bumped them in the EQL lock and, to satisfy the root's exact pins,
rewrote the root Cargo.toml without updating the root Cargo.lock or the Go
guest's recipher pin. Every root `--locked` build and the guest failed.
The entry is now `versioning-strategy: lockfile-only`, so it cannot edit
any manifest, and it ignores the crates the root entry moves by hand
(recipher, cllw-ore, zerokms-protocol, vitaminc*), so its lockfile does not
move them apart from the root's.
scripts/__tests__/dependabot-eql-inherited-ignores.test.mjs fails when the
entry stops being lockfile-only, when the root entry ignores a crate it
does not, or on a stale exemption. It derives the path dependencies from
the tracked EQL manifests, and fails if they disappear rather than passing
vacuously.
Closes #1207
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
cipherstash-agent
left a comment
There was a problem hiding this comment.
Review attestation: automated /code-review medium + /cross-check-review-findings on b1ab8c8.
Result: no critical or blocking findings.
Checked by reading the diff against the repo (the new test was not run, see the last bullet):
- Ignore list matches the root entry. The root cargo entry (
.github/dependabot.yml:236-276) ignorescts-common,zerokms-protocol,recipher,cllw-oreandvitaminc*. The EQL entry already ignoredcts-common; this PR adds the other four, so the two lists now agree. - Manifest edits are blocked.
versioning-strategy: lockfile-onlyis a valid option for the cargo ecosystem and stops the EQL entry from editing the rootCargo.toml, which caused the #1205 and #1207 breakage. - The test's inputs resolve.
js-yamlis a root devDependency, andscripts/__tests__/lib/repo-root.mjsexists.- The
membersregex matches the rootCargo.tomllayout. - The path-dependency scan finds
stack-encryptandstack-kmsfromtests/encryptionandeql-bindings. The vacuity guard is therefore not trivially green. - Ignore rules that carry
update-types(the semver-major rule) are correctly not counted as ignores.
- No changeset needed. The change is CI and tooling only, with no published package or
skills/impact.AGENTS.mdalready liststests/encryptionas an EQL workspace member, so there is no meta-file drift. - Not verified here.
node_modulesis not installed in this run, so the new test was not executed. Please rely on CI for that.
Nit, non-blocking: the rewritten comment line at .github/dependabot.yml:142-143 (tests/encryption) and its own Cargo.lock. It needs its own entry because Dependabot's cargo) runs well past the wrapping of the lines around it.
Generated by Claude Code
freshtonic
left a comment
There was a problem hiding this comment.
Review of b1ab8c8. No blocking defects.
Checks. All checks pass. I ran the tests again in a worktree at this head:
scripts/vitest.config.mjs: 76 files, 1357 tests pass. The new file gives 5 of 5.e2e/tests/supply-chain.e2e.test.ts: 29 of 29 pass.- The other repo tests that read
.github/dependabot.yml(scripts/__tests__/impact-workflow.test.mjs,scripts/__tests__/musl-build-image.test.mjs) also pass.
Correctness:
- Rationale. The rationale is correct. #1205 changed only the root
Cargo.toml(+2 -2) andpackages/eql/Cargo.lock. lockfile-onlyis valid for cargo. The GitHub Dependabot options reference listscargoas an ecosystem that supportsversioning-strategy.- Directories and ecosystems do not change. No manifest loses its entry.
- The new ignores have an effect.
packages/eql/Cargo.lockresolvesrecipher0.3.1,cllw-ore0.5.0,zerokms-protocolandvitaminc0.5.1 throughstack-encryptandstack-kms, so each new ignore stops a real update.
Suggested follow-up PR:
- Write the security-update effect in the comment. The options reference marks
versioning-strategyas an option for both version updates and security updates. Thus, after this PR, the EQL entry also drops a security fix that needs a manifest edit. The new comment at.github/dependabot.yml:153-158does not say this. The earlier text at:163-165("Security fixes are unaffected") can now cause confusion. Give the same caveat that the ignore block gives at:193-195, and refer to osv-scanner. prettypleaseloses its Dependabot updates.packages/eql/crates/eql-codegen/Cargo.toml:16pinsprettyplease = "=0.2.37". Withlockfile-only, Dependabot cannot propose any change to it. If the pin is intentional (stable codegen output), write that in the manifest. If not, change it to a caret requirement.- The protect-ffi entry has the same risk, one level down.
languages/typescript/packages/protect-ffi/crates/protect-ffi/Cargo.toml:37depends by path oneql-bindings, which is in the EQL workspace. The protect-ffi cargo entry (.github/dependabot.yml:62-137) does not uselockfile-only. Thus a protect-ffi Dependabot PR can editpackages/eql/crates/eql-bindings/Cargo.tomlwithout a change topackages/eql/Cargo.lock. The risk is small today, becauseeql-bindingsuses plain caret requirements and protect-ffi does not enable thestack-encryptfeature. But the new test examines only the EQL-to-root edge. Think about making the test general: every cargo entry with a path dependency outside its own workspace must belockfile-only.
The EXEMPT reason check (CodeRabbit) and the line-wrap nit at :142-143 (earlier review) are still open. I do not repeat them.
Summary
Last month's Dependabot PR for the EQL workspace (#1205) broke CI for the whole repository. EQL is the SQL library this repo installs into customers' Postgres databases; it has its own Cargo workspace under
packages/eql. To bump two crates there, Dependabot rewrote the root workspace'sCargo.toml, but didn't update the rootCargo.lockor the Go guest that pins the same crate. Every root--lockedbuild and the Go guest job then failed.This PR stops the EQL Dependabot entry from ever editing a manifest, makes it ignore the crates the root workspace moves by hand, and adds a test so the two Dependabot entries can't drift apart again.
Changes
.github/dependabot.yml, the/packages/eqlcargo entry:versioning-strategy: lockfile-only: the entry can no longer edit anyCargo.toml, the root one included. An update that would need a manifest change is dropped rather than proposed.recipher,cllw-ore,zerokms-protocolandvitaminc*. These are the root entry's hand-moved crates that the EQL lockfile also resolves.cts-commonwas already ignored.tests/encryptiondepends by path onstack-kmsandstack-encrypt, andeql-bindingsonstack-encrypt. Those crates declare these dependencies{ workspace = true }, so the requirements come from the rootCargo.toml. I also fixed the stale "five members" count in the entry's header, which is six now,tests/encryptionincluded.New
scripts/__tests__/dependabot-eql-inherited-ignores.test.mjs, which fails when:lockfile-only;It reads the path dependencies from the tracked EQL manifests, so a new one is covered automatically.
Verification
main, listing the four missing crates. With the change, 5 of 5 pass.versioning-strategyfails the lockfile-only test;cllw-oreignore fails the comparison, namingcllw-ore;pnpm exec vitest run --config scripts/vitest.config.mjs: 76 files, 1357 tests pass.e2e/tests/supply-chain.e2e.test.ts, which also readsdependabot.yml: 29 of 29 pass.pnpm run code:check: 0 errors.lockfile-only. The next monthly EQL run will show it.Related
Review notes
lockfile-onlybeyond what Dependabot EQL PRs edit the root workspace's exact pins and break CI — ignore recipher, cllw-ore and vitaminc on the /packages/eql entry #1207 asked for, following review. The ignores alone covered only the root entry's named crates. Any other inherited root requirement would still have been raised in the root manifest, andlockfile-onlycloses that too.dependency-namepatterns as exact strings, for examplevitaminc*againstvitaminc*. This is deliberate: it asks for the same entry in both lists rather than reasoning about glob coverage.Summary by CodeRabbit