feat(collect_worker_interface): add typed schema assertions for reque… - #274
Merged
Conversation
…st/response validation - Introduced `--assert-typed-schemas` flag to `collect_worker_interface.py` to ensure all functions have typed request and response schemas, preventing the use of permissive `AnyValue` schemas. - Implemented `_typed_schema_violations` function to identify and report untyped schemas during assertions. - Updated the main function to enforce schema checks when using the `--assert-file` option. - Enhanced tests to cover scenarios for typed schema validation, ensuring compliance with the new requirements. - Updated documentation to reflect the necessity of typed schemas in worker functions.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (6)
📒 Files selected for processing (116)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Contributor
skill-check — worker0 verified, 22 skipped (no docs/).
Four for four. Nicely done. |
📝 WalkthroughWalkthroughThe PR adds typed-schema validation to worker interface collection, converts several worker function surfaces from untyped JSON values to typed request and response structs, introduces schema catalogs and golden snapshot tests, updates documentation, and bumps affected crate versions. ChangesTyped wire schemas across workers
Estimated code review effort🎯 5 (Critical) | ⏱️ ~100 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
ytallo
approved these changes
Jun 16, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
llm-router/src/registry/resolve.rs (2)
136-146:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize nested config objects before mutating them.
Line 142 panics when the existing entry contains a non-object
providersvalue, and Line 146 can panic when the provider slice already exists as a non-object. Normalize those nested values the same way Line 133 normalizes the top-level entry.Proposed fix
- let providers = entry - .as_object_mut() - .expect("object") - .entry("providers") - .or_insert_with(|| json!({})); - let slice = providers - .as_object_mut() - .expect("object") - .entry(&req.id) - .or_insert_with(|| json!({})); + let entry_obj = entry.as_object_mut().expect("entry normalized to object"); + let providers = entry_obj.entry("providers").or_insert_with(|| json!({})); + if !providers.is_object() { + *providers = json!({}); + } + let providers = providers + .as_object_mut() + .expect("providers normalized to object"); + let slice = providers.entry(&req.id).or_insert_with(|| json!({})); + if !slice.is_object() { + *slice = json!({}); + } slice["credential"] = credential;🤖 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 `@llm-router/src/registry/resolve.rs` around lines 136 - 146, The code panics when the nested providers entry or the individual provider slice (accessed via req.id) contain non-object values. After retrieving the providers entry from line 141 and before calling as_object_mut() on it, normalize it to an empty object if it is not already an object. Similarly, after retrieving the slice entry accessed via req.id from line 145-146, normalize it to an empty object if needed before calling as_object_mut() and accessing its credential field. Apply the same normalization pattern used at the top level (checking and replacing non-object values) to prevent panics when these nested values exist but are not objects.
123-130:⚠️ Potential issue | 🔴 CriticalAdd type constraint to credential schema field.
The
credentialfield inUpdateCredentialRequestis declared as a bareserde_json::Valuewithout a schema constraint. The golden schema shows:{ "default": null, "description": "The credential object to store (provider-specific shape)." }This lacks a
typeconstraint, allowing any value or null at the schema level, even though line 124 enforcesis_object()at runtime. Other fields in the same struct (id,token) correctly include type constraints;credentialshould too. Add"type": "object"to the schema to enforce data integrity at validation time, not just at runtime.🤖 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 `@llm-router/src/registry/resolve.rs` around lines 123 - 130, The `credential` field in the `UpdateCredentialRequest` struct lacks a type constraint in its schema definition, allowing any value or null at the schema level even though runtime validation enforces `is_object()`. Add a `"type": "object"` constraint to the credential field's schema definition (alongside the existing description) to match the pattern used by other fields like `id` and `token` in the same struct, ensuring data integrity is enforced at validation time rather than only at runtime.llm-router/src/config/on_changed.rs (1)
70-77:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon’t abort a flush after it may have drained
pending.The stored handle is aborted on every later event, but the task drains
pendingbefore awaiting the provider refresh calls. If a new config event arrives while the previous flush is already firing triggers,abort()can cancel the loop afterpendingwas emptied, dropping refreshes for some changed providers. Track “armed debounce” separately from “active flush,” or clear/swap the handle before draining so only sleeping debounce tasks are cancelled.🤖 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 `@llm-router/src/config/on_changed.rs` around lines 70 - 77, The flush_task handle is being aborted on every new config event, but this can cancel a task that has already drained the pending queue and started executing provider refresh calls. Separate the tracking of the debounce sleep phase from the active flush execution: either maintain two separate handles (one for the debounce task and one for the active flush), or clear the task handle stored in flush_task before draining pending so that subsequent abort() calls only cancel tasks that are still sleeping and haven't started processing yet. Ensure that once the task begins draining pending items in the tokio::spawn block, it cannot be aborted by new config events.
🧹 Nitpick comments (3)
.github/scripts/tests/test_collect_assert_non_empty.py (1)
58-104: ⚡ Quick winAdd a regression test for non-array
functions.The suite currently covers missing key / empty list / typed-vs-untyped schemas, but not malformed non-list
functionspayloads. Adding this case will guard the assertion hardening path and prevent bypass regressions.🤖 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 @.github/scripts/tests/test_collect_assert_non_empty.py around lines 58 - 104, Add a new regression test method to the TestAssertTypedSchemas class to guard against malformed non-list functions payloads. Create a test (similar in structure to test_fails_on_empty_response_schema) that uses write_payload with functions as a non-list value (such as a dictionary or string instead of an array), calls _run() with the output file, and asserts that the script returns a non-zero exit code to ensure the assertion properly detects and rejects this malformed input.llm-router/src/types/router.rs (1)
349-360: ⚡ Quick winUse
CredentialforUpdateCredentialRequest.credentialto keep this contract typed end-to-end.Line 360 currently accepts arbitrary JSON (
Value) while Line 161 returns a typedCredential; this weakens the typed wire contract and can permit payloads that don’t round-trip through resolve/update flows.♻️ Proposed change
pub struct UpdateCredentialRequest { @@ - /// The credential object to store (provider-specific shape). - #[serde(default)] - pub credential: Value, + /// The credential object to store. + pub credential: Credential, }🤖 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 `@llm-router/src/types/router.rs` around lines 349 - 360, The credential field in the UpdateCredentialRequest struct is currently typed as Value (arbitrary JSON), but it should be strongly typed as Credential to maintain consistency with the typed Credential response and ensure the wire contract is fully typed end-to-end. Change the type of the credential field from Value to Credential in the UpdateCredentialRequest struct definition.llm-router/tests/golden/schemas/router.models.get.json (1)
8-20: Confirm:idandproviderare intentionally optional with empty-string defaults; clarify whether the resulting silent null responses align with spec expectations.Fields default to empty strings with no handler-side validation—requests with omitted or empty parameters will return
null(the "cold-window signal" per spec § Capability defaults), not a bad-request error. This matches the documented behavior in the handler comment, but the contract ambiguity remains: clients that accidentally omit these fields will not be alerted to the error. If this cold-window behavior is correct, confirm it in a code comment clarifying the exception vs. other endpoints likerouter::routewhich validate required fields.🤖 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 `@llm-router/tests/golden/schemas/router.models.get.json` around lines 8 - 20, The ModelGetRequest schema defines id and provider fields with empty-string defaults and no validation, allowing requests with omitted or empty parameters to return null instead of raising an error. Clarify the intent by either adding a code comment in the schema or handler explaining that this empty-string default behavior and resulting null responses are intentional as a "cold-window signal" per the specification, distinguishing this from other endpoints like router::route which enforce required field validation. If this permissive behavior is not intentional, remove the default values and mark id and provider as required fields in the schema.
🤖 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 @.github/scripts/collect_worker_interface.py:
- Around line 148-156: The code currently uses `data.get("functions") or []`
which allows malformed values like dictionaries to bypass validation. Add an
explicit check after this line to validate that `functions` is actually a list
before running the assertions. If `functions` is not a list, log an error and
return 1 to reject the invalid interface payload. This validation must occur
before both the `--assert-non-empty` check and the `--assert-typed-schemas`
check to ensure invalid payloads cannot bypass either enforcement path.
In `@context-manager/tests/support/mod.rs`:
- Around line 101-111: The DEFINING array in the assert_typed_schema context is
missing the "const" keyword, which creates inconsistency with the publish
checker script that treats "const" as a schema-defining keyword. Add "const" to
the DEFINING array to maintain parity and ensure consistent behavior between
local tests and publish-time validation.
In `@llm-router/tests/golden/schemas/router.chat.json`:
- Around line 22-24: The writer_ref.direction field in the router.chat.json
schema currently uses a generic $ref to ChannelDirection definition that permits
both "read" and "write" values. Since writer_ref is documented as a write
channel, constrain the direction property to only allow the literal string value
"write" instead of referencing the full ChannelDirection definition. This fix
needs to be applied at two locations in the schema: the first occurrence at
lines 22-24 where direction is defined for writer_ref, and the second occurrence
at lines 85-92 where the same constraint should be applied.
In `@llm-router/tests/golden/schemas/router.models.reconcile.json`:
- Line 168: The description field in the schema contains a typo with "iii" that
should be removed. In the description string "Output of the
`router::models::reconcile` iii function.", delete the "iii " text (including
the space after it) so the description reads "Output of the
`router::models::reconcile` function."
In `@llm-router/tests/golden/schemas/router.provider.register.json`:
- Line 162: The config_schema field at line 162 is currently set to true, which
is a permissive JSON schema that accepts any value and contradicts the PR goal
of preventing permissive schemas. Replace this boolean true value with either a
concrete type definition, a well-defined union of allowed config shapes, or add
documentation explaining why this exception to the schema enforcement policy is
necessary for provider-specific configurations.
In `@llm-router/tests/golden/schemas/router.provider.resolve.json`:
- Line 59: The `provider_extra` field in the OAuth credential definition is set
to true, which accepts any arbitrary JSON without type validation, undermining
the typed schema. Either replace this permissive definition with a concrete
schema that defines the specific structure and types allowed for
provider-specific OAuth extensions, or add clear documentation explaining why
this field must remain permissive and what constraints apply to its usage.
In `@llm-router/tests/golden/schemas/router.provider.update_credential.json`:
- Around line 8-11: The credential field in the schema currently lacks a type
constraint or schema reference, accepting any JSON value without validation. Add
a type property or schema reference to the credential field definition to
enforce type safety. Consider referencing the Credential definition from
router.provider.resolve.json if available, or define an explicit schema that
matches the provider-specific shape mentioned in the description, ensuring the
field enforces the same typed schema constraints as the rest of the
configuration.
In `@provider-openai/tests/golden/schemas/provider.openai.stream.json`:
- Line 8: Remove the typo "iii" from the description string in the AgentFunction
description field. The text currently reads "These describe iii functions
exposed to the model" and should be corrected to "These describe functions
exposed to the model" by deleting "iii " (including the space after it).
- Line 646: In the provider.openai.stream.json file at the description field
around line 646, there is a typo where the word "iii" appears between "stream`"
and "function" in the text "Input of a provider worker's
`provider::<id>::stream` iii function". Remove this erroneous "iii" text so the
description reads "Input of a provider worker's `provider::<id>::stream`
function — what the router forwards per attempt. (No `PartialEq`:
`iii_sdk::StreamChannelRef` doesn't implement it.)"
In `@session-manager/tests/golden/schemas/session.store.publish-events.json`:
- Line 52: The `published` field currently uses `"format": "uint"` but should be
changed to `"format": "uint64"` to maintain consistency with other similar
non-negative integer fields like `message_count` that already use the `uint64`
format. Both fields have the same `minimum: 0.0` constraint, so update the
format value for the `published` field to match the standard format used
elsewhere in the schema catalog.
In `@session-manager/tests/schemas.rs`:
- Around line 119-122: The error handling in the `read_dir` block silently
returns when the schemas directory cannot be opened, causing the test to pass
even if the directory is missing. Since the catalog contains 29 functions and
the test expects corresponding golden files, a missing directory indicates a
configuration problem that should fail the test. Replace the silent return with
an explicit failure (e.g., using panic or assert) when the directory cannot be
accessed, or at minimum verify that the catalog is also empty before allowing
the test to pass silently. This ensures that real issues like misconfiguration
or accidental deletion are caught rather than hidden.
---
Outside diff comments:
In `@llm-router/src/config/on_changed.rs`:
- Around line 70-77: The flush_task handle is being aborted on every new config
event, but this can cancel a task that has already drained the pending queue and
started executing provider refresh calls. Separate the tracking of the debounce
sleep phase from the active flush execution: either maintain two separate
handles (one for the debounce task and one for the active flush), or clear the
task handle stored in flush_task before draining pending so that subsequent
abort() calls only cancel tasks that are still sleeping and haven't started
processing yet. Ensure that once the task begins draining pending items in the
tokio::spawn block, it cannot be aborted by new config events.
In `@llm-router/src/registry/resolve.rs`:
- Around line 136-146: The code panics when the nested providers entry or the
individual provider slice (accessed via req.id) contain non-object values. After
retrieving the providers entry from line 141 and before calling as_object_mut()
on it, normalize it to an empty object if it is not already an object.
Similarly, after retrieving the slice entry accessed via req.id from line
145-146, normalize it to an empty object if needed before calling
as_object_mut() and accessing its credential field. Apply the same normalization
pattern used at the top level (checking and replacing non-object values) to
prevent panics when these nested values exist but are not objects.
- Around line 123-130: The `credential` field in the `UpdateCredentialRequest`
struct lacks a type constraint in its schema definition, allowing any value or
null at the schema level even though runtime validation enforces `is_object()`.
Add a `"type": "object"` constraint to the credential field's schema definition
(alongside the existing description) to match the pattern used by other fields
like `id` and `token` in the same struct, ensuring data integrity is enforced at
validation time rather than only at runtime.
---
Nitpick comments:
In @.github/scripts/tests/test_collect_assert_non_empty.py:
- Around line 58-104: Add a new regression test method to the
TestAssertTypedSchemas class to guard against malformed non-list functions
payloads. Create a test (similar in structure to
test_fails_on_empty_response_schema) that uses write_payload with functions as a
non-list value (such as a dictionary or string instead of an array), calls
_run() with the output file, and asserts that the script returns a non-zero exit
code to ensure the assertion properly detects and rejects this malformed input.
In `@llm-router/src/types/router.rs`:
- Around line 349-360: The credential field in the UpdateCredentialRequest
struct is currently typed as Value (arbitrary JSON), but it should be strongly
typed as Credential to maintain consistency with the typed Credential response
and ensure the wire contract is fully typed end-to-end. Change the type of the
credential field from Value to Credential in the UpdateCredentialRequest struct
definition.
In `@llm-router/tests/golden/schemas/router.models.get.json`:
- Around line 8-20: The ModelGetRequest schema defines id and provider fields
with empty-string defaults and no validation, allowing requests with omitted or
empty parameters to return null instead of raising an error. Clarify the intent
by either adding a code comment in the schema or handler explaining that this
empty-string default behavior and resulting null responses are intentional as a
"cold-window signal" per the specification, distinguishing this from other
endpoints like router::route which enforce required field validation. If this
permissive behavior is not intentional, remove the default values and mark id
and provider as required fields in the schema.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c38ee50d-8fd4-4be8-8c9a-5761140574f8
⛔ Files ignored due to path filters (6)
approval-gate/Cargo.lockis excluded by!**/*.lockcontext-manager/Cargo.lockis excluded by!**/*.lockllm-router/Cargo.lockis excluded by!**/*.lockprovider-anthropic/Cargo.lockis excluded by!**/*.lockprovider-openai/Cargo.lockis excluded by!**/*.locksession-manager/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (116)
.github/scripts/collect_worker_interface.py.github/scripts/tests/test_collect_assert_non_empty.py.github/workflows/_publish-registry.ymlapproval-gate/Cargo.tomlapproval-gate/src/functions/mod.rsapproval-gate/src/functions/on_config_change.rsapproval-gate/src/functions/on_session_deleted.rsapproval-gate/src/functions/on_turn_completed.rsapproval-gate/src/functions/sweep.rsapproval-gate/src/types.rsapproval-gate/tests/golden/schemas/approval.on-config-change.jsonapproval-gate/tests/golden/schemas/approval.on-session-deleted.jsonapproval-gate/tests/golden/schemas/approval.on-turn-completed.jsonapproval-gate/tests/golden/schemas/approval.sweep.jsonapproval-gate/tests/schemas.rsapproval-gate/tests/support/mod.rscontext-manager/Cargo.tomlcontext-manager/tests/schemas.rscontext-manager/tests/support/mod.rsdocs/sops/binary-worker.mddocs/sops/new-worker.mdllm-router/Cargo.tomlllm-router/src/catalog/handlers.rsllm-router/src/catalog/reconcile.rsllm-router/src/chat/abort.rsllm-router/src/chat/chat.rsllm-router/src/chat/complete.rsllm-router/src/config/on_changed.rsllm-router/src/lib.rsllm-router/src/register.rsllm-router/src/registry/availability.rsllm-router/src/registry/register.rsllm-router/src/registry/resolve.rsllm-router/src/routing.rsllm-router/src/surface.rsllm-router/src/types/content.rsllm-router/src/types/credential.rsllm-router/src/types/errors.rsllm-router/src/types/events.rsllm-router/src/types/messages.rsllm-router/src/types/model.rsllm-router/src/types/router.rsllm-router/tests/golden/schemas/router.abort.jsonllm-router/tests/golden/schemas/router.chat.jsonllm-router/tests/golden/schemas/router.complete.jsonllm-router/tests/golden/schemas/router.models.get.jsonllm-router/tests/golden/schemas/router.models.list.jsonllm-router/tests/golden/schemas/router.models.reconcile.jsonllm-router/tests/golden/schemas/router.models.supports.jsonllm-router/tests/golden/schemas/router.on_config_changed.jsonllm-router/tests/golden/schemas/router.on_worker_available.jsonllm-router/tests/golden/schemas/router.provider.list.jsonllm-router/tests/golden/schemas/router.provider.register.jsonllm-router/tests/golden/schemas/router.provider.resolve.jsonllm-router/tests/golden/schemas/router.provider.update_credential.jsonllm-router/tests/golden/schemas/router.route.jsonllm-router/tests/schemas.rsllm-router/tests/support/mod.rsprovider-anthropic/Cargo.tomlprovider-anthropic/src/discovery.rsprovider-anthropic/src/errors.rsprovider-anthropic/src/lib.rsprovider-anthropic/src/register.rsprovider-anthropic/src/stream_fn.rsprovider-anthropic/src/surface.rsprovider-anthropic/tests/golden/schemas/provider.anthropic.on_router_ready.jsonprovider-anthropic/tests/golden/schemas/provider.anthropic.refresh_models.jsonprovider-anthropic/tests/golden/schemas/provider.anthropic.stream.jsonprovider-anthropic/tests/schemas.rsprovider-anthropic/tests/support/mod.rsprovider-openai/Cargo.tomlprovider-openai/src/discovery.rsprovider-openai/src/errors.rsprovider-openai/src/lib.rsprovider-openai/src/register.rsprovider-openai/src/stream_fn.rsprovider-openai/src/surface.rsprovider-openai/tests/golden/schemas/provider.openai.on_router_ready.jsonprovider-openai/tests/golden/schemas/provider.openai.refresh_models.jsonprovider-openai/tests/golden/schemas/provider.openai.stream.jsonprovider-openai/tests/schemas.rsprovider-openai/tests/support/mod.rssession-manager/Cargo.tomlsession-manager/src/configuration.rssession-manager/src/lib.rssession-manager/src/surface.rssession-manager/tests/golden/schemas/session.append-many.jsonsession-manager/tests/golden/schemas/session.append.jsonsession-manager/tests/golden/schemas/session.config-status.jsonsession-manager/tests/golden/schemas/session.create.jsonsession-manager/tests/golden/schemas/session.delete.jsonsession-manager/tests/golden/schemas/session.ensure.jsonsession-manager/tests/golden/schemas/session.fork.jsonsession-manager/tests/golden/schemas/session.get-message.jsonsession-manager/tests/golden/schemas/session.get.jsonsession-manager/tests/golden/schemas/session.list.jsonsession-manager/tests/golden/schemas/session.messages.jsonsession-manager/tests/golden/schemas/session.on-config-change.jsonsession-manager/tests/golden/schemas/session.set-active-leaf.jsonsession-manager/tests/golden/schemas/session.set-meta.jsonsession-manager/tests/golden/schemas/session.set-status.jsonsession-manager/tests/golden/schemas/session.store.delete-active-leaf.jsonsession-manager/tests/golden/schemas/session.store.delete-entries.jsonsession-manager/tests/golden/schemas/session.store.delete-meta.jsonsession-manager/tests/golden/schemas/session.store.get-active-leaf.jsonsession-manager/tests/golden/schemas/session.store.get-entry.jsonsession-manager/tests/golden/schemas/session.store.get-meta.jsonsession-manager/tests/golden/schemas/session.store.list-entries.jsonsession-manager/tests/golden/schemas/session.store.list-metas.jsonsession-manager/tests/golden/schemas/session.store.publish-events.jsonsession-manager/tests/golden/schemas/session.store.put-entry.jsonsession-manager/tests/golden/schemas/session.store.put-meta.jsonsession-manager/tests/golden/schemas/session.store.set-active-leaf.jsonsession-manager/tests/golden/schemas/session.update-message.jsonsession-manager/tests/schemas.rssession-manager/tests/support/mod.rs
Comment on lines
+148
to
+156
| functions = data.get("functions") or [] | ||
| if args.assert_non_empty and not functions: | ||
| print( | ||
| f"::error::no worker functions in {args.assert_file} (empty)", | ||
| file=sys.stderr, | ||
| ) | ||
| return 1 | ||
| if args.assert_typed_schemas: | ||
| violations = _typed_schema_violations(functions) |
There was a problem hiding this comment.
Validate functions is a JSON array before running assertions.
Both assertion paths currently coerce with or []. If functions is malformed but truthy (for example {}), --assert-non-empty can pass and --assert-typed-schemas can also pass because non-dict entries are skipped in _typed_schema_violations. This allows invalid interface payloads to bypass enforcement.
Suggested fix
- functions = data.get("functions") or []
+ functions = data.get("functions")
+ if not isinstance(functions, list):
+ print(
+ f"::error::{args.assert_file} has invalid payload['functions']; expected JSON array",
+ file=sys.stderr,
+ )
+ return 1
if args.assert_non_empty and not functions:
print(
f"::error::no worker functions in {args.assert_file} (empty)",
file=sys.stderr,
)
return 1
...
- functions = data.get("functions") or []
+ functions = data.get("functions")
+ if not isinstance(functions, list):
+ print(
+ f"::error::{args.out} has invalid payload['functions']; expected JSON array",
+ file=sys.stderr,
+ )
+ return 1
if args.assert_non_empty and not functions:
print(f"::error::no worker functions in {args.out} (empty)", file=sys.stderr)
return 1Also applies to: 205-210
🤖 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 @.github/scripts/collect_worker_interface.py around lines 148 - 156, The code
currently uses `data.get("functions") or []` which allows malformed values like
dictionaries to bypass validation. Add an explicit check after this line to
validate that `functions` is actually a list before running the assertions. If
`functions` is not a list, log an error and return 1 to reject the invalid
interface payload. This validation must occur before both the
`--assert-non-empty` check and the `--assert-typed-schemas` check to ensure
invalid payloads cannot bypass either enforcement path.
Comment on lines
+101
to
+111
| const DEFINING: [&str; 8] = [ | ||
| "type", | ||
| "properties", | ||
| "$ref", | ||
| "allOf", | ||
| "anyOf", | ||
| "oneOf", | ||
| "enum", | ||
| "items", | ||
| ]; | ||
| let has_defining = DEFINING.iter().any(|k| obj.contains_key(*k)); |
There was a problem hiding this comment.
Keep typed-schema keyword parity with the publish checker.
assert_typed_schema is missing "const" in DEFINING, while .github/scripts/collect_worker_interface.py treats it as schema-defining. This can create inconsistent pass/fail behavior across local tests vs publish-time assertion.
Suggested fix
- const DEFINING: [&str; 8] = [
+ const DEFINING: [&str; 9] = [
"type",
"properties",
"$ref",
"allOf",
"anyOf",
"oneOf",
"enum",
"items",
+ "const",
];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const DEFINING: [&str; 8] = [ | |
| "type", | |
| "properties", | |
| "$ref", | |
| "allOf", | |
| "anyOf", | |
| "oneOf", | |
| "enum", | |
| "items", | |
| ]; | |
| let has_defining = DEFINING.iter().any(|k| obj.contains_key(*k)); | |
| const DEFINING: [&str; 9] = [ | |
| "type", | |
| "properties", | |
| "$ref", | |
| "allOf", | |
| "anyOf", | |
| "oneOf", | |
| "enum", | |
| "items", | |
| "const", | |
| ]; | |
| let has_defining = DEFINING.iter().any(|k| obj.contains_key(*k)); |
🤖 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 `@context-manager/tests/support/mod.rs` around lines 101 - 111, The DEFINING
array in the assert_typed_schema context is missing the "const" keyword, which
creates inconsistency with the publish checker script that treats "const" as a
schema-defining keyword. Add "const" to the DEFINING array to maintain parity
and ensure consistent behavior between local tests and publish-time validation.
Comment on lines
+22
to
+24
| "direction": { | ||
| "$ref": "#/definitions/ChannelDirection" | ||
| } |
There was a problem hiding this comment.
Constrain writer_ref.direction to "write" in the request schema.
writer_ref is documented as a write channel, but the schema currently permits "read" too via StreamChannelRef. That allows invalid payloads to pass validation and fail later at runtime.
Suggested schema constraint
"writer_ref": {
"allOf": [
{
"$ref": "`#/definitions/StreamChannelRef`"
+ },
+ {
+ "type": "object",
+ "properties": {
+ "direction": { "const": "write" }
+ },
+ "required": ["direction"]
}
],
"description": "The caller's write channel (direction \"write\"); frames are relayed here."
}Also applies to: 85-92
🤖 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 `@llm-router/tests/golden/schemas/router.chat.json` around lines 22 - 24, The
writer_ref.direction field in the router.chat.json schema currently uses a
generic $ref to ChannelDirection definition that permits both "read" and "write"
values. Since writer_ref is documented as a write channel, constrain the
direction property to only allow the literal string value "write" instead of
referencing the full ChannelDirection definition. This fix needs to be applied
at two locations in the schema: the first occurrence at lines 22-24 where
direction is defined for writer_ref, and the second occurrence at lines 85-92
where the same constraint should be applied.
| }, | ||
| "response_schema": { | ||
| "$schema": "http://json-schema.org/draft-07/schema#", | ||
| "description": "Output of the `router::models::reconcile` iii function.", |
There was a problem hiding this comment.
Typo in response schema description.
The description contains "iii" which appears to be a typo or placeholder: "Output of the router::models::reconcile iii function."
Consider changing to "Output of the router::models::reconcile function."
🤖 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 `@llm-router/tests/golden/schemas/router.models.reconcile.json` at line 168,
The description field in the schema contains a typo with "iii" that should be
removed. In the description string "Output of the `router::models::reconcile`
iii function.", delete the "iii " text (including the space after it) so the
description reads "Output of the `router::models::reconcile` function."
| }, | ||
| "description": "Input of `router::provider::register` — a provider worker's declaration plus the optional re-registration token.", | ||
| "properties": { | ||
| "config_schema": true, |
There was a problem hiding this comment.
Permissive schema field contradicts typed schema enforcement.
Line 162 uses "config_schema": true, which is JSON Schema's boolean form that accepts any JSON value. This is effectively an untyped field, contradicting the PR's stated goal of "preventing the use of permissive AnyValue schemas."
If provider-specific config schemas genuinely require flexibility, consider documenting why this exception is necessary. Otherwise, define a concrete type or use a well-defined union of allowed config shapes.
🤖 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 `@llm-router/tests/golden/schemas/router.provider.register.json` at line 162,
The config_schema field at line 162 is currently set to true, which is a
permissive JSON schema that accepts any value and contradicts the PR goal of
preventing permissive schemas. Replace this boolean true value with either a
concrete type definition, a well-defined union of allowed config shapes, or add
documentation explaining why this exception to the schema enforcement policy is
necessary for provider-specific configurations.
Comment on lines
+8
to
+11
| "credential": { | ||
| "default": null, | ||
| "description": "The credential object to store (provider-specific shape)." | ||
| }, |
There was a problem hiding this comment.
Credential field lacks type constraint.
Lines 8–11 define the credential field with only a default and description, but no type or schema reference. This accepts any JSON value, creating another untyped surface that contradicts the typed schema enforcement objective. Reference the Credential definition from router.provider.resolve.json or define an explicit schema.
🤖 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 `@llm-router/tests/golden/schemas/router.provider.update_credential.json`
around lines 8 - 11, The credential field in the schema currently lacks a type
constraint or schema reference, accepting any JSON value without validation. Add
a type property or schema reference to the credential field definition to
enforce type safety. Consider referencing the Credential definition from
router.provider.resolve.json if available, or define an explicit schema that
matches the provider-specific shape mentioned in the description, ensuring the
field enforces the same typed schema constraints as the rest of the
configuration.
| "$schema": "http://json-schema.org/draft-07/schema#", | ||
| "definitions": { | ||
| "AgentFunction": { | ||
| "description": "Function invocation schema — what a provider sees as a `tools` array entry (README § Function invocation schema; adapter boundary). These describe iii functions exposed to the model, not provider-native tools.", |
There was a problem hiding this comment.
Typo in AgentFunction description.
The description contains "iii" which appears to be a typo or placeholder: "These describe iii functions exposed to the model".
Consider changing to "These describe functions exposed to the model".
🤖 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 `@provider-openai/tests/golden/schemas/provider.openai.stream.json` at line 8,
Remove the typo "iii" from the description string in the AgentFunction
description field. The text currently reads "These describe iii functions
exposed to the model" and should be corrected to "These describe functions
exposed to the model" by deleting "iii " (including the space after it).
| "type": "string" | ||
| } | ||
| }, | ||
| "description": "Input of a provider worker's `provider::<id>::stream` iii function — what the router forwards per attempt. (No `PartialEq`: `iii_sdk::StreamChannelRef` doesn't implement it.)", |
There was a problem hiding this comment.
Typo in request schema description.
The description contains "iii" which appears to be a typo or placeholder: "Input of a provider worker's provider::<id>::stream iii function".
Consider changing to "Input of a provider worker's provider::<id>::stream function".
🤖 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 `@provider-openai/tests/golden/schemas/provider.openai.stream.json` at line
646, In the provider.openai.stream.json file at the description field around
line 646, there is a typo where the word "iii" appears between "stream`" and
"function" in the text "Input of a provider worker's `provider::<id>::stream`
iii function". Remove this erroneous "iii" text so the description reads "Input
of a provider worker's `provider::<id>::stream` function — what the router
forwards per attempt. (No `PartialEq`: `iii_sdk::StreamChannelRef` doesn't
implement it.)"
| "properties": { | ||
| "published": { | ||
| "description": "Number of well-formed envelopes accepted and fanned out.", | ||
| "format": "uint", |
There was a problem hiding this comment.
Format inconsistency: use uint64 instead of uint for consistency.
The published field uses "format": "uint", while similar non-negative integer fields in other golden schemas use "format": "uint64" (e.g., message_count in session.store.list-metas.json line 31). Both represent non-negative integers with the same minimum: 0.0 constraint, so the format should be uniform across the schema catalog.
🔧 Proposed fix
- "format": "uint",
+ "format": "uint64",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "format": "uint", | |
| "format": "uint64", |
🤖 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 `@session-manager/tests/golden/schemas/session.store.publish-events.json` at
line 52, The `published` field currently uses `"format": "uint"` but should be
changed to `"format": "uint64"` to maintain consistency with other similar
non-negative integer fields like `message_count` that already use the `uint64`
format. Both fields have the same `minimum: 0.0` constraint, so update the
format value for the `published` field to match the standard format used
elsewhere in the schema catalog.
Comment on lines
+119
to
+122
| let entries = match std::fs::read_dir(&dir) { | ||
| Ok(e) => e, | ||
| Err(_) => return, | ||
| }; |
There was a problem hiding this comment.
Silent pass when schemas directory is missing may hide real issues.
The test returns early if read_dir fails, allowing the test to pass even if the entire tests/golden/schemas/ directory is missing. Since the catalog lists 29 functions and the wire_schema_snapshots_match_goldens test expects corresponding golden files, a missing directory likely indicates a real problem (misconfiguration, wrong path, accidental deletion) rather than a valid empty state.
Consider failing explicitly if the directory doesn't exist, or at minimum checking that the catalog is also empty:
🛡️ Proposed fix to fail when directory is missing
let entries = match std::fs::read_dir(&dir) {
Ok(e) => e,
- Err(_) => return,
+ Err(e) => panic!(
+ "schemas golden directory {:?} is unreadable: {e}\n\
+ Expected {} golden files for the catalog.",
+ dir, expected.len()
+ ),
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let entries = match std::fs::read_dir(&dir) { | |
| Ok(e) => e, | |
| Err(_) => return, | |
| }; | |
| let entries = match std::fs::read_dir(&dir) { | |
| Ok(e) => e, | |
| Err(e) => panic!( | |
| "schemas golden directory {:?} is unreadable: {e}\n\ | |
| Expected {} golden files for the catalog.", | |
| dir, expected.len() | |
| ), | |
| }; |
🤖 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 `@session-manager/tests/schemas.rs` around lines 119 - 122, The error handling
in the `read_dir` block silently returns when the schemas directory cannot be
opened, causing the test to pass even if the directory is missing. Since the
catalog contains 29 functions and the test expects corresponding golden files, a
missing directory indicates a configuration problem that should fail the test.
Replace the silent return with an explicit failure (e.g., using panic or assert)
when the directory cannot be accessed, or at minimum verify that the catalog is
also empty before allowing the test to pass silently. This ensures that real
issues like misconfiguration or accidental deletion are caught rather than
hidden.
ytallo
added a commit
that referenced
this pull request
Jun 18, 2026
…andlers The publish step's `collect_worker_interface.py --assert-typed-schemas` (added in #274) rejects functions whose request/response serialize as untyped AnyValue. `directory::on-config-change` and the internal `directory::__on_worker_added` were registered with `serde_json::Value`, producing empty schemas and failing publish. Give each a typed request/response struct deriving `schemars::JsonSchema` (matching the worker's existing `ListPromptsInput {}` convention), so the collected interface carries real schemas. No behavioural change — both handlers still ignore/forward the same fields. Claude-Session: https://claude.ai/code/session_013LM3EaciHB9zGF8zRvjzxD
ytallo
added a commit
that referenced
this pull request
Jun 18, 2026
…0 set (#288) * chore(workers): declare iii.worker.yaml dependency graph for the 1.0.0 set Add evidence-based `dependencies:` blocks to the workers in the harness dependency tree and align the harness's own ranges to the coordinated 1.0.0 release. Inter-worker edges use ^1.0.0 (resolve once the whole set ships at 1.0.0); system workers use ^0.19.0. - harness: iii-directory/session-manager/context-manager/llm-router/approval-gate -> ^1.0.0 - iii-directory, session-manager: configuration - context-manager: configuration, llm-router - llm-router: iii-state, configuration - provider-anthropic, provider-openai: iii-state, llm-router - approval-gate: iii-state, configuration, iii-directory, session-manager Cycle-forming edges omitted by design: approval-gate->harness (soft callback) and llm-router->providers (providers self-register). Claude-Session: https://claude.ai/code/session_013LM3EaciHB9zGF8zRvjzxD * chore(llm-router): remove unused configuration files Deleted obsolete `config.yaml` and `iii-permissions.yaml` files from the llm-router, as they are no longer needed for the current architecture and configuration management. This cleanup helps streamline the codebase and reduce confusion regarding configuration sources. * fix(iii-directory): type request/response schemas for the two event handlers The publish step's `collect_worker_interface.py --assert-typed-schemas` (added in #274) rejects functions whose request/response serialize as untyped AnyValue. `directory::on-config-change` and the internal `directory::__on_worker_added` were registered with `serde_json::Value`, producing empty schemas and failing publish. Give each a typed request/response struct deriving `schemars::JsonSchema` (matching the worker's existing `ListPromptsInput {}` convention), so the collected interface carries real schemas. No behavioural change — both handlers still ignore/forward the same fields. Claude-Session: https://claude.ai/code/session_013LM3EaciHB9zGF8zRvjzxD
5 tasks
This was referenced Jul 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…st/response validation
--assert-typed-schemasflag tocollect_worker_interface.pyto ensure all functions have typed request and response schemas, preventing the use of permissiveAnyValueschemas._typed_schema_violationsfunction to identify and report untyped schemas during assertions.--assert-fileoption.Summary by CodeRabbit
New Features
--assert-typed-schemasflag to prevent untyped request/response schemas in worker functions.Improvements
Documentation
Tests