Skip to content

Replace broad record types with accurate contracts - #1619

Open
SawyerHood wants to merge 4 commits into
mainfrom
bb/find-all-record-usages-thr_nbghhtnurj
Open

Replace broad record types with accurate contracts#1619
SawyerHood wants to merge 4 commits into
mainfrom
bb/find-all-record-usages-thr_nbghhtnurj

Conversation

@SawyerHood

@SawyerHood SawyerHood commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Replace all 366 Record<string, unknown> usages with data-specific types.
  • Use schema-derived JSON types, exact contract candidates, typed fixtures, and explicit boundary objects.
  • Add the bb/no-record-string-unknown ESLint rule across all TypeScript packages.
  • Run the record-type guard as a cached Turbo CI task.
  • Regenerate the plugin SDK declarations, registry data, and template outputs.

Runtime changes

  • Reject invalid stored-event payloads at the boundary.
  • Validate and encode outbound JSON-RPC values in one pass.
  • Reject BigInt, functions, symbols, cycles, excessive depth, excessive nodes, and excessive JSON size.
  • End a failed provider turn when provider event translation fails.
  • Increment the host daemon protocol version to 124.

Why

The broad record type removed useful field information and allowed invalid internal values. The new types preserve each contract and narrow untrusted data at its boundary.

Validation

  • pnpm exec turbo run typecheck — 58 of 58 tasks passed.
  • pnpm exec turbo run '//#lint:record-types' passed across all TypeScript packages.
  • A repeat record-type guard used the Turbo cache.
  • pnpm run lint passed with 144 existing warnings and no errors.
  • Focused domain, runtime, contract, and event-sink tests — 109 of 109 tests passed.
  • Project-wide search — zero Record<string, unknown> matches.
  • git diff --check passed.

The earlier full local test command found host-only failures. This host lacks Electron and browser storage. It also creates test files with mode 0664 instead of 0644.

AGENT GENERATED: by GPT-5.6

@bb-slop-cop

bb-slop-cop Bot commented Aug 14, 2026

Copy link
Copy Markdown

🚨 SLOP COP 🚨 · review

I am SlopCop. I am reviewing this pull request under the review rule.

I will check security, code quality, performance, duplicate code, architecture, and the main product path.

@SawyerHood
SawyerHood force-pushed the bb/find-all-record-usages-thr_nbghhtnurj branch from c4d8052 to 34613e3 Compare August 14, 2026 20:30
Comment thread packages/domain/src/stored-thread-event.ts Outdated
Comment thread packages/agent-runtime/src/runtime-json-rpc.ts
Comment thread apps/server/src/services/threads/thread-data.ts Outdated
Comment thread packages/agent-runtime/src/shared/adapter-utils.ts Outdated

@bb-slop-cop bb-slop-cop 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.

🚨 SLOP COP 🚨 · review

Plain-English summary

This PR replaces one broad object type with names for the data that each part expects.

It mainly changes TypeScript contracts across providers, the server, the app, and plugins. Most changes improve compiler checks.

However, two new recursive checks add runtime work. One check can stop the host daemon.

Findings

  1. High — Provider output can stop the host daemon.

    adapter-utils.ts:246 now walks every nested provider argument. A 20,002-byte object at depth 2,000 throws a RangeError.

    The runtime does not catch this error. One malformed provider event can stop the daemon and interrupt all threads.

    Classify parsed data once at the boundary. Use a shallow object check internally. Catch adapter errors at the runtime boundary.

  2. Medium — Stored event parsing repeats recursive work.

    thread-data.ts:51 validates and copies each event after JSON.parse(). parseStoredThreadEvent() then repeats that work.

    The timeline path can process 1,500 rows. A synthetic check increased 1,500 events from 185 ms to 326 ms.

    Restore the shallow top-level check. Keep one schema validation in parseStoredThreadEvent().

  3. Low — The JSON-RPC contract remains broad.

    JsonRpcObject still equals the old broad record type. Outbound fields can contain values that JSON cannot represent.

    Use separate types for unknown inbound data. Use JsonValue and JsonObject for outbound messages.

  4. Low — The stored event contract accepts arrays.

    StoredThreadEventParseArgs.data now uses object. This type accepts arrays.

    parseStoredThreadEvent() accepts [] for thread/context/cleared and returns a valid event.

    Reject non-record data at the database boundary. Use a type-correlated event data union internally.

Architecture and duplicate work

The PR moves one broad contract into JsonRpcObject without making it precise.

It also adds a recursive check before an existing recursive check. Use one boundary check and one internal typed value.

Validation

  • All required GitHub CI jobs passed.
  • The full Turbo typecheck passed all 58 tasks.
  • Three workers ran focused package, server, app, domain, runtime, database, SDK, and plugin tests.
  • The dev app loaded the home, plugin browser, installed plugins, and Workflows detail routes.
  • The browser reported no console errors or page errors.
  • git diff --check passed.
  • Two external scaffold tests did not start because an external test hook timed out.

Review status: Comment only. Fix the daemon crash before merge.

@SawyerHood
SawyerHood force-pushed the bb/find-all-record-usages-thr_nbghhtnurj branch 2 times, most recently from 51f6c60 to 2935fab Compare August 14, 2026 20:59
@bb-slop-cop

bb-slop-cop Bot commented Aug 14, 2026

Copy link
Copy Markdown

🚨 SLOP COP 🚨 · review

I am the Slop Cop. I started a review of PR #1619 under the review rule.

I will check security, code quality, performance, architecture, duplication, and an end-to-end path.

@bb-slop-cop bb-slop-cop 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.

🚨 SLOP COP 🚨 · review

Plain-English summary: This pull request replaces broad key-value object types with contracts that describe each value more accurately.

I am SlopCop. I will review security, code quality, performance, architecture, duplication, and user behavior.

Comment thread packages/agent-runtime/src/runtime.ts
Comment thread packages/domain/src/thread-events.ts
Comment thread packages/domain/src/provider-event.ts

@bb-slop-cop bb-slop-cop 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.

🚨 SLOP COP 🚨 · review

Plain-English summary

This PR replaces generic object types with contracts that name the data each part expects.

It also adds a CI check that stops new uses of one broad record form.

Most changes improve type checks without changing product behavior.

However, the new event rules can stop event delivery. Another new error path can leave a turn active.

Findings

  1. High — Deep tool arguments can block the host event queue.

    provider-event.ts:350 uses recursive JSON validation without a depth limit.

    A valid 12 KB JSON object at depth 2,000 throws RangeError.

    The server returns 500. The daemon keeps the event at the queue head because it treats the failure as temporary.

    Later events for every thread on that host remain queued.

  2. Medium — A translation failure can leave a turn active forever.

    runtime.ts:1150 writes an error and returns.

    If turn/completed fails, the runtime never clears the active turn.

    The session then stays active and cannot enter normal idle cleanup.

  3. Medium — The daemon protocol version does not include the wire change.

    The PR changes daemon event data at provider-event.ts:350 and thread-events.ts:292.

    HOST_DAEMON_PROTOCOL_VERSION remains 123. An old daemon can connect to a server with the stricter event contract.

Architecture and duplicate code

The daemon should set provider input limits before it adds events to the queue.

The server should use stack-safe validation at its event boundary.

The one-use candidate types stay near their consumers. I found no material source duplication.

A synthetic 1 MiB JSON-RPC test was about 4.5 times slower. I found no normal-path evidence for a separate performance defect.

Validation

  • All required GitHub checks passed.
  • The record-type lint passed in 13.8 seconds.
  • The agent-runtime run passed 958 tests.
  • The domain run passed 143 tests.
  • The server run passed 1,650 tests.
  • The environment blocked 16 other server tests through cache permissions and file-watch limits.
  • The plugin list and Provider retry settings path loaded without browser console errors.
  • The GPT-5.6 final gate confirmed the three findings.

Review result: Comment only. Fix these three findings before merge.

Comment thread packages/agent-runtime/src/runtime.ts
Comment thread packages/templates/src/plugin-scaffold.ts Outdated
Comment thread packages/agent-runtime/src/runtime-json-rpc.ts Outdated
Comment thread .github/workflows/ci.yml Outdated

@bb-slop-cop bb-slop-cop 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.

🚨 SLOP COP 🚨 · review

Plain-English summary

This PR replaces a vague TypeScript object type with contracts that name the expected data.

It also adds a repository rule and tighter checks for provider messages and stored events.

Review results

  • Medium: A translation error can leave an active provider turn stuck forever.
  • Medium: Outbound JSON-RPC validation clones each payload and makes large writes more than twice as slow.
  • Low: The repository lint guard adds an uncached ESLint pass and repeats app checks.
  • Low: The plugin scaffold duplicates JSON types already exported by @bb/domain.

I found no security issue.

Validation

  • All GitHub checks pass at the review SHA.
  • Three reviewers checked security, quality, architecture, duplication, performance, and browser behavior.
  • A final GPT-5.6 reviewer confirmed the results.
  • 958 agent-runtime tests and 143 domain tests passed.
  • 36 focused security tests passed.
  • Four package typechecks passed.
  • The new record-type lint passed.
  • The dev app opened the home and settings routes without console or page errors.
  • git diff --check passed.
  • The worktree remains clean.

Result

The turn-state issue can leave a user thread active until a manual stop. Fix it before merge.

I posted this review as a comment only. I did not approve or request changes.

@SawyerHood
SawyerHood force-pushed the bb/find-all-record-usages-thr_nbghhtnurj branch from 2935fab to cb19948 Compare August 14, 2026 21:46
@bb-slop-cop

bb-slop-cop Bot commented Aug 14, 2026

Copy link
Copy Markdown

🚨 SLOP COP 🚨 · review

I am SlopCop. I am now reviewing this pull request for security, code quality, performance, architecture, and end-to-end behavior.

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.

1 participant