fix(json): parse independent of key order and decode string escapes - #11
Merged
Conversation
`@str-parse-json` selected the containing object only when the wanted key
was written first, and JSON objects are unordered. A document with its
members reordered left the output hash unassigned, and the caller's
`${(@q)${(@z)...}}` then aborted with "bad set of key/value pairs for
associative array". Select the smallest object that declares the key.
String bodies were also stored raw, so `\\`, `\t`, `\n`, `\/` and
`\uXXXX` reached callers as backslashes. A caller cannot repair that
itself: replacing `\"` with `"` turns the JSON `"a\\\"b"`, which denotes
the literal `a\"b`, into `a"b`. Add `@str-unescape-json` and decode each
double-quoted body as it is captured.
Surrogate pairs, unknown escapes and malformed `\u` are left as written.
The two new parser tests fail against the previous behaviour. The suite
had covered the parser with one assertion on `{'a':{'b':'1'}}`, which is
not valid JSON and exercised neither defect.
Refs z-shell/zi#513
There was a problem hiding this comment.
🟡 Changes recommended
One critical and two moderate issues remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR updates the JSON parser to handle unordered object keys and decode string escapes.
Changes:
- Revises object selection for key-order independence.
- Adds and autoloads
@str-unescape-json. - Adds regression tests and documentation.
File summaries
| File | Summary | Findings |
|---|---|---|
zsh-string-lib.lib.zsh |
Registers the new helper. | No findings reported. |
tests/main.zunit |
Adds parser and escape-decoding tests. | No findings reported. |
functions/@str-unescape-json |
Implements JSON escape decoding. | Moderate (3 votes): bypasses caller-local REPLY; accepts surrogate code units contrary to documented behavior. |
functions/@str-parse-json |
Updates object selection and string handling. | Critical (3 votes): raw scanning can match key-like text inside quoted values. |
docs/README.md |
Documents revised behavior and fork maintenance. | No findings reported. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if [[ $__text = [[:space:]]#\{[[:space:]]#[\"\']${__key}[\"\']* ]]; then | ||
| # JSON objects are unordered, so the wanted object is the smallest one that | ||
| # declares the key, not merely one that happens to open with it. | ||
| if [[ $__text = [[:space:]]#\{*[\"\']${__key}[\"\'][[:space:]]#:* ]]; then |
Comment on lines
+35
to
+42
| if [[ $__esc == u && $__tail == (#b)([0-9a-fA-F](#c4))(*) ]] { | ||
| __code=16#$match[1] | ||
| __out+=${(#)__code} __rest=$match[2] | ||
| } elif (( ${+__map[$__esc]} )) { | ||
| __out+=$__map[$__esc] __rest=$__tail | ||
| } else { | ||
| __out+="\\$__esc" __rest=$__tail | ||
| } |
| } | ||
| done | ||
|
|
||
| typeset -g REPLY=$__out$__rest |
prettier 2.8.3, pinned by .trunk/trunk.yaml, predates GitHub alert syntax and folds the GitHub alert marker onto the following line, which stops the callout rendering at all. Write it as a bold lead-in instead of fighting the formatter.
ss-o
added a commit
that referenced
this pull request
Sep 12, 2026
The workflow triggered on `pull_request_target' with a bare actions/checkout, which resolves to the base branch. Every pull request run therefore tested `main': on #11 it reported "6 tests run" while that branch has 9, and reported success without executing a single changed line. The job installs zunit from public sources and reads no secrets, so the plain `pull_request' trigger is both correct and the safer default. Co-authored-by: Sal <ss-o@users.noreply.github.com>
This was referenced Sep 12, 2026
Closed
Closed
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
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.
Problem
Two independent defects in
@str-parse-json. Both were found inz-shell/zi,whose
.zi-parse-jsonis a fork of this function, so both copies had them.z-shell/zi#514 fixes the fork; this carries the same fixes back here.
Issues are disabled on this repository, so z-shell/zi#513 holds the full write-up.
Key order. The containing object was selected only when the wanted key
opened it. JSON objects are unordered, so writing the members the other way
round left the output hash unassigned and the caller's
${(@Q)${(@z)...}}thenaborted with
bad set of key/value pairs for associative array.Escapes. String bodies were stored raw:
"a\\b"a\\ba\b"p\tq"\t"l1\nl2"\n"xAy"AxAyA caller could not repair this without re-implementing the decoder: replacing
\"with"turns the JSON"a\\\"b", which denotes the literala\"b, intoa"b.Change
with it. For every document that parses today the smallest such object is the
same one, so existing results do not change.
@str-unescape-json, autoload it, and decode each double-quoted body as itis captured.
Surrogate pairs, unknown escapes such as
\q, and malformed\uare leftexactly as written.
Verification
zunit: 9 tests, all pass. The two new parser tests fail against the previousbehaviour, verified by running them on an unmodified
maincheckout (7 pass,2 fail).
zsh -nclean on both functions and the loader.zi-side tests was silently stripped of
Aby a quoting layer and passedwithout testing anything.
Docs
docs/README.mddocumented the first defect as a requirement ("It must be thefirst key in the object to map"); that line is corrected. The new function is
documented, and a note records that
z-shell/zicarries a maintained fork whichis not expected to stay byte-identical, so fixes are carried by hand in both
directions.
Note for reviewers
The suite previously covered the parser with a single assertion on
{'a':{'b':'1'}}, which is not valid JSON and exercised neither defect. That iswhy both copies of this function kept these bugs.
Refs z-shell/zi#513
Two CI observations from this PR
The ZUnit check on this pull request did not test this pull request. The
workflow triggers on
pull_request_targetwith a bareactions/checkout, whichdefaults to the base branch, so the run reports
6 tests runwhile this branchhas 9. Its green tick reflects
main, not the head. The verification above wasrun locally instead, including against an unmodified
maincheckout to confirmthe two new parser tests fail there. Switching the trigger to
pull_requestwould make the tick mean something; that is a separate change and is not
included here.
prettier 2.8.3, pinned by
.trunk/trunk.yaml, predates GitHub alert syntaxand folds a
> [!NOTE]marker onto the following line, which stops the calloutrendering. The fork note is written as a bold lead-in instead. Noting it so the
next person does not spend the same time on it.