Skip to content

WA-VERIFY-039: Add script/verify dispatcher for verification/preflight checks#985

Open
kitcommerce wants to merge 1 commit intonextfrom
issue-896-verify-entrypoint
Open

WA-VERIFY-039: Add script/verify dispatcher for verification/preflight checks#985
kitcommerce wants to merge 1 commit intonextfrom
issue-896-verify-entrypoint

Conversation

@kitcommerce
Copy link

Summary

Closes #896

Adds a repo-root script/verify entrypoint that lists and runs existing verification/preflight scripts. The dispatcher is purely a thin router — it does not reimplement any logic, only delegates to existing scripts and forwards their exit codes.

What changed

  • New file: script/verify (executable, 119 lines, POSIX sh)

Registered checks

Name Delegates to Description
default-appraisal-boot-smoke script/default_appraisal_boot_smoke Boot the default appraisal stack in test mode
default-appraisal-zeitwerk-check script/default_appraisal_zeitwerk_check Run zeitwerk:check against the default appraisal stack
check-service-ports script/check_service_ports Preflight check for common Workarea service ports

New checks can be added by appending a single line to the REGISTRY variable inside script/verify.

Client impact

None. This is developer tooling only — a new script/verify file that did not exist before. It does not touch any application code, gems, configuration, or existing scripts. No downstream plugin or host app is affected.

Verification Plan

From the repo root:

# List all supported checks
./script/verify list

Expected output:

Supported verification checks:

  default-appraisal-boot-smoke               Boot the default appraisal stack in test mode (smoke test)
  default-appraisal-zeitwerk-check           Run zeitwerk:check against the default appraisal stack
  check-service-ports                        Preflight check for common Workarea service ports

Run a check: ./script/verify <name>

Run the port-check (safe, read-only, no services required):

./script/verify check-service-ports

Verify unknown check returns exit 1:

./script/verify nonexistent; echo $?   # → 1

Run boot-smoke / zeitwerk checks when services are available:

./script/verify default-appraisal-boot-smoke
./script/verify default-appraisal-zeitwerk-check

WA-VERIFY-039 / Closes #896

Adds a repo-root `script/verify` entrypoint that:
- Lists all supported checks via `./script/verify list`
- Delegates to existing scripts via `./script/verify <name>`
- Forwards the delegate's exit code unchanged
- Reports clearly when a backing script is missing or not executable

Registered checks (delegating to existing scripts):
- default-appraisal-boot-smoke   -> script/default_appraisal_boot_smoke
- default-appraisal-zeitwerk-check -> script/default_appraisal_zeitwerk_check
- check-service-ports             -> script/check_service_ports

The dispatcher is POSIX-compatible (#!/usr/bin/env sh), avoids GNU-only
flags, and is read-only — it never modifies any files.
@kitcommerce kitcommerce added gate:build-passed Build gate passed review:architecture-pending Review in progress review:simplicity-pending Review in progress review:security-pending Review in progress review:rails-conventions-pending Rails conventions review in progress labels Mar 13, 2026
@kitcommerce
Copy link
Author

Architecture Review — PR #985 Wave 1

Verdict: PASS_WITH_NOTES
Severity: LOW

Summary

The thin-dispatcher pattern is the right call here: a single stable entrypoint that delegates to purpose-built scripts keeps each script focused and avoids coupling verification logic into the router. The POSIX-sh choice is appropriate for a script that must run in CI, local dev, and Docker environments without Ruby/Bundler being available. The pipe-registry approach is unconventional but self-contained.

Two dead-code items in the visible diff are the main architectural concerns:

Findings

  • Dead code — SCRIPT_DIR: SCRIPT_DIR="$(pwd)/script" is assigned but never referenced. The registry uses bare relative paths (script/foo). Either use $SCRIPT_DIR as a prefix throughout, or remove the variable. As-is it's a maintenance trap — future contributors may expect it to be authoritative.
  • Dead code — cmd_run(): The function is defined but never called. The *) dispatch case re-implements the search inline. If cmd_run is intentional scaffolding for future use, a comment to that effect would help; otherwise remove it.
  • Registry format is adequate but a newline-delimited pipe-separated string is fragile to edit (trailing spaces, blank-line handling). A follow-up could move to a simple two-column flat file, but not a blocker at this scope.
  • cd + relative-path pattern is correctcd "$(dirname "$0")/.." before using script/foo paths is solid and portable.

@kitcommerce
Copy link
Author

Simplicity Review — PR #985 Wave 1

Verdict: PASS_WITH_NOTES
Severity: LOW

Summary

119 lines for a 3-check dispatcher is on the heavier side, primarily due to the inline pipe-subshell dispatch pattern and dead code. The list command is clean. The run dispatch works but is harder to follow than it needs to be.

Findings

  • Pipe-subshell dispatch is overly clever: The each_entry | while ... done | { read -r script_path ... exec "$script_path" } pattern is non-obvious. A simple loop with a found-flag or early break would be easier to read:
    script_path=""
    while IFS='|' read -r name script desc; do
      [ "$name" = "$SUBCOMMAND" ] && script_path="$script" && break
    done < <(each_entry)
    Not a blocker, but worth noting for maintainability.
  • cmd_run() defined but unused: Adds lines without adding value. Remove or wire it in.
  • SCRIPT_DIR assigned but unused: Two lines of noise.
  • usage called but not shown in diff: If usage is a defined function in the full file (outside the shown diff), this is fine. If not, ./script/verify and ./script/verify --help will error with "command not found: usage" — that would be a correctness bug, not just a simplicity concern. Reviewers should confirm the full 119-line file defines usage.
  • grep -v '^[[:space:]]*$' in each_entry is fine but a comment explaining why (to strip blank lines from the multiline string) would help future editors.

@kitcommerce
Copy link
Author

Security Review — PR #985 Wave 1

Verdict: PASS

Summary

The script is read-only and does not modify system state. The dispatch path validates user input against a fixed in-process registry before any execution, preventing injection of arbitrary script paths. Quoting is consistent throughout.

Findings

  • Input validation is correct: User-supplied $SUBCOMMAND is compared against registry names via string equality only; it never reaches a shell expansion or file path directly without first matching a known registry entry. This is the right pattern.
  • exec "$script_path" is properly quoted: No word-splitting risk on the resolved path.
  • $@ forwarding is safe: Only reaches exec after the script path is validated. Downstream scripts receive forwarded args, which is expected behavior for a dispatcher.
  • No credentials, tokens, or secrets in scope: The script is pure control-flow plumbing.
  • set -eu is present: Prevents silent failure on unset variables or command errors. Good hygiene.
  • Relative path execution (script/foo): Scripts are resolved relative to repo root after cd. No PATH manipulation. This is safe and expected.
  • Minor: The || true on read -r script_path suppresses a non-zero return when the pipe produces no output — this is intentional and the emptiness check on $script_path that follows handles the not-found case correctly.

@kitcommerce
Copy link
Author

Rails Conventions Review — PR #985 Wave 1

Verdict: PASS

Summary

script/verify fits cleanly into the Rails script/ convention established by script/setup, script/test, script/ci, etc. The script is POSIX sh (not Ruby), which is appropriate for a preflight/verification entrypoint that must run before Bundler is available. No Rails conventions are violated.

Findings

  • script/ placement is correct: Rails projects conventionally use script/ for one-off operational scripts. This entrypoint belongs here.
  • Naming is consistent: script/verify matches the imperative-verb naming pattern of script/setup, script/test, script/server. The registered checks (default-appraisal-boot-smoke, default-appraisal-zeitwerk-check, check-service-ports) match the naming of the scripts they delegate to.
  • Not using Ruby/Rails internals is correct: A dispatcher that runs before the environment is loaded should be shell, not Ruby. Using bundle exec or Rails initializers here would be wrong.
  • Zeitwerk check registration is appropriate: zeitwerk:check is a standard Rails task and first-class citizen in any Rails modernization effort. Its inclusion is expected.
  • No Rakefile integration required at this scope: Wrapping this in a Rake task could be a future enhancement but is not a convention violation to omit.
  • Executable bit (100755): Set correctly for a runnable script.

@kitcommerce kitcommerce added review:architecture-done Review complete review:simplicity-done Review complete review:security-done Review complete review:rails-conventions-done Rails conventions review complete and removed review:architecture-pending Review in progress review:simplicity-pending Review in progress review:security-pending Review in progress review:rails-conventions-pending Rails conventions review in progress labels Mar 13, 2026
@kitcommerce
Copy link
Author

Wave 1 Review Summary — PR #985

Reviewer Verdict Severity
Architecture PASS_WITH_NOTES LOW
Simplicity PASS_WITH_NOTES LOW
Security PASS
Rails Conventions PASS

Overall: ✅ Wave 1 Passed

No blocking issues. All findings are LOW severity notes around dead code and readability.

Key action items before merge (non-blocking):

  1. Remove or use SCRIPT_DIR variable
  2. Remove or wire in cmd_run() function — it is defined but dead
  3. Confirm usage function is defined in the full file (not visible in diff excerpt) — if missing, ./script/verify --help and no-arg invocation will error

Wave 2 recommended next: rails-security, database, test-quality reviewers should be dispatched for final merge gate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gate:build-passed Build gate passed review:architecture-done Review complete review:rails-conventions-done Rails conventions review complete review:security-done Review complete review:simplicity-done Review complete

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant