Skip to content

fix(lib): align Python implementation with v0.6 spec#8

Open
visionik wants to merge 1 commit into
masterfrom
fix/v06-python-implementation
Open

fix(lib): align Python implementation with v0.6 spec#8
visionik wants to merge 1 commit into
masterfrom
fix/v06-python-implementation

Conversation

@visionik
Copy link
Copy Markdown
Collaborator

@visionik visionik commented Apr 5, 2026

Summary

Addresses all four P1 defects identified by Greptile in the review of PR #4, plus the P2 PROJECT.md typo.

Defects fixed

P1: compat/policy.py"failed" missing from VALID_STATUSES

The headline v0.6 status caused validation errors and ValueError in strict-mode builder calls. Added "failed" to the set.

P1: validation.py — validator rejects all v0.6 documents

The version check compared against the literal string "0.5". Introduced a SUPPORTED_VERSIONS frozenset ({"0.5", "0.6"}) and updated the check and error message accordingly.

P1: validation.py_validate_items ignores v0.6 items key on nested children

The validator only inspected subItems. Updated to prefer the items key when present, falling back to subItems, and routing the recursive call and error paths through the detected key name.

P1: models.pyPlanItem.from_dict() silently drops v0.6 items key

  • Added "items" to _PLAN_ITEM_FIELD_ORDER so it is not silently captured into extras
  • Updated from_dict to prefer items over subItems when both or either are present
  • Updated _known_item_values to emit "items" (v0.6 preferred) on serialization
  • Added PlanItem.failed factory via _StatusFactory

P1: builder.py — all three helpers hardcode version: "0.5"

to_document(), quick_todo(), and from_items() all emitted {"version": "0.5"}. Updated to "0.6".

P2: PROJECT.md — references TypeScript instead of Python

Fixed header and tech stack line.

Tests

141 passed. New tests added covering:

  • v0.6 document accepted by validator
  • "failed" status accepted at plan and item level
  • items key on PlanItem traversed by _validate_items
  • PlanItem.failed() factory produces correct status
  • Builder version string is "0.6" across all three helpers
  • Nested children serialized under items key (not subItems)
  • items key preferred over subItems when both present on parse

Four P1 defects identified in the Greptile review of PR #4:

- compat/policy.py: add 'failed' to VALID_STATUSES; was missing despite
  being the headline v0.6 status, causing validation errors and strict-
  mode raises for any item with status='failed'

- validation.py: introduce SUPPORTED_VERSIONS frozenset {'0.5','0.6'};
  update version check to accept both; update _validate_items to traverse
  nested children under either 'items' (v0.6 preferred) or 'subItems'
  (legacy compat) using nested_key selection

- models.py: add 'items' to _PLAN_ITEM_FIELD_ORDER so v0.6 nested
  children are not silently dropped into extras; update PlanItem.from_dict
  to prefer 'items' key over 'subItems'; update _known_item_values to emit
  'items' (v0.6 preferred) on write; add PlanItem.failed factory

- builder.py: replace hardcoded {'version':'0.5'} with '0.6' in all three
  document-producing functions (to_document, quick_todo, from_items)

Also fix PROJECT.md which referenced TypeScript instead of Python (P2).

Tests: 141 passed. Added explicit coverage for v0.6 document acceptance,
'failed' status, 'items' key traversal in validator, PlanItem.failed
factory, builder version string, and nested-items serialization key.
@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Apr 5, 2026

Greptile Summary

This PR correctly resolves all four P1 defects from the PR #4 review: "failed" is added to VALID_STATUSES, the version check in validation.py now accepts both "0.5" and "0.6" via a SUPPORTED_VERSIONS frozenset, _validate_items correctly traverses the v0.6 items key on nested children, PlanItem.from_dict prefers the v0.6 items key and PlanItem.failed factory is added, and all three builder helpers emit version: "0.6". Two minor P2 observations remain: the invalid_subitems_type error code is reused when the v0.6 items key is not a list (semantically imprecise for consumers checking the code string), and the inline classmethods for pending/running/blocked/cancelled/draft in the PlanItem class body are dead code since they are unconditionally replaced by _StatusFactory descriptors at module level.

Confidence Score: 5/5

Safe to merge — all four prior P1 defects are correctly fixed and covered by 141 passing tests.

All P1 issues from the previous review are resolved with correct implementations. The two remaining findings are P2 style observations (dead code and a naming imprecision in an error code) with no runtime impact.

No files require special attention.

Important Files Changed

Filename Overview
libvbrief/compat/policy.py Added 'failed' to VALID_STATUSES — straightforward, no issues.
libvbrief/validation.py SUPPORTED_VERSIONS frozenset added; _validate_items updated to prefer v0.6 'items' key — correct, with minor naming inconsistency where ISSUE_INVALID_SUBITEMS_TYPE is reused for 'items' key errors.
libvbrief/models.py PlanItem updated to parse/serialize 'items' key, 'failed' factory added via _StatusFactory — runtime correct; inline classmethods in class body are dead code.
libvbrief/builder.py All three document helpers updated to emit version '0.6' — correct.
tests/test_models_api.py New tests cover all P1 fixes: v0.6 'items' key preference, 'failed' factory, version string, and nested serialization key.
tests/test_validation.py New tests cover v0.6 document acceptance, 'failed' status, items key traversal, and unsupported version rejection.
tests/test-e2e.py End-to-end workflows pass with v0.6 version string and updated factory usage.
tests/test-fuzz.py PlanItem.failed added to factory fuzz coverage — no issues.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Input document dict] --> B{Item has 'items' key?}
    B -->|Yes - v0.6| C[Use 'items' for nested children]
    B -->|No - legacy v0.5| D[Fall back to 'subItems']
    C --> E[PlanItem.from_dict stores in .subItems field]
    D --> E
    E --> F[_validate_items validates nested children]
    F --> G{to_dict serialize}
    G --> H[_known_item_values always emits 'items' key]
    H --> I[Output always uses v0.6 'items' key]

    J[Builder / quick_todo / from_items] --> K[vBRIEFInfo version: '0.6']
    K --> I

    L[VALID_STATUSES] --> M[Now includes 'failed']
    M --> N[Validator accepts 'failed' status]
    M --> O[Builder strict mode accepts 'failed']

    P[SUPPORTED_VERSIONS] --> Q[frozenset of '0.5' and '0.6']
    Q --> R[Validator accepts both versions]
Loading
Prompt To Fix All With AI
This is a comment left during a code review.
Path: libvbrief/validation.py
Line: 207-211

Comment:
**`ISSUE_INVALID_SUBITEMS_TYPE` reused for v0.6 `items` key errors**

When the v0.6 `items` key is present but not a list, the error emitted uses `ISSUE_INVALID_SUBITEMS_TYPE` (`"invalid_subitems_type"`). The `path` and `message` are both correct, but any consumer doing `issue.code == "invalid_subitems_type"` and expecting it to refer to the legacy `subItems` key will see this code for a v0.6 `items` violation too. A dedicated constant like `ISSUE_INVALID_ITEMS_TYPE` in `compat/policy.py` would make the signal unambiguous.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: libvbrief/models.py
Line: 107-133

Comment:
**Inline classmethods are dead code superseded by `_StatusFactory`**

The `@classmethod` definitions for `pending`, `running`, `blocked`, `cancelled`, and `draft` inside the class body (lines 108–133) are unconditionally overwritten by `_StatusFactory` descriptors at module level (lines 499–505). The inline definitions are never called and can be removed without any behavioral change, which would reduce confusion about which path is actually executed.

How can I resolve this? If you propose a fix, please make it concise.

Reviews (1): Last reviewed commit: "fix(lib): align Python implementation wi..." | Re-trigger Greptile

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant