Remove local git hooks; add GetCategoryById endpoint; backend API response/CORS infra - #70
Merged
Merged
Conversation
…ponse/CORS infra Repo tooling: removes .husky/pre-commit and apps/admin-frontend/.lintstagedrc.json in favor of explicit quality-gate commands (docs/adr/0031); updates the trunk-based-workflow and branch-agnostic-precommit ADRs accordingly. Backend: adds GetCategoryById query/handler/endpoint for Categories; introduces ApiResponse/ApiProblemDetailsFactory and CORS/OpenAPI documentation setup extensions shared across identity-service and services-service; updates architecture_guard.py's database-boundary check to match. Co-Authored-By: Claude Sonnet 5 <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 ignored due to path filters (1)
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe pull request standardizes API response and error contracts, adds API documentation and category lookup support, updates identity client seeding, removes repository-owned Git hooks, and adjusts startup, package, and repository documentation. ChangesAPI standardization
Local Git hook removal
Repository and runtime maintenance
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant IdentityServiceApi
participant Scalar
participant IdentityProvider
Browser->>IdentityServiceApi: Request /api-docs
IdentityServiceApi->>Scalar: Render API reference
Scalar->>IdentityProvider: Start OAuth2 authorization-code flow
IdentityProvider-->>Scalar: Return authorization result and tokens
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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: 3
🧹 Nitpick comments (6)
backend/shared/Admin.SharedKernel.AspNetCore/ResultExtensions.cs (2)
27-53: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEnvelope silently dropped for non-
ObjectResultsuccess results.
ToActionResult<TValue>only injectsenvelopeif (actionResult is ObjectResult objectResult). IfonSuccessever returns a non-ObjectResult(for exampleFile(...)orRedirect(...)),envelopeis computed and discarded, and the response silently skips theApiResponse<T>contract with no warning. All current callers in this cohort returnObjectResultsubtypes (Ok,Created), so this is not an active bug today.Consider asserting or logging when
actionResultis not anObjectResult, so a future non-ObjectResultusage does not silently break the envelope contract.🤖 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 `@backend/shared/Admin.SharedKernel.AspNetCore/ResultExtensions.cs` around lines 27 - 53, Update ToActionResult<TValue> to explicitly handle the case where onSuccess returns a non-ObjectResult: assert or log that the ApiResponse<TValue> envelope cannot be applied, while preserving the existing envelope injection for ObjectResult subtypes.
71-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
ResolveCorrelationIdimplementation across two new shared-kernel files. Both files implement the identical rule (readX-Correlation-Id, fall back toTraceIdentifier), andResultExtensions.cshardcodes the header name literal instead of reusingApiProblemDetailsFactory's constant. This is a single root cause: the logic was never extracted to one shared location.
backend/shared/Admin.SharedKernel.AspNetCore/ResultExtensions.cs#L71-L78: remove this private method and callApiProblemDetailsFactory's correlation resolution instead (make the factory's methodinternal/publicor extract both to a sharedstatichelper inAdmin.SharedKernel.AspNetCore).backend/shared/Admin.SharedKernel.AspNetCore/ApiProblemDetailsFactory.cs#L72-L85: expose this method (or theCorrelationIdHeaderNameconstant plus resolution logic) soResultExtensions.cscan reuse it instead of re-implementing it.♻️ Proposed fix
// ApiProblemDetailsFactory.cs - private static string? ResolveCorrelationId(HttpContext? httpContext) + internal static string? ResolveCorrelationId(HttpContext? httpContext)// ResultExtensions.cs - private static string? ResolveCorrelationId(HttpContext httpContext) - { - if (httpContext.Request.Headers.TryGetValue("X-Correlation-Id", out var correlationId) && !StringValues.IsNullOrEmpty(correlationId)) - { - return correlationId.ToString(); - } - - return httpContext.TraceIdentifier; - } + private static string? ResolveCorrelationId(HttpContext httpContext) => + ApiProblemDetailsFactory.ResolveCorrelationId(httpContext);🤖 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 `@backend/shared/Admin.SharedKernel.AspNetCore/ResultExtensions.cs` around lines 71 - 78, The correlation ID resolution logic is duplicated across ApiProblemDetailsFactory and ResultExtensions. In backend/shared/Admin.SharedKernel.AspNetCore/ResultExtensions.cs lines 71-78, remove the private ResolveCorrelationId implementation and call the shared factory/helper instead; in backend/shared/Admin.SharedKernel.AspNetCore/ApiProblemDetailsFactory.cs lines 72-85, expose or extract the existing resolution method and CorrelationIdHeaderName so both callers reuse one implementation.backend/services/services-service/ServicesService.Api/Setup/DocumentationExtensions.cs (1)
100-102: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRead the demo tenant ID from configuration instead of hardcoding it.
The default value
"019f9b0b-e7fb-7ac6-84b7-5c8ed52c6120"duplicatesappsettings.Development.json'sDemoTenant:Id.AddApiDocumentationalready receivesconfiguration, so this method can readconfiguration["DemoTenant:Id"]instead of hardcoding the same GUID a second time. IfDemoTenant:Idchanges, this literal will silently go stale.♻️ Proposed fix
+ var demoTenantId = configuration["DemoTenant:Id"] ?? "019f9b0b-e7fb-7ac6-84b7-5c8ed52c6120"; + ... Schema = new OpenApiSchema { Type = JsonSchemaType.String, - Default = JsonValue.Create("019f9b0b-e7fb-7ac6-84b7-5c8ed52c6120"), + Default = JsonValue.Create(demoTenantId), },🤖 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 `@backend/services/services-service/ServicesService.Api/Setup/DocumentationExtensions.cs` around lines 100 - 102, Update the schema default in AddApiDocumentation to read the DemoTenant:Id value from the provided configuration instead of using the hardcoded GUID, preserving the existing JsonSchemaType.String setup.backend/shared/Admin.SharedKernel.Tests/ResultExtensionsTests.cs (1)
135-147: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStrengthen the envelope assertion; remove the restating comment.
The test name states it verifies wrapping in
ApiResponse, but the assertions at Line 144 and Line 146 only check thatresponseis not null and not the same reference ascomplexObject. Any other wrapper shape would also pass this test. Verify the actualApiResponse<T>type using reflection, sincecomplexObjectis an anonymous type.The comment at Line 145 restates what the assertion on Line 146 already expresses. Remove it, or replace it with a note on why reflection is needed for an anonymous type payload.
♻️ Proposed fix to verify the actual envelope type
var okResult = actionResult.Should().BeOfType<OkObjectResult>().Subject; - var response = okResult.Value.Should().NotBeNull(); - // Response should be wrapped in ApiResponse - response.Should().NotBeSameAs(complexObject); + var response = okResult.Value.Should().NotBeNull().Subject; + response.Should().NotBeSameAs(complexObject); + response.GetType().Should().Be(typeof(ApiResponse<>).MakeGenericType(complexObject.GetType()));As per coding guidelines, "Use code comments only to explain non-obvious reasons such as security defaults, library quirks, or ordering and transaction constraints; do not comment what the code does".
🤖 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 `@backend/shared/Admin.SharedKernel.Tests/ResultExtensionsTests.cs` around lines 135 - 147, Strengthen ToActionResult_Generic_OnSuccess_WithComplexObject_WrapsInEnvelope by asserting that the response runtime type is the generic ApiResponse<> envelope, using reflection because the payload is anonymous; retain the non-null check as needed and remove the redundant “wrapped in ApiResponse” comment.Source: Coding guidelines
backend/services/identity-service/IdentityService.Api/Program.cs (1)
16-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
publicIssuerresolution and validation across two files. Both files independently computeIdentity:PublicIssuer ?? Identity:Authority ?? throw new InvalidOperationException(...)from the sameIConfiguration, so the fallback rule and error message can drift between the OpenIddict issuer configuration and the OpenAPI/Scalar security-scheme URLs.
backend/services/identity-service/IdentityService.Api/Program.cs#L16-L22: keep computingpublicIssuerhere, but pass the resolved value intoAddApiDocumentationinstead ofbuilder.Configuration.backend/services/identity-service/IdentityService.Api/Setup/DocumentationExtensions.cs#L19-L22: changeAddApiDocumentationto accept the already-resolvedpublicIssuerstring parameter and drop its own fallback/throw logic.🤖 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 `@backend/services/identity-service/IdentityService.Api/Program.cs` around lines 16 - 22, Use the existing publicIssuer resolution in backend/services/identity-service/IdentityService.Api/Program.cs:16-22 and pass that resolved string to AddApiDocumentation instead of the configuration object. Update backend/services/identity-service/IdentityService.Api/Setup/DocumentationExtensions.cs:19-22 so AddApiDocumentation accepts the publicIssuer string and removes its duplicate fallback and validation, while using the parameter for the documentation security-scheme URLs.backend/services/identity-service/IdentityService.Api/Seed/DatabaseSeeder.cs (1)
99-110: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant
PopulateAsyncand duplicate URI merge calls beforeUpdateAsync.For the pinned OpenIddict 7.6.0,
UpdateAsync(adminPanel, descriptor, cancellationToken)copies descriptor values ontoadminPanelas part of the update flow, so the precedingPopulateAsynccall is not needed. The URI merge calls after it also re-run the same merge into the same descriptor object, which is a no-op.♻️ Proposed fix
if (adminPanel is null) { await applicationManager.CreateAsync(descriptor, cancellationToken); return; } - await applicationManager.PopulateAsync(adminPanel, descriptor, cancellationToken); - MergeUris(descriptor.RedirectUris, redirectUris); - MergeUris(descriptor.PostLogoutRedirectUris, postLogoutRedirectUris); - await applicationManager.UpdateAsync(adminPanel, descriptor, cancellationToken);🤖 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 `@backend/services/identity-service/IdentityService.Api/Seed/DatabaseSeeder.cs` around lines 99 - 110, In the existing seeding method, remove the PopulateAsync call and both MergeUris calls immediately before UpdateAsync; retain the direct UpdateAsync(adminPanel, descriptor, cancellationToken) flow so the descriptor values are applied once.
🤖 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 `@backend/shared/Admin.SharedKernel.AspNetCore/ApiResponse.cs`:
- Around line 3-33: Remove the XML documentation comments from the
ApiResponse<T> properties Data, Success, Timestamp, TraceId, and CorrelationId,
while preserving the class-level and type-parameter documentation and all
property declarations unchanged.
In `@README.md`:
- Around line 54-56: Revise the README statement describing Node.js version
enforcement to avoid claiming universal local and CI enforcement. Clarify that
.nvmrc selects the version for tools that read it, engines.node declares
compatibility, and the >=22.22 floor is explicitly enforced by the frontend CI
workflows.
In `@scripts/architecture_guard.py`:
- Around line 534-537: Update the removed_files logic in
scripts/architecture_guard.py around lines 534-537 to enumerate all regular
files under .husky, rather than checking only .husky/pre-commit, while
preserving the existing admin-frontend entry. In
scripts/tests/test_architecture_guard.py lines 739-777, add a fixture containing
a non-pre-commit Husky hook and assert that the guard reports a blocking
finding.
---
Nitpick comments:
In `@backend/services/identity-service/IdentityService.Api/Program.cs`:
- Around line 16-22: Use the existing publicIssuer resolution in
backend/services/identity-service/IdentityService.Api/Program.cs:16-22 and pass
that resolved string to AddApiDocumentation instead of the configuration object.
Update
backend/services/identity-service/IdentityService.Api/Setup/DocumentationExtensions.cs:19-22
so AddApiDocumentation accepts the publicIssuer string and removes its duplicate
fallback and validation, while using the parameter for the documentation
security-scheme URLs.
In
`@backend/services/identity-service/IdentityService.Api/Seed/DatabaseSeeder.cs`:
- Around line 99-110: In the existing seeding method, remove the PopulateAsync
call and both MergeUris calls immediately before UpdateAsync; retain the direct
UpdateAsync(adminPanel, descriptor, cancellationToken) flow so the descriptor
values are applied once.
In
`@backend/services/services-service/ServicesService.Api/Setup/DocumentationExtensions.cs`:
- Around line 100-102: Update the schema default in AddApiDocumentation to read
the DemoTenant:Id value from the provided configuration instead of using the
hardcoded GUID, preserving the existing JsonSchemaType.String setup.
In `@backend/shared/Admin.SharedKernel.AspNetCore/ResultExtensions.cs`:
- Around line 27-53: Update ToActionResult<TValue> to explicitly handle the case
where onSuccess returns a non-ObjectResult: assert or log that the
ApiResponse<TValue> envelope cannot be applied, while preserving the existing
envelope injection for ObjectResult subtypes.
- Around line 71-78: The correlation ID resolution logic is duplicated across
ApiProblemDetailsFactory and ResultExtensions. In
backend/shared/Admin.SharedKernel.AspNetCore/ResultExtensions.cs lines 71-78,
remove the private ResolveCorrelationId implementation and call the shared
factory/helper instead; in
backend/shared/Admin.SharedKernel.AspNetCore/ApiProblemDetailsFactory.cs lines
72-85, expose or extract the existing resolution method and
CorrelationIdHeaderName so both callers reuse one implementation.
In `@backend/shared/Admin.SharedKernel.Tests/ResultExtensionsTests.cs`:
- Around line 135-147: Strengthen
ToActionResult_Generic_OnSuccess_WithComplexObject_WrapsInEnvelope by asserting
that the response runtime type is the generic ApiResponse<> envelope, using
reflection because the payload is anonymous; retain the non-null check as needed
and remove the redundant “wrapped in ApiResponse” comment.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 871c58bb-e3aa-4320-ae8e-4f1af8e42597
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (39)
.gitignore.husky/pre-commitAGENTS.mdREADME.mdapps/admin-frontend/.lintstagedrc.jsonbackend/AppHost/AppHost.csbackend/Directory.Packages.propsbackend/services/identity-service/IdentityService.Api/IdentityService.Api.csprojbackend/services/identity-service/IdentityService.Api/Program.csbackend/services/identity-service/IdentityService.Api/Properties/launchSettings.jsonbackend/services/identity-service/IdentityService.Api/Seed/DatabaseSeeder.csbackend/services/identity-service/IdentityService.Api/Setup/CorsExtensions.csbackend/services/identity-service/IdentityService.Api/Setup/DocumentationExtensions.csbackend/services/identity-service/IdentityService.Api/appsettings.Development.jsonbackend/services/services-service/ServicesService.Api/Controllers/CategoriesController.csbackend/services/services-service/ServicesService.Api/Controllers/ServicesController.csbackend/services/services-service/ServicesService.Api/Controllers/TagsController.csbackend/services/services-service/ServicesService.Api/Program.csbackend/services/services-service/ServicesService.Api/Properties/launchSettings.jsonbackend/services/services-service/ServicesService.Api/ServicesService.Api.csprojbackend/services/services-service/ServicesService.Api/Setup/DocumentationExtensions.csbackend/services/services-service/ServicesService.Api/appsettings.Development.jsonbackend/services/services-service/ServicesService.Application/Categories/GetCategoryById/GetCategoryByIdQuery.csbackend/services/services-service/ServicesService.Application/Categories/GetCategoryById/GetCategoryByIdQueryHandler.csbackend/services/services-service/ServicesService.Tests/Categories/GetCategoryById/GetCategoryByIdQueryHandlerTests.csbackend/shared/Admin.SharedKernel.AspNetCore/ApiProblemDetails.csbackend/shared/Admin.SharedKernel.AspNetCore/ApiProblemDetailsFactory.csbackend/shared/Admin.SharedKernel.AspNetCore/ApiResponse.csbackend/shared/Admin.SharedKernel.AspNetCore/GenericExceptionHandler.csbackend/shared/Admin.SharedKernel.AspNetCore/ProblemDetailsAuthorizationMiddlewareResultHandler.csbackend/shared/Admin.SharedKernel.AspNetCore/ResultExtensions.csbackend/shared/Admin.SharedKernel.Tests/ResultExtensionsTests.csdocs/MONOREPO.mddocs/adr/0021-trunk-based-git-workflow.mddocs/adr/0030-branch-agnostic-precommit.mddocs/adr/0031-remove-local-git-hooks.mdpackage.jsonscripts/architecture_guard.pyscripts/tests/test_architecture_guard.py
💤 Files with no reviewable changes (2)
- apps/admin-frontend/.lintstagedrc.json
- .husky/pre-commit
Comment on lines
+3
to
+33
| /// <summary> | ||
| /// Standard success response envelope for all API endpoints. | ||
| /// Provides consistency with ApiProblemDetails for error responses. | ||
| /// </summary> | ||
| /// <typeparam name="T">The type of data being returned</typeparam> | ||
| public sealed class ApiResponse<T> | ||
| { | ||
| /// <summary> | ||
| /// The actual data payload. | ||
| /// </summary> | ||
| public required T Data { get; init; } | ||
|
|
||
| /// <summary> | ||
| /// Always true for success responses. | ||
| /// </summary> | ||
| public bool Success { get; init; } = true; | ||
|
|
||
| /// <summary> | ||
| /// UTC timestamp when the response was generated. | ||
| /// </summary> | ||
| public DateTimeOffset Timestamp { get; init; } = DateTimeOffset.UtcNow; | ||
|
|
||
| /// <summary> | ||
| /// Trace identifier for correlating the response with logs. | ||
| /// </summary> | ||
| public string? TraceId { get; init; } | ||
|
|
||
| /// <summary> | ||
| /// Correlation identifier for end-to-end tracing. | ||
| /// </summary> | ||
| public string? CorrelationId { get; init; } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove XML doc comments that restate the code.
Every property here has an XML doc comment that only repeats the property name in prose (for example, "Always true for success responses." on Success). None of these explain a non-obvious reason such as a security default, library quirk, or ordering constraint.
Remove them to keep this file aligned with the stated convention.
♻️ Proposed fix
namespace Admin.SharedKernel.AspNetCore;
-/// <summary>
-/// Standard success response envelope for all API endpoints.
-/// Provides consistency with ApiProblemDetails for error responses.
-/// </summary>
-/// <typeparam name="T">The type of data being returned</typeparam>
public sealed class ApiResponse<T>
{
- /// <summary>
- /// The actual data payload.
- /// </summary>
public required T Data { get; init; }
- /// <summary>
- /// Always true for success responses.
- /// </summary>
public bool Success { get; init; } = true;
- /// <summary>
- /// UTC timestamp when the response was generated.
- /// </summary>
public DateTimeOffset Timestamp { get; init; } = DateTimeOffset.UtcNow;
- /// <summary>
- /// Trace identifier for correlating the response with logs.
- /// </summary>
public string? TraceId { get; init; }
- /// <summary>
- /// Correlation identifier for end-to-end tracing.
- /// </summary>
public string? CorrelationId { get; init; }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// <summary> | |
| /// Standard success response envelope for all API endpoints. | |
| /// Provides consistency with ApiProblemDetails for error responses. | |
| /// </summary> | |
| /// <typeparam name="T">The type of data being returned</typeparam> | |
| public sealed class ApiResponse<T> | |
| { | |
| /// <summary> | |
| /// The actual data payload. | |
| /// </summary> | |
| public required T Data { get; init; } | |
| /// <summary> | |
| /// Always true for success responses. | |
| /// </summary> | |
| public bool Success { get; init; } = true; | |
| /// <summary> | |
| /// UTC timestamp when the response was generated. | |
| /// </summary> | |
| public DateTimeOffset Timestamp { get; init; } = DateTimeOffset.UtcNow; | |
| /// <summary> | |
| /// Trace identifier for correlating the response with logs. | |
| /// </summary> | |
| public string? TraceId { get; init; } | |
| /// <summary> | |
| /// Correlation identifier for end-to-end tracing. | |
| /// </summary> | |
| public string? CorrelationId { get; init; } | |
| public sealed class ApiResponse<T> | |
| { | |
| public required T Data { get; init; } | |
| public bool Success { get; init; } = true; | |
| public DateTimeOffset Timestamp { get; init; } = DateTimeOffset.UtcNow; | |
| public string? TraceId { get; init; } | |
| public string? CorrelationId { get; init; } | |
| } |
🤖 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 `@backend/shared/Admin.SharedKernel.AspNetCore/ApiResponse.cs` around lines 3 -
33, Remove the XML documentation comments from the ApiResponse<T> properties
Data, Success, Timestamp, TraceId, and CorrelationId, while preserving the
class-level and type-parameter documentation and all property declarations
unchanged.
Source: Coding guidelines
Comment on lines
+54
to
+56
| Node was previously documented as 22.18 while the frontend toolchain already | ||
| required >=22.22 — `.nvmrc`/`engines.node` now enforce the real floor | ||
| everywhere (local and CI) instead of letting them silently disagree. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant files =="
git ls-files | sed -n '1,200p' | grep -E '(^|/)(README\.md|\.nvmrc|package\.json|package-lock\.json|pnpm-lock\.yaml|yarn\.lock|.*\.ya?ml|.*\.json)$' | sed -n '1,160p'
echo
echo "== README around cited lines =="
cat -n README.md | sed -n '45,65p'
echo
echo "== Node-related declarations =="
for f in .nvmrc package.json; do
if [ -f "$f" ]; then
echo "--- $f ---"
cat -n "$f"
fi
done
echo
echo "== references to nvm/node-version/ci and Node >=22 in configs/workflows =="
rg -n --hidden --glob '!node_modules' --glob '!dist' --glob '!build' 'uses:\s*actions/(setup-node|checkout|core)\@|node-version|\.nvmrc|engines\.node|node-version-file|>=22\.22|22\.18|22\.(22|18)|node-version:|node-version-file:' -S . | sed -n '1,240p'
echo
echo "== changed files/stat =="
git diff --stat || true
git diff --numstat || trueRepository: evertonschuster/Agenza
Length of output: 10436
Narrow the claim about Node.js enforcement.
.nvmrc selects the Node version for tools that read it, and engines.node declares compatibility. The workflows show this floor in frontend CI, not universal local/CI enforcement.
Proposed wording
- required >=22.22 — `.nvmrc`/`engines.node` now enforce the real floor
- everywhere (local and CI) instead of letting them silently disagree.
+ required >=22.22 — `.nvmrc`/`engines.node` now declare the real floor
+ used by local tooling and frontend CI.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Node was previously documented as 22.18 while the frontend toolchain already | |
| required >=22.22 — `.nvmrc`/`engines.node` now enforce the real floor | |
| everywhere (local and CI) instead of letting them silently disagree. | |
| Node was previously documented as 22.18 while the frontend toolchain already | |
| required >=22.22 — `.nvmrc`/`engines.node` now declare the real floor | |
| used by local tooling and frontend CI. |
🤖 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 `@README.md` around lines 54 - 56, Revise the README statement describing
Node.js version enforcement to avoid claiming universal local and CI
enforcement. Clarify that .nvmrc selects the version for tools that read it,
engines.node declares compatibility, and the >=22.22 floor is explicitly
enforced by the frontend CI workflows.
Comment on lines
+534
to
537
| removed_files = [ | ||
| REPO_ROOT / ".husky" / "pre-commit", | ||
| REPO_ROOT / "apps" / "admin-frontend" / ".lintstagedrc.json", | ||
| ] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Detect every repository-owned Husky hook.
The guard checks only .husky/pre-commit. A committed .husky/commit-msg or another Husky hook violates ADR 0031 but produces no finding.
scripts/architecture_guard.py#L534-L537: enumerate regular files under.huskyinstead of checking onlypre-commit.scripts/tests/test_architecture_guard.py#L739-L777: add a fixture for a non-pre-commitHusky hook and assert a blocking finding.
As per coding guidelines, “A durable correction or architectural decision must be persisted in the appropriate … automated guard, or CI gate as applicable.”
📍 Affects 2 files
scripts/architecture_guard.py#L534-L537(this comment)scripts/tests/test_architecture_guard.py#L739-L777
🤖 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 `@scripts/architecture_guard.py` around lines 534 - 537, Update the
removed_files logic in scripts/architecture_guard.py around lines 534-537 to
enumerate all regular files under .husky, rather than checking only
.husky/pre-commit, while preserving the existing admin-frontend entry. In
scripts/tests/test_architecture_guard.py lines 739-777, add a fixture containing
a non-pre-commit Husky hook and assert that the guard reports a blocking
finding.
Source: Coding guidelines
Closed
5 tasks
evertonschuster
added a commit
that referenced
this pull request
Aug 2, 2026
The previous commit deleted .husky/pre-commit (a dangling reference once .lintstagedrc.json was gone), but check_branch_agnostic_precommit() still expected that file to exist with specific content, so architecture_guard.py was failing on this branch's own committed state. Replaces check_branch_agnostic_precommit with check_local_git_hooks_absent (verifying the hooks/tooling stay removed instead of checking removed-file content) and drops husky/lint-staged from package.json - the same fix #70 makes independently on its own branch; whichever PR merges first, the other's identical change is a no-op. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This was referenced Aug 2, 2026
This PR added GET /api/v1/categories/{id} to the backend but didn't
update the frontend's generated OpenAPI types, so CI's api-contract-check
correctly failed - it regenerates services-api.d.ts from the live
backend and diffs against the checked-in version. Regenerated against
this PR's own backend (npm run generate:api-types), verified clean with
npm run generate:api-types:check, and re-ran build/lint/format to
confirm nothing downstream broke.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Admin.SharedKernel.AspNetCore's ResultExtensions.ToActionResult now wraps every successful Result<TValue> response in an ApiResponse<T> envelope (data/success/timestamp/traceId/correlationId) - this PR's own change. scripts/smoke_oidc_contract.py wasn't updated to match, so its tenant- provisioning assertion read the old flat shape and failed even though the endpoint worked correctly (201, real tenantId, just nested under "data" now). The three assertions checking failure responses (401/403 "code" field) are unaffected - those go through ToProblemResult, which was not wrapped and never changed shape. This bug predates this PR split - it already failed identically on the original, unsplit PR (#69) before any of this work was divided up. Verified against a real locally-running Aspire stack: both `npm run generate:api-types:check` and `python scripts/smoke_oidc_contract.py` pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Merged
5 tasks
evertonschuster
added a commit
that referenced
this pull request
Aug 2, 2026
…ervices; physical reorg (#75) * Migrate Catalog (Categories/Services/Tags) to Result errors; remove Services vertical; physical reorg Split out of #69 (part 3 of 4 — see that PR for the full picture). Recreated from origin/main after #70 and #71 merged, since this repo's convention (and the split-large-coderabbit-pr skill) is a sequential series, not stacking on an unmerged branch — CodeRabbit also doesn't review PRs whose base isn't the default branch, so stacking silently skipped review for this and the next PR in the series. Same content as the original #72, just re-based; no functional change. This PR is larger than the <100-file target used for the other PRs in this series, deliberately — see "Why this couldn't be split further" below. - Categories, Services, and Tags all move to the Result-based error convention docs/adr/014 establishes: domain entities' create() methods, mappers, and API repositories return Result<T, AppError> instead of throwing; useAsync.ts (already Result-based, landed in #71) is the one hook every feature's data layer builds on now. - app/composition/container.ts's CatalogFacade drops the use-case-class indirection (ListCategories/CreateTag/etc. as separate classes) for direct repository delegation (`{ execute: repo.method }`) - there's no orchestration between the facade and the repository, so the extra class per operation wasn't earning its keep. The 24 now-orphaned use-case-class files (application/use-cases/{categories,services,tags}/) are deleted. - Services' frontend implementation (ServicesPage, ServiceForm, six ServicesPage.*.test.tsx files, all its components/hooks/models) is fully removed, reverting `/services` to a placeholder page (app/pages/ServicesPage/ServicesPage.tsx) - this vertical is going back to `stub` status, see docs/STATUS.md. - Categories moves to a routed create/edit dialog (features/catalog/presentation/categories/pages/CategoriesListPage/, .../CategoryEditorDialog/) per docs/adr/012, replacing the old flat CategoriesPage.tsx/useCategories.ts/CategoryEditorDialog.tsx shape. - Tags gets the equivalent internal move (hooks/useTagEditor.ts, pages/TagEditorDialog.tsx) and its own Result migration (Tag.ts/tagMapper.ts/ApiTagRepository.ts) - Tags itself is not being removed here, just migrated; its removal is a later PR in this stack (docs/adr/016). - shared/: AuthenticatedHttpClient's get/post/put/delete now return Result<T, AppError> instead of throwing; DeleteConfirmationDialog takes entityName/entityType instead of a raw title/description pair; useCreateInline is removed (no longer used once Services - its only consumer - is gone). - Also removes .husky/pre-commit and fixes architecture_guard.py's precommit check accordingly (already merged independently via #70; included here too since this branch's own ancestry needed it before #70 existed). ## Why this couldn't be split further I initially tried a narrower "Category-only foundation" PR (~99 files) deferring Services/Tags. That failed a real build: app/composition/ container.ts wires TagRepository with its *new* method signature directly (tagRepository.listAll(options) instead of the old (tenantContext, options) two-arg form) - not just a return-type change useAsync-style, but the interface itself. Making that build without also migrating TagRepository/ApiTagRepository/tagMapper/Tag.ts for real isn't a smaller wrapper shim - it's the same size of work as just finishing the migration, since there's no reduced version of an interface signature. Categories, Services, and Tags share container.ts's catalog wiring, router.tsx, and AuthenticatedHttpClient tightly enough that they're one atomic, verified-buildable unit at this layer - mirroring why Auth couldn't be split from Catalog either, just one layer down. ## Test plan - [x] `npm install` + `npm run build --workspace=apps/admin-frontend` — green - [x] `npm run lint --workspace=apps/admin-frontend` — clean, 0 warnings - [x] `npm run format:check --workspace=apps/admin-frontend` — clean - [x] `npm run test --workspace=apps/admin-frontend` — 368/368 passing - [x] `scripts/sync_agent_skills.py --check`, `scripts/check_agent_governance.py`, `scripts/architecture_guard.py` — all pass Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Fix categories-mobile.spec.ts's mock for the GET-by-id endpoint useCategoryEditor fetches its own category via GET /api/v1/categories/{id} (docs/adr/013), but this spec's route mock matched any /api/v1/categories* path and always returned the full list array regardless of whether the request was for the collection or a single id - so the by-id fetch received an array instead of a CategoryDto, and the edit dialog's Nome field never populated. Mock now inspects the last path segment and returns the matching single category (404 if not found) for a by-id GET, the full list otherwise. Verified against the real Playwright suite (production build + preview, matching CI): all 10 e2e specs pass, including this one. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
6 tasks
evertonschuster
added a commit
that referenced
this pull request
Aug 2, 2026
Split out of #69 (final part of this series — see that PR for the full picture). Recreated from origin/main after #70/#71/#75 merged, since this repo's convention (and the split-large-coderabbit-pr skill) is a sequential series, not stacking on an unmerged branch. Same content as the original #73, just re-based; no functional change. Removes the entire Tags vertical from apps/admin-frontend (domain, application, infrastructure, presentation, MSW handlers, E2E specs, nav entry, route, and catalog facade wiring) while intentionally retaining the backend Tag domain entity and /api/v1/tags endpoints, including Service's many-to-many relationship to Tag - a project-owner decision, see docs/adr/016-remove-tags-frontend.md. Categories replaces Tags as the reference CRUD implementation throughout the docs and the agenza-frontend-feature skill. ## Test plan - [x] `npm install` + `npm run build --workspace=apps/admin-frontend` — green - [x] `npm run lint --workspace=apps/admin-frontend` — clean, 0 warnings - [x] `npm run format:check --workspace=apps/admin-frontend` — clean - [x] `npm run test --workspace=apps/admin-frontend` — 305/305 passing - [x] `npx playwright test` (full e2e suite, production build + preview) — 8/8 passing - [x] `scripts/sync_agent_skills.py --check`, `scripts/check_agent_governance.py`, `scripts/architecture_guard.py` — all pass Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
4 tasks
evertonschuster
added a commit
that referenced
this pull request
Aug 2, 2026
…#77) These 3 files (agent-skills/agenza-frontend-feature/SKILL.md and its two synced copies under .claude/skills/ and .agents/skills/) live at the repo root, outside apps/admin-frontend/ — every diff I computed while splitting #69 into #70/#71/#75/#76 was scoped to apps/admin-frontend (and backend/ for #70), so these files' accumulated updates from this session (Catalog Result migration, Auth Result migration, and finally the Tags-removal doc pass replacing TagsPage/TagForm with Categories as the reference implementation) never made it into any of the split PRs, even though the actual code changes they describe are all correctly merged. Content taken directly from the original branch's final commit (4911abb), already reviewed and governance-checked at the time. Verified again here against the current merged main: sync_agent_skills.py --check, check_agent_governance.py, and architecture_guard.py all pass, and the file paths the skill references (CategoriesListPage.tsx, CategoryForm.tsx, categoryMapper.ts, AdminLayout.tsx) all exist in the current tree. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
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
Split out of #69 (part 1 of 4 — see that PR for the full picture; this one
is self-contained and independently mergeable).
.husky/pre-commitandapps/admin-frontend/.lintstagedrc.jsonin favor of running quality-gate commands explicitly, per
docs/adr/0031-remove-local-git-hooks.md; updatesdocs/adr/0021anddocs/adr/0030accordingly.GET /api/v1/categories/{id}(GetCategoryByIdQuery/Handler +test) to services-service.
ApiResponse/ApiProblemDetailsFactoryand per-serviceSetup/CorsExtensions.cs/Setup/DocumentationExtensions.csto bothidentity-service and services-service.
scripts/architecture_guard.py's database-boundary check tomatch the current
ServiceConfiguration.csshape.Test plan
dotnet build backend/AdminBackend.slnx— greendotnet test backend/AdminBackend.slnx— greenscripts/sync_agent_skills.py --check,scripts/check_agent_governance.py,scripts/architecture_guard.py— all pass🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Chores