BCH-1340: Auto-generate downstream risks for non-compliant leveraged responsibilities (Phase 4)#450
Merged
Conversation
…responsibilities (Phase 4) Non-compliant evidence on a downstream customer-responsibility now auto-generates a downstream-local risk attached to that responsibility, reusing the existing evidence-to-Risk-Template worker: - New risk_responsibility_links(risk_id, responsibility_uuid) table. - New RiskSourceTypeInheritedResponsibility = "inherited-responsibility". - resolveSSPsViaFilters now returns responsibility matches separately from control matches, since they feed a structurally different risk (dedupe-keyed on the responsibility uuid, linked via risk_responsibility_links, one risk per SSP+responsibility rather than per SSP+template). - createOrUpdateRisksForResponsibilities creates/dedupes/reopens these risks, mirroring createNewRiskForSSP. Auto-remediation on satisfied evidence requires no changes to handleEvidenceResolution — it already operates generically on any risk with a risk_evidence_links row. - Promote-to-POA&M works unchanged (verified, not modified). Upstream risk stays upstream: a filter_controls-only match never creates an inherited-responsibility risk anywhere.
📝 WalkthroughWalkthroughAdds the ChangesInherited Responsibility Risk Feature
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Worker as RiskEvidenceWorker
participant Resolver as resolveSSPsViaFilters
participant Database
Worker->>Resolver: resolve evidence labels
Resolver->>Database: query filter and leverage relationships
Database-->>Resolver: return SSP and responsibility matches
Resolver-->>Worker: return resolution results
Worker->>Database: create or update inherited-responsibility risks
Worker->>Database: persist evidence and responsibility links
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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: 2
🤖 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 `@internal/authz/manifest.yaml`:
- Line 146: The manifest’s documented `source_type` values are out of sync with
`RiskSourceType.IsValid()` because `risk-promotion` is listed but not accepted
by the risk model. Update the manifest entry to match the validation in
`RiskSourceType.IsValid()` and `BeforeCreate` by either removing
`risk-promotion` from the allowed values or adding a corresponding
`RiskSourceTypeRiskPromotion` constant and validation branch so the documented
set and runtime checks are consistent.
In
`@internal/service/worker/risk_evidence_worker_inherited_responsibility_integration_test.go`:
- Around line 51-98: The two seeding helpers in
InheritedResponsibilityRiskIntegrationSuite duplicate the logic already present
in the responsibility integration tests, so move the shared setup into a common
test helper and reuse it from both suites. Keep the existing behavior of
seedLeveragedResponsibility by allowing it to return the extra upstreamSSPID,
but centralize the shared export/provided/responsibility/link and filter
creation logic in a helper such as risk_evidence_worker_test_helpers_test.go so
both seedLeveragedResponsibility and seedResponsibilityFilter stay aligned.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro Plus
Run ID: eb0ff99e-c285-4c0f-97e2-5c05eea1f794
📒 Files selected for processing (9)
internal/authz/manifest.yamlinternal/service/migrator.gointernal/service/relational/risks/links.gointernal/service/relational/risks/models.gointernal/service/worker/risk_evidence_worker.gointernal/service/worker/risk_evidence_worker_inherited_responsibility_integration_test.gointernal/service/worker/risk_evidence_worker_responsibility_integration_test.gointernal/service/worker/risk_evidence_worker_test.gointernal/tests/migrate.go
…elper duplication Removes risk-promotion from risk.source_type's doc comment — it's actually a PoamItemSourceType value, not a RiskSourceType one, and was never accepted by RiskSourceType.IsValid(). Also centralizes the leveraged- responsibility seeding helpers shared by the BCH-1339 and BCH-1340 integration suites into a single test helper file.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/service/worker/risk_evidence_worker_inherited_responsibility_integration_test.go (1)
84-84: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
evidence.End.Add(1)adds only 1 nanosecond, not one day.
time.Duration(1)is 1 nanosecond. While this technically makes the satisfied evidence "newer" than the original, it's fragile and unclear — especially since theEvidenceEndarg on line 89 is a full day later ("2026-01-02T00:00:00Z"vs"2026-01-01T00:00:00Z"). UseAddDate(0, 0, 1)to add a full day, aligning the DBEndfield with the arg and making the intent obvious.🔧 Proposed fix
Start: evidence.Start, - End: evidence.End.Add(1), + End: evidence.End.AddDate(0, 0, 1), Status: datatypes.NewJSONType(oscalTypes_1_1_3.ObjectiveStatus{State: "satisfied"}),🤖 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 `@internal/service/worker/risk_evidence_worker_inherited_responsibility_integration_test.go` at line 84, In the evidence fixture using `evidence.End.Add(1)`, replace it with `evidence.End.AddDate(0, 0, 1)` so the `End` field advances by one full day and matches the `EvidenceEnd` argument.
🤖 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.
Outside diff comments:
In
`@internal/service/worker/risk_evidence_worker_inherited_responsibility_integration_test.go`:
- Line 84: In the evidence fixture using `evidence.End.Add(1)`, replace it with
`evidence.End.AddDate(0, 0, 1)` so the `End` field advances by one full day and
matches the `EvidenceEnd` argument.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1c18a91b-d991-4ac0-aa10-b8dc3261f8ba
📒 Files selected for processing (4)
internal/authz/manifest.yamlinternal/service/worker/risk_evidence_worker_inherited_responsibility_integration_test.gointernal/service/worker/risk_evidence_worker_responsibility_integration_test.gointernal/service/worker/risk_evidence_worker_test_helpers_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Phase 4 of Cross-SSP Control Export/Import: non-compliant evidence on a downstream customer-responsibility now auto-generates a downstream-local risk attached to that responsibility, reusing the existing evidence→Risk-Template worker. Depends on Phase 3 (#448, merged).
risk_responsibility_links(risk_id, responsibility_uuid)table.RiskSourceTypeInheritedResponsibility = "inherited-responsibility"(+IsValid()+ manifest doc value).resolveSSPsViaFiltersnow returns responsibility matches separately from control matches ([]resolvedResponsibilityInfo), since a responsibility match feeds a structurally different risk — dedupe-keyed onssp_id:template_id:responsibility:<uuid>, linked viarisk_responsibility_links, one risk per (SSP, responsibility) pair rather than per (SSP, template).createOrUpdateRisksForResponsibilities/createNewResponsibilityRisk/updateExistingResponsibilityRisk/createResponsibilityRiskLinksmirror the existing control-arm functions.handleEvidenceResolution— it already operates generically on any risk with arisk_evidence_linksrow, and these new risks get one too.RiskService.PromoteToPoam, not modified).Note: this changes
resolveSSPsViaFilters's return signature (added in #448/BCH-1339). That's intentional — BCH-1339 explicitly deferred risk generation to this ticket, so nothing depended on the old merged shape.Test plan
handleEvidenceResolutionauto-remediates an inherited-responsibility risk untouched.filter_controls-only match never creates an inherited-responsibility risk anywhere), promote-to-POA&M.make reviewable(swag + lint + full integration suite) passes clean.Out of scope
Upstream drift risk (Phase 5).
Summary by CodeRabbit
New Features
inherited-responsibilitysource type and lifecycle handling.Bug Fixes
Tests