Skip to content

fix: honor standalone doctor config and validate provider names - #213

Merged
steipete merged 1 commit into
mainfrom
fix/phase-five-provider-config
Sep 12, 2026
Merged

fix: honor standalone doctor config and validate provider names#213
steipete merged 1 commit into
mainfrom
fix/phase-five-provider-config

Conversation

@steipete

@steipete steipete commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

Before init, doctor --config ... could silently check Codex instead of the provider selected in the config: configuration loading was tied to required project state, and the missing-state handler discarded the loaded settings. Separately, inherited object names such as toString were accepted by the provider registry and failed later with provider.check is not a function.

User Impact

Doctor honors standalone config before initialization, preserves normal flag/environment precedence, and reports missing state without creating it. Unsupported provider names fail consistently with the existing usage error.

Why This Change Was Made

Load configuration independently from optional project metadata, reuse the normal provider-flag merge, and restrict registry lookup to its own entries. No harness permission or authentication policy changes.

Evidence

  • Six new cases fail on the old implementation; precedence preservation also has regression coverage.

  • Live before: an explicit config selecting mock reports codex and loses the configured model; --provider toString exits 1 with a method error.

  • Live after: the same explicit config reports provider mock and its fixture-model; --provider toString exits 2 with unsupported provider: toString.

  • Full validation: pnpm typecheck, pnpm lint, pnpm format:check, pnpm test (957 passed, 2 existing platform skips), and pnpm build pass.

  • Isolated Codex autoreview: scoped-clean at P0–P2 against the final formatted files.

  • Final installed-package smoke: 13 features mapped, including 3 CUDA; pnpm website:smoke also passed.

@steipete
steipete requested a review from a team as a code owner September 12, 2026 23:37
@clawsweeper

clawsweeper Bot commented Sep 12, 2026

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.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Sep 12, 2026
@clawsweeper

clawsweeper Bot commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed September 12, 2026, 7:39 PM ET / 23:39 UTC.

ClawSweeper review

What this changes

Doctor now honors configuration before project initialization, and provider lookup rejects inherited object-property names with the existing unsupported-provider error.

Merge readiness

Ready for maintainer review

Keep open: both defects remain on current main, and the focused patch addresses them without an identified correctness blocker.

Priority: P2
Reviewed head: 03f2a8114f5c9eef6ec19879e6e16f6b36c31f0f

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused repair with relevant live CLI observations, regression coverage, and no identified blocking defect.
Proof confidence 🐚 platinum hermit (4/6) Sufficient (live_output): The captured body supplies concrete before/after CLI observations for doctor’s standalone config selection and unsupported-name exit behavior. The built-in mock provider is a sufficient selection target for this configuration repair; external provider execution is unchanged.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (live_output): The captured body supplies concrete before/after CLI observations for doctor’s standalone config selection and unsupported-name exit behavior. The built-in mock provider is a sufficient selection target for this configuration repair; external provider execution is unchanged.
Evidence reviewed 7 items Verified introduced changes: The pinned main-to-head diff changes doctor configuration handling and adds an own-property registry check; neither repair is already present on the pinned main.
Existing configuration and state boundaries: loadConfig already implements discovery, environment precedence, and trusted Codex-config checks. loadProjectState throws after loading config when project metadata is absent; the old doctor catch discards those settings. The replacement retains the same root, state-path resolution, and project validation without creating state directories.
Fresh and initialized-project coverage: Three new doctor cases cover explicit config, environment-selected config, flag precedence, and absent state-directory preservation; four registry cases reject inherited keys. Existing workflow coverage checks initialized-project reasoning overrides and rejection of untrusted Codex passthrough configuration. Tests were inspected, not executed during this read-only review.
Findings None None.
Security None None.

How this fits together

Clawpatch’s doctor command combines project configuration, environment variables, and command flags to select a local coding provider. It reports provider availability and initialization state without starting a repair.

flowchart TD
  A[Config and environment] --> C[Resolve provider settings]
  B[Command flags] --> C
  C --> D[Read optional project state]
  D --> E{Supported provider name?}
  E -->|Yes| F[Check provider availability]
  E -->|No| G[Usage error]
  F --> H[Diagnostic report]
Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test delta Production −23 lines; tests +67 lines The repair removes duplicated configuration logic and adds seven focused regression cases.

Technical review

Best possible solution:

Keep doctor on the shared configuration path while treating project metadata as optional and accepting only registered provider names.

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

Yes: current-main source shows standalone config being discarded after missing-state detection and inherited registry keys being accepted. The supplied CLI observations corroborate both paths; no reproduction was executed in this review.

Is this the best way to solve the issue?

Yes: reusing the existing loader and flag merger removes the divergent path, while the own-property check narrowly rejects invalid names without changing supported providers or persisted settings.

AGENTS.md: found and applied where relevant.

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

Labels

Label changes:

  • add P2: This repairs bounded diagnostic configuration and provider-name validation defects without evidence of an urgent user-facing outage.
  • add proof: sufficient: Contributor real behavior proof is sufficient. The captured body supplies concrete before/after CLI observations for doctor’s standalone config selection and unsupported-name exit behavior. The built-in mock provider is a sufficient selection target for this configuration repair; external provider execution is unchanged.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The captured body supplies concrete before/after CLI observations for doctor’s standalone config selection and unsupported-name exit behavior. The built-in mock provider is a sufficient selection target for this configuration repair; external provider execution is unchanged.

Label justifications:

  • P2: This repairs bounded diagnostic configuration and provider-name validation defects without evidence of an urgent user-facing outage.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The captured body supplies concrete before/after CLI observations for doctor’s standalone config selection and unsupported-name exit behavior. The built-in mock provider is a sufficient selection target for this configuration repair; external provider execution is unchanged.
  • proof: sufficient: Contributor real behavior proof is sufficient. The captured body supplies concrete before/after CLI observations for doctor’s standalone config selection and unsupported-name exit behavior. The built-in mock provider is a sufficient selection target for this configuration repair; external provider execution is unchanged.

Evidence

What I checked:

  • Verified introduced changes: The pinned main-to-head diff changes doctor configuration handling and adds an own-property registry check; neither repair is already present on the pinned main. (src/app.ts:239, 03f2a8114f5c)
  • Existing configuration and state boundaries: loadConfig already implements discovery, environment precedence, and trusted Codex-config checks. loadProjectState throws after loading config when project metadata is absent; the old doctor catch discards those settings. The replacement retains the same root, state-path resolution, and project validation without creating state directories. (src/app-context.ts:16, 03f2a8114f5c)
  • Fresh and initialized-project coverage: Three new doctor cases cover explicit config, environment-selected config, flag precedence, and absent state-directory preservation; four registry cases reject inherited keys. Existing workflow coverage checks initialized-project reasoning overrides and rejection of untrusted Codex passthrough configuration. Tests were inspected, not executed during this read-only review. (src/doctor.test.ts:23, 03f2a8114f5c)
  • Supplied live behavior observations: The complete captured PR body reports that an explicit standalone config changed doctor output from codex with missing model to mock with fixture-model, and that --provider toString changed from exit 1 with a method error to exit 2 with unsupported provider: toString. These observations exercise configuration selection and CLI error handling; they do not establish external harness compatibility. Captured context sourceRevision: eb557202995742658a7ccc3bbbbf881ddd3d144f654c44cdc9d765dff131f016. (03f2a8114f5c)
  • Latest release still has the doctor defect: GitHub identifies v0.8.0 as the latest release. Its doctor implementation still catches missing initialization by discarding the loaded configuration and falling back to codex. (src/app.ts:235, e78e71836812)
  • Historical routing: Prior merged history includes the project-context extraction by Peter Steinberger, whose GitHub commit identity resolves to steipete. The inspected patch adds the current configuration-loading line in app-context.ts; this supports area routing, not attribution of the original bug. Broader historical searches encountered unavailable objects, so original feature introduction remains unestablished. (src/app-context.ts:17, 82f94f3085cc)

Likely related people:

  • Peter Steinberger: Raw commit 82f94f3 adds src/app-context.ts:17 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: 82f94f3085cc; files: src/app-context.ts)

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
steipete merged commit 8fdec84 into main Sep 12, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant