spec-kitty / spec-kitty/spec-kitty

[squad] MINOR findings on PR #4473 (docs(release): describe tokenless dependency flow)

Open
#4,542 0 comments 0 reactions 0 assignees View on GitHub
from:squad priority:P3 status:triage triage:needs-revision type:finding
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.