jpmschweitzer/settled-reach
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

feat(ci): pre-PR verification and content cross-reference validation #27

Closed
jpmschweitzer wants to merge 0 commits from ci into main
pull from: ci
merge into: :main
Conversation 3 Commits - Files Changed -
jpmschweitzer commented 2026-02-17 22:44:44 +01:00
Owner
Copy Link
Copy Source

Summary

Sprint 8 CI team deliverables from the Test Architecture Workshop (Epic #455):

  • #460: make pre-pr target — Full pre-PR chain: lint → build → test → content validation → fixture staleness. Branch-specific variants: pre-pr-server, pre-pr-client, pre-pr-content.
  • #464: Content cross-reference validation (9 checks) — Extends tooling/validate-content with Pass 2 (ContentIndex): canonical_id uniqueness, relationship targets, location slugs, dialogue locations, fact_ids, triangle membership, npc_count, dialogue line_ids, bidirectional relationships.
  • #465: Fixture staleness check — Built into pre-pr-fixtures target. Regenerates fixtures, checks git diff, exits 1 on stale (blocker per Tyre's argument).
  • Schema fix — Added focused (mood) and greeting (situation) to dialogue-pool schema for Sprint 7-8 content.

Current validation results

  • Pass 1 (schema): 47 validated, 0 errors
  • Pass 2 (cross-references): 0 errors, 21 warnings (npc_count mismatch + missing reciprocal relationships — real content issues for copy team)

Files changed

File Change
Makefile 8 new pre-PR targets
tooling/validate-content ContentIndex class with 9 cross-reference checks
content/_schema/dialogue-pool.schema.json Added focused, greeting enum values
docs/DEVOPS.md Pre-PR checks documentation

Test plan

  • make help shows new targets
  • make --dry-run pre-pr shows correct chain
  • make validate-content runs both passes successfully
  • Cross-reference checks detect real issues (npc_count, reciprocal relationships)
  • Schema validation passes after enum fix
## Summary Sprint 8 CI team deliverables from the Test Architecture Workshop (Epic #455): - **#460: `make pre-pr` target** — Full pre-PR chain: lint → build → test → content validation → fixture staleness. Branch-specific variants: `pre-pr-server`, `pre-pr-client`, `pre-pr-content`. - **#464: Content cross-reference validation (9 checks)** — Extends `tooling/validate-content` with Pass 2 (ContentIndex): canonical_id uniqueness, relationship targets, location slugs, dialogue locations, fact_ids, triangle membership, npc_count, dialogue line_ids, bidirectional relationships. - **#465: Fixture staleness check** — Built into `pre-pr-fixtures` target. Regenerates fixtures, checks git diff, exits 1 on stale (blocker per Tyre's argument). - **Schema fix** — Added `focused` (mood) and `greeting` (situation) to dialogue-pool schema for Sprint 7-8 content. ## Current validation results - Pass 1 (schema): 47 validated, 0 errors - Pass 2 (cross-references): 0 errors, 21 warnings (npc_count mismatch + missing reciprocal relationships — real content issues for copy team) ## Files changed | File | Change | |------|--------| | `Makefile` | 8 new pre-PR targets | | `tooling/validate-content` | ContentIndex class with 9 cross-reference checks | | `content/_schema/dialogue-pool.schema.json` | Added `focused`, `greeting` enum values | | `docs/DEVOPS.md` | Pre-PR checks documentation | ## Test plan - [x] `make help` shows new targets - [x] `make --dry-run pre-pr` shows correct chain - [x] `make validate-content` runs both passes successfully - [x] Cross-reference checks detect real issues (npc_count, reciprocal relationships) - [x] Schema validation passes after enum fix
jpmschweitzer added 4 commits 2026-02-17 22:44:45 +01:00
feat(ci): add make pre-pr target and fixture staleness check f06c8be313 
Implements make pre-pr chain: lint -> build -> test -> content
validation -> fixture staleness. Branch-specific variants:
pre-pr-server, pre-pr-client, pre-pr-content.

Fixture staleness is a blocker (exit 1) — stale fixtures cause
false positive client tests. Spec from hoshe-round3.md Section 5.

Tickets: #460, #465

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
feat(ci): add content cross-reference validation (9 checks) 65d3b5e50c 
Extends tooling/validate-content with Pass 2 cross-reference
validation via ContentIndex class. Nine checks:

1. canonical_id uniqueness (ERROR)
2. relationship target resolution (ERROR)
3. location slug resolution (ERROR)
4. dialogue location resolution (ERROR)
5. fact_id resolution (ERROR, advisory when catalogs empty)
6. triangle membership resolution (ERROR)
7. npc_count accuracy (WARNING)
8. dialogue line_id uniqueness (ERROR)
9. bidirectional relationship consistency (WARNING)

Pass 2 only runs if Pass 1 (schema) passes. Absorbs check-fact-ids
functionality. Spec from hoshe-round3.md Section 2.

Ticket: #464

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
fix(schema): add missing dialogue mood and situation values 16cccda411 
Adds 'focused' to mood enum and 'greeting' to situation enum in
dialogue-pool.schema.json. Both values are used by Sprint 7-8
dialogue content but were not added to the schema at the time.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
chore(meta): update changelog 06f47747c7 
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jpmschweitzer commented 2026-02-17 23:13:01 +01:00
Author
Owner
Copy Link
Copy Source

Review: ci → main (type: code)

Hoshe (Code Quality): APPROVE

Well-structured 2-pass validation engine with 9 meaningful cross-reference checks. Clean Makefile additions following existing conventions. One dead function and one logic gap worth noting.

# File Severity Issue
1 validate-content:85-94 warning _file_type defined but never called — dead code, remove
2 validate-content:~293 warning _check_4_dialogue_locations silently passes when district_locs is empty set — dialogue referencing nonexistent location won't be caught if district has no locations declared
3 Makefile:~160 warning pre-pr-fixtures suppresses stderr with 2>/dev/null — if fixture generation fails, error is swallowed and git diff check proceeds against stale fixtures (false pass)
4 validate-content:~168 suggestion _scan_knowledge regex fallback undocumented — explain what catalog format triggers it
5 validate-content:~299 suggestion _check_5_fact_ids early return prints no message when skipping — add advisory note to output
6 dialogue-pool.schema.json suggestion greeting/focused additions lack explicit D-035 decision trace
7 docs/DEVOPS.md suggestion Example commit message uses placeholder vN — provide concrete example

Tyre (Architecture): REQUEST_CHANGES

2-pass architecture sound, branch-scoped pre-PR variants well-designed. One critical logic bug and one scoping gap need fixing.

# File Severity Issue
1 Makefile:~148 pre-pr-fixtures critical 2>/dev/null on cargo test --test gen_fixtures swallows compilation/runtime failures — fixture generation that errors produces false pass on the staleness check (the exact scenario this gate prevents)
2 validate-content:~416 _check_8 warning Line ID uniqueness check is per-file only — two dialogue files for same location with overlapping IDs would pass. Should be cross-file.
3 validate-content:~441 _check_9 suggestion Bidirectional relationship check will produce false warnings on intentional asymmetric relationships (D-034) — document that asymmetric is valid
4 dialogue-pool.schema.json:~91 suggestion focused brings mood count to 9 but D-035 says "8 moods for v0.1" — update decision or document addendum

Verdict: CHANGES REQUESTED

Key issues:

  1. pre-pr-fixtures 2>/dev/null must be removed — swallowed errors defeat the safety gate (both reviewers flagged)
  2. _check_8_dialogue_line_ids should validate cross-file, not just per-file
  3. Dead function _file_type — remove
  4. Empty district_locs bypass in check 4 — silent false negative
## Review: ci → main (type: code) ### Hoshe (Code Quality): APPROVE Well-structured 2-pass validation engine with 9 meaningful cross-reference checks. Clean Makefile additions following existing conventions. One dead function and one logic gap worth noting. | # | File | Severity | Issue | |---|------|----------|-------| | 1 | validate-content:85-94 | warning | `_file_type` defined but never called — dead code, remove | | 2 | validate-content:~293 | warning | `_check_4_dialogue_locations` silently passes when `district_locs` is empty set — dialogue referencing nonexistent location won't be caught if district has no locations declared | | 3 | Makefile:~160 | warning | `pre-pr-fixtures` suppresses stderr with `2>/dev/null` — if fixture generation fails, error is swallowed and `git diff` check proceeds against stale fixtures (false pass) | | 4 | validate-content:~168 | suggestion | `_scan_knowledge` regex fallback undocumented — explain what catalog format triggers it | | 5 | validate-content:~299 | suggestion | `_check_5_fact_ids` early return prints no message when skipping — add advisory note to output | | 6 | dialogue-pool.schema.json | suggestion | `greeting`/`focused` additions lack explicit D-035 decision trace | | 7 | docs/DEVOPS.md | suggestion | Example commit message uses placeholder `vN` — provide concrete example | ### Tyre (Architecture): REQUEST_CHANGES 2-pass architecture sound, branch-scoped pre-PR variants well-designed. One critical logic bug and one scoping gap need fixing. | # | File | Severity | Issue | |---|------|----------|-------| | 1 | Makefile:~148 `pre-pr-fixtures` | critical | `2>/dev/null` on `cargo test --test gen_fixtures` swallows compilation/runtime failures — fixture generation that errors produces false pass on the staleness check (the exact scenario this gate prevents) | | 2 | validate-content:~416 `_check_8` | warning | Line ID uniqueness check is per-file only — two dialogue files for same location with overlapping IDs would pass. Should be cross-file. | | 3 | validate-content:~441 `_check_9` | suggestion | Bidirectional relationship check will produce false warnings on intentional asymmetric relationships (D-034) — document that asymmetric is valid | | 4 | dialogue-pool.schema.json:~91 | suggestion | `focused` brings mood count to 9 but D-035 says "8 moods for v0.1" — update decision or document addendum | ### Verdict: CHANGES REQUESTED **Key issues:** 1. `pre-pr-fixtures` `2>/dev/null` must be removed — swallowed errors defeat the safety gate (both reviewers flagged) 2. `_check_8_dialogue_line_ids` should validate cross-file, not just per-file 3. Dead function `_file_type` — remove 4. Empty `district_locs` bypass in check 4 — silent false negative
jpmschweitzer added 1 commit 2026-02-18 00:23:30 +01:00
fix(ci): address PR review — filter comments, trim whitespace, warn on missing scripts 4af2197c59 
Address all review comments from Hoshe and Tyre on PR #27:
- Remove 2>/dev/null from pre-pr-fixtures (critical: swallowed errors)
- Remove dead _file_type function
- Check 4: error on districts with no locations declared
- Check 5: print advisory message when skipping
- Check 8: cross-file line ID uniqueness (not just per-file)
- Check 9: document D-034 asymmetric relationships in docstring
- Document regex fallback rationale in _scan_knowledge
- Add D-035 decision trace to schema descriptions
- Use concrete protocol version in DEVOPS.md example
- Amend D-035 with focused (9th mood) and greeting (14th situation)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jpmschweitzer commented 2026-02-18 00:24:08 +01:00
Author
Owner
Copy Link
Copy Source

All 10 review items addressed in 4af2197:

Key fixes (4):

  1. Removed 2>/dev/null from pre-pr-fixtures — compilation failures now surface correctly
  2. Removed dead _file_type function
  3. Check 4 now errors when district has empty locations (was silent pass)
  4. Check 8 now validates cross-file line ID uniqueness (was per-file only)

Suggestions (6):
5. Check 5 prints advisory message when skipping
6. Documented regex fallback rationale in _scan_knowledge
7. Check 9 docstring documents D-034 asymmetric relationships
8. Schema descriptions include D-035 decision trace
9. DEVOPS.md uses concrete example (v8) instead of placeholder
10. D-035 amendment added for focused (9th mood) and greeting (14th situation)

All 10 review items addressed in 4af2197: **Key fixes (4):** 1. Removed `2>/dev/null` from `pre-pr-fixtures` — compilation failures now surface correctly 2. Removed dead `_file_type` function 3. Check 4 now errors when district has empty locations (was silent pass) 4. Check 8 now validates cross-file line ID uniqueness (was per-file only) **Suggestions (6):** 5. Check 5 prints advisory message when skipping 6. Documented regex fallback rationale in `_scan_knowledge` 7. Check 9 docstring documents D-034 asymmetric relationships 8. Schema descriptions include D-035 decision trace 9. DEVOPS.md uses concrete example (v8) instead of placeholder 10. D-035 amendment added for `focused` (9th mood) and `greeting` (14th situation)
jpmschweitzer commented 2026-02-18 01:14:59 +01:00
Author
Owner
Copy Link
Copy Source

Review: ci → main (type: code) — Re-review after fixes

Hoshe (Code Quality): APPROVE

All 10 previous review items are correctly addressed in 4af2197. The critical error suppression bug is fixed, dead code removed, Check 4 and Check 8 logic gaps closed. Two minor new issues found — neither blocking.

# File Severity Issue
1 validate-content:~437 suggestion Check 8 cross-file duplicate fires twice when a line ID is also a per-file duplicate — over-reports, never under-reports. Add continue after per-file branch
2 decisions/content.md:D-035 suggestion Body text still says "13 situations" and "8 moods" — inconsistent with amendment (14/9). Update body or add inline note
3 validate-content:171 suggestion prefix="" parameter in _extract_fact_ids_from_dict is unused dead code — remove

Tyre (Architecture): APPROVE

All 4 previous Tyre items correctly fixed. Two-pass architecture sound, no coupling concerns, pre-pr pipeline correctly ordered. Three minor new issues.

# File Severity Issue
1 docs/DEVOPS.md:~114 suggestion Hardcoded v8 in example commit will go stale — revert to vN or v{N} template
2 Makefile + validate-content warning Script imports jsonschema and yaml but setup-tooling doesn't install them — fresh clone fails with ModuleNotFoundError. Add requirements.txt + pip install to setup
3 validate-content:~449 suggestion Check 9 docstring cites D-034 (THE FRIEND) but correct authority for asymmetric relationships is D-024 or D-041 — one-line fix

Verdict: APPROVED

All previous blocking issues resolved. 6 new items are suggestions/warnings — no blockers.

## Review: ci → main (type: code) — Re-review after fixes ### Hoshe (Code Quality): APPROVE All 10 previous review items are correctly addressed in 4af2197. The critical error suppression bug is fixed, dead code removed, Check 4 and Check 8 logic gaps closed. Two minor new issues found — neither blocking. | # | File | Severity | Issue | |---|------|----------|-------| | 1 | validate-content:~437 | suggestion | Check 8 cross-file duplicate fires twice when a line ID is also a per-file duplicate — over-reports, never under-reports. Add `continue` after per-file branch | | 2 | decisions/content.md:D-035 | suggestion | Body text still says "13 situations" and "8 moods" — inconsistent with amendment (14/9). Update body or add inline note | | 3 | validate-content:171 | suggestion | `prefix=""` parameter in `_extract_fact_ids_from_dict` is unused dead code — remove | ### Tyre (Architecture): APPROVE All 4 previous Tyre items correctly fixed. Two-pass architecture sound, no coupling concerns, pre-pr pipeline correctly ordered. Three minor new issues. | # | File | Severity | Issue | |---|------|----------|-------| | 1 | docs/DEVOPS.md:~114 | suggestion | Hardcoded `v8` in example commit will go stale — revert to `vN` or `v{N}` template | | 2 | Makefile + validate-content | warning | Script imports `jsonschema` and `yaml` but `setup-tooling` doesn't install them — fresh clone fails with `ModuleNotFoundError`. Add `requirements.txt` + pip install to setup | | 3 | validate-content:~449 | suggestion | Check 9 docstring cites D-034 (THE FRIEND) but correct authority for asymmetric relationships is D-024 or D-041 — one-line fix | ### Verdict: APPROVED All previous blocking issues resolved. 6 new items are suggestions/warnings — no blockers.
jpmschweitzer closed this pull request 2026-02-18 01:15:27 +01:00

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
No items
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
jpmschweitzer (Jeroen Schweitzer)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: jpmschweitzer/settled-reach#27