From bb3ae7e10de083618f499bb80261d2a2c656015b Mon Sep 17 00:00:00 2001 From: NewMe505 Date: Tue, 27 Oct 2026 22:41:56 -0700 Subject: [PATCH 1/3] fix(product): worker python is the product's env; advisor-cap text; skip defers (#69) I-114: a Claude Code worker turn, LocalSandbox.exec and the local app start run with the product interpreter's directory first on PATH (with_interpreter_first), so `python` is the product's environment with its approved dependencies, not SCIO's own under uv run. I-115: the advisor-cap halt names the cap that fired: the run's total or one call per step. I-116 (the owner's decision): skip defers a feature. It stays open, a skip finishes it only in its own milestone (FeatureRun.milestone), per-milestone costs count their own runs, the release refuses while a feature is not done, the PR closes only passed features' issues, and update_progress re-reads PRODUCT.md before it routes. I-117: a subagent's shell-script edit, logged; guardrail 17 extended. Guardrails 35 to 37 (20 to 34 are taken by PR #70). Built by three parallel agents with owned files; integration, the LocalSandbox and app-start sweep, docs and ledger by the orchestrator. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 3 + README.md | 6 +- docs/ARCHITECTURE.md | 2 +- docs/ISSUE_LEDGER.md | 85 ++++++++++ docs/LESSONS_LEARNED.md | 5 +- docs/adr/0011-milestone-loop.md | 2 +- scio/aci/apps.py | 9 +- scio/aci/sandbox/local.py | 25 ++- scio/cli.py | 3 + scio/harness/providers/claude_code.py | 11 +- scio/loop/claude_code_worker.py | 36 +++- scio/loop/escalation.py | 16 +- scio/product/build.py | 88 ++++++++-- scio/product/progress.py | 11 +- scio/state/models.py | 31 +++- tests/graph/test_claude_code_backend_t0.py | 1 + tests/graph/test_product_build_fake.py | 188 ++++++++++++++++++++- tests/unit/test_claude_code_backend.py | 133 ++++++++++++++- tests/unit/test_escalation.py | 48 +++++- tests/unit/test_product_progress.py | 48 +++++- tests/unit/test_runtime.py | 24 +++ tests/unit/test_state.py | 81 +++++++++ 22 files changed, 813 insertions(+), 43 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8ee58fe..cc941aa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,9 @@ Format: [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). Versioning: [S - CLAUDE.md and CONTRIBUTING protocols 4 and 5: Always/Never rules each backed by a guard, and a bottleneck audit after every pass (issue #61). ### Fixed +- On the Claude Code worker backend a feature's `python` is the product's environment, with its approved dependencies, not SCIO's own: the worker turn's PATH starts with the product interpreter's directory (I-114). +- The advisor-cap halt names the cap that fired: the feature's total, or the one call per step (I-115). +- `skip` at a halted feature defers it to the next milestone instead of dropping it, and the release refuses while a feature in `PRODUCT.md` is not done; removing a feature from `PRODUCT.md` drops it (I-116, the owner's decision). - `scio build --gh` no longer fails at its first issue on a repository without the `product-mode` label: the GitHub client creates a missing label before it names it (an existing one is left as it is), and a GitHub refusal stops `scio build` with the error and how to resume instead of a traceback (I-113). - A hand edit of `PRODUCT.md` is no longer lost: the milestone's sync commit records it, a fresh feature attempt commits one made while the build waited, SCIO's own checkouts and resets carry it, and the next milestone of the same thread re-reads it (I-109). - A quality bar can no longer promise tools the stack's profile does not use: each profile names its fixed tools, the interviewer and the planner see them, and the normaliser replaces a contradicting line with the profile's fact, quoting the original (I-107). diff --git a/README.md b/README.md index 82753db..f5ad382 100644 --- a/README.md +++ b/README.md @@ -2,7 +2,7 @@ A model-agnostic, multi-agent framework built on LangGraph that turns a prompt into product-ready software. Today it takes one software task, from a spec or a GitHub Issue, to a merged, tested and documented pull request, reliably and live. Product Mode (M6 to M11) extends that to whole products, from a prompt or a `PRODUCT.md`, in Python or TypeScript, one approved milestone at a time, through a release gate to a tagged release. -> Status: **v0.8.0 released; Product Mode M6 to M11 and the post-M11 follow-ups done.** The product eval suite passes live on one commit: a CLI todo app (p001), a REST API (p002) and a web app graded in a real browser (p003), each built from a fixed `PRODUCT.md` to a released v0.1.0 whose hidden black-box tests pass ($11.63 estimated for all three, nothing billed). `scio new` and `scio build` have also bootstrapped and built live products on the Claude Code backend: `todo_tags` (M8), `bookmarks_api` (M9: a Python HTTP service with a third-party dependency, black-box acceptance against the started server, $5.38 estimated, nothing billed) and `notes_service` (M10: a TypeScript HTTP service with zod, eslint, tsc, npm audit and black-box acceptance all green, $4.61 estimated, nothing billed). Post-M11 follow-ups ([issue #58](https://github.com/NewMe505/SCIO/issues/58), [PR #59](https://github.com/NewMe505/SCIO/pull/59), [ADR 0016](docs/adr/0016-post-m11-follow-ups.md)) are done and released in 0.8.0: squash-safe `--gh` merges and a release PR, `scio upgrade` (live: `bookmarks_api` upgraded and its M2 built, $4.56 estimated), Node in the Docker sandbox, `run_app` in a container, parallel CI tests. Merged since: Always/Never guardrails enforced by a docs check and a push-gate hook (#61). In progress: a live product test, `graphcalc` (a TI-Nspire-style graphing calculator built by v0.8.0), and the fixes it finds (#64, merged: the quality bar follows the stack, hand edits of PRODUCT.md survive, `scio new` threads report, spend spans invocations; [issue #66](https://github.com/NewMe505/SCIO/issues/66): GitHub labels are created before use). Task mode is proven live: ten regression tasks at pass^5 = 1.00 offline, nightly live suites green. Roadmap: [docs/PRODUCT_MODE.md](docs/PRODUCT_MODE.md); decision records: [docs/adr/](docs/adr/); board: [SCIO Product Mode](https://github.com/users/NewMe505/projects/1); milestones: [M6 to M11](https://github.com/NewMe505/SCIO/milestones). +> Status: **v0.8.0 released; Product Mode M6 to M11 and the post-M11 follow-ups done.** The product eval suite passes live on one commit: a CLI todo app (p001), a REST API (p002) and a web app graded in a real browser (p003), each built from a fixed `PRODUCT.md` to a released v0.1.0 whose hidden black-box tests pass ($11.63 estimated for all three, nothing billed). `scio new` and `scio build` have also bootstrapped and built live products on the Claude Code backend: `todo_tags` (M8), `bookmarks_api` (M9: a Python HTTP service with a third-party dependency, black-box acceptance against the started server, $5.38 estimated, nothing billed) and `notes_service` (M10: a TypeScript HTTP service with zod, eslint, tsc, npm audit and black-box acceptance all green, $4.61 estimated, nothing billed). Post-M11 follow-ups ([issue #58](https://github.com/NewMe505/SCIO/issues/58), [PR #59](https://github.com/NewMe505/SCIO/pull/59), [ADR 0016](docs/adr/0016-post-m11-follow-ups.md)) are done and released in 0.8.0: squash-safe `--gh` merges and a release PR, `scio upgrade` (live: `bookmarks_api` upgraded and its M2 built, $4.56 estimated), Node in the Docker sandbox, `run_app` in a container, parallel CI tests. Merged since: Always/Never guardrails enforced by a docs check and a push-gate hook (#61). In progress: a live product test, `graphcalc` (a TI-Nspire-style graphing calculator built by v0.8.0), and the fixes it finds (#64, merged: the quality bar follows the stack, hand edits of PRODUCT.md survive, `scio new` threads report, spend spans invocations; #66, merged: GitHub labels are created before use; [issue #69](https://github.com/NewMe505/SCIO/issues/69): the worker's `python` is the product's environment, `skip` defers a feature). Task mode is proven live: ten regression tasks at pass^5 = 1.00 offline, nightly live suites green. Roadmap: [docs/PRODUCT_MODE.md](docs/PRODUCT_MODE.md); decision records: [docs/adr/](docs/adr/); board: [SCIO Product Mode](https://github.com/users/NewMe505/projects/1); milestones: [M6 to M11](https://github.com/NewMe505/SCIO/milestones). ## Product Mode in one paragraph A product orchestrator sits above the proven task graph. It interviews you (two to four Opus questions), writes a normalised `PRODUCT.md`, picks a toolchain profile, and bootstraps a repository with CI and a walking skeleton. Then, milestone by milestone, it decomposes work into GitHub issues, runs each feature through the task graph with the tier pinned, verifies the integrated result against a fixed quality bar (tests with a coverage floor, lint, types, build, smoke and e2e against the running app, dependency audit, secret scan, working README instructions), and opens one milestone PR for a human to merge. Any failing check overrides any model verdict. There is one writer, so a product builds serially; there is no product-level dollar cap by default, but per-feature tier caps, stall caps and iteration caps remain as runaway guards, and cost is reported at every approval stop. @@ -77,6 +77,8 @@ the task graph on its own checkpoint thread on branch `milestone/`. After the profile's full suite and lint run on the branch, an Opus milestone gate gives a verdict that any failing check overrides, and one milestone PR waits for you. A crash at any point resumes without a duplicated issue, commit or PR; a feature that halts stops for one decision (`retry`, `skip`, or stop). +`skip` defers the feature to the next milestone; the release waits until every feature in `PRODUCT.md` +is done, so a feature leaves the product only when you remove it from `PRODUCT.md`. Every feature's verify also holds it to the product's coverage floor (`SCIO_COVERAGE_FLOOR`, 80% of the product's own code by default). When no milestone remains, the release gate (M11) runs the whole bar on `main` (coverage, secret scan, release files, the README's run commands, black-box acceptance) @@ -89,7 +91,7 @@ uv run scio build --product ./todo [--gh you/todo] # runs the next milest uv run scio build --product ./todo --decision merged # after you merged the PR (offline: "merge") uv run scio build --product ./todo --decision release # at the release gate (or "stop") uv run scio build --product ./todo --decision "merge and stop" # record the milestone, end the thread -uv run scio build --product ./todo --decision skip # or retry / stop, when a feature halted +uv run scio build --product ./todo --decision skip # defer it; or retry / stop, when a feature halted uv run scio status --product ./todo # progress.json as text uv run scio report --thread build-todo_cli # feature runs, cost, escalations uv run scio upgrade --product ./todo # bring an older product to today's bar diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index d05471a..041ceb0 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -895,7 +895,7 @@ Assumptions are numbered F‑n and referenced above. Change one by PR; record th - The real end-to-end run (`NewMe505/scio-eval-sandbox`, issue #1 → PR #2) found that `git add -A` committed bytecode caches; `prepare_repo` now writes repo-local excludes (I-028). ### Product mode graphs (M7, M8) -Two graphs sit above the task graph (ADR 0007, ADR 0010, ADR 0011). `scio new` runs `product_new`: interview, normalised `PRODUCT.md`, bootstrap, optional publish, with an interrupt at every human stop. `scio build` runs `product_build`, the milestone loop: the orchestrator picks the first open feature, the milestone planner groups up to four, issues are created idempotently, each feature runs through the task graph as a subgraph on its own checkpoint thread (the `feature` node loops until the milestone's features have passed or been skipped), the integration checks and the milestone gate run on the branch, one PR waits for the human merge, and `update_progress` loops back to plan the next milestone. Both diagrams below are generated from the compiled graphs. +Two graphs sit above the task graph (ADR 0007, ADR 0010, ADR 0011). `scio new` runs `product_new`: interview, normalised `PRODUCT.md`, bootstrap, optional publish, with an interrupt at every human stop. `scio build` runs `product_build`, the milestone loop: the orchestrator picks the first open feature, the milestone planner groups up to four, issues are created idempotently, each feature runs through the task graph as a subgraph on its own checkpoint thread (the `feature` node loops until the milestone's features have passed or been skipped; a skipped feature is deferred, so it stays open and the next milestone plans it again, I-116), the integration checks and the milestone gate run on the branch, one PR waits for the human merge, and `update_progress` loops back to plan the next milestone. Both diagrams below are generated from the compiled graphs. ```mermaid diff --git a/docs/ISSUE_LEDGER.md b/docs/ISSUE_LEDGER.md index 35101d8..e82bc37 100644 --- a/docs/ISSUE_LEDGER.md +++ b/docs/ISSUE_LEDGER.md @@ -964,3 +964,88 @@ Template: - The fake: it now refuses a missing label and a missing milestone, as GitHub does. - Guard: with the strict fake, `test_github_mode_survives_squash_merged_milestone_prs` fails on `main` with S4's exact error (`could not add label: 'product-mode' not found`) and passes now. Also `test_an_issue_label_is_created_once_and_an_existing_one_is_left_alone`, `test_a_github_refusal_stops_scio_build_with_a_resume_hint_not_a_traceback`, and guardrails 18 and 19. - Resolved: 2026-10-27 · Status: resolved + +### I-114 · On the Claude Code worker backend a feature's `python` is SCIO's own environment, not the product's +- Found: 2026-10-28 · the graphcalc live run (RUN_LOG S5a), thread `build-graphcalc-m1`, P4 · the coder reported "plain python resolves to SCIO's own venv (SCIO\.venv\Scripts\python.exe), which has pytest but no sympy and no pip"; every other way of invoking the product's interpreter was denied by the allow-rules; 36 tool calls went on it (`tool_cap`), and a stray probe file it made and deleted tripped `plan_drift` +- Symptom: P4 halted with `advisor_cap` after the escalations; the product's approved dependency (sympy) is in `graphcalc/.scio/sandbox/venv` but unreachable from the worker's own commands + - The retry hit the same wall. a1 (03:35 to 04:01, $3.22): 36 tool calls, `advisor cap hit (2/3, step S1a)`, `plan_drift`. a2 (04:19 to 04:32, $1.93): 34 tool calls, `advisor cap hit (1/3, step S1)`, `low_confidence`, "hit a hard environment blocker before writing the diff". + - Calling the product venv's `python.exe` by path was denied too: the worker's allow-rule is `Bash(python *)`. + - Both attempts failed on the environment, never on the math. In a2 the planner also named a file that does not exist (`graphcalc/evaluator.py`); the advisor replanned, which is the designed path, so it gets no entry of its own. +- Root cause, confirmed: `claude_runner` starts `claude -p` with `subprocess_env()`, which is SCIO's own environment. Under `uv run` that puts SCIO's `.venv` first on PATH, so the worker turn's Bash `python` was SCIO's interpreter. The product interpreter (`sandbox.python`, set by `ensure_product_env`) was used only where SCIO itself rewrote a literal `python` (`resolve_command`). It went unseen because SCIO depends on pyyaml, the only dependency of the earlier live products; graphcalc's sympy is not in SCIO's environment. +- Fix: + - `with_interpreter_first(env, interpreter)` (`scio/aci/sandbox/local.py`) puts the product interpreter's directory first on PATH. It keeps one PATH key in its own spelling (`Path` on Windows), drops a repeat, and sets `VIRTUAL_ENV` to the environment root. It changes nothing when the stack has no interpreter of its own. + - `worker_env` uses it for both of a worker turn's `claude -p` calls (the turn and the summary re-ask), built per turn because `sandbox.python` can be set after the worker exists. + - The `Runner` gains an optional `env`. + - `BACKEND_NOTE` tells the worker that `python` already runs the product's environment. + - The worker part was done by a parallel agent that owned the backend files. +- Sweep: `grep -rn "subprocess_env()\|subprocess.run(\|Popen(" scio`, run by the agent. + - The worker: fixed. + - `LocalSandbox.exec` passed SCIO's PATH to every product command, rewriting only a leading `python`. Fixed: it now uses `with_interpreter_first`, so a product command or a child the product spawns that starts `python` by name gets the product's. + - The local `start_app` Popen: fixed the same way. + - Not affected: the Docker sandbox (product code runs in the container's environment), the GitHub client (`gh`), the tool-less provider calls (`--tools ""`, no cwd), the eval solver (SCIO's interpreter on purpose; its product command already uses `sb.python`), and the scaffold's `sys.executable`. + - The graph test's fake runner gained `env`. +- Guard: + - `test_worker_turn_runs_with_the_product_interpreter_first_on_path` and `test_worker_without_a_product_interpreter_runs_with_subprocess_env`. + - `test_worker_env_keeps_one_path_variable_in_its_own_spelling`, `test_claude_runner_uses_a_given_env_else_subprocess_env` and `test_worker_system_prompt_says_python_is_the_product_environment`. All five fail on `main`. + - `test_every_local_command_runs_with_the_product_interpreter_first_on_path`. + - Guardrail 35. + - Live: on the next run, the coder's `python -c "import sys; print(sys.executable)"` should point under `.scio/sandbox`. The tests stop at the process boundary. +- Resolved: 2026-10-28 · Status: resolved + +### I-115 · The advisor-cap halt names the feature's total when the per-step cap stopped it +- Found: 2026-10-28 · the graphcalc live run (RUN_LOG S5b) · halt text `advisor cap hit (2/3, step S1a)`, trigger `plan_drift`; `advisor_allowed` also caps advisor calls at one per step +- Symptom: "2/3" reads as headroom left, while the cap that stopped the feature was the step's 1/1 +- Root cause: `decide` (`scio/loop/escalation.py`) printed the run's total whichever of `Budget.advisor_allowed`'s two conditions refused: the run's cap, or one advisor call per step. +- Fix: the halt checks `used >= cap` first and then says `advisor cap hit (3/3 for this run, step S1a)`; the total wins when both caps are spent. Otherwise it says `advisor already consulted on step S1a (one call per step; 2/3 for this run)`. "Run", not "feature": the advisor budget is per run, and product mode gives each feature its own run. +- Sweep: `grep -rn "advisor cap\|advisor_calls_used\|advisor_calls_max\|advisor_allowed\|advisor_calls_on_step" scio tests docs`. + - `review_decision` returns None with no text. + - The worker's and the report's `used/max` lines are spend summaries, not cap claims. + - `verify.py`'s ledger line and `checkpoint_gate.py`'s interrupt echo the triage reason, so they pick up the fix. + - No test, eval or doc asserted the old string. + - Done by a parallel agent that owned `escalation.py` and its test file; the old file run on the same state gave the old text. +- Guard: `test_halt_reason_names_the_per_step_cap_when_the_total_has_headroom` (fails on `main`: `advisor cap hit (2/3, step S1)`), `test_halt_reason_names_the_run_total_when_it_is_spent`, and an exact-text check in `test_cap_per_step_and_per_run_route_to_halt_with_advisor_cap_exit`, plus guardrail 36 +- Resolved: 2026-10-28 · Status: resolved + +### I-116 · `skip` at a halted feature drops the feature from the product without a word +- Found: 2026-10-28 · the graphcalc live run (RUN_LOG S5, note) · `skip` marks the feature skipped, and `open_feature_ids` (`scio/product/progress.py`) never plans it again +- Symptom (expected): a feature in the approved PRODUCT.md never gets built, and nothing at the release says so +- Root cause, confirmed: ADR 0011 named `skip` without saying what it means afterwards, and the code made it permanent. + - `open_feature_ids` left `skipped` out, so the next milestone never planned the feature. + - Behind that was a second gap: `ProductBuildState.next_feature` counted a skipped run as finished across milestones, so even a re-planned feature would have been passed over in the same build thread. + - The release never compared PRODUCT.md with progress. + - A milestone PR said `Closes #n` for a skipped feature's issue. + - `update_progress` routed to the release without re-reading PRODUCT.md, so a feature added at the last merge stop was left out (the I-109 sweep had missed this path). +- Fix: the owner's decision is that skip defers. ADR 0011 is amended, and the README and ARCHITECTURE say the same. + - `open_feature_ids` returns every feature that is not `done`. + - `FeatureRun.milestone` is set by the `feature` node. `next_feature` counts a skip as finished only within its own milestone; a run recorded before this change has None and keeps the old rule. + - `ProductBuildState.milestone_runs()` scopes the milestone's cost, attempts and NOTES line to that milestone's runs. + - The halt question says that skip defers, and that removing the feature from PRODUCT.md drops it. + - The skip's ledger line says "deferred". + - `plan_milestone` logs which deferred features it plans again. + - `release_check` refuses (`release_failed`) while a feature of the spec is not `done`, naming each one. + - The PR body closes issues only for passed features (`Refs #n (Pn deferred, not closed)` otherwise). + - `update_progress` re-reads PRODUCT.md before it routes. + - `scio build` prints the halt's reason. + - Done by a parallel agent that owned `progress.py`, `build.py` and `models.py`; its first workaround, which relabelled old skipped runs, was replaced by the milestone field so nothing is counted twice. +- Known limits, not reachable in any existing thread (graphcalc never answered skip): + - A thread that recorded a skip under the old code and is paused at its gate or merge stop would pass the feature over again, because the old run has milestone None. + - A re-read at the last stop can leave `progress.json` uncommitted on `main` until the next build's plan commits it, as `load`'s sync does. +- Sweep: `grep -rn '"skipped"\|skipped\|next_feature\|open_feature_ids\|\.status ==\|\.status !=\|status in (' scio/product scio/cli.py scio/state/models.py evals scio/graph` and `grep -rn "FeatureRun(\|feature_runs\|latest_run\|next_feature" scio evals tests`. + - Fixed: every reader that treated `skipped` as finished, the PR body's `Closes`, and the per-milestone cost readers. + - Correct as they are: `latest_run` for the attempt number (it counts across milestones on purpose, so a deferred feature gets attempt 2 on a new thread), the whole-thread sums (`total_usd`, the report, the eval solver), and the display-only readers. + - The eval solver answers `stop`, never `skip`. +- Guard: + - `test_a_skipped_feature_is_deferred_to_the_next_milestone_not_dropped`, end to end through the Fake. M2 plans P1 and P2 again, P1 runs on `-p1-a2`, and each milestone's cost is its own. + - `test_a_skip_finishes_a_feature_only_in_the_milestone_it_was_skipped_in` and `test_a_skipped_feature_stays_open_and_is_planned_first_again`. + - `test_the_release_gate_refuses_while_a_feature_is_not_done` and `test_a_milestone_pr_closes_only_the_issues_of_passed_features`. + - `test_a_feature_added_at_the_last_merge_stop_is_planned_not_left_out_of_the_release`. + - All fail on `main`. Plus guardrail 37. +- Resolved: 2026-10-28 · Status: resolved + +### I-117 · A parallel agent made one edit through a shell-embedded script with `\n` escapes (I-112's rule again) +- Found: 2026-10-28 · issue #69, the I-116 agent's own report · one edit (removing its first test and its imports) went through a shell-embedded Python script holding `\n` escapes, which CLAUDE.md forbids; the brief repeated the rule +- Symptom: none. The agent checked the file afterwards (LF only, content correct), and the full check passes +- Root cause: the rule is `manual` (guardrail 17), and a subagent briefed with it still reached for the shell when one edit spanned several places +- Fix: the file was verified, nothing to undo +- Sweep: the other two agents' reports name no shell-script edits; the orchestrator's script edits this session asserted their anchors. Guard: guardrail 17 now cites I-117, and the brief template for parallel agents names the rule in its first lines. The working tree's line-ending check (`file ... | grep CRLF`, run at the gate) printed nothing +- Resolved: 2026-10-28 · Status: resolved diff --git a/docs/LESSONS_LEARNED.md b/docs/LESSONS_LEARNED.md index fff799d..d776689 100644 --- a/docs/LESSONS_LEARNED.md +++ b/docs/LESSONS_LEARNED.md @@ -22,9 +22,12 @@ Every bug and every correction becomes one rule here (the owner's rule, 2026-10- 14. **Always** read a checkpoint with the model of the graph that wrote it; every kind of thread (task, `scio new`, `scio build`) has a report (I-108). Guard: `symbol:load_product_state`, `test:test_cli_build_status_and_report` 15. **Always** record a thread's spend across processes: a resume carries the checkpoint's spend into the new process, and the guard still caps each process (I-110). Guard: `symbol:carry_spend`, `test:test_a_thread_records_the_spend_of_every_invocation` 16. **Always** match a Python definition by name with its generic form too (`def name[T](`), not only `def name(` (I-111). Guard: `test:test_guard_exists_looks_in_the_repository` -17. **Always** make an edit whose text contains a backslash with the Edit or Write tool, never through a shell-embedded script; a script edit asserts its anchor count before it writes (I-071, I-072, I-112). Guard: `manual:` a hook cannot tell a mangled backslash from an intended `\n` escape; the anchor assert turns a miss into a loud failure, never a silent wrong edit +17. **Always** make an edit whose text contains a backslash with the Edit or Write tool, never through a shell-embedded script; a script edit asserts its anchor count before it writes, and a parallel agent's brief repeats this rule in its first lines (I-071, I-072, I-112, I-117). Guard: `manual:` a hook cannot tell a mangled backslash from an intended `\n` escape; the anchor assert turns a miss into a loud failure, never a silent wrong edit 18. **Never** let a test fake accept what the real service refuses; a fake GitHub refuses a missing label or milestone, and SCIO creates a resource before it names it (I-113). Guard: `symbol:ensure_label`, `test:test_an_issue_label_is_created_once_and_an_existing_one_is_left_alone` 19. **Never** end a CLI command with a traceback when an external service refuses a step; say what failed and how to resume from the checkpoint (I-100, I-113). Guard: `test:test_a_github_refusal_stops_scio_build_with_a_resume_hint_not_a_traceback` +35. **Always** run a product's commands, and a worker turn, with the product's interpreter first on PATH, never with SCIO's inherited environment (I-114). Guard: `symbol:with_interpreter_first`, `test:test_worker_turn_runs_with_the_product_interpreter_first_on_path`, `test:test_every_local_command_runs_with_the_product_interpreter_first_on_path` +36. **Always** name the cap that stopped a run (the run's total or one advisor call per step), never a counter that hides it (I-115). Guard: `test:test_halt_reason_names_the_per_step_cap_when_the_total_has_headroom` +37. **Never** let a human decision silently drop a feature from the product: `skip` defers it, a skip finishes a feature only in its own milestone, and the release refuses while any feature in PRODUCT.md is not done (I-116). Guard: `test:test_a_skipped_feature_is_deferred_to_the_next_milestone_not_dropped`, `test:test_the_release_gate_refuses_while_a_feature_is_not_done`, `test:test_a_skip_finishes_a_feature_only_in_the_milestone_it_was_skipped_in` ## Standing pre-flight list (apply at the start of every milestone) diff --git a/docs/adr/0011-milestone-loop.md b/docs/adr/0011-milestone-loop.md index 3f498ef..4c9e516 100644 --- a/docs/adr/0011-milestone-loop.md +++ b/docs/adr/0011-milestone-loop.md @@ -12,7 +12,7 @@ After M7 a repository exists with `PRODUCT.md` (features as black-box criteria, 2. **A second graph, the same mechanics** (`scio/product/build.py`): `load -> plan_milestone -> issues -> feature* -> integrate -> milestone_gate -> [gate_decision] -> milestone_pr -> merge_wait -> update_progress -> plan_milestone | done` over `ProductBuildState`, compiled with the shared checkpointer, durability `sync`, thread `build-` (one build thread per product; `scio build` resumes it until the product is done). 3. **The orchestrator picks the work; the planner proposes the grouping** (I-046). The next feature is always the first open one in `PRODUCT.md` order. The `milestone_planner` role (Opus, `MilestonePlanOutput`) proposes which open features form the milestone and a tier per feature; `normalise_milestone` keeps only a consecutive prefix starting at the first open feature, caps it at four, defaults tiers to T1 and records every change as a ledger note. The milestone branch is `milestone/` from `main`. 4. **Tracking is idempotent by title.** With a GitHub client: `ensure_milestone("Mn: title")` and `ensure_issue("Pk: description", ...)` look up the title before creating, and each is wrapped in `run_effect` with a probe (`find_milestone`, `find_issue`). Offline: one Markdown file per feature under `.scio/product/issues/`, committed once on the milestone branch with a `[scio:issues:Mn]` marker. -5. **Each feature attempt is a task subgraph on its own thread.** `feature_context` derives a `RunContext` from the orchestrator's: the same sandbox (the product repository), effect log and prompts; a `Harness.child` with its own tier-pinned `BudgetGuard` and cost ledger; no GitHub client. The thread id is deterministic, `build---a`, so a crash mid-feature resumes the same subgraph (`thread_started` probes the checkpoint store); `run_graph`/`resume_graph` accept the orchestrator's open saver so both graphs share one store. The `TaskSpec` handed over carries the feature's criteria as acceptance, `milestone/` as base branch, the issue number and the pinned tier. The task graph's `merge_wait` is answered by the orchestrator, which fast-forwards `milestone/` to the feature branch (effect with an `is-ancestor` probe). Any other pause, or a non-success exit, becomes **one** product-level interrupt: `retry` (a new attempt, new thread), `skip`, or stop (`feature_failed`). The subgraph's own interrupt is closed with the same answer. +5. **Each feature attempt is a task subgraph on its own thread.** `feature_context` derives a `RunContext` from the orchestrator's: the same sandbox (the product repository), effect log and prompts; a `Harness.child` with its own tier-pinned `BudgetGuard` and cost ledger; no GitHub client. The thread id is deterministic, `build---a`, so a crash mid-feature resumes the same subgraph (`thread_started` probes the checkpoint store); `run_graph`/`resume_graph` accept the orchestrator's open saver so both graphs share one store. The `TaskSpec` handed over carries the feature's criteria as acceptance, `milestone/` as base branch, the issue number and the pinned tier. The task graph's `merge_wait` is answered by the orchestrator, which fast-forwards `milestone/` to the feature branch (effect with an `is-ancestor` probe). Any other pause, or a non-success exit, becomes **one** product-level interrupt: `retry` (a new attempt, new thread), `skip`, or stop (`feature_failed`). The subgraph's own interrupt is closed with the same answer. *Amended 2026-10-28 (I-116, the owner's decision):* `skip` defers the feature to the next milestone. A skipped feature stays open, the release refuses while any feature in PRODUCT.md is not done, and a feature leaves the product only when it is removed from PRODUCT.md. 6. **Ground truth outranks the gate.** `integrate` runs the profile's full suite and lint on the milestone branch; a failure halts with `integration_failed` before any model is asked. `milestone_gate` (Opus, `GateOutput`) sees the features, each run's outcome, the integration output and a diff stat; a `pass` verdict is overridden to `escalate_human` when the checks failed or a feature is not `passed`. A non-pass verdict on green checks is one human question (`proceed` or stop). 7. **One PR per milestone, one human merge.** The PR body (cost per feature and for the orchestrator, escalations resolved/total, plan versus done, `Closes #n` per issue, the integration output, the gate verdict) is pushed with the branch and opened as a draft then marked ready; offline it is written to `.scio/product/milestones/Mn-pr.md`. `merge_wait` interrupts; `merged` with GitHub checks `pr_merged` and pulls `main` (asks again if the PR is still open); `merge` offline merges `--no-ff` with a `[scio:merge:Mn]` marker. `update_progress` records the milestone and its features (status, commit, thread, tier, issue, cost) in `progress.json`, prepends a line to `NOTES.md`, commits on `main` with `[scio:progress:Mn]`, and loops to the next milestone while features remain, unless the human answered `merge and stop`, in which case the thread ends after the record (the first live milestone needed a stop before the next one started spending). Threads are named per starting milestone, `build--m`, and `scio build` starts a fresh thread (`-r2`, ...) when the one for the next milestone has already ended. 8. **Verify counts new files.** The first Fake build found that `git diff --stat ` ignores untracked files, so a feature delivered only by new files read as "no changes" (I-063). `diff_stat` now appends every untracked file and the full diff uses `git add -N` first. diff --git a/scio/aci/apps.py b/scio/aci/apps.py index 6792aaf..2efc263 100644 --- a/scio/aci/apps.py +++ b/scio/aci/apps.py @@ -9,6 +9,7 @@ from __future__ import annotations +import os import socket import subprocess import time @@ -18,7 +19,12 @@ from dataclasses import dataclass, field from scio.aci.sandbox.base import Sandbox -from scio.aci.sandbox.local import INTERPRETER_WORD, LocalSandbox, resolve_command +from scio.aci.sandbox.local import ( + INTERPRETER_WORD, + LocalSandbox, + resolve_command, + with_interpreter_first, +) from scio.toolchain import Toolchain, python_toolchain HEALTH_TIMEOUT_S = 30 @@ -107,6 +113,7 @@ def start_app( proc = subprocess.Popen( argv, cwd=sb.root, + env=with_interpreter_first(os.environ, sb.python), # the app's children too (I-114) stdout=subprocess.PIPE, stderr=subprocess.STDOUT, text=True, diff --git a/scio/aci/sandbox/local.py b/scio/aci/sandbox/local.py index 2653fb1..3db6975 100644 --- a/scio/aci/sandbox/local.py +++ b/scio/aci/sandbox/local.py @@ -33,6 +33,26 @@ def _text(data: str | bytes | None) -> str: return data.decode("utf-8", "replace") if isinstance(data, bytes) else data +def with_interpreter_first(env: Mapping[str, str], interpreter: str | None) -> dict[str, str]: + """``env`` with the product interpreter's directory first on PATH and ``VIRTUAL_ENV`` at + its environment root (I-114): SCIO runs under ``uv run``, so its own interpreter leads + the inherited PATH, and a product command that starts ``python`` (or a console script of + the product's environment) by name would get SCIO's. SCIO's directories stay after it for + the profile's other tools. Unchanged when there is no interpreter of the product's own.""" + out = dict(env) + if not interpreter: + return out + bin_dir = Path(interpreter).parent + # Windows spells the key ``Path`` and matches names case-insensitively: one entry stays. + keys = [k for k in out if k.upper() == "PATH"] + entries = [e for k in keys for e in out.pop(k).split(os.pathsep) if e] + first = os.path.normcase(os.path.normpath(str(bin_dir))) + rest = [e for e in entries if os.path.normcase(os.path.normpath(e)) != first] + out[keys[0] if keys else "PATH"] = os.pathsep.join([str(bin_dir), *rest]) + out["VIRTUAL_ENV"] = str(bin_dir.parent) + return out + + def resolve_command(argv: Sequence[str], interpreter: str | None) -> list[str]: """What the host actually spawns for a profile command. @@ -92,7 +112,10 @@ def exec( ) -> ExecResult: timeout = timeout_s or self.limits.timeout_s argv = resolve_command(argv, self.python) - run_env = {**os.environ, "PYTHONUTF8": "1", "SCIO_SANDBOX": "local", **(env or {})} + run_env = with_interpreter_first( + {**os.environ, "PYTHONUTF8": "1", "SCIO_SANDBOX": "local", **(env or {})}, + self.python, # the product's interpreter for every command, not only `python` (I-114) + ) allow_network = self.limits.network if network is None else network if not allow_network: run_env.setdefault("PIP_NO_INDEX", "1") diff --git a/scio/cli.py b/scio/cli.py index 8dee5dd..abe6e09 100644 --- a/scio/cli.py +++ b/scio/cli.py @@ -99,6 +99,9 @@ def _print_build(outcome: Any) -> None: from scio.product.build import total_usd print(f"usd {total_usd(s):.4f} (orchestrator {s.budgets.usd_used:.4f})") + if s.exit_reason is not None and s.ledger: + # a halt says why here, not only in `scio report`'s timeline (I-116's release refusal) + print(f"reason: {s.ledger[-1].text}") if s.release is not None: # the release gate (M11) shipped = s.release.url or ", ".join(s.release.artifacts) or "not tagged" print(f"release {s.release.tag} {'green' if s.release.ok else 'FAILED'}: {shipped}") diff --git a/scio/harness/providers/claude_code.py b/scio/harness/providers/claude_code.py index 14cce46..49e6007 100644 --- a/scio/harness/providers/claude_code.py +++ b/scio/harness/providers/claude_code.py @@ -10,7 +10,9 @@ explorer) go through :class:`scio.loop.claude_code_worker.ClaudeCodeWorker` instead. The subprocess environment drops ``CLAUDECODE`` (print mode refuses to nest otherwise) and -``ANTHROPIC_API_KEY`` (which would shadow the subscription login). +``ANTHROPIC_API_KEY`` (which would shadow the subscription login). A worker turn hands the +runner its own environment, built on the same one, so its Bash commands run the product's +interpreter rather than SCIO's (I-114). """ from __future__ import annotations @@ -54,7 +56,9 @@ class ClaudeRunResult: Runner = Callable[..., ClaudeRunResult] -"""``runner(argv, *, cwd, stdin, timeout_s) -> ClaudeRunResult``; argv excludes the binary.""" +"""``runner(argv, *, cwd, stdin, timeout_s, env=None) -> ClaudeRunResult``; argv excludes the +binary. ``env`` is the whole process environment when given (a worker turn's, with the product +interpreter first on PATH, I-114); None means :func:`subprocess_env`.""" def subprocess_env(base: Mapping[str, str] | None = None) -> dict[str, str]: @@ -73,12 +77,13 @@ def run( cwd: str | Path | None = None, stdin: str | None = None, timeout_s: float = 600.0, + env: Mapping[str, str] | None = None, ) -> ClaudeRunResult: proc = subprocess.run( [binary, *argv], input=stdin, cwd=str(cwd) if cwd else None, - env=subprocess_env(), + env=dict(env) if env is not None else subprocess_env(), capture_output=True, text=True, encoding="utf-8", diff --git a/scio/loop/claude_code_worker.py b/scio/loop/claude_code_worker.py index 25edb0c..f003ebd 100644 --- a/scio/loop/claude_code_worker.py +++ b/scio/loop/claude_code_worker.py @@ -13,13 +13,15 @@ Verify stays SCIO's ground truth; the ACI's per-edit lint gate does not apply inside a Claude Code turn. Cost is Claude Code's own estimate, folded into the harness ledger and -budget through :meth:`scio.harness.core.Harness.account`. +budget through :meth:`scio.harness.core.Harness.account`. Both calls run with +:func:`worker_env`, so the turn's Bash commands find the product's interpreter first (I-114). """ from __future__ import annotations import json import tempfile +from collections.abc import Mapping from dataclasses import dataclass, field from pathlib import Path from typing import Any @@ -27,6 +29,7 @@ from pydantic import BaseModel, ValidationError from scio.aci.sandbox.base import Sandbox +from scio.aci.sandbox.local import with_interpreter_first from scio.harness.core import Harness from scio.harness.failures import FatalError from scio.harness.providers.claude_code import ( @@ -36,6 +39,7 @@ claude_runner, parse_result, served_model, + subprocess_env, usage_from_result, ) from scio.harness.types import Message, Part, TextPart, ToolResultPart, ToolUsePart, Usage @@ -59,9 +63,25 @@ "Grep and Glob for repo_search, Read for view_file, Edit and Write for edit_file, and " "Bash for run_tests ({test_cmd}). Dependencies are installed by the orchestrator from the " "product's approved list before your turn; never install packages yourself, ask for a " - "design decision instead. Keep every path inside the repository root, never run git " + "design decision instead. When the product has an interpreter of its own, `python` " + "already runs the product's environment with its approved dependencies installed; never " + "look for another interpreter. Keep every path inside the repository root, never run git " "commands that change history, and stop when the tests you were asked for pass." ) +"""Appended to the role's system prompt. The interpreter sentence is I-114: a live coder spent +36 tool calls hunting for an interpreter that could import a product dependency.""" + + +def worker_env(sandbox: Sandbox, base: Mapping[str, str] | None = None) -> dict[str, str]: + """The environment of a worker turn's ``claude -p`` process (I-114). + + :func:`subprocess_env`, plus the product interpreter's directory first on PATH and + ``VIRTUAL_ENV`` at that interpreter's environment root, so the turn's Bash commands run + the product's interpreter with its approved dependencies, not SCIO's own (which is first + on SCIO's PATH under ``uv run``). SCIO's directories stay on PATH after it for the + profile's other tools. Unchanged when the sandbox has no interpreter of its own. + """ + return with_interpreter_first(subprocess_env(base), getattr(sandbox, "python", None)) def allowed_tools_for(tool_names: tuple[str, ...], toolchain: Toolchain) -> list[str]: @@ -223,6 +243,9 @@ def run_typed[T: BaseModel]( system = self.prompts.get(self.role).render() + BACKEND_NOTE.format(test_cmd=test_cmd) snap = self.harness.budget.snapshot() remaining = snap.usd_max - snap.usd_used + # Built per turn, not in __init__: the install phase sets ``sandbox.python`` after the + # worker exists (a resumed build, a lazy install_deps). + env = worker_env(self.sandbox) with tempfile.TemporaryDirectory(prefix="scio-claude-") as tmp: system_path = Path(tmp) / "system.md" system_path.write_text(system, encoding="utf-8", newline="\n") @@ -250,7 +273,7 @@ def run_typed[T: BaseModel]( ] self.calls.append(list(argv)) res = self._run( - argv, cwd=self.sandbox.root, stdin=user_message, timeout_s=self.timeout_s + argv, cwd=self.sandbox.root, stdin=user_message, timeout_s=self.timeout_s, env=env ) turn = parse_stream(res.stdout) if not turn.result: @@ -270,7 +293,7 @@ def run_typed[T: BaseModel]( # The typed closing output: one more turn on the same session, no tools. schema = wire_schema(output_model) output, summary_text, calls = self._summary( - session_id, schema, output_model, summary_prompt, model, tags + session_id, schema, output_model, summary_prompt, model, tags, env ) messages.append(Message.assistant(summary_text)) return LoopResult( @@ -289,6 +312,7 @@ def _summary[T: BaseModel]( prompt: str, model: str, tags: dict[str, str] | None, + env: Mapping[str, str], ) -> tuple[T, str, int]: last_error = "" for attempt in range(1, 3): @@ -313,7 +337,9 @@ def _summary[T: BaseModel]( json.dumps(schema), ] self.calls.append(list(argv)) - res = self._run(argv, cwd=self.sandbox.root, stdin=ask, timeout_s=self.timeout_s) + res = self._run( + argv, cwd=self.sandbox.root, stdin=ask, timeout_s=self.timeout_s, env=env + ) try: data = parse_result(res) except FatalError as exc: diff --git a/scio/loop/escalation.py b/scio/loop/escalation.py index 09437a5..b8cd1b5 100644 --- a/scio/loop/escalation.py +++ b/scio/loop/escalation.py @@ -111,11 +111,17 @@ def decide(state: SCIOState, cfg: EscalationConfig, *, root: str | None = None) route="advise", triggers=triggers, reason=", ".join(t.value for t in triggers) ) used, cap = state.budgets.advisor_calls_used, state.budgets.advisor_calls_max - return TriageDecision( - route="halt", - triggers=triggers, - reason=f"advisor cap hit ({used}/{cap}, step {step_id})", - ) + # I-115: name the cap that fired. The per-step cap (one call per step) refuses while the + # run's total still has headroom, and "2/3" alone reads as headroom left. The total wins + # when both are spent. + if used >= cap: + reason = f"advisor cap hit ({used}/{cap} for this run, step {step_id})" + else: + reason = ( + f"advisor already consulted on step {step_id} " + f"(one call per step; {used}/{cap} for this run)" + ) + return TriageDecision(route="halt", triggers=triggers, reason=reason) def review_decision(state: SCIOState, cfg: EscalationConfig) -> TriageDecision | None: diff --git a/scio/product/build.py b/scio/product/build.py index 3d3759e..3553ae6 100644 --- a/scio/product/build.py +++ b/scio/product/build.py @@ -165,10 +165,22 @@ def reread_product_md( def _open_features(spec: ProductSpec, progress: ProgressFile) -> list[ProductFeature]: + """The open features (a skipped one included: it is deferred, I-116) in PRODUCT.md order.""" open_ids = set(progress.open_feature_ids()) return [f for f in spec.features if f.id in open_ids] +def _not_done(spec: ProductSpec, progress: ProgressFile) -> list[str]: + """``" "`` for every feature of ``spec`` that ``progress.json`` does not + record as ``done`` (a feature without a record is ``no record``): what blocks a release.""" + out: list[str] = [] + for f in spec.features: + rec = progress.feature(f.id) + if rec is None or rec.status != "done": + out.append(f"{f.id} {rec.status if rec else 'no record'}") + return out + + def _feature(spec: ProductSpec, fid: str) -> ProductFeature: f = next((x for x in spec.features if x.id == fid), None) if f is None: @@ -430,14 +442,18 @@ def render_milestone_pr_body(state: ProductBuildState, spec: ProductSpec) -> str f"| {f.id} {f.description} | {mf.tier.value} | {status} | {commit} | {usd} | {esc} |" ) if (n := state.issues.get(mf.id)) is not None: - closes.append(f"Closes #{n}") + # A skipped feature is deferred, not delivered: its issue stays open for the + # milestone that builds it (I-116). + passed = run is not None and run.status == "passed" + closes.append(f"Closes #{n}" if passed else f"Refs #{n} ({f.id} deferred, not closed)") gate = state.last_gate gate_line = ( f"{gate.verdict}: {gate.reasoning[:400]}" if gate is not None else "(no gate verdict)" ) integration = state.integration or "(integration checks not run)" - runs = [r for r in state.feature_runs if r.feature_id in plan.feature_ids()] - attempts = sum(1 for _ in runs) + # this milestone's runs only: a deferred feature's earlier attempts belong to theirs (I-116) + runs = state.milestone_runs() + attempts = len(runs) text = "\n".join( [ "## Summary", @@ -600,6 +616,17 @@ def node(state: ProductBuildState) -> dict[str, Any]: ) ] entries += [fact(n) for n in notes] + deferred = [ + fid + for fid in plan.feature_ids() + if (rec := progress.feature(fid)) is not None and rec.status == "skipped" + ] + if deferred: + entries.append( + fact( + f"deferred by a skip, planned again in {plan.id}: {', '.join(deferred)} (I-116)" + ) + ) # The milestone branch from main (probe: it exists). def apply() -> str: @@ -755,7 +782,9 @@ def feature(ctx: RunContext, saver: BaseCheckpointSaver | None) -> Node: The thread id is deterministic per attempt, so a crash mid-feature resumes the same subgraph; its ``merge_wait`` is answered by the orchestrator (the milestone PR is the human's stop); any other pause or a failed exit stops here for one human decision: - ``retry`` (a new attempt), ``skip``, or anything else to stop the build. + ``retry`` (a new attempt), ``skip``, or anything else to stop the build. ``skip`` defers + the feature: this milestone goes on without it, it stays open in ``progress.json`` and the + next milestone plans it again; only removing it from ``PRODUCT.md`` drops it (I-116). """ def node(state: ProductBuildState) -> dict[str, Any]: @@ -776,6 +805,7 @@ def node(state: ProductBuildState) -> dict[str, Any]: attempt=attempt, tier=feat.tier, issue_number=state.issues.get(feat.id), + milestone=plan.id, # a skip finishes the feature in this milestone only (I-116) ) entries: list[Any] = [] try: @@ -878,9 +908,12 @@ def probe() -> str | None: answer = str( interrupt( { + # I-116: a skip defers, it never drops; the question says so plainly. "question": f"Feature {feat.id} ({reason}, attempt {attempt}) needs a " - "decision: reply 'retry' to run it again, 'skip' to leave it out of the " - "milestone, anything else to stop the build.", + "decision: reply 'retry' to run it again; 'skip' to defer it: this " + "milestone goes on without it, it stays open and the next milestone plans " + "it again (the release waits for it; to drop it for good, remove it from " + "PRODUCT.md); anything else to stop the build.", "feature": feat.id, "thread": thread, "exit_reason": reason, @@ -908,7 +941,13 @@ def probe() -> str | None: } if choice.startswith("skip"): record = feature_run_from_state(run, final, status="skipped") - entries.append(decision(f"{feat.id} skipped by human ({reason})", source=Role.HUMAN)) + entries.append( + decision( + f"{feat.id} skipped by human ({reason}): deferred to the next milestone, " + "still open (I-116)", + source=Role.HUMAN, + ) + ) return { "feature_runs": [*state.feature_runs, record], "human_notes": notes, @@ -1331,7 +1370,10 @@ def after_merge_wait(state: ProductBuildState) -> Literal["update", "merge_wait" def update_progress(ctx: RunContext) -> Node: """Record the merged milestone in ``progress.json`` and ``NOTES.md`` on ``main`` (one - commit, idempotent), then loop to the next milestone or finish.""" + commit, idempotent), then loop to the next milestone or finish. A feature skipped in this + milestone is recorded ``skipped``, which is open, so the loop plans it again rather than + reaching the release without it; ``PRODUCT.md`` is re-read before the route is chosen, so + a feature added at this merge stop is planned too (I-116).""" def node(state: ProductBuildState) -> dict[str, Any]: spec, plan, pr = state.spec, state.milestone, state.pr @@ -1343,7 +1385,9 @@ def apply() -> str: _git(ctx, "checkout", "-q", "main") progress = _read_progress(ctx) progress.build_thread = ctx.thread_id - runs = [r for r in state.feature_runs if r.feature_id in plan.feature_ids()] + # this milestone's runs only: a deferred feature's earlier attempts were recorded + # with the milestone they ran in (I-116) + runs = state.milestone_runs() record = MilestoneRecord( id=plan.id, title=plan.title, @@ -1362,6 +1406,7 @@ def apply() -> str: if rec is None: rec = FeatureRecord(id=mf.id, description=_feature(spec, mf.id).description) progress.features.append(rec) + # skipped is open: deferred to the next milestone, never dropped (I-116) rec.status = "done" if run and run.status == "passed" else "skipped" rec.commit = run.commit_sha if run else None rec.thread = run.thread_id if run else None @@ -1398,14 +1443,21 @@ def apply() -> str: apply=apply, probe=lambda: find_commit(sb, marker), ) - remaining = _read_progress(ctx).open_feature_ids() - entries = [ + # PRODUCT.md as it is now, before routing (I-116, I-109): a feature added by hand at + # this merge stop gets its record and is planned, rather than the release going out + # without it; the sync is committed with the next milestone's bookkeeping. + progress = _read_progress(ctx) + spec_now, reread = reread_product_md(ctx, spec, progress) + remaining = progress.open_feature_ids() + entries = [fact(n) for n in reread] + [ fact( f"progress.json updated for {plan.id} at {sha[:12]}; " f"{len(remaining)} open feature(s) left" + + (f": {', '.join(remaining)}" if remaining else "") ) ] return { + "spec": spec_now, "milestones_done": [*state.milestones_done, plan.id], "milestone": None, # recorded in progress.json; the next plan sets a new one "pr": None, @@ -1433,13 +1485,25 @@ def release_check(ctx: RunContext) -> Node: """Once no milestone remains: the product-ready bar on ``main`` (every ``integrate`` check, acceptance included) and the version the product reports about itself. A failing check ends the build without a release (ADR 0007: checks override any verdict); a - version that is already tagged ends it with nothing to do.""" + version that is already tagged ends it with nothing to do. A feature of the spec that + ``progress.json`` on ``main`` does not record as ``done`` refuses the release (I-116).""" def node(state: ProductBuildState) -> dict[str, Any]: spec = state.spec assert spec is not None sb, tc = ctx.sandbox, ctx.toolchain _git(ctx, "checkout", "-q", "main") + # Defence in depth (I-116): a skipped feature stays open, so the loop reaches this + # gate only once every feature is done; a stale record must not ship without one. + if missing := _not_done(spec, _read_progress(ctx)): + return _halt( + ctx, + "release_failed", + "release refused: not every feature in PRODUCT.md is done in progress.json (" + + ", ".join(missing) + + "); run scio build to finish them, or remove a feature from PRODUCT.md to " + "drop it", + ) try: product_env(ctx, spec) except RuntimeError as exc: diff --git a/scio/product/progress.py b/scio/product/progress.py index 757ff72..eabc6a2 100644 --- a/scio/product/progress.py +++ b/scio/product/progress.py @@ -29,6 +29,7 @@ MAX_FEATURES_PER_MILESTONE = 4 FeatureState = Literal["todo", "in_progress", "done", "skipped", "failed"] +"""Only ``done`` is finished; ``skipped`` means deferred to a later milestone (I-116).""" MilestoneState = Literal["planned", "in_progress", "done", "failed"] @@ -71,7 +72,10 @@ def milestone(self, mid: str) -> MilestoneRecord | None: return next((m for m in self.milestones if m.id == mid), None) def open_feature_ids(self) -> list[str]: - return [f.id for f in self.features if f.status in ("todo", "in_progress", "failed")] + """Every feature not ``done``, in record order. A ``skipped`` feature is open: the + human's ``skip`` defers it to the next milestone, and only removing it from PRODUCT.md + drops it (I-116; a skip used to drop it silently and the release went out without it).""" + return [f.id for f in self.features if f.status != "done"] def next_milestone_number(self) -> int: return len(self.milestones) + 1 @@ -232,7 +236,10 @@ def normalise_milestone( """The planner proposes, the orchestrator decides (I-046): the milestone is the longest prefix of the open features (PRODUCT.md order) the planner named, capped at :data:`MAX_FEATURES_PER_MILESTONE`; an empty or out-of-order proposal becomes the first - open feature alone. Tiers default to T1. Notes explain every change.""" + open feature alone. Tiers default to T1. Notes explain every change. A feature skipped in + an earlier milestone is open and keeps its place in PRODUCT.md order, so a planner cannot + route around it: leaving it out stops the milestone before it, and when it is the first + open feature the milestone is that feature alone (I-116).""" notes: list[str] = [] if not open_features: raise ValueError("no open features to plan") diff --git a/scio/state/models.py b/scio/state/models.py index 2fd09f8..3baf5c4 100644 --- a/scio/state/models.py +++ b/scio/state/models.py @@ -509,6 +509,10 @@ class FeatureRun(BaseModel): escalations: int = 0 escalations_resolved: int = 0 human_notes: list[str] = Field(default_factory=list) + milestone: str | None = None + """The milestone this run belongs to (``M2``). A build thread keeps its runs across + milestones, and a skip defers a feature to the next one (I-116), so "finished" and "this + milestone's cost" are per milestone. None on a run recorded before the field existed.""" class MilestonePR(BaseModel): @@ -595,15 +599,38 @@ def latest_run(self, feature_id: str) -> FeatureRun | None: return next((r for r in reversed(self.feature_runs) if r.feature_id == feature_id), None) def next_feature(self) -> MilestoneFeatureOutput | None: - """The first feature of the milestone without a passed or skipped run.""" + """The first feature of the milestone that no run has finished: a passed run finishes + it, and so does a run skipped in this milestone. A skip defers the feature to the next + milestone (I-116), so a run skipped in an earlier one does not finish it here, and the + feature runs again with its next attempt. A run recorded before runs carried their + milestone (``milestone`` None) keeps the old rule: skipped finishes.""" if self.milestone is None: return None + mid = self.milestone.id for f in self.milestone.features: run = self.latest_run(f.id) - if run is None or run.status not in ("passed", "skipped"): + if run is None: return f + if run.status == "passed": + continue + if run.status == "skipped" and run.milestone in (None, mid): + continue + return f return None + def milestone_runs(self) -> list[FeatureRun]: + """Every run of the current milestone, oldest first: its cost, attempts and record + (I-116). A run without a milestone (recorded before the field) is matched by feature + id, as before; it may then belong to an earlier milestone of the same thread.""" + if self.milestone is None: + return [] + ids, mid = set(self.milestone.feature_ids()), self.milestone.id + return [ + r + for r in self.feature_runs + if r.feature_id in ids and (r.milestone is None or r.milestone == mid) + ] + # ---------- the graph state ---------- diff --git a/tests/graph/test_claude_code_backend_t0.py b/tests/graph/test_claude_code_backend_t0.py index cc4076b..604e622 100644 --- a/tests/graph/test_claude_code_backend_t0.py +++ b/tests/graph/test_claude_code_backend_t0.py @@ -57,6 +57,7 @@ def __call__( cwd: str | Path | None = None, stdin: str | None = None, timeout_s: float = 0.0, + env: Any = None, # the worker passes the product's environment (I-114) ) -> ClaudeRunResult: self.calls.append(list(argv)) root = Path(str(cwd)) diff --git a/tests/graph/test_product_build_fake.py b/tests/graph/test_product_build_fake.py index b9ef369..0f1e15a 100644 --- a/tests/graph/test_product_build_fake.py +++ b/tests/graph/test_product_build_fake.py @@ -28,14 +28,17 @@ from scio.product import resume_product, run_product from scio.product.build import ( MILESTONES_DIR, + after_release_check, build_thread_id, integrate, integration_checks, milestone_gate, + release_check, + render_milestone_pr_body, resume_build, run_build, ) -from scio.product.progress import PRODUCT_MD, PROGRESS_PATH, ProgressFile +from scio.product.progress import PRODUCT_MD, PROGRESS_PATH, ProgressFile, parse_product_md from scio.settings import Settings from scio.state.models import ( FeatureRun, @@ -633,8 +636,9 @@ def test_feature_failure_stops_for_one_human_decision( tmp_path: Path, mode: str, inner_pause: bool ) -> None: """A feature that halts (a stall exit, or the task graph's own human interrupt) stops the - build once: 'skip' leaves it out, 'retry' starts a new attempt on a new thread, anything - else halts the build with feature_failed.""" + build once: 'skip' defers it (the milestone goes on without it; the next milestone plans + it again, I-116), 'retry' starts a new attempt on a new thread, anything else halts the + build with feature_failed.""" harness = _bootstrap(tmp_path, ScriptedResponder(coder_mode=mode)) ctx = _ctx(tmp_path, harness, build_thread_id(PRODUCT)) try: @@ -675,6 +679,136 @@ def test_feature_failure_stops_for_one_human_decision( ctx.close() +def test_a_skipped_feature_is_deferred_to_the_next_milestone_not_dropped(tmp_path: Path) -> None: + """I-116 (live): 'skip' recorded the feature ``skipped``, which left the open set, so the + loop never planned it again and reached the release without it. Now the skip defers: M1 + records both skipped features, M2 plans them again first, and the deferred feature runs + again on its next attempt's thread: the build thread keeps M1's skipped run, which + finishes the feature in M1 only. M2's PR and record count M2's runs, not M1's again.""" + harness = _bootstrap(tmp_path, ScriptedResponder(coder_mode="idle")) + repo = tmp_path / "product" + ctx = _ctx(tmp_path, harness, build_thread_id(PRODUCT)) + try: + out = run_build(ctx) + assert out.paused and out.interrupt and out.interrupt["feature"] == "P1" + question = out.interrupt["question"] + assert "'skip' to defer it" in question and "remove it from PRODUCT.md" in question + out = resume_build(ctx, "skip") + assert out.paused and out.interrupt and out.interrupt["feature"] == "P2" + out = resume_build(ctx, "skip") + # both features skipped: the gate's pass is overridden and the human is asked + assert out.paused and out.state.phase == "gate", out.state.ledger[-3:] + assert out.state.last_gate and "features not passed: P1, P2" in ( + out.state.last_gate.reasoning + ) + out = resume_build(ctx, "proceed") + assert out.paused and out.state.phase == "merge_wait" + assert out.interrupt and out.interrupt["features"] == ["P1: skipped", "P2: skipped"] + out = resume_build(ctx, "merge") + + # M1 is recorded with its skipped features, which stay open ... + progress = _progress(repo) + assert [(m.id, m.features) for m in progress.milestones] == [("M1", ["P1", "P2"])] + assert [(f.id, f.status) for f in progress.features] == [ + ("P1", "skipped"), + ("P2", "skipped"), + ("P3", "todo"), + ] + assert progress.open_feature_ids() == ["P1", "P2", "P3"] + notes = (repo / "NOTES.md").read_text(encoding="utf-8") + assert "M1 Milestone 1 merged by `scio build`: P1 skipped, P2 skipped" in notes + # ... so the loop plans M2 rather than the release, with the deferred features first, + # and P1 runs again: attempt 2 on a new thread, not M1's skipped run passed over + s = out.state + assert out.paused and out.interrupt, s.ledger[-3:] + assert s.milestones_done == ["M1"] and s.release is None + assert s.milestone and s.milestone.id == "M2" and s.milestone.feature_ids() == ["P1", "P2"] + assert out.interrupt["feature"] == "P1" and out.interrupt["thread"].endswith("-p1-a2") + assert [(r.feature_id, r.attempt, r.status, r.milestone) for r in s.feature_runs] == [ + ("P1", 1, "skipped", "M1"), + ("P2", 1, "skipped", "M1"), + ] + texts = [e.text for e in s.ledger] + assert sum("deferred to the next milestone" in t for t in texts) == 2 + assert any(t.startswith("M2 Milestone 2: P1@T0, P2@T1") for t in texts) + assert any("deferred by a skip, planned again in M2: P1, P2" in t for t in texts) + + # skipped again in M2: M2's PR and record count M2's runs only, not M1's as well + out = resume_build(ctx, "skip") + assert out.paused and out.interrupt and out.interrupt["thread"].endswith("-p2-a2") + out = resume_build(ctx, "skip") + assert out.paused and out.state.phase == "gate" + out = resume_build(ctx, "proceed") + s = out.state + assert out.paused and s.phase == "merge_wait" and s.pr and s.pr.body_path + m1 = [r for r in s.feature_runs if r.milestone == "M1"] + m2 = [r for r in s.feature_runs if r.milestone == "M2"] + assert [(r.feature_id, r.attempt) for r in m2] == [("P1", 2), ("P2", 2)] + assert s.milestone_runs() == m2 and all(r.usd > 0 for r in m1 + m2) + body = (repo / s.pr.body_path).read_text(encoding="utf-8") + assert f"Features: ${sum(r.usd for r in m2):.4f} over 2 run(s)" in body + out = resume_build(ctx, "merge and stop") + assert not out.paused and out.state.phase == "done" + finally: + ctx.close() + progress = _progress(repo) + assert [(m.id, m.usd) for m in progress.milestones] == [ + ("M1", round(sum(r.usd for r in m1), 4)), + ("M2", round(sum(r.usd for r in m2), 4)), + ] + assert [f.status for f in progress.features] == ["skipped", "skipped", "todo"] + notes = (repo / "NOTES.md").read_text(encoding="utf-8") + assert "M2 Milestone 2 merged by `scio build`: P1 skipped, P2 skipped;" in notes + + +def test_a_milestone_pr_closes_only_the_issues_of_passed_features(tmp_path: Path) -> None: + """I-116 sweep: the milestone PR said ``Closes #n`` for a skipped feature too, so with + GitHub its merge closed the issue of a feature that was only deferred.""" + harness = _harness(_settings(tmp_path), ScriptedResponder()) + ctx = _ctx(tmp_path, harness, "build-closes") + try: + state = _state_for_gate(ctx, integration_ok=True, statuses=["passed", "skipped"]) + state = state.model_copy(update={"issues": {"P1": 11, "P2": 12}}) + assert state.spec is not None + body = render_milestone_pr_body(state, state.spec) + assert "Closes #11" in body and "Closes #12" not in body + assert "Refs #12 (P2 deferred, not closed)" in body + finally: + ctx.close() + + +def test_the_release_gate_refuses_while_a_feature_is_not_done(tmp_path: Path) -> None: + """I-116 defence in depth: with a skip deferring, the loop reaches the release only once + every feature is done; a stale ``progress.json`` that still records one as not done stops + the release with the features named, before anything is checked or tagged.""" + harness = _bootstrap(tmp_path) + repo = tmp_path / "product" + progress = _progress(repo) + for rec in progress.features: + rec.status = "skipped" if rec.id == "P2" else "done" + (repo / PROGRESS_PATH).write_text(progress.to_json(), encoding="utf-8", newline="\n") + _git(repo, "commit", "-q", "-am", "chore: a stale progress record") + ctx = _ctx(tmp_path, harness, "build-release-guard") + try: + spec = parse_product_md((repo / PRODUCT_MD).read_text(encoding="utf-8")) + assert [f.id for f in spec.features] == ["P1", "P2", "P3"] + state = ProductBuildState( + thread_id=ctx.thread_id, product_dir=str(repo), spec=spec, budgets=ctx.budget + ) + patch = release_check(ctx)(state) + assert patch.get("exit_reason") == "release_failed", patch.get("phase") + assert patch["phase"] == "halted" and "release" not in patch + text = patch["ledger"][-1].text + assert text.startswith("release refused") and "(P2 skipped)" in text + assert "remove a feature from PRODUCT.md" in text + assert after_release_check(state.model_copy(update={"exit_reason": "release_failed"})) == ( + "halt" + ) + finally: + ctx.close() + assert _git(repo, "tag", "--list") == "" + + def test_milestone_gate_escalation_asks_the_human(tmp_path: Path) -> None: harness = _bootstrap(tmp_path, ScriptedResponder(milestone_gate_verdict="escalate_human")) ctx = _ctx(tmp_path, harness, build_thread_id(PRODUCT)) @@ -1060,6 +1194,54 @@ def test_a_hand_edit_of_product_md_survives_resets_merges_and_reaches_main(tmp_p assert _git(repo, "status", "--porcelain", "--", PRODUCT_MD) == "" # committed, not dangling +def test_a_feature_added_at_the_last_merge_stop_is_planned_not_left_out_of_the_release( + tmp_path: Path, +) -> None: + """I-116 with I-109: ``update_progress`` chose the route from ``progress.json`` alone, so + a feature added to PRODUCT.md by hand at the last merge stop had no record yet: nothing + was open, the build went to the release gate, and the release would ship without it. + PRODUCT.md is re-read before the route is chosen now, so the feature gets its record and + a milestone of its own.""" + responder = ScriptedResponder(milestone_size=4) # one milestone builds P1, P2 and P3 + harness = _bootstrap(tmp_path, responder) + repo = tmp_path / "product" + md = repo / PRODUCT_MD + ctx = _ctx(tmp_path, harness, build_thread_id(PRODUCT)) + try: + out = run_build(ctx) + assert out.paused and out.state.phase == "merge_wait", out.state.ledger[-3:] + assert out.state.milestone and out.state.milestone.feature_ids() == ["P1", "P2", "P3"] + bar = "## Quality bar (release gate)\n" + text = md.read_text(encoding="utf-8") + added = "### P4: Slugify todo titles\n- [ ] `todo slug A` prints a\n\n" + md.write_text(text.replace(bar, added + bar, 1), encoding="utf-8", newline="\n") + assert [f.id for f in parse_product_md(md.read_text(encoding="utf-8")).features][-1] == "P4" + responder.coder_mode = "idle" # P4 halts at once: this test is about the route + out = resume_build(ctx, "merge") + s = out.state + assert s.milestones_done == ["M1"] and s.release is None, s.ledger[-3:] + assert s.milestone and s.milestone.id == "M2" and s.milestone.feature_ids() == ["P4"] + assert out.paused and out.interrupt and out.interrupt["feature"] == "P4" + assert s.spec and [f.id for f in s.spec.features] == ["P1", "P2", "P3", "P4"] + texts = [e.text for e in s.ledger] + assert any("P4 added from PRODUCT.md" in t for t in texts) + assert any("1 open feature(s) left: P4" in t for t in texts) + out = resume_build(ctx, "stop") + assert not out.paused and out.state.exit_reason == "feature_failed" + finally: + ctx.close() + # the record and the edit are committed with M2's bookkeeping, not left dangling on main + recorded = ProgressFile.from_json(_git(repo, "show", f"milestone/2:{PROGRESS_PATH}")) + assert [(f.id, f.status) for f in recorded.features] == [ + ("P1", "done"), + ("P2", "done"), + ("P3", "done"), + ("P4", "todo"), + ] + assert "### P4: Slugify todo titles" in _git(repo, "show", f"milestone/2:{PRODUCT_MD}") + assert _git(repo, "tag", "--list") == "" + + def test_a_github_refusal_stops_scio_build_with_a_resume_hint_not_a_traceback( tmp_path: Path, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] ) -> None: diff --git a/tests/unit/test_claude_code_backend.py b/tests/unit/test_claude_code_backend.py index bc4a3dc..0dd0681 100644 --- a/tests/unit/test_claude_code_backend.py +++ b/tests/unit/test_claude_code_backend.py @@ -9,7 +9,9 @@ from __future__ import annotations import json -from collections.abc import Sequence +import os +import subprocess +from collections.abc import Mapping, Sequence from pathlib import Path from typing import Any @@ -24,6 +26,7 @@ from scio.harness.providers.claude_code import ( ClaudeCodeProvider, ClaudeRunResult, + claude_runner, render_messages, subprocess_env, ) @@ -39,13 +42,17 @@ class FakeClaude: - """Canned ``claude -p`` runs keyed by the call order; records argv, cwd and stdin.""" + """Canned ``claude -p`` runs keyed by the call order; records argv, cwd and stdin, the + process environment (``envs``, None when the caller left the default) and the text of + each system prompt file while it still exists (``systems``).""" def __init__( self, results: list[dict[str, Any] | list[dict[str, Any]] | ClaudeRunResult] ) -> None: self.results = list(results) self.calls: list[tuple[list[str], str | None, str | None]] = [] + self.envs: list[dict[str, str] | None] = [] + self.systems: list[str] = [] def __call__( self, @@ -54,8 +61,13 @@ def __call__( cwd: str | Path | None = None, stdin: str | None = None, timeout_s: float = 0.0, + env: Mapping[str, str] | None = None, ) -> ClaudeRunResult: self.calls.append((list(argv), str(cwd) if cwd else None, stdin)) + self.envs.append(dict(env) if env is not None else None) + if "--system-prompt-file" in argv: + path = Path(_flag(list(argv), "--system-prompt-file")) + self.systems.append(path.read_text(encoding="utf-8")) nxt = self.results.pop(0) if isinstance(nxt, ClaudeRunResult): return nxt @@ -98,6 +110,26 @@ def test_subprocess_env_strips_nesting_and_api_key_variables() -> None: assert env == {"PATH": "p", "HOME": "h"} +def test_claude_runner_uses_a_given_env_else_subprocess_env( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """I-114: a worker turn hands the runner its own environment; the provider's calls pass + none and keep :func:`subprocess_env`.""" + seen: list[dict[str, str]] = [] + + def fake_run(argv: list[str], **kwargs: Any) -> subprocess.CompletedProcess[str]: + seen.append(kwargs["env"]) + return subprocess.CompletedProcess(argv, 0, "{}", "") + + monkeypatch.setenv("CLAUDECODE", "1") + monkeypatch.setattr(subprocess, "run", fake_run) + run = claude_runner("claude") + run(["-p"], stdin="q", env={"PATH": "product-bin", "K": "v"}) + run(["-p"], stdin="q") + assert seen[0] == {"PATH": "product-bin", "K": "v"} + assert seen[1] == subprocess_env() and "CLAUDECODE" not in seen[1] + + def test_provider_structured_call_is_one_restricted_turn_with_a_schema() -> None: fake = FakeClaude( [_result(structured_output={"tier": "T0", "rationale": "r", "confidence": 0.9})] @@ -555,3 +587,100 @@ def test_claude_code_backend_requires_the_local_sandbox(tmp_path: Path) -> None: assert "Bash(rm *)" in allowed_tools_for(("edit_file", "delete_file"), python_toolchain()) finally: ctx.close() + + +# ---------- the turn's environment (I-114) ---------- + + +def _run_worker(sandbox: LocalSandbox) -> FakeClaude: + """One worker turn plus its typed summary on ``sandbox``; returns the fake runner.""" + settings = Settings(provider_override="fake") + config = ModelsConfig.load(settings.models_path) + harness = Harness(config, ProviderRegistry(config, settings), settings=settings) + summary = { + "step_id": "S1", "summary": "done", "files_touched": [], "confidence": "high", + "needs_design_decision": False, "design_question": None, "next_action": "verify", + "tests_added": [], "sweep": None, + } # fmt: skip + fake = FakeClaude([STREAM, _result(session_id="sess-w", structured_output=summary)]) + worker = ClaudeCodeWorker( + harness, sandbox, PromptRegistry(), "coder", + toolchain=python_toolchain(), tool_names=("edit_file", "run_tests"), runner=fake, + ) # fmt: skip + worker.run("task") + assert len(fake.calls) == 2 + return fake + + +def _path_entries(env: Mapping[str, str]) -> list[str]: + keys = [k for k in env if k.upper() == "PATH"] + assert len(keys) == 1, keys # one PATH variable, whatever its spelling + return env[keys[0]].split(os.pathsep) + + +def test_worker_turn_runs_with_the_product_interpreter_first_on_path( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """I-114 (live run): the turn's Bash ran SCIO's own interpreter, first on SCIO's PATH + under ``uv run``, which lacked the product's approved dependency; the coder spent 36 + tool calls looking for one that had it. Both calls now lead PATH with the directory of + ``sandbox.python`` and set ``VIRTUAL_ENV`` to its environment root.""" + interpreter = tmp_path / "product-env" / "bin" / "python" + scio_bin, other = str(tmp_path / "scio-bin"), str(tmp_path / "other") + monkeypatch.setenv("PATH", os.pathsep.join([scio_bin, str(interpreter.parent), other])) + monkeypatch.setenv("CLAUDECODE", "1") + fake = _run_worker(LocalSandbox(tmp_path / "repo", python=str(interpreter))) + assert len(fake.envs) == 2 # the turn and the --resume summary + for env in fake.envs: + assert env is not None + # prepended once, SCIO's own directories kept after it in order (the lint tool) + assert _path_entries(env) == [str(interpreter.parent), scio_bin, other] + assert env["VIRTUAL_ENV"] == str(interpreter.parent.parent) + assert "CLAUDECODE" not in env # still built on subprocess_env + + +def test_worker_without_a_product_interpreter_runs_with_subprocess_env( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """A stack with no interpreter of its own (``sandbox.python`` is None) keeps the plain + ``claude -p`` environment; SCIO injects no ``VIRTUAL_ENV``.""" + monkeypatch.delenv("VIRTUAL_ENV", raising=False) # ``uv run`` sets SCIO's own + monkeypatch.setenv("CLAUDECODE", "1") + fake = _run_worker(LocalSandbox(tmp_path)) + assert fake.envs == [subprocess_env(), subprocess_env()] + assert all(env is not None and "VIRTUAL_ENV" not in env for env in fake.envs) + + +def test_worker_env_keeps_one_path_variable_in_its_own_spelling(tmp_path: Path) -> None: + """Windows spells the key ``Path``: it is updated in place, not shadowed by a second + ``PATH``; an environment with no PATH gets one.""" + from scio.loop.claude_code_worker import worker_env + + interpreter = tmp_path / "product-env" / "bin" / "python" + bin_dir = str(interpreter.parent) + sandbox = LocalSandbox(tmp_path / "repo", python=str(interpreter)) + env = worker_env(sandbox, {"Path": os.pathsep.join(["a", bin_dir]), "ANTHROPIC_API_KEY": "x"}) + assert env == { + "Path": os.pathsep.join([bin_dir, "a"]), + "VIRTUAL_ENV": str(interpreter.parent.parent), + } + assert worker_env(sandbox, {"HOME": "h"})["PATH"] == bin_dir + assert worker_env(LocalSandbox(tmp_path / "plain"), {"Path": "a"}) == {"Path": "a"} + # ``env_interpreter`` joins a Windows root and its relative path with "/": normalised + mixed = f"{tmp_path}/product-env/bin/python" + joined = worker_env(LocalSandbox(tmp_path / "repo", python=mixed), {"PATH": "a"}) + assert _path_entries(joined) == [str(Path(mixed).parent), "a"] + assert joined["VIRTUAL_ENV"] == str(Path(mixed).parent.parent) + + +def test_worker_system_prompt_says_python_is_the_product_environment(tmp_path: Path) -> None: + """I-114: the backend note tells the worker the interpreter question is already settled, + so it never goes looking for another one; the rest of the note is kept.""" + fake = _run_worker(LocalSandbox(tmp_path)) + assert len(fake.systems) == 1 # the summary call resumes the session, no new prompt + system = " ".join(fake.systems[0].split()) + assert ( + "`python` already runs the product's environment with its approved dependencies " + "installed; never look for another interpreter." in system + ) + assert "never install packages yourself" in system and "Bash for run_tests" in system diff --git a/tests/unit/test_escalation.py b/tests/unit/test_escalation.py index 728ebe0..6c62d4b 100644 --- a/tests/unit/test_escalation.py +++ b/tests/unit/test_escalation.py @@ -246,7 +246,10 @@ def test_cap_per_step_and_per_run_route_to_halt_with_advisor_cap_exit() -> None: ), ) d = decide(per_step, CFG) - assert d.route == "halt" and exit_for(d) is ExitReason.ADVISOR_CAP and "cap" in d.reason + assert d.route == "halt" and exit_for(d) is ExitReason.ADVISOR_CAP + assert d.reason == ( + "advisor already consulted on step S1 (one call per step; 1/3 for this run)" + ) per_run = _state( progress=trig, budgets=Budget(tokens_max=1, usd_max=1, advisor_calls_max=1, advisor_calls_used=1), @@ -265,6 +268,49 @@ def test_cap_per_step_and_per_run_route_to_halt_with_advisor_cap_exit() -> None: assert decide(other_step, CFG).route == "advise" +def test_halt_reason_names_the_run_total_when_it_is_spent() -> None: + """I-115: a spent total says so, and wins when the step's one call is spent too.""" + trig = ProgressLedger(current_step_id="S1", stall_count=3, last_failure_kind="tests") + total = _state( + progress=trig, + budgets=Budget(tokens_max=1, usd_max=1, advisor_calls_max=3, advisor_calls_used=3), + ) + d = decide(total, CFG) + assert d.route == "halt" and exit_for(d) is ExitReason.ADVISOR_CAP + assert d.reason == "advisor cap hit (3/3 for this run, step S1)" + both = _state( + progress=trig, + budgets=Budget( + tokens_max=1, + usd_max=1, + advisor_calls_max=3, + advisor_calls_used=3, + advisor_calls_on_step={"S1": 1}, + ), + ) + assert decide(both, CFG).reason == "advisor cap hit (3/3 for this run, step S1)" + + +def test_halt_reason_names_the_per_step_cap_when_the_total_has_headroom() -> None: + """I-115, the graphcalc live run: 2/3 used, but the step already had its one call. The + old text, ``advisor cap hit (2/3, step S1)``, read as headroom left.""" + s = _state( + progress=ProgressLedger(current_step_id="S1", stall_count=3, last_failure_kind="tests"), + budgets=Budget( + tokens_max=1, + usd_max=1, + advisor_calls_max=3, + advisor_calls_used=2, + advisor_calls_on_step={"S0": 1, "S1": 1}, + ), + ) + d = decide(s, CFG) + assert d.route == "halt" and exit_for(d) is ExitReason.ADVISOR_CAP + assert d.reason == ( + "advisor already consulted on step S1 (one call per step; 2/3 for this run)" + ) + + def test_exit_reason_wins_over_everything() -> None: s = _state( exit_reason=ExitReason.BUDGET, diff --git a/tests/unit/test_product_progress.py b/tests/unit/test_product_progress.py index f4c35ab..c88c399 100644 --- a/tests/unit/test_product_progress.py +++ b/tests/unit/test_product_progress.py @@ -5,7 +5,13 @@ import pytest -from scio.product.build import feature_task, issue_body, issue_title +from scio.product.build import ( + _not_done, + _open_features, + feature_task, + issue_body, + issue_title, +) from scio.product.progress import ( MAX_FEATURES_PER_MILESTONE, ProgressFile, @@ -69,6 +75,46 @@ def test_progress_file_starts_every_feature_todo_and_syncs_with_product_md() -> assert [f.id for f in again.features] == ["P1", "P2"] and "removed" in notes[0] +def test_a_skipped_feature_stays_open_and_is_planned_first_again() -> None: + """I-116: 'skip' at a halted feature defers it, it does not drop it. ``skipped`` used to + leave the open set, so the next milestone never planned the feature and the release went + out without it. Now it is open, keeps its PRODUCT.md place, and a planner cannot route + around it; only ``done`` is finished.""" + spec = _spec(4) + progress = ProgressFile.initial(spec, bootstrap_commit=None) + for fid, status in (("P1", "skipped"), ("P2", "done"), ("P3", "failed")): + rec = progress.feature(fid) + assert rec is not None + rec.status = status # type: ignore[assignment] + assert progress.open_feature_ids() == ["P1", "P3", "P4"] + open_features = _open_features(spec, progress) # what plan_milestone hands the normaliser + assert [f.id for f in open_features] == ["P1", "P3", "P4"] + + out = MilestonePlanOutput( + milestone_title="Next", + features=[MilestoneFeatureOutput(id="P1"), MilestoneFeatureOutput(id="P3")], + rationale="r", + ) + plan, _ = normalise_milestone(out, open_features, 2) + assert plan.feature_ids() == ["P1", "P3"] + + # a planner that leaves the deferred feature out gets it alone, not the features after it + out = MilestonePlanOutput( + milestone_title="Next", + features=[MilestoneFeatureOutput(id="P3"), MilestoneFeatureOutput(id="P4")], + rationale="r", + ) + plan, notes = normalise_milestone(out, open_features, 2) + assert plan.feature_ids() == ["P1"] and any("alone" in n for n in notes) + + # the release guard names what is not done; a feature with no record counts too + progress.features = [f for f in progress.features if f.id != "P4"] + assert _not_done(spec, progress) == ["P1 skipped", "P3 failed", "P4 no record"] + for rec in progress.features: + rec.status = "done" + assert progress.open_feature_ids() == [] and _not_done(_spec(3), progress) == [] + + def test_milestone_normaliser_keeps_a_consecutive_prefix_in_product_order() -> None: spec = _spec(6) open_features = spec.features[1:] # P1 done diff --git a/tests/unit/test_runtime.py b/tests/unit/test_runtime.py index 010d740..5374cb7 100644 --- a/tests/unit/test_runtime.py +++ b/tests/unit/test_runtime.py @@ -2,6 +2,7 @@ from __future__ import annotations +import os import shlex import socket import sys @@ -347,3 +348,26 @@ def test_normalise_package_handles_npm_specs() -> None: assert normalise_package("@types/node@22") == "@types/node" assert normalise_package("@Scope/Pkg") == "@scope/pkg" assert normalise_package("left-pad") == "left-pad" + + +def test_every_local_command_runs_with_the_product_interpreter_first_on_path( + tmp_path: Path, +) -> None: + """I-114: SCIO runs under `uv run`, so its own interpreter leads the inherited PATH; only a + literal `python` was rewritten, so a product command (or a child the product spawns) + that starts `python` by name got SCIO's. Every local command now gets the product's + interpreter directory first and VIRTUAL_ENV at its environment root.""" + sb = LocalSandbox(tmp_path) + venv = tmp_path / ".scio" / "sandbox" / "venv" + sb.python = str(venv / "Scripts" / "python.exe") # need not exist: only its directory is used + probe = ( + "import os; print(os.environ['PATH'].split(os.pathsep)[0]); " + "print(os.environ.get('VIRTUAL_ENV', '-'))" + ) + r = sb.exec([sys.executable, "-c", probe]) # not the word `python`: no rewrite + first, root = r.stdout.strip().splitlines() + assert r.ok and Path(first) == venv / "Scripts" and Path(root) == venv, r.output + plain = LocalSandbox(tmp_path / "plain") + r = plain.exec([sys.executable, "-c", probe]) + host_root = os.environ.get("VIRTUAL_ENV", "-") + assert r.ok and r.stdout.strip().splitlines()[1] == host_root # no product interpreter diff --git a/tests/unit/test_state.py b/tests/unit/test_state.py index d39c532..8c93739 100644 --- a/tests/unit/test_state.py +++ b/tests/unit/test_state.py @@ -14,6 +14,13 @@ error_signature, progress_fingerprint, ) +from scio.state.models import ( + FeatureRun, + MilestoneFeatureOutput, + MilestonePlan, + ProductBuildState, + Tier, +) from scio.state.signatures import advance TB_A = """Traceback (most recent call last): @@ -100,3 +107,77 @@ def test_state_helpers_and_sweep_field() -> None: ) # fmt: skip assert out.sweep is not None and out.sweep.pattern == "write_text(" assert st.github.full_name == "o/r" and Role.DEBUGGER.value == "debugger" + + +def _build_state(milestone: int, runs: list[FeatureRun]) -> ProductBuildState: + plan = MilestonePlan( + number=milestone, + title="t", + branch=f"milestone/{milestone}", + features=[MilestoneFeatureOutput(id="P1"), MilestoneFeatureOutput(id="P3")], + ) + return ProductBuildState( + thread_id="b", + product_dir="/p", + milestone=plan, + feature_runs=runs, + budgets=Budget(tokens_max=1, usd_max=1, advisor_calls_max=0), + ) + + +def _run( + fid: str, attempt: int, status: str, milestone: str | None, usd: float = 0.0 +) -> FeatureRun: + return FeatureRun( + feature_id=fid, + thread_id=f"b-{fid.lower()}-a{attempt}", + attempt=attempt, + tier=Tier.T1, + status=status, # type: ignore[arg-type] + milestone=milestone, + usd=usd, + ) + + +def test_a_skip_finishes_a_feature_only_in_the_milestone_it_was_skipped_in() -> None: + """I-116: 'skip' defers a feature to the next milestone, and a build thread keeps its runs + across milestones. A run skipped in M1 used to finish P1 in M2 as well, so the deferred + feature was planned again and passed over; now it runs again with its next attempt.""" + m1_skip = _run("P1", 1, "skipped", "M1", usd=0.5) + m1_pass = _run("P2", 1, "passed", "M1", usd=0.25) + + in_m1 = _build_state(1, [m1_skip, m1_pass]) + nxt = in_m1.next_feature() + assert nxt is not None and nxt.id == "P3" # skipped in M1, so M1 goes on without P1 + + in_m2 = _build_state(2, [m1_skip, m1_pass]) + nxt = in_m2.next_feature() + assert nxt is not None and nxt.id == "P1" # deferred to M2: it runs again there + latest = in_m2.latest_run("P1") + assert latest is not None and latest.attempt == 1 # so M2's attempt is a2, a new thread + assert in_m2.milestone_runs() == [] # M1's cost and attempts are not M2's + + m2_skip = _run("P1", 2, "skipped", "M2", usd=0.75) + again = _build_state(2, [m1_skip, m1_pass, m2_skip]) + nxt = again.next_feature() + assert nxt is not None and nxt.id == "P3" # skipped in M2 itself: finished for M2 + assert again.milestone_runs() == [m2_skip] + + done = _build_state(2, [m1_skip, m1_pass, m2_skip, _run("P3", 1, "passed", "M2")]) + assert done.next_feature() is None + + +def test_runs_recorded_before_the_milestone_field_keep_the_old_rules() -> None: + """A checkpoint written before I-116 has runs without a milestone: a skip still finishes + the feature, and the milestone's runs are matched by feature id, as before.""" + legacy = [_run("P1", 1, "skipped", None), _run("P2", 1, "passed", None)] + st = _build_state(2, legacy) + nxt = st.next_feature() + assert nxt is not None and nxt.id == "P3" + assert st.milestone_runs() == [legacy[0]] + assert ( + FeatureRun.model_validate( + {"feature_id": "P1", "thread_id": "t", "tier": "T1", "status": "passed"} + ).milestone + is None + ) From 771a42e40490ab50a393ff2f8f2092d3f867c9da Mon Sep 17 00:00:00 2001 From: NewMe505 Date: Tue, 27 Oct 2026 23:15:43 -0700 Subject: [PATCH 2/3] fix(product): the review's three findings on the S5 fixes (#69) An independent review of bb3ae7e found: - a skip recorded by older code (milestone None) finished its feature in every later milestone, looping plan, pass-over and escalation; closing a milestone now stamps such runs (ProductBuildState.runs_stamped) - update_progress re-read PRODUCT.md after its commit, so a release cut next tagged a main that disagreed with it; the re-read now happens inside the progress commit, which includes PRODUCT.md, and release re-checks PRODUCT.md after the human's release decision - on Windows a bare command word resolved against SCIO's PATH, not the product-first one; resolve_command takes the command's PATH Co-Authored-By: Claude Opus 5.5 --- docs/ISSUE_LEDGER.md | 9 +++-- scio/aci/apps.py | 6 ++- scio/aci/sandbox/local.py | 17 ++++++-- scio/product/build.py | 28 +++++++++++-- scio/state/models.py | 11 +++++ tests/graph/test_product_build_fake.py | 56 +++++++++++++++++++++++++- tests/unit/test_runtime.py | 16 ++++++++ tests/unit/test_state.py | 36 +++++++++++++++++ 8 files changed, 164 insertions(+), 15 deletions(-) diff --git a/docs/ISSUE_LEDGER.md b/docs/ISSUE_LEDGER.md index e82bc37..d64f168 100644 --- a/docs/ISSUE_LEDGER.md +++ b/docs/ISSUE_LEDGER.md @@ -982,6 +982,7 @@ Template: - The worker: fixed. - `LocalSandbox.exec` passed SCIO's PATH to every product command, rewriting only a leading `python`. Fixed: it now uses `with_interpreter_first`, so a product command or a child the product spawns that starts `python` by name gets the product's. - The local `start_app` Popen: fixed the same way. + - Found by the independent review: on Windows `resolve_command` looked up a bare command word with `shutil.which` against SCIO's PATH, not the product-first one. It now takes the command's own PATH (`env_path(run_env)`), in `LocalSandbox.exec` and `start_app` (`test_a_bare_command_word_resolves_through_the_product_first_path`, Windows-only). - Not affected: the Docker sandbox (product code runs in the container's environment), the GitHub client (`gh`), the tool-less provider calls (`--tools ""`, no cwd), the eval solver (SCIO's interpreter on purpose; its product command already uses `sb.python`), and the scaffold's `sys.executable`. - The graph test's fake runner gained `env`. - Guard: @@ -1027,9 +1028,10 @@ Template: - `update_progress` re-reads PRODUCT.md before it routes. - `scio build` prints the halt's reason. - Done by a parallel agent that owned `progress.py`, `build.py` and `models.py`; its first workaround, which relabelled old skipped runs, was replaced by the milestone field so nothing is counted twice. -- Known limits, not reachable in any existing thread (graphcalc never answered skip): - - A thread that recorded a skip under the old code and is paused at its gate or merge stop would pass the feature over again, because the old run has milestone None. - - A re-read at the last stop can leave `progress.json` uncommitted on `main` until the next build's plan commits it, as `load`'s sync does. +- Found by the independent review of the commit, and fixed before the push: + - A skip recorded by older code (milestone None) finished its feature in every later milestone of its thread, so the loop planned the feature, passed it over and escalated, every milestone. `ProductBuildState.runs_stamped` lets `update_progress` stamp the closing milestone on such runs, and later milestones read them exactly. Before the review I had called this a one-time limit; it repeated forever. + - The re-read ran after `update_progress`'s commit, so on the release path the tag pointed at a `main` whose PRODUCT.md and progress.json disagreed with the working tree (with `--gh`, a squash merge's `sync_main` reset could even drop the progress sync). The re-read now runs inside the progress commit, and PRODUCT.md is one of its paths. + - `release_decision` is a human stop too, so `release` re-reads PRODUCT.md and refuses (`release_failed`) when a feature added there is not done, before it cuts or tags anything. - Sweep: `grep -rn '"skipped"\|skipped\|next_feature\|open_feature_ids\|\.status ==\|\.status !=\|status in (' scio/product scio/cli.py scio/state/models.py evals scio/graph` and `grep -rn "FeatureRun(\|feature_runs\|latest_run\|next_feature" scio evals tests`. - Fixed: every reader that treated `skipped` as finished, the PR body's `Closes`, and the per-milestone cost readers. - Correct as they are: `latest_run` for the attempt number (it counts across milestones on purpose, so a deferred feature gets attempt 2 on a new thread), the whole-thread sums (`total_usd`, the report, the eval solver), and the display-only readers. @@ -1040,6 +1042,7 @@ Template: - `test_the_release_gate_refuses_while_a_feature_is_not_done` and `test_a_milestone_pr_closes_only_the_issues_of_passed_features`. - `test_a_feature_added_at_the_last_merge_stop_is_planned_not_left_out_of_the_release`. - All fail on `main`. Plus guardrail 37. + - From the review's fixes: `test_closing_a_milestone_stamps_runs_recorded_before_the_milestone_field`, `test_a_feature_added_at_the_release_stop_holds_the_release`, and the last-merge-stop and hand-edit tests asserting that the progress commit on `main` holds PRODUCT.md. - Resolved: 2026-10-28 · Status: resolved ### I-117 · A parallel agent made one edit through a shell-embedded script with `\n` escapes (I-112's rule again) diff --git a/scio/aci/apps.py b/scio/aci/apps.py index 2efc263..466ff39 100644 --- a/scio/aci/apps.py +++ b/scio/aci/apps.py @@ -22,6 +22,7 @@ from scio.aci.sandbox.local import ( INTERPRETER_WORD, LocalSandbox, + env_path, resolve_command, with_interpreter_first, ) @@ -109,11 +110,12 @@ def start_app( argv = list(argv) tc = toolchain or python_toolchain() if isinstance(sb, LocalSandbox): - argv = resolve_command(argv, sb.python) + env = with_interpreter_first(os.environ, sb.python) # the app's children too (I-114) + argv = resolve_command(argv, sb.python, path=env_path(env)) proc = subprocess.Popen( argv, cwd=sb.root, - env=with_interpreter_first(os.environ, sb.python), # the app's children too (I-114) + env=env, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, text=True, diff --git a/scio/aci/sandbox/local.py b/scio/aci/sandbox/local.py index 3db6975..e2e448f 100644 --- a/scio/aci/sandbox/local.py +++ b/scio/aci/sandbox/local.py @@ -53,13 +53,22 @@ def with_interpreter_first(env: Mapping[str, str], interpreter: str | None) -> d return out -def resolve_command(argv: Sequence[str], interpreter: str | None) -> list[str]: +def env_path(env: Mapping[str, str]) -> str | None: + """The PATH value of ``env``, whatever the key's spelling (``Path`` on Windows).""" + return next((v for k, v in env.items() if k.upper() == "PATH"), None) + + +def resolve_command( + argv: Sequence[str], interpreter: str | None, *, path: str | None = None +) -> list[str]: """What the host actually spawns for a profile command. ``python`` becomes the product environment's interpreter once it exists, else the one running SCIO, never whatever is on PATH. On Windows a bare command word (``npm``, ``npx``) is resolved through PATH first, because ``CreateProcess`` does not try the - ``.cmd`` shims npm ships and Python would report "command not found" (I-081). + ``.cmd`` shims npm ships and Python would report "command not found" (I-081). ``path`` + is the PATH the command will run with (the product's first, I-114), so a word both + environments provide resolves to the product's, as it does on POSIX. """ argv = list(argv) if not argv: @@ -68,7 +77,7 @@ def resolve_command(argv: Sequence[str], interpreter: str | None) -> list[str]: if head == INTERPRETER_WORD: argv[0] = interpreter or sys.executable elif sys.platform == "win32" and Path(head).name == head and not head.lower().endswith(".exe"): - found = shutil.which(head) + found = shutil.which(head, path=path) if found: argv[0] = found return argv @@ -111,11 +120,11 @@ def exec( network: bool | None = None, ) -> ExecResult: timeout = timeout_s or self.limits.timeout_s - argv = resolve_command(argv, self.python) run_env = with_interpreter_first( {**os.environ, "PYTHONUTF8": "1", "SCIO_SANDBOX": "local", **(env or {})}, self.python, # the product's interpreter for every command, not only `python` (I-114) ) + argv = resolve_command(argv, self.python, path=env_path(run_env)) allow_network = self.limits.network if network is None else network if not allow_network: run_env.setdefault("PIP_NO_INDEX", "1") diff --git a/scio/product/build.py b/scio/product/build.py index 3553ae6..c096e1c 100644 --- a/scio/product/build.py +++ b/scio/product/build.py @@ -1380,6 +1380,7 @@ def node(state: ProductBuildState) -> dict[str, Any]: assert spec is not None and plan is not None and pr is not None sb = ctx.sandbox marker = f"[scio:progress:{plan.id}]" + synced: list[str] = [] # the re-read's notes, from inside the commit def apply() -> str: _git(ctx, "checkout", "-q", "main") @@ -1413,6 +1414,11 @@ def apply() -> str: rec.tier = mf.tier.value rec.issue = state.issues.get(mf.id) rec.usd = run.usd if run else 0.0 + # PRODUCT.md as it is now, inside this commit (I-116, I-109): a feature added or + # removed by hand at this merge stop is synced before the route is chosen, and + # the sync and the edit are committed here, so a release cut right after + # tags a main that agrees with them (I-116 review) + synced.extend(reread_product_md(ctx, spec, progress)[1]) _write_progress(ctx, progress) notes_path = f"{sb.root}/NOTES.md" notes = sb.read_text(notes_path) if sb.exists(notes_path) else "# Notes\n\n" @@ -1425,7 +1431,7 @@ def apply() -> str: sb.write_text(notes_path, f"{head}{sep}{line}{tail}" if sep else notes + line) paths = [ p - for p in (PROGRESS_PATH, "NOTES.md", MILESTONES_DIR) + for p in (PROGRESS_PATH, "NOTES.md", MILESTONES_DIR, PRODUCT_MD) if sb.exists(f"{sb.root}/{p}") ] _git(ctx, "add", "-A", *paths) @@ -1443,11 +1449,11 @@ def apply() -> str: apply=apply, probe=lambda: find_commit(sb, marker), ) - # PRODUCT.md as it is now, before routing (I-116, I-109): a feature added by hand at - # this merge stop gets its record and is planned, rather than the release going out - # without it; the sync is committed with the next milestone's bookkeeping. + # The spec for routing: the commit above already holds the re-read (a crash after it + # re-reads the committed file here, which changes nothing on disk). progress = _read_progress(ctx) spec_now, reread = reread_product_md(ctx, spec, progress) + reread = synced or reread remaining = progress.open_feature_ids() entries = [fact(n) for n in reread] + [ fact( @@ -1458,6 +1464,7 @@ def apply() -> str: ] return { "spec": spec_now, + "feature_runs": state.runs_stamped(plan.id), # older runs get this milestone "milestones_done": [*state.milestones_done, plan.id], "milestone": None, # recorded in progress.json; the next plan sets a new one "pr": None, @@ -1608,6 +1615,19 @@ def release(ctx: RunContext) -> Node: def node(state: ProductBuildState) -> dict[str, Any]: spec, rel = state.spec, state.release assert spec is not None and rel is not None + # the release decision was a human stop: a feature added to PRODUCT.md there is not + # built yet, so the release waits; the next `scio build` plans it (I-116 review) + progress = _read_progress(ctx) + spec_now, _ = reread_product_md(ctx, spec, progress) + blocked = _not_done(spec_now, progress) + if blocked: + return _halt( + ctx, + "release_failed", + "PRODUCT.md changed at the release stop; not done: " + + ", ".join(blocked) + + ". Run scio build again to plan them (I-116)", + ) sb = ctx.sandbox marker = f"[scio:release:{rel.tag}]" changelog = f"{sb.root}/CHANGELOG.md" diff --git a/scio/state/models.py b/scio/state/models.py index 3baf5c4..2dfeffa 100644 --- a/scio/state/models.py +++ b/scio/state/models.py @@ -631,6 +631,17 @@ def milestone_runs(self) -> list[FeatureRun]: if r.feature_id in ids and (r.milestone is None or r.milestone == mid) ] + def runs_stamped(self, milestone_id: str) -> list[FeatureRun]: + """``feature_runs`` with every run that has no milestone (recorded before the field) + stamped ``milestone_id``, the milestone that is closing: such a run belongs to it or + to an earlier one, so later milestones read it exactly. Without this, a skip + recorded by older code finished its feature in every later milestone and the loop + planned it, passed it over and escalated, milestone after milestone (I-116 review).""" + return [ + r if r.milestone is not None else r.model_copy(update={"milestone": milestone_id}) + for r in self.feature_runs + ] + # ---------- the graph state ---------- diff --git a/tests/graph/test_product_build_fake.py b/tests/graph/test_product_build_fake.py index 0f1e15a..21cd1d8 100644 --- a/tests/graph/test_product_build_fake.py +++ b/tests/graph/test_product_build_fake.py @@ -1188,9 +1188,14 @@ def test_a_hand_edit_of_product_md_survives_resets_merges_and_reaches_main(tmp_p assert any("re-read" in e.text for e in out.state.ledger) finally: ctx.close() - assert first in _git(repo, "show", f"main:{PRODUCT_MD}") # M1's merge took the first edit + # M1's merge took the first edit; M1's progress commit on main records the edit made at + # its merge stop (I-116 review), so main and milestone/2 both carry the second one + merged = _git(repo, "log", "-1", "--format=%H", "--grep=Merge milestone/1", "main") + assert first in _git(repo, "show", f"{merged}:{PRODUCT_MD}") + progress_commit = _git(repo, "log", "-1", "--format=%H", "--grep=scio:progress:M1", "main") + assert PRODUCT_MD in _git(repo, "show", "--name-only", "--format=", progress_commit) + assert second in _git(repo, "show", f"main:{PRODUCT_MD}") assert second in _git(repo, "show", f"milestone/2:{PRODUCT_MD}") - assert sum("[scio:sync:M2:plan_milestone]" in s for s in _subjects(repo, "milestone/2")) == 1 assert _git(repo, "status", "--porcelain", "--", PRODUCT_MD) == "" # committed, not dangling @@ -1239,6 +1244,11 @@ def test_a_feature_added_at_the_last_merge_stop_is_planned_not_left_out_of_the_r ("P4", "todo"), ] assert "### P4: Slugify todo titles" in _git(repo, "show", f"milestone/2:{PRODUCT_MD}") + # committed on main by M1's progress commit itself, so a release cut right there would + # tag a main that agrees with PRODUCT.md (I-116 review) + main_progress = ProgressFile.from_json(_git(repo, "show", f"main:{PROGRESS_PATH}")) + assert "P4" in [f.id for f in main_progress.features] + assert "### P4: Slugify todo titles" in _git(repo, "show", f"main:{PRODUCT_MD}") assert _git(repo, "tag", "--list") == "" @@ -1271,3 +1281,45 @@ def refused(*args: Any, **kwargs: Any) -> Any: err = capsys.readouterr().err assert "scio build stopped on GitHub" in err and "product-mode" in err assert "run the same scio build command again to resume" in err and "Traceback" not in err + + +def test_a_feature_added_at_the_release_stop_holds_the_release(tmp_path: Path) -> None: + """I-116 review: `release_decision` is a human stop too. A feature added to PRODUCT.md + there is not built, so `release` re-reads PRODUCT.md and refuses, naming it, before it + cuts or tags anything; the next `scio build` plans it.""" + from scio.product.build import release + from scio.state.models import ReleaseRecord + + harness = _bootstrap(tmp_path) + repo = tmp_path / "product" + progress = _progress(repo) + for rec in progress.features: + rec.status = "done" + (repo / PROGRESS_PATH).write_text(progress.to_json(), encoding="utf-8", newline="\n") + _git(repo, "commit", "-q", "-am", "chore: every feature done") + md = repo / PRODUCT_MD + spec = parse_product_md(md.read_text(encoding="utf-8")) + bar = "## Quality bar (release gate)" + md.write_text( + md.read_text(encoding="utf-8").replace( + bar, "### P4: Added at the release stop\n- [ ] it works\n\n" + bar, 1 + ), + encoding="utf-8", + newline="\n", + ) + ctx = _ctx(tmp_path, harness, "build-release-stop") + try: + state = ProductBuildState( + thread_id=ctx.thread_id, + product_dir=str(repo), + spec=spec, + budgets=ctx.budget, + release=ReleaseRecord(version="0.1.0", tag="v0.1.0", report="", ok=True), + ) + patch = release(ctx)(state) + assert patch.get("exit_reason") == "release_failed", patch.get("phase") + assert "P4 todo" in patch["ledger"][-1].text + finally: + ctx.close() + assert _git(repo, "tag", "--list") == "" + assert not any("[scio:release:" in s for s in _subjects(repo, "main")) diff --git a/tests/unit/test_runtime.py b/tests/unit/test_runtime.py index 5374cb7..db4dc15 100644 --- a/tests/unit/test_runtime.py +++ b/tests/unit/test_runtime.py @@ -371,3 +371,19 @@ def test_every_local_command_runs_with_the_product_interpreter_first_on_path( r = plain.exec([sys.executable, "-c", probe]) host_root = os.environ.get("VIRTUAL_ENV", "-") assert r.ok and r.stdout.strip().splitlines()[1] == host_root # no product interpreter + + +@pytest.mark.skipif( + sys.platform != "win32", reason="the PATH lookup of a bare word is Windows-only" +) +def test_a_bare_command_word_resolves_through_the_product_first_path(tmp_path: Path) -> None: + """I-114 review: on Windows a bare word is looked up with `shutil.which`, which used SCIO's + PATH even after the product's interpreter directory was put first for the command.""" + from scio.aci.sandbox.local import resolve_command + + product, scio_env = tmp_path / "product-bin", tmp_path / "scio-bin" + for d in (product, scio_env): + d.mkdir() + (d / "sciotool.cmd").write_text("@echo off\n", encoding="utf-8") + path = os.pathsep.join([str(product), str(scio_env)]) + assert Path(resolve_command(["sciotool", "-v"], None, path=path)[0]).parent == product diff --git a/tests/unit/test_state.py b/tests/unit/test_state.py index 8c93739..e8399d7 100644 --- a/tests/unit/test_state.py +++ b/tests/unit/test_state.py @@ -181,3 +181,39 @@ def test_runs_recorded_before_the_milestone_field_keep_the_old_rules() -> None: ).milestone is None ) + + +def test_closing_a_milestone_stamps_runs_recorded_before_the_milestone_field() -> None: + """I-116 review: a skip recorded by older code has no milestone, and a skip without one + finished its feature in every later milestone, so the loop planned the feature, passed + it over and escalated, milestone after milestone. Closing a milestone stamps such runs, + and the next milestone runs the deferred feature again.""" + from scio.state.models import ( + FeatureRun, + MilestoneFeatureOutput, + MilestonePlan, + ProductBuildState, + Tier, + ) + + old = FeatureRun( + feature_id="P1", attempt=1, thread_id="t-p1-a1", tier=Tier.T1, status="skipped" + ) + m2 = MilestonePlan( + number=2, + title="again", + branch="milestone/2", + features=[MilestoneFeatureOutput(id="P1")], + ) + state = ProductBuildState( + thread_id="t", + product_dir=".", + feature_runs=[old], + budgets=Budget(tokens_max=1, usd_max=1.0, advisor_calls_max=1), + ) + stamped = state.runs_stamped("M1") + assert [r.milestone for r in stamped] == ["M1"] and old.milestone is None + assert state.model_copy(update={"milestone": m2}).next_feature() is None # the old rule + after = state.model_copy(update={"feature_runs": stamped, "milestone": m2}) + nxt = after.next_feature() + assert nxt is not None and nxt.id == "P1" # deferred, so it runs again in M2 From 8748b4120b21539b982ad1f187a6d12d6240151f Mon Sep 17 00:00:00 2001 From: NewMe505 Date: Tue, 27 Oct 2026 23:46:36 -0700 Subject: [PATCH 3/3] test(sandbox): the shutil.which fake accepts the command's PATH (I-118) The I-114 review fix passes `path=` to shutil.which; the one test that fakes it for resolve_command took one argument. Ledger I-118 and guardrail 38. Co-Authored-By: Claude Opus 5.5 --- docs/ISSUE_LEDGER.md | 8 ++++++++ docs/LESSONS_LEARNED.md | 1 + tests/unit/test_sandbox.py | 3 ++- 3 files changed, 11 insertions(+), 1 deletion(-) diff --git a/docs/ISSUE_LEDGER.md b/docs/ISSUE_LEDGER.md index d64f168..a82798c 100644 --- a/docs/ISSUE_LEDGER.md +++ b/docs/ISSUE_LEDGER.md @@ -1052,3 +1052,11 @@ Template: - Fix: the file was verified, nothing to undo - Sweep: the other two agents' reports name no shell-script edits; the orchestrator's script edits this session asserted their anchors. Guard: guardrail 17 now cites I-117, and the brief template for parallel agents names the rule in its first lines. The working tree's line-ending check (`file ... | grep CRLF`, run at the gate) printed nothing - Resolved: 2026-10-28 · Status: resolved + +### I-118 · Changing how `shutil.which` is called broke a test that replaced it with a one-argument fake +- Found: 2026-10-28 · issue #69, the full check on `771a42e` · `TypeError: test_resolve_command_rewrites_the_interpreter_and_finds_cmd_shims_on_windows..() got an unexpected keyword argument 'path'` (`tests/unit/test_sandbox.py:252`); 1 failed, 400 passed +- Symptom: the pre-push check failed; nothing was pushed +- Root cause: the review fix for I-114 made `resolve_command` call `shutil.which(head, path=...)`, and I ran the tests of the files I had changed but not the test that monkeypatches `shutil.which` for `resolve_command` +- Fix: the fake accepts `path` +- Sweep: every fake of `shutil.which` in the tests (`grep -rn "shutil.which\|\"which\"" tests`): `test_sandbox.py:252` (fixed) and `test_aci_tools.py:111`, which patches `repo_search`'s own call (unchanged signature). Guard: the test itself, now with `path`, plus guardrail 38: before changing how a callable is called, grep for its fakes and monkeypatches +- Resolved: 2026-10-28 · Status: resolved diff --git a/docs/LESSONS_LEARNED.md b/docs/LESSONS_LEARNED.md index d776689..bcdbb8b 100644 --- a/docs/LESSONS_LEARNED.md +++ b/docs/LESSONS_LEARNED.md @@ -28,6 +28,7 @@ Every bug and every correction becomes one rule here (the owner's rule, 2026-10- 35. **Always** run a product's commands, and a worker turn, with the product's interpreter first on PATH, never with SCIO's inherited environment (I-114). Guard: `symbol:with_interpreter_first`, `test:test_worker_turn_runs_with_the_product_interpreter_first_on_path`, `test:test_every_local_command_runs_with_the_product_interpreter_first_on_path` 36. **Always** name the cap that stopped a run (the run's total or one advisor call per step), never a counter that hides it (I-115). Guard: `test:test_halt_reason_names_the_per_step_cap_when_the_total_has_headroom` 37. **Never** let a human decision silently drop a feature from the product: `skip` defers it, a skip finishes a feature only in its own milestone, and the release refuses while any feature in PRODUCT.md is not done (I-116). Guard: `test:test_a_skipped_feature_is_deferred_to_the_next_milestone_not_dropped`, `test:test_the_release_gate_refuses_while_a_feature_is_not_done`, `test:test_a_skip_finishes_a_feature_only_in_the_milestone_it_was_skipped_in` +38. **Always** grep for a callable's fakes and monkeypatches before changing how it is called, and run the tests that patch it, not only the tests of the changed files (I-118). Guard: `test:test_resolve_command_rewrites_the_interpreter_and_finds_cmd_shims_on_windows` ## Standing pre-flight list (apply at the start of every milestone) diff --git a/tests/unit/test_sandbox.py b/tests/unit/test_sandbox.py index 066463c..754a341 100644 --- a/tests/unit/test_sandbox.py +++ b/tests/unit/test_sandbox.py @@ -249,7 +249,8 @@ def test_resolve_command_rewrites_the_interpreter_and_finds_cmd_shims_on_windows assert resolve_command(["python"], None) == [sys.executable] assert resolve_command([], None) == [] monkeypatch.setattr(sys, "platform", "win32") - monkeypatch.setattr(shutil, "which", lambda name: {"npm": "C:/n/npm.CMD"}.get(name)) + # `path`: the command's own PATH, the product's first (I-114 review, I-118) + monkeypatch.setattr(shutil, "which", lambda name, path=None: {"npm": "C:/n/npm.CMD"}.get(name)) assert resolve_command(["npm", "install"], None) == ["C:/n/npm.CMD", "install"] assert resolve_command(["node", "x.ts"], None) == ["node", "x.ts"] # not found: unchanged assert resolve_command(["C:/n/node.exe", "x"], None) == ["C:/n/node.exe", "x"] # a path