Skip to content

fix(ci): unbreak Dart Format and Dart Analyze on main - #1181

Merged
hiroshihorie merged 7 commits into
mainfrom
hiroshi/fix-ci-format-analyze
Aug 26, 2026
Merged

fix(ci): unbreak Dart Format and Dart Analyze on main#1181
hiroshihorie merged 7 commits into
mainfrom
hiroshi/fix-ci-format-analyze

Conversation

@hiroshihorie

Copy link
Copy Markdown
Member

Build has been failing on main and therefore on every PR, regardless of what the PR changes — visible on #1180, max/fix-ios-audio-session-before-recording and sync-upstream-2.11.0, all of which show Build ✗ with Changeset Check ✓. Two jobs are responsible.

Dart Format

Three files under scripts/ are not formatted, so dart format . --set-exit-if-changed exits 1. Straight from the CI log:

Formatted scripts/check_version.dart
Formatted scripts/create_change.dart
Formatted scripts/create_version.dart
Formatted 227 files (3 changed) in 0.72 seconds.
##[error]Process completed with exit code 1

Formatting only, no behaviour change.

Dart Analyze

lib/src/token_source/caching.dart:138:9 • unawaited_return_in_try_block
lib/src/token_source/caching.dart:144:7 • unawaited_return_in_try_block

CachingTokenSource.fetch returned resultFuture from inside its try, so the surrounding catch could not observe a failure from that future and finally removed the in-flight entry before it settled. Both returns now await.

Behaviour is unchanged — the completer is completed on the line immediately before each return, so the awaited future is already resolved — but errors now route through the existing catch, and the in-flight map is cleared only once the request settles. AGENTS.md calls out un-awaited async state updates as a recurring bug class here, so this is a real latent issue rather than a lint silence. Includes a patch type="fixed" changeset.

The analysis_options churn, and a coverage hole it was hiding

flutter analyze kept leaving analysis_options.yaml modified. The cause is AnalysisOptionsMigration (flutter_tools/lib/src/migrations/analysis_options_migration.dart, invoked from project.dart:429), which appends seven patterns — build/**, android/**, ios/**, web/**, windows/**, macos/**, linux/** — on every flutter analyze / pub get / run. It is unconditional; there is no feature flag or config to disable it, unlike the UIScene migrator's enable-uiscene-migration.

Committing the patterns makes it a no-op. But one of them matters: this package keeps real Dart source in web/ (the E2EE worker), not just assets, so web/** silently drops it from analysis. That was already happening in CI, since flutter analyze runs the migrator before it analyzes.

Passing the path explicitly does not work — dart analyze web/ still honours the root exclude. I confirmed this by appending a deliberate type error to web/e2ee.logger.dart: it reported No issues found!. Giving web/ its own analysis_options.yaml creates a separate context that is genuinely analyzed; the same canary then correctly reports return_of_invalid_type. That file mirrors the root's error overrides and its formatter settings (page_width: 120, trailing_commas: preserve) so dart format output is unaffected, and build.yaml gains a dart analyze web/ step.

Verified locally

dart format . --set-exit-if-changed   PASS  (227 files, 0 changed)
flutter analyze                        PASS  (No issues found)
dart analyze web/                      PASS  (and fails on an injected error)
flutter test                           PASS  (+398)
dart run scripts/check_version.dart    PASS
import_sorter --exit-if-changed        PASS

Not included

Pinning the Flutter version in .github/actions/setup-flutter/action.yml, which currently passes only channel: stable and so tracks whatever the newest stable release is. That is a genuine fragility — unawaited_return_in_try_block arrived this way — but it is a separate decision from unbreaking CI, and worth its own discussion.

Draft because #1180 should confirm this actually turns Build green once rebased on it.

`dart format . --set-exit-if-changed` reformats three files under
scripts/, which fails the Dart Format CI job on main and therefore on
every PR. No behaviour change; formatting only.
`fetch` returned `resultFuture` from inside its `try` block, so the
surrounding `catch` could not observe a failure from that future and the
`finally` clause removed the in-flight entry before it settled. Dart
3.13's analyzer flags this as `unawaited_return_in_try_block`, failing
the Dart Analyze CI job; un-awaited async state updates are also a
recurring source of bugs in this codebase.

Both returns now `await` the future, so errors route through the existing
catch and the in-flight map is cleaned up only once the request is done.
The completer is completed immediately before each return, so the awaited
future is already resolved and behaviour is unchanged.
`AnalysisOptionsMigration` in flutter_tools appends build/**, android/**,
ios/**, web/**, windows/**, macos/** and linux/** to analysis_options.yaml
on every `flutter analyze`, `pub get` and `run`. It is unconditional --
there is no feature flag or config to disable it -- so the file showed up
as modified after any Flutter command.

Committing the patterns makes the migrator a no-op. It also documents the
one that matters here: web/ holds the E2EE worker sources, not just
assets, so excluding it silently drops them from analysis. That already
happens in CI today, because `flutter analyze` runs the migrator before
analyzing; the following commit restores that coverage explicitly.
The root analysis_options.yaml now excludes web/**, because Flutter's
AnalysisOptionsMigration forces that pattern in and cannot be turned off.
In this package web/ is not assets -- it holds the E2EE worker sources --
so that exclusion silently dropped them from analysis. This already
happened in CI, since `flutter analyze` runs the migrator before it
analyzes.

Passing the path explicitly does not help: `dart analyze web/` still
honours the root exclude and reports success on a file with a type error.
Giving web/ its own analysis_options.yaml creates a separate context that
is genuinely analyzed. Verified by appending a deliberate type error to
web/e2ee.logger.dart, which the new setup reports and the old one did not.

The file mirrors the root's error overrides and its formatter settings
(page_width 120, trailing_commas preserve) so `dart format` output is
unchanged, and CI gains a `dart analyze web/` step.
@hiroshihorie
hiroshihorie marked this pull request as ready for review August 26, 2026 17:36
devin-ai-integration[bot]

This comment was marked as resolved.

The web/ config only carried the analyzer and formatter sections, so the
E2EE sources lost prefer_single_quotes, prefer_final_locals,
unawaited_futures and discarded_futures. Confirmed via canary: those
lints did not fire before this change and do now. web/ passes clean.
@hiroshihorie
hiroshihorie merged commit d93f52d into main Aug 26, 2026
14 of 15 checks passed
@hiroshihorie
hiroshihorie deleted the hiroshi/fix-ci-format-analyze branch August 26, 2026 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant