fix(doctor): cover triage's homing prerequisites, and tell a failed read from an absent one (#4300) - #4337
Conversation
No preview deploy
|
…ead from an absent one (#4300)
c7220a7 to
f176c61
Compare
|
review-skill: advisory — blocking-set PR (manual merge) PR #4337 touches the control plane — Reviewed-head: @ f176c61 Verified against #4300's acceptance criteria + the skill-rigor checklist — all checks pass. Acceptance criteria
Skill rigor
Rebase verification — the three conflict hunks The rebase is genuine and linear: the head's sole parent is
The falsified claim, and the stale count
Claimed verification, spot-checked at this head Re-ran rather than assumed:
One non-blocking observation (out of scope for #4300 — the PR is not blocked by it)
All six acceptance criteria and all five rigor checks pass. This advisory authorizes no merge. Verdict-written: 2026-07-27T01:37:48Z |
|
review-code: advisory — blocking-set PR (manual merge) PR #4337 is control-plane: Reviewed-head: @ f176c61 This is the code half of a mixed code + skill PR; the skill half carries its own
Acceptance criteria (the code half's contribution)
Code hygiene / correctness
All checks pass on the code half. This advisory authorizes no merge. As a §CP PR it needs a Verdict-written: 2026-07-27T01:38:41Z |
Handoff — PASSED and banked, awaiting a control-plane approvalEngine instance State: all 6 ACs and all 5 rigor checks PASS at head Next step, and the only one: a Do not rebase or push to this branch. Any head movement unbinds both verdicts (ADR 0058) and would cost a full re-review. Three human approvals were destroyed that way earlier today. Why the rebase was safe, for whoever needs to trust itThis PR was deliberately held behind #4346 (both touch The hunk worth knowing about: #4346 added an "unreadable state is UNKNOWN, never the negative answer" bullet while this PR replaced an adjacent one. A mechanical resolution would have silently dropped one. Both were kept, deliberately uncollapsed — the body paragraph is the adopter-facing reporting model, the Conventions bullet is maintainer-facing mechanism with its own citations. Collapsing loses one audience. Base defect reproduced, not argued: against a One non-blocking observation, deliberately NOT fixed here
Marker hygiene: do not hand-edit, PATCH, or delete any verdict marker here. |
Fixes #4300
doctorgreened a repo wheretriagehad no non-kill home for any issue. Two causes, and the second is the one worth reading: the required-label list was narrower than what the guards consume, and the check that read it could not tell a failed read from a genuine absence — so a checker passed while the thing it checks was unknown.What changed
1.
doctor.shcheck1c— the label table is now a presentation mirror, not a second source.The required set is single-sourced in
packages/pipeline-cli/src/tools/vocabulary-preflight/vocabulary.ts(#4272, landed), which assembles it from the constants the guards themselves scope on.doctor.sh's table only adds the colour + description agh label createneeds. Rows gained aTIERcolumn: tier 1 fails the run, tier 3 only ever WARNs.Tier-1 rows added:
wayfinder:backlog,axis:pipeline-hardening(the two labels that make triage's standing-lane exemption reachable) andarea:infra(the platform discriminatorlanescopes on).wayfinder:mapis a tier-3 row — it is the wayfinder chart front door, an ideation surface, not a first-run prerequisite.2. New check
1d— the mirror cannot silently fall behind the source.doctor.shreads the source through a newpipeline-cli vocabulary-preflight labelsverb (the only seam a bash preflight has onto a TS constant) and reds if its table no longer covers it. This is the check that would have caught the original bug: withwayfinder:backlogremoved from the table,1cstill reports✓ all 17 required pipeline labels existand1dreports✗ … DRIFTED … absent from the table above: wayfinder:backlog.3. Every check reports THREE states, never two.
This is the point of the fix, not a side effect.
1cpreviously capturedgh api … 2>/dev/nullinto a string and branched on emptiness, so a 403 and a genuinely label-less repo were the same value — and the message picked one ("could not read repo labels") for both. Now the read's outcome is captured separately from its content:1clabels1dvocabulary source3dopen milestone✓ all N required pipeline labels exist✓ … covers all N labels of the shared vocabulary✓ N open milestone(s)✗ N of M required … absent+ create commands✗ … has DRIFTED … absent from the table above: …⚠ no open milestone — triage can exempt … but cannot home one into an arc✗ … UNDETERMINED — the label read on <repo> FAILED (unknown, NOT absent)⚠ … UNVERIFIED against the shared vocabulary (unknown, NOT confirmed)⚠ … UNDETERMINED — the read did not succeed (unknown, NOT 'no milestone')A failed read never resolves to "fine", and never scores absence off a value it never obtained.
4. Open-milestone existence is a Tier-3 WARN (
3d), never a Tier-1 fail.Triage homes an issue into an existing open milestone and is forbidden from creating one (ADR 0072 §3), so a freshly-adopted repo legitimately has none — reddening on that would fail the correct state of a brand-new adopter. The WARN names the consequence: triage can exempt to a standing lane but cannot home into an arc/campaign.
5. The hardcoded
15is gone — the count is derived from the table (all 18 required pipeline labels existtoday).The label auto-create question, settled by observation
#4268 flagged as explicitly unverified whether
POST /repos/{owner}/{repo}/issues/{n}/labelsauto-creates a missing label.doctor.sh's header rested on the assertion that it does not. It does. Measured 2026-07-26 against this repo: POSTing a label name that returned 404 fromGET /labels/{name}succeeded with HTTP 200 and materialized the label repo-wide at GitHub's default greyedededwith a null description. (The probe label was removed from the issue and deleted repo-wide;GETre-confirms 404.)Corroborating trace:
p3,area:infra, andaxis:a11yin this repo all carry exactly that signature —color=ededed,description=null— the fingerprint of labels minted by a write rather than created deliberately.So the stated rationale for Tier 1 was false, and the header now says the true thing:
reportsilently succeeds and mints an off-taxonomy label, and the damage lands on the read side, where?labels=status:triagedand every guard's label scoping match nothing and the pipeline runs protecting nothing. That is a stronger reason for Tier 1 than the one it replaces, so the check keeps its tier — the design is safe under either answer, and the answer is now recorded rather than assumed.Verification
Four states exercised by running the script, not by argument:
✓ all 18 required pipeline labels exist,✓ … covers all 16 labels of the shared vocabulary,✓ 14 open milestone(s).✗ 2 of 18 required pipeline label(s) absentnamingwayfinder:backlogandaxis:pipeline-hardeningeach with its create command, plus thewayfinder:mapand no-milestone WARNs.✗ required labels UNDETERMINED … (unknown, NOT absent), exit 1, and zero labels reported absent.⚠ … UNVERIFIED … (unknown, NOT confirmed), every other check unaffected.1cstill greens and1dreds by name.Checks:
pnpm typecheckclean;pnpm lint:worktreeclean (3 files);pipeline-clisuite 177 files / 2794 tests passing, includingcore-import-closure.unit.test.ts(ADR 0218 — the new subcommand is reached only throughregistry.ts's pruned fan-out edge and adds no core import);gh-phoenix lint-skillsclean on both edited skill files;shellcheckclean on the new code (one pre-existing SC2043 on an untouched line); script re-run under/bin/bash3.2 (the macOS default) since the heredoc parsing note in the file depends on it.Deviations / scope left out
REQUIRED_LABELS(16) and doctor's tier-1 set (18) are not equal in either direction: doctor additionally requiresstatus:planningandstatus:awaiting-release, which no guard scopes on butplan-epicandship-itwrite. Rather than edit Pipeline label vocabulary + roadmap table + §CP path are hardcoded with no config seam #4272's landed set,1dasserts only the direction that matters — doctor's set must cover the shared one. Doctor being a deliberate superset is recorded in the file.1ddegrades to a WARN when the CLI cannot be resolved, rather than failing Tier 1. Doctor's own Tier 3 treats@kampus/pipeline-cliresolving as optional, so making Tier 1 hard-depend on it would contradict the tier design for the exact fresh-repo adopter doctor serves. The honest report is "unverified", not a silent pass and not a false red.allow_auto_merge+ merge-queue ruleset) is untouched — same file, different proposition and API surface, independently pickable per triage note 4.doctor.sh/doctor/SKILL.md. This PR still authors none of ship-it's enqueue needs repo auto-merge + a queue ruleset no fresh repo has #4303's checks — feat(doctor): preflight the two merge-queue governance preconditions ship-it needs (#4303) #4346's2c/2dblocks are byte-identical here (72 lines, verified by diff againstorigin/main).claude-plugins/kampus-pipeline/README.mdis now in scope — one file more than the pre-rebase head. feat(doctor): preflight the two merge-queue governance preconditions ship-it needs (#4303) #4346 left the README's twothe 15 required labelsphrasings standing; this PR is what widens the required set to 18, so that prose went stale as a direct consequence of this diff and is corrected here rather than deferred. Both sites are now count-free (a hardcoded count is the thing that goes stale;doctor.shderives the number at runtime). The same edit also drops the README's copy of the falsified "a first run that fails deep inside agh apicall" claim, which this PR measured and removed fromdoctor.shanddoctor/SKILL.md— leaving the third copy would have shipped the PR contradicting itself.doctor/SKILL.mdwere resolved semantically, keeping both sides: (a) the "load-bearing pair" paragraph — this PR's corrected wording (which deletes the falsified round-trips-into-a-raw-API-error clause) is kept plus feat(doctor): preflight the two merge-queue governance preconditions ship-it needs (#4303) #4346's repo-admin-governance-pair paragraph, and feat(doctor): preflight the two merge-queue governance preconditions ship-it needs (#4303) #4346'sTier-2 pair→Tier-2 checksrewording is kept because Tier 2 now holds four checks; (b) the Conventions bullet — feat(doctor): preflight the two merge-queue governance preconditions ship-it needs (#4303) #4346's newAn unreadable state is UNKNOWNbullet is kept alongside this PR'sLABELS table is a presentation mirrorbullet, which replaced the older canonical-set bullet the two sides fought over.Rebase (2026-07-26)
Rebased
c7220a75→f176c618ontoorigin/mainatce581d3c, which now contains #4346 (merged 23:40:56Z). Three conflict hunks, all in the two shareddoctor/files; the threevocabulary-preflightfiles applied clean. Re-verified against the rebased head, not inherited from the pre-rebase run:pnpm typecheckclean,pnpm lintclean,@kampus/pipeline-cli177 files / 2805 tests passing,doctor.shrun live (exit 0, all four of this PR's checks and both of #4346's green), and the stubbed-403 label read re-reproduced —✗ required labels UNDETERMINED … (unknown, NOT absent), exit 1, zero labels reported absent.Per ADR 0058 the head moved, so this PR carries no bound verdict — it was deliberately rebased before review so the review binds the head that ships.
§CP
This PR is control-plane:
claude-plugins/kampus-pipeline/skills/doctor/doctor.shmatches the^claude-plugins/kampus-pipeline/skills/([^/]+/)*[^/]+\.sh$clause ofCONTROL_PLANE_REas re-resolved fromorigin/main. It is human-merged and takes areview-skillverdict. Thepackages/pipeline-cli/src/tools/vocabulary-preflight/**files are not §CP on their own; the.shis what carries it.