Repository navigation
refactor(doctor): inline GitHub skill discovery inputs - #140343
Merged
Merged
Conversation
Contributor
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
Contributor
|
Codex review: blocked before merge. Reviewed September 6, 2026, 2:12 PM ET / 18:12 UTC. ClawSweeper reviewWhat this changesSimplifies Doctor’s GitHub CLI configuration hint by constructing discovery inputs directly and removing private forwarding helpers without changing displayed guidance. Merge readiness⛔ Blocked before merge - 1 item remains The refactor is useful and behavior-preserving, with no actionable findings. Current main still contains the wrappers. The earlier migration-proof blocker is unsupported by this diff and should not carry forward. Priority: P3 Review scores
Verification
How this fits togetherDoctor checks each agent’s skill readiness and presents diagnostic guidance. For eligible GitHub skills, it checks configuration-file locations and displays a hint when the CLI’s effective location differs from an authenticated location. flowchart TD
A[Doctor skill check] --> B[Agent skill status]
B --> C{GitHub skill eligible?}
C -->|Yes| D[Check config file locations]
E[Process environment and filesystem] --> D
D --> F{Location mismatch?}
F -->|Yes| G[Display GitHub CLI hint]
C -->|No| H[No hint]
F -->|No| H
Before merge
Agent review detailsSecurityNone. PR surfaceSource -15. Total -15 across 1 file. View PR surface stats
Review metricsNone. Stored data modelPersistent data-model change detected: Technical reviewBest possible solution: Keep discovery policy in its existing owner and construct its runtime inputs directly where Doctor builds the hint. Do we have a high-confidence way to reproduce the issue? Not applicable: this PR proposes behavior-preserving cleanup, not a reported runtime defect; source inspection supports equivalent inputs, gating, and output. Is this the best way to solve the issue? Yes. Inlining the single-use private wrappers reduces indirection while retaining the canonical detector and existing behavioral coverage. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against add29edb0b64. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (1 earlier review cycle)
|
steipete
force-pushed
the
steipete/deletion-doctor-gh-discovery
branch
from
September 6, 2026 18:07
837ce68 to
8b621c7
Compare
steipete
marked this pull request as ready for review
September 6, 2026 18:07
Contributor
Author
|
Merged via squash.
|
vincentkoc
added a commit
that referenced
this pull request
Sep 6, 2026
* origin/main: (25 commits) docs(releases): keep the release-notes nav group to version routes (#140353) fix(ui): hide trust wrappers in imported chat history fix(subagents): subagent list building decodes unrelated saved prompts in large session stores (#139540) test(process): share relay startup fixtures (#140345) refactor(doctor): inline GitHub skill discovery inputs (#140343) refactor(ui): share channel status type definitions (#140331) refactor: simplify queue state and infrastructure dispatch (#140311) fix: require saved agent turns before update verification succeeds (#140274) fix(update): keep the service profile when switching to Git (#140263) fix(ui): prevent image avatars overlapping mobile chat (#140341) fix(test): accept release subpages in docs route shape (#140337) refactor(plugins): share uninstall mutation ownership (#140322) refactor(agents): use canonical tool execution arguments (#140280) perf(cli): keep inference metadata and domain help lazy (#140327) refactor(plugins): simplify self-hosted setup results (#140332) refactor(config): reuse parent lookup for unset operations (#140326) fix(plugins): preserve exact installed-index mutation receipts (#140328) test(agents): simplify MCP and LSP boundary fixtures (#140312) fix(ui): preserve literal source previews and copied whitespace (#140306) fix: OpenCode Go/Zen requests are missing the required x-opencode-session header (#137464) ...
github-actions Bot
pushed a commit
to Desicool/openclaw
that referenced
this pull request
Sep 7, 2026
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
1 participant
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.
What Problem This Solves
Doctor's GitHub CLI hint path went through two private wrappers for default inputs and a purported test injection seam. Neither has another caller; tests use the real Doctor entry and the canonical discovery owner. This production-deletion follow-up to #135868 removes that indirection.
Why This Change Was Made
Construct the same discovery input directly in the hint owner, before the existing eligibility checks. Pass the existing filesystem predicate directly, combine the identical missing/ineligible early returns, and infer the detector result type.
defaultGhConfigDiscoveryInputanddescribeGhConfigDirHintFromDiscoverywrapperssrc/commands/doctor-skills.ts:22owns the same inputs and detection. Real Doctor hint output remains covered bysrc/commands/doctor-skills.test.ts:207.detectGhConfigDirMismatchstill receives the same platform, environment and existence check;src/skills/lifecycle/gh-config-discovery.test.ts:28retains its 18 platform/discovery cases unchanged.src/commands/doctor-skills.test.ts:230protects ineligible suppression; non-GitHub skill cases at:102continue through the same real Doctor entry.Whole-repository search finds no external caller of the deleted private functions.
git log -S defaultGhConfigDiscoveryInputreaches the shallow history boundarye357203be57; no older introduction claim is made. No public or shipped injection contract is removed.User Impact
No observable behavior change. Discovery input construction order, eligibility, configuration hints and displayed output remain unchanged. No tests were edited or removed.
Evidence
Doctor skills and canonical GitHub config discovery suites: all 28 tests passed before and after.
Independent Codex review: scoped-clean, no actionable P0–P2 findings.
Production +6 / −21 = −15 lines. Tests/support, tooling and docs: unchanged.
Full
node scripts/check-changed.mjspassed, including typechecking, Knip, lint and runtime import-cycle checks.Rebased onto the main-side docs route repair #140337. Earlier source validation remains byte-identical; the lockfile is unchanged.
ClawSweeper disposition: its technical re-review confirms that configuration repair, persistence and authorization code are unchanged, and says the earlier migration-proof blocker is unsupported. The rendered “Add data-model compatibility proof” checklist is therefore skipped: this diff changes no schema, stored representation, migration, retention, concurrency, recovery or projection semantics. The same 28 existing tests and identical discovery inputs/output protect the actual contract. Exact-head CI run 34050729824 is green.