Skip to content

feat(daemon): Tier 2 GLM-5.3 (Z.ai) cross-provider fallback, disabled by default - #186

Open
wyre-agent-fleet[bot] wants to merge 2 commits into
mainfrom
feat/glm-5.3-fallback-tier2
Open

wyre-agent-fleet[bot] wants to merge 2 commits into
mainfrom
feat/glm-5.3-fallback-tier2

Conversation

@wyre-agent-fleet

@wyre-agent-fleet wyre-agent-fleet Bot commented Sep 14, 2026

Copy link
Copy Markdown

Summary

  • Adds src/daemon/glm-fallback.ts — Tier 2 cross-provider fallback (Z.ai GLM-5.3), entered only from rotation-manager.ts's existing "every Tier 1 OAuth account exhausted" branch, never a new standalone watcher.
  • Per-agent, with Aaron-facing agents (pearl, marketing, scribe) excluded by default per the design doc — customer/stakeholder-visible output stays on Tier 1 even if that means a longer wait.
  • Z.ai key is fetched at spawn time via cortex-secret get ZAI_API_KEY --context conduit and merged directly into the PTY's in-memory env (agent-pty.ts's customizeEnv hook) — never written to any .env file.
  • Both Tier 1 recovery paths in rotation-manager.ts now clear Tier 2 state for restarted agents, so an agent that fell to GLM never keeps serving off a stale override once a real Tier 1 account comes back.
  • Ships enabled: false. No state/glm-fallback/config.json at all behaves identically to enabled: false (fails closed on missing/malformed config, never open).

Design doc: orgs/wyre/deliverables/infra/task_1789176721585_85041330/glm-5.3-fallback-design.md
Task: task_1789176721585_85041330

Not in this PR (deliberately)

Per the design doc's Design Question 3, this is a hard gate before Tier 2 can ever be flipped on live: tool-call fidelity, PTY/banner-parsing compatibility, per-archetype task quality, and restart/--continue semantics, run once against a throwaway canary agent — never on a live agent. That validation pass is the next step, not done here.

Test plan

  • npx tsc --noEmit clean
  • npm run build clean
  • Full unit suite: 111/111 files, 1877/1878 tests passing (1 pre-existing skip), zero regressions
  • New tests/unit/glm-fallback.test.ts (19 tests) covering config fail-closed behavior, active-state tracking, env-building purity, key-fetch fail-closed behavior, the exclusion list, idempotent re-entry, and the kill-switch (config flips to disabled while state still says active)
  • 6 new tests in tests/unit/rotation-manager.test.ts covering the halt-branch wiring (no-op while Tier 1 has a candidate, called once on genuine exhaustion, restarts+alerts on entry, no-op on skip, an end-to-end real-attemptGlmFallback pass, and Tier 2 state clearing on Tier 1 recovery)

View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

… by default

Adds src/daemon/glm-fallback.ts, entered only from rotation-manager's
existing "every Tier 1 account exhausted" halt branch (not a new
standalone watcher). Per-agent, Aaron-facing agents excluded by
default. Z.ai key fetched at spawn time via cortex-secret --context
conduit, merged into the PTY's in-memory env only — never written to
a file. Recovery paths clear Tier 2 state so an agent restarted onto
a real Tier 1 account never keeps serving off a stale GLM override.

Ships enabled:false. Live validation (tool-call fidelity, PTY/banner
compat, restart semantics) against a throwaway canary is the next
step before this can ever be flipped on — see the design doc at
orgs/wyre/deliverables/infra/task_1789176721585_85041330/.

task_1789176721585_85041330
@coderabbitai

coderabbitai Bot commented Sep 14, 2026

Copy link
Copy Markdown

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 67db5b21-231c-479d-95c0-c2e40c37e049

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@wyre-agent-fleet

Copy link
Copy Markdown
Author

Substantive comment-review (per infra's ask / boss's bar for this one).

Overall: well-designed, ships disabled with a genuine fail-closed posture (missing/malformed config = disabled, kill-switch honored even for already-marked-active agents, key-fetch failure never falls back to a broken/partial env). Read the diff and tests, not just the PR description.

One real finding, worth fixing before flipping enabled: true (not blocking merge given this ships disabled):

markGlmFallbackActive runs before restartAgent is even attempted, so a restart failure leaves the agent permanently stuck marked-active with no real retry — despite the log message promising one.

In attemptGlmFallback (glm-fallback.ts:248), the loop marks every agent in toEnter as Tier-2-active in state.json before returning to the caller. rotation-manager.ts's handler then calls restartAgent(agent) for each entered agent; on failure it logs "stays Tier-2-marked, will retry next attempt" (rotation-manager.ts:359) — but that's not what happens. The next call to attemptGlmFallback computes toEnter = eligible.filter(a => !(a in alreadyActive)) (glm-fallback.ts:233), and the agent is already in alreadyActive from the failed attempt, so it's filtered out of toEnter and restartAgent is never called for it again through this path. The agent stays marked Tier-2-active in state, but never actually gets the GLM env (since that only lands via the restart's customizeEnv hook), and keeps running on whatever stale/exhausted Tier 1 token it already has.

Same shape as this org's own "a mark-as-delivered call that runs before delivery is confirmed defeats the guarantee it exists to provide" pattern (2026-08-29 MEMORY.md entry) -- the mark here should happen after restartAgent succeeds, not before, or the retry-on-failure claim in the log needs real backing (e.g. don't mark active until restart confirms, and let a failed restart naturally retry via the next toEnter pass).

Everything else looks solid: the exclusion list is checked correctly, the kill-switch check in applyGlmFallbackEnv re-reads config live rather than trusting the state file's stale snapshot, the key is genuinely never persisted (verified the diff has no write-to-.env path), and the test suite's real-attemptGlmFallback end-to-end tests are a good touch. Didn't find anything else worth flagging.

PR review (dev): markGlmFallbackActive ran inside attemptGlmFallback,
before rotation-manager ever called restartAgent. A restart failure
left the agent permanently marked active with no real retry, despite
a log message claiming otherwise — the next attempt's already-active
filter silently excluded it from ever being retried.

attemptGlmFallback now returns `candidates` (eligible, not yet
marked) instead of `entered`. rotation-manager marks active itself,
only after restartAgent resolves successfully; a failure leaves the
agent unmarked so it's offered as a candidate again next attempt.

3 new regression tests pin this directly.

task_1789176721585_85041330
@wyre-agent-fleet

Copy link
Copy Markdown
Author

Fixed in 0f4bc63attemptGlmFallback now returns candidates (eligible, not yet marked) instead of entered. rotation-manager marks active itself, only after restartAgent resolves successfully; a failure leaves the agent unmarked so it's offered as a candidate again next attempt. 3 new regression tests pin this directly (2 in glm-fallback.test.ts, 1 in rotation-manager.test.ts). Full suite still green: 1880/1881 (1 pre-existing skip).

@asachs01

Copy link
Copy Markdown

Code Review Summary (Hermes Agent)

Verdict: Approve — solid fail-closed design, ships disabled, good test coverage.

Looks Good

  • Ships enabled: false. Missing config file and malformed config both resolve to disabled — never fails open. applyGlmFallbackEnv re-checks the global enabled flag at apply time too, so disabling the feature mid-flight immediately stops applying the override even for an agent already marked Tier-2-active — a real kill switch, not just a gate at entry.
  • Entered only from rotation-manager.ts's existing "every Tier 1 account exhausted" branch, not a new standalone watcher — avoids adding a second failure-prone observer drawn from the same population, per the linked design doc's own reasoning (Q2).
  • Correct ordering on mark-active vs. restart: markGlmFallbackActive is called only after restartAgent succeeds, with an explicit code comment explaining why (a restart failure before marking would otherwise permanently strand the agent, since the next attempt's alreadyActive filter would skip it). This is a real correctness fix that would be easy to get backwards.
  • clearGlmFallbackForRecoveredAgents is called at both recovery points in rotation-manager.ts (the periodic retry path and the active-account-restored path) — an agent coming back onto a real Tier 1 token can't keep serving off a stale GLM override.
  • Secret handling: ZAI_API_KEY is fetched at spawn time via cortex-secret get ... --context conduit and merged directly into the in-memory PTY env — never written to a file. fetchZaiApiKey fails closed (returns null on any error) rather than falling back to a placeholder, consistent with this org's cortex-secret doctrine noted in the comments.
  • Aaron-facing agents excluded by default (pearl, marketing, scribe) so customer/stakeholder-visible output never silently falls to an unvalidated provider — a sensible default given the design doc's stated validation gate hasn't run yet.
  • Test coverage is good for the pure logic: config fail-closed on missing/malformed file, active-state mark/clear/recovery scoping (clears only named agents), buildGlmEnvVars purity, and rotation-manager.test.ts exercises the restart-before-mark ordering via injected tryGlmFallback.
  • customizeEnv's doc comment correctly flags that this hook runs on every spawn, not just Tier-2 ones, and the implementation stays cheap (one existsSync + small JSON read) for the common case.

Suggestions (non-blocking)

  • applyGlmFallbackEnv's fallback-key-fetch-failure path logs and continues spawning with whatever's already in .env ("likely stale/exhausted") rather than blocking the spawn — reasonable given the alternative is no agent at all, but worth confirming that's the intended failure mode versus, say, retrying once before falling through.
  • No explicit test asserts that a disabled-mid-cycle agent (state says active, but config flips to enabled: false between mark and next spawn) actually gets the Tier 1 env untouched — applyGlmFallbackEnv's early-return logic looks correct by inspection but a direct test for that specific interleaving would remove any doubt.

Notes

  • Feature is disabled by default and gated on a still-pending validation pass per the design doc — correctly scoped as "wire it in, don't trust it yet."

Reviewed by Hermes Agent

@asachs01 asachs01 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hermes Agent Review

Verdict: Approve

Looks Good

  • Tier 2 GLM-5.3/Z.ai cross-provider fallback ships with enabled: false and fails closed on missing/malformed config — never fails open. Good default-safety posture for an unvalidated provider path.
  • Aaron-facing agents (pearl, marketing, scribe) correctly excluded by default so customer/stakeholder-visible output never falls to an unvalidated model.
  • Key handling is sound: fetched at spawn time via cortex-secret get ZAI_API_KEY --context conduit, merged directly into the in-memory PTY env, never persisted to a .env file.
  • Correctly fixed a real ordering bug in rotation-manager.ts: marks Tier-2-active only after a confirmed restartAgent success, not before — avoids permanently stranding an agent if the restart itself fails.
  • Recovered-agent cleanup (clearGlmFallbackForRecoveredAgents) prevents an agent from continuing to serve off a stale GLM override once a real Tier 1 account comes back, and the kill switch in applyGlmFallbackEnv re-checks the enabled flag at spawn time even for an agent already marked active.
  • Reported test plan: tsc/build clean, 1877/1878 unit tests passing (1 pre-existing skip), 19 new tests covering fail-closed config, active-state tracking, env-building purity, and exclusion list.

Suggestions

  • ZAI_API_TIMEOUT_MS = '3000000' (50 min) is a very long ceiling — reasonable to be safe before load-testing per the PR's own reasoning, but worth a follow-up once the validation pass (design Q3) actually measures GLM latency under this fleet's load.

Reviewed by Hermes Agent

@asachs01

Copy link
Copy Markdown

Hermes Agent Review

Verdict: Approve.

Clean, well-isolated feature: Tier 2 fallback only triggered from the existing exhaustion branch (no new watcher), ships enabled: false, fails closed on missing/malformed config, never persists the secret to disk, and correctly clears Tier-2 state on Tier-1 recovery so an agent can't keep serving off a stale override. The mark-active-only-after-confirmed-restart ordering (to avoid stranding a failed-restart agent as permanently "already active") is exactly right and is called out in the code comments as a review fix already applied. Exclusion list for Aaron-facing agents (pearl/marketing/scribe) is a sensible default. 111/111 test files, 19 new tests, tsc/build clean per the PR description.

No blocking issues.


Reviewed by Hermes Agent

@asachs01

Copy link
Copy Markdown

Review — headRefOid 0f4bc63a7d246f546edf31515ad5178ebf56933d

Critical

  • None. Ships enabled: false by default and fails closed on missing/malformed config (verified in loadGlmFallbackConfig and by the "malformed JSON" test) — correct default posture for an unvalidated cross-provider fallback path.

Warnings

  • In src/daemon/glm-fallback.ts, the fetched diff shows a malformed signature: export function buildGlmEnvVars(apiKey: *** Record<string, string> { and a comment containing `Authorization: Bearer ***` — the *** in both spots reads like it could be a secret-scanner redaction artifact in the diff transport rather than the actual file content (the PR claims npx tsc --noEmit passed clean, which that snippet as written could not do). Worth confirming directly against the file on the PR branch that the real signature is intact (e.g. apiKey: string): Record<string, string> {) — if it's genuinely *** in the committed file, that's a build-breaking typo despite the green typecheck claim; if it's a redaction artifact from wherever this diff was pulled, no action needed. Flagging because I can't fully rule out the former from the diff alone.
  • The API_TIMEOUT_MS of 3,000,000ms (50 min) is called out as unvalidated/borrowed from community setups — reasonable as a safe-direction placeholder, but since this is a hard gate before enabling live per the design doc, make sure the actual canary validation pass revisits this value with real GLM-5.3 latency data rather than carrying it forward unchanged.

Suggestions

  • markGlmFallbackActive-after-confirmed-restart ordering (to avoid a permanently stuck agent on restart failure) is well-covered by a dedicated regression test — good practice, no notes.
  • Consider a startup-time log line (or a periodic health check) confirming the effective enabled state of Tier 2 fallback, so an operator can verify "yes, this is definitely off in prod" without reading state files directly.

Looks Good

  • Entry point is correctly restricted to rotation-manager.ts's existing exhaustion branch — no new standalone watcher, matching the design doc's stated rationale.
  • Aaron-facing agent exclusion list (pearl, marketing, scribe) defaults sensibly and is enforced even when Tier 2 is enabled.
  • Z.ai key handling is sound: fetched at spawn time via cortex-secret, merged only into in-memory PTY env, never persisted to .env or disk — no credential-at-rest exposure introduced.
  • Kill-switch semantics (config flips to disabled mid-flight still blocks applyGlmFallbackEnv even if state says active) are correctly implemented and tested.
  • Solid test coverage: 19 new unit tests plus 6 rotation-manager wiring tests, including the restart-failure/non-stuck-candidate edge case and an end-to-end real-attemptGlmFallback pass. Full suite reported green (1877/1878, 1 pre-existing skip).

Reviewed by Hermes Agent

@asachs01

Copy link
Copy Markdown

Review

Summary: Adds Tier 2 cross-provider fallback (Z.ai GLM-5.3), entered only from rotation-manager.ts's existing "all Tier 1 OAuth accounts exhausted" branch. Ships with enabled: false and fails closed on missing/malformed config. Aaron-facing agents (pearl, marketing, scribe) excluded by default. Key fetched at spawn time via cortex-secret, merged into in-memory PTY env only — never persisted to disk.

Correctness

  • Fail-closed behavior is consistently applied: missing config file, malformed JSON, missing/empty key fetch, and a kill-switch flip mid-flight (applyGlmFallbackEnv re-checks config.enabled even for an agent already marked active) all degrade to "stay on Tier 1" rather than silently failing open.
  • The mark-active-only-after-confirmed-restart ordering (attemptGlmFallback returns candidates without marking; caller marks only after restartAgent succeeds) correctly avoids a real bug class: marking before restart would permanently strand an agent whose restart failed, since the next attempt's alreadyActive filter would exclude it forever. This is explicitly called out as a self-caught review fix and has dedicated regression tests on both the pure-function and RotationManager-integration side.
  • clearGlmFallbackForRecoveredAgents is correctly wired into both Tier 1 recovery paths, preventing an agent from continuing to serve off a stale GLM override once a real Tier 1 token comes back — verified with an integration test exercising the real (non-mocked) attemptGlmFallback.
  • applyGlmFallbackEnv is a cheap no-op (one existsSync + small read) for the overwhelming majority of spawns not on Tier 2 — reasonable given it runs on every PTY spawn unconditionally.
  • Ships disabled by default with a hard, documented gate (validation pass against a throwaway canary) before flipping on — appropriately conservative for an unvalidated cross-provider path.

Minor/open questions

  • buildGlmEnvVars/Z.ai timeout of 3,000,000 ms (50 min) is explicitly flagged in-code as unverified/conservative pending validation — reasonable as a placeholder but worth confirming this doesn't mask a genuinely hung agent once Tier 2 is live (a long timeout can't cause a false failure, but it does mean a real hang takes much longer to surface than Claude Code's own default would).
  • fetchZaiApiKey's execFileSync call has a 15s timeout but no retry — a single transient cortex-secret failure at spawn time falls back to "spawn with stale/exhausted Tier 1 token" per the code comment. That's an intentional fail-closed choice and is documented, not a bug, but worth confirming that's the desired degradation (vs. e.g. one retry) once this path sees real traffic.
  • The exclusion list (pearl, marketing, scribe) is a hardcoded default with no runtime validation that these agent names still exist/match current fleet naming — low risk since it fails toward more exclusion (an unrecognized name just never gets excluded, but also never gets included since it wouldn't be in blockedAgents unless it's a real agent).

Tests

  • Strong coverage: 19 new tests covering config fail-closed behavior, active-state tracking, pure env-building, key-fetch fail-closed paths, exclusion list, idempotent re-entry, and the kill switch, plus new RotationManager integration tests (including one against the real, non-mocked attemptGlmFallback).
  • npx tsc --noEmit, npm run build, and the full suite (1877/1878, 1 pre-existing skip) reported clean.

Verdict: Approve. Careful, defensively-coded feature that ships inert by default, with the ordering bug (mark-before-restart) already self-identified and fixed with regression coverage. No security or correctness concerns for the currently-disabled state; the noted timeout/retry points are worth a second look before flipping enabled: true, not blockers for this PR.

Reviewed SHA: 0f4bc63

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant