quality-debt: .agents/scripts/milestone-validation-worker.sh — PR #2519 review feedback (critical)

[marcusquinn]

Unactioned Review Feedback

Source PR: #2519
File: .agents/scripts/milestone-validation-worker.sh
Reviewers: coderabbit
Findings: 5
Max severity: critical

---

MEDIUM: coderabbit (coderabbitai[bot])

File: .agents/scripts/milestone-validation-worker.sh:27
⚠️ Potential issue | 🟠 Major

Add a machine-consumable report mode for orchestrator integration.

Output is currently human-readable only. A --json mode is needed for deterministic upstream parsing of check counts, failures, warnings, and exit status.

Also applies to: 947-993

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.agents/scripts/milestone-validation-worker.sh around lines 17 - 27, Add a
--json CLI flag to .agents/scripts/milestone-validation-worker.sh and wire it
into the option parsing so the script sets a boolean (e.g., json_output)
alongside existing flags; when json_output is true, suppress human-readable logs
and emit a single machine-readable JSON object to stdout at the end of the run
summarizing total_checks, failures_count, warnings_count, failures (array of
failure objects with id/message/context), warnings (array), and final exit_code;
ensure all places that currently print or exit on error (the main validation
loop / result aggregation code referenced in the script’s main logic and
finalization block around the current human report output and the region
currently repeated at 947-993) accumulate structured data into those arrays and
counts so the JSON can be emitted deterministically, and keep existing exit
status behavior unchanged (script should still exit non-zero on failures) while
returning the same numeric exit code in the JSON exit_code field.

View comment

CRITICAL: coderabbit (coderabbitai[bot])

File: .agents/scripts/milestone-validation-worker.sh:247
⚠️ Potential issue | 🔴 Critical

Reject --max-retries 0 to prevent false PASS with zero checks.

0 is currently accepted, which skips the loop entirely and can return success without running any validation.

Suggested fix

-      if ! echo "$2" | grep -qE '^[0-9]+$'; then
-        log_error "--max-retries requires a numeric value, got: $2"
+      if ! echo "$2" | grep -qE '^[1-9][0-9]*$'; then
+        log_error "--max-retries requires a positive integer (>=1), got: $2"
         return 2
       fi

Also applies to: 1055-1079

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.agents/scripts/milestone-validation-worker.sh around lines 240 - 247, The
--max-retries handler currently accepts 0 which causes the validation loop to be
skipped; update the parsing logic around the --max-retries case to reject
non-positive values by checking that "$2" is a positive integer (>0) before
assigning MAX_RETRIES, returning an error (and calling log_error) if the value
is 0 or negative; update the same validation in the duplicate block referenced
(the similar handler at lines 1055-1079) so both places enforce a strictly
positive integer for MAX_RETRIES and keep using require_value "$arg" "${2-}" to
ensure a value exists.

View comment

HIGH: coderabbit (coderabbitai[bot])

File: .agents/scripts/milestone-validation-worker.sh:598
⚠️ Potential issue | 🟠 Major

ShellCheck scope misses nested scripts under .agents/scripts/**.

find ... -maxdepth 1 only validates top-level scripts and misses subdirectories.

Suggested fix

-sc_output=$(find "$repo_path/.agents/scripts" -maxdepth 1 -name "*.sh" -exec shellcheck {} + 2>&1) || sc_exit=$?
+sc_output=$(find "$repo_path/.agents/scripts" -type f -name "*.sh" -exec shellcheck {} + 2>&1) || sc_exit=$?

As per coding guidelines ".agents/scripts/**/*.sh: Run ShellCheck on all shell scripts before committing".

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

	if [[ -d "$repo_path/.agents/scripts" ]]; then
		# Run ShellCheck on scripts if available
		if command -v shellcheck >/dev/null 2>&1; then
			local sc_output
			local sc_exit=0
			sc_output=$(find "$repo_path/.agents/scripts" -type f -name "*.sh" -exec shellcheck {} + 2>&1) || sc_exit=$?

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.agents/scripts/milestone-validation-worker.sh around lines 592 - 598, The
ShellCheck invocation currently limits scanning to top-level files by using find
with -maxdepth 1; update the find command used around repo_path/.agents/scripts
(the block that sets sc_output and sc_exit and calls shellcheck) to recurse into
subdirectories (e.g., remove -maxdepth 1 and ensure you still pass all *.sh
files to shellcheck and capture stderr into sc_output while preserving sc_exit
handling) so that all .agents/scripts/**/*.sh files are linted by shellcheck.

View comment

MEDIUM: coderabbit (coderabbitai[bot])

File: .agents/scripts/milestone-validation-worker.sh:711
⚠️ Potential issue | 🟠 Major

Lint/type-check is implemented as blocking, not advisory.

These paths call record_fail, which flips milestone result to failed. That conflicts with the documented behavior where lint/type-check should be advisory.

Suggested fix

-        record_fail "Linter ($pkg_cmd run lint)" "$issue_count issues found"
+        record_warning "Linter ($pkg_cmd run lint)" "$issue_count issues found"

-        record_fail "TypeScript type check" "$error_count type errors"
+        record_warning "TypeScript type check" "$error_count type errors"

-        record_fail "Linter (ruff)" "$issue_count issues"
+        record_warning "Linter (ruff)" "$issue_count issues"

Also applies to: 726-729, 746-749

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.agents/scripts/milestone-validation-worker.sh around lines 705 - 711, The
lint/type-check blocks currently call record_fail (using variables lint_exit,
lint_output and invoking "$pkg_cmd run lint"), which makes the milestone fail;
change these to be advisory by calling record_pass instead (e.g., replace
record_fail "Linter ($pkg_cmd run lint)" "$issue_count issues found" with
record_pass "Linter ($pkg_cmd run lint)" "$issue_count issues found"), keeping
the existing issue_count computation from lint_output so the message shows how
many issues were found; apply the same replacement for the other lint/type-check
occurrences (the blocks around lines shown) so lint/type-check no longer flips
the milestone to failed.

View comment

CRITICAL: coderabbit (coderabbitai[bot])

File: .agents/scripts/milestone-validation-worker.sh:844
⚠️ Potential issue | 🔴 Critical

Avoid early process termination on dependency install failure.

check_dependencies returns 1, and with set -e that can abort before report generation, mission status update, and fix-task creation.

Suggested fix

-      record_fail "Dependency installation" "$pkg_cmd install failed with exit code $install_exit"
-      return 1
+      record_fail "Dependency installation" "$pkg_cmd install failed with exit code $install_exit"
+      return 0

Also applies to: 1064-1064

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.agents/scripts/milestone-validation-worker.sh around lines 841 - 844, The
check_dependencies block currently calls record_fail and then returns 1 (e.g.,
in the install error path shown), which under set -e can abort the whole script;
instead, change the failure handling inside the check_dependencies function (and
the similar location around the second occurrence) to not return a non-zero
status immediately: capture the install_exit, call record_fail "Dependency
installation" with the error message, set or append to a persistent failure
flag/array (e.g., dependency_failure=1 or failures+=("dependency")), and then
continue so the function returns 0; update any callers to inspect that flag
after check_dependencies completes and handle reporting/mission status/fix-task
creation accordingly. Ensure you update both places referenced (the shown
install_exit path and the other occurrence) and keep record_fail usage
unchanged.

View comment

---

Auto-generated by quality-feedback-helper.sh scan-merged. Review each finding and either fix the code or dismiss with a reason.

[johnwaldo]

Triaged: 5 findings on milestone-validation-worker.sh. Feature requests (--json mode, better error handling). Recommend addressing in a feature improvement PR.

[github-actions]

Completed via PR #4631. merged to main.

[marcusquinn]

Completed via PR #4631.

• Model: sonnet (anthropic/claude-sonnet-4-6)
• Attempts: 1
• Duration: ~5 minutes