Skip to content

feat(islo): implement Crabbox run session handles#181

Merged
steipete merged 6 commits into
openclaw:mainfrom
zozo123:spike/harbor-integration
May 29, 2026
Merged

feat(islo): implement Crabbox run session handles#181
steipete merged 6 commits into
openclaw:mainfrom
zozo123:spike/harbor-integration

Conversation

@zozo123
Copy link
Copy Markdown
Contributor

@zozo123 zozo123 commented May 28, 2026

Summary

  • Implement the existing Crabbox RunSessionHandle / FeatureRunSession contract for the Islo delegated provider.
  • Populate RunResult.Session so crabbox run --provider islo --keep --lease-output session.json -- ... gives orchestrators a stable lease handle.
  • Add a regression test for kept Islo session handles and cleanup command metadata.

Dependencies

Provider model

  • This PR does not make Harbor Islo-only. It makes Islo the first delegated provider, after Blacksmith, to implement the generic Crabbox session-handle contract.
  • Other Crabbox providers can adopt the same FeatureRunSession + RunResult.Session shape in follow-up PRs.

Test plan

  • go test ./internal/providers/islo

@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented May 28, 2026

Codex review: needs real behavior proof before merge. Reviewed May 28, 2026, 9:04 PM ET / 01:04 UTC.

Summary
The branch adds Islo FeatureRunSession metadata, returns an Islo RunSessionHandle with lease, slug, keep/reuse, and cleanup metadata, and adds Islo backend regression tests.

Reproducibility: not applicable. as a feature PR rather than a bug report. Source inspection shows current main rejects --lease-output for Islo because the provider does not advertise FeatureRunSession, while the branch adds that provider capability.

Review metrics: none identified.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🦞 diamond lobster
Result: blocked until real behavior proof is added.

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

Rank-up moves:

  • Post redacted terminal output or logs from crabbox run --provider islo --keep --lease-output session.json -- ... showing the generated JSON and cleanup command result.
  • Redact private details such as API keys, IP addresses, non-public endpoints, and other secrets before posting proof; updating the PR body should trigger a fresh ClawSweeper review, or a maintainer can comment @clawsweeper re-review.

Proof guidance:

  • [P1] Needs real behavior proof before merge: No redacted terminal output, logs, recording, or linked artifact shows a real kept Islo run producing lease-output JSON and a cleanup result after the latest head update. 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] No after-fix live Islo proof has been posted for crabbox run --provider islo --keep --lease-output session.json -- ..., so maintainers cannot verify the advertised JSON handle and cleanup command against the real provider path rather than the fake backend test.

Maintainer options:

  1. Require live Islo session proof (recommended)
    Ask for redacted terminal output, logs, or a linked artifact from a real kept Islo run showing the generated session JSON and cleanup command result before merge.
  2. Accept the live-provider uncertainty
    Maintainers can intentionally merge based on source review and fake-backend coverage if they are willing to own any live Islo mismatch discovered after merge.
  3. Pause in favor of the broader tracker
    If Islo is not the right first provider to validate this contract, pause or close this PR and continue the decision in Track run-session support across Crabbox providers #182.

Next step before merge

  • [P1] No narrow code repair lane was found; the contributor needs to provide real Islo runtime proof before maintainer merge consideration.

Security
Cleared: No concrete security or supply-chain concern was found; the diff stays in Go provider code/tests, adds no dependencies or workflows, and quotes the generated cleanup command.

Review details

Best possible solution:

Land the narrow Islo run-session support after redacted live Islo proof confirms kept lease-output JSON and cleanup behavior; keep #182 as the broader provider adoption tracker.

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

Not applicable as a feature PR rather than a bug report. Source inspection shows current main rejects --lease-output for Islo because the provider does not advertise FeatureRunSession, while the branch adds that provider capability.

Is this the best way to solve the issue?

Yes, the implementation path appears to be the narrow maintainable path because it uses the existing provider-neutral contract and keeps Islo-specific lifecycle logic inside the Islo adapter. The remaining blocker is real runtime proof, not a different code design found in review.

AGENTS.md: found and applied where relevant.

Codex review notes: model gpt-5.5, reasoning high; reviewed against a7c1a7af29fb.

Label changes

Label changes:

  • add merge-risk: 🚨 other: The branch exposes a new live provider session-handle path that fake-backend tests and ordinary CI cannot prove against Islo itself.

Label justifications:

  • P2: This is a normal provider capability improvement limited to Islo and external orchestrator workflows.
  • merge-risk: 🚨 other: The branch exposes a new live provider session-handle path that fake-backend tests and ordinary CI cannot prove against Islo itself.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦞 diamond lobster.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: No redacted terminal output, logs, recording, or linked artifact shows a real kept Islo run producing lease-output JSON and a cleanup result after the latest head update. 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:

  • Repository policy read: AGENTS.md was read fully; the review applied its provider-neutral and provider-adapter boundary guidance to this Islo-specific change. (AGENTS.md:1, a7c1a7af29fb)
  • Current main lacks Islo run-session support: On current main, the Islo provider advertises FeatureURLBridge only, so --lease-output remains gated off for provider=islo. (internal/providers/islo/provider.go:25, a7c1a7af29fb)
  • CLI gate depends on provider feature: crabbox run rejects --lease-output unless the selected provider advertises FeatureRunSession, making the provider feature addition necessary for this capability. (internal/cli/run.go:379, a7c1a7af29fb)
  • Existing generic contract: FeatureRunSession, RunResult.Session, and RunSessionHandle already exist in core, so the PR uses an existing extension point rather than adding a new core API. (internal/cli/provider_backend.go:171, a7c1a7af29fb)
  • Reference implementation exists: Blacksmith already fills RunSessionHandle with provider, lease ID, slug, keep/reuse state, and cleanup metadata; the Islo branch follows that established pattern. (internal/providers/blacksmith/backend.go:204, 7bd2c5099d9c)
  • PR branch implementation: The PR head constructs RunResult.Session after acquiring or resolving the Islo lease and preserves session metadata even when sync or workspace preparation returns an error. (internal/providers/islo/backend.go:186, b3e1e01f16b8)

Likely related people:

  • steipete: Blame and history show Peter Steinberger on the current Islo run path, provider metadata, and the existing Blacksmith run-session implementation used as the reference shape. (role: feature owner / recent area contributor; confidence: high; commits: 7bd2c5099d9c, 983f7f617af2, fd90dd91605d; files: internal/providers/islo/backend.go, internal/providers/islo/provider.go, internal/providers/blacksmith/backend.go)
  • zozo123: Current main history shows Yossi Eliaz touching Islo provider features and adding adjacent delegated-provider work, making them relevant for provider lifecycle review. (role: adjacent delegated-provider contributor; confidence: medium; commits: ed692508089d, 48939ed52ee3; files: internal/providers/islo/provider.go, internal/providers/azuredynamicsessions, docs/providers/islo.md)
  • RomneyDa: GitHub PR metadata for the merged delegated run-session output work identifies RomneyDa as the author of the core RunSessionHandle / FeatureRunSession contract this PR extends. (role: generic contract introducer; confidence: medium; commits: 76829952a9d8; files: internal/cli/provider_backend.go, internal/cli/run.go, internal/providers/blacksmith/backend.go)
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.

@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. P2 Normal priority bug or improvement with limited blast radius. labels May 28, 2026
@zozo123 zozo123 changed the title feat(islo): emit run session handles feat(islo): implement Crabbox run session handles May 28, 2026
@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 May 28, 2026
@zozo123
Copy link
Copy Markdown
Contributor Author

zozo123 commented May 28, 2026

Cross-repo stack note:

This PR implements the generic Crabbox RunSessionHandle / FeatureRunSession contract for Islo. The goal is not to make Harbor Islo-only; Islo is the first delegated-provider validation case for a broader Harbor + Crabbox integration.

Related Harbor PRs:

Future provider work can adopt the same FeatureRunSession + RunResult.Session shape for other Crabbox providers.

@zozo123
Copy link
Copy Markdown
Contributor Author

zozo123 commented May 28, 2026

Tracking issue for adopting the generic run-session contract across Crabbox providers: #182

@steipete steipete force-pushed the spike/harbor-integration branch from 7c5f03e to b3e1e01 Compare May 29, 2026 01:00
@clawsweeper clawsweeper Bot added the merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. label May 29, 2026
@steipete steipete merged commit 73c7b11 into openclaw:main May 29, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. 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.

2 participants