Skip to content

test(sdk): expand verb-field-lint's missing-sample message with actionable fix (#228) - #371

Merged
kjgbot merged 1 commit into
mainfrom
fix/verb-field-lint-actionable-msg-228
Sep 12, 2026
Merged

kjgbot merged 1 commit into
mainfrom
fix/verb-field-lint-actionable-msg-228

Conversation

@kjgbot

@kjgbot kjgbot commented Sep 12, 2026 •

Copy link
Copy Markdown
Contributor

Closes #228. Dogfooded via implement-slice.flow.ts on finn-mini.

Summary

Extends the foreignFieldValue throw in packages/sdk/tests/verb-field-lint.test.ts to name (a) which map to edit, (b) the expected shape, and (c) the reason the pin exists.

Test plan

  • Deliberately-missing sample declaration reproduces the new multi-line error text.
  • Existing 90+ verb-field-lint tests still pass.

Note

Low Risk
Test-only error message text; no production paths or validation logic change.

Overview
Improves the verb-field-lint test helper when a new step field is missing from VERB_FIELD_VALUES: the thrown error now tells you to add an entry in packages/sdk/tests/verb-field-lint.test.ts, shows the expected key shape, and explains why the pin matters (undefined values survive in-memory validation but disappear in YAML, so coverage would be fake).

No runtime or validator behavior changes—only the failure text developers see while extending the foreign-field test generator.

Reviewed by Cursor Bugbot for commit d0c50c3. 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: 2f2aaa81-86b2-4c6b-ad90-edcb4a5d0ecc


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.

@kjgbot

kjgbot commented Sep 12, 2026

Copy link
Copy Markdown
Contributor Author

maintainability lens — PASS

Maintainability review — PR #371

Scope. Single-line error-message expansion in packages/sdk/tests/verb-field-lint.test.ts:96 → now a multi-line message telling the reader exactly which file to edit, what pattern to add, and why the guard exists.

Blockers

None.

Concerns

  1. Duplicated rationale, one copy will rot (verb-field-lint.test.ts:86-93 vs the new message at :96-102). The docblock above foreignFieldValue already explains the same "in-memory validateSpec survives, YAML drops it" invariant that the new error text now restates in slightly different words ("silently produce {field: undefined} … drops on YAML serialization"). Two prose copies of the same contract will drift the next time someone tweaks the reasoning. Consider referencing the docblock ("see docblock above") or removing one of the two. Not a blocker because both currently agree, but the whole point of this lens is: could a stranger safely edit this in six months? They will edit one and forget the other.

  2. Self-referential file path in the error string (:97). The message hard-codes packages/sdk/tests/verb-field-lint.test.ts. If this test is moved or renamed (plausible — it's a lint-style helper), the guidance in the error points at the wrong file with no compile-time signal. A relative pointer ("in this file, edit VERB_FIELD_VALUES") would be equally actionable and rot-proof.

Notes

  • The change genuinely improves the "stranger in six months" scenario — the why is captured at the point of failure, which is exactly where a future contributor will land. That is unusually good practice.
  • No test asserts the new message content, so wording drift is safe (this is fine — behavior-under-test is the throw, not the string).
  • Template literal is well-formed; the escaped backticks around VERB_FIELD_VALUES render as intended in terminal output.
  • No new implicit contract or failure-handling surface introduced; the guard's semantics are unchanged.

Net: a small, well-motivated readability improvement to a fail-closed guard. The concerns are drift-hygiene, not correctness.

REVIEW_PASSED

@kjgbot

kjgbot commented Sep 12, 2026

Copy link
Copy Markdown
Contributor Author

history lens — PASS

Blockers: none. PR #371 passes the three HISTORY criteria.

  • Repeated mistakes: In packages/sdk/tests/verb-field-lint.test.ts, diff lines 94–103 preserve the Object.hasOwn guard, unconditional throw for missing samples, and existing return value. The change does not restore the unchecked lookup previously removed because YAML serialization dropped undefined-valued fields. Lines 97–101 instead explain that failure mode and identify the required repair. This also avoids DRIVE-LOG’s recorded mistake of restoring deliberately deleted behavior during integration.
  • New RFC contradictions: Lines 96–101 change diagnostic text only. They introduce no kernel vocabulary, execution path, journal behavior, or gate-authority change. In particular, editing this helper’s message does not weaken its assertions or missing-sample refusal.
  • Commit truthfulness: The commit describes expanding the missing-sample message with an actionable fix. That matches the diff; it makes no claims about completed tests, mutation verification, or production behavior.

Captured scope evidence:

$ git diff 'd0c50c3f^' d0c50c3f --numstat
6	1	packages/sdk/tests/verb-field-lint.test.ts

Concerns: The new wording “passes in-memory validation” at diff line 100 is less precise than the existing comment: the undefined-valued property remains enumerable, allowing the rejection test to exercise it. This wording ambiguity does not reintroduce the bug or establish a HISTORY blocker.

Notes: I ran the requested history command and inspected repository guidance and relevant operational history. The PR’s test-plan boxes remain unchecked; I did not execute tests and make no test-pass claim. The older gate reference in ops/NEXT.md is outside this diff’s correctness scope.

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:pass H:pass S:missing)

Lens transcripts posted as sibling comments above.

@kjgbot
kjgbot merged commit b2bc559 into main Sep 12, 2026
6 of 8 checks passed
@kjgbot
kjgbot deleted the fix/verb-field-lint-actionable-msg-228 branch September 12, 2026 18:03
kjgbot pushed a commit that referenced this pull request Sep 12, 2026
…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 added a commit that referenced this pull request Sep 12, 2026
…followup) (#379)

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

Co-authored-by: kjgbot <kjgbot@agentrelay.dev>
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.

verb-field-lint: the pin's failure message doesn't say what to do (caught two lanes)

2 participants