Skip to content

Add advisory for phases with 31-50 requirements (#10)#14

Merged
jcmaker merged 2 commits into
jcmaker:mainfrom
Diyaaa-12:feat/phase-requirements-advisory
Jul 7, 2026
Merged

Add advisory for phases with 31-50 requirements (#10)#14
jcmaker merged 2 commits into
jcmaker:mainfrom
Diyaaa-12:feat/phase-requirements-advisory

Conversation

@Diyaaa-12

@Diyaaa-12 Diyaaa-12 commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

What & why

Adds a soft advisory (not a hard fail) when a PRD phase has 31–50
requirements, nudging authors toward the recommended <= 30 per phase without
blocking the build. CHECK 5 in validate_prd.py still hard-fails above 50 as
before — only the 31–50 range now prints an ADVISORY line while keeping exit
code 0.

Also fixes a pre-existing cross-platform bug surfaced while testing on
Windows: validate_prd.py crashed with UnicodeEncodeError when printing
Korean PRD content to a Windows console (cp1252 default encoding). Added
sys.stdout.reconfigure(encoding="utf-8") / sys.stderr.reconfigure(...) at
the top of the script, and set encoding="utf-8" on the subprocess.run call
in the test helper so the test suite doesn't hit the same issue capturing
output on Windows.

Closes #10

Checks

  • Ran the three local checks and they pass:
    • python3 skills/prd-maker/scripts/test_validate_prd.py
    • python3 .github/scripts/check_skill_structure.py
    • python3 .github/scripts/check_manifests.py
  • Updated docs (README.md / README.ko.md) if usage or behavior changed — not touched; this only changes internal linter output, no user-facing usage change
  • If I bumped the version, I bumped it in both .claude-plugin/plugin.json and .codex-plugin/plugin.json — no version bump made, not touched

Notes for reviewers

  • No docs update needed — this only changes internal linter output
    (an advisory line), not the PRD template or user-facing CLI usage. Happy to
    add a note to README if you'd prefer it documented.
  • check5_requirements_cap() now returns a 3-tuple (passed, detail, advisories) instead of 2. Updated the one caller in run_checks()
    accordingly — grep confirms no other callers.
  • Flagging the Windows Unicode fix separately in case you'd rather split it
    into its own PR — it's unrelated to the advisory feature but I hit it while
    running the required local checks and didn't want to leave the linter
    broken on Windows.

@Diyaaa-12 Diyaaa-12 requested a review from jcmaker as a code owner July 7, 2026 18:22

@jcmaker jcmaker left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this — really solid work! 🙏 I ran everything locally and the feature itself is in great shape:

  • ✅ All 19 unit tests pass (test_validate_prd.py)
  • check_skill_structure.py and check_manifests.py both pass
  • ✅ Verified the advisory end-to-end: a phase with 35 requirements keeps exit code 0, CHECK 5 still PASS, and the ADVISORY - Phase "..." has 35 requirements (recommended <= 30) line prints as intended. Nice touch keeping the hard-fail at >50 untouched.
  • ✅ The Windows reconfigure(encoding="utf-8") fix is a sensible cross-platform improvement.

One small cleanup before merge 🧹 — in validate_prd.py the imports got duplicated during the patch:

import re
import sys
import re          # <- duplicate
import sys         # <- duplicate
from pathlib import Path
...
from pathlib import Path   # <- duplicate (line 31)

Could you collapse those down to a single import re, import sys, and one from pathlib import Path? It's harmless functionally, but let's keep the top of the file clean. (Tiny nit while you're in there: the two test classes lost their blank-line separation — class TestCheck5RequirementsCap and class TestAssumptions are now flush against the previous block; PEP 8 likes 2 blank lines between top-level classes.)

Once the imports are deduped I'll merge. Thanks again! 🚀

@Diyaaa-12

Copy link
Copy Markdown
Contributor Author

Cleaned up — imports deduped (single import re, import sys, from pathlib import Path) and PEP 8 blank-line spacing fixed between TestCheck5RequirementsCap and TestAssumptions. All 19 tests still passing locally. Ready for another look whenever you get a chance 🙏

@jcmaker jcmaker merged commit b3999db into jcmaker:main Jul 7, 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.

Linter: advise (don't fail) when a phase has more than 30 requirements

2 participants