[SDLC] Refine the SDLC human plan review workflow

[james-in-a-box]

Summary

Approve and advance to implement phase for issue #262

---

Issue: #262
Branch: egg/issue-262

This PR is managed by the SDLC pipeline. It will be marked ready for review once the implementation passes automated review.

[james-in-a-box]

egg is investigating the Lint check failure...

[james-in-a-box]

egg autofix completed. View run logs

[james-in-a-box]

No agent-mode design concerns.

This PR implements HITL (Human-In-The-Loop) decision workflows for the SDLC pipeline. From an agent-mode design perspective, this is well-aligned:

1. No excessive pre-fetching: The prompts provide lightweight procedural guidance (how to use the egg-contract CLI tool) rather than baking in large diffs or data. The agent is still free to explore the codebase and gather context as needed.

2. Appropriate structured output: The --format markdown flag produces machine-readable markers (<!-- egg-hitl-decision id=... -->) that are parsed by the sdlc-hitl.yml workflow—a legitimate downstream consumer. This isn't constraining the agent's natural language; it's providing a CLI tool the agent can choose to use when structured interaction is needed.

3. No post-processing pipeline: The agent posts directly to GitHub. The workflow only watches for checkbox state changes to advance phases—it doesn't parse agent output to take actions the agent couldn't take directly.

4. Procedural context is orienting, not constraining: The prompts explain how to use the HITL tools (which the agent wouldn't know otherwise) while still leaving the what to the agent's judgment. The agent decides when to create decisions, what questions to ask, and what to include in its analysis.

— Authored by egg

[james-in-a-box]

egg agent-mode-design completed. View run logs

[james-in-a-box]

Code Review: PR #332 - Refine the SDLC human plan review workflow

Overall this PR implements the HITL decision workflow enhancements as specified in issue #262. The changes are well-structured and the tests provide good coverage of the new functionality. However, I found several issues that should be addressed.

---

Issues

1. Potential XSS vulnerability in markdown output (Security)

format_decision_markdown in sandbox/egg_lib/contract_cli.py:456-466 does not sanitize the question or option label values before embedding them in markdown output:

lines = [
    f"<!-- egg-hitl-decision id={decision_id} -->",
    "",
    f"**{question}**",
    "",
]

for opt in options:
    lines.append(f"- [ ] {opt['label']}")

If an attacker controls the question text (e.g., via a malicious issue title that gets parsed), they could inject markdown or HTML that GitHub renders. The decision_id is similarly not validated before being embedded in the HTML comment.

Recommendation: Validate decision_id against the expected pattern [a-z0-9-]+ before output. Consider whether question and option labels need escaping for markdown special characters (though GitHub's markdown renderer does provide some protection).

2. Regex pattern mismatch between test and workflow (Correctness)

In tests/workflows/test_hitl_integration.py:26:

DECISION_ID_PATTERN = re.compile(r"<!-- egg-hitl-decision id=([a-z0-9-]+)")

The test captures the ID in group 1. However, the workflow at sdlc-hitl.yml:57 uses:

grep -oP '<!-- egg-hitl-decision id=\K[a-z0-9-]+'

The \K in the grep pattern resets the match start, so it outputs only the ID (not in a group). This is fine for the workflow, but the test regex doesn't exactly mirror the workflow's behavior because the regex allows matching <!-- egg-hitl-decision id=decision-1--> (no trailing space or close bracket required).

The current test passes, but it's a weak guarantee. A decision ID like decision-1-->injected would match the test regex but fail the workflow.

Recommendation: Make the test regex stricter to match only valid decision ID patterns that end at a valid boundary (space or >).

3. Missing validation of decision_id format (Robustness)

The format_decision_markdown function accepts any string as decision_id, but the workflow regex expects [a-z0-9-]+. If cmd_add_decision ever generates an ID with unexpected characters (e.g., capital letters from user input), the workflow won't detect it.

Currently the code generates IDs like decision-{next_id} which is safe, but there's no validation to prevent future regressions.

Recommendation: Add validation in format_decision_markdown or document the expected format contract.

4. Race condition not mitigated for concurrent decision creation (Correctness - Documented)

The TOCTOU race condition in cmd_add_decision (lines 484-486) is documented but not mitigated:

# NOTE: TOCTOU race condition exists here - concurrent calls may get same ID.
# The gateway should handle conflicts appropriately.

This is documented, which is good. However, the gateway's handling of duplicate decision IDs should be verified. If two agents create decision-1 simultaneously, what happens?

Recommendation: Verify the gateway behavior for this edge case is correct, or add a test for it.

5. Test uses path manipulation that may not be portable (Minor)

In tests/workflows/test_hitl_integration.py:14:

sys.path.insert(0, str(Path(__file__).parent.parent.parent / "sandbox"))

This same pattern appears in tests/sandbox/test_contract_cli.py:15. Consider using a conftest.py to handle path setup consistently.

6. Documentation references non-existent workflow line numbers (Documentation)

In tests/workflows/test_hitl_integration.py:24-25:

# Pattern from sdlc-hitl.yml line 57:
# grep -oP '<!-- egg-hitl-decision id=\K[a-z0-9-]+' | head -1

The actual line numbers may drift as the workflow file changes. Consider referencing by step name instead:

# Pattern from sdlc-hitl.yml step "Parse decision changes"

---

Minor observations (non-blocking)

1. The contract file at .egg-state/contracts/262.json is 298 lines - quite large. Consider whether all the phase/task metadata is necessary for the implementation to begin.

2. The --format json default naming is slightly misleading since the "json" format actually outputs a simple text message (Created decision decision-1: ...), not JSON. Consider renaming to --format text or actually outputting JSON.

3. Good test coverage for the markdown formatting function. The edge case tests for special characters and empty options are appreciated.

---

Summary

The implementation correctly addresses the requirements from issue #262. The core logic is sound and the tests are comprehensive. The main concerns are:

1. Input validation for the decision ID to prevent injection or workflow detection failures
2. Ensuring the test regex patterns exactly match what the workflow expects

These are not blocking issues but should be addressed to harden the implementation.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

[james-in-a-box]

No agent-mode design concerns.

The changes since my last review (89688886) add input validation for decision IDs and fix test regex patterns. These are defensive code quality improvements that don't affect agent-mode design alignment.

The original assessment remains valid: the HITL workflow provides orienting context (how to use the CLI tool) rather than constraining pre-fetched data, and the structured output markers serve a legitimate downstream consumer (the workflow that detects checkbox changes).

— Authored by egg

[james-in-a-box]

egg agent-mode-design completed. View run logs

[james-in-a-box]

Re-review: Changes since commit 8968888

The new commit (99efd2b4) properly addresses the main concerns from my previous review.

Issues Addressed ✓

1. Decision ID validation added: validate_decision_id() at contract_cli.py:441-457 correctly enforces [a-z0-9-]+ pattern and is called from format_decision_markdown().

2. Test regex made stricter: The lookahead (?=[ >]) at test_hitl_integration.py:27 ensures tests validate that the ID is followed by a proper boundary character.

3. XSS/injection prevention: The validation rejects uppercase, underscores, spaces, and special characters that could cause workflow detection issues.

4. Comment updated: Line 24-25 now references step name instead of line number.

5. Comprehensive tests added: TestValidateDecisionId class covers valid IDs, uppercase, underscores, spaces, special chars, and empty string.

Minor Issue (Non-blocking)

Inconsistent line number reference style (test_hitl_integration.py:29-30)

The commit message claims to "Update comment to reference step name instead of line number", but lines 29-30 still use line number references:

# Pattern from sdlc-hitl.yml line 72-73:
# grep -oP '^\s*-\s*\[x\]\s*\K.+'

This should reference the step name ("Parse decision changes") for consistency with the decision ID pattern comment above it. This is cosmetic and doesn't block approval.

Verification

I verified the validation logic works correctly:

• Empty strings are rejected
• Uppercase letters are rejected
• The validation is called before markdown output is generated

The changes are correct and the tests provide good coverage.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

[james-in-a-box]

Re-review: Changes since commit 99efd2b

The new commit (15a0837c) updates the remaining line number references to step/job names as noted in my previous review.

Changes Verified ✓

The test file tests/workflows/test_hitl_integration.py now correctly references:

• Line 25: # Pattern from sdlc-hitl.yml step "Parse decision changes": → matches step name at workflow line 48
• Line 31: # Phase approval markers from sdlc-hitl.yml job "handle-approval": → matches job name at workflow line 370

These comment updates are cosmetic and correct. The references now point to stable identifiers (step/job names) rather than line numbers that drift as the workflow file changes.

All Previous Feedback Addressed

1. Decision ID validation ✓ (commit 99efd2b)
2. Test regex made stricter ✓ (commit 99efd2b)
3. Line number references updated ✓ (commit 15a0837)

No issues found in the new changes.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

[james-in-a-box]

No agent-mode design concerns.

The only change since my last review (99efd2b) updates comment references from line numbers to step/job names in test_hitl_integration.py. This is a cosmetic documentation improvement that doesn't affect agent-mode design alignment.

The original assessment remains valid: this PR implements HITL decision workflows with appropriate orienting context (CLI tool usage guidance) rather than constraining pre-fetched data.

— Authored by egg

[james-in-a-box]

egg agent-mode-design completed. View run logs

[james-in-a-box]

egg is addressing review feedback...

[james-in-a-box]

egg feedback addressed. View run logs

[james-in-a-box]

PR #332 has already been merged. No further agent-mode design review needed.

The change since my last review (15a0837c) was only the removal of the contract file (.egg-state/contracts/262.json) - a standard cleanup before merge that has no agent-mode design implications.

— Authored by egg

[james-in-a-box]

egg agent-mode-design completed. View run logs

[james-in-a-box]

Re-review: Changes since commit 15a0837

This PR has been merged. The only change since my last review was commit 9822a5fb:

File removed: .egg-state/contracts/262.json

This is standard cleanup - contract state files are runtime artifacts that should not be committed to the repository. This removal is correct and expected before merge.

Review Summary

All feedback from previous reviews was properly addressed:

1. ✓ Decision ID validation added (validate_decision_id())
2. ✓ Test regex made stricter with boundary lookahead
3. ✓ Line number references updated to step/job names
4. ✓ Contract file removed before merge

No issues found in the final changes.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs