fix(cli): render StoryboardStepResult.hints[] in all reporting surfaces#894
fix(cli): render StoryboardStepResult.hints[] in all reporting surfaces#894bokelley wants to merge 3 commits into
Conversation
The runner emits context_value_rejected hints when a seller's error traces back to a $context.* substitution, but all reporting surfaces (CLI step printers, JUnit XML, storyboard step command) silently dropped them. This patch wires up the three human-readable printers and the JUnit formatter so the diagnostic is visible wherever a developer looks. The JUnit formatter is extracted from bin/adcp.js into src/lib/testing/storyboard/junit.ts so it can be unit-tested without bootstrapping the full CLI dependency tree. Closes #879 https://claude.ai/code/session_01CokHw4pHKBK2v1aYVE9LfL
IPR Policy Agreement RequiredThank you for your contribution! Before we can accept your pull request, you must agree to our Intellectual Property Rights Policy. By making a Contribution, you agree that:
To agree, please comment below with the exact phrase: You can read the full IPR Policy here. I have read the IPR Policy You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
- Add @internal to formatStoryboardResultsAsJUnit JSDoc: resolves the public/internal signal mismatch flagged by dx-expert (the function is CLI tooling, not a public @adcp/client API) - Add comment to CLI hint blocks noting the runner only emits hints on failed steps, documenting the invariant raised by code-reviewer - Update changeset to mention the formatter extraction and new tests https://claude.ai/code/session_01CokHw4pHKBK2v1aYVE9LfL
Auto-generated by sync-version script during build; version.ts was behind package.json on this branch. https://claude.ai/code/session_01CokHw4pHKBK2v1aYVE9LfL
|
Closing as obsolete — the core hint-rendering work in this PR landed via #891 before this branch was rebased. The three remaining deltas (extract |
…ck (#900) * feat(cli): extract JUnit formatter + step-cmd hints + message= fallback Cherry-picks the three salvageable deltas from closed PR #894 (which overlapped heavily with merged #891 after a long rebase drift): 1. Extracts `formatStoryboardResultsAsJUnit` from `bin/adcp.js` into `src/lib/testing/storyboard/junit.ts` (@internal). Now unit-testable as a pure function — 10 tests pinning XML structure, hint integration, and escaping added in `test/lib/storyboard-junit-formatter.test.js`. 2. Wires `printStepHints` into `adcp storyboard step` — a printer site that #891 missed. Parameterizes the indent so both the phase-nested (` `) and column-zero step printers share the helper. 3. Adds `<failure message=…>` attribute fallback to `hints?.[0]?.message` when `step.error` is absent. Closes an edge opened by the #883 gate widening (validation-only failures with no task-level error): CI dashboards that only read the attribute now surface the hint. Drops the now-unused `formatHintsForFailureBody` export from `adcp-step-hints.js` and its tests — the JUnit module owns its own hint rendering. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: move @internal onto the function + drop dead re-export (reviewer) Code-reviewer flagged: the file-level `@internal` JSDoc was binding to the next declaration (the import of `StoryboardResult`), so stripInternal removed the import but left the function signature referencing an undeclared name. Emitted `.d.ts` was: export declare function formatStoryboardResultsAsJUnit( results: StoryboardResult[] ): string; — any downstream TS consumer hit TS2304. Fixed by moving the @internal annotation onto the function itself; d.ts now correctly emits `export {};`. Also drops the re-export from `storyboard/index.ts` — stripInternal already strips it from the public type surface and the CLI reaches into `dist/lib/testing/storyboard/junit.js` directly, so the re-export was dead for every consumer. Testing-reviewer nits addressed: - New test: empty results produces valid empty testsuites element - New test: multi-storyboard + multi-phase aggregation (catches off-by- ones in totalTests / suiteTests reducers) - New test: `step.error === ''` (empty string, not undefined) → first hint message fallback kicks in - Replaced brittle escape-pipeline-reconstruction regex with explicit entity assertions; scoped `'` check to the `message=` attribute where it's actually load-bearing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Wires
StoryboardStepResult.hints[]into every human-readable and CI reporting surface that was silently dropping it. The runner already populateshintswithcontext_value_rejecteddiagnostics (landed in #875/#870); this PR makes them visible.Also extracts
formatStoryboardResultsAsJUnitfrombin/adcp.jsintosrc/lib/testing/storyboard/junit.ts(marked@internal) so the formatter can be unit-tested without bootstrapping the full CLI dependency tree.What changed
bin/adcp.jsadcp storyboard run(single + multi-instance): printsHint: <message>below theError:line for each failing step with hintsadcp storyboard step: same, matching that command's no-indent error styleformatStoryboardResultsAsJUnitfunction (now imported fromdist/lib)src/lib/testing/storyboard/junit.ts(new,@internal)<failure>body betweenstep.errorand failing validationsmessage=attribute falls back tostep.hints?.[0]?.messagewhenstep.erroris absent — so CI systems that only read the attribute still surface the diagnostic@internalJSDoc and absence from top-level barrel signal it is not part of the public@adcp/clientAPITest plan
node --test test/lib/storyboard-junit-hints.test.js— 5/5 pass (new snapshot tests)node --test test/lib/storyboard-rejection-hints.test.js— 21/21 pass (existing)node --test test/lib/storyboard-runner-contract.test.js— 29/30 pass (1 pre-existing skip unchanged)npm run format:check— cleanNote for reviewer: comply path
Planning experts flagged whether to also thread hints through
ComplianceFailureso they appear inadcp complyhuman-readable output. The dx-expert called this a follow-up concern; the protocol-expert called it out-of-scope (the comply JSON path already serializes hints verbatim). This PR takes the narrower scope per the issue's stated acceptance criteria.Nits noted (not fixed)
xmlEscapecall is correct; a test would pin it)message=attribute omits theHint:prefix intentionally for brevity (pinned by test on line 108 ofstoryboard-junit-hints.test.js)Closes #879
Pre-PR review:
@internaladded to exported symbol to clarify public/internal boundary); 2 nits noted aboveSession: https://claude.ai/code/session_01CokHw4pHKBK2v1aYVE9LfL