# Feature Requirements: review-fixes-before-merge > Code review of the fix/codeql-scanning-alerts branch identified 3 issues: a double-sanitization bug causing display corruption, missing shlex.quote() on temp script paths, and unverified CodeQL alert dismissals. - Skill: `tools-only/feature-requirements-review-fixes-before-merge` (Agent Skill, multi-file: 3 files) - Install (CLI): `npx skillmds@latest add tools-only/feature-requirements-review-fixes-before-merge` - Raw SKILL.md: https://api.skillmd.com/api/skills/tools-only/feature-requirements-review-fixes-before-merge/raw - Safety review: pending (external: skill-scanner PASS, skillspector PASS) - Works with: Claude Code, Claude.ai, OpenAI Codex - Category: Coding & Dev Tools - Author: tools-only (https://skillmd.com/u/tools-only) - Updated: 2026-09-29 - Page: https://skillmd.com/skills/tools-only/feature-requirements-review-fixes-before-merge --- # Feature Requirements: review-fixes-before-merge ## Metadata - **Feature**: review-fixes-before-merge - **Status**: APPROVED - **Created**: 2026-02-15 - **Author**: Factory Plan Mode - **Parent**: fix/codeql-scanning-alerts (addendum to existing branch) --- ## 1. Problem Statement ### 1.1 Background Code review of the `fix/codeql-scanning-alerts` branch identified 3 issues: a double-sanitization bug causing display corruption, missing `shlex.quote()` on temp script paths, and unverified CodeQL alert dismissals. ### 1.2 Problem - **Double sanitization**: `_sanitize_pr_content()` called at parse time (line 198) AND output time (line 370) in pr_engine.py. Causes `` → `<Tag>` → `&lt;Tag&gt;` in PR bodies. - **Unquoted script path**: `GIT_SEQUENCE_EDITOR` set via f-string without quoting at history_engine.py:573,664. Breaks if TMPDIR contains spaces. - **Unverified dismissals**: FR-009 (cyclic imports) and FR-011 (undefined exports) used GitHub UI dismissal. Need confirmation they're resolved. ### 1.3 Impact Without fixes: commit messages with `<`, `>`, `&` render as `&lt;` in PR bodies; edge case failure with spaces in temp paths; incomplete audit trail for CodeQL dismissals. --- ## 2. Users ### 2.1 Primary Users - ZERG maintainers creating PRs via `zerg git pr` - ZERG maintainers using `zerg git rewrite` ### 2.2 User Stories - As a maintainer, I want PR bodies to render commit messages correctly so angle brackets and ampersands display properly - As a maintainer, I want rebase scripts to work regardless of temp directory path --- ## 3. Functional Requirements | ID | Requirement | Priority | Notes | |----|-------------|----------|-------| | FR-001 | Remove parse-time `_sanitize_pr_content()` call at pr_engine.py:198 | Must | Keep output-boundary calls at lines 370, 381, 401, 411 | | FR-002 | Add `shlex.quote()` to script_path in history_engine.py:573 | Should | `f"python3 {shlex.quote(script_path)}"` | | FR-003 | Add `shlex.quote()` to script_path in history_engine.py:664 | Should | Same pattern as FR-002 | | FR-004 | Confirm FR-009/FR-011 CodeQL alerts are resolved | Must | Verified: `gh api` shows 0 open cyclic-import/undefined-export alerts | ### 3.1 Detailed Changes **FR-001 (pr_engine.py:198):** ```python # BEFORE (line 198): message=_sanitize_pr_content(message.strip()), # AFTER: message=message.strip(), ``` Store raw commit messages in CommitInfo. Sanitization happens at output boundary (lines 370, 381, 401, 411). **FR-002/FR-003 (history_engine.py:573, 664):** ```python # BEFORE: env["GIT_SEQUENCE_EDITOR"] = f"python3 {script_path}" # AFTER: env["GIT_SEQUENCE_EDITOR"] = f"python3 {shlex.quote(script_path)}" ``` Requires `import shlex` at top of file (or verify existing). **FR-004 (verification only):** - `gh api repos/rocklambros/zerg/code-scanning/alerts` confirms 0 open alerts for `cyclic-import` and `undefined-export` rules. - No code changes needed. Document as verified in CHANGELOG. ### 3.2 Business Rules - No behavioral changes to PR content beyond fixing double-encoding - `_parse_commit_type()` operates on raw message (line 193), before sanitization was applied — unaffected by this change - All 6 call sites of `_sanitize_pr_content` reviewed: only line 198 is redundant --- ## 4. Non-Functional Requirements ### 4.1 Performance - No performance impact ### 4.2 Security - Sanitization at output boundary is the standard secure pattern (OWASP) - `shlex.quote()` prevents shell injection in edge cases ### 4.3 Reliability - All existing tests must pass - `ruff check` must show no new lint errors --- ## 5. Scope ### 5.1 In Scope - pr_engine.py: Remove redundant parse-time sanitization - history_engine.py: Add shlex.quote() to 2 locations - Verify CodeQL alert status for FR-009/FR-011 - CHANGELOG.md update ### 5.2 Out of Scope - Refactoring _sanitize_pr_content() itself - Adding tests for shlex.quote edge case (TMPDIR with spaces is OS-level) - Any other CodeQL fixes ### 5.3 Constraints - Changes go on existing `fix/codeql-scanning-alerts` branch - Must not break any existing tests --- ## 6. Dependencies | Dependency | Type | Status | |------------|------|--------| | Existing test suite | Required | Available | | `shlex` stdlib module | Required | Available (Python stdlib) | --- ## 7. Acceptance Criteria - [ ] PR body renders `` correctly (no double-encoding) - [ ] `shlex.quote()` wraps script_path at both locations - [ ] `python -m pytest tests/ --timeout=120` — green - [ ] `ruff check zerg/git/pr_engine.py zerg/git/history_engine.py` — clean - [ ] CodeQL cyclic-import/undefined-export alerts confirmed at 0 --- ## 8. Open Questions | ID | Question | Status | |----|----------|--------| | Q-001 | Sanitize at output vs parse time? | Resolved: Output boundary | | Q-002 | Are FR-009/FR-011 alerts dismissed? | Resolved: Confirmed 0 open via gh api | --- ## 9. Approval | Role | Name | Date | Signature | |------|------|------|-----------| | Product | klambros | 2026-02-15 | APPROVED | | Engineering | Claude | 2026-02-15 | APPROVED | --- ## 10. Documentation Impact Analysis | File | Required Update | Priority | |------|-----------------|----------| | CHANGELOG.md | Add entry for double-sanitization fix and shlex.quote | Must | --- ## 11. Estimated Effort 3 files changed, ~6 lines modified. Minimal risk. ~5 minutes implementation.