DEV Community

My Harness Used One Label for Three Different Failures.

Three fixtures, three separate calls into the same reducer. Here is the complete failure_reasons each one returned, unedited: unreadable arriving args -> ["SANDBOX_EVENT_CARDINALITY_INVALID", "EXEC_ARGUMENTS_MISMATCH", "TOOL_RESPONSE_CARDINALITY_INVALID"] usable args, different -> ["SANDBOX_EVENT_CARDINALITY_INVALID", "EXEC_ARGUMENTS_MISMATCH", "TOOL_RESPONSE_CARDINALITY_INVALID"] our comparison threw -> ["SANDBOX_EVENT_CARDINALITY_INVALID", "EXEC_ARGUMENTS_MISMATCH", "TOOL_RESPONSE_CARDINALITY_INVALID"] These are minimal fixtures with no sandbox event and no tool response, so the first and last codes fire in all three and are expected. I am printing them anyway. A post about a receipt that hides which party failed has no business showing you a cleaned-up receipt. The middle line is the one that matters, and across three genuinely different failures it never changes. Fixture one sends arguments the parser rejects. Fixture two sends a usable call that disagrees with what I froze. Fixture three sends an object my own canonicalizer rejects, so the comparison never completes. Constructed inputs, so none of this establishes who caused a failure in production. But one name covers all three, and that name says argument mismatch even when nothing was compared. A failure in the checking stage reads as a deviation in the thing being checked. The reason it reads that way pm25coder put it in one line, in the comments of the schema-comparison piece (permalink to the comment): EXEC_ARGUMENTS_MISMATCH misreads as a verdict on the model precisely because its name carries no subject. Three different observations arrive under one name: arguments that were rejected, a comparison that completed and found a difference, and a comparison that never finished. The label is not wrong that something happened. It is silent about which of the three, and a reader fills that in. What the code actually did One try was wrapping three different jobs: if (call) { try { const actual = parseStrictJson(call.function.arguments); // can the arriving args be read if (typeof actual?.command === 'string' && Buffer.byteLength(actual.command, 'utf8') > 256) { failures.add('EXEC_COMMAND_OVERSIZE'); } else if (!canonicalJsonBytes(actual) // does the comparison work .equals(canonicalJsonBytes(prepared.expectedExecArguments))) { failures.add('EXEC_ARGUMENTS_MISMATCH'); // do they differ } } catch { failures.add('EXEC_ARGUMENTS_MISMATCH'); // ...everything lands here } } parseStrictJson throws when what arrived is unreadable. canonicalJsonBytes throws when the comparison itself cannot run. Both fell into the same catch, and the catch named the arguments. Full file if you want to read around it: scripts/judgment/live.mjs . The third catch site was in scripts/pr2/reducer.mjs . The fix Split the parse from the compare, and give each stage its own catch: let actual; try { actual = parseStrictJson(call.function.arguments); } catch { failures.add('EXEC_ARGUMENTS_INVALID'); } if (actual !== undefined) { try { if (typeof actual?.command === 'string' && Buffer.byteLength(actual.command, 'utf8') > 256) { failures.add('EXEC_COMMAND_OVERSIZE'); } else if (!canonicalJsonBytes(actual) .equals(canonicalJsonBytes(prepared.expectedExecArguments))) { failures.add('EXEC_ARGUMENTS_MISMATCH'); } } catch { failures.add('EXEC_COMPARATOR_ERROR'); } } Same three fixtures, same unedited arrays, only the middle line moves: unreadable arriving args -> ["SANDBOX_EVENT_CARDINALITY_INVALID", "EXEC_ARGUMENTS_INVALID", "TOOL_RESPONSE_CARDINALITY_INVALID"] usable args, different -> ["SANDBOX_EVENT_CARDINALITY_INVALID", "EXEC_ARGUMENTS_MISMATCH", "TOOL_RESPONSE_CARDINALITY_INVALID"] our comparison threw -> ["SANDBOX_EVENT_CARDINALITY_INVALID", "EXEC_COMPARATOR_ERROR", "TOOL_RESPONSE_CARDINALITY_INVALID"] The change spans three source files, plus the tests: dd1a654 . The part that was not a rename pm25coder called it "a rename, not a redesign," and from outside the repo that is exactly what it looks like. Inside, two things were waiting. There are two whitelists, not one. Failure reasons are filtered through an ordered array before they reach the output. A reason that is not in the array is silently dropped. Register the new name in the code and not in the array, and that failure stops appearing. Other failures in the same run still show, so the run does not go green by itself, but the one you just added becomes invisible. That is its own defect, and a worse one than the thing this patch fixes. A filter that discards unrecognised codes is designed to fail quietly. And there are two of these arrays, in two modules, and they are not copies. One holds 26 codes, the other holds 20, and the 20 are a strict subset of the 26. They disagree about what to do with a code neither expected, and they disagree backwards: the narrower list is the one that refuses to guess. if (!FAILURE_ORDER.includes(reason)) throw new TypeError(unknown failure reason: ${reason}); The throwing version is the correct one. The silent one should be replaced by it, and the two lists should be one registry. I did not do that here, because bundling a registry refactor into a naming fix would make both harder to review and would put a behaviour change in a commit that claims to be about labels. It is on the list as its own change. And there was a third catch site doing the same collapse in a different module, which I only found by grepping for every place that name was added rather than trusting the two I knew about. The tests, including the one that is supposed to pass Six of them. The important thing is not that they pass. Run against the parent commit, five of the six fail. Run against the patch, all six pass. The one that passes both ways is there on purpose: All six are in one file, if you want to run the ablation yourself: test/exec-comparator-error.test.mjs . arriving args unusable -> EXEC_ARGUMENTS_INVALID fails on parent usable args, differ -> EXEC_ARGUMENTS_MISMATCH passes on both EXEC_COMPARATOR_ERROR fails on parent pr2 reducer, unusable args -> EXEC_ARGUMENTS_INVALID fails on parent prepared transport, digest -> EXEC_COMPARATOR_ERROR fails on parent pr2 reducer, digest read -> EXEC_COMPARATOR_ERROR fails on parent The control is doing something specific. Splitting one catch into two created two new boundaries that the mismatch path now has to survive. If either boundary swallowed a case it should have passed through, a suite that only asserted the two new names would still be green, because the case it ate would simply never be asserted. The control fails the moment the original path stops producing the original name. The comparator failures are induced, not waited for. The arriving call parses cleanly and my own expected object carries a BigInt the canonicalizer refuses. That refusal is deliberate, not fragile. The serializer accepts a closed set - null, boolean, safe integer, NFC string, array, plain object - and rejects everything else, because its output is hashed and a canonical form cannot have alternatives. So a BigInt in an expected object is an invalid internal type, and the test is contrived at the type level. Be precise about what that buys: it proves the catch fires when the comparison cannot complete on otherwise-valid input. It does not prove a spontaneous bug in the canonicalizer, and I am not claiming one. The two digest-read tests induce a different internal failure using a throwing getter, and each one asserts the intended read was actually reached and that the error survives the final filter. Two things I got wrong on the way, both caught by someone else My first repair ran the collapse backwards. I moved the whole catch to EXEC_COMPARATOR_ERROR , which meant unreadable arriving args were now reported as my machinery failing. Same defect, opposite direction. The reason it survived my own review is worth more than the bug. I wrote the implementation, then wrote a test asserting what the implementation did. An assertion written against unverified output cannot fail, because it was derived from the thing it is supposed to check. That workflow guarantees you codify your own bugs, and it produced a test that defended the error. Then I claimed one of the three catches was dead code. I had constructed a bad expected object, watched it get caught by an earlier gate, and concluded nothing could reach that catch. One input class, generalized to all inputs. It is reachable - a failing read on the manifest digest lands there, because that read sits inside the try while the one I tested sits outside it. Neither was caught by me. The backwards repair was caught before commit. The dead-code claim went into the first commit and came out in an amendment, so it was caught before push. What this does not do It does not put the contract in the name. pm25coder's fuller point was that the label should say what it was compared against, something closer to args_mismatch_under_contract= , so the name states the authority rather than leaving a reader to infer a subject. That is not built. The names are separated. They still carry no contract id. EXEC_ARGUMENTS_INVALID does not identify who produced bad arguments. It marks a boundary. The arriving arguments were rejected, either by the parser or, on the PR2 path, by argument validation. Valid JSON can still fail that validation. Who produced them is not established by that result. The model, the relay, or the transport are all still live possibilities, and the name stops where the evidence stops. Naming a producer there would be the same defect with a friendlier label. That neutrality costs something real, and it is fair to say so. An operator wants to know if the model is emitting garbage, and unparseable JSON on a structured tool call usually is exactly that. The code declines to tell them, which is semantically clean and operationally thinner. The fix is not to guess in the name - it is to record enough in the receipt that the attribution can be made afterward by someone

Read on DEV Community ↗ ← Back to News

Comments

No comments yet. Start the discussion.