Deep Review: 20260405-215736-fix-slice-negative-index

Date	2026-04-05 21:57
Repo	mikefarah/yq
Round	1
Author	Jan Dubois <jan@jandubois.com>
Branch	`fix-slice-negative-index`
Commits	`b18f62b0` Fix panic on negative slice indices that underflow after adjustment
Reviewers	Claude Opus 4.6, Codex GPT 5.4, Gemini 3.1 Pro
Verdict	Merge with fixes — the fix is correct but the regression test does not trigger the original panic
Wall-clock time	18 min 8 s

Executive Summary ¶

This change clamps both slice index bounds to [0, len(Content)] after negative-to-positive adjustment, preventing the out-of-bounds panic reported by OSS-Fuzz. The clamping logic is correct and symmetric. The regression test, however, uses an expression (.[0:-99999]) that produces the same empty-sequence result with or without the fix — it never triggers the original panic, so it provides no protection against regression.

Critical Issues ¶

None.

Important Issues ¶

I1. Regression test does not exercise the original panic (important, gap) Claude Codex Gemini ¶

The expression .[0:-99999] only exercises second-index clamping (lines 54–55 of operator_slice.go). On unpatched code, relativeSecondNumber computes to 3 + (-99999) = -99996, making the loop condition 0 < -99996 false — the loop terminates immediately without any out-of-bounds access. The test produces the same empty sequence with or without the fix.

pkg/yqlib/operator_slice_test.go

		expression: `.[-11:-10]`,
		expected: []string{
			"D0, P[], (!!seq)::- cat1\n",
		},
	},
	{
		// Regression test for https://issues.oss-fuzz.com/issues/438776028
		// Negative slice index that underflows after adjustment must not
		// cause an out-of-bounds access.
		skipDoc:    true,
		document:   `[a, b, c]`,
		expression: `.[0:-99999]`,
		expected: []string{
			"D0, P[], (!!seq)::[]\n",
		},
	},
}

The reported panic requires a first-index underflow: .[-99999:3] computes relativeFirstNumber = 3 + (-99999) = -99996, then the loop accesses lhsNode.Content[-99996] at line 65, triggering the panic. Gemini verified this by running the expression against unpatched code.

pkg/yqlib/operator_slice.go

		log.Debugf("calculateIndicesToTraverse: slice from %v to %v", relativeFirstNumber, relativeSecondNumber)

		var newResults []*CandidateNode
		for i := relativeFirstNumber; i < relativeSecondNumber; i++ {
			newResults = append(newResults, lhsNode.Content[i])
		}

The fix adds first-index clamping at lines 39–43 that has no test coverage.

pkg/yqlib/operator_slice.go

		relativeFirstNumber := firstNumber
		if relativeFirstNumber < 0 {
			relativeFirstNumber = len(lhsNode.Content) + firstNumber
		}
		if relativeFirstNumber < 0 {
			relativeFirstNumber = 0
		} else if relativeFirstNumber > len(lhsNode.Content) {
			relativeFirstNumber = len(lhsNode.Content)
		}

Fix: add tests that exercise first-index underflow (which panics without the fix) and ideally both-index underflow:

 	{
 		skipDoc:    true,
 		document:   `[a, b, c]`,
 		expression: `.[0:-99999]`,
 		expected: []string{
 			"D0, P[], (!!seq)::[]\n",
 		},
 	},
+	{
+		skipDoc:    true,
+		document:   `[a, b, c]`,
+		expression: `.[-99999:3]`,
+		expected: []string{
+			"D0, P[], (!!seq)::- a\n- b\n- c\n",
+		},
+	},
+	{
+		skipDoc:    true,
+		document:   `[a, b, c]`,
+		expression: `.[-99999:-99998]`,
+		expected: []string{
+			"D0, P[], (!!seq)::[]\n",
+		},
+	},

Suggestions ¶

S1. Extract shared clamping logic into a helper (suggestion, enhancement) Claude Gemini ¶

The same negative-adjustment-then-clamp pattern appears twice, once per index. A helper would remove duplication and make the intent self-documenting.

pkg/yqlib/operator_slice.go

		relativeFirstNumber := firstNumber
		if relativeFirstNumber < 0 {
			relativeFirstNumber = len(lhsNode.Content) + firstNumber
		}
		if relativeFirstNumber < 0 {
			relativeFirstNumber = 0
		} else if relativeFirstNumber > len(lhsNode.Content) {
			relativeFirstNumber = len(lhsNode.Content)
		}

		secondNumber, err := getSliceNumber(d, context, lhsNode, expressionNode.RHS)
		if err != nil {
			return Context{}, err
		}

		relativeSecondNumber := secondNumber
		if relativeSecondNumber < 0 {
			relativeSecondNumber = len(lhsNode.Content) + secondNumber
		}
		if relativeSecondNumber < 0 {
			relativeSecondNumber = 0
		} else if relativeSecondNumber > len(lhsNode.Content) {
			relativeSecondNumber = len(lhsNode.Content)
		}

Fix: extract a helper and simplify both call sites:

func clampSliceIndex(index, length int) int {
	if index < 0 {
		index += length
	}
	if index < 0 {
		return 0
	}
	if index > length {
		return length
	}
	return index
}

Both call sites reduce to relativeFirstNumber := clampSliceIndex(firstNumber, len(lhsNode.Content)).

S2. No type guard for non-sequence nodes (suggestion, gap) Gemini ¶

sliceArrayOperator assumes lhsNode is a sequence. Slicing a scalar or mapping node silently produces a confusing result (an empty sequence tagged !!str or !!map). Gemini verified: echo '"hello"' | yq eval '.[0:2]' outputs !!str []. This is pre-existing and not worsened by the fix; it may be intentional.

pkg/yqlib/operator_slice.go

	for el := context.MatchingNodes.Front(); el != nil; el = el.Next() {
		lhsNode := el.Value.(*CandidateNode)

Fix: if not intentional, add a lhsNode.Kind != SequenceNode guard with a clear error message.

Design Observations ¶

Strengths

The fix clamps both indices symmetrically, even though only the second index was reported. This prevents the same panic through the first-index path. (in-scope) Claude
Clamping uses [0, len(Content)] rather than [0, len(Content)-1], correctly preserving half-open interval semantics. (in-scope) Claude

Testing Assessment ¶

The existing test suite covers normal slicing, omitted first/second index, negative indices within range, indices beyond array length, and multi-array contexts. The new regression test covers the new second-index clamping path but does not exercise the original panic.

Untested scenarios ranked by risk:

First-index underflow (.[-99999:3]) — exercises lines 39–40, the only new code path without coverage. (see I1)
Both indices underflow (.[-99999:-99998]) — exercises both clamping paths simultaneously.
First-index underflow on an empty sequence ([] | .[-99999:]) — edge case combining empty array with underflow.

Documentation Assessment ¶

No documentation gaps. The commit message explains the root cause, fix, and links the OSS-Fuzz issue.

Commit Structure ¶

Single commit, single logical change. The message accurately describes the fix. Clean.

Agent Performance Retro ¶

Claude

Claude read both changed files, ran git blame, ran the test suite, and searched the codebase for similar patterns. It correctly identified the missing first-index test coverage (I1) and the helper extraction opportunity (S1). It also noted two design strengths (symmetric clamping and correct half-open interval semantics).

Claude did not verify whether the existing test actually triggers the panic on unpatched code — it assumed the test was partially effective rather than wholly ineffective.

Codex

Codex was the fastest agent (82s). It ran targeted git blame on both the fix and test lines, ran the test suite, and searched for related patterns. It identified the missing first-index test (I1) with a clear explanation and concrete fix suggestion including both first-index and both-index underflow tests.

Like Claude, Codex did not verify whether the existing test panics on unpatched code.

Gemini

Gemini was the most thorough in verification, spending 344s and making 42 tool calls. It checked out the pre-fix commit and ran the test expression against unpatched code, confirming that .[0:-99999] does not panic. It also verified that .[-99999:-99998] does panic on unpatched code. This empirical verification was the review's most valuable contribution.

Gemini also found a pre-existing type-guard gap (S2) by testing echo '"hello"' | yq eval '.[0:2]' — an issue no other agent discovered.

Gemini never ran git blame, consistent with known quota limitations for the daily Pro model. However, it compensated by running code against both patched and unpatched versions — a stronger verification method for this review.

Tool call highlights

Claude: Used git blame (1 call) to verify authorship. Ran tests (1 call). Searched for similar patterns with 4 grep calls. Efficient 11-call session.
Codex: Used git blame (2 targeted calls on specific line ranges). Ran tests twice. Searched with 4 rg calls. Efficient 16-call session.
Gemini: Never ran git blame. Made 14 git checkout calls to switch between patched and unpatched code — high volume but justified by the empirical verification approach. Ran 12 echo | go run commands testing various edge cases. Extensive exploration (42 calls total) that produced the review's key insight.

Summary

	Claude Opus 4.6	Codex GPT 5.4	Gemini 3.1 Pro
Duration	143s	82s	344s
Findings	0C 1I 1S	0C 1I 0S	0C 1I 2S
Tool calls	11 (Read 3, Bash 4, Grep 4)	16 (exec_command 14)	42 (run_shell_command 35, read_file 4)
Design observations	2	0	0
False positives	0	0	0
Downgraded	0	0	1 (I→S)
Unique insights	0	0	1 (S2)
Files reviewed	2	2	2
Coverage misses	0	0	0

Reconciliation: Gemini I2 (missing type guard for non-sequence nodes): important → suggestion S2. The issue is pre-existing and unrelated to the negative-index fix; it does not meet the "important" bar for a change-scoped review.

Gemini provided the most value on this review. Its empirical verification — running the test expression against unpatched code — revealed that the regression test is not merely incomplete but wholly ineffective. Claude and Codex both identified the missing first-index coverage but missed that the existing test also fails to serve its stated purpose. Codex was the most efficient, delivering a focused review in under 90 seconds.