Repository navigation
Review follow-ups for 0.10.1: a late schema defect is -32603, the missing tests, the dead clause, the pattern cost - #79
Conversation
…sing tests, the dead clause, the pattern cost From Sean MacGuire's review of 0.10.1. A tools/call against a schema the catalog made unenforceable after startup is answered -32603 naming the tool and the keyword (it was a tool result saying "invalid arguments", blaming the client); the new test fails on 0.10.1's server. Tests for additionalProperties as a schema (it kills the review's surviving mutant, the branch replaced by :ok), a list-form type refusing a value of none of its types, and every README keyword accepted well-formed; the `when key in @enforced` clause, which no keyword could reach, and the list it read are removed. The threat model's arguments row says what `pattern` costs (PCRE's match limit, fail closed; 80 ms measured for the worst case a lane tried), inside the row so no threat or status moves. CHANGELOG: the change, and 0.10.1's `nil` reason reading `null`, recorded late. The stdio test helper says why its device is latin1. Gate: sixteen steps pass. Signed-off-by: Ayla Croft <aylacroft@proton.me>
minitru
left a comment
minitru
left a comment
There was a problem hiding this comment.
The schema is now checked twice per call. validate_and_dispatch calls Schema.check_schema, then Schema.validate, which calls check_schema again and then compile_patterns. Every pattern is compiled three times per tools/call. It's the cost the PR defers ("compile once at startup"), but this change makes it worse first. A cheap fix is a validate variant that skips the re-check.
The -32603 text can carry an inspected Elixir term. The check_schema reason uses inspect(key) for a non-string keyword and inspect(name) for a non-string property name, and that reason now goes into the error message. The threat model's leak row says a -32603 never carries an inspected term. The comment's argument (already advertised in tools/list) holds for string keys but not for these two branches. Only a host bug triggers it, but the claim and the code disagree.
The keyword list still lives in two places. Removing @enforced was right, since the clause was unreachable. But the new "every README keyword" test hand-copies the nineteen names, so the README and the test can drift. Parsing the list out of the README would make the test match its name.
Prompts keep the old behaviour. prompt_arguments still turns a schema defect into "invalid arguments" (-32602). That schema is derived by the package, so it's probably unreachable; a one-line comment saying so would close the question.
The -32603 test is good: it uses a catalog that changes after startup, asserts both the tool name and the keyword, and cleans up the process dictionary.
There was a problem hiding this comment.
Posted here by mistake. This was the review of PR 80 and has moved there: #80 (review). It does not request changes to this PR.
There was a problem hiding this comment.
Posted here by mistake. This was the review of beam_mcp_signer PR 13 and has moved there: ScriptKittyOS/beam_mcp_signer#13 (review)
2 participants
Sean's non-blocking points from the 0.10.1 review, one commit:
tools/callagainst a schema the catalog made unenforceable after startup is now-32603naming the tool and keyword, not an "invalid arguments" tool result (the test fails on 0.10.1's server).additionalPropertiesas a schema (kills the surviving mutant), list-formtyperefusal, every README keyword accepted well-formed.when key in @enforcedclause and its list removed.patterncost (inside the row: no threat or status moves, so the 1.0.0 clock is unaffected).nilreason now readingnull.Left for later, as the review said: compiling the schema once at startup (it touches
ToolSpec, a public struct). Local gate: sixteen steps pass.