fix: make workspace quality gates deterministic - #417
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new workspace bootstrapper should prune generated directories during discovery and should catch Process.start failures to provide actionable errors instead of stack traces.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR makes the repository’s documented quality gates (format/analyze) deterministic by introducing a single workspace bootstrap entrypoint that resolves all maintained package roots before running the root gates, and then aligning CI + contributor/maintainer guidance to use that same entrypoint.
Changes:
- Add
tool/prepare_workspace.dartto deterministicallypub getall maintained package roots (with lockfile enforcement for the chat app) and fail closed on unclassified/missing packages. - Update CI, the test-matrix command, PR template, and docs to run the workspace preparation step before format/analyze.
- Apply formatter-only changes to the previously reported test files.
File summaries
| File | Description |
|---|---|
| website/docs/maintainers/release-workflow.md | Adds workspace bootstrap to release checklist before format/analyze. |
| website/docs/maintainers/docs-site.md | Documents the new canonical preparation boundary and exclusions. |
| tool/testing/test_matrix.dart | Makes the “static-format-analyze” row fail-closed and prep-aware. |
| tool/prepare_workspace.dart | New deterministic workspace bootstrapper and manifest validation. |
| test/unit/tooling/test_matrix_test.dart | Asserts the test-matrix row keeps preparation ahead of format/analyze. |
| test/unit/tooling/prepare_workspace_test.dart | Adds unit coverage for manifest validation, ordering, lockfile enforcement, and symlink behavior. |
| test/unit/tooling/macos_litert_lm_prepare_app_script_test.dart | Formatter-only updates to match canonical formatting. |
| test/unit/core/engine/engine_test.dart | Formatter-only updates to match canonical formatting. |
| test/integration/engine_reloading_test.dart | Formatter-only updates to match canonical formatting. |
| test/integration/core/template/llama_cpp_template_detection_integration_test.dart | Formatter-only updates to match canonical formatting. |
| test/e2e/backends/llama_cpp_chat_template_backend_e2e_test.dart | Formatter-only updates to match canonical formatting. |
| README.md | Adds workspace preparation to the documented “package changes” command sequence. |
| CONTRIBUTING.md | Updates prerequisites and guidance to reflect Flutter requirement for repo-wide gates + new bootstrap step. |
| AGENTS.md | Replaces root dart pub get with the canonical workspace bootstrap command. |
| .github/workflows/ci.yml | Uses tool/prepare_workspace.dart as the CI dependency installation boundary. |
| .github/pull_request_template.md | Adds the workspace preparation command to the PR test plan checklist. |
Review details
- Files reviewed: 16/16 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
The workspace discovery ignore logic is not robust to real Flutter-generated Flutter/ephemeral and iOS .symlinks paths, which can make subsequent bootstrap runs fail with unclassified package errors.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
test/unit/tooling/prepare_workspace_test.dart:70
- The generated Flutter ephemeral tree path casing differs across platforms; in this repo it’s commonly
.../Flutter/ephemeral/...(capitalFlutter). Updating this fixture to use the real casing (and adding an iOS.symlinksexample) will ensure the ignore logic is validated against the actual generated directory names.
for (final path in <String>[
'example/chat_app/.dart_tool/generated/pubspec.yaml',
'example/chat_app/build/generated/pubspec.yaml',
'example/chat_app/macos/flutter/ephemeral/plugin/pubspec.yaml',
]) {
- Files reviewed: 16/16 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
The new canonical prepare_workspace entry point can terminate with an uncaught ProcessException (e.g., missing flutter), producing a stack trace instead of a stable exit code and actionable diagnostics.
Review details
Suppressed comments (1)
tool/prepare_workspace.dart:172
runWorkspaceCommanddoes not handleProcess.startfailures (e.g.,flutter/dartnot on PATH, permissions). In that case Dart will throw aProcessExceptionand the script will terminate with a stack trace instead of a clear, actionable error and stable exit code for CI/contributors.
final process = await Process.start(
executable,
arguments,
workingDirectory: workingDirectory,
mode: ProcessStartMode.inheritStdio,
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The changes are cohesive, well-tested for the new workspace bootstrap behavior, and consistently applied across CI and documentation without introducing runtime behavior changes.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The new bootstrap logic is explicit, fail-closed, CI-integrated, and covered by targeted unit tests that validate ordering, pruning, and failure propagation.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The bootstrap logic, CI integration, and documentation updates are consistent and are backed by targeted regression tests for the previously non-deterministic workspace conditions.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Final QA handoff: PR #417 remains draft/open/mergeable. Exact base |
There was a problem hiding this comment.
🟡 Changes recommended
There are verified documentation and tooling issues (CONTRIBUTING bootstrap path mismatch and generated-directory pruning gaps) that should be addressed before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
tool/prepare_workspace.dart:114
discoverWorkspacePubspecscurrently prunes.dart_tool,build,.symlinks, andFlutter/ephemeral, but it will still recursively traverse CocoaPods output likeios/Pods//macos/Pods/(these are gitignored inexample/chat_app/{ios,macos}/.gitignore). That can maketool/prepare_workspace.dartunexpectedly slow on machines that have built the iOS/macOS runners. Consider pruningPodsthe same way as other generated directories.
final name = segments.last;
return name == '.dart_tool' ||
name == 'build' ||
name == '.symlinks' ||
_isFlutterEphemeral(segments);
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The new deterministic workspace bootstrap is well-scoped, covered by targeted unit tests, and integrated consistently into CI and contributor documentation without introducing runtime behavior changes.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
A new unit test is platform-dependent (Windows path separators) and should be normalized to avoid failing on Windows.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
test/unit/tooling/prepare_workspace_test.dart:40
- The test builds
relativepaths usingsubstring, which preserves platform separators (e.g.example\chat_appon Windows) but later compares againstworkspacePackagesentries that use/. This makes the assertion platform-dependent and can fail on Windows.
commandRunner: (executable, arguments, workingDirectory) async {
final relative = workingDirectory == root.path
? '.'
: workingDirectory.substring(root.path.length + 1);
commands.add('$executable ${arguments.join(' ')} @ $relative');
return 0;
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The bootstrap behavior is well-scoped, consistently integrated across CI/docs/templates, and includes targeted unit tests covering the key determinism and failure-mode requirements.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The changes are tooling-only, well-covered by targeted unit tests, and consistently integrated across CI and documentation with no apparent runtime impact.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
tool/prepare_workspace.dart:161
prepareWorkspacereturns64for manifest/layout failures, but the exit code is a magic number here. Naming it (and documenting it as EX_USAGE) makes the intent clearer and reduces the chance of accidental drift if the code later needs to reuse the same exit code in multiple places.
final layoutErrors = validateWorkspaceManifest(repositoryRoot);
if (layoutErrors.isNotEmpty) {
for (final error in layoutErrors) {
stderr.writeln(error);
}
return 64;
}
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The bootstrap logic is cohesive and well-tested, CI is updated to use it consistently, and the documentation changes align with the new canonical quality-gate workflow.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
Closes #405.
Summary
.symlinks, build output, and CocoaPodsPodsThis is contributor-tooling scope only. Runtime APIs and supported model, backend, and platform behavior are unchanged.
Before / after
On the original report baseline, running only the root
dart pub getleft nested examples unresolved: repository-wide format exited 1 with unresolvedflutter_lintswarnings and five toolchain-dependent rewrites, while analysis reported 9,224 cascading nested-package issues.At exact head
2033e97c860bd2545b47011aaae1da077d3978bb, with the canonical current-stable toolchain:dart run tool/prepare_workspace.dartclassifies all eight maintained package roots and resolves the root plus five examplesThe two companion packages remain fail-closed manifest members but retain their existing dedicated dependency, analysis, test, SwiftPM, and publish-dry-run lanes. This avoids dirtying the root checkout with companion analyzer migrations while preserving their full validation.
Production completeness
/Validation
dart run tool/prepare_workspace.dart— 8 roots classified; root + 5 examples resolveddart format --output=none --set-exit-if-changed .— 567 files, 0 changesdart analyze— no issuesdart test -p vm -j 1 --exclude-tags local-only— 1,553 passed, 71 fixture-dependent skipsdart run tool/testing/check_platform_boundaries.dartdart run tool/testing/verify_release_docs_versions.dart./tool/docs/build_site.sh./tool/docs/validate_links.shgit diff --check32617906980— all 11 checks passed, including Windows, macOS, Linux VM coverage, Chrome, Web Chat, docs, companions, analysis, prompt parity, and aggregate coverageThe changed behavior is workspace discovery/preparation, so a new model-specific smoke is N/A. The full VM and hosted Web Chat/native jobs retain representative real runtime/model coverage without claiming it validates tooling semantics.
Independent QA and review follow-through
Prior independent and Copilot reviews exposed and drove durable fixes for:
Flutter/ephemeral,.symlinks, and CocoaPods pathsFresh independent QA passed on exact head
2033e97c860bd2545b47011aaae1da077d3978bbwith no blocking finding. Exact-head Copilot rereview recommends approval with zero new comments. All four review threads were answered and resolved.Risk and sequencing
The canonical repository-wide gate now requires Flutter and validates the maintained package boundary before running. Runtime behavior is unchanged.
PR #420 overlaps CI/template/AGENTS/test-matrix files. To preserve both contracts cleanly, merge this workspace-bootstrap fix first; then integrate current
maininto #420 and revalidate its regression gate on the combined tree.