Skip to content

Scope startup version probes to enabled providers#2278

Closed
kiranmagic7 wants to merge 1 commit into
steipete:mainfrom
kiranmagic7:kiran/scope-version-probes-20260718
Closed

Scope startup version probes to enabled providers#2278
kiranmagic7 wants to merge 1 commit into
steipete:mainfrom
kiranmagic7:kiran/scope-version-probes-20260718

Conversation

@kiranmagic7

Copy link
Copy Markdown
Contributor

Summary

  • Scope startup version detection to enabled/display providers instead of every registered provider.
  • Keeps disabled providers from running version probes at launch; for Antigravity this avoids the detectVersion -> isRunning -> antigravity-ps path when the provider is disabled.
  • Add a regression test for the provider selection helper.

Fixes #2267.

Tests

  • make check
  • git diff --check
  • swiftc -parse Sources/CodexBar/UsageStore.swift
  • swiftc -parse Tests/CodexBarTests/UsageStoreVersionDetectionTests.swift
  • Attempted: swift test --skip-update --filter UsageStoreVersionDetectionTests (stalled before test execution while downloading the Sparkle 2.9.3 binary artifact)

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

Copy link
Copy Markdown

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: e8fd17def2

ℹ️ 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 on lines +1266 to +1267
enabledProviders: self.enabledProvidersForDisplay(),
implementations: ProviderCatalog.all)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Rerun version detection when providers become enabled

On a fresh install or when a user enables a provider during the session, this snapshots enabledProvidersForDisplay() before provider detection/toggle changes have finished. SettingsStore.runInitialProviderDetectionIfNeeded() schedules detection asynchronously and later enables Claude/Gemini/Antigravity, but detectVersions() is only invoked from UsageStore init, so those newly enabled providers never get entries in versions until the app is relaunched and the menu/sidebar fall back to plain CLI names. Please rerun version detection, or probe just the newly enabled providers, after provider enablement changes.

Useful? React with 👍 / 👎.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. labels Jul 18, 2026
@clawsweeper

clawsweeper Bot commented Jul 18, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed July 17, 2026, 9:30 PM ET / July 18, 2026, 01:30 UTC.

Summary
The branch filters startup provider version detection to providers enabled for display and adds a helper-level regression test.

Reproducibility: no. live current-head reproduction was provided, but the source-level startup ordering and the existing review comment give a clear path to reproduce the newly introduced first-launch regression.

Review metrics: 1 noteworthy metric.

  • Patch scope: 2 files affected; 27 additions, 1 deletion. The implementation is small, but its only new test exercises the selection helper rather than the asynchronous startup transition that causes the regression.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🦐 gold shrimp
Result: blocked until real behavior proof from a real setup is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P1] Rebase the branch and repair the asynchronous provider-enablement path.
  • [P1] Add focused coverage for providers enabled after the initial version-detection snapshot.
  • [P1] Post redacted after-fix runtime evidence; updating the PR body should trigger a fresh ClawSweeper review, or ask a maintainer to comment @clawsweeper re-review if it does not.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR reports static checks and an attempted test run, but contains no after-fix runtime evidence showing disabled providers are skipped while providers enabled during startup still receive version metadata; add redacted live logs, terminal output, or a focused runtime recording. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

  • [P1] On a fresh install, providers enabled asynchronously after the snapshot can display generic CLI names instead of detected version metadata until the app is relaunched.
  • [P1] The branch is currently reported as not cleanly mergeable, so the repair must be rebased and revalidated against current main.

Maintainer options:

  1. Refresh versions after enablement (recommended)
    Rebase, rerun version detection for providers enabled by initial detection, and add a regression test that verifies version metadata is populated without a relaunch.
  2. Pause the filtering change
    Do not merge the filter until startup provider detection and version detection have a coherent lifecycle that preserves first-launch metadata.

Next step before merge

  • [P1] The patch has a concrete repair, but the contributor must address the startup lifecycle regression, rebase the dirty branch, and provide real behavior proof before merge.

Security
Cleared: The diff only changes in-process provider selection for an existing version-probe path and adds a test; no new security or supply-chain concern was identified.

Review findings

  • [P2] Rerun version detection after providers become enabled — Sources/CodexBar/UsageStore.swift:1265
Review details

Best possible solution:

Preserve the enabled-provider filter, then trigger targeted version detection after initial provider detection enables providers and cover both the already-enabled and newly-enabled startup paths.

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

No live current-head reproduction was provided, but the source-level startup ordering and the existing review comment give a clear path to reproduce the newly introduced first-launch regression.

Is this the best way to solve the issue?

No. Filtering disabled providers is the right direction, but version detection must also run after initial detection changes provider enablement so the existing startup experience remains complete.

Full review comments:

  • [P2] Rerun version detection after providers become enabled — Sources/CodexBar/UsageStore.swift:1265
    The filter captures enabled providers before asynchronous initial detection can enable Claude, Gemini, or Antigravity. Because this method only runs during UsageStore initialization, those providers receive no version entry until relaunch. Trigger detection after enablement changes (or detect newly enabled providers) and add a test covering that transition.
    Confidence: 0.96

Overall correctness: patch is incorrect
Overall confidence: 0.94

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 6a152bdf8b48.

Label changes

Label changes:

  • add P2: The patch can regress normal first-launch provider presentation for users whose providers are enabled asynchronously during startup.
  • add merge-risk: 🚨 compatibility: Existing startup detection flows can lose version metadata until a relaunch when this snapshot excludes providers that become enabled later.
  • add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦐 gold shrimp.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR reports static checks and an attempted test run, but contains no after-fix runtime evidence showing disabled providers are skipped while providers enabled during startup still receive version metadata; add redacted live logs, terminal output, or a focused runtime recording. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Label justifications:

  • P2: The patch can regress normal first-launch provider presentation for users whose providers are enabled asynchronously during startup.
  • merge-risk: 🚨 compatibility: Existing startup detection flows can lose version metadata until a relaunch when this snapshot excludes providers that become enabled later.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦐 gold shrimp.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR reports static checks and an attempted test run, but contains no after-fix runtime evidence showing disabled providers are skipped while providers enabled during startup still receive version metadata; add redacted live logs, terminal output, or a focused runtime recording. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

What I checked:

  • Patch regression: The changed call snapshots enabledProvidersForDisplay() before version detection, while the documented review evidence shows initial provider detection can asynchronously enable providers later in the same session; those providers are then omitted from versions until relaunch. (Sources/CodexBar/UsageStore.swift:1265, e8fd17def21a)
  • Current implementation surface: Current main still contains UsageStore.detectVersions() as the startup version-detection path and does not contain the proposed implementationsForVersionDetection helper, so this PR remains necessary rather than already being implemented on main. (Sources/CodexBar/UsageStore.swift:1262, 6a152bdf8b48)
  • Review feedback: The existing line review identifies the concrete fresh-install/session-enable failure mode and asks for version detection to rerun after provider enablement changes. (Sources/CodexBar/UsageStore.swift:1265, e8fd17def21a)
  • Merge state: GitHub reports the branch as dirty against main, so it needs a rebase and a refreshed validation pass after resolving the functional issue. (e8fd17def21a)

Likely related people:

  • steipete: The affected startup-provider and usage-store behavior is in the central CodexBar application code; repository ownership is the available routing signal for this interaction. (role: repository owner and likely routing contact; confidence: low; files: Sources/CodexBar/UsageStore.swift, Sources/CodexBar/SettingsStore+ProviderDetection.swift)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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.

How this review workflow works
  • 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

Copy link
Copy Markdown
Owner

Thank you, Kiran — this was a same-day collision with #2277, which landed just before this PR was opened and fixes the same #2267 startup probe path. I compared both patches directly. Your helper and regression test cover the disabled-provider gate, but #2277 already includes that behavior plus the important settings-observation path that re-runs version detection when a provider is enabled later, and broader empty/enabling coverage. I also checked CLIHelpers.detectVersion: that path runs only for the provider explicitly requested by the CLI after a successful non-web fetch, so there is no additional startup all-provider probe to carry over. No unique delta remains to cherry-pick. Closing as superseded, with credit for the independent diagnosis and focused patch.

@steipete steipete closed this Jul 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ps -ax (Antigravity process probe) runs on every launch even with Antigravity disabled, triggering repeated "access data from other apps" TCC prompt

2 participants