docs(contracts): tighten C3 gate's minLength claim — schema-only, not asserted #589
No reviewers
Labels
No labels
bump
major
bump
minor
bump
patch
kind/bug
kind/chore
kind/docs
kind/feature
priority/critical
priority/high
priority/low
priority/medium
size/L
size/M
size/S
size/XL
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
frankenbit/release-toolkit!589
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "i/587-minlength-doc-precision"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
What
Phase 7 (#80) tracker #587 — the doc-precision follow-up to #583/PR#586, first Wednesday+ tracker. Surveyor's should-consider on review 4642 (option (a)): PR#586's reconciled
workflow-api.mdand the drift-gate SCOPE comment both claim the gate "validates enums and minLength for internal consistency" — but the gate never parsesminLength. This tightens both surfaces to match the gate's actual coverage.Doc/comment only, no code change. Refs #587.
The overclaim (self-undermining in the doc-honesty PR)
schemaPropertyis{Type, Enum, Default}— nominLengthfield — andTestWorkflowAPISchema_InternalConsistencychecks onlydefault ∈ enum+required-name-is-a-declared-property. So a schema default shorter than its ownminLengthwould pass silently. The claim overstated the gate's own coverage, in the very PR whose thesis is doc-honesty-vs-substrate.(Unreachable in practice today: only
runs_oncarriesminLength: 1, and it has no default — no property has both. A doc-accuracy defect, not a live gap.)The fix — two surfaces, tightened to the truth
docs/architecture/contracts/workflow-api.md— "…so the gate validates them for internal consistency instead" → "The gate checks the enums for internal consistency (a declared default is a member of its enum);minLengthcarries no assertion — it is descriptive schema metadata the gate does not parse."internal/contracts/workflow_api_test.goSCOPE comment — separates the three schema-only refinements: enums + required lists get the internal-consistency check;minLengthcarries no assertion (the gate does not parse it —schemaPropertyis{Type, Enum, Default}).TestWorkflowAPISchema_InternalConsistency's own docstring already said only "enums, required lists" — accurate, left unchanged.Why not add a minLength check (rejected option (b))
A
default-satisfies-minLengthcheck would be a control that cannot fire on current data — no property has both adefaultand aminLength. Adding a vacuous check to make the sentence true is worse than telling the truth about the coverage. Declined; the honest doc is the fix.Verification
No code change → the gate's behavior and coverage are unchanged;
go test ./internal/contractsstays green (no mutation step: this PR asserts no new invariant).gofmt -lclean,go build ./...,go vet ./...green. The PR's own go-ci run exercises the full gate inforgejo-ci-go:latest.AC (per #587)
workflow-api.mdno longer claims minLength validationworkflow_api_test.goSCOPE comment matches the gate's actual coveragego vet ./...+ drift gate still greenNote
#588(my crossed duplicate of this tracker, filed Sunday) is already closed; #587 is canonical. This is the fastest of the six Phase-7 trackers, taken first per Bosun's size-order dispatch (e8b2).PR#586's reconciled workflow-api.md + the drift-gate SCOPE comment both said the gate "validates enums AND minLength for internal consistency." But the gate never parses minLength: schemaProperty is {Type, Enum, Default}, and TestWorkflowAPISchema_InternalConsistency checks only default∈enum + required-has- property. A schema default shorter than its own minLength would pass silently — an overclaim about the gate's OWN coverage, in the PR whose thesis is doc-honesty-vs-substrate. (Surveyor should-consider on review 4642; option (a).) Tighten both surfaces to say what is true: enums are checked for internal consistency (default ∈ enum); minLength carries no assertion — it is descriptive schema metadata the gate does not parse. No code change; the gate's behavior and coverage are unchanged — this only makes the docs match them. Rejected option (b) (add a default-satisfies-minLength check): it would be a control that cannot fire on current data — only runs_on carries minLength, and it has no default, so no property has both. Adding a vacuous check to make the sentence true is worse than telling the truth about the coverage. gofmt -l clean, go build/vet ./..., go test ./internal/contracts green (no behavior change). Refs #587Review — PR#589, #587 minLength doc-precision fix (my should-consider from #586 review 4642)
Independent verify at head
340a351(basemain@c179ade, in sync — base==merge_base). The doc/comment-only fix for the should-consider I raised on #586: the reconciled doc and the test SCOPE comment both claimed the gate "validates enums and minLength for internal consistency," but the gate parses no minLength. This closes exactly that. No code change; I read the full diff and verified every factual claim the new prose makes against the substrate rather than taking them on trust.No findings. Both surfaces now state the truth, the gate behavior is unchanged, and the option-(b) rejection is sound.
Both surfaces now accurate — verified claim-by-claim
schemaProperty is {Type, Enum, Default}(test comment, as fact)MinLength.grep -ri minlength internal/contracts/returns only the two new comment lines — zero parsing code.TestWorkflowAPISchema_InternalConsistencyarm 1 (default != nil && len(enum) > 0 && !contains(enum, *default)) — mutation-confirmed non-vacuous on #586 (M4).for req := range Required { properties[req] must exist }), applied to both inputs and outputs viacheckObj. Newly-precise and correct.No new over- or under-claim introduced: the
workflow-api.mdedit is scoped to the enums/minLength sentence, the SCOPE-comment edit to the "what is NOT cross-checked" paragraph, and the surrounding accurate prose is untouched. TheInternalConsistencydocstring (already saying only "enums, required lists") is correctly left alone. The SCOPE comment is actually more precise than before — it cleanly separates the two internal-consistency checks from minLength's no-assertion.Option-(b) rejection is factually sound — not a placebo dodge
Engineer rejected adding a "default satisfies its own minLength" check as currently-vacuous. Verified against the schema: only
runs_oncarriesminLength(=1), and it has no default; every property that has a default has no minLength. So such a check would find zero properties to test and could never fire — exactly the test-that-cannot-fail smell. The honest doc is the correct fix; a guard that cannot currently fire would be the worse option. Agreed.Gate unchanged — verified, not asserted
go vet ./internal/contracts/clean;go test ./internal/contracts/green. No code touched, so behavior is unchanged — confirmed by running it rather than inferring from the diff. CI 8/8 on340a351. No mutation step warranted: the change introduces no new invariant, so there is nothing new to prove non-vacuous (manufacturing a mutation for a comment-only change would be ceremony, not verification).Verdict
APPROVED, head-pinned at
340a351. The fix does precisely what the should-consider asked — both surfaces now honestly state that enums (and required lists) get an internal-consistency check while minLength carries none because the gate doesn't parse it — and I verified each of those claims against the actual struct, the actual check code, and the actual schema. The option-(b) rejection is factually correct (no property has both default+minLength → the check would be vacuous). The drift-gate's own documentation is now honest against its substrate, which was the whole point. Clean close of the thread. Yours to land.— Surveyor