fix(writeback): publish ledger entries to QuickBooks one entry at a time

[jfrench9]

Summary

QuickBooks write-back tracked QuickBooks ids per event, so an event's entries were published together or not at all. That caused three defects on qb_authoritative / hybrid graphs:

• Out-of-period publish. Close published every entry linked to an event, whatever its period. A January close could publish and post a February entry.
• Duplicates after a partial failure. When QuickBooks rejected entry k of a multi-entry event, the ids of entries 0..k−1 were lost. A retry after QB's ~5-minute RequestId window posted them again, and the next sync imported the orphans.
• Auto-reversals never reached QuickBooks. A schedule's reversal entry carried no event link, so close never selected it. QuickBooks got each month's accrual and never its reversal.

Changes

• Per-entry tracking. Ids are recorded as entry id → QB id in metadata.qb_entry_ids. qb_external_id is still written, comma-joined, for the cross-source matcher in the loader. Each POST's RequestId is the entry id.
• Close is scoped to its period. Close passes entry_ids for the entries in the period it is closing. The event becomes fulfilled once no draft of it remains, so an entry in a later period publishes when that period closes.
• Validate before posting. Every entry is built (accounts resolved, amounts validated) before the first POST. A mapping error posts nothing.
• Partial failure keeps what landed. On a partial failure, the ids that landed are recorded and those entries are posted. The rest stay draft.
• Reversals are linked. schedule_entry_due links the reversal entry to its event.
• execute-event-block applies close's source rule. Synced-in QuickBooks events are never sent back.
• Events with no ledger rows are refused. An event with no drafted ledger rows now gets a 409 instead of publishing its captured metadata. Previously that put an entry in QuickBooks the ledger never held.
• No illegal pending. A failed publish moves the event to pending only where that transition is legal. captured → pending used to strand the event.
• Re-OAuth can't switch company. Re-authorizing a QuickBooks connection against a different company returns 409. A soft-deleted row is revived only while the current connection is still pending_oauth.

Compatibility

• Events published before this change carry qb_external_id without qb_entry_ids. They are treated as fully published, exactly as before.
• Reversal entries drafted before this change are still unlinked. They keep posting locally at close but won't publish to QuickBooks. That existing data needs a one-time backfill that links each reversal to its original's event.

Tests

• Real-Postgres tests:
  • an event with August and September entries publishes one per close;
  • the eligibility predicate is per entry;
  • a mid-batch rejection reports what landed;
  • a mapping error posts nothing;
  • entry_ids scoping.
• Unit tests cover:
  • the source rule;
  • refusing an event without rows;
  • status handling on failure;
  • reversal linking;
  • the re-OAuth realm check.
• just test-all passes.

🤖 Generated with Claude Code

[github-actions]

Change classification: normal — platform or application code changed.

A normal change carries a documented automated review on this pull request before merge, in place of a second human approver. The change-classification job requests that review and passes once it has been posted. Recorded by the change-classification job.

[jfrench9]

@claude please review this PR

[claude]

Claude finished @jfrench9's task in 3m 50s —— View job

---

Review

• Gather context (PR description, diff, changed files)
• Review per-entry QB id tracking (event_block/commands.py, qb_writeback.py)
• Review close-service period scoping
• Review reversal linking (schedule_entry_due.py)
• Review re-OAuth company-switch guard (routers/graphs/connections/oauth.py)
• Review test coverage
• Post final review

Note: the sandbox's network fetch was unavailable, so I read the changed files directly on the checked-out branch rather than diffing against origin/main.

Summary

This is a well-scoped, carefully-reasoned fix. The per-entry tracking model (metadata.qb_entry_ids, entry id as RequestId), the "build everything before the first POST" validation order, the savepoint-per-entry isolation in _publish_drafts_to_qb, and the reversal-linking fix all match the description and are backed by real-Postgres tests that exercise the actual failure modes (mid-batch rejection, mapping error, period scoping). The legacy-compat handling (qb_external_id without qb_entry_ids ⇒ treated as fully published) is documented and consistently applied everywhere it matters (commands.py:1133, fiscal_calendar/qb_writeback.py:124-135).

Two things worth a look, neither blocking:

1. ExecuteEventBlockResponse.qb_external_id can silently drop ids on a multi-entry publish — robosystems/operations/event_block/commands.py:1338-1344

return ExecuteEventBlockResponse(
    event_id=str(event.id),
    status=str(event.status),
    qb_external_id=next(iter(newly_published.values()), None)
    or next(iter(published.values()), None),
    qb_error=None,
)

ExecuteEventBlockRequest has no entry_ids field — only close_service._publish_drafts_to_qb passes it (scoped to one entry). The REST/MCP execute-event-block operation calls this with entry_ids=None, which publishes every outstanding draft on the event in one call. Now that a single event can legitimately carry more than one entry (the reversal-linking fix in schedule_entry_due.py is a direct example — accrual + reversal on the same event), a direct/manual publish call posting both in one shot returns only one arbitrary QB id in qb_external_id, even though event.metadata.qb_entry_ids correctly has both. Not a data-integrity issue (the DB record is right), but callers reading the response instead of re-fetching the event will undercount. Worth either exposing the per-entry map on the response or documenting that qb_entry_ids is the source of truth for multi-entry publishes.

Fix this →

2. QBClient is reconstructed (and re-authenticates) per entry in the close pre-publish loop — robosystems/operations/roboledger/fiscal_calendar/close_service.py:625-676 calls execute_event_block once per (entry, event) pair, and execute_event_block unconditionally builds a fresh QBClient (robosystems/operations/event_block/commands.py:1250), whose __init__ calls auth_client.refresh() and persists rotated tokens on every construction (robosystems/adapters/quickbooks/client/api.py:145,187). For an event with more than one draft entry landing in the same closing period, that's N token refreshes + N credential writes to the platform DB for what is logically one QB session. Today this is mostly latent (RL-originated multi-entry-in-one-period events are rare — the reversal is normally in the next period), but it's a real cost multiplier if that changes, and it compounds the "large batch holds the transaction open" warning already logged a few lines up. Not asking for a fix in this PR, just flagging for a follow-up: hoist connection/credential resolution and QBClient construction out of the per-entry loop.

Everything else checked out:

• _draft_entries / post_event_to_qb: builds all entries before posting (confirmed a build-time QBWritebackError carries published={} via the exception's own __init__, so "a mapping error posts nothing" holds even though the build happens outside the try/except).
• _entry_not_yet_in_qb predicate (fiscal_calendar/qb_writeback.py:124) correctly falls through to "already published" for legacy events and to the per-entry has_key check for new ones — verified against Entry.id's column type (String), so the JSONB has_key comparison is valid.
• _publish_drafts_to_qb's per-entry session.begin_nested() savepoint is the right fix for the "one bad entry poisons the whole close transaction" failure mode described in its own comment, and the session.commit() after the loop (durability boundary for qb_external_id markers) is placed correctly before WritebackFailed is raised.
• schedule_entry_due.py: both the primary entry and its reversal get triggered_by_event_id set in one UPDATE ... WHERE id IN (...), which is what makes the reversal visible to writeback_source_clause()/close.
• OAuth re-auth guard: the realm-mismatch 409 check runs strictly before store_tokens, so no partial state changes on a rejected company switch; the soft-delete revival path is correctly gated on is_pending (computed from the connection state read before the 409 check, not after any mutation) and orders prior.restore() before pending.delete() so a crash mid-revival never leaves both connections gone.
• _assert_retractable's qb_external_id check blocking retraction is conservative but correct — it can't distinguish "fully published" from "one entry of several published," and refusing retraction either way is the safe answer.

Test coverage matches the claims in the PR description (period-scoped publish, per-entry eligibility, mid-batch rejection, mapping-error-posts-nothing, reversal linking, re-OAuth realm check) and the new test_execute_per_entry_db.py / test_qb_writeback_payload.py cases exercise real Postgres rather than mocking the query layer, which is the right call given how much of this fix is about SQL predicates.