chore!: the audit trims, and the kept surface says why it stays - #96
Open
btravers wants to merge 2 commits into
Open
chore!: the audit trims, and the kept surface says why it stays#96btravers wants to merge 2 commits into
btravers wants to merge 2 commits into
Conversation
From the 2026-08-23 ponytail audit, re-triaged under the library lens: unused by the examples is not unused, because the examples are scenarios rather than the library's one user. Cut: di's `Unmet` export, whose documented purpose (shaped modules re-declaring the gates with it) was impossible — declaration emit keeps the alias unreduced and TS2883 follows, which is why every in-repo sugar already inlined; http's scheme-port memoisation, measured unnecessary (di resolves by id, a fresh class per call is the same lookup, 50/50 and 26/26 green without it); the `declared` double-cast; `Tagged`/`Principal`'s twice-guarded `keyof Schemes`; and the `rpcVerified` fixture's async wrapper around a sync value. Kept, with the reason now in the TSDoc where the next audit will look: `HasMark` (tooling over a contract — an OpenAPI generator deciding whether to emit `security` is the canonical reader), `authenticatorPort` (a test composition substituting one scheme's authenticator provides on this port), and `Http.authenticators` (the introspection surface a hand-rolled composition reads).
There was a problem hiding this comment.
Pull request overview
Removes an impossible-to-consume @btravstack/di public type export (Unmet) and applies a set of audit-driven trims/clarifications across @btravstack/http, including tightening type-level auth shapes and documenting why certain “no in-repo caller” exports remain part of the library surface.
Changes:
- Remove
Unmetfrom@btravstack/di’s public exports and update in-repo docs/comments to explain the declaration-emit limitation motivating the removal. - Trim
@btravstack/httpinternals (syncrpcVerified, simplify auth principal typing, remove some redundant casts) and add “kept surface” TSDoc forHasMarkandHttp.authenticators. - Add a changeset bumping all packages (minor) for the public-surface change.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/http/src/test-fixtures.ts | Makes rpcVerified return synchronously and removes a redundant async wrapper. |
| packages/http/src/principal.ts | Tightens Tagged/Principal typing around scheme keys. |
| packages/http/src/orpc.ts | Expands TSDoc for HasMark to document the external consumer rationale. |
| packages/http/src/define-http.ts | Adds rationale TSDoc for Http.authenticators and simplifies the declared cast. |
| packages/http/src/auth.ts | Removes scheme-port memoization and adds rationale TSDoc for authenticatorPort. |
| packages/http/src/auth.spec.ts | Updates call site for rpcVerified to match the now-sync fixture shape. |
| packages/di/src/module.ts | Removes the exported Unmet alias and updates inline gate commentary accordingly. |
| packages/di/src/index.ts | Stops re-exporting Unmet from the public index. |
| packages/di/CLAUDE.md | Updates DI documentation to remove/replace now-invalid claims about Unmet. |
| .changeset/ponytail-trims.md | Adds a minor changeset for all packages documenting the Unmet removal and related trims. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
88
to
+99
| export const authenticatorPort = <const S extends string>( | ||
| scheme: S, | ||
| ): PortClassOf<`HttpAuthenticator:${S}`, AuthenticatorService<unknown>> => { | ||
| const id = `HttpAuthenticator:${scheme}` as const; | ||
| // Memoised: `defineHttp` asks for a scheme's port when it binds the | ||
| // authenticator and `routerFor` asks again for every scheme its contract | ||
| // names, and two `Port(id)` calls under one id are di's duplicate-id warning. | ||
| const existing = ports.get(id); | ||
| if (existing !== undefined) return existing as never; | ||
| // Not memoised: di identifies a port by its `portId` string and the instance | ||
| // type is branded by the id literal, so two classes minted under one id are | ||
| // the same type and the same lookup — `defineHttp` binding and `routerFor` | ||
| // depending resolve to one provider either way (measured: the suite passes | ||
| // with a fresh class per call). | ||
| // oxlint-disable-next-line typescript/no-extraneous-class -- a port is a phantom token; only a class expression carries the construct signature `PortClassOf` describes | ||
| const minted = class extends Port(id)<AuthenticatorService<unknown>> {}; | ||
| ports.set(id, minted); | ||
| return minted as never; | ||
| return class extends Port(`HttpAuthenticator:${scheme}`)< | ||
| AuthenticatorService<unknown> | ||
| > {} as never; |
…rcised rpcSubstitutedAppOf is a hand-rolled composition providing its own stub on the `user` scheme's port — recomposition, this repo's stated way to swap an adapter, not a second defineHttp registry and not a provider layered over one (di refuses two providers for a port). It reuses verifiedRouter, whose real authenticator depends on a TokenTable; the stub graph never builds either, and the spec proves it by serving a token the real table would refuse. The TSDoc written earlier in this branch said "no in-repo example does" — true for one commit; it now points at the fixture instead.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The code half of the 2026-08-23 audit, re-triaged under the correction that
reshaped it: this is a library, and the examples are scenarios, not its one
user — so "no in-repo caller" is not evidence against an export.
Cut
Unmet(di) — the one surface removal, and not for being unused: itsdocumented purpose was impossible. A shaped module re-declaring the gates
with it hits declaration emit keeping the alias unreduced — TS2883 naming
imported modules' internal ports — which is exactly why every in-repo sugar
already inlined the computation. A third-party sugar hits the same wall.
NeedsGateis unchanged. Thepackages/di/CLAUDE.mdpassage that claimedthe export existed for the sugars now records why it cannot.
portIdand theinstance type is branded by the id literal, so a fresh class per call is the
same type and the same lookup. Measured, not argued: 50/50 and 26/26 green
without it, real servers included; the comment carries the measurement.
declareddouble-cast,Tagged/Principal'stwice-guarded
keyof Schemes, andrpcVerified's async wrapper around asynchronous value (its one caller loses a meaningless
await).Kept, and now saying why
Three exports the audit initially flagged survive the library lens, and each
now carries TSDoc naming the consumer it exists for — so the next audit reads
a stated purpose instead of re-deriving an absence:
HasMark— tooling over a contract; an OpenAPI generator decidingwhether to emit
securityat all is the canonical reader.authenticatorPort— a test composition substituting one scheme'sauthenticator provides its own on this port instead of minting a second
registry.
Http.authenticators— the introspection surface a hand-rolledcomposition or custom sugar reads.
Net
−47/+47 across 9 files (the deletions bought room for the purpose TSDoc, which
is the point: the cut was dead claims, and the additions are live ones).
Gate
All six green from the root:
format --check,lint,typecheck(31/31),knip,test(30/30),build(10/10). Changeset included —minor, allnine, since
Unmet's removal is a (never-usable) public-surface change.