feat(appstash): add createConfigStore() for context and credential management#63
Conversation
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| } | ||
|
|
||
| function loadSettings(): GlobalSettings { | ||
| return readJson(settingsPath(), DEFAULT_SETTINGS); |
There was a problem hiding this comment.
🔴 Shared mutable DEFAULT_SETTINGS object is mutated via loadSettings(), polluting state across store instances
When the settings file doesn't exist, loadSettings() returns the module-level DEFAULT_SETTINGS object reference directly (via readJson at line 89). Callers like setCurrentContext() and deleteContext() then mutate this returned object (e.g., settings.currentContext = name at line 158), permanently polluting the shared default.
Root Cause and Impact
The readJson function at packages/appstash/src/config-store.ts:49-58 returns the fallback parameter by reference when the file doesn't exist. Since loadSettings() passes the module-level DEFAULT_SETTINGS constant (packages/appstash/src/config-store.ts:47), any mutation to the returned object mutates DEFAULT_SETTINGS itself.
The mutation happens in two places:
setCurrentContext()at line 158:settings.currentContext = name;deleteContext()at line 136:settings.currentContext = undefined;
Scenario: In a single process, if store A calls setCurrentContext('production') before its settings file exists, DEFAULT_SETTINGS becomes { currentContext: 'production' }. Later, if store B (for a different tool) calls loadSettings() before its own settings file exists, it receives { currentContext: 'production' } instead of {}, causing it to incorrectly think it has an active context.
While saveSettings does write to disk (so subsequent reads from the same store will read from the file), the in-memory DEFAULT_SETTINGS remains polluted for any other store instance or any call path where the file doesn't yet exist.
| return readJson(settingsPath(), DEFAULT_SETTINGS); | |
| return readJson(settingsPath(), { ...DEFAULT_SETTINGS }); |
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
feat(appstash): add createConfigStore() for context and credential management
Summary
Adds a higher-level
createConfigStore(toolName)API toappstashthat provides kubectl-style context and credential management. This is a direct extraction and parameterization of the existing config-manager from thecncCLI (packages/cli/src/config/config-manager.tsin the constructive repo), generalized so any tool can use it.New API:
createConfigStore(toolName, options?)→ConfigStorecreateContext,loadContext,listContexts,deleteContext,getCurrentContext,setCurrentContextsetCredentials,getCredentials,removeCredentials,hasValidCredentialsloadSettings,saveSettingsStorage layout uses the existing appstash directory structure:
~/.{toolName}/config/settings.json,~/.{toolName}/config/contexts/{name}.json,~/.{toolName}/config/credentials.json(mode0o600).Files changed:
packages/appstash/src/config-store.ts— new module (~220 lines)packages/appstash/src/index.ts— re-exportscreateConfigStore+ typespackages/appstash/__tests__/config-store.test.ts— 30 test cases covering settings, context CRUD, credentials, expiry validation, tool isolation, and a full end-to-end workflowReview & Testing Checklist for Human
contextPath()uses thenameparameter directly in${name}.jsonwith no sanitization. A name like../../foowould write outside thecontexts/directory. Consider whether input validation is needed here or if it's the caller's responsibility.DEFAULT_SETTINGSreference:loadSettings()returns theDEFAULT_SETTINGSobject directly when no file exists. If the caller mutates the returned object (e.g.,settings.currentContext = 'x'), it would mutate the module-level default. Verify this is acceptable or if a spread/clone is needed.writeJsonpassesmode: 0o600towriteFileSync, but verify that Node.js actually applies the mode when overwriting an existing file (vs. only on creation).contextPathside-effect:contextPath()creates thecontexts/directory on every call, including reads. This is harmless but worth being aware of.Recommended test plan: Run
pnpm testinpackages/appstash/— all 46 tests should pass (16 original + 30 new). Also runpnpm buildto verify the TypeScript compiles cleanly.Notes
eslint.config.jsfound) — not introduced by this PR.