String.replace() corrupts secret values containing $ patterns

working.replace(full, sv) treats the second argument as a replacement string with special $-patterns. If the resolved secret value contains $$, $&, $`, $', or $n, JavaScript will interpret them as replacement patterns rather than literal characters — silently corrupting the injected secret.

For example, a database password pa$$w0rd would become pa$w0rd, and a value like $&extra would expand to the matched pattern text prepended to extra.

Use a replacer function to prevent this:

This is a comment left during a code review.
Path: src/config/secret-resolution.ts
Line: 424-426

Comment:
**`String.replace()` corrupts secret values containing `$` patterns**

`working.replace(full, sv)` treats the second argument as a replacement string with special `$`-patterns. If the resolved secret value contains `$$`, `$&`, `` $` ``, `$'`, or `$n`, JavaScript will interpret them as replacement patterns rather than literal characters — silently corrupting the injected secret.

For example, a database password `pa$$w0rd` would become `pa$w0rd`, and a value like `$&extra` would expand to the matched pattern text prepended to `extra`.

Use a replacer function to prevent this:

```suggestion
  for (const { full, value: sv } of resolved) {
    working = working.replace(full, () => sv);
  }
```

How can I resolve this? If you propose a fix, please make it concise.

Cache TTL only reads from GCP provider config

The cache TTL is always sourced exclusively from secretsConfig.providers.gcp.cacheTtlSeconds, even though cacheTtlSeconds is defined as a valid field on every provider in SecretsConfig. For the env provider, this means the 5-minute default TTL is always applied regardless of any per-provider cacheTtlSeconds configuration. While env var caching is generally harmless, this will silently ignore any cacheTtlSeconds setting on non-GCP providers and will become a footgun when future providers (AWS, Vault, etc.) are added.

Consider computing a per-resolution TTL from the set of referenced providers, or moving the TTL to a top-level secrets.cacheTtlSeconds field rather than nesting it inside a specific provider's config.

This is a comment left during a code review.
Path: src/config/secret-resolution.ts
Line: 476-480

Comment:
**Cache TTL only reads from GCP provider config**

The cache TTL is always sourced exclusively from `secretsConfig.providers.gcp.cacheTtlSeconds`, even though `cacheTtlSeconds` is defined as a valid field on every provider in `SecretsConfig`. For the `env` provider, this means the 5-minute default TTL is always applied regardless of any per-provider `cacheTtlSeconds` configuration. While env var caching is generally harmless, this will silently ignore any `cacheTtlSeconds` setting on non-GCP providers and will become a footgun when future providers (AWS, Vault, etc.) are added.

Consider computing a per-resolution TTL from the set of referenced providers, or moving the TTL to a top-level `secrets.cacheTtlSeconds` field rather than nesting it inside a specific provider's config.

How can I resolve this? If you propose a fix, please make it concise.

Module-level cache is written by both GcpSecretProvider and fetchWithCache

GcpSecretProvider.getSecret() writes to secretCache directly (line 201), and fetchWithCache also writes to secretCache after calling the provider (line 357). For GCP secrets, the cache key is identical in both places (gcp:${name}#${ver}), so the entry is written twice per miss — first inside the provider, then overwritten by fetchWithCache. The stale-while-revalidate fallback in fetchWithCache (lines 360–364) works correctly because the expired entry survives in the map until it is overwritten, but the double-write is architecturally confusing and makes the two caching paths hard to reason about.

Consider having GcpSecretProvider.getSecret() not write to secretCache directly and instead rely entirely on the fetchWithCache layer, or alternatively remove the fetchWithCache layer and push the stale-while-revalidate logic into each provider.

This is a comment left during a code review.
Path: src/config/secret-resolution.ts
Line: 83

Comment:
**Module-level cache is written by both `GcpSecretProvider` and `fetchWithCache`**

`GcpSecretProvider.getSecret()` writes to `secretCache` directly (line 201), and `fetchWithCache` also writes to `secretCache` after calling the provider (line 357). For GCP secrets, the cache key is identical in both places (`gcp:${name}#${ver}`), so the entry is written twice per miss — first inside the provider, then overwritten by `fetchWithCache`. The stale-while-revalidate fallback in `fetchWithCache` (lines 360–364) works correctly because the expired entry survives in the map until it is overwritten, but the double-write is architecturally confusing and makes the two caching paths hard to reason about.

Consider having `GcpSecretProvider.getSecret()` not write to `secretCache` directly and instead rely entirely on the `fetchWithCache` layer, or alternatively remove the `fetchWithCache` layer and push the stale-while-revalidate logic into each provider.

How can I resolve this? If you propose a fix, please make it concise.

_{Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!}

Scope secret cache keys to provider instance

fetchWithCache keys entries only by provider name, secret name, and version, so two separate resolutions that use the same provider name and secret key but different backing config (for example different GCP projects or different injected env maps) will reuse the first value and skip the real provider lookup. This can return the wrong secret across configs until TTL expiry, which is a data-isolation and correctness issue; include provider-specific identity in the cache key (or keep cache per provider instance).

Useful? React with 👍 / 👎.

Prevent optional grep lookups from aborting migration

This script enables set -euo pipefail, but these grep | cut command substitutions are used for optional key discovery; when a file exists but the key is missing, grep exits with status 1 and the whole migration exits immediately. In real environments with partial credential files, scanning stops early and no migration completes, so these lookups need a non-failing fallback (for example || true).

Useful? React with 👍 / 👎.

Include project identity in GCP cache key

This cache key is shared across all GcpSecretProvider instances but only includes secret name/version, so two providers pointed at different GCP projects (or credentials files) can collide and return each other’s cached value without contacting Secret Manager. In a multi-agent or multi-project process this can leak the wrong secret until TTL expiry; scope the key by provider instance identity (for example project + credentialsFile) or use per-instance cache storage.

Useful? React with 👍 / 👎.

Purge Alpaca file when either migrated key exists

The Alpaca plaintext purge runs only when openclaw-chai-alpaca-key-id was discovered, but migration stores openclaw-chai-alpaca-secret-key independently. If a host only has ALPACA_SECRET_KEY set, migration reports success yet ~/.config/alpaca/sandbox.env is never rewritten, leaving the plaintext secret on disk. The purge condition should cover either migrated Alpaca key (or redact whichever key was migrated).

Useful? React with 👍 / 👎.

Avoid purging unrelated entries from .git-credentials

This purge rewrites ~/.git-credentials as soon as one GitHub PAT is migrated, but the script only extracts a single GitHub token (head -1) and does not account for other credentials that may be stored in the same file. On hosts that keep multiple GitHub accounts or non-GitHub entries there, running migration will silently delete still-needed credentials and break git auth for those remotes.

Useful? React with 👍 / 👎.

Preserve inserted secret values during token replacement

Replacing refs by repeatedly calling working.replace(full, value) can mutate previously inserted secret data instead of the original placeholders. If one resolved secret contains text like another placeholder (for example secret A includes ${env:B} while the config also has ${env:B}), the later pass can replace inside A's injected value and leave the real B placeholder unresolved, producing incorrect config output.

Useful? React with 👍 / 👎.

Print runnable next-step script paths in bootstrap output

The bootstrap summary tells operators to run ./scripts/migrate-secrets.sh and ./scripts/set-iam-bindings.sh, but this commit places both files under scripts/secrets/. Following the printed instructions results in command-not-found errors, which breaks the guided setup flow immediately after bootstrap completes.

Useful? React with 👍 / 👎.

Treat REST API enable failures as errors

The fallback branch unconditionally prints success after curl -s -X POST ... > /dev/null, but curl exits 0 even on HTTP 4xx/5xx unless --fail is used. In environments where gcloud services enable fails and the VM service account lacks Service Usage permissions, this path reports ✅ (via REST API) even though the API was not enabled, so later bootstrap steps fail with confusing errors.

Useful? React with 👍 / 👎.

Capture gho_ tokens when migrating GitHub credentials

The scan logic says it handles GitHub tokens with ghp_ or gho_ prefixes, but the extraction regex only matches ghp_. If .git-credentials contains a gho_ token (a common format from gh auth login), the script silently misses it, leaving that plaintext credential unmigrated.

Useful? React with 👍 / 👎.

Avoid serving stale secrets after access is revoked

fetchWithCache falls back to an expired cached value on any provider error, so when a secret is deleted or IAM access is revoked (for example GCP NOT_FOUND/PERMISSION_DENIED), resolution still returns the old secret instead of failing. In long-lived processes this can keep revoked credentials usable indefinitely and defeats emergency rotation/revocation until restart or manual cache clear.

Useful? React with 👍 / 👎.

Deduplicate concurrent secret fetches in one resolution pass

resolveString resolves all references in parallel, but fetchWithCache only writes to cache after awaiting provider.getSecret, so duplicate references encountered in the same pass can trigger multiple simultaneous backend reads before any cache entry exists. This defeats the intended per-pass dedupe and can significantly increase Secret Manager traffic/quota usage when configs reuse the same secret repeatedly.

Useful? React with 👍 / 👎.

Use absolute key paths in verification snippet

The verification command asks users to replace only PROJECT/SECRET, but the sample passes key files as '~/.config/gcp/...' inside JavaScript strings, and Node does not expand ~ there. If operators run the snippet as printed, SecretManagerServiceClient receives a literal ~ path and fails with ENOENT, so the IAM isolation check reports errors even when bindings are correct.

Useful? React with 👍 / 👎.