mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-06-12 08:51:53 +00:00
fix(dashboard): address profile-unification review findings
Review findings (dev review on PR #44007): 1. HIGH — stale page state on profile switch: pages load data on mount and didn't consume the profile scope, so a page opened under profile A kept showing A's state while writes silently targeted the newly selected B. Fixed structurally: ProfileKeyedRoutes wraps the routed page tree and keys it by the selected profile, remounting every page (fresh state + refetch) on switch. ChatPage keeps its own remount (channel keyed on scopedProfile). 2. HIGH — /api/model/auxiliary read was unscoped while /api/model/set wrote scoped (Models page could show default's aux pins while editing worker's). Endpoint now takes profile + _profile_scope, added to PROFILE_SCOPED_PREFIXES, HTTPException re-raise so ghost profiles 404 instead of 500. Regression test asserts read/write symmetry with differing worker/default aux config. 3. MEDIUM — tools post-setup spawned unscoped from the profile-aware drawer. Now spawns 'hermes -p <profile> tools post-setup <key>' (same mechanism as hub installs); drawer threads its profile prop. Most hooks install machine-level artifacts where the scope is inert, but hooks reading config/env now see the drawer's HERMES_HOME. 4. LOW — ty warnings: env Optional asserts before subscript/membership, fastapi import replaced with web_server.HTTPException re-use. 298 tests green across the four affected suites; tsc -b + vite build green; aux scoping E2E-verified with real imports.
This commit is contained in:
parent
e600f69515
commit
92bcd1568d
5 changed files with 149 additions and 20 deletions
|
|
@ -2735,7 +2735,7 @@ def get_recommended_default_model(provider: str = ""):
|
||||||
|
|
||||||
|
|
||||||
@app.get("/api/model/auxiliary")
|
@app.get("/api/model/auxiliary")
|
||||||
def get_auxiliary_models():
|
def get_auxiliary_models(profile: Optional[str] = None):
|
||||||
"""Return current auxiliary task assignments.
|
"""Return current auxiliary task assignments.
|
||||||
|
|
||||||
Shape:
|
Shape:
|
||||||
|
|
@ -2746,9 +2746,14 @@ def get_auxiliary_models():
|
||||||
],
|
],
|
||||||
"main": {"provider": "openrouter", "model": "anthropic/claude-opus-4.7"},
|
"main": {"provider": "openrouter", "model": "anthropic/claude-opus-4.7"},
|
||||||
}
|
}
|
||||||
|
|
||||||
|
``profile`` scopes the read — without it, the Models page would show
|
||||||
|
the dashboard profile's auxiliary pins while /api/model/set wrote the
|
||||||
|
selected profile's (read/write asymmetry).
|
||||||
"""
|
"""
|
||||||
try:
|
try:
|
||||||
cfg = load_config()
|
with _profile_scope(profile):
|
||||||
|
cfg = load_config()
|
||||||
aux_cfg = cfg.get("auxiliary", {})
|
aux_cfg = cfg.get("auxiliary", {})
|
||||||
if not isinstance(aux_cfg, dict):
|
if not isinstance(aux_cfg, dict):
|
||||||
aux_cfg = {}
|
aux_cfg = {}
|
||||||
|
|
@ -2773,6 +2778,8 @@ def get_auxiliary_models():
|
||||||
main = {"provider": "", "model": str(model_cfg) if model_cfg else ""}
|
main = {"provider": "", "model": str(model_cfg) if model_cfg else ""}
|
||||||
|
|
||||||
return {"tasks": tasks, "main": main}
|
return {"tasks": tasks, "main": main}
|
||||||
|
except HTTPException:
|
||||||
|
raise
|
||||||
except Exception:
|
except Exception:
|
||||||
_log.exception("GET /api/model/auxiliary failed")
|
_log.exception("GET /api/model/auxiliary failed")
|
||||||
raise HTTPException(status_code=500, detail="Failed to read auxiliary config")
|
raise HTTPException(status_code=500, detail="Failed to read auxiliary config")
|
||||||
|
|
@ -8790,10 +8797,13 @@ async def save_toolset_env(name: str, body: ToolsetEnvUpdate, profile: Optional[
|
||||||
|
|
||||||
class ToolsetPostSetup(BaseModel):
|
class ToolsetPostSetup(BaseModel):
|
||||||
key: str
|
key: str
|
||||||
|
profile: Optional[str] = None
|
||||||
|
|
||||||
|
|
||||||
@app.post("/api/tools/toolsets/{name}/post-setup")
|
@app.post("/api/tools/toolsets/{name}/post-setup")
|
||||||
async def run_toolset_post_setup(name: str, body: ToolsetPostSetup):
|
async def run_toolset_post_setup(
|
||||||
|
name: str, body: ToolsetPostSetup, profile: Optional[str] = None
|
||||||
|
):
|
||||||
"""Spawn a provider's post-setup install hook as a background action.
|
"""Spawn a provider's post-setup install hook as a background action.
|
||||||
|
|
||||||
Post-setup hooks (npm install for browser/Camofox, pip install for
|
Post-setup hooks (npm install for browser/Camofox, pip install for
|
||||||
|
|
@ -8803,6 +8813,12 @@ async def run_toolset_post_setup(name: str, body: ToolsetPostSetup):
|
||||||
``GET /api/actions/tools-post-setup/status``. The ``key`` is validated
|
``GET /api/actions/tools-post-setup/status``. The ``key`` is validated
|
||||||
against the declared post-setup allowlist before spawning. Returns 400
|
against the declared post-setup allowlist before spawning. Returns 400
|
||||||
for unknown toolset or post-setup key.
|
for unknown toolset or post-setup key.
|
||||||
|
|
||||||
|
``profile`` spawns the hook as ``hermes -p <profile> tools post-setup``.
|
||||||
|
Most hooks install machine-level artifacts (repo node_modules, shared
|
||||||
|
pip packages) where the scope is inert, but hooks that read config or
|
||||||
|
write per-profile state must see the same HERMES_HOME the rest of the
|
||||||
|
drawer's writes targeted — so the scope is threaded for consistency.
|
||||||
"""
|
"""
|
||||||
from hermes_cli.tools_config import (
|
from hermes_cli.tools_config import (
|
||||||
_get_effective_configurable_toolsets,
|
_get_effective_configurable_toolsets,
|
||||||
|
|
@ -8820,8 +8836,12 @@ async def run_toolset_post_setup(name: str, body: ToolsetPostSetup):
|
||||||
|
|
||||||
try:
|
try:
|
||||||
proc = _spawn_hermes_action(
|
proc = _spawn_hermes_action(
|
||||||
["tools", "post-setup", body.key], "tools-post-setup"
|
_profile_cli_args(profile or body.profile)
|
||||||
|
+ ["tools", "post-setup", body.key],
|
||||||
|
"tools-post-setup",
|
||||||
)
|
)
|
||||||
|
except HTTPException:
|
||||||
|
raise
|
||||||
except Exception as exc:
|
except Exception as exc:
|
||||||
_log.exception("Failed to spawn tools post-setup")
|
_log.exception("Failed to spawn tools post-setup")
|
||||||
raise HTTPException(
|
raise HTTPException(
|
||||||
|
|
|
||||||
|
|
@ -196,6 +196,93 @@ class TestProfileScopedModel:
|
||||||
if isinstance(default_model, dict):
|
if isinstance(default_model, dict):
|
||||||
assert default_model.get("default") != "test/model-1"
|
assert default_model.get("default") != "test/model-1"
|
||||||
|
|
||||||
|
def test_auxiliary_read_scoped_matches_write_target(
|
||||||
|
self, client, isolated_profiles
|
||||||
|
):
|
||||||
|
"""Reads and writes must scope symmetrically: an aux pin written to
|
||||||
|
the worker profile must show up ONLY in the worker-scoped read.
|
||||||
|
(Regression: /api/model/auxiliary used to read unscoped while
|
||||||
|
/api/model/set wrote scoped — the Models page displayed the
|
||||||
|
dashboard profile's pins while editing the selected profile's.)"""
|
||||||
|
(isolated_profiles["worker_beta"] / "config.yaml").write_text(
|
||||||
|
"auxiliary:\n vision:\n provider: openrouter\n"
|
||||||
|
" model: worker/vision-pin\n",
|
||||||
|
encoding="utf-8",
|
||||||
|
)
|
||||||
|
resp = client.get("/api/model/auxiliary", params={"profile": "worker_beta"})
|
||||||
|
assert resp.status_code == 200
|
||||||
|
vision = next(t for t in resp.json()["tasks"] if t["task"] == "vision")
|
||||||
|
assert vision["model"] == "worker/vision-pin"
|
||||||
|
|
||||||
|
# Unscoped read = the dashboard's own profile, which has no pin.
|
||||||
|
resp = client.get("/api/model/auxiliary")
|
||||||
|
assert resp.status_code == 200
|
||||||
|
vision = next(t for t in resp.json()["tasks"] if t["task"] == "vision")
|
||||||
|
assert vision["model"] != "worker/vision-pin"
|
||||||
|
|
||||||
|
def test_auxiliary_unknown_profile_404(self, client, isolated_profiles):
|
||||||
|
resp = client.get("/api/model/auxiliary", params={"profile": "ghost"})
|
||||||
|
assert resp.status_code == 404
|
||||||
|
|
||||||
|
|
||||||
|
class TestProfileScopedPostSetup:
|
||||||
|
def test_post_setup_spawns_with_profile_flag(
|
||||||
|
self, client, isolated_profiles, monkeypatch
|
||||||
|
):
|
||||||
|
"""Post-setup runs in a -p scoped subprocess so hooks that read
|
||||||
|
config / write per-profile state see the same HERMES_HOME the rest
|
||||||
|
of the drawer's writes targeted."""
|
||||||
|
import hermes_cli.web_server as web_server
|
||||||
|
|
||||||
|
calls = []
|
||||||
|
|
||||||
|
class _FakeProc:
|
||||||
|
pid = 777
|
||||||
|
|
||||||
|
monkeypatch.setattr(
|
||||||
|
web_server,
|
||||||
|
"_spawn_hermes_action",
|
||||||
|
lambda subcommand, name: calls.append(list(subcommand)) or _FakeProc(),
|
||||||
|
)
|
||||||
|
monkeypatch.setattr(
|
||||||
|
"hermes_cli.tools_config.valid_post_setup_keys",
|
||||||
|
lambda: {"agent_browser"},
|
||||||
|
)
|
||||||
|
resp = client.post(
|
||||||
|
"/api/tools/toolsets/browser/post-setup",
|
||||||
|
json={"key": "agent_browser", "profile": "worker_beta"},
|
||||||
|
)
|
||||||
|
assert resp.status_code == 200
|
||||||
|
assert calls == [
|
||||||
|
["-p", "worker_beta", "tools", "post-setup", "agent_browser"]
|
||||||
|
]
|
||||||
|
|
||||||
|
def test_post_setup_without_profile_keeps_legacy_argv(
|
||||||
|
self, client, isolated_profiles, monkeypatch
|
||||||
|
):
|
||||||
|
import hermes_cli.web_server as web_server
|
||||||
|
|
||||||
|
calls = []
|
||||||
|
|
||||||
|
class _FakeProc:
|
||||||
|
pid = 777
|
||||||
|
|
||||||
|
monkeypatch.setattr(
|
||||||
|
web_server,
|
||||||
|
"_spawn_hermes_action",
|
||||||
|
lambda subcommand, name: calls.append(list(subcommand)) or _FakeProc(),
|
||||||
|
)
|
||||||
|
monkeypatch.setattr(
|
||||||
|
"hermes_cli.tools_config.valid_post_setup_keys",
|
||||||
|
lambda: {"agent_browser"},
|
||||||
|
)
|
||||||
|
resp = client.post(
|
||||||
|
"/api/tools/toolsets/browser/post-setup",
|
||||||
|
json={"key": "agent_browser"},
|
||||||
|
)
|
||||||
|
assert resp.status_code == 200
|
||||||
|
assert calls == [["tools", "post-setup", "agent_browser"]]
|
||||||
|
|
||||||
|
|
||||||
class TestProfileScopedChatPty:
|
class TestProfileScopedChatPty:
|
||||||
def test_chat_argv_scopes_hermes_home(self, isolated_profiles, monkeypatch):
|
def test_chat_argv_scopes_hermes_home(self, isolated_profiles, monkeypatch):
|
||||||
|
|
@ -207,6 +294,7 @@ class TestProfileScopedChatPty:
|
||||||
raising=False,
|
raising=False,
|
||||||
)
|
)
|
||||||
argv, cwd, env = web_server._resolve_chat_argv(profile="worker_beta")
|
argv, cwd, env = web_server._resolve_chat_argv(profile="worker_beta")
|
||||||
|
assert env is not None
|
||||||
assert env["HERMES_HOME"] == str(isolated_profiles["worker_beta"])
|
assert env["HERMES_HOME"] == str(isolated_profiles["worker_beta"])
|
||||||
# Scoped chat must NOT attach to the dashboard's in-memory gateway.
|
# Scoped chat must NOT attach to the dashboard's in-memory gateway.
|
||||||
assert "HERMES_TUI_GATEWAY_URL" not in env
|
assert "HERMES_TUI_GATEWAY_URL" not in env
|
||||||
|
|
@ -220,10 +308,10 @@ class TestProfileScopedChatPty:
|
||||||
raising=False,
|
raising=False,
|
||||||
)
|
)
|
||||||
argv, cwd, env = web_server._resolve_chat_argv()
|
argv, cwd, env = web_server._resolve_chat_argv()
|
||||||
|
assert env is not None
|
||||||
assert env.get("HERMES_HOME") != str(isolated_profiles["worker_beta"])
|
assert env.get("HERMES_HOME") != str(isolated_profiles["worker_beta"])
|
||||||
|
|
||||||
def test_chat_argv_unknown_profile_raises(self, isolated_profiles, monkeypatch):
|
def test_chat_argv_unknown_profile_raises(self, isolated_profiles, monkeypatch):
|
||||||
from fastapi import HTTPException
|
|
||||||
import hermes_cli.web_server as web_server
|
import hermes_cli.web_server as web_server
|
||||||
|
|
||||||
monkeypatch.setattr(
|
monkeypatch.setattr(
|
||||||
|
|
@ -231,6 +319,8 @@ class TestProfileScopedChatPty:
|
||||||
lambda root, tui_dev=False: (["cat"], None),
|
lambda root, tui_dev=False: (["cat"], None),
|
||||||
raising=False,
|
raising=False,
|
||||||
)
|
)
|
||||||
with pytest.raises(HTTPException) as exc:
|
# Reuse the HTTPException class web_server itself raises — avoids a
|
||||||
|
# direct fastapi import (unresolvable in the ty lint environment).
|
||||||
|
with pytest.raises(web_server.HTTPException) as exc:
|
||||||
web_server._resolve_chat_argv(profile="ghost")
|
web_server._resolve_chat_argv(profile="ghost")
|
||||||
assert exc.value.status_code == 404
|
assert exc.value.status_code == 404
|
||||||
|
|
|
||||||
|
|
@ -65,6 +65,7 @@ import { useSidebarStatus } from "@/hooks/useSidebarStatus";
|
||||||
import { AuthWidget } from "@/components/AuthWidget";
|
import { AuthWidget } from "@/components/AuthWidget";
|
||||||
import { PageHeaderProvider } from "@/contexts/PageHeaderProvider";
|
import { PageHeaderProvider } from "@/contexts/PageHeaderProvider";
|
||||||
import { ProfileProvider } from "@/contexts/ProfileProvider";
|
import { ProfileProvider } from "@/contexts/ProfileProvider";
|
||||||
|
import { useProfileScope } from "@/contexts/useProfileScope";
|
||||||
import { ProfileSwitcher } from "@/components/ProfileSwitcher";
|
import { ProfileSwitcher } from "@/components/ProfileSwitcher";
|
||||||
import { ProfileScopeBanner } from "@/components/ProfileScopeBanner";
|
import { ProfileScopeBanner } from "@/components/ProfileScopeBanner";
|
||||||
import { useSystemActions } from "@/contexts/useSystemActions";
|
import { useSystemActions } from "@/contexts/useSystemActions";
|
||||||
|
|
@ -734,17 +735,19 @@ export default function App() {
|
||||||
"min-h-0 flex flex-1 flex-col",
|
"min-h-0 flex flex-1 flex-col",
|
||||||
)}
|
)}
|
||||||
>
|
>
|
||||||
<Routes>
|
<ProfileKeyedRoutes>
|
||||||
{routes.map(({ key, path, element }) => (
|
<Routes>
|
||||||
<Route key={key} path={path} element={element} />
|
{routes.map(({ key, path, element }) => (
|
||||||
))}
|
<Route key={key} path={path} element={element} />
|
||||||
<Route
|
))}
|
||||||
path="*"
|
<Route
|
||||||
element={
|
path="*"
|
||||||
<UnknownRouteFallback pluginsLoading={pluginsLoading} />
|
element={
|
||||||
}
|
<UnknownRouteFallback pluginsLoading={pluginsLoading} />
|
||||||
/>
|
}
|
||||||
</Routes>
|
/>
|
||||||
|
</Routes>
|
||||||
|
</ProfileKeyedRoutes>
|
||||||
|
|
||||||
{embeddedChat &&
|
{embeddedChat &&
|
||||||
!chatOverriddenByPlugin &&
|
!chatOverriddenByPlugin &&
|
||||||
|
|
@ -786,6 +789,21 @@ export default function App() {
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Remounts the entire routed page tree when the global management profile
|
||||||
|
* changes. Pages load their data on mount; without this, a page opened
|
||||||
|
* under profile A would keep showing A's state while writes (via the
|
||||||
|
* fetchJSON ?profile= injection) silently targeted the newly selected
|
||||||
|
* profile B — the exact stale-target footgun the switcher exists to kill.
|
||||||
|
* Keying by profile resets every page's local state so it refetches under
|
||||||
|
* the new scope. The persistent ChatPage host below handles its own
|
||||||
|
* remount (channel keyed on scopedProfile).
|
||||||
|
*/
|
||||||
|
function ProfileKeyedRoutes({ children }: { children: ReactNode }) {
|
||||||
|
const { profile } = useProfileScope();
|
||||||
|
return <div key={profile || "__own__"} className="contents">{children}</div>;
|
||||||
|
}
|
||||||
|
|
||||||
function SidebarNavLink({
|
function SidebarNavLink({
|
||||||
closeMobile,
|
closeMobile,
|
||||||
collapsed,
|
collapsed,
|
||||||
|
|
|
||||||
|
|
@ -198,7 +198,7 @@ export function ToolsetConfigDrawer({ toolset, profile, onClose, onChanged }: Pr
|
||||||
setPostSetupLog([]);
|
setPostSetupLog([]);
|
||||||
setPostSetupKey(provider.post_setup);
|
setPostSetupKey(provider.post_setup);
|
||||||
try {
|
try {
|
||||||
await api.runToolsetPostSetup(toolset.name, provider.post_setup);
|
await api.runToolsetPostSetup(toolset.name, provider.post_setup, profile);
|
||||||
// Bump the trigger so the poll effect (re)starts tailing the log.
|
// Bump the trigger so the poll effect (re)starts tailing the log.
|
||||||
setPostSetupTrigger((n) => n + 1);
|
setPostSetupTrigger((n) => n + 1);
|
||||||
} catch (e) {
|
} catch (e) {
|
||||||
|
|
|
||||||
|
|
@ -70,6 +70,7 @@ const PROFILE_SCOPED_PREFIXES = [
|
||||||
"/api/mcp",
|
"/api/mcp",
|
||||||
"/api/model/info",
|
"/api/model/info",
|
||||||
"/api/model/set",
|
"/api/model/set",
|
||||||
|
"/api/model/auxiliary",
|
||||||
];
|
];
|
||||||
|
|
||||||
function withManagementProfile(url: string): string {
|
function withManagementProfile(url: string): string {
|
||||||
|
|
@ -636,13 +637,13 @@ export const api = {
|
||||||
body: JSON.stringify({ env, profile: profile || undefined }),
|
body: JSON.stringify({ env, profile: profile || undefined }),
|
||||||
},
|
},
|
||||||
),
|
),
|
||||||
runToolsetPostSetup: (name: string, key: string) =>
|
runToolsetPostSetup: (name: string, key: string, profile?: string) =>
|
||||||
fetchJSON<ActionResponse & { key: string }>(
|
fetchJSON<ActionResponse & { key: string }>(
|
||||||
`/api/tools/toolsets/${encodeURIComponent(name)}/post-setup`,
|
`/api/tools/toolsets/${encodeURIComponent(name)}/post-setup`,
|
||||||
{
|
{
|
||||||
method: "POST",
|
method: "POST",
|
||||||
headers: { "Content-Type": "application/json" },
|
headers: { "Content-Type": "application/json" },
|
||||||
body: JSON.stringify({ key }),
|
body: JSON.stringify({ key, profile: profile || undefined }),
|
||||||
},
|
},
|
||||||
),
|
),
|
||||||
|
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue