Xero invoice path: document push + Finish Job — job-xero-invoice green (13 of 40)

[corrin]

Slice 2b — invoice path: document push + Finish Job (job-xero-invoice green, 13/40)

Second of the three slice-2 PRs (2a sync engine #46 · 2b this · 2c quote path next). Ports the Xero document push and the Finish Job workspace from v1.

What ports

Document manager base + invoice push (apps/xero/documents/{base,invoice}.py, apps/xero/helpers.py)

• XeroDocumentManager with the config guards (branding theme, quote terms), best-effort history notes, and fail-early construction; XeroInvoiceManager builds a provider-agnostic InvoicePayload, calls get_provider().create_invoice(), and owns local persistence: Invoice mirror row from the provider's canonical raw payload, billing_metadata audit trail, JobEvent, and a same-request recalculate_job_invoicing_state (ledgered — v1 left fully_invoiced to the hourly sync, which under XERO_READONLY never sees the invoice).
• Endpoints POST /api/xero/create_invoice/{job_id} and DELETE /api/xero/delete_invoice/{job_id} keep v1's URL fragments so the spec's request matcher ports unedited. Amount derivation goes through the already-ported calculate_invoice_amount (modes: full / costs-to-date / percent / amount).

Purchase-order push (apps/xero/documents/po.py) — USER-COMMITTED scope

• Create-vs-update keyed on xero_id with the zero-UUID sentinel treated as absent on read and write (v1 stored it), paged recovery of the real id by PO number, per-line xero_line_item_id backfill with duplicate-safe description matching, status mapping that raises on unknown statuses. No spec covers PO push; apps/xero/tests/test_po_manager.py (12 tests) is its gate.
• Ledgered: v1's successful PO delete always returned 500 (its success dict failed its own response serializer).

Readonly works by construction: XeroReadOnlyProvider fabricates well-formed results — INV-E2E-* numbers, GST-exclusive fake totals from the line items × CompanyDefaults.gst_rate — so the endpoint path is byte-identical under XERO_READONLY and the E2E balance-settles-to-$0 assertion exercises the real persistence code. Missing write overrides hit tripwires, not the tenant.

Finish Job backend (apps/accounting/services/finish_job_summary.py, apps/job/api.py)

• Server-owned balance (v1-verbatim arithmetic: remaining excl/incl GST, outstanding, over-invoiced reported rather than netted off), completion-checklist read/patch through Job.save()'s field-change audit machinery (unknown keys 422 via extra="forbid", schema field lists pinned to Job.COMPLETION_CHECKLIST_FIELDS at import), and the ETagged invoice list. The finish GET is deliberately un-ETagged: the balance moves when invoices sync in without touching job.updated_at.

Frontend (JobFinishTab.tsx, JobInvoiceCard.tsx, lazy-loaded into JobDetailPage)

• Balance, checklist, labour-hours cards, estimate/quote/actual comparison, quote accuracy; invoice list with create dialog (modes per pricing methodology) and delete. All money formatted from server values — no client recomputation. All v1 automation ids preserved. apiErrorMessage now also reads the document endpoints' error key so calc/configuration guidance reaches the user.

Spec: job-xero-invoice.spec.ts ports near-verbatim, with one deliberate deviation — it creates its own job instead of using sharedEditJobUrl, because it fully invoices the job and the shared fixture is read-only by contract (v1 mutated it and survived on run ordering).

Adversarial review (2 subagents) — notable catches, all fixed

• Provider-driven HTTP statuses (429/503 on live-Xero failures, 401 mid-request revocation) weren't in the declared response maps — ninja would raise ConfigError instead of returning the error payload. Statuses now clamp to the declared map; the cause stays in error (ADR 0038).
• The PO zero-UUID sentinel could be stored when the unpaginated recovery listing missed the PO (>100 POs) — guard added on write, recovery listing paged.
• TanStack v5 refetch() never rejects, so the "list could not be refreshed" toast was unreachable; an invoices load error rendered as the "No invoices" empty state.
• Known, deliberately not touched (v1-parity): calculate_invoice_amount doesn't quantize, so a sub-cent basis total (e.g. 157.985) can never read fully-invoiced. Real jobs have 2dp values; flagged for 2c or later rather than changing the money path mid-slice.

Verified

• ./scripts/ops/run_e2e.sh: 38 passed — job-xero-invoice green under XERO_READONLY (13/40 specs), all prior specs green with the sync beat live.
• Backend: 1617 unit tests green; new coverage: PO manager 12, invoice manager 7 (error contract / raw_json / readonly fabrication), finish summary 11, finish API 7. Frontend: JobFinishTab component tests 3.
• Expensive tier clean (mypy strict zero-baseline, import-linter, find-duplicates, deptry, schema-current, status table).
• Parity ledger: renamed: entries for all four document operation ids; two new behaviour entries in docs/accepted-api-differences.yml.

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features
  • Added a Finish Job workspace with financial summaries, invoice balances, cost comparisons, completion checklists, and variance indicators.
  • Added invoice listing and management, including full, percentage-based, cost-to-date, and custom-amount invoicing.
  • Added Xero invoice and purchase-order creation, updating, deletion, and invoice file attachments.
  • Added checklist updates with validation and activity history.
• Bug Fixes
  • Invoice changes now immediately recalculate the job’s fully invoiced status and remaining balance.
  • Improved error messages for accounting and Xero operations.

[coderabbitai]

Warning

Review limit reached

@corrin, you've reached your PR review limit, so we couldn't start this review.

Next review available in:24 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 052efda7-ef2c-4202-adef-941ebddc6980

📥 Commits

Reviewing files that changed from the base of the PR and between b318f2e and 09176e1.

📒 Files selected for processing (7)

• apps/xero/documents/invoice.py
• apps/xero/tests/test_invoice_manager.py
• apps/xero/tests/test_provider_documents.py
• config/tests/test_contract_gates.py
• docs/code-quality.md
• docs/rewrite-status.md
• scripts/checks/status_table.py

📝 Walkthrough

Walkthrough

This PR adds Finish Job financial and checklist APIs, Xero invoice and purchase-order synchronization, provider contracts, read-only behavior, and frontend Finish Job and invoice-management components with unit, API, and end-to-end tests.

Changes

Finish Job and Xero workflows

Layer / File(s)	Summary
Finish Job calculations and APIs apps/accounting/..., apps/job/..., frontend/schema.v2.yml	Adds financial summaries, checklist validation and updates, invoice listing, conditional responses, and Finish Job API contracts.
Accounting contracts and Xero provider operations apps/accounting/types.py, apps/accounting/provider.py, apps/xero/helpers.py, apps/xero/provider.py, apps/xero/readonly_provider.py, stubs/xero_python/...	Adds provider-agnostic document DTOs, Xero invoice and purchase-order operations, payload and error helpers, read-only results, and SDK stubs.
Xero document managers and endpoints apps/xero/documents/..., apps/xero/api.py, apps/xero/tests/...	Adds invoice and purchase-order validation, payload construction, persistence, deletion, authenticated endpoints, and manager tests.
Finish Job workspace and invoice UI frontend/src/features/job/..., frontend/src/api/..., frontend/src/lib/format.ts, frontend/tests/e2e/...	Adds lazy-loaded Finish Job and invoice components with financial displays, checklist updates, invoice actions, error handling, and end-to-end coverage.
Compatibility records and progress documentation docs/accepted-api-differences.yml, docs/code-quality.md, docs/rewrite-status.md, scripts/v1-frontend-operations.yml	Records API behavior changes, updated quality metrics, operation rename mappings, and rewrite progress.

Estimated code review effort: 5 (Critical) | ~120 minutes

Sequence Diagram(s)

sequenceDiagram
participant JobFinishTab
participant JobAPI
participant FinishJobSummary
participant JobService
JobFinishTab->>JobAPI: GET finish summary and checklist
JobAPI->>FinishJobSummary: calculate financial totals
FinishJobSummary-->>JobAPI: return summary
JobAPI-->>JobFinishTab: return JobFinishResponse
JobFinishTab->>JobAPI: PATCH checklist fields
JobAPI->>JobService: update checklist
JobService-->>JobAPI: return updated job

Loading

sequenceDiagram
participant JobInvoiceCard
participant XeroAPI
participant XeroInvoiceManager
participant XeroAccountingProvider
JobInvoiceCard->>XeroAPI: submit invoice request
XeroAPI->>XeroInvoiceManager: create document
XeroInvoiceManager->>XeroAccountingProvider: create InvoicePayload
XeroAccountingProvider-->>XeroInvoiceManager: return DocumentResult
XeroInvoiceManager-->>XeroAPI: return persisted invoice result
XeroAPI-->>JobInvoiceCard: return success or error

Loading

Possibly related PRs

• corrin/docketworks_v2#45: Extends the accounting provider and Xero abstractions used by this PR.
• corrin/docketworks_v2#18: Provides purchase-order models and status behavior used by Xero purchase-order synchronization.
• corrin/docketworks_v2#10: Provides the workshop PDF generation used by invoice attachment handling.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 44.10% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the main Xero invoice document push and Finish Job changes, with a useful E2E status indicator.
Description check	✅ Passed	The description is detailed, structured, and covers scope, behavior, verification results, known limitations, and parity changes.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch xero/invoice-path

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 14

🧹 Nitpick comments (5)

frontend/src/api/index.ts (1)

81-81: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Remove code-narration comments or document their constraint.

Both comments describe nearby code without stating the rejected alternative and the factual constraint.

• frontend/src/api/index.ts#L81-L81: Remove the export-group narration, or explain why feature code must import these generated operations through frontend/src/api/index.ts.
• frontend/src/features/job/JobFinishTab.tsx#L157-L157: Remove the ticket-heading comment. It does not document a runtime or business constraint.

As per coding guidelines, “Comments must document the rejected obvious alternative and the factual constraint that rejected it; remove code narration and review-feedback echoes.”

🤖 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 `@frontend/src/api/index.ts` at line 81, Remove the code-narration comments at
frontend/src/api/index.ts:81-81 and
frontend/src/features/job/JobFinishTab.tsx:157-157; no replacement documentation
is needed because neither comment states a factual constraint or rejected
alternative.

Source: Coding guidelines

apps/xero/tests/test_invoice_manager.py (1)

86-138: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a delete happy-path test.

TestErrorContract covers the three failure paths of delete_document. No test covers the success path, so nothing asserts that the local Invoice row is removed, that recalculate_job_invoicing_state runs, and that the invoice_deleted job event records the invoice number. That recalculation is the same same-request effect the create test protects at Line 256-258, and the Finish Job tab depends on it.

Do you want me to write this test?

🤖 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/xero/tests/test_invoice_manager.py` around lines 86 - 138, Add a
success-case test in TestErrorContract for delete_document using a configured
provider and Xero invoice ID, then assert the local Invoice is deleted,
recalculate_job_invoicing_state is called, and the invoice_deleted job event
records the invoice number. Keep the test focused on the same-request effects of
the happy path.

apps/xero/tests/test_po_manager.py (1)

98-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Cover the write direction of the zero-UUID sentinel too.

This test covers the read direction: a stored sentinel routes to create. The module docstring at Line 4-5 names "storing Xero's zero-UUID sentinel as a real id" as the expensive silent failure, and that is the write direction, guarded by _save_po_with_xero_data at apps/xero/documents/po.py Line 151.

Add a test where the provider returns DocumentResult(success=True, external_id=ZERO_UUID) and assert that po.xero_id stays None after sync_to_xero. This also pins the behavior discussed on apps/xero/provider.py Line 280-314.

Do you want me to write this test?

🤖 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/xero/tests/test_po_manager.py` around lines 98 - 109, Extend
test_zero_uuid_is_treated_as_unsynced to cover the write path: configure the
provider’s successful DocumentResult with external_id=ZERO_UUID, run
manager.sync_to_xero(), then refresh or inspect po.xero_id and assert it remains
None. Preserve the existing create-path assertion while exercising
_save_po_with_xero_data’s zero-UUID handling.

apps/xero/api.py (2)

317-328: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Type the clamp parameter as the contract, not as object.

XeroDocumentResponse declares status. Annotating the parameter object discards that contract and forces a runtime comparison against an untyped value. Declare int | None so mypy checks every call site.

♻️ Proposed signature change

-def _document_error_status(status: object) -> int:+def _document_error_status(status: int | None) -> int:

As per coding guidelines: "Treat type annotations as data contracts: use named types such as dataclasses, TypedDicts, or Protocols for complex inline types."

🤖 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/xero/api.py` around lines 317 - 328, Update the _document_error_status
parameter annotation from object to int | None, matching the status contract
declared by XeroDocumentResponse. Keep the existing 404 handling and 400
fallback unchanged, and let mypy validate all call sites against the narrowed
type.

Source: Coding guidelines

---

358-366: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract the repeated Xero-auth 401 block.

The same get_valid_token() guard and XeroAuthRequiredOut payload appear in all four new endpoints (lines 358-366, 458-466, 543-551, 612-620). The two purchase-order endpoints also repeat the identical lookup and supplier guard. One helper per repeated block keeps the message and status in step.

♻️ Proposed helper

def_xero_auth_required() ->Status[XeroAuthRequiredOut]:
"""Build the one 401 body every Xero document endpoint returns."""returnStatus(
401,
XeroAuthRequiredOut(
success=False,
redirect_to_auth=True,
message="Your Xero session has expired. Please log in again.",
),
)

As per coding guidelines: "Use one implementation per concept; search before implementing and extend a near-match instead of creating a parallel implementation."

🤖 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/xero/api.py` around lines 358 - 366, Extract the repeated
get_valid_token() failure response into a shared _xero_auth_required() helper
returning the existing 401 Status[XeroAuthRequiredOut] payload, then replace the
guards in all four Xero document endpoints with that helper. Also extract and
reuse a helper for the identical purchase-order lookup and supplier guard,
preserving the current validation behavior and messages.

Source: Coding guidelines

🤖 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 `@apps/job/tests/test_finish_api.py`:
- Around line 105-115: Correct the inline comment in test_lists_job_invoices to
state that the second invoice belongs to the same company but has no job, so it
must not appear in the job-scoped results. Do not change the test setup or
assertions.
In `@apps/xero/api.py`:
- Around line 64-72: Consolidate the duplicated `_staff` helpers by moving the
implementation from `apps/job/api.py` into `apps/core` and importing that shared
helper in `apps/xero/api.py`, `apps/purchasing/api.py`, and the applicable
callers. Remove redundant local definitions, while preserving any
Timesheet-specific narrowing behavior only if it cannot use the shared
implementation.
- Around line 402-405: Update the invoice creation flow around
XeroInvoiceManager.create_document to make retries safe at the Xero API
boundary: either supply a stable idempotency key for the same job/document
across retry attempts, or disable/prohibit RateLimitedRESTClient retries for
this non-idempotent POST. Preserve retries for safe operations and ensure one
logical create cannot produce duplicate Xero documents.
In `@apps/xero/documents/base.py`:
- Around line 82-99: Move the document-type validation before the best-effort
exception handler in both _add_xero_history_note (apps/xero/documents/base.py,
lines 82-99) and _add_history_note (apps/xero/provider.py, lines 407-421):
reject values outside ("invoice", "quote") before entering try, while leaving
provider calls inside the existing handler so only provider failures are
absorbed.
In `@apps/xero/documents/invoice.py`:
- Around line 232-241: Update create_document to validate that billing_metadata
contains target_total, prior_invoiced_total, and calculated_amount before any
direct reads. After validation, access all three keys directly and remove the
"0" defaults from the Decimal conversions so missing contract fields fail rather
than producing an uncomputed remaining_to_invoice value.
- Around line 198-216: Update the invoice creation flow after the existing
result.external_id and result.number validation to validate that raw contains
_sub_total, _total_tax, _total, and _amount_due before constructing Invoice.
Fail early using the existing validation pattern when any key is missing, then
read the validated values without zero fallbacks so invoice totals cannot be
silently stored as 0.00.
In `@apps/xero/documents/po.py`:
- Around line 23-25: Move ZERO_UUID to a shared module, remove the duplicate
definitions from documents/po.py and XeroAccountingProvider, and import the
shared constant in both consumers. Preserve the existing sentinel value and all
checks that use it.
- Around line 55-59: The Xero sync validation path must enforce the draft-only
rule for first-time creation. Update validate_for_xero_sync() to invoke
state_valid_for_xero() before allowing creation, while preserving updates for
POs with an existing Xero ID; keep the implementation and docstring of
state_valid_for_xero() consistent with this behavior.
In `@apps/xero/provider.py`:
- Around line 227-237: The invoice pre-read in the deletion flow must validate
that the API response contains an invoice and that its contact is present before
constructing the DELETED Invoice. Mirror the guards and user-facing error
messages used by delete_purchase_order, referencing the existing
invoice-deletion method and its existing/api/contact symbols while preserving
the normal construction path for valid data.
- Around line 280-314: Update the zero-UUID recovery flow around
_find_po_by_number so that when recovery returns None and po_id remains
ZERO_UUID, return a failed DocumentResult with no external_id instead of
continuing to the success path. Preserve the existing validation-error handling
and only construct the online_url and successful result after a real purchase
order ID is available.
- Around line 316-335: Update _find_po_by_number to enforce a finite maximum
page count while scanning purchase orders. Stop paging and return None once the
limit is reached, while preserving the existing match return and empty-page
termination behavior.
In `@frontend/src/features/job/JobInvoiceCard.tsx`:
- Around line 344-359: Give both numeric inputs in JobInvoiceCard accessible
names: add a visible label or aria-label to the percentage input at
frontend/src/features/job/JobInvoiceCard.tsx:344-359 identifying it as “Invoice
percentage of quote,” and to the amount input at
frontend/src/features/job/JobInvoiceCard.tsx:396-410 identifying it as “Invoice
amount.”
- Around line 233-239: Update the invoice link handler’s window.open call to
pass noopener,noreferrer in its third argument when opening invoice.online_url,
while preserving the existing URL check and toast behavior.
In `@frontend/src/lib/format.ts`:
- Around line 26-34: Update the NZ_DATE formatter used by formatDate to set
timeZone to Pacific/Auckland, ensuring invoice dates render consistently across
environments. Revise the formatter comment to state that per-page formatters are
rejected because invoice dates must display identical text across the workspace.
---
Nitpick comments:
In `@apps/xero/api.py`:
- Around line 317-328: Update the _document_error_status parameter annotation
from object to int | None, matching the status contract declared by
XeroDocumentResponse. Keep the existing 404 handling and 400 fallback unchanged,
and let mypy validate all call sites against the narrowed type.
- Around line 358-366: Extract the repeated get_valid_token() failure response
into a shared _xero_auth_required() helper returning the existing 401
Status[XeroAuthRequiredOut] payload, then replace the guards in all four Xero
document endpoints with that helper. Also extract and reuse a helper for the
identical purchase-order lookup and supplier guard, preserving the current
validation behavior and messages.
In `@apps/xero/tests/test_invoice_manager.py`:
- Around line 86-138: Add a success-case test in TestErrorContract for
delete_document using a configured provider and Xero invoice ID, then assert the
local Invoice is deleted, recalculate_job_invoicing_state is called, and the
invoice_deleted job event records the invoice number. Keep the test focused on
the same-request effects of the happy path.
In `@apps/xero/tests/test_po_manager.py`:
- Around line 98-109: Extend test_zero_uuid_is_treated_as_unsynced to cover the
write path: configure the provider’s successful DocumentResult with
external_id=ZERO_UUID, run manager.sync_to_xero(), then refresh or inspect
po.xero_id and assert it remains None. Preserve the existing create-path
assertion while exercising _save_po_with_xero_data’s zero-UUID handling.
In `@frontend/src/api/index.ts`:
- Line 81: Remove the code-narration comments at frontend/src/api/index.ts:81-81
and frontend/src/features/job/JobFinishTab.tsx:157-157; no replacement
documentation is needed because neither comment states a factual constraint or
rejected alternative.

🪄 Autofix

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: ea55edb0-3b70-4970-ae0f-3dafb7b206f3

📥 Commits

Reviewing files that changed from the base of the PR and between b5feeab and d1eea03.

⛔ Files ignored due to path filters (5)

• frontend/src/api/generated/@tanstack/react-query.gen.ts is excluded by !**/generated/**
• frontend/src/api/generated/index.ts is excluded by !**/generated/**
• frontend/src/api/generated/sdk.gen.ts is excluded by !**/generated/**
• frontend/src/api/generated/types.gen.ts is excluded by !**/generated/**
• frontend/src/api/generated/zod.gen.ts is excluded by !**/generated/**

📒 Files selected for processing (32)

• apps/accounting/provider.py
• apps/accounting/services/finish_job_summary.py
• apps/accounting/tests/test_finish_job_summary.py
• apps/accounting/types.py
• apps/company/tests/job_fixtures.py
• apps/job/api.py
• apps/job/schemas.py
• apps/job/services/job_service.py
• apps/job/tests/test_finish_api.py
• apps/xero/api.py
• apps/xero/documents/base.py
• apps/xero/documents/invoice.py
• apps/xero/documents/po.py
• apps/xero/helpers.py
• apps/xero/provider.py
• apps/xero/readonly_provider.py
• apps/xero/tests/test_invoice_manager.py
• apps/xero/tests/test_po_manager.py
• docs/accepted-api-differences.yml
• docs/code-quality.md
• docs/rewrite-status.md
• frontend/schema.v2.yml
• frontend/src/api/error-message.ts
• frontend/src/api/index.ts
• frontend/src/features/job/JobDetailPage.tsx
• frontend/src/features/job/JobFinishTab.test.tsx
• frontend/src/features/job/JobFinishTab.tsx
• frontend/src/features/job/JobInvoiceCard.tsx
• frontend/src/lib/format.ts
• frontend/tests/e2e/job/job-xero-invoice.spec.ts
• scripts/v1-frontend-operations.yml
• stubs/xero_python/accounting/__init__.pyi

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

docs/code-quality.md (1)

24-34: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Regenerate the suppression total.

Line 24 reports 431, but the visible rows sum to 430: fixed suppressions total 13 and rule-coded suppressions total 417. scripts/checks/code_quality.py derives the total from the counted entries, so this report is internally inconsistent.

Regenerate the file, then run uv run python -m scripts.checks.code_quality --check.

Expected table correction

-| TOTAL suppressions | 431 |+| TOTAL suppressions | 430 |

The canonical metric generator in scripts/checks/code_quality.py is the source of truth.

🤖 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/code-quality.md` around lines 24 - 34, Regenerate the suppression table
in docs/code-quality.md using scripts/checks/code_quality.py so TOTAL
suppressions matches the visible counted entries (430). Then run uv run python
-m scripts.checks.code_quality --check and ensure validation passes.

♻️ Duplicate comments (1)

apps/xero/documents/invoice.py (1)

246-251: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Validate billing_metadata before invoice creation.

Lines 249-251 parse required values after provider.create_invoice() and Invoice.objects.create(). If a required key is missing or non-decimal, the request fails after the financial operation has completed.

Parse and validate these fields at the start of create_document, before provider submission. Reuse the validated values when creating the audit event.

As per coding guidelines: “Fail early: check invalid cases first, validate required inputs up front.”

🤖 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/xero/documents/invoice.py` around lines 246 - 251, Update
create_document to parse and validate billing_metadata["target_total"],
billing_metadata["prior_invoiced_total"], and
billing_metadata["calculated_amount"] before calling provider.create_invoice()
or Invoice.objects.create(). Reuse these validated Decimal values when
constructing the audit event, removing the later direct parsing while preserving
the existing required-key and invalid-decimal failures.

Source: Coding guidelines

🧹 Nitpick comments (1)

apps/xero/tests/test_provider_documents.py (1)

216-236: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert the recovery pagination request.

_find_po_by_number iterates get_purchase_orders(tenant_id, page=page), but this test only asserts two calls. A regression that requests page one twice can still pass because the mocked side_effect returns a different response each time.

Assert the page argument for both get_purchase_orders calls.

🤖 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/xero/tests/test_provider_documents.py` around lines 216 - 236, Update
test_zero_uuid_recovers_real_id_across_pages to inspect
api.get_purchase_orders.call_args_list and assert the two calls use page=1 and
page=2 respectively, while preserving the existing tenant_id assertions and
recovery expectations.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@docs/code-quality.md`:
- Around line 24-34: Regenerate the suppression table in docs/code-quality.md
using scripts/checks/code_quality.py so TOTAL suppressions matches the visible
counted entries (430). Then run uv run python -m scripts.checks.code_quality
--check and ensure validation passes.
---
Duplicate comments:
In `@apps/xero/documents/invoice.py`:
- Around line 246-251: Update create_document to parse and validate
billing_metadata["target_total"], billing_metadata["prior_invoiced_total"], and
billing_metadata["calculated_amount"] before calling provider.create_invoice()
or Invoice.objects.create(). Reuse these validated Decimal values when
constructing the audit event, removing the later direct parsing while preserving
the existing required-key and invalid-decimal failures.
---
Nitpick comments:
In `@apps/xero/tests/test_provider_documents.py`:
- Around line 216-236: Update test_zero_uuid_recovers_real_id_across_pages to
inspect api.get_purchase_orders.call_args_list and assert the two calls use
page=1 and page=2 respectively, while preserving the existing tenant_id
assertions and recovery expectations.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 10f8aa42-f281-4b7d-8f3f-3e62a97f3bec

📥 Commits

Reviewing files that changed from the base of the PR and between d1eea03 and b318f2e.

📒 Files selected for processing (14)

• apps/job/tests/test_finish_api.py
• apps/xero/api.py
• apps/xero/constants.py
• apps/xero/documents/base.py
• apps/xero/documents/invoice.py
• apps/xero/documents/po.py
• apps/xero/provider.py
• apps/xero/tests/test_document_api.py
• apps/xero/tests/test_po_manager.py
• apps/xero/tests/test_provider_documents.py
• docs/code-quality.md
• docs/rewrite-status.md
• frontend/src/features/job/JobInvoiceCard.tsx
• frontend/src/lib/format.ts

🚧 Files skipped from review as they are similar to previous changes (9)

• frontend/src/lib/format.ts
• apps/job/tests/test_finish_api.py
• apps/xero/documents/base.py
• apps/xero/tests/test_po_manager.py
• docs/rewrite-status.md
• apps/xero/provider.py
• apps/xero/api.py
• frontend/src/features/job/JobInvoiceCard.tsx
• apps/xero/documents/po.py

[corrin]

Round-2 items, addressed in 2fb241c:

• billing_metadata validation ordering — fixed: the three Decimals now parse at the top of create_document, before anything reaches the provider, and the audit event reuses the validated values.
• page assertions in the zero-UUID recovery test — fixed: the test now asserts page=1, page=2 from call_args_list.
• docs/code-quality.md TOTAL 'mismatch' — rebutting: the file is generated and uv run python -m scripts.checks.code_quality --check passes against it, so it is exactly what the generator produces; the TOTAL row includes suppression categories beyond the per-code breakdown rows by design. Hand-editing a generated file is what the gate exists to prevent.

[corrin]

Verification upgraded per the corrected test regime: XERO_READONLY is only for dev sessions pointed at production (hotfixes) — testing runs with writes live. Re-ran the full suite with XERO_READONLY=false against the demo tenant: 38 passed, with job-xero-invoice exercising the real provider path end-to-end (actual create_invoices, workshop-PDF attachment, history note — 9.7s vs 5.3s under readonly, the difference being the real Xero round-trips). Local .env flipped to XERO_READONLY=false as the standing dev configuration; rewrite-status records the regime in 46a06d3.

[corrin]

Slice 2b — invoice path: document push + Finish Job (job-xero-invoice green, 13/40)

Second of the three slice-2 PRs (2a sync engine #46 · 2b this · 2c quote path next). Ports the Xero document push and the Finish Job workspace from v1.

What ports

Document manager base + invoice push (apps/xero/documents/{base,invoice}.py, apps/xero/helpers.py)

• XeroDocumentManager with the config guards (branding theme, quote terms), best-effort history notes, and fail-early construction; XeroInvoiceManager builds a provider-agnostic InvoicePayload, calls get_provider().create_invoice(), and owns local persistence: Invoice mirror row from the provider's canonical raw payload, billing_metadata audit trail, JobEvent, and a same-request recalculate_job_invoicing_state (ledgered — v1 left fully_invoiced to the hourly sync, which under XERO_READONLY never sees the invoice).
• Endpoints POST /api/xero/create_invoice/{job_id} and DELETE /api/xero/delete_invoice/{job_id} keep v1's URL fragments so the spec's request matcher ports unedited. Amount derivation goes through the already-ported calculate_invoice_amount (modes: full / costs-to-date / percent / amount).

Purchase-order push (apps/xero/documents/po.py) — USER-COMMITTED scope

• Create-vs-update keyed on xero_id with the zero-UUID sentinel treated as absent on read and write (v1 stored it), paged recovery of the real id by PO number, per-line xero_line_item_id backfill with duplicate-safe description matching, status mapping that raises on unknown statuses. No spec covers PO push; apps/xero/tests/test_po_manager.py (12 tests) is its gate.
• Ledgered: v1's successful PO delete always returned 500 (its success dict failed its own response serializer).

Readonly works by construction: XeroReadOnlyProvider fabricates well-formed results — INV-E2E-* numbers, GST-exclusive fake totals from the line items × CompanyDefaults.gst_rate — so the endpoint path is byte-identical under XERO_READONLY and the E2E balance-settles-to-$0 assertion exercises the real persistence code. Missing write overrides hit tripwires, not the tenant.

Finish Job backend (apps/accounting/services/finish_job_summary.py, apps/job/api.py)

• Server-owned balance (v1-verbatim arithmetic: remaining excl/incl GST, outstanding, over-invoiced reported rather than netted off), completion-checklist read/patch through Job.save()'s field-change audit machinery (unknown keys 422 via extra="forbid", schema field lists pinned to Job.COMPLETION_CHECKLIST_FIELDS at import), and the ETagged invoice list. The finish GET is deliberately un-ETagged: the balance moves when invoices sync in without touching job.updated_at.

Frontend (JobFinishTab.tsx, JobInvoiceCard.tsx, lazy-loaded into JobDetailPage)

• Balance, checklist, labour-hours cards, estimate/quote/actual comparison, quote accuracy; invoice list with create dialog (modes per pricing methodology) and delete. All money formatted from server values — no client recomputation. All v1 automation ids preserved. apiErrorMessage now also reads the document endpoints' error key so calc/configuration guidance reaches the user.

Spec: job-xero-invoice.spec.ts ports near-verbatim, with one deliberate deviation — it creates its own job instead of using sharedEditJobUrl, because it fully invoices the job and the shared fixture is read-only by contract (v1 mutated it and survived on run ordering).

Adversarial review (2 subagents) — notable catches, all fixed

• Provider-driven HTTP statuses (429/503 on live-Xero failures, 401 mid-request revocation) weren't in the declared response maps — ninja would raise ConfigError instead of returning the error payload. Statuses now clamp to the declared map; the cause stays in error (ADR 0038).
• The PO zero-UUID sentinel could be stored when the unpaginated recovery listing missed the PO (>100 POs) — guard added on write, recovery listing paged.
• TanStack v5 refetch() never rejects, so the "list could not be refreshed" toast was unreachable; an invoices load error rendered as the "No invoices" empty state.
• Known, deliberately not touched (v1-parity): calculate_invoice_amount doesn't quantize, so a sub-cent basis total (e.g. 157.985) can never read fully-invoiced. Real jobs have 2dp values; flagged for 2c or later rather than changing the money path mid-slice.

Verified

• ./scripts/ops/run_e2e.sh: 38 passed — job-xero-invoice green under XERO_READONLY (13/40 specs), all prior specs green with the sync beat live.
• Backend: 1617 unit tests green; new coverage: PO manager 12, invoice manager 7 (error contract / raw_json / readonly fabrication), finish summary 11, finish API 7. Frontend: JobFinishTab component tests 3.
• Expensive tier clean (mypy strict zero-baseline, import-linter, find-duplicates, deptry, schema-current, status table).
• Parity ledger: renamed: entries for all four document operation ids; two new behaviour entries in docs/accepted-api-differences.yml.

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features
  • Added a Finish Job workspace with financial summaries, invoice balances, cost comparisons, completion checklists, and variance indicators.
  • Added invoice listing and management, including full, percentage-based, cost-to-date, and custom-amount invoicing.
  • Added Xero invoice and purchase-order creation, updating, deletion, and invoice file attachments.
  • Added checklist updates with validation and activity history.
• Bug Fixes
  • Invoice changes now immediately recalculate the job’s fully invoiced status and remaining balance.
  • Improved error messages for accounting and Xero operations.

[coderabbitai]

Warning

Review limit reached

@corrin, you've reached your PR review limit, so we couldn't start this review.

Next review available in:24 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 052efda7-ef2c-4202-adef-941ebddc6980

📥 Commits

Reviewing files that changed from the base of the PR and between b318f2e and 09176e1.

📒 Files selected for processing (7)

• apps/xero/documents/invoice.py
• apps/xero/tests/test_invoice_manager.py
• apps/xero/tests/test_provider_documents.py
• config/tests/test_contract_gates.py
• docs/code-quality.md
• docs/rewrite-status.md
• scripts/checks/status_table.py

📝 Walkthrough

Walkthrough

This PR adds Finish Job financial and checklist APIs, Xero invoice and purchase-order synchronization, provider contracts, read-only behavior, and frontend Finish Job and invoice-management components with unit, API, and end-to-end tests.

Changes

Finish Job and Xero workflows

Layer / File(s)	Summary
Finish Job calculations and APIs apps/accounting/..., apps/job/..., frontend/schema.v2.yml	Adds financial summaries, checklist validation and updates, invoice listing, conditional responses, and Finish Job API contracts.
Accounting contracts and Xero provider operations apps/accounting/types.py, apps/accounting/provider.py, apps/xero/helpers.py, apps/xero/provider.py, apps/xero/readonly_provider.py, stubs/xero_python/...	Adds provider-agnostic document DTOs, Xero invoice and purchase-order operations, payload and error helpers, read-only results, and SDK stubs.
Xero document managers and endpoints apps/xero/documents/..., apps/xero/api.py, apps/xero/tests/...	Adds invoice and purchase-order validation, payload construction, persistence, deletion, authenticated endpoints, and manager tests.
Finish Job workspace and invoice UI frontend/src/features/job/..., frontend/src/api/..., frontend/src/lib/format.ts, frontend/tests/e2e/...	Adds lazy-loaded Finish Job and invoice components with financial displays, checklist updates, invoice actions, error handling, and end-to-end coverage.
Compatibility records and progress documentation docs/accepted-api-differences.yml, docs/code-quality.md, docs/rewrite-status.md, scripts/v1-frontend-operations.yml	Records API behavior changes, updated quality metrics, operation rename mappings, and rewrite progress.

Estimated code review effort: 5 (Critical) | ~120 minutes

Sequence Diagram(s)

sequenceDiagram
participant JobFinishTab
participant JobAPI
participant FinishJobSummary
participant JobService
JobFinishTab->>JobAPI: GET finish summary and checklist
JobAPI->>FinishJobSummary: calculate financial totals
FinishJobSummary-->>JobAPI: return summary
JobAPI-->>JobFinishTab: return JobFinishResponse
JobFinishTab->>JobAPI: PATCH checklist fields
JobAPI->>JobService: update checklist
JobService-->>JobAPI: return updated job

Loading

sequenceDiagram
participant JobInvoiceCard
participant XeroAPI
participant XeroInvoiceManager
participant XeroAccountingProvider
JobInvoiceCard->>XeroAPI: submit invoice request
XeroAPI->>XeroInvoiceManager: create document
XeroInvoiceManager->>XeroAccountingProvider: create InvoicePayload
XeroAccountingProvider-->>XeroInvoiceManager: return DocumentResult
XeroInvoiceManager-->>XeroAPI: return persisted invoice result
XeroAPI-->>JobInvoiceCard: return success or error

Loading

Possibly related PRs

• corrin/docketworks_v2#45: Extends the accounting provider and Xero abstractions used by this PR.
• corrin/docketworks_v2#18: Provides purchase-order models and status behavior used by Xero purchase-order synchronization.
• corrin/docketworks_v2#10: Provides the workshop PDF generation used by invoice attachment handling.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 44.10% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the main Xero invoice document push and Finish Job changes, with a useful E2E status indicator.
Description check	✅ Passed	The description is detailed, structured, and covers scope, behavior, verification results, known limitations, and parity changes.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch xero/invoice-path

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 14

🧹 Nitpick comments (5)

frontend/src/api/index.ts (1)

81-81: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Remove code-narration comments or document their constraint.

Both comments describe nearby code without stating the rejected alternative and the factual constraint.

• frontend/src/api/index.ts#L81-L81: Remove the export-group narration, or explain why feature code must import these generated operations through frontend/src/api/index.ts.
• frontend/src/features/job/JobFinishTab.tsx#L157-L157: Remove the ticket-heading comment. It does not document a runtime or business constraint.

As per coding guidelines, “Comments must document the rejected obvious alternative and the factual constraint that rejected it; remove code narration and review-feedback echoes.”

🤖 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 `@frontend/src/api/index.ts` at line 81, Remove the code-narration comments at
frontend/src/api/index.ts:81-81 and
frontend/src/features/job/JobFinishTab.tsx:157-157; no replacement documentation
is needed because neither comment states a factual constraint or rejected
alternative.

Source: Coding guidelines

apps/xero/tests/test_invoice_manager.py (1)

86-138: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a delete happy-path test.

TestErrorContract covers the three failure paths of delete_document. No test covers the success path, so nothing asserts that the local Invoice row is removed, that recalculate_job_invoicing_state runs, and that the invoice_deleted job event records the invoice number. That recalculation is the same same-request effect the create test protects at Line 256-258, and the Finish Job tab depends on it.

Do you want me to write this test?

🤖 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/xero/tests/test_invoice_manager.py` around lines 86 - 138, Add a
success-case test in TestErrorContract for delete_document using a configured
provider and Xero invoice ID, then assert the local Invoice is deleted,
recalculate_job_invoicing_state is called, and the invoice_deleted job event
records the invoice number. Keep the test focused on the same-request effects of
the happy path.

apps/xero/tests/test_po_manager.py (1)

98-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Cover the write direction of the zero-UUID sentinel too.

This test covers the read direction: a stored sentinel routes to create. The module docstring at Line 4-5 names "storing Xero's zero-UUID sentinel as a real id" as the expensive silent failure, and that is the write direction, guarded by _save_po_with_xero_data at apps/xero/documents/po.py Line 151.

Add a test where the provider returns DocumentResult(success=True, external_id=ZERO_UUID) and assert that po.xero_id stays None after sync_to_xero. This also pins the behavior discussed on apps/xero/provider.py Line 280-314.

Do you want me to write this test?

🤖 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/xero/tests/test_po_manager.py` around lines 98 - 109, Extend
test_zero_uuid_is_treated_as_unsynced to cover the write path: configure the
provider’s successful DocumentResult with external_id=ZERO_UUID, run
manager.sync_to_xero(), then refresh or inspect po.xero_id and assert it remains
None. Preserve the existing create-path assertion while exercising
_save_po_with_xero_data’s zero-UUID handling.

apps/xero/api.py (2)

317-328: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Type the clamp parameter as the contract, not as object.

XeroDocumentResponse declares status. Annotating the parameter object discards that contract and forces a runtime comparison against an untyped value. Declare int | None so mypy checks every call site.

♻️ Proposed signature change

-def _document_error_status(status: object) -> int:+def _document_error_status(status: int | None) -> int:

As per coding guidelines: "Treat type annotations as data contracts: use named types such as dataclasses, TypedDicts, or Protocols for complex inline types."

🤖 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/xero/api.py` around lines 317 - 328, Update the _document_error_status
parameter annotation from object to int | None, matching the status contract
declared by XeroDocumentResponse. Keep the existing 404 handling and 400
fallback unchanged, and let mypy validate all call sites against the narrowed
type.

Source: Coding guidelines

---

358-366: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract the repeated Xero-auth 401 block.

The same get_valid_token() guard and XeroAuthRequiredOut payload appear in all four new endpoints (lines 358-366, 458-466, 543-551, 612-620). The two purchase-order endpoints also repeat the identical lookup and supplier guard. One helper per repeated block keeps the message and status in step.

♻️ Proposed helper

def_xero_auth_required() ->Status[XeroAuthRequiredOut]:
"""Build the one 401 body every Xero document endpoint returns."""returnStatus(
401,
XeroAuthRequiredOut(
success=False,
redirect_to_auth=True,
message="Your Xero session has expired. Please log in again.",
),
)

As per coding guidelines: "Use one implementation per concept; search before implementing and extend a near-match instead of creating a parallel implementation."

🤖 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/xero/api.py` around lines 358 - 366, Extract the repeated
get_valid_token() failure response into a shared _xero_auth_required() helper
returning the existing 401 Status[XeroAuthRequiredOut] payload, then replace the
guards in all four Xero document endpoints with that helper. Also extract and
reuse a helper for the identical purchase-order lookup and supplier guard,
preserving the current validation behavior and messages.

Source: Coding guidelines

🤖 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 `@apps/job/tests/test_finish_api.py`:
- Around line 105-115: Correct the inline comment in test_lists_job_invoices to
state that the second invoice belongs to the same company but has no job, so it
must not appear in the job-scoped results. Do not change the test setup or
assertions.
In `@apps/xero/api.py`:
- Around line 64-72: Consolidate the duplicated `_staff` helpers by moving the
implementation from `apps/job/api.py` into `apps/core` and importing that shared
helper in `apps/xero/api.py`, `apps/purchasing/api.py`, and the applicable
callers. Remove redundant local definitions, while preserving any
Timesheet-specific narrowing behavior only if it cannot use the shared
implementation.
- Around line 402-405: Update the invoice creation flow around
XeroInvoiceManager.create_document to make retries safe at the Xero API
boundary: either supply a stable idempotency key for the same job/document
across retry attempts, or disable/prohibit RateLimitedRESTClient retries for
this non-idempotent POST. Preserve retries for safe operations and ensure one
logical create cannot produce duplicate Xero documents.
In `@apps/xero/documents/base.py`:
- Around line 82-99: Move the document-type validation before the best-effort
exception handler in both _add_xero_history_note (apps/xero/documents/base.py,
lines 82-99) and _add_history_note (apps/xero/provider.py, lines 407-421):
reject values outside ("invoice", "quote") before entering try, while leaving
provider calls inside the existing handler so only provider failures are
absorbed.
In `@apps/xero/documents/invoice.py`:
- Around line 232-241: Update create_document to validate that billing_metadata
contains target_total, prior_invoiced_total, and calculated_amount before any
direct reads. After validation, access all three keys directly and remove the
"0" defaults from the Decimal conversions so missing contract fields fail rather
than producing an uncomputed remaining_to_invoice value.
- Around line 198-216: Update the invoice creation flow after the existing
result.external_id and result.number validation to validate that raw contains
_sub_total, _total_tax, _total, and _amount_due before constructing Invoice.
Fail early using the existing validation pattern when any key is missing, then
read the validated values without zero fallbacks so invoice totals cannot be
silently stored as 0.00.
In `@apps/xero/documents/po.py`:
- Around line 23-25: Move ZERO_UUID to a shared module, remove the duplicate
definitions from documents/po.py and XeroAccountingProvider, and import the
shared constant in both consumers. Preserve the existing sentinel value and all
checks that use it.
- Around line 55-59: The Xero sync validation path must enforce the draft-only
rule for first-time creation. Update validate_for_xero_sync() to invoke
state_valid_for_xero() before allowing creation, while preserving updates for
POs with an existing Xero ID; keep the implementation and docstring of
state_valid_for_xero() consistent with this behavior.
In `@apps/xero/provider.py`:
- Around line 227-237: The invoice pre-read in the deletion flow must validate
that the API response contains an invoice and that its contact is present before
constructing the DELETED Invoice. Mirror the guards and user-facing error
messages used by delete_purchase_order, referencing the existing
invoice-deletion method and its existing/api/contact symbols while preserving
the normal construction path for valid data.
- Around line 280-314: Update the zero-UUID recovery flow around
_find_po_by_number so that when recovery returns None and po_id remains
ZERO_UUID, return a failed DocumentResult with no external_id instead of
continuing to the success path. Preserve the existing validation-error handling
and only construct the online_url and successful result after a real purchase
order ID is available.
- Around line 316-335: Update _find_po_by_number to enforce a finite maximum
page count while scanning purchase orders. Stop paging and return None once the
limit is reached, while preserving the existing match return and empty-page
termination behavior.
In `@frontend/src/features/job/JobInvoiceCard.tsx`:
- Around line 344-359: Give both numeric inputs in JobInvoiceCard accessible
names: add a visible label or aria-label to the percentage input at
frontend/src/features/job/JobInvoiceCard.tsx:344-359 identifying it as “Invoice
percentage of quote,” and to the amount input at
frontend/src/features/job/JobInvoiceCard.tsx:396-410 identifying it as “Invoice
amount.”
- Around line 233-239: Update the invoice link handler’s window.open call to
pass noopener,noreferrer in its third argument when opening invoice.online_url,
while preserving the existing URL check and toast behavior.
In `@frontend/src/lib/format.ts`:
- Around line 26-34: Update the NZ_DATE formatter used by formatDate to set
timeZone to Pacific/Auckland, ensuring invoice dates render consistently across
environments. Revise the formatter comment to state that per-page formatters are
rejected because invoice dates must display identical text across the workspace.
---
Nitpick comments:
In `@apps/xero/api.py`:
- Around line 317-328: Update the _document_error_status parameter annotation
from object to int | None, matching the status contract declared by
XeroDocumentResponse. Keep the existing 404 handling and 400 fallback unchanged,
and let mypy validate all call sites against the narrowed type.
- Around line 358-366: Extract the repeated get_valid_token() failure response
into a shared _xero_auth_required() helper returning the existing 401
Status[XeroAuthRequiredOut] payload, then replace the guards in all four Xero
document endpoints with that helper. Also extract and reuse a helper for the
identical purchase-order lookup and supplier guard, preserving the current
validation behavior and messages.
In `@apps/xero/tests/test_invoice_manager.py`:
- Around line 86-138: Add a success-case test in TestErrorContract for
delete_document using a configured provider and Xero invoice ID, then assert the
local Invoice is deleted, recalculate_job_invoicing_state is called, and the
invoice_deleted job event records the invoice number. Keep the test focused on
the same-request effects of the happy path.
In `@apps/xero/tests/test_po_manager.py`:
- Around line 98-109: Extend test_zero_uuid_is_treated_as_unsynced to cover the
write path: configure the provider’s successful DocumentResult with
external_id=ZERO_UUID, run manager.sync_to_xero(), then refresh or inspect
po.xero_id and assert it remains None. Preserve the existing create-path
assertion while exercising _save_po_with_xero_data’s zero-UUID handling.
In `@frontend/src/api/index.ts`:
- Line 81: Remove the code-narration comments at frontend/src/api/index.ts:81-81
and frontend/src/features/job/JobFinishTab.tsx:157-157; no replacement
documentation is needed because neither comment states a factual constraint or
rejected alternative.

🪄 Autofix

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: ea55edb0-3b70-4970-ae0f-3dafb7b206f3

📥 Commits

Reviewing files that changed from the base of the PR and between b5feeab and d1eea03.

⛔ Files ignored due to path filters (5)

• frontend/src/api/generated/@tanstack/react-query.gen.ts is excluded by !**/generated/**
• frontend/src/api/generated/index.ts is excluded by !**/generated/**
• frontend/src/api/generated/sdk.gen.ts is excluded by !**/generated/**
• frontend/src/api/generated/types.gen.ts is excluded by !**/generated/**
• frontend/src/api/generated/zod.gen.ts is excluded by !**/generated/**

📒 Files selected for processing (32)

• apps/accounting/provider.py
• apps/accounting/services/finish_job_summary.py
• apps/accounting/tests/test_finish_job_summary.py
• apps/accounting/types.py
• apps/company/tests/job_fixtures.py
• apps/job/api.py
• apps/job/schemas.py
• apps/job/services/job_service.py
• apps/job/tests/test_finish_api.py
• apps/xero/api.py
• apps/xero/documents/base.py
• apps/xero/documents/invoice.py
• apps/xero/documents/po.py
• apps/xero/helpers.py
• apps/xero/provider.py
• apps/xero/readonly_provider.py
• apps/xero/tests/test_invoice_manager.py
• apps/xero/tests/test_po_manager.py
• docs/accepted-api-differences.yml
• docs/code-quality.md
• docs/rewrite-status.md
• frontend/schema.v2.yml
• frontend/src/api/error-message.ts
• frontend/src/api/index.ts
• frontend/src/features/job/JobDetailPage.tsx
• frontend/src/features/job/JobFinishTab.test.tsx
• frontend/src/features/job/JobFinishTab.tsx
• frontend/src/features/job/JobInvoiceCard.tsx
• frontend/src/lib/format.ts
• frontend/tests/e2e/job/job-xero-invoice.spec.ts
• scripts/v1-frontend-operations.yml
• stubs/xero_python/accounting/__init__.pyi

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

docs/code-quality.md (1)

24-34: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Regenerate the suppression total.

Line 24 reports 431, but the visible rows sum to 430: fixed suppressions total 13 and rule-coded suppressions total 417. scripts/checks/code_quality.py derives the total from the counted entries, so this report is internally inconsistent.

Regenerate the file, then run uv run python -m scripts.checks.code_quality --check.

Expected table correction

-| TOTAL suppressions | 431 |+| TOTAL suppressions | 430 |

The canonical metric generator in scripts/checks/code_quality.py is the source of truth.

🤖 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/code-quality.md` around lines 24 - 34, Regenerate the suppression table
in docs/code-quality.md using scripts/checks/code_quality.py so TOTAL
suppressions matches the visible counted entries (430). Then run uv run python
-m scripts.checks.code_quality --check and ensure validation passes.

♻️ Duplicate comments (1)

apps/xero/documents/invoice.py (1)

246-251: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Validate billing_metadata before invoice creation.

Lines 249-251 parse required values after provider.create_invoice() and Invoice.objects.create(). If a required key is missing or non-decimal, the request fails after the financial operation has completed.

Parse and validate these fields at the start of create_document, before provider submission. Reuse the validated values when creating the audit event.

As per coding guidelines: “Fail early: check invalid cases first, validate required inputs up front.”

🤖 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/xero/documents/invoice.py` around lines 246 - 251, Update
create_document to parse and validate billing_metadata["target_total"],
billing_metadata["prior_invoiced_total"], and
billing_metadata["calculated_amount"] before calling provider.create_invoice()
or Invoice.objects.create(). Reuse these validated Decimal values when
constructing the audit event, removing the later direct parsing while preserving
the existing required-key and invalid-decimal failures.

Source: Coding guidelines

🧹 Nitpick comments (1)

apps/xero/tests/test_provider_documents.py (1)

216-236: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert the recovery pagination request.

_find_po_by_number iterates get_purchase_orders(tenant_id, page=page), but this test only asserts two calls. A regression that requests page one twice can still pass because the mocked side_effect returns a different response each time.

Assert the page argument for both get_purchase_orders calls.

🤖 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/xero/tests/test_provider_documents.py` around lines 216 - 236, Update
test_zero_uuid_recovers_real_id_across_pages to inspect
api.get_purchase_orders.call_args_list and assert the two calls use page=1 and
page=2 respectively, while preserving the existing tenant_id assertions and
recovery expectations.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@docs/code-quality.md`:
- Around line 24-34: Regenerate the suppression table in docs/code-quality.md
using scripts/checks/code_quality.py so TOTAL suppressions matches the visible
counted entries (430). Then run uv run python -m scripts.checks.code_quality
--check and ensure validation passes.
---
Duplicate comments:
In `@apps/xero/documents/invoice.py`:
- Around line 246-251: Update create_document to parse and validate
billing_metadata["target_total"], billing_metadata["prior_invoiced_total"], and
billing_metadata["calculated_amount"] before calling provider.create_invoice()
or Invoice.objects.create(). Reuse these validated Decimal values when
constructing the audit event, removing the later direct parsing while preserving
the existing required-key and invalid-decimal failures.
---
Nitpick comments:
In `@apps/xero/tests/test_provider_documents.py`:
- Around line 216-236: Update test_zero_uuid_recovers_real_id_across_pages to
inspect api.get_purchase_orders.call_args_list and assert the two calls use
page=1 and page=2 respectively, while preserving the existing tenant_id
assertions and recovery expectations.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 10f8aa42-f281-4b7d-8f3f-3e62a97f3bec

📥 Commits

Reviewing files that changed from the base of the PR and between d1eea03 and b318f2e.

📒 Files selected for processing (14)

• apps/job/tests/test_finish_api.py
• apps/xero/api.py
• apps/xero/constants.py
• apps/xero/documents/base.py
• apps/xero/documents/invoice.py
• apps/xero/documents/po.py
• apps/xero/provider.py
• apps/xero/tests/test_document_api.py
• apps/xero/tests/test_po_manager.py
• apps/xero/tests/test_provider_documents.py
• docs/code-quality.md
• docs/rewrite-status.md
• frontend/src/features/job/JobInvoiceCard.tsx
• frontend/src/lib/format.ts

🚧 Files skipped from review as they are similar to previous changes (9)

• frontend/src/lib/format.ts
• apps/job/tests/test_finish_api.py
• apps/xero/documents/base.py
• apps/xero/tests/test_po_manager.py
• docs/rewrite-status.md
• apps/xero/provider.py
• apps/xero/api.py
• frontend/src/features/job/JobInvoiceCard.tsx
• apps/xero/documents/po.py

[corrin]

Round-2 items, addressed in 2fb241c:

• billing_metadata validation ordering — fixed: the three Decimals now parse at the top of create_document, before anything reaches the provider, and the audit event reuses the validated values.
• page assertions in the zero-UUID recovery test — fixed: the test now asserts page=1, page=2 from call_args_list.
• docs/code-quality.md TOTAL 'mismatch' — rebutting: the file is generated and uv run python -m scripts.checks.code_quality --check passes against it, so it is exactly what the generator produces; the TOTAL row includes suppression categories beyond the per-code breakdown rows by design. Hand-editing a generated file is what the gate exists to prevent.

[corrin]

Verification upgraded per the corrected test regime: XERO_READONLY is only for dev sessions pointed at production (hotfixes) — testing runs with writes live. Re-ran the full suite with XERO_READONLY=false against the demo tenant: 38 passed, with job-xero-invoice exercising the real provider path end-to-end (actual create_invoices, workshop-PDF attachment, history note — 9.7s vs 5.3s under readonly, the difference being the real Xero round-trips). Local .env flipped to XERO_READONLY=false as the standing dev configuration; rewrite-status records the regime in 46a06d3.

[corrin]

Slice 2b — invoice path: document push + Finish Job (job-xero-invoice green, 13/40)

Second of the three slice-2 PRs (2a sync engine #46 · 2b this · 2c quote path next). Ports the Xero document push and the Finish Job workspace from v1.

What ports

Document manager base + invoice push (apps/xero/documents/{base,invoice}.py, apps/xero/helpers.py)

• XeroDocumentManager with the config guards (branding theme, quote terms), best-effort history notes, and fail-early construction; XeroInvoiceManager builds a provider-agnostic InvoicePayload, calls get_provider().create_invoice(), and owns local persistence: Invoice mirror row from the provider's canonical raw payload, billing_metadata audit trail, JobEvent, and a same-request recalculate_job_invoicing_state (ledgered — v1 left fully_invoiced to the hourly sync, which under XERO_READONLY never sees the invoice).
• Endpoints POST /api/xero/create_invoice/{job_id} and DELETE /api/xero/delete_invoice/{job_id} keep v1's URL fragments so the spec's request matcher ports unedited. Amount derivation goes through the already-ported calculate_invoice_amount (modes: full / costs-to-date / percent / amount).

Purchase-order push (apps/xero/documents/po.py) — USER-COMMITTED scope

• Create-vs-update keyed on xero_id with the zero-UUID sentinel treated as absent on read and write (v1 stored it), paged recovery of the real id by PO number, per-line xero_line_item_id backfill with duplicate-safe description matching, status mapping that raises on unknown statuses. No spec covers PO push; apps/xero/tests/test_po_manager.py (12 tests) is its gate.
• Ledgered: v1's successful PO delete always returned 500 (its success dict failed its own response serializer).

Readonly works by construction: XeroReadOnlyProvider fabricates well-formed results — INV-E2E-* numbers, GST-exclusive fake totals from the line items × CompanyDefaults.gst_rate — so the endpoint path is byte-identical under XERO_READONLY and the E2E balance-settles-to-$0 assertion exercises the real persistence code. Missing write overrides hit tripwires, not the tenant.

Finish Job backend (apps/accounting/services/finish_job_summary.py, apps/job/api.py)

• Server-owned balance (v1-verbatim arithmetic: remaining excl/incl GST, outstanding, over-invoiced reported rather than netted off), completion-checklist read/patch through Job.save()'s field-change audit machinery (unknown keys 422 via extra="forbid", schema field lists pinned to Job.COMPLETION_CHECKLIST_FIELDS at import), and the ETagged invoice list. The finish GET is deliberately un-ETagged: the balance moves when invoices sync in without touching job.updated_at.

Frontend (JobFinishTab.tsx, JobInvoiceCard.tsx, lazy-loaded into JobDetailPage)

• Balance, checklist, labour-hours cards, estimate/quote/actual comparison, quote accuracy; invoice list with create dialog (modes per pricing methodology) and delete. All money formatted from server values — no client recomputation. All v1 automation ids preserved. apiErrorMessage now also reads the document endpoints' error key so calc/configuration guidance reaches the user.

Spec: job-xero-invoice.spec.ts ports near-verbatim, with one deliberate deviation — it creates its own job instead of using sharedEditJobUrl, because it fully invoices the job and the shared fixture is read-only by contract (v1 mutated it and survived on run ordering).

Adversarial review (2 subagents) — notable catches, all fixed

• Provider-driven HTTP statuses (429/503 on live-Xero failures, 401 mid-request revocation) weren't in the declared response maps — ninja would raise ConfigError instead of returning the error payload. Statuses now clamp to the declared map; the cause stays in error (ADR 0038).
• The PO zero-UUID sentinel could be stored when the unpaginated recovery listing missed the PO (>100 POs) — guard added on write, recovery listing paged.
• TanStack v5 refetch() never rejects, so the "list could not be refreshed" toast was unreachable; an invoices load error rendered as the "No invoices" empty state.
• Known, deliberately not touched (v1-parity): calculate_invoice_amount doesn't quantize, so a sub-cent basis total (e.g. 157.985) can never read fully-invoiced. Real jobs have 2dp values; flagged for 2c or later rather than changing the money path mid-slice.

Verified

• ./scripts/ops/run_e2e.sh: 38 passed — job-xero-invoice green under XERO_READONLY (13/40 specs), all prior specs green with the sync beat live.
• Backend: 1617 unit tests green; new coverage: PO manager 12, invoice manager 7 (error contract / raw_json / readonly fabrication), finish summary 11, finish API 7. Frontend: JobFinishTab component tests 3.
• Expensive tier clean (mypy strict zero-baseline, import-linter, find-duplicates, deptry, schema-current, status table).
• Parity ledger: renamed: entries for all four document operation ids; two new behaviour entries in docs/accepted-api-differences.yml.

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features
  • Added a Finish Job workspace with financial summaries, invoice balances, cost comparisons, completion checklists, and variance indicators.
  • Added invoice listing and management, including full, percentage-based, cost-to-date, and custom-amount invoicing.
  • Added Xero invoice and purchase-order creation, updating, deletion, and invoice file attachments.
  • Added checklist updates with validation and activity history.
• Bug Fixes
  • Invoice changes now immediately recalculate the job’s fully invoiced status and remaining balance.
  • Improved error messages for accounting and Xero operations.

[coderabbitai]

Warning

Review limit reached

@corrin, you've reached your PR review limit, so we couldn't start this review.

Next review available in:24 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 052efda7-ef2c-4202-adef-941ebddc6980

📥 Commits

Reviewing files that changed from the base of the PR and between b318f2e and 09176e1.

📒 Files selected for processing (7)

• apps/xero/documents/invoice.py
• apps/xero/tests/test_invoice_manager.py
• apps/xero/tests/test_provider_documents.py
• config/tests/test_contract_gates.py
• docs/code-quality.md
• docs/rewrite-status.md
• scripts/checks/status_table.py

📝 Walkthrough

Walkthrough

This PR adds Finish Job financial and checklist APIs, Xero invoice and purchase-order synchronization, provider contracts, read-only behavior, and frontend Finish Job and invoice-management components with unit, API, and end-to-end tests.

Changes

Finish Job and Xero workflows

Layer / File(s)	Summary
Finish Job calculations and APIs apps/accounting/..., apps/job/..., frontend/schema.v2.yml	Adds financial summaries, checklist validation and updates, invoice listing, conditional responses, and Finish Job API contracts.
Accounting contracts and Xero provider operations apps/accounting/types.py, apps/accounting/provider.py, apps/xero/helpers.py, apps/xero/provider.py, apps/xero/readonly_provider.py, stubs/xero_python/...	Adds provider-agnostic document DTOs, Xero invoice and purchase-order operations, payload and error helpers, read-only results, and SDK stubs.
Xero document managers and endpoints apps/xero/documents/..., apps/xero/api.py, apps/xero/tests/...	Adds invoice and purchase-order validation, payload construction, persistence, deletion, authenticated endpoints, and manager tests.
Finish Job workspace and invoice UI frontend/src/features/job/..., frontend/src/api/..., frontend/src/lib/format.ts, frontend/tests/e2e/...	Adds lazy-loaded Finish Job and invoice components with financial displays, checklist updates, invoice actions, error handling, and end-to-end coverage.
Compatibility records and progress documentation docs/accepted-api-differences.yml, docs/code-quality.md, docs/rewrite-status.md, scripts/v1-frontend-operations.yml	Records API behavior changes, updated quality metrics, operation rename mappings, and rewrite progress.

Estimated code review effort: 5 (Critical) | ~120 minutes

Sequence Diagram(s)

sequenceDiagram
participant JobFinishTab
participant JobAPI
participant FinishJobSummary
participant JobService
JobFinishTab->>JobAPI: GET finish summary and checklist
JobAPI->>FinishJobSummary: calculate financial totals
FinishJobSummary-->>JobAPI: return summary
JobAPI-->>JobFinishTab: return JobFinishResponse
JobFinishTab->>JobAPI: PATCH checklist fields
JobAPI->>JobService: update checklist
JobService-->>JobAPI: return updated job

Loading

sequenceDiagram
participant JobInvoiceCard
participant XeroAPI
participant XeroInvoiceManager
participant XeroAccountingProvider
JobInvoiceCard->>XeroAPI: submit invoice request
XeroAPI->>XeroInvoiceManager: create document
XeroInvoiceManager->>XeroAccountingProvider: create InvoicePayload
XeroAccountingProvider-->>XeroInvoiceManager: return DocumentResult
XeroInvoiceManager-->>XeroAPI: return persisted invoice result
XeroAPI-->>JobInvoiceCard: return success or error

Loading

Possibly related PRs

• corrin/docketworks_v2#45: Extends the accounting provider and Xero abstractions used by this PR.
• corrin/docketworks_v2#18: Provides purchase-order models and status behavior used by Xero purchase-order synchronization.
• corrin/docketworks_v2#10: Provides the workshop PDF generation used by invoice attachment handling.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 44.10% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the main Xero invoice document push and Finish Job changes, with a useful E2E status indicator.
Description check	✅ Passed	The description is detailed, structured, and covers scope, behavior, verification results, known limitations, and parity changes.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch xero/invoice-path

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 14

🧹 Nitpick comments (5)

frontend/src/api/index.ts (1)

81-81: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Remove code-narration comments or document their constraint.

Both comments describe nearby code without stating the rejected alternative and the factual constraint.

• frontend/src/api/index.ts#L81-L81: Remove the export-group narration, or explain why feature code must import these generated operations through frontend/src/api/index.ts.
• frontend/src/features/job/JobFinishTab.tsx#L157-L157: Remove the ticket-heading comment. It does not document a runtime or business constraint.

As per coding guidelines, “Comments must document the rejected obvious alternative and the factual constraint that rejected it; remove code narration and review-feedback echoes.”

🤖 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 `@frontend/src/api/index.ts` at line 81, Remove the code-narration comments at
frontend/src/api/index.ts:81-81 and
frontend/src/features/job/JobFinishTab.tsx:157-157; no replacement documentation
is needed because neither comment states a factual constraint or rejected
alternative.

Source: Coding guidelines

apps/xero/tests/test_invoice_manager.py (1)

86-138: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a delete happy-path test.

TestErrorContract covers the three failure paths of delete_document. No test covers the success path, so nothing asserts that the local Invoice row is removed, that recalculate_job_invoicing_state runs, and that the invoice_deleted job event records the invoice number. That recalculation is the same same-request effect the create test protects at Line 256-258, and the Finish Job tab depends on it.

Do you want me to write this test?

🤖 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/xero/tests/test_invoice_manager.py` around lines 86 - 138, Add a
success-case test in TestErrorContract for delete_document using a configured
provider and Xero invoice ID, then assert the local Invoice is deleted,
recalculate_job_invoicing_state is called, and the invoice_deleted job event
records the invoice number. Keep the test focused on the same-request effects of
the happy path.

apps/xero/tests/test_po_manager.py (1)

98-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Cover the write direction of the zero-UUID sentinel too.

This test covers the read direction: a stored sentinel routes to create. The module docstring at Line 4-5 names "storing Xero's zero-UUID sentinel as a real id" as the expensive silent failure, and that is the write direction, guarded by _save_po_with_xero_data at apps/xero/documents/po.py Line 151.

Add a test where the provider returns DocumentResult(success=True, external_id=ZERO_UUID) and assert that po.xero_id stays None after sync_to_xero. This also pins the behavior discussed on apps/xero/provider.py Line 280-314.

Do you want me to write this test?

🤖 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/xero/tests/test_po_manager.py` around lines 98 - 109, Extend
test_zero_uuid_is_treated_as_unsynced to cover the write path: configure the
provider’s successful DocumentResult with external_id=ZERO_UUID, run
manager.sync_to_xero(), then refresh or inspect po.xero_id and assert it remains
None. Preserve the existing create-path assertion while exercising
_save_po_with_xero_data’s zero-UUID handling.

apps/xero/api.py (2)

317-328: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Type the clamp parameter as the contract, not as object.

XeroDocumentResponse declares status. Annotating the parameter object discards that contract and forces a runtime comparison against an untyped value. Declare int | None so mypy checks every call site.

♻️ Proposed signature change

-def _document_error_status(status: object) -> int:+def _document_error_status(status: int | None) -> int:

As per coding guidelines: "Treat type annotations as data contracts: use named types such as dataclasses, TypedDicts, or Protocols for complex inline types."

🤖 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/xero/api.py` around lines 317 - 328, Update the _document_error_status
parameter annotation from object to int | None, matching the status contract
declared by XeroDocumentResponse. Keep the existing 404 handling and 400
fallback unchanged, and let mypy validate all call sites against the narrowed
type.

Source: Coding guidelines

---

358-366: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract the repeated Xero-auth 401 block.

The same get_valid_token() guard and XeroAuthRequiredOut payload appear in all four new endpoints (lines 358-366, 458-466, 543-551, 612-620). The two purchase-order endpoints also repeat the identical lookup and supplier guard. One helper per repeated block keeps the message and status in step.

♻️ Proposed helper

def_xero_auth_required() ->Status[XeroAuthRequiredOut]:
"""Build the one 401 body every Xero document endpoint returns."""returnStatus(
401,
XeroAuthRequiredOut(
success=False,
redirect_to_auth=True,
message="Your Xero session has expired. Please log in again.",
),
)

As per coding guidelines: "Use one implementation per concept; search before implementing and extend a near-match instead of creating a parallel implementation."

🤖 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/xero/api.py` around lines 358 - 366, Extract the repeated
get_valid_token() failure response into a shared _xero_auth_required() helper
returning the existing 401 Status[XeroAuthRequiredOut] payload, then replace the
guards in all four Xero document endpoints with that helper. Also extract and
reuse a helper for the identical purchase-order lookup and supplier guard,
preserving the current validation behavior and messages.

Source: Coding guidelines

🤖 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 `@apps/job/tests/test_finish_api.py`:
- Around line 105-115: Correct the inline comment in test_lists_job_invoices to
state that the second invoice belongs to the same company but has no job, so it
must not appear in the job-scoped results. Do not change the test setup or
assertions.
In `@apps/xero/api.py`:
- Around line 64-72: Consolidate the duplicated `_staff` helpers by moving the
implementation from `apps/job/api.py` into `apps/core` and importing that shared
helper in `apps/xero/api.py`, `apps/purchasing/api.py`, and the applicable
callers. Remove redundant local definitions, while preserving any
Timesheet-specific narrowing behavior only if it cannot use the shared
implementation.
- Around line 402-405: Update the invoice creation flow around
XeroInvoiceManager.create_document to make retries safe at the Xero API
boundary: either supply a stable idempotency key for the same job/document
across retry attempts, or disable/prohibit RateLimitedRESTClient retries for
this non-idempotent POST. Preserve retries for safe operations and ensure one
logical create cannot produce duplicate Xero documents.
In `@apps/xero/documents/base.py`:
- Around line 82-99: Move the document-type validation before the best-effort
exception handler in both _add_xero_history_note (apps/xero/documents/base.py,
lines 82-99) and _add_history_note (apps/xero/provider.py, lines 407-421):
reject values outside ("invoice", "quote") before entering try, while leaving
provider calls inside the existing handler so only provider failures are
absorbed.
In `@apps/xero/documents/invoice.py`:
- Around line 232-241: Update create_document to validate that billing_metadata
contains target_total, prior_invoiced_total, and calculated_amount before any
direct reads. After validation, access all three keys directly and remove the
"0" defaults from the Decimal conversions so missing contract fields fail rather
than producing an uncomputed remaining_to_invoice value.
- Around line 198-216: Update the invoice creation flow after the existing
result.external_id and result.number validation to validate that raw contains
_sub_total, _total_tax, _total, and _amount_due before constructing Invoice.
Fail early using the existing validation pattern when any key is missing, then
read the validated values without zero fallbacks so invoice totals cannot be
silently stored as 0.00.
In `@apps/xero/documents/po.py`:
- Around line 23-25: Move ZERO_UUID to a shared module, remove the duplicate
definitions from documents/po.py and XeroAccountingProvider, and import the
shared constant in both consumers. Preserve the existing sentinel value and all
checks that use it.
- Around line 55-59: The Xero sync validation path must enforce the draft-only
rule for first-time creation. Update validate_for_xero_sync() to invoke
state_valid_for_xero() before allowing creation, while preserving updates for
POs with an existing Xero ID; keep the implementation and docstring of
state_valid_for_xero() consistent with this behavior.
In `@apps/xero/provider.py`:
- Around line 227-237: The invoice pre-read in the deletion flow must validate
that the API response contains an invoice and that its contact is present before
constructing the DELETED Invoice. Mirror the guards and user-facing error
messages used by delete_purchase_order, referencing the existing
invoice-deletion method and its existing/api/contact symbols while preserving
the normal construction path for valid data.
- Around line 280-314: Update the zero-UUID recovery flow around
_find_po_by_number so that when recovery returns None and po_id remains
ZERO_UUID, return a failed DocumentResult with no external_id instead of
continuing to the success path. Preserve the existing validation-error handling
and only construct the online_url and successful result after a real purchase
order ID is available.
- Around line 316-335: Update _find_po_by_number to enforce a finite maximum
page count while scanning purchase orders. Stop paging and return None once the
limit is reached, while preserving the existing match return and empty-page
termination behavior.
In `@frontend/src/features/job/JobInvoiceCard.tsx`:
- Around line 344-359: Give both numeric inputs in JobInvoiceCard accessible
names: add a visible label or aria-label to the percentage input at
frontend/src/features/job/JobInvoiceCard.tsx:344-359 identifying it as “Invoice
percentage of quote,” and to the amount input at
frontend/src/features/job/JobInvoiceCard.tsx:396-410 identifying it as “Invoice
amount.”
- Around line 233-239: Update the invoice link handler’s window.open call to
pass noopener,noreferrer in its third argument when opening invoice.online_url,
while preserving the existing URL check and toast behavior.
In `@frontend/src/lib/format.ts`:
- Around line 26-34: Update the NZ_DATE formatter used by formatDate to set
timeZone to Pacific/Auckland, ensuring invoice dates render consistently across
environments. Revise the formatter comment to state that per-page formatters are
rejected because invoice dates must display identical text across the workspace.
---
Nitpick comments:
In `@apps/xero/api.py`:
- Around line 317-328: Update the _document_error_status parameter annotation
from object to int | None, matching the status contract declared by
XeroDocumentResponse. Keep the existing 404 handling and 400 fallback unchanged,
and let mypy validate all call sites against the narrowed type.
- Around line 358-366: Extract the repeated get_valid_token() failure response
into a shared _xero_auth_required() helper returning the existing 401
Status[XeroAuthRequiredOut] payload, then replace the guards in all four Xero
document endpoints with that helper. Also extract and reuse a helper for the
identical purchase-order lookup and supplier guard, preserving the current
validation behavior and messages.
In `@apps/xero/tests/test_invoice_manager.py`:
- Around line 86-138: Add a success-case test in TestErrorContract for
delete_document using a configured provider and Xero invoice ID, then assert the
local Invoice is deleted, recalculate_job_invoicing_state is called, and the
invoice_deleted job event records the invoice number. Keep the test focused on
the same-request effects of the happy path.
In `@apps/xero/tests/test_po_manager.py`:
- Around line 98-109: Extend test_zero_uuid_is_treated_as_unsynced to cover the
write path: configure the provider’s successful DocumentResult with
external_id=ZERO_UUID, run manager.sync_to_xero(), then refresh or inspect
po.xero_id and assert it remains None. Preserve the existing create-path
assertion while exercising _save_po_with_xero_data’s zero-UUID handling.
In `@frontend/src/api/index.ts`:
- Line 81: Remove the code-narration comments at frontend/src/api/index.ts:81-81
and frontend/src/features/job/JobFinishTab.tsx:157-157; no replacement
documentation is needed because neither comment states a factual constraint or
rejected alternative.

🪄 Autofix

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: ea55edb0-3b70-4970-ae0f-3dafb7b206f3

📥 Commits

Reviewing files that changed from the base of the PR and between b5feeab and d1eea03.

⛔ Files ignored due to path filters (5)

• frontend/src/api/generated/@tanstack/react-query.gen.ts is excluded by !**/generated/**
• frontend/src/api/generated/index.ts is excluded by !**/generated/**
• frontend/src/api/generated/sdk.gen.ts is excluded by !**/generated/**
• frontend/src/api/generated/types.gen.ts is excluded by !**/generated/**
• frontend/src/api/generated/zod.gen.ts is excluded by !**/generated/**

📒 Files selected for processing (32)

• apps/accounting/provider.py
• apps/accounting/services/finish_job_summary.py
• apps/accounting/tests/test_finish_job_summary.py
• apps/accounting/types.py
• apps/company/tests/job_fixtures.py
• apps/job/api.py
• apps/job/schemas.py
• apps/job/services/job_service.py
• apps/job/tests/test_finish_api.py
• apps/xero/api.py
• apps/xero/documents/base.py
• apps/xero/documents/invoice.py
• apps/xero/documents/po.py
• apps/xero/helpers.py
• apps/xero/provider.py
• apps/xero/readonly_provider.py
• apps/xero/tests/test_invoice_manager.py
• apps/xero/tests/test_po_manager.py
• docs/accepted-api-differences.yml
• docs/code-quality.md
• docs/rewrite-status.md
• frontend/schema.v2.yml
• frontend/src/api/error-message.ts
• frontend/src/api/index.ts
• frontend/src/features/job/JobDetailPage.tsx
• frontend/src/features/job/JobFinishTab.test.tsx
• frontend/src/features/job/JobFinishTab.tsx
• frontend/src/features/job/JobInvoiceCard.tsx
• frontend/src/lib/format.ts
• frontend/tests/e2e/job/job-xero-invoice.spec.ts
• scripts/v1-frontend-operations.yml
• stubs/xero_python/accounting/__init__.pyi

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

docs/code-quality.md (1)

24-34: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Regenerate the suppression total.

Line 24 reports 431, but the visible rows sum to 430: fixed suppressions total 13 and rule-coded suppressions total 417. scripts/checks/code_quality.py derives the total from the counted entries, so this report is internally inconsistent.

Regenerate the file, then run uv run python -m scripts.checks.code_quality --check.

Expected table correction

-| TOTAL suppressions | 431 |+| TOTAL suppressions | 430 |

The canonical metric generator in scripts/checks/code_quality.py is the source of truth.

🤖 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/code-quality.md` around lines 24 - 34, Regenerate the suppression table
in docs/code-quality.md using scripts/checks/code_quality.py so TOTAL
suppressions matches the visible counted entries (430). Then run uv run python
-m scripts.checks.code_quality --check and ensure validation passes.

♻️ Duplicate comments (1)

apps/xero/documents/invoice.py (1)

246-251: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Validate billing_metadata before invoice creation.

Lines 249-251 parse required values after provider.create_invoice() and Invoice.objects.create(). If a required key is missing or non-decimal, the request fails after the financial operation has completed.

Parse and validate these fields at the start of create_document, before provider submission. Reuse the validated values when creating the audit event.

As per coding guidelines: “Fail early: check invalid cases first, validate required inputs up front.”

🤖 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/xero/documents/invoice.py` around lines 246 - 251, Update
create_document to parse and validate billing_metadata["target_total"],
billing_metadata["prior_invoiced_total"], and
billing_metadata["calculated_amount"] before calling provider.create_invoice()
or Invoice.objects.create(). Reuse these validated Decimal values when
constructing the audit event, removing the later direct parsing while preserving
the existing required-key and invalid-decimal failures.

Source: Coding guidelines

🧹 Nitpick comments (1)

apps/xero/tests/test_provider_documents.py (1)

216-236: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert the recovery pagination request.

_find_po_by_number iterates get_purchase_orders(tenant_id, page=page), but this test only asserts two calls. A regression that requests page one twice can still pass because the mocked side_effect returns a different response each time.

Assert the page argument for both get_purchase_orders calls.

🤖 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/xero/tests/test_provider_documents.py` around lines 216 - 236, Update
test_zero_uuid_recovers_real_id_across_pages to inspect
api.get_purchase_orders.call_args_list and assert the two calls use page=1 and
page=2 respectively, while preserving the existing tenant_id assertions and
recovery expectations.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@docs/code-quality.md`:
- Around line 24-34: Regenerate the suppression table in docs/code-quality.md
using scripts/checks/code_quality.py so TOTAL suppressions matches the visible
counted entries (430). Then run uv run python -m scripts.checks.code_quality
--check and ensure validation passes.
---
Duplicate comments:
In `@apps/xero/documents/invoice.py`:
- Around line 246-251: Update create_document to parse and validate
billing_metadata["target_total"], billing_metadata["prior_invoiced_total"], and
billing_metadata["calculated_amount"] before calling provider.create_invoice()
or Invoice.objects.create(). Reuse these validated Decimal values when
constructing the audit event, removing the later direct parsing while preserving
the existing required-key and invalid-decimal failures.
---
Nitpick comments:
In `@apps/xero/tests/test_provider_documents.py`:
- Around line 216-236: Update test_zero_uuid_recovers_real_id_across_pages to
inspect api.get_purchase_orders.call_args_list and assert the two calls use
page=1 and page=2 respectively, while preserving the existing tenant_id
assertions and recovery expectations.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 10f8aa42-f281-4b7d-8f3f-3e62a97f3bec

📥 Commits

Reviewing files that changed from the base of the PR and between d1eea03 and b318f2e.

📒 Files selected for processing (14)

• apps/job/tests/test_finish_api.py
• apps/xero/api.py
• apps/xero/constants.py
• apps/xero/documents/base.py
• apps/xero/documents/invoice.py
• apps/xero/documents/po.py
• apps/xero/provider.py
• apps/xero/tests/test_document_api.py
• apps/xero/tests/test_po_manager.py
• apps/xero/tests/test_provider_documents.py
• docs/code-quality.md
• docs/rewrite-status.md
• frontend/src/features/job/JobInvoiceCard.tsx
• frontend/src/lib/format.ts

🚧 Files skipped from review as they are similar to previous changes (9)

• frontend/src/lib/format.ts
• apps/job/tests/test_finish_api.py
• apps/xero/documents/base.py
• apps/xero/tests/test_po_manager.py
• docs/rewrite-status.md
• apps/xero/provider.py
• apps/xero/api.py
• frontend/src/features/job/JobInvoiceCard.tsx
• apps/xero/documents/po.py

[corrin]

Round-2 items, addressed in 2fb241c:

• billing_metadata validation ordering — fixed: the three Decimals now parse at the top of create_document, before anything reaches the provider, and the audit event reuses the validated values.
• page assertions in the zero-UUID recovery test — fixed: the test now asserts page=1, page=2 from call_args_list.
• docs/code-quality.md TOTAL 'mismatch' — rebutting: the file is generated and uv run python -m scripts.checks.code_quality --check passes against it, so it is exactly what the generator produces; the TOTAL row includes suppression categories beyond the per-code breakdown rows by design. Hand-editing a generated file is what the gate exists to prevent.

[corrin]

Verification upgraded per the corrected test regime: XERO_READONLY is only for dev sessions pointed at production (hotfixes) — testing runs with writes live. Re-ran the full suite with XERO_READONLY=false against the demo tenant: 38 passed, with job-xero-invoice exercising the real provider path end-to-end (actual create_invoices, workshop-PDF attachment, history note — 9.7s vs 5.3s under readonly, the difference being the real Xero round-trips). Local .env flipped to XERO_READONLY=false as the standing dev configuration; rewrite-status records the regime in 46a06d3.

[corrin]

Slice 2b — invoice path: document push + Finish Job (job-xero-invoice green, 13/40)

Second of the three slice-2 PRs (2a sync engine #46 · 2b this · 2c quote path next). Ports the Xero document push and the Finish Job workspace from v1.

What ports

Document manager base + invoice push (apps/xero/documents/{base,invoice}.py, apps/xero/helpers.py)

• XeroDocumentManager with the config guards (branding theme, quote terms), best-effort history notes, and fail-early construction; XeroInvoiceManager builds a provider-agnostic InvoicePayload, calls get_provider().create_invoice(), and owns local persistence: Invoice mirror row from the provider's canonical raw payload, billing_metadata audit trail, JobEvent, and a same-request recalculate_job_invoicing_state (ledgered — v1 left fully_invoiced to the hourly sync, which under XERO_READONLY never sees the invoice).
• Endpoints POST /api/xero/create_invoice/{job_id} and DELETE /api/xero/delete_invoice/{job_id} keep v1's URL fragments so the spec's request matcher ports unedited. Amount derivation goes through the already-ported calculate_invoice_amount (modes: full / costs-to-date / percent / amount).

Purchase-order push (apps/xero/documents/po.py) — USER-COMMITTED scope

• Create-vs-update keyed on xero_id with the zero-UUID sentinel treated as absent on read and write (v1 stored it), paged recovery of the real id by PO number, per-line xero_line_item_id backfill with duplicate-safe description matching, status mapping that raises on unknown statuses. No spec covers PO push; apps/xero/tests/test_po_manager.py (12 tests) is its gate.
• Ledgered: v1's successful PO delete always returned 500 (its success dict failed its own response serializer).

Readonly works by construction: XeroReadOnlyProvider fabricates well-formed results — INV-E2E-* numbers, GST-exclusive fake totals from the line items × CompanyDefaults.gst_rate — so the endpoint path is byte-identical under XERO_READONLY and the E2E balance-settles-to-$0 assertion exercises the real persistence code. Missing write overrides hit tripwires, not the tenant.

Finish Job backend (apps/accounting/services/finish_job_summary.py, apps/job/api.py)

• Server-owned balance (v1-verbatim arithmetic: remaining excl/incl GST, outstanding, over-invoiced reported rather than netted off), completion-checklist read/patch through Job.save()'s field-change audit machinery (unknown keys 422 via extra="forbid", schema field lists pinned to Job.COMPLETION_CHECKLIST_FIELDS at import), and the ETagged invoice list. The finish GET is deliberately un-ETagged: the balance moves when invoices sync in without touching job.updated_at.

Frontend (JobFinishTab.tsx, JobInvoiceCard.tsx, lazy-loaded into JobDetailPage)

• Balance, checklist, labour-hours cards, estimate/quote/actual comparison, quote accuracy; invoice list with create dialog (modes per pricing methodology) and delete. All money formatted from server values — no client recomputation. All v1 automation ids preserved. apiErrorMessage now also reads the document endpoints' error key so calc/configuration guidance reaches the user.

Spec: job-xero-invoice.spec.ts ports near-verbatim, with one deliberate deviation — it creates its own job instead of using sharedEditJobUrl, because it fully invoices the job and the shared fixture is read-only by contract (v1 mutated it and survived on run ordering).

Adversarial review (2 subagents) — notable catches, all fixed

• Provider-driven HTTP statuses (429/503 on live-Xero failures, 401 mid-request revocation) weren't in the declared response maps — ninja would raise ConfigError instead of returning the error payload. Statuses now clamp to the declared map; the cause stays in error (ADR 0038).
• The PO zero-UUID sentinel could be stored when the unpaginated recovery listing missed the PO (>100 POs) — guard added on write, recovery listing paged.
• TanStack v5 refetch() never rejects, so the "list could not be refreshed" toast was unreachable; an invoices load error rendered as the "No invoices" empty state.
• Known, deliberately not touched (v1-parity): calculate_invoice_amount doesn't quantize, so a sub-cent basis total (e.g. 157.985) can never read fully-invoiced. Real jobs have 2dp values; flagged for 2c or later rather than changing the money path mid-slice.

Verified

• ./scripts/ops/run_e2e.sh: 38 passed — job-xero-invoice green under XERO_READONLY (13/40 specs), all prior specs green with the sync beat live.
• Backend: 1617 unit tests green; new coverage: PO manager 12, invoice manager 7 (error contract / raw_json / readonly fabrication), finish summary 11, finish API 7. Frontend: JobFinishTab component tests 3.
• Expensive tier clean (mypy strict zero-baseline, import-linter, find-duplicates, deptry, schema-current, status table).
• Parity ledger: renamed: entries for all four document operation ids; two new behaviour entries in docs/accepted-api-differences.yml.

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features
  • Added a Finish Job workspace with financial summaries, invoice balances, cost comparisons, completion checklists, and variance indicators.
  • Added invoice listing and management, including full, percentage-based, cost-to-date, and custom-amount invoicing.
  • Added Xero invoice and purchase-order creation, updating, deletion, and invoice file attachments.
  • Added checklist updates with validation and activity history.
• Bug Fixes
  • Invoice changes now immediately recalculate the job’s fully invoiced status and remaining balance.
  • Improved error messages for accounting and Xero operations.

[coderabbitai]

Warning

Review limit reached

@corrin, you've reached your PR review limit, so we couldn't start this review.

Next review available in:24 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 052efda7-ef2c-4202-adef-941ebddc6980

📥 Commits

Reviewing files that changed from the base of the PR and between b318f2e and 09176e1.

📒 Files selected for processing (7)

• apps/xero/documents/invoice.py
• apps/xero/tests/test_invoice_manager.py
• apps/xero/tests/test_provider_documents.py
• config/tests/test_contract_gates.py
• docs/code-quality.md
• docs/rewrite-status.md
• scripts/checks/status_table.py

📝 Walkthrough

Walkthrough

This PR adds Finish Job financial and checklist APIs, Xero invoice and purchase-order synchronization, provider contracts, read-only behavior, and frontend Finish Job and invoice-management components with unit, API, and end-to-end tests.

Changes

Finish Job and Xero workflows

Layer / File(s)	Summary
Finish Job calculations and APIs apps/accounting/..., apps/job/..., frontend/schema.v2.yml	Adds financial summaries, checklist validation and updates, invoice listing, conditional responses, and Finish Job API contracts.
Accounting contracts and Xero provider operations apps/accounting/types.py, apps/accounting/provider.py, apps/xero/helpers.py, apps/xero/provider.py, apps/xero/readonly_provider.py, stubs/xero_python/...	Adds provider-agnostic document DTOs, Xero invoice and purchase-order operations, payload and error helpers, read-only results, and SDK stubs.
Xero document managers and endpoints apps/xero/documents/..., apps/xero/api.py, apps/xero/tests/...	Adds invoice and purchase-order validation, payload construction, persistence, deletion, authenticated endpoints, and manager tests.
Finish Job workspace and invoice UI frontend/src/features/job/..., frontend/src/api/..., frontend/src/lib/format.ts, frontend/tests/e2e/...	Adds lazy-loaded Finish Job and invoice components with financial displays, checklist updates, invoice actions, error handling, and end-to-end coverage.
Compatibility records and progress documentation docs/accepted-api-differences.yml, docs/code-quality.md, docs/rewrite-status.md, scripts/v1-frontend-operations.yml	Records API behavior changes, updated quality metrics, operation rename mappings, and rewrite progress.

Estimated code review effort: 5 (Critical) | ~120 minutes

Sequence Diagram(s)

sequenceDiagram
participant JobFinishTab
participant JobAPI
participant FinishJobSummary
participant JobService
JobFinishTab->>JobAPI: GET finish summary and checklist
JobAPI->>FinishJobSummary: calculate financial totals
FinishJobSummary-->>JobAPI: return summary
JobAPI-->>JobFinishTab: return JobFinishResponse
JobFinishTab->>JobAPI: PATCH checklist fields
JobAPI->>JobService: update checklist
JobService-->>JobAPI: return updated job

Loading

sequenceDiagram
participant JobInvoiceCard
participant XeroAPI
participant XeroInvoiceManager
participant XeroAccountingProvider
JobInvoiceCard->>XeroAPI: submit invoice request
XeroAPI->>XeroInvoiceManager: create document
XeroInvoiceManager->>XeroAccountingProvider: create InvoicePayload
XeroAccountingProvider-->>XeroInvoiceManager: return DocumentResult
XeroInvoiceManager-->>XeroAPI: return persisted invoice result
XeroAPI-->>JobInvoiceCard: return success or error

Loading

Possibly related PRs

• corrin/docketworks_v2#45: Extends the accounting provider and Xero abstractions used by this PR.
• corrin/docketworks_v2#18: Provides purchase-order models and status behavior used by Xero purchase-order synchronization.
• corrin/docketworks_v2#10: Provides the workshop PDF generation used by invoice attachment handling.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 44.10% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the main Xero invoice document push and Finish Job changes, with a useful E2E status indicator.
Description check	✅ Passed	The description is detailed, structured, and covers scope, behavior, verification results, known limitations, and parity changes.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch xero/invoice-path

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 14

🧹 Nitpick comments (5)

frontend/src/api/index.ts (1)

81-81: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Remove code-narration comments or document their constraint.

Both comments describe nearby code without stating the rejected alternative and the factual constraint.

• frontend/src/api/index.ts#L81-L81: Remove the export-group narration, or explain why feature code must import these generated operations through frontend/src/api/index.ts.
• frontend/src/features/job/JobFinishTab.tsx#L157-L157: Remove the ticket-heading comment. It does not document a runtime or business constraint.

As per coding guidelines, “Comments must document the rejected obvious alternative and the factual constraint that rejected it; remove code narration and review-feedback echoes.”

🤖 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 `@frontend/src/api/index.ts` at line 81, Remove the code-narration comments at
frontend/src/api/index.ts:81-81 and
frontend/src/features/job/JobFinishTab.tsx:157-157; no replacement documentation
is needed because neither comment states a factual constraint or rejected
alternative.

Source: Coding guidelines

apps/xero/tests/test_invoice_manager.py (1)

86-138: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a delete happy-path test.

TestErrorContract covers the three failure paths of delete_document. No test covers the success path, so nothing asserts that the local Invoice row is removed, that recalculate_job_invoicing_state runs, and that the invoice_deleted job event records the invoice number. That recalculation is the same same-request effect the create test protects at Line 256-258, and the Finish Job tab depends on it.

Do you want me to write this test?

🤖 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/xero/tests/test_invoice_manager.py` around lines 86 - 138, Add a
success-case test in TestErrorContract for delete_document using a configured
provider and Xero invoice ID, then assert the local Invoice is deleted,
recalculate_job_invoicing_state is called, and the invoice_deleted job event
records the invoice number. Keep the test focused on the same-request effects of
the happy path.

apps/xero/tests/test_po_manager.py (1)

98-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Cover the write direction of the zero-UUID sentinel too.

This test covers the read direction: a stored sentinel routes to create. The module docstring at Line 4-5 names "storing Xero's zero-UUID sentinel as a real id" as the expensive silent failure, and that is the write direction, guarded by _save_po_with_xero_data at apps/xero/documents/po.py Line 151.

Add a test where the provider returns DocumentResult(success=True, external_id=ZERO_UUID) and assert that po.xero_id stays None after sync_to_xero. This also pins the behavior discussed on apps/xero/provider.py Line 280-314.

Do you want me to write this test?

🤖 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/xero/tests/test_po_manager.py` around lines 98 - 109, Extend
test_zero_uuid_is_treated_as_unsynced to cover the write path: configure the
provider’s successful DocumentResult with external_id=ZERO_UUID, run
manager.sync_to_xero(), then refresh or inspect po.xero_id and assert it remains
None. Preserve the existing create-path assertion while exercising
_save_po_with_xero_data’s zero-UUID handling.

apps/xero/api.py (2)

317-328: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Type the clamp parameter as the contract, not as object.

XeroDocumentResponse declares status. Annotating the parameter object discards that contract and forces a runtime comparison against an untyped value. Declare int | None so mypy checks every call site.

♻️ Proposed signature change

-def _document_error_status(status: object) -> int:+def _document_error_status(status: int | None) -> int:

As per coding guidelines: "Treat type annotations as data contracts: use named types such as dataclasses, TypedDicts, or Protocols for complex inline types."

🤖 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/xero/api.py` around lines 317 - 328, Update the _document_error_status
parameter annotation from object to int | None, matching the status contract
declared by XeroDocumentResponse. Keep the existing 404 handling and 400
fallback unchanged, and let mypy validate all call sites against the narrowed
type.

Source: Coding guidelines

---

358-366: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract the repeated Xero-auth 401 block.

The same get_valid_token() guard and XeroAuthRequiredOut payload appear in all four new endpoints (lines 358-366, 458-466, 543-551, 612-620). The two purchase-order endpoints also repeat the identical lookup and supplier guard. One helper per repeated block keeps the message and status in step.

♻️ Proposed helper

def_xero_auth_required() ->Status[XeroAuthRequiredOut]:
"""Build the one 401 body every Xero document endpoint returns."""returnStatus(
401,
XeroAuthRequiredOut(
success=False,
redirect_to_auth=True,
message="Your Xero session has expired. Please log in again.",
),
)

As per coding guidelines: "Use one implementation per concept; search before implementing and extend a near-match instead of creating a parallel implementation."

🤖 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/xero/api.py` around lines 358 - 366, Extract the repeated
get_valid_token() failure response into a shared _xero_auth_required() helper
returning the existing 401 Status[XeroAuthRequiredOut] payload, then replace the
guards in all four Xero document endpoints with that helper. Also extract and
reuse a helper for the identical purchase-order lookup and supplier guard,
preserving the current validation behavior and messages.

Source: Coding guidelines

🤖 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 `@apps/job/tests/test_finish_api.py`:
- Around line 105-115: Correct the inline comment in test_lists_job_invoices to
state that the second invoice belongs to the same company but has no job, so it
must not appear in the job-scoped results. Do not change the test setup or
assertions.
In `@apps/xero/api.py`:
- Around line 64-72: Consolidate the duplicated `_staff` helpers by moving the
implementation from `apps/job/api.py` into `apps/core` and importing that shared
helper in `apps/xero/api.py`, `apps/purchasing/api.py`, and the applicable
callers. Remove redundant local definitions, while preserving any
Timesheet-specific narrowing behavior only if it cannot use the shared
implementation.
- Around line 402-405: Update the invoice creation flow around
XeroInvoiceManager.create_document to make retries safe at the Xero API
boundary: either supply a stable idempotency key for the same job/document
across retry attempts, or disable/prohibit RateLimitedRESTClient retries for
this non-idempotent POST. Preserve retries for safe operations and ensure one
logical create cannot produce duplicate Xero documents.
In `@apps/xero/documents/base.py`:
- Around line 82-99: Move the document-type validation before the best-effort
exception handler in both _add_xero_history_note (apps/xero/documents/base.py,
lines 82-99) and _add_history_note (apps/xero/provider.py, lines 407-421):
reject values outside ("invoice", "quote") before entering try, while leaving
provider calls inside the existing handler so only provider failures are
absorbed.
In `@apps/xero/documents/invoice.py`:
- Around line 232-241: Update create_document to validate that billing_metadata
contains target_total, prior_invoiced_total, and calculated_amount before any
direct reads. After validation, access all three keys directly and remove the
"0" defaults from the Decimal conversions so missing contract fields fail rather
than producing an uncomputed remaining_to_invoice value.
- Around line 198-216: Update the invoice creation flow after the existing
result.external_id and result.number validation to validate that raw contains
_sub_total, _total_tax, _total, and _amount_due before constructing Invoice.
Fail early using the existing validation pattern when any key is missing, then
read the validated values without zero fallbacks so invoice totals cannot be
silently stored as 0.00.
In `@apps/xero/documents/po.py`:
- Around line 23-25: Move ZERO_UUID to a shared module, remove the duplicate
definitions from documents/po.py and XeroAccountingProvider, and import the
shared constant in both consumers. Preserve the existing sentinel value and all
checks that use it.
- Around line 55-59: The Xero sync validation path must enforce the draft-only
rule for first-time creation. Update validate_for_xero_sync() to invoke
state_valid_for_xero() before allowing creation, while preserving updates for
POs with an existing Xero ID; keep the implementation and docstring of
state_valid_for_xero() consistent with this behavior.
In `@apps/xero/provider.py`:
- Around line 227-237: The invoice pre-read in the deletion flow must validate
that the API response contains an invoice and that its contact is present before
constructing the DELETED Invoice. Mirror the guards and user-facing error
messages used by delete_purchase_order, referencing the existing
invoice-deletion method and its existing/api/contact symbols while preserving
the normal construction path for valid data.
- Around line 280-314: Update the zero-UUID recovery flow around
_find_po_by_number so that when recovery returns None and po_id remains
ZERO_UUID, return a failed DocumentResult with no external_id instead of
continuing to the success path. Preserve the existing validation-error handling
and only construct the online_url and successful result after a real purchase
order ID is available.
- Around line 316-335: Update _find_po_by_number to enforce a finite maximum
page count while scanning purchase orders. Stop paging and return None once the
limit is reached, while preserving the existing match return and empty-page
termination behavior.
In `@frontend/src/features/job/JobInvoiceCard.tsx`:
- Around line 344-359: Give both numeric inputs in JobInvoiceCard accessible
names: add a visible label or aria-label to the percentage input at
frontend/src/features/job/JobInvoiceCard.tsx:344-359 identifying it as “Invoice
percentage of quote,” and to the amount input at
frontend/src/features/job/JobInvoiceCard.tsx:396-410 identifying it as “Invoice
amount.”
- Around line 233-239: Update the invoice link handler’s window.open call to
pass noopener,noreferrer in its third argument when opening invoice.online_url,
while preserving the existing URL check and toast behavior.
In `@frontend/src/lib/format.ts`:
- Around line 26-34: Update the NZ_DATE formatter used by formatDate to set
timeZone to Pacific/Auckland, ensuring invoice dates render consistently across
environments. Revise the formatter comment to state that per-page formatters are
rejected because invoice dates must display identical text across the workspace.
---
Nitpick comments:
In `@apps/xero/api.py`:
- Around line 317-328: Update the _document_error_status parameter annotation
from object to int | None, matching the status contract declared by
XeroDocumentResponse. Keep the existing 404 handling and 400 fallback unchanged,
and let mypy validate all call sites against the narrowed type.
- Around line 358-366: Extract the repeated get_valid_token() failure response
into a shared _xero_auth_required() helper returning the existing 401
Status[XeroAuthRequiredOut] payload, then replace the guards in all four Xero
document endpoints with that helper. Also extract and reuse a helper for the
identical purchase-order lookup and supplier guard, preserving the current
validation behavior and messages.
In `@apps/xero/tests/test_invoice_manager.py`:
- Around line 86-138: Add a success-case test in TestErrorContract for
delete_document using a configured provider and Xero invoice ID, then assert the
local Invoice is deleted, recalculate_job_invoicing_state is called, and the
invoice_deleted job event records the invoice number. Keep the test focused on
the same-request effects of the happy path.
In `@apps/xero/tests/test_po_manager.py`:
- Around line 98-109: Extend test_zero_uuid_is_treated_as_unsynced to cover the
write path: configure the provider’s successful DocumentResult with
external_id=ZERO_UUID, run manager.sync_to_xero(), then refresh or inspect
po.xero_id and assert it remains None. Preserve the existing create-path
assertion while exercising _save_po_with_xero_data’s zero-UUID handling.
In `@frontend/src/api/index.ts`:
- Line 81: Remove the code-narration comments at frontend/src/api/index.ts:81-81
and frontend/src/features/job/JobFinishTab.tsx:157-157; no replacement
documentation is needed because neither comment states a factual constraint or
rejected alternative.

🪄 Autofix

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: ea55edb0-3b70-4970-ae0f-3dafb7b206f3

📥 Commits

Reviewing files that changed from the base of the PR and between b5feeab and d1eea03.

⛔ Files ignored due to path filters (5)

• frontend/src/api/generated/@tanstack/react-query.gen.ts is excluded by !**/generated/**
• frontend/src/api/generated/index.ts is excluded by !**/generated/**
• frontend/src/api/generated/sdk.gen.ts is excluded by !**/generated/**
• frontend/src/api/generated/types.gen.ts is excluded by !**/generated/**
• frontend/src/api/generated/zod.gen.ts is excluded by !**/generated/**

📒 Files selected for processing (32)

• apps/accounting/provider.py
• apps/accounting/services/finish_job_summary.py
• apps/accounting/tests/test_finish_job_summary.py
• apps/accounting/types.py
• apps/company/tests/job_fixtures.py
• apps/job/api.py
• apps/job/schemas.py
• apps/job/services/job_service.py
• apps/job/tests/test_finish_api.py
• apps/xero/api.py
• apps/xero/documents/base.py
• apps/xero/documents/invoice.py
• apps/xero/documents/po.py
• apps/xero/helpers.py
• apps/xero/provider.py
• apps/xero/readonly_provider.py
• apps/xero/tests/test_invoice_manager.py
• apps/xero/tests/test_po_manager.py
• docs/accepted-api-differences.yml
• docs/code-quality.md
• docs/rewrite-status.md
• frontend/schema.v2.yml
• frontend/src/api/error-message.ts
• frontend/src/api/index.ts
• frontend/src/features/job/JobDetailPage.tsx
• frontend/src/features/job/JobFinishTab.test.tsx
• frontend/src/features/job/JobFinishTab.tsx
• frontend/src/features/job/JobInvoiceCard.tsx
• frontend/src/lib/format.ts
• frontend/tests/e2e/job/job-xero-invoice.spec.ts
• scripts/v1-frontend-operations.yml
• stubs/xero_python/accounting/__init__.pyi

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

docs/code-quality.md (1)

24-34: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Regenerate the suppression total.

Line 24 reports 431, but the visible rows sum to 430: fixed suppressions total 13 and rule-coded suppressions total 417. scripts/checks/code_quality.py derives the total from the counted entries, so this report is internally inconsistent.

Regenerate the file, then run uv run python -m scripts.checks.code_quality --check.

Expected table correction

-| TOTAL suppressions | 431 |+| TOTAL suppressions | 430 |

The canonical metric generator in scripts/checks/code_quality.py is the source of truth.

🤖 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/code-quality.md` around lines 24 - 34, Regenerate the suppression table
in docs/code-quality.md using scripts/checks/code_quality.py so TOTAL
suppressions matches the visible counted entries (430). Then run uv run python
-m scripts.checks.code_quality --check and ensure validation passes.

♻️ Duplicate comments (1)

apps/xero/documents/invoice.py (1)

246-251: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Validate billing_metadata before invoice creation.

Lines 249-251 parse required values after provider.create_invoice() and Invoice.objects.create(). If a required key is missing or non-decimal, the request fails after the financial operation has completed.

Parse and validate these fields at the start of create_document, before provider submission. Reuse the validated values when creating the audit event.

As per coding guidelines: “Fail early: check invalid cases first, validate required inputs up front.”

🤖 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/xero/documents/invoice.py` around lines 246 - 251, Update
create_document to parse and validate billing_metadata["target_total"],
billing_metadata["prior_invoiced_total"], and
billing_metadata["calculated_amount"] before calling provider.create_invoice()
or Invoice.objects.create(). Reuse these validated Decimal values when
constructing the audit event, removing the later direct parsing while preserving
the existing required-key and invalid-decimal failures.

Source: Coding guidelines

🧹 Nitpick comments (1)

apps/xero/tests/test_provider_documents.py (1)

216-236: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert the recovery pagination request.

_find_po_by_number iterates get_purchase_orders(tenant_id, page=page), but this test only asserts two calls. A regression that requests page one twice can still pass because the mocked side_effect returns a different response each time.

Assert the page argument for both get_purchase_orders calls.

🤖 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/xero/tests/test_provider_documents.py` around lines 216 - 236, Update
test_zero_uuid_recovers_real_id_across_pages to inspect
api.get_purchase_orders.call_args_list and assert the two calls use page=1 and
page=2 respectively, while preserving the existing tenant_id assertions and
recovery expectations.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@docs/code-quality.md`:
- Around line 24-34: Regenerate the suppression table in docs/code-quality.md
using scripts/checks/code_quality.py so TOTAL suppressions matches the visible
counted entries (430). Then run uv run python -m scripts.checks.code_quality
--check and ensure validation passes.
---
Duplicate comments:
In `@apps/xero/documents/invoice.py`:
- Around line 246-251: Update create_document to parse and validate
billing_metadata["target_total"], billing_metadata["prior_invoiced_total"], and
billing_metadata["calculated_amount"] before calling provider.create_invoice()
or Invoice.objects.create(). Reuse these validated Decimal values when
constructing the audit event, removing the later direct parsing while preserving
the existing required-key and invalid-decimal failures.
---
Nitpick comments:
In `@apps/xero/tests/test_provider_documents.py`:
- Around line 216-236: Update test_zero_uuid_recovers_real_id_across_pages to
inspect api.get_purchase_orders.call_args_list and assert the two calls use
page=1 and page=2 respectively, while preserving the existing tenant_id
assertions and recovery expectations.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 10f8aa42-f281-4b7d-8f3f-3e62a97f3bec

📥 Commits

Reviewing files that changed from the base of the PR and between d1eea03 and b318f2e.

📒 Files selected for processing (14)

• apps/job/tests/test_finish_api.py
• apps/xero/api.py
• apps/xero/constants.py
• apps/xero/documents/base.py
• apps/xero/documents/invoice.py
• apps/xero/documents/po.py
• apps/xero/provider.py
• apps/xero/tests/test_document_api.py
• apps/xero/tests/test_po_manager.py
• apps/xero/tests/test_provider_documents.py
• docs/code-quality.md
• docs/rewrite-status.md
• frontend/src/features/job/JobInvoiceCard.tsx
• frontend/src/lib/format.ts

🚧 Files skipped from review as they are similar to previous changes (9)

• frontend/src/lib/format.ts
• apps/job/tests/test_finish_api.py
• apps/xero/documents/base.py
• apps/xero/tests/test_po_manager.py
• docs/rewrite-status.md
• apps/xero/provider.py
• apps/xero/api.py
• frontend/src/features/job/JobInvoiceCard.tsx
• apps/xero/documents/po.py

[corrin]

Round-2 items, addressed in 2fb241c:

• billing_metadata validation ordering — fixed: the three Decimals now parse at the top of create_document, before anything reaches the provider, and the audit event reuses the validated values.
• page assertions in the zero-UUID recovery test — fixed: the test now asserts page=1, page=2 from call_args_list.
• docs/code-quality.md TOTAL 'mismatch' — rebutting: the file is generated and uv run python -m scripts.checks.code_quality --check passes against it, so it is exactly what the generator produces; the TOTAL row includes suppression categories beyond the per-code breakdown rows by design. Hand-editing a generated file is what the gate exists to prevent.

[corrin]

Verification upgraded per the corrected test regime: XERO_READONLY is only for dev sessions pointed at production (hotfixes) — testing runs with writes live. Re-ran the full suite with XERO_READONLY=false against the demo tenant: 38 passed, with job-xero-invoice exercising the real provider path end-to-end (actual create_invoices, workshop-PDF attachment, history note — 9.7s vs 5.3s under readonly, the difference being the real Xero round-trips). Local .env flipped to XERO_READONLY=false as the standing dev configuration; rewrite-status records the regime in 46a06d3.

[corrin]

Slice 2b — invoice path: document push + Finish Job (job-xero-invoice green, 13/40)

Second of the three slice-2 PRs (2a sync engine #46 · 2b this · 2c quote path next). Ports the Xero document push and the Finish Job workspace from v1.

What ports

Document manager base + invoice push (apps/xero/documents/{base,invoice}.py, apps/xero/helpers.py)

• XeroDocumentManager with the config guards (branding theme, quote terms), best-effort history notes, and fail-early construction; XeroInvoiceManager builds a provider-agnostic InvoicePayload, calls get_provider().create_invoice(), and owns local persistence: Invoice mirror row from the provider's canonical raw payload, billing_metadata audit trail, JobEvent, and a same-request recalculate_job_invoicing_state (ledgered — v1 left fully_invoiced to the hourly sync, which under XERO_READONLY never sees the invoice).
• Endpoints POST /api/xero/create_invoice/{job_id} and DELETE /api/xero/delete_invoice/{job_id} keep v1's URL fragments so the spec's request matcher ports unedited. Amount derivation goes through the already-ported calculate_invoice_amount (modes: full / costs-to-date / percent / amount).

Purchase-order push (apps/xero/documents/po.py) — USER-COMMITTED scope

• Create-vs-update keyed on xero_id with the zero-UUID sentinel treated as absent on read and write (v1 stored it), paged recovery of the real id by PO number, per-line xero_line_item_id backfill with duplicate-safe description matching, status mapping that raises on unknown statuses. No spec covers PO push; apps/xero/tests/test_po_manager.py (12 tests) is its gate.
• Ledgered: v1's successful PO delete always returned 500 (its success dict failed its own response serializer).

Readonly works by construction: XeroReadOnlyProvider fabricates well-formed results — INV-E2E-* numbers, GST-exclusive fake totals from the line items × CompanyDefaults.gst_rate — so the endpoint path is byte-identical under XERO_READONLY and the E2E balance-settles-to-$0 assertion exercises the real persistence code. Missing write overrides hit tripwires, not the tenant.

Finish Job backend (apps/accounting/services/finish_job_summary.py, apps/job/api.py)

• Server-owned balance (v1-verbatim arithmetic: remaining excl/incl GST, outstanding, over-invoiced reported rather than netted off), completion-checklist read/patch through Job.save()'s field-change audit machinery (unknown keys 422 via extra="forbid", schema field lists pinned to Job.COMPLETION_CHECKLIST_FIELDS at import), and the ETagged invoice list. The finish GET is deliberately un-ETagged: the balance moves when invoices sync in without touching job.updated_at.

Frontend (JobFinishTab.tsx, JobInvoiceCard.tsx, lazy-loaded into JobDetailPage)

• Balance, checklist, labour-hours cards, estimate/quote/actual comparison, quote accuracy; invoice list with create dialog (modes per pricing methodology) and delete. All money formatted from server values — no client recomputation. All v1 automation ids preserved. apiErrorMessage now also reads the document endpoints' error key so calc/configuration guidance reaches the user.

Spec: job-xero-invoice.spec.ts ports near-verbatim, with one deliberate deviation — it creates its own job instead of using sharedEditJobUrl, because it fully invoices the job and the shared fixture is read-only by contract (v1 mutated it and survived on run ordering).

Adversarial review (2 subagents) — notable catches, all fixed

• Provider-driven HTTP statuses (429/503 on live-Xero failures, 401 mid-request revocation) weren't in the declared response maps — ninja would raise ConfigError instead of returning the error payload. Statuses now clamp to the declared map; the cause stays in error (ADR 0038).
• The PO zero-UUID sentinel could be stored when the unpaginated recovery listing missed the PO (>100 POs) — guard added on write, recovery listing paged.
• TanStack v5 refetch() never rejects, so the "list could not be refreshed" toast was unreachable; an invoices load error rendered as the "No invoices" empty state.
• Known, deliberately not touched (v1-parity): calculate_invoice_amount doesn't quantize, so a sub-cent basis total (e.g. 157.985) can never read fully-invoiced. Real jobs have 2dp values; flagged for 2c or later rather than changing the money path mid-slice.

Verified

• ./scripts/ops/run_e2e.sh: 38 passed — job-xero-invoice green under XERO_READONLY (13/40 specs), all prior specs green with the sync beat live.
• Backend: 1617 unit tests green; new coverage: PO manager 12, invoice manager 7 (error contract / raw_json / readonly fabrication), finish summary 11, finish API 7. Frontend: JobFinishTab component tests 3.
• Expensive tier clean (mypy strict zero-baseline, import-linter, find-duplicates, deptry, schema-current, status table).
• Parity ledger: renamed: entries for all four document operation ids; two new behaviour entries in docs/accepted-api-differences.yml.

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features
  • Added a Finish Job workspace with financial summaries, invoice balances, cost comparisons, completion checklists, and variance indicators.
  • Added invoice listing and management, including full, percentage-based, cost-to-date, and custom-amount invoicing.
  • Added Xero invoice and purchase-order creation, updating, deletion, and invoice file attachments.
  • Added checklist updates with validation and activity history.
• Bug Fixes
  • Invoice changes now immediately recalculate the job’s fully invoiced status and remaining balance.
  • Improved error messages for accounting and Xero operations.

[coderabbitai]

Warning

Review limit reached

@corrin, you've reached your PR review limit, so we couldn't start this review.

Next review available in:24 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 052efda7-ef2c-4202-adef-941ebddc6980

📥 Commits

Reviewing files that changed from the base of the PR and between b318f2e and 09176e1.

📒 Files selected for processing (7)

• apps/xero/documents/invoice.py
• apps/xero/tests/test_invoice_manager.py
• apps/xero/tests/test_provider_documents.py
• config/tests/test_contract_gates.py
• docs/code-quality.md
• docs/rewrite-status.md
• scripts/checks/status_table.py

📝 Walkthrough

Walkthrough

This PR adds Finish Job financial and checklist APIs, Xero invoice and purchase-order synchronization, provider contracts, read-only behavior, and frontend Finish Job and invoice-management components with unit, API, and end-to-end tests.

Changes

Finish Job and Xero workflows

Layer / File(s)	Summary
Finish Job calculations and APIs apps/accounting/..., apps/job/..., frontend/schema.v2.yml	Adds financial summaries, checklist validation and updates, invoice listing, conditional responses, and Finish Job API contracts.
Accounting contracts and Xero provider operations apps/accounting/types.py, apps/accounting/provider.py, apps/xero/helpers.py, apps/xero/provider.py, apps/xero/readonly_provider.py, stubs/xero_python/...	Adds provider-agnostic document DTOs, Xero invoice and purchase-order operations, payload and error helpers, read-only results, and SDK stubs.
Xero document managers and endpoints apps/xero/documents/..., apps/xero/api.py, apps/xero/tests/...	Adds invoice and purchase-order validation, payload construction, persistence, deletion, authenticated endpoints, and manager tests.
Finish Job workspace and invoice UI frontend/src/features/job/..., frontend/src/api/..., frontend/src/lib/format.ts, frontend/tests/e2e/...	Adds lazy-loaded Finish Job and invoice components with financial displays, checklist updates, invoice actions, error handling, and end-to-end coverage.
Compatibility records and progress documentation docs/accepted-api-differences.yml, docs/code-quality.md, docs/rewrite-status.md, scripts/v1-frontend-operations.yml	Records API behavior changes, updated quality metrics, operation rename mappings, and rewrite progress.

Estimated code review effort: 5 (Critical) | ~120 minutes

Sequence Diagram(s)

sequenceDiagram
participant JobFinishTab
participant JobAPI
participant FinishJobSummary
participant JobService
JobFinishTab->>JobAPI: GET finish summary and checklist
JobAPI->>FinishJobSummary: calculate financial totals
FinishJobSummary-->>JobAPI: return summary
JobAPI-->>JobFinishTab: return JobFinishResponse
JobFinishTab->>JobAPI: PATCH checklist fields
JobAPI->>JobService: update checklist
JobService-->>JobAPI: return updated job

Loading

sequenceDiagram
participant JobInvoiceCard
participant XeroAPI
participant XeroInvoiceManager
participant XeroAccountingProvider
JobInvoiceCard->>XeroAPI: submit invoice request
XeroAPI->>XeroInvoiceManager: create document
XeroInvoiceManager->>XeroAccountingProvider: create InvoicePayload
XeroAccountingProvider-->>XeroInvoiceManager: return DocumentResult
XeroInvoiceManager-->>XeroAPI: return persisted invoice result
XeroAPI-->>JobInvoiceCard: return success or error

Loading

Possibly related PRs

• corrin/docketworks_v2#45: Extends the accounting provider and Xero abstractions used by this PR.
• corrin/docketworks_v2#18: Provides purchase-order models and status behavior used by Xero purchase-order synchronization.
• corrin/docketworks_v2#10: Provides the workshop PDF generation used by invoice attachment handling.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 44.10% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the main Xero invoice document push and Finish Job changes, with a useful E2E status indicator.
Description check	✅ Passed	The description is detailed, structured, and covers scope, behavior, verification results, known limitations, and parity changes.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch xero/invoice-path

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 14

🧹 Nitpick comments (5)

frontend/src/api/index.ts (1)

81-81: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Remove code-narration comments or document their constraint.

Both comments describe nearby code without stating the rejected alternative and the factual constraint.

• frontend/src/api/index.ts#L81-L81: Remove the export-group narration, or explain why feature code must import these generated operations through frontend/src/api/index.ts.
• frontend/src/features/job/JobFinishTab.tsx#L157-L157: Remove the ticket-heading comment. It does not document a runtime or business constraint.

As per coding guidelines, “Comments must document the rejected obvious alternative and the factual constraint that rejected it; remove code narration and review-feedback echoes.”

🤖 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 `@frontend/src/api/index.ts` at line 81, Remove the code-narration comments at
frontend/src/api/index.ts:81-81 and
frontend/src/features/job/JobFinishTab.tsx:157-157; no replacement documentation
is needed because neither comment states a factual constraint or rejected
alternative.

Source: Coding guidelines

apps/xero/tests/test_invoice_manager.py (1)

86-138: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a delete happy-path test.

TestErrorContract covers the three failure paths of delete_document. No test covers the success path, so nothing asserts that the local Invoice row is removed, that recalculate_job_invoicing_state runs, and that the invoice_deleted job event records the invoice number. That recalculation is the same same-request effect the create test protects at Line 256-258, and the Finish Job tab depends on it.

Do you want me to write this test?

🤖 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/xero/tests/test_invoice_manager.py` around lines 86 - 138, Add a
success-case test in TestErrorContract for delete_document using a configured
provider and Xero invoice ID, then assert the local Invoice is deleted,
recalculate_job_invoicing_state is called, and the invoice_deleted job event
records the invoice number. Keep the test focused on the same-request effects of
the happy path.

apps/xero/tests/test_po_manager.py (1)

98-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Cover the write direction of the zero-UUID sentinel too.

This test covers the read direction: a stored sentinel routes to create. The module docstring at Line 4-5 names "storing Xero's zero-UUID sentinel as a real id" as the expensive silent failure, and that is the write direction, guarded by _save_po_with_xero_data at apps/xero/documents/po.py Line 151.

Add a test where the provider returns DocumentResult(success=True, external_id=ZERO_UUID) and assert that po.xero_id stays None after sync_to_xero. This also pins the behavior discussed on apps/xero/provider.py Line 280-314.

Do you want me to write this test?

🤖 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/xero/tests/test_po_manager.py` around lines 98 - 109, Extend
test_zero_uuid_is_treated_as_unsynced to cover the write path: configure the
provider’s successful DocumentResult with external_id=ZERO_UUID, run
manager.sync_to_xero(), then refresh or inspect po.xero_id and assert it remains
None. Preserve the existing create-path assertion while exercising
_save_po_with_xero_data’s zero-UUID handling.

apps/xero/api.py (2)

317-328: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Type the clamp parameter as the contract, not as object.

XeroDocumentResponse declares status. Annotating the parameter object discards that contract and forces a runtime comparison against an untyped value. Declare int | None so mypy checks every call site.

♻️ Proposed signature change

-def _document_error_status(status: object) -> int:+def _document_error_status(status: int | None) -> int:

As per coding guidelines: "Treat type annotations as data contracts: use named types such as dataclasses, TypedDicts, or Protocols for complex inline types."

🤖 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/xero/api.py` around lines 317 - 328, Update the _document_error_status
parameter annotation from object to int | None, matching the status contract
declared by XeroDocumentResponse. Keep the existing 404 handling and 400
fallback unchanged, and let mypy validate all call sites against the narrowed
type.

Source: Coding guidelines

---

358-366: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract the repeated Xero-auth 401 block.

The same get_valid_token() guard and XeroAuthRequiredOut payload appear in all four new endpoints (lines 358-366, 458-466, 543-551, 612-620). The two purchase-order endpoints also repeat the identical lookup and supplier guard. One helper per repeated block keeps the message and status in step.

♻️ Proposed helper

def_xero_auth_required() ->Status[XeroAuthRequiredOut]:
"""Build the one 401 body every Xero document endpoint returns."""returnStatus(
401,
XeroAuthRequiredOut(
success=False,
redirect_to_auth=True,
message="Your Xero session has expired. Please log in again.",
),
)

As per coding guidelines: "Use one implementation per concept; search before implementing and extend a near-match instead of creating a parallel implementation."

🤖 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/xero/api.py` around lines 358 - 366, Extract the repeated
get_valid_token() failure response into a shared _xero_auth_required() helper
returning the existing 401 Status[XeroAuthRequiredOut] payload, then replace the
guards in all four Xero document endpoints with that helper. Also extract and
reuse a helper for the identical purchase-order lookup and supplier guard,
preserving the current validation behavior and messages.

Source: Coding guidelines

🤖 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 `@apps/job/tests/test_finish_api.py`:
- Around line 105-115: Correct the inline comment in test_lists_job_invoices to
state that the second invoice belongs to the same company but has no job, so it
must not appear in the job-scoped results. Do not change the test setup or
assertions.
In `@apps/xero/api.py`:
- Around line 64-72: Consolidate the duplicated `_staff` helpers by moving the
implementation from `apps/job/api.py` into `apps/core` and importing that shared
helper in `apps/xero/api.py`, `apps/purchasing/api.py`, and the applicable
callers. Remove redundant local definitions, while preserving any
Timesheet-specific narrowing behavior only if it cannot use the shared
implementation.
- Around line 402-405: Update the invoice creation flow around
XeroInvoiceManager.create_document to make retries safe at the Xero API
boundary: either supply a stable idempotency key for the same job/document
across retry attempts, or disable/prohibit RateLimitedRESTClient retries for
this non-idempotent POST. Preserve retries for safe operations and ensure one
logical create cannot produce duplicate Xero documents.
In `@apps/xero/documents/base.py`:
- Around line 82-99: Move the document-type validation before the best-effort
exception handler in both _add_xero_history_note (apps/xero/documents/base.py,
lines 82-99) and _add_history_note (apps/xero/provider.py, lines 407-421):
reject values outside ("invoice", "quote") before entering try, while leaving
provider calls inside the existing handler so only provider failures are
absorbed.
In `@apps/xero/documents/invoice.py`:
- Around line 232-241: Update create_document to validate that billing_metadata
contains target_total, prior_invoiced_total, and calculated_amount before any
direct reads. After validation, access all three keys directly and remove the
"0" defaults from the Decimal conversions so missing contract fields fail rather
than producing an uncomputed remaining_to_invoice value.
- Around line 198-216: Update the invoice creation flow after the existing
result.external_id and result.number validation to validate that raw contains
_sub_total, _total_tax, _total, and _amount_due before constructing Invoice.
Fail early using the existing validation pattern when any key is missing, then
read the validated values without zero fallbacks so invoice totals cannot be
silently stored as 0.00.
In `@apps/xero/documents/po.py`:
- Around line 23-25: Move ZERO_UUID to a shared module, remove the duplicate
definitions from documents/po.py and XeroAccountingProvider, and import the
shared constant in both consumers. Preserve the existing sentinel value and all
checks that use it.
- Around line 55-59: The Xero sync validation path must enforce the draft-only
rule for first-time creation. Update validate_for_xero_sync() to invoke
state_valid_for_xero() before allowing creation, while preserving updates for
POs with an existing Xero ID; keep the implementation and docstring of
state_valid_for_xero() consistent with this behavior.
In `@apps/xero/provider.py`:
- Around line 227-237: The invoice pre-read in the deletion flow must validate
that the API response contains an invoice and that its contact is present before
constructing the DELETED Invoice. Mirror the guards and user-facing error
messages used by delete_purchase_order, referencing the existing
invoice-deletion method and its existing/api/contact symbols while preserving
the normal construction path for valid data.
- Around line 280-314: Update the zero-UUID recovery flow around
_find_po_by_number so that when recovery returns None and po_id remains
ZERO_UUID, return a failed DocumentResult with no external_id instead of
continuing to the success path. Preserve the existing validation-error handling
and only construct the online_url and successful result after a real purchase
order ID is available.
- Around line 316-335: Update _find_po_by_number to enforce a finite maximum
page count while scanning purchase orders. Stop paging and return None once the
limit is reached, while preserving the existing match return and empty-page
termination behavior.
In `@frontend/src/features/job/JobInvoiceCard.tsx`:
- Around line 344-359: Give both numeric inputs in JobInvoiceCard accessible
names: add a visible label or aria-label to the percentage input at
frontend/src/features/job/JobInvoiceCard.tsx:344-359 identifying it as “Invoice
percentage of quote,” and to the amount input at
frontend/src/features/job/JobInvoiceCard.tsx:396-410 identifying it as “Invoice
amount.”
- Around line 233-239: Update the invoice link handler’s window.open call to
pass noopener,noreferrer in its third argument when opening invoice.online_url,
while preserving the existing URL check and toast behavior.
In `@frontend/src/lib/format.ts`:
- Around line 26-34: Update the NZ_DATE formatter used by formatDate to set
timeZone to Pacific/Auckland, ensuring invoice dates render consistently across
environments. Revise the formatter comment to state that per-page formatters are
rejected because invoice dates must display identical text across the workspace.
---
Nitpick comments:
In `@apps/xero/api.py`:
- Around line 317-328: Update the _document_error_status parameter annotation
from object to int | None, matching the status contract declared by
XeroDocumentResponse. Keep the existing 404 handling and 400 fallback unchanged,
and let mypy validate all call sites against the narrowed type.
- Around line 358-366: Extract the repeated get_valid_token() failure response
into a shared _xero_auth_required() helper returning the existing 401
Status[XeroAuthRequiredOut] payload, then replace the guards in all four Xero
document endpoints with that helper. Also extract and reuse a helper for the
identical purchase-order lookup and supplier guard, preserving the current
validation behavior and messages.
In `@apps/xero/tests/test_invoice_manager.py`:
- Around line 86-138: Add a success-case test in TestErrorContract for
delete_document using a configured provider and Xero invoice ID, then assert the
local Invoice is deleted, recalculate_job_invoicing_state is called, and the
invoice_deleted job event records the invoice number. Keep the test focused on
the same-request effects of the happy path.
In `@apps/xero/tests/test_po_manager.py`:
- Around line 98-109: Extend test_zero_uuid_is_treated_as_unsynced to cover the
write path: configure the provider’s successful DocumentResult with
external_id=ZERO_UUID, run manager.sync_to_xero(), then refresh or inspect
po.xero_id and assert it remains None. Preserve the existing create-path
assertion while exercising _save_po_with_xero_data’s zero-UUID handling.
In `@frontend/src/api/index.ts`:
- Line 81: Remove the code-narration comments at frontend/src/api/index.ts:81-81
and frontend/src/features/job/JobFinishTab.tsx:157-157; no replacement
documentation is needed because neither comment states a factual constraint or
rejected alternative.

🪄 Autofix

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: ea55edb0-3b70-4970-ae0f-3dafb7b206f3

📥 Commits

Reviewing files that changed from the base of the PR and between b5feeab and d1eea03.

⛔ Files ignored due to path filters (5)

• frontend/src/api/generated/@tanstack/react-query.gen.ts is excluded by !**/generated/**
• frontend/src/api/generated/index.ts is excluded by !**/generated/**
• frontend/src/api/generated/sdk.gen.ts is excluded by !**/generated/**
• frontend/src/api/generated/types.gen.ts is excluded by !**/generated/**
• frontend/src/api/generated/zod.gen.ts is excluded by !**/generated/**

📒 Files selected for processing (32)

• apps/accounting/provider.py
• apps/accounting/services/finish_job_summary.py
• apps/accounting/tests/test_finish_job_summary.py
• apps/accounting/types.py
• apps/company/tests/job_fixtures.py
• apps/job/api.py
• apps/job/schemas.py
• apps/job/services/job_service.py
• apps/job/tests/test_finish_api.py
• apps/xero/api.py
• apps/xero/documents/base.py
• apps/xero/documents/invoice.py
• apps/xero/documents/po.py
• apps/xero/helpers.py
• apps/xero/provider.py
• apps/xero/readonly_provider.py
• apps/xero/tests/test_invoice_manager.py
• apps/xero/tests/test_po_manager.py
• docs/accepted-api-differences.yml
• docs/code-quality.md
• docs/rewrite-status.md
• frontend/schema.v2.yml
• frontend/src/api/error-message.ts
• frontend/src/api/index.ts
• frontend/src/features/job/JobDetailPage.tsx
• frontend/src/features/job/JobFinishTab.test.tsx
• frontend/src/features/job/JobFinishTab.tsx
• frontend/src/features/job/JobInvoiceCard.tsx
• frontend/src/lib/format.ts
• frontend/tests/e2e/job/job-xero-invoice.spec.ts
• scripts/v1-frontend-operations.yml
• stubs/xero_python/accounting/__init__.pyi

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

docs/code-quality.md (1)

24-34: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Regenerate the suppression total.

Line 24 reports 431, but the visible rows sum to 430: fixed suppressions total 13 and rule-coded suppressions total 417. scripts/checks/code_quality.py derives the total from the counted entries, so this report is internally inconsistent.

Regenerate the file, then run uv run python -m scripts.checks.code_quality --check.

Expected table correction

-| TOTAL suppressions | 431 |+| TOTAL suppressions | 430 |

The canonical metric generator in scripts/checks/code_quality.py is the source of truth.

🤖 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/code-quality.md` around lines 24 - 34, Regenerate the suppression table
in docs/code-quality.md using scripts/checks/code_quality.py so TOTAL
suppressions matches the visible counted entries (430). Then run uv run python
-m scripts.checks.code_quality --check and ensure validation passes.

♻️ Duplicate comments (1)

apps/xero/documents/invoice.py (1)

246-251: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Validate billing_metadata before invoice creation.

Lines 249-251 parse required values after provider.create_invoice() and Invoice.objects.create(). If a required key is missing or non-decimal, the request fails after the financial operation has completed.

Parse and validate these fields at the start of create_document, before provider submission. Reuse the validated values when creating the audit event.

As per coding guidelines: “Fail early: check invalid cases first, validate required inputs up front.”

🤖 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/xero/documents/invoice.py` around lines 246 - 251, Update
create_document to parse and validate billing_metadata["target_total"],
billing_metadata["prior_invoiced_total"], and
billing_metadata["calculated_amount"] before calling provider.create_invoice()
or Invoice.objects.create(). Reuse these validated Decimal values when
constructing the audit event, removing the later direct parsing while preserving
the existing required-key and invalid-decimal failures.

Source: Coding guidelines

🧹 Nitpick comments (1)

apps/xero/tests/test_provider_documents.py (1)

216-236: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert the recovery pagination request.

_find_po_by_number iterates get_purchase_orders(tenant_id, page=page), but this test only asserts two calls. A regression that requests page one twice can still pass because the mocked side_effect returns a different response each time.

Assert the page argument for both get_purchase_orders calls.

🤖 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/xero/tests/test_provider_documents.py` around lines 216 - 236, Update
test_zero_uuid_recovers_real_id_across_pages to inspect
api.get_purchase_orders.call_args_list and assert the two calls use page=1 and
page=2 respectively, while preserving the existing tenant_id assertions and
recovery expectations.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@docs/code-quality.md`:
- Around line 24-34: Regenerate the suppression table in docs/code-quality.md
using scripts/checks/code_quality.py so TOTAL suppressions matches the visible
counted entries (430). Then run uv run python -m scripts.checks.code_quality
--check and ensure validation passes.
---
Duplicate comments:
In `@apps/xero/documents/invoice.py`:
- Around line 246-251: Update create_document to parse and validate
billing_metadata["target_total"], billing_metadata["prior_invoiced_total"], and
billing_metadata["calculated_amount"] before calling provider.create_invoice()
or Invoice.objects.create(). Reuse these validated Decimal values when
constructing the audit event, removing the later direct parsing while preserving
the existing required-key and invalid-decimal failures.
---
Nitpick comments:
In `@apps/xero/tests/test_provider_documents.py`:
- Around line 216-236: Update test_zero_uuid_recovers_real_id_across_pages to
inspect api.get_purchase_orders.call_args_list and assert the two calls use
page=1 and page=2 respectively, while preserving the existing tenant_id
assertions and recovery expectations.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 10f8aa42-f281-4b7d-8f3f-3e62a97f3bec

📥 Commits

Reviewing files that changed from the base of the PR and between d1eea03 and b318f2e.

📒 Files selected for processing (14)

• apps/job/tests/test_finish_api.py
• apps/xero/api.py
• apps/xero/constants.py
• apps/xero/documents/base.py
• apps/xero/documents/invoice.py
• apps/xero/documents/po.py
• apps/xero/provider.py
• apps/xero/tests/test_document_api.py
• apps/xero/tests/test_po_manager.py
• apps/xero/tests/test_provider_documents.py
• docs/code-quality.md
• docs/rewrite-status.md
• frontend/src/features/job/JobInvoiceCard.tsx
• frontend/src/lib/format.ts

🚧 Files skipped from review as they are similar to previous changes (9)

• frontend/src/lib/format.ts
• apps/job/tests/test_finish_api.py
• apps/xero/documents/base.py
• apps/xero/tests/test_po_manager.py
• docs/rewrite-status.md
• apps/xero/provider.py
• apps/xero/api.py
• frontend/src/features/job/JobInvoiceCard.tsx
• apps/xero/documents/po.py

[corrin]

Round-2 items, addressed in 2fb241c:

• billing_metadata validation ordering — fixed: the three Decimals now parse at the top of create_document, before anything reaches the provider, and the audit event reuses the validated values.
• page assertions in the zero-UUID recovery test — fixed: the test now asserts page=1, page=2 from call_args_list.
• docs/code-quality.md TOTAL 'mismatch' — rebutting: the file is generated and uv run python -m scripts.checks.code_quality --check passes against it, so it is exactly what the generator produces; the TOTAL row includes suppression categories beyond the per-code breakdown rows by design. Hand-editing a generated file is what the gate exists to prevent.

[corrin]

Verification upgraded per the corrected test regime: XERO_READONLY is only for dev sessions pointed at production (hotfixes) — testing runs with writes live. Re-ran the full suite with XERO_READONLY=false against the demo tenant: 38 passed, with job-xero-invoice exercising the real provider path end-to-end (actual create_invoices, workshop-PDF attachment, history note — 9.7s vs 5.3s under readonly, the difference being the real Xero round-trips). Local .env flipped to XERO_READONLY=false as the standing dev configuration; rewrite-status records the regime in 46a06d3.

[corrin]

Slice 2b — invoice path: document push + Finish Job (job-xero-invoice green, 13/40)

Second of the three slice-2 PRs (2a sync engine #46 · 2b this · 2c quote path next). Ports the Xero document push and the Finish Job workspace from v1.

What ports

Document manager base + invoice push (apps/xero/documents/{base,invoice}.py, apps/xero/helpers.py)

• XeroDocumentManager with the config guards (branding theme, quote terms), best-effort history notes, and fail-early construction; XeroInvoiceManager builds a provider-agnostic InvoicePayload, calls get_provider().create_invoice(), and owns local persistence: Invoice mirror row from the provider's canonical raw payload, billing_metadata audit trail, JobEvent, and a same-request recalculate_job_invoicing_state (ledgered — v1 left fully_invoiced to the hourly sync, which under XERO_READONLY never sees the invoice).
• Endpoints POST /api/xero/create_invoice/{job_id} and DELETE /api/xero/delete_invoice/{job_id} keep v1's URL fragments so the spec's request matcher ports unedited. Amount derivation goes through the already-ported calculate_invoice_amount (modes: full / costs-to-date / percent / amount).

Purchase-order push (apps/xero/documents/po.py) — USER-COMMITTED scope

• Create-vs-update keyed on xero_id with the zero-UUID sentinel treated as absent on read and write (v1 stored it), paged recovery of the real id by PO number, per-line xero_line_item_id backfill with duplicate-safe description matching, status mapping that raises on unknown statuses. No spec covers PO push; apps/xero/tests/test_po_manager.py (12 tests) is its gate.
• Ledgered: v1's successful PO delete always returned 500 (its success dict failed its own response serializer).

Readonly works by construction: XeroReadOnlyProvider fabricates well-formed results — INV-E2E-* numbers, GST-exclusive fake totals from the line items × CompanyDefaults.gst_rate — so the endpoint path is byte-identical under XERO_READONLY and the E2E balance-settles-to-$0 assertion exercises the real persistence code. Missing write overrides hit tripwires, not the tenant.

Finish Job backend (apps/accounting/services/finish_job_summary.py, apps/job/api.py)

• Server-owned balance (v1-verbatim arithmetic: remaining excl/incl GST, outstanding, over-invoiced reported rather than netted off), completion-checklist read/patch through Job.save()'s field-change audit machinery (unknown keys 422 via extra="forbid", schema field lists pinned to Job.COMPLETION_CHECKLIST_FIELDS at import), and the ETagged invoice list. The finish GET is deliberately un-ETagged: the balance moves when invoices sync in without touching job.updated_at.

Frontend (JobFinishTab.tsx, JobInvoiceCard.tsx, lazy-loaded into JobDetailPage)

• Balance, checklist, labour-hours cards, estimate/quote/actual comparison, quote accuracy; invoice list with create dialog (modes per pricing methodology) and delete. All money formatted from server values — no client recomputation. All v1 automation ids preserved. apiErrorMessage now also reads the document endpoints' error key so calc/configuration guidance reaches the user.

Spec: job-xero-invoice.spec.ts ports near-verbatim, with one deliberate deviation — it creates its own job instead of using sharedEditJobUrl, because it fully invoices the job and the shared fixture is read-only by contract (v1 mutated it and survived on run ordering).

Adversarial review (2 subagents) — notable catches, all fixed

• Provider-driven HTTP statuses (429/503 on live-Xero failures, 401 mid-request revocation) weren't in the declared response maps — ninja would raise ConfigError instead of returning the error payload. Statuses now clamp to the declared map; the cause stays in error (ADR 0038).
• The PO zero-UUID sentinel could be stored when the unpaginated recovery listing missed the PO (>100 POs) — guard added on write, recovery listing paged.
• TanStack v5 refetch() never rejects, so the "list could not be refreshed" toast was unreachable; an invoices load error rendered as the "No invoices" empty state.
• Known, deliberately not touched (v1-parity): calculate_invoice_amount doesn't quantize, so a sub-cent basis total (e.g. 157.985) can never read fully-invoiced. Real jobs have 2dp values; flagged for 2c or later rather than changing the money path mid-slice.

Verified

• ./scripts/ops/run_e2e.sh: 38 passed — job-xero-invoice green under XERO_READONLY (13/40 specs), all prior specs green with the sync beat live.
• Backend: 1617 unit tests green; new coverage: PO manager 12, invoice manager 7 (error contract / raw_json / readonly fabrication), finish summary 11, finish API 7. Frontend: JobFinishTab component tests 3.
• Expensive tier clean (mypy strict zero-baseline, import-linter, find-duplicates, deptry, schema-current, status table).
• Parity ledger: renamed: entries for all four document operation ids; two new behaviour entries in docs/accepted-api-differences.yml.

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features
  • Added a Finish Job workspace with financial summaries, invoice balances, cost comparisons, completion checklists, and variance indicators.
  • Added invoice listing and management, including full, percentage-based, cost-to-date, and custom-amount invoicing.
  • Added Xero invoice and purchase-order creation, updating, deletion, and invoice file attachments.
  • Added checklist updates with validation and activity history.
• Bug Fixes
  • Invoice changes now immediately recalculate the job’s fully invoiced status and remaining balance.
  • Improved error messages for accounting and Xero operations.

[coderabbitai]

Warning

Review limit reached

@corrin, you've reached your PR review limit, so we couldn't start this review.

Next review available in:24 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 052efda7-ef2c-4202-adef-941ebddc6980

📥 Commits

Reviewing files that changed from the base of the PR and between b318f2e and 09176e1.

📒 Files selected for processing (7)

• apps/xero/documents/invoice.py
• apps/xero/tests/test_invoice_manager.py
• apps/xero/tests/test_provider_documents.py
• config/tests/test_contract_gates.py
• docs/code-quality.md
• docs/rewrite-status.md
• scripts/checks/status_table.py

📝 Walkthrough

Walkthrough

This PR adds Finish Job financial and checklist APIs, Xero invoice and purchase-order synchronization, provider contracts, read-only behavior, and frontend Finish Job and invoice-management components with unit, API, and end-to-end tests.

Changes

Finish Job and Xero workflows

Layer / File(s)	Summary
Finish Job calculations and APIs apps/accounting/..., apps/job/..., frontend/schema.v2.yml	Adds financial summaries, checklist validation and updates, invoice listing, conditional responses, and Finish Job API contracts.
Accounting contracts and Xero provider operations apps/accounting/types.py, apps/accounting/provider.py, apps/xero/helpers.py, apps/xero/provider.py, apps/xero/readonly_provider.py, stubs/xero_python/...	Adds provider-agnostic document DTOs, Xero invoice and purchase-order operations, payload and error helpers, read-only results, and SDK stubs.
Xero document managers and endpoints apps/xero/documents/..., apps/xero/api.py, apps/xero/tests/...	Adds invoice and purchase-order validation, payload construction, persistence, deletion, authenticated endpoints, and manager tests.
Finish Job workspace and invoice UI frontend/src/features/job/..., frontend/src/api/..., frontend/src/lib/format.ts, frontend/tests/e2e/...	Adds lazy-loaded Finish Job and invoice components with financial displays, checklist updates, invoice actions, error handling, and end-to-end coverage.
Compatibility records and progress documentation docs/accepted-api-differences.yml, docs/code-quality.md, docs/rewrite-status.md, scripts/v1-frontend-operations.yml	Records API behavior changes, updated quality metrics, operation rename mappings, and rewrite progress.

Estimated code review effort: 5 (Critical) | ~120 minutes

Sequence Diagram(s)

sequenceDiagram
participant JobFinishTab
participant JobAPI
participant FinishJobSummary
participant JobService
JobFinishTab->>JobAPI: GET finish summary and checklist
JobAPI->>FinishJobSummary: calculate financial totals
FinishJobSummary-->>JobAPI: return summary
JobAPI-->>JobFinishTab: return JobFinishResponse
JobFinishTab->>JobAPI: PATCH checklist fields
JobAPI->>JobService: update checklist
JobService-->>JobAPI: return updated job

Loading

sequenceDiagram
participant JobInvoiceCard
participant XeroAPI
participant XeroInvoiceManager
participant XeroAccountingProvider
JobInvoiceCard->>XeroAPI: submit invoice request
XeroAPI->>XeroInvoiceManager: create document
XeroInvoiceManager->>XeroAccountingProvider: create InvoicePayload
XeroAccountingProvider-->>XeroInvoiceManager: return DocumentResult
XeroInvoiceManager-->>XeroAPI: return persisted invoice result
XeroAPI-->>JobInvoiceCard: return success or error

Loading

Possibly related PRs

• corrin/docketworks_v2#45: Extends the accounting provider and Xero abstractions used by this PR.
• corrin/docketworks_v2#18: Provides purchase-order models and status behavior used by Xero purchase-order synchronization.
• corrin/docketworks_v2#10: Provides the workshop PDF generation used by invoice attachment handling.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 44.10% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the main Xero invoice document push and Finish Job changes, with a useful E2E status indicator.
Description check	✅ Passed	The description is detailed, structured, and covers scope, behavior, verification results, known limitations, and parity changes.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch xero/invoice-path

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 14

🧹 Nitpick comments (5)

frontend/src/api/index.ts (1)

81-81: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Remove code-narration comments or document their constraint.

Both comments describe nearby code without stating the rejected alternative and the factual constraint.

• frontend/src/api/index.ts#L81-L81: Remove the export-group narration, or explain why feature code must import these generated operations through frontend/src/api/index.ts.
• frontend/src/features/job/JobFinishTab.tsx#L157-L157: Remove the ticket-heading comment. It does not document a runtime or business constraint.

As per coding guidelines, “Comments must document the rejected obvious alternative and the factual constraint that rejected it; remove code narration and review-feedback echoes.”

🤖 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 `@frontend/src/api/index.ts` at line 81, Remove the code-narration comments at
frontend/src/api/index.ts:81-81 and
frontend/src/features/job/JobFinishTab.tsx:157-157; no replacement documentation
is needed because neither comment states a factual constraint or rejected
alternative.

Source: Coding guidelines

apps/xero/tests/test_invoice_manager.py (1)

86-138: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a delete happy-path test.

TestErrorContract covers the three failure paths of delete_document. No test covers the success path, so nothing asserts that the local Invoice row is removed, that recalculate_job_invoicing_state runs, and that the invoice_deleted job event records the invoice number. That recalculation is the same same-request effect the create test protects at Line 256-258, and the Finish Job tab depends on it.

Do you want me to write this test?

🤖 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/xero/tests/test_invoice_manager.py` around lines 86 - 138, Add a
success-case test in TestErrorContract for delete_document using a configured
provider and Xero invoice ID, then assert the local Invoice is deleted,
recalculate_job_invoicing_state is called, and the invoice_deleted job event
records the invoice number. Keep the test focused on the same-request effects of
the happy path.

apps/xero/tests/test_po_manager.py (1)

98-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Cover the write direction of the zero-UUID sentinel too.

This test covers the read direction: a stored sentinel routes to create. The module docstring at Line 4-5 names "storing Xero's zero-UUID sentinel as a real id" as the expensive silent failure, and that is the write direction, guarded by _save_po_with_xero_data at apps/xero/documents/po.py Line 151.

Add a test where the provider returns DocumentResult(success=True, external_id=ZERO_UUID) and assert that po.xero_id stays None after sync_to_xero. This also pins the behavior discussed on apps/xero/provider.py Line 280-314.

Do you want me to write this test?

🤖 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/xero/tests/test_po_manager.py` around lines 98 - 109, Extend
test_zero_uuid_is_treated_as_unsynced to cover the write path: configure the
provider’s successful DocumentResult with external_id=ZERO_UUID, run
manager.sync_to_xero(), then refresh or inspect po.xero_id and assert it remains
None. Preserve the existing create-path assertion while exercising
_save_po_with_xero_data’s zero-UUID handling.

apps/xero/api.py (2)

317-328: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Type the clamp parameter as the contract, not as object.

XeroDocumentResponse declares status. Annotating the parameter object discards that contract and forces a runtime comparison against an untyped value. Declare int | None so mypy checks every call site.

♻️ Proposed signature change

-def _document_error_status(status: object) -> int:+def _document_error_status(status: int | None) -> int:

As per coding guidelines: "Treat type annotations as data contracts: use named types such as dataclasses, TypedDicts, or Protocols for complex inline types."

🤖 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/xero/api.py` around lines 317 - 328, Update the _document_error_status
parameter annotation from object to int | None, matching the status contract
declared by XeroDocumentResponse. Keep the existing 404 handling and 400
fallback unchanged, and let mypy validate all call sites against the narrowed
type.

Source: Coding guidelines

---

358-366: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract the repeated Xero-auth 401 block.

The same get_valid_token() guard and XeroAuthRequiredOut payload appear in all four new endpoints (lines 358-366, 458-466, 543-551, 612-620). The two purchase-order endpoints also repeat the identical lookup and supplier guard. One helper per repeated block keeps the message and status in step.

♻️ Proposed helper

def_xero_auth_required() ->Status[XeroAuthRequiredOut]:
"""Build the one 401 body every Xero document endpoint returns."""returnStatus(
401,
XeroAuthRequiredOut(
success=False,
redirect_to_auth=True,
message="Your Xero session has expired. Please log in again.",
),
)

As per coding guidelines: "Use one implementation per concept; search before implementing and extend a near-match instead of creating a parallel implementation."

🤖 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/xero/api.py` around lines 358 - 366, Extract the repeated
get_valid_token() failure response into a shared _xero_auth_required() helper
returning the existing 401 Status[XeroAuthRequiredOut] payload, then replace the
guards in all four Xero document endpoints with that helper. Also extract and
reuse a helper for the identical purchase-order lookup and supplier guard,
preserving the current validation behavior and messages.

Source: Coding guidelines

🤖 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 `@apps/job/tests/test_finish_api.py`:
- Around line 105-115: Correct the inline comment in test_lists_job_invoices to
state that the second invoice belongs to the same company but has no job, so it
must not appear in the job-scoped results. Do not change the test setup or
assertions.
In `@apps/xero/api.py`:
- Around line 64-72: Consolidate the duplicated `_staff` helpers by moving the
implementation from `apps/job/api.py` into `apps/core` and importing that shared
helper in `apps/xero/api.py`, `apps/purchasing/api.py`, and the applicable
callers. Remove redundant local definitions, while preserving any
Timesheet-specific narrowing behavior only if it cannot use the shared
implementation.
- Around line 402-405: Update the invoice creation flow around
XeroInvoiceManager.create_document to make retries safe at the Xero API
boundary: either supply a stable idempotency key for the same job/document
across retry attempts, or disable/prohibit RateLimitedRESTClient retries for
this non-idempotent POST. Preserve retries for safe operations and ensure one
logical create cannot produce duplicate Xero documents.
In `@apps/xero/documents/base.py`:
- Around line 82-99: Move the document-type validation before the best-effort
exception handler in both _add_xero_history_note (apps/xero/documents/base.py,
lines 82-99) and _add_history_note (apps/xero/provider.py, lines 407-421):
reject values outside ("invoice", "quote") before entering try, while leaving
provider calls inside the existing handler so only provider failures are
absorbed.
In `@apps/xero/documents/invoice.py`:
- Around line 232-241: Update create_document to validate that billing_metadata
contains target_total, prior_invoiced_total, and calculated_amount before any
direct reads. After validation, access all three keys directly and remove the
"0" defaults from the Decimal conversions so missing contract fields fail rather
than producing an uncomputed remaining_to_invoice value.
- Around line 198-216: Update the invoice creation flow after the existing
result.external_id and result.number validation to validate that raw contains
_sub_total, _total_tax, _total, and _amount_due before constructing Invoice.
Fail early using the existing validation pattern when any key is missing, then
read the validated values without zero fallbacks so invoice totals cannot be
silently stored as 0.00.
In `@apps/xero/documents/po.py`:
- Around line 23-25: Move ZERO_UUID to a shared module, remove the duplicate
definitions from documents/po.py and XeroAccountingProvider, and import the
shared constant in both consumers. Preserve the existing sentinel value and all
checks that use it.
- Around line 55-59: The Xero sync validation path must enforce the draft-only
rule for first-time creation. Update validate_for_xero_sync() to invoke
state_valid_for_xero() before allowing creation, while preserving updates for
POs with an existing Xero ID; keep the implementation and docstring of
state_valid_for_xero() consistent with this behavior.
In `@apps/xero/provider.py`:
- Around line 227-237: The invoice pre-read in the deletion flow must validate
that the API response contains an invoice and that its contact is present before
constructing the DELETED Invoice. Mirror the guards and user-facing error
messages used by delete_purchase_order, referencing the existing
invoice-deletion method and its existing/api/contact symbols while preserving
the normal construction path for valid data.
- Around line 280-314: Update the zero-UUID recovery flow around
_find_po_by_number so that when recovery returns None and po_id remains
ZERO_UUID, return a failed DocumentResult with no external_id instead of
continuing to the success path. Preserve the existing validation-error handling
and only construct the online_url and successful result after a real purchase
order ID is available.
- Around line 316-335: Update _find_po_by_number to enforce a finite maximum
page count while scanning purchase orders. Stop paging and return None once the
limit is reached, while preserving the existing match return and empty-page
termination behavior.
In `@frontend/src/features/job/JobInvoiceCard.tsx`:
- Around line 344-359: Give both numeric inputs in JobInvoiceCard accessible
names: add a visible label or aria-label to the percentage input at
frontend/src/features/job/JobInvoiceCard.tsx:344-359 identifying it as “Invoice
percentage of quote,” and to the amount input at
frontend/src/features/job/JobInvoiceCard.tsx:396-410 identifying it as “Invoice
amount.”
- Around line 233-239: Update the invoice link handler’s window.open call to
pass noopener,noreferrer in its third argument when opening invoice.online_url,
while preserving the existing URL check and toast behavior.
In `@frontend/src/lib/format.ts`:
- Around line 26-34: Update the NZ_DATE formatter used by formatDate to set
timeZone to Pacific/Auckland, ensuring invoice dates render consistently across
environments. Revise the formatter comment to state that per-page formatters are
rejected because invoice dates must display identical text across the workspace.
---
Nitpick comments:
In `@apps/xero/api.py`:
- Around line 317-328: Update the _document_error_status parameter annotation
from object to int | None, matching the status contract declared by
XeroDocumentResponse. Keep the existing 404 handling and 400 fallback unchanged,
and let mypy validate all call sites against the narrowed type.
- Around line 358-366: Extract the repeated get_valid_token() failure response
into a shared _xero_auth_required() helper returning the existing 401
Status[XeroAuthRequiredOut] payload, then replace the guards in all four Xero
document endpoints with that helper. Also extract and reuse a helper for the
identical purchase-order lookup and supplier guard, preserving the current
validation behavior and messages.
In `@apps/xero/tests/test_invoice_manager.py`:
- Around line 86-138: Add a success-case test in TestErrorContract for
delete_document using a configured provider and Xero invoice ID, then assert the
local Invoice is deleted, recalculate_job_invoicing_state is called, and the
invoice_deleted job event records the invoice number. Keep the test focused on
the same-request effects of the happy path.
In `@apps/xero/tests/test_po_manager.py`:
- Around line 98-109: Extend test_zero_uuid_is_treated_as_unsynced to cover the
write path: configure the provider’s successful DocumentResult with
external_id=ZERO_UUID, run manager.sync_to_xero(), then refresh or inspect
po.xero_id and assert it remains None. Preserve the existing create-path
assertion while exercising _save_po_with_xero_data’s zero-UUID handling.
In `@frontend/src/api/index.ts`:
- Line 81: Remove the code-narration comments at frontend/src/api/index.ts:81-81
and frontend/src/features/job/JobFinishTab.tsx:157-157; no replacement
documentation is needed because neither comment states a factual constraint or
rejected alternative.

🪄 Autofix

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: ea55edb0-3b70-4970-ae0f-3dafb7b206f3

📥 Commits

Reviewing files that changed from the base of the PR and between b5feeab and d1eea03.

⛔ Files ignored due to path filters (5)

• frontend/src/api/generated/@tanstack/react-query.gen.ts is excluded by !**/generated/**
• frontend/src/api/generated/index.ts is excluded by !**/generated/**
• frontend/src/api/generated/sdk.gen.ts is excluded by !**/generated/**
• frontend/src/api/generated/types.gen.ts is excluded by !**/generated/**
• frontend/src/api/generated/zod.gen.ts is excluded by !**/generated/**

📒 Files selected for processing (32)

• apps/accounting/provider.py
• apps/accounting/services/finish_job_summary.py
• apps/accounting/tests/test_finish_job_summary.py
• apps/accounting/types.py
• apps/company/tests/job_fixtures.py
• apps/job/api.py
• apps/job/schemas.py
• apps/job/services/job_service.py
• apps/job/tests/test_finish_api.py
• apps/xero/api.py
• apps/xero/documents/base.py
• apps/xero/documents/invoice.py
• apps/xero/documents/po.py
• apps/xero/helpers.py
• apps/xero/provider.py
• apps/xero/readonly_provider.py
• apps/xero/tests/test_invoice_manager.py
• apps/xero/tests/test_po_manager.py
• docs/accepted-api-differences.yml
• docs/code-quality.md
• docs/rewrite-status.md
• frontend/schema.v2.yml
• frontend/src/api/error-message.ts
• frontend/src/api/index.ts
• frontend/src/features/job/JobDetailPage.tsx
• frontend/src/features/job/JobFinishTab.test.tsx
• frontend/src/features/job/JobFinishTab.tsx
• frontend/src/features/job/JobInvoiceCard.tsx
• frontend/src/lib/format.ts
• frontend/tests/e2e/job/job-xero-invoice.spec.ts
• scripts/v1-frontend-operations.yml
• stubs/xero_python/accounting/__init__.pyi

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

docs/code-quality.md (1)

24-34: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Regenerate the suppression total.

Line 24 reports 431, but the visible rows sum to 430: fixed suppressions total 13 and rule-coded suppressions total 417. scripts/checks/code_quality.py derives the total from the counted entries, so this report is internally inconsistent.

Regenerate the file, then run uv run python -m scripts.checks.code_quality --check.

Expected table correction

-| TOTAL suppressions | 431 |+| TOTAL suppressions | 430 |

The canonical metric generator in scripts/checks/code_quality.py is the source of truth.

🤖 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/code-quality.md` around lines 24 - 34, Regenerate the suppression table
in docs/code-quality.md using scripts/checks/code_quality.py so TOTAL
suppressions matches the visible counted entries (430). Then run uv run python
-m scripts.checks.code_quality --check and ensure validation passes.

♻️ Duplicate comments (1)

apps/xero/documents/invoice.py (1)

246-251: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Validate billing_metadata before invoice creation.

Lines 249-251 parse required values after provider.create_invoice() and Invoice.objects.create(). If a required key is missing or non-decimal, the request fails after the financial operation has completed.

Parse and validate these fields at the start of create_document, before provider submission. Reuse the validated values when creating the audit event.

As per coding guidelines: “Fail early: check invalid cases first, validate required inputs up front.”

🤖 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/xero/documents/invoice.py` around lines 246 - 251, Update
create_document to parse and validate billing_metadata["target_total"],
billing_metadata["prior_invoiced_total"], and
billing_metadata["calculated_amount"] before calling provider.create_invoice()
or Invoice.objects.create(). Reuse these validated Decimal values when
constructing the audit event, removing the later direct parsing while preserving
the existing required-key and invalid-decimal failures.

Source: Coding guidelines

🧹 Nitpick comments (1)

apps/xero/tests/test_provider_documents.py (1)

216-236: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert the recovery pagination request.

_find_po_by_number iterates get_purchase_orders(tenant_id, page=page), but this test only asserts two calls. A regression that requests page one twice can still pass because the mocked side_effect returns a different response each time.

Assert the page argument for both get_purchase_orders calls.

🤖 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/xero/tests/test_provider_documents.py` around lines 216 - 236, Update
test_zero_uuid_recovers_real_id_across_pages to inspect
api.get_purchase_orders.call_args_list and assert the two calls use page=1 and
page=2 respectively, while preserving the existing tenant_id assertions and
recovery expectations.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@docs/code-quality.md`:
- Around line 24-34: Regenerate the suppression table in docs/code-quality.md
using scripts/checks/code_quality.py so TOTAL suppressions matches the visible
counted entries (430). Then run uv run python -m scripts.checks.code_quality
--check and ensure validation passes.
---
Duplicate comments:
In `@apps/xero/documents/invoice.py`:
- Around line 246-251: Update create_document to parse and validate
billing_metadata["target_total"], billing_metadata["prior_invoiced_total"], and
billing_metadata["calculated_amount"] before calling provider.create_invoice()
or Invoice.objects.create(). Reuse these validated Decimal values when
constructing the audit event, removing the later direct parsing while preserving
the existing required-key and invalid-decimal failures.
---
Nitpick comments:
In `@apps/xero/tests/test_provider_documents.py`:
- Around line 216-236: Update test_zero_uuid_recovers_real_id_across_pages to
inspect api.get_purchase_orders.call_args_list and assert the two calls use
page=1 and page=2 respectively, while preserving the existing tenant_id
assertions and recovery expectations.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 10f8aa42-f281-4b7d-8f3f-3e62a97f3bec

📥 Commits

Reviewing files that changed from the base of the PR and between d1eea03 and b318f2e.

📒 Files selected for processing (14)

• apps/job/tests/test_finish_api.py
• apps/xero/api.py
• apps/xero/constants.py
• apps/xero/documents/base.py
• apps/xero/documents/invoice.py
• apps/xero/documents/po.py
• apps/xero/provider.py
• apps/xero/tests/test_document_api.py
• apps/xero/tests/test_po_manager.py
• apps/xero/tests/test_provider_documents.py
• docs/code-quality.md
• docs/rewrite-status.md
• frontend/src/features/job/JobInvoiceCard.tsx
• frontend/src/lib/format.ts

🚧 Files skipped from review as they are similar to previous changes (9)

• frontend/src/lib/format.ts
• apps/job/tests/test_finish_api.py
• apps/xero/documents/base.py
• apps/xero/tests/test_po_manager.py
• docs/rewrite-status.md
• apps/xero/provider.py
• apps/xero/api.py
• frontend/src/features/job/JobInvoiceCard.tsx
• apps/xero/documents/po.py

[corrin]

Round-2 items, addressed in 2fb241c:

• billing_metadata validation ordering — fixed: the three Decimals now parse at the top of create_document, before anything reaches the provider, and the audit event reuses the validated values.
• page assertions in the zero-UUID recovery test — fixed: the test now asserts page=1, page=2 from call_args_list.
• docs/code-quality.md TOTAL 'mismatch' — rebutting: the file is generated and uv run python -m scripts.checks.code_quality --check passes against it, so it is exactly what the generator produces; the TOTAL row includes suppression categories beyond the per-code breakdown rows by design. Hand-editing a generated file is what the gate exists to prevent.

[corrin]

Verification upgraded per the corrected test regime: XERO_READONLY is only for dev sessions pointed at production (hotfixes) — testing runs with writes live. Re-ran the full suite with XERO_READONLY=false against the demo tenant: 38 passed, with job-xero-invoice exercising the real provider path end-to-end (actual create_invoices, workshop-PDF attachment, history note — 9.7s vs 5.3s under readonly, the difference being the real Xero round-trips). Local .env flipped to XERO_READONLY=false as the standing dev configuration; rewrite-status records the regime in 46a06d3.

[corrin]

Slice 2b — invoice path: document push + Finish Job (job-xero-invoice green, 13/40)

Second of the three slice-2 PRs (2a sync engine #46 · 2b this · 2c quote path next). Ports the Xero document push and the Finish Job workspace from v1.

What ports

Document manager base + invoice push (apps/xero/documents/{base,invoice}.py, apps/xero/helpers.py)

• XeroDocumentManager with the config guards (branding theme, quote terms), best-effort history notes, and fail-early construction; XeroInvoiceManager builds a provider-agnostic InvoicePayload, calls get_provider().create_invoice(), and owns local persistence: Invoice mirror row from the provider's canonical raw payload, billing_metadata audit trail, JobEvent, and a same-request recalculate_job_invoicing_state (ledgered — v1 left fully_invoiced to the hourly sync, which under XERO_READONLY never sees the invoice).
• Endpoints POST /api/xero/create_invoice/{job_id} and DELETE /api/xero/delete_invoice/{job_id} keep v1's URL fragments so the spec's request matcher ports unedited. Amount derivation goes through the already-ported calculate_invoice_amount (modes: full / costs-to-date / percent / amount).

Purchase-order push (apps/xero/documents/po.py) — USER-COMMITTED scope

• Create-vs-update keyed on xero_id with the zero-UUID sentinel treated as absent on read and write (v1 stored it), paged recovery of the real id by PO number, per-line xero_line_item_id backfill with duplicate-safe description matching, status mapping that raises on unknown statuses. No spec covers PO push; apps/xero/tests/test_po_manager.py (12 tests) is its gate.
• Ledgered: v1's successful PO delete always returned 500 (its success dict failed its own response serializer).

Readonly works by construction: XeroReadOnlyProvider fabricates well-formed results — INV-E2E-* numbers, GST-exclusive fake totals from the line items × CompanyDefaults.gst_rate — so the endpoint path is byte-identical under XERO_READONLY and the E2E balance-settles-to-$0 assertion exercises the real persistence code. Missing write overrides hit tripwires, not the tenant.

Finish Job backend (apps/accounting/services/finish_job_summary.py, apps/job/api.py)

• Server-owned balance (v1-verbatim arithmetic: remaining excl/incl GST, outstanding, over-invoiced reported rather than netted off), completion-checklist read/patch through Job.save()'s field-change audit machinery (unknown keys 422 via extra="forbid", schema field lists pinned to Job.COMPLETION_CHECKLIST_FIELDS at import), and the ETagged invoice list. The finish GET is deliberately un-ETagged: the balance moves when invoices sync in without touching job.updated_at.

Frontend (JobFinishTab.tsx, JobInvoiceCard.tsx, lazy-loaded into JobDetailPage)

• Balance, checklist, labour-hours cards, estimate/quote/actual comparison, quote accuracy; invoice list with create dialog (modes per pricing methodology) and delete. All money formatted from server values — no client recomputation. All v1 automation ids preserved. apiErrorMessage now also reads the document endpoints' error key so calc/configuration guidance reaches the user.

Spec: job-xero-invoice.spec.ts ports near-verbatim, with one deliberate deviation — it creates its own job instead of using sharedEditJobUrl, because it fully invoices the job and the shared fixture is read-only by contract (v1 mutated it and survived on run ordering).

Adversarial review (2 subagents) — notable catches, all fixed

• Provider-driven HTTP statuses (429/503 on live-Xero failures, 401 mid-request revocation) weren't in the declared response maps — ninja would raise ConfigError instead of returning the error payload. Statuses now clamp to the declared map; the cause stays in error (ADR 0038).
• The PO zero-UUID sentinel could be stored when the unpaginated recovery listing missed the PO (>100 POs) — guard added on write, recovery listing paged.
• TanStack v5 refetch() never rejects, so the "list could not be refreshed" toast was unreachable; an invoices load error rendered as the "No invoices" empty state.
• Known, deliberately not touched (v1-parity): calculate_invoice_amount doesn't quantize, so a sub-cent basis total (e.g. 157.985) can never read fully-invoiced. Real jobs have 2dp values; flagged for 2c or later rather than changing the money path mid-slice.

Verified

• ./scripts/ops/run_e2e.sh: 38 passed — job-xero-invoice green under XERO_READONLY (13/40 specs), all prior specs green with the sync beat live.
• Backend: 1617 unit tests green; new coverage: PO manager 12, invoice manager 7 (error contract / raw_json / readonly fabrication), finish summary 11, finish API 7. Frontend: JobFinishTab component tests 3.
• Expensive tier clean (mypy strict zero-baseline, import-linter, find-duplicates, deptry, schema-current, status table).
• Parity ledger: renamed: entries for all four document operation ids; two new behaviour entries in docs/accepted-api-differences.yml.

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features
  • Added a Finish Job workspace with financial summaries, invoice balances, cost comparisons, completion checklists, and variance indicators.
  • Added invoice listing and management, including full, percentage-based, cost-to-date, and custom-amount invoicing.
  • Added Xero invoice and purchase-order creation, updating, deletion, and invoice file attachments.
  • Added checklist updates with validation and activity history.
• Bug Fixes
  • Invoice changes now immediately recalculate the job’s fully invoiced status and remaining balance.
  • Improved error messages for accounting and Xero operations.

[coderabbitai]

Warning

Review limit reached

@corrin, you've reached your PR review limit, so we couldn't start this review.

Next review available in:24 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 052efda7-ef2c-4202-adef-941ebddc6980

📥 Commits

Reviewing files that changed from the base of the PR and between b318f2e and 09176e1.

📒 Files selected for processing (7)

• apps/xero/documents/invoice.py
• apps/xero/tests/test_invoice_manager.py
• apps/xero/tests/test_provider_documents.py
• config/tests/test_contract_gates.py
• docs/code-quality.md
• docs/rewrite-status.md
• scripts/checks/status_table.py

📝 Walkthrough

Walkthrough

This PR adds Finish Job financial and checklist APIs, Xero invoice and purchase-order synchronization, provider contracts, read-only behavior, and frontend Finish Job and invoice-management components with unit, API, and end-to-end tests.

Changes

Finish Job and Xero workflows

Layer / File(s)	Summary
Finish Job calculations and APIs apps/accounting/..., apps/job/..., frontend/schema.v2.yml	Adds financial summaries, checklist validation and updates, invoice listing, conditional responses, and Finish Job API contracts.
Accounting contracts and Xero provider operations apps/accounting/types.py, apps/accounting/provider.py, apps/xero/helpers.py, apps/xero/provider.py, apps/xero/readonly_provider.py, stubs/xero_python/...	Adds provider-agnostic document DTOs, Xero invoice and purchase-order operations, payload and error helpers, read-only results, and SDK stubs.
Xero document managers and endpoints apps/xero/documents/..., apps/xero/api.py, apps/xero/tests/...	Adds invoice and purchase-order validation, payload construction, persistence, deletion, authenticated endpoints, and manager tests.
Finish Job workspace and invoice UI frontend/src/features/job/..., frontend/src/api/..., frontend/src/lib/format.ts, frontend/tests/e2e/...	Adds lazy-loaded Finish Job and invoice components with financial displays, checklist updates, invoice actions, error handling, and end-to-end coverage.
Compatibility records and progress documentation docs/accepted-api-differences.yml, docs/code-quality.md, docs/rewrite-status.md, scripts/v1-frontend-operations.yml	Records API behavior changes, updated quality metrics, operation rename mappings, and rewrite progress.

Estimated code review effort: 5 (Critical) | ~120 minutes

Sequence Diagram(s)

sequenceDiagram
participant JobFinishTab
participant JobAPI
participant FinishJobSummary
participant JobService
JobFinishTab->>JobAPI: GET finish summary and checklist
JobAPI->>FinishJobSummary: calculate financial totals
FinishJobSummary-->>JobAPI: return summary
JobAPI-->>JobFinishTab: return JobFinishResponse
JobFinishTab->>JobAPI: PATCH checklist fields
JobAPI->>JobService: update checklist
JobService-->>JobAPI: return updated job

Loading

sequenceDiagram
participant JobInvoiceCard
participant XeroAPI
participant XeroInvoiceManager
participant XeroAccountingProvider
JobInvoiceCard->>XeroAPI: submit invoice request
XeroAPI->>XeroInvoiceManager: create document
XeroInvoiceManager->>XeroAccountingProvider: create InvoicePayload
XeroAccountingProvider-->>XeroInvoiceManager: return DocumentResult
XeroInvoiceManager-->>XeroAPI: return persisted invoice result
XeroAPI-->>JobInvoiceCard: return success or error

Loading

Possibly related PRs

• corrin/docketworks_v2#45: Extends the accounting provider and Xero abstractions used by this PR.
• corrin/docketworks_v2#18: Provides purchase-order models and status behavior used by Xero purchase-order synchronization.
• corrin/docketworks_v2#10: Provides the workshop PDF generation used by invoice attachment handling.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 44.10% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the main Xero invoice document push and Finish Job changes, with a useful E2E status indicator.
Description check	✅ Passed	The description is detailed, structured, and covers scope, behavior, verification results, known limitations, and parity changes.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch xero/invoice-path

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 14

🧹 Nitpick comments (5)

frontend/src/api/index.ts (1)

81-81: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Remove code-narration comments or document their constraint.

Both comments describe nearby code without stating the rejected alternative and the factual constraint.

• frontend/src/api/index.ts#L81-L81: Remove the export-group narration, or explain why feature code must import these generated operations through frontend/src/api/index.ts.
• frontend/src/features/job/JobFinishTab.tsx#L157-L157: Remove the ticket-heading comment. It does not document a runtime or business constraint.

As per coding guidelines, “Comments must document the rejected obvious alternative and the factual constraint that rejected it; remove code narration and review-feedback echoes.”

🤖 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 `@frontend/src/api/index.ts` at line 81, Remove the code-narration comments at
frontend/src/api/index.ts:81-81 and
frontend/src/features/job/JobFinishTab.tsx:157-157; no replacement documentation
is needed because neither comment states a factual constraint or rejected
alternative.

Source: Coding guidelines

apps/xero/tests/test_invoice_manager.py (1)

86-138: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a delete happy-path test.

TestErrorContract covers the three failure paths of delete_document. No test covers the success path, so nothing asserts that the local Invoice row is removed, that recalculate_job_invoicing_state runs, and that the invoice_deleted job event records the invoice number. That recalculation is the same same-request effect the create test protects at Line 256-258, and the Finish Job tab depends on it.

Do you want me to write this test?

🤖 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/xero/tests/test_invoice_manager.py` around lines 86 - 138, Add a
success-case test in TestErrorContract for delete_document using a configured
provider and Xero invoice ID, then assert the local Invoice is deleted,
recalculate_job_invoicing_state is called, and the invoice_deleted job event
records the invoice number. Keep the test focused on the same-request effects of
the happy path.

apps/xero/tests/test_po_manager.py (1)

98-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Cover the write direction of the zero-UUID sentinel too.

This test covers the read direction: a stored sentinel routes to create. The module docstring at Line 4-5 names "storing Xero's zero-UUID sentinel as a real id" as the expensive silent failure, and that is the write direction, guarded by _save_po_with_xero_data at apps/xero/documents/po.py Line 151.

Add a test where the provider returns DocumentResult(success=True, external_id=ZERO_UUID) and assert that po.xero_id stays None after sync_to_xero. This also pins the behavior discussed on apps/xero/provider.py Line 280-314.

Do you want me to write this test?

🤖 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/xero/tests/test_po_manager.py` around lines 98 - 109, Extend
test_zero_uuid_is_treated_as_unsynced to cover the write path: configure the
provider’s successful DocumentResult with external_id=ZERO_UUID, run
manager.sync_to_xero(), then refresh or inspect po.xero_id and assert it remains
None. Preserve the existing create-path assertion while exercising
_save_po_with_xero_data’s zero-UUID handling.

apps/xero/api.py (2)

317-328: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Type the clamp parameter as the contract, not as object.

XeroDocumentResponse declares status. Annotating the parameter object discards that contract and forces a runtime comparison against an untyped value. Declare int | None so mypy checks every call site.

♻️ Proposed signature change

-def _document_error_status(status: object) -> int:+def _document_error_status(status: int | None) -> int:

As per coding guidelines: "Treat type annotations as data contracts: use named types such as dataclasses, TypedDicts, or Protocols for complex inline types."

🤖 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/xero/api.py` around lines 317 - 328, Update the _document_error_status
parameter annotation from object to int | None, matching the status contract
declared by XeroDocumentResponse. Keep the existing 404 handling and 400
fallback unchanged, and let mypy validate all call sites against the narrowed
type.

Source: Coding guidelines

---

358-366: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract the repeated Xero-auth 401 block.

The same get_valid_token() guard and XeroAuthRequiredOut payload appear in all four new endpoints (lines 358-366, 458-466, 543-551, 612-620). The two purchase-order endpoints also repeat the identical lookup and supplier guard. One helper per repeated block keeps the message and status in step.

♻️ Proposed helper

def_xero_auth_required() ->Status[XeroAuthRequiredOut]:
"""Build the one 401 body every Xero document endpoint returns."""returnStatus(
401,
XeroAuthRequiredOut(
success=False,
redirect_to_auth=True,
message="Your Xero session has expired. Please log in again.",
),
)

As per coding guidelines: "Use one implementation per concept; search before implementing and extend a near-match instead of creating a parallel implementation."

🤖 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/xero/api.py` around lines 358 - 366, Extract the repeated
get_valid_token() failure response into a shared _xero_auth_required() helper
returning the existing 401 Status[XeroAuthRequiredOut] payload, then replace the
guards in all four Xero document endpoints with that helper. Also extract and
reuse a helper for the identical purchase-order lookup and supplier guard,
preserving the current validation behavior and messages.

Source: Coding guidelines

🤖 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 `@apps/job/tests/test_finish_api.py`:
- Around line 105-115: Correct the inline comment in test_lists_job_invoices to
state that the second invoice belongs to the same company but has no job, so it
must not appear in the job-scoped results. Do not change the test setup or
assertions.
In `@apps/xero/api.py`:
- Around line 64-72: Consolidate the duplicated `_staff` helpers by moving the
implementation from `apps/job/api.py` into `apps/core` and importing that shared
helper in `apps/xero/api.py`, `apps/purchasing/api.py`, and the applicable
callers. Remove redundant local definitions, while preserving any
Timesheet-specific narrowing behavior only if it cannot use the shared
implementation.
- Around line 402-405: Update the invoice creation flow around
XeroInvoiceManager.create_document to make retries safe at the Xero API
boundary: either supply a stable idempotency key for the same job/document
across retry attempts, or disable/prohibit RateLimitedRESTClient retries for
this non-idempotent POST. Preserve retries for safe operations and ensure one
logical create cannot produce duplicate Xero documents.
In `@apps/xero/documents/base.py`:
- Around line 82-99: Move the document-type validation before the best-effort
exception handler in both _add_xero_history_note (apps/xero/documents/base.py,
lines 82-99) and _add_history_note (apps/xero/provider.py, lines 407-421):
reject values outside ("invoice", "quote") before entering try, while leaving
provider calls inside the existing handler so only provider failures are
absorbed.
In `@apps/xero/documents/invoice.py`:
- Around line 232-241: Update create_document to validate that billing_metadata
contains target_total, prior_invoiced_total, and calculated_amount before any
direct reads. After validation, access all three keys directly and remove the
"0" defaults from the Decimal conversions so missing contract fields fail rather
than producing an uncomputed remaining_to_invoice value.
- Around line 198-216: Update the invoice creation flow after the existing
result.external_id and result.number validation to validate that raw contains
_sub_total, _total_tax, _total, and _amount_due before constructing Invoice.
Fail early using the existing validation pattern when any key is missing, then
read the validated values without zero fallbacks so invoice totals cannot be
silently stored as 0.00.
In `@apps/xero/documents/po.py`:
- Around line 23-25: Move ZERO_UUID to a shared module, remove the duplicate
definitions from documents/po.py and XeroAccountingProvider, and import the
shared constant in both consumers. Preserve the existing sentinel value and all
checks that use it.
- Around line 55-59: The Xero sync validation path must enforce the draft-only
rule for first-time creation. Update validate_for_xero_sync() to invoke
state_valid_for_xero() before allowing creation, while preserving updates for
POs with an existing Xero ID; keep the implementation and docstring of
state_valid_for_xero() consistent with this behavior.
In `@apps/xero/provider.py`:
- Around line 227-237: The invoice pre-read in the deletion flow must validate
that the API response contains an invoice and that its contact is present before
constructing the DELETED Invoice. Mirror the guards and user-facing error
messages used by delete_purchase_order, referencing the existing
invoice-deletion method and its existing/api/contact symbols while preserving
the normal construction path for valid data.
- Around line 280-314: Update the zero-UUID recovery flow around
_find_po_by_number so that when recovery returns None and po_id remains
ZERO_UUID, return a failed DocumentResult with no external_id instead of
continuing to the success path. Preserve the existing validation-error handling
and only construct the online_url and successful result after a real purchase
order ID is available.
- Around line 316-335: Update _find_po_by_number to enforce a finite maximum
page count while scanning purchase orders. Stop paging and return None once the
limit is reached, while preserving the existing match return and empty-page
termination behavior.
In `@frontend/src/features/job/JobInvoiceCard.tsx`:
- Around line 344-359: Give both numeric inputs in JobInvoiceCard accessible
names: add a visible label or aria-label to the percentage input at
frontend/src/features/job/JobInvoiceCard.tsx:344-359 identifying it as “Invoice
percentage of quote,” and to the amount input at
frontend/src/features/job/JobInvoiceCard.tsx:396-410 identifying it as “Invoice
amount.”
- Around line 233-239: Update the invoice link handler’s window.open call to
pass noopener,noreferrer in its third argument when opening invoice.online_url,
while preserving the existing URL check and toast behavior.
In `@frontend/src/lib/format.ts`:
- Around line 26-34: Update the NZ_DATE formatter used by formatDate to set
timeZone to Pacific/Auckland, ensuring invoice dates render consistently across
environments. Revise the formatter comment to state that per-page formatters are
rejected because invoice dates must display identical text across the workspace.
---
Nitpick comments:
In `@apps/xero/api.py`:
- Around line 317-328: Update the _document_error_status parameter annotation
from object to int | None, matching the status contract declared by
XeroDocumentResponse. Keep the existing 404 handling and 400 fallback unchanged,
and let mypy validate all call sites against the narrowed type.
- Around line 358-366: Extract the repeated get_valid_token() failure response
into a shared _xero_auth_required() helper returning the existing 401
Status[XeroAuthRequiredOut] payload, then replace the guards in all four Xero
document endpoints with that helper. Also extract and reuse a helper for the
identical purchase-order lookup and supplier guard, preserving the current
validation behavior and messages.
In `@apps/xero/tests/test_invoice_manager.py`:
- Around line 86-138: Add a success-case test in TestErrorContract for
delete_document using a configured provider and Xero invoice ID, then assert the
local Invoice is deleted, recalculate_job_invoicing_state is called, and the
invoice_deleted job event records the invoice number. Keep the test focused on
the same-request effects of the happy path.
In `@apps/xero/tests/test_po_manager.py`:
- Around line 98-109: Extend test_zero_uuid_is_treated_as_unsynced to cover the
write path: configure the provider’s successful DocumentResult with
external_id=ZERO_UUID, run manager.sync_to_xero(), then refresh or inspect
po.xero_id and assert it remains None. Preserve the existing create-path
assertion while exercising _save_po_with_xero_data’s zero-UUID handling.
In `@frontend/src/api/index.ts`:
- Line 81: Remove the code-narration comments at frontend/src/api/index.ts:81-81
and frontend/src/features/job/JobFinishTab.tsx:157-157; no replacement
documentation is needed because neither comment states a factual constraint or
rejected alternative.

🪄 Autofix

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: ea55edb0-3b70-4970-ae0f-3dafb7b206f3

📥 Commits

Reviewing files that changed from the base of the PR and between b5feeab and d1eea03.

⛔ Files ignored due to path filters (5)

• frontend/src/api/generated/@tanstack/react-query.gen.ts is excluded by !**/generated/**
• frontend/src/api/generated/index.ts is excluded by !**/generated/**
• frontend/src/api/generated/sdk.gen.ts is excluded by !**/generated/**
• frontend/src/api/generated/types.gen.ts is excluded by !**/generated/**
• frontend/src/api/generated/zod.gen.ts is excluded by !**/generated/**

📒 Files selected for processing (32)

• apps/accounting/provider.py
• apps/accounting/services/finish_job_summary.py
• apps/accounting/tests/test_finish_job_summary.py
• apps/accounting/types.py
• apps/company/tests/job_fixtures.py
• apps/job/api.py
• apps/job/schemas.py
• apps/job/services/job_service.py
• apps/job/tests/test_finish_api.py
• apps/xero/api.py
• apps/xero/documents/base.py
• apps/xero/documents/invoice.py
• apps/xero/documents/po.py
• apps/xero/helpers.py
• apps/xero/provider.py
• apps/xero/readonly_provider.py
• apps/xero/tests/test_invoice_manager.py
• apps/xero/tests/test_po_manager.py
• docs/accepted-api-differences.yml
• docs/code-quality.md
• docs/rewrite-status.md
• frontend/schema.v2.yml
• frontend/src/api/error-message.ts
• frontend/src/api/index.ts
• frontend/src/features/job/JobDetailPage.tsx
• frontend/src/features/job/JobFinishTab.test.tsx
• frontend/src/features/job/JobFinishTab.tsx
• frontend/src/features/job/JobInvoiceCard.tsx
• frontend/src/lib/format.ts
• frontend/tests/e2e/job/job-xero-invoice.spec.ts
• scripts/v1-frontend-operations.yml
• stubs/xero_python/accounting/__init__.pyi

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

docs/code-quality.md (1)

24-34: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Regenerate the suppression total.

Line 24 reports 431, but the visible rows sum to 430: fixed suppressions total 13 and rule-coded suppressions total 417. scripts/checks/code_quality.py derives the total from the counted entries, so this report is internally inconsistent.

Regenerate the file, then run uv run python -m scripts.checks.code_quality --check.

Expected table correction

-| TOTAL suppressions | 431 |+| TOTAL suppressions | 430 |

The canonical metric generator in scripts/checks/code_quality.py is the source of truth.

🤖 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/code-quality.md` around lines 24 - 34, Regenerate the suppression table
in docs/code-quality.md using scripts/checks/code_quality.py so TOTAL
suppressions matches the visible counted entries (430). Then run uv run python
-m scripts.checks.code_quality --check and ensure validation passes.

♻️ Duplicate comments (1)

apps/xero/documents/invoice.py (1)

246-251: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Validate billing_metadata before invoice creation.

Lines 249-251 parse required values after provider.create_invoice() and Invoice.objects.create(). If a required key is missing or non-decimal, the request fails after the financial operation has completed.

Parse and validate these fields at the start of create_document, before provider submission. Reuse the validated values when creating the audit event.

As per coding guidelines: “Fail early: check invalid cases first, validate required inputs up front.”

🤖 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/xero/documents/invoice.py` around lines 246 - 251, Update
create_document to parse and validate billing_metadata["target_total"],
billing_metadata["prior_invoiced_total"], and
billing_metadata["calculated_amount"] before calling provider.create_invoice()
or Invoice.objects.create(). Reuse these validated Decimal values when
constructing the audit event, removing the later direct parsing while preserving
the existing required-key and invalid-decimal failures.

Source: Coding guidelines

🧹 Nitpick comments (1)

apps/xero/tests/test_provider_documents.py (1)

216-236: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert the recovery pagination request.

_find_po_by_number iterates get_purchase_orders(tenant_id, page=page), but this test only asserts two calls. A regression that requests page one twice can still pass because the mocked side_effect returns a different response each time.

Assert the page argument for both get_purchase_orders calls.

🤖 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/xero/tests/test_provider_documents.py` around lines 216 - 236, Update
test_zero_uuid_recovers_real_id_across_pages to inspect
api.get_purchase_orders.call_args_list and assert the two calls use page=1 and
page=2 respectively, while preserving the existing tenant_id assertions and
recovery expectations.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@docs/code-quality.md`:
- Around line 24-34: Regenerate the suppression table in docs/code-quality.md
using scripts/checks/code_quality.py so TOTAL suppressions matches the visible
counted entries (430). Then run uv run python -m scripts.checks.code_quality
--check and ensure validation passes.
---
Duplicate comments:
In `@apps/xero/documents/invoice.py`:
- Around line 246-251: Update create_document to parse and validate
billing_metadata["target_total"], billing_metadata["prior_invoiced_total"], and
billing_metadata["calculated_amount"] before calling provider.create_invoice()
or Invoice.objects.create(). Reuse these validated Decimal values when
constructing the audit event, removing the later direct parsing while preserving
the existing required-key and invalid-decimal failures.
---
Nitpick comments:
In `@apps/xero/tests/test_provider_documents.py`:
- Around line 216-236: Update test_zero_uuid_recovers_real_id_across_pages to
inspect api.get_purchase_orders.call_args_list and assert the two calls use
page=1 and page=2 respectively, while preserving the existing tenant_id
assertions and recovery expectations.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 10f8aa42-f281-4b7d-8f3f-3e62a97f3bec

📥 Commits

Reviewing files that changed from the base of the PR and between d1eea03 and b318f2e.

📒 Files selected for processing (14)

• apps/job/tests/test_finish_api.py
• apps/xero/api.py
• apps/xero/constants.py
• apps/xero/documents/base.py
• apps/xero/documents/invoice.py
• apps/xero/documents/po.py
• apps/xero/provider.py
• apps/xero/tests/test_document_api.py
• apps/xero/tests/test_po_manager.py
• apps/xero/tests/test_provider_documents.py
• docs/code-quality.md
• docs/rewrite-status.md
• frontend/src/features/job/JobInvoiceCard.tsx
• frontend/src/lib/format.ts

🚧 Files skipped from review as they are similar to previous changes (9)

• frontend/src/lib/format.ts
• apps/job/tests/test_finish_api.py
• apps/xero/documents/base.py
• apps/xero/tests/test_po_manager.py
• docs/rewrite-status.md
• apps/xero/provider.py
• apps/xero/api.py
• frontend/src/features/job/JobInvoiceCard.tsx
• apps/xero/documents/po.py

[corrin]

Round-2 items, addressed in 2fb241c:

• billing_metadata validation ordering — fixed: the three Decimals now parse at the top of create_document, before anything reaches the provider, and the audit event reuses the validated values.
• page assertions in the zero-UUID recovery test — fixed: the test now asserts page=1, page=2 from call_args_list.
• docs/code-quality.md TOTAL 'mismatch' — rebutting: the file is generated and uv run python -m scripts.checks.code_quality --check passes against it, so it is exactly what the generator produces; the TOTAL row includes suppression categories beyond the per-code breakdown rows by design. Hand-editing a generated file is what the gate exists to prevent.

[corrin]

Verification upgraded per the corrected test regime: XERO_READONLY is only for dev sessions pointed at production (hotfixes) — testing runs with writes live. Re-ran the full suite with XERO_READONLY=false against the demo tenant: 38 passed, with job-xero-invoice exercising the real provider path end-to-end (actual create_invoices, workshop-PDF attachment, history note — 9.7s vs 5.3s under readonly, the difference being the real Xero round-trips). Local .env flipped to XERO_READONLY=false as the standing dev configuration; rewrite-status records the regime in 46a06d3.

[corrin]

Slice 2b — invoice path: document push + Finish Job (job-xero-invoice green, 13/40)

Second of the three slice-2 PRs (2a sync engine #46 · 2b this · 2c quote path next). Ports the Xero document push and the Finish Job workspace from v1.

What ports

Document manager base + invoice push (apps/xero/documents/{base,invoice}.py, apps/xero/helpers.py)

• XeroDocumentManager with the config guards (branding theme, quote terms), best-effort history notes, and fail-early construction; XeroInvoiceManager builds a provider-agnostic InvoicePayload, calls get_provider().create_invoice(), and owns local persistence: Invoice mirror row from the provider's canonical raw payload, billing_metadata audit trail, JobEvent, and a same-request recalculate_job_invoicing_state (ledgered — v1 left fully_invoiced to the hourly sync, which under XERO_READONLY never sees the invoice).
• Endpoints POST /api/xero/create_invoice/{job_id} and DELETE /api/xero/delete_invoice/{job_id} keep v1's URL fragments so the spec's request matcher ports unedited. Amount derivation goes through the already-ported calculate_invoice_amount (modes: full / costs-to-date / percent / amount).

Purchase-order push (apps/xero/documents/po.py) — USER-COMMITTED scope

• Create-vs-update keyed on xero_id with the zero-UUID sentinel treated as absent on read and write (v1 stored it), paged recovery of the real id by PO number, per-line xero_line_item_id backfill with duplicate-safe description matching, status mapping that raises on unknown statuses. No spec covers PO push; apps/xero/tests/test_po_manager.py (12 tests) is its gate.
• Ledgered: v1's successful PO delete always returned 500 (its success dict failed its own response serializer).

Readonly works by construction: XeroReadOnlyProvider fabricates well-formed results — INV-E2E-* numbers, GST-exclusive fake totals from the line items × CompanyDefaults.gst_rate — so the endpoint path is byte-identical under XERO_READONLY and the E2E balance-settles-to-$0 assertion exercises the real persistence code. Missing write overrides hit tripwires, not the tenant.

Finish Job backend (apps/accounting/services/finish_job_summary.py, apps/job/api.py)

• Server-owned balance (v1-verbatim arithmetic: remaining excl/incl GST, outstanding, over-invoiced reported rather than netted off), completion-checklist read/patch through Job.save()'s field-change audit machinery (unknown keys 422 via extra="forbid", schema field lists pinned to Job.COMPLETION_CHECKLIST_FIELDS at import), and the ETagged invoice list. The finish GET is deliberately un-ETagged: the balance moves when invoices sync in without touching job.updated_at.

Frontend (JobFinishTab.tsx, JobInvoiceCard.tsx, lazy-loaded into JobDetailPage)

• Balance, checklist, labour-hours cards, estimate/quote/actual comparison, quote accuracy; invoice list with create dialog (modes per pricing methodology) and delete. All money formatted from server values — no client recomputation. All v1 automation ids preserved. apiErrorMessage now also reads the document endpoints' error key so calc/configuration guidance reaches the user.

Spec: job-xero-invoice.spec.ts ports near-verbatim, with one deliberate deviation — it creates its own job instead of using sharedEditJobUrl, because it fully invoices the job and the shared fixture is read-only by contract (v1 mutated it and survived on run ordering).

Adversarial review (2 subagents) — notable catches, all fixed

• Provider-driven HTTP statuses (429/503 on live-Xero failures, 401 mid-request revocation) weren't in the declared response maps — ninja would raise ConfigError instead of returning the error payload. Statuses now clamp to the declared map; the cause stays in error (ADR 0038).
• The PO zero-UUID sentinel could be stored when the unpaginated recovery listing missed the PO (>100 POs) — guard added on write, recovery listing paged.
• TanStack v5 refetch() never rejects, so the "list could not be refreshed" toast was unreachable; an invoices load error rendered as the "No invoices" empty state.
• Known, deliberately not touched (v1-parity): calculate_invoice_amount doesn't quantize, so a sub-cent basis total (e.g. 157.985) can never read fully-invoiced. Real jobs have 2dp values; flagged for 2c or later rather than changing the money path mid-slice.

Verified

• ./scripts/ops/run_e2e.sh: 38 passed — job-xero-invoice green under XERO_READONLY (13/40 specs), all prior specs green with the sync beat live.
• Backend: 1617 unit tests green; new coverage: PO manager 12, invoice manager 7 (error contract / raw_json / readonly fabrication), finish summary 11, finish API 7. Frontend: JobFinishTab component tests 3.
• Expensive tier clean (mypy strict zero-baseline, import-linter, find-duplicates, deptry, schema-current, status table).
• Parity ledger: renamed: entries for all four document operation ids; two new behaviour entries in docs/accepted-api-differences.yml.

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features
  • Added a Finish Job workspace with financial summaries, invoice balances, cost comparisons, completion checklists, and variance indicators.
  • Added invoice listing and management, including full, percentage-based, cost-to-date, and custom-amount invoicing.
  • Added Xero invoice and purchase-order creation, updating, deletion, and invoice file attachments.
  • Added checklist updates with validation and activity history.
• Bug Fixes
  • Invoice changes now immediately recalculate the job’s fully invoiced status and remaining balance.
  • Improved error messages for accounting and Xero operations.

[coderabbitai]

Warning

Review limit reached

@corrin, you've reached your PR review limit, so we couldn't start this review.

Next review available in:24 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 052efda7-ef2c-4202-adef-941ebddc6980

📥 Commits

Reviewing files that changed from the base of the PR and between b318f2e and 09176e1.

📒 Files selected for processing (7)

• apps/xero/documents/invoice.py
• apps/xero/tests/test_invoice_manager.py
• apps/xero/tests/test_provider_documents.py
• config/tests/test_contract_gates.py
• docs/code-quality.md
• docs/rewrite-status.md
• scripts/checks/status_table.py

📝 Walkthrough

Walkthrough

This PR adds Finish Job financial and checklist APIs, Xero invoice and purchase-order synchronization, provider contracts, read-only behavior, and frontend Finish Job and invoice-management components with unit, API, and end-to-end tests.

Changes

Finish Job and Xero workflows

Layer / File(s)	Summary
Finish Job calculations and APIs apps/accounting/..., apps/job/..., frontend/schema.v2.yml	Adds financial summaries, checklist validation and updates, invoice listing, conditional responses, and Finish Job API contracts.
Accounting contracts and Xero provider operations apps/accounting/types.py, apps/accounting/provider.py, apps/xero/helpers.py, apps/xero/provider.py, apps/xero/readonly_provider.py, stubs/xero_python/...	Adds provider-agnostic document DTOs, Xero invoice and purchase-order operations, payload and error helpers, read-only results, and SDK stubs.
Xero document managers and endpoints apps/xero/documents/..., apps/xero/api.py, apps/xero/tests/...	Adds invoice and purchase-order validation, payload construction, persistence, deletion, authenticated endpoints, and manager tests.
Finish Job workspace and invoice UI frontend/src/features/job/..., frontend/src/api/..., frontend/src/lib/format.ts, frontend/tests/e2e/...	Adds lazy-loaded Finish Job and invoice components with financial displays, checklist updates, invoice actions, error handling, and end-to-end coverage.
Compatibility records and progress documentation docs/accepted-api-differences.yml, docs/code-quality.md, docs/rewrite-status.md, scripts/v1-frontend-operations.yml	Records API behavior changes, updated quality metrics, operation rename mappings, and rewrite progress.

Estimated code review effort: 5 (Critical) | ~120 minutes

Sequence Diagram(s)

sequenceDiagram
participant JobFinishTab
participant JobAPI
participant FinishJobSummary
participant JobService
JobFinishTab->>JobAPI: GET finish summary and checklist
JobAPI->>FinishJobSummary: calculate financial totals
FinishJobSummary-->>JobAPI: return summary
JobAPI-->>JobFinishTab: return JobFinishResponse
JobFinishTab->>JobAPI: PATCH checklist fields
JobAPI->>JobService: update checklist
JobService-->>JobAPI: return updated job

Loading

sequenceDiagram
participant JobInvoiceCard
participant XeroAPI
participant XeroInvoiceManager
participant XeroAccountingProvider
JobInvoiceCard->>XeroAPI: submit invoice request
XeroAPI->>XeroInvoiceManager: create document
XeroInvoiceManager->>XeroAccountingProvider: create InvoicePayload
XeroAccountingProvider-->>XeroInvoiceManager: return DocumentResult
XeroInvoiceManager-->>XeroAPI: return persisted invoice result
XeroAPI-->>JobInvoiceCard: return success or error

Loading

Possibly related PRs

• corrin/docketworks_v2#45: Extends the accounting provider and Xero abstractions used by this PR.
• corrin/docketworks_v2#18: Provides purchase-order models and status behavior used by Xero purchase-order synchronization.
• corrin/docketworks_v2#10: Provides the workshop PDF generation used by invoice attachment handling.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 44.10% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the main Xero invoice document push and Finish Job changes, with a useful E2E status indicator.
Description check	✅ Passed	The description is detailed, structured, and covers scope, behavior, verification results, known limitations, and parity changes.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch xero/invoice-path

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 14

🧹 Nitpick comments (5)

frontend/src/api/index.ts (1)

81-81: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Remove code-narration comments or document their constraint.

Both comments describe nearby code without stating the rejected alternative and the factual constraint.

• frontend/src/api/index.ts#L81-L81: Remove the export-group narration, or explain why feature code must import these generated operations through frontend/src/api/index.ts.
• frontend/src/features/job/JobFinishTab.tsx#L157-L157: Remove the ticket-heading comment. It does not document a runtime or business constraint.

As per coding guidelines, “Comments must document the rejected obvious alternative and the factual constraint that rejected it; remove code narration and review-feedback echoes.”

🤖 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 `@frontend/src/api/index.ts` at line 81, Remove the code-narration comments at
frontend/src/api/index.ts:81-81 and
frontend/src/features/job/JobFinishTab.tsx:157-157; no replacement documentation
is needed because neither comment states a factual constraint or rejected
alternative.

Source: Coding guidelines

apps/xero/tests/test_invoice_manager.py (1)

86-138: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a delete happy-path test.

TestErrorContract covers the three failure paths of delete_document. No test covers the success path, so nothing asserts that the local Invoice row is removed, that recalculate_job_invoicing_state runs, and that the invoice_deleted job event records the invoice number. That recalculation is the same same-request effect the create test protects at Line 256-258, and the Finish Job tab depends on it.

Do you want me to write this test?

🤖 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/xero/tests/test_invoice_manager.py` around lines 86 - 138, Add a
success-case test in TestErrorContract for delete_document using a configured
provider and Xero invoice ID, then assert the local Invoice is deleted,
recalculate_job_invoicing_state is called, and the invoice_deleted job event
records the invoice number. Keep the test focused on the same-request effects of
the happy path.

apps/xero/tests/test_po_manager.py (1)

98-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Cover the write direction of the zero-UUID sentinel too.

This test covers the read direction: a stored sentinel routes to create. The module docstring at Line 4-5 names "storing Xero's zero-UUID sentinel as a real id" as the expensive silent failure, and that is the write direction, guarded by _save_po_with_xero_data at apps/xero/documents/po.py Line 151.

Add a test where the provider returns DocumentResult(success=True, external_id=ZERO_UUID) and assert that po.xero_id stays None after sync_to_xero. This also pins the behavior discussed on apps/xero/provider.py Line 280-314.

Do you want me to write this test?

🤖 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/xero/tests/test_po_manager.py` around lines 98 - 109, Extend
test_zero_uuid_is_treated_as_unsynced to cover the write path: configure the
provider’s successful DocumentResult with external_id=ZERO_UUID, run
manager.sync_to_xero(), then refresh or inspect po.xero_id and assert it remains
None. Preserve the existing create-path assertion while exercising
_save_po_with_xero_data’s zero-UUID handling.

apps/xero/api.py (2)

317-328: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Type the clamp parameter as the contract, not as object.

XeroDocumentResponse declares status. Annotating the parameter object discards that contract and forces a runtime comparison against an untyped value. Declare int | None so mypy checks every call site.

♻️ Proposed signature change

-def _document_error_status(status: object) -> int:+def _document_error_status(status: int | None) -> int:

As per coding guidelines: "Treat type annotations as data contracts: use named types such as dataclasses, TypedDicts, or Protocols for complex inline types."

🤖 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/xero/api.py` around lines 317 - 328, Update the _document_error_status
parameter annotation from object to int | None, matching the status contract
declared by XeroDocumentResponse. Keep the existing 404 handling and 400
fallback unchanged, and let mypy validate all call sites against the narrowed
type.

Source: Coding guidelines

---

358-366: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract the repeated Xero-auth 401 block.

The same get_valid_token() guard and XeroAuthRequiredOut payload appear in all four new endpoints (lines 358-366, 458-466, 543-551, 612-620). The two purchase-order endpoints also repeat the identical lookup and supplier guard. One helper per repeated block keeps the message and status in step.

♻️ Proposed helper

def_xero_auth_required() ->Status[XeroAuthRequiredOut]:
"""Build the one 401 body every Xero document endpoint returns."""returnStatus(
401,
XeroAuthRequiredOut(
success=False,
redirect_to_auth=True,
message="Your Xero session has expired. Please log in again.",
),
)

As per coding guidelines: "Use one implementation per concept; search before implementing and extend a near-match instead of creating a parallel implementation."

🤖 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/xero/api.py` around lines 358 - 366, Extract the repeated
get_valid_token() failure response into a shared _xero_auth_required() helper
returning the existing 401 Status[XeroAuthRequiredOut] payload, then replace the
guards in all four Xero document endpoints with that helper. Also extract and
reuse a helper for the identical purchase-order lookup and supplier guard,
preserving the current validation behavior and messages.

Source: Coding guidelines

🤖 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 `@apps/job/tests/test_finish_api.py`:
- Around line 105-115: Correct the inline comment in test_lists_job_invoices to
state that the second invoice belongs to the same company but has no job, so it
must not appear in the job-scoped results. Do not change the test setup or
assertions.
In `@apps/xero/api.py`:
- Around line 64-72: Consolidate the duplicated `_staff` helpers by moving the
implementation from `apps/job/api.py` into `apps/core` and importing that shared
helper in `apps/xero/api.py`, `apps/purchasing/api.py`, and the applicable
callers. Remove redundant local definitions, while preserving any
Timesheet-specific narrowing behavior only if it cannot use the shared
implementation.
- Around line 402-405: Update the invoice creation flow around
XeroInvoiceManager.create_document to make retries safe at the Xero API
boundary: either supply a stable idempotency key for the same job/document
across retry attempts, or disable/prohibit RateLimitedRESTClient retries for
this non-idempotent POST. Preserve retries for safe operations and ensure one
logical create cannot produce duplicate Xero documents.
In `@apps/xero/documents/base.py`:
- Around line 82-99: Move the document-type validation before the best-effort
exception handler in both _add_xero_history_note (apps/xero/documents/base.py,
lines 82-99) and _add_history_note (apps/xero/provider.py, lines 407-421):
reject values outside ("invoice", "quote") before entering try, while leaving
provider calls inside the existing handler so only provider failures are
absorbed.
In `@apps/xero/documents/invoice.py`:
- Around line 232-241: Update create_document to validate that billing_metadata
contains target_total, prior_invoiced_total, and calculated_amount before any
direct reads. After validation, access all three keys directly and remove the
"0" defaults from the Decimal conversions so missing contract fields fail rather
than producing an uncomputed remaining_to_invoice value.
- Around line 198-216: Update the invoice creation flow after the existing
result.external_id and result.number validation to validate that raw contains
_sub_total, _total_tax, _total, and _amount_due before constructing Invoice.
Fail early using the existing validation pattern when any key is missing, then
read the validated values without zero fallbacks so invoice totals cannot be
silently stored as 0.00.
In `@apps/xero/documents/po.py`:
- Around line 23-25: Move ZERO_UUID to a shared module, remove the duplicate
definitions from documents/po.py and XeroAccountingProvider, and import the
shared constant in both consumers. Preserve the existing sentinel value and all
checks that use it.
- Around line 55-59: The Xero sync validation path must enforce the draft-only
rule for first-time creation. Update validate_for_xero_sync() to invoke
state_valid_for_xero() before allowing creation, while preserving updates for
POs with an existing Xero ID; keep the implementation and docstring of
state_valid_for_xero() consistent with this behavior.
In `@apps/xero/provider.py`:
- Around line 227-237: The invoice pre-read in the deletion flow must validate
that the API response contains an invoice and that its contact is present before
constructing the DELETED Invoice. Mirror the guards and user-facing error
messages used by delete_purchase_order, referencing the existing
invoice-deletion method and its existing/api/contact symbols while preserving
the normal construction path for valid data.
- Around line 280-314: Update the zero-UUID recovery flow around
_find_po_by_number so that when recovery returns None and po_id remains
ZERO_UUID, return a failed DocumentResult with no external_id instead of
continuing to the success path. Preserve the existing validation-error handling
and only construct the online_url and successful result after a real purchase
order ID is available.
- Around line 316-335: Update _find_po_by_number to enforce a finite maximum
page count while scanning purchase orders. Stop paging and return None once the
limit is reached, while preserving the existing match return and empty-page
termination behavior.
In `@frontend/src/features/job/JobInvoiceCard.tsx`:
- Around line 344-359: Give both numeric inputs in JobInvoiceCard accessible
names: add a visible label or aria-label to the percentage input at
frontend/src/features/job/JobInvoiceCard.tsx:344-359 identifying it as “Invoice
percentage of quote,” and to the amount input at
frontend/src/features/job/JobInvoiceCard.tsx:396-410 identifying it as “Invoice
amount.”
- Around line 233-239: Update the invoice link handler’s window.open call to
pass noopener,noreferrer in its third argument when opening invoice.online_url,
while preserving the existing URL check and toast behavior.
In `@frontend/src/lib/format.ts`:
- Around line 26-34: Update the NZ_DATE formatter used by formatDate to set
timeZone to Pacific/Auckland, ensuring invoice dates render consistently across
environments. Revise the formatter comment to state that per-page formatters are
rejected because invoice dates must display identical text across the workspace.
---
Nitpick comments:
In `@apps/xero/api.py`:
- Around line 317-328: Update the _document_error_status parameter annotation
from object to int | None, matching the status contract declared by
XeroDocumentResponse. Keep the existing 404 handling and 400 fallback unchanged,
and let mypy validate all call sites against the narrowed type.
- Around line 358-366: Extract the repeated get_valid_token() failure response
into a shared _xero_auth_required() helper returning the existing 401
Status[XeroAuthRequiredOut] payload, then replace the guards in all four Xero
document endpoints with that helper. Also extract and reuse a helper for the
identical purchase-order lookup and supplier guard, preserving the current
validation behavior and messages.
In `@apps/xero/tests/test_invoice_manager.py`:
- Around line 86-138: Add a success-case test in TestErrorContract for
delete_document using a configured provider and Xero invoice ID, then assert the
local Invoice is deleted, recalculate_job_invoicing_state is called, and the
invoice_deleted job event records the invoice number. Keep the test focused on
the same-request effects of the happy path.
In `@apps/xero/tests/test_po_manager.py`:
- Around line 98-109: Extend test_zero_uuid_is_treated_as_unsynced to cover the
write path: configure the provider’s successful DocumentResult with
external_id=ZERO_UUID, run manager.sync_to_xero(), then refresh or inspect
po.xero_id and assert it remains None. Preserve the existing create-path
assertion while exercising _save_po_with_xero_data’s zero-UUID handling.
In `@frontend/src/api/index.ts`:
- Line 81: Remove the code-narration comments at frontend/src/api/index.ts:81-81
and frontend/src/features/job/JobFinishTab.tsx:157-157; no replacement
documentation is needed because neither comment states a factual constraint or
rejected alternative.

🪄 Autofix

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: ea55edb0-3b70-4970-ae0f-3dafb7b206f3

📥 Commits

Reviewing files that changed from the base of the PR and between b5feeab and d1eea03.

⛔ Files ignored due to path filters (5)

• frontend/src/api/generated/@tanstack/react-query.gen.ts is excluded by !**/generated/**
• frontend/src/api/generated/index.ts is excluded by !**/generated/**
• frontend/src/api/generated/sdk.gen.ts is excluded by !**/generated/**
• frontend/src/api/generated/types.gen.ts is excluded by !**/generated/**
• frontend/src/api/generated/zod.gen.ts is excluded by !**/generated/**

📒 Files selected for processing (32)

• apps/accounting/provider.py
• apps/accounting/services/finish_job_summary.py
• apps/accounting/tests/test_finish_job_summary.py
• apps/accounting/types.py
• apps/company/tests/job_fixtures.py
• apps/job/api.py
• apps/job/schemas.py
• apps/job/services/job_service.py
• apps/job/tests/test_finish_api.py
• apps/xero/api.py
• apps/xero/documents/base.py
• apps/xero/documents/invoice.py
• apps/xero/documents/po.py
• apps/xero/helpers.py
• apps/xero/provider.py
• apps/xero/readonly_provider.py
• apps/xero/tests/test_invoice_manager.py
• apps/xero/tests/test_po_manager.py
• docs/accepted-api-differences.yml
• docs/code-quality.md
• docs/rewrite-status.md
• frontend/schema.v2.yml
• frontend/src/api/error-message.ts
• frontend/src/api/index.ts
• frontend/src/features/job/JobDetailPage.tsx
• frontend/src/features/job/JobFinishTab.test.tsx
• frontend/src/features/job/JobFinishTab.tsx
• frontend/src/features/job/JobInvoiceCard.tsx
• frontend/src/lib/format.ts
• frontend/tests/e2e/job/job-xero-invoice.spec.ts
• scripts/v1-frontend-operations.yml
• stubs/xero_python/accounting/__init__.pyi

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

docs/code-quality.md (1)

24-34: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Regenerate the suppression total.

Line 24 reports 431, but the visible rows sum to 430: fixed suppressions total 13 and rule-coded suppressions total 417. scripts/checks/code_quality.py derives the total from the counted entries, so this report is internally inconsistent.

Regenerate the file, then run uv run python -m scripts.checks.code_quality --check.

Expected table correction

-| TOTAL suppressions | 431 |+| TOTAL suppressions | 430 |

The canonical metric generator in scripts/checks/code_quality.py is the source of truth.

🤖 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/code-quality.md` around lines 24 - 34, Regenerate the suppression table
in docs/code-quality.md using scripts/checks/code_quality.py so TOTAL
suppressions matches the visible counted entries (430). Then run uv run python
-m scripts.checks.code_quality --check and ensure validation passes.

♻️ Duplicate comments (1)

apps/xero/documents/invoice.py (1)

246-251: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Validate billing_metadata before invoice creation.

Lines 249-251 parse required values after provider.create_invoice() and Invoice.objects.create(). If a required key is missing or non-decimal, the request fails after the financial operation has completed.

Parse and validate these fields at the start of create_document, before provider submission. Reuse the validated values when creating the audit event.

As per coding guidelines: “Fail early: check invalid cases first, validate required inputs up front.”

🤖 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/xero/documents/invoice.py` around lines 246 - 251, Update
create_document to parse and validate billing_metadata["target_total"],
billing_metadata["prior_invoiced_total"], and
billing_metadata["calculated_amount"] before calling provider.create_invoice()
or Invoice.objects.create(). Reuse these validated Decimal values when
constructing the audit event, removing the later direct parsing while preserving
the existing required-key and invalid-decimal failures.

Source: Coding guidelines

🧹 Nitpick comments (1)

apps/xero/tests/test_provider_documents.py (1)

216-236: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert the recovery pagination request.

_find_po_by_number iterates get_purchase_orders(tenant_id, page=page), but this test only asserts two calls. A regression that requests page one twice can still pass because the mocked side_effect returns a different response each time.

Assert the page argument for both get_purchase_orders calls.

🤖 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/xero/tests/test_provider_documents.py` around lines 216 - 236, Update
test_zero_uuid_recovers_real_id_across_pages to inspect
api.get_purchase_orders.call_args_list and assert the two calls use page=1 and
page=2 respectively, while preserving the existing tenant_id assertions and
recovery expectations.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@docs/code-quality.md`:
- Around line 24-34: Regenerate the suppression table in docs/code-quality.md
using scripts/checks/code_quality.py so TOTAL suppressions matches the visible
counted entries (430). Then run uv run python -m scripts.checks.code_quality
--check and ensure validation passes.
---
Duplicate comments:
In `@apps/xero/documents/invoice.py`:
- Around line 246-251: Update create_document to parse and validate
billing_metadata["target_total"], billing_metadata["prior_invoiced_total"], and
billing_metadata["calculated_amount"] before calling provider.create_invoice()
or Invoice.objects.create(). Reuse these validated Decimal values when
constructing the audit event, removing the later direct parsing while preserving
the existing required-key and invalid-decimal failures.
---
Nitpick comments:
In `@apps/xero/tests/test_provider_documents.py`:
- Around line 216-236: Update test_zero_uuid_recovers_real_id_across_pages to
inspect api.get_purchase_orders.call_args_list and assert the two calls use
page=1 and page=2 respectively, while preserving the existing tenant_id
assertions and recovery expectations.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 10f8aa42-f281-4b7d-8f3f-3e62a97f3bec

📥 Commits

Reviewing files that changed from the base of the PR and between d1eea03 and b318f2e.

📒 Files selected for processing (14)

• apps/job/tests/test_finish_api.py
• apps/xero/api.py
• apps/xero/constants.py
• apps/xero/documents/base.py
• apps/xero/documents/invoice.py
• apps/xero/documents/po.py
• apps/xero/provider.py
• apps/xero/tests/test_document_api.py
• apps/xero/tests/test_po_manager.py
• apps/xero/tests/test_provider_documents.py
• docs/code-quality.md
• docs/rewrite-status.md
• frontend/src/features/job/JobInvoiceCard.tsx
• frontend/src/lib/format.ts

🚧 Files skipped from review as they are similar to previous changes (9)

• frontend/src/lib/format.ts
• apps/job/tests/test_finish_api.py
• apps/xero/documents/base.py
• apps/xero/tests/test_po_manager.py
• docs/rewrite-status.md
• apps/xero/provider.py
• apps/xero/api.py
• frontend/src/features/job/JobInvoiceCard.tsx
• apps/xero/documents/po.py

[corrin]

Round-2 items, addressed in 2fb241c:

• billing_metadata validation ordering — fixed: the three Decimals now parse at the top of create_document, before anything reaches the provider, and the audit event reuses the validated values.
• page assertions in the zero-UUID recovery test — fixed: the test now asserts page=1, page=2 from call_args_list.
• docs/code-quality.md TOTAL 'mismatch' — rebutting: the file is generated and uv run python -m scripts.checks.code_quality --check passes against it, so it is exactly what the generator produces; the TOTAL row includes suppression categories beyond the per-code breakdown rows by design. Hand-editing a generated file is what the gate exists to prevent.

[corrin]

Verification upgraded per the corrected test regime: XERO_READONLY is only for dev sessions pointed at production (hotfixes) — testing runs with writes live. Re-ran the full suite with XERO_READONLY=false against the demo tenant: 38 passed, with job-xero-invoice exercising the real provider path end-to-end (actual create_invoices, workshop-PDF attachment, history note — 9.7s vs 5.3s under readonly, the difference being the real Xero round-trips). Local .env flipped to XERO_READONLY=false as the standing dev configuration; rewrite-status records the regime in 46a06d3.