Phase B.1: Final acceptance and critical SoD hardening - #4
Merged
Conversation
Derive participation from NoteStatusHistory start-work and submit-for-verification transitions instead of LastProcessedByUserId alone, and document Phase B.1 as accepted after merge. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
Reviewer's GuideThis PR hardens Critical SoD for note closure by enforcing separation-of-duties against all actual processors using NoteStatusHistory, extends unit/integration coverage for multi-user edge cases and non-critical policy, and updates Phase B.1 documentation (completion report, test matrix, permissions matrix, state machine, README) to reflect final acceptance and the new SoD rules. Sequence diagram for Critical SoD enforcement on verify-closuresequenceDiagram
actor User
participant NoteWorkflowService
participant db
User->>NoteWorkflowService: VerifyClosureAsync(id, request, cancellationToken)
NoteWorkflowService->>NoteWorkflowService: NoteStateMachine.EnsureAllowed(note.Status, NoteStatus.Closed)
NoteWorkflowService->>NoteWorkflowService: RequireUserId()
NoteWorkflowService->>NoteWorkflowService: EnforceCriticalSoDAsync(note, actorId, cancellationToken)
alt note.Severity != NoteSeverity.Critical
NoteWorkflowService-->>NoteWorkflowService: return (no SoD check)
else note.Severity == NoteSeverity.Critical
NoteWorkflowService->>db: NoteStatusHistories.AnyAsync(predicate, cancellationToken)
db-->>NoteWorkflowService: participated
alt participated == true
NoteWorkflowService-->>User: throw InvalidOperationException
else participated == false
NoteWorkflowService-->>NoteWorkflowService: proceed with closure
end
end
NoteWorkflowService->>NoteWorkflowService: apply closure mutation and history
NoteWorkflowService-->>User: NoteDetailDto
Flow diagram for identifying processing participation from NoteStatusHistoryflowchart LR
A[Assigned] --> B[InProgress]
C[Reopened] --> B
B --> D[PendingVerification]
D --> B
subgraph ProcessingTransitions
A --> B
C --> B
B --> D
end
subgraph NonProcessingTransitions
D --> B
end
E[User participated in any ProcessingTransitions] --> F[Blocked from verify-closure on Critical note]
G[No participation in ProcessingTransitions] --> H[Allowed to verify-closure if Notes.VerifyClosure and scope]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Review limit reached
Next review available in: 55 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughCritical note closure now identifies processing participants from ChangesCritical SoD hardening
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Verifier
participant VerifyClosureAsync
participant NoteStatusHistories
Verifier->>VerifyClosureAsync: request verify-closure
VerifyClosureAsync->>NoteStatusHistories: query processing transitions
NoteStatusHistories-->>VerifyClosureAsync: participation result
VerifyClosureAsync-->>Verifier: reject participant or continue closure
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements hardening for the Critical Separation of Duties (SoD) policy on operational notes. The validation logic in NoteWorkflowService has been updated to asynchronously query the append-only NoteStatusHistory to ensure that any user who participated in the actual processing of a critical note (such as starting work or submitting for verification) is blocked from verifying its final closure. This restriction also applies to system administrators. Additionally, the PR updates relevant documentation and introduces comprehensive unit and integration tests to verify these SoD rules under various multi-user scenarios. There are no review comments to address, and the changes look solid.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
AppendProcessingHistoryand some of the new unit test helpers you call_db.SaveChanges()repeatedly for single-row inserts; consider batching these changes and saving once per test setup to keep the tests faster and reduce noise in future additions. - The Critical SoD logic encodes specific
(FromStatus, ToStatus)pairs directly inEnforceCriticalSoDAsync; it may be worth centralizing these transitions (e.g., a shared helper or configuration) so that any future state-machine changes don’t silently diverge between SoD enforcement and the documentation/matrix.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `AppendProcessingHistory` and some of the new unit test helpers you call `_db.SaveChanges()` repeatedly for single-row inserts; consider batching these changes and saving once per test setup to keep the tests faster and reduce noise in future additions.
- The Critical SoD logic encodes specific `(FromStatus, ToStatus)` pairs directly in `EnforceCriticalSoDAsync`; it may be worth centralizing these transitions (e.g., a shared helper or configuration) so that any future state-machine changes don’t silently diverge between SoD enforcement and the documentation/matrix.
## Individual Comments
### Comment 1
<location path="src/backend/tests/Baseera.IntegrationTests/NotesCoreIntegrationTests.cs" line_range="191-200" />
<code_context>
+ public async Task Independent_verifier_outside_scope_cannot_close_critical_note()
</code_context>
<issue_to_address>
**suggestion (testing):** Add DB assertions to confirm no closure or side-effects when an out-of-scope verifier is rejected
This integration test only checks the 404 response for the out-of-scope verifier. Because Critical SoD and scoping are security-sensitive, please also assert that the note remains in `PendingVerification` and that no `NoteStatusHistory` or `AuditLogs` entries for `NoteClosed` are created, similar to `Critical_note_all_processors_are_blocked_from_final_closure`. This will confirm the API does not mutate state when access is denied.
Suggested implementation:
```csharp
[IntegrationConnectionFact]
public async Task Independent_verifier_outside_scope_cannot_close_critical_note()
{
// Arrange users: admin, in-scope worker, and verifier that is outside the note's facility scope
await _factory.SeedUserAsync(
"sod-out-admin",
"مسؤول",
[RoleCodes.SystemAdministrator],
(ScopeType.Global, null, null));
await _factory.SeedUserWithPermissionsAsync(
"sod-out-worker",
"معالج",
[RoleCodes.FacilityCoordinator],
[],
(ScopeType.Facility, SeedIds.RegionA, SeedIds.FacilityA1));
await _factory.SeedUserAsync(
"sod-out-verifier",
"معتمد خارج النطاق",
[RoleCodes.FacilityDirector],
(ScopeType.Facility, SeedIds.RegionB, SeedIds.FacilityB1));
// Arrange a critical SoD note that belongs to RegionA / FacilityA1 and is pending verification
using var arrangeScope = _factory.Services.CreateScope();
var arrangeDb = arrangeScope.ServiceProvider.GetRequiredService<BaseeraDbContext>();
var worker = await arrangeDb.Users.SingleAsync(u => u.UserName == "sod-out-worker");
var admin = await arrangeDb.Users.SingleAsync(u => u.UserName == "sod-out-admin");
var note = new OperationalNote
{
Title = "Critical SoD note - out of scope verifier",
Description = "Integration test note for out-of-scope verifier closure attempt",
RegionId = SeedIds.RegionA,
FacilityId = SeedIds.FacilityA1,
Status = NoteStatus.PendingVerification,
CreatedByUserId = admin.Id,
AssignedProcessorUserId = worker.Id,
IsCritical = true,
};
arrangeDb.OperationalNotes.Add(note);
await arrangeDb.SaveChangesAsync();
// Act: attempt to close the note with the out-of-scope verifier
var client = await _factory.CreateClientForUserAsync("sod-out-verifier");
var closeRequest = new
{
noteId = note.Id,
status = "Closed",
reason = "Attempting closure from out-of-scope verifier",
};
var response = await client.PostAsJsonAsync($"/api/notes/{note.Id}/close", closeRequest);
// Assert: API denies access (404 or equivalent not-found for scoping)
Assert.Equal(HttpStatusCode.NotFound, response.StatusCode);
// Assert: database state has NOT been mutated
// The note must remain pending verification, with no closure status history or audit log entries.
using var verifyScope = _factory.Services.CreateScope();
var verifyDb = verifyScope.ServiceProvider.GetRequiredService<BaseeraDbContext>();
var entity = await verifyDb.OperationalNotes.SingleAsync(n => n.Id == note.Id);
Assert.Equal(NoteStatus.PendingVerification, entity.Status);
Assert.Equal(
0,
await verifyDb.NoteStatusHistories.CountAsync(
h => h.OperationalNoteId == note.Id && h.ToStatus == NoteStatus.Closed));
Assert.Equal(
0,
await verifyDb.AuditLogs.CountAsync(
a => a.EntityId == note.Id.ToString() && a.Action == "NoteClosed"));
```
1. If the existing implementation of `Independent_verifier_outside_scope_cannot_close_critical_note` already creates the note and calls the API using helper methods (similar to `Critical_note_all_processors_are_blocked_from_final_closure`), you should merge the DB assertion block at the end of the test instead of replacing the whole body. The key part to keep is:
- Creating a `verifyScope`, `verifyDb`
- Loading the same `note` used in the test
- Asserting `NoteStatus.PendingVerification`
- Asserting `NoteStatusHistories` and `AuditLogs` have **zero** `NoteClosed` entries.
2. Replace `CreateClientForUserAsync`, the route `"/api/notes/{note.Id}/close"`, and the request payload with whatever helpers and endpoints are already used in other closure tests (e.g. `Critical_note_all_processors_are_blocked_from_final_closure`) to keep consistency with the rest of the test suite.
3. Ensure `NoteStatus.PendingVerification`, `NoteStatusHistory`, and `AuditLogs` are the same entities and enums used in the other tests; adjust property names (`OperationalNoteId`, `Action`, etc.) if they differ in your actual model.
</issue_to_address>
### Comment 2
<location path="docs/phase-b1-completion-report.md" line_range="157" />
<code_context>
+
+### Tests
+
+- Unit: all-processors rejection, SystemAdmin, mutation-order, multi-user A→B→C, non-critical policy, assigner/creator/independent verifier success.
+- Integration: `Critical_note_all_processors_are_blocked_from_final_closure`, `Independent_verifier_outside_scope_cannot_close_critical_note`.
+
</code_context>
<issue_to_address>
**suggestion (typo):** توحيد تسمية الدور بين SystemAdmin و SystemAdministrator في المستندات.
في قسم الاختبارات هنا ذُكر الدور باسم "SystemAdmin"، بينما في أجزاء أخرى من التوثيق (مثل الجملة "SystemAdministrator does not bypass SoD") يُستخدم "SystemAdministrator". يُرجى توحيد الاسم المستخدم لهذا الدور في جميع المستندات.
Suggested implementation:
```
### Tests
- Unit: all-processors rejection, SystemAdministrator, mutation-order, multi-user A→B→C, non-critical policy, assigner/creator/independent verifier success.
- Integration: `Critical_note_all_processors_are_blocked_from_final_closure`, `Independent_verifier_outside_scope_cannot_close_critical_note`.
```
1. Search the entire `docs/phase-b1-completion-report.md` for any other occurrences of `SystemAdmin` and replace them with `SystemAdministrator` to keep the terminology consistent.
2. If there are cross-references from other documentation files (e.g., design docs or SoD policy docs) that still use `SystemAdmin`, consider updating them as well so all docs use `SystemAdministrator` uniformly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…ope. Co-authored-by: Cursor <cursoragent@cursor.com>
|
11 tasks
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
LastProcessedByUserId).bda6bdd).Problem
After User A
start-workand User Bsubmit-for-verification,LastProcessedByUserIdbecame B, so User A could stillverify-closuredespite processing participation.Solution
EnforceCriticalSoDAsyncqueries append-onlyNoteStatusHistoryfor:Assigned → InProgress/Reopened → InProgress(start-work)InProgress → PendingVerification(submit-for-verification)Excluded:
PendingVerification → InProgress(return-for-rework). Create/submit/assign/cancel/reopen/view are not processing. SoD runs before any mutation.Tests
Critical_note_all_processors_are_blocked_from_final_closure,Independent_verifier_outside_scope_cannot_close_critical_note(with no side-effect assertions).Docs
README, completion report (
Phase B.1 Accepted), test matrix, permissions matrix, state machine.Verification (tip
2d61411)Explicit non-goals
Phase B.2, notifications, jobs, dashboards, reports, corrective actions.
Do not merge until explicit acceptance.