fix(archon): restore orchestrator.py — deleted as collateral, un-reds its test suite - #2464
Merged
Merged
Conversation
… its test suite
`67a11bada` retired pmoves/services/archon/ as "dead Python", stating:
"The Python files crash-loop because they import server.main which 0.6.0
deleted in its TS rewrite."
That is true for main.py — it does import_module("server.main") at :556 plus
server.config, server.services.credential_service and server.api_routes. It was
genuinely dead and stays retired, along with mcp_server.py and the Dockerfile.
It is NOT true for orchestrator.py, which imports only stdlib:
import asyncio, logging, uuid
from typing import ...
Zero vendor coupling. It takes a publish callable by constructor injection. It
could not have crash-looped, and it still passes its full suite on current main:
39/39 green, unchanged.
Why this matters beyond one file: issue #2267 deleted this in the same breath as
filing "D5: NATS bridge sidecar for archon.* subjects — 0.6.0 TS server has no
NATS client. Large." orchestrator.py IS that bridge's working core. It dispatches
archon.crawl.request[.v1], publishes archon.crawl.result.v1, and emits
_publish_task_update() -> archon.task.update.v1 at every lifecycle point, with
failure publishing and shutdown(), built on services.common.events (the validated
envelope helper).
The retirement verified "no compose or Makefile references remain". That was the
wrong test: nothing referenced it BECAUSE the bridge had not been wired yet — that
was the pending work, not evidence of deadness.
Meanwhile the contract layer it targets has since been completed:
- closed schemas for all 8 archon.* subjects (#2336)
- topics.json entries with schema bindings for all 8
- the ARCHON JetStream stream (#2397, archon.>, limits, 30d, 512MB),
verified on 5090
So D5 is no longer "build a bridge from scratch" — it is "add a subscriber loop
and mint handlers to tested code".
Side effect: pmoves/tests/services/test_archon_orchestrator.py (172-test suite from
#1224) was left behind by the retirement and has been RED on main ever since —
ModuleNotFoundError: No module named 'services.archon', which aborts collection
rather than failing one test. This restores it to green.
Scope: orchestrator.py only. main.py, mcp_server.py, Dockerfile, requirements*,
CLAUDE.md, README.md and .env.standalone.example remain retired.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
📝 WalkthroughWalkthroughAdds ChangesArchon orchestration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ArchonOrchestrator
participant TaskState
participant PublishCallable
Client->>ArchonOrchestrator: dispatch crawl request
ArchonOrchestrator->>TaskState: record queued status
ArchonOrchestrator->>PublishCallable: publish acceptance update
ArchonOrchestrator->>TaskState: record processing status
ArchonOrchestrator->>PublishCallable: publish crawl result
ArchonOrchestrator->>TaskState: record completed or failed status
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@pmoves/services/archon/orchestrator.py`:
- Around line 66-87: Validate crawl payloads in _handle_crawl_request and each
ingest payload in the ingest handler against the schemas from
services/common/events.py before resolving identifiers, recording task state, or
publishing updates. Apply the change at pmoves/services/archon/orchestrator.py
lines 66-87 and 146-171, preserving existing processing only for schema-valid
payloads.
- Around line 56-64: Update shutdown and dispatch so shutdown marks the
orchestrator as closed before cancelling tasks, and dispatch rejects or avoids
creating tasks once that state is set. In shutdown, capture the current _pending
tasks into a separate collection, cancel and await only that snapshot, and do
not clear tasks added during the await; ensure the shutdown state prevents such
additions.
- Around line 213-225: Update pmoves/services/archon/orchestrator.py lines
213-225 in _safe_publish_task_failure to record the failed state before
attempting notification, catch publisher exceptions, and log them without
re-raising. Ensure lines 49-54 use this non-raising failure helper, and update
lines 134-144 to use the same guarded notification path so crawl failure
handling cannot produce unobserved background-task exceptions.
- Around line 43-45: Update the handler resolution logic around the subject
lookup to accept only explicitly supported versioned subjects matching the
domain.entity.action.v{n} pattern; remove the fallback that strips any “.v”
suffix, and return no handler for unknown versions instead of routing them to an
unversioned handler.
- Around line 29-38: Raise docstring coverage in
pmoves/services/archon/orchestrator.py by adding concise docstrings to __init__
(including injected publisher behavior), the crawl
acceptance/background-processing method, _handle_ingest_event, the task-update
publication method, the state-mutation/locking method, the
failure-publication/task-tracking method, and the metadata-coercion method.
Update all listed ranges: 29-38, 66-96, 146-171, 173-194, 196-211, 213-230, and
232-236; no site is informational only.
🪄 Autofix
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: CHILL
Plan: Pro Plus
Run ID: 1e3b56bb-edf8-432c-94f6-02201f8acf3f
📒 Files selected for processing (1)
pmoves/services/archon/orchestrator.py
Comment on lines
+29
to
+38
| def __init__(self, publish: PublishCallable, *, logger: Optional[logging.Logger] = None) -> None: | ||
| self._publish = publish | ||
| self._logger = logger or logging.getLogger("archon.orchestrator") | ||
| self._tasks: Dict[str, Dict[str, Any]] = {} | ||
| self._lock = asyncio.Lock() | ||
| self._pending: set[asyncio.Task[Any]] = set() | ||
| self._handlers = { | ||
| **{subject: self._handle_crawl_request for subject in self._CRAWL_TOPICS}, | ||
| **{subject: self._handle_ingest_event for subject in self._INGEST_TOPICS}, | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add docstrings to meet the required coverage.
Only the public workflow methods have method docstrings. The restored constructor and private workflow methods do not. The file cannot meet 80% docstring coverage in this state.
pmoves/services/archon/orchestrator.py#L29-L38: document initialization and injected publisher behavior.pmoves/services/archon/orchestrator.py#L66-L96: document crawl acceptance and background processing.pmoves/services/archon/orchestrator.py#L146-L171: document ingest-event handling.pmoves/services/archon/orchestrator.py#L173-L194: document task-update publication.pmoves/services/archon/orchestrator.py#L196-L211: document state mutation and locking.pmoves/services/archon/orchestrator.py#L213-L230: document failure publication and task tracking.pmoves/services/archon/orchestrator.py#L232-L236: document metadata coercion.
As per coding guidelines, new Python code must maintain at least 80% docstring coverage.
📍 Affects 1 file
pmoves/services/archon/orchestrator.py#L29-L38(this comment)pmoves/services/archon/orchestrator.py#L66-L96pmoves/services/archon/orchestrator.py#L146-L171pmoves/services/archon/orchestrator.py#L173-L194pmoves/services/archon/orchestrator.py#L196-L211pmoves/services/archon/orchestrator.py#L213-L230pmoves/services/archon/orchestrator.py#L232-L236
🤖 Prompt for 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.
In `@pmoves/services/archon/orchestrator.py` around lines 29 - 38, Raise docstring
coverage in pmoves/services/archon/orchestrator.py by adding concise docstrings
to __init__ (including injected publisher behavior), the crawl
acceptance/background-processing method, _handle_ingest_event, the task-update
publication method, the state-mutation/locking method, the
failure-publication/task-tracking method, and the metadata-coercion method.
Update all listed ranges: 29-38, 66-96, 146-171, 173-194, 196-211, 213-230, and
232-236; no site is informational only.
Source: Coding guidelines
Comment on lines
+43
to
+45
| handler = self._handlers.get(subject) | ||
| if handler is None and ".v" in subject: | ||
| handler = self._handlers.get(subject.rsplit(".v", 1)[0]) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject unsupported subject versions.
Line 45 routes archon.crawl.request.v2 to the unversioned crawl handler even though _handlers does not register v2. A changed v2 payload can then run with v1 behavior and emit v1 events.
Register only supported versioned subjects. Return no handler for unknown versions. As per coding guidelines, NATS subjects must follow the domain.entity.action.v{n} pattern.
🤖 Prompt for 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.
In `@pmoves/services/archon/orchestrator.py` around lines 43 - 45, Update the
handler resolution logic around the subject lookup to accept only explicitly
supported versioned subjects matching the domain.entity.action.v{n} pattern;
remove the fallback that strips any “.v” suffix, and return no handler for
unknown versions instead of routing them to an unversioned handler.
Source: Coding guidelines
Comment on lines
+56
to
+64
| async def shutdown(self) -> None: | ||
| """Cancel any background work spawned by the orchestrator.""" | ||
|
|
||
| if not self._pending: | ||
| return | ||
| for task in list(self._pending): | ||
| task.cancel() | ||
| await asyncio.gather(*self._pending, return_exceptions=True) | ||
| self._pending.clear() |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Prevent new tasks after shutdown starts.
At Line 63, await asyncio.gather(...) yields control. dispatch() can then create a new background task. Line 64 clears that task from _pending without cancelling or awaiting it.
Set a shutdown flag before cancellation. Reject new dispatches or task creation after that point. Await a copied task set without clearing tasks created during the wait.
🤖 Prompt for 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.
In `@pmoves/services/archon/orchestrator.py` around lines 56 - 64, Update shutdown
and dispatch so shutdown marks the orchestrator as closed before cancelling
tasks, and dispatch rejects or avoids creating tasks once that state is set. In
shutdown, capture the current _pending tasks into a separate collection, cancel
and await only that snapshot, and do not clear tasks added during the await;
ensure the shutdown state prevents such additions.
Comment on lines
+66
to
+87
| async def _handle_crawl_request(self, event: Dict[str, Any]) -> str: | ||
| payload = event.get("payload") or {} | ||
| url = payload.get("url") | ||
| if not url: | ||
| raise ValueError("crawl request missing url") | ||
| task_id = payload.get("task_id") or str(uuid.uuid4()) | ||
| metadata = self._coerce_metadata(payload.get("metadata")) | ||
| correlation_id = event.get("correlation_id") | ||
| parent_id = event.get("id") | ||
|
|
||
| await self._record_task_state(task_id, "queued", url=url, extra=metadata) | ||
| await self._publish_task_update( | ||
| task_id, | ||
| status="queued", | ||
| message="crawl accepted", | ||
| correlation_id=correlation_id, | ||
| parent_id=parent_id, | ||
| metadata={"url": url, **metadata}, | ||
| ) | ||
|
|
||
| self._track(self._process_crawl(task_id, url, metadata, correlation_id, parent_id)) | ||
| return task_id |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Validate event payloads before state changes or publication.
The crawl path validates only url. The ingest path accepts identifiers and metadata without schema validation. Both paths then update task state and publish Archon events.
pmoves/services/archon/orchestrator.py#L66-L87: validate the crawl request payload before recording or publishing task state.pmoves/services/archon/orchestrator.py#L146-L171: validate each ingest payload before resolving its task identifier or publishing its update.
As per coding guidelines, “Validate event payloads against schemas before publishing events, using services/common/events.py.”
📍 Affects 1 file
pmoves/services/archon/orchestrator.py#L66-L87(this comment)pmoves/services/archon/orchestrator.py#L146-L171
🤖 Prompt for 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.
In `@pmoves/services/archon/orchestrator.py` around lines 66 - 87, Validate crawl
payloads in _handle_crawl_request and each ingest payload in the ingest handler
against the schemas from services/common/events.py before resolving identifiers,
recording task state, or publishing updates. Apply the change at
pmoves/services/archon/orchestrator.py lines 66-87 and 146-171, preserving
existing processing only for schema-valid payloads.
Source: Coding guidelines
Comment on lines
+213
to
+225
| async def _safe_publish_task_failure(self, event: Dict[str, Any], subject: str) -> None: | ||
| payload = event.get("payload") or {} | ||
| task_id = payload.get("task_id") | ||
| if not task_id: | ||
| return | ||
| await self._publish_task_update( | ||
| task_id, | ||
| status="failed", | ||
| message=f"handler crashed for {subject}", | ||
| correlation_id=event.get("correlation_id"), | ||
| parent_id=event.get("id"), | ||
| metadata=self._coerce_metadata(payload.get("metadata")), | ||
| ) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Contain failures while publishing failure updates.
_safe_publish_task_failure() calls _publish_task_update() without an exception guard. If the publisher fails, the dispatch fallback at Line 53 raises a second exception. The crawl failure handler has the same problem and can leave an unobserved background-task exception.
pmoves/services/archon/orchestrator.py#L213-L225: recordfailedstate before notification. Catch and log notification failures.pmoves/services/archon/orchestrator.py#L49-L54: rely on a failure helper that cannot raise.pmoves/services/archon/orchestrator.py#L134-L144: use the same guarded notification path.
Based on supplied pmoves/services/common/events.py:72-80, the injected publisher performs NATS network I/O.
📍 Affects 1 file
pmoves/services/archon/orchestrator.py#L213-L225(this comment)pmoves/services/archon/orchestrator.py#L49-L54pmoves/services/archon/orchestrator.py#L134-L144
🤖 Prompt for 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.
In `@pmoves/services/archon/orchestrator.py` around lines 213 - 225, Update
pmoves/services/archon/orchestrator.py lines 213-225 in
_safe_publish_task_failure to record the failed state before attempting
notification, catch publisher exceptions, and log them without re-raising.
Ensure lines 49-54 use this non-raising failure helper, and update lines 134-144
to use the same guarded notification path so crawl failure handling cannot
produce unobserved background-task exceptions.
This was referenced Aug 7, 2026
POWERFULMOVES
added a commit
that referenced
this pull request
Aug 8, 2026
Claims the lane closing the research/crawl hole between Agent Zero and Archon, scoped to PR #2468 (mai-ui-agent). The gap, verified end to end: Agent Zero has no crawl in its MCP registry (by design — the MC calls the DJ). Archon's contract is real: archon.crawl.request/ result.v1 registered with closed schemas, ARCHON stream live (#2397), dispatcher restored (#2464, 39/39 green). But _process_crawl echoes fragments and extracted_text back out of the REQUEST metadata and reports status completed — no fetch, no parse. Repo-wide there is no crawler: zero crawl4ai/firecrawl/scrapy in any Python. A crawl request returns empty and calls it success. Bounded to three things on #2468: compose home for the orphan Dockerfile, a MAI_UI_BACKEND=local|remote switch so cloud-on-4090 is reachable, and a CUDA guard that currently raises inside a log call on non-CUDA nodes. Not in this lane: wiring _process_crawl to the VL service, archon.mint.* (five of six subjects have no backing operation), Archon NATS emission. Stabilize first. Claimed up front — this register records a prior lane that ran unclaimed and collided with Mavis. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
POWERFULMOVES
added a commit
that referenced
this pull request
Aug 10, 2026
…contract, self-inflating weight, unregistered subject All four verified against the tree, not taken on the reviewer's word. The trigger was labelled 'exists today'. The three archon.mint.* subjects are registered in contracts/topics.json and have schemas, but nothing publishes them: searching *.py/*.ts/*.js/*.go returns no hits, and every match in the repo is a doc, a slash command, or a schema. The Archon NATS bridge is a prerequisite of v0, and the header, prose and components table now say so. Worse, the event cannot carry what the scorer needs. mint.confirmed.v1 is additionalProperties:false over exactly agent_id and confirmed_at -- no error_class, estimated_reduction, contributors or trail_ref, and mint.agent.v1 does not supply them either. An implementer following the old text could not have produced a Domino Record from the event it subscribes to. The spec now requires a correlated domino-candidate contract or a named lookup source per field, and notes that widening the QA-gate signal is the wrong lever. This also surfaced a scope mismatch the spec had with itself: its own 'first real domino' is PR #2464, a PR recovery, which is not an agent mint and would never arrive on that subject at all. The value metric inflated itself. Step 1 derives error_weight by counting error_class in known-roads.jsonl; Step 2 appends a domino line carrying that same error_class to that same file. The second domino for a class therefore counts the first one's paved road as another occurrence of the error, so weight and reported value grow each time a preventive pattern is scored without any new error occurring -- a metric defined as 'reduces FUTURE error' that pays out for scoring. Now counts typed incident records only, with the regression pinned in the acceptance test. tokenism.value.recorded.v1 was to be published but is in neither topics.json nor any schema file, so canonical envelope validation and subject auditing could not recognise it. Registration and a versioned schema are now v0 deliverables and acceptance criteria. Spec-text only; no implementation. Status stays 'draft for operator review'. Surfaced by Codex on #2516. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
POWERFULMOVES
added a commit
that referenced
this pull request
Aug 18, 2026
* spec(value-engine): value-engine domino v0 v0 spec for the value engine: value = future-error-reduction; verified work → Archon-mints → domino cascade → Known Roads pave → ToKenism → Wealth/DoX/BoTZ, with trails as victory stories. Captured from local working-tree state. Docs only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * spec(value-engine): close the four v0 gaps -- unbuilt trigger, empty contract, self-inflating weight, unregistered subject All four verified against the tree, not taken on the reviewer's word. The trigger was labelled 'exists today'. The three archon.mint.* subjects are registered in contracts/topics.json and have schemas, but nothing publishes them: searching *.py/*.ts/*.js/*.go returns no hits, and every match in the repo is a doc, a slash command, or a schema. The Archon NATS bridge is a prerequisite of v0, and the header, prose and components table now say so. Worse, the event cannot carry what the scorer needs. mint.confirmed.v1 is additionalProperties:false over exactly agent_id and confirmed_at -- no error_class, estimated_reduction, contributors or trail_ref, and mint.agent.v1 does not supply them either. An implementer following the old text could not have produced a Domino Record from the event it subscribes to. The spec now requires a correlated domino-candidate contract or a named lookup source per field, and notes that widening the QA-gate signal is the wrong lever. This also surfaced a scope mismatch the spec had with itself: its own 'first real domino' is PR #2464, a PR recovery, which is not an agent mint and would never arrive on that subject at all. The value metric inflated itself. Step 1 derives error_weight by counting error_class in known-roads.jsonl; Step 2 appends a domino line carrying that same error_class to that same file. The second domino for a class therefore counts the first one's paved road as another occurrence of the error, so weight and reported value grow each time a preventive pattern is scored without any new error occurring -- a metric defined as 'reduces FUTURE error' that pays out for scoring. Now counts typed incident records only, with the regression pinned in the acceptance test. tokenism.value.recorded.v1 was to be published but is in neither topics.json nor any schema file, so canonical envelope validation and subject auditing could not recognise it. Registration and a versioned schema are now v0 deliverables and acceptance criteria. Spec-text only; no implementation. Status stays 'draft for operator review'. Surfaced by Codex on #2516. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
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.
There is a red test on
mainright nowIt aborts collection rather than failing a single test, so it can mask other failures in the same invocation depending on how pytest is called. The 172-test suite from #1224 was left behind when its implementation was retired.
What #2267 got right, and what it got wrong
67a11badaretiredpmoves/services/archon/stating:Correct for
main.py—import_module("server.main")at:556, plusserver.config,server.services.credential_service,server.api_routes. Genuinely dead. Stays retired, along withmcp_server.pyand the Dockerfile.Not correct for
orchestrator.py:Stdlib only. Zero vendor coupling — it takes a
publishcallable by constructor injection. It could not have crash-looped, and it still passes its full suite on current main: 39/39, unchanged.Why this matters beyond one file
Issue #2267 deleted this in the same breath as filing:
orchestrator.pyis that bridge's working core:archon.crawl.request[.v1]archon.crawl.result.v1_publish_task_update()→archon.task.update.v1at every lifecycle pointshutdown()services.common.events— the validated-envelope helperThe retirement verified "no compose or Makefile references remain." That was the wrong test. Nothing referenced it because the bridge hadn't been wired yet — that was the pending work, not evidence of deadness.
The contract layer has since been completed
archon.*subjectstopics.jsonentries + schema bindingsarchon.>, limits, 30d, 512MB)So D5 is no longer "build a bridge from scratch" — it's "add a subscriber loop and mint handlers to tested code."
Scope
orchestrator.pyonly.main.py,mcp_server.py,Dockerfile,requirements*,CLAUDE.md,README.md,.env.standalone.exampleall remain retired.Verified from a clean worktree at
origin/main(0 behind) — several earlier conclusions in this lane came from a stale checkout, so this was re-derived from scratch.Related: #2397 (ARCHON stream), #2336 (closed schemas), #2267 (the retirement issue).
🤖 Generated with Claude Code
Summary by CodeRabbit