From 8dcf197adda798d0c56da41e7e304bf2d7628b47 Mon Sep 17 00:00:00 2001 From: Patrick Honkonen Date: Wed, 19 Aug 2026 15:56:28 -0400 Subject: [PATCH 1/8] [AI-94] llm: Evaluate SDK update PRs against the upstream commit range 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. --- .../evaluating-sdk-internal-updates/SKILL.md | 55 +++++ .../sdk-surface-diff.mjs | 162 ++++++++++++++ .github/CODEOWNERS | 1 + .../workflows/sdlc-sdk-update-evaluate.yml | 210 ++++++++++++++++++ eslint.config.mjs | 3 + 5 files changed, 431 insertions(+) create mode 100644 .claude/skills/evaluating-sdk-internal-updates/SKILL.md create mode 100644 .claude/skills/evaluating-sdk-internal-updates/sdk-surface-diff.mjs create mode 100644 .github/workflows/sdlc-sdk-update-evaluate.yml diff --git a/.claude/skills/evaluating-sdk-internal-updates/SKILL.md b/.claude/skills/evaluating-sdk-internal-updates/SKILL.md new file mode 100644 index 000000000000..b8305af31f16 --- /dev/null +++ b/.claude/skills/evaluating-sdk-internal-updates/SKILL.md @@ -0,0 +1,55 @@ +--- +name: evaluating-sdk-internal-updates +description: Evaluates a bitwarden/clients "Update sdk-internal to" PR against the sdk-internal commit range for compile-time and runtime breaking changes, maps affected symbols to TypeScript call sites, and applies clear in-scope fixes. Use when reviewing an SDK bump PR, an @bitwarden/sdk-internal version change in package.json, or triaging sdk-internal breaking changes. +allowed-tools: + - Bash(node .claude/skills/evaluating-sdk-internal-updates/sdk-surface-diff.mjs *) + - Bash(npm run test:types) + - Bash(npm test -- *) + - Bash(npx prettier *) + - Bash(gh pr diff:*) + - Bash(gh pr view:*) + - Bash(git -C * log *) + - Bash(git -C * show *) + - Bash(git grep:*) + - Bash(git add:*) + - Bash(git commit:*) + - Bash(grep:*) + - Read + - Grep + - Glob + - Edit + - Write + - Skill(writing-client-code) + - Skill(bitwarden-delivery-tools:committing-changes) +--- + +# Evaluating sdk-internal Updates + +**Identify both compile-time and runtime breaks before fixing anything — fixing the first break found is not finishing.** Steps 3-8 cover the entire commit range before step 9 starts. + +This SDK is the boundary where Protected Data becomes Vault Data. A fix must not send a decrypted value to an API service, log one, or move a decrypt out of the SDK into TypeScript. `.claude/CLAUDE.md` forbids new encryption logic in this repo, so a bump that appears to require some is a finding to report, not a fix to write. [Security definitions](https://contributing.bitwarden.com/architecture/security/definitions). + +## Identify + +Binding surface facts specific to this SDK: the pin is two npm versions in the root `package.json`, `@bitwarden/sdk-internal` and `@bitwarden/commercial-sdk-internal`, always the same string — `npm run lint:sdk-internal-versions` fails if they differ. The version encodes no commit; the sdk-internal SHA lives in `VERSION` inside the tarball. Nothing generated is committed, here or upstream: the API record is `bitwarden_wasm_internal.d.ts` inside the published package, whose root and `node/` copies are byte-identical, and `bitwarden_wasm_internal_bg.wasm.d.ts` is raw ABI and never a finding. + +The annotations behind this surface are `#[wasm_bindgen]` and `derive(..., Tsify)`. `#[uniffi::export]` is mobile-only and largely disjoint, so a uniffi sweep under-reports this surface badly. Serde attributes are part of the emitted type: `rename`, `rename_all`, `skip` and `Option` changes alter the `.d.ts` with no Rust signature change, and a `features = ["wasm"]` edit in `crates/bitwarden-wasm-internal/Cargo.toml` adds or removes types wholesale. Enum members are wire values (`CipherType.Login = 1`); a renumbering compiles clean at every call site and corrupts data. + +Most classes have a `private constructor()` and are reachable only through a parent client accessor (`sdk.user_crypto_management()`), so a renamed accessor breaks callers with the class itself unchanged. `tsconfig.base.json` sets `skipLibCheck: true`, so only call sites are checked. `bitwarden_license/bit-common/src/platform/sdk/sdk-alias.d.ts` re-exports the commercial package under the `@bitwarden/sdk-internal` module name and ESLint forbids importing it directly, so call sites grep identically for both packages. + +1. `gh pr diff -R bitwarden/clients -- package.json` → the old and new version. +2. Locate the `bitwarden/sdk-internal` clone in a sibling directory — `../sdk-internal`, where CI places it. If there is none, stop and tell the user it is a required prerequisite for this skill; do not clone it yourself. +3. Read the baseline type check. **In CI it has already run and the prompt names its log; grep it for `error TS` and treat that as this step's output. Do not run it.** Locally, `npm run test:types`. Note any failure and continue — a clean run rules out compile breaks only where the baseline looks, and step 7 is where it does not look. +4. `node .claude/skills/evaluating-sdk-internal-updates/sdk-surface-diff.mjs `, then again with `--commercial`. It prints RANGE (including the sdk-internal SHA pair step 5 needs), REMOVED, ADDED and MUTATED, keyed per member: a class method, interface property and enum member each get their own `Owner.member` entry. In ADDED, only a pre-existing owner is a break — a new non-optional property on an existing interface breaks every construction site. +5. `git -C ../sdk-internal log --reverse --oneline ..`, then `show` each commit. Classify per hunk, not per commit — a commit with one additive headline change can still carry a second, unrelated breaking hunk. A hunk that only touches a macro invocation does not show the surface; read the macro's definition before classifying it. This is also what explains a property the step 4 diff shows renamed or gone. +6. For every symbol from step 4, `git grep -n ''` from the repo root for the bare name — no module list and no import-prefix filter, since everything arrives as a plain `@bitwarden/sdk-internal` import and both apps and `bitwarden_license` consume it. Use `git grep`, not `grep -r`, which also searches `node_modules/@bitwarden/*sdk-internal/` and reports the bindings themselves as call sites. A surface change stays a candidate until a call site makes it a finding. +7. Three sets of call sites step 3 does not cover. Read each one the range touches. + - `apps/` and `bitwarden_license/` get no dedicated `tsc` leg and ride only on `tsc-strict`, which filters by file, so an ordinary type error inside a `@ts-strict-ignore` file is discarded: `git grep -l '@bitwarden/sdk-internal' -- 'apps/**' 'bitwarden_license/**' | xargs grep -l '@ts-strict-ignore'`. + - The root `tsconfig.json` excludes app and `bitwarden_license` spec files, so `git grep -l '@bitwarden/sdk-internal' -- 'apps/**/*.spec.ts' 'bitwarden_license/**/*.spec.ts'` is type-checked by nothing. + - `git grep -l 'jest.mock("@bitwarden/sdk-internal"'` — factory-form mocks are not checked against the real module and drift silently. `libs/common/src/platform/spec/mock-sdk.service.ts` and `libs/common/spec/jest-sdk-client-factory.ts` are structural and break loudest, the latter on any `PasswordManagerClient` constructor change. +8. Report under these headings, which are the same across every repo running this evaluation: `## SDK bump evaluated`, `## Compile-time breaks`, `## Runtime considerations` (wire-value and serialization findings go here; omit the heading when there are none), `## Everything else in range — confirmed safe`, `## Commit`. Give each finding its commit, symbol and call sites, and state whether the type check passed inside `## Compile-time breaks` rather than as its own section. Cite sdk-internal commits and PRs as `bitwarden/sdk-internal#` or a full URL — a bare `#` copied from a commit subject auto-links into bitwarden/clients and tags an unrelated PR. + +## Resolve + +9. Fix anything found, compile-time or runtime, whenever the correct behavior is clear and in scope. For a new required argument or method with no existing consumer, grep the underlying concept rather than the new name, which does not exist yet; with no consumer, stub it — an empty body with a `// no-op` comment, or a default return, never a `throw`, which turns a stub into a crash. A sibling's structure is a template; its behavior is not evidence for yours, so read the SDK to choose between two plausible argument values. Never edit `package.json`: the pin is the bump's output, not the fix's. If unsure, report it instead of guessing, along with anything needing a product decision. Invoke `Skill(writing-client-code)` first if the fix is not purely mechanical, then commit with `Skill(bitwarden-delivery-tools:committing-changes)`. +10. `npx prettier --write `, then `npm run test:types`, then `npm test -- --maxWorkers=2`. Run all three even in CI: `lint.yml` runs `npm run lint` on the PR, and `--maxWorkers=2` is this repo's workaround for Jest exhausting memory. diff --git a/.claude/skills/evaluating-sdk-internal-updates/sdk-surface-diff.mjs b/.claude/skills/evaluating-sdk-internal-updates/sdk-surface-diff.mjs new file mode 100644 index 000000000000..c1665f1514af --- /dev/null +++ b/.claude/skills/evaluating-sdk-internal-updates/sdk-surface-diff.mjs @@ -0,0 +1,162 @@ +#!/usr/bin/env node +// The SDK's TypeScript surface is committed in neither repo; it exists only inside the published +// tarballs. This reconstructs it from two of them and diffs it at member granularity. + +import { execFileSync } from "node:child_process"; +import { existsSync, mkdirSync, readFileSync, readdirSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import ts from "typescript"; + +const args = process.argv.slice(2); +const commercial = args.includes("--commercial"); +const [oldVersion, newVersion] = args.filter((arg) => !arg.startsWith("--")); + +if (!oldVersion || !newVersion) { + console.error("usage: sdk-surface-diff.mjs [--commercial]"); + process.exit(2); +} + +const pkg = commercial ? "@bitwarden/commercial-sdk-internal" : "@bitwarden/sdk-internal"; +const cache = join(process.env.RUNNER_TEMP ?? tmpdir(), "sdk-surface", pkg.replace(/\W/g, "-")); + +function fail(message) { + console.error(`error: ${message}`); + process.exit(1); +} + +function fetchSurface(version) { + const dir = join(cache, version); + const tarball = join(dir, "package"); + const dts = join(tarball, "bitwarden_wasm_internal.d.ts"); + + if (!existsSync(dts)) { + mkdirSync(dir, { recursive: true }); + execFileSync( + "npm", + ["pack", `${pkg}@${version}`, "--pack-destination", dir, "--loglevel=warn"], + { + stdio: ["ignore", "ignore", "inherit"], + }, + ); + const tgz = readdirSync(dir).find((file) => file.endsWith(".tgz")); + execFileSync("tar", [ + "-xzf", + join(dir, tgz), + "-C", + dir, + "package/bitwarden_wasm_internal.d.ts", + ]); + // Some commercial builds declare VERSION without shipping it, so extract it on its own. + try { + execFileSync("tar", ["-xzf", join(dir, tgz), "-C", dir, "package/VERSION"], { + stdio: "ignore", + }); + } catch { + /* the SHA pair comes from the public package */ + } + } + + const version_file = join(tarball, "VERSION"); + return { + text: readFileSync(dts, "utf8"), + sha: existsSync(version_file) ? readFileSync(version_file, "utf8").trim() : null, + }; +} + +const normalize = (text) => text.replace(/\s+/g, " ").trim(); + +// Owner keys carry the declaration up to its body, so a member change does not also dirty its owner. +function header(node, source) { + const text = node.getText(source); + const body = text.indexOf("{"); + return normalize(body === -1 ? text : text.slice(0, body)); +} + +function extract(text, label) { + const source = ts.createSourceFile(label, text, ts.ScriptTarget.Latest, false); + if (source.parseDiagnostics?.length) { + fail(`${label} did not parse: ${source.parseDiagnostics[0].messageText}`); + } + + const declarations = new Map(); + + for (const node of source.statements) { + const name = node.name?.getText(source); + if (!name) { + continue; + } + + if (ts.isFunctionDeclaration(node) || ts.isTypeAliasDeclaration(node)) { + declarations.set(name, normalize(node.getText(source))); + continue; + } + + if ( + !ts.isClassDeclaration(node) && + !ts.isInterfaceDeclaration(node) && + !ts.isEnumDeclaration(node) + ) { + continue; + } + + declarations.set(name, header(node, source)); + for (const member of node.members) { + const member_name = ts.isConstructorDeclaration(member) + ? "constructor" + : (member.name?.getText(source) ?? "[index]"); + declarations.set(`${name}.${member_name}`, normalize(member.getText(source))); + } + } + + return declarations; +} + +const before = fetchSurface(oldVersion); +const after = fetchSurface(newVersion); +const old_surface = extract(before.text, `${oldVersion}.d.ts`); +const new_surface = extract(after.text, `${newVersion}.d.ts`); + +// An empty extraction reads as "nothing changed", the one wrong answer this must never give. +if (old_surface.size === 0 || new_surface.size === 0) { + fail( + `extracted ${old_surface.size} and ${new_surface.size} declarations; ` + + "the extractor no longer matches the generated form", + ); +} + +const removed = [...old_surface.keys()].filter((key) => !new_surface.has(key)).sort(); +const added = [...new_surface.keys()].filter((key) => !old_surface.has(key)).sort(); +const mutated = [...old_surface.keys()] + .filter((key) => new_surface.has(key) && new_surface.get(key) !== old_surface.get(key)) + .sort(); + +const added_owners = new Set(added); +const ownerOf = (key) => (key.includes(".") ? key.slice(0, key.indexOf(".")) : null); + +console.log("## RANGE"); +console.log(`${pkg} ${oldVersion} -> ${newVersion}`); +if (before.sha && after.sha) { + console.log(`sdk-internal ${before.sha}..${after.sha}`); +} +console.log(`declarations ${old_surface.size} -> ${new_surface.size}`); + +console.log(`\n## REMOVED (${removed.length}) - absent at NEW; a compile break at every call site`); +for (const key of removed) { + console.log(` ${key}`); +} + +console.log(`\n## ADDED (${added.length}) - only a pre-existing owner is a break`); +for (const key of added) { + const owner = ownerOf(key); + const note = + owner === null ? "" : added_owners.has(owner) ? " (new owner)" : " (existing owner)"; + console.log(` ${key}${note}`); +} + +console.log(`\n## MUTATED (${mutated.length}) - key survives, its declaration changed`); +for (const key of mutated) { + console.log(` ${key}`); + console.log(` OLD: ${old_surface.get(key)}`); + console.log(` NEW: ${new_surface.get(key)}`); +} diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 1b2d1f47583c..43d92cc744f9 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -282,4 +282,5 @@ apps/desktop/native-messaging-test-runner/package-lock.json @bitwarden/team-pla .claude/ @bitwarden/team-ai-sme .github/workflows/respond.yml @bitwarden/team-ai-sme .github/workflows/review-code.yml @bitwarden/team-ai-sme +.github/workflows/sdlc-sdk-update-evaluate.yml @bitwarden/team-ai-sme @bitwarden/team-platform-dev .mcp.json @bitwarden/team-ai-sme diff --git a/.github/workflows/sdlc-sdk-update-evaluate.yml b/.github/workflows/sdlc-sdk-update-evaluate.yml new file mode 100644 index 000000000000..4f6d3aece158 --- /dev/null +++ b/.github/workflows/sdlc-sdk-update-evaluate.yml @@ -0,0 +1,210 @@ +name: SDLC / SDK Update Evaluation + +on: + pull_request: + types: [opened, synchronize] + +concurrency: + group: ${{ github.workflow }}-${{ github.event.pull_request.number }} + cancel-in-progress: true + +permissions: {} + +jobs: + evaluate: + name: Evaluate SDK Update + runs-on: ubuntu-24.04 + timeout-minutes: 60 + if: ${{ github.event.pull_request.head.ref == 'sdlc/sdk-update' && github.event.pull_request.head.repo.full_name == github.repository }} + permissions: + id-token: write # Azure OIDC login + actions: read # claude-code-action track_progress + contents: write # push a resolved fix + pull-requests: write # sticky comment + + env: + _BOT_NAME: "bw-ghapp[bot]" + _BOT_EMAIL: "178206702+bw-ghapp[bot]@users.noreply.github.com" + + steps: + - name: Skip if this bump was already evaluated + id: gate + env: + GH_TOKEN: ${{ github.token }} + PR_NUMBER: ${{ github.event.pull_request.number }} + REPO: ${{ github.repository }} + run: | + TIP=$(gh pr view "$PR_NUMBER" -R "$REPO" --json commits \ + --jq '.commits[-1] | "\(.authors[0].email)\t\(.messageHeadline)"') + TIP_EMAIL=${TIP%%$'\t'*} + TIP_SUBJECT=${TIP#*$'\t'} + + # sdk-update.yml gates on this same email, so both workflows agree on who owns the branch. + if [[ "$TIP_SUBJECT" != "Update sdk-internal to "* || "$TIP_EMAIL" != "$_BOT_EMAIL" ]]; then + echo "Branch tip is not a fresh bot-authored version bump ('$TIP_SUBJECT', $TIP_EMAIL) — skipping." + echo "skip=true" >> "$GITHUB_OUTPUT" + else + echo "version=${TIP_SUBJECT#Update sdk-internal to }" >> "$GITHUB_OUTPUT" + echo "skip=false" >> "$GITHUB_OUTPUT" + fi + + - name: Log in to Azure + if: steps.gate.outputs.skip == 'false' + uses: bitwarden/gh-actions/azure-login@main + with: + subscription_id: ${{ secrets.AZURE_SUBSCRIPTION_ID }} + tenant_id: ${{ secrets.AZURE_TENANT_ID }} + client_id: ${{ secrets.AZURE_CLIENT_ID }} + + - name: Get Azure Key Vault secrets + id: get-kv-secrets + if: steps.gate.outputs.skip == 'false' + uses: bitwarden/gh-actions/get-keyvault-secrets@main + with: + keyvault: gh-org-bitwarden + secrets: "ANTHROPIC-RESPONSE-API-KEY,BW-GHAPP-ID,BW-GHAPP-KEY" + + - name: Log out from Azure + if: steps.gate.outputs.skip == 'false' + uses: bitwarden/gh-actions/azure-logout@main + + - name: Generate GH App token + id: app-token + if: steps.gate.outputs.skip == 'false' + uses: actions/create-github-app-token@f8d387b68d61c58ab83c6c016672934102569859 # v3.0.0 + with: + app-id: ${{ steps.get-kv-secrets.outputs.BW-GHAPP-ID }} + private-key: ${{ steps.get-kv-secrets.outputs.BW-GHAPP-KEY }} + permission-contents: write + permission-pull-requests: write + + - name: Check out repo + if: steps.gate.outputs.skip == 'false' + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + token: ${{ steps.app-token.outputs.token }} + ref: ${{ github.event.pull_request.head.sha }} + fetch-depth: 0 + # Must stay false. A persisted token lands in .git/config as an http.extraheader, which the + # agent's `Bash(grep:*)` grant can read; the `Read(**/.git/**)` deny does not constrain Bash. + persist-credentials: false + + # Blobless: only needed to read why a surface change happened, never to build. + - name: Clone sdk-internal + if: steps.gate.outputs.skip == 'false' + run: git clone --quiet --filter=blob:none https://github.com/bitwarden/sdk-internal "${{ github.workspace }}/../sdk-internal" + + - name: Get Node Version + id: retrieve-node-version + if: steps.gate.outputs.skip == 'false' + run: | + NODE_NVMRC=$(cat .nvmrc) + NODE_VERSION=${NODE_NVMRC/v/''} + echo "node_version=$NODE_VERSION" >> "$GITHUB_OUTPUT" + + - name: Set up Node + if: steps.gate.outputs.skip == 'false' + uses: actions/setup-node@53b83947a5a98c8d113130e565377fae1a50d02f # v6.3.0 + with: + cache: "npm" + cache-dependency-path: "**/package-lock.json" + node-version: ${{ steps.retrieve-node-version.outputs.node_version }} + + - name: Install Node dependencies + if: steps.gate.outputs.skip == 'false' + run: npm ci + + # Any commit the agent makes has to be bot-authored or the next bump run's author guard fails. + - name: Configure git identity + if: steps.gate.outputs.skip == 'false' + run: | + git config user.name "$_BOT_NAME" + git config user.email "$_BOT_EMAIL" + + - name: Run the baseline type check + id: baseline + if: steps.gate.outputs.skip == 'false' + run: | + _LOG="$RUNNER_TEMP/baseline-test-types.log" + + # `set +e` covers the type check alone: a compile break has to reach the agent, not end the job. + set +e + npm run test:types > "$_LOG" 2>&1 + _EXIT=$? + set -e + + # test-types.js drives 49 legs through concurrently without awaiting it, so a type error and + # an OOM-killed tsc both exit non-zero through an unhandled rejection. Requiring a diagnostic + # separates the break this workflow exists to find from a log that proves nothing. + if [ "$_EXIT" -ne 0 ] && ! grep -q 'error TS' "$_LOG"; then + echo "::error::The baseline type check exited $_EXIT with no TS diagnostic, so its log cannot be read as evidence of anything." + tail -50 "$_LOG" + exit 1 + fi + + echo "log=$_LOG" >> "$GITHUB_OUTPUT" + + - name: Run Claude Code + id: claude + if: steps.gate.outputs.skip == 'false' + uses: anthropics/claude-code-action@be7b93b1907a4abad570368f3c74b6fe3807510b # v1.0.183 + env: + # Claude Code defaults to a 120-second Bash timeout and claude-code-action sets neither + # variable. Both stay well inside timeout-minutes. + BASH_DEFAULT_TIMEOUT_MS: "600000" + BASH_MAX_TIMEOUT_MS: "1800000" + with: + anthropic_api_key: ${{ steps.get-kv-secrets.outputs.ANTHROPIC-RESPONSE-API-KEY }} + allowed_bots: "bw-ghapp" + bot_id: "178206702" + bot_name: "bw-ghapp[bot]" + track_progress: true + use_sticky_comment: true + plugin_marketplaces: "https://github.com/bitwarden/ai-plugins.git" + plugins: | + bitwarden-delivery-tools@bitwarden-marketplace + prompt: | + PR #${{ github.event.pull_request.number }} in ${{ github.repository }} bumps the Bitwarden SDK to ${{ steps.gate.outputs.version }}. Evaluate it using the `evaluating-sdk-internal-updates` skill and resolve anything within scope that needs resolving, per the skill's own Resolve steps. + + `bitwarden/sdk-internal` is already cloned as a sibling directory at `${{ github.workspace }}/../sdk-internal`; use it directly, do not re-clone. `npm ci` has already run. A baseline type check has already been run for you and its output is at `${{ steps.baseline.outputs.log }}`; read it rather than running your own baseline. + + Commit any fix on the current branch (already checked out) using `Skill(bitwarden-delivery-tools:committing-changes)` for the message. Do not push; the workflow pushes separately. + + When done, replace the sticky PR comment with the skill's 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 "Bash(node .claude/skills/evaluating-sdk-internal-updates/sdk-surface-diff.mjs *),Bash(npm run test:types),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,mcp__github_comment__update_claude_comment" + --disallowedTools "Read(**/.git/**),Edit(**/.git/**),Write(**/.git/**),Edit(.github/**),Write(.github/**),Edit(.claude/**),Write(.claude/**)" + + - name: Push any resolved fix + if: steps.gate.outputs.skip == 'false' + env: + _BRANCH_NAME: ${{ github.event.pull_request.head.ref }} + _TOKEN: ${{ steps.app-token.outputs.token }} + run: | + # --untracked-files=no is required: npm leaves untracked, non-gitignored paths that a bare + # --porcelain would report, tripping this guard. + if [ -n "$(git status --porcelain --untracked-files=no)" ]; then + echo "::error::Claude left uncommitted changes to tracked files; nothing was pushed." + git status --porcelain --untracked-files=no + exit 1 + fi + + if git diff --quiet "origin/$_BRANCH_NAME" HEAD; then + echo "No new commits to push." + exit 0 + fi + + # A fix may touch SDK call sites, never CI or agent configuration, and never the pin: moving + # both pins would ship a version the bump did not intend, which nothing downstream catches. + CHANGED=$(git diff --name-only "origin/$_BRANCH_NAME" HEAD) + if echo "$CHANGED" | grep -qE '^(\.github/|\.claude/|package(-lock)?\.json$)'; then + echo "::error::Refusing to push: the fix touches CI, agent configuration, or the SDK pin." + echo "$CHANGED" + exit 1 + fi + + # Checkout does not persist credentials, so authenticate this push alone. `git -c` keeps the + # header out of .git/config and the token out of the remote URL git echoes on error. + git -c http.extraheader="AUTHORIZATION: basic $(printf '%s' "x-access-token:$_TOKEN" | base64 | tr -d '\n')" \ + push origin "HEAD:$_BRANCH_NAME" diff --git a/eslint.config.mjs b/eslint.config.mjs index 8ac291696edd..82bef23d3cd2 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -817,6 +817,9 @@ export default tseslint.config( "scripts/*.js", "jest.preset.js", + + // Agent configuration and its tooling, not shipped code. + ".claude/", ], }, ); From 5b068461bcb205c572bcb9c19d71c5726693d3bf Mon Sep 17 00:00:00 2001 From: Patrick Honkonen Date: Tue, 25 Aug 2026 11:05:17 -0400 Subject: [PATCH 2/8] Scope Skill grant in sdk-update-evaluate workflow to match SKILL.md --- .github/workflows/sdlc-sdk-update-evaluate.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/sdlc-sdk-update-evaluate.yml b/.github/workflows/sdlc-sdk-update-evaluate.yml index 4f6d3aece158..bd339e5a12f3 100644 --- a/.github/workflows/sdlc-sdk-update-evaluate.yml +++ b/.github/workflows/sdlc-sdk-update-evaluate.yml @@ -173,7 +173,7 @@ jobs: When done, replace the sticky PR comment with the skill's 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 "Bash(node .claude/skills/evaluating-sdk-internal-updates/sdk-surface-diff.mjs *),Bash(npm run test:types),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,mcp__github_comment__update_claude_comment" + --allowedTools "Bash(node .claude/skills/evaluating-sdk-internal-updates/sdk-surface-diff.mjs *),Bash(npm run test:types),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(writing-client-code),Skill(bitwarden-delivery-tools:committing-changes),mcp__github_comment__update_claude_comment" --disallowedTools "Read(**/.git/**),Edit(**/.git/**),Write(**/.git/**),Edit(.github/**),Write(.github/**),Edit(.claude/**),Write(.claude/**)" - name: Push any resolved fix From b4d12408068d90d925f081a2d1079336cb175e81 Mon Sep 17 00:00:00 2001 From: Patrick Honkonen Date: Wed, 26 Aug 2026 09:16:51 -0400 Subject: [PATCH 3/8] Address validate-ai review: fix false claim, missing lint step, unvalidated version arg --- .../skills/evaluating-sdk-internal-updates/SKILL.md | 7 ++++--- .../sdk-surface-diff.mjs | 13 +++++++++++++ 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/.claude/skills/evaluating-sdk-internal-updates/SKILL.md b/.claude/skills/evaluating-sdk-internal-updates/SKILL.md index b8305af31f16..40f89a809aa5 100644 --- a/.claude/skills/evaluating-sdk-internal-updates/SKILL.md +++ b/.claude/skills/evaluating-sdk-internal-updates/SKILL.md @@ -4,6 +4,7 @@ description: Evaluates a bitwarden/clients "Update sdk-internal to" PR against t allowed-tools: - 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:*) @@ -44,12 +45,12 @@ Most classes have a `private constructor()` and are reachable only through a par 5. `git -C ../sdk-internal log --reverse --oneline ..`, then `show` each commit. Classify per hunk, not per commit — a commit with one additive headline change can still carry a second, unrelated breaking hunk. A hunk that only touches a macro invocation does not show the surface; read the macro's definition before classifying it. This is also what explains a property the step 4 diff shows renamed or gone. 6. For every symbol from step 4, `git grep -n ''` from the repo root for the bare name — no module list and no import-prefix filter, since everything arrives as a plain `@bitwarden/sdk-internal` import and both apps and `bitwarden_license` consume it. Use `git grep`, not `grep -r`, which also searches `node_modules/@bitwarden/*sdk-internal/` and reports the bindings themselves as call sites. A surface change stays a candidate until a call site makes it a finding. 7. Three sets of call sites step 3 does not cover. Read each one the range touches. - - `apps/` and `bitwarden_license/` get no dedicated `tsc` leg and ride only on `tsc-strict`, which filters by file, so an ordinary type error inside a `@ts-strict-ignore` file is discarded: `git grep -l '@bitwarden/sdk-internal' -- 'apps/**' 'bitwarden_license/**' | xargs grep -l '@ts-strict-ignore'`. - - The root `tsconfig.json` excludes app and `bitwarden_license` spec files, so `git grep -l '@bitwarden/sdk-internal' -- 'apps/**/*.spec.ts' 'bitwarden_license/**/*.spec.ts'` is type-checked by nothing. + - `apps/` and `bitwarden_license/` get no dedicated `tsc` leg and ride only on `tsc-strict`, which filters by file, so an ordinary type error inside a `@ts-strict-ignore` file is discarded: `git grep -l '@bitwarden/sdk-internal' -- 'apps/**' 'bitwarden_license/**' | xargs -r grep -l '@ts-strict-ignore'`. + - The root `tsconfig.json` excludes app spec files but not `bitwarden_license` ones, so `git grep -l '@bitwarden/sdk-internal' -- 'apps/**/*.spec.ts'` is type-checked by nothing; the `bitwarden_license` equivalent is still covered by `tsc-strict`. - `git grep -l 'jest.mock("@bitwarden/sdk-internal"'` — factory-form mocks are not checked against the real module and drift silently. `libs/common/src/platform/spec/mock-sdk.service.ts` and `libs/common/spec/jest-sdk-client-factory.ts` are structural and break loudest, the latter on any `PasswordManagerClient` constructor change. 8. Report under these headings, which are the same across every repo running this evaluation: `## SDK bump evaluated`, `## Compile-time breaks`, `## Runtime considerations` (wire-value and serialization findings go here; omit the heading when there are none), `## Everything else in range — confirmed safe`, `## Commit`. Give each finding its commit, symbol and call sites, and state whether the type check passed inside `## Compile-time breaks` rather than as its own section. Cite sdk-internal commits and PRs as `bitwarden/sdk-internal#` or a full URL — a bare `#` copied from a commit subject auto-links into bitwarden/clients and tags an unrelated PR. ## Resolve 9. Fix anything found, compile-time or runtime, whenever the correct behavior is clear and in scope. For a new required argument or method with no existing consumer, grep the underlying concept rather than the new name, which does not exist yet; with no consumer, stub it — an empty body with a `// no-op` comment, or a default return, never a `throw`, which turns a stub into a crash. A sibling's structure is a template; its behavior is not evidence for yours, so read the SDK to choose between two plausible argument values. Never edit `package.json`: the pin is the bump's output, not the fix's. If unsure, report it instead of guessing, along with anything needing a product decision. Invoke `Skill(writing-client-code)` first if the fix is not purely mechanical, then commit with `Skill(bitwarden-delivery-tools:committing-changes)`. -10. `npx prettier --write `, then `npm run test:types`, then `npm test -- --maxWorkers=2`. Run all three even in CI: `lint.yml` runs `npm run lint` on the PR, and `--maxWorkers=2` is this repo's workaround for Jest exhausting memory. +10. `npm run lint:fix`, then `npx prettier --write `, then `npm run test:types`, then `npm test -- --maxWorkers=2`. Run all four even in CI: `lint.yml`'s `npm run lint` step runs ESLint as well as Prettier, and `--maxWorkers=2` is this repo's workaround for Jest exhausting memory. diff --git a/.claude/skills/evaluating-sdk-internal-updates/sdk-surface-diff.mjs b/.claude/skills/evaluating-sdk-internal-updates/sdk-surface-diff.mjs index c1665f1514af..a829027ea4f0 100644 --- a/.claude/skills/evaluating-sdk-internal-updates/sdk-surface-diff.mjs +++ b/.claude/skills/evaluating-sdk-internal-updates/sdk-surface-diff.mjs @@ -17,6 +17,16 @@ if (!oldVersion || !newVersion) { process.exit(2); } +// Both values become an `npm pack` spec and a cache-directory path component. Unvalidated, a +// git URL, file: path, or dist-tag also parses as a spec (running the fetched package's +// `prepare` script on pack), and `..` in the path component escapes the cache directory. +const versionPattern = /^\d+\.\d+\.\d+(-[0-9A-Za-z.-]+)?$/; +for (const version of [oldVersion, newVersion]) { + if (!versionPattern.test(version)) { + fail(`"${version}" is not a version`); + } +} + const pkg = commercial ? "@bitwarden/commercial-sdk-internal" : "@bitwarden/sdk-internal"; const cache = join(process.env.RUNNER_TEMP ?? tmpdir(), "sdk-surface", pkg.replace(/\W/g, "-")); @@ -40,6 +50,9 @@ function fetchSurface(version) { }, ); const tgz = readdirSync(dir).find((file) => file.endsWith(".tgz")); + if (!tgz) { + fail(`npm pack produced no tarball for ${pkg}@${version}`); + } execFileSync("tar", [ "-xzf", join(dir, tgz), From c702008626b69f6ff115b2e147809401c19133dd Mon Sep 17 00:00:00 2001 From: Patrick Honkonen Date: Wed, 26 Aug 2026 10:05:43 -0400 Subject: [PATCH 4/8] Run SDK update evaluation as the bitwarden-software-engineer agent --- .github/workflows/sdlc-sdk-update-evaluate.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/sdlc-sdk-update-evaluate.yml b/.github/workflows/sdlc-sdk-update-evaluate.yml index bd339e5a12f3..f45cd2845149 100644 --- a/.github/workflows/sdlc-sdk-update-evaluate.yml +++ b/.github/workflows/sdlc-sdk-update-evaluate.yml @@ -163,6 +163,7 @@ jobs: plugin_marketplaces: "https://github.com/bitwarden/ai-plugins.git" plugins: | bitwarden-delivery-tools@bitwarden-marketplace + bitwarden-software-engineer@bitwarden-marketplace prompt: | PR #${{ github.event.pull_request.number }} in ${{ github.repository }} bumps the Bitwarden SDK to ${{ steps.gate.outputs.version }}. Evaluate it using the `evaluating-sdk-internal-updates` skill and resolve anything within scope that needs resolving, per the skill's own Resolve steps. @@ -173,6 +174,7 @@ jobs: When done, replace the sticky PR comment with the skill's 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 + --agent bitwarden-software-engineer --allowedTools "Bash(node .claude/skills/evaluating-sdk-internal-updates/sdk-surface-diff.mjs *),Bash(npm run test:types),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(writing-client-code),Skill(bitwarden-delivery-tools:committing-changes),mcp__github_comment__update_claude_comment" --disallowedTools "Read(**/.git/**),Edit(**/.git/**),Write(**/.git/**),Edit(.github/**),Write(.github/**),Edit(.claude/**),Write(.claude/**)" From 0275bd099b382ff02d7f249d20119007bc0cbfea Mon Sep 17 00:00:00 2001 From: Patrick Honkonen Date: Wed, 26 Aug 2026 10:42:33 -0400 Subject: [PATCH 5/8] Address second validate-ai round: mirror lint:fix grant, drop ungranted xargs --- .claude/skills/evaluating-sdk-internal-updates/SKILL.md | 2 +- .github/workflows/sdlc-sdk-update-evaluate.yml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.claude/skills/evaluating-sdk-internal-updates/SKILL.md b/.claude/skills/evaluating-sdk-internal-updates/SKILL.md index 40f89a809aa5..87d05483149f 100644 --- a/.claude/skills/evaluating-sdk-internal-updates/SKILL.md +++ b/.claude/skills/evaluating-sdk-internal-updates/SKILL.md @@ -45,7 +45,7 @@ Most classes have a `private constructor()` and are reachable only through a par 5. `git -C ../sdk-internal log --reverse --oneline ..`, then `show` each commit. Classify per hunk, not per commit — a commit with one additive headline change can still carry a second, unrelated breaking hunk. A hunk that only touches a macro invocation does not show the surface; read the macro's definition before classifying it. This is also what explains a property the step 4 diff shows renamed or gone. 6. For every symbol from step 4, `git grep -n ''` from the repo root for the bare name — no module list and no import-prefix filter, since everything arrives as a plain `@bitwarden/sdk-internal` import and both apps and `bitwarden_license` consume it. Use `git grep`, not `grep -r`, which also searches `node_modules/@bitwarden/*sdk-internal/` and reports the bindings themselves as call sites. A surface change stays a candidate until a call site makes it a finding. 7. Three sets of call sites step 3 does not cover. Read each one the range touches. - - `apps/` and `bitwarden_license/` get no dedicated `tsc` leg and ride only on `tsc-strict`, which filters by file, so an ordinary type error inside a `@ts-strict-ignore` file is discarded: `git grep -l '@bitwarden/sdk-internal' -- 'apps/**' 'bitwarden_license/**' | xargs -r grep -l '@ts-strict-ignore'`. + - `apps/` and `bitwarden_license/` get no dedicated `tsc` leg and ride only on `tsc-strict`, which filters by file, so an ordinary type error inside a `@ts-strict-ignore` file is discarded: `git grep -l '@bitwarden/sdk-internal' -- 'apps/**' 'bitwarden_license/**'`, then check that file list for `@ts-strict-ignore` with the `Grep` tool rather than piping into `xargs`, which carries no grant of its own. - The root `tsconfig.json` excludes app spec files but not `bitwarden_license` ones, so `git grep -l '@bitwarden/sdk-internal' -- 'apps/**/*.spec.ts'` is type-checked by nothing; the `bitwarden_license` equivalent is still covered by `tsc-strict`. - `git grep -l 'jest.mock("@bitwarden/sdk-internal"'` — factory-form mocks are not checked against the real module and drift silently. `libs/common/src/platform/spec/mock-sdk.service.ts` and `libs/common/spec/jest-sdk-client-factory.ts` are structural and break loudest, the latter on any `PasswordManagerClient` constructor change. 8. Report under these headings, which are the same across every repo running this evaluation: `## SDK bump evaluated`, `## Compile-time breaks`, `## Runtime considerations` (wire-value and serialization findings go here; omit the heading when there are none), `## Everything else in range — confirmed safe`, `## Commit`. Give each finding its commit, symbol and call sites, and state whether the type check passed inside `## Compile-time breaks` rather than as its own section. Cite sdk-internal commits and PRs as `bitwarden/sdk-internal#` or a full URL — a bare `#` copied from a commit subject auto-links into bitwarden/clients and tags an unrelated PR. diff --git a/.github/workflows/sdlc-sdk-update-evaluate.yml b/.github/workflows/sdlc-sdk-update-evaluate.yml index f45cd2845149..89f30b8bcad5 100644 --- a/.github/workflows/sdlc-sdk-update-evaluate.yml +++ b/.github/workflows/sdlc-sdk-update-evaluate.yml @@ -175,7 +175,7 @@ jobs: claude_args: | --model opus --agent bitwarden-software-engineer - --allowedTools "Bash(node .claude/skills/evaluating-sdk-internal-updates/sdk-surface-diff.mjs *),Bash(npm run test:types),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(writing-client-code),Skill(bitwarden-delivery-tools:committing-changes),mcp__github_comment__update_claude_comment" + --allowedTools "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(writing-client-code),Skill(bitwarden-delivery-tools:committing-changes),mcp__github_comment__update_claude_comment" --disallowedTools "Read(**/.git/**),Edit(**/.git/**),Write(**/.git/**),Edit(.github/**),Write(.github/**),Edit(.claude/**),Write(.claude/**)" - name: Push any resolved fix From 3e3fdc9ef6fe7c1ffcaa6d715f3fa6f4a44abe17 Mon Sep 17 00:00:00 2001 From: Patrick Honkonen Date: Wed, 26 Aug 2026 10:46:51 -0400 Subject: [PATCH 6/8] Address minor findings: sibling-clone prerequisite, step numbering, clean pack/tar failures --- .../evaluating-sdk-internal-updates/SKILL.md | 4 +-- .../sdk-surface-diff.mjs | 36 +++++++++++-------- 2 files changed, 24 insertions(+), 16 deletions(-) diff --git a/.claude/skills/evaluating-sdk-internal-updates/SKILL.md b/.claude/skills/evaluating-sdk-internal-updates/SKILL.md index 87d05483149f..bc940b75ebaa 100644 --- a/.claude/skills/evaluating-sdk-internal-updates/SKILL.md +++ b/.claude/skills/evaluating-sdk-internal-updates/SKILL.md @@ -1,6 +1,6 @@ --- name: evaluating-sdk-internal-updates -description: Evaluates a bitwarden/clients "Update sdk-internal to" PR against the sdk-internal commit range for compile-time and runtime breaking changes, maps affected symbols to TypeScript call sites, and applies clear in-scope fixes. Use when reviewing an SDK bump PR, an @bitwarden/sdk-internal version change in package.json, or triaging sdk-internal breaking changes. +description: Evaluates a bitwarden/clients "Update sdk-internal to" PR against the sdk-internal commit range for compile-time and runtime breaking changes, maps affected symbols to TypeScript call sites, and applies clear in-scope fixes. Use when reviewing an SDK bump PR, an @bitwarden/sdk-internal version change in package.json, or triaging sdk-internal breaking changes. Requires a sibling `bitwarden/sdk-internal` clone. allowed-tools: - Bash(node .claude/skills/evaluating-sdk-internal-updates/sdk-surface-diff.mjs *) - Bash(npm run test:types) @@ -26,7 +26,7 @@ allowed-tools: # Evaluating sdk-internal Updates -**Identify both compile-time and runtime breaks before fixing anything — fixing the first break found is not finishing.** Steps 3-8 cover the entire commit range before step 9 starts. +**Identify both compile-time and runtime breaks before fixing anything — fixing the first break found is not finishing.** Steps 4-7 cover the entire commit range, step 8 reports it, and step 9 starts only after that. This SDK is the boundary where Protected Data becomes Vault Data. A fix must not send a decrypted value to an API service, log one, or move a decrypt out of the SDK into TypeScript. `.claude/CLAUDE.md` forbids new encryption logic in this repo, so a bump that appears to require some is a finding to report, not a fix to write. [Security definitions](https://contributing.bitwarden.com/architecture/security/definitions). diff --git a/.claude/skills/evaluating-sdk-internal-updates/sdk-surface-diff.mjs b/.claude/skills/evaluating-sdk-internal-updates/sdk-surface-diff.mjs index a829027ea4f0..52a8df324341 100644 --- a/.claude/skills/evaluating-sdk-internal-updates/sdk-surface-diff.mjs +++ b/.claude/skills/evaluating-sdk-internal-updates/sdk-surface-diff.mjs @@ -42,24 +42,32 @@ function fetchSurface(version) { if (!existsSync(dts)) { mkdirSync(dir, { recursive: true }); - execFileSync( - "npm", - ["pack", `${pkg}@${version}`, "--pack-destination", dir, "--loglevel=warn"], - { - stdio: ["ignore", "ignore", "inherit"], - }, - ); + try { + execFileSync( + "npm", + ["pack", `${pkg}@${version}`, "--pack-destination", dir, "--loglevel=warn"], + { + stdio: ["ignore", "ignore", "inherit"], + }, + ); + } catch (error) { + fail(`npm pack failed for ${pkg}@${version}: ${error.message}`); + } const tgz = readdirSync(dir).find((file) => file.endsWith(".tgz")); if (!tgz) { fail(`npm pack produced no tarball for ${pkg}@${version}`); } - execFileSync("tar", [ - "-xzf", - join(dir, tgz), - "-C", - dir, - "package/bitwarden_wasm_internal.d.ts", - ]); + try { + execFileSync("tar", [ + "-xzf", + join(dir, tgz), + "-C", + dir, + "package/bitwarden_wasm_internal.d.ts", + ]); + } catch (error) { + fail(`failed to extract bitwarden_wasm_internal.d.ts from ${tgz}: ${error.message}`); + } // Some commercial builds declare VERSION without shipping it, so extract it on its own. try { execFileSync("tar", ["-xzf", join(dir, tgz), "-C", dir, "package/VERSION"], { From a1993e91a9706681ac1275c69ece0b72d54f36d8 Mon Sep 17 00:00:00 2001 From: Patrick Honkonen Date: Wed, 26 Aug 2026 13:01:14 -0400 Subject: [PATCH 7/8] Dispatch a single bitwarden-software-engineer subagent only when fixes 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. --- .github/workflows/sdlc-sdk-update-evaluate.yml | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/.github/workflows/sdlc-sdk-update-evaluate.yml b/.github/workflows/sdlc-sdk-update-evaluate.yml index 89f30b8bcad5..c4d77e0c5d24 100644 --- a/.github/workflows/sdlc-sdk-update-evaluate.yml +++ b/.github/workflows/sdlc-sdk-update-evaluate.yml @@ -165,17 +165,16 @@ jobs: bitwarden-delivery-tools@bitwarden-marketplace bitwarden-software-engineer@bitwarden-marketplace prompt: | - PR #${{ github.event.pull_request.number }} in ${{ github.repository }} bumps the Bitwarden SDK to ${{ steps.gate.outputs.version }}. Evaluate it using the `evaluating-sdk-internal-updates` skill and resolve anything within scope that needs resolving, per the skill's own Resolve steps. + PR #${{ github.event.pull_request.number }} in ${{ github.repository }} bumps the Bitwarden SDK to ${{ steps.gate.outputs.version }}. Evaluate it yourself using the `evaluating-sdk-internal-updates` skill's Identify steps (1-7). `bitwarden/sdk-internal` is already cloned as a sibling directory at `${{ github.workspace }}/../sdk-internal`; use it directly, do not re-clone. `npm ci` has already run. A baseline type check has already been run for you and its output is at `${{ steps.baseline.outputs.log }}`; read it rather than running your own baseline. - Commit any fix on the current branch (already checked out) using `Skill(bitwarden-delivery-tools:committing-changes)` for the message. Do not push; the workflow pushes separately. + If step 7 turns up anything to resolve, dispatch a single subagent on the `bitwarden-software-engineer` agent to carry out the skill's Resolve steps (8-10) for every finding in one pass — decide the fix, implement it, and commit on the current branch (already checked out) using `Skill(bitwarden-delivery-tools:committing-changes)` for the message. Hand the subagent all the findings (commit, symbol, call sites for each) rather than having it redo the investigation, and rather than spawning one subagent per finding. Do not push; the workflow pushes separately. Skip the subagent entirely if step 7 finds nothing to resolve. - When done, replace the sticky PR comment with the skill's 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. + 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 7 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 - --agent bitwarden-software-engineer - --allowedTools "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(writing-client-code),Skill(bitwarden-delivery-tools:committing-changes),mcp__github_comment__update_claude_comment" + --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" --disallowedTools "Read(**/.git/**),Edit(**/.git/**),Write(**/.git/**),Edit(.github/**),Write(.github/**),Edit(.claude/**),Write(.claude/**)" - name: Push any resolved fix From 8f4a4c19bcca8990bbcde4a206825db290808a66 Mon Sep 17 00:00:00 2001 From: Patrick Honkonen Date: Wed, 26 Aug 2026 13:46:03 -0400 Subject: [PATCH 8/8] Fix skill step numbering and subagent Resolve-dispatch wording 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. --- .github/workflows/sdlc-sdk-update-evaluate.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/sdlc-sdk-update-evaluate.yml b/.github/workflows/sdlc-sdk-update-evaluate.yml index c4d77e0c5d24..b4dd3bc21c7e 100644 --- a/.github/workflows/sdlc-sdk-update-evaluate.yml +++ b/.github/workflows/sdlc-sdk-update-evaluate.yml @@ -165,13 +165,13 @@ jobs: bitwarden-delivery-tools@bitwarden-marketplace bitwarden-software-engineer@bitwarden-marketplace prompt: | - PR #${{ github.event.pull_request.number }} in ${{ github.repository }} bumps the Bitwarden SDK to ${{ steps.gate.outputs.version }}. Evaluate it yourself using the `evaluating-sdk-internal-updates` skill's Identify steps (1-7). + PR #${{ github.event.pull_request.number }} in ${{ github.repository }} bumps the Bitwarden SDK to ${{ steps.gate.outputs.version }}. Evaluate it yourself using the `evaluating-sdk-internal-updates` skill's Identify steps (1-8). `bitwarden/sdk-internal` is already cloned as a sibling directory at `${{ github.workspace }}/../sdk-internal`; use it directly, do not re-clone. `npm ci` has already run. A baseline type check has already been run for you and its output is at `${{ steps.baseline.outputs.log }}`; read it rather than running your own baseline. - If step 7 turns up anything to resolve, dispatch a single subagent on the `bitwarden-software-engineer` agent to carry out the skill's Resolve steps (8-10) for every finding in one pass — decide the fix, implement it, and commit on the current branch (already checked out) using `Skill(bitwarden-delivery-tools:committing-changes)` for the message. Hand the subagent all the findings (commit, symbol, call sites for each) rather than having it redo the investigation, and rather than spawning one subagent per finding. Do not push; the workflow pushes separately. Skip the subagent entirely if step 7 finds nothing to resolve. + If step 8 turns up anything to resolve, dispatch a single subagent on the `bitwarden-software-engineer` agent to invoke `Skill(evaluating-sdk-internal-updates)` itself and carry out its Resolve steps (9-10) for every finding in one pass — decide the fix, implement it, and commit on the current branch (already checked out) using `Skill(bitwarden-delivery-tools:committing-changes)` for the message. Hand the subagent all the findings (commit, symbol, call sites for each) so it doesn't redo the investigation, dispatching it once for every finding rather than once per finding. Do not push; the workflow pushes separately. Have it report back the commit SHA and each finding's disposition. Skip the subagent entirely if step 8 finds nothing to resolve. - 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 7 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. + 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"