fix: align dev-stack OH_PERSISTENCE_DIR and automation DB path with Docker#960
Open
tofarr wants to merge 4 commits into
Open
fix: align dev-stack OH_PERSISTENCE_DIR and automation DB path with Docker#960tofarr wants to merge 4 commits into
tofarr wants to merge 4 commits into
Conversation
…ocker Two path mismatches between `npm run dev` and Docker prevented config and automation data from being shared across the two modes: 1. **OH_PERSISTENCE_DIR wrong (supersedes PR #959)** Docker's entrypoint.sh sets OH_PERSISTENCE_DIR to $HOME/.openhands (OPENHANDS_DIR). PR #959 added the env var to buildAgentServerEnv() but used config.stateDir (~/.openhands/agent-canvas) — one level too deep. Settings and secrets written by Docker live at ~/.openhands/settings.toml etc; dev wrote to ~/.openhands/agent-canvas/settings.toml. Fix: use path.dirname(config.stateDir) = ~/.openhands, matching Docker. 2. **Automation DB in wrong directory** dev-with-automation.mjs put the SQLite DB at ~/.openhands/agent-canvas/automations.db. Docker puts it at ~/.openhands/automation/automations.db (from config/defaults.json paths.automationDb = "automation/automations.db" relative to OPENHANDS_DIR). Fix: use join(dirname(config.stateDir), SHARED_DEFAULTS.paths.automationDb) so both modes resolve to ~/.openhands/automation/automations.db. Also ensure the directory is created on startup (mirrors Docker's mkdir -p). 3. **Mock-LLM test cleanup** Update playwright.mock-llm.config.ts to clean both STATE_DIR and the new AUTOMATION_DB_DIR (.tmp/automation/) before each test run, since the DB now lives outside STATE_DIR. Co-authored-by: openhands <openhands@all-hands.dev>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…ations npm dev mode now writes conversations to ~/.openhands/agent-canvas/dev_conversations instead of conversations, keeping them separate from Docker's conversations dir. OH_CONVERSATIONS_PATH (set via buildAgentServerEnv) is the single control point; all mkdir and releaseStaleConversationLeases calls updated to match. Also condense verbose multi-line comments on OH_PERSISTENCE_DIR and AUTOMATION_DB_URL to single lines. Co-authored-by: openhands <openhands@all-hands.dev>
The outer config in dev-with-automation.mjs does not have conversationsPath (that is a SafeDevConfig property). Use the same join(stateDir, ...) pattern as the other dirs in the list. Also removes the debug console.log and restores dev-safe.mjs and dev-static.mjs to dev_conversations after the manual revert. Co-authored-by: openhands <openhands@all-hands.dev>
The implementation in buildConfigFromPorts was changed to use 'dev_conversations' as the subdirectory name, but the corresponding test assertion was not updated. Co-authored-by: openhands <openhands@all-hands.dev>
Contributor
📸 Snapshot Test Report✅ All snapshots match the main branch baselines.
✅ Unchanged snapshots (73)
Generated by the Snapshot Tests workflow. This comment was created by an AI agent (OpenHands) on behalf of the repo maintainers. |
Contributor
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
Contributor
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Overall Assessment
This is a clean, well-scoped fix to a genuine path-alignment problem. The root cause is clearly diagnosed, each change is minimal and purposeful, and the PR description gives an excellent explanation of the two independent bugs being addressed.
What works well:
- is now correctly set to (= ), matching Docker's — the previous attempt in #959 was one level too deep.
- is now derived via so the path is a single source of truth shared with Docker and .
- Segregating npm conversations under (via ) is the right call: the workaround for workspace-path mismatches is pragmatic and the trade-off is clearly documented in the PR description.
- Playwright now cleans both and the new — previously the automation DB would survive a wipe of and cause cross-run contamination.
- Test and updates are included and accurate.
A couple of minor nits inline; nothing blocking.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
| join(config.stateDir, "bash_events"), | ||
| join(config.stateDir, "storage"), | ||
| // Automation DB directory — matches docker/entrypoint.sh mkdir -p behaviour. | ||
| dirname(join(dirname(config.stateDir), SHARED_DEFAULTS.paths.automationDb)), |
Contributor
There was a problem hiding this comment.
Nit — nested is slightly hard to parse at a glance.
Since , the directory is always the fixed segment relative to the parent of . The double-dirname wrapping is correct, but this reads more clearly as:
or simply with a short comment. Non-blocking.
| process.exit(1); | ||
| } | ||
| const conversationsPath = join(config.stateDir, "conversations"); | ||
| const conversationsPath = join(config.stateDir, "dev_conversations"); |
Contributor
There was a problem hiding this comment.
Minor — conversations path is duplicated in instead of reading from .
(built in ) already holds . Both here and the identical expression at line 560 will drift silently if the subdirectory name ever changes in . Not a bug today, but worth keeping in mind for a follow-up if is ever refactored to expose a canonical .
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
3 participants
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.
Why
Two path mismatches between
npm run devand Docker caused configs and automations to be stored in different locations, so switching between the two modes produced a blank slate.Observed:
~/.openhands/agent-canvas/automations.db(created bynpm run dev)~/.openhands/automation/automations.db(created by Docker — the correct one)This PR supersedes #959 (which added
OH_PERSISTENCE_DIRwith the wrong value).Root Causes
1.
OH_PERSISTENCE_DIRset to wrong path (fixes #959's attempt)docker/entrypoint.shsets:PR #959 added
OH_PERSISTENCE_DIR: config.stateDirtobuildAgentServerEnv(PR #959 addedOH_PERSiris~/.openhands/agent-canvas— **one level too deep**. Settings/secrets written by Docker live at~PR #959 addedOH_PERSISTENCE_DIR: config.stateDirtobuildAgentServerEnvettPR #959 addedOH_PERSISTENCE_DIR: conronPRdirectorydev-with-automation.mjsusedjoin(config.stateDir, "automations.db")→~/.openhands/agent-canvas/automations.db.Docker uses
$OPENHANDS_DIR/$CONFIG_AUTOMATION_DBwhereCONFIG_AUTOMATION_DB = automation/automations.db(fromconfig/defaults.json) →~/.openhands/automation/automations.db.Summary
scripts/dev-safe.mjs: SetOH_PERSISTENCE_DIR: path.dirname(config.stateDir)(=~/.openhands), matching Docker; write npm conversations todev_conversations/instead ofconversations/scripts/dev-with-automation.mjs: Usejoin(dirname(config.stateDir), SHARED_DEFAULTS.paths.automationDb)forAUTOMATION_DB_URL(=~/.openhands/automation/automations.db); also create that directory on startupscripts/dev-static.mjs: Updated mkdir andreleaseStaleConversationLeasesto usedev_conversations/playwright.mock-llm.config.ts: Clean bothSTATE_DIRand the newAUTOMATION_DB_DIR(.tmp/automation/) before each test runAGENTS.md: Document the new automation DB locationKnown Limitations
Conversations are not shared between npm and Docker modes. Each conversation's
meta.jsoneConversations are not shared between npm and Docker modes. Each conversation'smeta.jsoneConversations are not shared between npm and Docker modes. Each conversation'smeta.rkspace/project/<id>) that do not exist on the host. Rather than try to share them, npm now uses a separatedev_conversations/directory (OH_CONVERSATIONS_PATH), so Docker'sconversations/dir is left untouched and vice versa.How to Test
npm run devand configure an LLM provider → confirm settings written to~/.openhands/(not~/.openhands/agent-canvas/)~/.openhands/agent-canvas/dev_conversations/, notconversations/Screenshots
My last conversation before switching npm to use
dev_conversations...And the first one afterwards...
And running in docker...
This PR was created by an AI agent (OpenHands) on behalf of the user.
🐳 Docker images for this PR
• GHCR package: https://github.com/OpenHands/agent-canvas/pkgs/container/agent-canvas
ghcr.io/openhands/agent-canvasghcr.io/openhands/agent-server:1.24.0-pythonopenhands-automation==1.0.0a5b8c93c3ed110076921e238cdaae84a8141e1b255Pull (multi-arch manifest)
# Multi-arch manifest — Docker automatically pulls the correct architecture docker pull ghcr.io/openhands/agent-canvas:sha-b8c93c3Run
All tags pushed for this build
About Multi-Architecture Support
sha-b8c93c3) is a multi-arch manifest supporting both amd64 and arm64sha-b8c93c3-amd64) are also available if needed