Repository navigation
fix: address code review feedback from PRs #4 and #5 - #7
Merged
Merged
Conversation
HIGH:
- Replace fragile JSON regex with brace-balanced parser
- Fix ambiguity ID collision by adding index (REQ-001:category:0)
MEDIUM:
- Fix case-sensitive clarify regex (CLARIFY now works)
- Move inline copy import to module top
- Fix singular/plural grammar ("1 Ambiguity" vs "2 Ambiguities")
LOW:
- Add dismiss command tests
- Document accept/dismiss limitation in docstring
6 new tests (280 total).
MEDIUM: - Wire formatters into agent.py (analysis + tickets display) - Fix (s)/(ies) pluralization with _pluralize() helper LOW: - Add ellipsis for truncated titles (_truncate helper) - Fix inconsistent size defaults (use None, not "M") - Add pipe escaping in table cells (_escape_pipe helper) Updated tests for new output format.
- Extract generic _extract_json_with_key to eliminate duplication between extract_requirements_from_output and _extract_tickets_from_output - Remove dead code: single-quote check in _is_tickets_response (LLM always outputs double quotes for JSON) - Always show assistant's prose response first, then append formatted tables below - previously formatted output replaced prose entirely Addresses LOW priority code review feedback.
MEDIUM fixes: - run_agent_turn: Fix return type from str to tuple[str, list[TResponseInputItem]] (pre-existing bug - function returns tuple but annotation said str) - _extract_balanced_braces: Only apply backslash escaping inside JSON strings, not in surrounding prose where backslashes could appear LOW fix: - test_agent.py: Add _flag() and _req() helpers to reduce line lengths from 100-130+ chars to under 99 (project standard) - All 17 long lines now comply with 99-char limit Added test: test_extract_with_backslash_in_prose - verifies backslash in prose (e.g., Windows paths) doesn't break JSON extraction.
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
Post-merge fixes addressing code review feedback from PR #4 (Ambiguity Feedback Loop) and PR #5 (Formatters).
HIGH Priority
_extract_balanced_braces)REQ-001:vague:0,REQ-001:vague:1)MEDIUM Priority
CLARIFY 1 "text"works, preserves text caseimport copyto top of file_pluralize()helper for "1 epic" vs "2 epics"run_agent_turncorrectly typed astuple[str, list[TResponseInputItem]]LOW Priority
_extract_json_with_key()replaces duplicate extraction_is_tickets_response_truncate()adds ellipsis for long titles_escape_pipe()for markdown table safety_flag()and_req()reduce line lengths to <99 charsTest Plan
ruff check)