Superseded: fix(hermes): scope Signet memory to named agents#501
Superseded: fix(hermes): scope Signet memory to named agents#501NicholaiVogel wants to merge 3 commits intomainfrom
Conversation
Assisted-by: Codex:gpt-5.4
NicholaiVogel
force-pushed
the
codex/hermes-agent-scoped-identity
branch
from
5cf1ec9 to
d7aaf8d
Compare
|
Marking as draft to prevent bot token burn. |
|
Hi @NicholaiVogel - I'm an automated code reviewer powered by pr-reviewer. I'm taking a look at the fixes in |
|
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:
d7aaf8d3
[Automated Bot Review] Identified one behavioral regression (priority inversion in identity loading) and two lower-severity issues.
Confidence: High [sufficient_diff_evidence, targeted_context_included] - The priority inversion in loadIdentity is directly provable from two changed functions in the diff: getAgentIdentityFiles now includes root fallback paths in the returned map (packages/core/src/agents.ts), and loadIdentity checks identityFiles["IDENTITY.md"] before agent.yaml (packages/daemon/src/hooks.ts:1073-1110). For any non-default named agent without an agent-local IDENTITY.md, getAgentIdentityFiles returns the root IDENTITY.md path under the key "IDENTITY.md", which loadIdentity then returns from immediately — bypassing agent.yaml. The regression is traceable end-to-end without runtime reproduction.
Unmapped findings (not on changed lines):
- packages/connector-hermes-agent/index.test.ts:47 - Nitpick:
SIGNET_AGENT_WORKSPACEnot restored inafterEach.
SIGNET_AGENT_WORKSPACE is captured in originalEnv and deleted in beforeEach, but afterEach does not call restoreEnv("SIGNET_AGENT_WORKSPACE"). For all tests except the last in the suite the beforeEach delete compensates, but the outer process env is not restored after the final test. Minor test hygiene issue, not a correctness bug in production code.
|
|
||
| const agentYaml = join(getAgentsDir(), "agent.yaml"); | ||
| if (existsSync(agentYaml)) { | ||
| try { |
There was a problem hiding this comment.
Blocking: Priority inversion — root IDENTITY.md now supersedes agent.yaml for all non-default named agents.
Before this PR the order was: (1) agent.yaml, (2) root IDENTITY.md. After, loadIdentity(identityFiles) checks identityFiles["IDENTITY.md"] first. For a named agent without an agent-local IDENTITY.md, getAgentIdentityFiles populates the map with the root fallback ($SIGNET_PATH/IDENTITY.md). That path is truthy and exists, so loadIdentity returns immediately and never reaches the agent.yaml block.
Concrete scenario that breaks: any named-agent session where (a) agent.yaml is the authoritative identity source, (b) the agent has no agent-local IDENTITY.md, and (c) root IDENTITY.md exists. The resulting identity.name will come from root IDENTITY.md (parsed via parseIdentityMarkdown) instead of from agent.yaml.
The test in hooks.test.ts sidesteps this by giving dot an agent-local IDENTITY.md, so it never exercises the fallback path.
Fix: only treat identityFiles["IDENTITY.md"] as higher priority than agent.yaml when the path is agent-specific (i.e. located under agents/{name}/). One approach: check whether the resolved path starts with join(getAgentsDir(), "agents", agentId) before returning early, otherwise fall through to the existing agent.yaml → root IDENTITY.md order.
| "type": "object", | ||
| "properties": { | ||
| "id": {"type": "string", "description": "Memory ID to modify."}, | ||
| "content": {"type": "string", "description": "New content."}, |
There was a problem hiding this comment.
Warning: path traversal via unsanitized agent_id in workspace resolution.
agent_id is used directly in agents_root / "agents" / agent_id. If SIGNET_AGENT_ID is set to a value containing .. (e.g. ../../etc), candidate resolves outside agents_root. The candidate.exists() guard reduces exploitability but does not eliminate it — a predictable path like ../../.agents would exist and be returned as the project workspace sent to the daemon.
The TS installer side has the same issue (join(expandedBasePath, "agents", signetAgentId) with only newline stripping). Since SIGNET_AGENT_ID is operator-controlled this is low-severity, but a .. check or Path.resolve() comparison against agents_root would close the gap.
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:
7978025f
Automated bot review (pr-reviewer). This PR wires named-agent identity scoping end-to-end for Hermes: expanding getAgentIdentityFiles to cover all identity files, threading the resolved map through loadIdentity/handleSessionStart, adding SIGNET_AGENT_WORKSPACE derivation in the connector installer, and renaming the Hermes plugin tools to the canonical memory_* names. The core intent is sound and the happy-path test (Dot's scoped identity, not Rose's) passes. Two previously-flagged items remain unresolved and one new test defect is introduced.
Confidence: High [sufficient_diff_evidence, targeted_context_included] - All three findings are directly provable from the changed lines. (1) loadIdentity in hooks.ts now checks identityFiles?.['IDENTITY.md'] before agent.yaml; getAgentIdentityFiles returns the root IDENTITY.md as a fallback when no agent-local copy exists, so the root file wins over agent.yaml for those agents — verifiable by reading the two functions together. (2) _sanitize_env in init.py strips only whitespace/newlines; .. sequences in SIGNET_AGENT_ID are passed unchanged to the Path constructor — verifiable in the added lines. (3) The broken not.toContain assertion uses JavaScript string-literal \n (two chars: backslash+n) against file content that contains real newlines — the assertion can never catch the regression it claims to guard.
|
|
||
| const agentYaml = join(getAgentsDir(), "agent.yaml"); | ||
| if (existsSync(agentYaml)) { | ||
| try { |
There was a problem hiding this comment.
[Blocking] Priority inversion: root IDENTITY.md now supersedes agent.yaml for named agents that have no agent-local IDENTITY.md.
getAgentIdentityFiles returns the root IDENTITY.md as a fallback when agents/{name}/IDENTITY.md does not exist (see agents.ts loop). loadIdentity then checks identityFiles?.['IDENTITY.md'] — which is the root file — before it checks agent.yaml. Any installation that keeps identity in agent.yaml alongside a root IDENTITY.md will silently have the wrong name/description for every named agent that lacks its own local override.
Pre-PR order: agent.yaml → root IDENTITY.md. Post-PR order (for named agents without a local IDENTITY.md): root IDENTITY.md → agent.yaml → root IDENTITY.md (never reached).
Fix options:
- Only put the agent-local path in
identityFiles['IDENTITY.md']; omit root-level fallbacks for files thatloadIdentityalready handles itself. - Check
agent.yamlbefore the identity-files map, keeping the existing priority.
| MEMORY_GET_SCHEMA, | ||
| MEMORY_LIST_SCHEMA, | ||
| MEMORY_MODIFY_SCHEMA, | ||
| MEMORY_FORGET_SCHEMA, |
There was a problem hiding this comment.
[Warning] Path traversal via unsanitized agent_id in _resolve_agent_workspace.
_sanitize_env strips leading/trailing whitespace and \r/\n characters, but does not strip path-separator sequences such as ../. agent_id sourced from SIGNET_AGENT_ID is passed directly to agents_root / 'agents' / agent_id. A value like ../../sensitive-dir produces a path outside $SIGNET_PATH/agents/ and, if that directory exists, the constructed path is returned as the active workspace and forwarded to the daemon as project. The .exists() guard limits exploitation to pre-existing directories, but it does not close the traversal; it merely trades arbitrary path write for path disclosure and potential cross-agent workspace aliasing.
Fix: reject any agent_id that contains /, \, or the substring .. before constructing the candidate path.
| expect(client).toContain('body["agentId"] = self._agent_id'); | ||
| expect(client).not.toContain('"agentId": self._agent_id,\\n }\\n if min_score'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
[Warning] Broken not.toContain assertion — always passes, never catches the regression it claims to guard.
expect(client).not.toContain('"agentId": self._agent_id,\\n }\\n if min_score');readFileSync(..., 'utf-8') returns a string with real newline characters (\n, one char). The test pattern uses JavaScript \\n — a two-character backslash+n literal — which can never appear in a file read with real newlines. This assertion will always pass regardless of whether the old behaviour is re-introduced, so it provides no regression protection for the stated invariant ("agentId not forced into all recall requests").
Fix: replace the \\n sequences with actual newlines inside a template literal, or restructure the test to instantiate a SignetClient and assert on the outgoing request body directly.
|
Superseded by #504 on the clean |
3 participants
Summary
Fixes Hermes Signet memory setup for named agents. A Hermes process configured as
dotnow loads Dot's scoped identity files instead of falling back to the root workspace identity.Changes
@signet/coreso agents can override the full identity file set, includingAGENTS.md,IDENTITY.md,MEMORY.md,USER.md, and related files.agentIdbefore reading prompt identity content.You are ...style identity files in addition toname:metadata.SIGNET_AGENT_WORKSPACEsupport to the Hermes memory plugin.SIGNET_AGENT_IDpoints at an existing agent.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)
No migrations touched.
Testing
bun test packages/core/src/__tests__/identity.test.ts packages/daemon/src/hooks.test.ts packages/connector-hermes-agent/index.test.tspython3 -m py_compile packages/connector-hermes-agent/hermes-plugin/__init__.pybun run --filter '@signet/connector-hermes-agent' typecheckbun run build:corebun run --filter '@signet/connector-hermes-agent' buildAlso smoke-tested on the Solvr Mac with Hermes configured as Dot. Session-start returned
Dorothy "Dot" Ashby, used/Users/solvrmac/.agents/agents/dotas project, and did not injectYou are Rose.AI disclosure
Assisted-bytags in commits)Notes
Root cause: Hermes provided an agent id, but the memory plugin still sent the current Hermes process directory as
project, while daemon session-start read root identity files. This let a named Hermes agent receive root identity instructions. The fix aligns Hermes with Signet's existing multi-agent model instead of adding a memory-only identity workaround.