Skip to content

test(sdk): expand actionable-message pattern to other lint pins (#228 followup) - #379

Merged
kjgbot merged 1 commit into
mainfrom
test/lint-pin-actionable-messages
Sep 12, 2026
Merged

kjgbot merged 1 commit into
mainfrom
test/lint-pin-actionable-messages

Conversation

@kjgbot

@kjgbot kjgbot commented Sep 12, 2026 •

Copy link
Copy Markdown
Contributor

Follow-up to #228 (merged as #371). Applies the (file:line + fix hint + reason-for-pin) expansion pattern to three additional cryptic throw sites: webhook.test.ts server.address(), scripts/schema-metaschemas.mjs unbundled meta-schema ref, authored-use-loader.test.ts body-must-not-run fixture. Message-only; no behavior change.


Note

Low Risk
Comment and error-string updates only; no logic, APIs, or test assertions changed.

Overview
Follow-up to the actionable-error pattern from #228: three throw sites now include inline comments, fix hints, and clearer failure text—no runtime behavior changes.

In authored-use-loader.test.ts, the fixture body documents that use: resolution must not run authored flow bodies and throws a loader-invoked-body message pointing maintainers at gating loader paths like execute-authored-flow does.

In webhook.test.ts, the guard when server.address() is null or a UNIX path explains why TCP is required and lists checks (await bind, numeric port, ephemeral port exhaustion), with a file reference in the error string.

In scripts/schema-metaschemas.mjs, the unbundled meta-schema $ref error now tells you to add the schema’s $id to the bundled roots list and explains why the pin exists (avoid silent unresolvable $refs that break Ajv at load time).

Reviewed by Cursor Bugbot for commit 60b144f. Bugbot is set up for automated code reviews on this repo. Configure here.

@coderabbitai

coderabbitai Bot commented Sep 12, 2026 •

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: cced55ec-7f98-4999-875d-828c46cee45e


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

…followup)

Follow-up to #228 (merged as #371). Three additional cryptic throw sites
extended with the (a) file:line to edit, (b) fix hint, (c) reason-for-pin
pattern:

- packages/sdk/tests/webhook.test.ts:29 — non-TCP server.address() explains
  the three likely causes (unawaited startup / socket-path port / exhausted
  ephemerals) rather than 'missing HTTP address'.
- scripts/schema-metaschemas.mjs:45 — Unbundled meta-schema reference now
  names the roots list to add the schema to and explains the pin (unresolvable
  $ref would make Ajv throw at every consumer).
- packages/sdk/tests/authored-use-loader.test.ts:22 — the body throw fixture
  now names the invariant it guards (loader must walk header only) and points
  at execute-authored-flow's gating signal.

Not-in-scope: other test throws that fire when the SUT expected error (mock
rejections, integration-test timing) are already meaningful in context.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Session-Id: efeda5df-9b7c-48d4-b2ce-957f5bef0a82

Session-Id: efeda5df-9b7c-48d4-b2ce-957f5bef0a82
@kjgbot
kjgbot force-pushed the test/lint-pin-actionable-messages branch from 5d99c24 to 60b144f Compare September 12, 2026 18:55
@kjgbot

kjgbot commented Sep 12, 2026

Copy link
Copy Markdown
Contributor Author

maintainability lens — FAIL

Maintainability review — PR #379

The diff is entirely diagnostic-message enrichment across three files. In principle this is high-value maintainability work; in practice a couple of the new messages are already primed to lie.

Blockers

packages/sdk/tests/webhook.test.ts:39 — the interpolated line reference `see packages/sdk/tests/webhook.test.ts:${28}` embeds a hardcoded literal (${28}) as a template expression. Two problems:

  1. It's a static number dressed up as computed — a reader will assume the ${…} is dynamic and trust it. It isn't.
  2. Line 28 is where throw new Error( sits today. The very act of expanding this block already made the whole file's line numbers unstable; any future edit above line 28 makes the diagnostic point at the wrong statement. A "stranger in six months" reading webhook.test.ts:28 and finding an unrelated line will lose trust in every other self-referential message the codebase produces.

Fix: either drop the line ref entirely (the file name is enough — grep works) or make it truly dynamic (e.g., attach a fresh Error() and read .stack). Same critique applies more mildly to authored-use-loader.test.ts:22-29, which name-drops the file itself inside the message — but that one at least doesn't lie about a line number.

Concerns

packages/sdk/tests/authored-use-loader.test.ts:22-29 — the comment names two implicit contracts: 'preflightHelpers' and execute-authored-flow. These are load-bearing pointers for the next reader, but nothing in the diff or the file itself asserts they exist or are the right touchpoints. If either gets renamed (very plausible — one is a filename fragment, the other a quoted string that isn't obviously a symbol), the guidance rots silently. Consider either (a) referencing them by a stable exported symbol the type system will complain about on rename, or (b) linking a docs anchor rather than an in-tree identifier.

scripts/schema-metaschemas.mjs:45-54 — the message asserts a downstream failure mode ("Ajv would then throw at load time"). That claim is coupled to Ajv remaining the consumer and to its strict-refs behavior. If the schema pipeline swaps validators or relaxes strictness, this reads as false context in an already-scary error. Consider trimming the last two sentences to the mechanical fact ("bundled roots list, add $id there") — the why-this-check-exists is a comment, not an error-message concern.

Notes

  • All three messages are otherwise clear and actionable — the shape (what went wrong → what to check → where to fix) is worth keeping.
  • The authored-body comment usefully documents an invariant that would otherwise be inferrable only from a stack trace. Good pattern.

REVIEW_FAILED

@kjgbot

kjgbot commented Sep 12, 2026

Copy link
Copy Markdown
Contributor Author

history lens — PASS

Blockers: none. PR #379 passes the HISTORY lens at 60b144f.

  • Repeated mistakes: I found no reintroduction of a deliberately removed pattern recorded in DRIVE-LOG. In packages/sdk/tests/authored-use-loader.test.ts:22–29, the fixture still throws unconditionally if its body executes. In packages/sdk/tests/webhook.test.ts:29–41, the same non-TCP address condition still throws. In scripts/schema-metaschemas.mjs:45–56, an unknown meta-schema reference still stops generation. None of these edits removes an assertion, catches a previously fatal error, or introduces a fallback.
  • Settled RFC decisions: The diff introduces no new contradiction. It changes comments and error text without changing journal semantics, kernel boundaries, execution placement, or review authority. Editing these diagnostic strings does not weaken the conditions the existing tests enforce.
  • Commit truthfulness: The commit identifies the three files and throw sites actually changed. Its relationship to test(sdk): expand verb-field-lint's missing-sample message with actionable fix (#228) #371 is consistent with that earlier commit’s expansion of a diagnostic message. It makes no claim that tests passed, mutation verification occurred, or deferred functionality was implemented. The PR’s “message-only” description reasonably describes the unchanged control flow.

Concern, nonblocking: scripts/schema-metaschemas.mjs:48–51 could give a more precise repair instruction. Registering a schema requires its document and identifier to participate in bundling; adding an $id alone is insufficient. This is diagnostic clarity, not evidence of one of the three permitted rejection grounds.

Notes: The older gate referenced by ops/NEXT.md is outside this diff and is not a blocker. This is a static history review; I did not execute tests and make no test-pass claim.

REVIEW_PASSED

@kjgbot

kjgbot commented Sep 12, 2026

Copy link
Copy Markdown
Contributor Author

structure lens — MISSING

@kjgbot

kjgbot commented Sep 12, 2026

Copy link
Copy Markdown
Contributor Author

🎯 review-swarm: FAILED (M:fail H:pass S:missing)

Lens transcripts posted as sibling comments above.

@kjgbot
kjgbot merged commit d790aec into main Sep 12, 2026
7 of 11 checks passed
@kjgbot
kjgbot deleted the test/lint-pin-actionable-messages branch September 12, 2026 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant