Skip to content

Explain Kiro CLI requirement when no fetch strategy is available - #2465

Merged
steipete merged 2 commits into
steipete:mainfrom
hxy91819:fix/kiro-cli-fetch-error
Aug 2, 2026
Merged

Explain Kiro CLI requirement when no fetch strategy is available#2465
steipete merged 2 commits into
steipete:mainfrom
hxy91819:fix/kiro-cli-fetch-error

Conversation

@hxy91819

Copy link
Copy Markdown
Contributor

Problem

When Kiro is enabled but kiro-cli is not installed or is not discoverable, CodexBar only reports:

No available fetch strategy for kiro.

This is not actionable for users. Kiro usage is currently collected exclusively through the local Kiro CLI, which also requires the user to complete kiro-cli login (including enterprise/IAM login flows).

Cause

The Kiro fetch strategy advertises itself as available only when kiro-cli can be resolved. If it is unavailable, the generic ProviderFetchError.noAvailableStrategy message is surfaced without provider-specific guidance.

Solution

Add a Kiro-specific error message that explains the CLI requirement, links to the Kiro CLI documentation, and tells the user to run kiro-cli login.

Other providers retain the existing generic error message.

Validation

  • swift test --filter KiroStatusProbeTests (56 tests passed)
  • make check (passed; SwiftFormat and SwiftLint reported no violations)

@hxy91819
hxy91819 force-pushed the fix/kiro-cli-fetch-error branch from 7cbdd58 to a96e849 Compare July 27, 2026 01:44

@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: 7cbdd58b06

ℹ️ 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".

try await fetcher.fetchResolvedCookie(logger: logger)
},
clearCached: { cached in
_ = CookieHeaderCache.clearIfCurrent(provider: .ollama, expected: cached)

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 Honor failed conditional clears before storing fallback cookies

When the cached Ollama cookie expires while another refresh or login has already replaced the cache, clearIfCurrent returns false, but this path discards that result and fetchAutomatic continues to import and store a browser cookie unconditionally. In that race the older in-flight refresh can overwrite the newer cache entry, switching the app back to a stale or different browser session; treat a failed conditional clear as a retry/abort before allowing storeResolved to run.

Useful? React with 👍 / 👎.

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. labels Jul 27, 2026
@clawsweeper

clawsweeper Bot commented Jul 27, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed August 1, 2026, 11:59 PM ET / August 2, 2026, 03:59 UTC.

ClawSweeper review

What this changes

Replace Kiro’s generic unavailable-fetch message with install and login instructions for the local Kiro CLI, plus a focused regression test.

Merge readiness

Blocked until real behavior proof from a real setup is added - 3 items remain

The patch is a narrow, correct repair for Kiro users whose only fetch strategy is unavailable because kiro-cli is absent, and current main still shows the generic error. It remains open because the branch provides tests and static-check claims but no redacted after-fix evidence from a real missing-CLI setup.

Priority: P3
Reviewed head: cebb14d16658d7331f53f03692b845cf89d4b8e0

Review scores

Measure Result What it means
Overall readiness 🧂 unranked krab (1/6) The implementation is focused and source-consistent, but real after-fix behavior proof is still a merge gate.
Proof confidence 🧂 unranked krab (1/6) Needs real behavior proof before merge: The PR body reports focused tests and make check, but it contains no redacted after-fix terminal output, runtime log, or app capture from a real Kiro setup without kiro-cli; add that evidence and update the PR body to trigger re-review.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Needs proof Needs real behavior proof before merge: The PR body reports focused tests and make check, but it contains no redacted after-fix terminal output, runtime log, or app capture from a real Kiro setup without kiro-cli; add that evidence and update the PR body to trigger re-review.
Evidence reviewed 5 items Current-main behavior: The shared unavailable-strategy error on current main still renders the generic provider message, so the requested Kiro guidance is not already implemented.
Source reproduction path: Kiro’s descriptor supplies only KiroCLIFetchStrategy, and that strategy is available only when TTYCommandRunner.which("kiro-cli") succeeds; a missing executable therefore leaves no Kiro fetch strategy.
Existing Kiro diagnostic boundary: When the probe can run, current code already distinguishes cliNotFound and notLoggedIn; the proposed branch addresses the earlier no-strategy path without changing probe behavior.
Findings None None.
Security None None.

How this fits together

CodexBar selects a provider-specific usage fetch strategy and presents its failure message in the app when no strategy can run. Kiro has one local CLI strategy, so an unavailable kiro-cli reaches the shared unavailable-strategy error rather than the probe’s more specific errors.

flowchart LR
    A[Kiro enabled] --> B[Kiro fetch plan]
    B --> C[Local Kiro CLI strategy]
    C --> D{kiro-cli discoverable?}
    D -->|No| E[Unavailable strategy error]
    D -->|Yes| F[Kiro usage probe]
    F --> G[Usage snapshot or probe error]
    E --> H[CodexBar error display]
    G --> H
Loading

Before merge

  • Add real behavior proof - Needs real behavior proof before merge: The PR body reports focused tests and make check, but it contains no redacted after-fix terminal output, runtime log, or app capture from a real Kiro setup without kiro-cli; add that evidence and update the PR body to trigger re-review.
  • Resolve merge risk (P1) - The hard-coded Kiro documentation URL and the actual app-visible message have not been demonstrated from a real missing-CLI environment after the change.
  • Complete next step (P2) - Human-only merge blocker: request redacted real after-fix proof from the contributor; there is no concrete repair-lane change for automation to make.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Patch scope 2 files affected; 16 added, 1 removed The change is limited to one shared error message and one focused regression test.

Merge-risk options

Maintainer options:

  1. Decide the mitigation before merge
    Keep the provider-specific fallback while adding a redacted live capture that shows a Kiro refresh without kiro-cli now presents the install-and-login guidance.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Technical review

Best possible solution:

Keep the provider-specific fallback while adding a redacted live capture that shows a Kiro refresh without kiro-cli now presents the install-and-login guidance.

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

Yes, from source: Kiro registers only a CLI strategy, whose availability check requires kiro-cli; when it is absent, the shared generic unavailable-strategy message is the remaining error path. No live provider probe was run because repository policy requires explicit permission for that account-facing validation.

Is this the best way to solve the issue?

Yes. A Kiro-only fallback message is the narrowest repair because it preserves every other provider’s existing generic error and aligns the unavailable-strategy path with Kiro’s existing CLI and login diagnostics.

AGENTS.md: found and applied where relevant.

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

Labels

Label justifications:

  • P3: This is a low-blast-radius diagnostic improvement for an optional provider’s missing local CLI prerequisite.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body reports focused tests and make check, but it contains no redacted after-fix terminal output, runtime log, or app capture from a real Kiro setup without kiro-cli; add that evidence and update the PR body to trigger re-review.

Evidence

What I checked:

Likely related people:

  • Peter Steinberger: Current main attributes the shared fetch-error implementation to this release state, and recent Kiro maintenance history also includes multiple follow-up commits. (role: recent shared-provider-path contributor; confidence: high; commits: b1170d83963d, 19a1be9ed99e; files: Sources/CodexBarCore/Providers/ProviderFetchPlan.swift, Sources/CodexBarCore/Providers/Kiro/KiroStatusProbe.swift)
  • Nathan Eror: Introduced the original Kiro CLI usage-tracking provider, making this person a useful historical routing candidate for its strategy boundary. (role: Kiro provider introducer; confidence: medium; commits: bbda93528714; files: Sources/CodexBarCore/Providers/Kiro/KiroProviderDescriptor.swift, Sources/CodexBarCore/Providers/Kiro/KiroStatusProbe.swift)

Rank-up moves

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

  • Add a redacted terminal capture, runtime log, or app screenshot showing a Kiro refresh without kiro-cli now displays the new guidance.
  • Keep private account details, paths, tokens, and non-public endpoints out of the capture; updating the PR body should trigger re-review, or a maintainer can comment @clawsweeper re-review.

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.

History

Review history (3 earlier review cycles)
  • reviewed 2026-07-27T01:48:14.241Z sha a96e849 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-31T23:01:08.275Z sha a96e849 :: needs real behavior proof before merge. :: none
  • reviewed 2026-08-02T03:05:20.154Z sha cebb14d :: needs real behavior proof before merge. :: none

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Jul 31, 2026
@steipete
steipete merged commit 41bc014 into steipete:main Aug 2, 2026
9 checks passed
steipete pushed a commit that referenced this pull request Aug 2, 2026
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. 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.

2 participants