Skip to content

feat(tasks): add vBRIEF validation tooling for centric document model (#333)#344

Merged
MScottAdams merged 6 commits intophase2/vbrief-cutoverfrom
agent6/feat/333-vbrief-validation
Apr 13, 2026
Merged

feat(tasks): add vBRIEF validation tooling for centric document model (#333)#344
MScottAdams merged 6 commits intophase2/vbrief-cutoverfrom
agent6/feat/333-vbrief-validation

Conversation

@MScottAdams
Copy link
Copy Markdown
Collaborator

Summary

Add vBRIEF validation tooling for the vBRIEF-centric document model, replacing and extending the current spec_validate.py for the new lifecycle folder structure.

Part of #309. Closes #333. Tracking: #338.

Changes

New files

  • scripts/vbrief_validate.py -- Main validation script with 6 validators:
    • Scope vBRIEF schema validation (vBRIEFInfo.version, plan.title/status/items, status enum, narratives)
    • Filename convention check (YYYY-MM-DD-descriptive-slug.vbrief.json per D7)
    • Folder/status consistency (D2/D13 status-to-folder mapping)
    • PROJECT-DEFINITION.vbrief.json validation (D3 -- narratives keys, items registry references)
    • Epic-story bidirectional link validation (D4 -- forward references + planRef back-references)
    • Origin provenance warnings (D11 -- pending/active without origin type)
  • tasks/vbrief.yml -- Task file with vbrief:validate task
  • tests/cli/test_vbrief_validate.py -- 43 subprocess-based tests covering all validators

Modified files

  • Taskfile.yml -- Added vbrief include and vbrief:validate as dependency of task check
  • CHANGELOG.md -- Added entry under [Unreleased]

Checklist

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 13, 2026

Greptile Summary

This PR introduces scripts/vbrief_validate.py, a six-validator script for the vBRIEF-centric lifecycle folder model (D2, D3, D4, D7, D11), wired into task check via tasks/vbrief.yml and covered by 43 subprocess-based tests. All P0/P1 findings from the previous review cycle (path traversal via ../ sequences, planRef non-string crash, D4 backward-check ref-type filtering, D11 substring match overreach) are resolved in the batch of fix commits on this branch.

Confidence Score: 5/5

Safe to merge — all prior P0/P1 findings are resolved; remaining findings are P2 edge-case gaps.

Every previously-flagged critical issue (path traversal, planRef type crash, D4 ref-type filter asymmetry, D11 substring overreach) has a corresponding fix commit. The two remaining findings are P2: a silent D4 skip for out-of-lifecycle-folder references and a missing test for item-level planRef. Neither blocks the primary validation path.

scripts/vbrief_validate.py lines 337–383 (D4 silent-skip branch) and tests/cli/test_vbrief_validate.py (item-level planRef coverage gap)

Important Files Changed

Filename Overview
scripts/vbrief_validate.py Main validation script with 6 validators; all previously flagged P0/P1 issues (path traversal, planRef type guard, D4 ref-type filtering, D11 substring match) are resolved in the latest commits
tests/cli/test_vbrief_validate.py 43 subprocess-based tests covering all six validators; item-level planRef path in D4 is the one untested branch
tasks/vbrief.yml Single-task file wiring vbrief_validate.py; PYTHONUTF8=1 correctly set for Windows cp1252 safety
Taskfile.yml Adds vbrief include (optional: true) and wires vbrief:validate as a dependency of task check; consistent with existing optional-include pattern
CHANGELOG.md Entry added under [Unreleased] with accurate description of all six validators and acceptance criteria references

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[task vbrief:validate] --> B[validate_all]
    B --> C[discover_vbriefs\nlifecycle folders only]
    C --> D{For each scope file}
    D --> E[load_vbrief]
    E --> F[validate_vbrief_schema\nD-schema]
    F --> G[validate_filename\nD7]
    G --> H[validate_folder_status\nD2/D13]
    H --> I[validate_origin_provenance\nD11 warnings]
    B --> J{PROJECT-DEFINITION\nexists?}
    J -->|yes| K[load + validate_vbrief_schema]
    K --> L[validate_project_definition\nD3: narratives + item refs]
    B --> M{any vBRIEFs loaded?}
    M -->|yes| N[validate_epic_story_links\nD4: forward + backward]
    N --> O{child in\nall_vbriefs?}
    O -->|yes| P[_has_plan_ref_to\nback-ref check]
    O -->|exists, not loaded| Q[silent skip]
    O -->|not exists| R[D4 error]
    B --> S{errors?}
    S -->|yes| T[exit 1]
    S -->|no| U[exit 0 + warnings]
Loading
Prompt To Fix All With AI
This is a comment left during a code review.
Path: scripts/vbrief_validate.py
Line: 337-383

Comment:
**D4 bidirectional check silently skips when referenced file exists but isn't loaded**

Both the forward check (line 338) and the backward check (implicit else around line 379) silently skip D4 validation when a target path `.exists()` on disk but is absent from `all_vbriefs`. The most practical trigger is a scope vBRIEF that references a file outside the five lifecycle folders (e.g. `vbrief/archive/old-story.vbrief.json`). In that case the epic's "child has no planRef back" check and the story's "parent doesn't list this file" check are both skipped, giving a false-valid result.

The forward check already has an inline comment; the backward path has no comment and no symmetrical guard. At minimum, adding an error for the out-of-scope case would prevent silent false negatives:

```python
elif parent_path and parent_path.exists():
    errors.append(
        f"{fp_display}: planRef references '{plan_ref}' which exists "
        "but is outside lifecycle folders — D4 check skipped (D4)"
    )
```

Or, if out-of-scope references are intended to be allowed, adding a comment parallel to the forward-check one would make the intent clear.

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

---

This is a comment left during a code review.
Path: tests/cli/test_vbrief_validate.py
Line: 454-626

Comment:
**Item-level `planRef` path is implemented but untested**

Both `_collect_plan_refs` and `_has_plan_ref_to` specifically support `planRef` set on a top-level `plan.items[*]` entry (not just the root `plan.planRef`). No test in `TestEpicStoryLinks` exercises this code path — all tests only set `plan_ref` at the plan root via `minimal_vbrief(plan_ref=...)`.

A story that carries its parent reference at item level would silently bypass backward D4 checks if this path regresses. Adding one positive and one negative case would close the gap:

```python
def test_item_level_planref_satisfies_d4(self, tmp_path):
    """planRef on a plan item (not the plan root) satisfies D4 back-reference."""
    vbrief_dir = tmp_path / "vbrief"
    make_lifecycle_dirs(vbrief_dir)
    epic_path = "active/2026-04-13-epic.vbrief.json"
    story_path = "active/2026-04-13-story.vbrief.json"
    write_vbrief(
        vbrief_dir / epic_path,
        minimal_vbrief(
            title="Epic", status="running",
            references=[{"uri": story_path, "type": "x-vbrief/plan"}],
        ),
    )
    story = minimal_vbrief(title="Story", status="running")
    story["plan"]["items"][0]["planRef"] = epic_path  # item-level planRef
    write_vbrief(vbrief_dir / story_path, story)
    result = run_validator(vbrief_dir)
    assert result.returncode == 0
```

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

Reviews (7): Last reviewed commit: "fix: filter D4 backward-check to x-vbrie..." | Re-trigger Greptile

Comment thread scripts/vbrief_validate.py Outdated
Comment thread scripts/vbrief_validate.py
Comment thread scripts/vbrief_validate.py
Comment thread scripts/vbrief_validate.py
…#333)

- Add scripts/vbrief_validate.py with 6 validators:
  - Scope vBRIEF schema validation (vBRIEFInfo.version, plan required fields, status enum)
  - Filename convention check (YYYY-MM-DD-descriptive-slug.vbrief.json per D7)
  - Folder/status consistency (D2/D13 status-to-folder mapping)
  - PROJECT-DEFINITION.vbrief.json validation (D3 narratives keys, items references)
  - Epic-story bidirectional link validation (D4 forward refs + planRef back-refs)
  - Origin provenance warnings (D11 pending/active without origin type)
- Add tasks/vbrief.yml with vbrief:validate task
- Wire vbrief:validate as dependency of task check pipeline in Taskfile.yml
- Add tests/cli/test_vbrief_validate.py with 43 subprocess-based tests
- Use ASCII-safe output markers for Windows cp1252 compatibility

Part of RFC #309. Closes #333. Tracking: #338.
- Tighten D11 origin-type matching: use startswith instead of substring containment
- Eliminate redundant discover_vbriefs call: return scope_count from validate_all
- Use original paths in D4 error messages via resolved_to_original mapping
- Add directory-containment guard on D3 file references (reject ../ traversal)
- Replace str.startswith with Path.is_relative_to for D3 containment check
- Add isinstance(uri, str) guard in _resolve_ref_path to handle non-string planRef
@MScottAdams MScottAdams force-pushed the agent6/feat/333-vbrief-validation branch from 6630a51 to ad5bac0 Compare April 13, 2026 19:15
…tch)

- Add type filter in validate_epic_story_links to only check x-vbrief/plan references
  for bidirectional parent-child linking (dependency/context refs are not parent-child)
- Add regression test: non-plan reference type does not trigger D4 errors
- Extract _collect_plan_refs helper to scan plan-level and item-level planRef
- Backward D4 check now uses _collect_plan_refs for symmetry with _has_plan_ref_to
- Add regression test for extended D11 origin type variants (github-issue-v2)
Comment thread scripts/vbrief_validate.py
- Add type == x-vbrief/plan filter to backward D4 parent-refs collection
- Add docstring note to _collect_plan_refs clarifying subItems exclusion
@MScottAdams MScottAdams merged commit bb9426c into phase2/vbrief-cutover Apr 13, 2026
3 checks passed
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