Skip to content

feat: reads/writes transitivity enforcement (DEP001/DEP002, ?bs 0.9)#51

Open
marcelofarias wants to merge 25 commits into
botkowski/reads-writes-annotationsfrom
botkowski/dep-check
Open

feat: reads/writes transitivity enforcement (DEP001/DEP002, ?bs 0.9)#51
marcelofarias wants to merge 25 commits into
botkowski/reads-writes-annotationsfrom
botkowski/dep-check

Conversation

@marcelofarias
Copy link
Copy Markdown
Owner

Summary

Implements the transitivity enforcement step described as "what comes next" in PR #47. Stacks on #47 (targets botkowski/reads-writes-annotations).

New behavior (?bs 0.9)

From ?bs 0.9, the compiler enforces that reads {} and writes {} annotations are complete with respect to same-file callees:

  • If fn A calls fn B and B declares reads { db }, fn A must also declare reads { db }
  • Same for writes {}

This makes the full resource dependency surface visible from any function's header alone — callers don't need to trace through the call graph.

?bs 0.9

fn fetchFromDb(id: string) reads { db } -> string = id

// DEP001: loadUser calls fetchFromDb which reads { db }, but loadUser only declares reads { cache }
fn loadUser(id: string) reads { cache } -> string = fetchFromDb(id)

// Fix:
fn loadUser(id: string) reads { cache, db } -> string = fetchFromDb(id)

Over-declaration is intentionally allowed — unlike capabilities, the compiler can't infer reads/writes labels from the body (they're user-defined strings), so conservative annotations are harmless.

Implementation

The pass mirrors cap-check.ts:

  1. Parse all fn declarations + build call graph
  2. Seed transitive reads/writes from each fn's own declarations
  3. Fixed-point closure: propagate callees' transitive sets back to callers
  4. Validate: declared ⊇ transitive for each fn

Only same-file resolution, same as cap-check.

Diagnostics

  • DEP001 — reads under-declared: fn A's reads {} doesn't cover all same-file callees' reads (transitively)
  • DEP002 — writes under-declared: same for writes {}

Both are compiler-inferred (no programmer annotation required to trigger them).

Files changed

  • passes/dep-check.ts: new pass (18 tests)
  • passes/version.ts: add '0.9' to SUPPORTED_VERSIONS
  • transform.ts: wire passDepCheck after passCapCheck (minVersion: '0.9')
  • error-codes.ts: DEP001 / DEP002 registry entries
  • primer.ts: update reads/writes docs to describe 0.9 enforcement
  • tests/dep-check.test.ts: 18 tests (DEP001, DEP002, transitive chains, version gating)
  • error-codes.test.ts, mcp/tests/server.test.ts: add DEP001/DEP002
  • mcp/explanations.ts: DEP001/DEP002 long-form explanations with verified examples

All 478 tests pass. pnpm -r build && pnpm test

Partial progress on #14

Marcelo Farias added 6 commits May 13, 2026 00:45
Adds reads { } and writes { } clauses to fn declarations. Both are parsed
from the function header (between uses {} and ->) and stored on FnDecl.
Metadata-only in 0.8: stripped from the TypeScript output, not yet
transitively enforced.

The annotations let bots declare which resource categories a function
reads from or writes to (user-defined labels: cache, db, metrics, etc.).
This is the first slice of the effect-signature design in issue #14.
Enforcement — transitive closure checking similar to cap-check — lands
in a follow-up once the syntax is settled in use.

Changes:
- parser/parse-fn.ts: FnDecl.reads / FnDecl.writes fields; parsed in a
  small loop after uses {} that accepts reads, writes, and intent in any
  order before ->
- passes/version.ts: add '0.8' to SUPPORTED_VERSIONS
- primer.ts: document reads {} / writes {} in the FUNCTIONS section
- tests/reads-writes.test.ts: 13 tests covering parse and TS-strip
- examples/node-app/src/user-cache.bs: end-to-end demo at ?bs 0.8

All 453 tests pass. pnpm -r build && pnpm test OK.
- detect duplicate reads/writes annotations and return null (parse error)
  instead of silently overwriting the first occurrence
- add tests for duplicate reads {} / writes {} returning null
- fix primer.ts: give reads {} and writes {} separate, correctly aligned
  description blocks so writes doesn't read as if it describes reads
- fix user-cache.bs example: mark fetchFromDb as async fn (body uses await)

455 tests pass.
…tes {}

Previously INT001 only caught uses {} vs pure intent. A function that
declares reads { cache } or writes { metrics } is also not pure — it
depends on or mutates external resources. The check now covers all three
annotation types.

Adds 4 tests; updates the error-codes rule/idiom/example to reflect the
wider contract. 460 tests pass.
- intent-check: use space-joined conflictsStr in rewrite field so the
  suggested fn header is valid botscript syntax (clauses are
  space-separated, not comma-separated)
- error-codes: INT001 rewrite option B now includes writes { ... }
  alongside reads { ... } since the rule covers both annotations
- primer: clarify that uses {} (if present) must precede reads/writes/
  intent — "any order" was inaccurate; uses {} is parsed in a fixed
  position before the annotation loop
From ?bs 0.9, the compiler enforces that reads {} and writes {} annotations
are complete with respect to same-file callees. If fn A calls fn B and B
declares reads { db }, A must also declare reads { db } — the full resource
dependency surface must be visible from the caller's header alone.

This is the 'what comes next' step described in PR #47, implemented the same
way CAP001 handles capability transitivity.

- passes/dep-check.ts: new pass; fixed-point closure over the call graph,
  same pattern as cap-check. Only activates at ?bs 0.9.
- DEP001: reads under-declared (callee reads a label caller doesn't declare)
- DEP002: writes under-declared (callee writes a label caller doesn't declare)
- Over-declaration is intentionally not checked (labels are user-defined
  strings; the compiler cannot verify them against the fn body)
- version.ts: add '0.9' to SUPPORTED_VERSIONS
- transform.ts: wire passDepCheck after passCapCheck (minVersion: '0.9')
- error-codes.ts: DEP001 / DEP002 registry entries with examples
- primer.ts: update reads/writes docs from 'metadata-only in 0.8' to
  document 0.9 transitivity enforcement
- tests/dep-check.test.ts: 18 tests covering DEP001, DEP002, mixed
  reads+writes, transitive chains, version gating
- error-codes.test.ts, mcp/tests/server.test.ts: add DEP001/DEP002
- mcp/explanations.ts: DEP001/DEP002 long-form explanations with
  fails/passes examples verified against the compiler

All 478 tests pass.

Partial progress on #14
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Implements ?bs 0.9 enforcement for reads {} / writes {} transitivity across same-file calls by adding a new compiler pass (dep-check) and registering the new diagnostics (DEP001/DEP002) across the compiler + MCP explanation surfaces.

Changes:

  • Add passDepCheck to compute transitive reads/writes and emit DEP001/DEP002 for under-declarations (?bs 0.9+).
  • Register new error codes (compiler + MCP) and wire the pass into the transform pipeline; add 0.9 to supported versions.
  • Add test coverage for dep-check behavior (including version gating) and update docs (primer) to reflect enforcement.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
packages/compiler/src/passes/dep-check.ts New compiler pass implementing reads/writes transitivity closure + DEP001/DEP002 emission.
packages/compiler/src/transform.ts Wires depCheck into the compiler pipeline (minVersion 0.9).
packages/compiler/src/passes/version.ts Adds 0.9 to SUPPORTED_VERSIONS.
packages/compiler/src/error-codes.ts Registers DEP001/DEP002 in the compiler error-code registry.
packages/compiler/src/primer.ts Updates language primer to describe 0.9 enforcement behavior.
packages/compiler/tests/dep-check.test.ts Adds tests for DEP001/DEP002, transitive chains, and version gating.
packages/compiler/tests/error-codes.test.ts Ensures DEP001/DEP002 are present in the compiler-emitted code list.
packages/mcp/src/explanations.ts Adds long-form MCP explanations for DEP001/DEP002 with examples.
packages/mcp/tests/server.test.ts Updates MCP known-code list to include DEP001/DEP002.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +153 to +154
// Skip top-level fns whose annotations come only from their OWN declarations
// (no callee contribution) — those are always trivially consistent.
Comment on lines +193 to +201
const tok = tokens[i];
if (!tok || tok.kind !== "ident") continue;
if (!fnNames.has(tok.text)) continue;
if (tok.text === fn.name) continue; // skip self-references
// Must be followed by `(` to be a call (not just a reference).
const nextIdx = nextSignificant(tokens, i + 1);
const next = tokens[nextIdx];
if (!next || next.kind !== "open" || next.text !== "(") continue;
callees.add(tok.text);
Comment on lines +272 to +273
const message =
`fn '${rec.decl.name}' calls '${pathStr.split(" -> ").at(-1)}' which ${kind} { ${firstLabel} }, ` +
Comment on lines +285 to +289
line,
column,
start: rec.decl.fnKeywordStart,
end: rec.decl.fnKeywordStart + 2,
message,
Comment on lines +252 to +258
// Build a description of the first missing label and its path.
const firstLabel = missingLabels[0]!;
const firstPath = transitiveMap.get(firstLabel)!;
const pathStr = formatPath(firstPath);

const allMissing = missingLabels.map((l) => `"${l}"`).join(", ");
const currentDecl =
Comment thread packages/compiler/src/primer.ts Outdated
enforced (DEP001 / DEP002): if
fn A calls fn B and B declares
reads { x }, A must also declare
reads { x }.
Marcelo Farias added 2 commits May 13, 2026 16:42
- intent-check: file header no longer claims body-shape verification is
  implemented today; rewrite text broadened from 'remove resource
  annotations' to 'remove the conflicting header clauses (uses/reads/writes)'
  so the suggested fix is unambiguous when the conflict is just uses {}
- primer: same softening for the intent: description; INT001 entry now
  explicitly mentions reads/writes as triggers, not just uses {}
- mcp/explanations: INT001 explanation updated to cover reads/writes
- examples/user-cache.bs: add a comment that lookupCache/writeCache are
  intentionally stubbed (annotations are metadata-only in 0.8)
- README: list 0.7 / 0.8 as opt-in pins
- CHANGELOG: backfill 0.7 (intent + INT001) and 0.8 (reads/writes + INT001
  extension) entries
- dep-check: remove misleading 'skip top-level fns' comment from the
  validation loop; the design already trivially handles self-only fns
  through the seed-then-check pattern
- dep-check: collectCallees now skips property accesses (`obj.helper(...)`
  and `obj?.helper(...)`) so a top-level fn name accidentally matching a
  method name no longer triggers false DEP001/DEP002. Mirrors cap-check's
  member-access guard. Two new tests cover both . and ?. cases
- dep-check: diagnostic message now says 'transitively calls' and includes
  the full call path in the main message (not just the leaf) when the
  dependency arrives via a chain. Trailing 'also missing: ...' tail lists
  remaining missing labels (replaces the dead 'allMissing' variable)
- dep-check: diagnostic span now ends at the fn name (not just the `fn`
  keyword) for better editor/LSP highlighting — matches cap-check
- primer: 0.9 enforcement description now explicitly covers writes (DEP002),
  not just reads (DEP001)
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

packages/compiler/src/passes/dep-check.ts:320

  • The non-via branches for callDescription/callPath appear unreachable for under-declaration errors (a missing label can't have kind: "declared" because declared labels are seeded into declaredReads/Writes and filtered out of missingLabels). Additionally, the fallback callPath text says the label is "directly declared on" the caller (rec.decl.name), which is incorrect and would give wrong guidance if this branch ever became reachable. Recommend either removing the dead branches or fixing them to reference the leaf/callee function name.
  // For via-paths, name the call chain in the main message so the caller
  // sees that the dependency arrives through a callee, not from a direct call.
  const callDescription =
    firstPath.kind === "via"
      ? `${pathStr} — '${leaf}' ${kind} { ${firstLabel} }`
      : `'${leaf}' which ${kind} { ${firstLabel} }`;

  const message =
    `fn '${rec.decl.name}'${transitively} calls ${callDescription}, ` +
    `but '${rec.decl.name}' only declares ${kind} { ${currentDecl} }${otherTail}`;

  const callPath =
    firstPath.kind === "via"
      ? `call path: ${pathStr}`
      : `directly declared on '${rec.decl.name}'`;

Comment on lines +281 to +285
const pathStr = formatPath(firstPath);
const leaf = pathStr.split(" -> ").at(-1)!;
const transitively = firstPath.kind === "via" ? " transitively" : "";

const currentDecl =
Marcelo Farias added 3 commits May 13, 2026 20:44
…otations

# Conflicts:
#	packages/compiler/src/passes/intent-check.ts
#	packages/compiler/src/primer.ts
#	packages/compiler/tests/intent.test.ts
…check

# Conflicts:
#	packages/compiler/src/transform.ts
The 'transitively' flag was always set because missingLabels are by
construction always propagated from callees. Distinguish direct calls
(one hop: caller -> declaring fn) from multi-hop chains so the message
reads accurately in both cases.
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Comment thread packages/compiler/src/primer.ts Outdated
Comment on lines +50 to +54
enforced: if fn A calls fn B and B
declares reads { x }, A must also
declare reads { x } (DEP001). The
same rule applies to writes { x }
(DEP002).
Comment thread packages/compiler/src/passes/version.ts Outdated

export const SUPPORTED_VERSIONS: ReadonlyArray<string> = ["0.1", "0.2", "0.3", "0.4", "0.5", "0.6", "0.7", "0.8"];
export const SUPPORTED_VERSIONS: ReadonlyArray<string> = ["0.1", "0.2", "0.3", "0.4", "0.5", "0.6", "0.7", "0.8", "0.9"];
export const LATEST_VERSION = "0.1";
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

Comment thread packages/compiler/src/index.ts Outdated
export type { Program, Stmt, FnStmt, SourceRange } from "./parser/ast.js";
export type { FnDecl, FnBody, ParseFnOptions } from "./parser/parse-fn.js";
export { atLeast, LATEST_VERSION, SUPPORTED_VERSIONS } from "./passes/version.js";
export { atLeast, DEFAULT_VERSION, SUPPORTED_VERSIONS } from "./passes/version.js";
Comment thread packages/compiler/src/transform.ts Outdated
}

export { LATEST_VERSION, SUPPORTED_VERSIONS } from "./passes/version.js";
export { DEFAULT_VERSION, SUPPORTED_VERSIONS } from "./passes/version.js";
Marcelo Farias and others added 2 commits May 14, 2026 04:41
…rites to STDLIB.bs

- CHANGELOG: "user-defined strings" → "user-defined identifiers" (labels
  inside reads {} / writes {} are identifier tokens, not quoted strings)
- CHANGELOG: compat note now correctly describes forward-compatible parsing
  (reads/writes parse at any version; only enforcement is gated on 0.8)
- primer.ts: remove quotes around label examples so readers don't try
  reads { "cache" } (identifiers only, parseCapList silently drops strings)
- STDLIB.bs: bump to ?bs 0.8; add canonical reads {} / writes {} example
  (AGENTS.md §108: every new form must appear in STDLIB.bs exactly once)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Removing LATEST_VERSION from the public exports was a breaking change for
any consumer that imported it from @mbfarias/botscript-compiler or from
./transform. Add it back as a deprecated alias so downstream tooling does
not break — the rename was a pure implementation detail.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

const tok = tokens[i];
if (!tok || tok.kind !== "ident") continue;
if (!fnNames.has(tok.text)) continue;
if (tok.text === fn.name) continue; // skip self-references
@marcelofarias marcelofarias force-pushed the botkowski/reads-writes-annotations branch from ed0f410 to 69eac1f Compare May 14, 2026 11:44
… rebase)

Picks up the rebased botkowski/reads-writes-annotations, including:
- fix: duplicate reads/writes/intent annotations use first-wins, not null return
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

*
* DEP001 reads under-declared: fn A calls fn B which (transitively)
* reads a resource category that A does not declare. Diagnostic
* names the call path: "A -> B -> C [reads { x }]".
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Comment on lines +314 to +324
// For multi-hop chains, name the call chain in the main message so the
// caller sees that the dependency arrives through an intermediate callee.
// For direct calls (one hop), keep the message focused on the leaf.
const callDescription = directCall
? `'${leaf}' which ${kind} { ${firstLabel} }`
: `${pathStr} — '${leaf}' ${kind} { ${firstLabel} }`;

const message =
`fn '${rec.decl.name}'${transitively} calls ${callDescription}, ` +
`but '${rec.decl.name}' only declares ${kind} { ${currentDecl} }${otherTail}`;

For transitive chains (A -> B -> C), the diagnostic now shows "B -> C"
as the display path rather than "A -> B -> C", avoiding the awkward
"fn 'A' transitively calls A -> B -> C" where the caller appears twice.

Also removes the unnecessary self-name skip in collectCallees — the
closure step already ignores self-loops via `calleeDecl === rec.decl`,
so skipping by name was redundant and incorrectly suppressed calls to
shadowed nested fns with the same outer fn name.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Comment on lines +194 to +209
for (let i = fn.tokenStart; i < fn.tokenEnd; i++) {
if (insideAny(i, inner)) continue;
const tok = tokens[i];
if (!tok || tok.kind !== "ident") continue;
if (!fnNames.has(tok.text)) continue;
// Skip property accesses like `obj.helper(...)` or `obj?.helper(...)` —
// these are not same-file fn calls even if `helper` matches a top-level
// fn name. Mirrors cap-check's member-access guard.
const prevIdx = prevSignificant(tokens, i - 1);
const prev = tokens[prevIdx];
if (prev && ((prev.kind === "punct" && prev.text === ".") || prev.kind === "questionDot")) continue;
// Must be followed by `(` to be a call (not just a reference).
const nextIdx = nextSignificant(tokens, i + 1);
const next = tokens[nextIdx];
if (!next || next.kind !== "open" || next.text !== "(") continue;
callees.add(tok.text);
…l graph

Mirrors cap-check's guard (tok.text !== fn.name). Without the skip,
fn <name>( in the header was seen as an ident followed by (, creating a
spurious self-edge that could propagate reads/writes from other same-named
decls (e.g. nested fns) into the caller incorrectly.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Comment on lines +162 to +166
// ---------------------------------------------------------------------------
// INT001 extended: reads {} / writes {} also contradict "pure" intent
// ---------------------------------------------------------------------------

describe("INT001 — pure intent vs reads/writes annotations (?bs 0.8)", () => {
Resolves conflict: error-codes.test.ts now checks for all codes
(CAP001/002, DEP001/002, INT001, RES001, SYN001, UNS001-004).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.

The second describe block at line 240 was a verbatim duplicate of the
one at line 166 (minus the 0.7 gating case) — kept the more complete
first block. Addresses Copilot review on PR #51.
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

packages/mcp/src/explanations.ts:248

  • This says “The message names the call path…”, but for direct calls DEP002’s diagnostic message does not include a path string (it only names the leaf callee); the call path appears in the rewrite hint and multi-hop messages. Please reword to reflect the actual diagnostic output.
      "Add the missing label to the caller's `writes {}` clause to clear the diagnostic. " +
      "The message names the call path and the specific missing label.",

Comment on lines +25 to +27
expect(() => compile(src)).toThrow(new RegExp(`\\[${code}\\]`));
if (fragment) {
expect(() => compile(src)).toThrow(fragment);
Comment on lines +225 to +227
"The fix is simple: add the missing label(s) to the caller's `reads {}` clause. " +
"The diagnostic message names the call path (\"A -> B\") and the specific label, " +
"so the rewrite is mechanical.",
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants