feat: create accessible portal shell and shared design system - #65
Conversation
…zed dependency installation
…ctor scripts, configuration validation, and CI workflows.
…tooling, setup scripts, and configurable Docker services
…ions, and reformat source files
…service orchestration, and updated documentation
…ovenance, and standardized build gates
…gates, and document formal release management procedures.
… contract linting while upgrading to Go 1.25.0
…ract validation, and Go 1.25 upgrade
…th Redocly, and update CI workflows for schema validation and CodeQL.
…ping for cross-tenant data safety
…, period, and context models
…diction, and actor validation
…ability requirements in the architecture and development guidelines.
… to application scope, and enhance API error reporting.
…approval, and audit support
…and field definition versioning
…ycle API with observability metrics
…deterministic calculation, and authorization controls
…ulation, and immutability
…expanded metrics and documentation
…step versioned payments for payment-to-assessment logic
…ledger balance tracking, and reversal operations with idempotency support
…, reproducible TypeScript client generation, and event schema validation.
…Script client generation
…ent schema standards
…ndational UI styles
📝 WalkthroughWalkthroughThis change introduces balanced financial posting and payment-allocation workflows, expands API and event contract validation with generated clients, and establishes an accessible, responsive taxpayer portal with authentication, authorization, localization, and automated accessibility coverage. ChangesFinancial slice
Contract governance
Portal foundation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant HTTPAPI
participant AdministrationService
participant LedgerDomain
Client->>HTTPAPI: submit return
HTTPAPI->>AdministrationService: SubmitAndAssess
AdministrationService->>LedgerDomain: create assessment posting
LedgerDomain-->>AdministrationService: balanced posting
Client->>HTTPAPI: record payment with Idempotency-Key
HTTPAPI->>AdministrationService: RecordPaymentIdempotent
AdministrationService->>LedgerDomain: create payment receipt posting
LedgerDomain-->>AdministrationService: receipt posting
Client->>HTTPAPI: allocate payment with expected version
HTTPAPI->>AdministrationService: AllocatePayment
AdministrationService->>LedgerDomain: create allocation posting
LedgerDomain-->>AdministrationService: balanced allocation posting
HTTPAPI-->>Client: payment and ledger state
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 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: 11
🧹 Nitpick comments (8)
web/packages/ui/src/index.tsx (1)
71-89: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winProp spread can silently override the computed
role.
{...props}is spread afterrole, so any consumer-suppliedroleprop overrides the tone-derived role, defeating the component's purpose of guaranteeing correct alert semantics.TextFieldin this same file correctly spreads{...props}before its explicit ARIA attributes —Alertshould follow the same order.🛡️ Proposed fix
return ( <div + {...props} className={`or-alert or-alert--${tone}`} role={tone === "danger" ? "alert" : "status"} - {...props} > {children} </div> );🤖 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 `@web/packages/ui/src/index.tsx` around lines 71 - 89, Update the Alert component so its {...props} spread occurs before the computed role attribute, ensuring consumer-supplied props cannot override the tone-derived alert semantics; preserve the existing className and role computation.apps/taxpayer-portal/src/App.tsx (2)
114-180: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRoute generation implicitly assumes Dashboard is always
taxpayerNavigation[0].
items.slice(1)silently depends onnavigation.ts'staxpayerNavigationarray keeping the Dashboard entry first and unfiltered. Reordering the array, or adding a future item withoutrequiredPermissionahead of Dashboard, would duplicate/misroute the index page. Filtering by path is more robust to that coupling.♻️ Proposed fix
- {items.slice(1).map((item) => ( + {items.filter((item) => item.path !== "/").map((item) => (🤖 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 `@apps/taxpayer-portal/src/App.tsx` around lines 114 - 180, Update the route generation in Portal so non-dashboard routes are selected by excluding the dashboard path rather than using items.slice(1). Keep the explicit "/" Dashboard route unchanged, and ensure the filtered navigation items continue mapping to PlaceholderPage routes without duplicating or misrouting the index page.
37-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMost portal strings bypass the new i18n mechanism.
Only
productName/portalName/signOutgo throughuseTranslation; everything else inDashboard,PlaceholderPage, andComponentExamples(headings, card labels, form hints/errors, button text) is hardcoded English. Given this cohort explicitly introduces localization hooks, worth tracking these as follow-up i18n keys before non-English locales reach these views.🤖 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 `@apps/taxpayer-portal/src/App.tsx` around lines 37 - 112, Update Dashboard, PlaceholderPage, and ComponentExamples to source all user-facing strings through the existing useTranslation mechanism, adding dedicated i18n keys for headings, card labels/details, empty-state text, descriptions, alerts, field labels/hints/errors, and button text. Preserve the current rendered English text as translation values and keep the existing interpolation for PlaceholderPage’s title-derived message.apps/taxpayer-portal/src/App.test.tsx (1)
30-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for the mobile menu toggle itself.
Existing tests cover keyboard nav and focus-after-routing, but none exercise
PortalShell's hamburger toggle (aria-expandedflips,.sidebar--openapplied,closeMenufiring on nav click while the menu is open). This is one of the PR's headline features ("keyboard-accessible mobile navigation") and is currently untested.🤖 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 `@apps/taxpayer-portal/src/App.test.tsx` around lines 30 - 43, Add coverage in the App test suite for PortalShell’s mobile menu toggle: activate the hamburger control and verify aria-expanded changes and the sidebar--open class is applied, then click a navigation link while open and verify closeMenu collapses the menu. Preserve the existing keyboard navigation and focus-after-routing test.internal/administration/application/service.go (1)
892-895: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winHard-coded
XCRcurrency in the balance projection.Everything else in the ledger is currency-generic, but this pins the projection to
XCR. Any entry in another currency makesAdd/SubtractreturnErrCurrencyMismatch, surfacing as an opaque 422 onGET /taxpayers/{id}/ledger. Deriving the currency from the first entry (and rejecting mixed-currency sets explicitly) keeps the constraint honest.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/administration/application/service.go` around lines 892 - 895, Replace the hard-coded XCR initialization in the balance projection with currency derivation from the first ledger entry. Validate that all subsequent entries use the same currency and explicitly reject mixed-currency sets before calling Add or Subtract, preserving the existing LedgerBalance error flow.apps/api/http.go (1)
339-348: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueValidation failures in
paymentbypassfinancialSliceFailures.Bad currency (line 341) and bad amount (line 346) return 4xx without touching the failure counter, while the service error at line 358 does. Same for the missing-key 400 at line 351. Worth making the counting uniform so the failure rate is meaningful.
🤖 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 `@apps/api/http.go` around lines 339 - 348, Update the payment handler’s validation failure paths around NewCurrency, NewMoney, and the missing-key check to increment financialSliceFailures consistently before returning. Preserve the existing counter increment for service errors and ensure every payment failure, including these 4xx responses, is counted exactly once.internal/administration/application/financial_test.go (1)
22-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSwallowed errors in the shared fixture will produce misleading failures.
Register,ApproveRegistration,DraftReturn,ValidateReturn, andCalculateReturnall discard their error. If any step regresses, every test built onfinancialSlicefails at an unrelated later assertion (or on a zero-valuedtaxReturn.ID) instead of pointing at the real break. Since this helper backs six tests, the debugging cost compounds.♻️ Fail fast in the fixture
- registrationValue, _ := s.Register(requestScope, taxpayerValue.ID.String(), "SAMPLE_INCOME") - registrationValue, _ = s.ApproveRegistration(requestScope, registrationValue.ID.String()) - taxReturn, _ := s.DraftReturn( + registrationValue, err := s.Register(requestScope, taxpayerValue.ID.String(), "SAMPLE_INCOME") + if err != nil { + t.Fatal(err) + } + registrationValue, err = s.ApproveRegistration(requestScope, registrationValue.ID.String()) + if err != nil { + t.Fatal(err) + } + taxReturn, err := s.DraftReturn( requestScope, taxpayerValue.ID.String(), registrationValue.ID.String(), "FY-DEMO-2026", []filing.Line{{Code: "GROSS", AmountMinor: 1_000_00}}, ) - _, _ = s.ValidateReturn(requestScope, taxReturn.ID.String()) - _, _ = s.CalculateReturn(requestScope, taxReturn.ID.String()) + if err != nil { + t.Fatal(err) + } + if _, err = s.ValidateReturn(requestScope, taxReturn.ID.String()); err != nil { + t.Fatal(err) + } + if _, err = s.CalculateReturn(requestScope, taxReturn.ID.String()); err != nil { + t.Fatal(err) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/administration/application/financial_test.go` around lines 22 - 29, Update the shared financial fixture setup around Register, ApproveRegistration, DraftReturn, ValidateReturn, and CalculateReturn to check each returned error and fail the fixture immediately with the relevant error. Preserve the existing setup order and ensure subsequent steps use only successfully returned values, so failures identify the operation that regressed.apps/api/http_test.go (1)
194-214: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNew routes without HTTP-level coverage.
The slice exercises
POST /payments,POST /payments/{id}/allocations, and the ledger read, butGET /payments/{paymentID},GET /assessments/{assessmentID}, andPOST /ledger/postings/{postingID}/reverseare wired inRouterwith no request-level test. The reverse endpoint in particular has a distinct 201 status and a conflict path worth pinning down here.🤖 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 `@apps/api/http_test.go` around lines 194 - 214, Add request-level coverage in the relevant HTTP test around the existing payment allocation flow for GET /payments/{paymentID}, GET /assessments/{assessmentID}, and POST /ledger/postings/{postingID}/reverse. Assert successful response bodies for the two GET routes, and verify the reverse route returns 201 plus its expected payload; also add a second reverse request using the same posting ID to assert the documented conflict response.
🤖 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 @.github/workflows/contracts-ci.yml:
- Around line 22-24: Update the actions/checkout step to disable persisted
checkout credentials by setting persist-credentials to false in its with
configuration, while retaining fetch-depth: 0 for full-history access.
In `@apps/api/http.go`:
- Around line 417-434: The ledger handler currently fetches entries and balance
through separate service calls and lock acquisitions. Add or use a single
service method that authorizes once, takes one consistent snapshot, and returns
both ledger entries and their balance; update Handler.ledger to use that result
while preserving the existing error response and asOf retrieval.
- Around line 385-405: Move the paymentAllocationSuccesses.Add(1) call in the
payment allocation handler to immediately after h.s.AllocatePayment succeeds,
before writing the successful response. Ensure every subsequent failure path,
including NewMoney and AllocatePayment errors, increments financialSliceFailures
consistently with the other financial operations.
In `@contracts/openapi/openapi.yaml`:
- Around line 408-419: Extend the compatibility gate to compare response content
schemas, including the 200 response for the /taxpayers/{taxpayerId}/ledger
operation. Detect incompatible response-schema shape changes alongside the
existing path, operation, property, and required-field checks, and either
version the affected API surface or fail CI when such changes are introduced.
In `@docs/development/frontend-guidelines.md`:
- Line 4: Remove the duplicate “Frontend guidelines” heading from the document
while preserving the existing H1. Ensure the added content begins directly with
the portal guidance.
In `@internal/administration/application/service.go`:
- Around line 975-984: The ReversePosting flow must keep owning aggregate state
consistent with the reversal: update the related assessment’s Outstanding or the
related payment’s Allocated, Unapplied, Version, and Allocations as applicable
when creating the reversal. If those aggregates cannot be safely compensated,
restrict ReversePosting to posting kinds without aggregate state and return the
existing conflict/error response for unsupported kinds; extend coverage beyond
ledger balance to verify assessment and payment projections remain consistent.
- Around line 844-862: Update the financial write paths in
internal/administration/application/service.go at lines 844-862, 714-721, and
982-992: stage mutations to assessments, payments, postings, entries, and
aggregates, or perform the fallible record/emit and allocatePaymentLocked work
first, then commit all state only after every step succeeds. Ensure
AllocatePayment, receipt handling, and reversal handling leave no partial ledger
or aggregate changes when any audit or emit operation returns an error; all
three listed sites require this ordering fix.
- Around line 695-707: Update the assessment lookup in the payment flow around
the assessmentID branch to return ErrNotFound only when the assessment is
absent; handle an existing assessment with zero Outstanding using the supported
overpayment/unapplied-credit behavior, while preserving taxpayer and currency
validation.
In `@scripts/ci/check-openapi-compatibility.mjs`:
- Around line 21-42: The compatibility check around the path and schema
iteration only detects removals and newly required schema properties; replace or
extend it with semantic recursive comparisons of effective operations and
schemas. Ensure it flags removed response definitions, newly required
parameters, property type or constraint changes, and removed enum values,
preferably by using an established OpenAPI breaking-change checker if available.
In `@scripts/ci/generate-openapi-client.mjs`:
- Line 24: Update the generated request construction in the OpenAPI client
generation flow to add Content-Type: application/json whenever options.body is
present and serialized. Merge this with options.headers so an explicitly
provided caller Content-Type remains authoritative, while requests without a
body retain their current headers.
In `@scripts/ci/validate-openapi.mjs`:
- Line 19: Update the response-status validation condition to recognize wildcard
range keys such as "4XX" and "5XX" as error responses alongside numeric statuses
of 400 or higher, while continuing to exclude "401". Ensure these range
responses undergo the required application/problem+json validation.
---
Nitpick comments:
In `@apps/api/http_test.go`:
- Around line 194-214: Add request-level coverage in the relevant HTTP test
around the existing payment allocation flow for GET /payments/{paymentID}, GET
/assessments/{assessmentID}, and POST /ledger/postings/{postingID}/reverse.
Assert successful response bodies for the two GET routes, and verify the reverse
route returns 201 plus its expected payload; also add a second reverse request
using the same posting ID to assert the documented conflict response.
In `@apps/api/http.go`:
- Around line 339-348: Update the payment handler’s validation failure paths
around NewCurrency, NewMoney, and the missing-key check to increment
financialSliceFailures consistently before returning. Preserve the existing
counter increment for service errors and ensure every payment failure, including
these 4xx responses, is counted exactly once.
In `@apps/taxpayer-portal/src/App.test.tsx`:
- Around line 30-43: Add coverage in the App test suite for PortalShell’s mobile
menu toggle: activate the hamburger control and verify aria-expanded changes and
the sidebar--open class is applied, then click a navigation link while open and
verify closeMenu collapses the menu. Preserve the existing keyboard navigation
and focus-after-routing test.
In `@apps/taxpayer-portal/src/App.tsx`:
- Around line 114-180: Update the route generation in Portal so non-dashboard
routes are selected by excluding the dashboard path rather than using
items.slice(1). Keep the explicit "/" Dashboard route unchanged, and ensure the
filtered navigation items continue mapping to PlaceholderPage routes without
duplicating or misrouting the index page.
- Around line 37-112: Update Dashboard, PlaceholderPage, and ComponentExamples
to source all user-facing strings through the existing useTranslation mechanism,
adding dedicated i18n keys for headings, card labels/details, empty-state text,
descriptions, alerts, field labels/hints/errors, and button text. Preserve the
current rendered English text as translation values and keep the existing
interpolation for PlaceholderPage’s title-derived message.
In `@internal/administration/application/financial_test.go`:
- Around line 22-29: Update the shared financial fixture setup around Register,
ApproveRegistration, DraftReturn, ValidateReturn, and CalculateReturn to check
each returned error and fail the fixture immediately with the relevant error.
Preserve the existing setup order and ensure subsequent steps use only
successfully returned values, so failures identify the operation that regressed.
In `@internal/administration/application/service.go`:
- Around line 892-895: Replace the hard-coded XCR initialization in the balance
projection with currency derivation from the first ledger entry. Validate that
all subsequent entries use the same currency and explicitly reject
mixed-currency sets before calling Add or Subtract, preserving the existing
LedgerBalance error flow.
In `@web/packages/ui/src/index.tsx`:
- Around line 71-89: Update the Alert component so its {...props} spread occurs
before the computed role attribute, ensuring consumer-supplied props cannot
override the tone-derived alert semantics; preserve the existing className and
role computation.
🪄 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: 1fae8a00-dc1a-47ec-ad44-95721c745399
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (48)
.github/workflows/contracts-ci.yml.gitignoreMakefileapps/api/http.goapps/api/http_test.goapps/taxpayer-portal/package.jsonapps/taxpayer-portal/src/App.test.tsxapps/taxpayer-portal/src/App.tsxapps/taxpayer-portal/src/auth.tsxapps/taxpayer-portal/src/i18n.tsxapps/taxpayer-portal/src/main.tsxapps/taxpayer-portal/src/navigation.test.tsapps/taxpayer-portal/src/navigation.tsapps/taxpayer-portal/src/styles.cssapps/taxpayer-portal/src/styles.d.tsapps/taxpayer-portal/vite.config.tsclients/typescript/openapi-client.tscontracts/events/envelope.schema.jsoncontracts/events/examples/payment-allocated.v1.jsoncontracts/openapi/openapi.yamldatabase/migrations/000001_foundation.up.sqldocs/development/api-guidelines.mddocs/development/ci-quality-gates.mddocs/development/event-guidelines.mddocs/development/frontend-guidelines.mddocs/development/portal-design-system.mddocs/diagrams/ledger-posting.mddocs/diagrams/payment-allocation.mddocs/domain/assessment.mddocs/domain/ledger.mddocs/domain/payment.mddocs/operations/observability.mdinternal/administration/application/financial_test.gointernal/administration/application/service.gointernal/administration/application/service_test.gointernal/assessment/application/ports.gointernal/ledger/application/ports.gointernal/ledger/domain/model.gointernal/ledger/domain/model_test.gointernal/payment/application/ports.gopackage.jsonscripts/ci/check-openapi-compatibility.mjsscripts/ci/generate-openapi-client.mjsscripts/ci/validate-event-schemas.mjsscripts/ci/validate-openapi.mjsweb/packages/ui/package.jsonweb/packages/ui/src/index.tsxweb/packages/ui/src/styles.css
| - uses: actions/checkout@fbc6f3992d24b796d5a048ff273f7fcc4a7b6c09 # v5 | ||
| with: | ||
| fetch-depth: 0 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Disable persisted checkout credentials.
The checkout token is persisted in local Git config by default, while this job executes PR-controlled scripts. Disable persistence; full-history reads for git show do not require it.
with:
fetch-depth: 0
+ persist-credentials: false📝 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.
| - uses: actions/checkout@fbc6f3992d24b796d5a048ff273f7fcc4a7b6c09 # v5 | |
| with: | |
| fetch-depth: 0 | |
| - uses: actions/checkout@fbc6f3992d24b796d5a048ff273f7fcc4a7b6c09 # v5 | |
| with: | |
| fetch-depth: 0 | |
| persist-credentials: false |
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 22-24: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 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 @.github/workflows/contracts-ci.yml around lines 22 - 24, Update the
actions/checkout step to disable persisted checkout credentials by setting
persist-credentials to false in its with configuration, while retaining
fetch-depth: 0 for full-history access.
Source: Linters/SAST tools
| currency, err := foundation.NewCurrency(in.Currency, 2) | ||
| if err != nil { | ||
| financialSliceFailures.Add(1) | ||
| writeApplicationError(w, r, "Allocation failed", err) | ||
| return | ||
| } | ||
| paymentAllocationSuccesses.Add(1) | ||
| amount, err := foundation.NewMoney(in.AmountMinor, currency) | ||
| if err != nil { | ||
| writeApplicationError(w, r, "Allocation failed", err) | ||
| return | ||
| } | ||
| value, err := h.s.AllocatePayment( | ||
| requestContext(r), chi.URLParam(r, "paymentID"), in.AssessmentID, | ||
| amount, in.ExpectedVersion, | ||
| ) | ||
| if err != nil { | ||
| writeApplicationError(w, r, "Allocation failed", err) | ||
| return | ||
| } | ||
| write(w, http.StatusOK, value) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
paymentAllocationSuccesses is incremented before the allocation is attempted.
Line 391 fires right after currency parsing, so every request with a well-formed currency counts as a successful allocation — including ones that subsequently fail with conflict, forbidden, or not-found. The gauge for operation="payment_allocation" will over-report and the failure paths never touch financialSliceFailures, unlike payment and reversePosting.
🐛 Move the increment to the success path and count failures
currency, err := foundation.NewCurrency(in.Currency, 2)
if err != nil {
financialSliceFailures.Add(1)
writeApplicationError(w, r, "Allocation failed", err)
return
}
- paymentAllocationSuccesses.Add(1)
amount, err := foundation.NewMoney(in.AmountMinor, currency)
if err != nil {
+ financialSliceFailures.Add(1)
writeApplicationError(w, r, "Allocation failed", err)
return
}
value, err := h.s.AllocatePayment(
requestContext(r), chi.URLParam(r, "paymentID"), in.AssessmentID,
amount, in.ExpectedVersion,
)
if err != nil {
+ financialSliceFailures.Add(1)
writeApplicationError(w, r, "Allocation failed", err)
return
}
+ paymentAllocationSuccesses.Add(1)
write(w, http.StatusOK, value)📝 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.
| currency, err := foundation.NewCurrency(in.Currency, 2) | |
| if err != nil { | |
| financialSliceFailures.Add(1) | |
| writeApplicationError(w, r, "Allocation failed", err) | |
| return | |
| } | |
| paymentAllocationSuccesses.Add(1) | |
| amount, err := foundation.NewMoney(in.AmountMinor, currency) | |
| if err != nil { | |
| writeApplicationError(w, r, "Allocation failed", err) | |
| return | |
| } | |
| value, err := h.s.AllocatePayment( | |
| requestContext(r), chi.URLParam(r, "paymentID"), in.AssessmentID, | |
| amount, in.ExpectedVersion, | |
| ) | |
| if err != nil { | |
| writeApplicationError(w, r, "Allocation failed", err) | |
| return | |
| } | |
| write(w, http.StatusOK, value) | |
| currency, err := foundation.NewCurrency(in.Currency, 2) | |
| if err != nil { | |
| financialSliceFailures.Add(1) | |
| writeApplicationError(w, r, "Allocation failed", err) | |
| return | |
| } | |
| amount, err := foundation.NewMoney(in.AmountMinor, currency) | |
| if err != nil { | |
| financialSliceFailures.Add(1) | |
| writeApplicationError(w, r, "Allocation failed", err) | |
| return | |
| } | |
| value, err := h.s.AllocatePayment( | |
| requestContext(r), chi.URLParam(r, "paymentID"), in.AssessmentID, | |
| amount, in.ExpectedVersion, | |
| ) | |
| if err != nil { | |
| financialSliceFailures.Add(1) | |
| writeApplicationError(w, r, "Allocation failed", err) | |
| return | |
| } | |
| paymentAllocationSuccesses.Add(1) | |
| write(w, http.StatusOK, value) |
🤖 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 `@apps/api/http.go` around lines 385 - 405, Move the
paymentAllocationSuccesses.Add(1) call in the payment allocation handler to
immediately after h.s.AllocatePayment succeeds, before writing the successful
response. Ensure every subsequent failure path, including NewMoney and
AllocatePayment errors, increments financialSliceFailures consistently with the
other financial operations.
| func (h *Handler) ledger(w http.ResponseWriter, r *http.Request) { | ||
| scope := requestContext(r) | ||
| entries, err := h.s.Ledger(scope, chi.URLParam(r, "taxpayerID")) | ||
| if err != nil { | ||
| problem.Write(w, r, 422, "Ledger query failed", err) | ||
| writeApplicationError(w, r, "Ledger query failed", err) | ||
| return | ||
| } | ||
| asOf, err := h.s.CurrentTime(scope) | ||
| if err != nil { | ||
| problem.Write(w, r, 422, "Ledger query failed", err) | ||
| writeApplicationError(w, r, "Ledger query failed", err) | ||
| return | ||
| } | ||
| balance, err := h.s.LedgerBalance(scope, chi.URLParam(r, "taxpayerID")) | ||
| if err != nil { | ||
| writeApplicationError(w, r, "Ledger query failed", err) | ||
| return | ||
| } | ||
| write(w, 200, map[string]any{"entries": entries, "balance": balance, "asOf": asOf}) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
entries and balance are read under two separate lock acquisitions.
h.s.Ledger and h.s.LedgerBalance each take RLock independently, so a concurrent posting between the two calls yields a response whose balance does not reconcile with the returned entries. LedgerBalance also re-runs Ledger internally, duplicating the authorization check and the full linear scan of s.entries on every request.
A single service method returning both from one snapshot fixes the inconsistency and halves the work.
🤖 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 `@apps/api/http.go` around lines 417 - 434, The ledger handler currently
fetches entries and balance through separate service calls and lock
acquisitions. Add or use a single service method that authorizes once, takes one
consistent snapshot, and returns both ledger entries and their balance; update
Handler.ledger to use that result while preserving the existing error response
and asOf retrieval.
| "200": | ||
| { | ||
| description: Balanced append-only ledger and projected balances, | ||
| content: | ||
| { | ||
| application/json: | ||
| { schema: { $ref: "#/components/schemas/LedgerResponse" } }, | ||
| }, | ||
| }, | ||
| "401": { $ref: "#/components/responses/Unauthorized" }, | ||
| default: { $ref: "#/components/responses/Problem" }, | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect whether Payment/LedgerResponse existed with different shape before this PR
git log --oneline -3 -- contracts/openapi/openapi.yaml
BASE=$(git merge-base HEAD origin/main 2>/dev/null || git rev-parse HEAD~1)
git diff "$BASE" -- contracts/openapi/openapi.yaml | sed -n '1,200p'Repository: opencorex-org/openrevenue
Length of output: 7636
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Relevant OpenAPI diff around the ledger + schema changes =="
BASE=$(git merge-base HEAD origin/main 2>/dev/null || git rev-parse HEAD~1)
git diff "$BASE" -- contracts/openapi/openapi.yaml | sed -n '1,260p'
echo
echo "== Search for compatibility gate implementation and docs =="
rg -n --hidden --glob '!**/.git/**' \
-e 'compatibility gate|breaking change|OpenAPI|openapi compatibility|schema compatibility|versioned API surface|migration decision' \
. | sed -n '1,220p'Repository: opencorex-org/openrevenue
Length of output: 12751
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== OpenAPI file outline =="
ast-grep outline contracts/openapi/openapi.yaml --view expanded | sed -n '1,240p'
echo
echo "== Lines around the ledger response and the added schemas =="
nl -ba contracts/openapi/openapi.yaml | sed -n '380,820p'Repository: opencorex-org/openrevenue
Length of output: 337
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Files that mention ledger or versioning/migration notes =="
git ls-files | rg -n '(^|/)(README|CHANGELOG|MIGRAT|VERSION|docs|contracts/openapi|openapi).*(md|yaml|yml|json)?$' || true
rg -n --hidden --glob '!**/.git/**' \
-e 'ledger/postings/.*/reverse|LedgerResponse|expectedVersion|compatibility' \
contracts .github docs . | sed -n '1,220p'Repository: opencorex-org/openrevenue
Length of output: 5974
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Find compatibility-related files =="
fd -H -I -t f 'compat|breaking|openapi|schema' . | sed -n '1,200p'
echo
echo "== Grep likely gate code =="
rg -n --hidden --glob '!**/.git/**' \
-e 'breaking change|compatibility check|compatibility gate|OpenAPI.*diff|schema.*diff|required properties|response schema|operation removed|property required' \
.github contracts scripts src docs package.json . | sed -n '1,260p'
echo
echo "== Search for migration/version notes in docs =="
rg -n --hidden --glob '!**/.git/**' \
-e 'migration decision|breaking change|versioned API|API version|v2|compatibility gate' \
docs README* CHANGELOG* .github contracts | sed -n '1,220p'Repository: opencorex-org/openrevenue
Length of output: 2177
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== scripts/ci/check-openapi-compatibility.mjs =="
sed -n '1,220p' scripts/ci/check-openapi-compatibility.mjs
echo
echo "== docs/development/api-guidelines.md excerpt =="
sed -n '1,80p' docs/development/api-guidelines.md
echo
echo "== package.json contracts scripts =="
sed -n '1,80p' package.jsonRepository: opencorex-org/openrevenue
Length of output: 6284
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== LedgerResponse and related schemas in openapi.yaml =="
python3 - <<'PY'
from pathlib import Path
path = Path("contracts/openapi/openapi.yaml")
lines = path.read_text().splitlines()
for start,end in [(780,930)]:
for i in range(start, min(end, len(lines))+1):
print(f"{i:4d}: {lines[i-1]}")
PYRepository: opencorex-org/openrevenue
Length of output: 4308
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Compatibility script diff/logic =="
python3 - <<'PY'
from pathlib import Path
p = Path("scripts/ci/check-openapi-compatibility.mjs")
for i, line in enumerate(p.read_text().splitlines(), 1):
print(f"{i:4d}: {line}")
PYRepository: opencorex-org/openrevenue
Length of output: 3472
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Current OpenAPI contract around LedgerResponse =="
python3 - <<'PY'
from pathlib import Path
p = Path("contracts/openapi/openapi.yaml")
lines = p.read_text().splitlines()
for i in range(836, 885):
print(f"{i:4d}: {lines[i-1]}")
PYRepository: opencorex-org/openrevenue
Length of output: 2069
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' scripts/ci/check-openapi-compatibility.mjs
echo
sed -n '1,80p' docs/development/api-guidelines.md
echo
python3 - <<'PY'
from pathlib import Path
p = Path("contracts/openapi/openapi.yaml")
lines = p.read_text().splitlines()
for i in range(836, 885):
print(f"{i:4d}: {lines[i-1]}")
PYRepository: opencorex-org/openrevenue
Length of output: 6559
Extend the compatibility gate for response-body schema changes. The current check covers removed paths/operations and schema property/required-field diffs, but it does not compare a response’s content schema. That means the /taxpayers/{taxpayerId}/ledger 200 response can change shape without tripping CI; version the surface or add response-schema comparison here.
🤖 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 `@contracts/openapi/openapi.yaml` around lines 408 - 419, Extend the
compatibility gate to compare response content schemas, including the 200
response for the /taxpayers/{taxpayerId}/ledger operation. Detect incompatible
response-schema shape changes alongside the existing path, operation, property,
and required-field checks, and either version the affected API surface or fail
CI when such changes are introduced.
| # Frontend guidelines | ||
|
|
||
| Organize by feature, keep server state in TanStack Query, validate forms with Zod, and reuse workspace packages. Components render decisions but do not implement tax rules. All controls need accessible names, keyboard support, localization, loading/error states, and tests. Mock APIs with MSW and reserve Playwright for critical journeys. | ||
| # Frontend guidelines |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the duplicate document title.
Line 4 repeats the H1 from line 1 and triggers markdownlint MD024. Keep the existing title and begin this addition with the portal guidance.
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 4-4: Multiple headings with the same content
(MD024, no-duplicate-heading)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/development/frontend-guidelines.md` at line 4, Remove the duplicate
“Frontend guidelines” heading from the document while preserving the existing
H1. Ensure the added content begins directly with the portal guidance.
Source: Linters/SAST tools
| switch { | ||
| case payment.Unapplied.IsZero(): | ||
| payment.Status = "ALLOCATED" | ||
| default: | ||
| payment.Status = "PARTIALLY_ALLOCATED" | ||
| } | ||
| s.assessments[assessmentKey] = assessment | ||
| s.postings[scope.IsolationKey(posting.ID.String())] = posting | ||
| s.entries = append(s.entries, posting.Entries...) | ||
| if err := s.record(scope, "PaymentAllocated", "payment", payment.ID.String()); err != nil { | ||
| return err | ||
| } | ||
| if err := s.emit(scope, "PaymentAllocated", "payment", payment.ID.String(), map[string]string{ | ||
| "assessmentId": assessmentID, "postingId": posting.ID.String(), | ||
| }); err != nil { | ||
| return err | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
State is committed before the fallible record/emit steps across all three financial write paths. Each path appends ledger entries and mutates aggregates first, then calls s.record/s.emit, which can return an error — leaving the in-memory ledger and aggregate maps permanently inconsistent with no rollback.
internal/administration/application/service.go#L844-L862:s.assessmentsands.entriesare written at lines 850-852; a failure at 853-860 causesAllocatePaymentto skips.payments[key] = payment, so the assessment'sOutstandingdrops with no corresponding payment allocation.internal/administration/application/service.go#L714-L721: the receipt posting and its entries are appended at 715-716 beforeallocatePaymentLockedcan fail at 718, orphaning ledger entries with no stored payment.internal/administration/application/service.go#L982-L992: the reversal posting and entries are stored at 982-984 beforerecord/emitat 985-992, so a failure leaves the ledger reversed while the caller receives an error.
Perform the fallible audit/emit work before committing, or stage all mutations and apply them together once every step has succeeded.
📍 Affects 1 file
internal/administration/application/service.go#L844-L862(this comment)internal/administration/application/service.go#L714-L721internal/administration/application/service.go#L982-L992
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/administration/application/service.go` around lines 844 - 862,
Update the financial write paths in
internal/administration/application/service.go at lines 844-862, 714-721, and
982-992: stage mutations to assessments, payments, postings, entries, and
aggregates, or perform the fallible record/emit and allocatePaymentLocked work
first, then commit all state only after every step succeeds. Ensure
AllocatePayment, receipt handling, and reversal handling leave no partial ledger
or aggregate changes when any audit or emit operation returns an error; all
three listed sites require this ordering fix.
| if _, exists := s.reversedPostings[key]; exists || original.ReversalOf != nil { | ||
| return ledger.Posting{}, fmt.Errorf("%w: posting is already a reversal or has been reversed", ErrConflict) | ||
| } | ||
| reversal, err := ledger.NewReversalPosting(scope, original, s.clock.Now()) | ||
| if err != nil { | ||
| return ledger.Posting{}, err | ||
| } | ||
| s.postings[scope.IsolationKey(reversal.ID.String())] = reversal | ||
| s.reversedPostings[key] = reversal.ID.String() | ||
| s.entries = append(s.entries, reversal.Entries...) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Reversal corrects the ledger but leaves aggregate state stale.
Reversing an assessment posting swaps the ledger entries so LedgerBalance reads zero, but Assessment.Outstanding is untouched; likewise reversing a receipt or allocation posting leaves Payment.Allocated/Unapplied/Version/Allocations unchanged. GET /assessments/{id} and GET /payments/{id} will then contradict GET /taxpayers/{id}/ledger. TestLedgerPostingReversalIsBalancedAndSingleUse only asserts the projected balance, so this gap is untested.
Either compensate the owning aggregate when reversing, or restrict ReversePosting to posting kinds that carry no aggregate state.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/administration/application/service.go` around lines 975 - 984, The
ReversePosting flow must keep owning aggregate state consistent with the
reversal: update the related assessment’s Outstanding or the related payment’s
Allocated, Unapplied, Version, and Allocations as applicable when creating the
reversal. If those aggregates cannot be safely compensated, restrict
ReversePosting to posting kinds without aggregate state and return the existing
conflict/error response for unsupported kinds; extend coverage beyond ledger
balance to verify assessment and payment projections remain consistent.
| for (const [path, item] of Object.entries(previous.paths ?? {})) { | ||
| if (!current.paths?.[path]) { | ||
| breaks.push(`removed path ${path}`); | ||
| continue; | ||
| } | ||
| for (const method of ["get", "post", "put", "patch", "delete"]) { | ||
| if (item[method] && !current.paths[path][method]) breaks.push(`removed operation ${method.toUpperCase()} ${path}`); | ||
| } | ||
| } | ||
| for (const [name, schema] of Object.entries(previous.components?.schemas ?? {})) { | ||
| const next = current.components?.schemas?.[name]; | ||
| if (!next) { | ||
| breaks.push(`removed schema ${name}`); | ||
| continue; | ||
| } | ||
| for (const property of Object.keys(schema.properties ?? {})) { | ||
| if (!next.properties?.[property]) breaks.push(`removed property ${name}.${property}`); | ||
| } | ||
| for (const required of next.required ?? []) { | ||
| if (!(schema.required ?? []).includes(required)) breaks.push(`new required property ${name}.${required}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Detect breaking response and schema changes, not only removals.
This gate remains green if an advertised response is removed, a parameter becomes required, a property type changes, or an OpenAPI enum value is removed. Those changes break existing generated clients. Compare effective operations recursively—including parameters, request/response schemas, types, constraints, and enums—or use a semantic OpenAPI breaking-change checker.
🤖 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/ci/check-openapi-compatibility.mjs` around lines 21 - 42, The
compatibility check around the path and schema iteration only detects removals
and newly required schema properties; replace or extend it with semantic
recursive comparisons of effective operations and schemas. Ensure it flags
removed response definitions, newly required parameters, property type or
constraint changes, and removed enum values, preferably by using an established
OpenAPI breaking-change checker if available.
| params.push("options: RequestOptions = {}"); | ||
| const expression = path.replaceAll(/\{([^}]+)\}/g, "${encodeURIComponent($1)}"); | ||
| lines.push(` async ${operation.operationId}(${params.join(", ")}): Promise<unknown> {`); | ||
| lines.push(` const response = await this.request(\`\${this.baseUrl}${expression}\`, { method: "${method.toUpperCase()}", headers: options.headers, body: options.body === undefined ? undefined : JSON.stringify(options.body) });`); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Set Content-Type: application/json for serialized bodies.
Line 24 serializes JSON but forwards headers unchanged; generated POST/PUT/PATCH calls can be rejected as text/plain. Add the JSON content type when options.body is present, while preserving an explicit caller override.
Proposed fix
+ lines.push(" const headers = new Headers(options.headers);");
+ lines.push(' if (options.body !== undefined && !headers.has("content-type")) headers.set("content-type", "application/json");');
- lines.push(` const response = await this.request(\`\${this.baseUrl}${expression}\`, { method: "${method.toUpperCase()}", headers: options.headers, body: options.body === undefined ? undefined : JSON.stringify(options.body) });`);
+ lines.push(` const response = await this.request(\`\${this.baseUrl}${expression}\`, { method: "${method.toUpperCase()}", headers, body: options.body === undefined ? undefined : JSON.stringify(options.body) });`);📝 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.
| lines.push(` const response = await this.request(\`\${this.baseUrl}${expression}\`, { method: "${method.toUpperCase()}", headers: options.headers, body: options.body === undefined ? undefined : JSON.stringify(options.body) });`); | |
| lines.push(" const headers = new Headers(options.headers);"); | |
| lines.push(' if (options.body !== undefined && !headers.has("content-type")) headers.set("content-type", "application/json");'); | |
| lines.push(` const response = await this.request(\`\${this.baseUrl}${expression}\`, { method: "${method.toUpperCase()}", headers, body: options.body === undefined ? undefined : JSON.stringify(options.body) });`); |
🤖 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/ci/generate-openapi-client.mjs` at line 24, Update the generated
request construction in the OpenAPI client generation flow to add Content-Type:
application/json whenever options.body is present and serialized. Merge this
with options.headers so an explicitly provided caller Content-Type remains
authoritative, while requests without a body retain their current headers.
| else operationIds.add(operation.operationId); | ||
|
|
||
| for (const [status, response] of Object.entries(operation.responses ?? {})) { | ||
| if ((status === "default" || Number(status) >= 400) && status !== "401") { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate OpenAPI range response keys.
Number("4XX") and Number("5XX") are NaN, so valid range responses bypass the required application/problem+json check.
Proposed fix
- if ((status === "default" || Number(status) >= 400) && status !== "401") {
+ if (
+ (status === "default" || /^[45](?:\d{2}|XX)$/.test(status)) &&
+ status !== "401"
+ ) {📝 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.
| if ((status === "default" || Number(status) >= 400) && status !== "401") { | |
| if ( | |
| (status === "default" || /^[45](?:\d{2}|XX)$/.test(status)) && | |
| status !== "401" | |
| ) { |
🤖 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/ci/validate-openapi.mjs` at line 19, Update the response-status
validation condition to recognize wildcard range keys such as "4XX" and "5XX" as
error responses alongside numeric statuses of 400 or higher, while continuing to
exclude "401". Ensure these range responses undergo the required
application/problem+json validation.
1 participant
Summary
Creates a secure, responsive, and accessible application foundation shared by the taxpayer, officer, and administrator portal experiences.
Closes #15
Changes
PortalShellAccessibility verification
Automated tests cover:
Verification