-
Notifications
You must be signed in to change notification settings - Fork 742
fix(deep-scan): recover complete coverage without hiding incomplete worker results #657
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
6a06171
c1d997a
2db2548
cd66656
e19ff83
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -47,6 +47,9 @@ | |
| _PUBLICATION_FOLLOW_UP_WARNING = ( | ||
| "Saved scan evidence remains on disk; result publication needs follow-up:" | ||
| ) | ||
| _UNVERIFIED_COVERAGE_WARNING = ( | ||
| "Saved scan source is incomplete or has unverified coverage; coverage remains partial." | ||
| ) | ||
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
|
|
@@ -252,6 +255,11 @@ def merge_saved_results( | |
| } | ||
| if not isinstance(parent["findings"], list): | ||
| raise ContractError("Saved parent draft has no findings array") | ||
| if ( | ||
| parent_scan.get("complete", True) is not True | ||
| and _UNVERIFIED_COVERAGE_WARNING not in warnings | ||
| ): | ||
| warnings.append(_UNVERIFIED_COVERAGE_WARNING) | ||
| if not parent_scan.get("sealedAt"): | ||
| payload = _encoded(parent) | ||
| write_scan_local_bytes( | ||
|
|
@@ -277,6 +285,7 @@ def merge_saved_results( | |
| source_digests.update(parent_preserved_sources) | ||
| paths: dict[str, str | None] = {} | ||
| current_results: set[str] = set() | ||
| required_results: set[str] = set() | ||
| reducer_outputs: list[tuple[Any, str, list[str], int]] = [] | ||
| accepted_reducers = [ | ||
| worker | ||
|
|
@@ -293,6 +302,7 @@ def merge_saved_results( | |
| try: | ||
| latest_reducer = Path(reducer["result_manifest_path"]).relative_to(scan_dir).as_posix() | ||
| paths[latest_reducer] = None | ||
| required_results.add(latest_reducer) | ||
| except ValueError: | ||
| warnings.append("Skipped a reducer result outside the scan directory.") | ||
|
|
||
|
|
@@ -351,6 +361,8 @@ def reducer_output(directory: str, attempt: int, reducer_worker: Any) -> None: | |
| current_path = Path(worker["result_manifest_path"]).relative_to(scan_dir).as_posix() | ||
| paths[current_path] = worker["id"] | ||
| current_results.add(current_path) | ||
| if worker["status"] == "succeeded": | ||
| required_results.add(current_path) | ||
| except ValueError: | ||
| warnings.append("Skipped a worker result outside the scan directory.") | ||
|
|
||
|
|
@@ -381,6 +393,8 @@ def reducer_output(directory: str, attempt: int, reducer_worker: Any) -> None: | |
| except (ContractError, OSError, ValueError) as exc: | ||
| if (scan_dir / relative).exists(): | ||
| warnings.append(f"Preserved unreadable checkpoint {relative}: {exc}") | ||
| elif relative in required_results and _UNVERIFIED_COVERAGE_WARNING not in warnings: | ||
| warnings.append(_UNVERIFIED_COVERAGE_WARNING) | ||
| if frozen_source_digests is not None: | ||
| if frozen_source_digests.keys() - source_digests.keys(): | ||
| raise ContractError("Frozen stopped-scan checkpoint set is incomplete.") | ||
|
|
@@ -577,16 +591,30 @@ def valid_finding(value: Any) -> bool: | |
| for saved_path, current, saved_worker in sources | ||
| ) | ||
| ) | ||
| if ( | ||
| (relative != "parent" or not parent_manifest) | ||
| and not superseded | ||
| and ( | ||
| draft.get("complete") is False | ||
| or draft["coverage"].get("completeness") != "complete" | ||
| ) | ||
| and coverage.get("completeness") in {"complete", "unknown"} | ||
| ): | ||
| coverage["completeness"] = "partial" | ||
| if (relative != "parent" or not parent_manifest) and not superseded: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
For a successful dedup worker whose result exists but has AGENTS.md reference: sdk/typescript/AGENTS.md:L22-L23 Useful? React with 👍 / 👎. |
||
| source_coverage = draft["coverage"] | ||
| source_completeness = source_coverage.get("completeness") | ||
| source_complete = draft.get("complete", True) is True | ||
|
Comment on lines
+594
to
+597
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When the canonical parent manifest contains AGENTS.md reference: sdk/typescript/AGENTS.md:L22-L24 Useful? React with 👍 / 👎.
Comment on lines
+594
to
+597
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a succeeded discovery worker's current result has a malformed completion marker such as AGENTS.md reference: sdk/typescript/AGENTS.md:L23-L23 Useful? React with 👍 / 👎. |
||
| if ( | ||
| not source_complete | ||
| or ( | ||
| source_completeness != "complete" | ||
| and ( | ||
| worker_id is not None | ||
| or source_completeness != "partial" | ||
| or coverage.get("completeness") == "unknown" | ||
| ) | ||
| ) | ||
| or any( | ||
| not isinstance(source_coverage.get(field), list) | ||
| for field in ("surfaces", "explicitExclusions", "deferred") | ||
| ) | ||
| ) and _UNVERIFIED_COVERAGE_WARNING not in warnings: | ||
| warnings.append(_UNVERIFIED_COVERAGE_WARNING) | ||
| if ( | ||
| not source_complete or source_completeness != "complete" | ||
| ) and coverage.get("completeness") in {"complete", "unknown"}: | ||
| coverage["completeness"] = "partial" | ||
| if superseded and not stopped: | ||
| continue | ||
| if "threatModel" not in manifest["scan"] and isinstance(draft.get("threatModel"), dict): | ||
|
|
@@ -813,7 +841,11 @@ def valid_finding(value: Any) -> bool: | |
| used.add(item["id"]) | ||
| if field == "surfaces": | ||
| item.setdefault("receiptRefs", []) | ||
| if stopped or any(warning not in initial_warnings for warning in warnings): | ||
| if ( | ||
| stopped | ||
| or _UNVERIFIED_COVERAGE_WARNING in warnings | ||
| or any(warning not in initial_warnings for warning in warnings) | ||
| ): | ||
| coverage["completeness"] = "partial" | ||
| if stopped: | ||
| if not isinstance(coverage.get("deferred"), list): | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When an otherwise complete Deep Scan contains duplicate logical findings with the strongest or equal record first,
_recover_unsealed_findingssafely retains that record but emitsSkipped malformed finding N: duplicate logical finding.; this allowlist accepts only the inverse-orderretained stronger duplicatewarning. Consequently, equivalent findings seal with partial versus complete coverage solely based on their ordering. Treat the lossless duplicate-discard warning as non-coverage-affecting as well.AGENTS.md reference: sdk/typescript/AGENTS.md:L23-L23
Useful? React with 👍 / 👎.