fix(hermes): scope Signet memory to named agents#504
fix(hermes): scope Signet memory to named agents#504NicholaiVogel wants to merge 8 commits intomainfrom
Conversation
|
Hi @NicholaiVogel - I'm an automated code reviewer powered by pr-reviewer. I'm taking a look at the fixes in |
PR-Reviewer-Ant
left a comment
PR-Reviewer-Ant
left a comment
There was a problem hiding this comment.
Automated review by pr-reviewer | model: claude-sonnet-4-6 | commit:
0797f727
[Automated Bot Review — pr-reviewer] PR #504 correctly scopes Hermes memory to named-agent workspaces and extends recall timeouts. Found one blocking correctness bug in the new Python HTTP helpers, one warning-level auth gap in the TS registration helper, and one test-env leak.
Confidence: High [sufficient_diff_evidence, targeted_context_included] - All three findings are directly provable from the changed lines. (1) The _delete/_patch except-clauses are quoted verbatim — json.JSONDecodeError is absent, confirming the crash path. (2) The fetch headers in ensureNamedAgentRegistered are quoted verbatim — no Authorization header is present. (3) The afterEach block is quoted verbatim — restoreEnv("SIGNET_AGENT_WORKSPACE") is absent despite SIGNET_AGENT_WORKSPACE being in originalEnv and cleared in beforeEach.
| timeout: float = _TIMEOUT_SECS, | ||
| ) -> Optional[Dict[str, Any]]: | ||
| """DELETE from the daemon. Returns parsed response or None on failure.""" | ||
| url = f"{self._base_url}{path}" |
There was a problem hiding this comment.
json.loads(resp.read().decode("utf-8")) in _delete throws json.JSONDecodeError (a subclass of ValueError) when the daemon returns an empty body (e.g. 204 No Content). The except clause only catches HTTPError, URLError, OSError, TimeoutError — JSONDecodeError is not among them. The same bug exists in _patch at the equivalent line (~line 127). The exception propagates to handle_tool_call's outer except Exception block, which means memory_forget and memory_modify return a false {"error": "Signet memory_forget failed: Expecting value: line 1 column 1 (char 0)"} to the model even when the daemon accepted and executed the request. Fix: catch (ValueError, json.JSONDecodeError) alongside the existing exceptions, or guard with body = resp.read(); return json.loads(body) if body else {}.
| } | ||
|
|
||
| try { | ||
| const resp = await fetch(`${baseUrl}/api/agents`, { |
There was a problem hiding this comment.
ensureNamedAgentRegistered makes two bare fetch calls — the GET existence check and the POST to create the agent — with no Authorization header. The Python client's _headers() always injects Bearer ${SIGNET_TOKEN} when SIGNET_TOKEN is set, exactly because the daemon may require authentication for non-localhost deployments. Without the header, both calls will receive 401 from an authenticated daemon: the GET returns non-ok (falling through to the POST), and the POST also returns non-ok, producing a warning that looks like a daemon-unreachable error rather than an auth failure. The net effect is that agent auto-registration is silently non-functional for the remote-daemon configuration the PR explicitly acknowledges (the SIGNET_TOKEN warning in the .env section just below). Fix: read SIGNET_TOKEN from process.env (stripping newlines as the rest of this function does) and include Authorization: Bearer ${token} when present.
| @@ -42,6 +47,7 @@ afterEach(() => { | |||
| restoreEnv("HERMES_HOME"); | |||
There was a problem hiding this comment.
SIGNET_AGENT_WORKSPACE is captured in originalEnv, deleted in beforeEach, and set by at least one test ("derives SIGNET_AGENT_WORKSPACE for named agents"), but restoreEnv("SIGNET_AGENT_WORKSPACE") is absent from afterEach. Any test that runs after the scoping test will inherit the leaked workspace path, which can silently skew workspace resolution in subsequent assertions. Add restoreEnv("SIGNET_AGENT_WORKSPACE") to the afterEach block alongside the other keys.
PR-Reviewer-Ant
left a comment
PR-Reviewer-Ant
left a comment
There was a problem hiding this comment.
Automated review by pr-reviewer | model: claude-sonnet-4-6 | commit:
33005759
[Automated bot review — pr-reviewer] PR #504 correctly implements named-agent workspace scoping and the recall timeout fix described in the description. Four issues found: one warning-level breaking schema change, one warning-level auth gap on a new install-time mutation, and two nitpick-level test hygiene issues.
Confidence: Medium [sufficient_diff_evidence, missing_cross_module_context] - The breaking schema change in tools.ts (entity→entityName, value→attributes) is directly visible in the diff. Whether any existing callers depend on the old field names cannot be determined without the full daemon route handler and any external consumers of the MCP memory_store structured payload. The auth gap in ensureNamedAgentRegistered is directly visible; whether /api/agents requires a bearer token depends on the daemon auth middleware which is not included in this diff.
packages/daemon/src/mcp/tools.ts
Outdated
| z.object({ | ||
| entity: z.string(), | ||
| entityName: z.string(), | ||
| aspect: z.string(), |
There was a problem hiding this comment.
Breaking schema change in the structured.aspects array: entity (string) was renamed to entityName and value (string) was replaced by attributes (array of objects). Any caller that previously passed {entity, aspect, value} tuples will now have those fields silently dropped by Zod. The Python plugin's STRUCTURED_ASPECT_SCHEMA matches the new shape, so Python→daemon is now aligned, but the rename is not backward-compatible for any TypeScript/JS callers (MCP clients, direct API consumers) already using the old schema. The PR description does not mention this as a breaking change. If the structured parameter was purely internal or never shipped to external callers, this is fine — but that should be confirmed before merge.
| try { | ||
| const resp = await fetch(`${baseUrl}/api/agents`, { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, |
There was a problem hiding this comment.
ensureNamedAgentRegistered issues an unauthenticated POST to /api/agents (no Authorization header, no SIGNET_TOKEN injection). The existing daemon auth middleware protects mutation endpoints; on any instance running in non-localhost or token-required mode this POST will return 401/403 and the agent entry will not be created. The failure is soft (warning added, install still succeeds), and the warning message is actionable, so this is not a correctness regression for local setups. However, the install experience on remote authenticated daemons will silently degrade: the named-agent scope is never registered, so subsequent Hermes sessions will fall back to unscoped memory. Consider reading SIGNET_TOKEN from env (already done for configuredDaemonUrl) and forwarding it as a bearer token here.
| @@ -42,6 +47,7 @@ afterEach(() => { | |||
| restoreEnv("HERMES_HOME"); | |||
There was a problem hiding this comment.
SIGNET_AGENT_WORKSPACE is captured in originalEnv (line 11) and deleted in beforeEach (line 33), but restoreEnv("SIGNET_AGENT_WORKSPACE") is absent from afterEach. If a future test sets this env var, it will leak into subsequent tests. Add restoreEnv("SIGNET_AGENT_WORKSPACE") to the afterEach block alongside the other restores.
| if tags: | ||
| body["tags"] = tags | ||
| if who: | ||
| body["who"] = who |
There was a problem hiding this comment.
In the score_min filtering loop: float(row.get("score", 0.0)) will raise TypeError if a result row contains "score": null (JSON null → Python None), because dict.get returns the stored value (None) rather than the default when the key exists. The outer try/except Exception in handle_tool_call catches this and returns an error JSON, but it logs at ERROR level and drops the entire result set rather than gracefully skipping the bad row. Guard with float(row.get("score") or 0.0) or an explicit None check.
PR-Reviewer-Ant
left a comment
PR-Reviewer-Ant
left a comment
There was a problem hiding this comment.
Automated review by pr-reviewer | model: claude-sonnet-4-6 | commit:
fc7f85ef
[Automated Review — pr-reviewer bot] PR #504 introduces Hermes named-agent scoping, recall timeout increases, and expanded memory_store schema. The implementation broadly matches the stated goals. Four issues are worth discussion or fixing before merge.
Confidence: High [sufficient_diff_evidence, targeted_context_included] - All findings are grounded in explicit lines in the diff and the full changed-file contents provided. The afterEach env-restore omission is directly visible in index.test.ts. The unauthenticated fetch in ensureNamedAgentRegistered is explicit in src/index.ts. The getAgentIdentityFiles scope expansion is verifiable by diffing the old and new IDENTITY_FILES constant. The unchecked typecheck/lint items are stated in the PR checklist itself.
| @@ -42,6 +47,7 @@ afterEach(() => { | |||
| restoreEnv("HERMES_HOME"); | |||
There was a problem hiding this comment.
SIGNET_AGENT_WORKSPACE is captured in originalEnv (line 10) but is never restored in afterEach. Every other key added in this PR (SIGNET_SKIP_AGENT_REGISTER) is restored; SIGNET_AGENT_WORKSPACE is the only outlier. If the env var is set in the outer process before the suite runs, its original value will be lost after the suite completes. Add restoreEnv("SIGNET_AGENT_WORKSPACE") to the afterEach block.
| } | ||
|
|
||
| try { | ||
| const resp = await fetch(`${baseUrl}/api/agents`, { |
There was a problem hiding this comment.
ensureNamedAgentRegistered fires a POST /api/agents mutation without including an auth token. All other mutation paths in this codebase (remember, recall, etc.) conditionally include Authorization: Bearer $SIGNET_TOKEN when the token is available. For remote daemon deployments where auth is required, this request will get a 401/403, and the failure message will read Could not register Signet agent '…' with shared memory policy: [auth error body] — which is misleading. The SIGNET_TOKEN value should be read from env and included here for consistency, mirroring the Python client's _headers() pattern.
| name: agentId, | ||
| read_policy: "shared", | ||
| policy_group: null, | ||
| }), |
There was a problem hiding this comment.
ensureNamedAgentRegistered unconditionally registers the named agent with read_policy: "shared". There is no way for the installer to opt for isolated or group policy via env var or flag. If a user already has a named agent with isolated policy and re-runs install (e.g. after an upgrade), the GET succeeds and the POST is skipped — so existing policy is preserved. But on first install the policy is always forced to shared. This is undocumented and could silently over-share memory. At minimum the warning emitted on failure should mention signet agent create <id> --memory isolated as an alternative so users know their options.
| @@ -56,26 +62,20 @@ export function scaffoldAgent(name: string, agentsDir: string): void { | |||
| * exists on disk for the given agent. | |||
There was a problem hiding this comment.
The old implementation gave every file in SHARED_FILES (AGENTS.md, USER.md, TOOLS.md, HEARTBEAT.md, MEMORY.md, BOOTSTRAP.md) a fixed root path — per-agent overrides were only supported for SOUL.md and IDENTITY.md. This PR expands the fallback-first lookup to all eight files including MEMORY.md and USER.md. The test at packages/core/src/__tests__/identity.test.ts:107 confirms the new behavior is intentional, but the change is broader than the PR description implies ("scope Hermes Signet memory to the named agent workspace"). Specifically, MEMORY.md being per-agent changes the multi-agent shared-memory model for any existing deployment where MEMORY.md is the single shared synthesis artifact. Callers that relied on MEMORY.md always being root-level will now receive an agent-scoped override if one exists. This should be explicitly documented in the PR or in docs/ARCHITECTURE.md / docs/PIPELINE.md to avoid silent regressions for existing multi-agent setups.
| @@ -1,5 +1,5 @@ | |||
| import { afterEach, beforeEach, describe, expect, it } from "bun:test"; | |||
There was a problem hiding this comment.
The PR checklist shows bun run typecheck and bun run lint as unchecked. Per repo AGENTS.md the PR checklist is mandatory. The diff touches TypeScript across @signet/core, @signet/daemon, and @signet/connector-hermes-agent — any silent type error (e.g. in the new IdentityFileMap parameter threading through hooks.ts) would not be caught by the 45-test run alone. Please confirm both pass before merge.
|
Addressed the review feedback in What changed:
Validation: bun test packages/daemon/src/mcp/tools.test.ts packages/connector-hermes-agent/index.test.ts packages/cli/src/commands/memory.test.ts packages/core/src/__tests__/identity.test.ts packages/daemon/src/hooks.test.ts
python3 -m py_compile packages/connector-hermes-agent/hermes-plugin/__init__.py packages/connector-hermes-agent/hermes-plugin/client.py
bunx biome check packages/connector-hermes-agent/src/index.ts packages/connector-hermes-agent/index.test.ts packages/daemon/src/mcp/tools.ts packages/daemon/src/mcp/tools.test.ts
bun run --filter '@signet/connector-hermes-agent' typecheck
bun run --filter '@signet/core' typecheck
git diff --checkResults: 105 tests pass, py_compile passed, targeted Biome passed, targeted typechecks passed, diff check passed. Full |
PR-Reviewer-Ant
left a comment
PR-Reviewer-Ant
left a comment
There was a problem hiding this comment.
Automated review by pr-reviewer | model: claude-sonnet-4-6 | commit:
5bb4dff5
Harness output could not be parsed as structured JSON.
{
"summary": "[Automated Review — pr-reviewer bot] Four issues found: one cross-language logic divergence that could cause silent mis-scoping for the default `hermes-agent` agent ID, one stale `meta.noHits` field after client-side `score_min` filtering, one registration function that proceeds to POST on any non-OK GET (not just 404), and one breaking behavioral change in `getAgentIdentityFiles` for existing named-agent deployments.",
"verdict": "comment",
"confidence": {
"level": "medium",
"reasons": ["sufficient_diff_evidence", "missing_cross_module_context"],
"justification": "Issues 1-3 are directly provable from the changed lines. Issue 4 depends on whether any existing named-agent deployments have files in their agent subdirectory other than SOUL.md/IDENTITY.md — that runtime state is not visible in the diff alone."
},
"ui_screenshot_needed": false,
"comments": [
{
"file": "packages/connector-hermes-agent/hermes-plugin/__init__.py",
"line": 252,
"body": "**Logic divergence from the TypeScript connector.** `_resolve_agent_workspace` checks `agent_id != \"default\"` but does not exclude `\"hermes-agent\"` the way the TS connector does:\n\n```typescript\n// src/index.ts\n} else if (signetAgentId && signetAgentId !== \"hermes-agent\" && signetAgentId !== \"default\") {\n```\n\nConsequence: if `~/.agents/agents/hermes-agent/` exists on disk (a plausible layout when Hermes runs as a named agent), the Python plugin will silently adopt that directory as `self._project` and send it as the session-start `project`. The TS connector, however, will not write `SIGNET_AGENT_WORKSPACE` for `hermes-agent`, so the daemon sees a project path that was not explicitly configured. Add `agent_id != \"hermes-agent\"` to the guard to match the TS side.",
"severity": "warning"
},
{
"file": "packages/connector-hermes-agent/hermes-plugin/client.py",
"line": 396,
"body": "**`meta.noHits` is not updated after client-side `score_min` filtering.** After the filter, `result[\"results\"]` may be empty (`kept = []`) while `meta[\"noHits\"]` remains `False` (the daemon's original response had results). `totalReturned` is patched to `len(kept)` correctly, but `noHits` is left stale. Any caller that reads `meta.noHits` to decide whether to show a \"no results\" message will get the wrong signal when score thresholding removes all rows. Fix:\n\n```python\nresult[\"meta\"] = {**meta, \"totalReturned\": len(kept), \"noHits\": len(kept) == 0}\n```",
"severity": "warning"
},
{
"file": "packages/connector-hermes-agent/src/index.ts",
"line": 165,
"body": "**`ensureNamedAgentRegistered` proceeds to POST on any non-OK GET, not just 404.** If the daemon returns 401 (auth failure) or 500, `getResp.ok` is false, the catch block is skipped, and the code falls through to attempt a POST creation. The POST will also fail and generate a warning like "Could not register Signet agent 'X'…" — which misleads the user into thinking the agent doesn't exist, when the real issue is auth or a transient daemon error.\n\nConsider checking `getResp.status === 404` explicitly before falling through, or at minimum gating the POST on status 404:\n\n```typescript\nif (getResp.status !== 404) return; // already exists or unrecoverable error\n```",
"severity": "warning"
},
{
"file": "packages/core/src/agents.ts",
"line": 72,
"body": "**Behavioral breaking change for existing named-agent deployments.** Previously only `SOUL.md` and `IDENTITY.md` were checked in the agent subdirectory; all other files (`AGENTS.md`, `USER.md`, `TOOLS.md`, `HEARTBEAT.md`, `MEMORY.md`, `BOOTSTRAP.md`) always resolved from the workspace root. This PR changes the rule so all files check the agent subdir first.\n\nAny named-agent directory that already contains a file from the newly promoted set (most likely `AGENTS.md` or `MEMORY.md` — e.g., scaffolded by `scaffoldAgent` in an earlier version, or written by a user) will silently switch from reading the workspace-root version to the agent-local version on next daemon restart, with no migration warning.\n\nThis is the core intent of the PR and is documented correctly in ARCHITECTURE.md. Flagging here so reviewers confirm there is no automated migration concern or need for a startup warning when unexpected files are found in agent subdirectories.",
"severity": "warning"
}
]
}
[stderr]
SessionEnd hook [signet hook session-end -H claude-code] failed: /bin/sh: line 1: signet: command not found
|
Follow-up setup pass pushed in This makes Hermes Agent a real setup harness instead of a hidden non-interactive option:
Additional validation: bun test packages/daemon/src/mcp/tools.test.ts packages/connector-hermes-agent/index.test.ts packages/cli/src/commands/memory.test.ts packages/core/src/__tests__/identity.test.ts packages/daemon/src/hooks.test.ts packages/cli/src/features/setup-shared.test.ts
python3 -m py_compile packages/connector-hermes-agent/hermes-plugin/__init__.py packages/connector-hermes-agent/hermes-plugin/client.py
bunx biome check packages/core/src/identity.ts packages/core/src/index.ts packages/core/src/__tests__/identity.test.ts packages/connector-hermes-agent/src/index.ts packages/cli/src/features/setup.ts packages/cli/src/features/setup-migrate.ts packages/cli/src/features/setup-shared.test.ts packages/cli/src/commands/app.ts docs/CLI.md
bun run --filter '@signet/core' typecheck
bun run --filter '@signet/connector-hermes-agent' typecheck
git diff --checkResult: 118 tests pass, py_compile passed, targeted Biome passed, targeted typechecks passed, diff check passed. |
PR-Reviewer-Ant
left a comment
PR-Reviewer-Ant
left a comment
There was a problem hiding this comment.
Automated review by pr-reviewer | model: claude-sonnet-4-6 | commit:
7fa4bbc6
Harness output could not be parsed as structured JSON.
You're out of extra usage · resets 10pm (America/Denver)
[stderr]
SessionEnd hook [signet hook session-end -H claude-code] failed: /bin/sh: line 1: signet: command not found
|
Follow-up review pass pushed in Handled the actionable feedback:
Triage on the other inline comments:
Validation rerun: bun test packages/daemon/src/mcp/tools.test.ts packages/connector-hermes-agent/index.test.ts packages/cli/src/commands/memory.test.ts packages/core/src/__tests__/identity.test.ts packages/daemon/src/hooks.test.ts packages/cli/src/features/setup-shared.test.ts
python3 -m py_compile packages/connector-hermes-agent/hermes-plugin/__init__.py packages/connector-hermes-agent/hermes-plugin/client.py
bunx biome check packages/connector-hermes-agent/src/index.ts packages/connector-hermes-agent/index.test.ts packages/core/src/__tests__/identity.test.ts packages/daemon/src/mcp/tools.ts packages/daemon/src/mcp/tools.test.ts
bun run --filter '@signet/core' typecheck
bun run --filter '@signet/connector-hermes-agent' typecheck
git diff --checkResult: I also attempted the repo-level gates again:
The PR body has been updated with the current test count and the full-suite caveats. |
2 participants
Summary
Fixes Hermes Signet integration in three related ways:
Changes
SIGNET_AGENT_IDandSIGNET_AGENT_WORKSPACE.signet setupharness in interactive setup, non-interactive help text, migration detection, and setup docs.user-prompt-submitrecall hooks.signet recallCLI timeout to 30s for/api/memory/recall.hints,transcript, andstructuredsupport to the Hermesmemory_storeschema and daemon payload forwarding.memory_storeschema and payload forwarding with the daemon remember endpoint for hints, transcript, and structured graph metadata.memory_storesurface.Type
feat- new user-facing feature (bumps minor)fix- bug fixrefactor- restructure without behavior changechore- build, deps, config, docsperf- performance improvementtest- test coveragePackages affected
@signet/core@signet/daemon@signet/cli/ dashboard@signet/sdk@signet/connector-*@signet/webpredictorScreenshots
N/A. No UI changes.
PR Readiness (MANDATORY)
INDEX.md+dependencies.yaml)Migration Notes (if applicable)
N/A. No database migrations touched.
Compatibility note: these changes are additive to existing Hermes and MCP schemas. Existing memory tool calls remain valid, including legacy structured aspect tuples using
{entity, aspect, value}. Named agents may provide agent-local identity overrides for the standard identity files, includingMEMORY.md; if no agent-local file exists, the root workspace file remains the fallback.Testing
bun testpassesbun run typecheckpassesbun run lintpassesCommands run locally:
Result:
Full-suite note:
bun run typecheckwas also attempted. It still exits 2 on existing unrelated@signet/*-pi*package failures around missing@signet/pi-extension-baseand connector-base exports. The packages touched by this PR that expose typecheck scripts pass.bun run lintwas also attempted. It still exits 1 on the current repo-wide Biome baseline, mainly formatting diagnostics in existing package manifests outside this PR. Targeted Biome checks for this PR's changed TypeScript files pass.Manual validation:
signet recall --json "Nicholai durable facts Signet memory"succeeds locally after increasing the timeout.memory_search, and returns recall results instead of timing out.hints,transcript, andstructuredonmemory_store.AI disclosure
Notes
The broader recall ranking issue is separate from this schema fix. This PR makes the fields available and forwarded correctly so callers can provide retrieval-friendly hints instead of being limited to content, type, importance, tags, pinned, and project.