feat(compiler): SYN017 — warn on Notification() construction that bypasses the capability model (?bs 0.7+)#164
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new compiler syntax warning code SYN017 to flag direct Notification usage inside function bodies in ?bs 0.7+, because browser notifications are user-visible side effects that are not represented in botscript’s capability model. This also propagates through the error-code registry, MCP explain output, tests, and docs so tools can surface consistent guidance.
Changes:
- Register
SYN017in the compiler error-code registry and MCP explanations, including example fails/passes guidance. - Implement
SYN017detection in the compiler syn-check pass with suppression insideunsafeblocks /unsafe "…" fnbodies and exclusions matching existing SYN check patterns. - Add a dedicated
syn017-checktest suite and update existing allowlists/known-code lists and docs to includeSYN017.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Adds SYN017 to the set of stable diagnostic codes supported by the MCP explain tool. |
| packages/mcp/tests/server.test.ts | Updates MCP test expectations to include SYN017 as a known explanation code. |
| packages/mcp/src/explanations.ts | Adds the long-form MCP explanation entry for SYN017 with fails/passes examples. |
| packages/compiler/tests/syn017-check.test.ts | New test file covering SYN017 detection, exclusions, suppression, and message/severity. |
| packages/compiler/tests/error-codes.test.ts | Extends the exhaustive allowlist to include SYN017. |
| packages/compiler/src/passes/syn-check.ts | Implements the SYN017 token-scan detection and adds inline docs for the new check. |
| packages/compiler/src/error-codes.ts | Registers the SYN017 error-code entry (rule/idiom/rewrite/example) for bs explain. |
| AGENTS.md | Documents SYN017 in the agent-facing diagnostics table. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+60
to
+61
| * SYN017 A `new Notification(title)`, `Notification(title)`, or TypeScript instantiation | ||
| * form `new Notification<T>(title)` was detected in a fn body (?bs 0.7+). |
Comment on lines
+783
to
+787
| rule: | ||
| "`new Notification(title)` and bare `Notification(title)` calls create user-visible browser " + | ||
| "notifications at runtime — a side effect entirely invisible to botscript's capability model. " + | ||
| "No `uses {}`, `reads {}`, or `writes {}` declaration covers notification dispatch: callers " + | ||
| "cannot observe, audit, or suppress the UI effect from the fn's declared surface.", |
Comment on lines
+777
to
+785
| const hasNew17 = prev17 && prev17.kind === "ident" && prev17.text === "new"; | ||
| // Ternary guard: `cond ? new Notification(title) : other` | ||
| const prevBeforeNew17 = hasNew17 | ||
| ? tokens[prevSignificant(tokens, prevIdx17 - 1)] | ||
| : undefined; | ||
| const isTernaryConsequent17 = | ||
| (prev17 !== undefined && prev17 !== null && prev17.kind === "question") || | ||
| (prevBeforeNew17 !== undefined && prevBeforeNew17 !== null && prevBeforeNew17.kind === "question"); | ||
|
|
marcelofarias
force-pushed
the
botkowski/syn017-notification-bypass
branch
from
32bd6f5 to
af21de7
Compare
Comment on lines
+834
to
+838
| // Exclude: function/fn declarations named Notification | ||
| if (prev17 && prev17.kind === "ident" && prev17.text === "function") continue; | ||
| if (prev17 && prev17.kind === "keyword" && prev17.text === "fn") continue; | ||
|
|
||
| const hasNew17 = prev17 && prev17.kind === "ident" && prev17.text === "new"; |
Comment on lines
+837
to
+842
| // Generator: `function* Notification` — prev token is `*`, token before that is `function` | ||
| if (prev17 && prev17.kind === "punct" && prev17.text === "*") { | ||
| const prevPrevIdx17 = prevSignificant(tokens, prevIdx17 - 1); | ||
| const prevPrev17 = tokens[prevPrevIdx17]; | ||
| if (prevPrev17 && prevPrev17.kind === "ident" && prevPrev17.text === "function") continue; | ||
| } |
marcelofarias
force-pushed
the
botkowski/syn017-notification-bypass
branch
from
400a3be to
fa2bfe4
Compare
Comment on lines
+1035
to
+1046
| // Exclude method shorthands and TS method signatures. | ||
| if (callTok17.matchedAt !== undefined) { | ||
| const afterCloseIdx17 = nextSignificant(tokens, callTok17.matchedAt + 1); | ||
| const afterClose17 = tokens[afterCloseIdx17]; | ||
| if (afterClose17 && ( | ||
| (afterClose17.kind === "open" && afterClose17.text === "{") || | ||
| afterClose17.kind === "fatArrow" || | ||
| (!isTernaryConsequent17 && afterClose17.kind === "punct" && afterClose17.text === ":") | ||
| )) continue; | ||
| // Exclude TS method signatures with omitted return type or optional params: | ||
| // `{ Notification(title: string) }` / `{ Notification(title?: string) }`. | ||
| let hasTypeAnnotation17 = false; |
…, options) Copilot flagged the message suggesting `Notification?.(title, options)` as misleading — the original call may not have an options argument, and the diagnostic fires on Notification(title) too. Use `(...)` as a neutral placeholder, consistent with other SYN warnings. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…mple
Two fixes:
1. Add test for type T = { Notification() } — empty parens in a type literal
context was already excluded by the code at 1397-1424 (the prevOpen === '='
guard), but had no explicit test coverage. Copilot flagged the gap.
2. Fix the MCP explanations fails example: { body } shorthand inside the
Notification options object was hitting a compiler assertion. Simplified
to new Notification(title) which compiles cleanly and still demonstrates
the diagnostic.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ry idiom Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
marcelofarias
force-pushed
the
botkowski/syn017-notification-bypass
branch
from
86fc698 to
0624e63
Compare
There was a problem hiding this comment.
⚠️ Not ready to approve
The new SYN017 TS type-literal exclusion heuristic is too narrow and can produce false-positive warnings in valid type-literal contexts.
Copilot's findings
- Files reviewed: 8/8 changed files
- Comments generated: 1
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
Comment on lines
+1459
to
+1464
| const firstInsideBraceIdx17 = nextSignificant(tokens, openBraceIdx17 + 1); | ||
| const isNotificationFirst17 = firstInsideBraceIdx17 === i; | ||
| if (isNotificationFirst17 && prevOpen17 && ( | ||
| prevOpen17.kind === "eq" || // type T = { ... } | ||
| (prevOpen17.kind === "punct" && prevOpen17.text === ":") // x: { ... } | ||
| )) continue; |
…non-first members
type T = { x: string; Notification() } was incorrectly producing a SYN017
warning because the exclusion only fired when Notification was the first
significant token inside the enclosing type-literal brace.
Fix: expand the member-position check to also match when the token immediately
preceding Notification is a ';' or ',' separator (subsequent type members).
Gate the whole block on isEmptyParens17 so non-empty calls like
Notification('title') in value objects are unaffected.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
⚠️ Not ready to approve
There are verified inconsistencies/edge-case gaps in the new SYN017 implementation/documentation that should be resolved to match the PR’s stated exclusions and keep registry metadata consistent.
Copilot's findings
- Files reviewed: 8/8 changed files
- Comments generated: 2
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
Comment on lines
+1443
to
+1472
| // Conditions: (a) parens are empty (no significant tokens between `(` and `)`), | ||
| // (b) enclosing `{` is in a type context (preceded by `=` or `:`), | ||
| // (c) `Notification` is at a method-signature position: either the first | ||
| // significant token inside the `{`, or preceded by `;` / `,` | ||
| // (subsequent type member, e.g. `{ x: string; Notification() }`). | ||
| { | ||
| const isEmptyParens17 = nextSignificant(tokens, callIdx17 + 1) >= (callTok17.matchedAt as number); | ||
| if (isEmptyParens17) { | ||
| let closeBrace17 = afterClose17; | ||
| if (closeBrace17 && | ||
| closeBrace17.kind === "punct" && | ||
| (closeBrace17.text === ";" || closeBrace17.text === ",")) { | ||
| const nextAfterSepIdx17 = nextSignificant(tokens, afterCloseIdx17 + 1); | ||
| closeBrace17 = tokens[nextAfterSepIdx17]; | ||
| } | ||
| if (closeBrace17 && closeBrace17.kind === "close" && closeBrace17.text === "}" && | ||
| closeBrace17.matchedAt !== undefined) { | ||
| const openBraceIdx17 = closeBrace17.matchedAt; | ||
| const prevOpenIdx17 = prevSignificant(tokens, openBraceIdx17 - 1); | ||
| const prevOpen17 = tokens[prevOpenIdx17]; | ||
| const firstInsideBraceIdx17 = nextSignificant(tokens, openBraceIdx17 + 1); | ||
| // Method-signature position: first token in the type literal, or preceded by | ||
| // a member separator (handles `{ x: string; Notification() }` etc.) | ||
| const isAtMemberPos17 = | ||
| firstInsideBraceIdx17 === i || | ||
| (prev17 && prev17.kind === "punct" && (prev17.text === ";" || prev17.text === ",")); | ||
| if (isAtMemberPos17 && prevOpen17 && ( | ||
| prevOpen17.kind === "eq" || // type T = { ... } | ||
| (prevOpen17.kind === "punct" && prevOpen17.text === ":") // x: { ... } | ||
| )) continue; |
Copilot review feedback on PR #164: 1. syn-check.ts: the method-signature exclusion was too narrow — only caught `=` and `:` before `{`. Now also skips when `{` is preceded by `&`/`|`/`<`/`>` (intersection, union, generic) or `as`/`extends`/ `satisfies` keywords, covering all common TS type-literal positions. 2. explanations.ts: SYN017 title was "Notification() / new Notification()" (flipped order vs error-codes.ts "new Notification() / Notification()"). Aligned to match. Co-Authored-By: Botkowski <noreply@botkowski.ai>
There was a problem hiding this comment.
✅ Ready to approve
The change is additive and well-contained, with comprehensive tests and consistent updates across compiler, MCP, and documentation.
Note: this review does not count toward required approvals for merging.
Copilot's findings
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
There was a problem hiding this comment.
⚠️ Not ready to approve
The new SYN017 test fixtures use await inside non-async fn bodies, making the examples invalid JS/TS and inconsistent with existing compiler warning regression tests.
Copilot's findings
- Files reviewed: 8/8 changed files
- Comments generated: 2
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
| it("fires on Notification() in ternary consequent with await (not a method signature)", () => { | ||
| const src = | ||
| "?bs 0.7\n" + | ||
| "fn check(cond: boolean, title: string) -> void {\n" + |
| it("fires on new Notification() in ternary consequent with await (not a method signature)", () => { | ||
| const src = | ||
| "?bs 0.7\n" + | ||
| "fn check(cond: boolean, title: string) -> void {\n" + |
…ures as async fn Two ternary+await test fixtures used `await` inside a non-async fn body, producing invalid JS/TS. Changed both to `async fn ... -> Promise<void>`, consistent with how other SYN check tests handle ternary+await regressions (e.g. SYN007). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
⚠️ Not ready to approve
The new TS type-literal exclusion logic for empty-parens Notification() signatures appears to miss the generic-argument form Foo<Bar, { Notification() }> (false-positive warning risk) and should be corrected before approval.
Copilot's findings
- Files reviewed: 8/8 changed files
- Comments generated: 1
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
Comment on lines
+1469
to
+1483
| if (isAtMemberPos17 && prevOpen17 && ( | ||
| prevOpen17.kind === "eq" || // type T = { ... } | ||
| (prevOpen17.kind === "punct" && prevOpen17.text === ":") || // x: { ... } | ||
| (prevOpen17.kind === "operator" && ( // intersection / union / generic | ||
| prevOpen17.text === "&" || // Foo & { ... } | ||
| prevOpen17.text === "|" || // Foo | { ... } | ||
| prevOpen17.text === "<" || // Foo<{ ... }> | ||
| prevOpen17.text === ">" // closing generic: Foo<Bar, { ... }> | ||
| )) || | ||
| (prevOpen17.kind === "ident" && ( // keyword-led type positions | ||
| prevOpen17.text === "as" || // x as { ... } | ||
| prevOpen17.text === "extends" || // T extends { ... } | ||
| prevOpen17.text === "satisfies" // x satisfies { ... } | ||
| )) | ||
| )) continue; |
…args
Foo<Bar, { Notification() }> — the `{` is preceded by `,` (comma between
generic type arguments), which was not treated as a type-context token.
Replaces the incorrect `>` case (which never matched a real type-literal
position) with the correct `,` case; adds a regression test.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
✅ Ready to approve
The new diagnostic is correctly version-gated, suppressed in unsafe contexts, comprehensively tested, and fully wired through compiler registry, MCP explanations, and docs.
Note: this review does not count toward required approvals for merging.
Copilot's findings
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
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.
Summary
Adds
SYN017: fires a non-blocking warning whenever a fn body constructsnew Notification(title), calls bareNotification(title), uses the optional-call formNotification?.(title), or the generic-instantiation formnew Notification<T>(title)— all of which create user-visible browser notifications invisible to botscript's capability model.?bs 0.7+; silent below thatunsafe "…" { }blocks andunsafe "…" fnbodiesobj.Notification(...)), bare references, fn/function declarations namedNotification, object method shorthands namedNotification, TS type-literal method signatures (with or without optional params, using the?:fix consistent with SYN012–SYN016)new Notification()/Notification()/Notification?.()warning(transform never throws)Changes
packages/compiler/src/error-codes.ts— SYN017 entrypackages/compiler/src/passes/syn-check.ts— detection in single dispatch looppackages/compiler/tests/syn017-check.test.ts— 18 tests (new file)packages/compiler/tests/error-codes.test.ts— SYN017 added to allowlistpackages/mcp/src/explanations.ts— SYN017 explanation with fails/passespackages/mcp/tests/server.test.ts— SYN017 in KNOWN_CODESAGENTS.md+README.md— docs updatedTest plan
npx vitest run syn017— 18 tests passnpx vitest run server— MCP known-codes list includes SYN017npx vitest run error-codes— allowlist cleanpnpm -r build && npx vitest run)🤖 Generated with Claude Code