Plan Review: Publish HTML plans to GitHub Pages
Reviewed 2026-06-07 16:09 UTC · 5 core reviewers (architecture, completeness, testability, risk, conventions) · No UI signals detected
Approved with Revisions
Executive Summary: The plan is architecturally sound, well-aligned with project conventions, and appropriately scoped. All 5 reviewers approved with revisions. Three cross-reviewer concerns dominated: (1) test files described in the Tests section had no corresponding creation step, (2) Pages enablement ordering was implicit rather than enforced, and (3) no pre-deploy CI validation existed. Additional issues: hardcoded “32 plans” in acceptance criteria and underscore test filenames against the project’s kebab-case convention. All issues addressed with 12 inline edits to the source plan.
Role-by-Role Findings
Architecture Review
Fit: Architecturally sound — a thin static-site deployment layer that correctly avoids touching the existing hook-driven content pipeline.
- Path filter gap (medium): Workflow changes to
deploy-pages.yml itself wouldn’t trigger a redeploy. Fixed.
- No .nojekyll guard (low): Accidental deletion would silently re-enable Jekyll. Fixed: build job now asserts .nojekyll exists.
- Post-deploy-only smoke test (low): No pre-upload local validation. Mitigated: new step creates locally-runnable tests.
- Root redirect couples landing page (low): Changing destination requires code change. Acknowledged; deferred to Next Steps hub page.
- SHA divergence across workflows (low): No sync mechanism. Noted for future Dependabot config.
Completeness Review
Completeness: Well-structured with good specificity, but had two notable gaps around test file creation and prerequisite validation.
- Missing test file creation step (high): Tests specified but no step created them. Fixed: new Step 6 added.
- Pages enablement ordering implicit (medium): No note that Step 4 must precede merge. Fixed: explicit ordering added.
- SHA lookup guidance missing (medium): Implementer had to find SHAs independently. Fixed: references added to Step 3.
- Hardcoded “32” in AC6 (low): Would go stale. Fixed: dynamic language used.
- No rollback/recovery step (low): Addressed with troubleshooting paragraph in Verification.
Testability Review
Test coverage: Present but thin — tests existed on paper but not as executable files, with no pre-merge CI gate.
- No pre-deploy CI validation (high): Only post-deploy smoke test existed. Mitigated: .nojekyll assertion + locally-runnable test files.
- No Jekyll-suppression test (medium): Partially addressed via build-job assertion.
- AC4 not CI-falsifiable (medium): Depends on repo API state. Noted; verification section covers manual check.
- Hardcoded plan count (low): Fixed: dynamic count via find.
- No failure-mode tests (low): Addressed with troubleshooting guidance.
Risk Review
Risk level: Medium overall.
- Pages enablement prerequisite (high): Workflow fails silently if Pages not enabled. Fixed: ordering enforced in Step 4.
- Stale gallery index in CI (medium): Hook-maintained index could be stale for non-hook commits. Recommend CI-side regeneration as follow-up.
- Public content exposure (medium): Plans become public and indexable immediately. Recommend content audit before enabling.
- Concurrent deploy queue (medium): Rapid pushes can queue deploys. Acceptable with cancel-in-progress: false.
- Smoke test propagation delay (low): Fixed: retry loop specified.
- SHA staleness without Dependabot (low): Recommend Dependabot config as follow-up.
Conventions Review
Fit: Good alignment with project patterns. Workflow naming, concurrency groups, and permissions blocks all mirror existing workflows.
- Test file naming (medium): Used underscores instead of project-standard kebab-case. Fixed: all renamed to test-*.
- Missing tests/pages/ directory creation (low): Fixed: included in new Step 6.
- Path trigger syntax (low): Bracket array vs scalar form. Noted; functionally equivalent.
Agreements & Conflicts
Confirmed concerns (flagged by 2+ reviewers independently):
- Missing test file creation step (Completeness + Testability)
- Pages enablement ordering must be explicit (Completeness + Risk)
- No pre-deploy CI validation (Architecture + Testability)
- Hardcoded plan count is fragile (Completeness + Testability)
- Workflow path filter should include itself (Architecture + Risk)
Conflicts: None. All reviewers aligned on recommendations.
Highest-Risk Issues
1. Pages must be enabled before merge — Risk reviewer (high). The workflow fails silently if Pages is not enabled when it first triggers. Fixed: Step 4 now requires completion before PR merge, with admin access fallback guidance.
2. Test files must be created, not just described — Completeness reviewer (high). Three test scripts were specified in the Tests section with no corresponding creation step. Fixed: new Step 6 creates all three files with specific assertions.
3. No pre-deploy CI gate — Testability reviewer (high). A broken artifact could reach production with no pre-merge check. Mitigated: .nojekyll build assertion + locally-runnable unit/integration tests.
4. Public content exposure is irreversible — Risk reviewer (medium). Plans become publicly accessible and search-indexable immediately after first deploy. Recommend content audit before enabling Pages.
5. Gallery index staleness in CI — Risk reviewer (medium). The hook-maintained index may be stale for commits made without the PostToolUse hook. Recommend adding CI-side index regeneration before upload-pages-artifact.
Edits Applied to Source Plan
| Target | Action | Change |
|---|
| AC6 label | edit | Replaced “All 32 plan files” with “All plan files present in docs/plans/ at deploy time” |
| Step 3 why | edit | Added workflow self-trigger path, .nojekyll assertion, SHA lookup guidance |
| Step 3 verify | edit | Added path-filter and .nojekyll assertion checks |
| Step 4 why | edit | Added “must be completed before PR is merged” and admin access note |
| After Step 5 | insert | New Step 6: Create test files in tests/pages/ |
| Old Step 6 | edit | Renumbered to Step 7 |
| All test filenames | edit | Renamed from underscore to kebab-case (test_* to test-*) |
| Objective test | edit | Dynamic plan count + retry loop for propagation delay |
| Verification | edit | Removed hardcoded “32”; added troubleshooting paragraph |
| AC1 label | edit | Added workflow self-trigger and .nojekyll assertion mentions |
| Criteria list | append | New AC8: test scripts exist and pass locally |
| Progress label | edit | Updated from 0/7 to 0/8 |