Recovery: HybridClaw - recovery and local commands

[furukama]

Automated PR to merge recovery changes and local command flows for HybridClaw AI agent into main.

[Copilot]

Pull request overview

Adds local-only recovery/admin command flows for the HybridClaw agent, improving operator visibility (auth status), plugin lifecycle control (enable/disable), and retry/error messaging around HybridAI premium-model permission failures.

Changes:

• Add local-only /auth status hybridai and wire it through the command registry, TUI menu, and gateway command handler.
• Add plugin enable|disable across CLI/TUI/gateway, backed by a new setPluginEnabled() runtime-config writer and plugin runtime reload behavior.
• Improve HybridAI error parsing/messaging and retry/fallback decisions for premium-model permission errors; persist resolved chatbot ID in the session.

Reviewed changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
tests/tui-slash-menu.test.ts	Verifies new TUI menu entry for /auth status hybridai.
tests/tui-slash-command.test.ts	Adds coverage for mapping auth status hybridai and plugin disable ... to gateway args.
tests/hybridai-retry.test.ts	Ensures premium permission errors don’t trigger fallback/retry classification changes.
tests/hybridai-request-error.test.ts	Confirms premium permission errors are rewritten into an actionable message.
tests/gateway-status.test.ts	Adds end-to-end coverage for auth status hybridai and local-only restriction; minor expectation tweak.
tests/gateway-service.plugins.test.ts	Adds mocks and tests for gateway plugin disable + restriction + help text.
tests/gateway-service.bot-auth.test.ts	Verifies chatbot-id fallback is cached into the session.
tests/command-registry.test.ts	Ensures auth is TUI-only and adds parsing for plugin disable.
tests/cli.test.ts	Adds CLI test coverage for plugin disable.
src/tui.ts	Treats `plugin enable
src/plugins/plugin-config.ts	Introduces setPluginEnabled() and plugin existence checks for enabled-state changes.
src/gateway/gateway-service.ts	Adds auth status hybridai, plugin enable/disable handling + reload, and caches fallback chatbot id.
src/command-registry.ts	Registers auth as TUI-only; adds `plugin enable
src/cli/plugin-command.ts	Implements `hybridclaw plugin enable
src/cli/help.ts	Updates CLI help/usage text for plugin enable/disable.
docs/chat.html	Adjusts web chat error handling on /api/chat failures (removes banner update).
container/src/providers/shared.ts	Adds structured provider error parsing, premium-permission detection, and improved summaries/messages.
container/src/model-retry.ts	Avoids fallback on premium-model permission errors.
container/src/index.ts	Emits cleaner HybridAIRequestError messages without double-prefixing.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

In the enable/disable path, the CLI always prints "Updated runtime config at ..." even when setPluginEnabled() returns changed: false. In particular, setPluginEnabled() short-circuits for enable when there is no existing config entry (plugin already enabled by default) and does not write the config file, so this message is misleading. Consider only printing the "Updated runtime config" line when a write actually occurred (or have setPluginEnabled return an explicit written/changedConfig flag).

Suggested change

	console.log(`Updated runtime config at ${result.configPath}.`);
	if (result.changed) {
	console.log(`Updated runtime config at ${result.configPath}.`);
	}

[Copilot]

setPluginEnabled() always calls ensurePluginExistsForConfig() before changing state. This prevents disabling a plugin when it cannot be discovered (e.g., plugin directory removed/corrupt, or a configured path is now invalid), even though disabling is a safe recovery action to silence repeated discovery warnings and let the gateway start cleanly. Consider only requiring discovery when enabling, or allow disabling when the plugin has an existing config entry (even if discovery fails).

Suggested change

	await ensurePluginExistsForConfig(normalizedPluginId, nextConfig);
	if (enabled) {
	await ensurePluginExistsForConfig(normalizedPluginId, nextConfig);
	}

[Copilot]

Removing the setError(text) call in the submitPrompt catch block means request failures from /api/chat no longer update the error banner (errorEl), while other flows still rely on it. This can make send failures easy to miss (only a system chat message is added) and can also leave a stale banner from a previous error. If the banner is still intended, restore setError(text) here; if not, consider clearing it explicitly or removing the banner usage for consistency.

[furukama]

@claude Do a thorough review of this PR in terms of: KISS, YAGNI, DRY, Fail Fast, Secure by Default and performance/speed

[claude]

Claude finished @furukama's task in 2m 46s —— View job

---

PR Review: KISS, YAGNI, DRY, Fail Fast, Secure by Default, Performance

Tasks:

• Understand the request
• Read key changed files
• Analyze KISS violations
• Analyze YAGNI violations
• Analyze DRY violations
• Analyze Fail Fast patterns
• Analyze Secure by Default patterns
• Analyze performance/speed concerns
• Post final review

---

Overall Assessment

The PR adds coherent, well-tested features. Most of the code quality issues are medium severity — no show-stoppers, but there are a few worth fixing before merge, particularly the security boundary expansion and a few meaningful DRY/YAGNI violations.

---

KISS — Keep It Simple Stupid

Redundant pre-conditions before isHybridAIEmptyVisibleCompletion (container/src/index.ts:1039–1050)

The guard block checks toolCalls.length === 0 && !visibleAssistantText before calling isHybridAIEmptyVisibleCompletion(response), which itself re-checks both of those things internally. The outer conditions are redundant dead-code.

// before the call site:
toolCalls.length === 0 &&
!visibleAssistantText &&
parseRalphChoice(choice.message.content) === null &&
isHybridAIEmptyVisibleCompletion(response)  // ← re-checks tool_calls + visible text internally

Either inline the remaining conditions into isHybridAIEmptyVisibleCompletion or trust the helper to do its job. Fix this →

Complex rollback in gateway plugin enable/disable (src/gateway/gateway-service.ts:7028–7086)

The saveRuntimeConfig(previousConfig) + reloadPluginRuntime() re-call on failure is a 55-line handler with two async side effects and a manual rollback. If the second reloadPluginRuntime() also fails, the system is silently left in an ambiguous state. Consider extracting a applyPluginEnabledChange(pluginId, enabled, previousConfig) helper, or at minimum log the secondary reload failure.

---

YAGNI — You Aren't Gonna Need It

code field in ParsedProviderErrorBody is parsed but never used (container/src/providers/shared.ts:37–42)

The interface defines code: number | string | null and it's extracted in parseProviderErrorRecord, but nothing in this PR (or in the existing codebase based on the diff) reads parsed.code. Only type and message are used in summarizeErrorBody and isPremiumModelPermissionError.

interface ParsedProviderErrorBody {
  message: string | null;
  type: string | null;
  code: number | string | null; // ← extracted but never consumed
}

Fix this →

Over-engineered debug serialization (container/src/providers/shared.ts:155–173)

summarizeHybridAICompletionForDebug produces a JSON blob with contentPartTypes, visibleTextChars, toolCallCount, usage, etc. — all for a single console.error on an obscure empty-completion path. A simple template string (id=${}, model=${}, finish=${}, contentType=${}) would convey the same signal with far less code.

auth command with only one hardcoded provider choice (src/command-registry.ts:640–656)

The provider option has choices: [{ name: 'hybridai', value: 'hybridai' }] — exactly one entry. The abstraction of a choice array anticipates future providers that don't exist yet. Until there are two, the required arg could simply be validated at runtime. Not a breaking issue, but it adds definition boilerplate for zero present-day gain.

---

DRY — Don't Repeat Yourself

Local-session restriction guard duplicated (src/gateway/gateway-service.ts:6773–6780 and :7040–7047)

The exact same check appears twice within 300 lines:

req.guildId !== null ||
(req.channelId !== 'web' && req.channelId !== 'tui')

Both auth status hybridai and plugin enable/disable copy-paste this guard. Extract it:

function isLocalSession(req: GatewayCommandRequest): boolean {
  return req.guildId === null &&
    (req.channelId === 'web' || req.channelId === 'tui');
}

Fix this →

Help-text entries duplicated (with/without slash prefix) (src/gateway/gateway-service.ts:5973–5999)

The PR extends the pre-existing pattern of maintaining two parallel help-text arrays (one for bare commands, one for slash-prefixed). While this pattern predates this PR, each new command now needs to be added in two places. This is worth noting as tech debt — a single source-of-truth HELP_ENTRIES array that formats both forms would eliminate future drift.

---

Fail Fast

setPluginEnabled() runs expensive discovery before the early-return path (src/plugins/plugin-config.ts:218–257)

export async function setPluginEnabled(pluginId, enabled) {
  const normalizedPluginId = normalizePluginId(pluginId);
  const nextConfig = cloneConfig(getRuntimeConfig());
  await ensurePluginExistsForConfig(normalizedPluginId, nextConfig); // ← full plugin scan

  const existing = findPluginEntry(nextConfig, normalizedPluginId);
  if (enabled && !existing) {
    return { changed: false, ... }; // ← no-op early return
  }
  ...
}

ensurePluginExistsForConfig instantiates a PluginManager and calls discoverPlugins() — a full filesystem scan — before the enabled && !existing no-op path is checked. The Copilot review also flagged this. Move the existing check above the ensurePluginExistsForConfig call for the enable+no-entry short-circuit:

const existing = findPluginEntry(nextConfig, normalizedPluginId);
if (enabled && !existing) return { changed: false, ... }; // fast no-op
if (enabled) await ensurePluginExistsForConfig(...); // only when actually needed

Fix this →

---

Secure by Default

PROJECT_ROOT widens the container's file-access boundary (container/src/runtime-paths.ts:74, src/infra/host-runner.ts:353)

host-runner.ts now passes process.cwd() (the gateway's working directory) to the container as HYBRIDCLAW_AGENT_PROJECT_ROOT. In resolveRootBoundPath, this is treated as a second allowed root alongside the workspace root:

if (PROJECT_ROOT && isWithinRoot(resolvedActual, PROJECT_ROOT)) {
  return resolvedActual; // ← allowed, bypasses workspace root check
}

If the gateway runs from a directory that contains sensitive files (e.g., /home/user/ or a repo with .env, secrets), the agent can now read and potentially write those files — outside the intended workspace sandbox. This is a security boundary expansion. It should be tightened to only allow read access, or scoped to a specific safe subdirectory (e.g., the project's source files), not the entire cwd. Fix this →

--password-store=basic in browser login args (src/browser/browser-login.ts:30)

The new buildBrowserLoginArgs function adds --password-store=basic and --use-mock-keychain. These flags tell Chromium to store credentials as cleartext on disk rather than using the OS keychain. The comment says this is to "match the automation runtime so cookies/session state stay readable." This is reasonable for a dedicated automation profile, but should be documented in a code comment so future maintainers don't treat this as the safe default for general browsing. The fact that this function is now exported means it can be called in other contexts where the intent may not be as clear.

auth status hybridai exposes the credential file path (src/gateway/gateway-service.ts:1549)

buildHybridAIAuthStatusLines() includes Path: ${status.path} in the response. This reveals the filesystem path to the auth credentials file. While the command is local-only, path disclosure in structured output could be scraped by a malicious plugin or exfiltrated via session export. Consider omitting the path or making it opt-in.

---

Performance / Speed

Double-parsing of error body in isPremiumModelPermissionError (container/src/providers/shared.ts:123–135)

When isPremiumModelPermissionError(error) is called, it calls parseProviderErrorBody(error.body). But HybridAIRequestError was already constructed by calling summarizeErrorBody(body) which calls parseProviderErrorBody(body) internally. The body is JSON-parsed twice. On an error path this is harmless, but it could be avoided by caching the parsed result on the error object:

class HybridAIRequestError extends Error {
  readonly parsedBody: ParsedProviderErrorBody | null;
  constructor(message, status, body) {
    ...
    this.parsedBody = parseProviderErrorBody(body);
  }
}

resolvePlaywrightBrowsersPath has a side effect (container/src/browser-tools.ts:465)

The fallback at the end of the path-candidate loop calls ensureWritableDir(BROWSER_PLAYWRIGHT_CACHE) — which creates a directory as part of path resolution. A function named resolve* should be pure (return a path, not create it). The directory creation should be deferred to when the path is actually used.

HYBRIDAI_MAX_TOKENS doubled from 4096 → 8192 (src/config/config.ts, src/config/runtime-config.ts)

This doubles maximum output token budget per turn, which directly increases per-request latency and cost. No mention of this in the PR description. Worth confirming this is intentional and that the default model supports 8192 output tokens without performance regression.

---

Summary Table

Principle	Severity	Key Issues
KISS	Low–Medium	Redundant pre-conditions before isHybridAIEmptyVisibleCompletion; 55-line rollback handler
YAGNI	Low	Unused code field in parsed error; over-detailed debug serializer; single-choice provider option
DRY	Medium	Local-session guard duplicated in 2 places; dual help-text arrays extended
Fail Fast	Medium	Plugin discovery runs before no-op early return in setPluginEnabled
Secure by Default	High	PROJECT_ROOT widens sandbox file-access boundary; --password-store=basic on exported fn; credential path disclosure
Performance	Low	Double error-body parsing; side effect in path resolver; undocumented token increase