#27 docs: apply first-round code review findings

.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

.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",

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 <path>`
+  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.