Skip to content

fix(setup): verify and save provider starter aliases correctly - #139339

Merged
steipete merged 2 commits into
mainfrom
codex/130706-extract-setup-20260905
Sep 5, 2026
Merged

fix(setup): verify and save provider starter aliases correctly#139339
steipete merged 2 commits into
mainfrom
codex/130706-extract-setup-20260905

Conversation

@steipete

@steipete steipete commented Sep 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What Problem This Solves

Model Setup could reject a working provider when authentication returned a starter alias: inference reported the canonical model while setup compared it against the alias. Selected model metadata also needed to follow the canonical key without deleting existing operator settings. This is a focused extraction from #130706.

Why This Change Was Made

Use the selected provider's normalization hook during preparation, retaining its method receiver, then carry the canonical model through verification and persistence. Reuse the existing model-entry merge owner for both defaults and per-agent settings: explicit starter fields win, while unspecified fields and nested parameters survive. Guided discovery, custom setup and the Claude CLI probe/persist split retain their existing identities. Duplicate cloning and provider-key lookup are removed.

User Impact

API-key, token and provider-login setup can verify and save provider-owned starter aliases with the intended model settings. No configuration option, protocol, model catalog or persistence schema is added. Whole PR production: +51/-58, net -7; tests +91/-13, net +78; docs +4/-2.

Evidence

  • Starter-alias and method-receiver failures were reproduced before their repairs. Both canonical metadata collision cases failed before the follow-up; fresh setup remained a passing control.
  • Final 161 setup tests, 21 existing merge/auth helpers and one provider-auth-choice boundary test passed. Full changed checks and independent review through P2 passed.
  • Normal compiled authenticated OpenAI/gpt-5.6-luna setup passed for partial and empty starter metadata. Both returned HTTP 200, retained the provider receiver, preserved canonical defaults and per-agent settings, saved the canonical profile/model and removed alias rows. Isolated state and the Testbox were cleaned up. Final proof run.
  • Commit d6d50d1d45877f57da91c275300032c7f641d642 has tree 9d7210640a8572129d5c2a8029fde1cf10486520, exactly matching the compiled final live proof. The earlier full build and starter-alias live control also passed.
  • Earlier source-only provider loading exposed a separate dependency-export problem; the compiled operator path passed. An initial Groq credential returned HTTP 401 after reaching the correct canonical model. These are recorded failures, not claimed successes.

ClawSweeper follow-through: the canonical-settings finding and both Rank-up moves are addressed by merging unspecified canonical fields/nested params and extending activation coverage to fresh, partial and empty starter metadata in both maps. No finding is deferred.

@steipete
fix(setup): canonicalize provider starter models before verification
d64b893 
Use the selected provider's model-id hook, preserving its method receiver,
for raw authentication starters. Carry the canonical model through the
setup probe and persistence, including selected model metadata.

Keep guided and custom-provider selections canonical, and simplify copying
within the configuration projection's existing deep clone. Preserve the
Claude CLI probe/persistence split and current auth rollback ownership.

Refs #130706.
@clawsweeper

clawsweeper Bot commented Sep 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@openclaw-barnacle openclaw-barnacle Bot added docs Improvements or additions to documentation size: S maintainer Maintainer-authored PR labels Sep 5, 2026
@clawsweeper clawsweeper Bot added P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Sep 5, 2026
@clawsweeper

clawsweeper Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Codex review: blocked before merge. Reviewed September 5, 2026, 4:00 PM ET / 20:00 UTC.

ClawSweeper review

What this changes

The PR resolves provider starter aliases before setup verification and saves the canonical model name with the selected model metadata.

Regression provenance

Possible regression — suspected (reviewed change). No predecessor PR is attributed.

Merge readiness

⛔ Blocked before merge - 3 items remain

The alias repair remains useful, but its metadata projection can erase an existing canonical model’s saved settings. This maintainer PR should remain open.

Priority: P2
Reviewed head: d64b893a8ec3e979256c86ca28509ef01a556f1a

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) A focused owner-level repair with useful regression coverage, limited by a concrete existing-settings overwrite.
Proof confidence 🌊 off-meta tidepool Not applicable: The maintainer-authored PR is exempt from ordinary contributor proof requirements. Its supplied body reports compiled authenticated setup through the changed preparation and persistence path; linked logs could not be retrieved, and the reported scenario does not establish existing canonical-row preservation.
Patch quality 🦐 gold shrimp (3/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: The maintainer-authored PR is exempt from ordinary contributor proof requirements. Its supplied body reports compiled authenticated setup through the changed preparation and persistence path; linked logs could not be retrieved, and the reported scenario does not establish existing canonical-row preservation.
Evidence reviewed 11 items Introduced change survives the verified merge: The test merge records the pinned main commit first and PR head second. Comparing its result with main retains the canonical-key replacement assignments; this concern is introduced by the PR, not endpoint drift.
Canonical metadata collision: The projection starts with a deep clone of existing configuration, then assigns the raw starter entry wholesale to targetModelRef. Existing canonical fields absent from the starter entry are discarded. The per-agent assignment has the same behavior.
Existing metadata preservation contract: Model-map normalization already merges entries that collapse onto one canonical reference, including nested params. Provider auth patch preparation also merges existing configuration before projection; the new cross-key replacement loses that preservation.
Findings 1 actionable finding [P1] Preserve existing canonical metadata when projecting an alias
Security None None.

How this fits together

Model Setup turns provider credentials and a starter model into a verified inference connection. Its preparation step supplies both the live probe and the configuration later saved for the selected agent.

flowchart TD
  A[Provider credentials and starter model] --> B[Provider alias normalization]
  B --> C[Selected model metadata projection]
  D[Existing user settings] --> C
  C --> E[Live inference verification]
  E --> F{Verification succeeds}
  F -->|Yes| G[Save model and credential]
  F -->|No| H[Show setup failure]
Loading

Before merge

  • Preserve existing canonical metadata when projecting an alias (P1) - When modelRef and targetModelRef differ, the prepared configuration can contain both an existing canonical row and the provider’s new alias row. This assignment replaces the canonical row wholesale: for example, an existing { params: { temperature: 0.7 } } becomes {} when authentication returns an empty alias entry. createMergePatch then records those missing fields as deletions, which are persisted after a successful probe. The per-agent assignment below has the same defect. Merge the selected metadata while retaining unspecified canonical fields and nested params, and extend the activation cases with existing canonical rows.
  • Resolve merge risk (P1) - Existing users who configure an alias-backed starter can lose canonical model settings that the provider’s starter metadata does not specify; the same collision affects defaults and per-agent entries.
  • Complete next step (P2) - Repair canonical metadata collision handling for defaults and per-agent entries, with regression coverage proving existing settings survive.

Findings

  • [P1] Preserve existing canonical metadata when projecting an alias — src/system-agent/setup-inference-plan-helpers.ts:328-333
Agent review details

Security

None.

PR surface

Source -4, Tests +21, Docs +2. Total +19 across 4 files.

View PR surface stats
Area Files Added Removed Net
Source 2 35 39 -4
Tests 1 27 6 +21
Docs 1 4 2 +2
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 4 66 47 +19

Review metrics

Metric Value Why it matters
Production versus regression coverage Production net -4 lines; tests net +21 lines The owner-level repair removes redundant cloning while extending existing activation cases rather than adding another implementation path.
Settings collision surfaces 2 model maps affected; 0 new configuration options Both default and per-agent canonical metadata require preservation when a starter alias resolves onto an existing entry.

Merge-risk options

Maintainer options:

  1. Preserve canonical settings during alias projection (recommended)
    Merge rather than replace canonical metadata and cover existing defaults and per-agent rows before landing.

Technical review

Best possible solution:

Normalize at setup preparation, then merge selected metadata into the canonical entry while preserving unspecified existing fields and nested parameters.

Do we have a high-confidence way to reproduce the issue?

Yes, from source: prepopulate the canonical model entry with parameters, return an empty starter-alias entry from provider authentication, and normalize that alias to the existing model. The projection drops those parameters; execution was not performed in this read-only review.

Is this the best way to solve the issue?

Not yet. Normalizing at preparation is the right boundary, but wholesale replacement violates the existing model-map merge behavior; weakening winner verification would also leave persistence incorrect.

Full review comments:

  • [P1] Preserve existing canonical metadata when projecting an alias — src/system-agent/setup-inference-plan-helpers.ts:328-333
    When modelRef and targetModelRef differ, the prepared configuration can contain both an existing canonical row and the provider’s new alias row. This assignment replaces the canonical row wholesale: for example, an existing { params: { temperature: 0.7 } } becomes {} when authentication returns an empty alias entry. createMergePatch then records those missing fields as deletions, which are persisted after a successful probe. The per-agent assignment below has the same defect. Merge the selected metadata while retaining unspecified canonical fields and nested params, and extend the activation cases with existing canonical rows.
    Confidence: 0.97

Overall correctness: patch is incorrect
Overall confidence: 0.95

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 6b1314938d6d.

Labels

Label changes:

  • add P2: This is a bounded provider-setup repair with an introduced settings-preservation defect.
  • add merge-risk: 🚨 compatibility: The canonical-key assignment can silently remove existing user model settings during successful setup.
  • add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🌊 off-meta tidepool and patch quality is 🦐 gold shrimp.
  • add status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: The maintainer-authored PR is exempt from ordinary contributor proof requirements. Its supplied body reports compiled authenticated setup through the changed preparation and persistence path; linked logs could not be retrieved, and the reported scenario does not establish existing canonical-row preservation.

Label justifications:

  • P2: This is a bounded provider-setup repair with an introduced settings-preservation defect.
  • merge-risk: 🚨 compatibility: The canonical-key assignment can silently remove existing user model settings during successful setup.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🌊 off-meta tidepool and patch quality is 🦐 gold shrimp.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: The maintainer-authored PR is exempt from ordinary contributor proof requirements. Its supplied body reports compiled authenticated setup through the changed preparation and persistence path; linked logs could not be retrieved, and the reported scenario does not establish existing canonical-row preservation.

Evidence

What I checked:

  • Introduced change survives the verified merge: The test merge records the pinned main commit first and PR head second. Comparing its result with main retains the canonical-key replacement assignments; this concern is introduced by the PR, not endpoint drift. (src/system-agent/setup-inference-plan-helpers.ts:333, d854395f7f65)
  • Canonical metadata collision: The projection starts with a deep clone of existing configuration, then assigns the raw starter entry wholesale to targetModelRef. Existing canonical fields absent from the starter entry are discarded. The per-agent assignment has the same behavior. (src/system-agent/setup-inference-plan-helpers.ts:333, d64b893a8ec3)
  • Existing metadata preservation contract: Model-map normalization already merges entries that collapse onto one canonical reference, including nested params. Provider auth patch preparation also merges existing configuration before projection; the new cross-key replacement loses that preservation. (src/config/model-input.ts:109, d64b893a8ec3)
  • Replacement becomes a durable deletion: buildPreparedProviderTestPlan computes a merge patch from the original configuration to the projected configuration. createMergePatch emits null for removed fields, and applyManualAuthConfig applies that patch during persistence. Concurrent-change guards do not reject an unchanged pre-existing canonical row. (src/config/merge-patch.ts:19, d64b893a8ec3)
  • Owner, caller, probe, and persistence trace: activateSetupInference calls buildTestPlan, stages the selected model, runs runSetupInferenceTest, and invokes persistActivatedSetupInference only after verification. Credential-owner checks, cancellation checks, rollback, and final owner revalidation remain intact. Guided/custom selection bypasses the new normalization hook. (src/system-agent/setup-inference-activate.ts:179, d64b893a8ec3)
  • Normalization contract and implementation boundary: ProviderPlugin.normalizeModelId is the existing provider-owned alias-cleanup contract before lookup. This patch consumes that OpenClaw contract without changing it; its alias behavior does not depend on the Codex protocol or harness. The existing runtime helper invokes the same hook with its method receiver preserved. (src/plugins/provider-plugin.types.ts:194, d64b893a8ec3)

Likely related people:

  • steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • lei_lei: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Preserve unspecified canonical metadata, including nested params, in both default and per-agent maps.
  • Extend the activation regressions to cover fresh setup and alias selection over existing canonical settings.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@steipete
fix(setup): preserve canonical model settings during alias projection
d6d50d1 
Merge starter metadata into existing default and per-agent model entries
using the canonical configuration merge helper, preserving unspecified
fields and nested parameters. Reuse normalized provider-key lookup with
exact-key precedence instead of maintaining a duplicate helper.

Extend activation regressions for fresh, partial, and empty starter
metadata. The compiled setup path passed authenticated OpenAI completion
and persistence proof for both collision cases.
@steipete
steipete marked this pull request as ready for review September 5, 2026 21:30
@steipete
steipete merged commit c70504d into main Sep 5, 2026
248 checks passed
@steipete
steipete deleted the codex/130706-extract-setup-20260905 branch September 5, 2026 21:59
@steipete

steipete commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

Merged via squash.

github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Sep 6, 2026
@steipete
fix(setup): verify and save provider starter aliases correctly (openc…
23bb2cb 
…law#139339)

* fix(setup): canonicalize provider starter models before verification

Use the selected provider's model-id hook, preserving its method receiver,
for raw authentication starters. Carry the canonical model through the
setup probe and persistence, including selected model metadata.

Keep guided and custom-provider selections canonical, and simplify copying
within the configuration projection's existing deep clone. Preserve the
Claude CLI probe/persistence split and current auth rollback ownership.

Refs openclaw#130706.

* fix(setup): preserve canonical model settings during alias projection

Merge starter metadata into existing default and per-agent model entries
using the canonical configuration merge helper, preserving unspecified
fields and nested parameters. Reuse normalized provider-key lookup with
exact-key precedence instead of maintaining a duplicate helper.

Extend activation regressions for fresh, partial, and empty starter
metadata. The compiled setup path passed authenticated OpenAI completion
and persistence proof for both collision cases.
@steipete steipete mentioned this pull request Sep 10, 2026
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

docs Improvements or additions to documentation maintainer Maintainer-authored PR merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. P2 Normal backlog priority with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. size: M status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@steipete