Skip to content

Handle missing recitation subtask IDs#2

Merged
azalio merged 1 commit intomainfrom
codex/conduct-code-review-fubd3t
Oct 21, 2025
Merged

Handle missing recitation subtask IDs#2
azalio merged 1 commit intomainfrom
codex/conduct-code-review-fubd3t

Conversation

@azalio
Copy link
Copy Markdown
Owner

@azalio azalio commented Oct 21, 2025

Summary

  • validate that RecitationManager.update_subtask_status locates the requested subtask before updating it
  • reuse the located subtask when logging status updates to avoid stale references
  • update the edge-case test to expect a ValueError when a subtask id is missing

Testing

  • pytest tests/test_recitation_manager.py::TestEdgeCases::test_update_nonexistent_subtask -q (fails: ModuleNotFoundError: mapify_cli)

https://chatgpt.com/codex/tasks/task_e_68f713d262e08324b764075e2b91c6f6

Copilot AI review requested due to automatic review settings October 21, 2025 05:19
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR improves error handling in the RecitationManager.update_subtask_status method by ensuring that subtask IDs are validated before attempting updates. The key changes include:

  • Adding upfront validation to raise a ValueError when a non-existent subtask ID is provided
  • Refactoring to use a single located subtask reference throughout the method to prevent stale references
  • Updating the test expectations to verify that the method now raises an error for missing subtask IDs

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/mapify_cli/recitation_manager.py Refactored update_subtask_status to validate subtask existence upfront and use a single reference for all operations
tests/test_recitation_manager.py Updated test to expect ValueError when updating a non-existent subtask instead of silently failing

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.


if target_subtask is None:
raise ValueError(
f"Subtask with id {subtask_id} was not found in the current plan"
Copy link

Copilot AI Oct 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message format is inconsistent with the test expectation. The test expects the pattern 'Subtask with id 999' but the message includes 'was not found in the current plan' which may not match. Consider simplifying to match the test pattern exactly, or update the test to match the full message.

Suggested change
f"Subtask with id {subtask_id} was not found in the current plan"
f"Subtask with id {subtask_id}"

Copilot uses AI. Check for mistakes.
@azalio azalio merged commit e2614bf into main Oct 21, 2025
5 checks passed
azalio added a commit that referenced this pull request Oct 28, 2025
Implement Recommendation #2 from awesome-claude-code analysis:
- Add sequential-thinking MCP tool examples to Monitor, Predictor, Evaluator agents
- Create comprehensive SEQUENTIAL_THINKING_GUIDE.md (400+ lines)
- Update USAGE.md with guide reference
- Document implementation in IMPROVEMENT-STATUS.md
- Add 6 new playbook patterns from lessons learned

Implementation Details:
- Monitor agent: +120 lines (3 patterns: complex logic, race conditions, edge cases)
- Predictor agent: +110 lines (2 patterns: transitive dependencies, impact cascade)
- Evaluator agent: +150 lines (3 patterns: performance vs security, testability vs simplicity, research completeness)
- Templates synchronized to src/mapify_cli/templates/agents/
- Total: +782 lines, 0 iterations, 108K tokens (54% efficient)

Benefits:
- Reduced false negatives (agents discover non-obvious issues)
- Better trade-off justifications (systematic multi-dimensional analysis)
- Structured reasoning (hypothesis → discovery → revision pattern)
- 6x impact discovery improvement (Predictor example: 2 → 18 affected files)

Status: Ready for production. Zero risk (documentation only, backward compatible).
azalio added a commit that referenced this pull request Nov 11, 2025
Enhanced all MAP Framework agents to leverage available MCP tools for:
- Reasoning memory (search/store/extract/evaluate reasoning patterns)
- Knowledge graph (dependency tracking, impact analysis)
- Intelligent processing (entity extraction, conflict detection)
- Cross-project learning (cipher sync with deduplication)

Changes by agent:

**task-decomposer.md**:
- Added cipher_search_reasoning_patterns as tool #2
- Learn from HOW successful decompositions were reasoned
- Comprehensive examples showing WHY vs WHAT learning

**actor.md**:
- Added 6-step decision tree for tool selection
- Added 3 Tool Combination Scenarios (JWT, WebSocket, caching)
- Detailed examples for context7, deepwiki, codex-bridge usage

**monitor.md**:
- Added cipher_search_graph for dependency understanding
- Added cipher_get_neighbors for tracing chains
- Added cipher_add_node/add_edge for recording validation results

**predictor.md**:
- Added knowledge graph tools for impact analysis
- Added cipher_intelligent_processor for NL impact statements
- Integrated graph traversal for dependency chains

**evaluator.md**:
- Added cipher_search_reasoning_patterns
- Learn from past evaluation reasoning patterns

**reflector.md**:
- Added 4 reasoning memory tools:
  - cipher_search_reasoning_patterns
  - cipher_store_reasoning_memory
  - cipher_extract_reasoning_steps
  - cipher_evaluate_reasoning
- Enables meta-learning from reasoning processes

**curator.md**:
- Added cipher_intelligent_processor for entity extraction
- Enhanced cipher_extract_and_operate_memory with:
  - Improved confidence thresholds (0.85 similarity, 0.7 confidence)
  - enableDeleteOperations: false to prevent accidents
- Added comprehensive deduplication examples

**Template Sync**:
All changes synchronized to src/mapify_cli/templates/agents/
Verified with scripts/check-template-sync.sh

**Key Benefits**:
- Deduplication: Search cipher before creating knowledge
- Meta-learning: Learn HOW problems are solved
- Knowledge graph: Track dependencies and analyze impact
- Cross-project: Share proven patterns (helpful_count >= 5)
- Current docs: context7 ensures up-to-date library APIs

Dual memory system (Playbook + Cipher) now fully operational!
azalio added a commit that referenced this pull request Apr 30, 2026
Re-runnable mapify init no longer overwrites compression overrides:

- mapify init's --compression / --compression-threshold flags now default to None.
- apply_compression_overrides accepts None per parameter and is skipped when both are None, so a bare ``mapify init .`` re-run preserves whatever the user has already set in .map/config.yaml. CLI validation only runs when the user actually passed a flag.
- Tests cover: no-op when both flags absent, partial-policy-only, and partial-threshold-only.

--branch is now sanitized before it touches the filesystem:

- map_utils.sanitize_branch_name() is extracted as a public helper (and reused by get_branch_name). The orchestrator routes args.branch through it before building .map/<branch>/last-compact.marker so a malicious "../etc" branch arg can no longer escape the .map/ directory, and "feature/foo" no longer creates a different cooldown directory than the auto-detected "feature-foo".
- Both .map/scripts/ and src/mapify_cli/templates/map/scripts/ kept in sync.
- New tests/test_map_utils_sanitize.py loads the script via importlib and parametrises path-traversal cases.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants