Repository navigation
Unblock every E2E spec: three of four blocker endpoints - #37
Conversation
The SPA polls this on every tab-focus and compares each string against the one it cached, invalidating only the store that changed. Nothing could pass an E2E spec without it: App.vue calls it on every page, and tests/fixtures/auth.ts fails a test on any unexpected console error, so its absence made every spec fail for a reason that looked different each time. Lives in apps.operations rather than beside build-id in apps.core, which is where it belongs by subject -- v1's own docstring calls them deliberate sibling channels, one firing on deploys and one on data changes. Every provider reads a domain model and core sits BELOW the domain apps. Reaching them through the app registry would work and would be wrong: that seam exists for core-to-integration, and using it here would hide a real dependency to dodge the layer contract. operations is a domain app and imports its siblings openly. The row count paired with each Max(updated_at) is load-bearing, not decoration. Deleting the newest row leaves the timestamp pointing at an older survivor, so a timestamp-only version would tell a client its cache was still good -- the failure mode where nothing errors and the user simply sees stale data. Tested directly. Eight tests, each asserting a version MOVED rather than that the endpoint answered, including that datasets move independently: a single global version would make the SPA refetch everything on any write, which is what this exists to avoid. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NprUR1djEf2woAnod3eAzS
A store loads this on every page, so its absence failed every spec with an unexpected console error -- and job-cost-entry-data.spec.ts names it directly. Mounted under /api/workflow/ because that is the path v1's frontend calls. The models moved out of v1's workflow app in v2; the wire did not, and the URL is the contract. Read-only, and useful before the Xero port lands precisely because it reads nothing from Xero: it serves whatever a data restore brought in. Rows arrive with no xero_id until the tenant connects, which is a tested case rather than an assumption. The response is a bare array, not a paginated envelope, because v1's client is typed `z.array(XeroPayItem)`. `multiplier` is float rather than Decimal for the same reason -- a Decimal serialises as a JSON string and would fail `z.number()` in the SPA. Declaring `id` and the timestamps as `str` instead of UUID/datetime gave a 500 on any row, which the tests caught. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NprUR1djEf2woAnod3eAzS
v1 served this from an app called `workflow`. v2 has no such app, and I carried the name into the URL anyway on the grounds that "the URL is the contract". That rule exists for a reason CLAUDE.md states precisely: exact-URL parity only where an EXTERNAL party holds the URL -- the Xero OAuth redirect, the Xero webhook, CRM phone ingestion, ServiceApiKey consumers. Nobody outside holds this one. v2's frontend is generated from v2's schema and calls whatever we expose, so there was nothing to preserve. Now `/api/xero/pay-items/`, operation `xero_pay_items_list`, matching the app the model lives in. Checked the rest of v2 for the same mistake: no other route or operation id carries the `workflow` prefix. Same reasoning noted against the NotebookLM row, which belongs under `/api/ai/` for the same reason when it lands. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NprUR1djEf2woAnod3eAzS
Answers "what check would have caught this": I served pay items from /api/workflow/ when v2 has no workflow app, and nothing objected. The obvious rule -- every top-level path segment names a v2 app -- is wrong. Measured against the current routes it flags `companies`, `timesheets`, `people`, `build-id` and `data-versions`, five of twelve, all correct. A check that is noise on its first run is a check people learn to ignore. The narrow rule has no false positives: v1 had exactly two apps v2 dissolved, `workflow` and `client`, and no route may name either. Zero instances today, so it is pinned rather than counted. It cannot conflict with the four exact-URL surfaces an external party holds (Xero OAuth redirect, Xero webhook, CRM phone ingestion, ServiceApiKey consumers) because none of them names a dissolved app. Lives in export_openapi because that already boots Django and builds the API, so the check costs nothing extra and runs wherever the export runs -- the expensive tier and CI. Proven by reintroducing the exact mistake: the check names the offending path and exits 1, and passes again once reverted. render() split into build_spec() and render(spec) so the check reads the same schema that gets written. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NprUR1djEf2woAnod3eAzS
The SPA loads this singleton into a store on boot, and JobViewTabs renders JobEstimateTab only when it is present -- so its absence darkens the whole job cluster, not just the settings screen. Derived with ModelSchema rather than hand-listing 67 fields. v2 hand-writes every schema and uses ModelSchema zero times elsewhere, which is how a wire shape drifts from its column with nothing watching; 67 transcribed fields is 67 chances to disagree, and the disagreement only shows up as a runtime validation failure in the SPA. A test asserts the derived field set equals the concrete columns, so the derivation cannot quietly stop matching. Three things the gates and tests found rather than confirmed. get_solo() CANNOT create this row: shop_company is a NOT NULL FK with no default, so django-solo's create-on-demand dies with an IntegrityError naming a column instead of the problem. Inherited from v1, which declares it identically. The row arrives with the data restore, so its absence means an unseeded install -- now a message saying exactly that. full_clean's ValidationError escaped as a 500; a rejected settings value is the caller's to fix, so it converts to a 400. Noted in place: four apps carry their own copy of that flattening, three underscore-prefixed and so invisible to the duplication gate. It belongs in apps/core beside the envelope. The layer contract rejected the test, correctly. Seeding the singleton needs a real Company and authenticating needs a Staff, and core sits BELOW the domain apps -- core's own model dodges that with a string FK reference, but a test cannot. The test therefore lives in apps/company/tests, where those fixtures already are, and says why at the top. Reusing that app's `client` fixture also removed a staff builder I had duplicated; it is already authenticated, which is why the auth test now constructs a bare Client explicitly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NprUR1djEf2woAnod3eAzS
No ADR and no CLAUDE.md line said a safe method must not write. The 'idempotent' entries (ADR 0001 error persistence, ADR 0024 Celery tasks) are a different property -- idempotent is not safe. The codebase breaches it: django-solo ships get_solo() as get_or_create, ~12 services call it, and several are reached from GET report endpoints. GET /api/accounting/reports/staff-performance-summary/ -> staff_performance_service -> CompanyDefaults.get_solo() -> get_or_create. A plain report read creates a row, latent only because the row exists. Fixed structurally: CompanyDefaults.get_solo() is overridden to read and raise, so every call site is correct without touching any of them and a future one cannot get it wrong. The endpoint's own guard is deleted -- it existed to dodge get_solo, and there is nothing left to dodge (one accessor, ADR 0039). My previous reasoning was wrong even though the guard was right: I justified it by shop_company being NOT NULL, which implies a nullable column would make get_solo-on-a-GET fine. It would not. The schema made the bug visible; it was never what made it a bug. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NprUR1djEf2woAnod3eAzS
|
Warning Review limit reached
Next review available in: 22 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 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 configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR adds authenticated APIs for company defaults, dataset versions, NotebookLM links, and Xero pay items. It adds shared test provisioning, permission contracts, OpenAPI schemas, route validation, operation manifests, status gates, and refreshed rewrite documentation. ChangesAuthenticated API surfaces
Rewrite governance and parity tracking
Test isolation updates
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant Database
Client->>API: Request authenticated v2 endpoint
API->>Database: Read or validate domain data
Database-->>API: Domain records or dataset state
API-->>Client: Typed response or validation error
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (2)
apps/core/api.py (1)
152-164: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAccess the field directly instead of probing with
getattr.
_logo_urlreceives one of two literal field names. Both fields exist on the model, so thegetattr(..., None)default cannot trigger. The coding guidelines treat such probes as a smell and require direct access after validation.Pass the
FieldFileinstead of the field name.♻️ Proposed refactor
-def _logo_url(instance: CompanyDefaults, field_name: str) -> str | None: +def _logo_url(field_file: FieldFile) -> str | None: """Build the logo path relative to the site root, or None when unset. @@ - field_file = getattr(instance, field_name, None) if not field_file: return None return str(field_file.url)Then update both resolvers to pass
obj.logoandobj.logo_wide.🤖 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/core/api.py` around lines 152 - 164, Update _logo_url to accept a FieldFile directly and access its URL without getattr or a field-name parameter; update both resolver call sites to pass obj.logo and obj.logo_wide respectively, preserving the existing None behavior for unset files.Source: Coding guidelines
apps/company/tests/test_company_defaults_api.py (1)
122-132: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a case for a blank nullable text column.
This test covers
company_name, which declaresblank=False, sofull_clean()rejects"". The nullable text columns declareblank=True, sofull_clean()accepts""and the*_not_blankcheck constraint fails at the database. Add a case such as{"company_acronym": ""}and assert a 4xx.This case fails today. It passes after the
NullableTextfix requested onapps/core/api.pylines 143-149.🤖 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/company/tests/test_company_defaults_api.py` around lines 122 - 132, Extend test_patch_rejects_a_value_the_model_refuses with a blank nullable-text case using company_acronym, and assert the PATCH returns 400 or 422 while the stored value remains unchanged. Keep the existing company_name case intact and ensure the test exercises the NullableText validation path.
🤖 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/core/api.py`:
- Around line 146-148: Update the CompanyDefaults serializer’s Meta.exclude
configuration to also exclude the timestamp fields, including created_at and its
corresponding update timestamp, from CompanyDefaultsPatchIn while preserving the
existing exclusions.
In `@docs/cutover-checklist.md`:
- Around line 13-17: Reconcile the create-job blocker metric across all three
documented sites: update docs/cutover-checklist.md lines 13-17,
docs/rewrite-status.md lines 76-81, and docs/rewrite-status.md lines 559-575 to
use the authoritative 22-of-40 count, or explicitly document any differing
scope.
In `@docs/rewrite-status.md`:
- Around line 314-319: Reconcile the readiness records in
docs/rewrite-status.md: at lines 314-319, mark the company-defaults retrieve,
partial_update, and schema_retrieve routes as delivered and leave only the
NotebookLM menu route as a blocker; at lines 296-301, include the Xero router
and remove the statement that apps/xero contains only models and migrations; at
lines 437-440, add NotebookLmLink to the detailed NotebookLM model status.
- Around line 437-440: Update the “NotebookLM CRUD” readiness row in
docs/rewrite-status.md so its “Models present” field lists NotebookLmLink,
keeping the existing services, router, and admin-screen status unchanged.
- Around line 280-288: Update the operation-diff recipe in the documented
comparison so renamed IDs such as workflow_xero_pay_items_list and
xero_pay_items_list are canonicalized or handled through an explicit rename map
before calculating unported operations. Correct the accompanying v2 − v1 claim
and retain sorted reporting of genuine differences.
In `@scripts/checks/export_openapi.py`:
- Around line 53-55: Update the JSON round-trip in the code that prepares plain
data before build_spec() so the json.loads() result is validated as a recursive
JSON mapping rather than assigned directly to dict[str, object]. Define or reuse
the project’s recursive JSON type and use an appropriate runtime/type validation
step before assigning to plain, ensuring render() and path checks do not receive
or propagate Any.
In `@scripts/generate/gen_v1_operations.py`:
- Around line 79-87: Validate that V1_FRONTEND_SRC exists and is a directory
before invoking _called_operation_ids(); raise a clear RuntimeError when it is
missing or invalid, preventing generation from proceeding with an empty called
list.
- Around line 45-60: Update the HEADER snapshot date in
scripts/generate/gen_v1_operations.py to August 7, 2026, then regenerate
scripts/v1-frontend-operations.yml so its header reflects the corrected source
date. Apply the source change at scripts/generate/gen_v1_operations.py:45-60 and
the generated header update at scripts/v1-frontend-operations.yml:1.
- Around line 89-90: Validate the YAML loaded in the existing/renamed
initialization before using it: require a mapping document containing an
operations section and a renamed mapping whose keys and values are strings,
rejecting malformed shapes instead of silently defaulting them. Build renamed as
a typed dict[str, str] and preserve only validated ledger data for subsequent
generation.
---
Nitpick comments:
In `@apps/company/tests/test_company_defaults_api.py`:
- Around line 122-132: Extend test_patch_rejects_a_value_the_model_refuses with
a blank nullable-text case using company_acronym, and assert the PATCH returns
400 or 422 while the stored value remains unchanged. Keep the existing
company_name case intact and ensure the test exercises the NullableText
validation path.
In `@apps/core/api.py`:
- Around line 152-164: Update _logo_url to accept a FieldFile directly and
access its URL without getattr or a field-name parameter; update both resolver
call sites to pass obj.logo and obj.logo_wide respectively, preserving the
existing None behavior for unset files.
🪄 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: 57b3cecd-2167-4440-bef0-a08321ba5f58
⛔ Files ignored due to path filters (5)
frontend/src/api/generated/@tanstack/react-query.gen.tsis excluded by!**/generated/**frontend/src/api/generated/index.tsis excluded by!**/generated/**frontend/src/api/generated/sdk.gen.tsis excluded by!**/generated/**frontend/src/api/generated/types.gen.tsis excluded by!**/generated/**frontend/src/api/generated/zod.gen.tsis excluded by!**/generated/**
📒 Files selected for processing (18)
CLAUDE.mdapps/company/tests/test_company_defaults_api.pyapps/core/api.pyapps/core/models.pyapps/operations/api.pyapps/operations/tests/__init__.pyapps/operations/tests/conftest.pyapps/operations/tests/test_data_versions.pyapps/xero/api.pyapps/xero/tests/__init__.pyapps/xero/tests/test_pay_item_api.pyconfig/api.pydocs/cutover-checklist.mddocs/rewrite-status.mdfrontend/schema.v2.ymlscripts/checks/export_openapi.pyscripts/generate/gen_v1_operations.pyscripts/v1-frontend-operations.yml
| class Meta: | ||
| model = CompanyDefaults | ||
| exclude: ClassVar[list[str]] = ["id", "logo", "logo_wide"] |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
created_at is writable through PATCH.
Meta.exclude removes id, logo, and logo_wide only. created_at therefore appears in CompanyDefaultsPatchIn (see frontend/schema.v2.yml lines 900-905). created_at uses auto_now_add, which sets the value on insert only. On update the handler at line 200 assigns the supplied value and line 210 lists it in update_fields, so a caller can rewrite the creation timestamp.
Exclude the timestamp columns from the patch schema.
🐛 Proposed fix to exclude the timestamp columns
class Meta:
model = CompanyDefaults
- exclude: ClassVar[list[str]] = ["id", "logo", "logo_wide"]
+ exclude: ClassVar[list[str]] = [
+ "id",
+ "logo",
+ "logo_wide",
+ "created_at",
+ "updated_at",
+ ]
fields_optional = "__all__"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class Meta: | |
| model = CompanyDefaults | |
| exclude: ClassVar[list[str]] = ["id", "logo", "logo_wide"] | |
| class Meta: | |
| model = CompanyDefaults | |
| exclude: ClassVar[list[str]] = [ | |
| "id", | |
| "logo", | |
| "logo_wide", | |
| "created_at", | |
| "updated_at", | |
| ] |
🤖 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/core/api.py` around lines 146 - 148, Update the CompanyDefaults
serializer’s Meta.exclude configuration to also exclude the timestamp fields,
including created_at and its corresponding update timestamp, from
CompanyDefaultsPatchIn while preserving the existing exclusions.
| **The recipe matches on NAME, and v2 is deliberately renaming operations.** It | ||
| holds only while `v2 - v1 = ∅` (true as at 2026-08-08). Every rename — | ||
| `workflow_xero_pay_items_list` → `xero_pay_items_list` is the first, and the | ||
| porting rules invite more, since the API is free wherever no external party | ||
| holds the URL — breaks it in **both** directions at once: the v1 name reads as | ||
| still-unported and the v2 name becomes an orphan, so one rename inflates the | ||
| remaining count by one and hides a real gap. Print `sorted(v2 - v1)` every time | ||
| you run it; a non-empty result means the count is wrong, not that v2 grew a new | ||
| endpoint. Keep a rename map here when the list gets long enough to need one. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fix the operation-diff recipe for renamed operation IDs.
The recipe subtracts v2 operation names from v1 names. Therefore, workflow_xero_pay_items_list remains unported when v2 exposes xero_pay_items_list. The text also claims v2 - v1 is empty while documenting this rename. Add an explicit rename map or canonicalise both sets before calculating the difference.
🤖 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/rewrite-status.md` around lines 280 - 288, Update the operation-diff
recipe in the documented comparison so renamed IDs such as
workflow_xero_pay_items_list and xero_pay_items_list are canonicalized or
handled through an explicit rename map before calculating unported operations.
Correct the accompanying v2 − v1 claim and retain sorted reporting of genuine
differences.
| # json.loads is Any-typed; the cast records what the round-trip actually | ||
| # produces rather than letting Any leak into every caller (ADR 0028). | ||
| plain: dict[str, object] = json.loads(json.dumps(spec)) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
uv run mypy --strict --disallow-any-expr apps config manage.py scriptsRepository: corrin/docketworks_v2
Length of output: 199
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Repository files around check:\n'
git ls-files | rg '(^scripts/checks/export_openapi\.py$|pyproject\.toml$|poetry\.lock$|uv\.lock$|requirements.*\.txt$|mypy\.ini$)'
printf '\nTarget file context:\n'
cat -n scripts/checks/export_openapi.py | sed -n '1,140p'
printf '\nMypy config references:\n'
for f in $(git ls-files | rg '(^mypy\.ini$|pyproject\.toml$)'); do
echo "--- $f"
rg -n "mypy|disallow-any|strict|warn-return-any|warn-unused-ignores" "$f" || true
doneRepository: corrin/docketworks_v2
Length of output: 5823
🌐 Web query:
Python mypy documentation json.loads returns Any
💡 Result:
In Python, the json.loads function is annotated to return Any [1][2]. This is primarily because json.loads is a highly flexible API designed to handle arbitrary JSON data, which can include various combinations of nested objects, lists, strings, numbers, booleans, and null values [1]. There are two main reasons for this design choice: 1. Complexity of Representation: Representing the recursive, heterogeneous structure of JSON precisely in Python type hints is difficult [1]. While recursive type definitions have been discussed in the Python community, they are complex to implement and manage [1]. 2. API Flexibility: The json.loads function accepts several optional arguments—such as object_hook, parse_float, and the cls parameter—that allow users to customize the deserialization process [1][3]. These customizations can cause the function to return types other than the standard Python mappings (e.g., dicts for JSON objects, lists for JSON arrays) [1]. Because the function's output type can be fundamentally altered by these parameters, returning Any is the most accurate way to describe its behavior across all possible use cases [1]. If you encounter issues where mypy complains about receiving an Any type, you can use several strategies to narrow the type or suppress the warning: - Casting: Use typing.cast to inform mypy of the expected type [4][5]. y = typing.cast(Dict[str, Any], json.loads(data)) - Ignoring: If you are certain of the data structure, you can use a type ignore comment [4]. return json.loads(data) # type: ignore[no-any-return] - Validation: When handling untrusted JSON data, the best practice is to validate the structure at runtime (using libraries like pydantic or marshmallow) or manually check the types after loading [2].
Citations:
- 1: https://adamj.eu/tech/2021/06/14/python-type-hints-3-somewhat-unexpected-uses-of-typing-any-in-pythons-standard-library/
- 2: Question about type signature for "don't know" python/mypy#3712
- 3: https://docs.python.org/3/library/json.html
- 4: https://software.codidact.com/posts/289150
- 5: mypy not checking if calling passing a value of type Any to a function expecting some other type python/mypy#16563
🌐 Web query:
pypi typing-extensions TypedDict Recursive TypedDict json.loads cast
💡 Result:
Recursive TypedDicts are supported in modern Python type hinting, allowing you to define dictionaries that contain references to themselves, such as for tree or graph structures [1][2]. Recursive TypedDict Definition To define a recursive TypedDict, you can use forward references (strings) within the type definition [1][3]. Class-based syntax: from typing import TypedDict, List class Node(TypedDict): value: int children: List["Node"] Functional syntax: from typing import TypedDict, List # Note: Functional syntax for recursive types can occasionally trigger # "cyclic definition" errors in older mypy versions; class-based is safer [3]. Node = TypedDict("Node", {"value": int, "children": List["Node"]}) Typing Extensions The typing-extensions package on PyPI is the official backport for new typing features [4][5]. While TypedDict itself has been in the standard library since Python 3.8, typing-extensions provides bug fixes, consistency improvements, and backports for enhancements to TypedDict across different Python versions [5]. It is recommended to use typing-extensions if you need features introduced in newer Python releases or require consistent behavior across multiple versions [4][5]. json.loads and cast When dealing with JSON data, json.loads returns a standard dictionary at runtime [6]. Because TypedDict is a purely static type hint and does not exist as a class at runtime, you cannot cast a dict to a TypedDict using constructors [7][6]. To bridge the gap between runtime JSON and static analysis, use typing.cast [6]: import json from typing import cast data = json.loads('{"value": 1, "children": []}') node = cast(Node, data) Crucially, cast does not perform runtime validation; it only tells the static type checker (like mypy or pyright) to treat the dictionary as having the specified TypedDict structure [6]. If the JSON data does not match the TypedDict schema, your program may still fail at runtime when accessing keys or performing operations [6]. For robust runtime validation of JSON payloads, consider using libraries like Pydantic or Marshmallow instead of plain TypedDict [6].
Citations:
- 1: https://github.com/python/mypy/blob/bdef6ef8/test-data/unit/check-recursive-types.test
- 2: Support recursive TypedDicts python/mypy#13373
- 3: Different behaviour for recursive TypedDict defined as class/functional python/mypy#14460
- 4: https://pypi.org/project/typing-extensions/
- 5: https://typing-extensions.readthedocs.io/en/latest/
- 6: https://python-type-hints.com/core-type-hints-fundamentals/literal-and-typeddict/when-to-use-typeddict-vs-dataclasses/
- 7: https://mypy.readthedocs.io/en/latest/typed_dict.html
Validate the JSON boundary without Any.
json.loads() returns Any; assigning it to an annotated local does not make the return value dict[str, object]. Validate the decoded JSON into a recursive JSON mapping type before assigning it to plain, so build_spec() does not expose Any through render() or the path checks.
🧰 Tools
🪛 ast-grep (0.45.0)
[info] 54-54: use jsonify instead of json.dumps for JSON output
Context: json.dumps(spec)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/checks/export_openapi.py` around lines 53 - 55, Update the JSON
round-trip in the code that prepares plain data before build_spec() so the
json.loads() result is validated as a recursive JSON mapping rather than
assigned directly to dict[str, object]. Define or reuse the project’s recursive
JSON type and use an appropriate runtime/type validation step before assigning
to plain, ensuring render() and path checks do not receive or propagate Any.
Source: Coding guidelines
| HEADER = """\ | ||
| # v1's operation surface, recorded 2026-08-08 and frozen. | ||
| # | ||
| # GENERATED by scripts/generate/gen_v1_operations.py. Do not hand-edit the two | ||
| # lists; do hand-edit `renamed` when v2 deliberately renames an operation. | ||
| # | ||
| # WHY THIS EXISTS: the remaining-work count has to be derivable in CI, and CI | ||
| # checks out only this repo -- ../docketworks is not there. v1 is frozen, so its | ||
| # totals are constants and can simply be stored. | ||
| # | ||
| # WHAT THIS IS NOT: an authority on v2's contract. It records which operation | ||
| # NAMES v1's frontend called, never what shape v2 must serve. Commit 5cefdc5 | ||
| # deleted the parity ratchet precisely because that gate treated v1 as | ||
| # authoritative and so preserved the design v2 exists to replace. Nothing here | ||
| # can fail a build because v2 differs from v1. | ||
| """ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the future snapshot date at its source.
Both files state that the snapshot was recorded on August 8, 2026. The current date is August 7, 2026.
scripts/generate/gen_v1_operations.py#L45-L60: ChangeHEADERto record August 7, 2026.scripts/v1-frontend-operations.yml#L1-L1: Regenerate the header fromscripts/generate/gen_v1_operations.py.
📍 Affects 2 files
scripts/generate/gen_v1_operations.py#L45-L60(this comment)scripts/v1-frontend-operations.yml#L1-L1
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/generate/gen_v1_operations.py` around lines 45 - 60, Update the
HEADER snapshot date in scripts/generate/gen_v1_operations.py to August 7, 2026,
then regenerate scripts/v1-frontend-operations.yml so its header reflects the
corrected source date. Apply the source change at
scripts/generate/gen_v1_operations.py:45-60 and the generated header update at
scripts/v1-frontend-operations.yml:1.
| if not V1_SCHEMA.is_file(): | ||
| raise RuntimeError( | ||
| f"v1 schema not found at {V1_SCHEMA}. This generator runs only on a machine " | ||
| "with the v1 repo checked out beside this one; its committed output is what " | ||
| "CI reads." | ||
| ) | ||
|
|
||
| operations = _operation_ids(V1_SCHEMA) | ||
| called = _called_operation_ids(V1_FRONTEND_SRC, set(operations)) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate V1_FRONTEND_SRC before generating the manifest.
If the schema exists but the frontend source directory is absent, rglob() produces no call sites. The generator then writes an empty called list and can report all frontend work as complete.
Reject a missing or non-directory V1_FRONTEND_SRC before calling _called_operation_ids().
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/generate/gen_v1_operations.py` around lines 79 - 87, Validate that
V1_FRONTEND_SRC exists and is a directory before invoking
_called_operation_ids(); raise a clear RuntimeError when it is missing or
invalid, preventing generation from proceeding with an empty called list.
| existing = yaml.safe_load(OUTPUT_PATH.read_text()) if OUTPUT_PATH.is_file() else None | ||
| renamed = (existing or {}).get("renamed") or {} |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repository files matching gen_v1_operations.py:"
fd -a 'gen_v1_operations\.py$' . || true
file="$(fd 'gen_v1_operations\.py$' . | head -n 1 || true)"
if [ -n "${file:-}" ]; then
echo
echo "File: $file"
wc -l "$file"
echo
ast-grep outline "$file" || true
echo
echo "Relevant lines 1-180:"
sed -n '1,180p' "$file" | nl -ba
echo
echo "Search for renamed operations/type annotations/imports:"
rg -n "renamed|TYPE_CHECKING|dict|Dict|safe_load|OUTPUT_PATH|generate|write" "$file"
fiRepository: corrin/docketworks_v2
Length of output: 994
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="scripts/generate/gen_v1_operations.py"
echo "File size and relevant contents:"
python3 <<'PY'
from pathlib import Path
p = Path("scripts/generate/gen_v1_operations.py")
lines = p.read_text().splitlines()
print("total_lines", len(lines))
for i in range(1, 109):
print(f"{i:3d}: {lines[i-1]}")
PY
echo
echo "Imports/top-level:"
python3 - <<'PY'
from pathlib import Path
p = Path("scripts/generate/gen_v1_operations.py")
for line in enumerate(p.read_text().splitlines(), 1):
if line[0] <= 30:
print(f"{line[0]:3d}: {line[1]}")
PY
echo
echo "Behavioral probe: malformed YAML inputs and safe_load result type shape:"
python3 - <<'PY'
from pathlib import Path
import yaml
OUTPUT_PATH = Path("scripts/v1-frontend-operations.yml")
samples = [
("empty", ""),
("renamed_string", "renamed:\n old_api_v1.list_posts: new_api_v2.list_posts\n"),
("renamed_bad_string", "renamed: old_api_v1.list_posts: new_api_v2.list_posts\n"),
("renamed_bad_scalar", "renamed: 123\n"),
("renamed_bad_list", "renamed:\n - a: b\n"),
("renamed_bool", "renamed:\n old_api_v1.list_posts: true\n"),
("renamed_nonstring_key", "renamed:\n 123: new_api_v2.list_posts\n"),
]
def model(path):
return yaml.safe_load(path.read_text()) if path.is_file() else None
code_text = Path(__file__).read_text()
# Print safe_load for each sample without writing files
for name, content in samples:
try:
data = yaml.safe_load(content) if content else None
except Exception as exc:
data = f"exception:{type(exc).__name__}: {exc}"
print(f"{name}: {data!r} (type={type(data).__name__})")
if isinstance(data, dict):
renamed =(data.copy()).get("renamed") or {}
print(f" renamed={renamed!r} type={type(renamed).__name__} entries={len(renamed)}")
for k, v in renamed.items():
print(f" key={k!r} type={type(k).__name__} val={v!r} valtype={type(v).__name__}")
PYRepository: corrin/docketworks_v2
Length of output: 6564
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Python and yaml availability:"
python3 - <<'PY'
import sys
print("python", sys.version.split()[0], "executable", sys.executable)
try:
import yaml
print("pyyaml version", getattr(yaml, "__version__", "unknown"))
except Exception as exc:
print("pyyaml unavailable:", type(exc).__name__, str(exc))
PY
echo
echo "Static read for current renamed handling:"
sed -n '78,98p' scripts/generate/gen_v1_operations.py
echo
echo "Usages of YAML outputs/types related to renamed:"
rg -n '"renamed"|operations:|called:|safe_dump|safe_load' scripts scripts/generate scripts/checks v1-frontend-operations.yml 2>/dev/null || true
echo
echo "Local static verifier for input shape through type hints:"
python3 - <<'PY'
from pathlib import Path
root = Path("scripts/generate/gen_v1_operations.py")
text = root.read_text()
checks = {
"imports typing": any("from typing import" in text or "import typing" in text for _ in ()),
"adds dict[str,str] cast/variable": "dict[str, str]" in text or "cast(dict[str, str]" in text,
"has renamed validation": any(s in text for s in ("validate", "isinstance", "assert isinstance", "type_checking")),
}
for k, v in checks.items():
print(f"{k}: {v}")
PYRepository: corrin/docketworks_v2
Length of output: 2996
Validate renamed into dict[str, str].
yaml.safe_load() returns untyped data, and renamed = (existing or {}).get("renamed") or {} accepts non-dict values, non-string keys, or non-string values. If the generator writes that back into scripts/v1-frontend-operations.yml, downstream consumers can fail or interpret the ledger incorrectly. A document without operations is also malformed because the generator only preserves renamed. Parse a valid document, reject anything that does not match that ledger shape, and build a typed dict[str, str] for renamed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/generate/gen_v1_operations.py` around lines 89 - 90, Validate the
YAML loaded in the existing/renamed initialization before using it: require a
mapping document containing an operations section and a renamed mapping whose
keys and values are strings, rejecting malformed shapes instead of silently
defaulting them. Build renamed as a typed dict[str, str] and preserve only
validated ledger data for subsequent generation.
Source: Coding guidelines
The status table went stale twice by hand, which is why status_table.py exists, and the rows it did not measure kept rotting anyway. Operations still to port, operations exposed, specs ported and coverage are now computed. v1 is frozen for the rest of the port, so its operation surface is stored once in scripts/v1-frontend-operations.yml and the remaining count is that minus the live schema.v2.yml. Porting an operation lowers the number with no edit to any file. This is what makes the row measurable in CI at all: CI checks out this repo and nothing else, so a diff that reads ../docketworks cannot run there. The stored file is a work list, not a contract authority. It records which operation names v1's frontend called, never what shape v2 must serve; nothing here can fail a build because v2 differs from v1, only because v2 has drifted from its own record of what is left. That distinction is why 5cefdc5 deleted the parity ratchet, and why this is not a revival of it. Renames are the failure this can actually catch without v1 present. A rename nobody records makes the v1 name read as still missing AND the v2 name look brand new, so it inflates the remaining count while hiding a real gap behind it. Any v2 operation with no v1 ancestor and no entry saying why now fails the gate; 17 more renames are due as the workflow_* operations port. The table owns each number and prose may not hold a second copy: --check fails on a sentence that disagrees with its row. It caught one immediately, where the dead-surface figure was worded as an unported count. Coverage is read from the .coverage file CI writes a step earlier rather than recomputed, and a row whose input is missing or stale declines and keeps the file's value loudly, rather than reporting a number that looks measured and is wrong. Estimates of unfinished work are gone from rewrite-status.md rather than rounded. A forecast cannot be derived and cannot be checked, so it rots by construction; what remains is derived-and-gated or a frozen fact about v1. Also recorded as post-cutover work: purge "v1" and "v2" from comments, docstrings, docs and filenames. We document state, not change — a new reader cares what the system does now, not how it came to be. Tests plant both faults rather than only exercising the happy path: an unrecorded rename and prose quoting a superseded number each fail the gate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MXsngTGAyay3yY72TNEnrX
Three times the layer contract rejected a test and I worked around it: moved the company-defaults test out of apps/core, left the ai one unresolved, and reached for getattr(user, "is_superuser", False) because apps.ai cannot name Staff. The rejections were right and the workarounds were the problem. All three came from one wrong default. DocketWorks cannot boot without a CompanyDefaults row and cannot do anything without a Staff, so a test starting from an empty database exercises a state the product never runs in. One of my own tests asserted behaviour for a missing singleton -- a scenario that cannot occur. seed_job_prereqs() already built exactly this world; it was misnamed (nothing about it is job-specific), opt-in, and stranded in a domain app where core and ai could not reach it. It is now seed_docketworks_prereqs() in a root conftest.py, autouse for every database test. Two facts make that the right home: import-linter's root_packages are apps and config, so a top-level conftest is outside the layer contract; and pytest resolves fixtures by NAME, not by import, so apps/ai/tests gets a Staff without apps.ai importing apps.accounts. Both test files now sit beside the code they test, with lint-imports green -- the constraint is satisfied rather than dodged. Proven rather than assumed: a test declaring only django_db and no fixtures can call CompanyDefaults.get_solo(); a bare_install test still sees zero rows; a non-DB test never touches the database. Three company tests broke, correctly. They asserted the contents of the whole table -- `names == ["Acme", "Zeta"]` reads as a list assertion but is really an assertion that the installation is empty. Scoped to the rows each test creates. `client` stays pytest-django's anonymous client and `api` is the authenticated one. Naming an authenticated fixture `client` is how my own test_requires_authentication silently stopped testing authentication. Also replaces the two pre-existing getattr probes in core/auth.py with a StaffPermissions Protocol -- core cannot import Staff, and accounts cannot move down because Staff reaches into job.LabourSubtype. Verified the Protocol narrows: after isinstance, user.is_superuser typechecks as bool and a typo'd flag is an attr-defined error. Under getattr both passed silently, on a security boundary. No new ADR: the rules went into 0025 (how tests are written) and 0039 (one implementation per concept), which is where a reader would look. 32 ADRs at ~7,500 words is already smaller than rewrite-status.md alone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NprUR1djEf2woAnod3eAzS
CI caught a real defect the ModelSchema change introduced. Deriving from the model brings the COLUMN defaults with it, and a Decimal column with default=20.00 publishes as a string-typed field carrying a numeric default: annual_leave_loading: z.string().regex(...).optional().default(20) which does not compile. The backend was happy; tsc was not, and the generated client is where that surfaces. They are wrong on the wire in both directions, not merely awkward. On a response a default is meaningless -- the server always sends a value. On a partial update it misdescribes omission, which leaves the stored value alone rather than applying anything. drop_model_defaults() lives beside omittable() in apps/core/schemas.py, which already existed to solve the same problem one field at a time: four CRM PATCH fields once advertised default:"" against their own minLength:1. This is that fix applied to a whole derived schema. The honest trade of deriving: it copies everything, including what does not belong on the wire. Still better than transcribing 67 fields -- the failure was loud, at build, and fixed in one place rather than 67. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NprUR1djEf2woAnod3eAzS
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/schema.v2.yml (1)
825-905: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftAlign
CompanyDefaultsPatchInwith persisted field rules.
CompanyDefaultsOutdefinescompany_nameas non-null, but this PATCH schema accepts{"company_name": null}. It also exposescreated_atandupdated_atas client-writable fields. A presence-applied PATCH will either reject these advertised inputs later or corrupt audit data.Create a dedicated PATCH schema. Accept
nullonly for nullable columns. Exclude audit fields. UseNullableTextfor nullable text fields so blank strings are rejected.As per coding guidelines, nullable text must use
NullableText, and fake| Nonetypes must not model presence-only updates.🤖 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/schema.v2.yml` around lines 825 - 905, Update CompanyDefaultsPatchIn to be a dedicated presence-based PATCH schema matching persisted column nullability: keep company_name non-nullable, permit null only for genuinely nullable fields, and use NullableText for nullable text properties so blank strings are rejected. Remove created_at and updated_at from the client-writable properties, and do not model presence-only updates with fake “| None” types.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/company/tests/test_company_api.py`:
- Around line 57-64: Update the response-shape assertion in the test around the
rows and names logic to validate that every row has exactly the keys {"id",
"name"}, rather than checking only rows[0]. Preserve the existing sortedness and
ordering assertions.
In `@config/tests/test_contract_gates.py`:
- Around line 189-212: Update the status fixture to set module.sys.argv to
["status_table"] after loading the module, isolating status.main() from pytest
arguments. In tests that exercise --check, explicitly override module.sys.argv
with the required check arguments.
In `@conftest.py`:
- Around line 101-103: Update the autouse fixture guard around
seed_docketworks_prereqs() to recognize pytest-django’s database-dependent test
marker, including tests using db or transactional_db fixtures without an
explicit django_db marker. Preserve the bare_install exclusion while ensuring
all database-dependent tests receive the prerequisite seeding.
In `@docs/rewrite-status.md`:
- Around line 283-285: Remove the hard-coded “17 still to come” count from the
rename guidance in docs/rewrite-status.md, leaving the instructions general and
accurate. Do not add an unvalidated metric; only retain a numeric count if
status_table.py measures it and validates the corresponding prose claim.
In `@scripts/checks/status_table.py`:
- Around line 175-189: The V1 ledger loader must validate member types and
relationships before constructing V1Operations. Extend the validation around the
expected-field checks to ensure ledger entries are strings, called is a subset
of operations, renamed keys exist in operations, rename targets are unique, and
renamed targets do not overlap introduced entries; only then access the fields
and return V1Operations.
---
Outside diff comments:
In `@frontend/schema.v2.yml`:
- Around line 825-905: Update CompanyDefaultsPatchIn to be a dedicated
presence-based PATCH schema matching persisted column nullability: keep
company_name non-nullable, permit null only for genuinely nullable fields, and
use NullableText for nullable text properties so blank strings are rejected.
Remove created_at and updated_at from the client-writable properties, and do not
model presence-only updates with fake “| None” types.
🪄 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: 24d15f48-e293-48c4-a24a-0104e84edc42
⛔ Files ignored due to path filters (6)
frontend/src/api/generated/@tanstack/react-query.gen.tsis excluded by!**/generated/**frontend/src/api/generated/index.tsis excluded by!**/generated/**frontend/src/api/generated/sdk.gen.tsis excluded by!**/generated/**frontend/src/api/generated/types.gen.tsis excluded by!**/generated/**frontend/src/api/generated/zod.gen.tsis excluded by!**/generated/**uv.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
apps/ai/api.pyapps/ai/tests/test_notebook_lm_menu.pyapps/company/tests/test_company_api.pyapps/core/auth.pyapps/core/tests/test_company_defaults_api.pyconfig/api.pyconfig/tests/test_contract_gates.pyconftest.pydocs/adr/0025-tests-state-business-risk.mddocs/adr/0039-one-implementation-per-concept.mddocs/code-quality.mddocs/cutover-checklist.mddocs/rewrite-status.mdfrontend/schema.v2.ymlpyproject.tomlscripts/checks/status_table.pyscripts/generate/gen_v1_operations.pyscripts/v1-frontend-operations.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/cutover-checklist.md
- scripts/generate/gen_v1_operations.py
| rows = response.json() | ||
| # Sortedness asserted over the rows this test owns rather than the whole | ||
| # table: every install carries a shop company, so an exact list was | ||
| # asserting a state no real instance is ever in. | ||
| names = [row["name"] for row in rows] | ||
| assert names == sorted(names) | ||
| assert names.index("Acme") < names.index("Zeta") | ||
| assert set(rows[0]) == {"id", "name"} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate the response shape for every row.
assert set(rows[0]) == {"id", "name"} checks only the first row. Because this test accepts seeded rows, a later row can contain missing or extra fields without failing the test. Validate every returned row.
Proposed fix
- assert set(rows[0]) == {"id", "name"}
+ assert all(set(row) == {"id", "name"} for row in rows)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| rows = response.json() | |
| # Sortedness asserted over the rows this test owns rather than the whole | |
| # table: every install carries a shop company, so an exact list was | |
| # asserting a state no real instance is ever in. | |
| names = [row["name"] for row in rows] | |
| assert names == sorted(names) | |
| assert names.index("Acme") < names.index("Zeta") | |
| assert set(rows[0]) == {"id", "name"} | |
| rows = response.json() | |
| # Sortedness asserted over the rows this test owns rather than the whole | |
| # table: every install carries a shop company, so an exact list was | |
| # asserting a state no real instance is ever in. | |
| names = [row["name"] for row in rows] | |
| assert names == sorted(names) | |
| assert names.index("Acme") < names.index("Zeta") | |
| assert all(set(row) == {"id", "name"} for row in rows) |
🤖 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/company/tests/test_company_api.py` around lines 57 - 64, Update the
response-shape assertion in the test around the rows and names logic to validate
that every row has exactly the keys {"id", "name"}, rather than checking only
rows[0]. Preserve the existing sortedness and ordering assertions.
| @pytest.fixture | ||
| def status(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> ModuleType: | ||
| """The gate against a miniature repo: three called operations, one served.""" | ||
| module = _load_script("status_table") | ||
|
|
||
| v1 = tmp_path / "v1-frontend-operations.yml" | ||
| v1.write_text( | ||
| "e2e_spec_files: 7\n" | ||
| "operations: [alpha_list, beta_list, gamma_list, dead_list]\n" | ||
| "called: [alpha_list, beta_list, gamma_list]\n" | ||
| "renamed: {}\n" | ||
| "introduced: []\n" | ||
| ) | ||
| schema = tmp_path / "schema.v2.yml" | ||
| schema.write_text(" operationId: alpha_list\n") | ||
|
|
||
| monkeypatch.setattr(module, "V1_OPERATIONS_FILE", v1) | ||
| monkeypatch.setattr(module, "V2_SCHEMA", schema) | ||
| monkeypatch.setattr(module, "STATUS_DOC", tmp_path / "rewrite-status.md") | ||
| # Only the row under test, so these stay about the derivation rather than | ||
| # about which rows the real doc happens to carry. | ||
| label = "Backend operations still to port" | ||
| monkeypatch.setattr(module, "MEASURED", {label: module.MEASURED[label]}) | ||
| return module |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Isolate argparse from pytest arguments.
status.main() parses sys.argv before it evaluates the gate. If developers run pytest config/tests/test_contract_gates.py, its path argument causes parse_args() to exit before these tests reach their assertions.
Set module.sys.argv to ["status_table"] in this fixture. Override it explicitly in tests that need --check.
Proposed fix
monkeypatch.setattr(module, "V1_OPERATIONS_FILE", v1)
monkeypatch.setattr(module, "V2_SCHEMA", schema)
monkeypatch.setattr(module, "STATUS_DOC", tmp_path / "rewrite-status.md")
+ monkeypatch.setattr(module.sys, "argv", ["status_table"])As per coding guidelines, use scoped paths while iterating.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @pytest.fixture | |
| def status(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> ModuleType: | |
| """The gate against a miniature repo: three called operations, one served.""" | |
| module = _load_script("status_table") | |
| v1 = tmp_path / "v1-frontend-operations.yml" | |
| v1.write_text( | |
| "e2e_spec_files: 7\n" | |
| "operations: [alpha_list, beta_list, gamma_list, dead_list]\n" | |
| "called: [alpha_list, beta_list, gamma_list]\n" | |
| "renamed: {}\n" | |
| "introduced: []\n" | |
| ) | |
| schema = tmp_path / "schema.v2.yml" | |
| schema.write_text(" operationId: alpha_list\n") | |
| monkeypatch.setattr(module, "V1_OPERATIONS_FILE", v1) | |
| monkeypatch.setattr(module, "V2_SCHEMA", schema) | |
| monkeypatch.setattr(module, "STATUS_DOC", tmp_path / "rewrite-status.md") | |
| # Only the row under test, so these stay about the derivation rather than | |
| # about which rows the real doc happens to carry. | |
| label = "Backend operations still to port" | |
| monkeypatch.setattr(module, "MEASURED", {label: module.MEASURED[label]}) | |
| return module | |
| `@pytest.fixture` | |
| def status(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> ModuleType: | |
| """The gate against a miniature repo: three called operations, one served.""" | |
| module = _load_script("status_table") | |
| v1 = tmp_path / "v1-frontend-operations.yml" | |
| v1.write_text( | |
| "e2e_spec_files: 7\n" | |
| "operations: [alpha_list, beta_list, gamma_list, dead_list]\n" | |
| "called: [alpha_list, beta_list, gamma_list]\n" | |
| "renamed: {}\n" | |
| "introduced: []\n" | |
| ) | |
| schema = tmp_path / "schema.v2.yml" | |
| schema.write_text(" operationId: alpha_list\n") | |
| monkeypatch.setattr(module, "V1_OPERATIONS_FILE", v1) | |
| monkeypatch.setattr(module, "V2_SCHEMA", schema) | |
| monkeypatch.setattr(module, "STATUS_DOC", tmp_path / "rewrite-status.md") | |
| monkeypatch.setattr(module.sys, "argv", ["status_table"]) | |
| # Only the row under test, so these stay about the derivation rather than | |
| # about which rows the real doc happens to carry. | |
| label = "Backend operations still to port" | |
| monkeypatch.setattr(module, "MEASURED", {label: module.MEASURED[label]}) | |
| return module |
🤖 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 `@config/tests/test_contract_gates.py` around lines 189 - 212, Update the
status fixture to set module.sys.argv to ["status_table"] after loading the
module, isolating status.main() from pytest arguments. In tests that exercise
--check, explicitly override module.sys.argv with the required check arguments.
Source: Coding guidelines
| if "django_db" not in request.keywords or "bare_install" in request.keywords: | ||
| return | ||
| seed_docketworks_prereqs() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find database-fixture tests that do not necessarily carry pytest.mark.django_db.
rg -nP --glob '*.py' \
'^\s*def\s+test_[^(]*\([^)]*\b(db|transactional_db)\b' \
apps config tests 2>/dev/null || true
# Show the provisioning guard.
rg -n -C 5 'def _docketworks_prereqs|django_db.*request\.keywords|bare_install' conftest.pyRepository: corrin/docketworks_v2
Length of output: 1372
Provision tests that need database fixtures.
This prereq fixture only activates when django_db is in request.keywords. Tests that get database access through pytest-django fixtures such as db or transactional_db without that marker skip seed_docketworks_prereqs() and run from an empty installation.
Use pytest-django’s test-is--db-dependent marker in this autouse fixture, or update the guard to handle every database-fixture path.
🤖 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 `@conftest.py` around lines 101 - 103, Update the autouse fixture guard around
seed_docketworks_prereqs() to recognize pytest-django’s database-dependent test
marker, including tests using db or transactional_db fixtures without an
explicit django_db marker. Preserve the bare_install exclusion while ensuring
all database-dependent tests receive the prerequisite seeding.
| **Renames are the one thing you must record by hand.** `export_openapi.py` pins | ||
| dissolved v1 app names at zero, so every called `workflow_*` operation gets a | ||
| new name when it ports — 17 still to come. Add each to `renamed:` as you go: an |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove or derive the 17 still to come count.
Line 285 states a remaining-operation count that status_table.py does not measure or validate. It can become stale after an operation ports while --check still passes.
Remove the count, or add a measured row and prose-claim validation for this subset.
🤖 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/rewrite-status.md` around lines 283 - 285, Remove the hard-coded “17
still to come” count from the rename guidance in docs/rewrite-status.md, leaving
the instructions general and accurate. Do not add an unvalidated metric; only
retain a numeric count if status_table.py measures it and validates the
corresponding prose claim.
| for key, kind in expected.items(): | ||
| if key not in raw: | ||
| raise KeyError(f"{V1_OPERATIONS_FILE} is missing '{key}'") | ||
| if not isinstance(raw[key], kind): | ||
| raise TypeError( | ||
| f"{V1_OPERATIONS_FILE}: '{key}' must be {kind.__name__}, " | ||
| f"got {type(raw[key]).__name__}" | ||
| ) | ||
| return V1Operations( | ||
| e2e_spec_files=raw["e2e_spec_files"], | ||
| operations=raw["operations"], | ||
| called=raw["called"], | ||
| renamed=raw["renamed"], | ||
| introduced=raw["introduced"], | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate ledger members and mapping relationships.
Line 175 validates only outer container types. If two renamed keys map to one existing v2 operation, lines 205-208 classify both v1 operations as ported. The gate can then undercount remaining operations.
Validate string members, called ⊆ operations, rename keys in operations, unique rename targets, and disjoint renamed targets and introduced entries before returning V1Operations.
As per coding guidelines, “Treat type annotations as data contracts” and validate before direct access.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/checks/status_table.py` around lines 175 - 189, The V1 ledger loader
must validate member types and relationships before constructing V1Operations.
Extend the validation around the expected-field checks to ensure ledger entries
are strings, called is a subset of operations, renamed keys exist in operations,
rename targets are unique, and renamed targets do not overlap introduced
entries; only then access the fields and return V1Operations.
Source: Coding guidelines
Backend CI failed on the autouse baseline with a duplicate-key error at setup. Three test modules were seeding CompanyDefaults and the pay-item catalogue themselves, so the baseline collided with them. The collision was the symptom of the defect the baseline was meant to fix. I introduced a second copy of the seeder in the root conftest -- the pay-item catalogue, the earnings-rate constants and the CompanyDefaults insert all existed twice, in the very file written to stop concepts existing twice (ADR 0039). The two copies then disagreed on whether to pin id=1. So the seeder has one home, apps/company/tests/job_fixtures.py, beside the factories that depend on it. conftest.py keeps only the wiring: it may import anything, which is what lets a bottom-layer app's tests reach a Staff, but where the rows live is not a layering question. Removed, all of them redundant against the baseline: - 33 seed_job_prereqs() calls across 18 files, and the function itself - make_job()'s own seeding call - crm's job_env fixture and the same rows inlined in a service test - xero's pay-item tests, which created catalogue rows that already exist; they read the baseline now. The one case needing an unsynced row gives it a name the catalogue does not use, since name-per-api is unique - xero's local api fixture, which reached into apps.company.tests.conftest for authenticate() -- exactly the cross-app reach the root fixtures remove The one caller that wanted the returned pay item gets ordinary_time_pay_item(), which reads and never creates: a miss means the baseline did not apply, and that is worth failing on rather than papering over (ADR 0015). Not fixed by advancing the pk sequence. An explicit-pk insert does leave the sequence behind, but a CHECK constraint pins CompanyDefaults to id=1, so a second row cannot exist however the sequence is set -- resetting it would only change which error fires. Full suite green locally; the change alters fixture resolution for every test, so scoping the run would have proved nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NprUR1djEf2woAnod3eAzS
It fails in CI and passes locally, and `assert thumbnail_path is not None` cannot say which of three things happened: the task skipped the file, never ran, or ran and failed. Diagnosing it from that cost a round-trip and three wrong guesses. The task's skip conditions are silent by design -- a non-image mime_type or an inactive row returns without doing anything (apps/job/tasks.py:93-99) -- and anything it raises is persisted as an AppError that nothing reads. So assert the inputs before the output, and read the error row. No fix here: the cause is still unknown. Ruled out so far, so nobody re-treads them: the shared file root (this module is tmp_path-sandboxed since it was written), a swallowed task exception (CELERY_TASK_EAGER_PROPAGATES is on, so a raise would fail the 201 assertion first), mimetypes.guess_type (the upload path uses file_obj.content_type), and test ordering (pytest-randomly is not installed, so -p no:randomly is a no-op). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NprUR1djEf2woAnod3eAzS
Two gate failures, neither a code defect. The status table refused an operation with no v1 ancestor and no entry saying why. notebook_lm_links_menu_list is a rename: v1 served it from an app called `workflow` that v2 does not have, so the operation id lost the prefix along with the app. Recorded under `renamed:` beside the xero pay-items entry, which moved for the same reason. Left unrecorded it would have added one to the remaining count and hidden a real gap behind it. The derived rows moved with it: 95 operations still to port becomes 94, 179 exposed becomes 180, 1293 tests become 1296. nanoid <3.3.17 picked up a high advisory (GHSA-2v37-7h3g-55p8, custom generators loop indefinitely on size zero). Transitive, and the fix is a patch bump to 3.3.18. type-check and the unit suite pass on it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NprUR1djEf2woAnod3eAzS
The previous round narrowed it usefully: the row is active, its mime_type is image/png, and no AppError was persisted. So the task did not skip on either guard and did not raise -- create_thumbnail ran and the file was not where the lookup went. `thumbnail_path is None` still cannot say whether nothing was written or something was written elsewhere, and the paths are computed twice from settings read at call time (apps/job/tasks.py:105 and the JobFile property), so "elsewhere" is a real possibility rather than a formality. Assert the source file and the thumbnail directory's contents, which distinguishes them. Still intermittent -- it passed one CI run and failed the next -- so this is not a fix. It is a genuine defect either way: the endpoint promises a thumbnail and sometimes does not produce one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NprUR1djEf2woAnod3eAzS
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/job/tests/test_job_files_api.py (1)
151-155: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the CI-history narration from the test comment.
Keep the reason that
thumbnail_path is Nonecannot identify the failing task state. Removethat ambiguity cost a CI round-trip; it is review history, not a runtime constraint.As per coding guidelines, comments must record the rejected obvious alternative and the factual constraint that rejected it, not 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 `@apps/job/tests/test_job_files_api.py` around lines 151 - 155, Update the comment preceding the task input assertions to retain only the factual ambiguity of thumbnail_path being None and the task behaviors that cause it. Remove the CI round-trip narration while preserving the explanation that this value cannot distinguish skipped, unexecuted, or failed states.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_job_files_api.py`:
- Line 158: Update the test around _upload to scope AppError validation to this
upload: capture existing AppError IDs before uploading and assert no new
AppError exists afterward, or filter by the upload’s job_id when reliably
available. Replace the full-table values_list comparison with an efficient
.exists() check while preserving the successful thumbnail-generation assertion.
---
Nitpick comments:
In `@apps/job/tests/test_job_files_api.py`:
- Around line 151-155: Update the comment preceding the task input assertions to
retain only the factual ambiguity of thumbnail_path being None and the task
behaviors that cause it. Remove the CI round-trip narration while preserving the
explanation that this value cannot distinguish skipped, unexecuted, or failed
states.
🪄 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: 21bb00f1-a203-4855-aa0a-173e4451a9cf
⛔ Files ignored due to path filters (2)
frontend/package-lock.jsonis excluded by!**/package-lock.jsonfrontend/src/api/generated/zod.gen.tsis excluded by!**/generated/**
📒 Files selected for processing (29)
apps/accounting/tests/test_kpi_calendar_api.pyapps/accounting/tests/test_payroll_reconciliation_api.pyapps/accounting/tests/test_payroll_reconciliation_service.pyapps/accounting/tests/test_sales_pipeline_api.pyapps/accounting/tests/test_sales_pipeline_service.pyapps/accounting/tests/test_staff_performance_api.pyapps/company/tests/job_fixtures.pyapps/company/tests/test_company_api.pyapps/core/api.pyapps/core/schemas.pyapps/crm/tests/test_phone_call_api.pyapps/crm/tests/test_phone_call_service.pyapps/job/tests/conftest.pyapps/job/tests/test_job_api.pyapps/job/tests/test_job_files_api.pyapps/job/tests/test_kanban_api.pyapps/job/tests/test_kanban_service.pyapps/job/tests/test_month_end_api.pyapps/job/tests/test_month_end_automation.pyapps/purchasing/tests/conftest.pyapps/purchasing/tests/test_costline_approve_seam.pyapps/quoting/tests/conftest.pyapps/quoting/tests/test_models.pyapps/timesheet/tests/conftest.pyapps/xero/tests/test_pay_item_api.pyconftest.pydocs/rewrite-status.mdfrontend/schema.v2.ymlscripts/v1-frontend-operations.yml
💤 Files with no reviewable changes (9)
- apps/job/tests/test_month_end_api.py
- apps/job/tests/test_month_end_automation.py
- apps/accounting/tests/test_payroll_reconciliation_api.py
- apps/job/tests/test_job_api.py
- apps/quoting/tests/conftest.py
- apps/accounting/tests/test_sales_pipeline_service.py
- apps/accounting/tests/test_kpi_calendar_api.py
- apps/accounting/tests/test_sales_pipeline_api.py
- frontend/schema.v2.yml
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/xero/tests/test_pay_item_api.py
- scripts/v1-frontend-operations.yml
- apps/company/tests/test_company_api.py
- apps/core/api.py
- docs/rewrite-status.md
| # (apps/job/tasks.py:93-99) and persists anything it raises. | ||
| assert job_file.status == "active" | ||
| assert job_file.mime_type == "image/png" | ||
| assert list(AppError.objects.values_list("message", flat=True)) == [] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Scope the AppError assertion to this upload.
Line [158] requires the entire persisted AppError table to be empty. An unrelated fixture or setup error can fail this test even when thumbnail generation succeeds. Capture AppError IDs before _upload and assert that no new row exists, or filter by job_id if the task always stores it. Use .exists() instead of materializing every message.
🤖 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/job/tests/test_job_files_api.py` at line 158, Update the test around
_upload to scope AppError validation to this upload: capture existing AppError
IDs before uploading and assert no new AppError exists afterward, or filter by
the upload’s job_id when reliably available. Replace the full-table values_list
comparison with an efficient .exists() check while preserving the successful
thumbnail-generation assertion.
91.12 -> 91.30. The thumbnail diagnostic exercises apps/core/errors.py paths that nothing else reached. Coverage is the one row the status table cannot derive on its own -- it needs a --cov run to have happened -- so it is the one that goes stale by hand. The gate catching it is the gate working. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NprUR1djEf2woAnod3eAzS
Skipping this one, on both halves. Scoping: it is already scoped.
That is the whole point here: the previous failure mode was The cost is one query against an empty table, in a test. |
test_image_upload_creates_thumbnail_and_serves_it uploaded a PNG and asserted the thumbnail existed by the time the response returned. That only holds under CELERY_TASK_ALWAYS_EAGER, where .delay() runs the task body inline. It is a property of the test settings, not of the product: in production the endpoint publishes to Redis and returns, and the thumbnail appears when a worker gets to it. The test pinned behaviour the real system does not have. It also failed in CI about two runs in five while passing every time locally, and I chased that by bolting diagnostics onto it -- an AppError assertion, then a directory listing, then very nearly a check on the task's own log messages. The last would have asserted implementation text, which CLAUDE.md says to drop. None of them made the codebase better; they made a bad test more detailed. Three behaviours, each deterministic and each worth guarding: - the task, given an image on disk, writes a JPEG thumbnail. Called directly, no HTTP and no dispatch semantics, so nothing about queueing can make it flake. - the upload endpoint queues generation for an image and not for a text file. Its actual obligation, asserted by capturing .delay. - a generated thumbnail is served as image/jpeg and appears in the file list. This removes the flake as a side effect. Worth being explicit that it does so by deleting a test that could not have been right rather than by finding what made it intermittent, which I never established -- the source file was present, no AppError was persisted, and the thumbnail directory was empty, so the task did not reach its write. If the same cause bites production it will surface as a missing thumbnail, and the task test above is where it would then be pinned. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NprUR1djEf2woAnod3eAzS
1296 -> 1298 (one eager-dependent test became three deterministic ones) and 91.30 -> 91.31. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NprUR1djEf2woAnod3eAzS
The thumbnail split added two functions and one docstring mention. Both rows are generated; the gate catching them is the gate working. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NprUR1djEf2woAnod3eAzS
#37 squash-merged, so every file it touched conflicted with this branch's rebase of it. Resolved by origin: the schema vocabulary, the gate and its tests are this branch's work built on top of #37; test_job_files_api.py is #37's, which changed after the rebase. The OpenAPI schema, the generated client and both metrics files are regenerated rather than merged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NprUR1djEf2woAnod3eAzS
Four operations load on every page, and
tests/fixtures/auth.tsfails a test on any unexpected console error — so until they exist no spec can pass and every failure looks like a different bug. Three are done here.GET /api/data-versions/Four dataset version strings (
stock,kanban,kanban_related,crm_calls) the SPA polls on tab-focus to invalidate only the store that changed.Lives in
apps/operations, not besidebuild-idinapps/corewhere it belongs by subject — every provider reads a domain model and core sits below the domain apps. Reaching them through the app registry would work and would be wrong: that seam is for core→integration, and using it here would hide a real dependency to dodge the layer contract.The row count paired with each
Max(updated_at)is load-bearing. Deleting the newest row leaves the timestamp on an older survivor, so a timestamp-only version tells a client its cache is still good — the failure where nothing errors and the user just sees stale data. Tested directly.GET /api/xero/pay-items/Not v1's
/api/workflow/…. v2 has noworkflowapp, and CLAUDE.md keeps exact-URL parity only where an external party holds the URL. Nobody outside holds this one, so there was nothing to preserve and no reason to import a dead app's name.Three shape details that would each have broken the SPA at runtime rather than at build: bare array not a paginated envelope (
z.array(XeroPayItem)),multiplieras float not Decimal (a Decimal serialises to a JSON string and failsz.number()), andid/timestamps asUUID/datetime— I wrotestrfirst and every row 500'd.GET/PATCH /api/company-defaults/The widest blocker:
JobViewTabsrendersJobEstimateTabonly when the store is populated, so its absence darkens the whole job cluster.Derived with
ModelSchemarather than hand-listing 67 fields — v2 usesModelSchemanowhere else and hand-writes every schema, which is how a wire shape drifts from its column with nothing watching. A test asserts the derived field set equals the concrete columns.A GET never writes
The rule wasn't written down anywhere — no ADR, no CLAUDE.md line. The
idempotententries (ADR 0001, ADR 0024) are a different property; idempotent isn't safe.And the codebase breaches it:
Reading a report creates a row. Latent only because the row exists, across ~12 call sites. Fixed structurally:
get_solo()is overridden to read and raise, so every call site is correct without touching any of them.New check: routes named after a dissolved app
Answering "what would have caught the
/api/workflow/mistake". The obvious rule — every path segment names a v2 app — flagscompanies,timesheets,people,build-idanddata-versions: five of twelve, all correct. A check that's noise on its first run is one people learn to ignore.The narrow rule has no false positives: v1 had exactly two apps v2 dissolved (
workflow,client), and no route may name either. Proven by reintroducing the exact mistake — it names the path and exits 1.Remaining
workflow_notebook_lm_links_menu_list, which belongs under/api/ai/for the same reason as the pay items.🤖 Generated with Claude Code
https://claude.ai/code/session_01NprUR1djEf2woAnod3eAzS
Summary by CodeRabbit