Repository navigation
docs: correct orchestrator invocation and timeout guarantees - #115
groupthinking wants to merge 7 commits into
Conversation
Includes: - Architecture diagram and lifecycle flow - Full API reference for all classes and methods - Customization guide (verification gates, reducers) - Protocol compatibility table (legacy vs new-style) - 5 usage examples (minimal, fan-out, heterogeneous, custom gate, step-by-step) - Integration reference with existing modules - Configuration tuning guidance - Error handling and escalation tiers
Summary by CodeRabbit
WalkthroughAdds ChangesHierarchicalOrchestrator Documentation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Pre-merge checks:
|
There was a problem hiding this comment.
Pull request overview
Adds a new, comprehensive Markdown document describing the orchestrator_mapreduce.py module (Plan → Map → Reduce flow, public API/classes, customization points, protocol calling behavior, persistence, and examples) to help developers integrate and extend the orchestrator.
Changes:
- Introduces full module documentation for
orchestrator_mapreduce.pyindocs/orchestrator_mapreduce.md. - Adds architecture/lifecycle diagrams, API reference, customization contracts, and usage examples.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/orchestrator_mapreduce.md`:
- Line 21: The fenced code block at line 21 in the orchestrator_mapreduce.md
file is missing a language identifier, which violates the MD040 markdown linting
rule. Add the language identifier `text` immediately after the opening triple
backticks on the line containing the fenced code block start marker to specify
that this is a text/ASCII art block, ensuring compliance with markdown
standards.
- Line 520: The fenced code block at line 520 in the orchestrator_mapreduce.md
file is missing a language identifier, which violates the MD040 Markdown linting
rule. Add the language identifier `text` to the opening of the fenced code block
(the line with the opening triple backticks) since this contains an ASCII
diagram. Change the opening ``` to ```text to specify that the content is plain
text.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1cc1a810-26b5-4476-a2fa-6f3e527537c5
📒 Files selected for processing (1)
docs/orchestrator_mapreduce.md
There was a problem hiding this comment.
🟡 Changes recommended
Resolve the two remaining documentation inconsistencies.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
docs/orchestrator_mapreduce.md:185
- This new clarification conflicts with the lifecycle diagram, which still shows
Apply timeoutfollowed unconditionally byTrack outcome. A reader can therefore still infer that timed-out subtasks are tracked, contrary to the implementation and the text here; update the diagram to show tracking only for successful returns and non-timeout exceptions.
- Tracks successful returns and non-timeout exceptions via `track_outcome()` (non-blocking, via `run_in_executor`); timeout failures are not sent to this tracker
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
Update the remaining examples that pass inputs to the zero-argument health checker.
Review details
Suppressed comments (1)
docs/orchestrator_mapreduce.md:289
- This clarification still leaves the runnable endpoint examples above (lines 148–149) and in the fan-out section (lines 392–394) using
api_health_checkerwith per-endpoint inputs. That protocol is zero-argument, so_run_protocol_isolated()ignores those values and the examples do not validate the listed endpoints. Update both examples to use the separate parameterized protocol named by this section, or remove the per-endpoint inputs and describe the existing checker’s behavior.
The repository's existing `api_health_checker.task()` takes no arguments and checks its own endpoint list. To use the parameterized examples, implement a separate protocol like the following and use its name in `task_list`:
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
Co-authored-by: groupthinking <154503486+groupthinking@users.noreply.github.com>
Head branch was pushed to by a user without write access
There was a problem hiding this comment.
🟡 Changes recommended
The documentation inaccurately presents max_job_history as a persistent storage limit even though it only bounds in-memory job history, leaving an active clarification issue.
1 open issue
1 resolved since last review
Review effort: Lite
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues are reported, and the documentation, runnable examples, and focused behavior tests address the current Map/Reduce behavior and documented limitations.
No open issues 🎉
1 resolved since last review
Review effort: Lite
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues are reported, and the changes are focused on aligning documentation and examples with current behavior while adding targeted coverage for the documented execution semantics.
No open issues 🎉
Review effort: Lite

The Map/Reduce guide described invocation, thread cancellation, tracking and verification guarantees that the implementation does not provide. This update reconciles the complete guide and module docstrings with current
masterand adds executable local examples for all five documented usage patterns.api_health_checkernow acceptsservice/urlafter Consolidate workflow patterns with bounded correction and recovery #106;data_processoraccepts actualdata_path/sourceinputs. Runnable examples use real available data/log protocols and directories rather than silently ignored endpoint arguments or automatically generated placeholder protocols.success:falsebehavior, compatible retry inputs, actual Slack CLI escalation and the legacy mutation mechanism's lack of sandbox/rollback guarantees.max_job_historylimits only the in-memoryself.jobscollection. SQLite rows andSTATE.mdentries require separate retention; the guide no longer implies that pruning memory deletes persisted history.master, including Consolidate workflow patterns with bounded correction and recovery #106. The final difference from05e47c4b40bf5f9838fd1b837571afd283690bcais four files: the guide, module docstrings, one local example and eight focused tests. The orchestrator's executable AST is unchanged after excluding docstrings.Validation at
f896334b8f7b419721e363185f335a337aa5e940Both hosted Python jobs, 3.10 and 3.11, in the current CI run completed with 143 passed, 3 skipped and 2 existing warnings per job. The complete job logs verify Black formatting, TypeScript validation and the full pytest suite. All eight current checks finished: seven passed and CodeRabbit skipped its review. The prior source
d72faa95also passed four hosted Python jobs with the same test totals.The follow-up to Copilot's memory-retention finding changes only the guide. A local probe planned and persisted three actual jobs with
max_job_history=1: one job remained in memory, while all three SQLite rows and Markdown entries remained. The eight existing documentation behavior tests pass. Runtime code is unchanged by this follow-up.Independent local review verified all 255 current-master paths were preserved, all 257 candidate/source hashes matched, and the runtime AST remained unchanged. The five-pattern CLI and eight behavior tests passed. A separate probe used directories with 7 and 2 records and checked their actual SQLite/Markdown results, returned-failure/no-retry behavior, local escalation and the mechanical threshold. All 15 Python documentation blocks compile; lint passes on the new example and test files.
The three existing skips require configured quantum execution, a DWAVE token or an accessible MCP server. Repository-wide Flake8 retains advisory baseline diagnostics; this is not a claim that full-repository lint is clean. No live model provider, Slack recipient, production endpoint or deployment was contacted.
This corrects documentation and examples; the disclosed runtime limits remain. In particular, timeout does not terminate a worker thread, semantic-provider failure can fall back to mechanical approval, and legacy mutation is not sandboxed. No merge or production change is included in this review.