From b483f779c51d45c1df4d5f08814cedd298ebf06f Mon Sep 17 00:00:00 2001 From: gregoryfoster Date: Fri, 7 Aug 2026 19:56:20 +0000 Subject: [PATCH] #27 docs: apply first-round code review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CR items 1, 2, 4. Item 3 (submodules: true also fetching obra-superpowers, which no test dereferences) reviewed and stet — actions/checkout takes no per-submodule selector and the whole fetch is ~2.2s. 1. docs/SKILLS.md gains a "Submodules in CI" section. The behaviour lived only in a workflow comment and a closed issue, and a context search for "does CI check out git submodules for skills-vendor" ranked SKILLS.md above ci.yml — so the highest-authority answer was the doc that lacked the fact, in a section listing "shallow CI clone" among the checkouts where the symlink dangles. That section now says CI is no longer one of them. 2. The ci-pipeline and skills-doc artifact descriptions enumerated the workflow's other mechanisms and omitted this one. Content chunks auto-refresh; the authored descriptions do not, so codebase_context_index is re-run in this change per the AGENTS.md rule (#19 CR #17). 4. Drops the ci.yml comment's "neither nests, so `true` is enough" clause — a claim about two upstream repos, true when written and checked by nothing after. Replaced with the measured cost and the reason lint's omission is safe from config rather than from absence: ruff extend-excludes skills-vendor/, so it never reads what it never fetches. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/ci.yml | 12 +++++----- .socraticodecontextartifacts.json | 4 ++-- docs/SKILLS.md | 37 +++++++++++++++++++++++++++++++ 3 files changed, 45 insertions(+), 8 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 70b00f8..d167f12 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -85,12 +85,12 @@ jobs: # `submodules` is load-bearing for tests/test_skills_hook.py, which # dereferences the .claude/hooks/ symlink into skills-vendor/ — an # unpopulated submodule makes that link dangle and the test cannot pass. - # Both submodules are public HTTPS in .gitmodules, so no token is needed, - # and neither nests, so `true` is enough (`recursive` would only add a - # foreach pass). The coupling this buys: an upstream force-push that GCs a - # pinned SHA fails the job *at checkout*, which looks nothing like a test - # failure. The lint job deliberately omits this — ruff never reads - # skills-vendor/. + # Both submodules are public HTTPS in .gitmodules, so no token is needed. + # The coupling this buys: an upstream force-push that GCs a pinned SHA + # fails the job *at checkout*, which looks nothing like a test failure. + # The lint job deliberately omits this — ruff extend-excludes + # skills-vendor/ (pyproject.toml), so it never reads what it does not + # fetch. Costs ~2.2s of a ~1m10s job. See docs/SKILLS.md. - uses: actions/checkout@v5 with: submodules: true diff --git a/.socraticodecontextartifacts.json b/.socraticodecontextartifacts.json index f1cf795..caf9850 100644 --- a/.socraticodecontextartifacts.json +++ b/.socraticodecontextartifacts.json @@ -63,7 +63,7 @@ { "name": "skills-doc", "path": "./docs/SKILLS.md", - "description": "Agent skills index. The two-level symlink chain (.claude/skills -> skills/ -> skills-vendor/) and why it lets a local override shadow a vendor skill in both discovery systems, the vendor submodules, the local brainstorming override and its required override frontmatter, and the once-per-day SessionStart submodule refresh hook." + "description": "Agent skills index. The two-level symlink chain (.claude/skills -> skills/ -> skills-vendor/) and why it lets a local override shadow a vendor skill in both discovery systems, the vendor submodules, the local brainstorming override and its required override frontmatter, the once-per-day SessionStart submodule refresh hook and its suspension for the v1.2 curating-context hold, the write-guard hook that dangles on a submodule-less checkout, and how CI handles submodules — the test job checks out skills-vendor/ because test_skills_hook.py dereferences the hook symlink while lint does not need it, with the cost, the pinned-SHA semantics against the v1.2 hold, and the checkout-time failure mode accepted in exchange (#27)." }, { "name": "deployment-topology", @@ -83,7 +83,7 @@ { "name": "ci-pipeline", "path": "./.github/workflows/ci.yml", - "description": "GitHub Actions CI. Keyless Workload Identity Federation auth to GCP for the wheelhouse mirror, a fail-fast guard asserting the GCP_WIF_PROVIDER org variable is visible to this repo, ruff check and format gates, a co-core extras import smoke proving [extract] and [bus] wire up in a clean environment, and pytest with the coverage gate. Deliberately no Postgres service and no alembic steps — Replicator is DB-free." + "description": "GitHub Actions CI. Keyless Workload Identity Federation auth to GCP for the wheelhouse mirror, a fail-fast guard asserting the GCP_WIF_PROVIDER org variable is visible to this repo, ruff check and format gates, a co-core extras import smoke proving [extract] and [bus] wire up in a clean environment, and pytest with the coverage gate. Deliberately no Postgres service and no alembic steps — Replicator is DB-free. The test job's checkout carries `submodules: true` and the lint job deliberately does not (#27): tests/test_skills_hook.py dereferences the .claude/hooks/ symlink into skills-vendor/, and actions/checkout fetches no submodules by default, so without the key that test cannot pass in CI at all — it failed identically on every commit and main was red from 7bf5988 until the fix. Ruff extend-excludes skills-vendor/, so lint never reads what it never fetches. Costs ~2.2s of a ~1m10s job; checkout resolves the SHA pinned in this superproject rather than upstream tip, so it is independent of the v1.2 skills hold. Reasoning in docs/SKILLS.md." }, { "name": "dependency-contract", diff --git a/docs/SKILLS.md b/docs/SKILLS.md index eb1b87d..f07f3f8 100644 --- a/docs/SKILLS.md +++ b/docs/SKILLS.md @@ -104,6 +104,9 @@ hook as a side effect, even though it never inspects it. Tracked upstream as [gregoryfoster/skills#99](https://github.com/gregoryfoster/skills/issues/99); the guard itself is correctly non-blocking once it resolves. +CI is **no longer** one of those submodule-less checkouts — see [Submodules in CI](#submodules-in-ci) +below. A `git worktree add` still is. + ### From `obra-superpowers` | Skill | Purpose | @@ -151,3 +154,37 @@ makes upstream fixes to the script arrive on the normal submodule refresh; a cop version was current the day it was installed and drifts silently thereafter — this repo's had, for the whole `.skills/doctor.sh` commit path (#16). `readlink .claude/hooks/skills-submodule-update.sh` is the check; an empty result means someone re-copied it. + +## Submodules in CI + +`tests/test_skills_hook.py` pins that symlink, and its second assertion **dereferences** it — a +correctly-shaped link into an unpopulated submodule is a dangling no-op, which is the failure #16 +existed to catch. `actions/checkout` does not fetch submodules by default, so the `test` job carries +the key that makes the test runnable at all: + +```yaml +- uses: actions/checkout@v5 + with: + submodules: true +``` + +Without it the link dangles on the runner and the test **cannot pass in CI** — it fails identically +on every commit, which is indistinguishable from a check that just started failing and trains +everyone to merge past red. That is what happened: `main` was red from `7bf5988` until #27. + +Three things to know before touching it: + +- **`lint` deliberately omits the key.** Ruff `extend-exclude`s `skills-vendor/` (`pyproject.toml`), + so the linter never reads what the job never fetches. Both halves are load-bearing; either alone + would do. +- **It costs ~2.2s** of a ~1m10s job, for both submodules. `submodules: true` fetches + `obra-superpowers` too, which no test dereferences — `actions/checkout` takes no per-submodule + selector, and trading the declarative key for an imperative `git submodule update --init ` + step to save about a second is not worth it. +- **CI resolves the SHA pinned here, never upstream tip.** So the key neither lifts nor weakens the + v1.2 hold above. A pin far behind upstream is fine: GitHub serves arbitrary SHAs, so the shallow + submodule fetch resolves it. + +The coupling accepted in exchange: an upstream force-push that garbage-collects a pinned SHA fails +the job **at checkout**, an error that looks nothing like a test failure. If CI dies before the +`Install uv` step, read the checkout log before the test output.