fix: resolve all four library bugs; drive honest unit coverage to ~95%#368
Merged
Conversation
…-file strips
Three gaps in what "covered" means, found while auditing the denominator:
* Pure-Dart packages ignored // coverage:ignore-* markers: flutter test
honors them natively (ignore-marked platform.dart never appears in Flutter
lcov) but coverage:format_coverage only does so with --check-ignore
(bin/format_coverage.dart:223, checkIgnoredLines). Added the flag so both
halves of the repo obey the same ignore semantics.
* oidc_cli/lib/src/version.dart is generated ("Generated code. Do not
modify.") but does not match the *.g.dart convention -- added a second
remove_from_coverage pattern (multi -r verified empirically against
remove_from_coverage 2.0.0).
* The upload-coverage job combined integration lcov straight into the
Codecov upload with NO generated-file strip at all -- *.g.dart from
integration runs reached Codecov. Added the same strip step there, and
codecov.yml ignore globs as the reporting-side backstop (generated files,
example app, version.dart).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BYvJ2hcGrcZbY3aKGy7JW2
… runs The android/ios/linux/windows patrol jobs passed `--coverage-package oidc`. patrol_cli_plus treats that as an UNANCHORED regex over package names (commands/test.dart:222 -> RegExp.hasMatch in coverage_tool.dart:355), so all oidc* packages were already harvested -- but jose_plus, x509_plus and crypto_keys_plus were not, despite the conformance flows exercising them heavily (JWT signature verification, JWKS certificate parsing). The artifact numbers agree: jose_plus integration delta over unit coverage was exactly zero. Switch the four native patrol jobs to `--coverage-workspace`, the fork's purpose-built flag that includes every `workspace:` member from the root pubspec (coverage_tool.dart:358-371). Verified the flag exists in the published patrol_cli_plus 5.4.1 that CI installs: it landed in dee5f944a, which is contained in the patrol_cli_plus-v5.4.1 tag. The macos job uses plain `flutter test`, whose --coverage-package is a regex over package names (flutter test --help); replace the sloppy `"oidc*"` (regex, not glob: matches "oid" + any number of "c") with an anchored `"^(oidc|jose_plus|x509_plus|crypto_keys_plus)"`. The web job keeps `--coverage-package oidc`: its V8->lcov collector is a separate best-effort path (test.dart:402 guards the VM-service coverage route to !isWeb). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BYvJ2hcGrcZbY3aKGy7JW2
…om well-known URI Closes #356. `Uri.replace(query: null, fragment: null)` keeps the existing components (null means "unchanged"), so getIssuerFromOpenIdConfigWellKnownUri returned issuers carrying the original query/fragment, contradicting its own doc. `query: ''` is no fix either -- it serializes a dangling '?'. Build the issuer fresh from components instead (userInfo omitted when empty, port omitted when !hasPort so no default port is injected). Behavior note: a query-carrying well-known URL with the standard path layout (e.g. Entra's `?appid=` form) now inverts to a clean issuer instead of a query-carrying one; the doc comment previously called that form "cannot be inverted" and has been corrected. Downstream issuer mix-up defense stays with issuersAreIdentical. Tests: both pre-existing happy-path tests pass byte-identical; new tests cover query+fragment stripping (hasQuery/hasFragment false, no dangling '?'), the Entra `?appid=` derivation, and userInfo/non-default-port preservation. Adversarially verified by an independent reviewer lane. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BYvJ2hcGrcZbY3aKGy7JW2
…rence numbers Closes #357, #358. #357: ObjectIdentifier.toAsn1() manually base-128-encoded the OID content bytes and handed them to ASN1ObjectIdentifier(...), whose constructor (asn1lib 1.6.5) expects plain arc components and encodes them itself -- every re-encode path (AlgorithmIdentifier/Name/SubjectPublicKeyInfo/ X509Certificate.toAsn1, toPem) emitted doubly-encoded, corrupt DER. Now `return ASN1ObjectIdentifier(nodes);`. Proven by round-trip tests over five real OIDs, an independent DER cross-check re-parsing toAsn1().encodedBytes with ASN1Parser, and a vm-only test re-encoding rfc5280_cert1.cer's subject/issuer Names and recovering the original OIDs. #358: NoticeReference.fromAsn1 assigned toDart(...)'s List<dynamic> (of BigInt) to List<int> noticeNumbers -- a TypeError making any UserNotice with a noticeRef unparseable. Now mapped explicitly to List<int>. Proven by an in-memory noticeRef parse test ([1,2,300]). Both replace tests that previously had to avoid the buggy paths. Adversarially verified by an independent reviewer lane. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BYvJ2hcGrcZbY3aKGy7JW2
…ing crit Closes #359. `getTyped('crit')` returned the raw List<dynamic> JSON value cast to List<String>?, throwing a TypeError on any header that actually uses the parameter the getter exists for. Use getTypedList('crit'), exactly as `audience` does for 'aud' (lib/src/jwt.dart:37). Tests cover both the crit-present (['exp']) and crit-absent (null) reads. Adversarially verified by an independent reviewer lane. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BYvJ2hcGrcZbY3aKGy7JW2
Second campaign wave (10-lane parallel workflow, every lane independently
adversarially verified in a fresh context).
Surface honesty: each of the 16 packages gains test/library_surface_test.dart
importing its public barrels with meaningful assertions, so every file with
coverable lines enters the coverage denominator -- lcov only ever sees
LOADED libraries, so real code could previously hide entirely (and did:
oidc_web_core/src/user_manager_web.dart, 94 raw lines, appeared in no
report). Files still absent after loading are provably noise: the VM
collector emits no source entry for zero-executable-statement files
(verified against the raw coverage JSON for the empty-class par/req.dart
and loopback mock.dart, and const-only native_channel_constants.dart).
Cover lanes (verifier-measured, per own probes):
oidc_core 1838/2247 -> 2135/2247 (95.0%) 3 new suites incl.
user-manager flow/internals coverage
oidc_cli 444/624 -> 539/581 (92.8%)
crypto/x509/jose 2320/2367 -> 2360/2367 (99.7%)
flutter tails 429/457 -> 474/474 (100%)
web pair 216/237 -> 284/421 (oidc_web.dart is now visible and
honestly uncovered -- see the structural finding below)
Policy-compliant coverage:ignore markers (each with justification, each
audited by the lane verifier as genuinely unit-untestable): oidc_cli's
real-browser Process.run launches and the `dart pub token add` shell-out
(mutates the developer's real pub credential store; no injection seam).
Also removes a stale coverage:ignore-file from oidc_platform_interface's
platform.dart, whose 17 lines were already fully covered.
Known limitation recorded, not hidden: oidc_web/lib/oidc_web.dart compiles
only for browser targets, and CI's `flutter test` runs on the VM, so its
@teston('chrome') surface test is silently skipped there; its lines stay
visible-and-uncovered in the unit artifact until the tooling gap is closed
(tracked in a follow-up issue). Dead-code findings in oidc_cli login
commands are documented in the coverage tests rather than enshrined.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BYvJ2hcGrcZbY3aKGy7JW2
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR expands cross-package coverage, filters generated files from LCOV reporting, and adds targeted regression tests and fixes for OIDC core, JOSE, crypto, CLI, platform, and X.509 behavior. ChangesCoverage collection and filtering
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/oidc_cli/test/src/support/oidc_test_server.dart (1)
41-62: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReplace
?refreshTokenwith a conditional map entry.?refreshTokenis not valid Dart map syntax here; useif (refreshToken != null) 'refresh_token': refreshTokeninstead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/oidc_cli/test/src/support/oidc_test_server.dart` around lines 41 - 62, In the token response map returned by the test server helper, replace the invalid `?refreshToken` entry with a Dart collection-if entry: include `'refresh_token': refreshToken` only when `refreshToken != null`, preserving omission when no refresh token is provided.
🧹 Nitpick comments (2)
.github/workflows/tests.yaml (1)
489-495: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winTemplate injection: secret expanded via
${{ }}directly inrun:instead of shell env var.zizmor flags line 491:
${{env.OIDC_CONFORMANCE_TOKEN}}is interpolated directly into the shell script, whereas every other job in this file (android, ios, linux, windows) passes the same secret throughenv:and references it as a bash variable ($OIDC_CONFORMANCE_TOKEN/${OIDC_CONFORMANCE_TOKEN}). Direct${{ }}expansion in arun:block is a known anti-pattern since values are substituted as raw text before the shell parses the script, risking shell-metacharacter injection.🔒 Proposed fix: use shell variable expansion consistent with other jobs
- name: Integration Tests working-directory: packages/oidc/example + env: + OIDC_CONFORMANCE_TOKEN: ${{ secrets.OIDC_CONFORMANCE_TOKEN }} run: | # Work around flaky desktop integration test runs when executing # multiple test entrypoints in a single `flutter test integration_test`. flutter test integration_test/app_test.dart \ --coverage --branch-coverage --coverage-package "^(oidc|jose_plus|x509_plus|crypto_keys_plus)" \ -d macos --coverage-path coverage/macos-app-coverage.info \ - --dart-define=CI=true --dart-define=OIDC_CONFORMANCE_TOKEN=${{env.OIDC_CONFORMANCE_TOKEN}} \ + --dart-define=CI=true --dart-define=OIDC_CONFORMANCE_TOKEN=${OIDC_CONFORMANCE_TOKEN} \ -r expanded flutter test integration_test/offline_mode_test.dart \ --coverage --branch-coverage --coverage-package "^(oidc|jose_plus|x509_plus|crypto_keys_plus)" \ -d macos --coverage-path coverage/macos-offline-coverage.info \ - --dart-define=CI=true --dart-define=OIDC_CONFORMANCE_TOKEN=${{env.OIDC_CONFORMANCE_TOKEN}} \ + --dart-define=CI=true --dart-define=OIDC_CONFORMANCE_TOKEN=${OIDC_CONFORMANCE_TOKEN} \ -r expanded - env: - OIDC_CONFORMANCE_TOKEN: ${{ secrets.OIDC_CONFORMANCE_TOKEN }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/tests.yaml around lines 489 - 495, Replace the direct `${{env.OIDC_CONFORMANCE_TOKEN}}` interpolation in the Flutter test command with the shell variable `${OIDC_CONFORMANCE_TOKEN}`, and expose the secret through the step or job `env:` configuration. Keep the existing `OIDC_CONFORMANCE_TOKEN` environment mapping consistent with the other platform test jobs.Source: Linters/SAST tools
packages/oidc_cli/lib/src/cli_user_manager.dart (1)
106-130: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract duplicated browser-launch logic into a shared helper.
The platform-specific
Process.runbrowser launch block is identical in bothgetAuthorizationResponse(lines 116–129) andgetEndSessionResponse(lines 179–192). The coverage:ignore blocks now make this duplication more conspicuous. Extracting a_launchBrowser(Uri uri)helper would eliminate the copy-paste and ensure future fixes apply to both flows.♻️ Proposed refactor: extract a shared browser-launch helper
+ /// Launches the host OS default browser to [uri]. + /// Unconditionally shells out — no injection seam, so coverage is ignored. + Future<void> _launchBrowser(Uri uri) async { + try { + if (Platform.isWindows) { + await Process.run('rundll32', [ + 'url.dll,FileProtocolHandler', + uri.toString(), + ]); + } else if (Platform.isMacOS) { + await Process.run('open', [uri.toString()]); + } else if (Platform.isLinux) { + await Process.run('xdg-open', [uri.toString()]); + } + } on Exception catch (_) { + // ignore + } + } + `@override` Future<OidcAuthorizeResponse?> getAuthorizationResponse( ... printFunction: (uri) async { cliLogger.info('Please open the following link: $uri'); // coverage:ignore-start - // Unconditionally launches the host OS's real default browser via a - // subprocess. Exercising this in a unit test would pop a real - browser window as a side effect of running `dart test` (there is - // no injection seam for the OS-open call, unlike `oidc_desktop`'s - // `launchUrl` callback), so the whole login/logout suite - // deliberately never reaches this branch — see the note atop - // `login_interactive_command_test.dart` and - // `pub_proxy_command_test.dart` for the same convention applied to - // the analogous `dart pub token add` subprocess in `dart_pub.dart`. - try { - if (Platform.isWindows) { - await Process.run('rundll32', [ - 'url.dll,FileProtocolHandler', - uri.toString(), - ]); - } else if (Platform.isMacOS) { - await Process.run('open', [uri.toString()]); - } else if (Platform.isLinux) { - await Process.run('xdg-open', [uri.toString()]); - } - } on Exception catch (_) { - // ignore - } + // See `_launchBrowser`: no injection seam for the OS-open call. + await _launchBrowser(uri); // coverage:ignore-end }, ... // And similarly in getEndSessionResponse: printFunction: (uri) async { logger.info('Please open the following link: $uri'); // coverage:ignore-start - // See the matching note in `getAuthorizationResponse` above: this - // unconditionally shells out to launch the host OS's real default - // browser, which unit tests deliberately never trigger. - try { - if (Platform.isWindows) { - await Process.run('rundll32', [ - 'url.dll,FileProtocolHandler', - uri.toString(), - ]); - } else if (Platform.isMacOS) { - await Process.run('open', [uri.toString()]); - } else if (Platform.isLinux) { - await Process.run('xdg-open', [uri.toString()]); - } - } on Exception catch (_) { - // ignore - } + await _launchBrowser(uri); // coverage:ignore-end },Also applies to: 175-193
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/oidc_cli/lib/src/cli_user_manager.dart` around lines 106 - 130, Extract the duplicated platform-specific browser-launch Process.run logic from getAuthorizationResponse and getEndSessionResponse into a private _launchBrowser(Uri uri) helper. Move the shared coverage-ignore and exception handling into the helper, then replace both inline blocks with calls to _launchBrowser using their respective URI values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/oidc_cli/test/src/support/oidc_test_server.dart`:
- Around line 41-62: In the token response map returned by the test server
helper, replace the invalid `?refreshToken` entry with a Dart collection-if
entry: include `'refresh_token': refreshToken` only when `refreshToken != null`,
preserving omission when no refresh token is provided.
---
Nitpick comments:
In @.github/workflows/tests.yaml:
- Around line 489-495: Replace the direct `${{env.OIDC_CONFORMANCE_TOKEN}}`
interpolation in the Flutter test command with the shell variable
`${OIDC_CONFORMANCE_TOKEN}`, and expose the secret through the step or job
`env:` configuration. Keep the existing `OIDC_CONFORMANCE_TOKEN` environment
mapping consistent with the other platform test jobs.
In `@packages/oidc_cli/lib/src/cli_user_manager.dart`:
- Around line 106-130: Extract the duplicated platform-specific browser-launch
Process.run logic from getAuthorizationResponse and getEndSessionResponse into a
private _launchBrowser(Uri uri) helper. Move the shared coverage-ignore and
exception handling into the helper, then replace both inline blocks with calls
to _launchBrowser using their respective URI values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2171f7fe-563a-4618-acac-420ca2b0637a
📒 Files selected for processing (60)
.github/workflows/tests.yamlcodecov.ymlpackages/crypto_keys_plus/test/library_surface_test.dartpackages/crypto_keys_plus/test/pointycastle_ext_test.dartpackages/crypto_keys_plus/test/unexpected_key_type_test.dartpackages/jose_plus/lib/src/jose.dartpackages/jose_plus/test/ecdh_error_test.dartpackages/jose_plus/test/jose_test.dartpackages/jose_plus/test/jwa_error_test.dartpackages/jose_plus/test/jwe_error_test.dartpackages/jose_plus/test/jwk_error_test.dartpackages/jose_plus/test/library_surface_test.dartpackages/oidc/test/library_surface_test.dartpackages/oidc_android/test/library_surface_test.dartpackages/oidc_android/test/oidc_android_test.dartpackages/oidc_cli/lib/src/cli_user_manager.dartpackages/oidc_cli/lib/src/commands/login/login_device_command.dartpackages/oidc_cli/lib/src/commands/login/login_interactive_command.dartpackages/oidc_cli/lib/src/commands/login/login_password_command.dartpackages/oidc_cli/lib/src/commands/proxy/pub_proxy_command.dartpackages/oidc_cli/lib/src/utils/dart_pub.dartpackages/oidc_cli/test/library_surface_test.dartpackages/oidc_cli/test/src/cli_user_manager_test.dartpackages/oidc_cli/test/src/command_runner_test.dartpackages/oidc_cli/test/src/commands/login/login_device_command_test.dartpackages/oidc_cli/test/src/commands/login/login_interactive_command_test.dartpackages/oidc_cli/test/src/commands/login/login_password_command_test.dartpackages/oidc_cli/test/src/commands/logout_command_test.dartpackages/oidc_cli/test/src/commands/oidc_base_command_test.dartpackages/oidc_cli/test/src/commands/proxy/pub_proxy_command_test.dartpackages/oidc_cli/test/src/commands/token/token_commands_test.dartpackages/oidc_cli/test/src/commands/update_command_test.dartpackages/oidc_cli/test/src/file_oidc_store_test.dartpackages/oidc_cli/test/src/support/oidc_test_server.dartpackages/oidc_core/lib/src/utils.dartpackages/oidc_core/test/internal_utils_test.dartpackages/oidc_core/test/library_surface_test.dartpackages/oidc_core/test/pure_models_coverage_test.dartpackages/oidc_core/test/user_manager_flows_coverage_test.dartpackages/oidc_core/test/user_manager_internals_coverage_test.dartpackages/oidc_darwin/test/library_surface_test.dartpackages/oidc_darwin/test/oidc_darwin_test.dartpackages/oidc_default_store/test/library_surface_test.dartpackages/oidc_desktop/test/library_surface_test.dartpackages/oidc_desktop/test/oidc_desktop_test.dartpackages/oidc_linux/test/library_surface_test.dartpackages/oidc_loopback_listener/test/library_surface_test.dartpackages/oidc_platform_interface/lib/src/platform.dartpackages/oidc_platform_interface/test/library_surface_test.dartpackages/oidc_web/test/library_surface_test.dartpackages/oidc_web_core/test/library_surface_test.dartpackages/oidc_web_core/test/oidc_web_store_test.dartpackages/oidc_windows/test/library_surface_test.dartpackages/x509_plus/lib/src/extension.dartpackages/x509_plus/lib/src/objectidentifier.dartpackages/x509_plus/test/certificate_extra_test.dartpackages/x509_plus/test/extension_extra_test.dartpackages/x509_plus/test/library_surface_test.dartpackages/x509_plus/test/objectidentifier_test.dartpubspec.yaml
💤 Files with no reviewable changes (1)
- packages/oidc_platform_interface/lib/src/platform.dart
…etic Review findings on #368 (one blocking, two advisory), all three addressed: * certificate_extra_test.dart read DER fixtures in setUpAll with no platform pin -- the one unpinned dart:io fixture reader among the new tests, and a guaranteed failure on the un-swallowed `dart test --platform chrome` CI step. Now @teston('vm'), matching the #353 convention. Verified: x509_plus on chrome 60/60 (previously failed in isolation), vm 99/99. * The one pub-proxy test that reaches the real `dart` binary forwarded `cache list`, whose runtime scales with the machine's global pub cache (timed out at 30s on a large dev cache). Forward `token list` instead: read-only and O(token store). 5/5 in ~8s. * dart_pub.dart's ignore-file rationale leaked an authoring-session phrase ("the hard constraint elsewhere in this task"); trimmed to the technical rationale only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BYvJ2hcGrcZbY3aKGy7JW2
…wser loads fast Run 29118828702's unit_tests jobs died at GitHub's 6-hour limit: on the Firefox step, four packages finished by 21:07 and the remaining two (oidc_web_core, oidc_loopback_listener) never printed a single line for 4.5 hours -- the job-kill cleanup shows a pile of orphaned Firefox helper processes, i.e. a wedged browser launch. The code is exonerated: the full oidc_web_core suite passes 36/36 on local Firefox in 2 seconds, and loopback's suite is vm-pinned. The amplifier was our own flag: --ignore-timeouts disables ALL package:test timeouts including the browser-load timeout, so a routine launch flake became an infinite hang. Keep --timeout 60m (jose_plus's crypto tests are legitimately slow under dart2js) and drop --ignore-timeouts from the three web-test invocations: a wedged load now fails loudly in minutes and the job can simply be rerun. The iOS integration script's --ignore-timeouts is a different context and is untouched. Also adds the set -e that the #363 sweep missed on test:coverage (same trailing-statement swallow shape; not CI-invoked, fixed for consistency). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BYvJ2hcGrcZbY3aKGy7JW2
… fast The ed7b535 run proved the Firefox launch wedge recurs: its unit_tests jobs sat on the Firefox step for 3h13m (vs ~66min for the entire step on green runs). Dropping --ignore-timeouts made the wedge BOUNDED but not cheap: with no explicit --suite-load-timeout, package:test scales the suite connect/load timeout up to the per-test cap (60m), so every wedged launch still burned up to an hour before failing. --suite-load-timeout 4m (flag verified in dart test --help; load includes the dart2js compile) is ~5x the worst legitimate suite compile+load observed on green CI and converts a wedge into a fast, named failure that a simple job rerun clears. Applied to all three web-test invocations; implements the reviewer's advisory from the ed7b535 approval. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BYvJ2hcGrcZbY3aKGy7JW2
…e-load bound
My --suite-load-timeout 4m in melos test:web:single false-failed 8 suites
on the beta chrome step of run 29140552735 ("loading <file> (failed)" +
"TimeoutException after 0:04:00"). The timer evidently spans queue/
starvation time caused by jose_plus's crypto suites (keygen/sign/encrypt),
which take ~55min per browser under dart2js's ~200x slower BigInt
(#371) and starve other suites' loads while they run -- so any bound
under that window is wrong. Remove the flag entirely; --timeout 60m and
set -e stay, --ignore-timeouts stays removed (see the existing comment
on why: a wedged Firefox launch silently hung for 4.5h without it,
run 29118828702).
Instead, tag jose_plus's crypto-executor suites (jwa/jwe/jwk/ecdh/jws/
jwt_test.dart) as slow-web-crypto via a new packages/jose_plus/
dart_test.yaml and exclude them from PULL-REQUEST web runs only, via a
new WEB_EXCLUDE_TAGS env var threaded through melos's test:web:single
into `dart test --exclude-tags`. VM runs always execute every tag (full
PR coverage), and main-branch pushes keep full web crypto signal.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BYvJ2hcGrcZbY3aKGy7JW2
Review finding on 3e070c4: pss_eddsa_jws_test.dart generates RSA-PSS keys (PS256/PS384/PS512) and signs/verifies -- measured 4:06 standalone under dart2js, the dominant slice of the remaining PR-web time -- but was missed by the six-file tag sweep. Tagged identically. PR-web chrome run drops from 6:01 (140 tests) to 2:23 (129 tests); VM still runs all 233. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BYvJ2hcGrcZbY3aKGy7JW2
With the crypto suites excluded, the measured remainder of the browser steps is dart2js COMPILATION: dart test compiles every test file separately (~30-60s each on CI runners), and the web sweep spans ~100 suites across six packages -- oidc_core alone is 60+ -- twice per browser per matrix leg. Most of that buys nothing per-PR: crypto_keys_plus, x509_plus, jose_plus and oidc_loopback_listener have zero web-conditional or js-interop lib code (verified by grep), so their browser runs duplicate the VM signal exactly. Only oidc_web_core (4 js-interop files) genuinely diverges; oidc_core's divergence is a single 6-line conditional stub. Pull requests now browser-test only oidc_web_core (new test:web:pr:chrome/firefox melos scripts, packageFilters scope); pushes to main keep the full sweep on both browsers. Expected PR web time: minutes instead of the better part of an hour per browser per leg. Full-platform coverage semantics are unchanged on main. Windows note: melos exec runs multi-line scripts through cmd.exe locally, so these scripts (like the existing test:web:single) are CI/sh-only; the scope filter was verified locally (resolves to exactly oidc_web_core) and the underlying per-package browser suites pass on both chrome and firefox. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BYvJ2hcGrcZbY3aKGy7JW2
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pubspec.yaml (1)
155-186: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor:
test:web:pr:singlealways collects coverage whiletest:web:singleskips it forminSDK runs.
test:web:pr:single(line 151) unconditionally passes--coverage=coverage/${TEST_PLATFORM}, whereastest:web:single(line 180) omits coverage forTARGET_DART_SDK=min. If PR-scoped runs ever execute under theminSDK, this would add unnecessary coverage instrumentation overhead. Currently harmless given the single-package scope, but worth noting for consistency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pubspec.yaml` around lines 155 - 186, Update the test:web:pr:single configuration to mirror test:web:single: omit --coverage=coverage/${TEST_PLATFORM} when TARGET_DART_SDK is min, while preserving coverage for non-min SDK runs and any existing WASM behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pubspec.yaml`:
- Around line 155-186: Update the test:web:pr:single configuration to mirror
test:web:single: omit --coverage=coverage/${TEST_PLATFORM} when TARGET_DART_SDK
is min, while preserving coverage for non-min SDK runs and any existing WASM
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f9a1ff29-6366-49d6-841b-47eb79d0c7dd
📒 Files selected for processing (2)
.github/workflows/tests.yamlpubspec.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/tests.yaml
…efault package:test 1.31 puts no timeout on suite loads (suiteLoadTimeout defaults to Timeout.none), so a wedged browser suite load hangs until GitHub's 6h job kill. Run 29146393931 hit this twice on the PR-scoped steps: Firefox, then Chrome on the rerun, each finished one suite and stalled on the next load (63+ min on a ~20s step, past the --timeout 60m mark — proving no timer covers loads; --timeout only bounds tests). - test:web:pr:single: --suite-load-timeout 4m (three ~10s suite loads, no queue starvation) and drop --timeout 60m (sub-second tests; the 30s default also bounds a mid-test wedge). - test:web:single: --suite-load-timeout 2h on the full sweep — the load timer keeps running while suites queue behind jose's ~55min crypto executors, so the bound must clear that (a 4m bound false-failed 8 healthy suites, run 29140552735). 2h turns a wedge into a loud, rerunnable failure instead of a 6h hang. - Fix the stale comment claiming wedged loads fail loudly by default. Verified locally: the new PR-scope command runs oidc_web_core's 36 tests in 13s (chrome, coverage on, exit 0) under the locked test 1.31.0. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01McVLYD2qr9r6Hec2MpKZaU
The dart-level bounds added in e362d42 cover suite loads and test execution, but run 29148902320 proved a browser wedge can strike outside both: a Firefox PR-scope step hung 57+ minutes WITH the 4m suite-load bound active, so the hang was in browser launch, shutdown, or loader orchestration — phases no package:test timer covers. step timeout-minutes is the layer that bounds every hang shape: the runner kills the process tree and fails the step loudly, rerunnable instead of idling to GitHub's 6h job kill. PR-scope steps: 15m (observed 19-31s). Full-sweep steps: 150m (observed ~55-66min per browser on main pushes). Also corrects the suite count in the test:web:pr:single comment (oidc_web_core has five test suites, not three). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01McVLYD2qr9r6Hec2MpKZaU
…ss exit hang Log forensics across all three wedged runs (29146393931, 29148902320, 29150605150) show the same shape: every suite loads, every test passes, then dart test never exits — the runner idles from the last passing test until an outer timeout. Both browsers, both matrix legs. The earlier load-wedge reading was wrong: the hang is in the post-success phase (browser/suite teardown + Chrome coverage collection). dart-lang/test#2294 documents a multi-suite post-success exit hang where the runner reaches a successful end and a leaked async task keeps the process alive; --concurrency=1 is the reporter-verified workaround. - test:web:pr:single: add --concurrency=1 (five ~10s suites; serial costs seconds) and drop --coverage (removes the other post-success moving part; PR browser coverage is informational-only — codecov gets browser coverage from the full sweep on main pushes). - Keep --suite-load-timeout 4m and the step-level timeout-minutes backstop from the previous commits. Verified locally: the exact new command passes oidc_web_core's 36 tests (chrome, 1m26s, exit 0); 12 consecutive local runs of the OLD flags also pass on Windows — the hang reproduces only on CI Linux. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01McVLYD2qr9r6Hec2MpKZaU
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #368 +/- ##
===========================================
+ Coverage 74.48% 92.18% +17.70%
===========================================
Files 158 119 -39
Lines 8446 6131 -2315
Branches 1572 2151 +579
===========================================
- Hits 6291 5652 -639
+ Misses 2155 479 -1676 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #356, #357, #358, #359.
Bug fixes:
oidc_core: issuer derived from a well-known URL kept its query/fragment (Uri.replace(query: null)keeps the existing value). Rebuilt from components.x509_plus:toAsn1()double-encoded OIDs, sotoPem()and every other re-encode path emitted corrupt DER. Also fixed a type error that made UserNotice extensions with a noticeRef unparseable.jose_plus: readingJoseHeader.criticalthrew whenever the header actually hadcrit.Tests and coverage:
coverage:ignoreonly where no test seam exists (browser launch,dart pub token add), justified inlineCI:
--coverage-workspace); previously jose/x509/crypto were silently excludedformat_coverage --check-ignore, generated-file strips on the Codecov uploadFollow-ups: #369, #370, #371.
🤖 Generated with Claude Code
https://claude.ai/code/session_01BYvJ2hcGrcZbY3aKGy7JW2
Summary by CodeRabbit
Bug Fixes
critheader decoding so it’s parsed as the expected list form.Tests
Chores