feat: add verified remediation evidence handoff contract - #1006
feat: add verified remediation evidence handoff contract#1006seonghobae wants to merge 6 commits into
Conversation
|
Warning Review limit reachedNext included review available in 24 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (15)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@opencode-agent @cwl-noema-review please review exact current head 9b9d79e. The in-memory Mapping path now enforces the documented 2 MiB bound and rejects non-JSON values; focused tests 12 passed, exact changed-module coverage 137/137, full suite 1013 passed, appguardrail scan reports 0 deploy blockers, and git diff --check passed. |
|
Addressed Devin finding in |
|
@opencode-agent @cwl-noema-review please review exact current head 949873d. The final contract now bounds both Mapping inputs and generated artifacts at 2 MiB, validates revision and commit independently, and preserves inert/redacted transport. Validation: 13 focused tests, 100% statement/branch coverage (121 statements, 58 branches), full suite 1014 passed, appguardrail scan 0 deploy blockers, git diff --check clean. |
opencode-agent
Bot
added
area: api
|
Caution Review failedAn error occurred during the review process. Please try again later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| def _without_digest(payload: Mapping[str, Any]) -> dict[str, Any]: | ||
| """Return a shallow payload copy without its derived bundle digest.""" | ||
| return {key: value for key, value in payload.items() if key != "bundle_sha256"} | ||
|
|
||
|
|
||
| def _canonical_bytes(payload: Mapping[str, Any]) -> bytes: | ||
| """Serialize a handoff payload deterministically for hashing or transport.""" | ||
| return (json.dumps(payload, ensure_ascii=True, sort_keys=True, separators=(",", ":")) + "\n").encode( | ||
| "utf-8" | ||
| ) | ||
|
|
||
|
|
||
| def build_evidence_handoff( | ||
| findings: Iterable[Mapping[str, Any]], | ||
| *, | ||
| provenance: Mapping[str, Any] | None = None, | ||
| assurance: Mapping[str, Any] | None = None, | ||
| ) -> dict[str, Any]: | ||
| """Build a deterministic, redacted remediation handoff envelope.""" | ||
| payload: dict[str, Any] = { | ||
| "schema": HANDOFF_SCHEMA, | ||
| "version": HANDOFF_VERSION, | ||
| "findings": [_safe_finding(finding) for finding in findings], | ||
| "provenance": _safe_provenance(provenance), | ||
| } | ||
| safe_assurance = _safe_assurance(assurance) | ||
| if safe_assurance is not None: | ||
| payload["assurance"] = safe_assurance | ||
| payload["bundle_sha256"] = hashlib.sha256(_canonical_bytes(payload)).hexdigest() | ||
| if len(_canonical_bytes(payload)) > MAX_HANDOFF_BYTES: |
There was a problem hiding this comment.
📝 Info: Digest/verify round-trip is deterministic across insertion order and assurance presence
I verified the digest contract: build_evidence_handoff computes bundle_sha256 over the payload before inserting the digest field, and verify_evidence_handoff recomputes over _without_digest(payload). Because _canonical_bytes uses sort_keys=True with a trailing newline and all values are JSON-native, the bytes are identical across dict insertion order (confirmed by the reversed-provenance test) and whether or not assurance is present. No mismatch bug here.
Was this helpful? React with 👍 or 👎 to provide feedback.
| paths: | ||
| - "appguardrail_core/evidence_handoff.py" | ||
| - "tests/test_evidence_handoff.py" | ||
| - "docs/remediation-evidence-handoff.md" | ||
| - "docs/adr/0008-remediation-evidence-handoff.md" | ||
| - "CHANGELOG.d/928-evidence-handoff-contract.md" | ||
| - ".github/workflows/remediation-evidence-handoff-coverage.yml" | ||
| pull_request: | ||
| branches: [develop, main] | ||
| paths: | ||
| - "appguardrail_core/evidence_handoff.py" | ||
| - "tests/test_evidence_handoff.py" | ||
| - "docs/remediation-evidence-handoff.md" | ||
| - "docs/adr/0008-remediation-evidence-handoff.md" | ||
| - "CHANGELOG.d/928-evidence-handoff-contract.md" | ||
| - ".github/workflows/remediation-evidence-handoff-coverage.yml" |
There was a problem hiding this comment.
📝 Info: Coverage gate blind to dependency edits
The workflow triggers only on edits to evidence_handoff.py, its test, and listed docs. Changes to findings.py or issueops.py that alter handoff behavior will not re-run this exact-coverage gate.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if isinstance(outcome, str) and outcome in _OUTCOMES: | ||
| result["scan_outcome_code"] = outcome |
There was a problem hiding this comment.
🔍 scan_outcome_code allowlist may not match a future assurance producer
_OUTCOMES (evidence_handoff.py) allows clean/findings_present/incomplete/failed/untrusted, but the documented scan health states in OPERABILITY.md use different codes (completed_clean, completed_findings, inconclusive, engine_unavailable, engine_failed, policy_blocked, evidence_untrusted). No existing producer emits scan_outcome_code, so this is not currently a bug, but when a real scan-assurance producer is wired in, mismatched codes would be silently dropped from the handoff. Worth confirming the intended vocabulary before an integrating slice lands.
Was this helpful? React with 👍 or 👎 to provide feedback.
| - name: Verify exact handoff coverage | ||
| run: | | ||
| python -m scripts.ci.verify_module_coverage \ | ||
| --module appguardrail_core/evidence_handoff.py \ | ||
| --test tests/test_evidence_handoff.py |
There was a problem hiding this comment.
📝 Info: Focused coverage workflow enforces statement (not branch) coverage
The new workflow invokes scripts.ci.verify_module_coverage, which only measures statement coverage (scripts/ci/verify_module_coverage.py:49-68), while the PR description and docs claim 100% branch coverage. For example the conditional expression at evidence_handoff.py counts as a covered statement even if its else str(value) arm never executes. This gate alone will not catch missing branch coverage; branch coverage must be enforced elsewhere (e.g. tests.yml) if the 100% branch claim is a release requirement.
Was this helpful? React with 👍 or 👎 to provide feedback.
| "context", | ||
| ): | ||
| value = normalized.get(field) | ||
| safe[field] = value if isinstance(value, (str, int)) and not isinstance(value, bool) else str(value) |
There was a problem hiding this comment.
📝 Info: Selected finding identifier fields are copied without redaction
In _safe_finding (evidence_handoff.py) the fields rule_id, severity, file, line, category, context are copied verbatim from the normalized finding (only _TEXT_FIELDS and provenance identifiers pass through redact). If a file path or category ever contained a secret-shaped substring it would not be scrubbed. In practice these are structured classification fields with low secret risk, so this is not flagged as a bug, but it is worth being aware the redaction boundary intentionally excludes them.
Was this helpful? React with 👍 or 👎 to provide feedback.
| def _safe_identifier(value: Any, *, digest: bool = False) -> str | None: | ||
| """Return one bounded provenance identifier or omit malformed input.""" | ||
| if not isinstance(value, str) or not value or not value.isprintable(): | ||
| return None | ||
| value = redact(value.strip()) | ||
| if digest and not _DIGEST_RE.fullmatch(value): | ||
| return None | ||
| return value[:512] |
There was a problem hiding this comment.
🔍 redact() reuse may strip fractional-second timestamps from identifiers
_safe_identifier runs caller-provided identifiers (including generated_at) through redact (evidence_handoff.py), which is designed for job logs and strips ISO timestamps of the form YYYY-MM-DDThh:mm:ss.<frac>Z at line start (TS_RE in appguardrail_core/issueops.py:25-28). A generated_at value with fractional seconds would be reduced to an empty string, which is then stored as result[field] = "" rather than omitted (the emptiness check happens before redaction). Timestamps without fractional seconds are unaffected. Low impact but a slightly surprising interaction of reusing the log redactor on structured identifiers.
Was this helpful? React with 👍 or 👎 to provide feedback.
| def _safe_identifier(value: Any, *, digest: bool = False) -> str | None: | ||
| """Return one bounded provenance identifier or omit malformed input.""" | ||
| if not isinstance(value, str) or not value or not value.isprintable(): | ||
| return None | ||
| value = redact(value.strip()) | ||
| if digest and not _DIGEST_RE.fullmatch(value): | ||
| return None | ||
| return value[:512] |
There was a problem hiding this comment.
📝 Info: Digest fields require lowercase hex while commit allows uppercase
_DIGEST_RE only matches lowercase 64-hex (evidence_handoff.py), so any caller-supplied artifact_sha256/evidence_digest given in uppercase is silently dropped, whereas _COMMIT_RE accepts mixed case (:18). This is an intentional-looking strictness choice (hashlib emits lowercase), but consumers passing upper/mixed-case digests will find them omitted with no signal.
Was this helpful? React with 👍 or 👎 to provide feedback.
seonghobae
added
the
enhancement
ChatGPT Codex Connector
Summary: - Record 05:40 UTC exact-head evidence for #998 coverage/bootstrap SUCCESS, #972/#1006 non-force restacks onto develop@e71d37e, and #1130 Draft under canonical #1107. - Mark G-02 in progress on #972 and G-06 as a stacked successor of #1088 rather than further regex growth. Rationale: - #999 is the single writer of the product-technical gap baseline. - Restacked heads invalidate predecessor GREEN; the snapshot must not claim protected capability. Tests: - documentation-only; no scanner behavior change
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
OpenCode reviewed the current-head product diff. Coverage is a separate gate.
Changed files
.github/workflows/remediation-evidence-handoff-coverage.yml— GitHub Actions review jobCHANGELOG.d/928-evidence-handoff-contract.md— repository behaviorappguardrail_core/evidence_handoff.py— Python module behaviordocs/ERD.md— operator or user guidancedocs/OPERABILITY.md— operator or user guidancedocs/PRD.md— operator or user guidancedocs/TEST_STRATEGY.md— operator or user guidancedocs/THREAT_MODEL.md— operator or user guidancedocs/TRACEABILITY.md— operator or user guidancedocs/TRD.md— operator or user guidancedocs/UML.md— operator or user guidancedocs/adr/0008-remediation-evidence-handoff.md— operator or user guidancedocs/adr/README.md— operator or user guidancedocs/remediation-evidence-handoff.md— operator or user guidancetests/test_evidence_handoff.py— regression suite
Changed behavior
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Workflow: remediation-evidence-handoff-coverage.yml"]
S1 --> I1["GitHub Actions review job"]
I1 --> R1["Review risk: Workflow: remediation-evidence-handoff-coverage.yml"]
R1 --> V1["actionlint plus required checks"]
Evidence --> S2["Repository file: 928-evidence-handoff-contract.md"]
S2 --> I2["repository behavior"]
I2 --> R2["Review risk: Repository file: 928-evidence-handoff-contract.md"]
R2 --> V2["required checks"]
Evidence --> S3["Python: evidence_handoff.py"]
S3 --> I3["Python module behavior"]
I3 --> R3["Review risk: Python: evidence_handoff.py"]
R3 --> V3["pytest plus coverage"]
Evidence --> S4["Docs: ERD.md (11 files)"]
S4 --> I4["operator or user guidance"]
I4 --> R4["Review risk: Docs: ERD.md (11 files)"]
R4 --> V4["docs review"]
Evidence --> S5["Test: test_evidence_handoff.py"]
S5 --> I5["regression suite"]
I5 --> R5["Review risk: Test: test_evidence_handoff.py"]
R5 --> V5["targeted test run"]
Findings
No source-backed product finding is synthesized from the coverage gate. A coverage miss belongs in the status comment.
- Head SHA:
35c28e22b5d50f1da943718cdb1984dcee098d62 - Workflow run: 34111908691
- Workflow attempt: 1
- Coverage gate:
failure
Review outcome
Coverage is a gate, not the review. This body reviews the changed product files.
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Workflow: remediation-evidence-handoff-coverage.yml"]
S1 --> I1["GitHub Actions review job"]
I1 --> R1["Review risk: Workflow: remediation-evidence-handoff-coverage.yml"]
R1 --> V1["actionlint plus required checks"]
Evidence --> S2["Repository file: 928-evidence-handoff-contract.md"]
S2 --> I2["repository behavior"]
I2 --> R2["Review risk: Repository file: 928-evidence-handoff-contract.md"]
R2 --> V2["required checks"]
Evidence --> S3["Python: evidence_handoff.py"]
S3 --> I3["Python module behavior"]
I3 --> R3["Review risk: Python: evidence_handoff.py"]
R3 --> V3["pytest plus coverage"]
Evidence --> S4["Docs: ERD.md (11 files)"]
S4 --> I4["operator or user guidance"]
I4 --> R4["Review risk: Docs: ERD.md (11 files)"]
R4 --> V4["docs review"]
Evidence --> S5["Test: test_evidence_handoff.py"]
S5 --> I5["regression suite"]
I5 --> R5["Review risk: Test: test_evidence_handoff.py"]
R5 --> V5["targeted test run"]
OpenCode Review Overview
Coverage evidence did not pass, so approval is blocked. The formal pull-request review is the source-backed diff review, not this status comment. |
1 participant
Summary
Part of #928. This PR lands the standalone, transport-neutral remediation handoff contract; the dashboard clipboard/UI slice remains separate pending the repository Figma/Storybook design gate.
appguardrail_core.evidence_handoffwith report-safe normalization, IssueOps redaction, bounded fields, deterministic JSON, schema/version, 2 MiB bound, andbundle_sha256verification;Verification
uv run --with pytest pytest -q— 1,013 passedruff checkandgit diff --check— cleanappguardrail scan --codegraph .— 0 deploy blockers; 74 non-blocking docs/tests/examples/fixtures findingsScope boundary
The PR does not claim completion of the dashboard actions, clipboard fallback UI, live-region announcements, focus behavior, or browser E2E. Those require the current dashboard design/Storybook evidence and will be a follow-up slice.