test: comprehensive Application layer test coverage audit#37
Merged
Conversation
- Add partial unique index on (organization_id, name) for workflow_definitions where deleted_at IS NULL; catch UniqueConstraintException in Create/Update handlers - Add GetFormPickerQuery with handler and DTO for US-083 form-step picker - Update testing.md: mandate 3-path coverage (happy, not-found, constraint violation) - Update process.md: add mandatory Step 4.5 gap sweep before API layer - Update CLAUDE.md: add gap sweep to Priority Order pre-API checklist - Update feature file callouts for F01-US-047/051 and F03-US-083 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…coverage across all modules Adds explicit cross-org isolation tests to every Application handler that performs tenant-scoped lookups, ensuring security boundaries are tested by name rather than implicitly via NSubstitute default-null behaviour. Also fills missing not-found paths in RemoveField, ReorderFields, DeleteDataClass (DataModeling), CancelExecution failed-state guard (WorkflowEngine), and empty-sessions path (Identity). All 504 unit tests pass. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (22)
🚧 Files skipped from review as they are similar to previous changes (21)
📝 WalkthroughWalkthroughAdds a mandatory pre-API “gap sweep” governance step and integration-test requirements, implements FormBuilder’s GetFormPicker CQRS flow and DTO, enforces workflow (OrganizationId, Name) uniqueness via EF migration and partial index with handler-level conflict mapping, and expands org-isolation test coverage across modules. ChangesDocumentation and Process Governance
Form Picker Query Implementation
Workflow Name Uniqueness Constraint
Organization Isolation Test Coverage
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (5)
docs/playbooks/process.md-74-76 (1)
74-76:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSpecify the fenced block language here as well.
Without a language tag, markdownlint flags this block (MD040).
Suggested fix
-``` +```bash grep -r "Application: ⚠️\|Infrastructure: ⚠️" docs/epics/</details> <details> <summary>🤖 Prompt for AI Agents</summary>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/playbooks/process.mdaround lines 74 - 76, The fenced code block
containing the grep command lacks a language tag (triggers MD040); update the
block around the command grep -r "Application:⚠️ |Infrastructure:⚠️ "
docs/epics/ in docs/playbooks/process.md to include a language specifier (e.g.,
addbash before the block and close with), so markdownlint no longer
flags it.</details> </blockquote></details> <details> <summary>CLAUDE.md-393-395 (1)</summary><blockquote> `393-395`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Add a language identifier to this fenced code block.** This block is missing a fence language and will trigger MD040 in markdownlint. <details> <summary>Suggested fix</summary> ```diff -``` +```bash grep -r "Application: ⚠️\|Infrastructure: ⚠️" docs/epics/ ``` ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CLAUDE.md` around lines 393 - 395, The fenced code block containing the grep command (grep -r "Application: ⚠️\|Infrastructure: ⚠️" docs/epics/) is missing a language specifier and triggers markdownlint MD040; update that fenced block by adding a language identifier (e.g., bash) after the opening backticks so the block becomes a fenced code block with a language tag to satisfy MD040 and improve syntax highlighting. ``` </details> </blockquote></details> <details> <summary>tests/Modules/FormBuilder/Axis.FormBuilder.Application.Tests/Commands/ReorderFormFieldsHandlerTests.cs-57-57 (1)</summary><blockquote> `57-57`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Rename this test to satisfy the test naming rule.** Use `ReorderFormFields_WhenFormBelongsToAnotherOrg_ReturnsNotFound` instead of `Handle_When...`. As per coding guidelines "`tests/**/*.cs`: Test naming in .NET: `{Subject}_{Condition}_{ExpectedOutcome}`". <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Modules/FormBuilder/Axis.FormBuilder.Application.Tests/Commands/ReorderFormFieldsHandlerTests.cs` at line 57, Rename the test method Handle_WhenFormBelongsToAnotherOrg_ReturnsNotFound to follow the `{Subject}_{Condition}_{ExpectedOutcome}` rule: change its name to ReorderFormFields_WhenFormBelongsToAnotherOrg_ReturnsNotFound; update any references or attributes (e.g., the method declaration inside ReorderFormFieldsHandlerTests) so test frameworks and callers still find it. ``` </details> </blockquote></details> <details> <summary>tests/Modules/FormBuilder/Axis.FormBuilder.Application.Tests/Commands/RemoveFieldFromFormHandlerTests.cs-51-51 (1)</summary><blockquote> `51-51`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Rename this test to use a concrete subject name.** `Handle_When...` should follow the required `{Subject}_{Condition}_{ExpectedOutcome}` form (e.g., `RemoveFieldFromForm_WhenFormBelongsToAnotherOrg_ReturnsNotFound`). As per coding guidelines "`tests/**/*.cs`: Test naming in .NET: `{Subject}_{Condition}_{ExpectedOutcome}`". <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Modules/FormBuilder/Axis.FormBuilder.Application.Tests/Commands/RemoveFieldFromFormHandlerTests.cs` at line 51, Rename the test method Handle_WhenFormBelongsToAnotherOrg_ReturnsNotFound in the RemoveFieldFromFormHandlerTests class to follow the {Subject}_{Condition}_{ExpectedOutcome} pattern, e.g. RemoveFieldFromForm_WhenFormBelongsToAnotherOrg_ReturnsNotFound; update the method declaration and any references/usages (test runner attributes or call-sites) to use the new name so the test framework still discovers and runs it. ``` </details> </blockquote></details> <details> <summary>tests/Modules/FormBuilder/Axis.FormBuilder.Application.Tests/Commands/AddFieldToFormHandlerTests.cs-67-67 (1)</summary><blockquote> `67-67`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Rename test to match the required test naming convention.** Use a subject-specific prefix instead of `Handle_` (e.g., `AddFieldToForm_WhenFormBelongsToAnotherOrg_ReturnsNotFound`). As per coding guidelines "`tests/**/*.cs`: Test naming in .NET: `{Subject}_{Condition}_{ExpectedOutcome}`". <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Modules/FormBuilder/Axis.FormBuilder.Application.Tests/Commands/AddFieldToFormHandlerTests.cs` at line 67, Rename the test method that currently reads Handle_WhenFormBelongsToAnotherOrg_ReturnsNotFound to follow the {Subject}_{Condition}_{ExpectedOutcome} convention; change its name to AddFieldToForm_WhenFormBelongsToAnotherOrg_ReturnsNotFound (and update any references/usages such as test attributes or test runner expectations) so the subject reflects the command handler under test instead of the generic "Handle_". ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (4)</summary><blockquote> <details> <summary>src/Modules/FormBuilder/Axis.FormBuilder.Application/Queries/GetFormPicker/GetFormPickerHandler.cs (1)</summary><blockquote> `16-19`: _💤 Low value_ **Consider moving ordering to the database layer.** The current implementation fetches all forms and then orders them in memory. For better performance and scalability, ordering by `Name` should ideally be done at the database level within the repository method. However, for a picker with a small number of forms, the current approach is acceptable. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Modules/FormBuilder/Axis.FormBuilder.Application/Queries/GetFormPicker/GetFormPickerHandler.cs` around lines 16 - 19, GetFormPickerHandler is ordering the results in-memory (the call that does all.OrderBy(...).Select(...).ToList()); move the OrderBy(f => f.Name) into the repository/query that produces 'all' so the database performs sorting (e.g., add OrderBy on Name in the repository method that returns forms such as GetAll/GetForms/QueryForms), then update GetFormPickerHandler to project the already-ordered results into GetFormPickerDto without calling OrderBy in memory. ``` </details> </blockquote></details> <details> <summary>tests/Modules/WorkflowEngine/Axis.WorkflowEngine.Application.Tests/Queries/GetExecutionHandlerTests.cs (1)</summary><blockquote> `51-62`: _⚡ Quick win_ **Assert the repository call uses the mismatched organization id.** Add an interaction assertion so this test verifies org propagation, not only null output. <details> <summary>Proposed assertion</summary> ```diff Guid otherOrgId = Guid.NewGuid(); ExecutionResponse? result = await CreateHandler().Handle( new GetExecutionQuery(ExecId, otherOrgId), CancellationToken.None); result.Should().BeNull(); + await _execRepo.Received(1).GetWithStepsAsync(ExecId, otherOrgId); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Modules/WorkflowEngine/Axis.WorkflowEngine.Application.Tests/Queries/GetExecutionHandlerTests.cs` around lines 51 - 62, The test GetExecution_WhenExecutionBelongsToAnotherOrg_ReturnsNull currently only asserts the result is null; add an interaction assertion that the repository was called with the mismatched org id so org propagation is verified: after calling CreateHandler().Handle(...), assert that _execRepo.Received(1).GetWithStepsAsync(ExecId, otherOrgId) (or the equivalent Received() assertion for NSubstitute) was invoked, referencing the test method name GetExecution_WhenExecutionBelongsToAnotherOrg_ReturnsNull and the repository method GetWithStepsAsync to locate the code. ``` </details> </blockquote></details> <details> <summary>tests/Modules/WorkflowEngine/Axis.WorkflowEngine.Application.Tests/Queries/GetRetryHistoryHandlerTests.cs (1)</summary><blockquote> `51-62`: _⚡ Quick win_ **Make wrong-org behavior explicit instead of relying on unmatched substitute defaults.** Configure the wrong-org call explicitly and assert that exact call happened; this makes the isolation contract clearer and less brittle. <details> <summary>Proposed test hardening</summary> ```diff List<ExecutionSummaryResponse> retries = [BuildSummary(OriginalExecId)]; _execRepo.GetRetriesAsync(OriginalExecId, OrgId).Returns(retries); Guid otherOrgId = Guid.NewGuid(); + _execRepo.GetRetriesAsync(OriginalExecId, otherOrgId) + .Returns(new List<ExecutionSummaryResponse>()); IReadOnlyList<ExecutionSummaryResponse> result = await CreateHandler().Handle( new GetRetryHistoryQuery(OriginalExecId, otherOrgId), CancellationToken.None); result.Should().BeEmpty(); + await _execRepo.Received(1).GetRetriesAsync(OriginalExecId, otherOrgId); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Modules/WorkflowEngine/Axis.WorkflowEngine.Application.Tests/Queries/GetRetryHistoryHandlerTests.cs` around lines 51 - 62, The test GetRetryHistory_WhenQueriedWithWrongOrg_ReturnsEmptyList relies on default substitute behavior; explicitly configure the wrong-org call and assert it was invoked: set _execRepo.GetRetriesAsync(OriginalExecId, otherOrgId).Returns(new List<ExecutionSummaryResponse>()), call CreateHandler().Handle(new GetRetryHistoryQuery(OriginalExecId, otherOrgId), ...), assert result.Should().BeEmpty(), and add a substitute assertion _execRepo.Received(1).GetRetriesAsync(OriginalExecId, otherOrgId) to make the isolation contract explicit. ``` </details> </blockquote></details> <details> <summary>tests/Modules/WorkflowEngine/Axis.WorkflowEngine.Application.Tests/Commands/RetryExecutionWithContextHandlerTests.cs (1)</summary><blockquote> `86-99`: _⚡ Quick win_ **Strengthen this cross-org test with explicit interaction assertions.** The test currently checks only the final result. Add assertions that the handler queried with `otherOrgId` and performed no writes, so tenant isolation is verified explicitly. <details> <summary>Proposed test hardening</summary> ```diff Guid otherOrgId = Guid.NewGuid(); Result<Guid> result = await CreateHandler().Handle( new RetryExecutionWithContextCommand(failed.Id, otherOrgId, UserId, ModifiedContext), CancellationToken.None); result.IsFailure.Should().BeTrue(); result.ErrorCode.Should().Be(ErrorCodes.NotFound); + await _execRepo.Received(1).GetByIdAsync(failed.Id, otherOrgId); + await _execRepo.DidNotReceive().AddAsync( + Arg.Any<WorkflowExecution>(), + Arg.Any<CancellationToken>()); + await _uow.DidNotReceive().SaveChangesAsync(Arg.Any<CancellationToken>()); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Modules/WorkflowEngine/Axis.WorkflowEngine.Application.Tests/Commands/RetryExecutionWithContextHandlerTests.cs` around lines 86 - 99, The test must also assert repository interactions to prove tenant isolation: after calling CreateHandler().Handle(...) verify _execRepo.Received(1).GetByIdAsync(failed.Id, otherOrgId) was invoked and assert no write methods were called (e.g. _execRepo.DidNotReceiveWithAnyArgs().UpdateAsync(Arg.Any<...>()), DidNotReceiveWithAnyArgs().AddAsync(...), or any Save/Commit equivalent) so RetryExecutionWithContextHandler did not perform writes when orgs differ. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>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
@src/Modules/FormBuilder/Axis.FormBuilder.Application/Queries/GetFormPicker/GetFormPickerHandler.cs:
- Around line 14-19: GetFormPickerHandler projects f.Fields.Count but
FormRepository's GetAllAsync doesn't eagerly load the Fields navigation, causing
runtime errors or N+1 queries; update the repository implementation of
GetAllAsync (in the FormRepository class) to Include(f => f.Fields) on the
FormDefinitions query (and OrderBy/ToListAsync as appropriate) so returned
FormDefinition instances have their Fields populated, and add the necessary EF
Core using (Microsoft.EntityFrameworkCore) if missing.In
@tests/Modules/DataModeling/Axis.DataModeling.Application.Tests/Commands/UpdateFieldHandlerTests.cs:
- Around line 55-69: The test currently stubs _modelRepo.GetByIdAsync(model.Id,
OrgId) which allows a false-positive NotFound; change the stub to
_modelRepo.GetByIdAsync(model.Id, otherOrgId). Then add an assertion that the
repo did not persist changes (e.g. _modelRepo.DidNotReceive().UpdateAsync(...)
or DidNotReceiveWithAnyArgs for the repository method that saves the model) to
ensure the handler queried with otherOrgId and did not call the update/save
method.In
@tests/Modules/DataModeling/Axis.DataModeling.Application.Tests/Commands/UpdateModelHandlerTests.cs:
- Around line 53-67: The test currently stubs _modelRepo.GetByIdAsync(model.Id,
OrgId) which can hide whether the handler actually uses the command's org ID;
update the test so the repository returns the model only when called with the
original OrgId and returns null (or is not stubbed) when called with the
command's otherOrgId, then assert the handler invoked _modelRepo.GetByIdAsync
with the command's org ID (otherOrgId) — reference the test
UpdateModel_WhenModelBelongsToAnotherOrg_ReturnsNotFound, the call to
_modelRepo.GetByIdAsync, the CreateHandler() usage, and the UpdateModelCommand
to locate where to change the stubs and add a Received/verification for the
org-id argument.In
@tests/Modules/DataModeling/Axis.DataModeling.Application.Tests/Commands/UpdateRecordHandlerTests.cs:
- Around line 74-87: The test should explicitly assert org-scoped repo usage:
instead of relying on NSubstitute defaults, set
_modelRepo.GetByIdAsync(model.Id, Arg.Is(g => g ==
otherOrgId)).Returns((DataModel?)null) and then after calling
CreateHandler().Handle(new UpdateRecordCommand(... otherOrgId ...), ...) verify
the repository was called with that org by adding
_modelRepo.Received(1).GetByIdAsync(model.Id, otherOrgId); this ensures
UpdateRecord_WhenModelBelongsToAnotherOrg_ReturnsNotFound actually exercises org
scoping rather than relying on null defaults from _modelRepo.GetByIdAsync or the
original OrgId setup.In
@tests/Modules/DataModeling/Axis.DataModeling.Application.Tests/Queries/GetRecordsHandlerTests.cs:
- Around line 71-87: The test must also assert that the model repository was
queried with the other organization id to ensure the handler used the provided
org id; add an assertion that _modelRepo.Received() (or equivalent) was called
for GetByIdAsync with ModelId and otherOrgId inside
GetRecords_WhenModelBelongsToAnotherOrg_ReturnsNotFound so the test fails if the
wrong org argument is used (keep the existing GetPagedAsync DidNotReceive()
assertion).In
@tests/Modules/FormBuilder/Axis.FormBuilder.Application.Tests/Commands/AddFieldToFormHandlerTests.cs:
- Around line 66-79: The test currently allows _repo.GetByIdAsync to return the
form regardless of the org argument; update the test to assert and enforce the
org argument: configure the substitute so GetByIdAsync returns the form only
when called with the original OrgId (e.g., _repo.GetByIdAsync(form.Id, OrgId,
Arg.Any()).Returns(form)) and ensure calls with the
otherOrgId return null, then add a Received/DidNotReceive verification on
_repo.GetByIdAsync to confirm the handler was invoked with the otherOrgId (or
alternatively assert it was not invoked with OrgId) when calling
_handler.Handle(new AddFieldToFormCommand(...)); this hardens the isolation
around AddFieldToFormCommand and _handler.Handle behavior.In
@tests/Modules/FormBuilder/Axis.FormBuilder.Application.Tests/Commands/DeleteFormHandlerTests.cs:
- Around line 63-75: The test currently stubs _formRepo.GetByIdAsync(form.Id,
OrgId).Returns(form) but never asserts that the handler actually called
GetByIdAsync with the otherOrgId, so the test can pass without exercising
org-isolation; update the test
(DeleteForm_WhenFormBelongsToAnotherOrg_ReturnsNotFound) to keep the existing
stub for OrgId but after calling CreateHandler().Handle(...) assert that
_formRepo.Received(1).GetByIdAsync(form.Id, otherOrgId) (and optionally assert
_formRepo.DidNotReceive().GetByIdAsync(form.Id, OrgId)) so the test verifies the
handler queried the repository with the supplied otherOrgId when handling
DeleteFormCommand.In
@tests/Modules/FormBuilder/Axis.FormBuilder.Application.Tests/Commands/RemoveFieldFromFormHandlerTests.cs:
- Around line 50-63: The test currently stubs _repo.GetByIdAsync(form.Id, OrgId,
...) which lets the handler call with a different org succeed via unstubbed/null
behavior; change the arrange so the repository explicitly returns null when
looked up with the otherOrgId used by the command and add an assertion that
_repo.GetByIdAsync was invoked with form.Id and that otherOrgId. Concretely,
update the setup to return the form only for OrgId (or return null for
otherOrgId) and add _repo.Received(1).GetByIdAsync(form.Id, otherOrgId,
Arg.Any()) so the test verifies the org-specific lookup path
when calling RemoveFieldFromFormCommand.In
@tests/Modules/FormBuilder/Axis.FormBuilder.Application.Tests/Commands/ReorderFormFieldsHandlerTests.cs:
- Around line 56-70: The test should explicitly verify repository behavior for
the org argument: change the _repo.GetByIdAsync setup so it only returns the
form when called with the expected OrgId and ensure it returns null for
otherOrgId; for example, stub _repo.GetByIdAsync(form.Id, OrgId,
Arg.Any()) to return the form and stub
_repo.GetByIdAsync(form.Id, otherOrgId, Arg.Any()) to return
null (or use Arg.Is(g => g == OrgId) to scope the positive case), and
optionally assert that _repo.GetByIdAsync was invoked with the otherOrgId when
calling _handler.Handle(new ReorderFormFieldsCommand(..., otherOrgId, ...)) so
the test truly verifies org-specific behavior in
Handle_WhenFormBelongsToAnotherOrg_ReturnsNotFound and the
ReorderFormFieldsCommand flow.In
@tests/Modules/WorkflowBuilder/Axis.WorkflowBuilder.Application.Tests/AddStepHandlerTests.cs:
- Around line 52-56: The test currently stubs _repo.GetByIdAsync for OrgId but
invokes _handler.Handle with otherOrgId, allowing NSubstitute to return null for
the unmatched call; update the test to explicitly stub/verify the repository
call for the otherOrgId path (e.g., set _repo.GetByIdAsync(wf.Id, otherOrgId,
Arg.Any()).Returns(wf) or assert that _repo.GetByIdAsync was
called with otherOrgId after invoking _handler.Handle) so the test verifies
org-isolation and argument forwarding for AddStepCommand and the handler logic.In
@tests/Modules/WorkflowBuilder/Axis.WorkflowBuilder.Application.Tests/AddTransitionHandlerTests.cs:
- Around line 57-61: The test currently arranges _repo.GetByIdAsync(wf.Id,
OrgId, ...) but calls _handler.Handle with otherOrgId, which lets the test pass
via unmatched-call defaults; update the arrange to explicitly stub
_repo.GetByIdAsync(wf.Id, otherOrgId, Arg.Any()) to return wf
(or appropriate failure) and add a verification/assertion that
_repo.GetByIdAsync was called with wf.Id and otherOrgId; locate the calls around
_repo.GetByIdAsync, AddTransitionCommand, _handler.Handle and the
OrgId/otherOrgId variables to make this change.In
@tests/Modules/WorkflowBuilder/Axis.WorkflowBuilder.Application.Tests/AddTriggerHandlerTests.cs:
- Around line 51-55: The test currently only stubs _repo.GetByIdAsync for OrgId
and relies on NSubstitute defaults for otherOrgId; update the setup to
explicitly stub _repo.GetByIdAsync(wf.Id, otherOrgId,
Arg.Any()) to return wf, call _handler.Handle(new
AddTriggerCommand(wf.Id, otherOrgId, TriggerType.Manual, null), ...), and then
assert that _repo.Received(1).GetByIdAsync(wf.Id, otherOrgId,
Arg.Any()) was invoked so the org-isolation behavior is
explicitly verified (referencing _repo.GetByIdAsync, otherOrgId,
_handler.Handle, and AddTriggerCommand).In
@tests/Modules/WorkflowBuilder/Axis.WorkflowBuilder.Application.Tests/ArchiveWorkflowHandlerTests.cs:
- Around line 52-56: Test currently stubs _repo.GetByIdAsync for OrgId but calls
_handler.Handle with otherOrgId, allowing a false positive; update the test to
explicitly stub _repo.GetByIdAsync(wf.Id, otherOrgId,
Arg.Any()) to return null (or a NotFound result) so the
handler truly hits the failure path, then add a verification like
_repo.Received(1).GetByIdAsync(wf.Id, otherOrgId, Arg.Any())
to assert the org argument was forwarded (use the existing symbols:
_repo.GetByIdAsync, _handler.Handle, ArchiveWorkflowCommand, OrgId, otherOrgId,
Received).In
@tests/Modules/WorkflowBuilder/Axis.WorkflowBuilder.Application.Tests/ConfigureStepHandlerTests.cs:
- Around line 56-60: The test currently relies on a default substitute and
doesn't verify repository scoping by otherOrgId; update the test to explicitly
stub _repo.GetByIdAsync for the combination (wf.Id, otherOrgId,
Arg.Any()) to return null or a not-found result, then call
_handler.Handle(new ConfigureStepCommand(wf.Id, otherOrgId, step.Id, "Name",
null), ...) and assert that _repo.Received(1).GetByIdAsync(wf.Id, otherOrgId,
Arg.Any()) was invoked so the failure proves the call used
otherOrgId rather than an unmatched-call default.In
@tests/Modules/WorkflowBuilder/Axis.WorkflowBuilder.Application.Tests/DuplicateWorkflowHandlerTests.cs:
- Around line 70-74: The test currently only stubs _repo.GetByIdAsync for OrgId,
so the cross-org call can succeed due to NSubstitute defaults; explicitly stub
the repository for the otherOrgId case and assert it was invoked. Add a setup
for _repo.GetByIdAsync(wf.Id, otherOrgId, Arg.Any()) to
return null/NotFound (so the handler sees no workflow in the other org) and add
a Received() verification on _repo.GetByIdAsync(wf.Id, otherOrgId,
Arg.Any()) after calling _handler.Handle(new
DuplicateWorkflowCommand(...), CancellationToken.None) to ensure the cross-org
lookup was performed.In
@tests/Modules/WorkflowBuilder/Axis.WorkflowBuilder.Application.Tests/ExportWorkflowHandlerTests.cs:
- Around line 117-121: The test currently stubs _repo.GetByIdAsync(wf.Id, OrgId,
...) which allows the cross-org call to fall back to unmatched defaults;
instead, update the stub to expect the otherOrgId used in the query (call
_repo.GetByIdAsync with wf.Id and otherOrgId) and then assert that
_repo.GetByIdAsync was invoked with those exact arguments when calling
_handler.Handle(new ExportWorkflowQuery(wf.Id, otherOrgId), ...); specifically
modify the setup for GetByIdAsync and add a verification/assertion that the repo
was called with wf.Id and otherOrgId (and the cancellation token) so the test
explicitly enforces org scoping.In
@tests/Modules/WorkflowBuilder/Axis.WorkflowBuilder.Application.Tests/GetWorkflowHandlerTests.cs:
- Around line 55-59: The test currently stubs _repo.GetByIdAsync(wf.Id, OrgId,
...) which can mask whether the handler forwards the supplied organization;
change the setup to explicitly stub _repo.GetByIdAsync(wf.Id, otherOrgId,
Arg.Any()) and then assert that _repo.GetByIdAsync was
invoked with wf.Id and otherOrgId (e.g., Received(1) or equivalent) after
calling _handler.Handle(new GetWorkflowQuery(wf.Id, otherOrgId), ...). This
ensures GetWorkflowQuery -> _handler.Handle properly forwards otherOrgId to
_repo.GetByIdAsync rather than relying on a looser stub.
Minor comments:
In@CLAUDE.md:
- Around line 393-395: The fenced code block containing the grep command (grep
-r "Application:⚠️ |Infrastructure:⚠️ " docs/epics/) is missing a language
specifier and triggers markdownlint MD040; update that fenced block by adding a
language identifier (e.g., bash) after the opening backticks so the block
becomes a fenced code block with a language tag to satisfy MD040 and improve
syntax highlighting.In
@docs/playbooks/process.md:
- Around line 74-76: The fenced code block containing the grep command lacks a
language tag (triggers MD040); update the block around the command grep -r
"Application:⚠️ |Infrastructure:⚠️ " docs/epics/ in docs/playbooks/process.md
to include a language specifier (e.g., addbash before the block and close with), so markdownlint no longer flags it.In
@tests/Modules/FormBuilder/Axis.FormBuilder.Application.Tests/Commands/AddFieldToFormHandlerTests.cs:
- Line 67: Rename the test method that currently reads
Handle_WhenFormBelongsToAnotherOrg_ReturnsNotFound to follow the
{Subject}{Condition}{ExpectedOutcome} convention; change its name to
AddFieldToForm_WhenFormBelongsToAnotherOrg_ReturnsNotFound (and update any
references/usages such as test attributes or test runner expectations) so the
subject reflects the command handler under test instead of the generic
"Handle_".In
@tests/Modules/FormBuilder/Axis.FormBuilder.Application.Tests/Commands/RemoveFieldFromFormHandlerTests.cs:
- Line 51: Rename the test method
Handle_WhenFormBelongsToAnotherOrg_ReturnsNotFound in the
RemoveFieldFromFormHandlerTests class to follow the
{Subject}{Condition}{ExpectedOutcome} pattern, e.g.
RemoveFieldFromForm_WhenFormBelongsToAnotherOrg_ReturnsNotFound; update the
method declaration and any references/usages (test runner attributes or
call-sites) to use the new name so the test framework still discovers and runs
it.In
@tests/Modules/FormBuilder/Axis.FormBuilder.Application.Tests/Commands/ReorderFormFieldsHandlerTests.cs:
- Line 57: Rename the test method
Handle_WhenFormBelongsToAnotherOrg_ReturnsNotFound to follow the
{Subject}_{Condition}_{ExpectedOutcome}rule: change its name to
ReorderFormFields_WhenFormBelongsToAnotherOrg_ReturnsNotFound; update any
references or attributes (e.g., the method declaration inside
ReorderFormFieldsHandlerTests) so test frameworks and callers still find it.
Nitpick comments:
In
@src/Modules/FormBuilder/Axis.FormBuilder.Application/Queries/GetFormPicker/GetFormPickerHandler.cs:
- Around line 16-19: GetFormPickerHandler is ordering the results in-memory (the
call that does all.OrderBy(...).Select(...).ToList()); move the OrderBy(f =>
f.Name) into the repository/query that produces 'all' so the database performs
sorting (e.g., add OrderBy on Name in the repository method that returns forms
such as GetAll/GetForms/QueryForms), then update GetFormPickerHandler to project
the already-ordered results into GetFormPickerDto without calling OrderBy in
memory.In
@tests/Modules/WorkflowEngine/Axis.WorkflowEngine.Application.Tests/Commands/RetryExecutionWithContextHandlerTests.cs:
- Around line 86-99: The test must also assert repository interactions to prove
tenant isolation: after calling CreateHandler().Handle(...) verify
_execRepo.Received(1).GetByIdAsync(failed.Id, otherOrgId) was invoked and assert
no write methods were called (e.g.
_execRepo.DidNotReceiveWithAnyArgs().UpdateAsync(Arg.Any<...>()),
DidNotReceiveWithAnyArgs().AddAsync(...), or any Save/Commit equivalent) so
RetryExecutionWithContextHandler did not perform writes when orgs differ.In
@tests/Modules/WorkflowEngine/Axis.WorkflowEngine.Application.Tests/Queries/GetExecutionHandlerTests.cs:
- Around line 51-62: The test
GetExecution_WhenExecutionBelongsToAnotherOrg_ReturnsNull currently only asserts
the result is null; add an interaction assertion that the repository was called
with the mismatched org id so org propagation is verified: after calling
CreateHandler().Handle(...), assert that
_execRepo.Received(1).GetWithStepsAsync(ExecId, otherOrgId) (or the equivalent
Received() assertion for NSubstitute) was invoked, referencing the test method
name GetExecution_WhenExecutionBelongsToAnotherOrg_ReturnsNull and the
repository method GetWithStepsAsync to locate the code.In
@tests/Modules/WorkflowEngine/Axis.WorkflowEngine.Application.Tests/Queries/GetRetryHistoryHandlerTests.cs:
- Around line 51-62: The test
GetRetryHistory_WhenQueriedWithWrongOrg_ReturnsEmptyList relies on default
substitute behavior; explicitly configure the wrong-org call and assert it was
invoked: set _execRepo.GetRetriesAsync(OriginalExecId, otherOrgId).Returns(new
List()), call CreateHandler().Handle(new
GetRetryHistoryQuery(OriginalExecId, otherOrgId), ...), assert
result.Should().BeEmpty(), and add a substitute assertion
_execRepo.Received(1).GetRetriesAsync(OriginalExecId, otherOrgId) to make the
isolation contract explicit.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro Plus **Run ID**: `9aea1686-499d-4528-a34b-cedd641118f7` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 9fcfbdf96129d06e43251a77b6a805f9eba6fc06 and aea69ffdb6c82545bcd46fa8d7f1d57d3d6ba2b1. </details> <details> <summary>📒 Files selected for processing (57)</summary> * `CLAUDE.md` * `docs/epics/E04-workflow-builder/features/F01-workflow-definition.md` * `docs/epics/E05-form-builder/features/F03-workflow-integration.md` * `docs/playbooks/process.md` * `docs/playbooks/testing.md` * `src/Modules/FormBuilder/Axis.FormBuilder.Application/Queries/GetFormPicker/GetFormPickerDto.cs` * `src/Modules/FormBuilder/Axis.FormBuilder.Application/Queries/GetFormPicker/GetFormPickerHandler.cs` * `src/Modules/FormBuilder/Axis.FormBuilder.Application/Queries/GetFormPicker/GetFormPickerQuery.cs` * `src/Modules/WorkflowBuilder/Axis.WorkflowBuilder.Application/Commands/CreateWorkflow/CreateWorkflowHandler.cs` * `src/Modules/WorkflowBuilder/Axis.WorkflowBuilder.Application/Commands/UpdateWorkflow/UpdateWorkflowHandler.cs` * `src/Modules/WorkflowBuilder/Axis.WorkflowBuilder.Infrastructure/Migrations/20260518034752_AddUniqueConstraintOnWorkflowName.Designer.cs` * `src/Modules/WorkflowBuilder/Axis.WorkflowBuilder.Infrastructure/Migrations/20260518034752_AddUniqueConstraintOnWorkflowName.cs` * `src/Modules/WorkflowBuilder/Axis.WorkflowBuilder.Infrastructure/Migrations/WorkflowBuilderDbContextModelSnapshot.cs` * `src/Modules/WorkflowBuilder/Axis.WorkflowBuilder.Infrastructure/Persistence/Configurations/WorkflowDefinitionConfiguration.cs` * `tests/Modules/DataModeling/Axis.DataModeling.Application.Tests/Commands/AddFieldHandlerTests.cs` * `tests/Modules/DataModeling/Axis.DataModeling.Application.Tests/Commands/BulkDeleteRecordsHandlerTests.cs` * `tests/Modules/DataModeling/Axis.DataModeling.Application.Tests/Commands/CreateRecordHandlerTests.cs` * `tests/Modules/DataModeling/Axis.DataModeling.Application.Tests/Commands/DeleteDataClassHandlerTests.cs` * `tests/Modules/DataModeling/Axis.DataModeling.Application.Tests/Commands/DeleteModelHandlerTests.cs` * `tests/Modules/DataModeling/Axis.DataModeling.Application.Tests/Commands/DeleteRecordHandlerTests.cs` * `tests/Modules/DataModeling/Axis.DataModeling.Application.Tests/Commands/RemoveFieldHandlerTests.cs` * `tests/Modules/DataModeling/Axis.DataModeling.Application.Tests/Commands/ReorderFieldsHandlerTests.cs` * `tests/Modules/DataModeling/Axis.DataModeling.Application.Tests/Commands/UpdateFieldHandlerTests.cs` * `tests/Modules/DataModeling/Axis.DataModeling.Application.Tests/Commands/UpdateModelHandlerTests.cs` * `tests/Modules/DataModeling/Axis.DataModeling.Application.Tests/Commands/UpdateRecordHandlerTests.cs` * `tests/Modules/DataModeling/Axis.DataModeling.Application.Tests/Queries/GetRecordsHandlerTests.cs` * `tests/Modules/FormBuilder/Axis.FormBuilder.Application.Tests/Commands/AddFieldToFormHandlerTests.cs` * `tests/Modules/FormBuilder/Axis.FormBuilder.Application.Tests/Commands/DeleteFormHandlerTests.cs` * `tests/Modules/FormBuilder/Axis.FormBuilder.Application.Tests/Commands/RemoveFieldFromFormHandlerTests.cs` * `tests/Modules/FormBuilder/Axis.FormBuilder.Application.Tests/Commands/ReorderFormFieldsHandlerTests.cs` * `tests/Modules/FormBuilder/Axis.FormBuilder.Application.Tests/Commands/UpdateFormHandlerTests.cs` * `tests/Modules/FormBuilder/Axis.FormBuilder.Application.Tests/Queries/GetFormByIdHandlerTests.cs` * `tests/Modules/Identity/Axis.Identity.Application.Tests/Commands/AssignRoleToUserHandlerTests.cs` * `tests/Modules/Identity/Axis.Identity.Application.Tests/Commands/ChangePasswordHandlerTests.cs` * `tests/Modules/Identity/Axis.Identity.Application.Tests/Commands/DeactivateUserHandlerTests.cs` * `tests/Modules/Identity/Axis.Identity.Application.Tests/Commands/UpdateRoleHandlerTests.cs` * `tests/Modules/Identity/Axis.Identity.Application.Tests/Commands/UpdateUserProfileHandlerTests.cs` * `tests/Modules/Identity/Axis.Identity.Application.Tests/Queries/GetUserSessionsHandlerTests.cs` * `tests/Modules/WorkflowBuilder/Axis.WorkflowBuilder.Application.Tests/AddStepHandlerTests.cs` * `tests/Modules/WorkflowBuilder/Axis.WorkflowBuilder.Application.Tests/AddTransitionHandlerTests.cs` * `tests/Modules/WorkflowBuilder/Axis.WorkflowBuilder.Application.Tests/AddTriggerHandlerTests.cs` * `tests/Modules/WorkflowBuilder/Axis.WorkflowBuilder.Application.Tests/ArchiveWorkflowHandlerTests.cs` * `tests/Modules/WorkflowBuilder/Axis.WorkflowBuilder.Application.Tests/Commands/PublishWorkflowHandlerTests.cs` * `tests/Modules/WorkflowBuilder/Axis.WorkflowBuilder.Application.Tests/ConfigureStepHandlerTests.cs` * `tests/Modules/WorkflowBuilder/Axis.WorkflowBuilder.Application.Tests/DuplicateWorkflowHandlerTests.cs` * `tests/Modules/WorkflowBuilder/Axis.WorkflowBuilder.Application.Tests/ExportWorkflowHandlerTests.cs` * `tests/Modules/WorkflowBuilder/Axis.WorkflowBuilder.Application.Tests/GetWorkflowHandlerTests.cs` * `tests/Modules/WorkflowBuilder/Axis.WorkflowBuilder.Application.Tests/RemoveStepHandlerTests.cs` * `tests/Modules/WorkflowBuilder/Axis.WorkflowBuilder.Application.Tests/RemoveTransitionHandlerTests.cs` * `tests/Modules/WorkflowBuilder/Axis.WorkflowBuilder.Application.Tests/RemoveTriggerHandlerTests.cs` * `tests/Modules/WorkflowBuilder/Axis.WorkflowBuilder.Application.Tests/UnarchiveWorkflowHandlerTests.cs` * `tests/Modules/WorkflowBuilder/Axis.WorkflowBuilder.Application.Tests/UpdateWorkflowHandlerTests.cs` * `tests/Modules/WorkflowEngine/Axis.WorkflowEngine.Application.Tests/Commands/CancelExecutionHandlerTests.cs` * `tests/Modules/WorkflowEngine/Axis.WorkflowEngine.Application.Tests/Commands/RetryExecutionHandlerTests.cs` * `tests/Modules/WorkflowEngine/Axis.WorkflowEngine.Application.Tests/Commands/RetryExecutionWithContextHandlerTests.cs` * `tests/Modules/WorkflowEngine/Axis.WorkflowEngine.Application.Tests/Queries/GetExecutionHandlerTests.cs` * `tests/Modules/WorkflowEngine/Axis.WorkflowEngine.Application.Tests/Queries/GetRetryHistoryHandlerTests.cs` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Comment on lines
+14
to
+19
| IReadOnlyList<FormDefinition> all = await formRepo.GetAllAsync(query.OrganizationId, cancellationToken); | ||
|
|
||
| return all | ||
| .OrderBy(f => f.Name) | ||
| .Select(f => new GetFormPickerDto(f.Id, f.Name, f.Fields.Count)) | ||
| .ToList(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if GetAllAsync in IFormRepository or FormRepository includes Fields
# Search for GetAllAsync implementation in FormRepository
ast-grep --pattern $'class FormRepository$$$GetAllAsync($$$) {
$$$
}'
# Also search for Include statements in the repository
rg -n -A 10 -B 2 'GetAllAsync.*OrganizationId' --type cs -g '*FormRepository.cs'Repository: phuong-labs/axis
Length of output: 42
🏁 Script executed:
fd -t f -i "formrepository" --extension csRepository: phuong-labs/axis
Length of output: 332
🏁 Script executed:
fd -t f "GetFormPickerHandler" --extension csRepository: phuong-labs/axis
Length of output: 159
🏁 Script executed:
fd -t d "FormBuilder" | head -20Repository: phuong-labs/axis
Length of output: 456
🏁 Script executed:
cat -n src/Modules/FormBuilder/Axis.FormBuilder.Infrastructure/Repositories/FormRepository.csRepository: phuong-labs/axis
Length of output: 2203
🏁 Script executed:
cat -n src/Modules/FormBuilder/Axis.FormBuilder.Application/Repositories/IFormRepository.csRepository: phuong-labs/axis
Length of output: 885
🏁 Script executed:
cat -n src/Modules/FormBuilder/Axis.FormBuilder.Application/Queries/GetFormPicker/GetFormPickerHandler.csRepository: phuong-labs/axis
Length of output: 1070
Add .Include(f => f.Fields) to GetAllAsync query in FormRepository.
Line 18 accesses f.Fields.Count, but GetAllAsync does not eagerly load the Fields collection. The query must include the Fields navigation property:
Suggested fix
public async Task<IReadOnlyList<FormDefinition>> GetAllAsync(Guid organizationId, CancellationToken ct = default)
=> await context.FormDefinitions
.Where(f => f.OrganizationId == organizationId)
.Include(f => f.Fields)
.OrderBy(f => f.Name)
.ToListAsync(ct);Without this, the code will throw an exception at runtime (unloaded navigation property) or cause N+1 queries if lazy loading is enabled.
🤖 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
`@src/Modules/FormBuilder/Axis.FormBuilder.Application/Queries/GetFormPicker/GetFormPickerHandler.cs`
around lines 14 - 19, GetFormPickerHandler projects f.Fields.Count but
FormRepository's GetAllAsync doesn't eagerly load the Fields navigation, causing
runtime errors or N+1 queries; update the repository implementation of
GetAllAsync (in the FormRepository class) to Include(f => f.Fields) on the
FormDefinitions query (and OrderBy/ToListAsync as appropriate) so returned
FormDefinition instances have their Fields populated, and add the necessary EF
Core using (Microsoft.EntityFrameworkCore) if missing.
Adds missing test coverage for repository methods that were used by Application handlers but had no integration test: Identity: - UserRepository: GetByIdAsync cross-org isolation, FindByEmailGloballyAsync (exists + not found), GetByIdPlatformWideAsync (exists + not found) - RoleRepository: GetByIdAsync cross-org isolation, GetByIdsAsync (matching + cross-org), GetPagedAsync paged result - OrganizationRepository: GetByIdAsync not-found path WorkflowBuilder: - WorkflowRepository: GetByIdAsync cross-org isolation, GetPagedAsync paged result DataModeling: - DataModelRepository: GetPagedAsync paged result - DataClassRepository: GetPagedAsync paged result Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ation Address PR review: tests that relied on NSubstitute's default-null for unmatched calls did not conclusively prove the handler forwards the correct org ID to the repository. WorkflowBuilder (14 handlers): remove the OrgId stub, add an explicit otherOrgId → null stub, assert Received(1).GetByIdAsync with otherOrgId, and assert uow.DidNotReceive().SaveChangesAsync on all command handlers. DataModeling (UpdateField, UpdateModel, UpdateRecord, GetRecords): add Received(1).GetByIdAsync(id, otherOrgId) and DidNotReceive().SaveChangesAsync assertions without changing the existing OrgId stub. FormBuilder (AddField, DeleteForm, RemoveField, ReorderFields): same Received + DidNotReceive hardening pattern. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This was referenced May 18, 2026
5 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
_WhenBelongsToAnotherOrg_ReturnsNotFoundtests to every Application handler across all 5 modules (DataModeling, FormBuilder, WorkflowBuilder, WorkflowEngine, Identity) — security tenant boundaries are now documented and verified by name, not implicitly via NSubstitute's default-null behaviourRemoveField,ReorderFields,DeleteDataClass(not-found cases);CancelExecution(failed-execution guard);GetUserSessions(empty-list path)WorkflowDefinitionConfigurationunique-index filter, addedUniqueConstraintException→Conflicthandling inCreateWorkflow/UpdateWorkflow, addedGetFormPickerHandler, added EF migrationAddUniqueConstraintOnWorkflowNameTest results
504 unit tests — 0 failed, 0 skipped
Test plan
dotnet test unit-tests.slnf— all 504 tests pass, zero failures, zero skips🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests