feat(cli): add --plan-file and --track-plan-file options with comprehensive validation#8
Merged
Merged
Conversation
- Add end_loop() function to loop-common.sh for standardized state file renaming on exit (complete, cancel, maxiter, stop, unexpected) - Refactor loop-codex-stop-hook.sh to use end_loop() instead of inline mv - Fix bash validator to use command_modifies_file() helper with rm pattern for more robust plan.md protection - Fix DRY violation in loop-plan-file-validator.sh schema validation - Use schema-outdated.md template instead of hardcoded JSON - Add parent directory existence check in setup-rlcr-loop.sh for clearer error messages when path contains typos - Add negative tests for end_loop() function and path validation
Add tests for plan file modification detection in stop hook: - Test 9: Block when plan file is modified during loop - Test 10: Block when plan file is deleted during loop - Test 11: Block when plan backup is missing Also revert version to 1.1.2 (single bump on feature branch)
- Use find_active_loop() in loop-plan-file-validator.sh instead of duplicating logic - Update cancel-rlcr-loop.md to find newest directory first, then check state.md - Document rename-on-exit behavior (state.md -> <reason>-state.md) in README - Add Exit State Files documentation section
- Remove unused PLAN_IS_IGNORED variable from setup-rlcr-loop.sh - Consolidate redundant state file parsing in stop hook into single section - Update plan-file-modified check to use load_and_render_safe with template - Add || true to grep pipelines for pipefail compatibility with missing fields
- Quote plan_file value in state.md YAML to prevent injection with special characters like colons and hashes - Add validation to reject plan file paths containing spaces at setup - Make space trimming consistent across all hooks (use tr -d ' ') - Update hook parsing to strip YAML quotes from plan_file value - Add tests for space validation and YAML-quoted plan_file parsing - Document no-spaces restriction in help text
- Fix command injection bypass in bash validator by using broader pattern matching that catches shell expansion/substitution attempts - Quote start_branch in state.md to prevent YAML injection (completing the fix from 76982ea which only quoted plan_file) - Fix inconsistent quote handling in state parsers by removing tr -d ' ' from PLAN_FILE and adding quote removal to START_BRANCH parsing Tests added: - Command injection bypass prevention (5 tests): command substitution, glob expansion, brace expansion, piped commands, backtick substitution - YAML quote parsing (4 tests): quote stripping, branch mismatch detection, stop hook parsing, hyphenated paths
- Fix race condition: skip diff check for tracked plan files (rely on git status in UserPromptSubmit hook instead) - Add shell metacharacter validation to prevent injection in paths - Use command_modifies_file helper for plan.md bash validation - Add missing bash modification patterns (truncate, printf, exec) - Standardize YAML: remove unnecessary quotes from state file values - Standardize template fallback formatting with consistent blank lines - Optimize git submodule check by checking .gitmodules first - Add test for shell metacharacter rejection
Fix 5 issues identified in code review: 1. Bash validator regex bypass (Critical) - Pattern now matches .humanize-loop.local/plan.md directly - Changed [^/]+/plan\.md to (/[^/]+)?/plan\.md 2. YAML injection via branch names (Critical) - Added validation for YAML-unsafe characters in branch names - Rejects branches containing : # " ' ` or newlines 3. Race condition in stop hook (Critical) - Added git status check for tracked files in stop hook - Catches modifications between UserPromptSubmit and stop hook 4. Path resolution edge case (Important) - Added explicit error handling for cd failures - Provides clear error message for permission/symlink issues 5. Schema version handling (Important) - Changed from silent exit to JSON block response - Users now get clear instructions to cancel or update Added comprehensive tests for all fixes: - Test 8a: Direct .humanize-loop.local/plan.md bypass - Test 3.5: Path resolution error handling - Tests 9.5-9.7: YAML-unsafe branch name rejection - Test 12: Tracked file race condition detection - Test 13: Schema validation JSON block response
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b49dad6c79
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+180
to
+183
| if [[ "$PLAN_TRACKED" == "true" ]]; then | ||
| # Tracked file: check git status (final safety check for race condition) | ||
| PLAN_GIT_STATUS=$(git -C "$PROJECT_ROOT" status --porcelain "$PLAN_FILE" 2>/dev/null || echo "") | ||
| if [[ -n "$PLAN_GIT_STATUS" ]]; then |
There was a problem hiding this comment.
When plan_tracked is true, the stop hook only checks git status and allows exit if the working tree is clean. This means a user can edit the plan file and commit the change during the loop, making the status clean while the content no longer matches the original plan backup, so the loop will not detect the modification even though the templates and README state plan changes are forbidden. This bypasses the new integrity guarantee for tracked plans; consider comparing against the backup (or recording a hash in state) even in tracked mode so committed changes are still blocked.
Useful? React with 👍 / 👎.
Tests now work on both systems with 'main' and 'master' as default branch. - setup_test_loop() captures DEFAULT_BRANCH after git init - All state.md files use the actual branch name - Branch validation tests use BRANCH_TEST_DEFAULT variable
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
When plan_tracked is true, previously only git status was checked. This allowed bypassing integrity verification by committing plan changes (git status would be clean but content differs from backup). Now the stop hook always verifies content matches backup using diff, regardless of tracking mode. This catches both uncommitted and committed changes to the plan file. Added Test 14 to verify committed changes to tracked files are blocked.
|
No issues found. Checked for bugs and CLAUDE.md compliance. |
Contributor
Author
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 90e7c52ec9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+101
to
+102
| PLAN_IS_TRACKED=$(git -C "$PROJECT_ROOT" ls-files --error-unmatch "$PLAN_FILE" &>/dev/null && echo "true" || echo "false") | ||
| PLAN_GIT_STATUS=$(git -C "$PROJECT_ROOT" status --porcelain "$PLAN_FILE" 2>/dev/null || echo "") |
There was a problem hiding this comment.
The plan-path validation allows filenames beginning with -, but the git checks here pass the path as a bare argument (git ... ls-files/status "$PLAN_FILE"). If the plan file is named like -plan.md or plans/-plan.md, git interprets it as an option rather than a pathspec and the hook will misclassify the file as untracked/clean, or error out and block the loop even though the file is valid. To avoid this edge-case regression, pass -- before the pathspec (e.g., git ... -- "$PLAN_FILE") or explicitly reject plan filenames that start with -.
Useful? React with 👍 / 👎.
SihaoLiu
added a commit
that referenced
this pull request
Jan 17, 2026
Fixes based on Round 0 Codex review: HIGH - Haiku sub-agent invocation (AC-2.3): - Changed from generic "general-purpose" subagent to explicit "draft-relevance-checker" agent reference - Added clear Task tool parameters documentation - Referenced the custom agent in agents/draft-relevance-checker.md HIGH - Missing positive/negative test descriptions (AC-6): - Added TDD-style test format to Acceptance Criteria template - Each AC now includes Positive Tests (expected to PASS) and Negative Tests (expected to FAIL) sections - Added Generation Rule #8 enforcing TDD-style tests MEDIUM - Path boundary phrasing (AC-6.3): - Changed from negative phrasing ("What should NOT be done") to affirmative descriptions ("Maximum Acceptable Scope") - Added examples showing affirmative descriptions - Added Generation Rule #9 for affirmative path boundaries MEDIUM - Narrow path rule for deterministic drafts (AC-6.3): - Added "Note on Deterministic Designs" explaining when upper and lower bounds should converge - Added Generation Rule #10 for respecting fixed approaches
SihaoLiu
added a commit
that referenced
this pull request
Jan 17, 2026
…ns (v1.3.0) (#17) * Feature: Add gen-plan command to transform drafts into structured plans (v1.2.4) Add a new slash command /humanize:gen-plan that transforms user draft documents into well-structured implementation plans with acceptance criteria (AC-X format). New files: - commands/gen-plan.md: Slash command entry point - skills/gen-plan/SKILL.md: Main skill implementation with opus model - scripts/validate-gen-plan-io.sh: IO path validation script - agents/draft-relevance-checker.md: Haiku-based relevance checker Workflow: 1. Validates input/output paths with clear error messages 2. Uses haiku sub-agent to check draft relevance to repository 3. Analyzes draft for clarity, consistency, completeness, functionality 4. Engages user via AskUserQuestion to resolve issues 5. Generates plan.md with goals, ACs, path boundaries, and suggestions Additional changes: - Unified model names to use short aliases (opus, sonnet, haiku) across all skills - Updated test validation to accept short model aliases Updated version to 1.2.4 in plugin.json, marketplace.json, and README.md. * Fix: Address Codex review findings for gen-plan skill Fixes based on Round 0 Codex review: HIGH - Haiku sub-agent invocation (AC-2.3): - Changed from generic "general-purpose" subagent to explicit "draft-relevance-checker" agent reference - Added clear Task tool parameters documentation - Referenced the custom agent in agents/draft-relevance-checker.md HIGH - Missing positive/negative test descriptions (AC-6): - Added TDD-style test format to Acceptance Criteria template - Each AC now includes Positive Tests (expected to PASS) and Negative Tests (expected to FAIL) sections - Added Generation Rule #8 enforcing TDD-style tests MEDIUM - Path boundary phrasing (AC-6.3): - Changed from negative phrasing ("What should NOT be done") to affirmative descriptions ("Maximum Acceptable Scope") - Added examples showing affirmative descriptions - Added Generation Rule #9 for affirmative path boundaries MEDIUM - Narrow path rule for deterministic drafts (AC-6.3): - Added "Note on Deterministic Designs" explaining when upper and lower bounds should converge - Added Generation Rule #10 for respecting fixed approaches * Simplify: Clean up gen-plan implementation (Finalize Phase) Code simplification after Codex review approval: commands/gen-plan.md: - Simplified trailing description to single sentence - Consistent with other command files in the project scripts/validate-gen-plan-io.sh: - Removed unnecessary empty echo statements - Consolidated error messages for conciseness - 11 lines removed, functionality unchanged skills/gen-plan/SKILL.md: - Simplified Phase 6 from numbered list to concise paragraph - 4 lines removed, same information preserved All 49 tests pass. Validation script tested and works correctly. * Bump version to 1.3.0 for gen-plan feature release Version bump from 1.2.4 to 1.3.0 for the new gen-plan command feature. * Fix: Validate argument values before shift to ensure correct exit codes Bug fix: - When --input or --output is passed without a value, shift 2 would fail under set -e and exit with code 1 (mapped to "input not found") instead of exit code 6 ("invalid arguments") - Added checks for missing argument values before shift 2 - Also catches flags followed by another flag (e.g., --input --output) Test enhancement: - Added 10 comprehensive tests for validate-gen-plan-io.sh exit codes - Tests cover: missing values, unknown options, all validation exit codes - Total tests now: 59 (was 49) * Fix: Model validation requires exact match for short aliases The regex was treating opus/sonnet/haiku as prefixes, accepting invalid values like opus-v2, haiku123, or sonnet-fast. Fix: - Short aliases (opus, sonnet, haiku) now require exact match via ^...$ - Full model IDs (claude-*, gpt-*, etc.) still use prefix matching Added tests: - NT-6d: Rejects opus-v2 (partial match) - NT-6e: Rejects haiku123 (partial match) - NT-6f: Rejects sonnet-fast (partial match) Total tests: 62 * Fix: Reject directory paths for --output parameter Bug fix: - If --output pointed to an existing directory (e.g., /tmp/ instead of /tmp/plan.md), validation passed but Write would fail at runtime - Now rejects any existing path at output location (file, directory, symlink) - Added specific error message when output is a directory with suggestion Changes: - Check 4 now uses -d to detect directories with clear error message - Check 4 uses -e to catch any existing path (not just regular files) Added test: - validate-gen-plan-io: output is directory exits 4 Total tests: 63
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
--plan-fileCLI option as alternative to positional argument--track-plan-fileflag for git-tracked plan filesKey Features
CLI Options
--plan-file <path>: Explicit plan file path (alternative to positional arg)--track-plan-file: Flag indicating plan file should be tracked in gitValidation
Security Fixes
.humanize-loop.local/plan.mdState Management
plan.mdplan_tracked,start_branchcomplete-state.md,cancel-state.md,maxiter-state.md,unexpected-state.mdTest Plan
./tests/test-plan-file-validation.sh- 19 tests pass./tests/test-plan-file-hooks.sh- 24 tests pass./tests/test-state-exit-naming.sh- 10 tests passFiles Changed
scripts/setup-rlcr-loop.shloop-plan-file-validator.sh(new),loop-codex-stop-hook.sh,loop-write-validator.sh,loop-edit-validator.sh,loop-bash-validator.sh,lib/loop-common.shcancel-rlcr-loop.md,start-rlcr-loop.mdblock/plan-backup-protected.md,block/plan-file-modified.md,block/schema-outdated.mdtest-plan-file-validation.sh,test-plan-file-hooks.sh,test-state-exit-naming.sh.github/workflows/plan-file-test.yml