Repository navigation
Conversation
A model asked for an array param sometimes writes the array as a JSON string —
`"[{\"id\":1}]"` instead of `[{"id":1}]`. The data is correct; only the container
is wrong. Validation rejected it outright before the tool body ran, so the whole
turn was lost to a shape mistake.
Riffer already holds the principle this violates. coerce_value accepts a whole
number for a Float because "JSON Schema 'number' accepts integers; callers
shouldn't get a type that depends on whether the model wrote a decimal point".
The same is true of an array written as a string, and it matters more: an
unrecognised Integer is harmless, an unrecognised String drops the call.
Riffer::Params::Recovery decodes that one shape ahead of the type check, mirroring
Agent::StructuredOutput::Parser, which already recovers output wrapped in code
fences or prose. Only a value that decodes to an array is recovered; the decoded
value is then validated normally, so an item with a missing or wrong-typed field
fails as before. A payload that decodes to an object, prose, or a truncated
fragment is returned untouched and still rejected — the intent is ambiguous there,
and a tool acting on a guess is worse than a tool reporting the error. Parse
failures are rescued rather than raised, both because validation must stay a
validation error and because the parser embeds its input in the message.
BREAKING CHANGE: a wrong-type validation error now names the type received as well
as the one expected — "parts: expected array, got string" rather than "parts must
be a array". Callers asserting on the old wording need updating. The old message
named only the expectation, so a model given it had nothing to correct and would
sometimes repeat the same mistake; it also read ungrammatically for array and
object, which is how the gap surfaced.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
⛔ Files ignored due to path filters (2)
📒 Files selected for processing (7)
Included review availability: This review used your included allowance. 3 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour. 📜 Recent review details⏰ Context from checks skipped due to timeout. (9)
🔇 Additional comments (7)
📝 WalkthroughWalkthroughArray parameters now accept JSON-encoded strings when decoding produces an array. The decoded value goes through normal type validation. Type errors report both expected and received types. ChangesArray parameter recovery
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~12 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant NotesTool
participant Params
participant Recovery
NotesTool->>Params: Validate parts parameter
Params->>Recovery: Recover JSON string
Recovery-->>Params: Return decoded array or original value
Params->>Params: Validate array and nested items
Params-->>NotesTool: Return validated parts or validation error
Merge Risk: ⚪ Minimal · up to Array parameters now accept JSON-encoded array strings, and type errors report both the expected and received types. The change is limited in scope and tested. Callers that match the old "must be a" wording will need updating, as the PR states. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 5 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
drunkel
marked this pull request as draft
October 7, 2026 18:18
bottrall
requested review from
bottrall
and removed request for
bottrall and
sterlingwes
October 7, 2026 18:19
This branch has not been deployed
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
1 participant
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.
The problem
A model asked for an array param sometimes writes the array as a JSON string:
The data is correct — only the container is wrong.
Params#validaterejects it withparts must be a arraybefore the tool body runs, so the whole turn is lost to a shapemistake.
Why this belongs in riffer
Riffer already holds the principle this violates.
coerce_valueaccepts a whole number for aFloat:An array written as a string is the same thing, and it matters more: an unrecognised
Integeris harmless, an unrecognisedStringdrops the call. There's also precedent forlenient recovery of model output in the wrong wrapper —
Agent::StructuredOutput::Parser(#473) already recovers objects from code fences and prose.
Params::Recoverymirrors it.The change
Riffer::Params::Recovery.callruns immediately before the type check:Conservative by design. Only a value that decodes to an array is recovered, and the
decoded value is then validated normally — an item with a missing or wrong-typed field fails
exactly as before. A payload that decodes to an object, prose, or a truncated fragment is
returned untouched and still rejected: the intent there is ambiguous, and a tool acting on a
guess is worse than a tool reporting the error.
Parse failures are rescued rather than raised — partly so validation stays a validation
error, partly because
JSON::ParserErrorembeds its input in the message and callers may bepassing sensitive content through params.
Proof
Both tests were written first and run red against
main:Green after the change. Coverage added in
test/riffer/params/recovery_test.rb(the recoveryrules, including every shape deliberately not recovered),
params_test.rb(validate-level),and
tools/runtime_test.rb(a real tool call, dispatched from the raw argument string).Breaking change
A wrong-type error now names the received type too:
parts must be a arrayparts: expected array, got stringprice must be a numberprice: expected number, got booleanCallers asserting on the old wording need updating; 4 tests here did. This also fixes the
ungrammatical
a array/a object, which is how the gap surfaced.Checks
rake test— 2632 runs, 4083 assertions, 0 failuresrubocop— 269 files, no offensesbin/typecheck— no type errors (needed a manual sig forextend self, asParserdoes)rake rbs:generate— sigs regenerateddocs/TOOLS.mdupdated alongside the existingFloatparagraph🤖 Generated with Claude Code
Summary by CodeRabbit