docs: establish canonical detector and assurance architecture - #912
Conversation
📝 WalkthroughWalkthroughAppGuardrail의 제품·아키텍처·보안·운영·추적성 문서를 추가했습니다. 탐지 증거, 테넌트 경계, SSRF, 자동 수정, 외부 엔진 및 릴리스 조건을 정의했습니다. 문서 구조와 핵심 ADR을 검증하는 계약 테스트도 추가했습니다. ChangesAppGuardrail 문서 기준선
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
docs/UML.md (1)
103-105: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
tests_red상태의 의미를 수정하거나 명확히 정의하십시오.
tests_red가 실패한 테스트 상태를 뜻한다면, 현재tests_red에서executable_detector로 전이하면 실패한 검증을 통과한 탐지기로 승격합니다.tests_verified또는tests_green같은 통과 상태로 변경하십시오. 실패 상태는 별도의 실패 전이로 보내십시오.수정 예시
- detector_obligation --> tests_red: positive/negative/inconclusive evidence - tests_red --> executable_detector + detector_obligation --> tests_verified: positive/negative/inconclusive evidence + tests_verified --> executable_detector🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/UML.md` around lines 103 - 105, UML의 tests_red 상태 의미를 명확히 정의하고, 실패한 테스트 상태에서 executable_detector로 직접 전이하지 않도록 수정하십시오. 통과를 의미하는 tests_verified 또는 tests_green 상태를 executable_detector의 선행 상태로 사용하고, 실패 결과는 별도의 실패 전이로 연결하십시오.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/ERD.md`:
- Around line 84-104: Update ISSUE_CLAIM to include repository_full_name and
define uniqueness using the repository identifier together with issue_number and
claim_identifier, so claims from different repositories cannot collide. Document
the storage scope and canonical claim_identifier generation invariant, and add
validation tests covering cross-repository collisions and canonical ID
stability.
- Around line 43-52: Update the FINDING_RECORD and WORKFLOW_EVIDENCE schemas to
explicitly define required provenance and authentication fields, including
engine version, source, producer capability, and signature data. Document
required metadata keys, digest coverage, and signature validation rules rather
than relying solely on bounded_metadata_json. Add contract tests that verify
these fields and validation requirements.
In `@docs/OPERABILITY.md`:
- Around line 52-54: Update the Webhook operability contract to define retry
idempotency: require a stable delivery_id and receiver-side deduplication, or
explicitly prohibit retries under an at-most-once policy. Apply the same chosen
contract to the Webhook flow in UML.md and the webhook model in ERD.md, while
preserving destination validation and capped backoff requirements where retries
remain allowed.
In `@tests/test_documentation_contract.py`:
- Around line 9-25: Expand the documentation contract checks in
tests/test_documentation_contract.py:9-25 by adding the three canonical paths
from DOCUMENTATION.md lines 17-19 to REQUIRED_DOCUMENTS; update
tests/test_documentation_contract.py:45-46 to validate actual [label](path)
links for every mapped document; and update
tests/test_documentation_contract.py:69-81 to assert each ADR target exists
under ROOT / "docs/adr" / adr.
---
Nitpick comments:
In `@docs/UML.md`:
- Around line 103-105: UML의 tests_red 상태 의미를 명확히 정의하고, 실패한 테스트 상태에서
executable_detector로 직접 전이하지 않도록 수정하십시오. 통과를 의미하는 tests_verified 또는 tests_green
상태를 executable_detector의 선행 상태로 사용하고, 실패 결과는 별도의 실패 전이로 연결하십시오.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a4f05011-7c42-4b9a-b856-c31a07ca67db
📒 Files selected for processing (18)
ARCHITECTURE.mdDOCUMENTATION.mddocs/ERD.mddocs/OPERABILITY.mddocs/PRD.mddocs/TEST_STRATEGY.mddocs/THREAT_MODEL.mddocs/TRACEABILITY.mddocs/TRD.mddocs/UML.mddocs/adr/0001-executable-detector-truth.mddocs/adr/0002-prevention-versus-detection.mddocs/adr/0003-external-engine-provenance.mddocs/adr/0004-tenant-network-boundaries.mddocs/adr/0005-remediation-authority.mddocs/adr/0006-automation-authority.mddocs/adr/README.mdtests/test_documentation_contract.py
|
@coderabbitai review Please re-review current exact head |
|
I will assess the current contracts for evidence provenance, repository-scoped issue identities, at-most-once webhook delivery, future retry deduplication, and tests-verified detector maturity. I will not use resolved predecessor feedback as current evidence.
|
![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 cannot approve yet because required coverage evidence did not pass.
Review outcome
1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
-
Problem: The required coverage-evidence job result was
failure, so OpenCode cannot establish approval sufficiency for this head. -
Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.
-
Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports
successwith required evidence or explicit no-source not-applicable evidence. -
Regression test: Keep the approval branch checking
needs.coverage-evidence.result == successbefore posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present. -
Result: REQUEST_CHANGES
-
Reason: coverage-evidence result was
failure, so required test/docstring evidence was not proven for current headc302af588c7d73b2741bd46edb5b666d575ad944. -
Head SHA:
c302af588c7d73b2741bd46edb5b666d575ad944 -
Workflow run: 31318183269
-
Workflow attempt: 1
Coverage evidence
Coverage evidence job did not run or did not publish coverage evidence.
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (2 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (2 files)"]
R1 --> V1["required checks"]
Evidence --> S2["Docs (15 files)"]
S2 --> I2["operator or user guidance"]
I2 --> R2["Review risk: Docs (15 files)"]
R2 --> V2["docs review"]
Evidence --> S3["Test: test_documentation_contract.py"]
S3 --> I3["regression suite"]
I3 --> R3["Review risk: Test: test_documentation_contract.py"]
R3 --> V3["targeted test run"]
OpenCode Review Overview
Pull request overviewOpenCode cannot approve yet because required coverage evidence did not pass. Review outcome1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
Coverage evidenceCoverage evidence job did not run or did not publish coverage evidence. Changed-File Evidence Mapflowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (2 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (2 files)"]
R1 --> V1["required checks"]
Evidence --> S2["Docs (15 files)"]
S2 --> I2["operator or user guidance"]
I2 --> R2["Review risk: Docs (15 files)"]
R2 --> V2["docs review"]
Evidence --> S3["Test: test_documentation_contract.py"]
S3 --> I3["regression suite"]
I3 --> R3["Review risk: Test: test_documentation_contract.py"]
R3 --> V3["targeted test run"]
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/ERD.md`:
- Around line 97-99: The webhook SSRF contract must define protection against
TOCTOU between destination validation and the actual connection. Update
docs/ERD.md lines 97-99 to require policy evaluation for every attempt and
redirect hop, address pinning or equivalent connection-time control, and
rejection of private, link-local, and metadata addresses; update
docs/OPERABILITY.md lines 54-56 to apply the same controls to current and future
retry procedures and contract tests.
- Around line 85-93: Update the normalized evidence contract around
signed_payload_digest, evidence_digest, and finding_digest to define
deterministic canonical serialization: field order, omitted versus null fields,
numeric and Unicode normalization, bounded_metadata_json encoding, and hash
input encoding. Specify each digest’s exact coverage and how signed workflow
envelopes link to the resulting finding. Add contract tests that verify
identical bytes and digest values across producer and verifier implementations,
including the required edge cases.
- Around line 160-170: WORKFLOW_EVIDENCE의 필수 attestation 표현을 명확히 하십시오. 기존
signature_algorithm_code, signature_value 등 signature_* 필드를 attestation으로 사용할 경우
발급자·유형·값/참조와의 매핑 및 필수 조건을 문서화하고, 별도 개념이라면 해당 필드와 검증 규칙을 추가하십시오. Lines 183-185의
필수 attestation 누락 시 evidence_untrusted 처리와 일관되도록 정의를 맞추십시오.
In `@tests/test_documentation_contract.py`:
- Around line 126-133: Update test_detector_maturity_requires_verified_tests to
also assert that "tests_failed --> executable_detector" is absent from the UML,
preserving the existing prohibition for tests_red and ensuring failed tests
cannot mark a detector executable.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f39c9816-33e9-42af-980d-632b77094dcf
📒 Files selected for processing (5)
docs/ERD.mddocs/OPERABILITY.mddocs/PRD.mddocs/UML.mdtests/test_documentation_contract.py
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/PRD.md
- docs/UML.md
There was a problem hiding this comment.
Pull request overview
OpenCode cannot approve yet because required coverage evidence did not pass.
Review outcome
1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
-
Problem: The required coverage-evidence job result was
failure, so OpenCode cannot establish approval sufficiency for this head. -
Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.
-
Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports
successwith required evidence or explicit no-source not-applicable evidence. -
Regression test: Keep the approval branch checking
needs.coverage-evidence.result == successbefore posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present. -
Result: REQUEST_CHANGES
-
Reason: coverage-evidence result was
failure, so required test/docstring evidence was not proven for current head99ecd9ac60de538f9a2d24d3d03785268026a027. -
Head SHA:
99ecd9ac60de538f9a2d24d3d03785268026a027 -
Workflow run: 31321562807
-
Workflow attempt: 1
Coverage evidence
Coverage evidence job did not run or did not publish coverage evidence.
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (2 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (2 files)"]
R1 --> V1["required checks"]
Evidence --> S2["Docs (15 files)"]
S2 --> I2["operator or user guidance"]
I2 --> R2["Review risk: Docs (15 files)"]
R2 --> V2["docs review"]
Evidence --> S3["Test: test_documentation_contract.py"]
S3 --> I3["regression suite"]
I3 --> R3["Review risk: Test: test_documentation_contract.py"]
R3 --> V3["targeted test run"]
Merge protected develop into the canonical documentation branch, promote the separately verified webhook prevention and bounded scanner-detection controls, keep PR #911 labelled active, and update executable documentation contracts.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_documentation_contract.py`:
- Around line 68-75: 테스트의 PR 상태 검증을 분리된 포함 여부 검사에서 동일 주장 검증으로 변경하세요. PR `#924와` PR
`#910은` 각 PR 번호에 해당하는 기능 범위와 implemented-main 상태가 같은 문맥에 함께 있는지 확인하고, PR `#911도`
active-PR 상태와 해당 기능 범위를 동일 문맥에서 검증하도록 assertions를 업데이트하세요.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 87425fd5-748c-4348-b91f-fd391ae3df52
📒 Files selected for processing (9)
ARCHITECTURE.mdDOCUMENTATION.mddocs/ERD.mddocs/OPERABILITY.mddocs/PRD.mddocs/TEST_STRATEGY.mddocs/TRACEABILITY.mddocs/TRD.mdtests/test_documentation_contract.py
🚧 Files skipped from review as they are similar to previous changes (8)
- ARCHITECTURE.md
- DOCUMENTATION.md
- docs/OPERABILITY.md
- docs/PRD.md
- docs/TRACEABILITY.md
- docs/TRD.md
- docs/TEST_STRATEGY.md
- docs/ERD.md
Dismissed as stale predecessor-head review. The review was anchored to c302af5; the current head is 4c634dd after protected-develop reconciliation and all exact-head repository checks are successful. This dismissal is not approval; a fresh independent current-head OpenCode review is being requested.
|
@cwl-noema-review |
1 participant
Purpose
Persist AppGuardrail's cross-cutting product, detector, tenant/network, evidence, and operating model in canonical GitHub documentation instead of reconstructing it from README, issue bodies, workflows, reports, and feature-specific notes.
The documentation deliberately distinguishes executable detector truth from registry assertions, built-in matching from optional external engines, and application prevention from scanner detection.
Added
DOCUMENTATION.mddocs/PRD.mddocs/TRD.mdARCHITECTURE.mddocs/UML.mddocs/ERD.mddocs/THREAT_MODEL.mddocs/TEST_STRATEGY.mddocs/OPERABILITY.mddocs/TRACEABILITY.mdtests/test_documentation_contract.pySecurity/detection truth boundary
implemented-maincontrols on protecteddevelop.python-stored-ssrf-webhook-urlrule covers its tested Pythonset_webhookdirect/one-hop persistence contract, including conditional and non-enforcing guard blind spots; it is not presented as universal interprocedural SSRF taint analysis.pattern:fixtures are not presented as built-in execution by the current lightweight matcher unless a real structural engine executes them.Data-model boundary
The current control plane is SQLite-backed and tenant scoped. The issue-obligation ERD section is an active-PR logical model, not a claim that PR #911 introduced those entities as database tables. Future managed persistence requires explicit migrations/rollback/tenant/security/recovery evidence.
Current-head reconciliation
The branch contains a merge parent for protected
develop@77e3e0c5867b1143970fcdce80962bda8a8fc80fand no longer carries the prior 11-commit base lag. Canonical maturity assertions and their executable documentation contract were updated together; the current head is4c634ddce1065faac3203f4da071696e523150e2.Verification contract
The pytest documentation contract checks canonical-document existence, discoverability, current versus active-PR maturity, prevention/detection separation, bounded stored-SSRF detector scope, built-in/external detector truth, and ADR completeness. Current-head assertions bind each PR number, maturity state, and functional scope to one documentation line so unrelated status text cannot create a false pass. All exact-current-head CI, production statement/branch/docstring, SAST/security, independent review, branch protection, and package/evidence gates must pass before merge.