FEDX-7265: Dependencies used inside hook/ must be regular dependencies, not dev_dependencies

[engops-wk]

Opened by Dustin Pauze with the ai-sdlc workflow for FEDX-7265

Pull Request

Description

This PR extends dependency_validator to treat hook/ as a public-facing directory alongside lib/ and bin/. Dependencies used in hook scripts must now be declared as regular dependencies, not just dev-only dependencies, to ensure they're available at build/link time for consuming packages in production mode.

Motivation

Dependencies imported inside a package's hook/ directory run at build/link time (dart build, flutter build) via the package manager. If such a dependency is only declared as a dev_dependency, it won't be resolved when a consuming package installs in production mode, causing hook execution to fail at build time. dependency_validator previously only treated lib/ and bin/ as public-facing directories, so it did not flag dependencies used exclusively in hook/ as needing promotion to a regular dependency.

Changes

• Treats hook/ as a public-facing directory alongside lib/ and bin/ for the purposes of dependency validation (lib/src/constants.dart, lib/src/dependency_validator.dart)
• Adds publicDirNames constant and a publicDirsDescription() helper to build human-readable messages (e.g. lib/, bin/, or hook/) used in warning output for missing, under-promoted, over-promoted, and unused-executable-dependency checks
• Updates all warning log messages in dependency_validator.dart to reference the dynamic public directories list instead of hardcoded lib/ text
• Bumps package version to 6.0.0 and records this as a breaking change in CHANGELOG.md, since existing codebases with dev-only dependencies used in hook/ will now fail validation
• Updates README.md to document that hook/ is now considered public-facing and that hook-only dependencies must be regular dependencies (or explicitly ignored/excluded)
• Adds/updates tests in test/executable_test.dart and test/utils_test.dart covering: hook scripts using dev_dependencies (fails, unless ignored via config or embedded pubspec config), hook scripts using regular dependencies (passes), hook-only dependencies not being flagged as over-promoted, hook scripts using undeclared dependencies (fails), and the new publicDirsDescription() helper's output for both default and and conjunctions

Testing

• Run the automated test suite with dart test (or pub run test) and confirm all tests pass
• Create a sample package with hook/build.dart using a dev_dependency and verify validation fails appropriately
• Move the dependency to regular dependencies and verify validation passes
• Test the ignore/exclude configuration to ensure hook dependencies can be explicitly excluded
• Verify documentation updates are accurate and complete

Checklist

• Tests pass locally
• New/updated test cases cover hook/ scenarios
• Documentation (README.md) updated
• CHANGELOG.md updated with breaking change note
• Version bumped to 6.0.0
• Breaking change properly communicated

Intent

This change extends dependency validation to treat the hook/ directory as a public-facing directory alongside lib/ and bin/, ensuring that dependencies used in build hooks are properly declared as regular dependencies rather than dev-only. It addresses a critical issue where hook dependencies declared only in dev_dependencies would fail at build time when consumed by other packages in production mode.

How To QA

1. 1. Run 'dart test' or 'pub run test' and verify all tests pass, including new/updated cases in test/executable_test.dart and test/utils_test.dart
2. 2. Create a sample package with hook/build.dart importing a dev_dependency package. Run dependency_validator and verify it exits with code 1 and reports the dependency needs promotion, with message listing 'lib/, bin/, or hook/'
3. 3. Move the hook dependency to regular dependencies in pubspec.yaml and re-run dependency_validator. Verify it exits with code 0 and shows 'No dependency issues found!'
4. 4. Add the hook dependency to the ignore list in dart_dependency_validator.yaml while keeping it as dev_dependency, re-run, and verify validator passes with exit code 0
5. 5. Review README.md and CHANGELOG.md changes to confirm they accurately document the new behavior and breaking change for version 6.0.0

[engops-wk]

Review round 1 — CHANGES REQUESTED (posted as a comment; GitHub blocks REQUEST_CHANGES on one's own PR)

Do not merge: CI is red. The checks job (Workiva/gha-dart-oss/.github/workflows/checks.yaml@v0.1.14, which runs dart format --output=none --set-exit-if-changed . and dart analyze) fails on an unformatted line introduced at test/executable_test.dart:52. build and unit-tests must also be confirmed green. Please push fixes and re-request review once all three jobs pass.

Assessment of the change itself

The direction is correct and the implementation is appropriately minimal. Adding $root/hook/ to publicDirs in lib/src/dependency_validator.dart flows automatically through publicDartFiles/publicScssFiles/publicLessFiles, through publicDirGlobs (so hook files are properly excluded from the non-public set rather than double-counted), and therefore through all five checks: missing, missing-dev, over-promoted, under-promoted, and unused. No special-casing was needed, which is the right call.

Test coverage is well-chosen: dev_dependency in hook/ fails; regular dependency in hook/ passes (which also implicitly proves a hook-only dep is no longer flagged over-promoted — a real regression risk given the over-promoted logic keys off packagesUsedInPublicFiles); undeclared fails; and ignore still suppresses. That is the right matrix.

Blocking issues (inline comments posted)

1. Formatting — test/executable_test.dart:52 exceeds the line limit and is not dart format clean, failing checks.
2. Inconsistent user-facing messages — lib/src/dependency_validator.dart now emits lib/, bin/, or hook/ for missing and under-promoted, but still emits outside lib/ for missing-dev and over-promoted. The tool's own output contradicts itself.
3. CHANGELOG severity and accuracy — This will newly fail previously-passing consumer builds. The analogous bin/ expansion was documented as a Breaking Change in 4.0.0 with a stated resolution; match that precedent. Also, hooks run at build/link time (dart build, flutter build), not at pub get/install time, so the stated rationale is technically wrong.
4. README over-promoted bullet — Still says "only used outside lib/", which is now inaccurate.

Non-blocking notes

• Fixture is hook/post_install.dart; Dart only recognizes hook/build.dart and hook/link.dart. Renaming (or adding a hook/build.dart case) would make the tests self-documenting.
• Because the scan is directory-wide rather than entrypoint-based, repos using hook/ for unrelated scripts will now need exclude: ["hook/**"]. Acceptable tradeoff; item 3 is what makes it discoverable.
• The new groups use test('', ...), consistent with the existing file, so not a change request — but naming them would improve failure output.

[engops-wk]

Review round 2 — CHANGES REQUESTED (posted as a comment; GitHub blocks REQUEST_CHANGES on my own PR)

Not approved. CI is red on this commit.

The core change is right and small: adding '$root/hook/' to publicDirs in lib/src/dependency_validator.dart correctly reclassifies hook imports as public usage, and because publicDirs also drives publicDirGlobs, the non-public file walk and the over-promoted/unused sets stay internally consistent. Round-1 feedback on the README over-promoted bullet, the missing-dev-dependency / over-promoted message wording, and the CHANGELOG breaking-change framing was all addressed. Thank you.

Blocking: CI failed on this commit (147706b)

• checks / format — dart format --output=none --set-exit-if-changed . (SDK 3.7.2, from .github/workflows/ci.yaml → Workiva/gha-dart-oss/.github/workflows/checks.yaml@v0.1.14). Same failure flagged in round 1; the formatter still wants the long contains(...) string arguments in test/executable_test.dart reflowed.
• unit-tests / unit — dart test. test/executable_test.dart still asserts pre-change message text (notably "contain executables, and are only used outside of lib/"), and lib/src/dependency_validator.dart still emits at least one non-updated message, so expectations and output are out of sync.

The checks / dependency-validator job is not a signal here: it runs dart pub global activate dependency_validator ^4.1.0 — the published 4.x tool, not this PR's code — so it never exercises the new hook/ behavior. Don't treat green there as validation.

Push a commit that runs dart format . and gets dart test green on Dart 3.7.2, then re-request review.

Blocking correctness / consistency

1. One message was missed. The executables warning ("The following packages contain executables, and are only used outside of lib/. These should be downgraded to dev_dependencies") still says lib/ only — now inaccurate, and its test assertion fails.
2. The public-dir description is duplicated ~12 times across source and tests. Round 1 caught exactly this drift class; hoist a single derived constant so the next directory addition is a one-line change.

Blocking coverage gap

3. No test asserts the most user-visible consequence: a dependency used only in hook/ must no longer be reported as over-promoted. That's the regression that would silently reverse this behavior.
4. The new hook/ group omits the (deprecated pubspec method) variant its sibling groups have, and three new group(...) wrappers contain a single unnamed test(''), producing empty leaf test names.

Blocking release hygiene

5. pubspec.yaml is still version: 5.0.6 and the CHANGELOG entry has no target version. The precedent for this exact change category (bin/ in 4.0.0) was a major bump; shipping behavior-breaking validation as a patch fails consumers on a routine pub upgrade. Confirm and record the intended major version.
6. README documents the changed bullets but never explains what hook/ is or why it's public-facing. Add one line with the rationale and the ignore/exclude escape hatch.

Not blocking

• Consider whether hook/ should be config-gated for one minor release before default-on. I'm fine with default-on given the correctness risk it prevents — just make the migration path explicit in release notes.
• CHANGELOG's "build/link time (dart build, flutter build)" is the right fix; consider linking the Dart hooks docs.

[engops-wk]

Round 3 — CHANGES REQUESTED (posted as a comment; GitHub blocks REQUEST_CHANGES on an authored PR). Do not merge.

Round 2 feedback was addressed well: the publicDirNames/publicDirsDescription() single source of truth is in place, all five warning messages now derive from it, the README/CHANGELOG document the breaking behavior with a resolution path, the version is bumped to 6.0.0, and the new coverage includes the deprecated-pubspec ignore variant plus the "hook-only dep is not over-promoted" case. The core logic change (publicDirs derived from publicDirNames) is correct and minimal.

Blocking

1. CI is failing on 9414a46. Per .github/workflows/ci.yaml, the checks job (dart format / dart analyze) and the unit-tests job are red. I will not approve on a red build — the long contains(...) interpolations added in lib/src/dependency_validator.dart and test/executable_test.dart exceed the formatter's line width, the same class of failure flagged in round 2. Run dart format ., dart analyze, and dart test against SDK 3.7.2 (per .tool-versions) and push a green commit. Note that checks / dependency-validator runs the published 5.x validator, so it will not exercise hook/ — it is not evidence the feature works.
2. Test assertions are now tautological. Every message assertion in test/executable_test.dart interpolates publicDirsDescription(), so a regression in that helper would silently change both the tool output and the expectation. Pin the literal user-facing text once in test/utils_test.dart.
3. Hook fixtures use a filename that isn't part of the hook contract. hook/post_install.dart should be hook/build.dart (Dart's hook entrypoints are build.dart/link.dart); as written the tests only prove directory-name matching, not the real scenario, and the name perpetuates the "install time" framing we corrected in the CHANGELOG.

Non-blocking nits

• passes when hook scripts use regular dependencies and passes when hook-only dependency is not flagged as over-promoted build the identical project and both assert exit code 0; consider collapsing them into one test with both assertions.
• CHANGELOG.md dropped the # Unreleased / <!-- Add unreleased changes here --> placeholder. Consider keeping it above # 6.0.0.
• publicDirsDescription() is prose formatting living in constants.dart; lib/src/utils.dart would be a more natural home alongside bulletItems.

[dustinlessard-wf]

Don't adjust the version.

[dustinlessard-wf]

@cursor resolve merge conflicts

[cursor]

Merge conflicts on cursor/hook-dev-dependencies-0eac are resolved and pushed in commit 78b977b.

Conflicts resolved

CHANGELOG.md — Combined both sides under # Unreleased (per the “don’t adjust the version” guidance):

• Hook/ breaking change entry from the PR branch
• Nested packages (Nested packages should be ignored in the surrounding package #173) and language-version parsing entries from master

lib/src/dependency_validator.dart — Kept both behaviors:

• Master’s nested-package exclusion logic (listNestedPackages, nestedPackageGlobs)
• PR branch’s hook/ support via publicDirNames for publicDirs

Also merged in from master: test/nested_packages_test.dart, updates to utils.dart, import_export_ast_visitor.dart, and test/utils_test.dart.

dart analyze passes with no issues. PR #194 should now be mergeable from a conflict standpoint.

  

[robbecker-wf]

Let's get other non-major things merged and released before shipping this as a major