diff --git a/CHANGELOG.md b/CHANGELOG.md index 282e1235..7d666f0c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,6 +42,16 @@ starts. --- +## #734 — 2026-09-11 + +- **CHANGED** — Refresh the shared `upgrade.md` workflow and the shipped test files + together. Upgrade now refuses invalid refresh destinations or unreadable manifest + scope before replacing the installer, preserves template declines, and stops after + a failed required command. Correct the named path or input before retrying. +- **CHANGED** — CI interpreting the state-leak guard's exit status must retain + pytest's interruption/internal-error handling; leaks still fail an otherwise + successful session. Refresh `scripts/conftest.py` and its state-guard tests together. + ## #731 — 2026-09-10 - **CHANGED — state-write detection:** refresh the installed engine-root diff --git a/docs/agentic-dev-kit/workflows/upgrade.md b/docs/agentic-dev-kit/workflows/upgrade.md index bb11f8e9..3e1444d8 100644 --- a/docs/agentic-dev-kit/workflows/upgrade.md +++ b/docs/agentic-dev-kit/workflows/upgrade.md @@ -1,10 +1,10 @@ # Upgrade -Upgrade this repo's agentic-dev-kit installation. Runs on a branch. Two of its file -replacements are unconditional and named here rather than implied — Step 2 overwrites -`init.sh` and `docs/templates/*.tmpl` with the fetched kit's copies, because refreshing the -installer is the point of the step. **Everything else is gated**: `init.sh --no-clobber` for -the seeded docs, and your per-file decision in Step 3 for the engines. +Upgrade this repo's agentic-dev-kit installation. Runs on a branch. After preflight, +Step 2 replaces `init.sh` with the fetched kit's copy to refresh the installer. +Template refresh respects the baseline's recorded `not_installed` decisions; +a partial record skips template copies. Seeded docs use `init.sh --no-clobber`, +and Step 3 requires your per-file decision for engines. **Do not simplify that back to a blanket "non-destructive".** It said "never replaces a file without knowing it is safe to replace" for as long as Step 2 ran `init.sh` bare, which @@ -20,8 +20,8 @@ instruction it produced was *don't bother looking*, which is the opposite of wha was closed to make possible. So: **run Step 1's `kit_doctor` before Step 2 overwrites anything** — a locally-edited installer shows up there as `LOCALLY EDITED` and an out-of-date one as `STALE`, which is the difference between an edit you are about to lose -and a version you are meant to take. The unconditional `cp` itself is unchanged and -`#339` stays open for it. +and a version you are meant to take. Installer refresh still replaces local edits +after preflight. > **The invariant this rests on.** Engines are **kit-owned**; config is **adopter-owned**. > Everything project-specific — paths, tracker, review-bot markers, CI policy, model @@ -35,7 +35,8 @@ and a version you are meant to take. The unconditional `cp` itself is unchanged Four shapes exist in the wild and they need different handling. Determine which: ```bash -ls config/dev-model.yaml 2>/dev/null && echo "has config" || echo "NO CONFIG" +REPO="$(git rev-parse --show-toplevel)" || exit 1 +test -f "$REPO/config/dev-model.yaml" && echo "has config" || echo "NO CONFIG" ``` - **No `config/dev-model.yaml`** → this repo predates the config surface entirely (a kit @@ -55,7 +56,7 @@ from here every write must name which one.** Bind both roots now, before the fir write, and use them for the rest of the workflow: ```bash -REPO="$(git rev-parse --show-toplevel)" # the repo being upgraded +REPO="${REPO:?REPO is not set — re-run the config check}" # the repo being upgraded KIT=/tmp/agentic-dev-kit # the kit you are upgrading TO echo "REPO=$REPO"; echo "KIT=$KIT"; echo "pwd=$(pwd)" ``` @@ -360,10 +361,9 @@ Take the fetched kit's copy first: ```bash cd "$REPO" || exit 1 # every write below lands here, not in $KIT -cp "${KIT:?KIT is not set — re-run Step 0}/init.sh" "${REPO:?REPO is not set — re-run Step 0}/init.sh" -chmod +x "${REPO:?REPO is not set — re-run Step 0}/init.sh" # the kit ships it 100755; a copy can lose the bit -mkdir -p "${REPO:?REPO is not set — re-run Step 0}/docs/templates" -_gate_failed=0 +# Collect the permitted copies without changing the destination. +set -- +_partial=0 for _tmpl in "${KIT:?KIT is not set — re-run Step 0}"/docs/templates/*.tmpl; do _rel="docs/templates/$(basename "$_tmpl")" python3 -c 'import json,pathlib,sys @@ -395,10 +395,10 @@ sys.exit(0 if sys.argv[2] in declared else 1)' \ "${REPO:?REPO is not set — re-run Step 0}" "$_rel" && _verdict=0 || _verdict=$? case "$_verdict" in 0) echo "declined (recorded in not_installed) — not copied: $_rel" ;; - 1) cp "$_tmpl" "${REPO:?REPO is not set — re-run Step 0}/$_rel" ;; + 1) set -- "$@" "$_tmpl" ;; 3) echo "no declared scope recorded — not copied: $_rel"; _partial=1 ;; *) echo "STOP: $REPO/kit-manifest.json is not a readable manifest, so the declared set is unknown. Copied nothing." >&2 - _gate_failed=1; break ;; + exit 1 ;; esac done if [ "${_partial:-0}" -ne 0 ]; then @@ -409,11 +409,46 @@ if [ "${_partial:-0}" -ne 0 ]; then echo " whole key (see Step 5, and kit #388). Reconcile the paths Step 4 named and" >&2 echo " re-run this block if you want the templates refreshed." >&2 fi -if [ "$_gate_failed" -ne 0 ]; then - echo "Not running init.sh. Fix kit-manifest.json, then re-run this block." >&2 -else - "${REPO:?REPO is not set — re-run Step 0}/init.sh" --no-clobber -fi +# Check every refresh destination before the first write. No concurrent writer +# may alter these paths between preflight and the copies. +python3 -B - "${REPO:?REPO is not set — re-run Step 0}" "$@" <<'PYREFRESH' || exit 1 +import os +from pathlib import Path +import stat +import sys + +repo = Path(sys.argv[1]) +try: + if not repo.is_absolute() or repo.resolve() != repo or not repo.is_dir(): + raise ValueError(f"repository is not an absolute, unaliased directory: {repo}") + targets = [(repo / "init.sh", False), (repo / "docs", True), + (repo / "docs/templates", True)] + targets.extend((repo / "docs/templates" / Path(source).name, False) + for source in sys.argv[2:]) + for target, directory in targets: + for path in [*reversed(target.relative_to(repo).parents)][1:]: + parent = repo / path + if os.path.lexists(parent): + mode = parent.lstat().st_mode + if not stat.S_ISDIR(mode): + raise ValueError(f"refresh parent is not a real directory: {parent}") + if os.path.lexists(target): + info = target.lstat() + valid = stat.S_ISDIR(info.st_mode) if directory else ( + stat.S_ISREG(info.st_mode) and info.st_nlink == 1) + if not valid: + raise ValueError(f"refresh destination has an unexpected kind or alias: {target}") +except (OSError, ValueError) as exc: + print(f"STOP: {exc}. Copied nothing.", file=sys.stderr) + sys.exit(1) +PYREFRESH +cp "${KIT:?KIT is not set — re-run Step 0}/init.sh" "${REPO:?REPO is not set — re-run Step 0}/init.sh" || exit 1 +chmod +x "${REPO:?REPO is not set — re-run Step 0}/init.sh" || exit 1 +mkdir -p "${REPO:?REPO is not set — re-run Step 0}/docs/templates" || exit 1 +for _tmpl in "$@"; do + cp "$_tmpl" "${REPO:?REPO is not set — re-run Step 0}/docs/templates/$(basename "$_tmpl")" || exit 1 +done +"${REPO:?REPO is not set — re-run Step 0}/init.sh" --no-clobber || exit 1 ``` The refreshed migrator also owns the additive `parallel:` launcher block. It preserves @@ -525,8 +560,8 @@ implementation rather than continuing to invent one: worse half; - the refusal has to stop **the workflow**, not just the loop. `break` leaves the `for` loop and the next line still runs `init.sh` — printing "Copied nothing" and then - proceeding past the point the prose calls a hard stop. `_gate_failed` is what makes the - stop real; + proceeding past the point the prose calls a hard stop. `exit 1` stops before any + refresh write or initialization; - a well-formed object is not a readable scope. `{"kit_commit": …, "not_installed": 5}` raised `TypeError` on the membership test and exited 1 (copy); a **string** there was worse than that, because `in` on a string is a SUBSTRING test — no error, and a @@ -742,8 +777,9 @@ entries are exactly where the risk is. workflow, then take the rendered binding. Stop for irreconcilable local behavior rather than retaining an adapter that bypasses the new gate. PR `#595`'s `post-merge-systemize` entry is the worked instance. -- **Templates** (`docs/templates/`) — refresh freely; the *rendered* docs are yours and - are never touched. +- **Templates** (`docs/templates/`) — refresh only within the recorded install set, + using Step 2's gate. Preserve `not_installed` declines and skip refresh when the + baseline is partial. The *rendered* docs are yours and are never touched. - **`.claude/settings.json`** — if this repo has its own, **merge** the kit's hooks and permissions into it rather than replacing; it likely carries project-specific entries. @@ -781,13 +817,14 @@ Commit the rewritten `kit-manifest.json` with the rest of the upgrade. ## Step 5 — Verify ```bash -uv run "${REPO:?REPO is not set — re-run Step 0}"//kit_doctor.py --manifest /tmp/agentic-dev-kit/kit-manifest.json -tmp="$(mktemp -d)" && DEVKIT_STATE_ROOT="$tmp" uv run --with pytest --with pyyaml python "${REPO:?REPO is not set — re-run Step 0}"//run_installed_tests.py --root "${REPO:?REPO is not set — re-run Step 0}" -uv run "${REPO:?REPO is not set — re-run Step 0}"//check_doc_budget.py +uv run "${REPO:?REPO is not set — re-run Step 0}"//kit_doctor.py --manifest /tmp/agentic-dev-kit/kit-manifest.json || exit 1 +tmp="$(mktemp -d)" || exit 1 +DEVKIT_STATE_ROOT="$tmp" uv run --with pytest --with pyyaml python "${REPO:?REPO is not set — re-run Step 0}"//run_installed_tests.py --root "${REPO:?REPO is not set — re-run Step 0}" || exit 1 +uv run "${REPO:?REPO is not set — re-run Step 0}"//check_doc_budget.py || exit 1 ``` -**`DEVKIT_STATE_ROOT` is not optional here, and the `&&` is what makes it -fail closed.** `/pr_watch.py` computes its persistence root once, at import +**`DEVKIT_STATE_ROOT` is not optional here. Each command must succeed before +the next runs, including creation of the temporary root.** `/pr_watch.py` computes its persistence root once, at import time — the only engine that reaches `state/` at all, and it resolves at import rather than per call, so an override has to be in the environment before the process starts. The resolution has three branches, not two: @@ -813,13 +850,14 @@ declined directory to pytest and stopping before an installed test can run. That current form of the case `#40`/`#132` first exposed. -Write it as the two-step `tmp="$(mktemp -d)" && …`, not as an inline +Keep the separate assignment and its `|| exit 1` check before the test command, +not an inline `DEVKIT_STATE_ROOT=$(mktemp -d) …`. The inline form fails **open**: a failed `mktemp -d` prints nothing to stdout, so the var is set to the empty string, and `_resolve_state_root` treats an empty value as *no override at all* and falls back to the repo default — landing the whole suite in live `state/`, -which is the one outcome this line exists to prevent. The `&&` makes the -assignment's exit status gate the run, so a failed `mktemp` skips the tests +which is the one outcome this line exists to prevent. The explicit refusal makes +the assignment's exit status gate the run, so a failed `mktemp` skips the tests instead of silently redirecting them at the thing they would damage. `kit_doctor` should now report zero mismatches of every kind — `differs`, `STALE`, diff --git a/docs/kit-handoff-history.md b/docs/kit-handoff-history.md index 3f8846c3..55efdddb 100644 --- a/docs/kit-handoff-history.md +++ b/docs/kit-handoff-history.md @@ -5,6 +5,53 @@ and the next step there; this file is append-only history. ## Session log +## Session — 2026-09-07 (#534 residual repairs, in Claude Code) + +**Theme —** Repair what `#534` still carried, and field-verify the item that merged +without ever being checked in an adopter. + +- [PR #705](https://github.com/topij/agentic-dev-kit/pull/705) merged as `7cb0868`. + Item 1 (`_repo_layout` engine-dir resolution) was **not** re-done: it merged in + PR #545, and `kit-handoff-history.md`'s 2026-08-21 block records the residual — those + issues stayed open because nothing verified their acceptance criteria in the field. + That verification is what this session did. +- **The proposed kit-repo-only marker cannot carry cause 1, and the reason generalises.** + It skips on a *missing path*, and the question these tests need answered is whether the + file at that path is the kit's copy. `conftest.py` gains the predicate it cannot + express, derived from `kit_commit` being written only by `--record-install`. + `test_shipped_manifest_covers_every_kit_owned_file` is restated rather than skipped, so + an adopter gains coverage where they had a permanent red. +- **`.codex/hooks.json` and `.claude/settings.json` are the sharp case** — the kit prints + both and writes neither (`#303`), so in an adopter those paths hold hand-written + registrations and a path check accepts them. Reference copies now ship, engine-relative + and `KIT_OWNED`, with a drift guard pinning reference to live. +- **Field-verified in disposable copies of the adopter fixture**, the original asserted + equal to `fixture-inventory-after-reg01.json` before and unchanged after each run. + Item 1 resolves there; a cause-1 test went from failing to passing, and others from + failing to skipping. The silent false pass was proven fixed **by mutation, not by a passing run** — + a pass is what that defect looks like — and that mutation is what caught a read site an + edit had missed. +- **What the panel found is where the risk sat.** Its rounds are enumerated with their + heads and fixes in the [disposition](https://github.com/topij/agentic-dev-kit/pull/705#issuecomment-5574151329). + Every finding was in a claim the author made — a predicate said to be safe, an accessor + said to decline gracefully, a test said to guard a fix, a docstring describing a step + its function does not perform — and none was in a mechanism. Rounds whose CI was green + still carried them. +- **`#534` stays open.** The typed decline reasons are deliberately out of scope; the + disposition also carries the follow-up candidates the panel raised and this PR did not + take. +- An occurrence on [`#393`](https://github.com/topij/agentic-dev-kit/issues/393#issuecomment-5573705268) + records that the interpreter `uv run` resolves locally and the version + `.github/workflows/test.yml` pins are not the same, so a suite failure reproducible on + `main` is invisible to CI. That issue stays open. + +- **Session friction routed at close-out.** [`#706`](https://github.com/topij/agentic-dev-kit/issues/706) + files the manifest going stale between `--generate-manifest` and the commit, caught only + by the full suite. The guard is not broken — it caught every instance — so the finding is + about when it reports, and the proposed fix moves that to `scripts/hooks/pre-push`. + +The later replay decision and next action are in the latest session block. + ## Session — 2026-09-07 (live Codex hooks batch) **Theme —** Complete the parked Codex observations and preserve their scope. diff --git a/docs/kit-handoff.md b/docs/kit-handoff.md index db369767..5275c23a 100644 --- a/docs/kit-handoff.md +++ b/docs/kit-handoff.md @@ -14,10 +14,29 @@ > Older session blocks graduate to [`kit-handoff-history.md`](kit-handoff-history.md) once > this file crosses its line budget (`scripts/check_doc_budget.py`). -Last updated: 2026-09-11 — ITEM5-B kit repair merged; retained-update decision remains separate. +Last updated: 2026-09-11 — ITEM5-B source follow-up approved; retained update remains separate. Phase 5 delivery item 5 remains incomplete; item 6's replay remains complete. -## Latest session — 2026-09-11 (ITEM5-B repair delivery, in Codex) +## Latest session — 2026-09-11 (ITEM5-B source-review follow-up, in Codex) + +The operator approved the kit-only source repair, tests/release metadata, required +review and merge when clean, followed by preparation of a revised retained-update +packet. The [source repair record](../saved_plans/phase5-item5-b-source-review-repair_2026-09-11.md) +retains the exact approval, complete pre-fix review receipt and author verification. +[PR #734](https://github.com/topij/agentic-dev-kit/pull/734) carries that work; #733 +preserves the earlier unapproved packet and incomplete review history. + +No retained fixture/source update is approved. The special-file-root limitation, +ownership/function/field-exit distinctions, item 6/replay completion, #723 deferral, +#585 placement and delivered #724/#722 batch remain. The friction sweep stays parked. + +▶ Next: complete PR #734's required review and pr-watch, merge under the explicit +operator authority, then prepare the revised packet against the delivered source. +Obtain its exact retained-update approval before execution. Fixture merge stays excluded. + +______________________________________________________________________ + +## Session — 2026-09-11 (ITEM5-B repair delivery, in Codex) The operator approved ITEM5-B-KIT-REVIEW-02 and subsequently said “merge when ready.” [PR #731](https://github.com/topij/agentic-dev-kit/pull/731) merged as @@ -40,10 +59,7 @@ Ownership acceptance does not establish functionality or field exit. The repair not update the retained installation. Item 6's replay, #723's deferral, #585's earlier placement and #724's delivered #722 batch are preserved. The friction sweep stays parked. -▶ Next: prepare the exact retained-update decision packet for the merged #731 repair, -starting from the post-acceptance checkpoint linked in the execution record. Preparation -is authorized; execution needs its own exact decision. Do not repeat UPDATE-01 or the -completed replay, refresh the baseline, exercise clients/profiles/trackers, or merge the fixture PR. +The latest session block owns the subsequent source follow-up and next decision. ______________________________________________________________________ @@ -343,55 +359,6 @@ the later ITEM5-B execution, retained baseline and next action. ______________________________________________________________________ -## Session — 2026-09-07 (#534 residual repairs, in Claude Code) - -**Theme —** Repair what `#534` still carried, and field-verify the item that merged -without ever being checked in an adopter. - -- [PR #705](https://github.com/topij/agentic-dev-kit/pull/705) merged as `7cb0868`. - Item 1 (`_repo_layout` engine-dir resolution) was **not** re-done: it merged in - PR #545, and `kit-handoff-history.md`'s 2026-08-21 block records the residual — those - issues stayed open because nothing verified their acceptance criteria in the field. - That verification is what this session did. -- **The proposed kit-repo-only marker cannot carry cause 1, and the reason generalises.** - It skips on a *missing path*, and the question these tests need answered is whether the - file at that path is the kit's copy. `conftest.py` gains the predicate it cannot - express, derived from `kit_commit` being written only by `--record-install`. - `test_shipped_manifest_covers_every_kit_owned_file` is restated rather than skipped, so - an adopter gains coverage where they had a permanent red. -- **`.codex/hooks.json` and `.claude/settings.json` are the sharp case** — the kit prints - both and writes neither (`#303`), so in an adopter those paths hold hand-written - registrations and a path check accepts them. Reference copies now ship, engine-relative - and `KIT_OWNED`, with a drift guard pinning reference to live. -- **Field-verified in disposable copies of the adopter fixture**, the original asserted - equal to `fixture-inventory-after-reg01.json` before and unchanged after each run. - Item 1 resolves there; a cause-1 test went from failing to passing, and others from - failing to skipping. The silent false pass was proven fixed **by mutation, not by a passing run** — - a pass is what that defect looks like — and that mutation is what caught a read site an - edit had missed. -- **What the panel found is where the risk sat.** Its rounds are enumerated with their - heads and fixes in the [disposition](https://github.com/topij/agentic-dev-kit/pull/705#issuecomment-5574151329). - Every finding was in a claim the author made — a predicate said to be safe, an accessor - said to decline gracefully, a test said to guard a fix, a docstring describing a step - its function does not perform — and none was in a mechanism. Rounds whose CI was green - still carried them. -- **`#534` stays open.** The typed decline reasons are deliberately out of scope; the - disposition also carries the follow-up candidates the panel raised and this PR did not - take. -- An occurrence on [`#393`](https://github.com/topij/agentic-dev-kit/issues/393#issuecomment-5573705268) - records that the interpreter `uv run` resolves locally and the version - `.github/workflows/test.yml` pins are not the same, so a suite failure reproducible on - `main` is invisible to CI. That issue stays open. - -- **Session friction routed at close-out.** [`#706`](https://github.com/topij/agentic-dev-kit/issues/706) - files the manifest going stale between `--generate-manifest` and the commit, caught only - by the full suite. The guard is not broken — it caught every instance — so the finding is - about when it reports, and the proposed fix moves that to `scripts/hooks/pre-push`. - -The later replay decision and next action are in the latest session block. - -______________________________________________________________________ - > Older session entries (below the live blocks above) live in [`kit-handoff-history.md`](kit-handoff-history.md). > Active open items from them are folded into the "Open for next session" lists above. diff --git a/kit-manifest.json b/kit-manifest.json index b49e91b5..bd9d30b2 100644 --- a/kit-manifest.json +++ b/kit-manifest.json @@ -72,7 +72,7 @@ }, "docs/agentic-dev-kit/workflows/upgrade.md": { "role": "workflow", - "sha256": "c29baa57bc48b2d74e5e553745028f84bd2e8d400e88d891c9336d8496724cf6" + "sha256": "ef328af59f53a619e373c8f9bead1daf20b1f58715fa27c1e583b66ee0513623" }, "docs/agentic-dev-kit/workflows/wrap-up.md": { "role": "workflow", @@ -120,7 +120,7 @@ }, "scripts/conftest.py": { "role": "engine", - "sha256": "ecebbbc42cd6a8960e187abda41c5917e39aa1623b438ad932c7102cdf3ab03c" + "sha256": "b43fc728bbb1a5a71e6f91afcd37a07940014cceb34f073a0617fe2047a75e16" }, "scripts/dev_session.sh": { "role": "engine", @@ -265,7 +265,7 @@ }, "scripts/tests/test_init_sh.py": { "role": "test", - "sha256": "87cca005542d8dfc3ff923f5706ae77a63a4068b8c852499c2ac05718db323bc" + "sha256": "41ad37977dad41e6ec739bef369138d93e1f4ad9ff7036172ee8e61cb36c49a4" }, "scripts/tests/test_kit_doctor.py": { "role": "test", @@ -297,7 +297,7 @@ }, "scripts/tests/test_portability.py": { "role": "test", - "sha256": "6d889025b72306a4dfe96fcf3eefa8b7c75d7f08112003e49ecec6c3f4f27e0b" + "sha256": "0bc0e8fc7deb7a10b580c21a10670b3a04b802518d486ebcf062cb894cd8c1ec" }, "scripts/tests/test_pr_followup_hook.py": { "role": "test", @@ -321,7 +321,7 @@ }, "scripts/tests/test_state_guard.py": { "role": "test", - "sha256": "9420de32bcba5232f13e9d8258de6f5c08ccb0a398513ff83e01cd731f377613" + "sha256": "e4a4c5614250ac46509e706507317d1cb40338b1bba51699c902b7269bc84433" }, "scripts/verify_live_validation_bundle.py": { "role": "repo-only", diff --git a/saved_plans/codex-parity-plan_2026-08-23.md b/saved_plans/codex-parity-plan_2026-08-23.md index 8c6d67a3..d95ca827 100644 --- a/saved_plans/codex-parity-plan_2026-08-23.md +++ b/saved_plans/codex-parity-plan_2026-08-23.md @@ -451,8 +451,11 @@ historical observation it was and is not silently refreshed. inherited FIFO-root gap and P3 root-symlink coverage gap; the [follow-up packet](phase5-item5-b-review-followup-decision_2026-09-10.md) was approved as ITEM5-B-KIT-REVIEW-02 on 2026-09-11. The [follow-up execution](phase5-item5-b-review-followup-execution_2026-09-11.md) records the applied scope, final panel and separately authorized merge of kit #731 as - `e6d6e77d118454349f8e8bb046e99ef3009c5f5c`. Prepare the next exact retained-update - decision packet; neither repair approval nor kit merge authorizes its execution. + `e6d6e77d118454349f8e8bb046e99ef3009c5f5c`. The subsequent UPDATE-02 preparation + in #733 led to the separately approved [source-review repair](phase5-item5-b-source-review-repair_2026-09-11.md) + in #734. Complete its required review and merge when clean, then prepare the + revised exact retained-update packet. Neither repair approval nor kit merge + authorizes retained execution. Preserve the old packet and review receipts. Fixture merge remains excluded; the nonfunctional custom wrap-up carries ownership acceptance only. No original continuity or prior field credit was recreated. diff --git a/saved_plans/phase5-item5-b-source-review-repair-evidence_2026-09-11/approval.json b/saved_plans/phase5-item5-b-source-review-repair-evidence_2026-09-11/approval.json new file mode 100644 index 00000000..54e11b6b --- /dev/null +++ b/saved_plans/phase5-item5-b-source-review-repair-evidence_2026-09-11/approval.json @@ -0,0 +1,8 @@ +{ + "date": "2026-09-11", + "operator_text": "I approve that repair, its tests and release metadata, review and merge when clean, then preparation of a revised retained-update packet", + "scope_record": "saved_plans/phase5-item5-b-update02-review-triage_2026-09-11.md", + "scope_record_sha256": "88a76c174ee597c219773db799d393d704bf173002e1238fd70d3da69e19ee3a", + "retained_writes_approved": false, + "preceding_head": "57b697f7d93951ba3a1b1bd0c106e155a6ae89ca" +} diff --git a/saved_plans/phase5-item5-b-source-review-repair-evidence_2026-09-11/approved-scope.md b/saved_plans/phase5-item5-b-source-review-repair-evidence_2026-09-11/approved-scope.md new file mode 100644 index 00000000..062fe074 --- /dev/null +++ b/saved_plans/phase5-item5-b-source-review-repair-evidence_2026-09-11/approved-scope.md @@ -0,0 +1,101 @@ +# ITEM5-B-UPDATE-02 — current review triage + +**Source-scope decision pending; retained execution remains unapproved.** The +prepared source remains `e6d6e77d118454349f8e8bb046e99ef3009c5f5c`. Do not edit its +frozen payload copies to satisfy a review or obtain UPDATE-02 approval while the +source findings below lack an operator disposition. This record proposes a separate +kit-only repair; it does not approve that repair, a deferral or a new retained source. + +## Receipt and verification + +The complete [CodeRabbit review](https://github.com/topij/agentic-dev-kit/pull/733#pullrequestreview-5178085353) +of `cf373efd3a4d00b36987f73574d567eef004594a` was read with paginated `gh api` +review, inline-comment and issue-comment requests from +`/Users/topi/Coding/agentic-dev-kit` on 2026-09-11. The +[raw receipt and source comparison](phase5-item5-b-update02-evidence_2026-09-11/coderabbit-current-review.json.gz) +preserve complete bodies, including the combined inline findings and top-level +nitpick, before any following fix. The bot excluded compressed evidence; its review +is not a claim that it inspected those archives. + +`git show` and `git diff 60fe0dc7ad68922d064c0cf401cff2c4c6d607ac e6d6e77d118454349f8e8bb046e99ef3009c5f5c -- ` +in that directory at `cf373efd3a4d00b36987f73574d567eef004594a` on 2026-09-11 +established that the supplied workflow, conftest and state-guard test payloads equal +the selected source and cockpit files. The cited workflow sections and session-finish +hook predate #731; these findings are not newly introduced #731 regressions. +The evidence retains each payload digest, source blob and actual source diff. + +The documented config-check command, run from the cockpit root and then its `docs` +subdirectory at that same revision/date, found the root config but printed +`NO CONFIG` from `docs`. An isolated invocation of the extracted session-finish +hook, with a synthetic leak snapshot and session, replaced interruption and +internal-error statuses with the test-failure status. That is a bounded hook probe, +not a nested pytest session or retained-fixture test. No generic upgrade mutation, +symlink reproduction, initialization or retained writes ran during this triage. + +## Findings and proposed disposition + +| Review finding | Assessment and boundary | +|---|---| +| [Template contract](https://github.com/topij/agentic-dev-kit/pull/733#discussion_r3988648287) | Valid inherited contradiction: the opening and later free-refresh instruction conflict with `not_installed`. A source repair must preserve recorded template declines. | +| [Config path](https://github.com/topij/agentic-dev-kit/pull/733#discussion_r3988648291) | Reproduced inherited working-directory error. Resolve the intended repo before the config probe and use its absolute path. | +| [Manifest preflight](https://github.com/topij/agentic-dev-kit/pull/733#discussion_r3988648298) | Valid inherited ordering problem: init is overwritten before the manifest refusal. Preserve the distinction the bot's generated prompt conflates: corrupt/dangling manifests stop before mutation and initialization; a valid partial record skips template refresh but retains its existing initialization route. | +| [Destination aliases and failed mutations](https://github.com/topij/agentic-dev-kit/pull/733#discussion_r3988648306) | The combined comment identifies inherited destination-validation and unchecked-command gaps in generic upgrade. Static source inspection supports them; this triage did not reproduce writes through aliases. The local UPDATE-02 procedure does not execute generic upgrade or init and already requires destination checks. Repairing the shipped workflow is separate scope. | +| [Verification command status](https://github.com/topij/agentic-dev-kit/pull/733#discussion_r3988648314) | Valid inherited shell-block gap: later successful commands can mask earlier failures. The source workflow must stop on an unsuccessful required check. UPDATE-02 separately records each command's result. | +| [Pytest exit status](https://github.com/topij/agentic-dev-kit/pull/733#discussion_r3988648316) | The bounded hook probe confirms the inherited overwrite. A source repair must retain non-OK statuses and preserve the leak message; nested-session coverage is required before delivery. | +| [Validator Git environment](https://github.com/topij/agentic-dev-kit/pull/733#discussion_r3988648327) | Disputed: refusal before Git is the declared safety contract, not an accidental availability failure. The audit refuses inherited controls before Git reads; `check=True` prevents the revision command after refusal. The parent environment is unchanged between that check and the revision read. Silently sanitizing only validation would cease checking the environment later commands use. Keep refusal and require the operator to correct the invoking environment explicitly. | +| [Nested pytest timeout](https://github.com/topij/agentic-dev-kit/pull/733#pullrequestreview-5178085353) | Valid inherited harness limitation: `_run_pytest` lacks a timeout. A bounded source follow-up can report timeout with captured output. This does not extend the accepted state-root special-file detection scope. | + +The [existing environment proof](phase5-item5-b-update02-evidence_2026-09-11/runtime-guard-proof.json.gz) +records actual refusal and mutation/restoration evidence. It is preserved historical +evidence; no security probe or failed reviewer was rerun to answer the bot. + +These source findings remain open pending a scope decision. Recording them here +is not operator acceptance or a tracker deferral. No tracker payload is authorized. +The [incomplete adversarial receipt](https://github.com/topij/agentic-dev-kit/pull/733#issuecomment-5633204702) +also remains incomplete. CodeRabbit's delivered review does not retrospectively +complete that lens or review compressed evidence it excluded. + +## Proposed bounded kit-only follow-up + +After explicit approval, repair the active kit source in a separate branch/ready PR: + +- `docs/agentic-dev-kit/workflows/upgrade.md`: correct the template contract and + initial config-root lookup; perform read-only scope and destination validation + before mutation; stop on failed writes or required verification. Preserve absent, + partial, corrupt/dangling and recorded-decline distinctions. +- `scripts/conftest.py`: preserve incoming non-OK pytest exit status when reporting + a leak; retain the ordinary successful-session-to-failure transition and message. +- `scripts/tests/test_init_sh.py`, `scripts/tests/test_portability.py` and + `scripts/tests/test_state_guard.py`: use the existing workflow/hook harnesses for + the changed paths; cover preflight-before-write, command failure, declared scope, + alias refusal, config lookup from a subdirectory and interrupted/internal-error + nested sessions. Bound the nested child pytest helper and retain timeout output. +- `CHANGELOG.md` and the generated source `kit-manifest.json`: record the observable + contract changes and publish matching source hashes through the existing process. +- Scoped kit execution/handoff/sprint records: retain full review receipts before + fixes, full `make test` output, separate #561 shell parses, and #393 disclosure. + Finish required independent review and pr-watch before the kit repair's merge. + +This does not authorize editing UPDATE-02's frozen source or payloads in place. +After the separate repair is delivered, prepare a new exact source selection, +payload hashes, destination ledger, predicted baseline, verification and approval +question against fresh read-only retained checkpoints. Preserve the old packet, +questions, ledgers and evidence. The source revision for that replacement is not +known before delivery and must not be invented now. + +**Scope question:** Do you approve this separate kit-only follow-up for the source +findings above, including its listed tests, release metadata and scoped kit records, +with a ready PR, required review and merge when clean, followed by preparation of a +revised retained-update packet, while retained fixture/source writes and all existing +fixture/client/settings/tracker exclusions remain prohibited? + +This question approves no retained execution. The original exact UPDATE-02 +[approval question](phase5-item5-b-update02-decision_2026-09-11.md#exact-decision-and-next-session) +remains preserved and unanswered. Source-scope disposition and required kit review +must precede asking it or its replacement. + +**Next session:** obtain the source-scope decision above, then follow that decision; +resolve required-review availability without bypassing the recorded restriction. +Phase 5 item 5 remains incomplete. Item 6 and its replay remain complete, #723's +approved deferral and #585's earlier placement remain, and #724 delivered the #722 +batch. The friction sweep stays parked; no exercise is repeated or re-credited. diff --git a/saved_plans/phase5-item5-b-source-review-repair-evidence_2026-09-11/author-verification.json.gz b/saved_plans/phase5-item5-b-source-review-repair-evidence_2026-09-11/author-verification.json.gz new file mode 100644 index 00000000..8f794ae9 Binary files /dev/null and b/saved_plans/phase5-item5-b-source-review-repair-evidence_2026-09-11/author-verification.json.gz differ diff --git a/saved_plans/phase5-item5-b-source-review-repair-evidence_2026-09-11/original-review.json.gz b/saved_plans/phase5-item5-b-source-review-repair-evidence_2026-09-11/original-review.json.gz new file mode 100644 index 00000000..c8735f00 Binary files /dev/null and b/saved_plans/phase5-item5-b-source-review-repair-evidence_2026-09-11/original-review.json.gz differ diff --git a/saved_plans/phase5-item5-b-source-review-repair_2026-09-11.md b/saved_plans/phase5-item5-b-source-review-repair_2026-09-11.md new file mode 100644 index 00000000..a8ad05ce --- /dev/null +++ b/saved_plans/phase5-item5-b-source-review-repair_2026-09-11.md @@ -0,0 +1,61 @@ +# ITEM5-B source-review repair + +The operator approved the [bounded kit-only scope](phase5-item5-b-source-review-repair-evidence_2026-09-11/approved-scope.md) +on 2026-09-11: “I approve that repair, its tests and release metadata, review and +merge when clean, then preparation of a revised retained-update packet”. The +[approval record](phase5-item5-b-source-review-repair-evidence_2026-09-11/approval.json) +binds the scope bytes. This authorization covers the separate source repair, +its tests/release metadata, required review and merge when clean, followed by +preparation. It grants no retained fixture/source write or retained execution. + +The [complete original review](phase5-item5-b-source-review-repair-evidence_2026-09-11/original-review.json.gz) +was preserved and read before source fixes. It retains the complete CodeRabbit +review at `cf373efd3a4d00b36987f73574d567eef004594a` and the prior source comparison. +The approved changes repair inherited upgrade and test-harness behavior; they do +not change #731's delivered history or the original UPDATE-02 payloads and ledgers. + +The source branch starts at `bc0c33a3af93d78545050612649f49fa72107a40`, the #732 +merge read back from `origin/main` with `git fetch --no-tags origin main` and +`git rev-parse origin/main` in `/Users/topi/Coding/agentic-dev-kit` on 2026-09-11. +Kit #731 remains the earlier repair at `e6d6e77d118454349f8e8bb046e99ef3009c5f5c`. +PR #733 retains the unapproved packet and its incomplete independent-review receipts. +The new source review cannot retrospectively complete those receipts. + +The implementation moves manifest/scope decisions and refresh-destination checks +before the first copy, preserves recorded template declines and partial-record +handling, anchors the initial config probe, and stops on failed required commands. +Refresh preflight rejects existing destination aliases and unexpected file kinds; +its stated operating condition forbids concurrent writes between check and copy. +The pytest leak hook retains non-OK exit statuses, and the nested helper reports +timeouts with captured output. The inherited special-file-root detector limitation +remains accepted and unextended; ownership acceptance does not verify functionality +or field exit. + +[PR #734](https://github.com/topij/agentic-dev-kit/pull/734) carries this source repair. +The [author verification record](phase5-item5-b-source-review-repair-evidence_2026-09-11/author-verification.json.gz) +retains complete argv, working directory, revision, date, status and output. +`make test` in `/Users/topi/Coding/agentic-dev-kit` at +`bcd497bf9e565b7f96ebc856d245a3d0c47b680d` on 2026-09-11 printed +`2 failed, 2528 passed, 1 skipped in 390.50s (0:06:30)`. The failures were the +known #393 deep-JSON case and the temporary changelog heading awaiting the forge's +PR identifier. After creation assigned #734, the exact changelog extraction test +named in the record printed `1 passed in 6.61s` at that revision/date/directory +with the assigned heading in the working tree. This targeted correction does not +turn the earlier full run into a passing one. The focused workflow/hook run and +individual shell parses covering #561 are retained alongside it. + +Required independent review and delivery receipts belong to PR #734. Do not infer +source coverage from retained-fixture results or incomplete reviewers. The repair +preserves #393 and #561 as separate limitations; it changes neither implementation. + +**Next:** complete PR #734's required review and pr-watch, then merge under the +operator's scoped authority. Prepare a replacement retained-update packet against +the delivered immutable source with a fresh read-only checkpoint comparison, new +hash-bound payloads/ledger/baseline prediction, preservation/verification/rollback +and exact approval question. Preserve UPDATE-01's consumed decision and historical +UPDATE-02 records. No retained update follows without its separate exact approval. + +Phase 5 item 5 remains incomplete; item 6 and replay evidence remain complete. +Do not repeat or re-credit cs-toolkit #2222/#2223/#2255. #723 remains the approved +upstream deferral; #585 remains earlier outside Phase 6; #724 delivered #722's batch. +The friction sweep stays parked pending its exact operator decision. diff --git a/scripts/conftest.py b/scripts/conftest.py index f72f3ae7..c0623376 100644 --- a/scripts/conftest.py +++ b/scripts/conftest.py @@ -375,4 +375,5 @@ def pytest_sessionfinish(session: pytest.Session, exitstatus: int) -> None: # The verdict must survive the message having nowhere to go. print(detail, file=sys.stderr) session.shouldfail = summary - session.exitstatus = pytest.ExitCode.TESTS_FAILED + if exitstatus == pytest.ExitCode.OK: + session.exitstatus = pytest.ExitCode.TESTS_FAILED diff --git a/scripts/tests/test_init_sh.py b/scripts/tests/test_init_sh.py index 20a120a9..dfa567c0 100644 --- a/scripts/tests/test_init_sh.py +++ b/scripts/tests/test_init_sh.py @@ -4794,7 +4794,7 @@ def _upgrade_init_argv() -> list[str]: assert len(lines) == 1, f"expected one init.sh invocation, found {lines!r}" argv = shlex.split(lines[0].split("#")[0]) assert argv[0].endswith("init.sh"), argv - return argv[1:] + return argv[1:argv.index("||")] if "||" in argv else argv[1:] @pytest.mark.kit_repo_only("docs/templates", "docs/agentic-dev-kit/workflows/upgrade.md") @@ -4869,51 +4869,19 @@ def test_upgrade_workflows_init_invocation_adds_the_triage_config( def _upgrade_template_copy_block() -> str: - """The Step 2 refresh block from upgrade.md, minus the two lines that need a - real kit checkout and a real installer. + """Execute the refresh block, retaining its preflight and mutation ordering. - Anchored on the `cp` of `init.sh` like `_upgrade_init_argv`, so a future code - block elsewhere in the document cannot be picked up instead. + Only the owning-directory change is omitted so the wrong-cwd regression can + check absolute destinations independently. The initializer is stubbed by the + caller; its copy and chmod now stay in the block being exercised. """ - # The `init.sh` INVOCATION is kept and stubbed by `_run_template_copy`, not - # stripped. Stripping it is how a HIGH went uncaught: the gate's refusal used - # `break`, which leaves the `for` loop while the next line runs the installer - # anyway — "Copied nothing" followed by the workflow proceeding past its own - # hard stop. No test could see it while the interaction was cut out of the - # extract (review panel, adversarial lens). - # - # `cd` still goes, and for the opposite reason: keeping it would make - # `..._writes_into_repo_even_when_the_shell_sits_in_the_kit_clone` vacuous, - # since the block would put itself in the right tree before writing. Its - # presence in the SHIPPED block is asserted separately below. - kept = [ - line - for line in _step2_refresh_block().splitlines() - # `cd "$REPO"` specifically, not any `cd `. An over-broad strip is how - # the round-2 HIGH stayed invisible: whatever the extract removes, no - # test can see. If Step 2 ever gains a second `cd`, this must fail - # loudly rather than quietly review a block that is not what ships. - if not re.match( - r'^\s*(cd "\$REPO"|cp "(\$KIT|\$\{KIT:\?[^}]*\})/init\.sh"|chmod \+x)', - line, - ) - ] - body = "\n".join(kept) - # Asserted against the SHIPPED block, not the stripped one, and that is the - # point. The `cd` has to be stripped for the extracted body to run against a - # fixture, which left it pinned by nothing: delete `cd "$REPO"` from - # upgrade.md and every test in this section still passes, while `init.sh` - # resolves the config and `docs/templates/*.tmpl` against the KIT clone — - # #399's exact failure, one line over from the one being guarded. Found by - # the review bot on PR #401. - assert re.search(r'^\s*cd "\$REPO"', _step2_refresh_block(), re.M), ( - "Step 2 no longer cds into $REPO before running init.sh — the installer " - "resolves config and templates against the working directory (#399)" - ) - assert "not_installed" in body, ( - "the Step 2 code block no longer consults `not_installed` — the gate moved " - "into prose, or was removed (#398)" + block = _step2_refresh_block() + assert re.search(r'^\s*cd "\$REPO"', block, re.M) + body = "\n".join( + line for line in block.splitlines() + if not re.match(r'^\s*cd "\$REPO"', line) ) + assert "not_installed" in body return body @@ -4922,6 +4890,7 @@ def _fake_kit_templates(tmp_path: Path) -> Path: source path is hardcoded (that hardcoding is #343, not this test's subject).""" src = tmp_path / "kit" / "docs" / "templates" src.mkdir(parents=True) + (src.parent.parent / "init.sh").write_text("#!/bin/sh\nexit 0\n") for name in ("handoff.md.tmpl", "friction-log.md.tmpl", "AGENTS.md.tmpl"): (src / name).write_text(f"# {name}\n", encoding="utf-8") return src @@ -4942,7 +4911,7 @@ def _run_template_copy( # a fixture and tell us nothing about the guard. block = _upgrade_template_copy_block() stubbed = re.sub( - r'^(\s*)"(?:\$REPO|\$\{REPO:\?[^}]*\})/init\.sh" --no-clobber\s*$', + r'^(\s*)"(?:\$REPO|\$\{REPO:\?[^}]*\})/init\.sh" --no-clobber(?: \|\| exit 1)?\s*$', r'\1: > "$REPO/INIT_SH_RAN"', block, flags=re.M, @@ -6072,3 +6041,131 @@ def test_the_permissions_advisory_says_it_is_optional(tmp_path: Path) -> None: flowed = " ".join(block.split()) assert "optional" in flowed assert "skip this entirely if you would rather approve each command" in flowed + + +@pytest.mark.kit_repo_only("docs/agentic-dev-kit/workflows/upgrade.md") +@pytest.mark.parametrize("manifest_kind", ["corrupt", "dangling", "partial"]) +def test_refresh_manifest_preflight_precedes_installer_overwrite( + tmp_path: Path, manifest_kind: str +) -> None: + repo = tmp_path / "adopter" + repo.mkdir() + src = _fake_kit_templates(tmp_path) + installer = repo / "init.sh" + installer.write_text("old installer\n") + installer.chmod(0o644) + baseline = repo / "kit-manifest.json" + if manifest_kind == "dangling": + baseline.symlink_to(repo / "absent-baseline") + else: + baseline.write_text( + '{"kit_commit":"source","files":{}}' + if manifest_kind == "partial" else "{broken" + ) + result = _run_template_copy(repo, src, check=False) + if manifest_kind == "partial": + assert result.returncode == 0, result.stderr + assert installer.read_bytes() == (src.parent.parent / "init.sh").read_bytes() + assert (repo / "INIT_SH_RAN").exists() + assert not list((repo / "docs/templates").glob("*.tmpl")) + else: + assert result.returncode != 0 + assert installer.read_text() == "old installer\n" + assert installer.stat().st_mode & 0o777 == 0o644 + assert not (repo / "docs").exists() + _assert_gate_refused(repo, result, manifest_kind) + + +@pytest.mark.kit_repo_only("docs/agentic-dev-kit/workflows/upgrade.md") +@pytest.mark.parametrize( + "relative,kind", + [ + ("init.sh", "symlink"), + ("docs", "symlink-directory"), + ("docs/templates", "symlink-directory"), + ("docs/templates/AGENTS.md.tmpl", "symlink"), + ("init.sh", "fifo"), + ("docs/templates/AGENTS.md.tmpl", "hardlink"), + ], +) +def test_refresh_refuses_destination_aliases_before_any_copy( + tmp_path: Path, relative: str, kind: str +) -> None: + repo = tmp_path / "adopter" + repo.mkdir() + src = _fake_kit_templates(tmp_path) + outside = tmp_path / "outside" + outside.mkdir() + external = outside / "sentinel" + external.write_text("preserve external bytes\n") + target = repo / relative + target.parent.mkdir(parents=True, exist_ok=True) + if kind == "symlink-directory": + target.symlink_to(outside, target_is_directory=True) + elif kind == "symlink": + target.symlink_to(external) + elif kind == "hardlink": + os.link(external, target) + else: + os.mkfifo(target) + if relative != "init.sh": + (repo / "init.sh").write_text("old installer\n") + result = _run_template_copy(repo, src, check=False) + assert result.returncode != 0 + assert "STOP" in result.stderr + assert not (repo / "INIT_SH_RAN").exists() + assert external.read_text() == "preserve external bytes\n" + assert sorted(p.name for p in outside.iterdir()) == ["sentinel"] + if relative != "init.sh": + assert (repo / "init.sh").read_text() == "old installer\n" + + +@pytest.mark.kit_repo_only("docs/agentic-dev-kit/workflows/upgrade.md") +@pytest.mark.parametrize("failure", ["cp-init", "chmod", "mkdir", "cp-template"]) +def test_refresh_stops_after_a_failed_mutation( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, failure: str +) -> None: + repo = tmp_path / "adopter" + repo.mkdir() + src = _fake_kit_templates(tmp_path) + bins = tmp_path / "bin" + bins.mkdir() + trace = tmp_path / "calls" + for command in ("cp", "chmod", "mkdir"): + shim = bins / command + shim.write_text( + '#!/bin/sh\n' + f'kind={command}\n' + 'if [ "$kind" = cp ]; then\n' + ' case "$1" in */init.sh) kind=cp-init ;; *) kind=cp-template ;; esac\n' + 'fi\n' + 'echo "$kind" >> "$REFRESH_TRACE"\n' + '[ "$kind" != "$REFRESH_FAIL" ] || exit 23\n' + f'exec /bin/{command} "$@"\n' + ) + shim.chmod(0o755) + monkeypatch.setenv("PATH", str(bins) + os.pathsep + os.environ["PATH"]) + monkeypatch.setenv("REFRESH_TRACE", str(trace)) + monkeypatch.setenv("REFRESH_FAIL", failure) + result = _run_template_copy(repo, src, check=False) + assert result.returncode != 0 + calls = trace.read_text().splitlines() + order = ["cp-init", "chmod", "mkdir", "cp-template"] + assert calls == order[:order.index(failure) + 1] + assert not (repo / "INIT_SH_RAN").exists() + + +@pytest.mark.kit_repo_only("docs/agentic-dev-kit/workflows/upgrade.md") +def test_upgrade_config_probe_resolves_the_repo_from_a_subdirectory(tmp_path: Path) -> None: + repo = tmp_path / "adopter" + (repo / "config").mkdir(parents=True) + (repo / "config/dev-model.yaml").write_text("kit: {}\n") + (repo / "docs").mkdir() + subprocess.run(["git", "init", "-q", str(repo)], check=True) + text = (REPO_ROOT / "docs/agentic-dev-kit/workflows/upgrade.md").read_text() + block = text.split("## Step 0", 1)[1].split("```bash\n", 1)[1].split("```", 1)[0] + result = subprocess.run( + ["sh", "-c", block], cwd=repo / "docs", capture_output=True, text=True + ) + assert result.returncode == 0, result.stderr + assert result.stdout == "has config\n" diff --git a/scripts/tests/test_portability.py b/scripts/tests/test_portability.py index d9fd2be0..b48f87af 100644 --- a/scripts/tests/test_portability.py +++ b/scripts/tests/test_portability.py @@ -17327,3 +17327,42 @@ def test_parallel_adapters_carry_no_approval_policy_and_the_shared_workflow_does # family; a spelled-out flag is a directive it must not carry (a mutation that # injects one fails this — panel rounds 6, 8, 11). assert "--dangerously-skip-permissions" not in shared + + +@pytest.mark.kit_repo_only("docs/agentic-dev-kit/workflows/upgrade.md") +@pytest.mark.parametrize("failure", ["doctor", "mktemp", "suite", "budget", "none"]) +def test_upgrade_verification_stops_at_the_failed_command( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, failure: str +) -> None: + repo = tmp_path / "adopter" + repo.mkdir() + bins = tmp_path / "bin" + bins.mkdir() + calls = tmp_path / "calls" + for name in ("uv", "mktemp"): + script = bins / name + script.write_text( + '#!/bin/sh\n' + f'kind={name}\n' + 'if [ "$kind" = uv ]; then\n' + ' case "$*" in *kit_doctor.py*) kind=doctor ;; ' + '*run_installed_tests.py*) kind=suite ;; *) kind=budget ;; esac\n' + 'fi\n' + 'echo "$kind" >> "$VERIFY_CALLS"\n' + '[ "$kind" != "$VERIFY_FAIL" ] || exit 17\n' + 'if [ "$kind" = mktemp ]; then echo "$VERIFY_ROOT"; fi\n' + ) + script.chmod(0o755) + monkeypatch.setenv("PATH", str(bins) + os.pathsep + os.environ["PATH"]) + monkeypatch.setenv("VERIFY_CALLS", str(calls)) + monkeypatch.setenv("VERIFY_FAIL", failure) + monkeypatch.setenv("VERIFY_ROOT", str(tmp_path / "state")) + monkeypatch.setenv("REPO", str(repo)) + workflow = (REPO_ROOT / "docs/agentic-dev-kit/workflows/upgrade.md").read_text() + block = workflow.split("## Step 5 — Verify", 1)[1].split("```bash\n", 1)[1].split("```", 1)[0] + block = block.replace("", "scripts") + result = subprocess.run(["sh", "-c", block], cwd=repo, capture_output=True, text=True) + order = ["doctor", "mktemp", "suite", "budget"] + expected = order if failure == "none" else order[:order.index(failure) + 1] + assert calls.read_text().splitlines() == expected + assert (result.returncode == 0) == (failure == "none") diff --git a/scripts/tests/test_state_guard.py b/scripts/tests/test_state_guard.py index 10ecd54e..135749d5 100644 --- a/scripts/tests/test_state_guard.py +++ b/scripts/tests/test_state_guard.py @@ -266,13 +266,19 @@ def _run_pytest( other than what ships. """ env = {k: v for k, v in os.environ.items() if not k.startswith("DEVKIT_")} - return subprocess.run( - [sys.executable, "-m", "pytest", *args, "-q", "-p", "no:cacheprovider"], - cwd=cwd or root, - env=env, - capture_output=True, - text=True, - ) + try: + return subprocess.run( + [sys.executable, "-m", "pytest", *args, "-q", "-p", "no:cacheprovider"], + cwd=cwd or root, + env=env, + capture_output=True, + text=True, + timeout=60, + ) + except subprocess.TimeoutExpired as exc: + pytest.fail( + f"nested pytest timed out: stdout={exc.stdout!r}; stderr={exc.stderr!r}" + ) def _assert_guard_fired( @@ -777,3 +783,55 @@ def test_guard_leaves_root_file_symlinks_outside_snapshot( assert target.read_text() == ("before" if case == "new-link" else "after") assert result.returncode == 0, result.stdout + result.stderr assert _BANNER not in result.stdout + result.stderr + + +@pytest.mark.parametrize("exit_kind", ["interrupt", "internal-error"]) +def test_leak_report_preserves_non_ok_session_exit_status( + tmp_path: Path, exit_kind: str +) -> None: + _build_tree(tmp_path, leak_in=None) + tests = tmp_path / "scripts/tests" + if exit_kind == "interrupt": + (tests / "test_tests_probe.py").write_text( + "from pathlib import Path\n" + "def test_interrupt():\n" + f" (Path({str(tmp_path)!r}) / 'state').write_text('leak')\n" + " raise KeyboardInterrupt\n" + ) + expected = pytest.ExitCode.INTERRUPTED + else: + (tests / "conftest.py").write_text( + "from pathlib import Path\n" + "def pytest_collection_modifyitems(session, config, items):\n" + f" (Path({str(tmp_path)!r}) / 'state').write_text('leak')\n" + " raise RuntimeError('deliberate internal error')\n" + ) + expected = pytest.ExitCode.INTERNAL_ERROR + result = _run_pytest(tmp_path, _SHAPES["tests-only"]) + assert result.returncode == expected, result.stdout + result.stderr + assert _BANNER in result.stdout + assert _SUMMARY in result.stdout + assert (tmp_path / "state").read_text() == "leak" + + +def test_nested_pytest_timeout_reports_captured_output( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + actual_run = subprocess.run + def bounded_run(*args, **kwargs): + assert kwargs["timeout"] == 60 + kwargs["timeout"] = 3 + return actual_run(*args, **kwargs) + monkeypatch.setattr(subprocess, "run", bounded_run) + _build_tree(tmp_path, leak_in=None) + (tmp_path / "scripts/tests/test_tests_probe.py").write_text( + "import sys, time\n" + "def test_wait():\n" + " print('child stdout', flush=True)\n" + " print('child stderr', file=sys.stderr, flush=True)\n" + " time.sleep(30)\n" + ) + with pytest.raises(pytest.fail.Exception, match="nested pytest timed out") as caught: + _run_pytest(tmp_path, [*_SHAPES["tests-only"], "-s"]) + assert "child stdout" in str(caught.value) + assert "child stderr" in str(caught.value)