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

feat(ci): pre-commit FactId validation hook (#393) #19

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-13 17:18:11 +01:00
Owner
Copy Link
Copy Source

Summary

Sprint 5 CI delivery — pre-commit hook that validates fact_id references in content YAML against canonical knowledge catalogs.

  • tooling/check-fact-ids — grep-based validation script (<2s runtime)
    • Advisory mode when knowledge catalogs are stubs (exit 0, lists 28 referenced fact_ids)
    • Enforcing mode when catalogs are populated (exit 1, shows file:line for unknown fact_ids)
  • .config/hooks/pre-commit — modular hook dispatcher (extensible for future checks)
  • Makefile — check-fact-ids + setup-hooks targets, wired into make setup
  • docs/DEVOPS.md — content validation and pre-commit hooks documentation

Test plan

  • make check-fact-ids runs in advisory mode (catalogs are stubs)
  • Enforcing mode tested with temporary canonical fact — correctly flags 27/28 unknown fact_ids
  • Hook fires on git commit (verified by commit output)
  • make setup-hooks sets core.hooksPath correctly
## Summary Sprint 5 CI delivery — pre-commit hook that validates `fact_id` references in content YAML against canonical knowledge catalogs. - **`tooling/check-fact-ids`** — grep-based validation script (<2s runtime) - Advisory mode when knowledge catalogs are stubs (exit 0, lists 28 referenced fact_ids) - Enforcing mode when catalogs are populated (exit 1, shows file:line for unknown fact_ids) - **`.config/hooks/pre-commit`** — modular hook dispatcher (extensible for future checks) - **Makefile** — `check-fact-ids` + `setup-hooks` targets, wired into `make setup` - **docs/DEVOPS.md** — content validation and pre-commit hooks documentation ## Test plan - [x] `make check-fact-ids` runs in advisory mode (catalogs are stubs) - [x] Enforcing mode tested with temporary canonical fact — correctly flags 27/28 unknown fact_ids - [x] Hook fires on `git commit` (verified by commit output) - [x] `make setup-hooks` sets `core.hooksPath` correctly
jpmschweitzer added 2 commits 2026-02-13 17:18:11 +01:00
feat(ci): add pre-commit FactId validation hook (#393) fe204aa5f4 
Grep-based pre-commit check validating fact_id references in content
YAML against canonical knowledge catalogs. Runs in advisory mode when
catalogs are stubs (exit 0), switches to enforcing mode once populated
(exit 1 on unknown fact_ids with file:line output).

- tooling/check-fact-ids: core validation script (<2s runtime)
- .config/hooks/pre-commit: hook dispatcher for modular checks
- Makefile: check-fact-ids + setup-hooks targets, wired into setup
- docs/DEVOPS.md: content validation and pre-commit hooks sections

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

Review: ci -> main (PR #19)

Hoshe (Code Quality): REQUEST_CHANGES

Grep-based validation and modular hook infrastructure are well-designed. Two-mode (advisory/enforcing) is pragmatic. However, the fact_id extraction has bugs that will cause false negatives.

# File Severity Issue
1 tooling/check-fact-ids:21-28 critical Comment lines (# fact_id: ...) not filtered — grep matches them, creating phantom canonical IDs
2 tooling/check-fact-ids:21-28 critical Trailing whitespace not trimmed — grep -qxF exact match fails when YAML has trailing spaces
3 .config/hooks/pre-commit:15-20 warning Missing scripts silently skipped — could hide misconfiguration after fresh clone

Tyre (Architecture): APPROVE

Architecturally sound. Aligns with D-030 (layer-0 validation), D-041 (FactId schema). Two-mode design is correct.

# File Severity Issue
1 tooling/check-fact-ids:19-28 warning Add whitespace trimming to sed pipeline
2 docs/DEVOPS.md suggestion Consider documenting hook bypass for emergencies

Verdict: CHANGES REQUESTED

Fix: add comment-line filtering (grep -v '^\s*#') and whitespace trimming to the sed pipeline in tooling/check-fact-ids.

## Review: ci -> main (PR #19) ### Hoshe (Code Quality): REQUEST_CHANGES Grep-based validation and modular hook infrastructure are well-designed. Two-mode (advisory/enforcing) is pragmatic. However, the fact_id extraction has bugs that will cause false negatives. | # | File | Severity | Issue | |---|------|----------|-------| | 1 | tooling/check-fact-ids:21-28 | critical | Comment lines (# fact_id: ...) not filtered — grep matches them, creating phantom canonical IDs | | 2 | tooling/check-fact-ids:21-28 | critical | Trailing whitespace not trimmed — grep -qxF exact match fails when YAML has trailing spaces | | 3 | .config/hooks/pre-commit:15-20 | warning | Missing scripts silently skipped — could hide misconfiguration after fresh clone | ### Tyre (Architecture): APPROVE Architecturally sound. Aligns with D-030 (layer-0 validation), D-041 (FactId schema). Two-mode design is correct. | # | File | Severity | Issue | |---|------|----------|-------| | 1 | tooling/check-fact-ids:19-28 | warning | Add whitespace trimming to sed pipeline | | 2 | docs/DEVOPS.md | suggestion | Consider documenting hook bypass for emergencies | ### Verdict: CHANGES REQUESTED Fix: add comment-line filtering (grep -v '^\s*#') and whitespace trimming to the sed pipeline in tooling/check-fact-ids.
jpmschweitzer added 1 commit 2026-02-13 17:42:32 +01:00
fix(ci): address PR review — filter comments, trim whitespace, warn on missing scripts 163de8bf25 
- Filter YAML comment lines (grep -v '^\s*#') from both extraction
  pipelines to prevent phantom canonical IDs
- Trim trailing whitespace from extracted fact_ids so grep -qxF exact
  match works reliably
- Pre-commit dispatcher now prints explicit warning when a check script
  is missing instead of silently skipping
- Document --no-verify bypass for emergencies in DEVOPS.md

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jpmschweitzer commented 2026-02-13 17:42:41 +01:00
Author
Owner
Copy Link
Copy Source

All review items addressed in 163de8b:

  1. Comment line filtering (critical) — added grep -v '^\s*#' to both canonical and reference extraction pipelines
  2. Trailing whitespace trimming (critical) — added sed 's/[[:space:]]*$//' so grep -qxF exact match works reliably
  3. Missing script warning (warning) — dispatcher now prints explicit warning with remediation hint instead of silently skipping
  4. Hook bypass docs (suggestion) — documented git commit --no-verify in DEVOPS.md
All review items addressed in 163de8b: 1. **Comment line filtering** (critical) — added `grep -v '^\s*#'` to both canonical and reference extraction pipelines 2. **Trailing whitespace trimming** (critical) — added `sed 's/[[:space:]]*$//'` so `grep -qxF` exact match works reliably 3. **Missing script warning** (warning) — dispatcher now prints explicit warning with remediation hint instead of silently skipping 4. **Hook bypass docs** (suggestion) — documented `git commit --no-verify` in DEVOPS.md
jpmschweitzer commented 2026-02-13 18:10:01 +01:00
Author
Owner
Copy Link
Copy Source

Re-review: ci -> main (PR #19)

Hoshe (Code Quality): APPROVE

All three original issues fixed correctly in 163de8b. Both pipelines have symmetric comment filtering and whitespace trimming. Exact match logic reliable. Clear actionable warnings on missing scripts. Hook bypass documented.

Tyre (Architecture): APPROVE

Aligns with D-030 (layer-0 validation), D-041 (FactId schema). No new concerns.

Verdict: APPROVED

## Re-review: ci -> main (PR #19) ### Hoshe (Code Quality): APPROVE All three original issues fixed correctly in 163de8b. Both pipelines have symmetric comment filtering and whitespace trimming. Exact match logic reliable. Clear actionable warnings on missing scripts. Hook bypass documented. ### Tyre (Architecture): APPROVE Aligns with D-030 (layer-0 validation), D-041 (FactId schema). No new concerns. ### Verdict: APPROVED
jpmschweitzer closed this pull request 2026-02-13 18:10:33 +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#19