Skip to content

fix(services): two published .d.ts JSDoc comments stop describing behaviour their code does not have - #9635

Merged
os-project-manager merged 1 commit into
mainfrom
claude/issue-9611-service-jsdoc-truth
Aug 18, 2026
Merged

fix(services): two published .d.ts JSDoc comments stop describing behaviour their code does not have#9635
os-project-manager merged 1 commit into
mainfrom
claude/issue-9611-service-jsdoc-truth

Conversation

@os-project-manager

Copy link
Copy Markdown
Collaborator

Fixes#9611

Both comments are emitted by tsup into their package's built index.d.ts, so they are the editor tooltip an npm consumer sees — the same "published documentation asserting behaviour the runtime does not have" class as #9517 and #9532, in a third channel no gate reads. Neither package's behaviour changes.

Premise verified before implementing

Both claims re-measured against origin/main (ba0a84685) rather than taken from the card:

  • MemoryCacheAdapter.get() increments counters and returns entry.value — it does not delete-and-re-set the key, so a read does not move an entry back. The card's stop-condition ("if get() does re-insert on current main, the premise is false") did not fire.
  • set()'s overflow path takes this.store.keys().next().value — the first key in Map insertion order.
  • this.recordRuns = args.options?.recordRuns ?? true — boolean, default true, gating whether a sys_job_run row is written at all.

1. MemoryCacheAdapter — "LRU-style eviction" was never LRU

The class comment advertised TTL-based expiry and LRU-style eviction. Eviction has always been oldest-inserted (FIFO): neither a read nor an overwrite moves an entry back, the latter because Map.set on a key already present keeps its original insertion slot. Under a maxSize cap that is a materially different hit-rate profile from the one the tooltip promised anyone sizing a cache.

The comment is corrected rather than the code, per the ruling on the card: maxSize defaults to 0 (unlimited) so the eviction path is off by default and nothing shipped is getting FIFO where it expected LRU; there is no measured pull for LRU; and minting a real behaviour change to make a stale sentence true inverts the fix — the defect is that the documentation lies, not that the cache is wrong. A real LRU already exists in the repo (packages/metadata/src/utils/lru-cache.ts) for callers that need one.

The pin, and why each case discriminates

Four tests now pin the corrected sentence so it stops being an unenforced claim. Three of them are written to fail against a hypothetical LRU implementation: each reads or overwrites the oldest entry before overflowing the cache, then asserts that entry was evicted anyway while an untouched newer key survived. The fourth pins the maxSize: 0 default the comment also asserts.

Reverse verification — direction predicted before running, and observed: with a real LRU spliced into the working tree (promote-on-read in get(), promote-on-write in set()), the three discriminators go red and the pre-existing eviction tests stay green:

 × a read does NOT refresh eviction order — the read-hot oldest entry is still evicted
× an overwrite does NOT refresh eviction order either — Map.set keeps the original slot
× evicts strictly in insertion order across a longer run, reads notwithstanding
Tests 3 failed | 23 passed (26)

That second half is the load-bearing part: the two eviction tests that already existed pass under both policies, which is exactly how a comment contradicting them sat green. The ablation was applied to the committed state and restored with git checkout claude/issue-9611-service-jsdoc-truth -- ...; no ablation marker remains (grep -c ABLATION = 0) and the working tree is clean at the head below.

2. DbJobAdapterOptions.recordRuns — the comment described a different field

/** Soft cap on sys_job_run rows recorded per job (defaults to none — handled by retention jobs) */ made three claims and the code contradicts all three: a boolean not a count, defaulting to true not "none", gating whether the row is written at all rather than being trimmed later. The wording reads as if it belongs to the numeric JobRunRetention knob that ADR-0057 retired.

The replacement keeps the sharp consequence in sight — a reader who sets recordRuns: false expecting "no cap" gets run history switched off — and names the two things the flag does not affect, both measured rather than assumed: the sys_job row's own last_status / run_count / failure_count counters (bumpJob is called unconditionally in settle), and replay(), which writes its synthetic trigger: 'replay' row without consulting the flag.

Acceptance: checked in the emitted artifact, not the source

Rebuilt both packages and read the published declarations. In service-job/dist/index.d.ts and its .d.cts twin, grep -c 'Soft cap' = 0. In service-cache/dist/index.d.ts and its twin, the single surviving occurrence of LRU is the corrected sentence's own negation:

21: * entry. This is deliberately **not** LRU — neither reading an entry nor

Neither comment survives in the built .d.ts in a form the code contradicts.

Changeset: owed, and why

.changeset/service-jsdoc-declared-equals-actual.md, patch on both packages. AGENTS.md exempts pure bug fixes, but this fix's entire deliverable is text inside two published packages' .d.ts — with no version bump the corrected tooltip never reaches npm and the card's acceptance is unmet in the only channel it is about. Not breaking, so no ADR-0087 marker is required (check-adr-0087-registration confirms: "this PR adds no declared-breaking changeset").

Verification — union run at 9b07a5e34, the final commit

Derived with node scripts/pm/dispatch-gates.mjs against the actual changed paths from git merge-base (ba0a84685), then re-derived after the last commit; the set was unchanged. All at the head above, working tree clean.

gateresult
pnpm --filter @objectstack/service-cache --filter @objectstack/service-job test26 passed (3 files) · 75 passed (8 files)
pnpm --filter ... typecheck (both)tsc --noEmit clean
pnpm check:nul-bytesOK — 6174 files, no raw control bytes
pnpm check:changeset-gate-self-testsOK
pnpm check:objectui-changesetOK
pnpm check:test-source-aliasOK — 72 packages
pnpm check:type-source-resolutionOK — 76 packages
node scripts/check-adr-0087-registration.mjsOK — no declared-breaking changeset
node scripts/check-changeset-no-major.mjsOK
node scripts/check-empty-changeset.mjsOK — 1 declaring changeset
node scripts/docs-audit/check-affected-docs.mjsOK — 242 self-test cases
pnpm check:query-options-erasureratchet holds, none new
pnpm check:engine-double-contractOK — 319 pinned, no new double
pnpm check:where-matcherOK — 255 matchers, none new
pnpm check:type-check-coverageOK — 64/77 packages
pnpm check:type-check-debt --re-measureOK — 33 entries re-measured, none above its number

The ratchet's re-measure needs the built workspace closure, so turbo run build --filter=./packages/* --filter=./packages/*/* was run first (70/70 successful). Every heavy step ran under flock /tmp/os-heavy-verify.lock.

Out of scope, filed rather than absorbed

The card's scope fence is two comments and the pinning test. Two findings of the same class were left untouched and filed unassigned:

Neither is addressed by this PR.


Generated by Claude Code

…aviour their code does not have (#9611)
Both comments are emitted by tsup into their package's built `index.d.ts`, so
they are the editor tooltip an npm consumer sees — the same "published
documentation asserting behaviour the runtime does not have" class as #9517 and
#9532, in a third channel no gate reads. Neither package's behaviour changes.
1. `MemoryCacheAdapter` advertised "LRU-style eviction". The eviction path takes
the first key in `Map` insertion order and `get()` returns `entry.value`
without ever deleting and re-setting the key, so a read does not move an
entry back; nor does an overwrite, since `Map.set` on a present key keeps its
original slot. Eviction has always been oldest-inserted (FIFO). The comment
is corrected rather than the code: `maxSize` defaults to 0 (unlimited) so the
eviction path is off by default, there is no measured pull for LRU, and
minting a behaviour change to make a stale sentence true inverts the fix.
Four tests now pin the corrected sentence. Each is a discriminator against
LRU: it reads or overwrites the oldest entry before overflowing the cache,
then asserts that entry was evicted anyway while an untouched newer key
survived. The pre-existing eviction tests could not tell the policies apart,
which is how the wrong comment sat green.
2. `DbJobAdapterOptions.recordRuns` was documented as "Soft cap on sys_job_run
rows recorded per job (defaults to none — handled by retention jobs)" — a
count, defaulting to none, trimmed later. The code has a `boolean` defaulting
to `true` that gates whether the row is written at all. The wording looks
copied from the numeric `JobRunRetention` knob ADR-0057 retired. The
replacement states the real meaning and keeps the sharp consequence in sight:
a reader who sets `recordRuns: false` expecting "no cap" switches run history
off. It also names the two things the flag does not affect — the `sys_job`
row's own counters, and `replay()`, which writes its synthetic row regardless.
Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Y26DJEHSBhhAQ6wwfsHNza
@github-actionsgithub-actionsBot added size/m documentation Improvements or additions to documentation tests tooling labels Aug 18, 2026
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

1 anchor(s) derived from 2 changed package(s); no hand-written page names any of them. ✅

What this run could not see
  • 1 changed file(s) yielded no anchor (packages/services/service-cache/src/memory-cache-adapter.ts) — pages documenting those are invisible to this run

Coarse fallback — 5 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 40162f1e2cf386d5c40b09fdfd47a71640df2df9packageMentionDocs.

Which tree this was computed on

This run read content/docs from 876b32264a45eac7652d30d50af8d949c0b93384 — the merge of head 9b07a5e347acded4182a2f145ff8c98e80371562 into base 40162f1e2cf386d5c40b09fdfd47a71640df2df9, 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 876b32264a45eac7652d30d50af8d949c0b93384 && git checkout 876b32264a45eac7652d30d50af8d949c0b93384
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 40162f1e2cf386d5c40b09fdfd47a71640df2df9 9b07a5e347acded4182a2f145ff8c98e80371562 && git checkout -B drift-repro 40162f1e2cf386d5c40b09fdfd47a71640df2df9 && git merge --no-ff 9b07a5e347acded4182a2f145ff8c98e80371562
node scripts/docs-audit/affected-docs.mjs --json 40162f1e2cf386d5c40b09fdfd47a71640df2df9

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

@os-project-manager
os-project-manager marked this pull request as ready for review August 18, 2026 14:16
@os-project-manager
os-project-manager added this pull request to the merge queueAug 18, 2026
Merged via the queue into main with commit c07d6e8Aug 18, 2026
26 checks passed
@os-project-manager
os-project-manager deleted the claude/issue-9611-service-jsdoc-truth branch August 18, 2026 14:42
os-project-manager pushed a commit that referenced this pull request Aug 18, 2026
… landed
Merged rather than rebased: rebasing a pushed branch needs a force-push, which
the dev contract forbids outright. Same outcome, no forced history.
Both blockers are now on main (#9635 at c07d6e8, #9646 at 73010f1), so the
three points this card was holding are resolved here:
1. Field JSDoc — the clause naming replay as an exception is gone. Not a
mechanical delete: it left "Two things are unaffected either way" counting
wrong, so the sentence is rewritten to say `false` writes no rows at all,
replay included, with the sys_job counters as the one exception.
2. Class JSDoc — the "one row it does not govern" sentence is gone. The bullet
keeps its single {@link DbJobAdapterOptions.recordRuns} pointer rather than
paraphrasing the flag a second time, per the constraint #9631 was built on.
3. #9646's fifth test case, which pinned "replay writes anyway", is deleted.
It carried a comment saying it would change together with the class JSDoc if
this card ruled the carve-out shut. It did. A note stands where it was.
Test-file conflict was add/add — both describe blocks appended at the end. Both
are kept; they cover different paths (#9631's the wrap() path, this card's the
replay() path).
Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Y26DJEHSBhhAQ6wwfsHNza
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-project-manager@claude