From e885e9605ee15a8edd465a3c941c92c57cb18b02 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Baudet?= Date: Wed, 14 Oct 2026 13:08:30 -0600 Subject: [PATCH] feat(validator): add severity levels and suppressions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every finding now carries a stable rule ID and a severity from the new ruleSeverities map. Errors exit 2 and block the hook; warnings are printed but exit 0. Output marks errors with ✗ and warnings with ⚠ and ends with an error, warning, and suppressed count. A `# ffca-ignore: — ` comment in a pubspec suppresses that rule for the package. A suppression without a reason or with an unknown rule ID is an error and suppresses nothing; one that matches no finding is a warning in --all mode. Add the presentation-missing-own-domain warning held back from #24. The hook returns warnings to Claude as non-blocking additionalContext, covered by the new hooks/validate_layers_test.sh in the Hook Tests job. Closes #21 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017Vxxv9fVFGupLXEnmx5bSq --- .github/workflows/ci.yaml | 2 + AGENTS.md | 4 +- CLAUDE.md | 6 +- README.md | 36 ++- agents/ffca-layer-auditor.md | 2 +- hooks/validate_layers.sh | 8 + hooks/validate_layers_test.sh | 124 +++++++++ .../beta/beta_presentation/pubspec.yaml | 2 + .../omicron/omicron_data/pubspec.yaml | 6 + .../omicron/omicron_domain/pubspec.yaml | 4 + .../features/pi/pi_domain/pubspec.yaml | 6 + .../fixtures/invalid_workspace/pubspec.yaml | 3 + .../features/nu/nu_domain/pubspec.yaml | 4 + .../features/nu/nu_presentation/pubspec.yaml | 5 + .../features/rho/rho_domain/pubspec.yaml | 6 + .../features/xi/xi_data/pubspec.yaml | 6 + .../features/xi/xi_domain/pubspec.yaml | 4 + .../fixtures/warning_workspace/pubspec.yaml | 10 + scripts/test/validate_layers_test.dart | 82 +++++- scripts/validate_layers.dart | 252 ++++++++++++++++-- 20 files changed, 548 insertions(+), 24 deletions(-) create mode 100644 hooks/validate_layers_test.sh create mode 100644 scripts/test/fixtures/invalid_workspace/features/omicron/omicron_data/pubspec.yaml create mode 100644 scripts/test/fixtures/invalid_workspace/features/omicron/omicron_domain/pubspec.yaml create mode 100644 scripts/test/fixtures/invalid_workspace/features/pi/pi_domain/pubspec.yaml create mode 100644 scripts/test/fixtures/warning_workspace/features/nu/nu_domain/pubspec.yaml create mode 100644 scripts/test/fixtures/warning_workspace/features/nu/nu_presentation/pubspec.yaml create mode 100644 scripts/test/fixtures/warning_workspace/features/rho/rho_domain/pubspec.yaml create mode 100644 scripts/test/fixtures/warning_workspace/features/xi/xi_data/pubspec.yaml create mode 100644 scripts/test/fixtures/warning_workspace/features/xi/xi_domain/pubspec.yaml create mode 100644 scripts/test/fixtures/warning_workspace/pubspec.yaml diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index b46d1ce..f08e093 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -63,6 +63,8 @@ jobs: - uses: actions/checkout@v7 - name: Check VGV CLI hook tests run: bash hooks/check_vgv_cli_test.sh + - name: Validate layers hook tests + run: bash hooks/validate_layers_test.sh skills-lint: name: 🔍 Skills Lint diff --git a/AGENTS.md b/AGENTS.md index 06ba364..0cc422a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -24,6 +24,7 @@ hooks/ check_vgv_cli.sh # Checks the Very Good CLI version and auto-approves its MCP tools check_vgv_cli_test.sh # Tests for check_vgv_cli.sh validate_layers.sh # Runs the validator on edited pubspec.yaml files + validate_layers_test.sh # Tests for validate_layers.sh references/ ffca/ # Byte mirror of the VGV Engineering FFCA pages, single source of truth README.md # Manifest: which upstream page each file mirrors @@ -47,6 +48,7 @@ scripts/ fixtures/ valid_workspace/ # FFCA repo that must pass invalid_workspace/ # One package per violation the validator must catch + warning_workspace/ # Warnings and suppressed errors only, must exit 0 not_ffca/ # Non-FFCA repo the validator must skip skills/ ffca-architecture/SKILL.md # Orientation, layer rules, naming @@ -106,7 +108,7 @@ Documentation drifts when an asset changes and the docs describing it do not. Up - **The FFCA architecture changes.** Update the VGV Engineering page first, then run `dart run scripts/sync_reference.dart` to re-sync `references/ffca/`. Check every skill, the agent, and the code templates for section names that moved or rules that changed. If a dependency rule changed, update the rules table at the top of `scripts/validate_layers.dart` and its tests. - **A skill's scope or triggers change.** Update `description` and the matching row in the `README.md` Skills table. - **The validator's rules or flags change.** Update `scripts/test/validate_layers_test.dart` and its fixtures, the **Hooks** section of `README.md`, and the `## Hooks` section of `CLAUDE.md` if the hook's behavior changes. -- **A hook changes in `hooks/hooks.json`.** Update the **Hooks** section of `README.md` and the `## Hooks` section of `CLAUDE.md`. If it is `check_vgv_cli.sh`, update `hooks/check_vgv_cli_test.sh`. +- **A hook changes in `hooks/hooks.json`.** Update the **Hooks** section of `README.md` and the `## Hooks` section of `CLAUDE.md`. Update the hook's test, `hooks/check_vgv_cli_test.sh` or `hooks/validate_layers_test.sh`. - **An MCP tool is added, renamed, or removed.** Check every skill's `allowed-tools` and the **MCP Integration** section of `README.md`. Nothing validates those names. ## Checks diff --git a/CLAUDE.md b/CLAUDE.md index 055faeb..c629309 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -7,12 +7,12 @@ `hooks/hooks.json` defines one PostToolUse hook and one PreToolUse hook. -- `Edit|Write` matcher → `validate_layers.sh`. When the edited file is a `pubspec.yaml` inside an FFCA-shaped repo, meaning a `features/` folder exists above it, the hook runs `dart run scripts/validate_layers.dart --file `. The validator checks the edited package and its direct dependents. Exit 2 means a layer violation. The hook prints the rule and the fix to stderr and exits 2, which blocks Claude until the dependency is fixed. +- `Edit|Write` matcher → `validate_layers.sh`. When the edited file is a `pubspec.yaml` inside an FFCA-shaped repo, meaning a `features/` folder exists above it, the hook runs `dart run scripts/validate_layers.dart --file `. The validator checks the edited package and its direct dependents. Exit 2 means at least one error-severity finding. The hook prints the findings to stderr and exits 2, which blocks Claude until the dependency is fixed. When the validator exits 0 with output, every finding is a warning: the hook returns it as `additionalContext` JSON and exits 0, so Claude sees it without being blocked. A `# ffca-ignore: — ` comment in a pubspec suppresses that rule for that package. - Any other file, or a repo with no `features/` folder, exits 0 silently. - The hook skips with exit 0 when `jq` or `dart` is missing from `PATH`. A validator failure other than exit 2 is reported to stderr but never blocks. - `mcp__.*very-good-cli__.*` matcher → `check_vgv_cli.sh`. For a Very Good CLI MCP tool, it returns `allow` when `very_good --version` is 1.3.0 or newer and `deny` with an install or upgrade message when the CLI is missing or older. It stands aside with exit 0 for any other tool, when the version cannot be read, or when `jq` is missing. `allowed-tools` grants only last for the invoking turn, so this hook is what keeps the tools approved afterwards. -`hooks/check_vgv_cli_test.sh` covers the PreToolUse hook against a stubbed `very_good` and runs in CI under the **Hook Tests** job. +`hooks/check_vgv_cli_test.sh` covers the PreToolUse hook against a stubbed `very_good`, and `hooks/validate_layers_test.sh` covers the PostToolUse hook against a stubbed `dart`. Both run in CI under the **Hook Tests** job. -`scripts/test/validate_layers_test.dart` covers the validator and runs in CI under the **Layer Validator** job. Add a fixture and a case there when changing a rule. +`scripts/test/validate_layers_test.dart` covers the validator and runs in CI under the **Layer Validator** job. Add a fixture and a case there when changing a rule, and add a new rule's ID and severity to `ruleSeverities` and to the rules table in the **Hooks** section of `README.md`. diff --git a/README.md b/README.md index 5dda356..eb620a9 100644 --- a/README.md +++ b/README.md @@ -118,7 +118,7 @@ A PostToolUse hook runs on every `Edit` or `Write`. When the edited file is a `p | Hook | Event | Behavior | | --- | --- | --- | -| **Validate layers** (`validate_layers.sh`) | PostToolUse (`Edit`/`Write`) | Runs the FFCA validator incrementally on the edited package and its direct dependents. Exits 2 on a violation (blocking: Claude must fix the dependency before continuing), printing the rule and the fix. Passes silently otherwise | +| **Validate layers** (`validate_layers.sh`) | PostToolUse (`Edit`/`Write`) | Runs the FFCA validator incrementally on the edited package and its direct dependents. Exits 2 on an error (blocking: Claude must fix the dependency before continuing), printing the rule and the fix. Passes warnings to Claude as non-blocking context. Passes silently otherwise | | **Check VGV CLI** (`check_vgv_cli.sh`) | PreToolUse (`mcp__.*very-good-cli__.*`) | Auto-approves Very Good CLI MCP tool calls when the CLI is installed at 1.3.0 or newer, so they work in every run mode. Denies with an install or upgrade message when the CLI is missing or outdated. Stands aside for any other tool, or when the CLI version cannot be read | The validator checks package naming under `features/`, the layer dependency rules, that nothing depends on an app, and cycles. It also checks that each feature's packages fit together: a data package depends on its own `_domain` under `dependencies`, a data package never exists without a `_domain` in its feature, and a `_domain` package never depends on the Flutter SDK. @@ -129,6 +129,39 @@ The validator is also runnable directly for CI and audits, across the whole work dart run scripts/validate_layers.dart --all ``` +### Rules, severities, and suppressions + +Every finding carries a rule ID in brackets. An **error** exits 2 and blocks the hook. A **warning** is reported but exits 0. + +| Rule ID | Severity | Finding | +| --- | --- | --- | +| `package-name` | error | A package under `features/{f}/` is not named `{f}_domain`, `{f}_data`, `{f}_data_{backend}`, or `{f}_presentation` | +| `depends-on-app` | error | Any package depends on an app | +| `shared-depends-on-feature` | error | A shared package depends on a feature package | +| `presentation-depends-on-data` | error | A presentation package depends on a data package | +| `domain-depends-on-data` | error | A domain package depends on a data package | +| `domain-depends-on-presentation` | error | A domain package depends on a presentation package | +| `data-depends-on-presentation` | error | A data package depends on a presentation package | +| `data-depends-on-feature-data` | error | A data package depends on another feature's data package | +| `layer-dependency` | error | Any other dependency the rules table does not allow | +| `dependency-cycle` | error | Packages depend on each other in a cycle | +| `data-missing-own-domain` | error | A data package does not depend on its own `_domain` under `dependencies` | +| `data-without-domain` | error | A data package's feature has no `_domain` package | +| `domain-uses-flutter` | error | A domain package depends on the Flutter SDK | +| `presentation-missing-own-domain` | warning | A presentation package does not depend on its feature's existing `_domain` | +| `suppression-missing-reason` | error | An `ffca-ignore` comment has no reason | +| `suppression-unknown-rule` | error | An `ffca-ignore` comment names a rule that does not exist or cannot be suppressed | +| `unused-suppression` | warning | An `ffca-ignore` comment matches no finding (`--all` only) | + +To accept a finding as a deliberate exception, add a comment with the rule ID and a reason to the pubspec of the package it is reported on: + +```yaml +# ffca-ignore: data-missing-own-domain — legacy repository, moving to orders_domain in #123 +name: orders_data +``` + +A suppression covers every finding with that rule ID in that package. The reason is required, so each exception explains itself in review. The last three rules cannot be suppressed. `--all` reports how many findings were suppressed, and flags suppressions that no longer match anything so they can be removed. + ### Prerequisites - **Dart SDK** must be available on your `PATH`. The validator imports only `dart:io`, so it runs with just the SDK, no `dart pub get` required. @@ -138,6 +171,7 @@ Run the hook tests with: ```bash bash hooks/check_vgv_cli_test.sh +bash hooks/validate_layers_test.sh ``` ## MCP Integration diff --git a/agents/ffca-layer-auditor.md b/agents/ffca-layer-auditor.md index ab16c7a..e3eee6b 100644 --- a/agents/ffca-layer-auditor.md +++ b/agents/ffca-layer-auditor.md @@ -39,7 +39,7 @@ From the repo root, run the validator in full-graph mode and treat every line it dart run ${CLAUDE_PLUGIN_ROOT}/scripts/validate_layers.dart --all ``` -This is the authoritative check for the pubspec dependency graph: package naming under `features/`, the layer dependency rules, the no-dependency-on-an-app rule, workspace-wide cycles, and feature ownership (a data package depends on its own domain, a data package has a domain in its feature, a domain package never depends on the Flutter SDK). It prints each violation with its rule and fix and exits 2 if any are found, 0 if the graph is clean. Do not re-derive these checks by hand; the script is the source of truth for them. Read `references/ffca/project_structure.md`, sections *Dependency rules* and *Naming conventions*, only if you need to explain a finding. +This is the authoritative check for the pubspec dependency graph: package naming under `features/`, the layer dependency rules, the no-dependency-on-an-app rule, workspace-wide cycles, and feature ownership (a data package depends on its own domain, a data package has a domain in its feature, a domain package never depends on the Flutter SDK). It prints each finding with its rule ID, severity (`✗` error, `⚠` warning), and fix, then a summary line with the error, warning, and suppressed counts. It exits 2 if any error is found, and 0 when there are only warnings or nothing at all. An error makes the package `fail`; a warning makes it `warn`. A finding suppressed with an `# ffca-ignore:` comment is an accepted exception: do not report it as a violation, but list the suppression and its reason in the package's notes. Do not re-derive these checks by hand; the script is the source of truth for them. Read `references/ffca/project_structure.md`, sections *Dependency rules* and *Naming conventions*, only if you need to explain a finding. ## Step 2: source-level checks (qualitative) diff --git a/hooks/validate_layers.sh b/hooks/validate_layers.sh index 5dc91fa..22e0e67 100755 --- a/hooks/validate_layers.sh +++ b/hooks/validate_layers.sh @@ -5,6 +5,7 @@ set -euo pipefail # Mirrors the analyze hook in vgv-ai-flutter-plugin: parse the payload with jq, # filter to the files we care about, skip gracefully when prerequisites are # missing, then run the validator and propagate its exit code (2 blocks Claude). +# Warnings exit 0 and reach Claude as non-blocking additional context. # Read the hook payload from stdin. input=$(cat) @@ -64,4 +65,11 @@ if [[ "$code" -ne 0 ]]; then exit 0 fi +# Exit 0 with output means warnings only. Stderr on exit 0 never reaches +# Claude, so hand the warnings over as additional context, without blocking. +if [[ -n "$output" ]]; then + jq -n --arg ctx "FFCA validator warnings (non-blocking):"$'\n'"$output" \ + '{hookSpecificOutput: {hookEventName: "PostToolUse", additionalContext: $ctx}}' +fi + exit 0 diff --git a/hooks/validate_layers_test.sh b/hooks/validate_layers_test.sh new file mode 100644 index 0000000..5eca3d8 --- /dev/null +++ b/hooks/validate_layers_test.sh @@ -0,0 +1,124 @@ +#!/bin/bash +# Tests for validate_layers.sh +# +# Usage: bash hooks/validate_layers_test.sh +# +# The hook reads a JSON payload from stdin, runs the validator on an edited +# pubspec.yaml, and maps its result to one of four outcomes: silent, blocked +# (exit 2 with the findings on stderr), warned (exit 0 with the findings as +# additional context on stdout), or skipped. Every case runs against a stubbed +# dart on a PATH that contains nothing else, so results do not depend on the +# Dart SDK or on the validator itself, which scripts/test covers. + +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +HOOK="$SCRIPT_DIR/validate_layers.sh" + +PASSED=0 +FAILED=0 + +STUB_DIR="$(mktemp -d)" +trap 'rm -rf "$STUB_DIR"' EXIT + +BASE_PATH="$(dirname "$(command -v jq)"):/usr/bin:/bin:/usr/sbin:/sbin" + +# An FFCA-shaped repo and a non-FFCA one, for the edited file paths. +FFCA_PUBSPEC="$STUB_DIR/repo/features/cart/cart_data/pubspec.yaml" +OTHER_PUBSPEC="$STUB_DIR/other/packages/some_pkg/pubspec.yaml" +mkdir -p "$(dirname "$FFCA_PUBSPEC")" "$(dirname "$OTHER_PUBSPEC")" +touch "$FFCA_PUBSPEC" "$OTHER_PUBSPEC" + +# Install a stubbed dart that prints [output] to stderr and exits [code], the +# way the validator reports its findings. +# Usage: stub_dart [output] +stub_dart() { + printf '%s' "${2:-}" > "$STUB_DIR/dart_output" + printf '#!/bin/sh\ncat "%s" >&2\nexit %s\n' "$STUB_DIR/dart_output" "$1" > "$STUB_DIR/bin/dart" + chmod +x "$STUB_DIR/bin/dart" +} +mkdir -p "$STUB_DIR/bin" + +no_dart() { rm -f "$STUB_DIR/bin/dart"; } + +payload() { printf '{"tool_name":"Edit","tool_input":{"file_path":"%s"}}' "$1"; } + +# Prints "silent" (exit 0, no output), "blocked" (exit 2, findings on stderr), +# "warned" (exit 0, findings in additionalContext), or "exit:" / +# "malformed" for anything else, so a crashing hook cannot pass as silent. +run_hook() { + local input="$1" + local out err status=0 + out=$(printf '%s' "$input" \ + | env -i PATH="$STUB_DIR/bin:$BASE_PATH" CLAUDE_PLUGIN_ROOT="$STUB_DIR" \ + bash "$HOOK" 2>"$STUB_DIR/stderr") || status=$? + err=$(cat "$STUB_DIR/stderr") + if [ "$status" -eq 2 ] && [[ "$err" == *"FFCA findings"* ]]; then + echo "blocked" + elif [ "$status" -ne 0 ]; then + echo "exit:$status" + elif [ -z "$out" ]; then + echo "silent" + elif [ "$(echo "$out" | jq -r '.hookSpecificOutput.additionalContext // empty' \ + | grep -c 'FFCA findings')" -gt 0 ]; then + echo "warned" + else + echo "malformed" + fi +} + +assert_outcome() { + local expected="$1" label="$2" input="$3" + local result + result=$(run_hook "$input") + if [ "$result" = "$expected" ]; then + printf " \033[32mPASS\033[0m %-8s %s\n" "$expected" "$label" + PASSED=$((PASSED + 1)) + else + printf " \033[31mFAIL\033[0m expected %s but got %s: %s\n" "$expected" "$result" "$label" + FAILED=$((FAILED + 1)) + fi +} + +ERROR_OUTPUT='FFCA findings in features/cart/cart_data/pubspec.yaml: + ✗ [data-missing-own-domain] cart_data does not depend on cart_domain + → Add cart_domain under dependencies in the cart_data pubspec. +FFCA: 1 error, 0 warnings.' + +WARNING_OUTPUT='FFCA findings in features/cart/cart_presentation/pubspec.yaml: + ⚠ [presentation-missing-own-domain] cart_presentation does not depend on cart_domain + → Add cart_domain under dependencies in the cart_presentation pubspec. +FFCA: 0 errors, 1 warning.' + +echo "=== validate_layers tests ===" +echo "" +echo "--- Edits that are not this hook's business ---" +stub_dart 2 "$ERROR_OUTPUT" +assert_outcome silent "non-pubspec file" "$(payload "$STUB_DIR/repo/features/cart/cart_data/lib/cart_data.dart")" +assert_outcome silent "pubspec outside an FFCA repo" "$(payload "$OTHER_PUBSPEC")" +assert_outcome silent "no file path" '{"tool_name":"Edit","tool_input":{}}' + +echo "" +echo "--- Validator results on an FFCA pubspec ---" +stub_dart 0 +assert_outcome silent "clean pass" "$(payload "$FFCA_PUBSPEC")" +stub_dart 2 "$ERROR_OUTPUT" +assert_outcome blocked "error finding blocks" "$(payload "$FFCA_PUBSPEC")" +stub_dart 0 "$WARNING_OUTPUT" +assert_outcome warned "warnings reach Claude" "$(payload "$FFCA_PUBSPEC")" + +echo "" +echo "--- Validator failures never block ---" +stub_dart 1 "Unhandled exception" +assert_outcome silent "validator crash" "$(payload "$FFCA_PUBSPEC")" +stub_dart 64 "Usage: validate_layers.dart" +assert_outcome silent "usage error" "$(payload "$FFCA_PUBSPEC")" +no_dart +assert_outcome silent "dart not installed" "$(payload "$FFCA_PUBSPEC")" + +echo "" +echo "=== Results: $PASSED passed, $FAILED failed ===" + +if [ "$FAILED" -gt 0 ]; then + exit 1 +fi diff --git a/scripts/test/fixtures/invalid_workspace/features/beta/beta_presentation/pubspec.yaml b/scripts/test/fixtures/invalid_workspace/features/beta/beta_presentation/pubspec.yaml index 9aeab72..5b4124d 100644 --- a/scripts/test/fixtures/invalid_workspace/features/beta/beta_presentation/pubspec.yaml +++ b/scripts/test/fixtures/invalid_workspace/features/beta/beta_presentation/pubspec.yaml @@ -2,3 +2,5 @@ name: beta_presentation resolution: workspace environment: sdk: ^3.11.0 +dependencies: + beta_domain: any diff --git a/scripts/test/fixtures/invalid_workspace/features/omicron/omicron_data/pubspec.yaml b/scripts/test/fixtures/invalid_workspace/features/omicron/omicron_data/pubspec.yaml new file mode 100644 index 0000000..754cc8c --- /dev/null +++ b/scripts/test/fixtures/invalid_workspace/features/omicron/omicron_data/pubspec.yaml @@ -0,0 +1,6 @@ +# A suppression with no reason suppresses nothing and is itself an error. +# ffca-ignore: data-missing-own-domain +name: omicron_data +resolution: workspace +environment: + sdk: ^3.11.0 diff --git a/scripts/test/fixtures/invalid_workspace/features/omicron/omicron_domain/pubspec.yaml b/scripts/test/fixtures/invalid_workspace/features/omicron/omicron_domain/pubspec.yaml new file mode 100644 index 0000000..fda88ce --- /dev/null +++ b/scripts/test/fixtures/invalid_workspace/features/omicron/omicron_domain/pubspec.yaml @@ -0,0 +1,4 @@ +name: omicron_domain +resolution: workspace +environment: + sdk: ^3.11.0 diff --git a/scripts/test/fixtures/invalid_workspace/features/pi/pi_domain/pubspec.yaml b/scripts/test/fixtures/invalid_workspace/features/pi/pi_domain/pubspec.yaml new file mode 100644 index 0000000..6c0e958 --- /dev/null +++ b/scripts/test/fixtures/invalid_workspace/features/pi/pi_domain/pubspec.yaml @@ -0,0 +1,6 @@ +# A suppression naming a rule that does not exist. +# ffca-ignore: domain-uses-widgets — not a rule ID +name: pi_domain +resolution: workspace +environment: + sdk: ^3.11.0 diff --git a/scripts/test/fixtures/invalid_workspace/pubspec.yaml b/scripts/test/fixtures/invalid_workspace/pubspec.yaml index ad8d2dc..41f6d77 100644 --- a/scripts/test/fixtures/invalid_workspace/pubspec.yaml +++ b/scripts/test/fixtures/invalid_workspace/pubspec.yaml @@ -21,7 +21,10 @@ workspace: - features/lambda/lambda_domain - features/mu/mu_data - features/mu/mu_domain + - features/omicron/omicron_data + - features/omicron/omicron_domain - features/orders/orders_service + - features/pi/pi_domain - features/theta/theta_data - features/theta/theta_domain - features/zeta/zeta_domain diff --git a/scripts/test/fixtures/warning_workspace/features/nu/nu_domain/pubspec.yaml b/scripts/test/fixtures/warning_workspace/features/nu/nu_domain/pubspec.yaml new file mode 100644 index 0000000..a0f6da9 --- /dev/null +++ b/scripts/test/fixtures/warning_workspace/features/nu/nu_domain/pubspec.yaml @@ -0,0 +1,4 @@ +name: nu_domain +resolution: workspace +environment: + sdk: ^3.11.0 diff --git a/scripts/test/fixtures/warning_workspace/features/nu/nu_presentation/pubspec.yaml b/scripts/test/fixtures/warning_workspace/features/nu/nu_presentation/pubspec.yaml new file mode 100644 index 0000000..bfe7bc3 --- /dev/null +++ b/scripts/test/fixtures/warning_workspace/features/nu/nu_presentation/pubspec.yaml @@ -0,0 +1,5 @@ +# Presentation layer that does not depend on its own feature's domain. +name: nu_presentation +resolution: workspace +environment: + sdk: ^3.11.0 diff --git a/scripts/test/fixtures/warning_workspace/features/rho/rho_domain/pubspec.yaml b/scripts/test/fixtures/warning_workspace/features/rho/rho_domain/pubspec.yaml new file mode 100644 index 0000000..223a241 --- /dev/null +++ b/scripts/test/fixtures/warning_workspace/features/rho/rho_domain/pubspec.yaml @@ -0,0 +1,6 @@ +# A suppression that no longer matches any finding. +# ffca-ignore: domain-uses-flutter — Flutter dependency already removed +name: rho_domain +resolution: workspace +environment: + sdk: ^3.11.0 diff --git a/scripts/test/fixtures/warning_workspace/features/xi/xi_data/pubspec.yaml b/scripts/test/fixtures/warning_workspace/features/xi/xi_data/pubspec.yaml new file mode 100644 index 0000000..a681e08 --- /dev/null +++ b/scripts/test/fixtures/warning_workspace/features/xi/xi_data/pubspec.yaml @@ -0,0 +1,6 @@ +# An error suppressed with a reason. +# ffca-ignore: data-missing-own-domain — legacy repository, moving to xi_domain +name: xi_data +resolution: workspace +environment: + sdk: ^3.11.0 diff --git a/scripts/test/fixtures/warning_workspace/features/xi/xi_domain/pubspec.yaml b/scripts/test/fixtures/warning_workspace/features/xi/xi_domain/pubspec.yaml new file mode 100644 index 0000000..87d5eab --- /dev/null +++ b/scripts/test/fixtures/warning_workspace/features/xi/xi_domain/pubspec.yaml @@ -0,0 +1,4 @@ +name: xi_domain +resolution: workspace +environment: + sdk: ^3.11.0 diff --git a/scripts/test/fixtures/warning_workspace/pubspec.yaml b/scripts/test/fixtures/warning_workspace/pubspec.yaml new file mode 100644 index 0000000..b210053 --- /dev/null +++ b/scripts/test/fixtures/warning_workspace/pubspec.yaml @@ -0,0 +1,10 @@ +# Warnings and suppressed errors only: --all must exit 0. +name: _warning_workspace +environment: + sdk: ^3.11.0 +workspace: + - features/nu/nu_domain + - features/nu/nu_presentation + - features/rho/rho_domain + - features/xi/xi_data + - features/xi/xi_domain diff --git a/scripts/test/validate_layers_test.dart b/scripts/test/validate_layers_test.dart index 00e6d6a..63dd0bc 100644 --- a/scripts/test/validate_layers_test.dart +++ b/scripts/test/validate_layers_test.dart @@ -12,6 +12,7 @@ const _script = 'validate_layers.dart'; const _valid = 'test/fixtures/valid_workspace'; const _invalid = 'test/fixtures/invalid_workspace'; const _notFfca = 'test/fixtures/not_ffca'; +const _warning = 'test/fixtures/warning_workspace'; /// Runs the validator and returns (exitCode, stdout, stderr). ({int code, String out, String err}) _run(List args, {String? cwd}) { @@ -186,9 +187,31 @@ void main() { }); test('every violation includes a fix line', () { - // One arrow per violation; there are at least the eleven planted rules. + // One arrow per violation; there are at least the thirteen planted rules. final arrows = '→'.allMatches(err).length; - expect(arrows, greaterThanOrEqualTo(11)); + expect(arrows, greaterThanOrEqualTo(13)); + }); + + test('every finding carries a rule ID', () { + final findings = err.split('\n').where((l) => l.startsWith(' ✗')); + expect(findings, isNotEmpty); + for (final line in findings) { + expect(line, matches(RegExp(r'^ ✗ \[[a-z-]+\] '))); + } + }); + + test('ends with a summary line', () { + expect(err, contains(RegExp(r'FFCA: \d+ errors, 0 warnings\.'))); + }); + + test('a suppression without a reason suppresses nothing', () { + expect(err, contains('[suppression-missing-reason]')); + expect(err, contains('omicron_data does not depend on omicron_domain')); + }); + + test('a suppression naming an unknown rule', () { + expect(err, contains('[suppression-unknown-rule]')); + expect(err, contains('"domain-uses-widgets"')); }); }); @@ -312,6 +335,61 @@ void main() { }); }); + group('severities and suppressions', () { + test('--all exits 0 when every finding is a warning or suppressed', () { + final r = _run(['--all', _warning]); + expect(r.code, 0, reason: r.err); + expect(r.err, contains('FFCA: 0 errors, 2 warnings, 1 suppressed.')); + }); + + test('reports a warning with its rule ID', () { + final r = _run(['--all', _warning]); + expect( + r.err, + contains('⚠ [presentation-missing-own-domain] nu_presentation does ' + 'not depend on nu_domain'), + ); + }); + + test('does not print a suppressed finding', () { + final r = _run(['--all', _warning]); + expect(r.err, isNot(contains('xi_data'))); + }); + + test('reports an unused suppression in --all mode', () { + final r = _run(['--all', _warning]); + expect( + r.err, + contains('⚠ [unused-suppression] ffca-ignore: domain-uses-flutter on ' + 'line 2 of the rho_domain pubspec matches no finding'), + ); + }); + + test('incremental mode exits 0 on a warning and still prints it', () { + final r = _run([ + '--file', + _pubspec(_warning, 'features/nu/nu_presentation'), + ]); + expect(r.code, 0); + expect(r.err, contains('[presentation-missing-own-domain]')); + }); + + test('incremental mode is silent on a suppressed finding', () { + final r = _run(['--file', _pubspec(_warning, 'features/xi/xi_data')]); + expect(r.code, 0); + expect(r.err, isEmpty); + }); + + test('incremental mode does not report unused suppressions', () { + final r = _run([ + '--file', + _pubspec(_warning, 'features/rho/rho_domain'), + ]); + expect(r.code, 0); + expect(r.err, isEmpty); + }); + }); + group('graceful skips (exit 0)', () { test('non-pubspec file', () { final r = _run(['--file', '$_valid/README.md']); diff --git a/scripts/validate_layers.dart b/scripts/validate_layers.dart index 1c1ae73..0c063b2 100644 --- a/scripts/validate_layers.dart +++ b/scripts/validate_layers.dart @@ -7,7 +7,9 @@ // - by CI and the ffca-audit skill, across the whole workspace: // dart run validate_layers.dart --all // -// Exit codes: 0 = pass or graceful skip, 2 = FFCA violation, 64 = usage error. +// Exit codes: 0 = pass, warnings only, or graceful skip; 2 = at least one +// error-severity FFCA finding; 64 = usage error. Findings go to stderr, each +// with a rule ID that a `# ffca-ignore:` comment can suppress (see below). // // Imports are restricted to dart:io and dart:core so the script runs with just // the Dart SDK, no `dart pub get` required on the user's machine. @@ -44,6 +46,46 @@ const Map> rules = { 'shared': {'shared'}, }; +// --------------------------------------------------------------------------- +// Rule IDs and severities +// +// Every finding carries one of these stable IDs, which is also what a +// suppression names. An error exits 2 and blocks the hook; a warning is printed +// but leaves the exit code at 0. +// +// A finding is suppressed with a comment in the pubspec of the package it is +// reported on. The reason is required: +// # ffca-ignore: — +// The suppression-* rules and unused-suppression cannot be suppressed. +// --------------------------------------------------------------------------- +enum Severity { error, warning } + +const Map ruleSeverities = { + 'package-name': Severity.error, + 'depends-on-app': Severity.error, + 'shared-depends-on-feature': Severity.error, + 'presentation-depends-on-data': Severity.error, + 'domain-depends-on-data': Severity.error, + 'domain-depends-on-presentation': Severity.error, + 'data-depends-on-presentation': Severity.error, + 'data-depends-on-feature-data': Severity.error, + 'layer-dependency': Severity.error, + 'dependency-cycle': Severity.error, + 'data-missing-own-domain': Severity.error, + 'data-without-domain': Severity.error, + 'domain-uses-flutter': Severity.error, + 'presentation-missing-own-domain': Severity.warning, + 'suppression-missing-reason': Severity.error, + 'suppression-unknown-rule': Severity.error, + 'unused-suppression': Severity.warning, +}; + +const _unsuppressible = { + 'suppression-missing-reason', + 'suppression-unknown-rule', + 'unused-suppression', +}; + void main(List args) { if (args.contains('--all')) { exit(_runAll(args)); @@ -92,9 +134,10 @@ int _runIncremental(String rawPath) { } violations.addAll(_cycleViolations(workspace, from: edited)); - if (violations.isEmpty) return 0; // silent pass - _report(violations, root); - return 2; + final result = _applySuppressions(violations, scope, reportUnused: false); + if (result.findings.isEmpty) return 0; // silent pass + _report(result, root); + return _exitCode(result.findings); } /// Full-graph mode (CI and ffca-audit): validate every package and run a @@ -119,14 +162,98 @@ int _runAll(List args) { } violations.addAll(_cycleViolations(workspace)); - if (violations.isEmpty) { + final result = _applySuppressions( + violations, + workspace.packages, + reportUnused: true, + ); + if (result.findings.isEmpty) { + final suppressed = + result.suppressed > 0 ? ', ${result.suppressed} suppressed' : ''; stdout.writeln( - 'No FFCA violations found (${workspace.packages.length} packages checked).', + 'No FFCA violations found ' + '(${workspace.packages.length} packages checked$suppressed).', ); return 0; } - _report(violations, root); - return 2; + _report(result, root); + return _exitCode(result.findings); +} + +int _exitCode(List<_Violation> findings) => + findings.any((v) => v.severity == Severity.error) ? 2 : 0; + +// --------------------------------------------------------------------------- +// Suppressions +// --------------------------------------------------------------------------- + +/// Drops the findings suppressed in their package's pubspec and adds findings +/// for invalid suppressions in [scope]. Unused suppressions are only reported +/// with [reportUnused], in full mode, where every finding has been computed. +({List<_Violation> findings, int suppressed}) _applySuppressions( + List<_Violation> violations, + Iterable scope, { + required bool reportUnused, +}) { + bool valid(_Suppression s) => + s.reason.isNotEmpty && + ruleSeverities.containsKey(s.ruleId) && + !_unsuppressible.contains(s.ruleId); + + final used = <_Suppression>{}; + final findings = <_Violation>[]; + var suppressed = 0; + for (final v in violations) { + final match = v.package.suppressions + .where((s) => valid(s) && s.ruleId == v.id) + .firstOrNull; + if (match == null) { + findings.add(v); + } else { + used.add(match); + suppressed++; + } + } + + for (final p in scope) { + for (final s in p.suppressions) { + final where = 'line ${s.line} of the ${p.name} pubspec'; + if (!ruleSeverities.containsKey(s.ruleId) || + _unsuppressible.contains(s.ruleId)) { + findings.add( + _Violation( + id: 'suppression-unknown-rule', + package: p, + rule: 'ffca-ignore on $where names "${s.ruleId}", which is not a ' + 'rule that can be suppressed', + fix: 'Use one of the rule IDs shown in brackets in the validator ' + 'output, or remove the comment.', + ), + ); + } else if (s.reason.isEmpty) { + findings.add( + _Violation( + id: 'suppression-missing-reason', + package: p, + rule: 'ffca-ignore: ${s.ruleId} on $where has no reason, so it ' + 'suppresses nothing', + fix: 'Add the reason after the rule ID: ' + '# ffca-ignore: ${s.ruleId} — ', + ), + ); + } else if (reportUnused && !used.contains(s)) { + findings.add( + _Violation( + id: 'unused-suppression', + package: p, + rule: 'ffca-ignore: ${s.ruleId} on $where matches no finding', + fix: 'Remove the comment; the exception is no longer needed.', + ), + ); + } + } + } + return (findings: findings, suppressed: suppressed); } // --------------------------------------------------------------------------- @@ -140,6 +267,7 @@ List<_Violation> _checkPackage(Package p, Workspace ws) { if (p.top == 'features' && p.layer == 'unknown') { out.add( _Violation( + id: 'package-name', package: p, rule: '${p.name} is under features/${p.feature}/ but is not a valid ' 'FFCA package name (expected ${p.feature}_domain, ${p.feature}_data, ' @@ -182,6 +310,7 @@ List<_Violation> _checkOwnership(Package p, Workspace ws) { if (domain == null) { out.add( _Violation( + id: 'data-without-domain', package: p, rule: '${p.name} is a data layer but features/${p.feature}/ has no ' '$domainName (a data layer implements its own feature\'s domain)', @@ -194,6 +323,7 @@ List<_Violation> _checkOwnership(Package p, Workspace ws) { final devOnly = p.deps.contains(domain.dir); out.add( _Violation( + id: 'data-missing-own-domain', package: p, rule: devOnly ? '${p.name} declares $domainName only outside dependencies ' @@ -206,9 +336,30 @@ List<_Violation> _checkOwnership(Package p, Workspace ws) { } } + // A presentation-only feature has no domain, so this only applies when the + // feature has one. + if (p.layer == 'presentation') { + final domain = ws.packages + .where((d) => d.feature == p.feature && d.name == domainName) + .firstOrNull; + if (domain != null && !p.runtimeDeps.contains(domain.dir)) { + out.add( + _Violation( + id: 'presentation-missing-own-domain', + package: p, + rule: '${p.name} does not depend on $domainName (a presentation ' + "layer renders its own feature's domain)", + fix: 'Add $domainName under dependencies in the ${p.name} pubspec, ' + 'or suppress this with the reason the screen does not use it.', + ), + ); + } + } + if (p.layer == 'domain' && p.flutterSdkDeps.isNotEmpty) { out.add( _Violation( + id: 'domain-uses-flutter', package: p, rule: '${p.name} depends on the Flutter SDK through ' '${p.flutterSdkDeps.join(', ')} (the domain layer is pure Dart)', @@ -227,6 +378,7 @@ _Violation _dependencyViolation(Package src, Package tgt) { // Target is an app: nothing may depend on an app. if (tgt.layer == 'app') { return _Violation( + id: 'depends-on-app', package: src, rule: '$pair (nothing may depend on an app)', fix: 'Apps are top-level composition roots; remove the dependency on ' @@ -237,6 +389,7 @@ _Violation _dependencyViolation(Package src, Package tgt) { // Source is shared depending on a feature package. if (src.layer == 'shared') { return _Violation( + id: 'shared-depends-on-feature', package: src, rule: '$pair (shared packages must never depend on a feature)', fix: 'Move the shared code into the feature, or invert the dependency; ' @@ -247,6 +400,7 @@ _Violation _dependencyViolation(Package src, Package tgt) { switch ('${src.layer}->${tgt.layer}') { case 'presentation->data': return _Violation( + id: 'presentation-depends-on-data', package: src, rule: '$pair (presentation must never depend on a data layer)', fix: 'Depend on ${tgt.feature}_domain instead and access data through ' @@ -254,6 +408,7 @@ _Violation _dependencyViolation(Package src, Package tgt) { ); case 'domain->data': return _Violation( + id: 'domain-depends-on-data', package: src, rule: '$pair (a domain layer must never depend on a data layer)', fix: 'Domain packages define interfaces only; depend on other *_domain ' @@ -261,6 +416,7 @@ _Violation _dependencyViolation(Package src, Package tgt) { ); case 'domain->presentation': return _Violation( + id: 'domain-depends-on-presentation', package: src, rule: '$pair (a domain layer must never depend on a presentation layer)', @@ -269,6 +425,7 @@ _Violation _dependencyViolation(Package src, Package tgt) { ); case 'data->presentation': return _Violation( + id: 'data-depends-on-presentation', package: src, rule: '$pair (a data layer must never depend on a presentation layer)', fix: 'Data packages implement domain interfaces; depend on ' @@ -276,6 +433,7 @@ _Violation _dependencyViolation(Package src, Package tgt) { ); case 'data->data': return _Violation( + id: 'data-depends-on-feature-data', package: src, rule: "$pair (a data layer must never depend on another feature's data " 'layer)', @@ -284,6 +442,7 @@ _Violation _dependencyViolation(Package src, Package tgt) { ); default: return _Violation( + id: 'layer-dependency', package: src, rule: '$pair (a ${src.layer} layer must not depend on a ${tgt.layer} ' 'layer)', @@ -325,6 +484,7 @@ List<_Violation> _cycleViolations(Workspace ws, {Package? from}) { if (seenCycles.add(key)) { out.add( _Violation( + id: 'dependency-cycle', package: cycle.first == next.name ? next : stack[startIdx], rule: 'dependency cycle detected: ${cycle.join(' → ')}', fix: 'Break the cycle; FFCA requires an acyclic dependency ' @@ -375,6 +535,7 @@ Workspace _discover(String root) { final pkgDir = _normalize(entity.parent.absolute.path); final parsed = _parsePubspec(entity); final package = _buildPackage(pkgDir, entity, parsed.name, top, root); + package.suppressions.addAll(parsed.suppressions); package.flutterSdkDeps.addAll([ for (final dep in parsed.deps) if (dep.runtime && dep.sdk == 'flutter') dep.name, @@ -441,6 +602,7 @@ Package _buildPackage( deps: [], runtimeDeps: [], flutterSdkDeps: [], + suppressions: [], ); } @@ -461,6 +623,20 @@ _ParsedPubspec _parsePubspec(File file) { final deps = <_Dependency>[]; final lines = file.readAsLinesSync(); + // Suppressions are comments, so read them before comments are stripped. + final suppressions = <_Suppression>[]; + for (var i = 0; i < lines.length; i++) { + final match = _suppressionPattern.firstMatch(lines[i]); + if (match == null) continue; + suppressions.add( + _Suppression( + ruleId: match.group(1)!, + reason: match.group(2)!.replaceFirst(_reasonSeparator, '').trim(), + line: i + 1, + ), + ); + } + String? currentTop; for (var i = 0; i < lines.length; i++) { final line = _stripComment(lines[i]); @@ -514,9 +690,14 @@ _ParsedPubspec _parsePubspec(File file) { } } } - return _ParsedPubspec(name: name, deps: deps); + return _ParsedPubspec(name: name, deps: deps, suppressions: suppressions); } +/// `# ffca-ignore: `, where the separator is an +/// em dash, an en dash, a hyphen, or a colon. +final _suppressionPattern = RegExp(r'#\s*ffca-ignore:\s*([A-Za-z0-9_-]+)(.*)$'); +final _reasonSeparator = RegExp(r'^\s*[—–:-]+'); + String _stripComment(String line) { final hash = line.indexOf('#'); if (hash == -1) return line; @@ -542,22 +723,38 @@ String _unquote(String s) { // Output // --------------------------------------------------------------------------- -void _report(List<_Violation> violations, String root) { - // Group by the pubspec file the violation belongs to. +void _report( + ({List<_Violation> findings, int suppressed}) result, + String root, +) { + // Group by the pubspec file the finding belongs to. final byPackage = >{}; - for (final v in violations) { + for (final v in result.findings) { byPackage.putIfAbsent(v.package.relPath, () => []).add(v); } final paths = byPackage.keys.toList()..sort(); for (final path in paths) { - stderr.writeln('FFCA violation in $path:'); + stderr.writeln('FFCA findings in $path:'); for (final v in byPackage[path]!) { - stderr.writeln(' ✗ ${v.rule}'); + final mark = v.severity == Severity.error ? '✗' : '⚠'; + stderr.writeln(' $mark [${v.id}] ${v.rule}'); stderr.writeln(' → ${v.fix}'); } } + + final errors = + result.findings.where((v) => v.severity == Severity.error).length; + final warnings = result.findings.length - errors; + final suppressed = + result.suppressed > 0 ? ', ${result.suppressed} suppressed' : ''; + stderr.writeln( + 'FFCA: ${_plural(errors, 'error')}, ${_plural(warnings, 'warning')}' + '$suppressed.', + ); } +String _plural(int n, String word) => '$n $word${n == 1 ? '' : 's'}'; + // --------------------------------------------------------------------------- // Path helpers (avoid package:path so the script stays dependency-free) // --------------------------------------------------------------------------- @@ -609,6 +806,7 @@ class Package { required this.deps, required this.runtimeDeps, required this.flutterSdkDeps, + required this.suppressions, }); final String dir; // normalized absolute package directory @@ -620,12 +818,18 @@ class Package { final List deps; // normalized absolute dirs of workspace dependencies final List runtimeDeps; // the subset declared under `dependencies` final List flutterSdkDeps; // runtime deps with `sdk: flutter` + final List<_Suppression> suppressions; // `# ffca-ignore:` comments } class _ParsedPubspec { - _ParsedPubspec({required this.name, required this.deps}); + _ParsedPubspec({ + required this.name, + required this.deps, + required this.suppressions, + }); final String? name; final List<_Dependency> deps; + final List<_Suppression> suppressions; } class _Dependency { @@ -642,8 +846,24 @@ class _Dependency { } class _Violation { - _Violation({required this.package, required this.rule, required this.fix}); + _Violation({ + required this.id, + required this.package, + required this.rule, + required this.fix, + }); + final String id; // a key of ruleSeverities final Package package; final String rule; final String fix; + + Severity get severity => ruleSeverities[id]!; +} + +class _Suppression { + _Suppression( + {required this.ruleId, required this.reason, required this.line}); + final String ruleId; + final String reason; // empty when none was given + final int line; // 1-based line in the pubspec }