feat(cli): define Zoo protocol contracts - #1150
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded the ChangesZoo protocol package
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant Parent
participant ProtocolNegotiation
participant HostCommand
participant HostEventParser
participant PublicStream
participant RootResult
Parent->>ProtocolNegotiation: Send versions and required capabilities
ProtocolNegotiation-->>Parent: Return negotiated version or incompatibility
Parent->>HostCommand: Submit validated command
HostCommand->>HostEventParser: Receive ordered host events
HostEventParser->>PublicStream: Release redacted stream events
PublicStream->>RootResult: Validate authoritative root settlement
RootResult-->>Parent: Return outcome and exit metadata
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/zoo-protocol/src/redaction.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
packages/zoo-protocol/eslint.config.mjs (1)
1-4: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType the flat configuration as an array.
packages/config-eslint/base.jsexportsconfigas an array, and[...config]remains an array. The JSDoc declaresLinter.Config, which describes one config object. Use the repository's flat-config array type.Proposed fix
-/** `@type` {import("eslint").Linter.Config} */ +/** `@type` {import("eslint").Linter.Config[]} */Verify the exact type name against the installed ESLint declarations before applying the change.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/zoo-protocol/eslint.config.mjs` around lines 1 - 4, Update the JSDoc type annotation above the default export in eslint.config.mjs from the single-config Linter.Config type to ESLint’s flat-config array type, verifying the exact declaration name in the installed ESLint types. Keep the existing [...config] export unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/zoo-protocol/src/parity.ts`:
- Around line 3-11: Update SemanticTraceEntry to a discriminated union keyed by
literal type values, requiring the fields appropriate to each event;
specifically make task.result entries require a ZooOutcome outcome and preserve
required task identifiers where applicable. Update the validator to narrow on
the discriminant and reject task.result entries without outcome, including root
results.
- Around line 61-65: Replace the JSON.stringify-based comparison in the parity
comparison function with order-independent deep structural equality, normalizing
entries if needed so equivalent objects match regardless of property insertion
order. Preserve the existing { ok: true } result for equal values and generate
the difference message from normalized or structurally comparable
representations for unequal values.
In `@packages/zoo-protocol/src/public-events.ts`:
- Around line 162-165: Update validateStreamLifecycle in
packages/zoo-protocol/src/public-events.ts around lines 162-165 to count exactly
one task.result whose taskId equals rootTaskId, require that root result to be
the final event, and verify result.rootTaskId matches the event’s root task ID.
Extend packages/zoo-protocol/src/__tests__/contracts.test.ts lines 114-147 with
cases covering a child result followed by a root result, a child-only terminal
result, and mismatched event/result root task IDs.
In `@packages/zoo-protocol/src/redaction.ts`:
- Around line 20-29: Update redactValue so seen tracks only the current
recursion path: remove each processed object from seen after its array or object
branch completes, while preserving the circular-reference check. Add a
regression test covering repeated references such as the same child assigned to
left and right, ensuring both branches are fully redacted rather than marking
the second as circular.
---
Nitpick comments:
In `@packages/zoo-protocol/eslint.config.mjs`:
- Around line 1-4: Update the JSDoc type annotation above the default export in
eslint.config.mjs from the single-config Linter.Config type to ESLint’s
flat-config array type, verifying the exact declaration name in the installed
ESLint types. Keep the existing [...config] export unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6be73ecc-3a4b-421c-9f2a-a1ed63e68876
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (15)
AGENTS.mdCLAUDE.mdpackages/zoo-protocol/eslint.config.mjspackages/zoo-protocol/package.jsonpackages/zoo-protocol/src/__tests__/contracts.test.tspackages/zoo-protocol/src/host-commands.tspackages/zoo-protocol/src/host-events.tspackages/zoo-protocol/src/index.tspackages/zoo-protocol/src/outcomes.tspackages/zoo-protocol/src/parity.tspackages/zoo-protocol/src/public-events.tspackages/zoo-protocol/src/redaction.tspackages/zoo-protocol/src/version.tspackages/zoo-protocol/tsconfig.jsonpackages/zoo-protocol/vitest.config.ts
| export type SemanticTraceEntry = { | ||
| type: string | ||
| taskId?: string | ||
| parentTaskId?: string | ||
| toolCallId?: string | ||
| content?: string | ||
| outcome?: ZooOutcome | ||
| errorCode?: ZooErrorCode | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Enforce the required shape of task.result entries.
Every field except type is optional. This value type-checks and passes the validator:
{ type: "task.result", taskId: "root" }The validator then returns true without a ZooOutcome. Define SemanticTraceEntry as a discriminated union with literal event types and required fields. At minimum, reject a root result whose outcome is undefined.
Minimum validation fix
- return results.length === 1 && results[0]?.taskId === rootTaskId
+ return (
+ results.length === 1 &&
+ results[0]?.taskId === rootTaskId &&
+ results[0]?.outcome !== undefined
+ )Also applies to: 68-70
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/zoo-protocol/src/parity.ts` around lines 3 - 11, Update
SemanticTraceEntry to a discriminated union keyed by literal type values,
requiring the fields appropriate to each event; specifically make task.result
entries require a ZooOutcome outcome and preserve required task identifiers
where applicable. Update the validator to narrow on the discriminant and reject
task.result entries without outcome, including root results.
github-actions
Bot
added
awaiting-review
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (10)
packages/zoo-protocol/src/parity.ts (2)
260-261: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse prefix-length expressions instead of hand-computed offsets.
Line 261, Line 319, and Line 327 use the literals
4,5, and8to strip theask:,fail:, andtimeout:prefixes. Line 244 already uses the clearer"delegate:".lengthform. A wrong literal does not throw. It produces a truncated identifier or an identifier that still contains part of the prefix. Use the same form in all four places.♻️ Proposed refactor
- const askId = turn.slice(4) + const askId = turn.slice("ask:".length)- const errorCode = failedErrorCodeSchema.parse(turn.slice(5)) + const errorCode = failedErrorCodeSchema.parse(turn.slice("fail:".length))- const errorCode = zooErrorCodeSchema.parse(turn.slice(8)) + const errorCode = zooErrorCodeSchema.parse(turn.slice("timeout:".length))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/zoo-protocol/src/parity.ts` around lines 260 - 261, Replace the hand-computed slice offsets in the parity parsing logic with the corresponding prefix string length expressions for the ask:, fail:, and timeout: cases, matching the existing delegate: handling. Update the slices associated with askId and the other parsed identifiers while preserving their current behavior.
350-350: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReject malformed directives instead of treating them as message content.
Line 350 is an unconditional fallthrough. Any turn that does not match a directive becomes
message.upsertcontent. A typo in a fixture, for exampletol:read_file:call-1:README.mdorcancelwithout fields, becomes a plain message rather than an error. This harness is a parity oracle, so a silent reinterpretation produces a confusing trace diff instead of a clear fixture error.Guard the fallthrough against known directive prefixes.
♻️ Proposed refactor
+ if (/^[a-z_]+:/.test(turn)) throw new Error(`Unknown fake-provider directive: ${turn}`) trace.push({ type: "message.upsert", rootTaskId: "root", taskId: "root", content: turn })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/zoo-protocol/src/parity.ts` at line 350, Update the turn-processing logic in parity.ts so the unconditional message.upsert fallthrough rejects strings beginning with known directive prefixes when they fail directive parsing, rather than recording them as message content. Preserve message.upsert for ordinary non-directive turns, and report malformed directive input as a clear fixture error.packages/zoo-protocol/src/__tests__/contracts.test.ts (1)
2454-2469: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert error messages in the negative parity cases.
toThrow()without an argument passes for any thrown error. A fixture typo or an unrelatedTypeErrorwould still satisfy these assertions. Line 2453 already pins the message withtoThrow("Invalid tool fixture"). Apply the same precision to the other rejection cases in this file (Lines 2461, 2469, 2482, 2500, 2508, 2517, 2533).♻️ Proposed change for two of the cases
expect(() => runDeterministicFakeProvider({ id: "invalid-failure", prompt: "Fail", providerTurns: ["fail:task_timed_out"], expected: [], }), - ).toThrow() + ).toThrow(/task_timed_out/) expect(() => runDeterministicFakeProvider({ id: "trailing", prompt: "Cancel", providerTurns: ["cancel:cancel-1:user", "trailing"], expected: [], }), - ).toThrow() + ).toThrow("Fake-provider terminal directives must be the final turn")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/zoo-protocol/src/__tests__/contracts.test.ts` around lines 2454 - 2469, Update the negative parity assertions in contracts.test.ts so the existing runDeterministicFakeProvider cases use message-specific toThrow checks instead of bare toThrow. Keep the current fixtures and expectations in each test, but pin the thrown error text for the invalid-failure and trailing scenarios and the other listed rejection cases in this file, matching the already-precise "Invalid tool fixture" assertion pattern used earlier.packages/zoo-protocol/src/outcomes.ts (1)
93-96: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRead all fields from the parsed value.
Line 94 takes
outcomefromexitContextSchema.parse(context). Lines 95-96 takeerrorCodeandsignalfrom the unparsedcontext. The two sources are identical today, because the schema applies no transform or coercion. If a transform or default is added later,errorCodeandsignalbypass it.Destructure all three fields from the parse result.
♻️ Proposed refactor
export function exitCodeFor(context: ExitContext): number { - const { outcome } = exitContextSchema.parse(context) - const errorCode = "errorCode" in context ? context.errorCode : undefined - const signal = "signal" in context ? context.signal : undefined + const parsed = exitContextSchema.parse(context) + const { outcome } = parsed + const errorCode = "errorCode" in parsed ? parsed.errorCode : undefined + const signal = "signal" in parsed ? parsed.signal : undefined🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/zoo-protocol/src/outcomes.ts` around lines 93 - 96, Update exitCodeFor to destructure outcome, errorCode, and signal from the single exitContextSchema.parse(context) result, removing reads of errorCode and signal from the unparsed context.packages/zoo-protocol/src/host-events.ts (3)
248-249: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow the
sanitizeNonEventparameter instead of casting.
sanitizeNonEventaccepts the full raw union and returnsHostEventthroughas HostEvent. The cast hides the redaction contract. The current call site at line 285 passes only non-eventenvelopes, so behavior is correct today. If a future branch passes aneventenvelope, the cast returns an unredacted raw stream event typed as a redactedHostEvent, and the compiler does not report it.Exclude the
eventvariant in the parameter type.♻️ Proposed refactor
- const sanitizeNonEvent = (event: z.infer<typeof rawHostEventDiscriminatedSchema>): HostEvent => + const sanitizeNonEvent = ( + event: Exclude<z.infer<typeof rawHostEventDiscriminatedSchema>, { type: "event" }>, + ): HostEvent =>Also applies to: 284-288
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/zoo-protocol/src/host-events.ts` around lines 248 - 249, Update sanitizeNonEvent to accept a type that excludes the raw event envelope variant, rather than the full raw host-event union, and remove the HostEvent cast from its return path. Keep the existing non-event call site behavior unchanged while ensuring future event-envelope calls are rejected by the compiler.
248-258: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse one
command.errorredaction helper.
sanitizeNonEventrepeats the redaction body of thehostEventSchematransform at lines 103-114. The two copies must stay identical, because both produceHostEventvalues that callers treat as redacted. A new sensitive field added tozooErrorSchemamust be handled in both places.Extract the redaction into one function and call it from both sites.
♻️ Proposed refactor
+const redactCommandError = <T extends { error: { message: string; phase?: string } }>(event: T): T => ({ + ...event, + error: { + ...event.error, + message: redactText(event.error.message), + phase: event.error.phase === undefined ? undefined : redactText(event.error.phase), + }, +}) + export const hostEventSchema = hostEventDiscriminatedSchema .superRefine((event, context) => { if (event.type === "event" && event.event.hostId !== event.hostId) { context.addIssue({ code: z.ZodIssueCode.custom, message: "Normalized event hostId must match its host envelope" }) } }) - .transform((event) => - event.type === "command.error" - ? { - ...event, - error: { - ...event.error, - message: redactText(event.error.message), - phase: event.error.phase === undefined ? undefined : redactText(event.error.phase), - }, - } - : event, - ) + .transform((event) => (event.type === "command.error" ? redactCommandError(event) : event))const sanitizeNonEvent = (event: z.infer<typeof rawHostEventDiscriminatedSchema>): HostEvent => - event.type === "command.error" - ? { - ...event, - error: { - ...event.error, - message: redactText(event.error.message), - phase: event.error.phase === undefined ? undefined : redactText(event.error.phase), - }, - } - : event as HostEvent + event.type === "command.error" ? redactCommandError(event) : (event as HostEvent)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/zoo-protocol/src/host-events.ts` around lines 248 - 258, Extract the duplicated command.error redaction object construction from sanitizeNonEvent and the hostEventSchema transform into a shared helper function. Have both call sites use that helper so all HostEvent redaction, including future zooErrorSchema fields, remains consistent.
193-207: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare one stream-event key function with
public-events.ts.
eventKeyduplicatesstreamEventKeyinpackages/zoo-protocol/src/public-events.ts(lines 293-307). Both must produce identical keys, becauseassignmatches redactor output to queued envelopes by key. A field added to one copy and not the other silently breaks envelope matching and makesassignthrow "Missing host envelope for buffered Zoo stream event".Export
streamEventKeyfrompublic-events.tsand import it here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/zoo-protocol/src/host-events.ts` around lines 193 - 207, Remove the duplicated eventKey implementation in the host-events stream handling and import the existing streamEventKey from public-events.ts. Export streamEventKey from public-events.ts, then use that shared function wherever host-events currently computes keys so assign matches redacted events and queued envelopes consistently.packages/zoo-protocol/src/public-events.ts (1)
283-286: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the pass-through branch explicit.
default: return streamEventpasses any unhandled event type through without redaction. Every current type that reaches this branch carries only identifiers, enums, numbers, or an already-redactedresult, so the output is correct today.A new event type with a free-text field is emitted unredacted unless the author adds a case. This is a redaction boundary, so make the pass-through set explicit and let the compiler report a new type.
List the pass-through types as explicit cases and assert exhaustiveness with a
nevercheck indefault.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/zoo-protocol/src/public-events.ts` around lines 283 - 286, Update the event redaction switch surrounding the default branch to enumerate every currently pass-through event type as explicit cases returning streamEvent. Replace the unconditional default return with an exhaustiveness check that accepts never, so adding a new event type requires an explicit redaction decision.packages/zoo-protocol/src/redaction.ts (2)
3-4: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the duplicate secret-value pattern.
secretValueandcliSecretValuehave identical definitions. The two names imply a difference that does not exist. A future edit to one changes only some call sites.Keep one constant, or give
cliSecretValuethe shell-specific value grammar it implies.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/zoo-protocol/src/redaction.ts` around lines 3 - 4, Remove the duplicated secret-value pattern in redaction.ts by updating the constants near secretValue and cliSecretValue so they no longer share the same definition. Either reuse a single shared constant for both call sites, or change cliSecretValue to a distinct shell-specific grammar if that is the intended behavior. Keep the existing redaction symbols and replace only the redundant pattern source so future edits cannot diverge silently.
78-83: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueHoist the recompiled terminal-control pattern.
Line 80 rebuilds a regex from
terminalControl.sourceon every call.redactTextcalls this function for every string field of every event, so the pattern is recompiled on a hot path.
String.prototype.matchAlldoes not mutate the source regexlastIndex, so a module-level global constant is safe here.♻️ Proposed refactor
+const terminalControlGlobal = new RegExp(terminalControl.source, "g") + export function requiresFailClosedRedaction(value: string): boolean { if (unsafeTerminalEditing.test(value) || /(?:\r(?!\n)|[\v\f])/.test(value)) return true - return [...value.matchAll(new RegExp(terminalControl.source, "g"))].some(([control]) => + return [...value.matchAll(terminalControlGlobal)].some(([control]) => containsSensitiveAssignment(control), ) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/zoo-protocol/src/redaction.ts` around lines 78 - 83, Hoist the recompiled terminal-control pattern out of requiresFailClosedRedaction so the hot path does not create a new RegExp on every call. Add a module-level constant derived from terminalControl.source and reuse it inside requiresFailClosedRedaction, keeping the existing matchAll and containsSensitiveAssignment behavior unchanged. Ensure the shared regex is safe to reuse across calls, since matchAll does not mutate lastIndex.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/zoo-protocol/src/host-events.ts`:
- Around line 155-192: Update inputSize to track only the active ancestor path:
remove each object from seen after its array or object branch has finished
processing, while retaining detection for objects encountered again before their
branch completes. Preserve the existing depth and size-limit behavior and follow
the cleanup pattern used by redactValue.
In `@packages/zoo-protocol/src/public-events.ts`:
- Around line 462-486: Update the terminal.output aggregation path in
public-events so it no longer rescans the full pending buffer on every delta.
Reuse a hoisted TextEncoder and track pending byte length incrementally instead
of calling new TextEncoder().encode(pending.text), replace the newline boundary
check on pending.text with an endsWith("\n")-style check, and avoid recomputing
pending.pem from canonicalizeRedactionText(pending.text) on every push by
updating PEM state incrementally or only evaluating it when the buffer ends at a
newline. Keep the existing overflow and emit(key) behavior in the same pending
state handling block.
- Around line 720-741: The `hostSequenceFor` and `causalTerminal` helpers are
doing repeated linear scans over `events` and `commandEvents`, which makes the
validation path quadratic. Precompute a reference-keyed event-to-sequence map
and a commandId-to-lifecycle map once before the per-event and final-pass loops,
then update `hostSequenceFor` and `causalTerminal` to read from those caches
instead of calling `indexOf` and `filter`. Preserve the current semantics for
duplicate event objects by keeping the first observed sequence, and keep the
existing ack/terminal ordering checks unchanged in `public-events.ts`.
In `@packages/zoo-protocol/src/redaction.ts`:
- Around line 22-32: Update redactText to cap input text length before applying
secretPatterns, truncating oversized content and appending “[REDACTED]” before
returning. Preserve existing redaction behavior for text within the limit, and
ensure the cap applies to all content processed through redactText.
---
Nitpick comments:
In `@packages/zoo-protocol/src/__tests__/contracts.test.ts`:
- Around line 2454-2469: Update the negative parity assertions in
contracts.test.ts so the existing runDeterministicFakeProvider cases use
message-specific toThrow checks instead of bare toThrow. Keep the current
fixtures and expectations in each test, but pin the thrown error text for the
invalid-failure and trailing scenarios and the other listed rejection cases in
this file, matching the already-precise "Invalid tool fixture" assertion pattern
used earlier.
In `@packages/zoo-protocol/src/host-events.ts`:
- Around line 248-249: Update sanitizeNonEvent to accept a type that excludes
the raw event envelope variant, rather than the full raw host-event union, and
remove the HostEvent cast from its return path. Keep the existing non-event call
site behavior unchanged while ensuring future event-envelope calls are rejected
by the compiler.
- Around line 248-258: Extract the duplicated command.error redaction object
construction from sanitizeNonEvent and the hostEventSchema transform into a
shared helper function. Have both call sites use that helper so all HostEvent
redaction, including future zooErrorSchema fields, remains consistent.
- Around line 193-207: Remove the duplicated eventKey implementation in the
host-events stream handling and import the existing streamEventKey from
public-events.ts. Export streamEventKey from public-events.ts, then use that
shared function wherever host-events currently computes keys so assign matches
redacted events and queued envelopes consistently.
In `@packages/zoo-protocol/src/outcomes.ts`:
- Around line 93-96: Update exitCodeFor to destructure outcome, errorCode, and
signal from the single exitContextSchema.parse(context) result, removing reads
of errorCode and signal from the unparsed context.
In `@packages/zoo-protocol/src/parity.ts`:
- Around line 260-261: Replace the hand-computed slice offsets in the parity
parsing logic with the corresponding prefix string length expressions for the
ask:, fail:, and timeout: cases, matching the existing delegate: handling.
Update the slices associated with askId and the other parsed identifiers while
preserving their current behavior.
- Line 350: Update the turn-processing logic in parity.ts so the unconditional
message.upsert fallthrough rejects strings beginning with known directive
prefixes when they fail directive parsing, rather than recording them as message
content. Preserve message.upsert for ordinary non-directive turns, and report
malformed directive input as a clear fixture error.
In `@packages/zoo-protocol/src/public-events.ts`:
- Around line 283-286: Update the event redaction switch surrounding the default
branch to enumerate every currently pass-through event type as explicit cases
returning streamEvent. Replace the unconditional default return with an
exhaustiveness check that accepts never, so adding a new event type requires an
explicit redaction decision.
In `@packages/zoo-protocol/src/redaction.ts`:
- Around line 3-4: Remove the duplicated secret-value pattern in redaction.ts by
updating the constants near secretValue and cliSecretValue so they no longer
share the same definition. Either reuse a single shared constant for both call
sites, or change cliSecretValue to a distinct shell-specific grammar if that is
the intended behavior. Keep the existing redaction symbols and replace only the
redundant pattern source so future edits cannot diverge silently.
- Around line 78-83: Hoist the recompiled terminal-control pattern out of
requiresFailClosedRedaction so the hot path does not create a new RegExp on
every call. Add a module-level constant derived from terminalControl.source and
reuse it inside requiresFailClosedRedaction, keeping the existing matchAll and
containsSensitiveAssignment behavior unchanged. Ensure the shared regex is safe
to reuse across calls, since matchAll does not mutate lastIndex.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 77d2b3d4-18cd-417d-856e-6f071ce2c6d8
📒 Files selected for processing (12)
packages/zoo-protocol/eslint-suppressions.jsonpackages/zoo-protocol/package.jsonpackages/zoo-protocol/src/__tests__/contracts.test.tspackages/zoo-protocol/src/command-lifecycle.tspackages/zoo-protocol/src/host-commands.tspackages/zoo-protocol/src/host-events.tspackages/zoo-protocol/src/index.tspackages/zoo-protocol/src/outcomes.tspackages/zoo-protocol/src/parity.tspackages/zoo-protocol/src/public-events.tspackages/zoo-protocol/src/redaction.tspackages/zoo-protocol/src/version.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/zoo-protocol/package.json
- packages/zoo-protocol/src/index.ts
- packages/zoo-protocol/src/host-commands.ts
github-actions
Bot
added
the
awaiting-review
150db93 to
978e2d8
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/zoo-protocol/src/__tests__/contracts.test.ts (2)
2338-2361: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSelect parity scenarios by
idinstead of array position.Lines 2339, 2341, 2348, and 2361 index
parityScenariosby position. The assertions depend on which scenario sits at each index. If someone inserts or reorders a scenario inparity.ts, these tests check a different scenario and fail with an unclear message. Look up each scenario by itsid.♻️ Proposed helper
+const scenarioById = (id: string) => { + const scenario = parityScenarios.find((candidate) => candidate.id === id) + if (scenario === undefined) throw new Error(`Unknown parity scenario ${id}`) + return scenario +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/zoo-protocol/src/__tests__/contracts.test.ts` around lines 2338 - 2361, Update the tests around the parityScenarios references in “includes the prompt in fake-provider semantics,” “includes tool identity and arguments in fake-provider semantics,” and “detects child completion incorrectly settling the root” to select scenarios by their id rather than array position. Reuse the looked-up scenario consistently for both expected traces and provider inputs, preserving the existing assertions.
2446-2535: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the expected error for each rejection case.
Line 2453 asserts
.toThrow("Invalid tool fixture"). The remaining rejection cases use a bare.toThrow(). A bare.toThrow()passes on any thrown error, including an unrelatedTypeErrorfrom a future refactor ofrunDeterministicFakeProvider. Add a message matcher to each case so the test proves the intended validation fired.The same applies to lines 2480-2482, 2493-2500, 2501-2508, 2515-2517, and 2526-2533.
♻️ Example for the invalid-failure case
expect(() => runDeterministicFakeProvider({ id: "invalid-failure", prompt: "Fail", providerTurns: ["fail:task_timed_out"], expected: [], }), - ).toThrow() + ).toThrow(/timeout error code/i)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/zoo-protocol/src/__tests__/contracts.test.ts` around lines 2446 - 2535, Replace every bare .toThrow() in the rejection tests around runDeterministicFakeProvider with message matchers for the specific validation error each fixture is intended to trigger, including invalid-failure, trailing, unresolved states, ghost, duplicate, reused identifiers, and extra approval/cancellation fields. Preserve the existing .toThrow("Invalid tool fixture") assertion and use the validation messages exposed by the implementation rather than broad or empty matchers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/zoo-protocol/src/__tests__/contracts.test.ts`:
- Around line 2338-2361: Update the tests around the parityScenarios references
in “includes the prompt in fake-provider semantics,” “includes tool identity and
arguments in fake-provider semantics,” and “detects child completion incorrectly
settling the root” to select scenarios by their id rather than array position.
Reuse the looked-up scenario consistently for both expected traces and provider
inputs, preserving the existing assertions.
- Around line 2446-2535: Replace every bare .toThrow() in the rejection tests
around runDeterministicFakeProvider with message matchers for the specific
validation error each fixture is intended to trigger, including invalid-failure,
trailing, unresolved states, ghost, duplicate, reused identifiers, and extra
approval/cancellation fields. Preserve the existing .toThrow("Invalid tool
fixture") assertion and use the validation messages exposed by the
implementation rather than broad or empty matchers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c184f4ba-537a-4bb4-998a-b15bc3ea0dcc
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (17)
AGENTS.mdCLAUDE.mdpackages/zoo-protocol/eslint-suppressions.jsonpackages/zoo-protocol/eslint.config.mjspackages/zoo-protocol/package.jsonpackages/zoo-protocol/src/__tests__/contracts.test.tspackages/zoo-protocol/src/command-lifecycle.tspackages/zoo-protocol/src/host-commands.tspackages/zoo-protocol/src/host-events.tspackages/zoo-protocol/src/index.tspackages/zoo-protocol/src/outcomes.tspackages/zoo-protocol/src/parity.tspackages/zoo-protocol/src/public-events.tspackages/zoo-protocol/src/redaction.tspackages/zoo-protocol/src/version.tspackages/zoo-protocol/tsconfig.jsonpackages/zoo-protocol/vitest.config.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- packages/zoo-protocol/eslint-suppressions.json
- CLAUDE.md
- AGENTS.md
- packages/zoo-protocol/tsconfig.json
- packages/zoo-protocol/vitest.config.ts
- packages/zoo-protocol/package.json
- packages/zoo-protocol/eslint.config.mjs
- packages/zoo-protocol/src/outcomes.ts
- packages/zoo-protocol/src/index.ts
- packages/zoo-protocol/src/parity.ts
- packages/zoo-protocol/src/host-commands.ts
- packages/zoo-protocol/src/host-events.ts
github-actions
Bot
removed
the
awaiting-review
github-actions
Bot
added
the
awaiting-review
2 participants
Stack
Position 1 of 6 in the Zoo CLI stack.
mainfm/zoo-cli-headless-api(planned)gh stack initand local stack tracking succeeded;gh stack submit --auto --open --remote originreturnedStacked PRs are not enabled for this repository, so this chain uses the approved classic base-branch fallback while preserving native local ancestry.Scope
@roo-code/zoo-protocolwith strict versioned host commands/events and public result/NDJSON schemas.apps/cliunchanged.Acceptance Evidence
pnpm --dir packages/zoo-protocol test(16 tests)pnpm --dir packages/zoo-protocol check-typespnpm --dir packages/zoo-protocol exec eslint --prune-suppressions --max-warnings=0 srcpnpm --dir packages/zoo-protocol buildRisk
This PR defines private contracts only and does not activate the extension or expose the
zooexecutable. The main compatibility risk is prematurely freezing schemas; protocol and public schema versions are explicit so later breaking changes fail negotiation rather than parsing ambiguously.flowchart LR P[zoo parent] -->|strict HostCommand v1| C[@roo-code/zoo-protocol] C -->|validated command| H[zoo-host] H -->|ACK + DONE/ERROR| C H -->|monotonic normalized events| C C --> R[one authoritative root result] C --> N[stdout-pure NDJSON]Summary by CodeRabbit
New Features
Tests