Replace broad record types with accurate contracts - #1619
Conversation
|
🚨 SLOP COP 🚨 · I am SlopCop. I am reviewing this pull request under the I will check security, code quality, performance, duplicate code, architecture, and the main product path. |
c4d8052 to
34613e3
Compare
There was a problem hiding this comment.
🚨 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
-
High — Provider output can stop the host daemon.
adapter-utils.ts:246now walks every nested provider argument. A 20,002-byte object at depth 2,000 throws aRangeError.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.
-
Medium — Stored event parsing repeats recursive work.
thread-data.ts:51validates and copies each event afterJSON.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(). -
Low — The JSON-RPC contract remains broad.
JsonRpcObjectstill equals the old broad record type. Outbound fields can contain values that JSON cannot represent.Use separate types for unknown inbound data. Use
JsonValueandJsonObjectfor outbound messages. -
Low — The stored event contract accepts arrays.
StoredThreadEventParseArgs.datanow usesobject. This type accepts arrays.parseStoredThreadEvent()accepts[]forthread/context/clearedand 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 --checkpassed.- Two external scaffold tests did not start because an external test hook timed out.
Review status: Comment only. Fix the daemon crash before merge.
51f6c60 to
2935fab
Compare
|
🚨 SLOP COP 🚨 · I am the Slop Cop. I started a review of PR #1619 under the I will check security, code quality, performance, architecture, duplication, and an end-to-end path. |
There was a problem hiding this comment.
🚨 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.
There was a problem hiding this comment.
🚨 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
-
High — Deep tool arguments can block the host event queue.
provider-event.ts:350uses 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.
-
Medium — A translation failure can leave a turn active forever.
runtime.ts:1150writes an error and returns.If
turn/completedfails, the runtime never clears the active turn.The session then stays active and cannot enter normal idle cleanup.
-
Medium — The daemon protocol version does not include the wire change.
The PR changes daemon event data at
provider-event.ts:350andthread-events.ts:292.HOST_DAEMON_PROTOCOL_VERSIONremains 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.
There was a problem hiding this comment.
🚨 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 --checkpassed.- 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.
2935fab to
cb19948
Compare
|
🚨 SLOP COP 🚨 · I am SlopCop. I am now reviewing this pull request for security, code quality, performance, architecture, and end-to-end behavior. |
Summary
Record<string, unknown>usages with data-specific types.bb/no-record-string-unknownESLint rule across all TypeScript packages.Runtime changes
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.pnpm run lintpassed with 144 existing warnings and no errors.Record<string, unknown>matches.git diff --checkpassed.The earlier full local test command found host-only failures. This host lacks Electron and browser storage. It also creates test files with mode
0664instead of0644.