Add fiscal calendar, closing cadence, and close demo for RoboLedger#613
Conversation
- Introduced a comprehensive AI month-end close demo, including synthetic data generation for a consulting firm. - Added new files for demo setup, including data generation, accounting policies, and OLTP data writing. - Implemented a close workflow with a rolling 16-month window, ensuring evergreen data for testing. - Updated the .env.local.example to enable extensions and added relevant commands in the justfile for running the demo. - Created a README to guide users through the demo setup and execution process.
|
@claude please review this PR |
|
Claude finished @jfrench9's task in 3m 43s —— View job PR Review: Add fiscal calendar, closing cadence, and close demo for RoboLedger
This is a substantial, well-structured PR. The core domain design — rolling Critical: Migration breaking change needs a reconciliation pathThe PR deletes # On any env that has already run 0002:
alembic stamp 0001 # mark the DB as being at 0001Without this, deploying to staging will fail with Alembic seeing an unknown head. Consider adding the runbook to the PR description or a Issues1. Close-period logic is fully duplicated between REST and MCP ( The draft-to-posted transition, BS equation check, re-close routing, and 2. session.rollback()
return {"error": "unbalanced", ...} # ← returns normally, no exception raisedAfter 3. Private method
4. Closeable gate blocks closing on the last day of the month ( if period_end >= today: # ← strict >=
blockers.append(PERIOD_INCOMPLETE)This means January cannot be closed on January 31 — you must wait until February 1. Most accounting systems allow closing on the last day. If intentional (ensuring all intraday transactions are in), add a comment explaining the policy. If not, 5. Manual period parsing in MCP year, month = int(period[:4]), int(period[5:7])
period_start = date(year, month, 1)
period_end = date(year, month, monthrange(year, month)[1])
6. fp.closed_by = f"mcp:{graph_id}" # ← graph_id is not a user identityThe REST endpoint correctly uses 7. When 8. Two identical private functions. Extract to a shared helper module or Observations (no change required)
SummaryThe fiscal calendar design and implementation are solid — sequential enforcement, the audit trail, and the closeable gate logic are well-thought-out. The main concern before merging is items 1 (migration runbook for staging) and 2 (the |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
- Introduced PeriodCloseService to encapsulate the month-end close workflow, improving maintainability and reducing duplication across REST and MCP paths. - Updated ClosePeriodTool to leverage the new service, ensuring consistent error handling and user identity management. - Enhanced error handling for close operations, including specific exceptions for close gate failures, unbalanced ledgers, and missing fiscal periods. - Improved API responses for closing operations, providing clearer feedback on errors and outcomes. - Added unit tests for the new service and tool to ensure robust functionality and error handling.
…edger namespace - Updated imports across multiple files to transition from the `fiscal_calendar` and `schedules` modules to the new `roboledger` namespace, enhancing organization and clarity. - Introduced new modules for fiscal calendar and schedule operations, encapsulating related functionalities for better maintainability. - Ensured consistent error handling and improved code structure by centralizing related services and utilities under the `roboledger` directory. - Added documentation for new modules to clarify their purpose and usage within the system.
|
@claude can you review the latest 2 commits |
|
Claude finished @jfrench9's task in 3m 59s —— View job Review: Latest 2 Commits
These two commits are a focused, high-quality response to the prior review. Here's what they accomplish and what's left. Commit Commit Issues Addressed from Prior Review
Remaining Open IssuesIssue #4 — closeable gate still blocks on last day of month ( Critical migration runbook — still just described in the PR description, no concrete script. New Issues in These Commits1.
# __init__.py imports from close_service but NOT FiscalCalendarError:
from .close_service import (
CloseGateFailed, PeriodCloseError, PeriodCloseResult,
PeriodCloseService, PeriodNotFoundError, UnbalancedLedgerError,
)All callers currently work around this by importing from 2.
actor_id = getattr(self.client, "user_id", None) or f"mcp:{graph_id}"But actor_id=f"mcp:{graph_id}", # line 453 — graph_id is not a user identityThese two tools are now inconsistent. Fix this → Observations
SummaryThe two commits fully resolve the structural concerns from the prior review. The |
- Added detailed API endpoint documentation for local testing, clarifying usage and common mistakes. - Updated the cash flow handling in various modules to reflect that the cash flow statement is not yet supported, ensuring consistent messaging across the codebase. - Removed references to cash flow elements in taxonomy and tests, aligning with the current implementation status. - Improved error handling in financial statement tools to provide clear feedback when unsupported statement types are requested.
|
@claude can you review the latest changes |
|
Claude finished @jfrench9's task in 3m 55s —— View job Review: Latest Commit (
|
| # | Issue | Status |
|---|---|---|
| 2 (new) | ReopenPeriodTool.actor_id inconsistent with ClosePeriodTool |
✅ Fixed — getattr(self.client, "user_id", None) or f"mcp:{graph_id}" applied consistently |
| 1 (new) | FiscalCalendarError missing from package __init__.py |
❌ Still unresolved |
Notable Changes
_build_rows bug fix (fact_grid.py:881) — the old code emitted values=[0.0] * n_periods for subtotal/abstract rows. Since Pass 1 already computes rolled-up sums for parent nodes in computed_per_period, subtotal rows now read from there just like leaf nodes. This was a real rendering bug — users would see section headers with no value. The fix is correct and the unified emit path is cleaner.
Aspirational calendar detection (fact_grid.py:138) — _ledger_has_re_postings detects the demo case where a fiscal calendar has a closed_through marker but no actual RE postings backing it. When detected, closed_through is cleared so the synthetic close runs from inception and the balance sheet balances. The concept is sound and the comment explains it well.
Cash flow deferral — Removing the partially-implemented CF generator is pragmatic: seed data removed from taxonomy/seed.py, guard rail removed from guard_rails.py, source="system" correctly excludes taxonomy-root elements from COA queries, and CreateStructureRequest drops cash_flow_statement from its Literal. Comments throughout explain the deferral consistently.
Closing book order — "Period Close" moved to first position. Good UX.
Issues
1. Redundant query in generate_report_facts (fact_grid.py:127-139)
if closed_through is None:
closed_through = _derive_closed_through_from_ledger(session, mapping_id) # reads MAX(posting_date)
if closed_through is not None and not _ledger_has_re_postings(session, mapping_id): # runs LIMIT 1 query
closed_through = NoneWhen closed_through starts as None and _derive_closed_through_from_ledger returns a date, the second if fires and calls _ledger_has_re_postings — but _derive_closed_through_from_ledger already proved postings exist (it found a MAX date). _ledger_has_re_postings will always return True here; the extra query is wasted. The intent is to only nullify a calendar-provided closed_through that has no RE backing. The fix is elif:
if closed_through is None:
closed_through = _derive_closed_through_from_ledger(session, mapping_id)
elif not _ledger_has_re_postings(session, mapping_id):
closed_through = NoneThis matches the documented intent exactly: the derived path is always backed; only the externally-provided path needs the liveness check. Fix this →
2. _ = _validate_cash_flow is the wrong dead-code pattern (guard_rails.py:130)
_ = _validate_cash_flow # Keep alive for the linterAssigning to _ is the Python convention for "I'm intentionally discarding a value" (e.g., loop counters), not for pinning a function reference. It also pollutes the module namespace with an unexpected name. If the linter (basedpyright) flags it, the right suppression is inline on the function declaration:
def _validate_cash_flow(rows: list[FactRow]) -> ValidationResult: # pyright: ignore[reportUnusedFunction]Or, since this function is being kept specifically as a future placeholder, make that explicit by naming it for what it is. Fix this →
3. CreateStructureRequest removes cash_flow_statement from Literal (models/api/extensions/taxonomies.py:61-72)
Removing a value from a Literal type is a silent backwards-incompatible API change. If any tenant graph has an existing structure with structure_type = 'cash_flow_statement', round-tripping through this model (e.g., any endpoint that validates against it) will now fail with a 422. The DB column has no constraint enforcing this, so stale data can exist. Worth checking in staging before merge. If it's confirmed no records exist, a brief comment to that effect in the code would close the loop. Fix this →
Carried Over (still unresolved)
FiscalCalendarError not exported from package __init__.py — close_service.__all__ lists it with a "Re-export for caller convenience" comment, but __init__.py doesn't import it. Either add it to __init__.py imports, or remove the misleading comment from close_service. (From prior review — still open.)
Closeable gate period_end >= today (service.py:632) — Still >=, still no comment explaining why closing on the last day of the period is blocked. (From original review — still open.)
Observations
-
source="system"is clean and correct —COA_SOURCESin__init__.pystill doesn't include"system", so the FK-anchor root elements are invisible to Chart of Accounts queries. The model and migration check constraint both updated consistently. -
cash_flow_statementstill inVALID_STATEMENT_TYPESinfinancial_statement_tool.py— This is correct. SEC graphs can still generate cash flow statements; only the OLTP/roboledger path is rejected. The guard at line 192 handles the split cleanly. The tool description and SEC path are unaffected. -
CLAUDE.md additions are useful — The API endpoints table and curl examples are good developer ergonomics for anyone building against the local stack. The
X-API-KeyvsAuthorization: Bearerdistinction in particular saves debugging time.
Summary
The _build_rows subtotal fix is a real bug fix that improves report correctness. The aspirational-calendar detection is a thoughtful solution for the demo scenario. The CF deferral is clean. Items to address before merge: the elif fix (correctness/perf), the _ = pattern, and the backwards-compat note on the Literal change. The two carried-over items (FiscalCalendarError export, closeable gate comment) should also be closed out.
…r error handling - Bumped the `robosystems-client` version from 0.2.47 to 0.2.48 in `pyproject.toml` and `uv.lock`. - Enhanced error handling in fiscal calendar tools by refining imports and ensuring consistent usage of `FiscalCalendarError`. - Improved documentation comments regarding cash flow structure handling in the API, clarifying the current limitations and future plans for implementation.
|
@claude ready to merge? |
|
Claude finished @jfrench9's task in 4m 22s —— View job Merge Readiness Assessment
Short answer: Yes, with one pre-merge action and a Test CI run to confirm. What the Latest Commit Fixed
And from earlier commits (confirmed still in place):
One Pre-Merge Action RequiredMigration coordination for staging environments that have already applied
This is a deployment step, not a code change — no further commits needed. Just needs to be consciously handled at rollout time. CI Status
Recommend confirming Test CI passes before merging. All prior test runs on this branch have passed. OverallThis is a well-reviewed PR. Three rounds of feedback produced measurable improvements each time — the extraction of Merge when Test CI is green and migration coordination is confirmed. |
- Removed the `_resolve_private_closed_through` method and related logic to simplify the handling of retained earnings adjustments, ensuring that synthetic adjustments now always run from inception. - Updated the `generate_report_facts` function to eliminate the `closed_through` parameter, reflecting the new approach to retained earnings calculations. - Adjusted the report generation process to enhance clarity and maintainability, ensuring that real closing entries are correctly accounted for without double-counting prior earnings. - Removed associated tests for the deleted `_derive_closed_through_from_ledger` function, streamlining the test suite.
|
@claude can you review the last commit |
|
Claude finished @jfrench9's task in 3m 55s —— View job Review: Last Commit (
|
- Introduced a new test class `TestClosePriorPeriodsToRetainedEarnings` to validate the retained earnings calculations across various scenarios, including no closes, real closes, and partial closes. - Implemented mock session methods to simulate database interactions and ensure accurate testing of retained earnings logic. - Enhanced existing tests to cover edge cases and ensure the correctness of retained earnings adjustments after closing periods.
2 participants
Summary
Introduces a complete fiscal calendar and closing cadence system for RoboLedger, along with an end-to-end demo showcasing the month-end close workflow for a sample entity (Cascade Advisory Group LLC). This feature enables organizations to define fiscal calendars, manage closing schedules with task dependencies, and track period close progress through new APIs, MCP tools, and reporting enhancements.
Key Accomplishments
Fiscal Calendar System (New)
fiscal_calendar.py) and API request/response schemas for defining fiscal years, quarters, and periods with configurable start dates and period boundaries./fiscal_calendarand/periodsendpoint groups under the ledger router with comprehensive route coverage for calendar and period lifecycle management.fiscal_calendar_tools.pyexposing fiscal calendar operations to the AI middleware layer.Closing Cadence / Schedule Enhancements
schedule_tools.py(~433 lines added) to surface closing cadence management through the MCP tool interface.Financial Reporting Enhancements
Close Demo
examples/close_demo/) that provisions a sample entity graph, chart of accounts, trial balance data, fiscal calendar, and closing schedule, then walks through the close process.Schema & Migration Changes
0001_initial_schema.pymigration has been expanded (~139 lines added) to include fiscal calendar and related tables. The separate0002_add_entry_provenance.pymigration has been removed and folded into the initial schema.Breaking Changes
0002_add_entry_provenancedeleted: The entry provenance schema has been consolidated into0001_initial_schema. Any environment that has already run0002will need a migration reconciliation (e.g., stamp or fresh migrate). This should be coordinated before deploying to shared environments.Testing
tests/operations/fiscal_calendar/test_service.py— 804 lines covering calendar CRUD, period generation, edge casestests/operations/fiscal_calendar/test_periods.py— 145 lines covering period operationstests/routers/ledger/test_fiscal_calendar.py— 395 lines covering calendar API endpointstests/routers/ledger/test_periods.py— 609 lines covering period API endpointstests/operations/schedules/test_service.py— 636 lines of additions covering closing cadence schedule logicInfrastructure Considerations
.env.local.example) — ensure this is configured in all deployment environments.0002migration.🤖 Generated with Claude Code
Branch Info:
feature/roboledger-closing-cadencemainCo-Authored-By: Claude noreply@anthropic.com