Skip to content

#27 docs: apply first-round code review findings - #31

Merged
gregoryfoster merged 1 commit into
mainfrom
27-cr-round-1
Aug 7, 2026
Merged

#27 docs: apply first-round code review findings#31
gregoryfoster merged 1 commit into
mainfrom
27-cr-round-1

Conversation

@gregoryfoster

Copy link
Copy Markdown
Contributor

First-round CR on b8fab8d (the #27 CI fix). No bugs found; the gap was documentary.

Applied

1. docs/SKILLS.md gains a "Submodules in CI" section.
The behaviour lived only in a workflow comment and a closed issue. A codebase_context_search
for "does CI check out git submodules for skills-vendor" ranked docs/SKILLS.md (0.83) above
.github/workflows/ci.yml (0.45) — so the highest-authority answer an agent got was the doc that
lacked the fact, and specifically its section listing "shallow CI clone" among the checkouts where
the hook symlink dangles. That section now states CI is no longer one of them. Re-ran the same
query after indexing: the new content is what ranks.

2. Artifact descriptions refreshed and re-indexed.
ci-pipeline and skills-doc enumerated the workflow's other mechanisms and omitted this one.
The content chunks auto-refresh, but the authored descriptions do not — so codebase_context_index
is re-run in this change, per the AGENTS.md rule established by #19 CR #17.

4. Dropped an unverifiable clause from the ci.yml comment.
"neither nests, so true is enough" is a claim about two upstream repos, true when written and
checked by nothing thereafter. Replaced with the measured cost (~2.2s of a ~1m10s job) and the
stronger reason lint's omission is safe: ruff extend-excludes skills-vendor/ in
pyproject.toml, so it never reads what it never fetches — true by config, not merely by absence.

Stet

3. submodules: true also fetches obra-superpowers, which no test dereferences. Reviewed and
kept: actions/checkout takes no per-submodule selector, and trading the declarative key for an
imperative git submodule update --init <path> step to save ~1s is a bad exchange. Recorded in the
new doc section so the choice is visible rather than accidental.

Verification

  • uv run ruff check . — All checks passed
  • uv run ruff format --check . — 78 files already formatted
  • uv run pytest444 passed, 18 deselected, coverage 99.77%
  • codebase_context_index — 18 artifacts re-indexed; routing query re-run and confirmed

Three risks checked during review and empirically dismissed, recorded here so they are not
re-litigated: pytest collection does not widen to the 25 vendor .py files (testpaths = ["tests"];
CI collected the same 462/444 before and after); test_boundaries.py's rglob is SRC-scoped, never
REPO, so the charter AST scan cannot pick up vendor files; and a --depth=1 submodule fetch of a
SHA 40 commits behind upstream resolves fine against github.com — tested directly — so the v1.2 hold
pinning gregoryfoster-skills at 3fc7b71 will not break checkout as upstream moves on.

🤖 Generated with Claude Code

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) <noreply@anthropic.com>
@gregoryfoster
gregoryfoster merged commit 9bf5310 into main Aug 7, 2026
2 checks passed
@gregoryfoster
gregoryfoster deleted the 27-cr-round-1 branch August 7, 2026 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant