interp/interp_test.go (1)

1908-1927: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicated snapshot/live-refcount helpers across the three new sub-tests.

Each of the three new t.Run blocks defines its own snapshot type and a nearly identical run closure, all repeating the same live-refcount loop:

live := 0
for _, rc := range i.rc[1:] {
    if rc > 0 {
        live += rc
    }
}

Additionally, factoring run out as a closure shared across multiple cases entries departs from every other multi-case test in this file, which inlines setup/execution/assertions directly inside each t.Run(tt.name, func(t *testing.T) {...}) (e.g. the "host call releases the consumed callable ref...", "fused UPVAL_GET+CONST binop..." tables). As per coding guidelines, **/*_test.go should "Inline test setup, execution, and assertions unless the documented exception in §6.8 applies" — please confirm this shared-helper pattern falls under that documented exception, since it's a new style relative to the rest of the file.

At minimum, the duplicated live-refcount computation is worth consolidating.

♻️ Proposed dedup for the live-refcount computation

+func liveRefCount(i *Interpreter) int {
+	live := 0
+	for _, rc := range i.rc[1:] {
+		if rc > 0 {
+			live += rc
+		}
+	}
+	return live
+}

Then each run closure becomes live: liveRefCount(i).

Also applies to: 1979-1996, 2045-2063

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@interp/interp_test.go` around lines 1908 - 1927, Consolidate the repeated
live-reference counting from the three test cases into a shared liveRefCount
test helper, then have each run closure use it when populating snapshot.live.
Review the local snapshot/run helpers in the affected t.Run blocks and inline
setup, execution, and assertions unless the project’s documented exception
explicitly permits this shared closure pattern; preserve all existing snapshot
fields and test behavior.

Source: Coding guidelines

internal/cmd/geninterp/generate_test.go (1)

249-336: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff

This subtest asserts private implementation shape rather than behavior.

uses role based lowering names parses the generator's own sources and asserts exact struct field names/ordering (Lines 288-293) and the presence/absence of internal helper function names (Lines 295-318). This couples the test to internal structure rather than observable generator behavior, so ordinary refactors (renaming a helper, reordering a field) will break it even when generated output is unchanged.

Consider narrowing this to behavior that matters (e.g., generated-output assertions like the neighboring compose subtests), or at minimum limiting it to the naming invariants you truly want to freeze.

As per coding guidelines: "Tests must follow docs/coding-patterns.md §6 and assert behavior rather than private implementation shape."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/cmd/geninterp/generate_test.go` around lines 249 - 336, The subtest
uses AST inspection in “uses role based lowering names” to freeze private type
fields and helper names instead of validating behavior. Replace the exact
struct-field and function presence/absence assertions with generated-output or
other observable behavior assertions, retaining only naming invariants required
by the public generator contract and preserving the existing compose/lower
behavior checks.

Source: Coding guidelines

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes