Add --validators flag passthrough to plcc-check.sh

[fgiudici]

Accept the --validators argument in the plcc-check script and forward it to the plcc2fbc binary.
Drop the --fbc mode as the way to drop PLCC checks is to use --validators none now.

Fixes #57

[qodo-for-releng]

PR Summary by Qodo

Pass --validators through plcc-check.sh and fix --fbc default

✨ Enhancement 🐞 Bug fix 🕐 10-20 Minutes

AI Description

• Add a --validators  option to plcc-check.sh and forward it to plcc2fbc.
• Fix --fbc mode to use --validators none instead of an empty string.
• Update help text and examples to document the new flag.

Diagram

graph TD
  U["User / CI"] --> S["scripts/plcc-check.sh"] --> M{"Mode flag"}
  M -->|"--plcc"| A1["plcc2fbc args (+dump-plcc)"] --> P["plcc2fbc"] --> O1["validation.jsonl / slog.json"]
  M -->|"--fbc"| A2["plcc2fbc args (validators none)"] --> P --> O2["FBC outputs"]
  M -->|"default"| A3["plcc2fbc args (+validators passthrough)"] --> P --> O1

Loading

High-Level Assessment

The approach is appropriate: extend the script’s argument parsing, append --validators only when explicitly provided, and set a correct default for conversion-only mode (none). Alternatives like switching to getopts/argparse-style parsing would be disproportionate for this small script and wouldn’t materially improve correctness here.

Files changed (1) +10 / -1

Enhancement (1) +10 / -1

plcc-check.shAdd '--validators' passthrough and correct '--fbc' validator default +10/-1

Add '--validators' passthrough and correct '--fbc' validator default

• Extends the CLI help and option parsing to accept '--validators <v>' and forward it to 'plcc2fbc'. Adjusts '--fbc' mode to default to '--validators none' when no validators were explicitly requested, avoiding an empty-string argument.

scripts/plcc-check.sh

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 1:10 PM UTC · Completed 1:23 PM UTC
Commit: 432058a · View workflow run →

[qodo-for-releng]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

1. --validators missing value ✓ Resolved 🐞 Bug ☼ Reliability

Description

In plcc-check.sh, --validators unconditionally reads $2 while set -u is enabled, so calling
the script with --validators but no value terminates with an unbound variable error instead of
showing usage.

Code

scripts/plcc-check.sh[56]

+        --validators) validators="$2"; shift 2 ;;

Relevance

●●● Strong

Team has accepted multiple plcc-check.sh robustness fixes under set -u; missing-arg guard fits
pattern.

PR-#36

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The script enables nounset mode and the new parsing branch dereferences $2 without checking it
exists, which causes an immediate abort when --validators is provided without a value.

scripts/plcc-check.sh[16-60]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`scripts/plcc-check.sh` runs with `set -euo pipefail` and currently parses `--validators` by unconditionally expanding `$2`. When the user invokes `./plcc-check.sh --validators` (or `--validators` is last), `$2` is unset and bash exits immediately due to `set -u`, without printing a helpful usage message.

### Issue Context
This is new behavior introduced by the added `--validators` option.

### Fix Focus Areas
- scripts/plcc-check.sh[51-60]

### Suggested fix
- In the `--validators)` case, guard that an argument exists before reading `$2`.
 - Example pattern:
   - `if [[ $# -lt 2 ]]; then echo "Error: --validators requires a value" >&2; usage >&2; exit 1; fi`
   - then `validators="$2"; shift 2`
- (Optional) also reject values that look like another flag (e.g. start with `-`) if that cannot be a valid validator value in your CLI contract.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. --fbc validators precedence mismatch ⊘ Outdated 🐞 Bug ≡ Correctness

Description

--fbc is documented as skipping PLCC validation, but if the user supplies --validators <list>
the script forwards that list and does not force --validators none, so PLCC validation will run in
--fbc mode.

Code

scripts/plcc-check.sh[R110-119]

+if [[ -n "$validators" ]]; then
+    plcc2fbc_args+=(--validators "$validators")
+fi
if $validate_only; then
    plcc2fbc_args+=(--dump-plcc)
    echo "Running plcc2fbc with ${#operators[@]} operators (PLCC validation only)..."
elif $convert_only; then
-    plcc2fbc_args+=(--validators "")
+    if [[ -z "$validators" ]]; then
+        plcc2fbc_args+=(--validators none)
+    fi

Relevance

●●● Strong

Matches PR’s stated intent to make --fbc skip validation; similar script correctness fixes were
accepted before.

PR-#36

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The script’s help text promises --fbc skips PLCC validation, but the new argument-building logic
forwards user-specified validators and only adds none when validators is empty. In plcc2fbc, any
non-none validator selection (e.g. syntax) is resolved to validator functions and used during
loadAndValidate(), so validation will execute when forwarded.

scripts/plcc-check.sh[31-36]
scripts/plcc-check.sh[109-121]
cmd/plcc2fbc/main.go[262-273]
pkg/plcc/validation.go[183-210]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`scripts/plcc-check.sh` documents `--fbc` as “skip PLCC validation”, but the current argument construction allows `--fbc --validators syntax` (or any non-`none` list) to run PLCC validators because the script appends user-provided `--validators` before the `--fbc` branch and only appends `--validators none` when no user validators were provided.

### Issue Context
In `plcc2fbc`, non-`none` validator selections are resolved into actual validator functions and executed during `loadAndValidate()`. `none` is the special value that skips all validation.

### Fix Focus Areas
- scripts/plcc-check.sh[109-123]

### Suggested fix (choose one policy)
1. **Preserve current `--fbc` contract (recommended):**
  - If `--fbc` is set, always force `--validators none` regardless of `--validators` input (and ideally reject the combination with a clear error like `--fbc and --validators are incompatible`).
  - Implement by either:
    - building `plcc2fbc_args` after deciding mode, or
    - in the `--fbc` branch, error out when `validators` is non-empty.

2. **Allow override but fix docs:**
  - Update the `--fbc` help text to state that `--validators` overrides the default skip-validation behavior.

Make the behavior and documentation consistent.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Context used

✅ Compliance rules (platform): 29 rules

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}

[fullsend-ai-review]

Review

Findings

Medium

• [protected-path] scripts/plcc-check.sh — This PR modifies scripts/plcc-check.sh, which is under the protected scripts/ path. The PR is linked to issue Add support to the --validators option in the plcc-check script #57 and provides clear rationale for the change. Human approval is always required for protected-path changes, regardless of context.

Low

• [documentation-clarity] scripts/plcc-check.sh:33 — Help text for --plcc flag says "Validate PLCC data only (skip FBC generation)" but does not mention it can be combined with --validators to control which validators run. The two flags are orthogonal (--plcc controls output mode, --validators controls validation scope), and users may not realize they can use both together.
  Remediation: Update the --plcc help text to clarify it can be combined with --validators, e.g., "Validate PLCC data only (skip FBC generation); combine with --validators to control which validators run."

Previous run

Review

Findings

Medium

• [protected-path] scripts/plcc-check.sh — This PR modifies a file under the protected scripts/ path. The PR links to issue Add support to the --validators option in the plcc-check script #57 and provides clear rationale for the change (adding --validators flag passthrough and removing the deprecated --fbc flag). Human approval is always required for protected-path changes, regardless of context.

Previous run (2)

Review

Findings

Medium

• [protected-path] scripts/plcc-check.sh — This PR modifies a file under the protected scripts/ path. The PR description explains the change rationale (adding --validators flag passthrough, replacing --fbc), but human approval is always required for protected-path changes regardless of context.

• [scope-authorization-mismatch] scripts/plcc-check.sh — PR body states "Fixes feature: adopt retry-go for HTTP retry logic #51" but issue feature: adopt retry-go for HTTP retry logic #51 ("feature: adopt retry-go for HTTP retry logic") is about replacing the manual sleep-based retry loop in FetchFrom() with retry-go — completely unrelated to adding --validators flag passthrough to the wrapper script. The incorrect reference would auto-close issue feature: adopt retry-go for HTTP retry logic #51 when this PR merges. Consider updating the PR to reference the correct issue or creating a new one.

Low

• [error-handling-consistency] scripts/plcc-check.sh:54 — The new --validators handler includes explicit argument validation ($# -lt 2 check), but the existing -o handler does not perform the same guard. This creates inconsistent error-handling patterns within the argument parser. Consider adding the same check to -o, or removing it from --validators for consistency. (Pre-existing gap, not introduced by this PR.)

• [scope-documentation-gap] scripts/plcc-check.sh:34 — The usage text describes --validators as "passed through to plcc2fbc" but does not clarify the semantic difference from --plcc mode. Users unfamiliar with the tool may not realize --plcc skips FBC generation entirely while --validators controls which PLCC validation groups run during normal operation.

Previous run (3)

Review

Findings

High

• [protected-path] scripts/plcc-check.sh — This PR modifies scripts/plcc-check.sh, which is under the protected scripts/ path. The PR has no linked issue providing authorization for changes to governance/infrastructure files. Human approval is required for all protected-path changes.
  Remediation: Link to an issue that authorizes modifications to scripts/plcc-check.sh, or obtain explicit human reviewer approval for this change.

Low

• [edge-case] scripts/plcc-check.sh:59 — The --validators flag parsing uses shift 2 without guarding against a missing argument. If invoked as ./plcc-check.sh --validators with no following value, shift 2 may fail under set -e with an unclear error. This matches the pre-existing pattern for -o (line 50), so it is consistent with current style.

• [documentation-completeness] scripts/plcc-check.sh:42 — The new usage example demonstrates --validators standalone but does not show how it interacts with --plcc or --fbc modes.

---

Labels: PR adds new CLI flag passthrough capability to batch runner script

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 2:11 PM UTC · Completed 2:25 PM UTC
Commit: 0bc287c · View workflow run →

Superseded by updated review

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 3:30 PM UTC · Completed 3:43 PM UTC
Commit: 8c1bc19 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 4:05 PM UTC · Completed 4:18 PM UTC
Commit: 3fc98f4 · View workflow run →

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

[low] documentation-clarity

Help text for --plcc flag says 'Validate PLCC data only (skip FBC generation)' but does not mention it can be combined with --validators to control which validators run. The two flags are orthogonal (--plcc controls output mode, --validators controls validation scope), and users may not realize they can use both together.

Suggested fix: Update the --plcc help text to clarify it can be combined with --validators, e.g., 'Validate PLCC data only (skip FBC generation); combine with --validators to control which validators run.'