fix(*): report an invalid memory identity as the config error it is

[gloryfromca]

Summary

An identity EverOS cannot accept becomes a directory segment on its write path, so EverosBackend.start() rejects it. The rejection carried the one message a user can act on -- it names the key and the accepted pattern -- and no surface delivered it as one. Three of the four call sites fold the raise into logger.exception; under the TUI loguru is file-only, so there an invalid identity turned memory off with nothing on screen. The fourth, in import_commands._build_and_run, is not wrapped at all, so the ValueError escaped as a bare traceback. Where it was visible it was a traceback, which is not a thing a user reads as "edit this key".

The state machine then told the user the opposite of the truth. start() died before assigning a state, so _state stayed UNKNOWN, which is not in _NEVER_HAD_MEMORY; every store() after it counted a dropped write, and the session ended with:

1 turn(s) were not written to long-term memory because the memory service was
unavailable.

The service was fine. That sends the user to debug a server that never broke.

The change. Catch the ValueError in start(), give it a BAD_IDENTITY state, and file that state with the other two the user must fix themselves:

• terminal, so a stray probe cannot promote it to READY;
• never-had-memory, so no dropped-write count and no outage wording;
• the message goes to stderr, matching what the unconfigured-LLM and missing-binary paths beside it already do.

Two things came out of running it rather than reading it:

1. The message needs escaping. It quotes the accepted-character class, and rich reads [a-zA-Z0-9_.@+-] as a markup tag and eats it, leaving the user the pattern ^+$ to match their id against. The test now pins the full class.
2. raven import had the same defect on its own gate. It kept the outage wording and pointed at the server log, which holds nothing about a config error. It now names the config keys instead.

Before / after, same config (memory.userId: "bad/id"), same command:

- 1 turn(s) were not written to long-term memory because the memory service was
- unavailable.

+ Long-term memory is off: memory.userId='bad/id' is not accepted by EverOS: it
+ becomes a directory segment on the write path, so it must match
+ ^[a-zA-Z0-9_.@+-]+$ and must not be '.' or '..'.
+ Fix memory.userId / memory.agentId in your config.json, then start a new session.

Type

• Fix
• Feature
• Docs
• CI / tooling
• Refactor
• Other

Verification

Re-run after rebasing onto d35e398c, the current main.

uv run pytest tests/test_everos_backend.py tests/test_cli_import_commands.py \
              tests/test_everos_plugin_discovery.py -q
162 passed

uv run pytest -q
1 failed, 6478 passed, 43 skipped, 13 deselected

uv run ruff check <the four touched files>            All checks passed
uv run ruff format --check <the four touched files>   4 files already formatted

The one failure is tests/test_cli_theme.py::test_bold_accent_renders_styled_not_bare. It is pre-existing on this base: checked out d35e398c in a clean worktree and ran uv run pytest tests/test_cli_theme.py there, same failure, 1 failed / 44 passed. It passes in isolation and fails when the file runs as a whole, so it is order-dependent, and it is unrelated to anything here. That file is untouched by this change.

Manual check, sandbox RAVEN_HOME, memory.userId: "bad/id", raven agent -m "hi": prints the block quoted above, and the session no longer ends with the dropped-write line.

• Relevant tests pass locally
• Relevant lint / type checks pass locally
• User-facing docs or screenshots are updated when needed

Risk

Terminal-only wording plus one new enum member. No config, schema, or protocol change; an identity that was accepted before is accepted now, on the same code path. The only behaviour change for a valid setup is none.

For an invalid identity the turn behaves as it did -- memory off, turn completes -- and only what the user is told changes. Rollback is reverting the single commit.

ServiceState.BAD_IDENTITY is new. Every consumer was checked: raven/cli/import_commands.py (handled here) and tests/test_everos_plugin_discovery.py (reads READY only). _may_spawn allowlists UNKNOWN via _SPAWNABLE_STATES, so a new member is non-spawnable without a change.

• Security impact considered
• Backward compatibility considered
• Rollback path is clear for risky changes

Related Issues

Fixes #271

[claude]

Claude encountered an error after 0s —— View job

---

I'll analyze this and get back to you.

[gloryfromca]

No blockers; this can merge as far as I am concerned.

What I checked

The diff, the four start() call sites, the ServiceState machine, backward compatibility, the project rules in AGENTS.md / CONTEXT*.md, and whether the tests were weakened.

The change holds up

The premise checks out. _validate_identity is called from exactly one place (start()), and all four callers -- agent_commands.py:314, gateway_commands.py:398, tui_commands.py:746, import_commands.py:153 -- do fold a raise into logger.exception. Nothing else in the tree references _PATH_SAFE_ID_RE or _validate_identity, so no caller loses a signal by the raise becoming a state.

The state placement is right on both axes. _TERMINAL_STATES: identity comes from the ServiceLocator at construction and is never re-read, so no probe could ever promote out of it -- which is also why "start a new session" is the correct remedy in the message. _NEVER_HAD_MEMORY: with an invalid id no write would land regardless of the server's health, so counting the turn as a dropped write and blaming the service would send the user to debug something that never broke.

One thing the commit message understates: _state is initialised to READY when an adapter is injected (backend.py:379), not just to UNKNOWN. On that path the old raise left the state at READY, so store() would go on to write with the rejected identity. Setting BAD_IDENTITY closes that too.

_kick_probe returns before touching _last_probe_at for terminal states, so recall() on a bad identity does no work per turn.

The escape() is not theoretical -- I confirmed rich swallows the bracket class without it, and the test asserts the rendered form survives.

Repo rules

Commit header 65 chars, lowercase, ASCII, (*) scope for a two-package change; Co-authored-by trailer present. Both test files are existing ones extended rather than new files, per 5.4. Comments are English and carry a why. No new domain term needs a CONTEXT.md entry -- ServiceState members are not documented there. No assets touched.

Tests

Not weakened. test_illegal_identity_rejected_on_start became test_illegal_identity_is_reported_as_a_config_error, and the replacement asserts strictly more: the key name reaches stderr, the character class survives rich's markup parser, and the state lands on BAD_IDENTITY. The new outage test pins _dropped_writes == 0 and the absence of the "unavailable" line at stop().

Ran on 88984eb21613:

• uv run pytest tests/test_everos_backend.py tests/test_cli_import_commands.py -- 149 passed, 0 skipped.
• uv run pytest tests/ -- 6464 passed, 43 skipped, 1 failed.

The failure is tests/test_cli_theme.py::test_bold_accent_renders_styled_not_bare. It is not from this PR: that file is untouched, the failure reproduces from the file alone (pytest tests/test_cli_theme.py -- intra-file order pollution, it passes when run as a single node id), and it fails identically on origin/main in a clean worktree. Flagging it as a pre-existing bug someone should pick up separately, not as something this branch needs to fix. The 43 skips are the optional channel SDKs plus parametrize-level skips, all pre-existing.

One nit, non-blocking

On the raven import run path the user now gets the remedy twice with two different endings: start() prints to stderr "...Fix memory.userId / memory.agentId in your config.json, then start a new session", and _require_memory_service_ready then prints to stdout "...Fix memory.userId / memory.agentId in your config.json, then: raven import run". The second is the correct advice for a one-shot command and the first is not, but both are on screen. Options, in the order I would rank them: leave it (the wording is close enough that the reader takes the last one); or have the import gate print only the first line it adds -- "Memory identity is invalid; nothing would be imported" plus "Retry: raven import run" -- and let start()'s message carry the key names, since it always runs first on that path. Your call, and either is fine to merge as-is.

[0xKT]

One describe-level correction before merge, since the PR body becomes the squash
commit body on main.

R1 (describe): "Every caller folds the raise into logger.exception" holds for
agent_commands.py:311, gateway_commands.py:395 and tui_commands.py:743, but not
for import_commands.py:147 -- that call sits outside the try, so the ValueError
propagates out of _build_and_run as a bare traceback. #271 says the same thing
from the other side: "The agent REPL and the gateway do print the traceback, so
the silent case is the TUI specifically."

The fix is unaffected -- it replaces a traceback with an actionable line on those
surfaces and silence with one on the TUI. Only the sentence needs a rewrite, in
the PR body (it lands on main) and ideally the commit body too.

[gloryfromca]

R1 is real, and I owe you a correction of my own: my review said "all four callers -- agent_commands.py:314, gateway_commands.py:398, tui_commands.py:746, import_commands.py:153 -- do fold a raise into logger.exception". I read the region and took the try: that follows _require_memory_service_ready as wrapping the start() above it. It does not. So my review confirmed the wrong sentence rather than catching it.

Verified inventory on 88984eb21613:

Surface	What the user got before this change
agent REPL, agent_commands.py:314	except Exception: logger.exception. No redirect_loguru_to_file on this path, so loguru's default stderr sink is live -- traceback on the terminal.
gateway, gateway_commands.py:398	except Exception: logger.exception. redirect_loguru_to_file(terminal_level=log_cfg.console_level), and console_level defaults to "INFO" (schema.py:713), so the ERROR record reaches stderr -- traceback on the terminal.
TUI, tui_commands.py:746	except Exception: logger.exception. redirect_loguru_to_file("tui.log", ...) leaves terminal_level=None, so the sink is file-only -- nothing on screen.
raven import run / onboard import, import_commands.py:151	Not wrapped at all. The ValueError escapes _build_and_run and both call sites (import_commands.py:670, onboard_commands.py:2754) -- neither has an except -- so typer prints a bare traceback and the command aborts.

So your quote from #271 is the accurate framing: three surfaces showed it as a traceback, and the TUI is the one that was silent. I suspect "every caller" came from #271's own list, which enumerates four except Exception: logger.exception sites but reaches four by counting agent_commands twice (:402-407 and :499-505, both stale -- there is one backend.start() in that file today, at :314) and never mentions import_commands.

Suggested replacement for the two sentences in Summary:

The rejection carried the one message a user can act on -- it names the key and the accepted pattern -- and no surface delivered it as one. Three of the four call sites fold the raise into logger.exception; under the TUI loguru is file-only, so there an invalid identity turned memory off with nothing on screen. The fourth, import_commands.py:151, is not wrapped at all, so the ValueError escaped as a bare traceback. Where it was visible it was a traceback, which is not a thing a user reads as "edit this key".

That also keeps the "one-shot CLI run writes no log file" clause honest -- it is true of the agent REPL, but raven import does write import.log (import_commands.py:574); the ValueError just never went through loguru to land in it.

R2, same describe-only class, since you are editing the body anyway. The Risk section says "_may_spawn iterates the enum, so it covers the new member without a change." It does not iterate anything: _may_spawn is return self._state in _SPAWNABLE_STATES, and _SPAWNABLE_STATES is the allowlist frozenset({ServiceState.UNKNOWN}). The conclusion is right and the reasoning is backwards -- a new member is non-spawnable because the set is an allowlist of one, not because anything enumerates ServiceState. Nothing in raven/ iterates that enum. Accurate version: "_may_spawn allowlists UNKNOWN, so a new member is non-spawnable without a change."

On scope: editing the PR description is sufficient for what lands on main -- the repo squash-merges with squash_merge_commit_message=PR_BODY, so the description becomes the commit body and the individual commit bodies are dropped. Rewriting the commit body too would mean a rebase and a force-push, which AGENTS.md 3.4 puts behind an explicit ask; worth it only if you want git show on the branch to read correctly before the squash. Your call.

Agreed that the fix itself is unaffected: on all four surfaces it replaces a traceback or silence with an actionable line, and the import path keeps the same exit code (1) while trading the traceback for two printed lines. No blockers on the code from me.

[gloryfromca]

Blocking, and only on prose: the R1 correction went into the commit body, which the squash throws away, and not into the PR description, which is the copy that lands on main. The code is byte-identical to what I signed off and still has my sign-off.

What moved since 88984eb21613

Two things, no code: a rebase onto d35e398c (#349 landed), and the commit body reworded to take R1. I diffed the branch's own content at both heads -- git diff 5edcda9c 88984eb21613 against git diff d35e398c 1fc4c5d601d4 -- and they are identical, so the earlier review of the implementation stands unchanged.

Blocker: the fix is in the discarded half

1fc4c5d6's body now reads correctly ("Three of the four call sites fold the raise into logger.exception ... The fourth, in import_commands._build_and_run, does not wrap the call at all"). The PR description still opens with the sentence @0xKT asked to have rewritten:

The rejection carried the one message a user can act on -- it names the key and the accepted pattern -- and nobody ever saw it. Every caller folds the raise into logger.exception, and a one-shot CLI run writes no log file for it to land in.

That is the copy that survives. From the repo API:

allow_squash_merge:            true
allow_merge_commit:            false
allow_rebase_merge:            false
squash_merge_commit_message:   PR_BODY
squash_merge_commit_title:     PR_TITLE

Squash is the only method enabled, so there is no path on which 1fc4c5d6's body reaches main -- AGENTS.md 3.3 says the same thing, and the API confirms it is configured, not just documented. Merge as it stands and main permanently records "Every caller folds the raise into logger.exception" while the accurate paragraph is dropped, which is the exact inversion of what @0xKT asked for: "in the PR body (it lands on main) and ideally the commit body too." The "ideally" half is done; the required half is not.

The failure is not recoverable after the fact -- correcting it post-merge means rewriting main. That is why I am calling it blocking rather than a nit, on a change whose code I would otherwise approve as-is.

Fix is a description edit, no code and no force-push: replace that paragraph in ## Summary with the one already written in 1fc4c5d6's body. If you would rather keep the two in sync permanently, the cheap habit is to write the paragraph once and paste it both places at push time; nothing enforces it, and this is the failure mode it has.

While you are in there: R2 from my last comment is still live at the same address. ## Risk still says "_may_spawn iterates the enum, so it covers the new member without a change", and _may_spawn is return self._state in _SPAWNABLE_STATES over the one-member allowlist frozenset({ServiceState.UNKNOWN}). Right conclusion, wrong mechanism, and it lands on main with the same merge. Details in the earlier comment rather than repeated here.

Rebase verification (AGENTS.md 3.5 step 4)

Ran on 1fc4c5d601d4, after the rebase:

• uv run pytest tests/test_everos_backend.py tests/test_cli_import_commands.py -- 149 passed, 0 skipped.
• uv run pytest tests/ -- 6478 passed, 43 skipped, 1 failed. The +14 over the 6464 I saw at the old head are #349's tests arriving with the new base, not anything here.

The one failure is tests/test_cli_theme.py::test_bold_accent_renders_styled_not_bare again. Still pre-existing and still unrelated: raven/cli/_theme.py and its test are untouched both by this branch and by 5edcda9c..d35e398c, and it reproduces from the file alone (pytest tests/test_cli_theme.py -- 1 failed / 44 passed) while passing as a single node id, so it is intra-file order pollution, not a base or rebase effect. The 43 skips are the optional channel SDKs plus parametrize-level skips, unchanged.

Covered

The delta between the two heads, the rebase (new base, re-run tests, no new failures), the PR description and commit body against AGENTS.md 3.1.1 and 3.3 (both ASCII-clean, header 67 chars, Fixes #271 present, Co-authored-by present), and whether the reword changed any code (it did not). The implementation, callers, state machine, back-compat and test-weakening checks are from the earlier review and unchanged.

[gloryfromca]

Both corrections are in, in the PR body and in the commit body.

R1

Confirmed independently before editing. On the pre-change code the four backend.start() call sites are agent_commands.py:314, gateway_commands.py:398 and tui_commands.py:746 -- each inside try: ... except Exception: logger.exception(...) -- and import_commands._build_and_run, where the try: opens after _require_memory_service_ready(backend), so the start() above it is unwrapped and the ValueError leaves the coroutine. So the original sentence was wrong on the fourth site, and #271's framing is the accurate one.

I used gloryfromca's replacement paragraph with one change: import_commands._build_and_run instead of a line number. :147 (0xKT) and :151 (gloryfromca) are both off -- the call is at :153 post-change, :147 pre-change -- and a line number in a squash commit body goes stale on the next edit to that file anyway.

Dropping "a one-shot CLI run writes no log file for it to land in" also removes the inaccuracy about import.log, as noted.

R2

Also confirmed: _may_spawn is return self._state in _SPAWNABLE_STATES, and _SPAWNABLE_STATES = frozenset({ServiceState.UNKNOWN}). Nothing under raven/ iterates ServiceState. Reworded to "_may_spawn allowlists UNKNOWN via _SPAWNABLE_STATES, so a new member is non-spawnable without a change."

Scope

Both bodies were rewritten rather than the PR description alone, so git show on the branch reads correctly before the squash. That took a rebase and a force-push, authorized explicitly for this. The branch is now on top of d35e398c (main had moved one commit). No file overlap with this change; relevant tests re-run after the rebase.

The non-blocking nit from the review: leaving it

On raven import run the user sees start()'s "...then start a new session" on stderr and the gate's "...then: raven import run" on stdout.

Option b -- have the gate print only its own first line plus Retry: raven import run -- does not actually resolve what it identifies. start()'s "then start a new session" stays on screen either way, so the reader still gets two endings; b only removes the duplicated key names. It also costs an assertion: test_a_bad_identity_does_not_send_the_user_to_the_server_log calls _require_memory_service_ready in isolation and asserts memory.userId is in its output. Under b that assertion has to go, and "the key names are on screen" stops being tested and starts depending on the two functions running in that order on that path.

The clean version is start() describing only what is wrong and which key to edit, with each caller owning its own retry line -- but that means touching the agent, TUI and gateway call sites to add "start a new session" there, which is a wider change than this fix. Leaving the duplication as-is.

[gloryfromca]

Blocker lifted. No blockers; this can merge as far as I am concerned.

Both edits verified against the live PR description, not taken on report:

• R1 -- ## Summary now reads "no surface delivered it as one. Three of the four call sites fold the raise into logger.exception; under the TUI loguru is file-only ... The fourth, in import_commands._build_and_run, is not wrapped at all, so the ValueError escaped as a bare traceback." That is the accurate inventory, and the "one-shot CLI run writes no log file" clause is gone with it. Since squash takes PR_BODY, this is now the copy that lands on main.
• R2 -- ## Risk now reads "_may_spawn allowlists UNKNOWN via _SPAWNABLE_STATES, so a new member is non-spawnable without a change." Matches backend.py: return self._state in _SPAWNABLE_STATES over frozenset({ServiceState.UNKNOWN}).

Description is still ASCII-clean (grep -nP "[^\x00-\x7F]" on the fetched body, 0 matches), Fixes #271 intact, and the head sha is unchanged at 1fc4c5d601d4, so the commit body is the one I already checked and the code is byte-identical to what I reviewed.

I also re-ran the three numbers the Verification section claims, since this whole thread has been about claims matching reality:

uv run pytest tests/test_everos_backend.py tests/test_cli_import_commands.py               tests/test_everos_plugin_discovery.py -q     162 passed
uv run ruff check <the four touched files>                 All checks passed
uv run ruff format --check <the four touched files>        4 files already formatted

All three match, and the 1 failed, 6478 passed, 43 skipped full-suite line matches what I measured independently on this head last round -- the failure being the pre-existing test_cli_theme.py order-dependence in an untouched file.

Nothing outstanding from me on this revision.

[gloryfromca]

@claude

[claude]

Claude encountered an error after 1s —— View job

---

I'll analyze this and get back to you.