feat(winds): add minimal workspace execution CLI - #37
Conversation
|
Warning Review limit reached
Next review available in: 4 minutes Limit details: You’ve used all 3 included reviews currently available under your plan. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
TheHalfMoon
left a comment
TheHalfMoon
left a comment
There was a problem hiding this comment.
Exact-head correctness / safety / authority review — PASS for Spec 003 / T057.
Reviewed exact head 24aa673e68e87ffef1d7ab62570b14ef70c95ddb against canonical base ca4ed9ac623773a02abac9337c2463a102dbec1c and the active T057 boundary.
Findings:
- The diff is limited to the minimal CLI router, one focused CLI implementation module, and end-to-end CLI tests. No dependency, migration, workflow, protocol, daemon, renderer, Fleet, MCP/ACP/A2A, or verification-authority change is introduced.
- Existing
verify,promote, andrecoverimplementations are not behaviorally modified. - Workspace open/clone/profile operations delegate to the already accepted backend identity and validation paths rather than duplicating Git truth.
- Clone state-root/destination overlap is rejected before state-root creation, preventing the CLI preflight from creating the requested clone destination as a side effect of an invalid configuration.
runuses direct executable + argv semantics and the accepted T054/T055 lifecycle; command history can be disabled and transcript persistence is not enabled by this CLI slice.terminal-proofselects only a freshly rediscovered native profile, reuses the accepted owned PTY lifecycle, and terminates only the directly owned proof session. It does not claim persistent attachment, descendant ownership, or renderer/multiplexer behavior.executionbinds the persisted execution's workspace to the exact--repocanonical root before exposing metadata from a shared Winds state root.- Stored authority/source fields are surfaced rather than synthesized. WSL information is explicitly discovery-only and does not claim T062 integration support.
- The binary-level regression test proves stable workspace identity, explicit command lifecycle, durable execution inspection, cross-workspace rejection, native terminal lifecycle proof, clone overlap refusal-before-creation, and a safe successful clone.
- Exact-head quality #354, windows-terminal #129, release-candidate #198, SC-001 soak, and Linux/macOS release bundles all pass.
No actionable correctness, process-ownership, privacy, state-root, or verification-authority finding remains from this review. T058+ remains out of scope.
TheHalfMoon
left a comment
TheHalfMoon
left a comment
There was a problem hiding this comment.
Exact-head Ponytail v4.9.0 simplicity review — PASS for Spec 003 / T057.
Challenge applied to every new shape on exact head 24aa673e68e87ffef1d7ab62570b14ef70c95ddb:
- No new dependency is justified or added; the existing hand-rolled flat
--key valueparser remains sufficient for this proof slice. - No generic CLI framework, provider abstraction, plugin layer, terminal renderer, multiplexer, daemon/socket/API, remote runtime, or Agent Fleet machinery is introduced.
- Six flat proof commands map directly to six T057 needs; there is no speculative nested command taxonomy.
- New behavior is isolated in one concrete
cli_workspacemodule;main.rsonly wires routing/usage. - Backend validation, lifecycle, persistence, profile discovery, clone logic, and history policy are reused instead of reimplemented.
- The immediate
terminal-prooflifecycle is deliberately smaller than an interactive terminal UI and avoids inventing persistent attachment/session infrastructure before a later specification authorizes it. - WSL is reported only as discovery information; no new WSL profile/launcher framework is introduced merely for CLI symmetry.
- Manual execution JSON construction is local and concrete because persisted typed records are not a public serialization/API contract; adding a generic DTO/protocol layer would be premature.
- End-to-end tests are intentionally binary-facing and exercise the public CLI proof rather than creating a separate test harness abstraction.
No removable dependency/module/protocol/abstraction or speculative future-facing machinery was found. The implementation is small relative to the backend it exposes and remains inside T057.
TheHalfMoon
left a comment
TheHalfMoon
left a comment
There was a problem hiding this comment.
Independent-review gate handoff: deterministic exact-head gates and both author review checkpoints are complete on 24aa673e68e87ffef1d7ab62570b14ef70c95ddb. PR remains Draft. Independent review must evaluate this exact head; older-head summaries are not acceptance evidence.
|
@coderabbitai review Please perform the independent acceptance review on exact head |
|
🧠 Learnings used
|
|
/review Exact-head independent acceptance scope: review |
PR Summary by Qodofeat(winds): minimal workspace & execution CLI surface for T057
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
Code Review by Qodo
1. Local .git remote bypass
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
TheHalfMoon
left a comment
TheHalfMoon
left a comment
There was a problem hiding this comment.
Final exact-head correctness / safety / authority review — PASS for Spec 003 / T057.
Reviewed exact head 680d8e8894a0c6bfd1ef62a5bd373ef1f0a48d0e against canonical base ca4ed9ac623773a02abac9337c2463a102dbec1c, including the Qodo/Cubic remediation delta.
Final findings:
- Scope remains exactly three files: the flat CLI router, one concrete T057 CLI module, and binary-facing integration tests. No dependency, migration, workflow, protocol, daemon/server/socket, renderer, remote runtime, plugin/provider, MCP/ACP/A2A, Agent Fleet, Herdr transplant, or verification-authority change was introduced.
- Existing
verify,promote, andrecoverbehavior remains unchanged. - Existing-workspace operations retain the canonical
Repo::require_external_state_pathboundary through the existingwinds_homepath. - Clone preflight now fails before creating state when the planned state root overlaps the destination or is inside an absolute local clone source.
file://is deliberately rejected by this minimal CLI because proving its filesystem boundary would otherwise require new URL/path parsing machinery; network-style remotes remain unaffected. runpreserves direct executable + argv runtime semantics while respecting command-history disable; transcript persistence is not enabled by the CLI.terminal-proofre-discovers and selects an exact native profile, reuses the accepted directly-owned PTY lifecycle, and terminates only that owned proof session; it does not imply persistent attachment or descendant ownership.executionverifies that persisted execution workspace identity matches the exact canonical--reporoot before exposing metadata from a shared state root.- Stored status/source/domain facts are surfaced rather than synthesized.
- The integration tests now fail/skip safely when the host temp root cannot satisfy the CLI UTF-8 prerequisite and use an ownership-checked RAII temp guard so failed assertions do not strand test trees.
- The tests cover stable workspace identity, profiles, explicit command lifecycle/history-disable truth, execution inspection, cross-workspace rejection, native PTY lifecycle proof, local-source state-root rejection before mutation, destination overlap rejection before mutation, and successful safe clone.
- Exact-head quality #358, windows-terminal #133, release-candidate #202, SC-001 100-cycle soak, and Linux/macOS release bundles all pass.
No actionable correctness, process-ownership, state-root, privacy, deterministic-output, or verification-authority finding remains. T058+ remains out of scope.
TheHalfMoon
left a comment
TheHalfMoon
left a comment
There was a problem hiding this comment.
Final exact-head Ponytail v4.9.0 simplicity review — PASS for Spec 003 / T057.
Challenge applied to exact head 680d8e8894a0c6bfd1ef62a5bd373ef1f0a48d0e after independent-review remediation:
- No new dependency or CLI framework was added; the existing flat
--key valueparser remains adequate for the six proof commands. - No generic provider/protocol/DTO/plugin/multiplexer/renderer/daemon/remote/Fleet abstraction was introduced.
- The valid local-clone state-root safety gap is fixed with one concrete pre-clone boundary check rather than a generic clone/provider layer.
- The Qodo reliability issue is fixed with a tiny test-only ownership guard, not a production tempfile abstraction or dependency.
- Cubic's remaining P3 duplication suggestion is intentionally not implemented. Existing-workspace home resolution and pre-registration clone home resolution now have materially different mutation/safety contracts; extracting a callback/policy abstraction for only these two cases would broaden
main.rsand obscure the unique pre-clone no-mutation boundary. A third authorized state-root policy would be a concrete future trigger to extract a shared primitive. - The CLI module remains concrete and local; backend validation, lifecycle, persistence, profile discovery, clone logic, and history policy are reused.
- Binary-facing tests exercise the actual public proof surface and remain limited to the T057 contract.
No removable dependency/module/protocol or justified simplification remains. The final implementation stays smaller and easier to audit than the generic abstraction proposed by the remaining P3 maintainability comment.
|
/review Fresh exact-head acceptance review requested for Spec 003 / T057. Review exact head The prior Qodo non-UTF8 test reliability finding and Cubic local-clone state-root / panic-cleanup findings were remediated. The remaining Cubic duplication suggestion was reconciled under Ponytail as non-actionable because existing-workspace and pre-registration clone state-root resolution have materially different safety/mutation contracts. Exact-head deterministic gates are green: quality #358, windows-terminal #133, release-candidate #202, SC-001 soak, Linux x86-64 bundle, macOS arm64 bundle. Final exact-head author correctness/safety/authority and Ponytail reviews are PASS; review threads are resolved. Focus on correctness, safety, workspace/process ownership, privacy, deterministic JSON truth, active-spec compliance, and unnecessary complexity. Do not infer T058+, daemon/server/socket/public protocol, terminal renderer, persistent detached terminals, remote runtime, plugin/MCP/ACP/A2A, Agent Fleet, or Herdr scope. |
| if let Some((scheme, _)) = remote.split_once("://") { | ||
| if scheme.eq_ignore_ascii_case("file") { | ||
| return Err( | ||
| "T057 workspace-clone requires an absolute local path instead of file:// so Winds can prove the clone-source/state-root boundary" |
There was a problem hiding this comment.
1. workspace-clone forbids file:// 📘 Rule violation ⚙ Maintainability
The new workspace-clone path rejects file:// remotes, which is a public CLI behavior not described in the active Spec 003 documents for cloning a Git remote. This can cause spec/implementation drift and unexpected CLI failures for local remotes.
Agent Prompt
## Issue description
`workspace-clone` rejects `file://` remotes, but Spec 003 only states “Given a Git remote… clone…”, without specifying a `file://` prohibition. This introduces a new public error case not mapped to the active spec.
## Issue Context
The code currently enforces this restriction to “prove the clone-source/state-root boundary”, but that requirement is not explicitly captured in Spec 003 acceptance scenarios/requirements.
## Fix Focus Areas
- src/cli_workspace.rs[345-352]
- specs/003-workspace-execution-spine/spec.md[25-38]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| if let Ok(repo) = Repo::open(&canonical_remote) { | ||
| repo.require_external_state_path(state_root)?; | ||
| } |
There was a problem hiding this comment.
2. Local .git remote bypass 🐞 Bug ⛨ Security
require_clone_state_external_to_local_remote only enforces Repo::require_external_state_path when Repo::open(remote) succeeds; for local remotes that are Git dirs (e.g., /path/worktree/.git), Repo::open can fail and the stronger “outside checkout and git dir” guard is skipped. This can allow --home to be placed inside the source checkout even though other flows explicitly forbid writing Winds state under a repo’s root/common-dir.
Agent Prompt
## Issue description
`require_clone_state_external_to_local_remote` conditionally calls `Repo::open(&canonical_remote)` and silently skips `repo.require_external_state_path(state_root)` when the remote path is not a worktree root (e.g., a worktree’s `.git` directory). That leaves only the weaker `state_root.starts_with(canonical_remote)` check, which does not prevent state roots inside the enclosing checkout.
## Issue Context
The canonical invariant is enforced by `Repo::require_external_state_path` (state root must not be under repo root or git common dir). The clone CLI should preserve that invariant for local remotes, including when the local remote path is a Git directory rather than the worktree root.
## Fix Focus Areas
- src/cli_workspace.rs[344-369]
- src/git.rs[96-118]
## Suggested approach
- Keep the current `state_root.starts_with(canonical_remote)` guard.
- If `Repo::open(&canonical_remote)` fails and `canonical_remote.file_name() == Some(OsStr::new(".git"))`, additionally treat `canonical_remote.parent()` as the candidate worktree root and enforce `Repo::open(parent)?.require_external_state_path(state_root)`.
- Alternatively, add a helper that can derive the worktree root/common dir from either a worktree path or a git-dir path and always apply `require_external_state_path` when the remote is local.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| let Some(temp) = TestTempDir::new("winds-t057-cli") else { | ||
| return; | ||
| }; |
There was a problem hiding this comment.
3. Temp dir failures skip tests 🐞 Bug ☼ Reliability
The integration tests early-return when TestTempDir::new(...) returns None, so temp-dir creation/canonicalization failures (including rare name collisions) can silently skip these tests and still pass CI. This reduces test coverage and can mask regressions in the CLI surface the tests are meant to prove.
Agent Prompt
## Issue description
`t057_cli` tests can silently skip by returning early when temp dir setup fails (including rare `AlreadyExists` collisions), causing CI to pass without executing assertions.
## Issue Context
`TestTempDir::new` returns `Option<Self>` and callers do `else { return; }`. The helper also does not retry on `fs::create_dir` name collisions.
## Fix Focus Areas
- tests/t057_cli.rs[10-13]
- tests/t057_cli.rs[138-142]
- tests/t057_cli.rs[262-285]
## Suggested approach
- Change `TestTempDir::new` to return `Result<Self>` (or keep `Option` but `expect` in callers) so setup failures fail the test with diagnostics.
- Add a small retry loop in `TestTempDir::new` that retries on `ErrorKind::AlreadyExists` (e.g., append a per-process counter) rather than returning `None`.
- Keep the existing safety checks in `Drop`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 680d8e8 |
1 participant
Spec 003 / T057
Implements only the minimal user/CLI proof surface required by T057 on top of canonical main
ca4ed9ac623773a02abac9337c2463a102dbec1c.Flat CLI surface
winds workspace-open— inspect/register an existing Git workspace and emit deterministic JSONwinds workspace-clone— clone/register through the accepted T046 backend and emit sanitized persisted remote identitywinds profiles— emit workspace inventory plus concrete native shell profiles; WSL discovery is reported as AVAILABLE / UNAVAILABLE / UNSUPPORTED_ON_HOST without claiming T062 supportwinds run— execute one explicit absolute executable through the accepted T054/T055 ledger path with JSON argv and command-history enable/disablewinds terminal-proof— launch an exact selected native shell profile through the accepted PTY lifecycle and immediately terminate the directly owned proof session, proving launch/lifecycle persistence without adding a renderer or persistent multiplexerwinds execution— inspect one execution, its typed terminal/shell-command record, and ordered events as deterministic JSONShape
The existing hand-rolled top-level
--key valueparser is retained. No clap/subcommand framework or other dependency is added. New T057 behavior is isolated insrc/cli_workspace.rs;src/main.rschanges only route the six flat proof commands and extend usage text.Safety / authority
verify,promote, andrecoverimplementations are unchangedWINDS_HOMEboundaryfile://is rejected in this minimal CLI because proving the filesystem boundary would otherwise require new URL/path parsing machinerycommand(best-effort redacted persisted argv) ordisabled; runtime argv are unchanged--repocanonical root before returning metadataEnd-to-end proof
tests/t057_cli.rsexercises the built binary across:workspace-open -> stable reopen identity -> profiles -> run -> execution -> cross-workspace rejection -> terminal-proofand separately proves local-source state-root rejection before mutation, destination/state-root overlap rejection before mutation, and a successful safe clone. Test fixtures fail/skip safely when the host temporary root cannot satisfy the CLI UTF-8 prerequisite and use an ownership-checked RAII cleanup guard.
Exact-head deterministic evidence
Exact implementation head:
680d8e8894a0c6bfd1ef62a5bd373ef1f0a48d0eCanonical base/main:
ca4ed9ac623773a02abac9337c2463a102dbec1cwindows-latestca4ed9ac... -> 680d8e88...and reports Bugs (0) / Rule violations (0); prior reliability finding is resolvedExplicit boundaries
No T058+ work. No daemon/server/socket, public IPC/runtime protocol, terminal renderer, persistent detached terminal, remote runtime, plugin/provider framework, MCP/ACP/A2A, Agent Fleet, Herdr transplant, schema migration, dependency change, or verification-authority change.
T057 exact-head acceptance gates are satisfied. Merge is permitted only with unchanged-main and expected-head guards. Do not start T058 in this PR.