feat: add workspace support for packages check licenses

[realmeylisdev]

Summary

This PR adds workspace support to the packages check licenses command, enabling license checking in monorepo projects that use Dart's pub workspace feature.

Reopens #1444 (branch was restored per request).
Closes #1273

Changes

New Files

• lib/src/pubspec/pubspec.dart - Pubspec parser for detecting workspace configurations
• test/src/pubspec/pubspec_test.dart - 19 tests for the Pubspec parser

Modified Files

• lib/src/commands/packages/commands/check/commands/licenses.dart - Added workspace detection and dependency collection
• test/src/commands/packages/commands/check/commands/licenses_test.dart - Added 3 workspace support tests

How It Works

1. Auto-detection: When pubspec.yaml contains a workspace property, it's detected as a workspace root
2. Dependency collection: Dependencies are collected from all workspace members' pubspec.yaml files
3. Glob pattern support: Workspace paths like packages/* are supported (Dart 3.11+)
4. Nested workspaces: Recursively handles nested workspaces
5. Backwards compatible: Non-workspace projects work exactly as before

Test plan

• All existing license tests pass (42 tests)
• New workspace support tests pass (3 tests)
• New Pubspec parser tests pass (19 tests)
• Static analysis passes with no issues

[samitsv]

@realmeylisdev seems like there are some conflicts that needs to be resolved

[realmeylisdev]

now done.

[samitsv]

@marcossevilla could we get this PR reviewed which was de-prioritized last time ? #1444 (comment)

[marcossevilla]

@marcossevilla could we get this PR reviewed which was de-prioritized last time ? #1444 (comment)

hi @samitsv, I'll take a look during this week, thanks for pushing this!

also mentioning @brianegan as he was having this issue too, a review or test to check if it's fixing your issue is also appreciated 👍

[samitsv]

any update on this @marcossevilla

[samitsv]

any update on this @marcossevilla . Would be great to have this merged soon.

[marcossevilla]

hey @samitsv, we're currently trying the branch against internal repos to verify the changes, I'll leave an update once we finish testing

[brianegan]

@marcossevilla Functional testing done on my end! PR worked well for a workspace monorepo. Sorry for the delay here -- deadlines.

[marcossevilla]

@realmeylisdev CI is failing, also found we could just use https://pub.dev/packages/pubspec_parse instead of creating a pubspec library

[realmeylisdev]

CI Failure Analysis

The e2e (docs_site) failure is unrelated to this PR's changes. The e2e (flutter_plugin) cancellation is a cascade from fail-fast: true in the matrix strategy.

Root Cause

The docs_site e2e test generates a Docusaurus project and runs npm run build. The build fails because webpack's ProgressPlugin API dropped support for the name option:

ValidationError: Invalid options object. Progress Plugin has been initialized
using an options object that does not match the API schema.
 - options has an unknown property 'name'.

This is an upstream Docusaurus/webpack incompatibility in the very_good_docs_site template — the fix belongs in very_good_templates, not here.

PR Code Verification

• All 73 unit tests pass (pubspec parser + licenses command including workspace support)
• dart analyze --fatal-infos --fatal-warnings — clean
• dart format — clean
• All other CI checks (build on ubuntu/windows, all other e2e tests, pana, spell-check, link-check) pass

[marcossevilla]

hey @realmeylisdev, we need to use package:pubspec_parse still

[marcossevilla]

hey @realmeylisdev, are you able to work on the changes? otherwise we could close the PR and the team will tackle this issue separately

[realmeylisdev]

@realmeylisdev CI should be fixed, you can update your branch to get the latest changes

done.

[erickzanardo]

@vgvbot perform a code review focused on verifying nits and code readability

[unicoderbot]

I'll review the code and share my feedback shortly.

• Preparing environment
  • Cloning repository
  • Setting up dev environment
• Reviewing code
• Posting review

Code review submitted.

[unicoderbot]

This PR adds workspace support to packages check licenses, which is a useful feature with reasonable test coverage. Overall the approach (detect a workspace root, walk member pubspecs, build a Set of declared direct deps to filter pubspec.lock by name) is workable and backwards-compatible. A few concerns worth addressing:

Concerns

• _collectWorkspaceDependencies recurses into nested workspaces with no cycle protection. A pubspec that lists its own directory (or a parent) in workspace: would cause unbounded recursion.
• Pubspec.parse(..., lenient: true) accepts many malformed pubspecs without throwing. The malformed-pubspec test only passes because the YAML itself is unparsable; a structurally invalid but YAML-valid pubspec would silently parse with empty/default dep maps and contribute nothing — i.e. workspace members would be silently dropped.
• PubspecWorkspace.isWorkspaceMember is dead production code (only referenced by tests). Either drop it or use it to validate that resolved members declare resolution: workspace.
• The new workspace-aware filter and the helper that drives it (_collectWorkspaceDependencies) live in licenses.dart even though the logic is generic workspace-walking; moving it into lib/src/pubspec/pubspec.dart (or a sibling) would keep the command file focused and let other commands reuse it.
• The branch around dependencyTypes.contains('direct-main') || dependencyTypes.contains('direct-dev') is redundant once workspaceDependencies was already filtered by those types — the outer if can be removed for clarity, or the membership Sets should be tracked per-type to actually enforce the distinction.
• Test gaps: no coverage for (a) a dep declared as direct-main in one member and direct-dev in another (relevant since the merged Set loses that distinction), (b) _isGlobPattern's ?/[ branches, (c) cycle protection, and (d) a workspace member that also declares resolution: workspace mismatch.
• _isGlobPattern is a hand-rolled heuristic; pkg:glob can be queried more directly, and patterns like !packages/foo (negation) won't be classified as glob.

Positives

• Clean separation between pubspec.dart helpers and the command layer.
• Backwards-compatibility is preserved: when no pubspec.yaml is present or it isn't a workspace root, the original filtering path is used unchanged.
• Good integration test coverage for the common workspace shapes (direct-main/dev, transitive, overridden, nested, and non-workspace).

[marcossevilla]

hey @realmeylisdev, will you be able to apply the comments on this PR? thanks!

[realmeylisdev]

@marcossevilla applied the remaining bot review nits in d5668ab:

• Dropped lenient: true from tryParsePubspec so YAML-valid but structurally invalid pubspecs no longer silently parse with empty dep maps.
• Removed the unused PubspecWorkspace.isWorkspaceMember getter (dead in lib/, only tests referenced it).
• Replaced the hand-rolled _isGlobPattern heuristic with a single Glob.listSync path that handles literal and metachar patterns uniformly (so e.g. !packages/foo works without special-casing).
• Filled the remaining test gaps: structurally invalid pubspec, ?/[…]/{…} glob shapes, literal paths with non-metachar symbols, and a member that omits resolution: workspace.

Ready for another look.

[marcossevilla]

@vgvbot check the changes from your last review, resolve the comments that have been addressed and leave any comments if necessary

[unicoderbot]

I'll review the code and share my feedback shortly.

• Preparing environment
  • Cloning repository
  • Setting up dev environment
• Reviewing code
• Posting review

Code review submitted.

[unicoderbot]

Overall

This PR adds workspace support to packages check licenses, which fills a real gap for monorepo users. The approach — walk member pubspecs to build a union of direct deps, then intersect with the workspace's pubspec.lock — is reasonable. Test coverage is broad, covering globs (*, ?, [...], {...}), nested workspaces, malformed members, and backwards compatibility.

Main concerns

1. Duplicated filter logic in licenses.dart. The new workspace branch (lines 212–227) re-implements the transitive/direct-overridden checks that already exist in the non-workspace branch (lines 234–238). The two paths now have to stay in sync.
2. Dual signal/data return in collectWorkspaceDependencies. null means "not a workspace," a non-null Set means "workspace deps." Since isWorkspaceRoot already exists as a separate getter, this overloading is unnecessary and makes the call site read awkwardly (workspaceDependencies != null is really asking "are we in workspace mode?").
3. Redundant traversal on nested workspaces. When a member is itself a workspace root, its dependencies/devDependencies are added by the outer loop and by the recursive call's prologue. Set dedupes, but the data flow is harder to follow than it should be.
4. dependency_overrides is silently ignored when collecting workspace deps. Members that override deps will rely on the lock file's direct-overridden marker — which is fine in practice but worth either documenting in the dartdoc or handling explicitly.
5. Unnecessary churn at line 183: changing targetPath to targetDirectory.path doesn't change behavior (they're the same string) and bloats the diff.

Smaller notes

• Several test fixtures collide on the pubspec name field (e.g. _workspaceMemberSharedPubspecContent is used for appDir, where it produces name: shared), which makes the test scenarios harder to parse.
• tryParsePubspec catches Exception only; Pubspec.parse paths go through ParsedYamlException/YamlException which subclass FormatException, so this is correct — but the doc comment could state that explicitly.
• Cross-platform: Glob(pattern).listSync(root: root.path) will use the platform path.Context by default. Workspace globs in real pubspecs always use /, which works, but worth a quick sanity check on Windows CI before shipping.

[marcossevilla]

hey @realmeylisdev! let us know if you will be able to apply the changes, otherwise we can prioritize this change for the next CLI release (not #1605, but the following one)

[realmeylisdev]

Thanks @marcossevilla — I've applied the changes (pushed in d2b106d). Summary of this round:

lib/src/pubspec/pubspec.dart

• Moved the parser into the extension (PubspecWorkspace.tryParse) so the pubspec helpers live together.
• collectWorkspaceDependencies now returns a non-null Set (empty when not a workspace); workspace detection reads isWorkspaceRoot, removing the previous null/Set dual signal.
• Workspace-root members are collected via the recursive call only (no more double-add), extracted into a _directDependencies helper.
• Documented the intentional dependency_overrides skip, the strict/silent parse behavior, and the POSIX glob assumption.

lib/src/commands/packages/commands/check/commands/licenses.dart

• Unified the dependency filter so the transitive/direct-overridden handling lives in one place; workspace-vs-direct is decided via isWorkspaceRoot.
• Reverted the targetDirectory.path → targetPath diff noise.

Tests

• The circular-workspace test now asserts on the collected set instead of only returnsNormally.
• Renamed the mixed-deps fixture and added an app-named fixture so member names match their directories.
• Confirmed coverage for the direct-main-in-one-member / direct-dev-in-another and YAML-valid-but-pubspec-invalid cases.

100% coverage is maintained, and dart analyze --fatal-infos --fatal-warnings and dart format are clean. Happy to adjust anything further.

[realmeylisdev]

Heads up on CI: the failing e2e (test/commands/create/flutter_plugin/flutter_plugin_test.dart) check is inherited from main, not introduced by this PR.

• The same job is red on the feat: add --platforms flag support to flutter_app template #1620 merge commit on main (run 27950506626); every prior main commit passed it.
• The failure is in the generated flutter_plugin example (check_platform_name), where the test references isMacOS/isAndroid/isWeb/isWindows named parameters that aren't defined — template code this PR doesn't touch (it only changes lib/src/pubspec/, the licenses command, and their tests).
• The checks that exercise this PR's code all pass: build on ubuntu and windows, pana, check_links, spell-check, and the licenses e2e tests.

Re-merging main once that template issue is fixed should turn the check green.

[realmeylisdev]

Thanks @marcossevilla and @unicoderbot for the thorough review. The feedback has been addressed in the latest revision — a summary of each point and how it was resolved:

Architecture / API

• Move workspace logic into the pubspec library — all workspace traversal now lives in the PubspecWorkspace extension in lib/src/pubspec/pubspec.dart (tryParse, isWorkspaceRoot, resolveMembers, collectWorkspaceDependencies). The command file only calls into it.
• Coupled signals via a nullable return — collectWorkspaceDependencies now returns a Set<String> unconditionally (empty when not a workspace). The command reads workspace mode directly from pubspec?.isWorkspaceRoot ?? false, so there's no != null overloading at the call site.
• Redundant direct-main/direct-dev outer if — the filter is now a single expression; in workspace mode membership is decided by workspaceDependencies.contains(...), with transitive/direct-overridden handled in one place shared with the non-workspace path.

Correctness / robustness

• No cycle protection — recursion now threads a visited set of canonicalized paths and short-circuits on revisit, so a member that points back at the root (or itself) terminates. Covered by a dedicated test that also asserts the cycling member's dep is still collected.
• lenient: true masking malformed pubspecs — dropped; parsing is now strict, so a structurally invalid pubspec is treated as unparseable rather than silently yielding empty dep maps. The dartdoc on tryParse states explicitly that parse failures are swallowed and malformed members are skipped.
• Hand-rolled glob heuristic (_isGlobPattern) — removed. Patterns go straight through Glob(...).listSync(...), which handles literal vs. pattern paths uniformly (including negation), so the fragile !/{/[ classification is gone.
• Dead isWorkspaceMember — removed (YAGNI).
• Redundant add on nested workspace roots — when a member is itself a workspace root, its direct deps are no longer added twice; the direct add is skipped and the recursive call handles them, with a comment explaining the flow.
• dependency_overrides handling — documented as intentional on collectWorkspaceDependencies: overrides are surfaced by the caller through the lock file's direct-overridden marker rather than from member pubspecs.
• Workspace vs. lock-file "direct" divergence — called out in a comment at the filter site so the behavior is discoverable.

Tests

• Cross-member direct-main / direct-dev — added a test pinning the behavior: a dep declared direct-main in one member and direct-dev in another is still surfaced when only --dependency-type direct-main is requested.
• Cycle test was vacuous (returnsNormally) — now also asserts the returned set contains the cycling member's dependency.
• Fixture naming — _workspaceMemberWithDevDeps… renamed to _workspaceMemberWithMixedDeps…; fixtures whose name: didn't match their directory were fixed so member scenarios read naturally.
• YAML-valid-but-pubspec-invalid case — covered by the strict-parse change plus the malformed-member test.

One intentional divergence — Windows globs

The suggestion to pin Glob(pattern, context: p.posix) was not applied, and I'd like to keep it that way: listSync(root:) walks the real filesystem, whose entity paths use the platform separator. Pinning POSIX would make pattern/path matching disagree with the filesystem on Windows. The platform-default context already accepts the forward-slash patterns that pub workspaces always use, on every platform — so the default is the more portable choice here. I've left a comment at that site explaining the reasoning.

CI is green and the full suite passes locally (analyzer clean with --fatal-infos --fatal-warnings; workspace-support and pubspec tests all pass). Re-requesting a review when you have a chance 🙏

[unicoderbot]

All my review comments have been resolved. Approving this PR. If another review is needed, just re-request it.

[realmeylisdev]

Rebased onto the latest main — the branch is now up to date with no conflicts, and CI is green across all checks. All review threads from the earlier round are resolved, and @unicoderbot approved the latest revision.

@marcossevilla, when you have a moment, could you take another look? Happy to address anything further. Thanks!