fix(skills): persist user skill selection across process restarts#3
Merged
Conversation
UserStateRegistry was a bare in-memory dict. POST /api/skills mutated it
and nothing wrote to disk, so every process restart snapped users back to
DEFAULT_SOUL_SLUG (and whatever voice that skill carried).
- session_store: add load_skill_slug / save_skill_slug writing to
{SESSION_STORE_DIR}/{user_id}/skill.txt (same file-backed pattern the
rolling summary + orphan deliveries already use).
- user_state: UserStateRegistry.get() hydrates skill_slug from disk on
first creation of a UserState.
- app: POST /api/skills + POST /api/admin/skills persist after mutation.
- tests: new conftest.py points SESSION_STORE_DIR at a tmpdir before app
is imported (session_store captures the path at module-load) and resets
_REGISTRY between tests so hydration is deterministic. Two new endpoint
tests pin the restart-survives-selection contract (53 passing, was 51).
- docs: guides/users.md + reference/environment-variables.md + STATUS.md
updated per greenfield-docs rule.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
WalkthroughThe changes implement file-backed persistence for a user's active skill slug. New functions in Changes
Sequence DiagramsequenceDiagram
participant User
participant App as App Endpoint
participant Memory as UserState (Memory)
participant Disk as Filesystem
User->>App: POST /api/skills (skill_slug)
App->>Memory: Update user_state.skill_slug
App->>Disk: save_skill_slug(user_id, skill_slug)
Disk-->>App: Written to skill.txt
App-->>User: 200 OK
Note over User,Disk: Process Restart
User->>App: GET /api/users/{user_id}/state
App->>Memory: UserStateRegistry.get(user_id)
Memory->>Disk: load_skill_slug(user_id)
Disk-->>Memory: Read from skill.txt
Memory->>Memory: Initialize skill_slug from disk
Memory-->>App: UserState with persisted skill_slug
App-->>User: Returns previously saved skill_slug
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@agent/session_store.py`:
- Around line 185-196: save_skill_slug currently swallows write errors (logs and
returns) which hides persistence failures; change its contract so callers can
detect failure by either making save_skill_slug raise the exception on write
errors or return an explicit boolean status (e.g., True on success, False on
failure). Locate save_skill_slug and its use of _skill_slug_path, update the
function signature to propagate failures (raise or return bool), update the
except block to either re-raise the caught exception or return False, and update
all callers to handle the new return/exception so they can surface errors and
avoid reporting success when persistence failed.
In `@tests/conftest.py`:
- Around line 21-23: The temp dir created by tempfile.mkdtemp() and stored in
_TEST_SESSION_DIR is never removed; add a teardown to delete that directory
(shutil.rmtree) and unset os.environ["SESSION_STORE_DIR"] after tests finish.
Implement this by registering a finalizer (e.g., in the module-level pytest
fixture or implement pytest_sessionfinish) that references _TEST_SESSION_DIR and
calls shutil.rmtree(_TEST_SESSION_DIR, ignore_errors=True) and del
os.environ["SESSION_STORE_DIR"] (guarding presence) to avoid temp-dir leaks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7e616c4e-767a-48c5-bacf-b11550171176
⛔ Files ignored due to path filters (1)
STATUS.mdis excluded by!*.md
📒 Files selected for processing (7)
agent/session_store.pyagent/user_state.pyapp.pydocs/guides/users.mddocs/reference/environment-variables.mdtests/conftest.pytests/test_endpoints.py
Comment on lines
+185
to
+196
| def save_skill_slug(user_id: str, slug: str) -> None: | ||
| if not slug or not slug.strip(): | ||
| return | ||
| p = _skill_slug_path(user_id) | ||
| try: | ||
| p.parent.mkdir(parents=True, exist_ok=True) | ||
| p.write_text(slug.strip(), encoding="utf-8") | ||
| logger.info( | ||
| f"[session_store] saved skill slug for {user_id!r}: {slug!r}" | ||
| ) | ||
| except Exception as e: | ||
| logger.warning(f"[session_store] failed to write {p}: {e}") |
There was a problem hiding this comment.
Do not silently swallow skill-persistence write failures.
save_skill_slug logs and returns on write errors, so callers can still return success while persistence failed. That breaks the restart-survival guarantee.
Suggested contract change (fail/return status so caller can surface error)
-def save_skill_slug(user_id: str, slug: str) -> None:
+def save_skill_slug(user_id: str, slug: str) -> bool:
if not slug or not slug.strip():
- return
+ return False
p = _skill_slug_path(user_id)
try:
p.parent.mkdir(parents=True, exist_ok=True)
- p.write_text(slug.strip(), encoding="utf-8")
+ tmp = p.with_suffix(".skill.tmp")
+ tmp.write_text(slug.strip(), encoding="utf-8")
+ tmp.replace(p)
logger.info(
f"[session_store] saved skill slug for {user_id!r}: {slug!r}"
)
+ return True
except Exception as e:
logger.warning(f"[session_store] failed to write {p}: {e}")
+ return False🧰 Tools
🪛 Ruff (0.15.11)
[warning] 193-193: Logging statement uses f-string
(G004)
[warning] 195-195: Do not catch blind exception: Exception
(BLE001)
[warning] 196-196: Logging statement uses f-string
(G004)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@agent/session_store.py` around lines 185 - 196, save_skill_slug currently
swallows write errors (logs and returns) which hides persistence failures;
change its contract so callers can detect failure by either making
save_skill_slug raise the exception on write errors or return an explicit
boolean status (e.g., True on success, False on failure). Locate save_skill_slug
and its use of _skill_slug_path, update the function signature to propagate
failures (raise or return bool), update the except block to either re-raise the
caught exception or return False, and update all callers to handle the new
return/exception so they can surface errors and avoid reporting success when
persistence failed.
Comment on lines
+21
to
+23
| _TEST_SESSION_DIR = tempfile.mkdtemp(prefix="protovoice_tests_") | ||
| os.environ["SESSION_STORE_DIR"] = _TEST_SESSION_DIR | ||
|
|
There was a problem hiding this comment.
Add teardown for _TEST_SESSION_DIR to avoid temp-dir leaks.
tempfile.mkdtemp() is never removed; repeated local runs can accumulate stale directories.
Suggested cleanup
import os
+import shutil
import tempfile
+import atexit
@@
_TEST_SESSION_DIR = tempfile.mkdtemp(prefix="protovoice_tests_")
os.environ["SESSION_STORE_DIR"] = _TEST_SESSION_DIR
+atexit.register(lambda: shutil.rmtree(_TEST_SESSION_DIR, ignore_errors=True))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| _TEST_SESSION_DIR = tempfile.mkdtemp(prefix="protovoice_tests_") | |
| os.environ["SESSION_STORE_DIR"] = _TEST_SESSION_DIR | |
| import os | |
| import shutil | |
| import tempfile | |
| import atexit | |
| _TEST_SESSION_DIR = tempfile.mkdtemp(prefix="protovoice_tests_") | |
| os.environ["SESSION_STORE_DIR"] = _TEST_SESSION_DIR | |
| atexit.register(lambda: shutil.rmtree(_TEST_SESSION_DIR, ignore_errors=True)) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/conftest.py` around lines 21 - 23, The temp dir created by
tempfile.mkdtemp() and stored in _TEST_SESSION_DIR is never removed; add a
teardown to delete that directory (shutil.rmtree) and unset
os.environ["SESSION_STORE_DIR"] after tests finish. Implement this by
registering a finalizer (e.g., in the module-level pytest fixture or implement
pytest_sessionfinish) that references _TEST_SESSION_DIR and calls
shutil.rmtree(_TEST_SESSION_DIR, ignore_errors=True) and del
os.environ["SESSION_STORE_DIR"] (guarding presence) to avoid temp-dir leaks.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
UserStateRegistryis a bare in-memory dict —POST /api/skillsmutated it but nothing wrote to disk, so every process restart snapped users back toDEFAULT_SOUL_SLUG(and whatever voice that skill carries).{SESSION_STORE_DIR}/{user_id}/skill.txt, matching the existing pattern for rolling summaries + orphan deliveries.UserStateRegistry.get()hydrates on first access; both/api/skillsand/api/admin/skillspersist after mutation.What changed
agent/session_store.pyload_skill_slug/save_skill_slughelpers (file-backed, same layout as summaries).agent/user_state.pyUserStateRegistry.get()reads persisted slug when it first creates aUserState.app.pyset_skill+admin_set_skillcallsave_skill_slugafter assigning.tests/conftest.py(new)SESSION_STORE_DIRto a tmpdir beforeappis imported (module captures the path at load time). Resets_REGISTRYbetween tests so hydration is deterministic.tests/test_endpoints.py/api/skillsand/api/admin/skills. 53 passing total.docs/guides/users.md,docs/reference/environment-variables.md,STATUS.mdContext
This turned up while debugging a report that "voices don't stick across restart." The root cause of that specific report was actually a Fish sidecar issue (references on disk in the wrong path — the docker mount pointed at an empty dir), not this persistence gap. But the persistence gap is a real bug in its own right: once you fix the Fish side, any restart of the protoVoice process still resets every user's selection. This PR closes that window.
Test plan
.venv/bin/python -m pytest— 53 passing, ~3sGET /api/skillsreturns the picked slug asactive, and the session uses itsvoice:on next connect.POST /api/admin/skillsfor another user → restart → that user's slug is still set on first access.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation