spec-kitty / spec-kitty/spec-kitty
[squad] MINOR findings on PR #4473 (docs(release): describe tokenless dependency flow)
- Dominant language
- Python
- Stars
- 1.6k
- Forks
- 165
- Avg merge
- 14h 52m
- Merged PRs (30d)
- 303
Description
## MINOR findings from squad review (pass 2) of spec-kitty/spec-kitty#4473
PR: https://github.com/spec-kitty/spec-kitty/pull/4473
Verdict was `[squad] passed @db6d396e` — no MAJOR findings. Both items below are evidence/coverage hygiene rather than defects in the change itself, consolidated here per planning#1313.
Scope note, stated plainly: item 1 concerns the PR body's own test evidence and is only actionable while that PR is open. Item 2 references code that **already exists on `main`** — it is a pre-existing guard gap that PR #4473 neither introduces nor deepens — so unlike the usual squad-findings caveat, it does not wait on that PR to merge.
- [ ] `PR body:## Tests run` — the first command's claimed count is stale at the reviewed head: the body says **50 passed**, a live re-run of `.venv/bin/python -m pytest tests/release/test_release_ci_ownership.py -q` at `db6d396e` gives **52 passed**. The cause is established rather than guessed: the maintainer rebase (#4535 landing pass) added 29 lines and exactly two tests to this same file from `main` — `test_release_checklist_opens_the_next_cycle_right_after_the_tag` and `test_contributing_release_process_mirrors_the_post_tag_cycle_step` — so 50 + 2 = 52, and the count was honest at the pre-rebase head `de2b2388`. No failure is hidden and nothing is uncollected, so this is not the planning#942 overclaim shape. — Refresh the count to 52 so PROGRAM.md §5's "re-run evidence, not a one-time snapshot" holds at the head being judged.
- [ ] `tests/release/test_release_ci_ownership.py:539` — the invariant the docstring now documents is unguarded: **nothing asserts `release.yml` is tokenless.** The token half of `test_introduced_and_restore_rows_forbid_private_runner_and_token` loops only over `introduced_workflow_names()` (line 557), and `release.yml` is a member of `RESTORED_WORKFLOWS` (line 35), not `INTRODUCED_WORKFLOWS`; the parametrized guard at line 376 covers only `ci-quality.yml` and `ci-windows.yml`. Re-adding `SK_CI_TOKEN` to `release.yml` today would red no test. — Deferred under PROGRAM.md §7(b) (needs its own mission/design pass) rather than folded into #4473: this function's stated invariant is explicitly "no `SK_CI_TOKEN` on *public jobs*" and `release.yml` is, by the same docstring's words, not a public CI job, so adding the assertion inside this function would contradict the function's own contract. The fix is a separate release-scoped guard, and choosing its placement is a real decision, not a one-line fold.
### Why item 2 is worth keeping on the record
Squad pass 1's verdict on #4473 stated that "the test at line 347-348 (`assert "SK_CI_TOKEN" not in text` over the public CI workflows) already enforces the behavior the docstring now describes." Pass 2 refuted that by execution — the assertion is parametrized over `ci-quality.yml`/`ci-windows.yml` only, and `release.yml` belongs to the `restore` disposition. Left uncorrected, the record would tell a future reader that re-adding the token to `release.yml` reds a test. It does not.
Contributor guide
Research direction
Start by rerunning `.venv/bin/python -m pytest tests/release/test_release_ci_ownership.py -q` and inspect `tests/release/test_release_ci_ownership.py`, especially the guards around lines 376, 539, and 557. Check `release.yml`, `RESTORED_WORKFLOWS`, and PR #4473's test evidence. Done means the PR body reports 52 passed and the separate release-scoped token guard has an agreed placement and coverage.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- github-actions, python
- Domain
- ci-cd, documentation, release, testing
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 50/100