Skip to content

[camera_android_camerax] Migrate check-readiness skill from bash to Dart - #11943

Merged
auto-submit[bot] merged 52 commits into
mainfrom
convert-check-sh-to-dart
Jun 30, 2026
Merged

[camera_android_camerax] Migrate check-readiness skill from bash to Dart#11943
auto-submit[bot] merged 52 commits into
mainfrom
convert-check-sh-to-dart

Conversation

@reidbaker

@reidbakerreidbaker commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

1. Migrated check-readiness Skill to Dart

  • Replaced scripts/check.sh with a Dart implementation.
  • Implemented unit tests for the checker logic under test/check_test.dart

2. Tooling & CI Support for Agent Skills

  • Static Analysis (Opt-in): Added a new --analyze-skills-for flag to the analyze command. When set, it explicitly runs dart analyze on the .agents/skills directories of the configured packages.
  • CI Configuration: Configured the global analyze workflow in .ci/targets/analyze.yaml to run analysis on camera's skills via script/configs/skills_analysis.yaml.
  • Unit Testing: Because .agents/skills/check-readiness is set up as a standalone Dart package with its own pubspec.yaml, the repository's existing dart-test command automatically discovers and runs its unit tests.
  • Validation Exemption: Added .pubignore parsing support to RepositoryPackage and integrated it into the validate command. Any subpackage matching .pubignore patterns (e.g. .agents/skills/) is now skipped by the validate command, ensuring internal/contributor-only tools do not fail publishing hygiene checks.
  • Versioning & CHANGELOG Exemption: Classified paths starting with .agents/ as developer-only changes. This exempts skill-only or script-only modifications within the .agents/ directory from triggering version-bump and CHANGELOG update requirements.

I asked my agent to articulate the interaction between top level packages tool commands and .agents/skills. Then I filtered the list to the command I thought reviewers or people looking at this pr later would find interesting.

Interaction of Packages Tool Commands with .agents/skills

CommandTargeting ScopeRuns on .agents/skills?Interaction Details
analyzetopLevelOnlyYes (Explicitly)Runs pub get on the main package and all subpackages (including those in .agents/skills). If the main package is listed in --analyze-skills-for (configured in CI via script/configs/skills_analysis.yaml), it explicitly runs dart analyze --fatal-infos .agents/skills.
validateincludeAllSubpackagesNo (Skipped)Since .pubignore specifies .agents/, the skill packages are marked as ignored (isPubIgnored = true). The validate command skips them entirely (SKIPPING: Ignored by .pubignore), exempting developer-only helper tools from public-facing package hygiene rules.
dart-testincludeAllSubpackagesYesBecause it targets all subpackages, it scans .agents/skills/*. If a skill package contains a test/ directory, it executes the Dart/Flutter unit tests inside it.
formattopLevelOnlyYesUses recursive file gathering (getFilesForPackage) under the main package directory, formatting all Dart, C++, Java, Kotlin, and Swift files under .agents/skills/. It does not filter files using .pubignore.
license-checkN/A (Repository-wide)YesGathers all tracked files in Git via git ls-files. Any file checked into Git under .agents/skills/ is validated for standard first-party or recognized third-party license headers.
fixincludeAllSubpackagesYesRuns dart fix --apply inside any skill package directory that contains a pubspec.yaml.
update-dependencyincludeAllSubpackagesYesIf a targeted dependency is used by the skill package, its pubspec.yaml will be updated.
update-min-sdkincludeAllSubpackagesYesUpdates SDK constraints inside the skill package's pubspec.yaml when run.
remove-dev-dependenciesincludeAllSubpackagesYesRemoves development dependencies from the skill package's pubspec.yaml when targeted.
publish-checktopLevelOnlyNoRuns on the main package. Because .agents/ is listed in .pubignore, the directory is excluded from the package archive and not uploaded or checked.
publishtopLevelOnlyNoSimilar to publish-check, the .agents/ directory is skipped during publish due to .pubignore.
fetch-depstopLevelOnlyNoN/A
listN/ANoLists only top-level packages.

Pre-Review Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the gemini-code-assist bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.

Footnotes

  1. Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. 2

@flutter-dashboardflutter-dashboardBot added the CICD Run CI/CD label Jun 18, 2026
@reidbaker
reidbaker marked this pull request as draft June 18, 2026 23:41

@gemini-code-assistgemini-code-assistBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new local agent skill check-readiness to verify the environment, adds a custom lint rule to enforce that tracked skills are marked as internal, and implements proxy tests to run unit tests and validate skills. It also integrates several third-party skills via symlinks. The review feedback highlights several critical improvements: ensuring dependencies are resolved with pub get before running tests, avoiding crashes on broken symlinks by setting followLinks: false, adding safe type checks before casting YAML to a map, using Directory.current.path to robustly determine the workspace root, handling ProcessException when executing git commands, and updating the skill documentation to reference the Dart implementation instead of the duplicate bash script.

Comment threadpackages/camera/camera_android_camerax/test/skills_unit_tests_test.dart Outdated
Comment threadpackages/camera/camera_android_camerax/test/skills_unit_tests_test.dart Outdated
@reidbaker
reidbakerforce-pushed the convert-check-sh-to-dart branch from e1801ee to d98795bCompareJune 19, 2026 00:03
@github-actionsgithub-actionsBot removed the CICD Run CI/CD label Jun 19, 2026
@flutter-dashboardflutter-dashboardBot added the CICD Run CI/CD label Jun 19, 2026
@reidbaker
reidbaker requested a review from camsim99June 19, 2026 17:46
@github-actionsgithub-actionsBot removed the CICD Run CI/CD label Jun 19, 2026
@reidbakerreidbaker added the CICD Run CI/CD label Jun 19, 2026
- Relocate check.dart from bin/ to tool/ and update workspace root resolution.
- Swap pubspec.yaml dependencies and dev_dependencies ordering in check-readiness.
- Support analyze_skills package-level configuration in ci_config.yaml.
- Remove --analyze-skills-for command line flag and global skills_analysis.yaml config.
- Fix doc comment in repository_package.dart.
- Refactor AnalyzeCommand to avoid early returns and reuse package-level config.
- Fix lint issue in analyze_command.dart.

@reidbakerreidbaker left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review feedback.

Comment threadscript/tool/lib/src/analyze_command.dart Outdated
Comment threadscript/tool/lib/src/analyze_command.dart Outdated
if (!libOnly && _packagesWithSkills.contains(package.directory.basename)) {
final List<String> skillsErrors = _validateAgentsSkillsDirectory(package);
if (skillsErrors.isNotEmpty) {
return PackageResult.fail(skillsErrors);

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed the callsite, I also modified the readiness checker script to follow the same logic. We now check each tool even if the previous ones fail (and added a test to enforce that behavior)

@stuartmorgan-gstuartmorgan-g left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with some comment nits

if (skillsExitCode != 0) {
errors.add('Skills analysis failed');
}
errors.addAll(skillsErrors);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional: This could just be:

final errors =<Stirng>[
if (mainExitCode !=0) 'Main package analysis failed',
if (skillsExitCode !=0) 'Skills analysis failed',
...skillsErrors,
];

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you this is easier to read

@github-actionsgithub-actionsBot removed the CICD Run CI/CD label Jun 30, 2026
@reidbakerreidbaker added CICD Run CI/CD autosubmit Merge PR when tree becomes green via auto submit App labels Jun 30, 2026
@auto-submitauto-submitBot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jun 30, 2026
@auto-submit

Copy link
Copy Markdown
Contributor

autosubmit label was removed for flutter/packages/11943, because - The status or check suite Linux_web web_dart_unit_test_wasm_shard_1 master has failed. Please fix the issues identified (or deflake) before re-applying this label.

@github-actionsgithub-actionsBot removed the CICD Run CI/CD label Jun 30, 2026
@reidbakerreidbaker added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 30, 2026
@auto-submit

Copy link
Copy Markdown
Contributor

autosubmit label was removed for flutter/packages/11943, because - The status or check suite Mac_arm64 ios_platform_tests_shard_1 master has failed. Please fix the issues identified (or deflake) before re-applying this label.

@auto-submitauto-submitBot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jun 30, 2026
@reidbakerreidbaker added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 30, 2026
@auto-submit
auto-submitBot merged commit bdb1682 into mainJun 30, 2026
88 checks passed
@auto-submit
auto-submitBot deleted the convert-check-sh-to-dart branch June 30, 2026 22:14
auto-submitBot pushed a commit that referenced this pull request Jul 1, 2026
Adds a style guideline to `AGENTS.md` advising agents and developers against adding redundant or trivial comments that simply restate code logic in English.
This has been a pattern in my own observations and also was caught by stuart in #11943 after I missed it in my pre review. We currently have no evals so there is not a good way to test this change. ## Pre-Review Checklist
This PR is exempt from version/CHANGELOG updates and tests because it only modifies repository-wide guide documentation.
[^1]: Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling.
pullBot pushed a commit to safarmer/flutter that referenced this pull request Jul 1, 2026
…er#188863)
flutter/packages@274ed3e...e742106
2026-07-01 rmolivares@renzo-olivares.dev [cupertino_ui] Re-enable
`tab_scaffold_test.dart` (flutter/packages#12064)
2026-07-01 jmccandless@google.com [material_ui] Port flutter/flutter
flutter#184808 "Remove semantics_tester import from card_test.dart"
(flutter/packages#11965)
2026-07-01 rmolivares@renzo-olivares.dev [cupertino_ui] Migrate
`sliding_segmented_control_test.dart` to `SemanticsHandle`
(flutter/packages#11979)
2026-07-01 rmolivares@renzo-olivares.dev [cupertino_ui] Migrate
`route_test.dart` to `SemanticsHandle` (flutter/packages#11993)
2026-07-01 rmolivares@renzo-olivares.dev [cupertino_ui] Migrate
`nav_bar_test.dart` to `SemanticsHandle` (flutter/packages#11980)
2026-07-01 rmolivares@renzo-olivares.dev [cupertino_ui] Migrate
`segmented_control_test.dart` to `SemanticsHandle`
(flutter/packages#11982)
2026-07-01 rmolivares@renzo-olivares.dev [cupertino_ui] Re-enable
`text_field_test.dart` (flutter/packages#12067)
2026-06-30 r.anantheswar@gmail.com [camera_android_camerax] Pass
targetVideoEncodingBitRate to Recorder (flutter/packages#11960)
2026-06-30 1063596+reidbaker@users.noreply.github.com
[camera_android_camerax] Migrate check-readiness skill from bash to Dart
(flutter/packages#11943)
2026-06-30 21270878+elliette@users.noreply.github.com [material_ui]
Enable `time_picker_test` (flutter/packages#12061)
2026-06-30 36861262+QuncCccccc@users.noreply.github.com [cupertino_ui]
Migrate checkbox_test.dart to SemanticsHandle (flutter/packages#12065)
2026-06-30 64674824+yashas-hm@users.noreply.github.com [image_picker]
Handle limit: 1 in pickMultiImage and pickMultipleMedia gracefully
(flutter/packages#11825)
2026-06-30 faheemabbas766@gmail.com [cross_file] Document native
mimeType behavior (flutter/packages#11662)
2026-06-30 36861262+QuncCccccc@users.noreply.github.com [material_ui]
Remove `image_data.dart` imports from `circle_avatar_test.dart`,
`color_scheme_test.dart` (flutter/packages#12059)
2026-06-30 36861262+QuncCccccc@users.noreply.github.com [cupertino_ui]
Remove `image_data.dart` import from `scaffold_test.dart`
(flutter/packages#12060)
2026-06-30 36861262+QuncCccccc@users.noreply.github.com [cupertino_ui]
Remove widgets import from menu_anchor_test.dart
(flutter/packages#12068)
2026-06-30 21270878+elliette@users.noreply.github.com [material_ui]
Enable `checkbox_list_tile_test` (flutter/packages#12007)
2026-06-30 louisehsu@google.com [in_app_purchase_storekit] Expose
quantity in Transactions (flutter/packages#11879)
If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC flutter-ecosystem@google.com on the revert to ensure that a
human
is aware of the problem.
To file a bug in Flutter:
https://github.com/flutter/flutter/issues/new/choose
To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622
Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmitMerge PR when tree becomes green via auto submit Appp: camera

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@reidbaker@stuartmorgan-g