feat(openclaw): Re-implement auto-migration via OpenClaw native secrets system - #51
Open
KHAEntertainment wants to merge 6 commits into
Open
feat(openclaw): Re-implement auto-migration via OpenClaw native secrets system#51KHAEntertainment wants to merge 6 commits into
KHAEntertainment wants to merge 6 commits into
Conversation
…ts system
Implements the exec-provider based migration approach for OpenClaw secrets,
replacing the deprecated ${ENV_VAR} placeholder method.
## Changes
### Phase 1: Plan Types and Generation (src/openclaw/plan.ts)
- New SecretsApplyPlan types compatible with `openclaw secrets apply`
- buildExecProviderId() - creates exec provider IDs like `providers/openai/key`
- createSecretsApplyPlan() - generates complete plan JSON
- Validates exec provider IDs match OpenClaw's pattern
### Phase 2: Plan Generation in Migrate (src/openclaw/migrate.ts)
- Added analyzeAuthStoreForPlan() - analyzes single auth store
- Added generateSecretsApplyPlan() - orchestrates across all auth stores
- OAuth credentials correctly skipped with `oauth_not_supported` reason
- Only api_key and token types are migratable via exec provider
### Phase 3: CLI Updates
- openclaw-migrate.ts: Added --plan option generating clawvault-migration-plan.json
- openclaw-cleanup.ts (new): Detects redundant auth configs across agents
- openclaw.ts: Registered cleanup command
### Phase 4: Documentation (docs/MIGRATION.md)
- Updated with new recommended --plan workflow
- Tables showing migratable vs non-migratable credentials
- OAuth handling via `openclaw models auth login --sync-siblings`
## Migration Path
```bash
# Generate plan
clawvault openclaw migrate --plan --verbose
# Apply via OpenClaw
openclaw secrets apply --from ./clawvault-migration-plan.json
# For OAuth (not migratable via plan)
openclaw models auth login --provider google --sync-siblings
```
Closes #50
Contributor
WalkthroughAdds an exec-provider–based SecretsApplyPlan migration flow: plan types/utilities, analysis and plan-generation functions, CLI Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI as openclaw CLI
participant PlanGen as generateSecretsApplyPlan
participant Discover as discoverAuthStorePaths
participant Analyze as analyzeAuthStoreForPlan
participant Creator as createSecretsApplyPlan
participant FS as File System
User->>CLI: openclaw migrate --plan --provider-name clawvault
CLI->>PlanGen: handlePlanGeneration(options)
PlanGen->>Discover: discover auth-store paths
Discover-->>PlanGen: list of {agentId, path}
loop per agent
PlanGen->>Analyze: analyzeAuthStoreForPlan(agentId, path)
Analyze->>FS: read auth-profiles.json
Analyze-->>PlanGen: migratable + nonMigratable secrets
end
PlanGen->>Creator: createSecretsApplyPlan(analysis, providerName)
Creator-->>Creator: build targets & providerUpserts
Creator-->>PlanGen: SecretsApplyPlan JSON
PlanGen->>FS: write clawvault-migration-plan.json
PlanGen-->>User: print summary and next steps
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
src/cli/commands/openclaw-cleanup.ts (2)
84-87:readJsonFileis duplicated frommigrate.ts.This exact function exists in
migrate.ts(lines 144-147). Consider extracting it to a shared utility module to avoid duplication. Not critical, but helps keep things DRY:♻️ Extract to shared utility
You could create a
src/openclaw/utils.tsor similar:// src/openclaw/utils.ts import { promises as fs } from 'fs' export async function readJsonFile(path: string): Promise<unknown> { const data = await fs.readFile(path, 'utf-8') return JSON.parse(data) }Then import from both
migrate.tsandopenclaw-cleanup.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/commands/openclaw-cleanup.ts` around lines 84 - 87, The function readJsonFile in openclaw-cleanup.ts duplicates the same helper in migrate.ts; extract it into a shared utility (e.g., src/openclaw/utils.ts), export readJsonFile there (using promises fs.readFile + JSON.parse), then replace the local readJsonFile implementations in both openclaw-cleanup.ts and migrate.ts with an import of the shared readJsonFile to remove duplication and keep behavior identical.
114-116: Silent error handling may hide issues.Catching and ignoring all errors during auth store parsing means users won't know if a file is corrupt or inaccessible. Consider at least logging a warning in verbose mode:
♻️ Optional: Log warnings in verbose mode
This would require passing
verboseintoanalyzeAuthStoreRedundanciesor logging at the call site:} catch { - // ignore errors + // Silently skip files that can't be parsed + // Consider adding verbose logging here }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/commands/openclaw-cleanup.ts` around lines 114 - 116, The empty catch in analyzeAuthStoreRedundancies silently hides failures; update the error handling to at minimum log a warning (including the caught error) instead of ignoring it. Modify analyzeAuthStoreRedundancies (and its caller in openclaw-cleanup.ts if needed) to accept a verbose flag or a logger and inside the catch call logger.warn or console.warn with a message like "failed to parse auth store" plus the error details; if you choose the verbose route, thread the verbose parameter from the command handler into analyzeAuthStoreRedundancies and only emit the warning when verbose is true.src/cli/commands/openclaw-migrate.ts (2)
42-54: Output path is hardcoded to current working directory.The plan is always written to
./clawvault-migration-plan.jsonin the current working directory. This works for most use cases, but users running from different directories might be surprised by where the file lands. Consider adding an--outputoption for flexibility:♻️ Optional: Add configurable output path
interface OpenClawMigrateOptions { // ... existing options plan?: boolean providerName?: string + output?: string } // In option definitions: + .option('--output <path>', 'Output path for migration plan', 'clawvault-migration-plan.json') // In handlePlanGeneration: - const outputPath = 'clawvault-migration-plan.json' + const outputPath = options.output ?? 'clawvault-migration-plan.json'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/commands/openclaw-migrate.ts` around lines 42 - 54, The handlePlanGeneration function currently writes the migration plan to a hardcoded outputPath ('clawvault-migration-plan.json'); add an --output option to OpenClawMigrateOptions and the CLI so callers can specify a path, update handlePlanGeneration to read options.output (falling back to 'clawvault-migration-plan.json' when absent), and use that variable in the writeFile call; ensure any help text/defaults for the CLI flag are updated and that the option is passed through to generateSecretsApplyPlan/createSecretsApplyPlan callers if required.
43-47:providerNamepassed togenerateSecretsApplyPlanis not used there.The
generateSecretsApplyPlanfunction signature acceptsproviderNameandclawvaultPathin its options, but looking at the implementation inmigrate.ts(lines 574-579), it only usesopenclawDirandagentId. TheproviderNameis correctly passed tocreateSecretsApplyPlanon line 50, but passing it togenerateSecretsApplyPlanhas no effect. This isn't a bug (unused options are harmless), but the signature mismatch might confuse future maintainers.♻️ Cleaner: Only pass providerName where it's used
async function handlePlanGeneration(options: OpenClawMigrateOptions): Promise<void> { const analysis = await generateSecretsApplyPlan({ openclawDir: options.openclawDir, agentId: options.agentId, - providerName: options.providerName, }) const plan = createSecretsApplyPlan(analysis, { providerName: options.providerName, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/commands/openclaw-migrate.ts` around lines 43 - 47, The call to generateSecretsApplyPlan is passing providerName even though generateSecretsApplyPlan (implementation in migrate.ts) doesn't use it; remove providerName from the options object when calling generateSecretsApplyPlan in openclaw-migrate.ts and only pass providerName to createSecretsApplyPlan where it is actually consumed (or alternatively, if providerName should be used by generateSecretsApplyPlan, add it to the function implementation/signature and use it consistently). Ensure references to generateSecretsApplyPlan and createSecretsApplyPlan are updated accordingly so the passed options match the functions that consume them.src/openclaw/plan.ts (1)
148-158: Type mapping assumes only two credential types.The ternary
secret.field === 'key' ? 'auth-profiles.api_key.key' : 'auth-profiles.token.token'works becauseMIGRATABLE_CREDENTIAL_TYPEScurrently only includesapi_keyandtoken. If a third migratable type is added later, this logic would silently fall through totoken.token. Consider making this explicit:♻️ Optional: More explicit type mapping
const targets: SecretsApplyTarget[] = analysis.migratable.map(secret => ({ - type: secret.field === 'key' ? 'auth-profiles.api_key.key' : 'auth-profiles.token.token', + type: secret.field === 'key' + ? 'auth-profiles.api_key.key' + : secret.field === 'token' + ? 'auth-profiles.token.token' + : (() => { throw new Error(`Unexpected migratable field: ${secret.field}`) })(), path: buildAuthProfilePath(secret.profileId, secret.field),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openclaw/plan.ts` around lines 148 - 158, The current ternary in the targets construction (within the const targets: SecretsApplyTarget[] = analysis.migratable.map(...)) assumes only 'key' vs other and will silently map unknown secret.field values to the token type; change this to an explicit mapping (e.g., a switch or a lookup map keyed by secret.field) that returns 'auth-profiles.api_key.key' for the 'key' case, 'auth-profiles.token.token' for the token case, and throws or logs/returns an error for any unexpected secret.field so new credential types won't be silently misclassified; update the mapping used for the type property in the map callback and ensure any callers expecting SecretsApplyTarget still work.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/MIGRATION.md`:
- Around line 231-235: The fenced code block containing the ID mapping lines
(e.g., profileId: "openai:default", field: "key" → Exec ID:
"providers/openai/key" and profileId: "anthropic:default", field: "key" → Exec
ID: "providers/anthropic/key") is missing a language specifier; update the
opening fence to include a language (for example "text" or "yaml") so the block
becomes ```text (or ```yaml) followed by the two mapping lines and a closing
fence, ensuring consistent rendering and syntax highlighting.
- Around line 64-82: The fenced code block under "Example output:" is missing a
language specifier; update the triple-backtick fence that contains the sample
CLI output (starting with "SecretsApplyPlan generated:
clawvault-migration-plan.json") to include a language such as text or console
(e.g., ```text) so the static analysis warning is resolved and the output is
rendered correctly.
In `@src/cli/commands/openclaw-cleanup.ts`:
- Around line 145-159: The loop over agentsWithProfiles is incorrectly using
Object.entries([...agentsWithProfiles]) which yields [index, value] pairs so the
destructured [agentId] gets the numeric index; change the loop to iterate
directly over the Set (for (const agentId of agentsWithProfiles)) so agentId is
the actual agent ID string, keeping the rest of the logic that uses agentId with
allProfiles, fingerprintGroups, and agentsWithSharedOnly unchanged.
- Line 201: The CLI defines the '--apply' option but the command action handler
(the callback registered with .action for the openclaw-cleanup command) contains
no logic for it, so either remove the option or wire it into the existing flow:
update the action handler to detect opts.apply (in combination with
opts.consolidate if required) and perform the consolidation apply step (or call
the function that performs apply, e.g., applyConsolidationPlan) and emit
appropriate success/error logs; alternatively, remove the .option('--apply',
...) declaration or make the action handler explicitly log a clear "not
implemented" warning when opts.apply is true; also update the help/usage text
and tests to reflect the chosen behavior.
In `@src/openclaw/plan.ts`:
- Around line 106-110: The buildExecProviderId function can produce empty
segments (e.g., providers//) when provider or field sanitize to empty strings;
update buildExecProviderId to defensively handle empty sanitizedProvider or
sanitizedField by replacing them with a stable fallback (e.g., "unknown" or
"unnamed") or by throwing a clear error, and ensure the returned string never
contains empty segments or duplicate slashes; reference buildExecProviderId (and
optionally isValidExecProviderId) when making the change so validation stays
consistent.
---
Nitpick comments:
In `@src/cli/commands/openclaw-cleanup.ts`:
- Around line 84-87: The function readJsonFile in openclaw-cleanup.ts duplicates
the same helper in migrate.ts; extract it into a shared utility (e.g.,
src/openclaw/utils.ts), export readJsonFile there (using promises fs.readFile +
JSON.parse), then replace the local readJsonFile implementations in both
openclaw-cleanup.ts and migrate.ts with an import of the shared readJsonFile to
remove duplication and keep behavior identical.
- Around line 114-116: The empty catch in analyzeAuthStoreRedundancies silently
hides failures; update the error handling to at minimum log a warning (including
the caught error) instead of ignoring it. Modify analyzeAuthStoreRedundancies
(and its caller in openclaw-cleanup.ts if needed) to accept a verbose flag or a
logger and inside the catch call logger.warn or console.warn with a message like
"failed to parse auth store" plus the error details; if you choose the verbose
route, thread the verbose parameter from the command handler into
analyzeAuthStoreRedundancies and only emit the warning when verbose is true.
In `@src/cli/commands/openclaw-migrate.ts`:
- Around line 42-54: The handlePlanGeneration function currently writes the
migration plan to a hardcoded outputPath ('clawvault-migration-plan.json'); add
an --output option to OpenClawMigrateOptions and the CLI so callers can specify
a path, update handlePlanGeneration to read options.output (falling back to
'clawvault-migration-plan.json' when absent), and use that variable in the
writeFile call; ensure any help text/defaults for the CLI flag are updated and
that the option is passed through to
generateSecretsApplyPlan/createSecretsApplyPlan callers if required.
- Around line 43-47: The call to generateSecretsApplyPlan is passing
providerName even though generateSecretsApplyPlan (implementation in migrate.ts)
doesn't use it; remove providerName from the options object when calling
generateSecretsApplyPlan in openclaw-migrate.ts and only pass providerName to
createSecretsApplyPlan where it is actually consumed (or alternatively, if
providerName should be used by generateSecretsApplyPlan, add it to the function
implementation/signature and use it consistently). Ensure references to
generateSecretsApplyPlan and createSecretsApplyPlan are updated accordingly so
the passed options match the functions that consume them.
In `@src/openclaw/plan.ts`:
- Around line 148-158: The current ternary in the targets construction (within
the const targets: SecretsApplyTarget[] = analysis.migratable.map(...)) assumes
only 'key' vs other and will silently map unknown secret.field values to the
token type; change this to an explicit mapping (e.g., a switch or a lookup map
keyed by secret.field) that returns 'auth-profiles.api_key.key' for the 'key'
case, 'auth-profiles.token.token' for the token case, and throws or logs/returns
an error for any unexpected secret.field so new credential types won't be
silently misclassified; update the mapping used for the type property in the map
callback and ensure any callers expecting SecretsApplyTarget still work.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 809c6db0-10ae-43d9-80da-9764b9f5dc0c
📒 Files selected for processing (7)
docs/MIGRATION.mdsrc/cli/commands/openclaw-cleanup.tssrc/cli/commands/openclaw-migrate.tssrc/cli/commands/openclaw.tssrc/openclaw/index.tssrc/openclaw/migrate.tssrc/openclaw/plan.ts
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 4 file(s) based on 3 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
5 tasks
Fixed 4 file(s) based on 3 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/web/index.ts (1)
120-127:⚠️ Potential issue | 🟠 MajorSilent suppression of config validation errors hides user misconfigurations.
The try/catch here swallows all exceptions, but
loadConfig()already handles the "file missing" case internally by returning defaults. When it throws, it's signaling a real problem—like a malformed config file—that the user should know about. Think of it like your car's check-engine light: you want it to flash when something's actually wrong, not just quietly ignore the issue.Consider at minimum logging a warning so users can debug why their config isn't being applied:
🛠️ Proposed fix to surface config errors
// Load config for manage dashboard (secret metadata lookup). - // Falls back to empty config if the config file is missing or invalid. + // Falls back to empty config if the config file is missing; logs warning on validation errors. let manageConfig: ConfigSchema = { version: 1, secrets: {}, gateway: { restartOnUpdate: false, services: [] } } try { manageConfig = await loadConfig() - } catch { - // If config loading fails, fall back to empty config so the server still starts. + } catch (err) { + // loadConfig handles ENOENT internally, so if we're here it's a real error + if (err instanceof Error) { + console.warn(`[clawvault] Config load failed, using defaults: ${err.message}`) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/web/index.ts` around lines 120 - 127, The code silently swallows errors from loadConfig(), hiding real validation problems; update the try/catch around loadConfig() (where manageConfig: ConfigSchema is set) to catch the error into a variable (e.g. catch (err)) and log a warning that includes the error message/stack before falling back to the default manageConfig; use the existing logger if available or console.warn to emit "Failed to load manage config:" plus err to surface malformed-config issues while preserving the fallback behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/commands/openclaw-cleanup.ts`:
- Around line 128-137: The code currently treats multiple profile entries from
the same agent as multiple agents; change the logic in the sharedProfiles
assembly (loop over fingerprintGroups) to compute a Set of unique agentIds
(e.g., const agentIds = new Set(profiles.map(p => p.agentId))) and use
agentIds.size > 1 as the condition, set count to agentIds.size, and set agents
to Array.from(agentIds); apply the same change where agentsWithNoUniqueProfiles
is built (replace checks using profiles.length and profiles.map(...) with the
unique agentId Set) so both places count unique agents rather than raw profile
entries.
- Around line 161-186: The code currently groups profiles only by
profile.provider and flags a provider as global if the provider name appears in
every agent; instead require a shared secret identity (fingerprint) across
agents. Change the grouping logic (where providerAgents and providerProfileIds
are built and where globalProviderCandidates is computed) to key by a composite
of provider + profile.fingerprint (or the actual field that represents the
secret/key fingerprint in Profile) so you only mark providers that use the same
key across all agents; build a map like providerFingerprintAgents keyed by
`${provider}|${fingerprint}`, collect agent IDs and profileIds per key, then
when computing globalProviderCandidates require agents.size ===
allAgentIds.length (and allAgentIds.length > 1) for a single
provider+fingerprint entry and include the fingerprint in the candidate object.
Apply the same change to the other similar blocks referenced (the logic at the
other occurrences around 244-253 and 268-275).
In `@src/openclaw/plan.ts`:
- Around line 148-151: The current code builds pathSegments by splitting the
string returned from buildAuthProfilePath, which breaks when profile IDs contain
dots; instead construct pathSegments directly from raw segments. In the
SecretsApplyTarget creation (the targets array where you map
analysis.migratable), keep using buildAuthProfilePath(secret.profileId,
secret.field) for path but replace pathSegments:
buildAuthProfilePath(...).split('.') with an explicit array like ['profiles',
secret.profileId, secret.field] (or the correct top-level segment used by
buildAuthProfilePath if not "profiles") so profile IDs with dots remain a single
segment; update the mapping in the same block that defines
Targets/SecretsApplyTarget.
- Around line 106-109: buildExecProviderId currently collapses distinct valid
IDs because it replaces all non-alphanumeric characters with '-' even though
isValidExecProviderId allows '.', '_', ':' and '-'—so values like "foo.bar" and
"foo_bar" become identical. Update the sanitization in buildExecProviderId
(variables sanitizedProvider and sanitizedField) to preserve characters allowed
by isValidExecProviderId by only replacing characters not in the allowed set
(letters, digits, '.', '_', ':' and '-') and trim any disallowed
leading/trailing characters; keep the lowercase conversion and existing
fallbacks ('unknown'/'unnamed') intact so valid distinct IDs remain distinct.
---
Outside diff comments:
In `@src/web/index.ts`:
- Around line 120-127: The code silently swallows errors from loadConfig(),
hiding real validation problems; update the try/catch around loadConfig() (where
manageConfig: ConfigSchema is set) to catch the error into a variable (e.g.
catch (err)) and log a warning that includes the error message/stack before
falling back to the default manageConfig; use the existing logger if available
or console.warn to emit "Failed to load manage config:" plus err to surface
malformed-config issues while preserving the fallback behavior.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2bddf3d4-925e-476b-9021-1aa7d26f94a7
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
src/cli/commands/openclaw-cleanup.tssrc/openclaw/plan.tssrc/web/index.ts
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 3 file(s) based on 4 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 3 file(s) based on 4 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/openclaw/plan.ts (2)
61-69: Consider narrowingfieldtype for stronger guarantees.The
fieldproperty is typed asstring, but based on howmigrate.tspopulates migratable secrets, it's always either'key'or'token'. Narrowing the type would catch mismatches at compile time rather than letting unexpected values slip through to the runtime discriminant check increateSecretsApplyPlan.♻️ Proposed type narrowing
export interface MigratableSecret { agentId: string authStorePath: string profileId: string provider: string - field: string + field: 'key' | 'token' secretId: string // The exec provider ID, e.g., "providers/openai/key" length: number }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openclaw/plan.ts` around lines 61 - 69, The MigratableSecret interface's field is currently typed as string which allows invalid values to reach runtime; update the field property to a narrowed union type 'key' | 'token' in the MigratableSecret interface so TypeScript enforces the allowed values, then adjust any references in migrate.ts (where migratable secrets are populated) and createSecretsApplyPlan (where the discriminant is checked) to satisfy the new type; ensure compile errors are fixed by updating any constructions or conditionals to use the narrowed values.
106-110: Length constraint not enforced.The validation pattern
^[A-Za-z0-9][A-Za-z0-9._:/-]{0,255}$limits IDs to 256 characters total, butbuildExecProviderIddoesn't enforce this. If someone passes extremely long provider/field strings, the generated ID could exceed the limit and fail validation downstream.In practice, provider names are short (like "openai", "anthropic"), so this is unlikely to bite anyone - but if you want belt-and-suspenders safety, you could truncate or throw.
🛡️ Optional defensive truncation
export function buildExecProviderId(provider: string, field: string): string { const sanitizedProvider = provider.toLowerCase().replace(/[^a-z0-9._:-]+/g, '-').replace(/^[^a-z0-9]+|[^a-z0-9]+$/g, '') || 'unknown' const sanitizedField = field.toLowerCase().replace(/[^a-z0-9._:-]+/g, '-').replace(/^[^a-z0-9]+|[^a-z0-9]+$/g, '') || 'unnamed' - return `providers/${sanitizedProvider}/${sanitizedField}` + const id = `providers/${sanitizedProvider}/${sanitizedField}` + if (id.length > 256) { + throw new Error(`Exec provider ID exceeds 256 character limit: ${id.slice(0, 50)}...`) + } + return id }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openclaw/plan.ts` around lines 106 - 110, buildExecProviderId can produce IDs longer than the allowed 256-char pattern; modify buildExecProviderId to enforce the validation length by truncating sanitizedProvider and sanitizedField so the final string (including the "providers/" prefix and separating "/") is at most 256 characters, while preserving non-empty defaults ('unknown'/'unnamed') and the existing sanitization; calculate available length = 256 - "providers/".length - 1 (separator) and split that between provider and field (e.g., reserve at least 1 char for each, prefer giving provider more or split proportionally), then truncate sanitizedProvider and sanitizedField to their allotted lengths before constructing and returning `providers/${sanitizedProvider}/${sanitizedField}` so the returned ID always meets the regex and length constraint.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/openclaw/plan.ts`:
- Around line 61-69: The MigratableSecret interface's field is currently typed
as string which allows invalid values to reach runtime; update the field
property to a narrowed union type 'key' | 'token' in the MigratableSecret
interface so TypeScript enforces the allowed values, then adjust any references
in migrate.ts (where migratable secrets are populated) and
createSecretsApplyPlan (where the discriminant is checked) to satisfy the new
type; ensure compile errors are fixed by updating any constructions or
conditionals to use the narrowed values.
- Around line 106-110: buildExecProviderId can produce IDs longer than the
allowed 256-char pattern; modify buildExecProviderId to enforce the validation
length by truncating sanitizedProvider and sanitizedField so the final string
(including the "providers/" prefix and separating "/") is at most 256
characters, while preserving non-empty defaults ('unknown'/'unnamed') and the
existing sanitization; calculate available length = 256 - "providers/".length -
1 (separator) and split that between provider and field (e.g., reserve at least
1 char for each, prefer giving provider more or split proportionally), then
truncate sanitizedProvider and sanitizedField to their allotted lengths before
constructing and returning `providers/${sanitizedProvider}/${sanitizedField}` so
the returned ID always meets the regex and length constraint.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7150a2f6-7047-4081-b418-a5a99f048855
📒 Files selected for processing (2)
src/cli/commands/openclaw-cleanup.tssrc/openclaw/plan.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/cli/commands/openclaw-cleanup.ts
Contributor
|
@KHAEntertainment I've opened a new pull request, #52, to work on those changes. Once the pull request is ready, I'll request review from you. |
…ype narrowing, ID length enforcement, doc code block language specifiers Co-authored-by: KHAEntertainment <43256680+KHAEntertainment@users.noreply.github.com> Agent-Logs-Url: https://github.com/KHAEntertainment/clawvault/sessions/f266caee-ba37-485d-a8a9-765045dabc0e
fix(openclaw): correct misleading consolidation guidance, narrow field type, enforce ID length limit
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/openclaw/plan.ts`:
- Around line 151-164: Validate providerName and secretId before constructing
the SecretsApplyTarget entries: ensure options.providerName (and resulting
providerName) is non-empty/non-whitespace (reject and throw an error from the
function if invalid) and validate secret.secretId against the expected format
(e.g., non-empty and matches the service's id pattern) before using it in the
ref object; perform these checks right before building targets (around the
providerName assignment and the mapping that builds targets using
buildAuthProfilePath and SecretsApplyTarget) so the function fails fast with a
clear error instead of producing an invalid plan for openclaw secrets apply.
🪄 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: CHILL
Plan: Pro
Run ID: 598fa740-59be-4e68-8834-fc358b5725d5
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
docs/MIGRATION.mdsrc/cli/commands/openclaw-cleanup.tssrc/openclaw/plan.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/MIGRATION.md
- src/cli/commands/openclaw-cleanup.ts
Comment on lines
+151
to
+164
| const providerName = options.providerName ?? 'clawvault' | ||
| const clawvaultPath = options.clawvaultPath ?? 'clawvault' | ||
|
|
||
| const targets: SecretsApplyTarget[] = analysis.migratable.map(secret => ({ | ||
| type: secret.field === 'key' ? 'auth-profiles.api_key.key' : 'auth-profiles.token.token', | ||
| path: buildAuthProfilePath(secret.profileId, secret.field), | ||
| pathSegments: ['profiles', secret.profileId, secret.field], | ||
| agentId: secret.agentId, | ||
| ref: { | ||
| source: 'exec', | ||
| provider: providerName, | ||
| id: secret.secretId, | ||
| }, | ||
| })) |
Contributor
There was a problem hiding this comment.
Validate providerName and secretId before writing the plan.
On Line 151 and Line 162, invalid inputs can still slip into the generated JSON (e.g., empty providerName or malformed secretId) and only fail later during openclaw secrets apply. Fail fast here to keep migration errors immediate and actionable—like rejecting a shipping label with a blank destination before it leaves the warehouse.
Suggested fail-fast guard
export function createSecretsApplyPlan(
analysis: PlanAnalysis,
options: {
providerName?: string
clawvaultPath?: string
} = {}
): SecretsApplyPlan {
- const providerName = options.providerName ?? 'clawvault'
+ const providerName = (options.providerName ?? 'clawvault').trim()
+ if (!providerName) {
+ throw new Error('providerName must be a non-empty string')
+ }
const clawvaultPath = options.clawvaultPath ?? 'clawvault'
- const targets: SecretsApplyTarget[] = analysis.migratable.map(secret => ({
- type: secret.field === 'key' ? 'auth-profiles.api_key.key' : 'auth-profiles.token.token',
- path: buildAuthProfilePath(secret.profileId, secret.field),
- pathSegments: ['profiles', secret.profileId, secret.field],
- agentId: secret.agentId,
- ref: {
- source: 'exec',
- provider: providerName,
- id: secret.secretId,
- },
- }))
+ const targets: SecretsApplyTarget[] = analysis.migratable.map(secret => {
+ if (!isValidExecProviderId(secret.secretId)) {
+ throw new Error(`Invalid exec provider id: ${secret.secretId}`)
+ }
+ return {
+ type: secret.field === 'key' ? 'auth-profiles.api_key.key' : 'auth-profiles.token.token',
+ path: buildAuthProfilePath(secret.profileId, secret.field),
+ pathSegments: ['profiles', secret.profileId, secret.field],
+ agentId: secret.agentId,
+ ref: {
+ source: 'exec',
+ provider: providerName,
+ id: secret.secretId,
+ },
+ }
+ })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/openclaw/plan.ts` around lines 151 - 164, Validate providerName and
secretId before constructing the SecretsApplyTarget entries: ensure
options.providerName (and resulting providerName) is non-empty/non-whitespace
(reject and throw an error from the function if invalid) and validate
secret.secretId against the expected format (e.g., non-empty and matches the
service's id pattern) before using it in the ref object; perform these checks
right before building targets (around the providerName assignment and the
mapping that builds targets using buildAuthProfilePath and SecretsApplyTarget)
so the function fails fast with a clear error instead of producing an invalid
plan for openclaw secrets apply.
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
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
Re-implements the deprecated
clawvault openclaw migrate --applycommand using OpenClaw's native exec-provider protocol instead of${ENV_VAR}placeholder substitution.The original approach failed because OpenClaw treats
${ENV_VAR}strings inauth-profiles.jsonas literal text. The new approach usesopenclaw secrets apply --from <plan.json>which configures OpenClaw to fetch secrets at runtime via ClawVault's existingresolvecommand.Key Changes
Phase 1: Plan Types (
src/openclaw/plan.ts)SecretsApplyPlantypes compatible withopenclaw secrets applybuildExecProviderId()creates exec provider IDs (e.g.,providers/openai/key)createSecretsApplyPlan()generates complete plan JSON withproviderUpsertsPhase 2: Plan Generation (
src/openclaw/migrate.ts)analyzeAuthStoreForPlan()- analyzes single auth store for migratable secretsgenerateSecretsApplyPlan()- orchestrates analysis across all auth storesoauth_not_supportedreasonPhase 3: CLI Updates
openclaw-migrate.ts: Added--planoption generatingclawvault-migration-plan.jsonopenclaw-cleanup.ts: New command to detect redundant auth configurationsopenclaw.ts: Registered cleanup commandPhase 4: Documentation (
docs/MIGRATION.md)--planmodeopenclaw models auth login --sync-siblingsWhat CAN and CANNOT Be Migrated
api_keywithkeytokenwithtokenoauthcredentialsMigration Workflow
Test Plan
clawvault openclaw migrate --plan --verboseand verify plan.json is validopenclaw secrets apply --from plan.json --dry-runauth-profiles.jsonentries use exec refsclawvault resolvecorrectly resolves new exec provider IDsopenclaw models statusRelated Issues
--applyapproachSummary by CodeRabbit
New Features
Documentation
Server