[AI-94] llm: Evaluate SDK update PRs against the upstream commit range - #22697
[AI-94] llm: Evaluate SDK update PRs against the upstream commit range#22697SaintPatrck wants to merge 8 commits into
Conversation
The bump PR's whole diff is two pinned versions plus the lock file, so a break introduced upstream is found by whichever CI leg happens to catch it, or after merge. Android and iOS already evaluate their bump PR against the sdk-internal commit range behind it. Nothing generated is committed in either repository, so the API record is reconstructed from the published tarballs rather than diffed from git. That also makes a serde rename or an enum wire value renumbering a runtime break with no signal at any call site, which is why the evaluation covers the whole range rather than stopping at the first compile error.
Claude Code validationResult: Pass Validated the two files this pull request adds under Read from the CriticalNone. Major
Minor
What was verified clean
Checks run
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #22697 +/- ##
==========================================
+ Coverage 54.15% 54.84% +0.68%
==========================================
Files 4312 4340 +28
Lines 137026 138276 +1250
Branches 21686 21903 +217
==========================================
+ Hits 74208 75837 +1629
+ Misses 57399 56932 -467
- Partials 5419 5507 +88 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…idated version arg
…lean pack/tar failures
Committing changes skill requires preflight to complete. Described behavior is speculative. Deferring until undesired behavior is actually observed. |
…s are needed The primary session now runs the skill's Identify steps itself, keeping full tool access (including the sticky-comment MCP tool) for the audit trail this workflow exists to produce, and only hands off to the agent persona for Resolve work when there's something to fix. Also grants Skill(evaluating-sdk-internal-updates) explicitly, since the primary session's scoped Skill allowlist didn't already cover it.
This repo's evaluating-sdk-internal-updates skill splits Identify/Resolve at 1-8/9-10, not android's 1-7/8-10 that the prompt had carried over. Also have the subagent invoke the skill itself for the Resolve steps' guardrails, and report back the commit SHA and each finding's disposition, so the sticky comment has something to draw from.
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Reviewed the new Not covered: Skill review did not run — this review path cannot launch Code Review Details
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| Once the subagent has reported back — or immediately, if there was nothing to resolve — update the sticky PR comment yourself, replacing it with the step 8 report, structured as: "## SDK bump evaluated" (old → new version and sdk-internal SHA, commit count), "## Compile-time breaks" (found-and-fixed, or "none found"), "## Runtime considerations" (behavioral, serialization or wire-value notes worth flagging; omit the section if there are none), "## Everything else in range — confirmed safe" (brief, one line per commit or group), and "## Commit" (the SHA, or "no commit — nothing needed fixing"). State whether the verifying type check passed. Always post this, even when the conclusion is that nothing needs fixing — the comment is the audit trail that the check ran. | ||
| claude_args: | | ||
| --model opus | ||
| --allowedTools "Agent,Bash(node .claude/skills/evaluating-sdk-internal-updates/sdk-surface-diff.mjs *),Bash(npm run test:types),Bash(npm run lint:fix),Bash(npm test -- *),Bash(npx prettier *),Bash(gh pr diff:*),Bash(gh pr view:*),Bash(git -C * log *),Bash(git -C * show *),Bash(git -C * diff *),Bash(git add:*),Bash(git commit:*),Bash(git log:*),Bash(git show:*),Bash(git diff:*),Bash(git status:*),Bash(git grep:*),Bash(grep:*),Read,Grep,Glob,Edit,Write,Skill(evaluating-sdk-internal-updates),Skill(writing-client-code),Skill(bitwarden-delivery-tools:committing-changes),mcp__github_comment__update_claude_comment" |
There was a problem hiding this comment.
❓ QUESTION: Is Agent the intended grant here, rather than Task?
Details
The prompt on line 168 tells the primary agent to "dispatch a single subagent on the bitwarden-software-engineer agent", and the tool that does that is named Task in current Claude Code — that is the name used everywhere else in Bitwarden's own configs (bitwarden-code-review's code-review.md and claude-config-validator's validate-ai.md both list Task in allowed-tools). Agent does not appear as a tool name in any of them.
If Task is permission-gated in this non-interactive run, the Resolve half of the workflow — the part commit a1993e91 added — never dispatches and the fix is silently skipped. If it is not gated, the entry is simply dead. Either way Task is the name to grant.
1 participant
🎟️ Tracking
AI-94
📔 Objective
We're adding a skill that evaluates an "Update sdk-internal to" PR against the upstream sdk-internal commit range for compile-time and runtime breaking changes, plus the CI workflow that runs it automatically on the bump bot's PRs. Neither repo commits the SDK's generated TypeScript surface, so the skill ships a
sdk-surface-diff.mjsscript that reconstructs it from two published tarballs and diffs it at member granularity. The workflow scopes the agent's tool and skill grants tightly, since sdk-internal commit content reaches it directly as untrusted context.Process
flowchart TD A[Bump bot opens/updates SDK version bump PR] --> B[Gate: branch + author check] B --> C[Fetch secrets from Key Vault] C --> D[Mint GitHub App token] D --> E[Checkout PR branch] E --> F[Clone sdk-internal sibling repo] F --> G[Install dependencies] G --> H[Run baseline type check] H --> I[Primary agent: evaluate via skill's Identify steps] I --> J{Anything to resolve?} J -- Yes --> M[Dispatch bitwarden-software-engineer subagent: Resolve all findings] M --> K[Post sticky PR comment with report] J -- No --> K K --> L[Push fix commit, if one exists]🧪 Testing
How to test locally
0.2.0-main.971→0.2.0-main.975).Evaluate PR #22518 using the evaluating-sdk-internal-updates skill.## SDK bump evaluated,## Compile-time breaks,## Runtime considerations,## Everything else in range — confirmed safe,## Commit— and that any compile-time break found gets fixed, committed, and verified withnpm run test:types.node .claude/skills/evaluating-sdk-internal-updates/sdk-surface-diff.mjs 0.2.0-main.971 0.2.0-main.975, then again with--commercial.Example output (PR #22518)
SDK bump evaluated
@bitwarden/sdk-internaland@bitwarden/commercial-sdk-internal0.2.0-main.971->0.2.0-main.975. Both pins moved together, solint:sdk-internal-versionsis satisfied.The sdk-internal range is
b15ab94..0dfa89e, five commits:cc7daf103community PR workflows (bitwarden/sdk-internal#1386),8283c9abeCXF import crash on negative timestamps (bitwarden/sdk-internal#1362),fd74c9f56removal ofmake_key_pairandverify_asymmetric_keysfromCryptoClient(bitwarden/sdk-internal#1390),485773cb1aes256-cbc-hmac-sha256-aead as a COSE compat layer for type 2 symmetric keys (bitwarden/sdk-internal#1376), and0dfa89eaebiometric unlock over IPC with concurrent status requests (bitwarden/sdk-internal#1392).The published-surface diff did not run here: it needs
typescript, and our install is blocked by JFrog curation onejs@5.0.1(CVE-2023-29827, no fixed version), with Artifactory not yet mirroring0.2.0-main.975. Surface detection below therefore rests on reading the Rust range per hunk plus call-site greps, so the additive side is less certain than the removals, which are explicit in the diff. The build and test suite did not run for the same reason, so nothing here is test-verified.Compile-time breaks
None, and the type check passed: the CI baseline for this PR reports zero
error TS, everytsc --noEmitleg exited 0, andtsc-strictpassed all 4008 strict files.fd74c9f56does remove five items from the wasm surface: theCryptoClient.make_key_pairandCryptoClient.verify_asymmetric_keysmethods, plus theMakeKeyPairResponse,VerifyAsymmetricKeysRequestandVerifyAsymmetricKeysResponseinterfaces, all three of which carriedderive(Tsify)withinto_wasm_abi/from_wasm_abi. All five have zero call sites in this repo. The eighty-oddmakeKeyPairhits are a false lead: every one islegacyCompatKeyService.makeKeyPair, declared inlibs/legacy-crypto/src/abstractions/legacy-compat-key.service.tsand implemented inlibs/legacy-crypto/src/services/legacy-compat-key.service.tsin pure TypeScript overrsaGenerateKeyPairandwrapDecapsulationKey, so it never reached the removed SDK method.No Tsify enum variants changed in the range.
485773cb1rewrote 589 lines ofpin_lock_system.rs, which owns the Tsify-exposedPinLockTypeandPinUnlockStatus, but every variant reference in that diff sits inside moved or renamed test code and the definitions are untouched. There is no wire-value renumbering anywhere in range.Runtime considerations
CryptoClient.get_key_id_for_symmetric_keynow returns a key ID for v1 user keys where it previously returned nothing. In485773cb1,SymmetricCryptoKey::key_id()changed theAes256CbcHmacKeyarm fromNonetoSome(key.key_id()), deriving the ID as the key's RFC 9679 thumbprint truncated to key ID length. The exported signature is unchanged, which is why nothing failed to compile, and a 64-byte legacy key deserializes toAes256CbcHmacKey, so this covers the v1 user key format rather than an edge case.Both call sites branch on presence.
biometric-persistent-encryption-migration.tscompares the current user key's ID against the storedenrolledKeyIdand treats any mismatch asneedsMigration; a v1 user previously storednulland computednull, so the comparison was stable, and now computes a thumbprint against a storednull. Every v1 user with biometric unlock enabled and a persistent key is flagged as needing migration on first run after this bump, andrunMigrationsre-enrolls throughenrollPersistentandsetBiometricProtectedUnlockKeyForUser.renderer-biometrics.service.tsthen writes the non-null ID, so state converges and later runs are no-ops. The blast radius is a one-time forced re-enrollment rather than lost access or corrupted data: it rewrites a key the client already holds, and no decrypted value crosses a new boundary. What it does cost is an OS keychain write and whatever prompt that carries on Touch ID or Windows Hello, once, for a large share of desktop users at unlock.This is arguably the migration working as documented, since its own class comment lists a key ID appearing as a trigger. Whether a fleet-wide one-time re-enroll is an acceptable rollout cost is a product call, so nothing was changed here; the alternative is backfilling
enrolledKeyIdfor v1 keys to suppress it. Worth noting that CI cannot catch this either way, becausebiometric-persistent-encryption-migration.spec.tsandrenderer-biometrics.service.spec.tsboth stub this call through a factory-formjest.mock("@bitwarden/sdk-internal")and are never checked against the real module. The migration spec already holds both halves of the transition as separate green tests: the mismatch case is now the real-world state, and the case where neither side has a key ID no longer occurs for v1 keys.The COSE compat layer does not change any format we write.
485773cb1adds private-use COSE algorithm-70011and makesas_cose_key_view()returnSomefor AES-CBC-HMAC keys, which on its face risks writing something a V-2 client cannot read. The same commit adds an explicit guard inDataEnvelope::unsealrejectingAes256CbcKeyandAes256CbcHmacKeywithUnsupportedContentFormatbefore it consults the view, andEncStringstill emitsAes256Cbc_HmacSha256_B64for these keys. TheAes256CbcHmacKeyfield restructure from{ enc_key, mac_key }to a single composite{ key }preserves theenc_key || mac_keybyte layout at offsets 0 and 32.CXF import behavior changed benignly.
8283c9abeclamps negativecreationAtandmodifiedAtvalues tonullinparse_cxfbefore deserialization soUtc::now()is used downstream, fixing Google Password Manager exports that emit the Windows FILETIME epoch. Imports that previously failed outright now succeed with current timestamps on the affected items, and payloads needing no change are returned borrowed and untouched. No signature moved and there is no call site to adjust.Everything else in range — confirmed safe
cc7daf103touches only.github/workflows/and no crate code.0dfa89eaechangesipc_client.rsandipc_client_ext.rsinternals only, making the RPC receive loop skip aTypedReceiveError::Typinginstead of failing, because every RPC response shares one payload type name and therefore one topic, so a subscription also sees responses belonging to other in-flight requests. No exported signature moved, so our desktop biometrics mocks cannot have drifted against it, and the effect is strictly better reliability under concurrent biometric status requests.The remainder of
485773cb1is Rust-internal. Itspin_lock_system.rswork is private, with the newclassify_pin_envelopeunexported and the only derive changes addingPartialEq, Eq, which have no Tsify output.SymmetricKeyAlgorithm,Aes256CbcHmacKey,SymmetricCryptoKeyandCoseContentEncryptionAlgorithmcarry noTsifyorwasm_bindgenderive, so the largebitwarden-cryptodiff has no TypeScript shape at all.dangerous_get_v2_rotated_account_keysstill exists inkey_rotation.rs;fd74c9f56only trimmed it from auselist.The call sites our type check does not reach were reviewed and none intersect what the range touched. The eight
@ts-strict-ignoreSDK consumers underapps/andbitwarden_license/reference none of the removed or changed symbols, and no app orbitwarden_licensespec file imports them either.Commit
No commit, and nothing was edited. There are no compile-time breaks to fix, and the one runtime finding needs a decision on biometric re-enrollment rather than a mechanical change.
package.jsonis untouched, since the pin is the bump's output and not the fix's.