fix: mark every notice check prints, and carry notices as a list - #397
Conversation
|
@claude /review |
github-actions
Bot
added
the
Open OpenSpec
|
Claude finished @thecodedrift's task in 3m 38s —— View job Review completeReview mode: incremental — read 0 prior review thread(s) before reviewing.
Nothing new found. No confirmed or plausible correctness bugs survived verification. What I checked
One thing that's a documentation nuance rather than a bug worth blocking on: the proposal ( |
|
@claude /review |
|
Claude finished @thecodedrift's task in 2m 26s —— View job Review completeReview mode: incremental — read 0 prior review thread(s) before reviewing.
No correctness, security, or test-adequacy bugs found. The Worth a look (not blocking)[New] Documented external consumer of the changed envelope. [New] Two small duplications, low severity:
Considered and set aside: whether the changeset should be One pre-existing documentation nuance from the last review remains true and isn't repeated here as a new finding: the OpenSpec proposal's "archived here" wording vs. the dry-run-only archive described in |
Confirmed — no high/medium findings, nothing to change. The proposal-vs- — AI Coding Agent |
No high/medium findings, so nothing auto-fixed. Your three "worth a look" items are all
Separately, one gap this review round did catch indirectly: the changeset was missing the paragraph describing the runtime-plan notice fix (it was written but never staged into the amend it rode on). Fixed in The two deliberate positions in this PR stand and were not challenged: the renderers split on — AI Coding Agent |
github-actions
Bot
removed
the
Open OpenSpec
The `unavailable` branch built its notice as `joinNotices([...advisories, outcome.message]) ?? outcome.message`. On that branch `outcome.message` is a `string`, so the list handed to the joiner is never empty and the joiner never returns `undefined`. The `??` arm could not run. It read as a safety net, which is worse than no net: the next person to touch the joiner would have reasoned about a fallback path that does not exist.
…as a list `check`'s text output printed `Notice: ` once per element of `DispatchResult.notices`, while the producers glued several advisories into one element with `"\n"`. A run with a schema advisory and a Vale diagnostic therefore printed the first behind a marker and the second as an unlabelled stray line. That is the same defect `241e1c4` fixed in `verify`, still live in `check`. Fixed twice over, because one fix alone leaves the trap set: - Both renderers now split a notice on `"\n"` and prefix every line. A single notice can legitimately span lines — Vale's stderr is passed through as written — so this is needed even once the list is flat. - The underlying field is a list. `EngineOutcome.notices`, `ValeRunOutcome`, `ValeVerifyResult`, `SchemaLayerResult`, `RuleVerification` and `RuleTestResult` all carry `string[]`, one notice per element, so `runEngines` concatenates into a genuinely flat `DispatchResult.notices` and `check --json` stops publishing array elements that are several notices in a trench coat. Presentation belongs to the renderer; a producer that picked its own separator could only mis-render. `rules/verify.ts` was the producer that proved the point: it joined `language.notices` with a SPACE, so an off-list `language:` spelling and a `files:` glob its language cannot parse arrived as one run-on sentence behind one marker. There is now no separator convention left for a producer to get wrong. `packages/cli/src/util/notices.ts` holds the one helper, `collectNotices`, as a leaf module with no imports of its own: having `dispatch.ts` import it from `vale/run.ts` would add an edge inside `src/rules/` between modules that already sit close to the cycle #388 fixed. It drops `undefined` entries, and also `""`, which no longer renders as a bare marker saying nothing. BREAKING: the published `notice?: string` field becomes `notices: string[]` in `verifyOutputSchema.schema`, `valeVerifyOutputSchema` and the `verify`/`test` envelope — so `verify --json` and `test --json` emit `notices`. Replaced rather than mirrored: keeping a joined `notice` alongside would preserve the separator convention this change exists to remove, and a consumer could not safely split it in the first place. Empty, never absent, as `violations` beside it already is. Refs #390
…s copies Three gaps, all of them the reason the defect survived: - `collectNotices` gets its own tests: order preserved, `undefined` dropped, `""` dropped, empty input empty out, and a multi-line notice left as one element. - `check`'s TEXT output had no test at all for how it renders notices. The new one drives the built CLI over a project whose two Vale rules each draw a config advisory, and asserts each reaches its own `Notice: ` line. On the old code the two arrived joined and the second printed as an unmarked stray, so this fails there. - `check --json` now asserts the two are separate elements and that no element spans lines. The three separator-blind tests named in #390 asserted with `toContain` over the whole field, so a producer switching separator would have kept passing. They now assert on the elements.
Nine spec files mention "notice" and every one is about WHETHER one surfaces. Nothing said how several are separated, how they render, or what a machine consumer receives — so the four producers and two renderers agreed only by coincidence, and `check`'s renderer could ship the `241e1c4` defect without violating a requirement. Three ADDED requirements, one per capability that owns a producer or a renderer: - `cli-check` — one marker per notice, every line of a multi-line notice marked, and `--json` `notices` flat with one notice per element. - `cli-rule-validation` — `verify`/`test` carry notices as a list, one `notice:` marker each, and publish `notices` present-and-empty rather than absent. - `cli-vale-rule-engine` — independent advisories from one run stay separate notices; the engine picks no separator. ADDED rather than MODIFIED throughout: nothing standing describes this, so there is no requirement to restate, and a MODIFIED block against a requirement about a different question is how scenarios get dropped silently. Verified by dry-run archive and reset. Requirements 20/7/11 -> 21/8/12 and scenarios 55/37/37 -> 59/40/40, with no prior scenario missing from any of the three.
`patch`, and pre-1.0 settles it: the package is 0.11.2, where semver puts the public API outside the stability guarantee, so replacing a published field's shape does not earn more. The body says what a consumer crosses rather than leaning on the bump to say it.
…enderer `check` printed the runtime plan's notices — what a repair restored, what it could not, and why — in their own loop with NO marker, while the dispatched notices got `Notice: `. Both end up in the same `--json` `notices` array, so the same message looked like two different kinds of thing depending on which list it arrived on, and the plan's notices are exactly the ones whose whole purpose is explaining a rule that did not run. All three sources (plan notices, skipped runtime rules, dispatched notices) now go through one `warnNotice`, so they cannot drift apart again. `markNotice` joins `collectNotices` in the leaf module and is shared by both commands, which differ only in the marker: `Notice: ` at the left margin for `check`, ` notice: ` indented under the rule for `verify`. That is a presentation choice, not a second contract, and having one helper is what stops `241e1c4` being fixed in one renderer and left live in the other — which is the exact history of this bug. Per-line marking matters here for a reason that is latent rather than live: no notice the CLI produces today spans lines, but a runtime repair notice embeds an `Error.message` it did not author, and Vale's stderr is passed through as written. `markNotice` is unit-tested on the multi-line case directly, since nothing reachable end to end exercises it yet. The `cli-check` delta gains a sentence and a scenario for the marked-alike rule. Re-verified by dry-run archive and reset: requirements 20/7/11 -> 21/8/12 and scenarios 55/37/37 -> 60/40/40, no prior scenario missing.
The paragraph was written but never staged: the amend it was meant to ride on reported no staged files and went through with the earlier tree, so the shipped note described only the dispatched-notice half of a fix that has two.
This PR is the tip — no open PR is based on this branch — so the change archives here rather than on landing. `openspec-label.yml` reports an unarchived change directory, and `main` takes pull requests only, so a change that reaches `main` unarchived needs a second PR to do what the tip should have done. The real archive matches the dry run exactly, which is the check that matters: an archive REPLACES each requirement it names, so a delta that quietly drops a scenario leaves no trace and `validate --strict` still passes. cli-check 20 -> 21 requirements, 55 -> 60 scenarios cli-rule-validation 7 -> 8 requirements, 37 -> 40 scenarios cli-vale-rule-engine 11 -> 12 requirements, 37 -> 40 scenarios Zero prior scenarios missing in any of the three, compared set-wise rather than by count so a drop masked by an addition could not hide. The proposal's delivery-shape note said the change "is archived here" while tasks.md recorded only the dry run, which read as a contradiction. Both now say the same thing, and say why the tip is where it happens.
5790e3e to
48afe1a
Compare
1 participant
The user-visible bug, first
taskless checkprinted only the first line of its notice output behind a marker. A run with two advisories — a Vale config advisory beside Vale's own zero-exit diagnostic, say — rendered like this:The second advisory arrives as a bare, unindented line with nothing identifying it as a notice. This is not cleanup: it is the same defect
241e1c4fixed inverify, still live incheck, and it fails in the direction hardest to notice — the notice still appears, just wrongly attributed.check's renderer now splits each notice on"\n"and prefixes every line, exactly asverifydoes.The second one, in the same output
The runtime plan's notices were printed with no marker at all.
checkhad two loops:plan.noticeswent out bare,dispatched.noticesgotNotice:. Both land in the same--jsonnoticesarray, so the same message looked like two different kinds of thing depending on which list it arrived on — and the plan's notices are the ones whose entire purpose is explaining a rule that did not run.All three sources (plan notices, skipped runtime rules, dispatched notices) now go through one
warnNotice.Both commands now share
markNoticefrom the leaf module. They differ only in the marker —Notice:at the left margin forcheck,notice:indented under the rule forverify— which is a presentation choice, not a second contract. One helper is what stops241e1c4being fixed in one renderer and left live in the other, which is precisely this bug's history.Per-line marking on this path is latent rather than live: no notice the CLI produces today spans lines. But a runtime repair notice interpolates an
Error.messageit did not author, and Vale's stderr is passed through as written, so it will arrive without anyone choosing it.markNoticeis therefore unit-tested on the multi-line case directly rather than through a producer that cannot yet reach it.The structural half, so the trap stays sprung
Fixing the renderer alone leaves the cause in place. Four producers each built a
noticeby joining a list of strings, soDispatchResult.noticeswas assembled byflatMapover pre-joined strings — andflatMapdoes not split strings, so several notices arrived as one array element. That is whatcheck --jsonpublished.noticesis now a list all the way through:EngineOutcome,ValeAttempt/ValeRunOutcome,ValeVerifyResult,SchemaLayerResult,RuleVerificationandRuleTestResultall carrystring[], one notice per element.check --json'snoticesarray keeps its name and itsstring[]type; only its element boundaries change, so one element is now exactly one notice.Presentation belongs to the renderer. A producer no longer picks a separator, so it cannot pick the wrong one.
packages/cli/src/util/notices.tsholds the one helper,collectNotices, as a leaf module with no imports of its own —dispatch.tsimporting it fromvale/run.tswould add an edge insidesrc/rules/between modules that already sit close to the cycle #388 fixed. It dropsundefinedentries and also"", which previously survived the filter and rendered as a bare marker saying nothing.The
" "join inrules/verify.ts, and what it cost to removeThis was the open question in the brief, and the answer is that it no longer exists.
rules/verify.tsjoinedlanguage.noticeswith a space, not a newline. An sg rule declaring both an accepted-but-off-listlanguage:spelling and afiles:glob that language cannot parse produced one run-on sentence behind one marker. It was proposed to keep it as a documented exception. With the field a list, that call site does not join at all — it contributes two elements and the renderer marks each.What was measured before committing to that. The two pipelines are not independent: they converge at
runVale, which feedscheckviadispatch.tsandverify/testviavale/verify.ts→inspect.ts.run.tsglued the converter-skip notice and Vale's stderr into one string, so leavingValeRunOutcome.noticea joined string would have leftcheck --jsonpublishing an element with an embedded newline regardless. Carrying the list through the verify pipeline was therefore not optional extra scope — it was what thecheckfix already required.The real cost is the published surface, and it is three released fields, not the two the brief named:
v0.11.2?verifyOutputSchema.schema.notice520a382valeVerifyOutputSchema.notice5f2dbb2verify/testenveloperuleResult.noticeAll three become
notices: string[]. Replaced, not mirrored. A joinednoticeretained alongside would preserve the separator convention this PR exists to remove, and a consumer could not have split it safely in the first place, because nothing published the separator. Empty-and-present rather than absent, matchingviolationsbeside it, so a consumer can tell "nothing to report" from "this CLI does not report notices".check --jsonconsumers need change nothing.Pre-1.0 (
0.11.2) the bump stayspatch; the changeset body says what a consumer crosses rather than leaning on the bump to say it.Also cut: the
?? outcome.messagefallback at the Valeunavailablebranch, which was unreachable —outcome.messageis astringthere, so the joiner never returnedundefined.Spec
Nine spec files mention "notice" and every one is about whether one surfaces. Nothing said how several are separated, how they render, or what a machine consumer receives — which is why
checkcould ship this defect without violating a requirement. Three ADDED requirements, one each tocli-check,cli-rule-validationandcli-vale-rule-engine.ADDED rather than MODIFIED throughout: nothing standing describes this, so there is no requirement to restate, and a MODIFIED block against a requirement about a different question is how scenarios get dropped silently.
Verified by dry-run archive and reset to a captured SHA, per
CLAUDE.md:cli-checkcli-rule-validationcli-vale-rule-engineNo prior scenario is missing from any of the three. The change directory is left in place.
Tests
markNoticeon a multi-line notice: every line marked, not just the first.Notice:marker. The existing test usedtoContain("dangerously-run-scripts"), which passed on the unmarked output.check's TEXT output had no test of how it renders notices at all. The new one drives the built CLI over a project whose two Vale rules each draw a config advisory and asserts each reaches its ownNotice:line — it fails on the old code.check --jsonasserts the two are separate elements and that no element spans lines.collectNoticesgets unit tests: order preserved,undefineddropped,""dropped, empty in empty out, multi-line notice left as one element.toContainover the whole field, so a producer switching separator kept passing. They now assert on elements. No assertion was weakened.pnpm build,pnpm typecheck,pnpm lint(exit 0, includingpnpm cli check) andpnpm test(1709 passed, 103 files) all green.Fixes #390