feat(completeness): add --incomplete-policy to control exit behavior on incomplete scan data - #1019
Conversation
…on incomplete scan data Add `--incomplete-policy warn|error` (default: warn). When set to error, EXIT_ERROR (3) is returned on detection-impact incomplete scan data. Remediation gaps and ratchet behavior are unaffected. Closes OWASP#1018
- Arg parsing: default warn, parse warn/error, =form, invalid throws - Single-folder: detection+warn, detection+error, remediation+error, ratchet+warn - Multi-folder: detection+warn, detection+error, remediation+error, multi-folder detection, ratchet+warn - Fix action.yml: pass incomplete-policy through Apply security fixes step - Docs: add --incomplete-policy to CLI reference and workflow integration
|
Addressed all three review items: P1 — Behavioral tests (+16 tests, 1446 → 1462):
P2 — Action fix step: Added P3 — Docs:
|
|
Hey @luojiyin1987, thanks for this - the completeness work you've done has been great. There's an existing PR (#941) for the same feature that has priority since it came first. I've asked that contributor to address the outstanding changes within the next 48 hours. If they don't respond we'll close #941 and come back to yours. Will keep you posted! |
|
No worries at all — I'm not in a rush. Happy to wait and see how #941 goes. Thanks for keeping me posted! @sonukapoor |
sonukapoor
left a comment
There was a problem hiding this comment.
Nice work on this one. I went through all the changed files and the logic is correct end-to-end.
A few things I verified explicitly: shouldFailForIncompleteScan() returns false immediately for anything that isn't "error", and it correctly delegates to getCompletenessImpact().hasDetectionGap - so remediation gaps are genuinely unaffected regardless of the policy setting. The ratchet path in index.ts exits well before the shouldFailForIncompleteScan check, so those two code paths are truly independent. Multi-folder aggregation via aggregateMultiFolderCompleteness() correctly sums detection diagnostics across all folders before the policy check fires, so a detection gap in any one subfolder propagates to the exit code.
Test coverage is solid - you hit all the meaningful combinations: detection gap + warn (exit 0), detection gap + error (exit 3), remediation gap + error (still exit 0), multi-folder detection gap + error (exit 3), and ratchet + detection gap + warn (exit 3 regardless of policy). Left a couple of small notes inline - neither is a blocker, just things worth a quick look.
| const exitCode = incompleteFailure | ||
| ? EXIT_ERROR | ||
| : shouldFail && !options.fix | ||
| ? 1 |
There was a problem hiding this comment.
The new incompleteFailure ? EXIT_ERROR arm uses the named constant, which is great. The fallback still has literal 1 and 0 (pre-existing). Since you are already touching this expression, this is a natural moment to swap those to EXIT_FINDINGS : EXIT_OK for consistency - but totally fine to leave out of scope if you would rather keep the diff tight.
There was a problem hiding this comment.
Good point. I think I'd prefer to keep the pre-existing 1 / 0 unchanged here and keep this PR focused on the incomplete-scan policy. We can normalize the remaining exit-code literals separately if needed.
| fi | ||
|
|
||
| if [[ -n "${INPUT_INCOMPLETE_POLICY}" ]]; then | ||
| args+=("--incomplete-policy" "${INPUT_INCOMPLETE_POLICY}") |
There was a problem hiding this comment.
Just wanted to confirm this is intentional: if incomplete-policy: error is set and the scan step exits 3, the fix step (no explicit success() condition) still runs, hits the same detection gap, and also exits 3. The job still fails either way, so it is not wrong - just produces a second failure message. If that is the intended behavior, all good.
There was a problem hiding this comment.
Thanks for checking this. My understanding is that the fix step should not run in that case.
GitHub Actions implicitly applies success() to an if condition unless the expression includes a status-check function. So:
if: ${{ inputs.fix == 'true' }}
is effectively gated by the success of the preceding steps. If the scan exits 3 for incomplete-policy: error, Apply security fixes should therefore be skipped rather than producing a second failure.
So I think the current behavior is the intended one, without needing an additional explicit success() condition.
|
Merged - thank you @luojiyin1987! The --incomplete-policy work has been great throughout the whole completeness series. |
Summary
Add
--incomplete-policy warn|error(default:warn) to control exit code when scan detection data is incomplete.Behavior
--fail-onerroronly targets detection-impact incomplete data. Remediation gaps are never affected by this flag.Changes
src/types.ts—IncompletePolicytype, field onParsedOptionssrc/cli/args.ts— defaultwarn, parse--incomplete-policy warn|errorsrc/cli/help.ts— help text + examplesrc/scan/completeness.ts—shouldFailForIncompleteScan()helpersrc/index.ts— single-folder exit codesrc/scan/multi-folder-scan.ts— multi-folder exit code, shared aggregated completenessaction.yml—incomplete-policyinput (empty default for old CLI compat)tests/helpers.test.ts— updated expectationsDesign decisions
--ratchetdetection gap check runs independently.incompletePolicydoes not override it.Closes #898