feat: Drush CLI commands, AI skill files, E2E tests for full GUI/TUI parity

[jjroelofs]

Summary

Implements #18 — full GUI/TUI feature parity with AI-agent integration, following the architecture established in dxpr/rl#32, dxpr/dxpr_builder#4468, dxpr/dxpr_theme#803 + dxpr/dxpr_theme_helper#49.

Before: 22 GUI features, 0 TUI commands, 0% parity
After: 22 GUI features, 26 TUI commands (22 mapped + 4 TUI-only), 3 AI skill files, 10 E2E test files, 100% parity

Changes

Architecture Fixes

• Replace global pre-command hook with explicit switchToAdmin() per command
• Remove create() factory methods from all 7 command classes (drush.services.yml tagged services only)
• Add --dry-run to acs:card:edit, acs:idea:edit, acs:idea:implement
• Add switchToAdmin() calls to all 22 command methods
• Add getModulePath() and getProjectRoot() to base class

New: AI Skill Files & Setup Command (Phase 5)

• SetupCommands.php — acs:setup-ai with --host and --check
• .claude/skills/acs/SKILL.md with preamble auto-discovery
• .agents/skills/acs/SKILL.md condensed cross-agent version
• .agents/skills/acs/agents/openai.yaml
• hook_requirements() for skill file staleness detection
• .gitignore and drush.services.yml updates

New: E2E Test Suite (Phase 6)

• scripts/e2e/ with _helpers.sh, run-e2e-tests.sh, 8 test files
• docker-compose.yml e2e-test service, drush-e2e CI job

Documentation (Phase 7)

• desc.html: AI prompting examples for end users
• README.md: Drush CLI section, command table, AI integration, E2E docs

Lint Fixes

• Remove PHPCompatibility (incompatible with phpcs v4)
• Fix phpcbf exit code handling and array indentation

Test plan

• PHPCS: 0 errors
• PHPStan: pre-existing errors only (9, reduced from 11)
• E2E: docker compose --profile test run --rm e2e-test
• Manual: drush acs:setup-ai --check / --host=claude / --host=all
• Manual: Drupal status report shows skill file status

Closes #18

[jjroelofs]

I found four blocking issues while reviewing this against #18:

1. acs:sitemap makes the new drush-e2e job fail in the exact environment this PR adds. scripts/e2e/run-e2e-tests.sh:27-71 creates a local Drupal install but never starts an HTTP server, while scripts/e2e/test-report.sh:27-32 calls drush acs:sitemap -l http://localhost. The command path in src/Drush/Commands/ReportCommands.php:157-160 -> src/Service/ContentAnalyzer.php:212-302 always tries to fetch http://.../sitemap.xml over HTTP, so the job reliably dies with cURL 7. Unless the runner boots a reachable site or acs:sitemap stops depending on an external HTTP listener, phase 6 is still red.

2. acs:generate --category=... currently behaves the same as acs:generate:add, which collapses two distinct commands from the issue into one append-only action. src/Drush/Commands/GenerationCommands.php:74-76 routes category mode into generateForCategory(), and that method ends by calling RecommendationStorageService::addToSection() (src/Service/RecommendationStorageService.php:458-465). So rerunning acs:generate --category=content_gaps duplicates cards instead of regenerating that category, while acs:generate:add is already the dedicated “add more cards” command.

3. hook_requirements() treats documented partial installs from acs:setup-ai --host=claude|agents as outdated forever. The command explicitly advertises host-scoped installs in src/Drush/Commands/SetupCommands.php:19-24, but ai_content_strategy_requirements() always checks all three files (ai_content_strategy.install:35-61). After a successful drush acs:setup-ai --host=claude, status report will still warn that the two .agents files are missing, which makes the --host option misleading unless the selected host(s) are persisted and respected here.

4. The issue contract says mutating commands should return dry_run: true in their YAML envelope, but several implementations omit it. acs:card:delete (src/Drush/Commands/CardCommands.php:105-111), acs:idea:delete (src/Drush/Commands/IdeaCommands.php:171-174), all three category write commands (src/Drush/Commands/CategoryCommands.php:122-129, 211-214, 255-258), and acs:settings:set (src/Drush/Commands/SettingsCommands.php:55-58) return a dry-run message without the machine-readable flag. That breaks the safety contract from feat: Drush CLI commands, AI skill files, E2E tests, and documentation for full GUI/TUI parity #18/README for AI agents and scripts, because they can no longer reliably distinguish preview from commit for those commands.

[jjroelofs]

Critical Code Review — PR #19

Thorough review against the issue #18 spec and the referenced architecture from dxpr/rl#32, dxpr/dxpr_builder#4468, dxpr/dxpr_theme#803.

---

🔴 Critical

1. Operator precedence bug in ReportCommands::reportStatus() (line ~140)

'generated_at' => $stored['timestamp'] ?? NULL ? date('c', $stored['timestamp']) : NULL,

?? has lower precedence than ?:. This evaluates as $stored['timestamp'] ?? (NULL ? date('c', ...) : NULL) — meaning date() is never called. The timestamp is returned as a raw integer instead of an ISO 8601 string. Additionally, when $stored is NULL, accessing $stored['timestamp'] triggers a PHP warning.

Fix: ($stored['timestamp'] ?? NULL) ? date('c', $stored['timestamp']) : NULL — and guard for $stored === NULL first.

2. Static \Drupal:: calls in GenerationCommands::generateForCategory() (lines ~282, 285)

$card['uuid'] = \Drupal::service('uuid')->generate();
$card['content_ideas'] = \Drupal::service('ai_content_strategy.recommendation_storage')
    ->normalizeIdeasWithUuids($card['content_ideas']);

Directly contradicts the design principle of constructor injection via drush.services.yml. The UUID service is not injected at all, and $this->storage is already available but bypassed via the static call. The issue spec and all three reference PRs explicitly removed this pattern.

---

🟠 High

3. Missing --dry-run on generation commands

acs:generate, acs:generate:more, and acs:generate:add all mutate state (save/overwrite recommendations) but none supports --dry-run. The spec states: "All mutating commands support --dry-run." These are the most impactful mutating commands — they call external AI APIs and overwrite stored data.

4. acs:generate silently overwrites curated data

$this->storage->saveRecommendations() replaces the entire stored dataset. If a user has curated cards (marked ideas as implemented, added links, edited titles), running acs:generate destroys all that work. No confirmation, no --force flag, and no dry-run. This is the single most dangerous command and has zero safety rails.

5. No validation of --file path in ExportCommands::export()

file_put_contents($options['file'], $output) has no check for directory existence, writability, or return value (false on failure). The success envelope is returned unconditionally regardless of write outcome.

6. switchToAdmin() hardcodes UID 1 with silent failure

If UID 1 is disabled/deleted, or if the catch (\Exception) triggers, the command silently proceeds as the wrong user. This could produce confusing permission errors or incomplete results with no indication of root cause. At minimum, log a warning on failure.

7. Recursive regex in GenerationCommands (line ~160)

preg_match('/\[(?:[^\[\]]|(?R))*\]/', $text, $matches)

(?R) recursive patterns can cause catastrophic backtracking or stack overflow on large/malformed AI responses. PCRE hitting its recursion limit returns FALSE (error), not a non-match. The return value is not checked with preg_last_error(). A simpler approach: find first [ and use json_decode on substrings.

8. E2E tests have no fixtures — only error paths are tested

Issue #18 explicitly requires: "Creates test fixtures: sample categories, pre-populated recommendations via drush php:eval." The actual run-e2e-tests.sh creates no fixtures. Consequence:

• test-cards.sh: Only tests "not found" paths. Never tests editing/deleting an existing card.
• test-ideas.sh: Same — all tests are against nonexistent resources.
• test-export.sh: Only tests "No recommendations" error. Never tests actual YAML/JSON/CSV output.
• test-report.sh: Never verifies actual report content or filtering.

This suite validates error handling but zero happy-path functionality.

9. assert_success / assert_fail don't match spec

Issue #18 says: "assert_success — parses YAML, checks success: true" and "assert_fail — parses YAML, checks success: false". Actual implementation checks shell exit codes, not YAML content. Since all commands return exit 0 (even on logical errors), assert_fail would always fail. Moot point: neither function is ever called by any test file.

10. test-generation.sh has a vacuous assertion

assert_has "generate without provider returns message" ":" "$output"

This asserts the output contains a colon character. Any YAML output matches. This tests nothing.

---

🟡 Medium

11. successList() envelope missing message field

Returns success, count, items — but no message. The spec says envelope is success/message/data or items+count. This is inconsistent with success() which always includes message.

12. updateCategory() and acs:card:edit can't clear fields to empty strings

if (!empty($options['label'])) { ... }

!empty() means passing --instructions="" is silently ignored. Users cannot intentionally clear optional fields.

13. CSV export doesn't handle newlines in fields

csvRow() escapes double quotes but doesn't handle embedded newlines. Since rows are joined with \n, a description containing a literal newline breaks CSV structure (RFC 4180 violation).

14. Unused constructor dependencies in GenerationCommands

$schemaBuilder (CategorySchemaBuilder) and $configFactory (ConfigFactoryInterface) are injected and registered in drush.services.yml but never used in any method. Dead dependencies.

15. acs:export bypasses the YAML envelope contract

When no --file is given, export() returns raw content (YAML/JSON/CSV) without the success/message/data envelope. Intentional for piping, but breaks the stated contract. Should be documented in #[CLI\Help].

16. Many commands have only 1 alias tier instead of spec'd 2-3

Spec: "Three tiers: long, short, medium." Commands like acs:health, acs:generate, acs:export, acs:sitemap, acs:setup-ai only have the short alias. Missing medium aliases (e.g., acs:g for acs:generate).

17. Inconsistent Yaml::dump() parameters

ExportCommands calls Yaml::dump($data, 6, 2) directly rather than the inherited $this->yaml() (which uses inline level 4). Maintenance concern if output format needs to change globally.

18. Runner uses drupal/recommended-project:11.x-dev — unstable

The drupal-test service uses 11.2.x-dev (specific minor), but the E2E runner uses 11.x-dev (bleeding edge). Should pin to a specific minor.

19. CI missing "Generate summary" step from spec

Issue #18 includes a if: always() step to parse test output into a GitHub Actions summary table. Not implemented — test failures require scrolling raw logs.

20. Claude SKILL.md frontmatter deviates from spec

Spec shows trigger (singular) field. Implementation uses triggers (plural, as a list) plus name and version fields not in the spec. May affect Claude Code skill discovery.

21. openai.yaml uses different field names than spec

Spec shows name: ai-content-strategy. Implementation uses display_name: "AI Content Strategy" plus default_prompt not in spec.

22. set -euo pipefail in test scripts creates fragile coupling

All test scripts use set -e. They call Drush commands expecting error output, which works only because error responses return exit 0. If any command is refactored to use self::EXIT_FAILURE, tests silently abort instead of failing cleanly. || true should be appended to expected-failure invocations.

---

🔵 Low

23. formatCard() type-unsafe fallback — $idea['text'] ?? $idea assigns an array to a string field if text key is missing.

24. getProjectRoot() fallback getcwd() can return false — unlikely but would cause file_exists() to get a non-string argument.

25. md5_file() for staleness detection — functional but hash_file('sha256', ...) or direct content comparison would be more robust.

26. assert_has uses grep without -F flag — regex metacharacters in test strings could cause false matches. Should use grep -qF.

27. yq installed at runtime but never used — apk add yq in runner adds CI latency for a dependency only needed by assert_dry_run, which is never called.

28. No test for --undo, --description, --file, --category filter, --priority filter — multiple documented options have zero test coverage.

29. copy() return value unchecked in SetupCommands::installFile() — reports "installed" even if filesystem copy fails.

---

Summary

Severity	Count	Key Themes
Critical	2	Operator precedence bug producing wrong output; DI violation
High	8	Missing --dry-run on dangerous commands; destructive overwrite; E2E tests with no fixtures (error-path-only coverage); spec-violating test helpers
Medium	12	Envelope inconsistencies; can't clear fields; CSV bugs; dead code; CI/skill file spec deviations
Low	7	Type safety edge cases; minor robustness; untested options

Recommended priority for fixes:

1. Broken permission in routes #1 — the operator precedence bug is a definite runtime bug producing wrong output
2. #1: Fix access to routes for non-admin users #2 — static service calls violate the core architecture principle
3. Test main branch #3 + fix: resolve all Drupal coding standard violations and script issues #4 — acs:generate is the most dangerous command and has zero safety rails
4. Test main branch #8 + FIX drupal lint and check issues #9 — test fixtures + spec-compliant helpers would transform the E2E suite from error-handling-only to meaningful functional tests

[jjroelofs]

I re-checked the follow-up commits carefully. The fixes for reportStatus() precedence, the static \Drupal:: calls, the acs:generate --category append-vs-regenerate bug, the partial-install hook_requirements() warning, and the missing dry_run flags on the commands I originally called out all look good.

I still have a few unresolved concerns though:

1. acs:sitemap still is not actually exercised successfully in the E2E environment; the test was only weakened. scripts/e2e/run-e2e-tests.sh:27-54 still provisions a pure CLI Drupal install and never starts an HTTP listener, while src/Service/ContentAnalyzer.php:212-281 still fetches /sitemap.xml over HTTP. The new scripts/e2e/test-report.sh:56-60 now just does || true and asserts the output is non-empty, so the suite will pass even if acs:sitemap still returns success: false or throws a fatal. That doesn’t really resolve the original blocker; it just stops the suite from checking the command.

2. The generation commands still violate the issue-wide --dry-run contract. src/Drush/Commands/GenerationCommands.php:64-69, src/Drush/Commands/GenerationCommands.php:115-120, and src/Drush/Commands/GenerationCommands.php:185-189 still define acs:generate, acs:generate:more, and acs:generate:add with no --dry-run option at all, even though README.md:122 and .agents/skills/acs/SKILL.md:38 still say all state-changing commands support it. acs:generate is still the riskiest write path here because src/Drush/Commands/GenerationCommands.php:77-90 calls saveRecommendations(), which replaces the stored dataset in src/Service/RecommendationStorageService.php:152-165.

3. Clearing optional text fields is still impossible, so the CLI still falls short of true GUI/TUI parity for edits. src/Drush/Commands/CardCommands.php:44-46 and src/Drush/Commands/CardCommands.php:58-69 use empty()/!empty() for --title and --description, and src/Drush/Commands/CategoryCommands.php:193-201 does the same for --label, --instructions, and --description. In practice that means --description="" / --instructions="" are treated the same as “option omitted”, so users cannot clear those fields from Drush.

4. The E2E helper contract from feat: Drush CLI commands, AI skill files, E2E tests, and documentation for full GUI/TUI parity #18 is still not implemented. scripts/e2e/_helpers.sh:31-56 leaves assert_success() / assert_fail() checking shell exit codes only, not YAML success: true|false as the issue specified. Since these commands report most logical failures as YAML with exit 0, those helpers would still misclassify failures if anyone started using them. So the suite is greener now, but this part of the promised “match the rl/dxpr_builder pattern” is still incomplete.

[jjroelofs]

Follow-up Review — After Fix Commits (2ffed18..d01a597)

Four fix commits pushed. I've re-read every changed file against my original 29 findings. Scorecard below, then details on what's still open and new issues introduced by the fixes.

---

Scorecard

#	Finding	Status
1	Operator precedence bug (reportStatus)	FIXED — extracted to $timestamp, added (int) cast. Clean.
2	Static \Drupal:: calls	FIXED — UuidInterface injected, $this->storage used throughout. Zero \Drupal:: in GenerationCommands.
3	Missing --dry-run on generation commands	NOT FIXED
4	acs:generate overwrites curated data	NOT FIXED (reorganized, not resolved — see below)
5	--file path validation	FIXED — directory + writability check, file_put_contents return checked.
6	switchToAdmin() UID 1 / silent failure	PARTIALLY FIXED — warning now logged, but UID 1 still hardcoded.
7	Recursive regex	FIXED — replaced with strpos + Json::decode.
8	E2E tests have no fixtures	FIXED — realistic fixtures with deterministic UUIDs. Major improvement.
9	assert_success/assert_fail spec mismatch	NOT FIXED — still check exit codes, not YAML. Dead code (never called).
10	Vacuous assertion	FIXED — checks "Generation failed" now.
11	successList() missing message	FIXED
12	Can't clear fields to empty strings	NOT FIXED
13	CSV newline handling	NOT FIXED
14	Unused constructor deps	FIXED
15	acs:export bypasses envelope	NOT FIXED (not documented)
16	Missing medium aliases	ACCEPTABLE — top-level commands don't have sub-hierarchy.
17	Inconsistent Yaml::dump()	FIXED — all commands route through $this->yaml() now.
18	Runner uses 11.x-dev	FIXED — pinned to 11.1.x-dev.
19	CI missing summary step	NOT FIXED
20	SKILL.md frontmatter	FIXED
21	openai.yaml field names	FIXED
22	set -euo pipefail coupling	PARTIALLY FIXED — `
23–29	Low-severity items	Mixed: #23 formatCard FIXED, #26 grep -F FIXED, #29 copy() FIXED. Others not addressed.

Totals: 14 fixed, 2 acceptable, 2 partially fixed, 8 not fixed, plus new issues below.

---

Items that need pushback

#3 + #4 are NOT fixed — these are the biggest remaining gaps

acs:generate still has zero safety rails for the most destructive operation in the module.

The fix commit added regenerateCategory() + replaceSection() for acs:generate --category=X, which is a good refinement. But the core problems remain:

1. No --dry-run on acs:generate, acs:generate:more, or acs:generate:add. The spec is unambiguous: "All mutating commands support --dry-run." These are the commands that call external AI APIs (expensive, slow, non-deterministic) and overwrite stored data. They need --dry-run more than any other command.

2. acs:generate (no --category) still calls saveRecommendations() which replaces everything — all curated titles, edited descriptions, implementation links, everything. One invocation wipes out manual curation work with no warning.

3. regenerateCategory() calls generateRecommendations() which generates for ALL categories, then discards all but one (line 210–219). If a user runs drush acs:generate --category=content_gaps, the module makes AI API calls for every enabled category, then throws away everything except content_gaps. This wastes API quota and adds unnecessary latency.

#9 — assert_success/assert_fail are dead code that contradicts the spec

These helpers check exit codes, not YAML content. They're never called by any test. This is a spec compliance issue — the issue #18 explicitly defines these as YAML-aware assertions. Either fix them to parse YAML or remove them. Dead code that looks like it works but doesn't is worse than no code.

#12 — Can't clear fields to empty strings is still unfixed

CategoryCommands::updateCategory() lines 194–201 use !empty():

if (!empty($options['label'])) { ... }
if (!empty($options['instructions'])) { ... }

empty("") is true, so --instructions="" is silently ignored. Same for CardCommands::editCard(). Users cannot intentionally clear optional fields.

---

New issues introduced by the fix commits

NEW #1 — acs:report() envelope lacks message field (introduced alongside #11 fix)

ReportCommands::report() (line 80–91) uses $this->yaml($output, 6) directly instead of $this->success(). The response has success: true but no message key. Every other command now uses the proper envelope. This was presumably overlooked when fixing #11 for successList().

NEW #2 — regenerateCategory() makes wasteful AI calls

As noted above: $this->strategyGenerator->generateRecommendations() generates for ALL enabled categories. When called via acs:generate --category=content_gaps, all that extra work is thrown away. This should pass the category filter down to the generator, or at minimum document this as a known limitation.

NEW #3 — test-settings.sh doesn't verify persisted value

Line 32-33:

output=$($DRUSH acs:settings:get 2>&1)
assert_has "get after set shows update" "success: true" "$output"

After setting the prompt to "E2E test prompt", the verification only checks success: true — it never checks that "E2E test prompt" appears in the response. The set could silently fail and this assertion would still pass.

NEW #4 — test-generation.sh health assertion is too loose

Line 16:

assert_has "health returns provider status" "success:" "$output"

This matches both success: true and success: false. Without a configured provider, acs:health returns success: false, so this assertion passes on failure output. Should check for the specific expected outcome (e.g., "not ready" or "success: false").

NEW #5 — run-e2e-tests.sh leaks temp directory on failure

Line 131: bash "$test_file" — if a test file exits non-zero, set -e terminates the runner immediately. Line 139 rm -rf "$SITE_DIR" never executes. The temp directory (which contains a full Drupal install) is leaked on disk. Should use trap 'rm -rf "$SITE_DIR"' EXIT after line 28.

NEW #6 — No aggregate pass/fail summary across test files

Each test file resets PASS=0 FAIL=0 TOTAL=0 independently and calls print_summary at the end. run-e2e-tests.sh only reports "Completed $TESTS_RUN test files" (line 136) but never reports total assertions pass/fail across all files. If the first test file fails, set -e aborts and all subsequent test files are skipped entirely.

---

Bottom line

The critical bugs (#1, #2) are properly fixed. The E2E test suite is dramatically improved with real fixtures. But the two biggest architectural concerns — generation commands with no --dry-run and acs:generate silently destroying curated data — remain unaddressed. These are the highest-risk operations in the module (external AI API calls + data overwrite) and the spec explicitly requires --dry-run on all mutating commands. I'd want to see at least --dry-run on the generation commands before merge.

[jjroelofs]

I re-checked a3486e3 carefully. Most of the remaining review items are genuinely fixed, but I still found a few important problems that I don’t think are ready to hand-wave away:

1. The attempted field-clearing fix introduced a real data-loss regression in acs:category:update(). In src/Drush/Commands/CategoryCommands.php:175-201, the defaults for instructions / description are still empty strings, but the new checks are now !== NULL. That means a normal label-only update treats the omitted text options as explicit empty strings and clears them. I reproduced this locally on the current branch with a fresh Drupal install: create a category with --instructions='Keep me' --description='Keep desc', run drush acs:category:update repro --label='Repro Updated', then drush acs:category:get repro shows both fields blank. So this “fix” is currently destructive.

2. The acs:sitemap blocker still is not actually resolved; the test is still just weakened. scripts/e2e/test-report.sh:56-60 still runs drush acs:sitemap -l http://localhost in an environment where scripts/e2e/run-e2e-tests.sh:27-54 never starts an HTTP listener, and now it only asserts that the output is non-empty. Since src/Service/ContentAnalyzer.php:212-302 still fetches /sitemap.xml over HTTP, the suite will stay green even if acs:sitemap always returns success: false. That means the original parity/CI gap is still masked rather than fixed.

3. The new workflow summary step can hide E2E failures. In .github/workflows/review.yml:35-37, the test command is now piped through tee. I checked the latest Actions log for run 24069658670, and that step is executed with shell: /usr/bin/bash -e {0} (no pipefail). In that setup, docker compose --profile test run --rm e2e-test 2>&1 | tee /tmp/e2e-output.txt reports the exit status of tee, so a failing test run can still leave the job green.

4. acs:generate --category=... is still paying for a full-site generation and then discarding most of the result. src/Drush/Commands/GenerationCommands.php:263-275 still calls StrategyGenerator::generateRecommendations() for all enabled categories and only keeps $recommendations[$section]. So the new dry-run support is good, but the category-scoped path still wastes AI quota and latency on categories the user did not ask for.

[jjroelofs]

Third Review — After Commit a3486e3

Good progress. Many items are now properly resolved. But the commit introduced a regression in CategoryCommands, the --dry-run implementation has a fundamental design problem, and the field-clearing fix in CardCommands doesn't actually work.

---

Scorecard Update

#	Finding	Status
1	Operator precedence bug	FIXED (round 2)
2	Static \Drupal:: calls	FIXED (round 2)
3	Missing --dry-run on generation	EXISTS BUT FLAWED — see below
4	acs:generate overwrites curated data	PARTIALLY FIXED — warning in Help text only
5	--file path validation	FIXED (round 2)
6	switchToAdmin() UID 1	PARTIALLY FIXED (round 2)
7	Recursive regex	FIXED (round 2)
8	E2E fixtures	FIXED (round 2)
9	assert_success/assert_fail	FIXED but dead code (never called)
10	Vacuous assertion	FIXED (round 2)
11	successList() message	FIXED (round 2)
12	Can't clear fields to empty	NOT FIXED + REGRESSION — see below
13	CSV newline handling	FIXED
14	Unused constructor deps	FIXED (round 2)
15	acs:export bypasses envelope	FIXED (documented)
17	Yaml::dump() inconsistency	FIXED (round 2)
18	Runner 11.x-dev	FIXED (round 2)
19	CI summary step	FIXED
20	SKILL.md frontmatter	FIXED (round 2)
21	openai.yaml field names	FIXED (round 2)
NEW #1	acs:report message field	FIXED
NEW #2	Wasteful AI calls in regenerateCategory	NOT FIXED
NEW #3	test-settings.sh verify value	FIXED
NEW #4	health assertion too loose	FIXED
NEW #5	Temp dir leak	FIXED
NEW #6	Aggregate summary	PARTIALLY FIXED — file-level only; TOTAL_PASS/TOTAL_FAIL/TOTAL_TESTS on lines 122-124 of run-e2e-tests.sh are declared but never incremented (dead code)

---

Items requiring attention

#3 — --dry-run on generation commands: the flag exists but calls the AI API

This is the most significant issue in this round. All three generation commands now have --dry-run, but the implementation calls the AI provider before checking the flag:

GenerationCommands.php lines 96–122:

L97:  $recommendations = $this->strategyGenerator->generateRecommendations();
      // ... 13 lines of processing ...
L111: if ((bool) $options['dry-run']) {
        return $this->success('Dry run: ...', [...]);
      }

Same pattern in acs:generate:more (AI call at line ~189, dry-run check at ~206) and acs:generate:add / regenerateCategory().

Every --dry-run invocation burns the full AI API cost. It just doesn't save the results. Users will expect --dry-run to be a free preview, but they're paying for the full generation. This is arguably worse than not having the flag — it creates a false expectation of cheapness.

The correct fix: check --dry-run before calling the AI. For generation commands where a meaningful preview without the AI call is impossible, the dry-run should return information about what would happen (which categories would be regenerated, whether existing data would be overwritten, how many pages would be analyzed) without making the AI call.

#12 — Field clearing: the fix doesn't work AND introduces a regression

CardCommands — clearing doesn't work:

CardCommands::editCard() lines 36–45:

'title' => '',        // default
'description' => '',  // default
...
$has_title = $options['title'] !== NULL && $options['title'] !== '';

When user passes --title="", Drush delivers ''. The check '' !== '' is false, so $has_title is false. The empty string is not applied. The commit claimed this allows clearing, but the logic prevents it. This is identical behavior to the original !empty() — just written differently.

The root problem: both the "user didn't pass the option" and "user passed empty string" cases produce the same default value '', making them indistinguishable. To fix this, the default must be NULL, and the check should be $options['title'] !== NULL.

CategoryCommands — regression introduced:

CategoryCommands::updateCategory() lines 176–201:

'instructions' => '',  // default is ''
'description' => '',   // default is ''
...
if ($options['instructions'] !== NULL) {  // '' !== NULL is TRUE
    $changes['instructions'] = $options['instructions'];
}

When a user runs drush acs:category:update X --label="New" without passing --instructions or --description, the defaults are ''. Since '' !== NULL is true, both $changes['instructions'] = '' and $changes['description'] = '' are set. This silently clears instructions and description on every update call that doesn't explicitly re-supply them.

This is a data-loss regression. A simple label update would wipe out the category's AI instructions.

Fix: change defaults for instructions and description to NULL (not '').

---

New issues in this commit

NEW #7 — acs:sitemap still bypasses the response envelope

ReportCommands::sitemap() (lines 193–205) manually builds ['success' => TRUE, ...] and calls $this->yaml() directly — no message field. The acs:report method was fixed to use $this->success() in this commit, but acs:sitemap was overlooked. Minor inconsistency.

NEW #8 — run-e2e-tests.sh dead aggregate counters

Lines 122–124 declare TOTAL_PASS=0, TOTAL_FAIL=0, TOTAL_TESTS=0 but these are never incremented anywhere. Each test file's assertion counts live in subprocess scope and aren't propagated back. These three lines are dead code.

---

Summary

The big picture: 21 of the original 29 findings are now resolved or acceptable. The remaining items are:

1. Test main branch #3 — --dry-run calls the AI API — most impactful remaining issue. The flag exists but costs real money to use.
2. Return proper HTTP 500 status codes for AJAX errors #12 — CategoryCommands regression — silent field clearing on every update is a data-loss bug introduced by this commit. Needs immediate fix (change defaults from '' to NULL).
3. Return proper HTTP 500 status codes for AJAX errors #12 — CardCommands clearing doesn't work — same root cause (defaults should be NULL).
4. fix: resolve all Drupal coding standard violations and script issues #4 / NEW #1: Fix access to routes for non-admin users #2 — regenerateCategory() wasteful calls — generates all categories to use one. Acknowledged as a known limitation or worth fixing.

Items 2 is a blocker (data-loss regression). Item 1 is a strong recommendation. Items 3–4 are worth fixing but not blocking.

[jjroelofs]

Fourth Review — After Commits 7f00253 + 24723b6

All previously blocking items are now resolved. This is a thorough and responsive set of fixes.

---

Scorecard — Final

#	Finding	Status
CRITICAL
1	Operator precedence bug	FIXED (round 2)
2	Static \Drupal:: calls	FIXED (round 2)
HIGH
3	Missing --dry-run on generation	FIXED — dry-run checks happen before AI calls; zero API cost. Verified in all four paths: generate, generate:more, generate:add/generateForCategory, and regenerateCategory.
4	acs:generate overwrites curated data	ACCEPTABLE — #[CLI\Help] warning documents the behavior; --dry-run now warns "Existing recommendations will be replaced"; --category enables per-category regeneration as a safer alternative.
5	--file path validation	FIXED (round 2)
6	switchToAdmin() UID 1	PARTIALLY FIXED — warning logged on failure; UID 1 hardcoded is an acknowledged limitation.
7	Recursive regex	FIXED (round 2)
8	E2E fixtures	FIXED (round 2)
9	assert_success/assert_fail	FIXED — helpers now check YAML content. Still unused by test files (tests use assert_has pattern directly), but no longer broken.
10	Vacuous assertion	FIXED (round 2)
MEDIUM
11	successList() message	FIXED (round 2)
12	Can't clear fields / data-loss regression	FIXED — defaults changed to NULL in both CardCommands and CategoryCommands. $options['title'] !== NULL correctly distinguishes "not passed" (NULL) from "passed as empty" (''). Empty label returns explicit error (line 197-198). Instructions/description can be cleared to empty string.
13	CSV newline handling	FIXED (round 3)
14	Unused constructor deps	FIXED (round 2)
15	acs:export bypasses envelope	FIXED — documented in Help text (round 3)
17	Yaml::dump() inconsistency	FIXED (round 2)
18	Runner 11.x-dev	FIXED (round 2)
19	CI summary step	FIXED (round 3)
20	SKILL.md frontmatter	FIXED (round 2)
21	openai.yaml field names	FIXED (round 2)
NEW issues
NEW #1	acs:report message field	FIXED (round 3)
NEW #2	Wasteful AI calls in regenerateCategory	FIXED — new regenerateCategoryViaPrompt() makes a single category-specific AI call via categoryPromptBuilder.
NEW #3	test-settings.sh verify value	FIXED (round 3)
NEW #4	health assertion too loose	FIXED (round 3)
NEW #5	Temp dir leak	FIXED (round 3)
NEW #6	Aggregate summary dead code	FIXED — dead counters removed.
NEW #7	acs:sitemap bypasses envelope	FIXED — now uses $this->success('Sitemap retrieved.', $extra).
NEW #8	Dead aggregate counters	FIXED — removed.

---

Remaining observations (non-blocking)

1. regenerateCategoryViaPrompt() and generateForCategory() share significant code duplication

Both methods (lines 305-381 and 386-510) build prompt templates with identical template variable substitution blocks (~30 lines each: homepage content, menu formatting, URL formatting, strtr() with both {var} and {{ var }} syntax). This is a candidate for extraction into a shared buildPromptContext() helper. Not a bug, and can be addressed in a follow-up.

2. test-setup-ai.sh line 30 — missing || true on expected-error call

output=$($DRUSH acs:setup-ai --host=invalid 2>&1)

Not actually a crash risk since SetupCommands::setupAi() uses $this->error() (which returns a string with exit 0), but it's inconsistent with the || true pattern used for all other expected-error calls across the test suite.

3. test-report.sh line 62 — sitemap assertion checks "success:" not "success: true"

This matches both success: true and success: false. Documented as intentional (testing HTTP connectivity, not sitemap existence), which is reasonable.

4. CI summary grep pattern

The grep -E "(PASS|FAIL|Results|Running:|Completed|FAILED)" in review.yml will match every individual PASS/FAIL line, which could produce a very long summary for a full test run. Consider narrowing to just summary lines: "(Results:|Completed|FAILED)".

---

Verdict

All critical, high, and medium issues from the original review are resolved or acknowledged. The blocker regression from round 3 (CategoryCommands data-loss) is properly fixed. The --dry-run implementation is now correct — it previews without calling the AI API. The E2E test suite has real fixtures, meaningful assertions, proper cleanup, and a working CI summary.

No blocking issues remain. This is ready for merge.

[jjroelofs]

I took another careful pass over the latest head. Most of the earlier blockers do stay fixed, but I found a couple of things I don't think we should miss:

1. acs:generate --category=... now ignores the global system prompt configured via acs:settings:set.

   Full-site generation still goes through StrategyGenerator::generateRecommendations(), which reads ai_content_strategy.settings.system_prompt and applies it via setChatSystemRole() (src/Service/StrategyGenerator.php:185-188). The new category-only path in src/Drush/Commands/GenerationCommands.php:305-350 bypasses that service entirely and sends the hard-coded prompt from CategoryPromptBuilder::buildAddMorePrompts() (src/Service/CategoryPromptBuilder.php:223-301). So the same acs:generate command now behaves differently depending on whether --category is supplied. That is a real regression from the previous implementation, because category-scoped regeneration used to inherit the same global prompt customization as full generation.

2. acs:sitemap still is not covered by a true happy-path E2E.

   scripts/e2e/test-report.sh:56-64 now explicitly documents that the fixture does not provide a sitemap module and only asserts that the output contains success: and sitemap.xml. That means the test still passes on a structured failure (success: false) as long as the command reached an HTTP server. The issue calls for E2E coverage of the command set, and for acs:sitemap we still are not exercising a successful sitemap retrieval path.

3. The new generation --dry-run paths are still untested in E2E.

   GenerationCommands.php now has dedicated dry-run branches for acs:generate, acs:generate:add, acs:generate:more, and category regeneration, but scripts/e2e/test-generation.sh:18-31 still only checks the no-provider / nonexistent-resource error cases. Since these dry-run paths were added specifically to satisfy the mutating-command contract without burning AI calls, I think they should be covered before we call this comprehensive.

[jjroelofs]

I want to push back more clearly on the testing point, because this matters for merge confidence.

I don't think we should be satisfied with tests that can go green on failure output. That is exactly how false positives get baked into CI, and at that point the test is no longer protecting the behavior it claims to cover.

Two concrete examples in the current branch:

1. scripts/e2e/test-report.sh:56-64 does not assert a successful acs:sitemap run. It accepts any output containing success: and sitemap.xml, which means a structured failure (success: false) still passes. That is not a happy-path test; it is a connectivity check dressed up as feature coverage.

2. scripts/e2e/test-generation.sh:18-31 still does not exercise the new generation --dry-run success paths at all. Those branches were added specifically to satisfy the mutating-command contract safely, but the suite does not verify them.

For me, this is not just a nice-to-have cleanup. If the purpose of the E2E suite is to give us confidence that the command set works, then assertions need to fail when the feature fails. Otherwise we are paying the maintenance cost of tests without getting the safety benefit.

I would strongly prefer to see this addressed before merge:

• make acs:sitemap either run in a real success fixture and assert success: true, or stop presenting the current check as meaningful command coverage
• add E2E coverage for the new generation --dry-run branches so the latest safety fix is actually protected

Right now the test suite is still too capable of false positives in exactly the places we are relying on it for confidence.

[jjroelofs]

DRY Review — Refactoring Opportunities

Reviewed all 9 command classes for code duplication. The prompt-building logic is already cleanly extracted into callCategoryAi() + buildUserPrompt() — good work. Two remaining patterns are worth extracting:

---

1. Card + Idea lookup guard in IdeaCommands — 3 identical blocks

IdeaCommands.php has this exact 8-line block in editIdea (lines 45-53), implementIdea (lines 101-109), and deleteIdea (lines 154-162):

$card = $this->storage->getCardByUuid($section, $uuid);
if (!$card) {
    return $this->notFound('Card', $uuid, 'acs:report');
}
$idea_index = $this->storage->findIdeaIndexByUuid($section, $uuid, $idea_uuid);
if ($idea_index === NULL) {
    return $this->notFound('Idea', $idea_uuid, 'acs:report:card');
}

Suggestion: Extract to a base class helper like resolveIdea(string $section, string $uuid, string $idea_uuid): array|string that returns ['card' => $card, 'index' => $idea_index] on success or a YAML error string on failure. Each call site becomes:

$result = $this->resolveIdea($section, $uuid, $idea_uuid);
if (is_string($result)) { return $result; }
[$card, $idea_index] = [$result['card'], $result['index']];

Impact: Eliminates 16 duplicated lines across 3 methods in one file.

2. "Load enabled categories sorted by weight" — 3 occurrences across 3 files

ReportCommands.php (lines 47-50), ExportCommands.php (lines 47-50), and GenerationCommands.php (lines 92-96) all repeat:

$category_storage = $this->entityTypeManager->getStorage('recommendation_category');
$categories = $category_storage->loadByProperties(['status' => TRUE]);
uasort($categories, static fn(RecommendationCategory $a, RecommendationCategory $b): int =>
    $a->getWeight() <=> $b->getWeight());

Suggestion: Move to RecommendationStorageService::loadEnabledCategories(): array (it already holds the storage reference and is injected everywhere). The sort-by-weight logic is easy to forget or get inconsistent across callers.

Impact: Eliminates ~12 duplicated lines across 3 files, centralizes the sort order.

---

These are the only two patterns substantial enough to warrant extraction. The remaining duplication (category instanceof guards, card lookup guards, dry-run preambles) are standard 4-5 line guard clauses that are idiomatic in Drupal command classes — extracting them would add indirection without meaningful readability gain.

[jjroelofs]

I re-reviewed the latest commit carefully against the remaining review thread, and this round does address the outstanding concerns satisfactorily.

What I re-checked:

• the category-specific generation path now includes the global system_prompt again via GenerationCommands::callCategoryAi(), so acs:generate --category=... no longer diverges from the configured prompt behavior
• acs:sitemap now has a real happy-path E2E fixture: the runner creates a static sitemap.xml, and test-report.sh now asserts success: true, total_urls: 5, and actual URL content instead of accepting structured failure output
• the new generation --dry-run branches are now covered in E2E (acs:generate, acs:generate --category, acs:generate:add, and acs:generate:more)
• the helper extraction in GenerationCommands looks clean and did not introduce any obvious regressions

I also ran the targeted report and generation E2E slices locally on the current branch, and both passed with the new assertions.

At this point I don’t have further blockers from my side. The latest commit closes the loop on the false-positive testing concern and the system-prompt regression.

[jjroelofs]

Fifth Review — After Commits 5c9e5db + 2a21dee

Both commits are clean. All outstanding items resolved.

DRY Refactoring (2a21dee) — Verified

resolveIdea() extraction in IdeaCommands — Correctly implemented. The helper (lines 34-46) returns array|string, and all three callers (editIdea line 71, implementIdea line 122, deleteIdea line 170) use the is_string($resolved) guard pattern. deleteIdea correctly accesses $resolved['card'] and $resolved['idea_index'] for the confirmation text. Clean.

loadEnabledCategories() in RecommendationStorageService — Correctly implemented (lines 87-94). Proper PHPDoc return type, weight sorting centralized. All three callers updated: ExportCommands line 43, ReportCommands line 45, GenerationCommands line 94. ExportCommands correctly dropped its EntityTypeManagerInterface dependency (no longer needed), and drush.services.yml matches (line 42: only @ai_content_strategy.recommendation_storage).

Additional Fixes (5c9e5db) — Verified

System prompt in callCategoryAi() — ConfigFactoryInterface re-injected (constructor line 36, drush.services.yml line 57). The callCategoryAi() method reads the global system prompt and prepends it (lines 410-416), matching StrategyGenerator behavior.

Sitemap happy-path — run-e2e-tests.sh creates a static sitemap.xml with 5 URLs (lines 58-68). test-report.sh now asserts success: true, total_urls: 5, content_types, and URL content (lines 59-63). A proper happy-path test instead of the previous "check it doesn't crash" test.

Dry-run E2E coverage — test-generation.sh now tests all four dry-run paths (lines 20-50): acs:generate --dry-run, --category --dry-run, generate:add --dry-run, generate:more --dry-run. Each asserts success: true, dry_run: true via assert_dry_run, and path-specific fields. Good coverage.

test-setup-ai.sh — || true added to the --host=invalid call (line 30). Consistent with the rest of the test suite.

CI summary grep — Narrowed to "(Results:|Running:|Completed|FAILED:)" (line 45). No more PASS/FAIL line spam.

No new issues found.

All feedback from all five review rounds is addressed. Nothing to push back on.