feat: add quota-aware Hermes specialist router - #2
Conversation
|
AGENT_STATUS_UPDATE |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 72f70eaf38
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| timeout_seconds: int = 1800 | ||
| codex_binary: str = "/home/ubuntu/.npm-global/bin/codex" | ||
| codex_home: Path = Path.home() / ".codex" | ||
| state_path: Path = Path.home() / ".hermes" / "specialist-router-state.json" |
There was a problem hiding this comment.
This stores the router's Hermes runtime state under Path.home() / ".hermes" instead of the active get_hermes_home(). When the gateway runs under hermes -p <profile> or a custom HERMES_HOME, /model-route-status and the quota session-pool mapping will still read/write the default profile's state, leaking task/repository/session details across profiles and breaking profile isolation; AGENTS.md explicitly requires HERMES_HOME state paths to use get_hermes_home().
Useful? React with 👍 / 👎.
| if not spark["ok"] or FAILURE.search(spark.get("message", "")): | ||
| handoff = self._handoff(goal, repo, spark) | ||
| route = "sol" |
There was a problem hiding this comment.
When Sol is in reserve and a noncritical task was downgraded to Spark by the earlier reserve gate, this branch still escalates any failed or uncertain Spark attempt to Sol unconditionally. In that scenario the next block invokes gpt-5.6-sol despite the documented policy that noncritical Sol work stays on Spark while the weekly reserve is active, so a low-risk failure can drain the protected Sol quota.
Useful? React with 👍 / 👎.
| "goal": {"type": "string"}, | ||
| "repository": {"type": "string"}, | ||
| "risk": {"type": "string", "enum": ["auto", "low", "high", "critical"]}, | ||
| "simulate_spark_failure": {"type": "boolean"}, |
There was a problem hiding this comment.
Because this property is in the model-visible tool schema and route_tool forwards it directly, any prompt/model call that sets simulate_spark_failure=true skips the real Spark attempt and drives the escalation path instead. This looks like a test hook, but exposing it in production lets user content alter routing and consume the more limited Sol quota; keep the simulation path out of the registered schema/runtime handler.
Useful? React with 👍 / 👎.
| directive = _router.route_directive(text, decision) | ||
| return {"action": "rewrite", "text": directive} |
There was a problem hiding this comment.
This hook rewrites every coding-looking gateway message whenever the plugin is loaded, but the plugin toolset can still be disabled per platform or omitted from a restricted session's enabled_toolsets. In that configuration route_specialist_task is absent from the model schema while the prompt is explicitly told to call it, so normal coding messages turn into impossible tool-call instructions instead of falling back to the coordinator.
Useful? React with 👍 / 👎.
| reserve_percent: float = 20.0 | ||
| quota_cache_seconds: int = 120 | ||
| timeout_seconds: int = 1800 | ||
| codex_binary: str = "/home/ubuntu/.npm-global/bin/codex" |
There was a problem hiding this comment.
The default binary only works on one /home/ubuntu npm layout; users with Codex installed on PATH through npm, Nix, Homebrew, or a managed Hermes install will hit FileNotFoundError on every routed task unless they discover and override this hidden path. Since codex_binary is optional in the documented config, the default should use codex/shutil.which rather than an environment-specific absolute path.
Useful? React with 👍 / 👎.
| if resume_session_id: | ||
| cmd = [self.config.codex_binary, "--ask-for-approval", "never", "exec", "resume", "--skip-git-repo-check", "--json", "-m", model, resume_session_id, prompt] | ||
| else: | ||
| cmd = [self.config.codex_binary, "--ask-for-approval", "never", "exec", "--skip-git-repo-check", "--json", "--sandbox", "workspace-write", "-m", model, "-C", str(repo), prompt] |
There was a problem hiding this comment.
Every Codex invocation gets --sandbox workspace-write, including Spark discovery and the Spark review after Sol, even though those phases are explicitly supposed to inspect/review rather than edit. If either phase makes changes, it can mutate the repository before the Sol handoff or after Sol's implementation, so the final diff is no longer solely the implementer's output; the sandbox needs to be tightened for read-only phases.
Useful? React with 👍 / 👎.
| if sol["ok"]: | ||
| review = self._invoke("spark", _review_prompt(goal, sol), repo) | ||
| attempts.append(review) |
There was a problem hiding this comment.
The post-Sol Spark review is appended but its message is never checked with the same FAILURE predicate used for Spark implementation attempts. If the reviewer exits cleanly while reporting tests fail, blocked, or incomplete, _invoke leaves review["ok"] true and the final state reports the Sol implementation as verified, so rejected changes can be surfaced as successful.
Useful? React with 👍 / 👎.
| repo = Path(repository).expanduser().resolve() | ||
| if not goal.strip(): | ||
| raise ValueError("goal is required") | ||
| if not repo.is_dir(): | ||
| raise ValueError(f"repository does not exist: {repo}") |
There was a problem hiding this comment.
The model-visible repository argument is resolved and accepted as long as it is any directory, and _invoke then runs Codex with that directory as the writable sandbox root. A malicious or mistaken prompt can therefore ask the router to operate on /, ~/.hermes, or another unrelated checkout instead of the active conversation workspace, bypassing Hermes' normal workspace/path safeguards for file and terminal tools.
Useful? React with 👍 / 👎.
| text = getattr(event, "text", "") or "" | ||
| decision = _router.classify(text) |
There was a problem hiding this comment.
This hook classifies the raw gateway text before command dispatch and does not exempt event.is_command(). Built-in, quick, plugin, or skill commands whose name or arguments contain words like fix, test, lint, or repo will be rewritten into a specialist-route prompt before their handler sees them, so commands such as /queue fix this bug or a coding skill command no longer preserve their intended slash-command semantics.
Useful? React with 👍 / 👎.
| message, session_id = _parse_codex_jsonl(proc.stdout) | ||
| return {"pool": pool, "model": model, "ok": proc.returncode == 0 and bool(message), "message": message or proc.stderr[-2000:], "session_id": session_id} |
There was a problem hiding this comment.
When a resumed Codex run does not emit a fresh thread.started event, _parse_codex_jsonl returns None and the attempt is saved without the resume_session_id that was actually used. That loses the router's pool-to-session mapping for resumed specialist threads, so later quota parsing cannot attribute rate-limit telemetry for sessions whose JSONL omits the model and relies on the saved attempt mapping.
Useful? React with 👍 / 👎.
|
AGENT_STATUS_UPDATE |
|
AGENT_STATUS_UPDATE\nstatus: done\nagent: Hermes Agent\nrepo: djpapzin/hermes-agent\nthread_or_topic: Telegram Papzin & Crew / thread 6\nbranch: agent/0-quota-specialist-router-codex\npr_url: https://github.com/djpapzin/hermes-agent/pull/2\nissue_url: none\ncommit: 8501e4b\ntests: python -m pytest tests/plugins/specialist_router/test_router.py -q\nchecks: Telegram verification completed successfully via hermes send to telegram:-1003841390135:6; gateway was already active so no restart was required\nprivacy_check: passed\nsecret_check: passed\nintegrations_touched: Codex CLI, Hermes specialist-router plugin, GitHub PR comments, Telegram gateway\nservices_restarted: none\nfiles_changed: plugins/specialist_router/router.py; tests/plugins/specialist_router/test_router.py; docs/specialist-model-router.md\nsummary: Specialist prompt delivery and fallback are fixed, and Telegram delivery was verified end-to-end.\nblockers: none\nnext_recommended_action: If you want a live gateway restart anyway, run it from a shell outside the gateway process; otherwise proceed to the next task.\nnext_goal_suggestion: Resume the next queued Hermes task now that routing and Telegram delivery are verified.\nsafe_to_merge: no |
|
Agent lock/update:
|
|
@codex review Review the exact current head 0765c4a. Focus on quota-boundary behavior, Sol reserve preservation, max concurrent editing isolation, session resume/fallback behavior, |
|
@codex review |
|
AGENT_STATUS_UPDATE |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0765c4acdc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| stdin=subprocess.PIPE, | ||
| input=prompt, |
There was a problem hiding this comment.
With the real subprocess.run runner, passing both stdin=subprocess.PIPE and input=prompt raises ValueError: stdin and input arguments may not both be used before Codex is launched. Any coding message that reaches route_specialist_task therefore aborts inside _invoke instead of running Spark/Sol or using the fallback path; drop the explicit stdin and let input create the pipe.
Useful? React with 👍 / 👎.
| timeout_seconds: int = 1800 | ||
| codex_binary: str = "/home/ubuntu/.npm-global/bin/codex" | ||
| codex_home: Path = Path.home() / ".codex" | ||
| state_path: Path = Path.home() / ".hermes" / "specialist-router-state.json" |
There was a problem hiding this comment.
When Hermes runs with a non-default profile, this default path ignores HERMES_HOME and reads/writes the shared ~/.hermes/specialist-router-state.json, so profiles can leak task/attempt context into each other and /model-route-status or follow-up routing may reuse another profile's specialist state. The repo guidance in AGENTS.md requires get_hermes_home() for all Hermes state paths to keep profiles isolated.
Useful? React with 👍 / 👎.
| if not spark["ok"] or FAILURE.search(spark.get("message", "")): | ||
| handoff = self._handoff(resolved_goal, repo, spark) | ||
| route = "sol" |
There was a problem hiding this comment.
When Sol weekly quota is already in reserve, noncritical Sol work is initially downgraded to Spark at lines 207-208, but this branch unconditionally promotes the task back to Sol after any Spark failure or uncertain output. In the reserve-active case that means an ordinary failed Spark attempt still consumes the protected Sol reserve instead of falling back to the coordinator/manual path or requiring risk="critical".
Useful? React with 👍 / 👎.
| sol = quotas["sol"] | ||
| reserve = self.reserve_active(quotas) | ||
| burst = _burst_state(sol, reserve) | ||
| active = state.get("active_specialist_sessions", 0) |
There was a problem hiding this comment.
active_specialist_sessions is never written anywhere in the router state, and execute() starts Codex synchronously without incrementing, decrementing, or checking this counter. As a result /model-route-status always reports 0 / max_concurrent_editing while concurrent gateway sessions can still launch more than the configured two editing specialists, so the quota/concurrency policy is not actually enforced.
Useful? React with 👍 / 👎.
| def _looks_like_follow_up(self, goal: str) -> bool: | ||
| text = goal.strip() | ||
| return bool(text) and (len(text) <= 40 or bool(FOLLOW_UP.match(text))) |
There was a problem hiding this comment.
This treats every non-empty request of 40 characters or fewer as a follow-up whenever prior router state exists; since saved state always includes a routing_reason, unrelated new tasks like add tests or fix typo get bundled with the previous task/repository and even labeled as a previous specialist failure. That can send the specialist to stale context or leak the last routed repo/task into a new request; reuse state only for explicit follow-up phrases or session-scoped continuation signals.
Useful? React with 👍 / 👎.
| "goal": {"type": "string"}, | ||
| "repository": {"type": "string"}, | ||
| "risk": {"type": "string", "enum": ["auto", "low", "high", "critical"]}, | ||
| "simulate_spark_failure": {"type": "boolean"}, |
There was a problem hiding this comment.
Exposing simulate_spark_failure in the production model tool schema lets any prompted tool call skip the real Spark attempt and mark it failed, which then drives escalation/fallback behavior and can force Sol usage for otherwise routine work. If this is only for tests, keep it out of the public schema and gate it in test-only code instead of giving users or prompt injection a routing-control knob.
Useful? React with 👍 / 👎.
| state = { | ||
| "coordinator_model": self.config.coordinator_model, | ||
| "active_specialist": None, | ||
| "task": resolved_goal[:200], | ||
| "original_task": goal[:200], | ||
| "repository": str(repo), |
There was a problem hiding this comment.
This single state document is overwritten for every specialist run and stores the current task and repository without any session, user, or chat key. In a multi-chat gateway using one Hermes profile, /model-route-status can show another conversation's repo/task, and continuation routing has no way to distinguish which chat produced the saved specialist context; key the state by the gateway session/source before persisting it.
Useful? React with 👍 / 👎.
| directive = _router.route_directive(text, decision) | ||
| return {"action": "rewrite", "text": directive} |
There was a problem hiding this comment.
If an operator disables the specialist-router plugin toolset for a platform via hermes tools, the agent will no longer receive route_specialist_task, but this hook still rewrites coding messages to instruct the coordinator to call that exact tool. In that configuration every coding message is steered toward an unavailable tool instead of falling back to normal coordinator handling, so the hook should check platform toolset availability before rewriting.
Useful? React with 👍 / 👎.
| auto_review_enabled: str = "unknown" | ||
| quota_cache_seconds: int = 120 | ||
| timeout_seconds: int = 1800 | ||
| codex_binary: str = "/home/ubuntu/.npm-global/bin/codex" |
There was a problem hiding this comment.
This default binary path is specific to one Ubuntu/npm-global layout; packaged installs, macOS, and most developer machines expose codex elsewhere or only on PATH. Enabling the plugin without a matching config override will raise FileNotFoundError before the router can run or record fallback state, so the default should be codex/shutil.which("codex") rather than a host-specific absolute path.
Useful? React with 👍 / 👎.
| if item.get("type") == "agent_message" and item.get("text"): | ||
| messages.append(item["text"]) |
There was a problem hiding this comment.
codex exec --json is an event stream, not just the final answer (docs describe it as capturing every event), so appending every agent_message mixes interim narration with the final result. A successful Spark run that says tests failed earlier but now pass still leaves failed in the combined message, causing the FAILURE regex in execute() to escalate to Sol unnecessarily; keep the last/final agent message for routing decisions.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 39a0c2e683
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| timeout_seconds: int = 1800 | ||
| codex_binary: str = "/home/ubuntu/.npm-global/bin/codex" | ||
| codex_home: Path = Path.home() / ".codex" | ||
| state_path: Path = Path.home() / ".hermes" / "specialist-router-state.json" |
There was a problem hiding this comment.
When Hermes runs with a non-default profile, HERMES_HOME points at that profile's isolated directory, but this default still reads/writes the router state in the process user's global ~/.hermes. Enabling the plugin in multiple profiles will leak the last task/repository/session attempts into other profiles and make /model-route-status report another profile's state; use the profile-aware Hermes home for this state file.
Useful? React with 👍 / 👎.
| goal=str(args.get("goal") or ""), | ||
| repository=str(args.get("repository") or os.getcwd()), | ||
| risk=str(args.get("risk") or "auto"), | ||
| simulate_spark_failure=bool(args.get("simulate_spark_failure", False)), |
There was a problem hiding this comment.
Because this handler accepts simulate_spark_failure directly from the model tool arguments (and the schema advertises it below), a user prompt can set it to true and force the router to record a Spark failure without actually spending the required Spark attempt, immediately escalating a low-risk task to Sol. Keep this test hook out of the runtime tool surface or gate it behind a non-user-controlled test path.
Useful? React with 👍 / 👎.
| if not spark["ok"] or FAILURE.search(spark.get("message", "")): | ||
| handoff = self._handoff(resolved_goal, repo, spark) | ||
| route = "sol" |
There was a problem hiding this comment.
When Sol weekly quota is in reserve mode, the initial routing guard downgrades non-critical Sol work to Spark, but any Spark failure here immediately switches back to Sol regardless of risk. That still spends the protected Sol reserve for ordinary non-critical work whenever Spark returns an error/quota/uncertain result; the escalation should respect the same critical-only reserve gate or fall back to the coordinator/manual path.
Useful? React with 👍 / 👎.
| stdin=subprocess.PIPE, | ||
| input=prompt, |
There was a problem hiding this comment.
Every real specialist launch through subprocess.run hits Python's validation here: stdin=subprocess.PIPE cannot be combined with the input= argument, so _invoke() raises ValueError before Codex is started. This means any enabled routing tool call returns a tool execution error instead of running Spark/Sol or reaching the fallback path; drop the explicit stdin while keeping input=prompt.
Useful? React with 👍 / 👎.
|
|
||
| def _remaining(window: Mapping[str, Any]) -> float | None: | ||
| used = window.get("used_percent") | ||
| return None if used is None else max(0.0, 100.0 - float(used)) |
There was a problem hiding this comment.
If a Codex session records an unsupported used_percent value such as a string label, this float() raises ValueError, which is not caught by quotas(). In that case /model-route-status and the pre-dispatch route rewrite fail instead of reporting the unsupported external quota value as unknown, so parse failures should be contained at this conversion point.
Useful? React with 👍 / 👎.
| cmd = [self.config.codex_binary, "--ask-for-approval", "never", "exec", "resume", "--skip-git-repo-check", "--json", "-m", model, resume_session_id, "-"] | ||
| else: | ||
| cmd = [self.config.codex_binary, "--ask-for-approval", "never", "exec", "--skip-git-repo-check", "--json", "--sandbox", "workspace-write", "-m", model, "-C", str(repo), "-"] | ||
| proc = self._runner( |
There was a problem hiding this comment.
If the Codex binary is missing, cannot be executed, or a specialist run times out, this _runner(...) call raises before _invoke() can return a failed attempt. Because execute() does not catch that exception, the registry reports a raw tool error and no Sol escalation/coordinator fallback state is produced; wrap process launch failures as {ok: false, ...} attempts so the documented fallback path can run.
Useful? React with 👍 / 👎.
| elif route == "spark": | ||
| spark = self._invoke("spark", resolved_goal, repo, simulate=simulate_spark_failure, resume_session_id=resume_session_id) | ||
| attempts.append(spark) | ||
| if not spark["ok"] or FAILURE.search(spark.get("message", "")): |
There was a problem hiding this comment.
When Spark succeeds but its final message mentions words like error or failed while describing what it fixed or tests it ran, this broad text match treats the attempt as a failure and spends a Sol run anyway. For routine bug-fix prompts, successful reports commonly include those terms, so escalation should key off the process status or a structured failure signal rather than scanning the success text for generic words.
Useful? React with 👍 / 👎.
| directive = _router.route_directive(text, decision) | ||
| return {"action": "rewrite", "text": directive} |
There was a problem hiding this comment.
If an operator keeps the plugin enabled but disables the specialist-router toolset for a gateway platform via the normal tools configuration, this hook still rewrites coding messages to instruct the coordinator to call route_specialist_task. In that session the tool schema is absent, so ordinary coding messages become polluted with an impossible tool directive instead of falling back to normal coordinator handling; the hook should check that the route tool is available for the session before rewriting.
Useful? React with 👍 / 👎.
| state = self._load_state() | ||
| prior_goal = str(state.get("task") or state.get("original_task") or "").strip() |
There was a problem hiding this comment.
This loads one shared router state file for every gateway chat in the profile, so a short follow-up like "fix this" in a different chat after another user's failed specialist task is expanded with that prior task, repository, and failure details. The previous-task context should be keyed by session/chat (or passed explicitly) rather than read globally, otherwise unrelated conversations can route against the wrong repository.
Useful? React with 👍 / 👎.
| if route == "sol" and reserve and risk != "critical": | ||
| route = "spark" |
There was a problem hiding this comment.
When telemetry shows Sol has five_hour_remaining == 0 but weekly quota is still above the reserve threshold, this guard leaves high-risk noncritical work on Sol and starts a run that is already known to be unavailable. Since the status path explicitly reports the five-hour limit and recommends waiting or using Spark, the routing decision should also downgrade or fall back when the five-hour pool is exhausted.
Useful? React with 👍 / 👎.
1 participant
Summary
Validation
Supersedes #1, whose base snapshot predated synchronization of this fork main branch.