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.