Skip to content

fix(configure): re-read config hash after persist to avoid stale-hash race (#64188)#66528

Merged
vincentkoc merged 2 commits intoopenclaw:mainfrom
feiskyer:fix/64188-configure-copilot-race
Apr 15, 2026
Merged

fix(configure): re-read config hash after persist to avoid stale-hash race (#64188)#66528
vincentkoc merged 2 commits intoopenclaw:mainfrom
feiskyer:fix/64188-configure-copilot-race

Conversation

@feiskyer
Copy link
Copy Markdown
Contributor

Summary

Fix ConfigMutationConflictError when adding GitHub Copilot model via openclaw configure.

The configure wizard reads the config snapshot hash once at start, but plugins (e.g. github-copilot) can mutate the config file during promptAuthConfig(). When persistConfig() then tries to write, the on-disk hash has changed → conflict error.

Root Cause

persistConfig() used the initial hash for the first write, then set currentBaseHash = undefined — meaning:

  1. First write fails if a plugin mutated config in the meantime
  2. Subsequent writes skip hash validation entirely (silent overwrites)

Fix

  • On conflict, re-read the snapshot to get the fresh hash and retry (up to 3 attempts)
  • After each successful write, re-read the snapshot to refresh currentBaseHash instead of setting it to undefined

Test

Added test case that simulates plugin mutation during wizard flow and verifies the retry path.

Closes #64188

Security Impact

  1. Does this change handle user input or external data? No
  2. Does this touch authentication, authorization, or credential handling? No
  3. Does this affect sandboxing or process isolation? No
  4. Does this change network-facing code? No
  5. Does this modify security-related configuration? No

Human Verification

  • pnpm test src/commands/configure.wizard.test.ts — 9/9 passed
  • pnpm build — passed
  • Verified fix logic matches the race condition described in issue

@openclaw-barnacle openclaw-barnacle bot added commands Command implementations size: S labels Apr 14, 2026
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Apr 14, 2026

Greptile Summary

This PR fixes a ConfigMutationConflictError in the configure wizard by wrapping replaceConfigFile in a retry loop (up to 3 attempts) and refreshing currentBaseHash from the snapshot after each successful write, replacing the previous currentBaseHash = undefined pattern that silently bypassed hash validation on subsequent writes.

Confidence Score: 5/5

  • Safe to merge; the retry logic is correct and the one remaining note is a minor style suggestion.
  • The fix correctly handles the race condition: retries on conflict using the fresh hash, throws on exhaustion or non-conflict errors, and refreshes the hash after each successful write. The only finding is a P2 suggestion to use err.currentHash instead of an extra readConfigFileSnapshot call in the conflict handler.
  • No files require special attention.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: src/commands/configure.wizard.ts
Line: 469-475

Comment:
**Unnecessary extra read — `err.currentHash` already holds the fresh hash**

`ConfigMutationConflictError` is thrown by `assertBaseHashMatches`, which reads the current snapshot inside `replaceConfigFile` and sets `err.currentHash` to the on-disk hash at that moment. Re-reading the file immediately after catching the error is an extra round-trip and a new window for another mutation to slip in between the throw and the re-read.

```suggestion
          if (err instanceof ConfigMutationConflictError && attempt < maxRetries - 1) {
            // Config was mutated (likely by a plugin during auth setup)
            // Use the hash from the conflict error directly (already read inside replaceConfigFile)
            currentBaseHash = err.currentHash ?? undefined;
            continue;
          }
```

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

Reviews (1): Last reviewed commit: "fix(configure): re-read config hash afte..." | Re-trigger Greptile

Comment thread src/commands/configure.wizard.ts
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 61ba4e2b62

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/commands/configure.wizard.ts
@feiskyer feiskyer force-pushed the fix/64188-configure-copilot-race branch from 61ba4e2 to c9bd29c Compare April 14, 2026 12:48
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c9bd29c5d1

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/commands/configure.wizard.ts Outdated
@feiskyer feiskyer force-pushed the fix/64188-configure-copilot-race branch 3 times, most recently from b0c74e9 to 4915224 Compare April 15, 2026 03:22
@vincentkoc vincentkoc self-assigned this Apr 15, 2026
@vincentkoc vincentkoc force-pushed the fix/64188-configure-copilot-race branch from 4915224 to effe39d Compare April 15, 2026 10:00
@vincentkoc vincentkoc force-pushed the fix/64188-configure-copilot-race branch from effe39d to 0c4003a Compare April 15, 2026 10:02
@vincentkoc vincentkoc merged commit 804bb0f into openclaw:main Apr 15, 2026
7 checks passed
@vincentkoc
Copy link
Copy Markdown
Member

Merged via squash.

Thanks @feiskyer!

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0c4003a5be

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/commands/configure.wizard.ts
@feiskyer feiskyer deleted the fix/64188-configure-copilot-race branch April 15, 2026 11:53
xudaiyanzi pushed a commit to xudaiyanzi/openclaw that referenced this pull request Apr 17, 2026
… race (openclaw#64188) (openclaw#66528)

Merged via squash.

Prepared head SHA: 0c4003a
Co-authored-by: feiskyer <676637+feiskyer@users.noreply.github.com>
Co-authored-by: vincentkoc <25068+vincentkoc@users.noreply.github.com>
Reviewed-by: @vincentkoc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

commands Command implementations size: S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Github Copilot add model failed on openclaw configure

2 participants