fix: address claude-review feedback on blocker origins (#487)#531
Conversation
- Fix system guidance text to be generic ("System-initiated pause...")
covering both stall-timeout and PRD discovery pause origins
- Extract _REASON_STALL_DETECTED constant to eliminate magic string
in react_agent.py origin detection
- Add migration coverage test for the ALTER TABLE branch
(test_alter_table_migration_adds_created_by_column)
- Use BlockerOrigin enum in BlockerResponse (stronger typing + OpenAPI docs)
- Type blockers.create() param as Union[BlockerOrigin, str]
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
WalkthroughThe PR refactors blocker origin handling by allowing the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Review of PR 531 - Follow-up to 530 blocker origin feedback This is a clean, focused cleanup that correctly addresses the issues raised in the previous review. Each fix is targeted and well-scoped. _REASON_STALL_DETECTED constant - exactly the right fix. Eliminates all 5 magic string usages in one shot. The remaining reason strings ('escalated_to_blocker', 'max_iterations_reached') could similarly be extracted in future, but that is out of scope here. Migration test - test_alter_table_migration_adds_created_by_column is well-structured: creates an old-schema DB, confirms absence of the column, runs _init_database, confirms presence. This is exactly the regression guard this migration needed. The docstring update on the existing test (clarifying it tests the COALESCE fallback, not the ALTER TABLE path) is a nice touch. Guidance text - The new text covers stall-timeout AND discovery session pauses, which is more accurate than the previous stall-specific wording. Test updated in sync. One thing worth verifying: Union[BlockerOrigin, str] to SQLite serialization. The default is now BlockerOrigin.HUMAN (an enum), but callers in react_agent.py still pass plain strings ('system', 'agent'). The INSERT in blockers.create() needs to call .value on the enum (or use a StrEnum) to store 'human' rather than the enum repr. Since the tests pass this is presumably handled correctly, but worth a quick check of the INSERT to confirm there is no silent mismatch between stored value and what BlockerOrigin expects when reading back. BlockerResponse.created_by: BlockerOrigin - strict enum typing on the response model is correct and the OpenAPI docs benefit is real. Consistent with the core model approach from 530. Summary: All five issues from the original review are cleanly resolved. No scope creep, tests cover the new migration path, and the constant extraction is the right level of refactor. Ready to merge once the SQLite serialization path is confirmed. |
The frontend-tests CI job was disabled during the Phase 2 CLI-first refactor and never re-enabled when Phase 3 web UI became active. This allowed component changes to silently break tests across multiple PRs. CI change: - Uncomment frontend-tests job in test.yml (runs npm run test:coverage) - Add frontend-tests to test-summary needs and failure check - Exclude e2e/ (Playwright) and test-helpers.ts from Jest via jest.config.js Test fixes (component updated but test not): - QuickActions: rewrite tests to exercise all 5 pipeline states (broke in #493 — context-aware refactor changed button labels) - TaskCard: update dependency title to regex match Radix Tooltip text (broke in #524 — tooltip replaced title attribute) - TaskDetailModal: update dep count assertion to "Dependencies (N)" (broke in #524 — label changed from "2 dependencies") - BlockerCard: update success text to "answer recorded" (broke in #531 — success message reworded) - TaskBoardView: update empty state to "No tasks yet" (single block) (broke in #499 — empty state redesigned with CTA)
* feat(core): WorktreeRegistry, orphan cleanup, and get_base_branch (#533) Add atomic worktree registration and stale cleanup to complete the worktree-per-task isolation feature for parallel batch execution. Changes: - codeframe/core/worktrees.py: add WorktreeRegistry (register/unregister/ list_stale/cleanup_stale), get_base_branch(), list_worktrees() - codeframe/core/sandbox/context.py: use get_base_branch() instead of hardcoded "main" when creating worktrees - codeframe/core/conductor.py: call WorktreeRegistry.cleanup_stale() at _execute_parallel() start when isolation == worktree - codeframe/cli/env_commands.py: report stale worktrees in cf env doctor - tests/core/test_worktrees.py: 13 new tests for registry, get_base_branch, list_worktrees, and conductor orphan cleanup integration Closes #533 * fix(web-ui): re-enable frontend CI and fix 5 stale tests The frontend-tests CI job was disabled during the Phase 2 CLI-first refactor and never re-enabled when Phase 3 web UI became active. This allowed component changes to silently break tests across multiple PRs. CI change: - Uncomment frontend-tests job in test.yml (runs npm run test:coverage) - Add frontend-tests to test-summary needs and failure check - Exclude e2e/ (Playwright) and test-helpers.ts from Jest via jest.config.js Test fixes (component updated but test not): - QuickActions: rewrite tests to exercise all 5 pipeline states (broke in #493 — context-aware refactor changed button labels) - TaskCard: update dependency title to regex match Radix Tooltip text (broke in #524 — tooltip replaced title attribute) - TaskDetailModal: update dep count assertion to "Dependencies (N)" (broke in #524 — label changed from "2 dependencies") - BlockerCard: update success text to "answer recorded" (broke in #531 — success message reworded) - TaskBoardView: update empty state to "No tasks yet" (single block) (broke in #499 — empty state redesigned with CTA) * fix(core): address code review feedback on WorktreeRegistry (#537) - wire register()/unregister() into create_execution_context() — registry was defined but never populated, making cleanup_stale() a no-op - fix list_stale() to catch PermissionError separately (process alive, different owner is not stale); previously OSError caught both cases - guard list_worktrees() against non-list JSON (returns [] for dicts etc.) - fix get_base_branch() to return "main" in detached HEAD state (git returns literal "HEAD" — now treated as fallback) - fix conductor.py string comparison to use IsolationLevel.WORKTREE.value - replace misleading conductor test with two focused tests that patch WorktreeRegistry.cleanup_stale directly and assert call/no-call - add tests for detached HEAD fallback and non-list JSON guard --------- Co-authored-by: Test User <test@example.com>
1 participant
Summary
Follow-up to #530 addressing feedback from
claude-reviewthat was not read before merging.Issues fixed:
_REASON_STALL_DETECTED = "stall_detected"constant inreact_agent.py, replacing all 5 raw string usagestest_alter_table_migration_adds_created_by_columnwhich creates a DB with the old schema (nocreated_by), runs_init_database, and asserts the column is addedBlockerResponse.created_by: Changed fromstr = "human"toBlockerOrigin = BlockerOrigin.HUMAN— free Pydantic validation + OpenAPI enum docsblockers.create()parameter: Changed fromstrtoUnion[BlockerOrigin, str]Test Plan
Why this wasn't caught before
The CI
claude-reviewcheck had completed and posted feedback before I merged, but I merged without reading it. CodeRabbit was still pending. Both should have been reviewed first.Summary by CodeRabbit
New Features
Bug Fixes
Tests