Skip to content

Declare the SMTP port range once and generate its refusal from the constants - #13191

Merged
os-elon merged 2 commits into
mainfrom
claude/issue-12993-smtp-port-contract
Aug 29, 2026
Merged

Declare the SMTP port range once and generate its refusal from the constants#13191
os-elon merged 2 commits into
mainfrom
claude/issue-12993-smtp-port-contract

Conversation

@os-elon

Copy link
Copy Markdown
Collaborator

Fixes#12993

The SMTP port bound 1-65535 was hand-written three times across two packages. This declares it once, generates the refusal from it, and holds the third site equal across the package boundary without creating a dependency edge.

The measurement that chose the shape

The card left open which layer should own the shared bound, and the dispatch brief's working assumption was that the transport owns it and the settings manifest derives. I measured the dependency direction before implementing, and it does not support an import in either direction:

depends on the other?
service-settings -> plugin-emailno edge at all — the plugin is named only in prose
plugin-email -> service-settingsdevDependency only, documented in-tree as "test-only, no runtime edge"

So making the manifest import the constant would add a runtime edge from a service to a plugin, invert the layering, close a cycle in the workspace graph, and pull nodemailer into the settings service's install closure. Per the brief's own fence I did not create it.

What I did instead — stated loudly rather than picked silently. The repo has already answered this exact question, for this exact pair of packages and this exact manifest: mail-manifest-providers.contract.test.ts holds the provider dropdown equal to EMAIL_TRANSPORT_PROVIDERS with a cross-package assertion over that same devDependency, and its header says why — "deliberately a CROSS-PACKAGE assertion rather than two mirrored literal lists: a literal pinned on each side can be 'fixed' by editing the other literal, which is exactly how the two drifted apart in the first place." This card's bound now rides the same mechanism. Net effect: 3 independent spellings become 1 declaration + 1 mirror that cannot drift, and 0 new dependency edges in either direction.

What changed

  • transports/smtp-port-contract.ts (new) owns SMTP_PORT_MIN / SMTP_PORT_MAX, the predicate, and the sentence.
  • Site 2 no longer exists. The message is generated from the constants, which is the stronger of the two options the card named — it deletes the drift instead of checking for it. A test comparing two literals would still leave two literals.
  • Site 3 keeps its numbers and gains a comment naming the owner, plus the cross-package assertion above.
  • Behaviour-preserving. The previous inline expression is kept verbatim as the oracle and the new predicate is asserted to agree with it across a table covering both edges, the non-finite values and the non-integers.

The fence

The SMTP floor is 1; the CLI listen floor is 0 and they are deliberately not the same contract. packages/cli/** was read as shape precedent and not edited, extended, or imported from. A dedicated case fails if the floor moves, and another fails if this package ever imports the CLI's module — so a future "unification" that made 0 a legal SMTP port goes red instead of silent.

Reverse verification

Three ablations, each committed-then-mutated, each with an absolute-path restore trap, each restore proven by git hash-object matching the HEAD blob and git diff HEAD empty:

ablationresult
ceiling drifts to 655343 red — cross-package mirror, accept-set oracle, single-declaration count
floor unified to 0 (the forbidden repair)3 red — floor fence, accept set, form mirror
message re-spelled as a literal1 red — single-declaration count only

The third is the load-bearing one: the re-spelled message produces a byte-identical output string, so all six equality assertions still passed and only the declaration count caught it. That is the difference between "generated" and "merely tested", made executable.

Scope breach, declared

My dispatch surface was plugin-email/src, service-settings/src/manifests, their tests and a changeset. Two files outside it were changed, both mechanically forced by this change and prescribed by the gate that failed:

  • scripts/cross-package-test-inputs.mjscheck:cross-package-test-inputs went red because the new test imports maskComments from the repo-root js-comment-mask.mjs, an escaping read. The entry added is the same pair @objectstack/metadata, @objectstack/cloud-connection and @objectstack/runtime already declare for the identical import.
  • turbo.json — the @objectstack/plugin-email#test task, with the inputs the gate printed verbatim, so the cache is keyed on those files.

The alternative was a private comment-stripper, which check:comment-mask-adoption exists to prevent ("14 private comment-stripper(s) ... A new one reds here"). Flagging rather than asking, because leaving the gate red was not an option and the remedy was the gate's own prescription.

The same gate also caught two things in my test and both are fixed: it named the CLI's path in a string literal (the collector takes quoted paths without parsing, which would have forced an input radius over the CLI's source), and its seed was import.meta.url, a TS1470 in this CJS-typed package.

Verification — union run on 75dd2a67e, the final commit

  • pnpm --filter @objectstack/plugin-email --filter @objectstack/service-settings test436/436 and 519/519 pass.
  • ... typecheck — both Done. This package's tests are in front of tsc (proven the hard way: two earlier drafts failed typecheck on the new test file), so this is a measurement of the new code, not a gap.
  • pnpm lint (repo-wide eslint . --no-inline-config) — exit 0, whole farm, not narrowed.
  • Gate families derived with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack from the actual changed set, re-derived after scripts/ and turbo.json entered it. Every matched and convention-triggered gate passes except the two named below, including check:cross-package-test-inputs, check:comment-mask-adoption, check:turbo-task-graph, check:engine-double-contract, check:where-matcher and check:type-check-debt (--re-measure: 31 ledger entries, 1570 raw tsc errors, none above its recorded number, surplus none).

Two gates are NOT MEASURED here and neither is a pass:

  • scripts/pm/check-half-states.mjs exits 3 — PREREQUISITE NOT MET: no valid GitHub credential in this container, so nothing was swept. (pnpm check:pm-half-states, the self-test invocation, does pass: 1551 cases.)
  • scripts/check-test-completeness.mjs exits 1 on a usage error — it consumes a turbo test log CI supplies. Not a red gate; it never entered its own body.

Out of scope, filed not fixed

Both are the same family but a different defect class from this card, so repairing either here would have broken this PR's behaviour-preservation claim.

Also observed and deliberately not filed: packages/spec/src/system/settings-manifest.test.ts:571 re-spells min: 1, max: 65535 in a mail-manifest fixture. It enforces nothing — it exercises SpecifierSchema parsing with arbitrary values — so it is not a fourth copy of this contract. Per the card, driver-sql's 65535 is a MySQL row-byte budget and was not touched.


Generated by Claude Code

… its refusal
The bound 1-65535 was hand-written three times across two packages: the
enforcement in SmtpTransport, the `(expected 1-65535)` literal on the very
next line, and `min: 1, max: 65535` on the mail settings form's `smtp_port`
field. The first two were adjacent lines -- changing the check without
changing the sentence produced a refusal that misstated its own rule, and
nothing failed.
`transports/smtp-port-contract.ts` now owns the constants, the predicate and
the sentence. The message is GENERATED from the constants rather than
re-spelled, so the second spelling no longer exists to drift.
The settings manifest keeps its own numbers on purpose: service-settings does
not depend on plugin-email, and plugin-email depends on it only as a test-only
devDependency, so importing the constant there would add a runtime edge from a
service to a plugin and invert the layering. The two are held equal by a
cross-package assertion over that existing devDependency instead -- the same
mechanism that already holds the provider dropdown equal to
EMAIL_TRANSPORT_PROVIDERS.
Behaviour-preserving: the accept set is pinned against the previous inline
expression, kept verbatim as the oracle. The floor stays 1 and is pinned
explicitly -- it is deliberately NOT the CLI's listen range, which floors at 0.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012WkdHQwHr2KQmaX7P1BHzi
…ge input radius
`check:cross-package-test-inputs` flagged the new pin test: it imports
`maskComments` from the repo-root `js-comment-mask.mjs`, which is an escaping
read, and a test whose real inputs are wider than its package is invisible to
both the affected-subset filter and the turbo cache.
Declares the same pair @objectstack/metadata, @objectstack/cloud-connection and
@objectstack/runtime already declare for the identical import, plus the
matching `@objectstack/plugin-email#test` turbo task so the cache is keyed on
it. The `.d.mts` sibling is declared because it types `maskComments`, so this
package's `tsc --noEmit` verdict depends on it too.
Two follow-on fixes in the test itself, both found by that gate:
- it named `packages/cli/.../port-contract.ts` in a string literal, and the
collector takes quoted paths without parsing, which would have forced an
input radius over the CLI's source for a string the test only reads out of
its own file. The control now uses the bare filename, which carries no
separator and is refused as too generic.
- the seed moved from `import.meta.url` to `__dirname`: this package is
CJS-typed, so the meta-property is a TS1470 under `module: NodeNext` and
the package's tests are in front of tsc.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012WkdHQwHr2KQmaX7P1BHzi
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

5 anchor(s) derived from 2 changed package(s); no hand-written page names any of them, so this run has nothing to listnot a clean bill of health. This check sees only pages that NAME a derived anchor: one that documents this change in prose, or enumerates it in an authoring dialect, names none and stays invisible to it on every run.

What this run could not see
  • 2 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 47 of 219 client-bound route-ledger rows — the other 172 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 172: 14 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 10 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 39d5d8a6001c210918f6f4dd716b3f2c44f72f8bpackageMentionDocs.

Which tree this was computed on

This run read content/docs from b3d854401f615117fe1aa867fb185b52774cf4b1 — the merge of head 75dd2a67e17ec6f40d844dfd37c86e3c5f728c1e into base 39d5d8a6001c210918f6f4dd716b3f2c44f72f8b, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin b3d854401f615117fe1aa867fb185b52774cf4b1 && git checkout b3d854401f615117fe1aa867fb185b52774cf4b1
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 39d5d8a6001c210918f6f4dd716b3f2c44f72f8b 75dd2a67e17ec6f40d844dfd37c86e3c5f728c1e && git checkout -B drift-repro 39d5d8a6001c210918f6f4dd716b3f2c44f72f8b && git merge --no-ff 75dd2a67e17ec6f40d844dfd37c86e3c5f728c1e
node scripts/docs-audit/affected-docs.mjs --json 39d5d8a6001c210918f6f4dd716b3f2c44f72f8b

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/mteststooling

Projects

None yet

2 participants

@os-elon@claude