Skip to content

✅ Prove Workspace effects survive a real host crash (#365 slice 7) - #419

Merged
taras merged 11 commits into
mainfrom
agent/issue-365-7-crash-boundary
Aug 10, 2026
Merged

✅ Prove Workspace effects survive a real host crash (#365 slice 7)#419
taras merged 11 commits into
mainfrom
agent/issue-365-7-crash-boundary

Conversation

@taras

@taras taras commented Aug 9, 2026

Copy link
Copy Markdown
Owner

Why

Issue #365 claims that one Workspace expansion produces one effect and one
caller-owned SQLite transaction, and that host interruption publishes none of
it. Every proof of that claim so far ends its transaction inside the test
process, where Effection tears the scope down — a cancellation, a failure or a
closed scope, never a crash. At the kill point the transaction is open;
SIGKILL then runs no application cleanup, so nothing commits and nothing
rolls back. The operating system closes the connection and releases its locks,
and the next connection to open the database observes recovery to the last
committed state — which is what decides the outcome, rather than anything the
adapter executes.

This is #365 slice 7, the final proof layer. It adds no product behavior — but
it does remove one host import from shared source, because the boundary test
this PR strengthens found it.

What changes

Before:

  • Interruption was proven by cancellation, exception and scope teardown
    (WAC6, WAC13, WAC14). Restart was proven for the ordinary journal only
    (WJ25). Historical restoration was proven inside one process (WRR4).
  • The runtime boundary check read two files and searched them for eight names.

After:

  • WAC24–WAC26 prove the same boundary with real processes: a real SIGKILL
    during an uncommitted effect, a cold restart of a committed Workspace, and a
    cold reconstruction of an older event's root.
  • DLC13 sweeps the whole shared coordination surface, decides module loading by
    parsing it, recognizes host modules by shape rather than by an enumerated
    list, refuses destinations it cannot read statically, and proves it can fail
    before it runs.
  • packages/workflow/src/run.ts allocates its run id through Web Crypto
    instead of node:crypto, so shared source names no host.

How it works

parent commits a baseline
  → crash child resumes it, writes, stops at the routed-append hook
  → parent reads the same database on a second connection
  → SIGKILL
  → inspector process reopens through the production provider

The crash timeline

Step Who State
1 parent Workspace mutation + root published, one filtered Yield appended, provider scope closed
2 crash child resumes the run; the baseline Yield replays (baselineExecutions: 0)
3 crash child BEGIN IMMEDIATE; mutation savepoint writes /crash.txt through DOFS
4 crash child root captured, current_root published, secret gate runs (gateCalls: 1)
5 crash child routed Yield inserted through transaction.journal
6 crash child kill pointafterRoutedJournalAppend reports and suspends; the transaction is open and never reaches COMMIT
7 parent second raw read-only connection sees the baseline only
8 parent process.kill(pid, "SIGKILL") — no application cleanup runs; the OS closes the connection and releases its locks. join() reports signal: "SIGKILL", no exit code
9 inspector new process, new production provider: recovery to the last committed state — baseline filesystem, root, counts and journal

The child's readings at step 6 and the parent's at step 7 are of one database at
one moment, through the connection that made the writes and through a
connection that did not:

child (authoritative) parent (second connection)
/crash.txt present absent
retained roots 3 2
current_root the new root the baseline root
journal rows 2 1

Review guide

Start with: packages/workflow/tests/workspace-crash-recovery.test.ts

Then review:

  1. tests/support/workspace-crash-child.ts — the killed process and the inspector
  2. tests/support/workspace-restart-child.ts — commit, cold read, cold restoration
  3. tests/workspace-effect.test.ts DLC13 — the boundary sweep
  4. packages/workflow/src/run.ts — the one production change
  5. scripts/runtime-test-exclusions.ts, specs, architecture.md

Look carefully at:

  • The crash child assembles the adapter's own modules rather than calling
    useWorkflowRunStorage. The routed-append hook is installed when the
    connection registry is constructed, and the provider constructs its own; the
    child needs both that hook and the authoritative connection to read
    uncommitted rows from. The inspector and both restart processes use the
    provider itself.
  • The baseline is recorded as an unfinished run rather than as a completed
    durableRun. A journal holding a Close is a run with nothing left to
    execute, so the crash child's effect would never run at all.
  • The crash child holds an uncleared timer. Deno exits when its event loop
    empties, and a suspended Effection task is not on it.
  • run.ts now calls crypto.randomUUID(). Web Crypto is the cross-runtime
    standard and packages/core/src/secrets/findings.ts already draws its key
    the same way, so this needs no new abstraction or contextual capability. Run
    ids remain cryptographically random.

What must stay true

  • One expansion, one effect, one caller-owned transaction — enforced by the
    slice-6 coordinator, checked across a process boundary by WAC24.
  • No second long-lived DOFS connection — the only other reader is a
    short-lived raw read-only DatabaseSync that distinguishes committed from
    uncommitted visibility.
  • No host type crosses into shared production APIs — enforced by module
    structure, checked by DLC13 over the globbed surface.
  • No shared module reads a host global. process, Deno, Bun, Buffer,
    globalThis, navigator, __dirname and __filename are recognized by the
    TypeScript compiler: each scanned source becomes a one-file ts.Program with
    no lib and no module resolution, and a name is the host's when the type
    checker finds nothing in the file declaring it. Value and type scopes,
    hoisting, aliasing, namespace, import =, mapped and infer type
    parameters, accessors and shadowing therefore follow the language rather than
    a list kept in the test, and cross-runtime Web APIs such as crypto are
    never crossings. A shorthand property is asked for its value symbol rather
    than its property symbol, because { process } declares a property and reads
    a binding with one name and only the second is the question. The only
    syntactic question left is whether an identifier refers to a binding at all,
    and that is asked of the grammar rather than of a list of node kinds: an
    IdentifierName fills a name, propertyName or label slot of its parent
    — or a qualified name's right — and an IdentifierReference is anywhere
    else. Member accesses, statement labels, imported members, object keys, named
    tuple elements, import attributes, enum members and every declaration's own
    name are therefore labels because of what they are.
  • No shared module loads a module only one host can resolve. Module loading is
    read from parsed syntax — static imports and re-exports, import type,
    type-position import(), dynamic import(), import = and require()
    with quoted and no-substitution template specifiers decoded, so comments and
    strings that merely contain import syntax load nothing. A destination the
    surface computes is refused, because nothing can show it is not a host
    module. Host schemes, whole path segments naming any runtime this repository
    builds an entry point for (deno, node, bun, compiled, cloudflare,
    workerd), vendored sources and host processes are classified by shape
    rather than by a list, so the next such import is caught without anyone
    having predicted it. Segments are compared whole, so nodes/, bundle.ts
    and vendors/ are not crossings for containing a runtime's name.

How to verify it

  • WAC24 proves a SIGKILL between the routed append and the commit
    publishes nothing, and fails if the transaction were committed before the
    parent looked. Verified by mutation: moving the child's announcement to after
    withWorkspaceEffects returns makes expect(during.outside).toEqual(before)
    fail with 5 blob refs against 2, 3 blobs against 2, and a changed current
    root.
  • WAC25 proves a cold process observes the committed filesystem, current
    root, ordered events, event identities and event-to-root associations, and
    fails if a recorded effect ran again — each effect appends its name to a
    marker file, and the first process's two lines must still be two.
  • WAC26 proves the older event's root reconstructs exactly. The negative
    lookup for /tree/file.txt happens before restoration and is answered from
    the live frontier, so the successful read afterwards is cache invalidation
    rather than a cache never consulted. The hardlink relationship is proven by
    identity rather than by inspection: resnapshotRoot === historical can only
    hold if the canonical manifest's hardlink group was reproduced. The manifests
    the historical root references and the later root does not are asserted
    non-empty, so restoration cannot have borrowed live content.
  • DLC13 fails on the crossing that was actually present: restoring
    import { randomUUID } from "node:crypto" to run.ts makes it report
    {"packages/workflow/src/run.ts": ["node:crypto"]}. Each evasion a text
    match would miss was checked the same way, by putting it into real shared
    source and watching the scan report it — await import(`node:crypto`)
    and await import("node\u003acrypto") both report node:crypto, and
    const t = "node:crypto"; await import(t) reports
    a computed module specifier. The regressions additionally assert
    node:crypto, node:os, node:fs (as import type, type-position
    import() and import =), bun:sqlite and ../deno.ts, and assert that
    the same specifier in a comment, a string or a template loads nothing. A
    module whose text imports something must yield a specifier, so a parse that
    stopped working cannot report the surface as clean.
  • DLC13's architecture is what the mutation discriminates. Replacing the
    compiler with the previous hand-written resolver reports a crossing for every
    form the language binds and it did not know — import Deno = require(...)
    ["Deno"], namespace Deno["Deno"],
    type Rename<T> = { [process in keyof T]: T[process] }["process"],
    type Value<T> = T extends infer Buffer ? Buffer : never["Buffer"],
    process: for (;;) { break process; }["process"],
    class Queue { get process() {…} }["process"] — while the compiler
    reports none of them. Both still report process.pid, Deno.cwd() and a
    reference outside its shadow, so the mutation isolates resolution rather than
    detection. Resolving a shorthand property with the ordinary
    getSymbolAtLocation() reports nothing for const environment = { process };
    against expected ["process"] — a fail-open hole, since the property the
    literal declares is always local. Each label classification is held on its
    own: treating a named tuple element as a reference reports
    ["process", "Deno"] for type Pair = [process: string, Deno?: number], and
    treating an import attribute as one reports ["process"] for its key, while
    type Pair = [value: typeof process] and the node:fs beside an attribute
    stay reported either way. Deleting the host-global scan still fails
    on process.pid, and narrowing adapter recognition back to Deno still fails
    on ./node.ts.
  • WR1–WR17 still pass with the run id drawn from Web Crypto, so the
    allocator change is covered by the suite that owns run identity.

deno task lint, deno task check (0 errors) and deno task check:jsr pass on this head, as do the focused DLC13, crash-recovery and WorkflowRun identity suites (3 suites, 32 steps). The complete three-runtime battery is CI's: the parser import is new under Node and Bun, and test-node and test-bun are where that is proven rather than on one developer's machine. The new suite is Deno-only and registered for Node and Bun in scripts/runtime-test-exclusions.ts against this issue; both helpers still typecheck under tsconfig.node.json.

#415 has merged, so this PR now targets main directly and its CI runs the
full set of jobs. The battery above is the local run on the rebased head.

Scope

Included

  • WAC24–WAC26 and their two subprocess helpers
  • The strengthened DLC13 boundary sweep, and the one shared-source host import
    it found (node:crypto in packages/workflow/src/run.ts)
  • The Node/Bun runtime exclusion for the new Deno-only suite
  • Present-tense process-recovery statements in architecture.md,
    specs/workflow-spec.md §9.6 and specs/workflow-workspace-spec.md
    §10.1/§13, and the WAC24–WAC26 conformance rows

Intentionally unchanged

  • The Workspace coordinator, transaction, schema and root format: the only
    production edit is the run-id allocator's import in
    packages/workflow/src/run.ts
  • Public xmd workflow start/resume; API.Files and <File> integration
  • History, fork and root-selection commands or APIs
  • Worker Shell; Repository or Git effects; external effects
  • Schema, root format, transaction, journal and secret policy
  • Garbage collection; writable FUSE; native subprocess execution; workerd
  • WS0, WTX10, WRR4, WRR9, WRR10/10b, WJ25 and WAC1–WAC23 are untouched; this
    adds only the process-level composition they do not cover

New abstractions

  • tests/support/workspace-process.ts exists because the constants and the
    filesystem reader are shared by two child processes and the suite that
    launches them, and importing them from a child would run that child's
    main() inside the test.

New dependencies

  • Package: typescript@^5.0.0 — already a root devDependency; this adds the
    deno.json import-map entry, one line in deno.lock, so a workspace member
    can resolve it.
  • Used for: parsing module-loading syntax in DLC13.
  • Why existing dependencies are insufficient: acorn is in the import map but
    parses JavaScript, and the scanned surface is TypeScript.

Risks and limitations

  • The crash child assembles the adapter rather than installing the provider,
    for the reason above. If a future slice exposes the connection hooks through
    a provider-level seam, the child should use it.
  • code() strips comments for the identifier scan and does not model
    regular-expression literals. No module on the scanned surface contains one
    that could hide a forbidden name, and the scanner's own failure cases are
    asserted. Module loading no longer depends on it at all — that is decided by
    a parse.

Scope confirmation

  • Every changed file supports the purpose described above.
  • Unrelated cleanup and formatting changes are excluded.
  • Generated or mechanical changes are clearly identified.
  • The description matches the final diff and test results.

Closes #365

Rebased onto main after #415 merged. My identifiers moved from WAC19–WAC21 to
WAC24–WAC26, because #415 landed with WAC19–WAC23 of its own.

@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown

PR #419: ✅ Prove Workspace effects survive a real host crash (#365 slice 7)

13 files, +1515 / -21

Scope

🔴 PR has 1536 lines changed. Split into focused PRs.

🟡 1536 lines changed. PRs under 400 receive more thorough review.

🟡 Changes span 6 directories.

🟡 PR mixes config and source changes.

Structural

🟡 1 console statements.

Slop

✅ Slop indicators look low.

Static Analysis

Oxlint: 2 diagnostics across 2 files (1 rule)
Density: 0.001 violations/added-line

no-floating-promises (2): packages/workflow/tests/support/workspace-restart-child.ts, packages/workflow/tests/support/workspace-crash-child.ts

Correctness

No extraneous code patterns detected.

Base automatically changed from agent/issue-365-6-atomic-workspace to main August 10, 2026 06:14
taras added 2 commits August 10, 2026 02:17
Every other atomic-Workspace proof ends its transaction inside the test
process, where Effection tears the scope down. A crash does none of that: it
leaves an open SQLite transaction with nobody to roll it back, and what the
database holds afterwards is SQLite's recovery rather than anything the adapter
runs.

So this is made of real processes. One resumes a committed run, performs a real
Workspace effect, and stops at the accepted construction-time routed-append
hook with the mutation, the immutable root, the current-root pointer and the
routed journal row all written and none of them committed. It says so on
standard output, a second connection is shown seeing only the baseline, and
then it is killed with SIGKILL. A different process, through a newly installed
production provider, finds the baseline filesystem, root, retained counts and
journal exactly.

Two more processes commit a Workspace history and reconstruct it cold: the
second observes the committed filesystem, current root, ordered events, event
identities and event-to-root associations without performing a recorded effect
again, then selects the older event's root through the adapter-private
materializer and rebuilds its topology, bytes, modes, hardlinks and symbolic
links from the DOFS content that root retains.

The crash child assembles the adapter's own modules rather than calling
`useWorkflowRunStorage`, because the routed-append hook is installed when the
connection registry is constructed and the provider constructs its own. The
inspector and both restart processes use the provider.

No production behavior changes.
The boundary check read two files and searched them for eight names, which
answered a smaller question than the one it was named after: whether any shared
module of the coordination surface names a host at all.

It now globs that surface — the workflow package outside its Deno adapter, and
the durable-stream coordination modules — and refuses storage and runtime
implementation types, the adapter's private connection, savepoint and
transaction-token identities, runtime detection, process globals, and imports
that reach an adapter, a vendored source or a host process.

Two things make the sweep answerable. It reads code rather than the file: these
modules explain in their own prose that they name no host, and a substring
search finds the explanation. And it proves it can fail before it runs, on a
crossing and on a comment that only looks like one, because a glob that matched
nothing would report the cleanest boundary of all.

The list of what it must have found is written down, so a module that stops
being matched fails instead of quietly leaving the surface.
@taras
taras force-pushed the agent/issue-365-7-crash-boundary branch from 728d96f to 6b265ff Compare August 10, 2026 06:25
taras added 2 commits August 10, 2026 02:47
DLC13 claimed the shared coordination surface names no host, and its own glob
already covered a module that imported one:

    packages/workflow/src/run.ts
    import { randomUUID } from "node:crypto";

The detector listed `node:sqlite`, `node:process` and `node:child_process` —
the host modules somebody had thought of — so the import that was actually
there went unreported and a green run confirmed the blind spot instead of the
boundary.

A module specifier only one host can resolve is now recognized by its shape:
the `node:`, `bun:`, `deno:` and `cloudflare:` schemes, an adapter path, a
vendored source, a host process. Specifiers are read from the import forms
rather than matched as text, so `node:crypto` in a comment or a string is
prose and `import … from "node:crypto"` is a crossing — both asserted, along
with the `node:crypto` case that this test used to miss.

Shared code allocates a run id through Web Crypto, which every supported
runtime resolves, the way `createFingerprinter` already draws its key. The id
is still allocated with cryptographic randomness; only the module that
provides it stops naming a host.
The new recovery prose said a killed process "leaves its transaction open",
which describes something that cannot happen: the process is gone, so the
operating system closes its connection and releases every lock it held. What
is left is a database the next connection recovers, not a transaction still
running somewhere.

It also timed the guarantee to the writer's life — "while the process is
alive" — when what a second connection actually observes is the last committed
state for as long as the writer's transaction is uncommitted. That is the
ordinary isolation boundary rather than a rule that only applies to crashes,
and saying it the other way invites a reader to expect a special case.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Found 6 redundant comments. Inline suggestions to remove them below.

const pinnedCommit = yield* revParse(`${base}^{commit}`);
return { runId: randomUUID(), base, pinnedCommit };
// Web Crypto rather than `node:crypto`: a run id is allocated in shared
// code, which names no host.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Redundant comment — restates what the code does.

Suggested change
// code, which names no host.

return;
}
// Every read below is on the connection that opened the transaction, so
// it sees that transaction's own uncommitted writes. Nothing else can.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Redundant comment — restates what the code does.

Suggested change
// it sees that transaction's own uncommitted writes. Nothing else can.

});
// Deno leaves when its event loop is empty, and a suspended Effection
// task is not on it. A timer nothing clears is what keeps this process
// and its open transaction alive until the signal arrives.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Redundant comment — restates what the code does.

Suggested change
// and its open transaction alive until the signal arrives.

// and its open transaction alive until the signal arrives.
setInterval(() => {}, 1_000);
// The transaction stays open from here until the operating system takes
// this process away, which is what the process is for.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Redundant comment — restates what the code does.

Suggested change
// this process away, which is what the process is for.

function* workflow(): Workflow<void> {
// The run this process resumes already holds this effect's result, so it
// replays. Executing it would mean the crash effect below is not the
// first live work of the process, and the count says which happened.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Redundant comment — restates what the code does.

Suggested change
// first live work of the process, and the count says which happened.


main(function* () {
// `process.argv` rather than `Deno.args`: this file is Deno-only to run, and
// still has to typecheck under the Node project like every other source.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Redundant comment — restates what the code does.

Suggested change
// still has to typecheck under the Node project like every other source.

DLC13 claimed a fail-closed runtime-neutral boundary and asked a regular
expression to find it. Module loading is not a pattern, so four different
things went the wrong way at once:

    await import(`node:crypto`)          // a template literal it never matched
    await import("node:crypto")     // an escape it never decoded
    const t = "node:crypto";
    await import(t)                      // a destination it could not read
    const note = `import "node:crypto"`; // prose it rejected as an import

The scan now reads parsed syntax: static imports and re-exports, `import
type`, type-position `import()`, dynamic `import()`, `import =` and
`require()`. The parser hands back decoded specifiers, so an escape and a
no-substitution template are the module they name, and characters that only
look like an import — in a comment, a string, a template — load nothing and
are reported as nothing.

A specifier this surface computes is refused rather than skipped. Nothing can
show it is not a host module, and a boundary that admits what it cannot read
is not a boundary.

The scan also refuses to be quiet about its own failure: a module whose text
imports something must yield a specifier, so a parse that stopped working
reports every file as clean exactly once and then fails.

Host schemes, adapter paths, vendored sources and host processes are still
classified by shape rather than by an enumerated list.
@taras
taras marked this pull request as ready for review August 10, 2026 07:12
DLC13 said it rejected process globals and runtime adapter imports by shape,
and did neither:

    const pid = process.pid;   // reported nothing
    import "./node.ts";        // reported nothing

The globals were a substring list, and `process` could not go on it: `Bun` is
inside `Bundle` and `Deno` inside `Denominator`, so text matching would have
started rejecting words for their spelling. They are read from the parse
instead — an identifier that refers to the binding it names, so `preprocessor`,
`x.process` and `{ process: 1 }` are not uses of a host and `crypto` is a
standard rather than a host at all.

The adapter rule knew `/deno/` and `/deno.ts`, which is the adapter that
happens to exist. Code Rule 12 names four of them for the CLI alone, so any
whole path segment naming a runtime this repository builds an entry point for
is one. Segments are compared whole for the same reason the globals are parsed:
`nodes/`, `bundle.ts` and `vendors/` contain a runtime's name without being one.

Both recognitions are held by mutation. Deleting the global scan fails on
`process.pid`; narrowing the adapter rule back to Deno fails on `./node.ts`.

The crash suite's and crash child's opening comments said a killed process
leaves SQLite holding a transaction. The transaction is open at the kill point,
`SIGKILL` runs no application cleanup, the operating system closes the
connection and releases its locks, and the next connection recovers the
interrupted transaction to the last committed state — which is what they now
say, matching architecture.md.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Found 2 redundant comments. Inline suggestions to remove them below.

const pinnedCommit = yield* revParse(`${base}^{commit}`);
return { runId: randomUUID(), base, pinnedCommit };
// Web Crypto rather than `node:crypto`: a run id is allocated in shared
// code, which names no host.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Redundant comment — restates what the code does.

Suggested change
// code, which names no host.


main(function* () {
// `process.argv` rather than `Deno.args`: this file is Deno-only to run, and
// still has to typecheck under the Node project like every other source.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Redundant comment — restates what the code does.

Suggested change
// still has to typecheck under the Node project like every other source.

The global scan asked whether an identifier was spelled `process` and whether
it sat in a declaration position. Neither question is the one that matters, so
three ordinary things were reported as crossings:

    function inspect(process: { pid: number }) { return process.pid; }
    const Buffer = 1; const b = Buffer;
    import { Deno } from "./host.ts"; Deno.cwd();

A parameter, a local and an import named `process` are not the host's
`process`. What separates them from the real thing is not spelling or position
but what declared the name, so the scan now walks the enclosing scope chain
outward from each reference and reports only the ones nothing declared.

Shadowing therefore ends where its scope does: a parameter named `process`
covers its own function and no more, and a `const Deno` inside a block leaves
the reference after that block still reported.

Three mutations hold the classifier now. Dropping binding resolution reports
`process` for the parameter case; deleting the global scan misses
`process.pid`; narrowing adapter recognition back to Deno misses `./node.ts`.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Found 3 redundant comments. Inline suggestions to remove them below.

const pinnedCommit = yield* revParse(`${base}^{commit}`);
return { runId: randomUUID(), base, pinnedCommit };
// Web Crypto rather than `node:crypto`: a run id is allocated in shared
// code, which names no host.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Redundant comment — restates what the code does.

Suggested change
// code, which names no host.

});
// Deno leaves when its event loop is empty, and a suspended Effection
// task is not on it. A timer nothing clears is what keeps this process
// and its open transaction alive until the signal arrives.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Redundant comment — restates what the code does.

Suggested change
// and its open transaction alive until the signal arrives.

// and its open transaction alive until the signal arrives.
setInterval(() => {}, 1_000);
// The transaction stays open from here until the operating system takes
// this process away, which is what the process is for.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Redundant comment — restates what the code does.

Suggested change
// this process away, which is what the process is for.

taras added 2 commits August 10, 2026 03:53
The resolver knew a handful of node shapes rather than the language's binding
rules, so four ordinary declarations were still read as ambient host globals:

    const { process: local } = deps;
    import { Deno as portable } from "./host.ts";
    function read<Deno>(value: Deno): Deno { return value; }
    function read() { if (ready) var process = portable; return process.pid; }

Each is a different rule. An aliased member is a label on both sides — the
name being taken and the name it is given — and neither reads a binding. A type
parameter binds its own declaration's type scope. And `var` belongs to the
containing function however deeply the statement that writes it is nested, so
collecting a block's own statements could never find it.

All four are now decided by what the language says declares a name: alias
property names are labels, type parameters join the scope of the function,
class, interface or alias that introduces them, and every `var` in a function
body is hoisted to the function, skipping the nested functions and classes that
own their own.

Shadow termination is unchanged and still asserted: a type parameter covers its
own signature, a hoisted `var` covers its own function, and a reference after
either is reported.

Reverting to the previous resolver false-positives on all seven newly covered
categories — both alias forms, the re-export alias, function and interface type
parameters, and `var` hoisted out of an `if` and out of a `for` — while
`process.pid` and `Deno.cwd()` stay reported.
Four rounds of review found four more declaration forms the hand-written
resolver did not know, and this round found six: `import =`, `namespace`,
mapped-type and `infer` type parameters, statement labels, and accessor
members. The pattern is the defect. A boundary that claims to follow the
language's binding rules cannot be a growing inventory of the declaration
shapes somebody remembered.

Resolution is now the compiler's. Each scanned source becomes a one-file
`ts.Program` with no lib and no module resolution, and every candidate
identifier is handed to the type checker: a name is the host's when nothing in
the file declares it. Value scopes and type scopes, hoisting, aliasing,
namespaces, mapped and inferred type parameters, accessors and shadowing all
come from the compiler, and no rule about any of them is written here.

What stays is one syntactic question the checker cannot be asked: whether an
identifier refers to a binding at all. The member in `x.process`, the loop
label in `break process`, the imported member in `{ Deno as portable }` and
the key in `{ process: local }` are labels, not references — the language says
so, and there are only these positions.

The architecture is what the mutation now discriminates. Restoring the manual
resolver reports a crossing for every one of the six forms above, while the
compiler reports none and both still report `process.pid`, `Deno.cwd()` and a
reference outside its shadow.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Found 2 redundant comments. Inline suggestions to remove them below.

const pinnedCommit = yield* revParse(`${base}^{commit}`);
return { runId: randomUUID(), base, pinnedCommit };
// Web Crypto rather than `node:crypto`: a run id is allocated in shared
// code, which names no host.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Redundant comment — restates what the code does.

Suggested change
// code, which names no host.


main(function* () {
// `process.argv` rather than `Deno.args`: this file is Deno-only to run, and
// still has to typecheck under the Node project like every other source.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Redundant comment — restates what the code does.

Suggested change
// still has to typecheck under the Node project like every other source.

`{ process }` writes one name in two roles: the property the object literal
declares and the value it reads. The ordinary symbol at that identifier is the
property, and the property is declared right there — so asking for it answered
that every host global becomes locally declared the moment it is put in an
object.

    const environment = { process };      // reported nothing
    const runtimes = { Deno, Bun };       // reported nothing

That direction is the dangerous one. The scan is meant to fail closed, and this
made it quietly admit the exact read it exists to catch, in the shortest way
anyone would write it.

A shorthand assignment's value symbol is what the name refers to, so that is
what the scan now asks for. A declared `process` used as `{ process }` still
resolves to its declaration and is still accepted, and `const { process } =
deps` is a binding element rather than a shorthand assignment and was never
this question.

Replacing the value symbol with the ordinary one reports nothing for
`{ process }`, which is the mutation this keeps.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Found 2 redundant comments. Inline suggestions to remove them below.

const pinnedCommit = yield* revParse(`${base}^{commit}`);
return { runId: randomUUID(), base, pinnedCommit };
// Web Crypto rather than `node:crypto`: a run id is allocated in shared
// code, which names no host.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Redundant comment — restates what the code does.

Suggested change
// code, which names no host.

// and its open transaction alive until the signal arrives.
setInterval(() => {}, 1_000);
// The transaction stays open from here until the operating system takes
// this process away, which is what the process is for.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Redundant comment — restates what the code does.

Suggested change
// this process away, which is what the process is for.

Two more label positions were being read as ambient globals:

    type Pair = [process: string, Deno?: number];
    import data from "./x.json" with { process: "portable" };

Both were omissions of the same kind as the last several: the classifier
listed the parent node kinds someone had thought of, so every position it had
not been shown was a reference by default.

TypeScript already draws this line structurally. An `IdentifierName` fills a
`name`, `propertyName` or `label` slot of the node that owns it — and a
qualified name's `right` is the same thing in type position — while an
`IdentifierReference` appears anywhere else. Asking which slot the identifier
occupies replaces the list of kinds with the rule the grammar uses, so named
tuple elements, import attributes, enum members, property signatures and every
declaration's own name are labels because of what they are rather than because
they were reported.

The shorthand property remains the single name slot that is also a read, and
is still resolved to the binding it refers to.

Each classification is held on its own: treating a named tuple member as a
reference reports `["process", "Deno"]` for the tuple, and treating an import
attribute as one reports `["process"]` for the attribute, while
`type Pair = [value: typeof process]` and the `node:fs` beside an attribute
stay reported either way.
@taras
taras merged commit a534ed7 into main Aug 10, 2026
16 checks passed
@taras
taras deleted the agent/issue-365-7-crash-boundary branch August 10, 2026 08:30
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.

Commit Workspace mutations and journal results atomically

1 participant