test(task): add contract tests for repair-args double-encode detection#3332
test(task): add contract tests for repair-args double-encode detection#3332oldschoola wants to merge 2 commits into
Conversation
17 tests covering:
- repairDoubleEncodedJsonString: double-encode detection via structural
escapes (", \, \uXXXX), false-positive avoidance (Windows paths,
regexes, lone \n mentions), reference stability when unchanged
- repairTaskParams: flat/batch form repair, per-task-item repair,
null/undefined defensive handling, partially streamed params
roboomp
left a comment
There was a problem hiding this comment.
review:p0 for a small test-only change with no production behavior risk; targeted repair tests pass locally (27 pass, including the existing suite).
Two should-fix cleanup items: the new file duplicates existing repair-args coverage on origin/main, and one string “same reference” assertion is tautological for a primitive.
Thanks for tightening coverage around the task argument repair path.
| import { repairDoubleEncodedJsonString, repairTaskParams } from "../../src/task/repair-args"; | ||
| import type { TaskParams } from "../../src/task/types"; | ||
|
|
||
| describe("repairDoubleEncodedJsonString", () => { |
There was a problem hiding this comment.
should-fix: origin/main already has packages/coding-agent/test/tools/task-repair-args.test.ts covering the same repairDoubleEncodedJsonString contract (uniform decode, Windows paths, regexes, lone \n, no-op). Adding a second suite here duplicates those cases and splits the contract across two files; please fold only the new batch/context/defensive cases into the existing test file, or move the existing suite rather than keeping both.
| expect(repairDoubleEncodedJsonString(input)).toBe(input); | ||
| }); | ||
|
|
||
| it("returns same reference when nothing changed", () => { |
There was a problem hiding this comment.
should-fix: string is a primitive, so toBe(input) here checks value equality, not object identity/reference reuse. This duplicates the no-backslash no-op assertions above without defending a separate contract; either drop it or change the test to an object-level identity case like repairTaskParams already does.
Address review feedback: origin/main already had packages/coding-agent/test/tools/task-repair-args.test.ts covering the same repairDoubleEncodedJsonString contracts. - Removed duplicate packages/coding-agent/test/task/repair-args.test.ts - Added only new cases to the existing file: - backslash-backslash decode - batch form context repair - per-task-item repair - null/undefined defensive handling - partially streamed params (missing fields)
|
Fixed in the latest push. Removed the duplicate |
|
Closing — the reviewer correctly identified that |
Summary
Adds 17 contract tests for
repairDoubleEncodedJsonStringandrepairTaskParamsinpackages/coding-agent/src/task/repair-args.ts— a pure function module with no prior test coverage.What's tested
repairDoubleEncodedJsonString(9 tests)\nmention untouched (incidental, not double-encoded —hasDoubleEncodeSignaturerequires a structural escape or 2+ backslashes)\\)\Uis not a valid JSON escape)\dis not a valid JSON escape)repairTaskParams(8 tests)Verification
bun checkpasses