Skip to content

llm: Pre-fetch the SDK artifact before Claude runs in SDK-eval workflow - #7270

Merged
SaintPatrck merged 2 commits into
mainfrom
llm/fix-sdk-eval-prefetch-sdk
Aug 14, 2026
Merged

llm: Pre-fetch the SDK artifact before Claude runs in SDK-eval workflow#7270
SaintPatrck merged 2 commits into
mainfrom
llm/fix-sdk-eval-prefetch-sdk

Conversation

@SaintPatrck

Copy link
Copy Markdown
Contributor

🎟️ Tracking

#7267 — the SDK-eval workflow's compile verification still could not authenticate to GitHub Packages after #7268.

📔 Objective

claude-code-action assigns its own resolved GitHub token to GITHUB_TOKEN and GH_TOKEN in the environment it hands Claude's Bash tool, so no value the workflow places in that environment reaches Gradle, and the token it substitutes carries no packages:read grant for the SDK package. Resolving the dependency in a plain shell step ahead of Claude populates the Gradle module cache instead, so the compile Claude runs reads the fixed SDK version from cache and needs no credential of its own.

continue-on-error keeps a real compile break flowing through to Claude, since diagnosing that break is the evaluation's purpose. Because that also tolerates a resolution or toolchain failure, which would leave the cache cold and the evaluation reporting an authentication error as though the bump caused it, a following step asserts the artifact is actually cached and fails loudly when it is not.

This keeps secrets.GITHUB_TOKEN out of every surface the agent can read, which matters on a public repository, and leaves the action's own token handling alone so the sticky evaluation comment keeps updating in place.

The evaluation's compile step could not resolve com.bitwarden:sdk-android and
failed with 401 Unauthorized. claude-code-action resolves its own GitHub token
and assigns it to GITHUB_TOKEN and GH_TOKEN in the environment it hands
Claude's Bash tool, so no value the workflow places in that environment reaches
Gradle, and the token it substitutes carries no packages:read grant for the SDK
package.

Resolving the dependency in a plain shell step ahead of Claude populates the
Gradle module cache, so the compile Claude runs reads the fixed SDK version
from cache and needs no credential of its own. continue-on-error keeps a real
compile break flowing through to Claude instead of ending the job, since
diagnosing that break is the evaluation's purpose. Because that also tolerates
a resolution or toolchain failure, which would leave the cache cold and the
evaluation reporting an authentication error as though the bump caused it, a
following step asserts the artifact is actually cached and fails loudly when it
is not.

This keeps secrets.GITHUB_TOKEN out of every surface the agent can read, which
matters on a public repository, and leaves the action's own token handling
alone so the sticky evaluation comment keeps updating in place rather than
posting a new comment per bump.
authenticator declares no product flavors, so the flavored task name does not
exist there, and Gradle rejects an unknown task before running anything.
@SaintPatrck SaintPatrck added the ai-review Request a Claude code review label Aug 14, 2026
@github-actions github-actions Bot added the t:llm Change Type - LLM related change (e.g. CLAUDE.md files) label Aug 14, 2026
@github-actions

github-actions Bot commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Claude Configuration Validation — PR #7270

Scope: one changed file, .claude/skills/evaluating-sdk-internal-updates/SKILL.md
(read from the .claude-pr/ snapshot per review policy; line numbers below are from that copy).

The entire diff is one line. Step 3 (line 33) changed from:

./gradlew <module>:compileStandardDebugKotlin

to:

./gradlew app:compileStandardDebugKotlin authenticator:compileDebugKotlin

Verdict: approve. No critical or major issues. The change is factually correct and an
improvement over the <module> placeholder. Findings below are minor, and most are pre-existing
content the diff did not touch.


1. Plugin Validation — SKIPPED (not applicable)

No plugin directories changed. There is no plugins/ or .claude-plugin/ content in this PR, so
the plugin-validator agent was not run.

2. Skill Review — RUN

plugin-dev:skill-reviewer reviewed the changed skill. I independently verified its factual
claims against the repo's Gradle config before reporting them.

Correctness of the changed line — CONFIRMED CORRECT

Claim Verification
app needs the Standard flavor in the task name app/build.gradle.kts:142-150 declares a mode dimension with standard (default) and fdroid. app:compileStandardDebugKotlin is valid.
authenticator must not have a flavor in the task name authenticator/build.gradle.kts declares no productFlavors at all — only buildTypes { debug, release } (line 87+). authenticator:compileDebugKotlin is the correct task; authenticator:compileStandardDebugKotlin would not resolve.
Both modules are genuinely SDK consumers Both reference libs.versions.bitwardenSdk (app/build.gradle.kts:83, authenticator/build.gradle.kts:69). Naming exactly these two is complete.

Naming both tasks explicitly also removes a real failure mode: a reader expanding the old
<module> placeholder would most likely have written
authenticator:compileStandardDebugKotlin, which does not exist.

Structure and quality checks — PASS

  • YAML frontmatter present and valid; name is kebab-case and matches the directory name.
  • description is third person, ~350 chars, with concrete triggers ("reviewing an SDK bump PR",
    "a bitwardenSdk version change", "triaging sdk-internal breaking changes"). No change needed.
  • 46 lines / 717 words — well within the 500-line limit. Single-file layout with no
    references/ or examples/ is appropriate at this size; splitting it would hurt readability.
  • Writing style is consistently imperative/infinitive with numbered steps.
  • No broken file references. The two Skill(...) targets resolve: implementing-android-code
    exists at .claude/skills/implementing-android-code/SKILL.md, and
    bitwarden-delivery-tools:committing-changes is a marketplace plugin referenced throughout
    .claude/CLAUDE.md.

Findings

MINOR / warning — line 45 says "task" (singular) but step 3 now runs two tasks

Step 10 reads "Verify with the same compile task used in step 3." After this change step 3 invokes
two tasks. Not functionally broken — the back-reference still resolves — but slightly misleading.

Remediation: pluralize, or inline the command so the step survives reordering:

10. Verify with the same compile command from step 3
    (`./gradlew app:compileStandardDebugKotlin authenticator:compileDebugKotlin`).

MINOR / warning — compile*Kotlin covers main source sets only, so test-source breaks slip past steps 3 and 10

Verified: 103 files under app/src/test and authenticator/src/test import real UniFFI types
(e.g. com.bitwarden.crypto.Kdf at app/src/test/.../SdkStateBridgeTest.kt:6,
AuthenticatorBridgeRepositoryTest.kt:11, KdfManagerTest.kt:9). An SDK signature change that
breaks a mock factory passes both step 3 and step 10 cleanly and only surfaces in CI.

This is a pre-existing gap, not introduced here, but the PR is already editing this exact line so
it is cheap to close:

./gradlew app:compileStandardDebugUnitTestKotlin authenticator:compileDebugUnitTestKotlin

(or add a sentence to step 10 noting that test compilation must also be checked). Optional for
this PR.

3. Configuration & Security Review — RUN

Used the claude-config-validator:reviewing-claude-config skill (skills checklist +
priority framework).

Security scan — CLEAN

  • No secrets. Pattern scan for API keys, tokens, passwords, bearer values, ghp_, AKIA over
    the changed file: no matches. No hardcoded credentials.
  • No committed local settings. git ls-files confirms no settings.local.json is tracked.
  • No settings, hook, agent, or command files changed, so there are no permission-scoping,
    auto-approval, or file-access changes to assess in this PR.
  • No dangerous commands introduced. The changed line adds only two read-only-ish Gradle
    compile tasks. No rm, chmod, curl | sh, or credential handling anywhere in the file.

Pre-existing observations (out of this PR's diff — informational only)

These are in unchanged parts of the file. Listing them for the maintainer; none should block this
PR, which does not touch them.

  • allowed-tools wildcard style (lines 6, 7, 11). Bash(git -C * log *),
    Bash(git -C * show *), and Bash(./gradlew *) use space-separated mid-string * wildcards.
    The convention documented in the validator's own reference is exact match or prefix:*
    (as correctly used on lines 5, 8, 9, 10), with an explicit note to "use specific command
    patterns, not wildcards." Whether these mid-string forms match at runtime depends on CLI
    version, so I am not asserting they fail — but if they do not match, line 11 is the rule that
    gates the very command this PR fixes, which would make step 3 prompt or be denied. Worth a
    maintainer check; if it needs fixing, Bash(./gradlew:*) is the safe form.
  • Step 1 may lack the tools it needs (line 31). "Locate the local bitwarden/sdk-internal
    clone (check sibling directories to this repo)" requires listing outside the workspace.
    Glob/Grep are workspace-scoped and no ls/find Bash rule is granted. Consider adding
    Bash(ls:*).
  • Write granted but unused (line 16). The workflow only edits existing call sites and
    reports. Dropping Write would tighten least privilege.
  • com.bitwarden.core is ambiguous (line 27). It is listed as a UniFFI-emitted package, but
    this repo's own :core module owns com.bitwarden.core.data.* (DispatcherManager,
    asSuccess, …). Combined with step 6's "grep the whole repo for the bare symbol name," this
    invites false positives. One disambiguating clause would help.
  • Adjacent file, likely the source of the confusion this PR fixes:
    .claude/skills/build-test-verify/SKILL.md:61 documents
    ./gradlew authenticator:testStandardDebugUnitTest, which cannot resolve for the same reason —
    :authenticator has no standard flavor. Correct task is authenticator:testDebugUnitTest.
    Worth a follow-up PR.

Summary

Section Status
1. Plugin validation Skipped — no plugins changed
2. Skill review (skill-reviewer) Pass — 2 minor warnings
3. Config & security review (reviewing-claude-config) Pass — no security issues

Errors (must fix): none.

Warnings (should fix, both optional for this PR): singular "task" wording at line 45; step 3/10
do not compile test sources despite 103 test files importing SDK types.

The one-line change was verified correct against app/build.gradle.kts and
authenticator/build.gradle.kts, and fixes a placeholder that would most likely have been
expanded into a non-existent Gradle task.

@github-actions

github-actions Bot commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

This PR replaces the $GITHUB_ENV credential export in the SDK-eval workflow with a plain-shell pre-fetch step that resolves and compiles ahead of claude-code-action, plus a follow-up assertion that the sdk-android .aar actually landed in the Gradle module cache, and pins step 3 of evaluating-sdk-internal-updates to the two concrete compile tasks. I verified that com.bitwarden:sdk-android is consumed only by :app and :authenticator (app/build.gradle.kts:277, authenticator/build.gradle.kts:229), so app:compileStandardDebugKotlin authenticator:compileDebugKotlin covers every SDK consumer, and that the task names match each module's flavor configuration (:app has a mode dimension, :authenticator has none). The env: GITHUB_TOKEN pattern on the pre-fetch step matches the existing convention in .github/workflows/sdlc-sdk-update.yml:228, and scoping the token to a single step rather than the whole job via $GITHUB_ENV narrows the surface the agent can read on a public repo. The verify step's cache path, version sed, find guard, and if: gate all check out against gradle/libs.versions.toml:32 and the modules-2/files-2.1 layout.

Code Review Details

No findings.

@codecov

codecov Bot commented Aug 14, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.25%. Comparing base (1e67c10) to head (702801d).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #7270   +/-   ##
=======================================
  Coverage   86.25%   86.25%           
=======================================
  Files         891      891           
  Lines       65294    65294           
  Branches     9808     9808           
=======================================
  Hits        56320    56320           
  Misses       5472     5472           
  Partials     3502     3502           
Flag Coverage Δ
app-data 17.87% <ø> (ø)
app-ui-auth-tools 18.76% <ø> (ø)
app-ui-platform 16.41% <ø> (ø)
app-ui-vault 27.32% <ø> (ø)
authenticator 6.09% <ø> (ø)
lib-core-network-bridge 4.10% <ø> (ø)
lib-data-ui 1.20% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@SaintPatrck
SaintPatrck marked this pull request as ready for review August 14, 2026 17:20
@SaintPatrck
SaintPatrck requested review from a team as code owners August 14, 2026 17:20
@SaintPatrck
SaintPatrck added this pull request to the merge queue Aug 14, 2026
Merged via the queue into main with commit 38d79cb Aug 14, 2026
28 of 33 checks passed
@SaintPatrck
SaintPatrck deleted the llm/fix-sdk-eval-prefetch-sdk branch August 14, 2026 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review Request a Claude code review t:llm Change Type - LLM related change (e.g. CLAUDE.md files)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants