refactor: consolidate duplicated type predicates (isRecord, hasLastUsage) and add shared isDefined/isNonNull - #2407
Merged
Merged
Conversation
jonathanKingston
added
the
ci-full
…ites `.filter((x): x is T => x !== null)` writes the narrowing as an inline type predicate. TypeScript never checks that the annotation follows from the body, so the two can drift apart silently, and an anonymous predicate cannot be tested — 34 such filters across 19 files were the widest unverified claims left with the suppression baseline empty. Adds `isDefined` / `isNonNull` to `@copse/std` (dependency-free, so the extracted packages can use them too) with a test that pins both the runtime behaviour and the narrowing, re-exported as `@shared/nullish.ts` for app code. Every pure nullish filter now calls one of the two. Part of #1332. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`isRecord` had been written 23 times. `@copse/std` is dependency-free specifically so both the app and the extracted packages can import it, so the reason the package copies existed (AGENTS.md's app -> package direction) no longer holds — `hooks-dialects` and `plugin-sdk` already import it from there. Five of the copies dropped the `!Array.isArray` clause and so accepted arrays as records. Every use of those five is `isRecord(v) && typeof v['field'] === '...'`, which an array fails anyway, so the canonical predicate is a narrowing with no behavioural difference. `tool-args-format` and `lm-studio-provider` spelled the same three conditions in a different order. Also records the shared predicates in docs/type-safety.md so the next boundary parser reaches for one instead of writing a 23rd. Part of #1332. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`hasLastUsage` existed five times: one exported and tested in `src/main/services/providers/provider-usage.ts`, three verbatim copies, and a variant in `redacting-provider` that narrowed `LLMProvider` rather than `unknown`. Two of them sit in extracted packages, which cannot import from `src/main`. `LLMProvider` and `ModelUsage` both live in `@copse/llm`, so the predicate belongs there. `ProviderWithUsage.lastUsage` is now `ModelUsage | null` — the shape `llm-complete-text` already declared, and a superset of the two-field shape the other copies used, so no caller loses a field. The existing test moves with it; `src/main/services/providers/provider-usage.ts` goes away and its three importers take the package path directly. Closes #1332. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jonathanKingston
force-pushed
the
claude/issue-1332-1ba353
branch
from
September 6, 2026 09:18
b103294 to
ff232ec
Compare
Contributor
🖥️ PR preview
|
Contributor
Reference screenshots ready for reviewReview GitHub’s image diffs in screenshot PR #2409. Rendered for If this source branch moves, a later successful render closes the stale review PR and replaces this link. |
Screenshot candidates rendered for parent PR #2407 at `ff232eca60c4b6622573d42213899fa91366f6ff` by [CI run 34024303450](https://github.com/copse-dev/agent-pane/actions/runs/34024303450). Review GitHub's image diffs, then merge this PR (or enable auto-merge) to apply the accepted references to `claude/issue-1332-1ba353`. This branch contains only PNG candidates from the immutable `reference-screenshot-candidates-34024303450` artifact and never targets `main`. If the parent branch has advanced beyond the source SHA above, do not merge this PR; the successful CI run for the new head will replace it. Co-authored-by: jonathanKingston <338988+jonathanKingston@users.noreply.github.com>
jonathanKingston
enabled auto-merge (squash)
September 6, 2026 09:59
jonathanKingston
pushed a commit
that referenced
this pull request
Sep 6, 2026
#2407 (the #1332 half of this cluster) landed while this was open. The two do not overlap by design — it took the 26 presence checks this PR deliberately left alone, and this PR took the membership and narrowing ones — so all six conflicts were import lines and one docs section, not logic. Resolutions: - `packages/std/src/index.ts`, `decision-log.ts`, `deferred-approval.ts`, `roadmap-pane.ts` — both imports kept. - `redacting-provider.ts` — main's version wins outright. It moved `hasLastUsage` into `@copse/llm/provider-usage.ts`, which deletes the local copy this PR had converted to the checked form; the shared one is now the only one, and it is in the inventory. - `docs/type-safety.md` — both sections kept, with `memberOf` folded into main's "reach for the shared predicate" list so the two do not read as competing advice. The inventory is regenerated against the merged tree: 183 asserted predicates on main, 118 here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J9mJfbsrvEeTMUdxTKf8xZ
jonathanKingston
added a commit
that referenced
this pull request
Sep 6, 2026
) Closes #1330. **183 hand-written predicates → 118**, and a ratchet so the count can only go one way. Six commits plus a merge, in the order the issue lays the work out. > Rebased onto #2407, which landed while this was open. The two do not overlap by design — that PR took the 26 presence checks this one deliberately left alone; this one took the membership and narrowing ones. All six conflicts were import lines and one docs section, no logic. Base figures below are against `main` **with** #2407. ## 1. The blocker isn't one #1330 says the remedy is unavailable until `@typescript-eslint/explicit-function-return-type` is exempted, because obtaining inference means deleting the return annotation. Measured against this repo's config, in both directions: | | lints? | | --- | --- | | `function isFoo(v: unknown) { return typeof v === 'string' }` | ✗ `explicit-function-return-type` **and** `explicit-module-boundary-types` | | `const isFoo: (v: unknown) => v is Foo = (v) => typeof v === 'string'` | ✓ clean | | `xs.filter((x) => typeof x === 'string')` | ✓ clean | So only the route the issue proposed violates the rule. Moving the annotation to the **binding** keeps the signature written down and gets the check for free — `allowTypedFunctionExpressions` is on by default — and an unannotated arrow in argument position never triggered the rule at all. The annotated form is also the *better* remedy, not just the available one. Inference by deletion degrades silently: a body that stops narrowing yields plain `boolean` and the caller quietly gets a wider type. The annotated form is `TS2677` instead — `return true` fails to compile, which is exactly the case the issue opens with. **No lint config changes in this PR.** ## 2. Membership: 28 assertions → 1 `.includes()` / `.some()` / `.has()` don't narrow, so these can be neither checked nor inferred — the issue's "at least 14 that genuinely need the assertion". They don't; they need to stop being written by hand. ```ts export const isThemePreference = memberOf(THEME_PREFERENCES) ``` `memberOf` (`packages/std/src/member-of.ts`) holds the codebase's single membership `is` assertion. Its test checks the exact contract — `memberOf(list)(value) === list.includes(value)` — over a cross-product of 12 member lists and a hostile corpus (prototype keys, near-misses, symbols, bigints, `NaN`, functions with a matching `toString`), plus compile-time narrowing assertions. Per `docs/type-safety.md` I checked it can fail: a `return true` body turns **6 of its 10 cases red**. Two things fell out: - **`decision-log.ts` and `deferred-approval.ts` spelled their member lists twice** — once as a union, once as a `Set` literal — so the predicate could silently disagree with the type it claimed. The list is now the source and the type derives from it, which makes that drift unrepresentable rather than merely tested. - Three sites keep an explicit annotation because their declared input is narrower than `unknown` (`isCursorPermissionHookEvent`, `isServiceTier`, `isHookDialect`). The factory's return type is checked against it. ## 3. Convert what's cheap — measured, not guessed I converted **all 100** function-declaration predicates to the annotated form and kept only what `tsc` accepted: **18**. That is the honest headline, and it is much less than the issue anticipates. What survives is `instanceof`, `Array.isArray`, `in` over an object union, a discriminant comparison, and a literal-union disjunction. What doesn't is the bulk of this codebase's predicates: **structural boundary parsers**, where indexed access doesn't narrow the object. `isRecord` fails twice over — the negated `Array.isArray` stops inference producing a predicate at all, and even without it the most the compiler concludes is `v is object`, which has no index signature. There is no way to write it that the compiler checks. For the inline `.filter((x): x is T => …)` arrows — the one shape that can *never* satisfy the "exported predicates must be tested" rule, because an anonymous predicate has no test surface — I stripped all 73 non-presence annotations and **diffed the resolved type of every call expression in every touched file**. 24 had moved: - `Boolean(part)` doesn't narrow at all — `(string | undefined)[]`, not `string[]`; - in `cursor-adapter.ts` two filters lost their `wireEvent` narrowing and started reporting a *different* event set, still compiling; - where the input array is `any[]` (a `storageGet` read), the annotation was the only thing pinning the element type, and removing it trips `no-unsafe-*`. So the batch that landed is the 21 sites where **nothing moves**: every call-expression type across the 17 files is byte-identical before and after. The 52 excluded sites keep their annotation and stay in the inventory. ## 4. The ratchet The follow-up comment on #1330 is the real finding: 161 → 212 in four months, with 15 covered. Counting by hand can't keep up, and nothing in the build objects — `no-unsafe-type-assertion` doesn't flag a predicate, and the suppression baseline is empty, so an unverified `x is T` passes every gate we have. `scripts/type-predicate-inventory.test.ts` lists the 118 that remain and fails **both ways**, the same shape as `module-boundaries.test.ts`: a predicate not on the list fails, and a list entry whose predicate is gone fails. Converting one forces its line out in the same change, so the list can only shrink. I verified both directions by adding a predicate and by removing one. Only the **asserted** form is counted — `memberOf(TUPLE)`, an annotated binding and an unannotated `.filter()` arrow are all absent by construction, so the cheapest predicate to add is now also the honest one. That classification is load-bearing, so `scripts/lib/type-predicates.mts` has its own unit test for the ten shapes it has to tell apart. `memberOf`'s own assertion is *in* the list, as are `isDefined` / `isNonNull` from #2407. That is the point: the shared ones are the audited ones, and the list says which they are. ## 5. Docs `docs/type-safety.md` gains the three checked forms, the lint finding above, and a measured table of **what TypeScript can and cannot infer a predicate from** — including that a preceding `const` is fine but an early `return false` is not, and the two traps that make a blanket conversion unsafe. The point is that nobody re-derives this. `memberOf` is folded into #2407's "reach for the shared predicate" list rather than competing with it. ## Numbers Counted by `scripts/lib/type-predicates.mts` over tracked non-test `src/` + `packages/` + `scripts/`, which is why the base is 183 rather than the issue's 212. | | main (incl. #2407) | here | | --- | --- | --- | | asserted predicates | 183 | **118** | | files carrying one | 113 | 74 | | membership predicates written by hand | 28 | 0 | The remaining 118 are overwhelmingly structural boundary parsers. Driving those to zero is a decoder-at-the-boundary job, not a predicate job — `docs/type-safety.md` now says so and points at `safeJsonParse(text, decodeWithSchema(schema))`. ## Verification `pnpm run check` end to end on the merged tree: typecheck (both projects), lint, format, demo-site, dead-code, oracle, e2e-syntax, and **8,587 passing / 0 failing** unit tests. No visual eval. The seven renderer files touched are predicate-form substitutions with identical runtime semantics and unchanged DOM — `main.ts`'s `Set.has` and `memberOf`'s internal `Set.has` are the same call, and every other change is type-level. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01J9mJfbsrvEeTMUdxTKf8xZ --------- Co-authored-by: Claude <noreply@anthropic.com>
jonathanKingston
pushed a commit
that referenced
this pull request
Sep 6, 2026
`thread-container.ts` carried its own `isRecord`, byte-identical to the one #2407 consolidated into `@shared/unknown-value.ts`. #2419 then made the type predicate inventory shrink-only, so the duplicate registered as a new unlisted predicate and failed `check` on the merge with main. The file already imports through `@shared`, so the alias resolves on every path this module is built for, the guest bundle included. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NzkY3kYdcQvYk8EH3EW8uv
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
1 participant
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.
Closes #1332.
Three mechanical passes, one commit each, in the order the issue suggests.
1.
isDefined/isNonNull— the highest-leverage additionThere was no shared presence predicate. 34 sites across 19 files wrote one inline:
TypeScript never checks that
change is GitChangefollows from the body, so the annotation and the condition can drift apart silently, and an anonymous predicate cannot be tested. Both helpers now live inpackages/std/src/nullish.ts— dependency-free, so the extracted packages can use them too — re-exported as@shared/nullish.tsfor app code, with a test that pins the runtime behaviour and the narrowing (const nonNull: number[] = values.filter(isNonNull)only compiles if the predicate is right). Perdocs/type-safety.md, the test was checked against areturn truebody: all 6 go red.The three call sites with an extra clause (
x !== undefined && x.trim().length > 0) were left alone — they are not presence checks.2.
isRecord— 22 local copies deletedThe issue scoped this to "~6 in
src/**, not 10", on the grounds that thepackages/**copies cannot import from@shared. That caveat is now stale:@copse/stdexists precisely as the dependency-free leaf both sides share, andhooks-dialects/plugin-sdkalready importisRecordfrom it. So all 22 go, leaving one definition.Two behavioural notes:
!Array.isArray(value)and so accepted arrays as records (stream-retry,spine-schema, and three test files). Every use of those five isisRecord(v) && typeof v['field'] === '…', which an array fails anyway — the canonical predicate is a narrowing with no reachable behaviour change.tool-args-format(!!value && …) andlm-studio-provider(reordered clauses) spelled the identical three conditions differently.Also adds a short section to
docs/type-safety.mdpointing at the shared predicates, so the next boundary parser reaches for one instead of writing a 23rd.3.
hasLastUsage— five copies into@copse/llmOne was exported and tested in
src/main/services/providers/provider-usage.ts; three were verbatim copies;redacting-providerhad a variant narrowingLLMProviderrather thanunknown. Two live in extracted packages and so could not import the tested one.LLMProviderandModelUsageboth live in@copse/llm, so the predicate moved there with its test.ProviderWithUsage.lastUsageis nowModelUsage | null— the shapellm-complete-textalready declared, and a superset of the two-field shape the others used, so no caller loses a field.src/main/services/providers/provider-usage.tsis gone; its three importers take the package path.Out of scope
Step 4 of the issue ("revisit #1330's export question against whatever is left") is a hand-off to #1330, which stays open.
Verification
pnpm run check— typecheck (both projects), lint, format, dead-code, oracle, e2e-syntax, demo-site, and the 8556-test unit suite (0 failures).No visual eval: the four renderer files touched (
tool-args-format,model-options,composer-editor,roadmap-pane) are predicate substitutions with identical results and unchanged DOM.🤖 Generated with Claude Code