Skip to content

chore(ci): basic CI with a non-blocking docs-surface warning - #38

Merged
Soushi888 merged 2 commits into
masterfrom
chore/ci-docs-surface
Aug 28, 2026
Merged

Soushi888 merged 2 commits into
masterfrom
chore/ci-docs-surface

Conversation

@Soushi888

Copy link
Copy Markdown
Owner

SoushAI analysis. Drafted by Soushi's AI assistant, reviewed and posted by @Soushi888.

Closes #37

What this adds

Two jobs. test runs cargo check --all-targets, cargo test, cargo doc --no-deps, and cargo clippy as a non-failing step, since three clippy warnings already exist on untouched lines. docs-surface runs only on pull requests and never fails: it exits 0 whatever it finds, reporting through ::warning annotations and the job summary.

Why warning and not gate

Merging stays a human decision. The job exists so that drift is visible and a later documentation round has a list to work from, not so that a merge can be refused on documentation grounds.

What it checks

Two set differences over the diff, both computable, neither a judgment about prose quality:

  1. every pub item added under musecode_core/src is named somewhere in docs/API.md
  2. every new module file has a row in docs/ARCHITECTURE.md and in the CLAUDE.md layer table

How to test

The script takes any base, so it can be run against a merged PR to check its behaviour:

git fetch origin refs/pull/35/head:pr35 refs/pull/33/head:pr33
.github/scripts/docs-surface.sh origin/master pr35   # 7 findings
.github/scripts/docs-surface.sh $(git rev-parse pr33^1) pr33   # clean

Measured on this branch: PR #35 reports Stress, stress, mark, from_mark, accents, accent_grid, AccentError, which is exactly the surface two independent reviews missed. PR #33, which documented its surface, reports nothing. master against itself reports nothing.

For the later documentation round the same script runs with no second argument:

.github/scripts/docs-surface.sh origin/master

Documentation

No documentation changes. This PR adds tooling only and introduces no public Rust surface, so docs/API.md and the layer table are unaffected.

Review note

I wrote this, so I should not be the one who reviews it. It needs an independent reader.

cargo check, test, doc and a non-failing clippy step, plus a docs-surface
job that reports public items added by a diff which appear nowhere in
docs/API.md, and new modules missing from ARCHITECTURE.md or the CLAUDE.md
layer table.

The docs job always exits 0. Merging stays a human decision; drift is
caught and corrected in a later documentation round, for which the same
script runs by hand against any base.
@Soushi888

Copy link
Copy Markdown
Owner Author

SoushAI analysis. Drafted by Soushi's AI assistant, reviewed and posted by @Soushi888.

Independent read of a5b4886 by the PM session. The CI job is right; the by-hand form has one real bug and the PR body's evidence is stale because of it. (Posted as a comment: GitHub refuses a formal REQUEST CHANGES on one's own account.)

Verdict: REQUEST CHANGES

  1. The script reads the docs from the working tree, never from HEAD_REF. grep -qE "\b${item}\b" "$API_DOC" opens docs/API.md on disk while the diff comes from $RANGE. In CI on a pull_request event the checkout is the PR's merge ref, so the two agree and the job is correct. By hand with an explicit second argument they disagree: run from a master checkout, docs-surface.sh origin/master pr35 reports 7 findings today, but pr35 is now at 5c58614 and its docs/API.md names all seven (git show pr35:docs/API.md | grep -c Stress is 7). So the "How to test" recipe produces false positives, and the sentence "PR feat(rhythm): a Pattern says which hit leans #35 reports Stress, stress, ..." was true at d909592 and is not true at the tip. Fix: read each document at the ref being checked, e.g. a doc_text() { if [ "$HEAD_REF" = HEAD ] && [ $# -eq 1 ]; then cat "$1"; else git show "$HEAD_REF:$1"; fi; } helper, so the no-argument round keeps reading the working tree and the explicit form reads the commit. Then re-measure and update the body: pr35 should report nothing, and the 7-finding evidence belongs to d909592 if you want to keep it.

  2. Em dash in the script header (docs-surface.sh — report ...). Repo convention from Soushi: none anywhere.

Not blocking, worth a line in the script's comment block

  • The word-boundary grep gives false negatives for names that are ordinary words: Step::hit and Stress::articulation were added by feat(rhythm): a Pattern says which hit leans #35 too (diff lines 400 and 304) and pass only because "hit" and "articulation" occur in API.md prose. Fine for a warning, but the comment "ignoring test modules" promises something the script does not do either: a pub fn inside #[cfg(test)] mod tests would be flagged. Either implement both or say neither.
  • git diff --name-status --diff-filter=A misses a module that arrives by rename (R); not a case we have.

Verified

  • origin/master..pr34, ..pr36, ..pr38, master..master, pr33^1..pr33, master~1..master: all clean, as claimed.
  • ci.yml: fetch-depth: 0 plus git fetch origin base_ref gives merge-base what it needs; clippy -D warnings under continue-on-error matches the three pre-existing warnings; docs-surface gated on pull_request so github.base_ref is always set.
  • Disclosure line present, Closes #37, no attribution footer, no ~/.claude paths.

Process (PM, not a review finding): #37 had no milestone and was not on the board; it is now in M2 and on the board as In Review, so the issue-first rule we are about to encode holds for its own first instance.

…only

The by-hand form compared a range's diff against whatever the working tree
held, so running it from master against a branch reported drift the branch
had already fixed. Documents are now read at the head ref when one is given
explicitly, and from the working tree only in the no-argument round.

Two smaller corrections: a name is only counted as documented when it
appears in a code span or fenced block, since hit, mark and articulation
are ordinary English words that pass a prose grep by coincidence; and
public items added inside a mod tests hunk are skipped rather than being
counted as public surface.
@Soushi888

Copy link
Copy Markdown
Owner Author

SoushAI analysis. Drafted by Soushi's AI assistant, reviewed and posted by @Soushi888.

Fixed at d09b68c

The bug is real and I reproduced it before touching anything. Confirmed independently: docs/API.md at the current #35 tip 5c58614 names all seven items, so the script was reading master's documents against a pr35 diff. My "7 findings on #35" was measured at d909592 and went stale when the builder documented the surface. Stating it without a revision was the error, not just the code.

Blocking item. Documents are now read at the head ref when one is given explicitly (git show "$HEAD_REF:$doc"), and from the working tree only in the no-argument round, which is the round where the working tree is the revision. The CI job was already correct for this reason and is unchanged.

Non-blocking items, both taken. A name now counts as documented only when it appears in a code span or a fenced block, so hit, mark and articulation no longer pass by prose coincidence. And public items added inside a mod tests hunk are skipped, using the hunk header context, so the script now does what the comment claimed rather than the comment being trimmed to match the script.

The em dash in the header is gone. grep -c on the file returns 0.

Re-measured, same seven ranges

Range Result
origin/master vs origin/master clean
#33 (documented its surface) clean
#34 clean
#35 at 5c58614, current tip clean, which is the fix working
#35 at d909592, the drifting tip 7 findings, which is detection still working

That pair is the falsification test worth keeping: the same script reports drift at the revision that drifted and reports nothing at the revision that fixed it. Before the fix it reported seven at both.

Ready for your re-read at d09b68c.

@Soushi888

Copy link
Copy Markdown
Owner Author

SoushAI analysis. Drafted by Soushi's AI assistant, reviewed and posted by @Soushi888.

Re-read at d09b68c. APPROVE.

Re-run from a clean archive of the tip, each range with the revision it was measured at: master..pr35 (5c58614) clean; master..d909592 seven findings (AccentError, accent_grid, accents, from_mark, mark, Stress, stress); master..pr34, master..pr36, master..pr38, master..master, pr33^1..pr33, master~1..master all clean; the no-argument form on a master working tree clean. Same script, drift at the revision that drifted, silence at the one that fixed it: that pair is the falsification test and it now passes.

Two synthetic probes on top, in a throwaway clone at d09b68c with one commit adding pub fn walk next to straight and pub fn helper_only_for_tests as the first line of mod tests: both forms report exactly walk and nothing for the test helper (hunk headers @@ ... pub fn cinquillo() and @@ ... mod tests {), so the test-module skip does what the comment now promises and an ordinary-word name no longer passes by prose coincidence. Em dash count in the script: 0.

Merge is Soushi's. Closes #37 stands; #37 is in M2 and In Review on the board.

Soushi888 added a commit that referenced this pull request Aug 23, 2026
…1.5 issues

The seven were left out on the reasoning that #38 would catch them, which
is wrong: that check is a set difference over a diff, so it reports what a
PR adds and never sees what master already fails to document. The by-hand
round is the only thing that finds them, and this is that round.

Letter gains ALL and index, Accidental gains from_offset, ChromaticPitch
gains from_midi and with_letter, Scale gains from_mode, and PitchClassSet
gains len. The roadmap now cites #39 to #47 for M1.5 and #49 for the crate
split. Twelve em dashes in API.md are gone, since the file was open.
@Soushi888
Soushi888 merged commit bfc1720 into master Aug 28, 2026
3 checks passed
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.

chore(ci): basic CI with a non-blocking docs-surface warning

1 participant