From 99db41e2485de7214975c88f0dcbac320ada92b7 Mon Sep 17 00:00:00 2001 From: Topi Jarvinen Date: Sat, 25 Jul 2026 09:14:55 +0300 Subject: [PATCH 1/2] fix: address CodeRabbit's post-merge findings on #16 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The review landed AFTER the merge — I treated a queued bot as an unavailable one and recorded the fallback receipt too early. Details in the PR body. All four findings were valid; two are real logic holes in the /upgrade skill. 1. (major) /upgrade mutated before branching. Step 2 ran init.sh — which rewrites config, installs hooks, and renders docs — while the branch was not created until Step 3, so the migration could land on whatever branch the operator was on, contradicting the skill's own "runs on a branch" guarantee. Branch creation moved ahead of the first mutation; the duplicate step in Step 3 replaced with a confirmation. 2. (major) /upgrade ran a stale migrator. It fetched the current kit and then invoked the TARGET repo's existing init.sh — the old one, without the new schema migrations — so an upgrade would report success while applying none of them. The exact silent-no-op class this stack has been removing. init.sh is now refreshed from the fetched kit before being run, with a chmod +x note since a copy can drop the bit. 3. (minor) An em dash inside a shell code block in README became an argument if copy-pasted. Replaced with a real shell comment. 4. (major) check_doc_budget.py crashed with an unhandled KeyError on a config with no doc_budgets section, instead of the intended one-line error + exit 2. CodeRabbit attributed this to the new fail-loud get(); that attribution is wrong — devmodel_config.get was equally fail-loud and main() already caught only (FileNotFoundError, ValueError), so the gap predates the kitconfig switch. Valid bug either way, now caught, with a subprocess-level regression test asserting exit 2 and no traceback. Manifest regenerated (the CI gate caught the stale hash, as designed). --- .claude/commands/upgrade.md | 31 ++++++++++++++++++++++++------- README.md | 2 +- kit-manifest.json | 2 +- scripts/check_doc_budget.py | 2 +- scripts/tests/test_kitconfig.py | 21 +++++++++++++++++++++ 5 files changed, 48 insertions(+), 10 deletions(-) diff --git a/.claude/commands/upgrade.md b/.claude/commands/upgrade.md index 7bcad13..5fb99db 100644 --- a/.claude/commands/upgrade.md +++ b/.claude/commands/upgrade.md @@ -61,13 +61,32 @@ silent failure mode: "older kit version" from "hand-edited". The report narrows it by schema version; you confirm with an actual diff in Step 3. -## Step 2 — Migrate the config (idempotent) +## Step 2 — Branch, refresh the migrator, then migrate + +**Branch first.** Everything from here mutates the repo — config, hooks, rendered +docs — so the "runs on a branch" guarantee has to be established *before* the first +mutation, not before the file copies in Step 3: + +```bash +git checkout -b chore/kit-upgrade +``` + +**Then refresh `init.sh` itself before running it.** This is the step that is easy to get +backwards: the repo's existing `init.sh` is the *old* one, and it does not contain the new +schema migrations — so running it would report success while silently applying nothing new. +Take the fetched kit's copy first: ```bash +cp /tmp/agentic-dev-kit/init.sh ./init.sh +chmod +x init.sh # the kit ships it 100755; a copy can lose the bit ./init.sh ``` -This is the supported config upgrade path, and it is safe to re-run any number of times. +(Equivalently, run `/tmp/agentic-dev-kit/init.sh` from this repo's root — it resolves +`config/dev-model.yaml` relative to the working directory, not to its own location. Copying +first is preferred, since the refreshed `init.sh` is part of what you are upgrading to.) + +`init.sh` is the supported config upgrade path, and it is safe to re-run any number of times. It only ever **adds** missing keys, never guesses over an existing value; it probes `paths.engines` from where engines actually are rather than defaulting; it stamps `kit.version`; it installs the pre-push hook as a shim (honoring `core.hooksPath`); and @@ -79,11 +98,9 @@ Press Enter through every prompt to keep current values. Then re-read the diff o ## Step 3 — Refresh engines, by state -Work through `kit_doctor`'s file list. **Branch first:** - -```bash -git checkout -b chore/kit-upgrade -``` +Work through `kit_doctor`'s file list. You are already on the branch from Step 2 — +`init.sh` refreshed itself and migrated the config there, so those changes are captured +too. Confirm with `git branch --show-current` before the first copy. - **`unchanged`** → copy the new version straight in. It is provably untouched, so there is nothing to lose. diff --git a/README.md b/README.md index 9d4228b..13b9720 100644 --- a/README.md +++ b/README.md @@ -134,7 +134,7 @@ copy-in has no version marker and no record of whether it was edited, so nothing tell an older engine from a locally-patched one: ```sh -uv run scripts/kit_doctor.py # or: python scripts/kit_doctor.py — stdlib only +uv run scripts/kit_doctor.py # or: python scripts/kit_doctor.py # stdlib only, no uv needed ``` Per kit-owned file it reports `unchanged` (safe to replace outright), `differs` (diff diff --git a/kit-manifest.json b/kit-manifest.json index 9d06c12..f5bcd79 100644 --- a/kit-manifest.json +++ b/kit-manifest.json @@ -51,7 +51,7 @@ }, "scripts/check_doc_budget.py": { "role": "engine", - "sha256": "a402424a12278e6a2ba55f90692aef9e44e9f030932529b5e85208590fc4e6dd" + "sha256": "b56e93c8f2584d3086d3773dabcdaf440028af46546412a8bf704187079f4f48" }, "scripts/dev_session.sh": { "role": "engine", diff --git a/scripts/check_doc_budget.py b/scripts/check_doc_budget.py index dfd4c77..311afaa 100755 --- a/scripts/check_doc_budget.py +++ b/scripts/check_doc_budget.py @@ -147,7 +147,7 @@ def main(argv: list[str] | None = None) -> int: try: statuses = evaluate(args.root, args.config) - except (FileNotFoundError, ValueError) as exc: + except (FileNotFoundError, KeyError, ValueError) as exc: print(f"error: {exc}", file=sys.stderr) return 2 diff --git a/scripts/tests/test_kitconfig.py b/scripts/tests/test_kitconfig.py index 4dc8c65..bb80895 100644 --- a/scripts/tests/test_kitconfig.py +++ b/scripts/tests/test_kitconfig.py @@ -225,3 +225,24 @@ def test_review_skipped_lives_only_in_unavailable_markers(): unavailable = kitconfig.get_str_list(config, "review.unavailable_markers", []) assert "review skipped" in unavailable assert not (set(noise) & set(unavailable)), "markers must not appear in both lists" + + +def test_check_doc_budget_handles_a_config_without_doc_budgets(tmp_path): + """The config reader is fail-loud by design (KeyError with no default), so + every engine that calls it must catch that too — otherwise a config missing + an optional-looking section crashes with a traceback instead of the intended + one-line error and exit 2. Caught post-merge on #16; the gap predated the + switch to kitconfig (devmodel_config.get was equally fail-loud).""" + import subprocess + + cfg = tmp_path / "dev-model.yaml" + cfg.write_text("kit:\n version: 2\npaths:\n handoff: docs/handoff.md\n", encoding="utf-8") + result = subprocess.run( # noqa: S603 + [sys.executable, str(REPO_ROOT / "scripts" / "check_doc_budget.py"), "--config", str(cfg)], + capture_output=True, + text=True, + check=False, + ) + assert result.returncode == 2, result.stderr + assert "Traceback" not in result.stderr, result.stderr + assert "doc_budgets" in result.stderr From 0c0fb70f9aec0b9f2709eb4127b5ff2dcc23c6f8 Mon Sep 17 00:00:00 2001 From: Topi Jarvinen Date: Sat, 25 Jul 2026 09:17:46 +0300 Subject: [PATCH 2/2] fix(upgrade): copy templates before running init.sh MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Caught by the fallback review pass on this PR. init.sh resolves docs/templates/*.tmpl relative to the WORKING DIRECTORY, so Step 2 ran the refreshed migrator before Step 4 had copied the templates in — it prints 'note: template … missing — skipped' and seeds nothing. Harmless noise for a repo whose narrative docs are already in use (they would be left untouched regardless), but a PARTIALLY-adopted repo missing one of the four docs would silently not get it seeded — and partial adoptions are the common shape, not the exception. Verified both directions: templates absent -> four 'skipped' notes and no seeding; templates present -> seeded. Also corrected the claim that running the fetched kit's init.sh in place is 'equivalent' — every path it reads resolves against the working directory, so it needs the templates here either way. --- .claude/commands/upgrade.md | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/.claude/commands/upgrade.md b/.claude/commands/upgrade.md index 5fb99db..c6e954e 100644 --- a/.claude/commands/upgrade.md +++ b/.claude/commands/upgrade.md @@ -78,13 +78,21 @@ Take the fetched kit's copy first: ```bash cp /tmp/agentic-dev-kit/init.sh ./init.sh -chmod +x init.sh # the kit ships it 100755; a copy can lose the bit +chmod +x init.sh # the kit ships it 100755; a copy can lose the bit +mkdir -p docs/templates && cp /tmp/agentic-dev-kit/docs/templates/*.tmpl docs/templates/ ./init.sh ``` -(Equivalently, run `/tmp/agentic-dev-kit/init.sh` from this repo's root — it resolves -`config/dev-model.yaml` relative to the working directory, not to its own location. Copying -first is preferred, since the refreshed `init.sh` is part of what you are upgrading to.) +The templates have to land **before** `init.sh` runs, not with the other file copies in Step +4: `init.sh` resolves `docs/templates/*.tmpl` relative to the working directory, so without +them it prints `note: template … missing — skipped` and seeds nothing. For a repo whose +narrative docs are already in use that is merely noise (they would have been left untouched +anyway), but a **partially-adopted** repo missing one of the four docs would silently not get +it seeded. + +Note this is also why running `/tmp/agentic-dev-kit/init.sh` in place of the copy is *not* +equivalent: every path it reads — the config, the templates — resolves against the working +directory, not against its own location, so it still needs the templates present here. `init.sh` is the supported config upgrade path, and it is safe to re-run any number of times. It only ever **adds** missing keys, never guesses over an existing value; it probes