fix(skills): stop teaching the removed LoggerBrowser, and fix the calls it left behind - #401
Merged
Merged
Conversation
…ls it left behind `skills/b24jssdk-core/SKILL.md` and `skills/b24jssdk-recipes/SKILL.md` presented `LoggerBrowser.build(...)` as the way to obtain a logger. That class is tagged `@removed 3.0.0` (#277). These files are what an agent reads before writing code, so our own instructions were producing deprecated calls today and code that breaks at the next major. Swapping the constructor is not enough, which is the part worth reading twice. `LoggerBrowser` took variadic arguments (`info(...params: any[])`); `LoggerInterface` takes `(message: string, context?: Record<string, any>)`, and has `warning`, not `warn`. A mechanical rename leaves `logger.info('Hello,', name)` compiling — an array or an Error satisfies `Record<string, any>` structurally — while the second value is silently reshaped or dropped. Two shipped recipes already had exactly that, so this is a bug fix and not only a deprecation sweep: - `12-oauth-install.ts` logged `logger.error('install failed', e)`. An Error's message and stack are not own enumerable properties, so the context serialised as `{}` and the failure vanished from the log — in the handler for a Bitrix24 install callback, where losing the reason is expensive. - `10-error-handling.ts` passed `res.getErrorMessages()` as the context, landing the array as `{ '0': …, '1': … }`. Node and browser now get different guidance rather than one wrong answer: `LoggerFactory.createForBrowser` in the frame example, and an explicit `ConsoleV2Handler(..., { useStyles: false })` for the backend and recipe snippets, because the browser factory emits CSS styling a terminal prints as literal noise. Also `$b24.setLogger(logger)` rather than `setLogger?.()` — it is a required member of `TypeB24`, so the optional call implied it might be absent. Guarded by three table-driven checks across both SKILL files and every recipe: no `LoggerBrowser`/`LoggerType`, no `logger.warn(`, and every logger call's second argument is an object literal. Each verified by mutation. The third guard needed a scanner rather than a regex, and the first attempt is worth recording: matching `logger.<level>(\s*[^,)]+,` treats the first comma it meets as the argument separator, but most of these calls pass a template literal whose text contains commas — ten files failed on their own log messages. The second attempt then broke on a template literal nested inside `${}` in `05-disk-files.ts`. It now tracks quote and interpolation state with a stack. That is the same defect #139 item 2 reported in the docs code transform, found again while writing the guard against it. Refs #277 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F22e2ft66y7nuBJjzdThBr
…scanner to an ESLint rule The review was right that the scanner was the wrong layer. `eslint-rules/` already holds two hand-written AST rules for logger callsites (`no-credential-in-logger`, `require-catch-on-logger-call`), and this is a third of the same kind. Working on the AST also removes a class of defect the scanner demonstrably had: a regex literal containing a comma or a quote broke argument splitting, silently, and no amount of care in a hand-written lexer makes that impossible. The rule is scoped to `skills/b24jssdk-recipes/**` only. It demands an object literal at the callsite, which is a teaching convention rather than a correctness rule — hoisting the context into a variable is perfectly correct code, and the SDK's own deprecated `LoggerBrowser` passthroughs are written that way. It earns its place in files shipped for an agent to copy, where the literal is what makes the parameter's meaning visible at the point of copying. Enabling it repo-wide reported twelve false positives in `logger/browser.ts` alone. The two remaining guards stay in the spec, because ESLint cannot see inside a Markdown fence and `SKILL.md` is where the deprecated logger was being taught. They now DISCOVER the skill files instead of naming three by hand: the tree has seven `SKILL.md` and four of them were unguarded, so a new skill could have taught `LoggerBrowser` and passed. Verified by planting one in `b24jssdk-helpers`, which the old list did not cover and the new sweep fails on. Also from the review: - Both SKILL files now say why `requestInfo` is safe to log — `AjaxError`'s constructor redacts it, so the safety is an SDK contract and not caller discipline. Without that, an agent may rebuild the same context by hand from the original params and put a live credential in a log. - `12-oauth-install.ts` records that logging `message`/`stack` there is safe only while the credential store throws plain filesystem errors. - Refreshed the `audited:` stamp on `79.security.md`, which cites the recipe this PR modifies. Filed #402 for the gap both mechanisms leave: a wrong call SHAPE inside a `SKILL.md` fence is caught by neither, and compiling skill fences the way `docs:typecheck-blocks` compiles the docs site would close it for every deprecated symbol at once rather than one substring guard at a time. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F22e2ft66y7nuBJjzdThBr
…e guards
The rule shipped without a spec while both of its siblings in `eslint-rules/`
have one — and its own doc comment carefully enumerates edge cases that nothing
was pinning. Those distinctions were exercised only by whichever recipe happened
to hit each branch, so the coverage would erode silently. Now the same shape as
the sibling specs: the real rule module through ESLint's `Linter` over known-bad
and known-good snippets, plus an assertion that `eslint.config.mjs` actually
wires it — including that it is NOT applied to the SDK source, which is a
deliberate scope decision rather than an omission.
Two gaps found by review, both latent rather than live:
Skill discovery skipped a symlinked directory. `Dirent.isDirectory()` reports
the dirent's own type, so it is false for a symlink even when the link points at
a directory — a symlinked skill could have taught `LoggerBrowser` and passed,
which is precisely the blind spot the discovery replaced. Verified: `linked
isDirectory=false isSymlink=true`.
The rule exempted an explicit `undefined` as "deliberately no context". It
cannot tell that from a context the author meant to build and forgot, and
`logger.info('m')` already says "no context" unambiguously — so the exemption
was both unnecessary and inconsistent with rejecting a known-good identifier.
Removed.
Also softened the discovery count from a hard floor of seven to non-empty.
Pinning today's headcount would fail on a legitimate skill removal for a reason
unrelated to logger hygiene; discovery already makes a stale list impossible, so
the check only needs to rule out an empty sweep.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F22e2ft66y7nuBJjzdThBr
7 tasks
IgorShevchik
pushed a commit
that referenced
this pull request
Aug 27, 2026
…at found `docs:typecheck-blocks` has compiled the fences under `docs/content/**` since #109 — 155 blocks, a CI gate. Nothing equivalent covered `skills/*/SKILL.md`, though those are what an AI agent reads BEFORE writing code: a broken snippet there is not a page someone might misread, it is a template that gets reproduced. #401 is the proof — the skills taught `LoggerBrowser`, removed in 3.0.0, for months, and hand review never caught it. This adds `skills:typecheck-blocks` over 68 fences, sharing the engine with the docs gate rather than copying it: extraction, `// @check-ignore`, the mapping of a tsc diagnostic back to `file:line:col`, and the GitHub annotation escaping now live once in `_typecheck-blocks.mjs`. `docs-typecheck` still reports 155 blocks, 0 errors — behaviour unchanged. It also generalises where the current guards do not. The substring checks in `recipe-hygiene.unit.spec.ts` catch one named symbol each, so every deprecation needs a new guard; a compiler covers the whole surface at once, which matters for #277 and its 22-symbol removal list. WHAT THE FIRST RUN FOUND, on files reviewed by hand many times: - `B24HelperManager` is not exported. The documentation gives it its own page, the helpers skill teaches constructing it directly for backend code, and `import { B24HelperManager } from '@bitrix24/b24jssdk'` resolved to nothing — confirmed `undefined` in the built bundle. Now exported (additive; the #383 reference gate correctly demanded the row, 96 -> 97). - `helper.license` / `helper.payment` do not exist — they are `licenseInfo` / `paymentInfo`. The skill's own frontmatter advertises license and payment. - `destroyB24Helper` imported from the package root; it is a member of `useB24Helper()`. - `Text.toB24Format(...)` resolving to the DOM `Text`, in two files, because the fence never imported the SDK's `Text`. Silent: the name exists, so nothing complained until a method was called on it. - `selectAccess({})` where the parameter is `string[]`; CRM ids written as `'D_42'` where the type is `number[]`; `getJsonObject<T>()`, which takes no type argument; a `call.make()` with no type argument reading `.item` off `unknown`. - Fragments that were not TypeScript at all — `filter: { … }` as a statement, a try/catch `return`ing outside any function. Made complete rather than ignored: `const params = { filter: { … } }` also shows the reader where `filter` lives. The 24 missing imports were inserted by a throwaway script driven by the gate's own output, not by hand. Two things about the harness worth knowing: `.skills-typecheck/globals.d.ts` never declares a real SDK export. If a fence uses `Text` or `AjaxResult`, it must import it — an agent copying the snippet needs that line, and an ambient would make the gate certify code that does not run. Placeholders belonging to the reader's own application, and framework globals that really are auto-imported, are declared there. The header says so. An empty sweep is an ERROR, not a pass. A glob that stops matching would otherwise report "0 errors" and read as healthy. Also fixes a test that could not fail: `docs-typecheck.test.mjs` carried an inline re-implementation of `extractTsBlocks`, with a comment claiming it "mirrors the production logic exactly". Sixteen tests were therefore passing against a copy, so a bug in the real extractor could not redden any of them. It is exported now, and the tests bind to it — verified by breaking the real extractor, which fails 9 of them and previously failed none. Closes #402 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F22e2ft66y7nuBJjzdThBr
IgorShevchik
added a commit
that referenced
this pull request
Aug 27, 2026
…HelperManager (#404) Added: `skills:typecheck-blocks`, a CI gate compiling every ```ts fence in `skills/*/SKILL.md` — 68 blocks — the way `docs:typecheck-blocks` has guarded the documentation site since #109. Skill files are what an AI agent reads before writing code, so a broken snippet there is not a page someone might misread, it is a template that gets reproduced; #401 found them teaching a class removed in 3.0.0, for months, past repeated hand review. Added: `B24HelperManager` is now exported from the package root. It already had its own documentation page and the helpers skill already taught importing it, but the import resolved to `undefined` in the built bundle. Exporting it rather than deleting the documentation was a deliberate call — the class is shaped as public API (a constructor taking `TypeB24`, `setLogger`, getters that throw named errors before init) — and it commits 3.0.0 to treating one more class as already-public. Fixed: the inaccuracies that first run found, on files reviewed by hand many times. `helper.license` / `helper.payment` do not exist — the getters are `licenseInfo` / `paymentInfo`, and the skill's own frontmatter advertised the wrong names. `destroyB24Helper` imported from the package root when it is a member of `useB24Helper()`. `Text.toB24Format(...)` silently resolving to the DOM `Text` in two files, because the fence never imported the SDK's. `selectAccess({})` where the parameter is a `string[]`; CRM ids written as `'D_42'` where the type is `number[]`; a type argument `getJsonObject` does not accept; a `call.make()` with no type argument reading `.item` off `unknown`. Plus fences that were not valid TypeScript at all, made complete rather than ignored. Changed: `docs-lint` treats an uncommitted — or gitignored — cited source as modified now, instead of reading committed history alone. The old behaviour passed locally and then failed in CI on the same change, twice, because a source modified but not yet committed still reported its old date. The repository status is now read once per run rather than once per cited link. Also: `docs-typecheck.test.mjs` carried an inline re-implementation of the block extractor, so sixteen tests were passing against a copy and a bug in the real one could not redden any of them. The extractor is shared and exported now, and the tests bind to it. Closes #402
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
2 participants
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.
Found during the #277 analysis. Non-breaking, ships in
2.x, and shrinks the blast radius before the 3.0.0 removal PR is written.The problem
skills/b24jssdk-core/SKILL.md(3 sites) andskills/b24jssdk-recipes/SKILL.md(2 sites) presentedLoggerBrowser.build(...)as the way to obtain a logger. That class is tagged@removed 3.0.0.These files are what an AI agent reads before writing code. So our own instructions were emitting a deprecated API today, and producing code that breaks at the next major — the opposite of what a skill file is for.
Swapping the constructor is not enough
This is the part worth reading twice, and the reason this PR is larger than "five find-and-replaces".
LoggerBrowser(removed)LoggerInterface(current)info(...params: any[])info(message: string, context?: Record<string, any>)warn()warning()A mechanical rename leaves
logger.info('Hello,', name)compiling — an array or an Error satisfiesRecord<string, any>structurally — while the second value is silently reshaped or dropped.Two shipped recipes already had exactly that
So this is a bug fix, not only a deprecation sweep:
12-oauth-install.tsloggedlogger.error('install failed', e). An Error'smessageandstackare not own enumerable properties, so the context serialised as{}and the failure vanished from the log — in the handler for a Bitrix24 install callback, where losing the reason is expensive.10-error-handling.tspassedres.getErrorMessages()as the context, landing the array as{ '0': …, '1': … }.Neither is caught by
skills:typecheck, which is why they had survived.Node and browser now get different guidance
Previously one answer, wrong for one of the two:
LoggerFactory.createForBrowser('App', isDev), the documented migration target.Logger.create(name)plus an explicitConsoleV2Handler(level, { useStyles: false }), because the browser factory emits CSS styling that a terminal prints as literal noise. This is also what recipe01-crm-analytics.tsalready did correctly, so the skills now match a working example rather than contradicting it.Also
$b24.setLogger(logger)rather thansetLogger?.(logger)— it is a required member ofTypeB24, and the optional call implied it might be absent.Guards
Three table-driven checks across both SKILL files,
b24jssdk-rest/SKILL.md, and every recipe: noLoggerBrowser/LoggerType, nologger.warn(, and every logger call's second argument is an object literal. Each verified by mutation — reintroducing the old constructor, the old level name, and the originallogger.error('install failed', e)each fail the suite.The third guard needed a scanner, and the first two attempts were wrong
Worth recording, because it is the same defect twice:
logger.<level>\(\s*[^,)]+,treats the first comma it meets as the argument separator — but most of these calls pass a template literal whose text contains commas. Ten files failed on their own log messages.`(${it.TYPE}${it.SIZE ? `, ${it.SIZE} bytes` : ''})`in05-disk-files.ts— a template literal holding an interpolation holding another template literal holding a comma. The flat flag closed the outer template on the inner backtick.It now tracks quote and interpolation state with a stack. That is precisely what #139 item 2 reported about the docs code transform — counting delimiters without tracking whether you are inside a string — found again while writing the guard against it.
Checks
pnpm run typecheck— all eight passes, 0 errorsskills:typecheck,skills:unit(184 tests),jsSdk:unit+jsSdk:typeslint(1 pre-existing unrelated warning indocs/server/api/ai.post.ts),lint:md,docs-lint --strict,check-v3-method-refs,md-internal-linksRefs #277
🤖 Generated with Claude Code
https://claude.ai/code/session_01F22e2ft66y7nuBJjzdThBr
Generated by Claude Code