Deep Review: 20260405-211911-fix-map-equality-null-key

Date	2026-04-05 21:19
Repo	mikefarah/yq
Round	1
Author	Jan Dubois
Branch	`fix-map-equality-null-key`
Commits	`f7d884ae` Fix panic in recurseNodeObjectEqual when comparing maps with null keys
Reviewers	Claude Opus 4.6, Codex GPT 5.4, Gemini 3.1 Pro
Verdict	Merge with fixes — the one-line fix is correct, but the same bug persists in `containsObject`
Wall-clock time	11 min 6 s

Executive Summary ¶

This branch replaces findInArray with findKeyInMap in recurseNodeObjectEqual, fixing an out-of-bounds panic when comparing maps whose keys include null values. The fix is correct, minimal, and well tested. All three reviewers independently identified the same parallel bug in containsObject, which uses findInArray on a MappingNode with a %2 guard that prevents the panic but produces false negatives.

Critical Issues ¶

None.

Important Issues ¶

I1. containsObject has the same findInArray bug (important, gap) Claude Codex Gemini ¶

containsObject at line 49 searches a MappingNode's Content array with findInArray, which iterates by 1 and can match a key against a value at an odd index. The %2 guard at line 51 prevents the out-of-bounds panic this branch fixes in recurseNodeObjectEqual, but it introduces false negatives: when a null key exists later in the map but a null value appears first, findInArray returns the value's odd index, the guard rejects it, and the function reports the key as missing.

pkg/yqlib/operator_contains.go

	}
	for index := 0; index < len(rhs.Content); index = index + 2 {
		rhsKey := rhs.Content[index]
		rhsValue := rhs.Content[index+1]
		log.Debugf("Looking for %v in the lhs", rhsKey.Value)
		lhsKeyIndex := findInArray(lhs, rhsKey)
		log.Debugf("index is %v", lhsKeyIndex)
		if lhsKeyIndex < 0 || lhsKeyIndex%2 != 0 {
			return false, nil
		}
		lhsValue := lhs.Content[lhsKeyIndex+1]
		log.Debugf("lhsValue is %v", lhsValue.Value)

		itemInArray, err := contains(lhsValue, rhsValue)

Codex verified this live: printf '? 1\n: ~\n? ~\n: x\n' | yq 'contains({~: "x"})' returns false even though the map contains null: x.

git blame confirms this code predates the current change (Mike Farah, 2021, commit 2db8140d7).

Fix: replace findInArray with findKeyInMap and remove the even-index guard.

-       lhsKeyIndex := findInArray(lhs, rhsKey)
+       lhsKeyIndex := findKeyInMap(lhs, rhsKey)
        log.Debugf("index is %v", lhsKeyIndex)
-       if lhsKeyIndex < 0 || lhsKeyIndex%2 != 0 {
+       if lhsKeyIndex < 0 {
            return false, nil
        }

Suggestions ¶

None.

Design Observations ¶

Concerns

1. Map lookups lack a single, safe primitive (in-scope) Claude Codex Gemini — findInArray accepts any *CandidateNode without distinguishing sequences from maps. This makes it easy to use on a MappingNode by mistake, as both recurseNodeObjectEqual and containsObject demonstrate. Converting all map-key searches to findKeyInMap (or adding a type assertion to findInArray) would eliminate this class of bug rather than requiring each call site to remember the stride.

Strengths

The fix reuses the existing findKeyInMap function rather than introducing new abstractions.
Both a low-level unit test (TestRecurseNodeObjectEqual) and an operator-level integration test (. += . with sequence keys) validate the fix from different angles.

Testing Assessment ¶

The new tests cover the reported crash well:

lib_test.go:304-320 — directly verifies recurseNodeObjectEqual returns false for maps with different key types (null vs int).
operator_add_test.go:531-541 — end-to-end regression test using the exact input shape from the OSS-Fuzz report.

Untested scenarios (ranked by risk):

contains with null keys where a null value appears earlier in the map (exercises the containsObject parallel bug from I1).

Documentation Assessment ¶

No updates needed. The regression test comments include the OSS-Fuzz issue link, which is appropriate documentation for a fuzz-discovered bug fix.

Commit Structure ¶

Single commit with a clear message stating what changed and why. The scope is tight: one bug fix with two regression tests.

Agent Performance Retro ¶

Claude

Found I1 (containsObject gap), rated it Important. Provided a concrete false-negative example and traced the %2 guard logic. Ran 4 git blame commands to confirm the code predates this change. Also grepped for all callers of findInArray and findKeyInMap, read operator_omit.go and operator_add.go to check for further instances, and ran the test suite to verify the new tests pass. Accurate, thorough, no false positives.

Codex

Found I1, also rated Important. Distinguished itself by live-verifying the bug: ran printf '? 1\n: ~\n? ~\n: x\n' | go run ./yq.go 'contains({~: "x"})' and confirmed the false-negative output. Also read operator_subtract.go and operator_contains_test.go to check for parallel patterns. Ran git blame and the test suite. No false positives.

Gemini

Found the same issue as C1 (Critical). Wrote test files in its worktree to verify the bug. Ran 1 git blame (expected — Gemini's Pro model quota limits blame calls). Also checked operator_has.go for related patterns. No false positives.

Tool call highlights:

Claude — 4 git blame calls, systematic grep for findInArray/findKeyInMap/recursiveNodeEqual. Read 6 files beyond the 3 changed files.
Codex — 2 git blame calls, 4 rg searches. Uniquely ran go run ./yq.go to verify the bug produces wrong output live.
Gemini — 1 git blame call, 6 grep searches. Wrote 2 test files to verify the bug experimentally. Broader grep for find across the codebase.

Summary

	Claude Opus 4.6	Codex GPT 5.4	Gemini 3.1 Pro
Duration	151s	114s	140s
Findings	0C 1I 0S	0C 1I 0S	0C 1I 0S
Tool calls	20 (Read 10, Bash 7)	23 (exec_command 20)	19 (grep 6, shell 6)
Design observations	2	3	2
False positives	0	0	0
Unique insights	0	1 (live verification)	0
Files reviewed	3/3	3/3	3/3
Coverage misses	0	0	0

Downgraded: Gemini rated the containsObject finding as Critical; consolidated to Important. The %2 guard prevents the panic — the bug produces false negatives, not crashes. A pre-existing gap with no reported user impact does not meet the Critical bar.

Reconciliation: Gemini P1 containsObject finding: critical → important I1.

All three agents converged on the same finding with zero false positives. Codex's live verification added the most value by confirming the false-negative behaviour with a concrete command. Claude's systematic blame and grep coverage was the most thorough for attribution. Gemini's experimental test-writing approach was creative but produced no additional findings.

Review Process Notes ¶

No skill improvements or repo context updates warranted. All three agents found the same finding independently — the prompt guided them well. The small diff size made complete coverage straightforward.