diff --git a/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/baseline/branch.json b/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/baseline/branch.json new file mode 100644 index 0000000..c58da8e --- /dev/null +++ b/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/baseline/branch.json @@ -0,0 +1 @@ +{"data":"alanvardy-var-981-refactor-orksorksorks"} diff --git a/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/baseline/help.txt b/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/baseline/help.txt new file mode 100644 index 0000000..b6a737b --- /dev/null +++ b/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/baseline/help.txt @@ -0,0 +1,18 @@ + +Usage: orksorksorks [OPTIONS] + +Commands: + init (i) Create a new orksorksorks.toml file; refuses if it already exists + branch Print the current git branch + artifact_directory Print the artifact directory path (cwd/.pi/orksorksorks//) + step Determine the current step from present trigger artifacts + model Print the model for the current step (from `config.models`) + thinking Print the thinking budget for the current step (from `config.models`) + prompt Print the prompt for the current step (from `config.prompts`) + script Print the raw script for the current step (from `config.scripts`) + help Print this message or the help of the given subcommand(s) + +Options: + -j, --json Output results as JSON + -h, --help Print help + -V, --version Print version diff --git a/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/baseline/nextest.txt b/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/baseline/nextest.txt new file mode 100644 index 0000000..c0a7af9 --- /dev/null +++ b/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/baseline/nextest.txt @@ -0,0 +1,5 @@ + PASS [ 0.008s] (195/197) orksorksorks::bin/orksorksorks git::tests::parse_branch_output_trims_trailing_newline + PASS [ 0.082s] (196/197) orksorksorks::bin/orksorksorks git::tests::current_branch_in_returns_configured_branch + PASS [ 0.095s] (197/197) orksorksorks::bin/orksorksorks git::tests::current_branch_in_detached_head_errors +──────────── + Summary [ 0.752s] 197 tests run: 197 passed, 0 skipped diff --git a/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/baseline/script-help.txt b/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/baseline/script-help.txt new file mode 100644 index 0000000..82ce79f --- /dev/null +++ b/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/baseline/script-help.txt @@ -0,0 +1,11 @@ +Print the raw script for the current step (from `config.scripts`) + +Usage: orksorksorks script [OPTIONS] [STEP_NAME] + +Arguments: + [STEP_NAME] Step name override; defaults to the current step derived from present trigger artifacts + +Options: + --config Path to the TOML config file; defaults to the config directory (the same resolution as `init`) + -j, --json Output results as JSON + -h, --help Print help diff --git a/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/baseline/step-help.txt b/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/baseline/step-help.txt new file mode 100644 index 0000000..d163010 --- /dev/null +++ b/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/baseline/step-help.txt @@ -0,0 +1,9 @@ +Determine the current step from present trigger artifacts + +Usage: orksorksorks step [OPTIONS] + +Options: + --config Path to the TOML config file; defaults to the config directory (the same resolution as `init`) + -j, --json Output results as JSON + --step Override the current step (from `config.steps`) by name + -h, --help Print help diff --git a/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/baseline/step-missing.json b/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/baseline/step-missing.json new file mode 100644 index 0000000..5d8f3b9 --- /dev/null +++ b/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/baseline/step-missing.json @@ -0,0 +1 @@ +{"error":{"message":"could not read config file at /nonexistent/orks.toml (specified via --config): No such file or directory (os error 2)","source":"io"}} diff --git a/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/baseline/step-missing.txt b/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/baseline/step-missing.txt new file mode 100644 index 0000000..6418c4e --- /dev/null +++ b/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/baseline/step-missing.txt @@ -0,0 +1,4 @@ + + +Error from io: +could not read config file at /nonexistent/orks.toml (specified via --config): No such file or directory (os error 2) diff --git a/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/conventions.md b/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/conventions.md new file mode 100644 index 0000000..fbe807f --- /dev/null +++ b/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/conventions.md @@ -0,0 +1,49 @@ +# Conventions — orksorksorks + +Shared factual appendix: commands, test inventory, and gotchas. Research/design/structure/plan phases rely on this instead of re-reading the tree. + +## Canonical commands + +Local gate (authoritative, run before committing): `scripts/test.sh` +1. `cargo fmt --all` (scripts/test.sh:9) +2. `cargo check` (scripts/test.sh:12) +3. `cargo clippy --tests -- -D warnings` (scripts/test.sh:15) +4. `cargo nextest run --no-tests pass` (scripts/test.sh:18) — note **nextest**, not libtest +5. Forbidden-strings gate: `rg -i -g '*.rs' 'TODO:|todo:|FIXME|fixme|dbg!|DEBUG:|FIXTURE:'` must match nothing (scripts/test.sh:22-25) + +CI (`.github/workflows/ci-pr.yml` → `_reusable-lint.yml` + `_reusable-test.yml`, ubuntu-latest, stable toolchain): +- lint: `cargo check --locked --all-features` (_reusable-lint.yml:17); `cargo fmt --all -- --check` (:19); `cargo clippy --all-targets --all-features --locked -- -D warnings` (:21); same forbidden-strings gate (:23-25) +- test: `cargo nextest run --profile ci --all-features --no-tests pass` (_reusable-test.yml:25); coverage `cargo llvm-cov nextest --profile ci --all-features --lcov` (:28) → codecov (:31); junit via test-results-action (:36-38) + +Notes: +- Local clippy is `--tests`; CI is `--all-targets --all-features --locked`. `nextest` is required locally (not cargo test). +- `build.rs` emits `cargo:rustc-env` for BUILD_TARGET/PROFILE/TIMESTAMP (build.rs:5-11) using `chrono` (Cargo.toml:31); `#[tokio::main]` on main (src/main.rs:82). +- `#![warn(missing_docs)]` (src/main.rs:6) — clippy `-D warnings` makes missing `///` docs on `pub`/`pub(crate)` items a gate failure (convention: every pub item documented, see src/config_dir.rs:11-17, :24-26, :67-72). + +## Test-suite inventory + +Integration tests (10 files, `tests/`): every file spawns the compiled binary via `assert_cmd::Command::cargo_bin("orksorksorks")` (tests/init_creates_file.rs:17) with tempdirs + injected `XDG_CONFIG_HOME`; asserts exit status, raw stdout/stderr bytes, JSON envelopes. No `#[cfg(unix/windows)]` gating anywhere in tests/ (grep-verified). The crate is binary-only, so integration tests cannot import crate types (tests/init_creates_file.rs:22-24). + +| File | Covers | +|---|---| +| `tests/architecture.rs` | rust_arkitect: commands import allowlist (:51-78) + module-order cycle ban (:89-140) | +| `tests/init_creates_file.rs` | init: template byte-equality, success message, readonly failure, `io`/`config-exists`/`config-dir` JSON sources, `--config` override, no-overwrite | +| `tests/json_output.rs` | JSON envelope `data` field, no-ANSI plain text | +| `tests/branch.rs` | branch name text + `-j`, outside-repo failure | +| `tests/artifact_directory.rs` | composed path w/ trailing slash + `/`→`-`, canonicalization, `-j`, outside-repo failure | +| `tests/config_validation.rs` | 8 exact (stderr phrase / `error.source`) pairs: config:version, config:duplicate-name, config:empty-name, config:empty-model, config:duplicate-trigger, config:multiple-default, config:missing-prompt, config:missing-model | +| `tests/step.rs` | artifact-derived step, default fallback, XDG, cwd-config-ignored guard, `--step` override (git-less/detached HEAD), missing-path stderr spelling, unknown-step text vs JSON | +| `tests/model.rs` (+thinking) | exact model strings, `-j` data, XDG, `--step` override | +| `tests/prompt.rs` | frontmatter exact text, `show_frontmatter=false`, positional rejected (exit 2) | +| `tests/script.rs` | never emits frontmatter, positional override, `script` error tags, XDG | + +Unit tests: `#[cfg(test)] mod tests` at end of every module — src/commands/mod.rs:437 (≈1,000 lines: parse routes, routing, handler logic, helper resolution, error tag/message pairs), src/config.rs:254, src/config_dir.rs:116, src/errors.rs:73, src/format.rs:26, src/git.rs:39. Convention: `use super::*;`, pretty_assertions where useful, behavior-sentence test names. + +## Gotchas + +- **Relative-path include**: `include_str!("../../templates/default.toml")` (src/commands/mod.rs:181) and its integration-test twin `tests/init_creates_file.rs:25,107` resolve relative to the source file; path depth changes if the call moves into deeper submodule files. +- **ANSI colors**: `src/format.rs` `apply_color` strips under `cfg!(test)` (format.rs:6-12) — affects in-process unit tests only. For the real binary, `colored` 3.1.1 strips color on non-tty output (empirically verified: piped stdout has no `\x1b`; all "no ANSI" integration assertions rely on this). Color appears only on interactive terminals. `Error`'s `Display` owns all coloring; callers must not pre-apply ANSI (src/errors.rs:5-8). +- **rust_arkitect dual spellings**: dependencies are recorded "exactly as written" (tests/architecture.rs:21-25), so both `crate::x` and `orksorksorks::x` spellings are allowlisted (10 entries, :56-75) — any new `use crate::...` path in moved code must be a sibling-module spelling already covered. Submodule files under `src/commands/` keep the `orksorksorks::commands` identity — no new module-level rules needed. +- **stdout/stderr split**: text-mode errors → stderr with two leading blank lines (`\n\n{e}`, src/main.rs:32-33); JSON-mode errors → stdout as `{"error":{"message","source"}}` (src/main.rs:46-47); both exit 1 (src/main.rs:76-77). Handlers never print — they return `Result`. +- **Integration tests spawn real processes**: tempdirs are restored (readonly test explicitly `set_readonly(false)` for cleanup — tests/init_creates_file.rs:1-3); macOS tempdir canonicalization `/var`→`/private/var` needed (tests/artifact_directory.rs:40-42). +- **`--no-tests pass`**: nextest invocation tolerates empty test sets; `--profile ci` + `--all-features` only in CI. \ No newline at end of file diff --git a/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/design.md b/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/design.md new file mode 100644 index 0000000..a9e8ae1 --- /dev/null +++ b/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/design.md @@ -0,0 +1,164 @@ +# Design Discussion + +## Current State + +`orksorksorks` is a binary-only Rust CLI (`edition 2024`) whose `src/main.rs:8-13` declares six +modules: `commands` (the only directory module), `config`, `config_dir`, `errors`, `format`, `git` +(all flat single files). `src/commands/mod.rs` is 1,459 lines — half the crate's 2,884 — and mixes +four concerns with clean internal seams (research Q1): + +- **Parser** (`:1-117`): `const NAME/AUTHOR/ABOUT/LONG_VERSION` (`:7-18`); `pub struct Cli` + (`:21-38`, `#[derive(Parser, Clone)]`, `--json` global flag, `#[command(subcommand)]`); + `pub enum Commands` (`:40-117`) with eight variants (`Init`, `Branch`, `ArtifactDirectory`, + `Step`, `Model`, `Thinking`, `Prompt`, `Script`), registered purely through clap-derive + attributes — no manual registry. +- **Router** (`:119-157`): private `select_command_with_env(cli, &ConfigEnv)` resolves the config + path per variant via `config_dir::config_file_path_with_env` and `?`-propagates, then dispatches + to each handler; public `select_command(&Cli)` delegates with `ConfigEnv::from_env()`. +- **Handlers** (`:161-434`): eight private `fn -> Result` — `init_command`, + `branch_command`, `artifact_directory_command`, `step_command`, `model_command`, + `thinking_command`, `prompt_command`, `script_command`. Handlers never print; they return data. +- **Helpers** (`:195-359`): private `artifact_dir_path`, `determine_step`, `resolve_model`, + `resolve_step`, `resolve_prompt`, `resolve_script` — a mutually-recursive resolution layer sharing + the `Error::new(tag, message)` idiom. +- **Tests** (`:437-1460`, ~1,000 lines): a single flat `#[cfg(test)] mod tests` with `use super::*`, + covering parse routes, routing arms, handler strings, helper results, and exact + `Error` tag/message pairs. + +Only three symbols leave `commands`: `Cli` (+ its `pub json`/`command` fields), `Commands`, and +`select_command` (research Q2). `src/main.rs:62-64,83` uses exactly those. The crate is heavily +pinned: 10 integration test files spawn the real binary and assert exit codes, exact stdout/stderr +bytes, the `{"data"}`/`{"error":{"message","source"}}` JSON envelope, absence of `\x1b`, template +byte-equality, and clap parse rejection (research Q4). `tests/architecture.rs` uses rust_arkitect: +a `commands` import allowlist in both `crate::` and `orksorksorks::` spellings (`:56-75`) plus a +total module-order cycle ban `main < commands < config < config_dir < git < errors < format` +(`:86-140`). rust_arkitect attributes every file under `src/commands/` to the logical module +`orksorksorks::commands` (`:21-25`), so moving code *within* `commands` produces no external edge +changes and needs no new architecture rules. + +A prior attempt (draft PR #29) was never merged and contains no usable scheme — only a `DELETEME` +file. + +## Desired End State + +`src/commands/mod.rs` becomes a small, readable root file holding the public surface (`Cli`, +`Commands`, `select_command`), the router, and the parser tests. Each handler family lives in its +own submodule file under `src/commands/`, and the shared resolution helpers live in +`commands/resolve.rs`. Every new file follows the sibling-module shape exactly: imports → items → +helpers → trailing `#[cfg(test)] mod tests`. + +Concretely, `src/commands/` becomes: + +``` +mod.rs Cli, Commands, select_command_with_env, select_command, parse tests +init.rs init_command + tests +branch.rs branch_command + tests +artifact_directory.rs artifact_directory_command + tests +step.rs step_command + tests +model.rs model_command + tests +thinking.rs thinking_command + tests +prompt.rs prompt_command + tests +script.rs script_command + tests +resolve.rs artifact_dir_path, determine_step, resolve_model/step/prompt/script + tests +``` + +Verification is behavioral, not structural: the full gate (`scripts/test.sh`) passes unchanged, and +specifically: + +- All 10 integration test files pass with **no edits** — same CLI surface, same text output, same + JSON envelope, same exit codes, same error tags, no ANSI in captures. +- `tests/architecture.rs` passes untouched — the `commands` logical module's dependency set is + identical, and no submodule file introduces a `use` outside the existing allowlist. +- Unit tests move with their subject and continue to pin the same parse routes, dispatch arms, + handler strings, helper results, and `Error` tag/message pairs. +- `src/main.rs` is unmodified. +- No file in `src/commands/` exceeds ~300 lines (largest is `mod.rs` with `Cli`+`Commands`+router+ + parse tests; handlers are 20–60 lines each). + +## Patterns to Follow + +- **Trailing test module per file** — `mod tests` at the *end* of every module, `use super::*;`, + behavior-sentence names, `tempfile::tempdir()`, `pretty_assertions` (src/config.rs:254, + src/config_dir.rs:116, src/git.rs:39). Each new submodule gets its own; parse tests stay with + `Cli` in `mod.rs`; `resolve_*` tests move to `resolve.rs`; handler tests move with their handler. +- **Three-tier visibility** — `pub` for the crate surface (`Cli`, `Commands`, `select_command`), + `pub(crate)` for cross-module internals, private for file-local helpers (src/config.rs:131, + src/config_dir.rs:72,106; src/git.rs:28). Moved handlers/helpers become `pub(crate)` so + `mod.rs`'s router can call them. +- **`///` docs on every `pub` and `pub(crate)` item** — `#![warn(missing_docs)]` (src/main.rs:6) + plus clippy `-D warnings` makes this a hard gate; struct fields each get a doc line + (src/config_dir.rs:11-17, src/config.rs:10-19). This is the single most likely way to break the + build during the move. +- **`Error::new(tag, message)` with lowercase, namespaced tags** — `"config-exists"`, `"step"`, + `"model"`, `"prompt"`, `"script"`, `"config:*"` (src/commands/mod.rs:171-434, src/config.rs:134-231). + Do not invent new tags; preserve every existing tag verbatim. +- **`_in`-suffixed injectable-dir twins for testability** — src/git.rs:8,17. Only relevant if a + helper gains a dir parameter; do not add one gratuitously (behavior-preserving move). +- **Free snake_case imperative functions, no artificial parent type** — the crate uses free `fn`s; + do not introduce a `Commands` impl or context struct just to avoid passing arguments. +- **rust_arkitect dual-spelling allowlist** — any `use crate::…`/`use orksorksorks::…` added by the + move must already be one of the 10 allowlisted sibling spellings (tests/architecture.rs:56-75). + Sibling five + `clap`/`std`/test deps cover everything `commands` imports today. +- **Patterns NOT to follow**: do **not** add a second nesting level (`commands/handlers/*`), + `pub mod` declarations, or a central `tests.rs` — the repo has never used them (research Q3) and + the convention is per-module trailing tests. + +## Design Decisions + +1. **Domain-per-command files**: one submodule per handler family plus `resolve.rs` — mirrors + crate naming (`commands::step` ↔ `config::Step`) and the 1:1 `tests/*.rs` files; keeps each file + 20–60 lines. +2. **Helpers co-located in `commands/resolve.rs`**: the six resolution helpers stay together — they + are a mutually-recursive layer with one shared idiom and one cohesive test group. +3. **Tests co-located per submodule**: each new file carries its own trailing `mod tests` — the only + option consistent with the crate-wide convention; avoids recreating a 1,000-line file. +4. **`pub(crate)` handlers in private submodules**: `mod.rs` declares `mod init;` etc. (private), + handlers/helpers are `pub(crate)`, and `select_command_with_env` `use`s them. Root keeps only the + three `pub` items, so `src/main.rs` is untouched. +5. **`Cli`/`Commands` stay in `commands/mod.rs`**: smallest diff, no re-export indirection, + `main.rs` contract literally unchanged. +6. **`include_str!` stays as-is**: `../../templates/default.toml` (src/commands/mod.rs:181) resolves + relative to the source file, and `commands/init.rs` is the same directory depth as + `commands/mod.rs` — verified that repo-root `templates/default.toml` is the target, so the + literal survives the move with no edit. +7. **Behavior-preserving, mechanical move**: no logic edits, no signature changes beyond + `pub(crate)` visibility, no tag/message changes. Any behavioral difference is a bug. +8. **No child tickets**: all work lands on the main ticket. + +## What We're NOT Doing + +- **No logic refactors** — no deduplication of the `resolve_*` family, no extraction of shared + error-construction helpers, no restructuring of `select_command_with_env`. +- **No change to `Cli`/`Commands`/clap attributes** — variant names, flag names, `long_about=None`, + `arg_required_else_help`, and kebab naming all stay byte-identical. +- **No changes to `src/main.rs`, `src/config.rs`, `src/config_dir.rs`, `src/errors.rs`, + `src/format.rs`, `src/git.rs`, `build.rs`, or `scripts/`. +- **No edits to any file in `tests/`** — they are the verification harness; if one needs editing, the + move changed behavior. +- **No new architecture rules** and no changes to `tests/architecture.rs` — submodule files inherit + the `commands` identity. +- **No second nesting level, no `pub mod`, no re-export shims, no central test file.** +- **No new dependencies, no Cargo.toml changes.** +- **No documentation-site or README work.** + +## Open Risks + +- **`missing_docs` / clippy `-D warnings`**: newly `pub(crate)` handlers and helpers need `///` + docs. Every moved item must carry its doc across; the gate will catch omissions but they cost a + round-trip. +- **Test-module `use` hygiene**: inline tests use `use super::*;` plus `crate::config` and + `CommandFactory` (src/commands/mod.rs:439-441). After the split, `super::*` no longer reaches + `Cli`/`Commands` from, e.g., `step.rs`'s tests; those tests will need explicit + `use crate::commands::{Cli, Commands};` or equivalent. This is the most likely mechanical + breakage and is behavior-neutral. +- **Router arm edits**: each arm changes from a bare call to a path (`init::init_command(&path)`). + A typo here changes routing; integration tests (`tests/step.rs`, `tests/model.rs`, etc.) must + catch it — they dispatch each variant and assert output. +- **`determine_step` / `artifact_dir_path` cross-references**: helpers in `resolve.rs` call + `crate::git::current_branch` and take `&Step`; their move is straightforward but the helper test + group (~570-833) is large and must be relocated without alteration. +- **Line-count assumption**: research's 487-vs-809 discrepancy on `src/config.rs` was flagged + immaterial; no design decision here depends on it, and `src/config.rs` is out of scope. +- **No golden files**: verification relies entirely on the existing test suite; if a behavior is + *not* pinned by a test, the move could silently change it. Mitigation: keep the move mechanical + and diff-review each new file against its original region. diff --git a/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/done.md b/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/done.md new file mode 100644 index 0000000..f99cdd3 --- /dev/null +++ b/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/done.md @@ -0,0 +1,7 @@ +# Done + +- **Branch / head SHA**: `alanvardy-var-981-refactor-orksorksorks` @ `17f8c92d86ff529fcd44d65dd6a376ad4fd9d5cb` (6 commits ahead of `main`, 0 behind, tree clean) +- **Mechanical checks**: `scripts/test.sh` PASS — `cargo fmt`, `cargo check`, `cargo clippy --tests -- -D warnings`, `cargo nextest run` (197 tests, 197 passed, incl. both `tests/architecture.rs` tests), TODO/FIXME/dbg gate. Independent golden re-check by the parent: 6/6 baseline outputs byte-identical (`--help`, `-j branch`, `step --help`, `script --help`, `-j step` missing-config JSON, missing-config text); error path exits 1 as pinned. +- **Review outcome**: One fresh-context `reviewer` (single bounded pass, diff + pre-move original supplied). Verdict: **behavior preservation CONFIRMED, merge OK, no blockers/P0/P1/P2**. No fixes applied — the diff is a character-identical relocation (`fn` → `pub(crate)`, `resolve::` call prefixes, per-file trailing test modules) with no production-code `.unwrap()`/`panic!`. Deduplication of the repeated step-derivation blocks was declined as explicitly out of scope (design decision 8 / "What We're NOT Doing"). + - Optional/nits declined: `mod.rs` is 617 lines vs the design's ~300 estimate — parse tests must stay co-located with `Cli` per design, so it is a sanctioned exception. Commit `17f8c92` (Phase 5) swept all 22 `.pi/orksorksorks//` artifacts in one commit rather than per-phase; accepted because the `qrspi` skill mandates committing phase artifacts and `main` already tracks 67 such files (repo `AGENTS.md` wording conflicts — noted, not blocking). +- **Remaining manual items**: None blocking. The parent independently re-ran the baseline golden diffs. `implement.md`'s per-file character-identity review and the baseline comparisons were covered; the `--version` LONG_VERSION block line contents were not separately diffed by the parent (clap output is unchanged by this diff). Ticket branch is ready for merge. diff --git a/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/implement.md b/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/implement.md new file mode 100644 index 0000000..4cdaf5a --- /dev/null +++ b/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/implement.md @@ -0,0 +1,72 @@ +# Implementation Summary + +Plan: `.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/plan.md` — split `src/commands/mod.rs` (1459 lines) into a root file plus one submodule per handler family and a shared `resolve.rs`. All six phases implemented and verified; all automated items checked. + +## Commits + +| Phase | Commit | Description | +|-------|--------|-------------| +| 1 | — | Baseline freeze (no code — captures only, gitignored `baseline/` artifacts) | +| 2 | d9f5fbb | shared resolution layer — `commands/resolve.rs` (6 helpers + 22 tests) | +| 3 | 519076f | leaf handlers — `init.rs`, `branch.rs`, `artifact_directory.rs` | +| 4 | 4362481 | config-driven handlers — `step.rs`, `model.rs`, `thinking.rs` | +| 5 | a20af38 | frontmatter/script handlers — `prompt.rs`, `script.rs` | +| 6 | — | Final verification sweep (no code changes needed, no commit) | + +Housekeeping (pre-phase, rebase prerequisite): `chore: remove DELETEME placeholder` — the +unstaged `DELETEME` deletion (file text: "git rm before merging") blocked the required +rebase onto `origin/main`; committed as standalone housekeeping. The branch was rebased on +`origin/main` (3 new commits) and force-pushed once with `--force-with-lease` so phase +commits fast-forward; all phase pushes were clean fast-forwards. + +## End state + +- `src/commands/` — root `mod.rs` (617 lines: imports, constants, `Cli`, `Commands`, + router, parse/routing tests) + `resolve.rs` (542), `init.rs` (31), `branch.rs` (7), + `artifact_directory.rs` (10), `step.rs` (79), `model.rs` (24), `thinking.rs` (24), + `prompt.rs` (105), `script.rs` (70). Every file < 300 lines except the two sanctioned + exceptions (`mod.rs` ~600, `resolve.rs` ~560) per the plan's "Corrected acceptance criteria". +- Nothing outside `src/commands/` touched: `tests/` (incl. `tests/architecture.rs`), + `src/main.rs`, `src/config*.rs`, `src/errors.rs`, `src/format.rs`, `src/git.rs`, + `templates/`, `scripts/`, `Cargo.toml`, `build.rs` all verified untouched via + `git diff --stat origin/main`. +- All byte-identity checks passed: moved bodies/docs/tests character-identical apart from + the sanctioned `pub(crate)` promotions, `resolve::` prefixes, and test-module relocations. + Frontmatter string in `prompt.rs` and the `"script"` error tag preserved byte-for-byte. + +## Automated Checks + +- [x] `scripts/test.sh` (fmt → check → clippy `--tests -- -D warnings` → nextest → forbidden strings) passes: 197 tests run, 197 passed — identical count across all gates, including baseline +- [x] `cargo nextest run resolve::` — 22/22 relocated resolve tests +- [x] `cargo nextest run select_command_routes` — 6/6 router tests after each arm re-pointing +- [x] `cargo nextest run step` / `model` / `thinking` — integration suites pass +- [x] `cargo nextest run prompt` / `script` — integration suites + 3 relocated unit tests pass +- [x] Architecture tests pass (`commands_imports_only_downward_modules`, `no_upward_imports_or_cycles` in `tests/architecture.rs`) — untouched, graph unchanged +- [x] `cargo clippy --tests -- -D warnings` clean at every phase (no unused imports; `crate::git` dropped from `mod.rs` in Phase 5 as planned) +- [x] `git status --short` — only `src/commands/` changes (plus gitignored artifact dir) +- [x] Baseline diffs empty: `--help`, `-j branch`, `-j step --config /nonexistent/orks.toml` byte-identical to `baseline/` +- [x] `rg 'TODO:|todo:|FIXME|fixme|dbg!|DEBUG:|FIXTURE:' src/commands/` — no matches + +## Manual Verification Items (from the plan) + +- [ ] Eye-check `baseline/help.txt` contains all eight subcommand names and the `LONG_VERSION` block (build target/profile/timestamp lines — note: the block is emitted by `--version`, not `--help`; worker verified it present via `--version`) +- [ ] `cargo run --quiet -- -j branch` output still parses as `{"data":""}` +- [ ] `cargo run --quiet -- --help` matches `baseline/help.txt` (`diff`) +- [ ] `./target/debug/orksorksorks init --help` matches `baseline/help.txt`-style expectations (unchanged clap output) +- [ ] `./target/debug/orksorksorks -j step --config /nonexistent/orks.toml` still emits the same error envelope as `baseline/step-missing.json` +- [ ] `./target/debug/orksorksorks step --help` matches `baseline/step-help.txt` +- [ ] `./target/debug/orksorksorks -j step --config /nonexistent/orks.toml` matches `baseline/step-missing.json`; text mode matches `baseline/step-missing.txt` +- [ ] `./target/debug/orksorksorks script --help` matches `baseline/script-help.txt` +- [ ] `./target/debug/orksorksorks -j branch` matches `baseline/branch.json` +- [ ] Per-file diff review: each new file's body is character-identical to its original `mod.rs` region apart from `pub(crate)`, the `resolve::` prefixes, and the trailing test module relocation +- [ ] Confirm the routing arms read cleanly in `mod.rs` and each namespaced call matches its file (e.g. `prompt::prompt_command`) + +Note: workers already ran byte-identical diffs for several of the manual items above (step-help, script-help, branch.json, step-missing.*, `--help`) — they are listed as manual for the owner to confirm independently. + +## Observations (non-blocking, for the record) + +- **Nextest filter zero-matches in plan.md**: filters like `init_`, `artifact_`, `json_output`, `architecture` match no tests by name as written; workers ran the real suite names instead (e.g. `tests/architecture.rs` tests are `commands_imports_only_downward_modules` / `no_upward_imports_or_cycles`). All underlying suites pass. +- `mod.rs` lands at 617 lines (17 above the ~600 estimate) and `resolve.rs` at 542 (18 below ~560) — within the plan's corrected acceptance criteria; both sanctioned exceptions to the < 300 rule. +- Plan checkbox counts varied slightly from the implement-prompt notes (e.g. 5 vs 4 automated items in Phase 2); the plan's own counts were honored. +- The `-j step --config /nonexistent/orks.toml` error-path command exits status 1 (pinned envelope behavior) — don't chain it with `&&` when diffing. +- `.pi/orksorksorks` is hidden from tooling via the repo `.ignore` file (not `.gitignore`), so the artifact dir legitimately shows in `git status --short`. \ No newline at end of file diff --git a/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/large.md b/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/large.md new file mode 100644 index 0000000..3ec6d70 --- /dev/null +++ b/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/large.md @@ -0,0 +1,7 @@ +# Task + +Refactor the orksorksorks Rust CLI so that `src/commands/mod.rs` (currently 1,459 of the crate's 2,884 total lines — the CLI parser, all subcommand structs, and their handlers in one file) is split into an organized set of submodules. The ticket explicitly calls for researching how to organize the codebase before moving code, and the change must be behavior-preserving (same CLI surface, same output). + +## Why LARGE + +UNKNOWNS + DESIGN_SIGN-OFF: the ticket itself asks to "research how to organize the codebase" — no organization scheme is given, several viable module groupings exist (by command domain, by shared-helper extraction, by parser-vs-handler split), and this is the central command surface of the crate, so a design pass with human sign-off is expected. A prior attempt (draft PR #29) was never merged, corroborating that a naive one-pass split is not satisfactory. \ No newline at end of file diff --git a/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/phase1_output.md b/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/phase1_output.md new file mode 100644 index 0000000..0e7c066 --- /dev/null +++ b/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/phase1_output.md @@ -0,0 +1,54 @@ +# Phase 1 Output — Baseline freeze (no code) + +Status: **COMPLETE** — no commit made (no code changes; baseline/ is artifact-only). + +## scripts/test.sh (pristine tree) +PASSED. Final lines: + +``` + Summary [ 0.751s] 197 tests run: 197 passed, 0 skipped +=== TODO/FIXME/dbg gate === +=== SUCCESS === +``` + +## wc -l src/commands/mod.rs +`1459` — matches expectation. + +## nextest summary → baseline/nextest.txt +``` + PASS [ 0.008s] (195/197) ... git::tests::parse_branch_output_trims_trailing_newline + PASS [ 0.082s] (196/197) ... git::tests::current_branch_in_returns_configured_branch + PASS [ 0.095s] (197/197) ... git::tests::current_branch_in_detached_head_errors +──────────── + Summary [ 0.752s] 197 tests run: 197 passed, 0 skipped +``` + +## baseline/ captures +``` +total 56 +branch.json 51 bytes {"data":"alanvardy-var-981-refactor-orksorksorks"} +help.txt 899 bytes +nextest.txt 459 bytes +script-help.txt 445 bytes +step-help.txt 389 bytes +step-missing.json 156 bytes {"error":{"message":"could not read config file at /nonexistent/orks.toml (specified via --config): No such file or directory (os error 2)","source":"io"}} +step-missing.txt 135 bytes +``` +All captured with `cargo build --quiet` first; nonexistent-config invocations exit 1 as expected and capture the envelope/error cleanly. + +## git status +``` +?? .pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/ +``` +`git diff --stat` is empty — **no source edits**. Note: the artifact dir is excluded via `.ignore`, not `.gitignore` (repo `.gitignore` only has `/target`), so it shows as a single untracked directory — exactly the expected "only the new untracked baseline dir" state. + +## Manual eye-check (for user confirmation) +`baseline/help.txt` lists all eight subcommand names: `init`, `branch`, `artifact_directory`, `step`, `model`, `thinking`, `prompt`, `script` (plus `help`). +The `LONG_VERSION` block is not in `--help` output; it is emitted by `--version` (`orksorksorks 0.1.0` + `build target: aarch64-apple-darwin` + `build profile: debug` + `build timestamp: 2026-09-12T20:39:47.218622+00:00`) — verified present. + +## plan.md +All 5 Phase 1 automated checkboxes marked `[x]`; the Manual item left `[ ]` per instructions. + +## Notes +- `.ignore` (not `.gitignore`) hides `.pi/orksorksorks`; status shows the artifact dir as untracked, matching "no source edits" intent. +- No commit: phase explicitly makes no code changes and baseline is artifact-only. \ No newline at end of file diff --git a/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/phase2_output.md b/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/phase2_output.md new file mode 100644 index 0000000..fc28dff --- /dev/null +++ b/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/phase2_output.md @@ -0,0 +1,39 @@ +# Phase 2 output — Shared resolution layer (`commands/resolve.rs`) + +## Commit +- SHA: `d9f5fbb` — `Phase 2: shared resolution layer — commands/resolve.rs` +- Pushed to `origin/alanvardy-var-981-refactor-orksorksorks` (fast-forward `265a2bb..d9f5fbb`; no force needed). + +## Changes +- **Created `src/commands/resolve.rs`** (542 lines): imports `use crate::config::{Config, Step};` + `use crate::errors::Error;`; the six helpers moved **verbatim** from `mod.rs` in original source order (`artifact_dir_path`, `determine_step`, `resolve_model`, `resolve_step`, `resolve_prompt`, `resolve_script`), each with its `///` doc, visibility promoted to `pub(crate)`; `#[cfg(test)] mod tests` with `use super::*;` and the **22 relocated tests** (5 path-composition, 7 step-derivation, 10 resolve hit/miss) — no `use crate::git;` (none of the helpers calls git). +- **Modified `src/commands/mod.rs`** (1459 → 924 lines): added `mod resolve;` after the import block; every helper call-site in the still-resident handlers prefixed with `resolve::` (7× `artifact_dir_path`, 5× `determine_step`, 5× `resolve_step`, 2× `resolve_model`, 1× `resolve_prompt`, 1× `resolve_script`); deleted the six helper definitions; removed `use crate::config::{Config, Step};` from the module top and from the test module. Top imports now exactly: `use crate::errors::Error; use crate::git; use clap::{Parser, Subcommand}; use std::path::PathBuf;`. +- No other files touched (`git diff --stat`: only the two files above). + +## Verification +| Command | Result | +|---|---| +| `cargo check` | passed | +| `scripts/test.sh` (fmt → check → clippy -D warnings → nextest → forbidden strings) | passed — **197 tests / 197 passed** (matches baseline) | +| `cargo nextest run resolve::` | passed — **22/22** relocated tests | +| `cargo nextest run select_command_routes` | passed — **6/6** router tests | +| `cargo clippy --tests -- -D warnings` | clean (no unused `Config`/`Step` import) | +| `./target/debug/orksorksorks --help` vs `baseline/help.txt` | identical (`diff` clean) | +| `./target/debug/orksorksorks -j branch` | `{"data":"alanvardy-var-981-refactor-orksorksorks"}` (manual item, confirmed) | + +## Byte-identity check +Scripted comparison of every moved item against `git show HEAD:src/commands/mod.rs`: +- All 22 test bodies byte-identical after the planned 4-space re-indent. +- All 6 helper bodies byte-identical after the planned `pub(crate) ` promotion on the signature line. +- Assertions also confirmed no moved fn/test still exists in `mod.rs` and exactly the expected prefix counts landed. + +## plan.md +Phase 2 automated checkboxes (5) marked `[x]`; the 2 manual items left unchecked. + +## Observations / notes +1. The plan's Phase 2 automated list actually contains **5** checkboxes (`cargo check`, `test.sh`, `resolve::`, `select_command_routes`, `clippy`), not 4 — all 5 checked. +2. An initial transform attempt corrupted the `use clap::CommandFactory;` indent (substring-replace matched both import sites); it was caught by diff review and fixed with line-oriented removal. Re-verified; final `scripts/test.sh` gate (includes `cargo fmt --all`) confirms formatting is canonical. +3. `mod.rs` tests remaining in place rely on `step_command`/`prompt_command`/`script_command` (still resident) — none of the remaining tests reference the moved helpers or `Config`/`Step` types, so removing both imports was safe (clippy confirms). +4. Total crate line delta: 1466 vs 1459 original (+7: `mod resolve;`, its blank line, and resolve.rs separator lines) — within the plan's "± a few" allowance. + +## Risks for later phases +- None blocking. Later phases must remember `resolve::` is now the only way to reach these helpers, and `mod.rs` no longer imports `Config`/`Step` (Phase 4 handlers must add their own imports or use `crate::config::...` fully qualified as the plan specifies). \ No newline at end of file diff --git a/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/phase3_output.md b/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/phase3_output.md new file mode 100644 index 0000000..a92adaa --- /dev/null +++ b/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/phase3_output.md @@ -0,0 +1,28 @@ +# Phase 3 Report — Leaf handlers: `init.rs`, `branch.rs`, `artifact_directory.rs` + +## Commit +- `519076f` "Phase 3: leaf handlers — init, branch, artifact_directory" — pushed to origin (fast-forward, no force needed). + +## Changed files +- `src/commands/init.rs` (new, 31 lines) — `pub(crate) fn init_command`, body moved verbatim from `mod.rs` (incl. inner `use std::io::Write;`, `include_str!("../../templates/default.toml")`, `crate::format::green_string`). Import: `crate::errors::Error`. No test module (per plan). +- `src/commands/branch.rs` (new, 7 lines) — `pub(crate) fn branch_command` delegating to `git::current_branch()`. Imports: `crate::errors::Error`, `crate::git`. No test module. +- `src/commands/artifact_directory.rs` (new, 10 lines) — `pub(crate) fn artifact_directory_command` using `std::env::current_dir()?` + `resolve::artifact_dir_path(&cwd, &git::current_branch()?)`. Imports: `super::resolve`, `crate::errors::Error`, `crate::git`. No test module. +- `src/commands/mod.rs` (modified) — added `mod artifact_directory; mod branch; mod init;`; re-pointed the three router arms to `init::init_command(&path)`, `branch::branch_command()`, `artifact_directory::artifact_directory_command()`; deleted the three handler definitions (45 lines removed). Imports unchanged this phase (`Error`, `git` still used by remaining handlers). + +## Verification results +| Check | Result | +|---|---| +| `cargo check` | passed | +| `scripts/test.sh` (fmt, check, clippy `--tests -- -D warnings`, nextest 197/197, forbidden strings) | passed | +| `cargo nextest run branch` | 11 passed, 186 skipped | +| `cargo nextest run artifact_directory` | 5 passed, 192 skipped | +| `cargo nextest run init` | 13 passed, 184 skipped (includes the 8 `tests/init_creates_file.rs` integration tests + init routing/parse unit tests) | +| `./target/debug/orksorksorks -j step --config /nonexistent/orks.toml` vs `baseline/step-missing.json` | byte-identical | +| `./target/debug/orksorksorks init --help` | unchanged clap output | +| `./target/debug/orksorksorks -j branch` / `-j artifact_directory` | correct data in real checkout | + +## plan.md +All 5 Phase 3 automated items checked `[x]`; Manual items left unchecked. + +## Observation (filter naming, not a code mismatch) +The plan's `cargo nextest run init_` and `cargo nextest run artifact_` filters match **zero** tests under the installed nextest (the suites are keyed by bin name `init_creates_file` / `artifact_directory`, and a trailing `_` prefix pattern doesn't bind). The intended suites were run by their real names and all pass; the full gate (197 tests) also passes. No plan deviation needed. \ No newline at end of file diff --git a/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/phase4_output.md b/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/phase4_output.md new file mode 100644 index 0000000..ea75eff --- /dev/null +++ b/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/phase4_output.md @@ -0,0 +1,75 @@ +# Phase 4 Output — Config-driven handlers: `step.rs`, `model.rs`, `thinking.rs` + +## Result: DONE — committed `4362481`, pushed to origin + +## Commit + +``` +4362481 Phase 4: config-driven handlers — step, model, thinking +4 files changed, 133 insertions(+), 116 deletions(-) +create mode 100644 src/commands/model.rs +create mode 100644 src/commands/step.rs +create mode 100644 src/commands/thinking.rs +``` + +Pushed: `519076f..4362481 alanvardy-var-981-refactor-orksorksorks -> alanvardy-var-981-refactor-orksorksorks` + +## Changes made + +1. **`src/commands/step.rs` (new, 79 lines)** — `pub(crate) fn step_command` moved verbatim from + `mod.rs` (doc + body), helper calls already carrying the `resolve::` prefix from Phase 2 + (`resolve::resolve_step`, `resolve::artifact_dir_path`, `resolve::determine_step`), + `git::current_branch` unchanged. Imports: `use super::resolve; use crate::errors::Error; + use crate::git;`. Trailing `#[cfg(test)] mod tests { use super::*; }` with the two relocated + tests `step_command_flag_succeeds_in_non_git_dir` and + `step_command_flag_unknown_name_tags_step` (bodies verbatim; `ConfigPathSource::ExplicitFlag` + fully qualified, no extra imports). +2. **`src/commands/model.rs` (new, 24 lines)** — `pub(crate) fn model_command` moved verbatim + (returns `model.model`). Same three imports. No test module (per plan deviation 3/5). +3. **`src/commands/thinking.rs` (new, 24 lines)** — `pub(crate) fn thinking_command` moved + verbatim (returns `model.thinking`). Same three imports. No test module. +4. **`src/commands/mod.rs`** — added `mod model; mod step; mod thinking;` declarations; + re-pointed the three router arms to `step::step_command(...)`, `model::model_command(...)`, + `thinking::thinking_command(...)`; deleted the three handler definitions and the two + `step_command_*` tests. Imports unchanged (`crate::git` and `Error` still used by + prompt/script handlers and the router). Diff: +6/−116 lines net. + +## Verification (all passed) + +| Check | Result | +|---|---| +| `cargo check` | PASS | +| `scripts/test.sh` (fmt → check → clippy `--tests -- -D warnings` → nextest → forbidden strings) | PASS — 197/197, `=== SUCCESS ===` | +| `cargo nextest run step_command` (2 relocated unit tests) | PASS (2/2, `commands::step::tests::*`) | +| `cargo nextest run step` (integration `tests/step.rs`) | PASS (67/67 matched) | +| `cargo nextest run model` (integration `tests/model.rs`) | PASS (23/23 matched) | +| `cargo nextest run thinking` (thinking block in `tests/model.rs`) | PASS (12/12 matched) | +| `cargo nextest run select_command_routes` (router arms after re-pointing) | PASS (6/6) | +| `step --help` vs `baseline/step-help.txt` | byte-identical diff | +| `-j step --config /nonexistent/orks.toml` vs `baseline/step-missing.json` | byte-identical diff | +| text `step --config /nonexistent/orks.toml` vs `baseline/step-missing.txt` | byte-identical diff | + +Line counts vs plan criteria: `step.rs` 79 (~85 expected), `model.rs`/`thinking.rs` 24 each +(~40 expected, no test module), `mod.rs` 775 (continues to shrink in Phase 5). + +## plan.md checkboxes + +All 5 Phase 4 AUTOMATED items checked (`- [x]`); both Manual items left unchecked. +(Note: the task text said "3 AUTOMATED items" but the plan lists 5 — all 5 automated items +were run and checked. The `.pi/` dir is gitignored, so this edit is not part of the commit.) + +## Working tree state + +Only `?? .pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/` untracked (gitignored +artifact dir). No source changes outside `src/commands/`. `git status --short` clean otherwise. + +## Observations / risks + +- None blocking. Body bytes were copied character-identical (see the mod.rs diff — removed + regions and new files match); only additions were `pub(crate)`, module declarations, and the + namespaced router arms. +- `cargo nextest run step`/`model`/`thinking` are substring filters that also match + `config::`/`config_dir::` tests; the plan's intent (integration + relocated unit suites) is + satisfied — verified explicitly with the `step_command` filter and `select_command_routes`. +- Next phase (5) will move `prompt.rs`/`script.rs` and drop the last `crate::git` import from + `mod.rs`; imports are still consistent in the current tree. \ No newline at end of file diff --git a/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/phase5_output.md b/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/phase5_output.md new file mode 100644 index 0000000..72e5129 --- /dev/null +++ b/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/phase5_output.md @@ -0,0 +1,33 @@ +# Phase 5 Output — Frontmatter/script handlers (`prompt.rs`, `script.rs`) + +## Commit +- **SHA:** `a20af38` — `Phase 5: frontmatter/script handlers — prompt, script` +- **Push:** fast-forward `4362481..a20af38` to `origin/alanvardy-var-981-refactor-orksorksorks` — succeeded, no force needed. +- Working tree post-commit: clean except the expected untracked/gitignored artifact dir `.pi/orksorksorks/`. + +## Changes +- **Created `src/commands/prompt.rs`** (104 lines): `pub(crate) fn prompt_command` moved verbatim from mod.rs (docs + body, only `fn` → `pub(crate) fn`); imports `use super::resolve; use crate::errors::Error; use crate::git;`; trailing `#[cfg(test)] mod tests` with `use super::*;` containing the 2 relocated tests. +- **Created `src/commands/script.rs`** (69 lines): `pub(crate) fn script_command` moved verbatim; same import shape; trailing test module with the 1 relocated test. +- **Modified `src/commands/mod.rs`** (775 → 618 lines): removed `use crate::git;` (top imports are now exactly `use crate::errors::Error;`, `use clap::{Parser, Subcommand};`, `use std::path::PathBuf;`); added `mod prompt;` and `mod script;` (nine module declarations total); re-pointed the two router arms to `prompt::prompt_command` / `script::script_command`; deleted both handler definitions and the three handler-direct tests. Root now holds only imports, NAME/AUTHOR/ABOUT/LONG_VERSION, `Cli`, `Commands`, router, and parse/routing tests. + +## Byte-identity confirmation +- Extracted the original handler blocks from `HEAD:src/commands/mod.rs` (before this phase) and the new files; diff empty after the single permitted `fn` → `pub(crate) fn` transformation on the signature line. +- **Frontmatter string** `"## Important variables\n..."` in `prompt.rs` is **byte-identical** to the original (regex-extracted and compared). +- **`"script"` error tag** `Error::new("script", ...)` preserved in `script.rs`; `script.rs` contains `## Important variables` nowhere (no-frontmatter contract intact). + +## Verification results (all passed) +- `cargo check` — pass +- `scripts/test.sh` (fmt, check, clippy --tests -- -D warnings, nextest, forbidden-strings) — pass; **197/197 tests** +- `cargo nextest run prompt` — 29 tests passed (incl. integration `tests/prompt.rs` + relocated unit tests, which also ran as `cargo nextest run prompt_command_step` → 2/2 passed as `commands::prompt::tests::*`) +- `cargo nextest run script` — 19 tests passed (incl. `tests/script.rs`; relocated unit test confirmed via `cargo nextest run script_command_` → 1/1 passed as `commands::script::tests::script_command_step_without_script_errors`) +- json envelope/no-ANSI — the plan's `cargo nextest run json_output` filter matches 0 tests (test names are `init_json_returns_valid_json_with_data_field`, `init_no_json_prints_plain_text` in `tests/json_output.rs`); ran the actual names via `cargo nextest run init_json init_no_json` → 3 tests passed. Full gate's own nextest run already covered them. +- `cargo clippy --tests -- -D warnings` — clean (no leftover `crate::git` import warning) +- **Manual confirmations:** `script --help` vs `baseline/script-help.txt` — **identical**; `-j branch` vs `baseline/branch.json` — **identical**. + +## plan.md +- Phase 5's 6 automated checkboxes checked; 2 manual items left unchecked. + +## Observations +- The task prompt said "5 automated items"; plan.md Phase 5 lists 6 — all 6 were verified and checked (the count in the task was a miscount; the plan is authoritative). +- `cargo nextest run json_output` is a zero-match filter as written in plan.md — noted for Phase 6; actual json tests pass. +- `git show HEAD:src/commands/mod.rs` used for byte-identity reference (pre-phase state); nothing outside `src/commands/` touched; `tests/`, `src/main.rs`, etc. untouched. \ No newline at end of file diff --git a/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/phase6_output.md b/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/phase6_output.md new file mode 100644 index 0000000..e6e5281 --- /dev/null +++ b/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/phase6_output.md @@ -0,0 +1,90 @@ +# Phase 6: Final verification sweep — output + +## Result: ALL AUTOMATED CHECKS PASS — no code changes, no commit + +Phase 6 was a verification-only phase. No leftover imports or dead items were found, so +nothing under `src/commands/` was touched and no commit was made this phase. + +## Per-check results + +### 1. `scripts/test.sh` — PASS +- fmt, check, clippy `--tests -- -D warnings`, nextest, forbidden-strings all green. +- Nextest: **197 tests run: 197 passed, 0 skipped**. +- `=== SUCCESS ===` printed. + +### 2. `cargo nextest run architecture` — see note; actual architecture tests PASS +- **Nextest filter quirk:** the literal filter `architecture` matches **0 tests** — the two + tests in `tests/architecture.rs` are named `commands_imports_only_downward_modules` and + `no_upward_imports_or_cycles`, neither starting with "architecture". Plan's command is a + zero-match. +- Ran them by name instead: + ```bash + cargo nextest run -E 'test(commands_imports_only_downward_modules) or test(no_upward_imports_or_cycles)' + ``` + → **2 tests run: 2 passed** (`orksorksorks::architecture commands_imports_only_downward_modules`, + `orksorksorks::architecture no_upward_imports_or_cycles`). Architecture graph confirmed + unchanged (`tests/architecture.rs` untouched). + +### 3. `git status --short` — PASS +Only `?? .pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/` (artifact/baseline dir). +No source edits. Note: `.github/.gitignore` only ignores `/target`; `.pi/orksorksorks` is +hidden from rg/tools via the `.ignore` file (not from git), so it legitimately shows as +untracked in `git status`. + +### 4. `git diff --stat origin/main` — PASS +Only `src/commands/` files changed; `tests/` and `src/main.rs` untouched. + +### 5. `wc -l src/commands/*.rs` — PASS (meets "Corrected acceptance criteria") +``` + 10 src/commands/artifact_directory.rs + 7 src/commands/branch.rs + 31 src/commands/init.rs + 617 src/commands/mod.rs + 24 src/commands/model.rs + 105 src/commands/prompt.rs + 542 src/commands/resolve.rs + 70 src/commands/script.rs + 79 src/commands/step.rs + 24 src/commands/thinking.rs + 1509 total +``` +Every file < 300 lines **except** the two sanctioned exceptions (`mod.rs` ~600 target → 617; +`resolve.rs` ~560 target → 542). All others within plan targets: +`step.rs` 79 (~85), `prompt.rs` 105 (~115), `script.rs` 70 (~80), `model.rs`/`thinking.rs` 24 +(~40), `init.rs` 31 (<40), `branch.rs` 7 (<40), `artifact_directory.rs` 10 (<40). + +### 6. Baseline diffs — PASS (all IDENTICAL) +```bash +cargo build --quiet +./target/debug/orksorksorks --help > /tmp/ork-help.txt 2>&1 +diff baseline/help.txt /tmp/ork-help.txt # IDENTICAL +./target/debug/orksorksorks -j branch > /tmp/ork-branch.txt 2>&1 +diff baseline/branch.json /tmp/ork-branch.txt # IDENTICAL +./target/debug/orksorksorks -j step --config /nonexistent/orks.toml > /tmp/ork-step.json 2>&1 +diff baseline/step-missing.json /tmp/ork-step.json # IDENTICAL (diff exit 0) +``` +Note: the `-j step --config /nonexistent/orks.toml` command exits with status 1 (expected +error path), so it must not be chained with `&&` before the `diff` (the plan's inline chain +works because the `>` redirect output is what matters; observed safe behavior: run then diff +separately). + +### 7. Forbidden strings — PASS +`rg -n 'TODO:|todo:|FIXME|fixme|dbg!|DEBUG:|FIXTURE:' src/commands/` → no matches (exit 1). + +## plan.md update +Phase 6 automated items: **7/7 checked** (`- [x]`). The parent's instruction said "6 automated +items" but the plan lists 7 — all 7 were honored. The 2 Manual items remain unchecked (leave +for user confirmation). + +## Commit status +**No commit made** — zero source changes were needed in Phase 6. Branch +`alanvardy-var-981-refactor-orksorksorks` remains at `a20af38` (Phase 5) — nothing to push. + +## Observations +- `mod.rs` (617) is 17 lines above the ~600 estimate and `resolve.rs` (542) 18 below ~560 — + both within the corrected acceptance criteria and well under the problematic 300-line rule. +- Nextest filter naming: several plan commands filter on names that don't exist verbatim + (`architecture` here; earlier phases noted the same for `init_`/`artifact_`/`json_output`). + The underlying suites all pass when addressed by real test names. +- The `-j step ...` error-path command returns exit 1; that is the pinned error envelope + behavior, confirmed byte-identical to baseline. \ No newline at end of file diff --git a/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/plan.md b/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/plan.md new file mode 100644 index 0000000..fde84af --- /dev/null +++ b/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/plan.md @@ -0,0 +1,575 @@ +# Implementation Plan + +## Overview + +Split `src/commands/mod.rs` (1,459 lines) into a small root file plus one submodule per +handler family and a shared `resolve.rs`, by mechanically moving existing items — +**no logic changes** except visibility promotions (`pub(crate)`) and import hygiene. +Every stage ends with the unchanged `scripts/test.sh` gate green and the 10 integration +test files plus `tests/architecture.rs` untouched. + +This plan implements `structure.md` stages 1–6 in order. It depends on `design.md`, +`research.md`, and `conventions.md`; line numbers below are orientation only — every item is +named so it is unambiguous even after earlier stages shift line numbers. + +### Ground rules (apply to every stage) + +- **Move verbatim.** Cut each named item (function + its `///` docs + its tests) from + `mod.rs` and paste it unchanged, except: add `pub(crate)` to moved fns, re-point moved + tests, and re-point call sites with a `resolve::` prefix. +- **Never edit anything outside `src/commands/`** — `src/main.rs`, `src/config*.rs`, + `src/errors.rs`, `src/format.rs`, `src/git.rs`, `templates/`, `tests/`, `scripts/`, + `Cargo.toml`, `build.rs` are all frozen. +- **Gate:** run `scripts/test.sh` at the end of every stage (fmt → check → clippy + `--tests -- -D warnings` → nextest → forbidden-strings). Never declare a stage done while + it fails. +- **`#![warn(missing_docs)]` + clippy `-D warnings`:** every moved `pub(crate)` item keeps its + `///` doc. This is the most likely self-inflicted gate failure. +- **Intra-`commands` paths use `super::resolve`**, never `crate::commands::resolve` — keeps + rust_arkitect free of a `commands → commands` edge. Sibling paths (`crate::config`, + `crate::config_dir`, `crate::errors`, `crate::format`, `crate::git`) are already allowlisted + in both spellings (`tests/architecture.rs:56-75`). +- **No child tickets** — all work lands on this ticket, one branch. +- **Deviation from `structure.md` Stage 6 / design "≤ ~300 lines"** (read this): with + `Cli`+`Commands`+router+parse/routing tests kept in `mod.rs` (design decision 5) and all + `resolve_*` tests co-located in `resolve.rs` (design decisions 2–3), the two files land at + **~600 and ~560 lines** respectively — the arithmetic is in "Corrected acceptance criteria" + below. The 300-line figure is not attainable without moving parse tests out of `mod.rs`, + which design explicitly forbids. The Stage 6 check below uses the corrected criterion. No + other structural change is needed. + +### Corrected acceptance criteria + +| File | Expected after split | +|---|---| +| `mod.rs` | ~600 lines: imports, `NAME`/`AUTHOR`/`ABOUT`/`LONG_VERSION`, `Cli`, `Commands`, router, parse/routing tests only | +| `resolve.rs` | ~560 lines: 6 helpers + 22 helper tests | +| `init.rs` / `branch.rs` / `artifact_directory.rs` | < 40 lines each, no test module | +| `step.rs` | ~85 lines (handler + 2 tests) | +| `model.rs` / `thinking.rs` | ~40 lines each, no test module | +| `prompt.rs` | ~115 lines (handler + 2 tests) | +| `script.rs` | ~80 lines (handler + 1 test) | + +Every file is < 300 lines **except** `mod.rs` (parser + parser tests) and `resolve.rs` +(shared layer + its test group). Total crate lines unchanged (± a few for module declarations +and import lines). If the owner wants those two under 300, stop and re-run the `structure` +step with a different grouping — do **not** silently move tests out. + +--- + +## Phase 1: Baseline freeze (no code) + +Record the pre-move reference state so any un-pinned behavior drift is detectable. The design +flags "no golden files" as the top verification risk; this buys the cheap safety net. + +### Changes + +None. Captures are artifacts under the ticket directory, not sources. + +### Verification + +#### Automated +- [x] `scripts/test.sh` passes on the pristine tree +- [x] Record `wc -l src/commands/mod.rs` → expect `1459` +- [x] Record the nextest summary counts (e.g. `cargo nextest run --no-tests pass 2>&1 | tail -5`) + into `baseline/nextest.txt` +- [x] Capture reference output bytes (all deterministic, no tempdirs needed): + ```bash + mkdir -p .pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/baseline + cargo build --quiet + ./target/debug/orksorksorks --help > .pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/baseline/help.txt 2>&1 + ./target/debug/orksorksorks step --help > .pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/baseline/step-help.txt 2>&1 + ./target/debug/orksorksorks script --help > .pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/baseline/script-help.txt 2>&1 + ./target/debug/orksorksorks -j branch > .pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/baseline/branch.json 2>&1 + ./target/debug/orksorksorks -j step --config /nonexistent/orks.toml > .pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/baseline/step-missing.json 2>&1 + ./target/debug/orksorksorks step --config /nonexistent/orks.toml > .pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/baseline/step-missing.txt 2>&1 + ``` +- [x] `git status --short` shows only the new untracked `baseline/` directory (no source edits) + +#### Manual +- [ ] Eye-check `baseline/help.txt` contains all eight subcommand names and the `LONG_VERSION` + block (build target/profile/timestamp lines) + +--- + +## Phase 2: Shared resolution layer — `commands/resolve.rs` + +Extracts the mutually-recursive helper tier every config-driven handler calls. This is the +bottom-most stage: every later handler file consumes `resolve::`. + +### Changes + +#### 1. `src/commands/resolve.rs` — **create** + +Module header + the six helpers, moved verbatim (docs included), visibility promoted to +`pub(crate)`, in original source order: + +```rust +use crate::config::{Config, Step}; +use crate::errors::Error; + +/// +pub(crate) fn artifact_dir_path(cwd: &std::path::Path, branch: &str) -> String { /* moved */ } + +/// +pub(crate) fn determine_step(config: &Config, artifact_dir: &str) -> Result { /* moved */ } + +/// +pub(crate) fn resolve_model(config: &Config, name: &str) -> Result { /* moved */ } + +/// +pub(crate) fn resolve_step(config: &Config, name: &str) -> Result { /* moved */ } + +/// +pub(crate) fn resolve_prompt(config: &Config, name: &str) -> Result { /* moved */ } + +/// +pub(crate) fn resolve_script(config: &Config, name: &str) -> Result { /* moved */ } + +#[cfg(test)] +mod tests { + use super::*; + // + the 22 relocated tests listed below (each already uses `tempfile::tempdir()`, + // `pretty_assertions::assert_eq` inline; `Config`/`Step` come in via `super::*`) +} +``` + +**Note (deviation from `structure.md`):** `resolve.rs` needs **no `use crate::git;`** — none +of the six helpers calls git. `artifact_directory_command`/`step_command`/etc. (the git +callers) are the ones that import it. + +Relocated tests (cut from `mod.rs`, keep bodies byte-identical): + +| Function (current line) | Group | +|---|---| +| `artifact_dir_path_appends_trailing_slash` (536) | path composition | +| `artifact_dir_path_is_plain` (543) | path composition | +| `artifact_dir_path_handles_branch_with_slashes` (549) | path composition | +| `artifact_dir_path_replaces_all_slashes` (556) | path composition | +| `artifact_dir_path_leaves_slashless_branch_unchanged` (563) | path composition | +| `determine_step_returns_step_with_present_artifact` (570) | step derivation | +| `determine_step_prefers_last_step_in_reverse` (599) | step derivation | +| `determine_step_empty_trigger_is_default_fallback` (629) | step derivation | +| `determine_step_empty_trigger_does_not_shadow_real_match` (660) | step derivation | +| `determine_step_empty_trigger_matches_before_later_artifact` (689) | step derivation | +| `determine_step_latest_empty_trigger_wins` (720) | step derivation | +| `determine_step_no_match_errors_with_step_tag` (814) | step derivation | +| `resolve_model_returns_model_for_matching_name` (840) | resolve hit/miss | +| `resolve_model_missing_name_errors_with_model_tag` (861) | resolve hit/miss | +| `resolve_step_returns_matching_step` (880) | resolve hit/miss | +| `resolve_step_unknown_name_tags_step_error` (901) | resolve hit/miss | +| `resolve_step_first_match_wins` (925) | resolve hit/miss | +| `resolve_prompt_returns_content_for_matching_name` (1368) | resolve hit/miss | +| `resolve_prompt_missing_name_errors_with_prompt_tag` (1387) | resolve hit/miss | +| `resolve_script_hit_returns_content` (1405) | resolve hit/miss | +| `resolve_script_miss_returns_script_tag` (1421) | resolve hit/miss | +| `resolve_script_first_match_wins` (1439) | resolve hit/miss | + +These tests use `crate::config::Model`/`Prompt`/`Script` fully qualified and reach +`Config`/`Step` through `use super::*;` — no extra imports. + +#### 2. `src/commands/mod.rs` — **modify** + +- Add module declaration near the top: `mod resolve;` +- Re-point every helper call site (in the still-resident handlers) with a `resolve::` prefix: + - `artifact_directory_command`: `artifact_dir_path(` → `resolve::artifact_dir_path(` + - `step_command`, `model_command`, `thinking_command`, `prompt_command`, `script_command`: + prefix `artifact_dir_path`, `determine_step`, `resolve_step`, `resolve_model`, + `resolve_prompt`, `resolve_script` calls with `resolve::` +- Delete the six helper definitions. +- Remove the now-unused module-level import `use crate::config::{Config, Step};` (handlers use + only `crate::config::read_config` fully qualified and the returned values). +- In the test module, delete `use crate::config::{Config, Step};` (no remaining `mod.rs` test + uses them); keep `use super::*;` and `use clap::CommandFactory;`. + +Import ledger — `mod.rs` top after this phase: + +```rust +use crate::errors::Error; +use crate::git; +use clap::{Parser, Subcommand}; +use std::path::PathBuf; +``` + +### Verification + +#### Automated +- [x] `cargo check` passes (fast sanity before the full gate) +- [x] `scripts/test.sh` passes +- [x] `cargo nextest run resolve::` passes — all 22 relocated tests +- [x] `cargo nextest run select_command_routes` passes — router arms unchanged +- [x] `cargo clippy --tests -- -D warnings` clean (no unused `Config`/`Step` import left behind) + +#### Manual +- [ ] `cargo run --quiet -- -j branch` output still parses as `{"data":""}` +- [ ] `cargo run --quiet -- --help` matches `baseline/help.txt` (`diff`) + +--- + +## Phase 3: Leaf handlers — `init.rs`, `branch.rs`, `artifact_directory.rs` + +The three least-coupled handlers. `init` is standalone (template write + guard), `branch` is a +one-line git delegate, `artifact_directory` consumes `resolve::artifact_dir_path`. Green tests +prove file-creation/no-overwrite and the composed path shape before any config-driven handler +moves. + +### Changes + +#### 1. `src/commands/init.rs` — **create** + +```rust +use crate::errors::Error; + +/// Create a new `orksorksorks.toml` file with default configuration. +pub(crate) fn init_command(path: &std::path::Path) -> Result { + // body moved verbatim (mod.rs:161-193) +} +``` + +Body keeps `use std::io::Write;` inside the fn, `include_str!("../../templates/default.toml")` +(depth is identical to `mod.rs` — verified in design decision 6), and +`crate::format::green_string(...)`. No test module (no handler-direct init test exists; init is +covered by `select_command_routes_init`, which stays in `mod.rs`, and by +`tests/init_creates_file.rs`). + +#### 2. `src/commands/branch.rs` — **create** + +```rust +use crate::errors::Error; +use crate::git; + +/// Handle the `branch` subcommand: return the current git branch, plain. +pub(crate) fn branch_command() -> Result { + git::current_branch() +} +``` + +No test module. + +#### 3. `src/commands/artifact_directory.rs` — **create** + +```rust +use super::resolve; +use crate::errors::Error; +use crate::git; + +/// Handle the `artifact_directory` subcommand: return +/// `$PWD/.pi/orksorksorks//`, plain (no directory creation). +pub(crate) fn artifact_directory_command() -> Result { + let cwd = std::env::current_dir()?; + Ok(resolve::artifact_dir_path(&cwd, &git::current_branch()?)) +} +``` + +No test module (path-composition unit tests already live in `resolve.rs`). + +#### 4. `src/commands/mod.rs` — **modify** + +- Add `mod init; mod branch; mod artifact_directory;` (with `mod resolve;`). +- Re-point the three router arms in `select_command_with_env`: + - `init_command(&path)` → `init::init_command(&path)` + - `branch_command()` → `branch::branch_command()` + - `artifact_directory_command()` → `artifact_directory::artifact_directory_command()` +- Delete the three handler definitions. +- Imports unchanged this phase (`Error` and `git` are still used by the remaining handlers). + +### Verification + +#### Automated +- [x] `cargo check` passes +- [x] `scripts/test.sh` passes +- [x] `cargo nextest run init_` passes (integration `tests/init_creates_file.rs`, 8 tests) +- [x] `cargo nextest run branch` passes (integration `tests/branch.rs` + routing test) +- [x] `cargo nextest run artifact_` passes (integration `tests/artifact_directory.rs`) + +#### Manual +- [ ] `./target/debug/orksorksorks init --help` matches `baseline/help.txt`-style expectations + (unchanged clap output) +- [ ] `./target/debug/orksorksorks -j step --config /nonexistent/orks.toml` still emits the same + error envelope as `baseline/step-missing.json` + +--- + +## Phase 4: Config-driven handlers — `step.rs`, `model.rs`, `thinking.rs` + +The three handlers sharing the identical "read config → explicit `--step` or derive from +cwd+git+artifacts → resolve" shape. `resolve.rs` must already exist (Phase 2). + +### Changes + +#### 1. `src/commands/step.rs` — **create** + +```rust +use super::resolve; +use crate::errors::Error; +use crate::git; + +/// Handle the `step` subcommand: ... (doc moved verbatim) +pub(crate) fn step_command( + path: &std::path::Path, + source: crate::config_dir::ConfigPathSource, + step: Option, +) -> Result { + let cfg = crate::config::read_config(path, source)?; + let step = if let Some(name) = step { + resolve::resolve_step(&cfg, &name)? + } else { + let cwd = std::env::current_dir()?; + let artifact_dir = resolve::artifact_dir_path(&cwd, &git::current_branch()?); + resolve::determine_step(&cfg, &artifact_dir)? + }; + Ok(step.name) +} + +#[cfg(test)] +mod tests { + use super::*; + // 2 relocated tests (bodies verbatim) +} +``` + +Relocated tests (cut from `mod.rs`): + +| Function (current line) | Asserts | +|---|---| +| `step_command_flag_succeeds_in_non_git_dir` (1239) | `--step` override returns `"one"` with no git repo | +| `step_command_flag_unknown_name_tags_step` (1263) | error `source == "step"`, message contains `no step named "nope"` | + +Both call `crate::config_dir::ConfigPathSource::ExplicitFlag` fully qualified and need no extra +imports beyond `super::*`. + +#### 2. `src/commands/model.rs` — **create** + +Same shape, `pub(crate) fn model_command(path, source, step)`, body moved verbatim from +`mod.rs:301-320`, with `resolve::resolve_step`, `resolve::artifact_dir_path`, +`resolve::determine_step`, `resolve::resolve_model`, `git::current_branch`. Imports: + +```rust +use super::resolve; +use crate::errors::Error; +use crate::git; +``` + +No test module (model is covered by `resolve_model_*` unit tests in `resolve.rs` + +`tests/model.rs` integration; no direct `model_command` unit test exists today). + +#### 3. `src/commands/thinking.rs` — **create** + +Identical shape to `model.rs` (`thinking_command`, moved verbatim from `mod.rs:322-339`; +returns `model.thinking`). Same three imports. No test module. + +#### 4. `src/commands/mod.rs` — **modify** + +- Add `mod step; mod model; mod thinking;`. +- Re-point the three arms: `step::step_command(&path, source, step.clone())`, + `model::model_command(&path, source, step.clone())`, + `thinking::thinking_command(&path, source, step.clone())`. +- Delete `step_command`, `model_command`, `thinking_command`. +- Delete the two `step_command_*` tests from the test module. +- Imports unchanged (`git` still used by prompt/script handlers; `Error` by the router). + +### Verification + +#### Automated +- [x] `cargo check` passes +- [x] `scripts/test.sh` passes +- [x] `cargo nextest run step` passes (integration `tests/step.rs` + 2 relocated unit tests) +- [x] `cargo nextest run model` passes (integration `tests/model.rs`) +- [x] `cargo nextest run thinking` passes (integration `tests/model.rs` thinking block) + +#### Manual +- [ ] `./target/debug/orksorksorks step --help` matches `baseline/step-help.txt` +- [ ] `./target/debug/orksorksorks -j step --config /nonexistent/orks.toml` matches + `baseline/step-missing.json`; text mode matches `baseline/step-missing.txt` + +--- + +## Phase 5: Frontmatter/script handlers — `prompt.rs`, `script.rs` + +The two remaining handler families, with the largest behavioral pins (frontmatter emission, +positional `script `, `show_frontmatter = false`). Moving them last exercises those +pins against an otherwise-finished split. + +### Changes + +#### 1. `src/commands/prompt.rs` — **create** + +```rust +use super::resolve; +use crate::errors::Error; +use crate::git; + +/// Handle the `prompt` subcommand: ... (doc moved verbatim) +pub(crate) fn prompt_command( + path: &std::path::Path, + source: crate::config_dir::ConfigPathSource, + step: Option, +) -> Result { + // body moved verbatim from mod.rs:372-409 +} +``` + +The `format!("## Important variables\n...")` frontmatter string must be preserved +byte-for-byte; only the helper calls gain `resolve::`. + +Relocated tests (cut from `mod.rs`): + +| Function (current line) | Asserts | +|---|---| +| `prompt_command_step_flag_unknown_name_tags_step` (1285) | `--step nope` → `source == "step"` | +| `prompt_command_step_flag_known_step_missing_prompt_tags_validation` (1309) | `config:missing-prompt` | + +#### 2. `src/commands/script.rs` — **create** + +```rust +use super::resolve; +use crate::errors::Error; +use crate::git; + +/// Handle the `script` subcommand: ... (doc moved verbatim) +pub(crate) fn script_command( + path: &std::path::Path, + source: crate::config_dir::ConfigPathSource, + step_name: Option, +) -> Result { + // body moved verbatim from mod.rs:411-434 +} +``` + +Preserve the `"script"` tag for no-script/unknown-name and the no-frontmatter contract. + +Relocated test (cut from `mod.rs`): + +| Function (current line) | Asserts | +|---|---| +| `script_command_step_without_script_errors` (1336) | `source == "script"`, message contains `has no script configured` | + +#### 3. `src/commands/mod.rs` — **modify** + +- Add `mod prompt; mod script;`. +- Re-point the final two arms: `prompt::prompt_command(&path, source, step.clone())`, + `script::script_command(&path, source, step_name.clone())`. +- Delete `prompt_command`, `script_command` and the three relocated tests. +- Remove the now-unused module-level `use crate::git;`. + +`mod.rs` top after this phase: + +```rust +use crate::errors::Error; +use clap::{Parser, Subcommand}; +use std::path::PathBuf; +``` + +The root now holds only `NAME`/`AUTHOR`/`ABOUT`/`LONG_VERSION`, `Cli`, `Commands`, +`select_command_with_env`, `select_command`, the nine `mod` declarations, and the parse/routing +tests. + +### Verification + +#### Automated +- [x] `cargo check` passes +- [x] `scripts/test.sh` passes +- [x] `cargo nextest run prompt` passes (integration `tests/prompt.rs` + 2 relocated unit tests) +- [x] `cargo nextest run script` passes (integration `tests/script.rs` + 1 relocated unit test) +- [x] `cargo nextest run json_output` passes (envelope + no-ANSI) +- [x] `cargo clippy --tests -- -D warnings` clean (no unused `crate::git` import) + +#### Manual +- [ ] `./target/debug/orksorksorks script --help` matches `baseline/script-help.txt` +- [ ] `./target/debug/orksorksorks -j branch` matches `baseline/branch.json` + +--- + +## Phase 6: Final verification sweep (no new behavior) + +Proves the end state matches the design: root file small and readable, nothing outside +`src/commands/` touched, architecture graph unchanged. + +### Changes + +None functional. Touch `mod.rs` only if a leftover unused import or dead item makes the gate +fail (e.g. removing a stray `use`). + +### Verification + +#### Automated +- [x] `scripts/test.sh` passes (fmt, check, clippy `-D warnings`, nextest, forbidden strings) +- [x] `cargo nextest run architecture` passes — `tests/architecture.rs` untouched +- [x] `git status --short` shows changes only under `src/commands/` (plus the untracked + `baseline/` artifact dir) +- [x] `git diff --stat` confirms `tests/` and `src/main.rs` untouched +- [x] `wc -l src/commands/*.rs` — every file under 300 lines **except** `mod.rs` (~600) and + `resolve.rs` (~560); see "Corrected acceptance criteria" +- [x] Baseline diffs are empty: + ```bash + ./target/debug/orksorksorks --help > /tmp/ork-help.txt 2>&1 + diff .pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/baseline/help.txt /tmp/ork-help.txt + ./target/debug/orksorksorks -j branch > /tmp/ork-branch.txt 2>&1 + diff .pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/baseline/branch.json /tmp/ork-branch.txt + ./target/debug/orksorksorks -j step --config /nonexistent/orks.toml > /tmp/ork-step.json 2>&1 + diff .pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/baseline/step-missing.json /tmp/ork-step.json + ``` +- [x] `rg -n 'TODO:|todo:|FIXME|fixme|dbg!|DEBUG:|FIXTURE:' src/commands/` matches nothing + (also enforced by `scripts/test.sh`) + +#### Manual +- [ ] Per-file diff review: each new file's body is character-identical to its original + `mod.rs` region apart from `pub(crate)`, the `resolve::` prefixes, and the trailing test + module relocation. Use `git diff` and the baseline captures to confirm no silent change. +- [ ] Confirm the routing arms read cleanly in `mod.rs` and each namespaced call matches its + file (e.g. `prompt::prompt_command`), i.e. no typo slipped past the integration tests. + +--- + +## Import ledger (per stage, `src/commands/mod.rs`) + +| Phase | `crate::config::{Config, Step}` | `crate::errors::Error` | `crate::git` | `clap` | `std::path::PathBuf` | +|---|---|---|---|---|---| +| Start | yes | yes | yes | yes | yes | +| 2 | **remove** | yes | yes | yes | yes | +| 3 | – | yes | yes | yes | yes | +| 4 | – | yes | yes | yes | yes | +| 5 | – | yes | **remove** | yes | yes | + +`mod resolve;` is added in Phase 2 and stays forever (siblings reach it via `super::resolve`); +it is a declaration, not an import, so it never warns. + +## New-file import summary + +| File | Non-test imports | +|---|---| +| `resolve.rs` | `crate::config::{Config, Step}`, `crate::errors::Error` | +| `init.rs` | `crate::errors::Error` | +| `branch.rs` | `crate::errors::Error`, `crate::git` | +| `artifact_directory.rs` | `super::resolve`, `crate::errors::Error`, `crate::git` | +| `step.rs` | `super::resolve`, `crate::errors::Error`, `crate::git` | +| `model.rs` | `super::resolve`, `crate::errors::Error`, `crate::git` | +| `thinking.rs` | `super::resolve`, `crate::errors::Error`, `crate::git` | +| `prompt.rs` | `super::resolve`, `crate::errors::Error`, `crate::git` | +| `script.rs` | `super::resolve`, `crate::errors::Error`, `crate::git` | + +All `crate::` spellings are already in the rust_arkitect allowlist (`crate::config`, +`crate::errors`, `crate::git`); `super::resolve` is an intra-`commands` path and records no +external edge. No `tests/architecture.rs` change is required. + +## Deviations from `structure.md` (recorded) + +1. **`resolve.rs` does not import `crate::git`** (structure Stage 2 lists it). None of the six + helpers calls git — the git calls live in the handler files. +2. **`resolve.rs` does not touch `crate::format`.** Only `init_command` uses `green_string`. +3. **No empty `#[cfg(test)] mod tests`.** `init.rs`, `branch.rs`, `artifact_directory.rs`, + `model.rs`, `thinking.rs` have no relocated handler-direct unit tests (none exist today — + those behaviors are pinned by routing tests in `mod.rs` and by integration tests). Adding + empty test modules would be dead code. `step.rs`, `prompt.rs`, `script.rs`, and `resolve.rs` + carry real trailing test modules. +4. **Stage 6 line-count criterion corrected** from "all files < 300" to the table in + "Corrected acceptance criteria": `mod.rs` and `resolve.rs` land at ~600/~560 under the + design's mandated grouping. If strict < 300 is required, re-run `structure`/`design` with + parse tests or the resolve test group split out — this plan does not do that. +5. **Phase 4 header** in structure says step/model/thinking all have relocated handler tests; + only `step` does (`model`/`thinking` coverage is `resolve_*` unit tests + `tests/model.rs`). + +## Resume rule + +Every phase produces a parent-consistent, compiling tree. If a stage's gate fails, fix within +that stage; the phases above it are already independently valuable and can land on their own. \ No newline at end of file diff --git a/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/questions.md b/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/questions.md new file mode 100644 index 0000000..5b0735e --- /dev/null +++ b/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/questions.md @@ -0,0 +1,17 @@ +# Research Questions + +## Context + +The crate is a small Rust CLI (`src/main.rs` declares six flat modules: `commands`, `config`, `config_dir`, `errors`, `format`, `git`). `src/commands/mod.rs` is the crate's largest file: it holds the clap parser structs, the subcommand enum, the routing match, every command handler, shared resolution helpers, and a large inline test module. The questions below map the internal structure of that file, the contracts that bind it to the rest of the crate, the conventions used by its sibling modules, the test surface that pins observable behavior, and the error/output flow that connects handlers to stdout. + +## Questions + +1. **Internal structure of `src/commands/mod.rs`**: Inventory the file top to bottom — the `Cli` struct with all its clap attributes, the `Commands` enum with every variant and its fields, the routing function(s) and every match arm, each command handler (signature, arguments, return type, what it does), each shared helper (signature and call sites), and the test module's shape. Which symbols are `pub` vs private, and what internal dependencies exist between items in the file (which handlers call which helpers)? + +2. **Cross-module contracts**: What does `src/main.rs` require from the `commands` module — every type, function, or field it uses and their visibility? What does `src/commands/mod.rs` use from the other five modules (every import and usage)? What exactly does `tests/architecture.rs` (rust_arkitect) pin: the dependency rule, the import allowlist for `commands`, and the command surface (build rules, how it's run)? How would each of these contracts be expressed if code moved between files within the `commands` module? + +3. **Module organization conventions**: For each sibling top-level module (`src/config.rs`, `src/config_dir.rs`, `src/errors.rs`, `src/format.rs`, `src/git.rs`), describe its internal organization: declaration order, `pub` items vs private, how it returns/uses `crate::errors::Error`, doc-comment conventions, and how its `#[cfg(test)] mod tests` is arranged. Are there any nested modules, subdirectories, or multi-file module trees anywhere in the repo (src/, tests/, build.rs, or scripts) that establish a submodule pattern? What naming conventions apply to modules, functions, and types? + +4. **Test surface pinning CLI behavior**: Inventory every test that pins observable behavior. For each of the 9 files in `tests/` (e.g. `tests/init_creates_file.rs`, `tests/json_output.rs`), say what it exercises, how it invokes the binary, and which exact outputs it asserts (text, JSON envelope fields, ANSI handling, exit codes, error messages). For the inline `mod tests` in `src/commands/mod.rs`, group the unit tests by what behavior they cover (parse routes, handler logic, helper resolution). Describe the `tests/architecture.rs` rules precisely. What behaviors would a reader use to verify an output-preserving change? + +5. **Error and output flow**: Trace how a command handler's result becomes terminal output. How does `src/main.rs` route `CommandResult` through the text/JSON/result output paths, and what does each emit? How do handlers produce errors — what does `src/errors.rs` define, how are errors constructed in handlers, and how does an error reach the user (message format, JSON shape, exit behavior)? Where does color/ANSI get applied, and what is the `apply_color` chokepoint's contract? \ No newline at end of file diff --git a/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/research.md b/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/research.md new file mode 100644 index 0000000..142b17c --- /dev/null +++ b/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/research.md @@ -0,0 +1,79 @@ +# Research Findings + +Crate: `orksorksorks` — binary-only Rust CLI (`edition 2024`), six modules declared in `src/main.rs:8-13`: `commands` (directory module), `config`, `config_dir`, `errors`, `format`, `git` (single flat files). `src/commands/mod.rs` is ~1,460 lines (the crate's only multi-hundred-line file, ~half of the crate's ~2,884). + +## Q1: Internal structure of `src/commands/mod.rs` + +### Findings +- **Imports/constants** (`src/commands/mod.rs:1-18`): imports `crate::config::{Config, Step}` (`:1`), `crate::errors::Error` (`:2`), `crate::git` (`:3`), `clap::{Parser, Subcommand}` (`:4`), `std::path::PathBuf` (`:5`); `const NAME/AUTHOR/ABOUT` from `env!` (`:7-9`), `LONG_VERSION` via `concat!` (`:10-18`) — consumed only by the clap `#[command(...)]` attributes on `Cli` (`:22-28`). +- **`pub struct Cli`** (`:21-38`): `#[derive(Parser, Clone)]` (`:21`), `#[command(name, author, version, about, long_about=None)]` (`:22-28`), `arg_required_else_help` (`:29`); `pub json: bool` — `#[arg(short='j', long, global=true, default_value_t=false)]` (`:32-34`); `pub command: Commands` — `#[command(subcommand)]` (`:36-37`). +- **`pub enum Commands`** (`:40-117`): `#[derive(Subcommand, Debug, Clone)]` (`:40`). Variants: `Init { config: Option }` `-c/--config` (`:43-48`); `Branch` unit (`:51`); `ArtifactDirectory` unit with `#[command(name="artifact_directory")]` (`:54-56`); `Step { config, step }` (`:59-69`); `Model` (`:72-82`), `Thinking` (`:85-95`), `Prompt` (`:98-108`) all with `--config`/`--step`; `Script { step_name: Option positional STEP_NAME, config }` (`:111-116`). Registration is purely clap-derive attributes; no manual registry. +- **Routing**: private `select_command_with_env(cli, env: &ConfigEnv)` (`:119`) resolves the config path for every config-bearing variant via `config_dir::config_file_path_with_env(config.as_deref(), env)?` (`:122-143`); arms: `Init`→`init_command(&path)` (`:123-125`), `Branch`→`branch_command()` (`:126`), `ArtifactDirectory`→`artifact_directory_command()` (`:127`), `Step`→`step_command(...)` (`:128-130`), `Model` (`:131-133`), `Thinking` (`:134-136`), `Prompt` (`:137-139`), `Script` (`:140-143`). Public `select_command(cli: &Cli)` (`:156`) delegates with `ConfigEnv::from_env()` (`:157`). +- **Handlers** (all private, all `Result`): `init_command` (`:161`) — `create_dir_all`, `create_new(true)`, `AlreadyExists`→`Error::new("config-exists", ...)` (`:171-176`), writes `include_str!("../../templates/default.toml")` + flush/sync (`:177-179`), returns `format::green_string("✓ Created ")` (`:180-183`); `branch_command` (`:204`) — `git::current_branch()`; `artifact_directory_command` (`:210`) — cwd + branch → `artifact_dir_path`; `step_command` (`:281`), `model_command` (`:301`), `thinking_command` (`:322`), `prompt_command` (`:372`), `script_command` (`:411`) — all read config → resolve step (by `--step` name or derived from cwd+git+artifact dir) → resolve model/prompt/script; `prompt_command` prepends `## Important variables` frontmatter unless `show_frontmatter=false` (`:406-419`); `script_command` errors `"script"` when step has no script (`:430-434`). +- **Shared helpers** (all private): `artifact_dir_path(cwd, branch)` (`:195`) — pure `"{cwd}/.pi/orksorksorks/{branch}/"`, `/`→`-`, trailing slash; `determine_step(config, artifact_dir)` (`:224`) — reverse-iterates, empty-trigger default fallback, last forward match wins, `"step"` error on no match (`:242-245`); `resolve_model` (`:251`, `"model"` err `:258`); `resolve_step` (`:263`, `"step"` err `:271`); `resolve_prompt` (`:341`, `"prompt"` err `:348`); `resolve_script` (`:352`, `"script"` err `:359`). +- **`#[cfg(test)] mod tests`** (`:437-1460`, single flat inline module, `use super::*` + `crate::config` + `CommandFactory` `:439-441`): (~1,000 lines) groups — `cli_try_parse_*` parse pins incl. rejecting no-subcommand (`:462`) and kebab `artifact-directory` (`:529`); `select_command_routes_*` routing dispatch proved via missing-config → `"io"` (`:444`, `:777`, `:952`, `:969`, `:1199`, `:1219`); `cli_command_debug_assert` (`:514`); `artifact_dir_path_*` (`:536-568`); `determine_step_*` default/priority (`:570-833`); `resolve_*` hit/miss/error-tag tests (`:840-1460`); handler-direct tests (`:1239-1420`). Uses `tempfile::tempdir()`, `pretty_assertions`, asserts `err.source`/`err.message`. +- **Visibility**: `pub` only `Cli` (+ `pub` fields `json`/`command`), `Commands`, `select_command`. All handlers, helpers, `select_command_with_env`, constants are private. Call graph: routing → handlers → helpers, entirely intra-file. + +## Q2: Cross-module contracts + +### Findings +- **main.rs's contract with `commands`**: `mod commands;` (`src/main.rs:8`); `commands::Cli` type (`src/main.rs:62,83`); `cli.json` field read (`src/main.rs:63`); `commands::select_command(&cli)` (`src/main.rs:64`); `Cli::parse()` via `use clap::Parser` (`src/main.rs:15`). Exactly two public items + one public field — everything else in `mod.rs` is crate-private. +- **`commands`'s imports from siblings** (`src/commands/mod.rs`): `config` — `Config`/`Step` types (`:1`), `read_config` at `:286,:306,:327,:377,:416`, `Model` in `resolve_model` (`:251`); `config_dir` — `ConfigEnv` (`:119,:157`), `config_file_path_with_env` (`:122-149`), `ConfigPathSource` (`:283,:303,:324,:374,:413`); `errors` — `Error` (`:2`), `Error::new` at `:163-432`; `format` — `green_string` (`:184`); `git` — `current_branch` (`:205,:212,:291,:311,:332,:384,:397,:425`). Plus `include_str!("../../templates/default.toml")` (`:181`). +- **`tests/architecture.rs` (rust_arkitect)**: two tests call `assert_complies` (`:33`) → `Arkitect::ensure_that(Project::from_current_crate())` (`:35`). `commands_imports_only_downward_modules` (`:51`) allowlists `orksorksorks::commands` depending only on the 5 sibling modules in **both** `crate::` and `orksorksorks::` spellings (10 entries, `:56-75`) plus `clap`, `std::path`, `std::io`, `std::fs`, `std::env`, `tempfile`, `pretty_assertions` — deps recorded "exactly as written" (`:21-25`). `no_upward_imports_or_cycles` (`:89`) enforces total order `main < commands < config < config_dir < git < errors < format` (`:86`) with `it_must_not_depend_on` blocks (`:92-140`). +- **If code moves within `commands`**: rust_arkitect attributes every file under `src/commands/` to logical module `orksorksorks::commands` (doc `:21-25`), so the external edge set is unchanged; intra-`commands` `use super::...` paths are not external edges. Any new `use crate::...` spelling still must match the allowlist (the existing 10 spellings suffice for the 5 sibling modules). main.rs keeps working if `Cli`, `Commands`, `select_command` remain `pub` in the `commands` root (no re-export needed) — otherwise main needs a deeper path (`src/main.rs:62-64,83`). Every handler→helper call currently bare-name (`:286-428`) becomes a path/`use` + visibility change. + +## Q3: Module organization conventions + +### Findings +- **Only directory module**: `mod commands;` resolves to `src/commands/mod.rs`; the other five are flat single files (`src/main.rs:8-13`). No `pub mod`, no non-test inline `mod`, no further nesting anywhere in the repo (only the six modules + five `#[cfg(test)] mod tests`). +- **Per-module shape** (all follow: imports → items → helpers → trailing `mod tests`): + - `src/config.rs` (487 lines): structs `Step`/`Model`/`Prompt`/`Script` (`:9-57`) → root `Config` (`:60`) → `impl Default` (`:87`) → `pub fn read_config` (`:107`) → `pub const CONFIG_VERSION` (`:128`) → `impl Config { pub(crate) fn validate }` (`:130-131`) → private helpers (`:236,:247`) → `mod tests` (`:254`). Leaf structs `pub` with `pub` fields. + - `src/config_dir.rs` (167 lines): only sibling with a `//!` module doc (`:1`); `pub(crate) struct ConfigEnv` with `pub` fields for test construction (`:17`) + `from_env` (`:26`); `pub enum ConfigPathSource` (`:45`) + `Display` (`:56`); `pub(crate) fn resolve_config_dir_with_env` (`:72`), `pub(crate) fn config_file_path_with_env` (`:106`); `mod tests` (`:116`). + - `src/errors.rs` (93 lines): `pub struct Error { pub message, pub source }` (`:17`) → `Error::new` (`:22-31`) → `Display` (colors via `yellow_string`/`red_string`, `:36-39`) → `std::error::Error` (`:44`) → three `From` impls (`:46-71`) → `mod tests` (`:73`). + - `src/format.rs` (49 lines): private `apply_color` (`:6-12`, `cfg!(test)` chokepoint) → three thin pub wrappers (`:14,:18,:22`) → `mod tests` (`:26`). No `Error` usage. + - `src/git.rs` (77 lines): `pub fn current_branch` (`:8`) delegates to `pub fn current_branch_in(dir)` (`:17`) (injectable-dir testability); private `parse_branch_output` (`:28`); `mod tests` (`:39`). +- **Visibility idiom**: `pub` for crate-surface types/fns; `pub(crate)` for cross-module internal items (`Config::validate` config.rs:131; all config_dir fns/struct); private for file-local helpers. Functions are snake_case imperative verbs (`read_config`, `current_branch`, `green_string`); `_in`-suffixed twin takes explicit dir (`current_branch_in` git.rs:17); consts SCREAMING_SNAKE (`CONFIG_VERSION` config.rs:128, `FILE_NAME` config_dir.rs:9). +- **Doc convention**: `#![warn(missing_docs)]` (`src/main.rs:6`) — `///` on every `pub` AND `pub(crate)` item (verified: config_dir.rs:8-17, :24-26, :67-72, :101-106); free fns document behavior + error-tag contract (git.rs:3-6, config.rs:100-106); struct fields each get `///` (config.rs:10-19). Tests assert `err.source` tags and message substrings (config.rs:457-460). +- **Error idiom**: every fallible fn returns `Result`; errors built with `Error::new(tag, message)`, lowercase tags, `"config:*"` namespacing (`config.rs:134-231`); `read_config` maps read failure to `"io"` embedding path + resolution source (`config.rs:111-118`). + +## Q4: Test surface pinning CLI behavior + +### Findings +- **Integration tests** (`tests/`, 10 files) all spawn the real binary: `assert_cmd::Command::cargo_bin("orksorksorks")` (tests/init_creates_file.rs:17) in disposable tempdirs, asserting exit status, raw stdout/stderr, and serde_json-parsed envelopes. + - `init_creates_file.rs` (8 tests): default-content byte-equality with `include_str!("../templates/default.toml")` (`:25-30`); "✓ Created {path}" substring (`:38-42`); readonly-dir failure (`:48`); `-j` errors carry `"source":"io"` (`:76`) / `"config-exists"` (`:118,:149`) / `"config-dir"` (`:171`); `--config` overrides hostile XDG (`:92-107`); no-overwrite guard (`:111-126`). + - `json_output.rs`: `-j` envelope has `"data"` + parses as JSON (`:4-20`); plain text has no `\x1b` (`:25-42`). + - `branch.rs`: stdout == branch name, no ANSI (`:36-47`); `-j` data (`:49-64`); outside repo fails (`:64`). + - `artifact_directory.rs`: canonicalized (macOS `/var`→`/private/var`, `:40-42`) `{cwd}/.pi/orksorksorks/{branch}/` trailing slash, no ANSI, text + `-j` (`:46-84`). + - `config_validation.rs`: table of 8 exact (stderr phrase / `error.source`) pairs e.g. `"unsupported config version"`/`config:version` (`:56`), `"duplicate name"`/`config:duplicate-name` (`:65`), … `"references unknown model"`/`config:missing-model` (`:155`); asserts text-mode stderr phrase + no ANSI, `-j` mode valid JSON with tag (`:22-54`); valid config → `"one"` (`:168-193`). + - `step.rs`: artifact-derived step name (`:55,:78`), default-step fallback (`:113`), real-artifact-beats-default (`:157`), XDG resolution (`:207`), cwd-config ignored regression guard (`:251`), missing-config path spelled in stderr (`:269,:301`), `--step` works git-less/detached HEAD (`:365,:393`), unknown step: text→stderr/stdout-empty, `-j`→`{"error":{"source":"step",...}}` (`:448-473`). + - `model.rs`/`thinking`: pins exact model strings (`openrouter/deepseek/pro` `:57`, `.../flash` `:81`), `-j` data, XDG, `--step` overrides (`:303,:330`); thinking twins pin "high" (`:186-262`). + - `prompt.rs`: frontmatter exact content `"## Important variables\nThese are literal text values, not shell or\nenvironment variables."` (`:148-162`); full frontmatter lines `step = one\n` / `branch = main\n` / `artifact_directory = {canonical}/.pi/.../main/\n` (`:279-312`); `show_frontmatter=false` hides them (`:325`); positional prompt rejected (exit 2, `:435`). + - `script.rs`: no frontmatter ever (`:117,:135,:165-191,:282`); positional `script one` override (`:142`); `"script"` errors for unknown name and step-without-script (`:193,:238`); XDG (`:266`). +- **`tests/architecture.rs`**: rules as in Q2 — dependency allowlist (`:51-78`) + module-order cycle ban (`:89-140`). +- **Inline unit tests** (`src/commands/mod.rs:437-1460`): parse routes, routing arms, handler logic, helper resolution — all asserting exact strings and `Error` tag/message pairs (details in Q1). `format.rs:29-47` pins ANSI-stripping under `cfg!(test)`; `errors.rs:78-85` pins plain text `"Error from "` format. +- **Behavior-preservation checklist** (what a reader verifies): exit codes (0/1/2), exact text output and `-j` JSON envelope (`data`/`error.message`/`error.source`), absence of `\x1b` in test captures, stderr phrases + path spellings, file byte-equality with template, clap help/parse acceptance (incl. `arg_required_else_help`), and the architecture import graph. All asserted unchanged with no golden files (only `include_str!` template comparison). + +## Q5: Error and output flow + +### Findings +- **Handler → terminal flow** (`src/main.rs`): `main` parses `Cli` (`:93`) → `run_command(cli)` (`:62-79`): snapshot `cli.json` (`:63`), `commands::select_command(&cli)` (`:64`) → wrap in `CommandResult { result, json }` (`:19-24, :65-74`) → `output_result` (`:53-59`) → exit 1 if `result.is_err()` (`:76-77`). +- **`output_text`** (`:27-35`): Ok → `println!("{data}")` stdout; Err → `eprintln!("\n\n{e}")` stderr with two leading blank lines (rendered via `Error`'s `Display`). +- **`output_json`** (`:39-49`): Ok → `{"data": data}` to stdout; Err → `{"error": {"message", "source"}}` to **stdout** (JSON errors go to stdout, text errors to stderr; both exit 1). +- **`src/errors.rs`**: `pub struct Error { pub message, pub source }` (`:17`), `Debug, Clone, PartialEq, Eq, Serialize` (`:15`); `Error::new` (`:25-31`); `Display` is the text rendering contract: `Error from {yellow source}:\n{red message}` (`:33-41`), doc states Display owns all coloring (`:5-8`); `From`→`"io"`, `From`→`"toml::ser"`/`"toml::de"` (`:46-71`). +- **Error construction sites**: routing `?`-propagates (`mod.rs:122-143`); handlers: `"config-exists"` (`:171-176`), `"io"` via `From` (`:178`), `"step"` (`:242-245,:269`), `"model"` (`:257`), `"prompt"` (`:347`), `"script"` (`:358,:430-434`); git: `"git"` for nonzero/empty output, `"io"` for spawn failure (`git.rs:30-37,:7`); config: `"io"`/`"toml::de"` (`config.rs:111-133`), `"config:*"` from `validate` (`config.rs:134-231`); config_dir: `"config-dir"` (`config_dir.rs:95-105`). +- **Color/ANSI contract** (`src/format.rs`): `apply_color` (`:6-12`) is the only chokepoint — `cfg!(test)` → plain, else `colored`'s `Colorize`; `green_string`/`red_string`/`yellow_string` (`:14-22`) delegate. Only two callers in the crate: `Error`'s `Display` (errors.rs:38-39) and `init_command`'s success `✓` (mod.rs:180-183). **Empirically verified** (built and ran the binary with piped stdout): the `colored` crate strips ANSI on non-tty output, so integration-test children emit no `\x1b`; the `cfg!(test)` branch only affects in-process unit tests. Interactive usage is where color appears. + +## Cross-Cutting Observations + +- **File identity**: `commands` is the crate's only directory module; its `mod.rs` mixes parser structs, routing, handlers, and helpers with a ~1,000-line test module — the four concerns have clean internal boundaries (enum → routing → handlers → helpers → tests) visible as natural seams. +- **Minimal escape surface**: only `Cli`, `Commands`, `select_command` leave `commands`; `main.rs` reads exactly `Cli.json` and calls `select_command`. Any reorganization that keeps these three `pub` in the `commands` root is invisible to `main.rs`. +- **rust_arkitect stability**: submodule files under `src/commands/` inherit the `orksorksorks::commands` logical identity, so the dependency edges don't change; new `use crate::...` spellings must match the dual-spelling allowlist (tests/architecture.rs:56-75), and the sibling five + clap/std/test deps cover everything `commands` currently imports. +- **Convention spine to inherit** (from Q3): trailing `mod tests`, `use super::*;` in tests, `pub`/`pub(crate)`/private visibility tiers, `///` docs on every `pub`/`pub(crate)` item (`#![warn(missing_docs)]` main.rs:6), `Error::new(tag, message)` lowercase-tag errors, `_in`-suffixed injectable-dir functions for testability, behavior-sentence test names. +- **Escape hatches handled**: `include_str!("../../templates/default.toml")` (mod.rs:181) resolves relative to the **source file** — deepening the path (moving the call into a submodule file) changes the relative depth and the same literal would point at `src/templates/...` (needs one more `../` per directory level). The clap `#[command(...)]` attributes need `NAME/AUTHOR/ABOUT/LONG_VERSION` (mod.rs:7-18) in scope where `Cli` is defined. +- **Behavior surface is fully pinned**: integration tests assert every observable contract (exit codes, exact strings, JSON envelope, no-ANSI, template bytes, clap parse rejection), so an output-preserving split is verifiable without goldens; the inline test module pins one level down (handler strings, helper results, error tag/message pairs). + +## Open Areas + +- `src/config.rs` was reported as 487 lines by the pattern-finder while the locator said 809 — the pattern-finder's figure was likely taken from a trimmed read; the exact length is immaterial to the questions (structure is what matters). +- Whether the `colored` crate's non-tty stripping is version-dependent (3.1.1 currently) — the empirical check (this session) matched the tests' expectations; no `NO_COLOR` handling was found in the crate. +- Git history of `src/commands/` (whether it ever had submodules) was not examined — no trace remains in the current tree. \ No newline at end of file diff --git a/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/structure.md b/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/structure.md new file mode 100644 index 0000000..df78132 --- /dev/null +++ b/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/structure.md @@ -0,0 +1,183 @@ +# Structure Outline + +## Approach + +Behavior-preserving, mechanical file split of `src/commands/mod.rs` (~1,460 lines) into one +file per concern under `src/commands/`. `Cli`, `Commands`, `select_command_with_env`, +`select_command` and the parse/routing tests stay in `mod.rs`; the shared resolution helpers move +to `resolve.rs`; each handler family moves to its own file. Only change to logic: promoted +visibility (`pub(crate)`) on moved items and updated `use`/import hygiene. Every stage ends with the +same green gate, because the existing 10 integration files + `tests/architecture.rs` are the +verification harness and must pass **unedited** at every stage. Because this is a *move*, the "tests +that accompany the code" are the existing unit tests travelling with their subject plus the +integration suite; each stage keeps them green (moved unit tests re-point `use super::*;` at +`crate::commands::{Cli, Commands}` where needed). + +**Cross-cutting note (horizontal-build exception):** the router in `mod.rs` cannot be stubbed out — +it must be re-pointed at the moved path (`init::init_command`) in the *same* stage the callee moves, +or the tree stops compiling. So the router's *arm spellings* change incrementally (one line per +stage), while the router's *body* (match arms, `config_file_path_with_env` resolution) is only +finalized as the last handler leaves. Each stage re-verifies the routing tests, so a typo is caught +immediately rather than at the end. + +--- + +## Stage 1: Baseline freeze (no code) + +Record the pre-move reference state so any un-pinned behavior drift is detectable. The design flags +"no golden files" as the top verification risk; this stage buys the cheap safety net. + +**Files**: none modified. Optional captures under `.pi/orksorksorks//` (help text, a few +JSON envelopes) are artifacts, not sources. +**Key changes**: none. + +**Tests**: none new. `scripts/test.sh` on the pristine tree. +**Verify**: `scripts/test.sh` green. Capture `cargo run -- --help`, `cargo run -- -j step` +(valid + invalid config) stdout bytes for later diffing. Also record +`wc -l src/commands/mod.rs` and the test counts (nextest summary) as the line-count/coverage +baseline. + +--- + +## Stage 2: Shared resolution layer — `commands/resolve.rs` (bottom-most) + +Extracts the mutually-recursive helper tier every config-driven handler calls. Its green unit tests +prove artifact-path composition, default/priority step derivation, and all six `resolve_*` +hit/miss/error-tag contracts — the foundation Stages 4–5 consume. + +**Files**: new `src/commands/resolve.rs`; modified `src/commands/mod.rs`. +**Key changes**: +- `pub(crate) fn artifact_dir_path(cwd: &std::path::Path, branch: &str) -> String` — moved, now `pub(crate)` +- `pub(crate) fn determine_step(config: &Config, artifact_dir: &str) -> Result` — moved +- `pub(crate) fn resolve_model(config: &Config, name: &str) -> Result` — moved +- `pub(crate) fn resolve_step(config: &Config, name: &str) -> Result` — moved +- `pub(crate) fn resolve_prompt(config: &Config, name: &str) -> Result` — moved +- `pub(crate) fn resolve_script(config: &Config, name: &str) -> Result` — moved +- `mod.rs`: add private `mod resolve;`, call sites become `resolve::artifact_dir_path(...)` etc.; + drop helpers from the root; prune now-unused root imports (`Config`, `Step` stay only if the + router still needs them — it does not); each moved item keeps its `///` doc verbatim + (`missing_docs` + `clippy -D warnings`). +- `resolve.rs` imports: `crate::config::{Config, Step}`, `crate::errors::Error`, `crate::git`; + trailing `#[cfg(test)] mod tests` with `use super::*;`, `tempfile`, `pretty_assertions`. + +**Tests**: relocated `artifact_dir_path_*`, `determine_step_*` (default fallback, reverse priority, +no-match `"step"` error), and `resolve_model_*`/`resolve_step_*`/`resolve_prompt_*`/`resolve_script_*` +(hit, miss, exact `Error { source, message }` pair) — happy + sad paths both already present. +**Verify**: `cargo check` after the move, then `scripts/test.sh` green; fast loop +`cargo nextest run resolve::`. Router tests (`select_command_routes_*`) must still pass — the arms +now path through `resolve::` for step derivation. + +--- + +## Stage 3: Leaf handler families — `init.rs`, `branch.rs`, `artifact_directory.rs` + +The three handlers with the least coupling: `init` is standalone (template write + `config-exists` +guard), `branch` is a one-line git delegate, `artifact_directory` consumes `resolve::artifact_dir_path`. +Green tests prove the file-creation/no-overwrite contract and the composed path shape before any +config-driven handler moves. + +**Files**: new `src/commands/init.rs`, `src/commands/branch.rs`, +`src/commands/artifact_directory.rs`; modified `src/commands/mod.rs`. +**Key changes**: +- `pub(crate) fn init_command(path: &std::path::Path) -> Result` — moved; keeps + `include_str!("../../templates/default.toml")` byte-identical (same directory depth as today — + verified in design decision 6) and `crate::format::green_string`. +- `pub(crate) fn branch_command() -> Result` — moved; `git::current_branch()`. +- `pub(crate) fn artifact_directory_command() -> Result` — moved; calls + `resolve::artifact_dir_path`. +- `mod.rs`: add `mod init; mod branch; mod artifact_directory;`; arms become + `init::init_command(&path)`, `branch::branch_command()`, + `artifact_directory::artifact_directory_command()`. + +**Tests**: handler-direct unit tests move with each file (init success message, `config-exists` +tag, `artifact_dir_path` composition); integration `tests/init_creates_file.rs` (8 tests: template +byte-equality, readonly failure, `io`/`config-exists`/`config-dir` JSON sources, `--config` +override, no-overwrite) and `tests/branch.rs`, `tests/artifact_directory.rs` (path shape, macOS +canonicalization, no ANSI, `-j`) — **unedited**. +**Verify**: `scripts/test.sh` green; `cargo nextest run init_ branch_ artifact_`. + +--- + +## Stage 4: Config-driven step/model/thinking — `step.rs`, `model.rs`, `thinking.rs` + +The three handlers that share the identical "read config → explicit `--step` or derive from +cwd+git+artifacts → resolve" shape. Green tests prove `--step` override works git-less/detached, +missing-config fails with `"io"`, and unknown names surface the right tags. + +**Files**: new `src/commands/step.rs`, `src/commands/model.rs`, `src/commands/thinking.rs`; +modified `src/commands/mod.rs`. +**Key changes**: +- `pub(crate) fn step_command(path: &std::path::Path, source: crate::config_dir::ConfigPathSource, step: Option) -> Result` — moved +- `pub(crate) fn model_command(...) -> Result` — moved (same 3 params) +- `pub(crate) fn thinking_command(...) -> Result` — moved (same 3 params) +- Body helpers now path-qualified: `crate::config::read_config`, `resolve::resolve_step`, + `resolve::artifact_dir_path`, `resolve::determine_step`, `resolve::resolve_model`, + `git::current_branch`. +- `mod.rs`: add three `mod` decls; arms become `step::step_command(&path, source, step.clone())` + etc.; prune root imports that are now handler-only. + +**Tests**: relocated handler-direct tests for step/model/thinking (exact model strings, tag/message +pairs); integration `tests/step.rs` (artifact-derived step, default fallback, XDG, cwd-config +guard, missing-path stderr spelling, `--step` git-less/detached, unknown step text-vs-JSON) and +`tests/model.rs` (exact `openrouter/...` strings, thinking values, `-j`, `--step` override) — **unedited**. +**Verify**: `scripts/test.sh` green; `cargo nextest run step:: model:: thinking::`. + +--- + +## Stage 5: Frontmatter/script handlers — `prompt.rs`, `script.rs` + +The two remaining handler families — the ones with extra contract surface (frontmatter emission, +positional `script `, `show_frontmatter = false`). Moving them last means the largest +behavioral pins are exercised against an otherwise-finished split. + +**Files**: new `src/commands/prompt.rs`, `src/commands/script.rs`; modified +`src/commands/mod.rs`. +**Key changes**: +- `pub(crate) fn prompt_command(path, source, step: Option) -> Result` — moved; + frontmatter `format!` string preserved byte-for-byte. +- `pub(crate) fn script_command(path, source, step_name: Option) -> Result` — + moved; `"script"` error for no-script/unknown-name preserved. +- `mod.rs`: add two `mod` decls; final arm updates; root now holds only `Cli`, `Commands`, + `select_command_with_env`, `select_command`, `mod` declarations, and the parse/routing tests. + +**Tests**: relocated `prompt_command`/`script_command` unit tests; integration `tests/prompt.rs` +(frontmatter exact text, `step =`/`branch =`/`artifact_directory =` lines, `show_frontmatter=false`, +positional rejected exit 2) and `tests/script.rs` (never emits frontmatter, positional override, +`"script"` tags, XDG) — **unedited**. +**Verify**: `scripts/test.sh` green; `cargo nextest run prompt_ script_ json_output`. + +--- + +## Stage 6: Final verification sweep (no new behavior) + +Proves the end state matches the design: root file small and readable, nothing outside +`src/commands/` touched, architecture graph unchanged. + +**Files**: `src/commands/mod.rs` (only if a leftover unused import/dead item remains). +**Key changes**: none functional. `mod.rs` must be ≤ ~300 lines and hold only parser types, router, +and parse/routing tests. + +**Tests**: none new. +**Verify**: `scripts/test.sh` green (fmt, check, clippy `-D warnings`, nextest, forbidden strings); +`git status --short` shows changes only under `src/commands/`; `tests/` and `src/main.rs` untouched +(`git diff --stat`); `cargo nextest run architecture` passes; diff Stage-1 captures against fresh +`--help`/`-j` output; `wc -l src/commands/*.rs` all under ~300; per-file diff review against the +original `mod.rs` regions (the design's "mechanical move" mitigation). + +--- + +## Testing Checkpoints + +| Stage | Must be green before advancing | +|---|---| +| 1 Baseline | Full `scripts/test.sh` on pristine tree + captures recorded | +| 2 `resolve.rs` | `scripts/test.sh`; helper unit tests relocated intact; router tests pass | +| 3 Leaf handlers | `scripts/test.sh`; `tests/init_creates_file.rs`, `branch.rs`, `artifact_directory.rs` unedited and green | +| 4 step/model/thinking | `scripts/test.sh`; `tests/step.rs`, `tests/model.rs` green | +| 5 prompt/script | `scripts/test.sh`; `tests/prompt.rs`, `tests/script.rs`, `tests/json_output.rs` green | +| 6 Sweep | `scripts/test.sh` + `architecture` green; only `src/commands/**` changed; all files <300 lines | + +Resume rule: if any stage's gate fails, fix within that stage — the stages below it are already +independently valuable and can land on their own (each is a parent-consistent, compiling tree). + +Next: run `!1` to plan \ No newline at end of file diff --git a/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/task.md b/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/task.md new file mode 100644 index 0000000..b10729f --- /dev/null +++ b/.pi/orksorksorks/alanvardy-var-981-refactor-orksorksorks/task.md @@ -0,0 +1,3 @@ +# Task + +Refactor the orksorksorks Rust CLI so that `src/commands/mod.rs` — currently 1,459 of the crate's 2,884 lines, containing the clap parser structs, the subcommand enum, routing, and every command handler plus ~1,000 lines of inline tests — is split into an organized set of submodules. The organization scheme is to be decided through research and design; the change must be behavior-preserving (same CLI surface, same output, same error messages), and must respect the existing rust_arkitect architecture constraints in `tests/architecture.rs`. \ No newline at end of file diff --git a/src/commands/artifact_directory.rs b/src/commands/artifact_directory.rs new file mode 100644 index 0000000..ad739ec --- /dev/null +++ b/src/commands/artifact_directory.rs @@ -0,0 +1,10 @@ +use super::resolve; +use crate::errors::Error; +use crate::git; + +/// Handle the `artifact_directory` subcommand: return +/// `$PWD/.pi/orksorksorks//`, plain (no directory creation). +pub(crate) fn artifact_directory_command() -> Result { + let cwd = std::env::current_dir()?; + Ok(resolve::artifact_dir_path(&cwd, &git::current_branch()?)) +} diff --git a/src/commands/branch.rs b/src/commands/branch.rs new file mode 100644 index 0000000..bd6582e --- /dev/null +++ b/src/commands/branch.rs @@ -0,0 +1,7 @@ +use crate::errors::Error; +use crate::git; + +/// Handle the `branch` subcommand: return the current git branch, plain. +pub(crate) fn branch_command() -> Result { + git::current_branch() +} diff --git a/src/commands/init.rs b/src/commands/init.rs new file mode 100644 index 0000000..568c111 --- /dev/null +++ b/src/commands/init.rs @@ -0,0 +1,42 @@ +use crate::errors::Error; + +/// Content of the default config template, embedded at compile time. +const DEFAULT_TEMPLATE: &str = include_str!("../../templates/default.toml"); + +/// Create a new `orksorksorks.toml` file with default configuration. +pub(crate) fn init_command(path: &std::path::Path) -> Result { + use std::io::Write; + + if let Some(parent) = path.parent() { + std::fs::create_dir_all(parent)?; + } + let mut file = create_new_file(path)?; + file.write_all(DEFAULT_TEMPLATE.as_bytes())?; + file.flush()?; + file.sync_all()?; + Ok(crate::format::green_string(&format!( + "✓ Created {}", + path.display() + ))) +} + +/// Open `path` with create-new semantics (fails on any existing path — file +/// or symlink — without following), mapping an already-existing path to the +/// `config-exists` error instead of a raw I/O failure so callers can +/// distinguish "refused to overwrite" from other open errors. +fn create_new_file(path: &std::path::Path) -> Result { + std::fs::OpenOptions::new() + .write(true) + .create_new(true) + .open(path) + .map_err(|e| { + if e.kind() == std::io::ErrorKind::AlreadyExists { + Error::new( + "config-exists", + &format!("{} already exists; not overwriting", path.display()), + ) + } else { + Error::from(e) + } + }) +} diff --git a/src/commands/mod.rs b/src/commands/mod.rs index c2dbc04..5b90465 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -1,9 +1,17 @@ -use crate::config::{Config, Step}; use crate::errors::Error; -use crate::git; use clap::{Parser, Subcommand}; use std::path::PathBuf; +mod artifact_directory; +mod branch; +mod init; +mod model; +mod prompt; +mod resolve; +mod script; +mod step; +mod thinking; + const NAME: &str = env!("CARGO_PKG_NAME"); const AUTHOR: &str = env!("CARGO_PKG_AUTHORS"); const ABOUT: &str = env!("CARGO_PKG_DESCRIPTION"); @@ -120,34 +128,34 @@ fn select_command_with_env(cli: &Cli, env: &crate::config_dir::ConfigEnv) -> Res match &cli.command { Commands::Init { config } => { let (path, _) = crate::config_dir::config_file_path_with_env(config.as_deref(), env)?; - init_command(&path) + init::init_command(&path) } - Commands::Branch => branch_command(), - Commands::ArtifactDirectory => artifact_directory_command(), + Commands::Branch => branch::branch_command(), + Commands::ArtifactDirectory => artifact_directory::artifact_directory_command(), Commands::Step { config, step } => { let (path, source) = crate::config_dir::config_file_path_with_env(config.as_deref(), env)?; - step_command(&path, source, step.clone()) + step::step_command(&path, source, step.clone()) } Commands::Model { config, step } => { let (path, source) = crate::config_dir::config_file_path_with_env(config.as_deref(), env)?; - model_command(&path, source, step.clone()) + model::model_command(&path, source, step.clone()) } Commands::Thinking { config, step } => { let (path, source) = crate::config_dir::config_file_path_with_env(config.as_deref(), env)?; - thinking_command(&path, source, step.clone()) + thinking::thinking_command(&path, source, step.clone()) } Commands::Prompt { step, config } => { let (path, source) = crate::config_dir::config_file_path_with_env(config.as_deref(), env)?; - prompt_command(&path, source, step.clone()) + prompt::prompt_command(&path, source, step.clone()) } Commands::Script { step_name, config } => { let (path, source) = crate::config_dir::config_file_path_with_env(config.as_deref(), env)?; - script_command(&path, source, step_name.clone()) + script::script_command(&path, source, step_name.clone()) } } } @@ -157,287 +165,9 @@ pub fn select_command(cli: &Cli) -> Result { select_command_with_env(cli, &crate::config_dir::ConfigEnv::from_env()) } -/// Create a new `orksorksorks.toml` file with default configuration. -fn init_command(path: &std::path::Path) -> Result { - use std::io::Write; - - if let Some(parent) = path.parent() { - std::fs::create_dir_all(parent)?; - } - let mut file = std::fs::OpenOptions::new() - .write(true) - .create_new(true) - .open(path) - .map_err(|e| { - if e.kind() == std::io::ErrorKind::AlreadyExists { - Error::new( - "config-exists", - &format!("{} already exists; not overwriting", path.display()), - ) - } else { - Error::from(e) - } - })?; - file.write_all(include_str!("../../templates/default.toml").as_bytes())?; - file.flush()?; - file.sync_all()?; - Ok(crate::format::green_string(&format!( - "✓ Created {}", - path.display() - ))) -} - -/// Compose the artifact-directory path from a cwd and a branch name. -/// -/// Slashes in the branch name are normalized to hyphens so the branch is a -/// single path segment. Pure — no git, no I/O, no error path. The trailing -/// slash is part of the contract (see `task.md`). -fn artifact_dir_path(cwd: &std::path::Path, branch: &str) -> String { - format!( - "{}/.pi/orksorksorks/{}/", - cwd.display(), - branch.replace('/', "-") - ) -} - -/// Handle the `branch` subcommand: return the current git branch, plain. -fn branch_command() -> Result { - git::current_branch() -} - -/// Handle the `artifact_directory` subcommand: return -/// `$PWD/.pi/orksorksorks//`, plain (no directory creation). -fn artifact_directory_command() -> Result { - let cwd = std::env::current_dir()?; - Ok(artifact_dir_path(&cwd, &git::current_branch()?)) -} - -/// Reverse-iterate steps and return the *matched step* (not just its name), -/// so callers like `step` and `model` can read different fields (`name` vs -/// `model`). A step with an empty `trigger_artifact` is a default: it never -/// matches by existence, but is returned instead of erroring when no other -/// step's artifact exists. Configs loaded through `read_config` have at most -/// one default (enforced by `config:multiple-default`); this function still -/// tolerates several and the last such step in the list wins. -/// `artifact_dir` must end in a trailing slash — the same string-composition -/// convention as `artifact_dir_path`. -fn determine_step(config: &Config, artifact_dir: &str) -> Result { - let mut default = None; - for step in config.steps.iter().rev() { - if step.trigger_artifact.is_empty() { - // Empty trigger = default step; never used while a real match - // is possible, only as fallback. First seen in reverse = last - // forward, which wins (reverse-priority convention). - default.get_or_insert_with(|| step.clone()); - continue; - } - let path = format!("{artifact_dir}{}", step.trigger_artifact); - if std::path::Path::new(&path).try_exists()? { - return Ok(step.clone()); - } - } - if let Some(step) = default { - return Ok(step); - } - Err(Error::new( - "step", - &format!("{artifact_dir}: no trigger artifact matched"), - )) -} - -/// Look up the named model (the current step's `model` reference) in -/// `config.models` and return the matching entry, so callers like `model` -/// and `thinking` can read different fields (`model` vs `thinking`). -fn resolve_model(config: &Config, name: &str) -> Result { - for m in config.models.iter() { - if m.name == name { - return Ok(m.clone()); - } - } - Err(Error::new("model", &format!("no model named {name:?}"))) -} - -/// Look up the named step in `config.steps` and return the matching entry. -/// First match wins; an unknown name errors with the `"step"` tag (the same -/// tag `determine_step` uses, so callers discriminate by message). -fn resolve_step(config: &Config, name: &str) -> Result { - for s in config.steps.iter() { - if s.name == name { - return Ok(s.clone()); - } - } - Err(Error::new("step", &format!("no step named {name:?}"))) -} - -/// Handle the `step` subcommand: read the config and return the current step -/// name. With `--step `, the name replaces artifact-based derivation -/// (no git is consulted); otherwise the step is derived from cwd + git -/// branch + trigger artifacts. -/// -/// `path` is the already-resolved config location: either the explicit -/// `--config` argument or the config-directory default (`config_dir.rs`). -/// `read_config` runs before git resolution so a missing/unreadable config -/// deterministically fails with `"io"`. -fn step_command( - path: &std::path::Path, - source: crate::config_dir::ConfigPathSource, - step: Option, -) -> Result { - let cfg = crate::config::read_config(path, source)?; - let step = if let Some(name) = step { - resolve_step(&cfg, &name)? - } else { - let cwd = std::env::current_dir()?; - let artifact_dir = artifact_dir_path(&cwd, &git::current_branch()?); - determine_step(&cfg, &artifact_dir)? - }; - Ok(step.name) -} - -/// Handle the `model` subcommand: determine the current step (via -/// `--step ` when given, else artifact derivation), read the model -/// *name* it references, and resolve that name against `config.models` to -/// the concrete model string. -fn model_command( - path: &std::path::Path, - source: crate::config_dir::ConfigPathSource, - step: Option, -) -> Result { - let cfg = crate::config::read_config(path, source)?; - let step = if let Some(name) = step { - resolve_step(&cfg, &name)? - } else { - let cwd = std::env::current_dir()?; - let artifact_dir = artifact_dir_path(&cwd, &git::current_branch()?); - determine_step(&cfg, &artifact_dir)? - }; - let model = resolve_model(&cfg, &step.model)?; - Ok(model.model) -} - -/// Handle the `thinking` subcommand: determine the current step (via -/// `--step ` when given, else artifact derivation), read the model -/// *name* it references, and resolve that name against `config.models` to -/// its thinking-budget value. -fn thinking_command( - path: &std::path::Path, - source: crate::config_dir::ConfigPathSource, - step: Option, -) -> Result { - let cfg = crate::config::read_config(path, source)?; - let step = if let Some(name) = step { - resolve_step(&cfg, &name)? - } else { - let cwd = std::env::current_dir()?; - let artifact_dir = artifact_dir_path(&cwd, &git::current_branch()?); - determine_step(&cfg, &artifact_dir)? - }; - let model = resolve_model(&cfg, &step.model)?; - Ok(model.thinking) -} - -/// Look up the named prompt (a `config.prompts` key, usually a step name) -/// and return its content. -fn resolve_prompt(config: &Config, name: &str) -> Result { - for p in config.prompts.iter() { - if p.name == name { - return Ok(p.content.clone()); - } - } - Err(Error::new("prompt", &format!("no prompt named {name:?}"))) -} - -/// Look up the named script (a `config.scripts` key referenced by a step's -/// `script` field) and return its raw content. -fn resolve_script(config: &Config, name: &str) -> Result { - for s in config.scripts.iter() { - if s.name == name { - return Ok(s.content.clone()); - } - } - Err(Error::new("script", &format!("no script named {name:?}"))) -} - -/// Handle the `prompt` subcommand: read the config and return the prompt -/// content for the current step, or for the explicitly named step when -/// `--step ` is provided as an override. Output is prefixed with a -/// frontmatter block (important-variables header, step, branch, artifact -/// directory) above the prompt content unless `show_frontmatter = false` -/// in the config. -/// -/// `--step` skips only *step derivation*: the frontmatter block still -/// resolves the git branch, so with the default `show_frontmatter = true`, -/// `prompt --step ` needs a git checkout (unlike `step`/`model`/ -/// `thinking`, which consult git only to derive the step). -fn prompt_command( - path: &std::path::Path, - source: crate::config_dir::ConfigPathSource, - step: Option, -) -> Result { - let cfg = crate::config::read_config(path, source)?; - let name = if let Some(name) = step { - resolve_step(&cfg, &name)?.name - } else { - // No explicit step: derive the current step from trigger artifacts, - // exactly like `step`/`model`/`thinking`. - let cwd = std::env::current_dir()?; - let artifact_dir = artifact_dir_path(&cwd, &git::current_branch()?); - determine_step(&cfg, &artifact_dir)?.name - }; - let content = resolve_prompt(&cfg, &name)?; - - // Frontmatter carries the run context (step, branch, artifact dir) above - // the prompt output unless disabled by `show_frontmatter = false`. - // Branch/artifact resolution is deferred until after the prompt resolves - // so an unknown prompt keeps failing with its `"prompt"` error instead - // of a git error. `step` is the effective prompt name (the explicit - // override when given, else the derived step). - if cfg.show_frontmatter { - let cwd = std::env::current_dir()?; - let branch = git::current_branch()?; - let artifact_dir = artifact_dir_path(&cwd, &branch); - return Ok(format!( - "## Important variables\nThese are literal text values, not shell or\nenvironment variables. Wherever a prompt writes $ (or\n($variable)path), substitute the value shown below as plain text; never\nwrite $variable in a shell command.\nstep = {}\nbranch = {}\nartifact_directory = {}\n\n{}", - name, branch, artifact_dir, content - )); - } - Ok(content) -} - -/// Handle the `script` subcommand: read the config and return the raw -/// `[[scripts]]` content referenced by the current step's `script` field, -/// or by the explicitly named step when `step_name` overrides detection. -/// No frontmatter is emitted (the content is meant to be run/piped). -fn script_command( - path: &std::path::Path, - source: crate::config_dir::ConfigPathSource, - step_name: Option, -) -> Result { - let cfg = crate::config::read_config(path, source)?; - let step = if let Some(name) = step_name { - // Explicit name: resolve by name (no git/artifact work), so unknown - // script-name / no-script errors surface as `"script"`, never `"git"`. - resolve_step(&cfg, &name)? - } else { - // No explicit step: derive the current step from trigger artifacts, - // exactly like `step`/`model`/`thinking`/`prompt`. - let cwd = std::env::current_dir()?; - let artifact_dir = artifact_dir_path(&cwd, &git::current_branch()?); - determine_step(&cfg, &artifact_dir)? - }; - let script_name = step.script.ok_or_else(|| { - Error::new( - "script", - &format!("step {:?} has no script configured", step.name), - ) - })?; - resolve_script(&cfg, &script_name) -} - #[cfg(test)] mod tests { use super::*; - use crate::config::{Config, Step}; use clap::CommandFactory; #[test] @@ -532,218 +262,6 @@ mod tests { assert!(result.is_err()); } - #[test] - fn artifact_dir_path_appends_trailing_slash() { - use pretty_assertions::assert_eq; - let path = artifact_dir_path(std::path::Path::new("/repo"), "main"); - assert_eq!(path, "/repo/.pi/orksorksorks/main/"); - } - - #[test] - fn artifact_dir_path_is_plain() { - let path = artifact_dir_path(std::path::Path::new("/repo"), "main"); - assert!(!path.contains('\x1b'), "{path}"); - } - - #[test] - fn artifact_dir_path_handles_branch_with_slashes() { - use pretty_assertions::assert_eq; - let path = artifact_dir_path(std::path::Path::new("/repo"), "feature/x"); - assert_eq!(path, "/repo/.pi/orksorksorks/feature-x/"); - } - - #[test] - fn artifact_dir_path_replaces_all_slashes() { - use pretty_assertions::assert_eq; - let path = artifact_dir_path(std::path::Path::new("/repo"), "a/b/c"); - assert_eq!(path, "/repo/.pi/orksorksorks/a-b-c/"); - } - - #[test] - fn artifact_dir_path_leaves_slashless_branch_unchanged() { - use pretty_assertions::assert_eq; - let path = artifact_dir_path(std::path::Path::new("/repo"), "main"); - assert_eq!(path, "/repo/.pi/orksorksorks/main/"); - } - - #[test] - fn determine_step_returns_step_with_present_artifact() { - let dir = tempfile::tempdir().unwrap(); - std::fs::write(dir.path().join("first.txt"), "").unwrap(); - let config = Config { - version: "0.1.0".to_string(), - show_frontmatter: true, - steps: vec![ - Step { - name: "one".to_string(), - trigger_artifact: "first.txt".to_string(), - model: "small".to_string(), - script: None, - }, - Step { - name: "two".to_string(), - trigger_artifact: "second.txt".to_string(), - model: "high".to_string(), - script: None, - }, - ], - models: vec![], - prompts: vec![], - scripts: vec![], - }; - let artifact_dir = format!("{}/", dir.path().display()); - assert_eq!(determine_step(&config, &artifact_dir).unwrap().name, "one"); - } - - #[test] - fn determine_step_prefers_last_step_in_reverse() { - let dir = tempfile::tempdir().unwrap(); - std::fs::write(dir.path().join("first.txt"), "").unwrap(); - std::fs::write(dir.path().join("second.txt"), "").unwrap(); - let config = Config { - version: "0.1.0".to_string(), - show_frontmatter: true, - steps: vec![ - Step { - name: "one".to_string(), - trigger_artifact: "first.txt".to_string(), - model: "small".to_string(), - script: None, - }, - Step { - name: "two".to_string(), - trigger_artifact: "second.txt".to_string(), - model: "high".to_string(), - script: None, - }, - ], - models: vec![], - prompts: vec![], - scripts: vec![], - }; - let artifact_dir = format!("{}/", dir.path().display()); - assert_eq!(determine_step(&config, &artifact_dir).unwrap().name, "two"); - } - - #[test] - fn determine_step_empty_trigger_is_default_fallback() { - let dir = tempfile::tempdir().unwrap(); - let config = Config { - version: "0.1.0".to_string(), - show_frontmatter: true, - steps: vec![ - Step { - name: "one".to_string(), - trigger_artifact: "first.txt".to_string(), - model: "small".to_string(), - script: None, - }, - Step { - name: "default".to_string(), - trigger_artifact: String::new(), - model: "small".to_string(), - script: None, - }, - ], - models: vec![], - prompts: vec![], - scripts: vec![], - }; - let artifact_dir = format!("{}/", dir.path().display()); - assert_eq!( - determine_step(&config, &artifact_dir).unwrap().name, - "default" - ); - } - - #[test] - fn determine_step_empty_trigger_does_not_shadow_real_match() { - let dir = tempfile::tempdir().unwrap(); - std::fs::write(dir.path().join("first.txt"), "").unwrap(); - let config = Config { - version: "0.1.0".to_string(), - show_frontmatter: true, - steps: vec![ - Step { - name: "one".to_string(), - trigger_artifact: "first.txt".to_string(), - model: "small".to_string(), - script: None, - }, - Step { - name: "default".to_string(), - trigger_artifact: String::new(), - model: "small".to_string(), - script: None, - }, - ], - models: vec![], - prompts: vec![], - scripts: vec![], - }; - let artifact_dir = format!("{}/", dir.path().display()); - assert_eq!(determine_step(&config, &artifact_dir).unwrap().name, "one"); - } - - #[test] - fn determine_step_empty_trigger_matches_before_later_artifact() { - // The default is a last resort: it does not beat a later step's - // real artifact, only the absence of any artifact. - let dir = tempfile::tempdir().unwrap(); - std::fs::write(dir.path().join("second.txt"), "").unwrap(); - let config = Config { - version: "0.1.0".to_string(), - show_frontmatter: true, - steps: vec![ - Step { - name: "default".to_string(), - trigger_artifact: String::new(), - model: "small".to_string(), - script: None, - }, - Step { - name: "two".to_string(), - trigger_artifact: "second.txt".to_string(), - model: "high".to_string(), - script: None, - }, - ], - models: vec![], - prompts: vec![], - scripts: vec![], - }; - let artifact_dir = format!("{}/", dir.path().display()); - assert_eq!(determine_step(&config, &artifact_dir).unwrap().name, "two"); - } - - #[test] - fn determine_step_latest_empty_trigger_wins() { - let dir = tempfile::tempdir().unwrap(); - let config = Config { - version: "0.1.0".to_string(), - show_frontmatter: true, - steps: vec![ - Step { - name: "one".to_string(), - trigger_artifact: String::new(), - model: "small".to_string(), - script: None, - }, - Step { - name: "two".to_string(), - trigger_artifact: String::new(), - model: "high".to_string(), - script: None, - }, - ], - models: vec![], - prompts: vec![], - scripts: vec![], - }; - let artifact_dir = format!("{}/", dir.path().display()); - assert_eq!(determine_step(&config, &artifact_dir).unwrap().name, "two"); - } - #[test] fn cli_try_parse_accepts_step() { use clap::Parser; @@ -810,144 +328,6 @@ mod tests { } } - #[test] - fn determine_step_no_match_errors_with_step_tag() { - let dir = tempfile::tempdir().unwrap(); - let config = Config { - version: "0.1.0".to_string(), - show_frontmatter: true, - steps: vec![Step { - name: "one".to_string(), - trigger_artifact: "first.txt".to_string(), - model: "small".to_string(), - script: None, - }], - models: vec![], - prompts: vec![], - scripts: vec![], - }; - let artifact_dir = format!("{}/", dir.path().display()); - let err = determine_step(&config, &artifact_dir).unwrap_err(); - assert_eq!(err.source, "step"); - assert!( - err.message.contains("no trigger artifact matched"), - "{}", - err.message - ); - } - - #[test] - fn resolve_model_returns_model_for_matching_name() { - let config = Config { - version: "0.1.0".to_string(), - show_frontmatter: true, - steps: vec![], - models: vec![crate::config::Model { - name: "small".to_string(), - model: "openrouter/deepseek/flash".to_string(), - thinking: "high".to_string(), - }], - prompts: vec![], - scripts: vec![], - }; - assert_eq!( - resolve_model(&config, "small").unwrap().model, - "openrouter/deepseek/flash", - ); - assert_eq!(resolve_model(&config, "small").unwrap().thinking, "high"); - } - - #[test] - fn resolve_model_missing_name_errors_with_model_tag() { - let config = Config { - version: "0.1.0".to_string(), - show_frontmatter: true, - steps: vec![], - models: vec![crate::config::Model { - name: "small".to_string(), - model: "openrouter/deepseek/flash".to_string(), - thinking: "high".to_string(), - }], - prompts: vec![], - scripts: vec![], - }; - let err = resolve_model(&config, "large").unwrap_err(); - assert_eq!(err.source, "model"); - assert!(err.message.contains("no model named"), "{}", err.message); - } - - #[test] - fn resolve_step_returns_matching_step() { - let config = Config { - version: "0.1.0".to_string(), - show_frontmatter: true, - steps: vec![Step { - name: "one".to_string(), - trigger_artifact: "first.txt".to_string(), - model: "small".to_string(), - script: None, - }], - models: vec![], - prompts: vec![], - scripts: vec![], - }; - let step = resolve_step(&config, "one").unwrap(); - assert_eq!(step.name, "one"); - assert_eq!(step.trigger_artifact, "first.txt"); - assert_eq!(step.model, "small"); - } - - #[test] - fn resolve_step_unknown_name_tags_step_error() { - let config = Config { - version: "0.1.0".to_string(), - show_frontmatter: true, - steps: vec![Step { - name: "one".to_string(), - trigger_artifact: String::new(), - model: "small".to_string(), - script: None, - }], - models: vec![], - prompts: vec![], - scripts: vec![], - }; - let err = resolve_step(&config, "nope").unwrap_err(); - assert_eq!(err.source, "step"); - assert!( - err.message.contains("no step named \"nope\""), - "{}", - err.message - ); - } - - #[test] - fn resolve_step_first_match_wins() { - let config = Config { - version: "0.1.0".to_string(), - show_frontmatter: true, - steps: vec![ - Step { - name: "dup".to_string(), - trigger_artifact: String::new(), - model: "small".to_string(), - script: None, - }, - Step { - name: "dup".to_string(), - trigger_artifact: String::new(), - model: "high".to_string(), - script: None, - }, - ], - models: vec![], - prompts: vec![], - scripts: vec![], - }; - let step = resolve_step(&config, "dup").unwrap(); - assert_eq!(step.model, "small"); - } - #[test] fn select_command_routes_model() { let cli = Cli { @@ -1234,226 +614,4 @@ mod tests { let err = select_command(&cli).unwrap_err(); assert_eq!(err.source, "io"); } - - #[test] - fn step_command_flag_succeeds_in_non_git_dir() { - // A tempdir with NO git repo: bare step would fail in git::current_branch, - // so success proves `--step` never shells out to git or reads cwd. - let dir = tempfile::tempdir().unwrap(); - std::fs::write( - dir.path().join("orksorksorks.toml"), - concat!( - "version = \"0.1.0\"\n", - "[[steps]]\nname = \"one\"\ntrigger_artifact = \"first.txt\"\nmodel = \"small\"\n", - "[[models]]\nname = \"small\"\nmodel = \"openrouter/deepseek/flash\"\nthinking = \"high\"\n", - "[[prompts]]\nname = \"one\"\ncontent = \"one\"\n", - ), - ) - .unwrap(); - let out = step_command( - &dir.path().join("orksorksorks.toml"), - crate::config_dir::ConfigPathSource::ExplicitFlag, - Some("one".to_string()), - ) - .unwrap(); - assert_eq!(out, "one"); - } - - #[test] - fn step_command_flag_unknown_name_tags_step() { - let dir = tempfile::tempdir().unwrap(); - std::fs::write( - dir.path().join("orksorksorks.toml"), - "version = \"0.1.0\"\n", - ) - .unwrap(); - let err = step_command( - &dir.path().join("orksorksorks.toml"), - crate::config_dir::ConfigPathSource::ExplicitFlag, - Some("nope".to_string()), - ) - .unwrap_err(); - assert_eq!(err.source, "step"); - assert!( - err.message.contains("no step named \"nope\""), - "{}", - err.message - ); - } - - #[test] - fn prompt_command_step_flag_unknown_name_tags_step() { - // `--step nope` with no [[steps]]: fails in resolve_step with the - // `"step"` tag — before resolve_prompt ever runs. - let dir = tempfile::tempdir().unwrap(); - std::fs::write( - dir.path().join("orksorksorks.toml"), - "version = \"0.1.0\"\n", - ) - .unwrap(); - let err = prompt_command( - &dir.path().join("orksorksorks.toml"), - crate::config_dir::ConfigPathSource::ExplicitFlag, - Some("nope".to_string()), - ) - .unwrap_err(); - assert_eq!(err.source, "step"); - assert!( - err.message.contains("no step named \"nope\""), - "{}", - err.message - ); - } - - #[test] - fn prompt_command_step_flag_known_step_missing_prompt_tags_validation() { - // `--step one` where step `one` exists but has no [[prompts]] entry: - // the config is invalid, so `read_config` fails fast with - // `config:missing-prompt` before resolve_step/resolve_prompt run. The - // runtime `"prompt"` tag is unreachable through the validated - // `--step` path, since every step must have a matching prompt. - let dir = tempfile::tempdir().unwrap(); - std::fs::write( - dir.path().join("orksorksorks.toml"), - "version = \"0.1.0\"\n[[steps]]\nname = \"one\"\ntrigger_artifact = \"first.txt\"\nmodel = \"small\"\n", - ) - .unwrap(); - let err = prompt_command( - &dir.path().join("orksorksorks.toml"), - crate::config_dir::ConfigPathSource::ExplicitFlag, - Some("one".to_string()), - ) - .unwrap_err(); - assert_eq!(err.source, "config:missing-prompt"); - assert!( - err.message.contains("has no matching prompt"), - "{}", - err.message - ); - } - - #[test] - fn script_command_step_without_script_errors() { - // Step `one` exists (validated config with matching prompt/model) but - // has no `script` key: `read_config` succeeds, then the explicit-name - // path resolves the step and fails on the missing script reference - // with the `"script"` tag — before resolve_script ever runs. The - // explicit-name path avoids git, so no repo is needed. - let dir = tempfile::tempdir().unwrap(); - std::fs::write( - dir.path().join("orksorksorks.toml"), - concat!( - "version = \"0.1.0\"\n", - "[[steps]]\nname = \"one\"\ntrigger_artifact = \"first.txt\"\nmodel = \"small\"\n", - "[[models]]\nname = \"small\"\nmodel = \"openrouter/deepseek/flash\"\nthinking = \"high\"\n", - "[[prompts]]\nname = \"one\"\ncontent = \"one\"\n", - ), - ) - .unwrap(); - let err = script_command( - &dir.path().join("orksorksorks.toml"), - crate::config_dir::ConfigPathSource::ExplicitFlag, - Some("one".to_string()), - ) - .unwrap_err(); - assert_eq!(err.source, "script"); - assert!( - err.message.contains("has no script configured"), - "{}", - err.message - ); - } - - #[test] - fn resolve_prompt_returns_content_for_matching_name() { - let config = Config { - version: "0.1.0".to_string(), - show_frontmatter: true, - steps: vec![], - models: vec![], - prompts: vec![crate::config::Prompt { - name: "questions".to_string(), - content: "# Question — Decompose the Task\n".to_string(), - }], - scripts: vec![], - }; - assert_eq!( - resolve_prompt(&config, "questions").unwrap(), - "# Question — Decompose the Task\n", - ); - } - - #[test] - fn resolve_prompt_missing_name_errors_with_prompt_tag() { - let config = Config { - version: "0.1.0".to_string(), - show_frontmatter: true, - steps: vec![], - models: vec![], - prompts: vec![crate::config::Prompt { - name: "questions".to_string(), - content: String::new(), - }], - scripts: vec![], - }; - let err = resolve_prompt(&config, "research").unwrap_err(); - assert_eq!(err.source, "prompt"); - assert!(err.message.contains("no prompt named"), "{}", err.message); - } - - #[test] - fn resolve_script_hit_returns_content() { - let config = Config { - version: "0.1.0".to_string(), - show_frontmatter: true, - steps: vec![], - models: vec![], - prompts: vec![], - scripts: vec![crate::config::Script { - name: "run-one".to_string(), - content: "#!/bin/bash\n".to_string(), - }], - }; - assert_eq!(resolve_script(&config, "run-one").unwrap(), "#!/bin/bash\n",); - } - - #[test] - fn resolve_script_miss_returns_script_tag() { - let config = Config { - version: "0.1.0".to_string(), - show_frontmatter: true, - steps: vec![], - models: vec![], - prompts: vec![], - scripts: vec![crate::config::Script { - name: "run-one".to_string(), - content: String::new(), - }], - }; - let err = resolve_script(&config, "nope").unwrap_err(); - assert_eq!(err.source, "script"); - assert!(err.message.contains("no script named"), "{}", err.message); - } - - #[test] - fn resolve_script_first_match_wins() { - let config = Config { - version: "0.1.0".to_string(), - show_frontmatter: true, - steps: vec![], - models: vec![], - prompts: vec![], - scripts: vec![ - crate::config::Script { - name: "dup".to_string(), - content: "first\n".to_string(), - }, - crate::config::Script { - name: "dup".to_string(), - content: "second\n".to_string(), - }, - ], - }; - assert_eq!(resolve_script(&config, "dup").unwrap(), "first\n",); - } } diff --git a/src/commands/model.rs b/src/commands/model.rs new file mode 100644 index 0000000..f9c5bd1 --- /dev/null +++ b/src/commands/model.rs @@ -0,0 +1,16 @@ +use super::resolve; +use crate::errors::Error; + +/// Handle the `model` subcommand: determine the current step (via +/// `--step ` when given, else artifact derivation), read the model +/// *name* it references, and resolve that name against `config.models` to +/// the concrete model string. +pub(crate) fn model_command( + path: &std::path::Path, + source: crate::config_dir::ConfigPathSource, + step: Option, +) -> Result { + let (cfg, step) = resolve::read_config_and_step(path, source, step)?; + let model = resolve::resolve_model(&cfg, &step.model)?; + Ok(model.model) +} diff --git a/src/commands/prompt.rs b/src/commands/prompt.rs new file mode 100644 index 0000000..a6c19a9 --- /dev/null +++ b/src/commands/prompt.rs @@ -0,0 +1,97 @@ +use super::resolve; +use crate::errors::Error; +use crate::git; + +/// Handle the `prompt` subcommand: read the config and return the prompt +/// content for the current step, or for the explicitly named step when +/// `--step ` is provided as an override. Output is prefixed with a +/// frontmatter block (important-variables header, step, branch, artifact +/// directory) above the prompt content unless `show_frontmatter = false` +/// in the config. +/// +/// `--step` skips only *step derivation*: the frontmatter block still +/// resolves the git branch, so with the default `show_frontmatter = true`, +/// `prompt --step ` needs a git checkout (unlike `step`/`model`/ +/// `thinking`, which consult git only to derive the step). +pub(crate) fn prompt_command( + path: &std::path::Path, + source: crate::config_dir::ConfigPathSource, + step: Option, +) -> Result { + let (cfg, step) = resolve::read_config_and_step(path, source, step)?; + let name = step.name; + let content = resolve::resolve_prompt(&cfg, &name)?; + + // Frontmatter carries the run context (step, branch, artifact dir) above + // the prompt output unless disabled by `show_frontmatter = false`. + // Branch/artifact resolution is deferred until after the prompt resolves + // so an unknown prompt keeps failing with its `"prompt"` error instead + // of a git error. `step` is the effective prompt name (the explicit + // override when given, else the derived step). + if cfg.show_frontmatter { + let cwd = std::env::current_dir()?; + let branch = git::current_branch()?; + let artifact_dir = resolve::artifact_dir_path(&cwd, &branch); + return Ok(format!( + "## Important variables\nThese are literal text values, not shell or\nenvironment variables. Wherever a prompt writes $ (or\n($variable)path), substitute the value shown below as plain text; never\nwrite $variable in a shell command.\nstep = {}\nbranch = {}\nartifact_directory = {}\n\n{}", + name, branch, artifact_dir, content + )); + } + Ok(content) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn prompt_command_step_flag_unknown_name_tags_step() { + // `--step nope` with no [[steps]]: fails in resolve_step with the + // `"step"` tag — before resolve_prompt ever runs. + let dir = tempfile::tempdir().unwrap(); + std::fs::write( + dir.path().join("orksorksorks.toml"), + "version = \"0.1.0\"\n", + ) + .unwrap(); + let err = prompt_command( + &dir.path().join("orksorksorks.toml"), + crate::config_dir::ConfigPathSource::ExplicitFlag, + Some("nope".to_string()), + ) + .unwrap_err(); + assert_eq!(err.source, "step"); + assert!( + err.message.contains("no step named \"nope\""), + "{}", + err.message + ); + } + + #[test] + fn prompt_command_step_flag_known_step_missing_prompt_tags_validation() { + // `--step one` where step `one` exists but has no [[prompts]] entry: + // the config is invalid, so `read_config` fails fast with + // `config:missing-prompt` before resolve_step/resolve_prompt run. The + // runtime `"prompt"` tag is unreachable through the validated + // `--step` path, since every step must have a matching prompt. + let dir = tempfile::tempdir().unwrap(); + std::fs::write( + dir.path().join("orksorksorks.toml"), + "version = \"0.1.0\"\n[[steps]]\nname = \"one\"\ntrigger_artifact = \"first.txt\"\nmodel = \"small\"\n", + ) + .unwrap(); + let err = prompt_command( + &dir.path().join("orksorksorks.toml"), + crate::config_dir::ConfigPathSource::ExplicitFlag, + Some("one".to_string()), + ) + .unwrap_err(); + assert_eq!(err.source, "config:missing-prompt"); + assert!( + err.message.contains("has no matching prompt"), + "{}", + err.message + ); + } +} diff --git a/src/commands/resolve.rs b/src/commands/resolve.rs new file mode 100644 index 0000000..20b8b55 --- /dev/null +++ b/src/commands/resolve.rs @@ -0,0 +1,617 @@ +use crate::config::{Config, Step}; +use crate::errors::Error; +use crate::git; + +/// Compose the artifact-directory path from a cwd and a branch name. +/// +/// Slashes in the branch name are normalized to hyphens so the branch is a +/// single path segment. Pure — no git, no I/O, no error path. The trailing +/// slash is part of the contract (see `task.md`). +pub(crate) fn artifact_dir_path(cwd: &std::path::Path, branch: &str) -> String { + format!( + "{}/.pi/orksorksorks/{}/", + cwd.display(), + branch.replace('/', "-") + ) +} + +/// Reverse-iterate steps and return the *matched step* (not just its name), +/// so callers like `step` and `model` can read different fields (`name` vs +/// `model`). A step with an empty `trigger_artifact` is a default: it never +/// matches by existence, but is returned instead of erroring when no other +/// step's artifact exists. Configs loaded through `read_config` have at most +/// one default (enforced by `config:multiple-default`); this function still +/// tolerates several and the last such step in the list wins. +/// `artifact_dir` must end in a trailing slash — the same string-composition +/// convention as `artifact_dir_path`. +pub(crate) fn determine_step(config: &Config, artifact_dir: &str) -> Result { + let mut default = None; + for step in config.steps.iter().rev() { + if step.trigger_artifact.is_empty() { + // Empty trigger = default step; never used while a real match + // is possible, only as fallback. First seen in reverse = last + // forward, which wins (reverse-priority convention). + default.get_or_insert_with(|| step.clone()); + continue; + } + let path = format!("{artifact_dir}{}", step.trigger_artifact); + if std::path::Path::new(&path).try_exists()? { + return Ok(step.clone()); + } + } + if let Some(step) = default { + return Ok(step); + } + Err(Error::new( + "step", + &format!("{artifact_dir}: no trigger artifact matched"), + )) +} + +/// Read the config and resolve the current step: the explicitly named +/// `--step ` when given, else the step derived from cwd + git branch +/// + trigger artifacts. +/// +/// The config is returned alongside the step because callers resolve +/// step-dependent values against it (`model`/`thinking` resolve the model, +/// `prompt`/`script` resolve prompts/scripts, `step` reads the name). +/// `read_config` runs before any git/env work so a missing/unreadable config +/// deterministically fails with `"io"`, and the explicit-name path never +/// shells out to git or reads cwd (so `--step` works in non-repos). +pub(crate) fn read_config_and_step( + path: &std::path::Path, + source: crate::config_dir::ConfigPathSource, + step: Option, +) -> Result<(Config, Step), Error> { + let cfg = crate::config::read_config(path, source)?; + let step = if let Some(name) = step { + resolve_step(&cfg, &name)? + } else { + let cwd = std::env::current_dir()?; + let artifact_dir = artifact_dir_path(&cwd, &git::current_branch()?); + determine_step(&cfg, &artifact_dir)? + }; + Ok((cfg, step)) +} + +/// Look up the named model (the current step's `model` reference) in +/// `config.models` and return the matching entry, so callers like `model` +/// and `thinking` can read different fields (`model` vs `thinking`). +pub(crate) fn resolve_model(config: &Config, name: &str) -> Result { + for m in config.models.iter() { + if m.name == name { + return Ok(m.clone()); + } + } + Err(Error::new("model", &format!("no model named {name:?}"))) +} + +/// Look up the named step in `config.steps` and return the matching entry. +/// First match wins; an unknown name errors with the `"step"` tag (the same +/// tag `determine_step` uses, so callers discriminate by message). +pub(crate) fn resolve_step(config: &Config, name: &str) -> Result { + for s in config.steps.iter() { + if s.name == name { + return Ok(s.clone()); + } + } + Err(Error::new("step", &format!("no step named {name:?}"))) +} + +/// Look up the named prompt (a `config.prompts` key, usually a step name) +/// and return its content. +pub(crate) fn resolve_prompt(config: &Config, name: &str) -> Result { + for p in config.prompts.iter() { + if p.name == name { + return Ok(p.content.clone()); + } + } + Err(Error::new("prompt", &format!("no prompt named {name:?}"))) +} + +/// Look up the named script (a `config.scripts` key referenced by a step's +/// `script` field) and return its raw content. +pub(crate) fn resolve_script(config: &Config, name: &str) -> Result { + for s in config.scripts.iter() { + if s.name == name { + return Ok(s.content.clone()); + } + } + Err(Error::new("script", &format!("no script named {name:?}"))) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn artifact_dir_path_appends_trailing_slash() { + use pretty_assertions::assert_eq; + let path = artifact_dir_path(std::path::Path::new("/repo"), "main"); + assert_eq!(path, "/repo/.pi/orksorksorks/main/"); + } + + #[test] + fn artifact_dir_path_is_plain() { + let path = artifact_dir_path(std::path::Path::new("/repo"), "main"); + assert!(!path.contains('\x1b'), "{path}"); + } + + #[test] + fn artifact_dir_path_handles_branch_with_slashes() { + use pretty_assertions::assert_eq; + let path = artifact_dir_path(std::path::Path::new("/repo"), "feature/x"); + assert_eq!(path, "/repo/.pi/orksorksorks/feature-x/"); + } + + #[test] + fn artifact_dir_path_replaces_all_slashes() { + use pretty_assertions::assert_eq; + let path = artifact_dir_path(std::path::Path::new("/repo"), "a/b/c"); + assert_eq!(path, "/repo/.pi/orksorksorks/a-b-c/"); + } + + #[test] + fn artifact_dir_path_leaves_slashless_branch_unchanged() { + use pretty_assertions::assert_eq; + let path = artifact_dir_path(std::path::Path::new("/repo"), "main"); + assert_eq!(path, "/repo/.pi/orksorksorks/main/"); + } + + #[test] + fn determine_step_returns_step_with_present_artifact() { + let dir = tempfile::tempdir().unwrap(); + std::fs::write(dir.path().join("first.txt"), "").unwrap(); + let config = Config { + version: "0.1.0".to_string(), + show_frontmatter: true, + steps: vec![ + Step { + name: "one".to_string(), + trigger_artifact: "first.txt".to_string(), + model: "small".to_string(), + script: None, + }, + Step { + name: "two".to_string(), + trigger_artifact: "second.txt".to_string(), + model: "high".to_string(), + script: None, + }, + ], + models: vec![], + prompts: vec![], + scripts: vec![], + }; + let artifact_dir = format!("{}/", dir.path().display()); + assert_eq!(determine_step(&config, &artifact_dir).unwrap().name, "one"); + } + + #[test] + fn determine_step_prefers_last_step_in_reverse() { + let dir = tempfile::tempdir().unwrap(); + std::fs::write(dir.path().join("first.txt"), "").unwrap(); + std::fs::write(dir.path().join("second.txt"), "").unwrap(); + let config = Config { + version: "0.1.0".to_string(), + show_frontmatter: true, + steps: vec![ + Step { + name: "one".to_string(), + trigger_artifact: "first.txt".to_string(), + model: "small".to_string(), + script: None, + }, + Step { + name: "two".to_string(), + trigger_artifact: "second.txt".to_string(), + model: "high".to_string(), + script: None, + }, + ], + models: vec![], + prompts: vec![], + scripts: vec![], + }; + let artifact_dir = format!("{}/", dir.path().display()); + assert_eq!(determine_step(&config, &artifact_dir).unwrap().name, "two"); + } + + #[test] + fn determine_step_empty_trigger_is_default_fallback() { + let dir = tempfile::tempdir().unwrap(); + let config = Config { + version: "0.1.0".to_string(), + show_frontmatter: true, + steps: vec![ + Step { + name: "one".to_string(), + trigger_artifact: "first.txt".to_string(), + model: "small".to_string(), + script: None, + }, + Step { + name: "default".to_string(), + trigger_artifact: String::new(), + model: "small".to_string(), + script: None, + }, + ], + models: vec![], + prompts: vec![], + scripts: vec![], + }; + let artifact_dir = format!("{}/", dir.path().display()); + assert_eq!( + determine_step(&config, &artifact_dir).unwrap().name, + "default" + ); + } + + #[test] + fn determine_step_empty_trigger_does_not_shadow_real_match() { + let dir = tempfile::tempdir().unwrap(); + std::fs::write(dir.path().join("first.txt"), "").unwrap(); + let config = Config { + version: "0.1.0".to_string(), + show_frontmatter: true, + steps: vec![ + Step { + name: "one".to_string(), + trigger_artifact: "first.txt".to_string(), + model: "small".to_string(), + script: None, + }, + Step { + name: "default".to_string(), + trigger_artifact: String::new(), + model: "small".to_string(), + script: None, + }, + ], + models: vec![], + prompts: vec![], + scripts: vec![], + }; + let artifact_dir = format!("{}/", dir.path().display()); + assert_eq!(determine_step(&config, &artifact_dir).unwrap().name, "one"); + } + + #[test] + fn determine_step_empty_trigger_matches_before_later_artifact() { + // The default is a last resort: it does not beat a later step's + // real artifact, only the absence of any artifact. + let dir = tempfile::tempdir().unwrap(); + std::fs::write(dir.path().join("second.txt"), "").unwrap(); + let config = Config { + version: "0.1.0".to_string(), + show_frontmatter: true, + steps: vec![ + Step { + name: "default".to_string(), + trigger_artifact: String::new(), + model: "small".to_string(), + script: None, + }, + Step { + name: "two".to_string(), + trigger_artifact: "second.txt".to_string(), + model: "high".to_string(), + script: None, + }, + ], + models: vec![], + prompts: vec![], + scripts: vec![], + }; + let artifact_dir = format!("{}/", dir.path().display()); + assert_eq!(determine_step(&config, &artifact_dir).unwrap().name, "two"); + } + + #[test] + fn determine_step_latest_empty_trigger_wins() { + let dir = tempfile::tempdir().unwrap(); + let config = Config { + version: "0.1.0".to_string(), + show_frontmatter: true, + steps: vec![ + Step { + name: "one".to_string(), + trigger_artifact: String::new(), + model: "small".to_string(), + script: None, + }, + Step { + name: "two".to_string(), + trigger_artifact: String::new(), + model: "high".to_string(), + script: None, + }, + ], + models: vec![], + prompts: vec![], + scripts: vec![], + }; + let artifact_dir = format!("{}/", dir.path().display()); + assert_eq!(determine_step(&config, &artifact_dir).unwrap().name, "two"); + } + + #[test] + fn determine_step_no_match_errors_with_step_tag() { + let dir = tempfile::tempdir().unwrap(); + let config = Config { + version: "0.1.0".to_string(), + show_frontmatter: true, + steps: vec![Step { + name: "one".to_string(), + trigger_artifact: "first.txt".to_string(), + model: "small".to_string(), + script: None, + }], + models: vec![], + prompts: vec![], + scripts: vec![], + }; + let artifact_dir = format!("{}/", dir.path().display()); + let err = determine_step(&config, &artifact_dir).unwrap_err(); + assert_eq!(err.source, "step"); + assert!( + err.message.contains("no trigger artifact matched"), + "{}", + err.message + ); + } + + #[test] + fn resolve_model_returns_model_for_matching_name() { + let config = Config { + version: "0.1.0".to_string(), + show_frontmatter: true, + steps: vec![], + models: vec![crate::config::Model { + name: "small".to_string(), + model: "openrouter/deepseek/flash".to_string(), + thinking: "high".to_string(), + }], + prompts: vec![], + scripts: vec![], + }; + assert_eq!( + resolve_model(&config, "small").unwrap().model, + "openrouter/deepseek/flash", + ); + assert_eq!(resolve_model(&config, "small").unwrap().thinking, "high"); + } + + #[test] + fn resolve_model_missing_name_errors_with_model_tag() { + let config = Config { + version: "0.1.0".to_string(), + show_frontmatter: true, + steps: vec![], + models: vec![crate::config::Model { + name: "small".to_string(), + model: "openrouter/deepseek/flash".to_string(), + thinking: "high".to_string(), + }], + prompts: vec![], + scripts: vec![], + }; + let err = resolve_model(&config, "large").unwrap_err(); + assert_eq!(err.source, "model"); + assert!(err.message.contains("no model named"), "{}", err.message); + } + + #[test] + fn resolve_step_returns_matching_step() { + let config = Config { + version: "0.1.0".to_string(), + show_frontmatter: true, + steps: vec![Step { + name: "one".to_string(), + trigger_artifact: "first.txt".to_string(), + model: "small".to_string(), + script: None, + }], + models: vec![], + prompts: vec![], + scripts: vec![], + }; + let step = resolve_step(&config, "one").unwrap(); + assert_eq!(step.name, "one"); + assert_eq!(step.trigger_artifact, "first.txt"); + assert_eq!(step.model, "small"); + } + + #[test] + fn resolve_step_unknown_name_tags_step_error() { + let config = Config { + version: "0.1.0".to_string(), + show_frontmatter: true, + steps: vec![Step { + name: "one".to_string(), + trigger_artifact: String::new(), + model: "small".to_string(), + script: None, + }], + models: vec![], + prompts: vec![], + scripts: vec![], + }; + let err = resolve_step(&config, "nope").unwrap_err(); + assert_eq!(err.source, "step"); + assert!( + err.message.contains("no step named \"nope\""), + "{}", + err.message + ); + } + + #[test] + fn read_config_and_step_explicit_name_resolves_step() { + // A tempdir with NO git repo: happy path must complete without git, + // proving the explicit-name arm never shells out or reads cwd. + let dir = tempfile::tempdir().unwrap(); + std::fs::write( + dir.path().join("orksorksorks.toml"), + concat!( + "version = \"0.1.0\"\n", + "[[steps]]\nname = \"one\"\ntrigger_artifact = \"first.txt\"\nmodel = \"small\"\n", + "[[models]]\nname = \"small\"\nmodel = \"openrouter/deepseek/flash\"\nthinking = \"high\"\n", + "[[prompts]]\nname = \"one\"\ncontent = \"one\"\n", + ), + ) + .unwrap(); + let (cfg, step) = read_config_and_step( + &dir.path().join("orksorksorks.toml"), + crate::config_dir::ConfigPathSource::ExplicitFlag, + Some("one".to_string()), + ) + .unwrap(); + assert!(cfg.steps.len() == 1, "{}", cfg.steps.len()); + assert_eq!(step.name, "one"); + assert_eq!(step.model, "small"); + } + + #[test] + fn read_config_and_step_unknown_name_tags_step() { + let dir = tempfile::tempdir().unwrap(); + std::fs::write( + dir.path().join("orksorksorks.toml"), + "version = \"0.1.0\"\n", + ) + .unwrap(); + let err = read_config_and_step( + &dir.path().join("orksorksorks.toml"), + crate::config_dir::ConfigPathSource::ExplicitFlag, + Some("nope".to_string()), + ) + .unwrap_err(); + assert_eq!(err.source, "step"); + assert!( + err.message.contains("no step named \"nope\""), + "{}", + err.message + ); + } + + #[test] + fn resolve_step_first_match_wins() { + let config = Config { + version: "0.1.0".to_string(), + show_frontmatter: true, + steps: vec![ + Step { + name: "dup".to_string(), + trigger_artifact: String::new(), + model: "small".to_string(), + script: None, + }, + Step { + name: "dup".to_string(), + trigger_artifact: String::new(), + model: "high".to_string(), + script: None, + }, + ], + models: vec![], + prompts: vec![], + scripts: vec![], + }; + let step = resolve_step(&config, "dup").unwrap(); + assert_eq!(step.model, "small"); + } + + #[test] + fn resolve_prompt_returns_content_for_matching_name() { + let config = Config { + version: "0.1.0".to_string(), + show_frontmatter: true, + steps: vec![], + models: vec![], + prompts: vec![crate::config::Prompt { + name: "questions".to_string(), + content: "# Question — Decompose the Task\n".to_string(), + }], + scripts: vec![], + }; + assert_eq!( + resolve_prompt(&config, "questions").unwrap(), + "# Question — Decompose the Task\n", + ); + } + + #[test] + fn resolve_prompt_missing_name_errors_with_prompt_tag() { + let config = Config { + version: "0.1.0".to_string(), + show_frontmatter: true, + steps: vec![], + models: vec![], + prompts: vec![crate::config::Prompt { + name: "questions".to_string(), + content: String::new(), + }], + scripts: vec![], + }; + let err = resolve_prompt(&config, "research").unwrap_err(); + assert_eq!(err.source, "prompt"); + assert!(err.message.contains("no prompt named"), "{}", err.message); + } + + #[test] + fn resolve_script_hit_returns_content() { + let config = Config { + version: "0.1.0".to_string(), + show_frontmatter: true, + steps: vec![], + models: vec![], + prompts: vec![], + scripts: vec![crate::config::Script { + name: "run-one".to_string(), + content: "#!/bin/bash\n".to_string(), + }], + }; + assert_eq!(resolve_script(&config, "run-one").unwrap(), "#!/bin/bash\n",); + } + + #[test] + fn resolve_script_miss_returns_script_tag() { + let config = Config { + version: "0.1.0".to_string(), + show_frontmatter: true, + steps: vec![], + models: vec![], + prompts: vec![], + scripts: vec![crate::config::Script { + name: "run-one".to_string(), + content: String::new(), + }], + }; + let err = resolve_script(&config, "nope").unwrap_err(); + assert_eq!(err.source, "script"); + assert!(err.message.contains("no script named"), "{}", err.message); + } + + #[test] + fn resolve_script_first_match_wins() { + let config = Config { + version: "0.1.0".to_string(), + show_frontmatter: true, + steps: vec![], + models: vec![], + prompts: vec![], + scripts: vec![ + crate::config::Script { + name: "dup".to_string(), + content: "first\n".to_string(), + }, + crate::config::Script { + name: "dup".to_string(), + content: "second\n".to_string(), + }, + ], + }; + assert_eq!(resolve_script(&config, "dup").unwrap(), "first\n",); + } +} diff --git a/src/commands/script.rs b/src/commands/script.rs new file mode 100644 index 0000000..bb464f4 --- /dev/null +++ b/src/commands/script.rs @@ -0,0 +1,58 @@ +use super::resolve; +use crate::errors::Error; + +/// Handle the `script` subcommand: read the config and return the raw +/// `[[scripts]]` content referenced by the current step's `script` field, +/// or by the explicitly named step when `step_name` overrides detection. +/// No frontmatter is emitted (the content is meant to be run/piped). +pub(crate) fn script_command( + path: &std::path::Path, + source: crate::config_dir::ConfigPathSource, + step_name: Option, +) -> Result { + let (cfg, step) = resolve::read_config_and_step(path, source, step_name)?; + let script_name = step.script.ok_or_else(|| { + Error::new( + "script", + &format!("step {:?} has no script configured", step.name), + ) + })?; + resolve::resolve_script(&cfg, &script_name) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn script_command_step_without_script_errors() { + // Step `one` exists (validated config with matching prompt/model) but + // has no `script` key: `read_config` succeeds, then the explicit-name + // path resolves the step and fails on the missing script reference + // with the `"script"` tag — before resolve_script ever runs. The + // explicit-name path avoids git, so no repo is needed. + let dir = tempfile::tempdir().unwrap(); + std::fs::write( + dir.path().join("orksorksorks.toml"), + concat!( + "version = \"0.1.0\"\n", + "[[steps]]\nname = \"one\"\ntrigger_artifact = \"first.txt\"\nmodel = \"small\"\n", + "[[models]]\nname = \"small\"\nmodel = \"openrouter/deepseek/flash\"\nthinking = \"high\"\n", + "[[prompts]]\nname = \"one\"\ncontent = \"one\"\n", + ), + ) + .unwrap(); + let err = script_command( + &dir.path().join("orksorksorks.toml"), + crate::config_dir::ConfigPathSource::ExplicitFlag, + Some("one".to_string()), + ) + .unwrap_err(); + assert_eq!(err.source, "script"); + assert!( + err.message.contains("has no script configured"), + "{}", + err.message + ); + } +} diff --git a/src/commands/step.rs b/src/commands/step.rs new file mode 100644 index 0000000..2d16a98 --- /dev/null +++ b/src/commands/step.rs @@ -0,0 +1,71 @@ +use super::resolve; +use crate::errors::Error; + +/// Handle the `step` subcommand: read the config and return the current step +/// name. With `--step `, the name replaces artifact-based derivation +/// (no git is consulted); otherwise the step is derived from cwd + git +/// branch + trigger artifacts. +/// +/// `path` is the already-resolved config location: either the explicit +/// `--config` argument or the config-directory default (`config_dir.rs`). +/// `read_config` runs before git resolution so a missing/unreadable config +/// deterministically fails with `"io"`. +pub(crate) fn step_command( + path: &std::path::Path, + source: crate::config_dir::ConfigPathSource, + step: Option, +) -> Result { + let (_, step) = resolve::read_config_and_step(path, source, step)?; + Ok(step.name) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn step_command_flag_succeeds_in_non_git_dir() { + // A tempdir with NO git repo: bare step would fail in git::current_branch, + // so success proves `--step` never shells out to git or reads cwd. + let dir = tempfile::tempdir().unwrap(); + std::fs::write( + dir.path().join("orksorksorks.toml"), + concat!( + "version = \"0.1.0\"\n", + "[[steps]]\nname = \"one\"\ntrigger_artifact = \"first.txt\"\nmodel = \"small\"\n", + "[[models]]\nname = \"small\"\nmodel = \"openrouter/deepseek/flash\"\nthinking = \"high\"\n", + "[[prompts]]\nname = \"one\"\ncontent = \"one\"\n", + ), + ) + .unwrap(); + let out = step_command( + &dir.path().join("orksorksorks.toml"), + crate::config_dir::ConfigPathSource::ExplicitFlag, + Some("one".to_string()), + ) + .unwrap(); + assert_eq!(out, "one"); + } + + #[test] + fn step_command_flag_unknown_name_tags_step() { + let dir = tempfile::tempdir().unwrap(); + std::fs::write( + dir.path().join("orksorksorks.toml"), + "version = \"0.1.0\"\n", + ) + .unwrap(); + let err = step_command( + &dir.path().join("orksorksorks.toml"), + crate::config_dir::ConfigPathSource::ExplicitFlag, + Some("nope".to_string()), + ) + .unwrap_err(); + assert_eq!(err.source, "step"); + assert!( + err.message.contains("no step named \"nope\""), + "{}", + err.message + ); + } +} diff --git a/src/commands/thinking.rs b/src/commands/thinking.rs new file mode 100644 index 0000000..6ec189e --- /dev/null +++ b/src/commands/thinking.rs @@ -0,0 +1,16 @@ +use super::resolve; +use crate::errors::Error; + +/// Handle the `thinking` subcommand: determine the current step (via +/// `--step ` when given, else artifact derivation), read the model +/// *name* it references, and resolve that name against `config.models` to +/// its thinking-budget value. +pub(crate) fn thinking_command( + path: &std::path::Path, + source: crate::config_dir::ConfigPathSource, + step: Option, +) -> Result { + let (cfg, step) = resolve::read_config_and_step(path, source, step)?; + let model = resolve::resolve_model(&cfg, &step.model)?; + Ok(model.thinking) +}