test(storage): drop unused lock fixture parameter

[Kiran01bm]

Summary

Drops the dead env parameter from the shared lock test fixtures and every call site.

Why

The parameter was retained during a test-fixture move purely for signature compatibility; both fixture bodies discard it. There was never anywhere for it to go: storage.Lock carries no environment field at all — locks are keyed by database name and type, with repository, PR, and owner alongside — so nothing environment-scoped could have been lost. It is pure noise at ~250 call sites and misleads readers into thinking lock fixtures are environment-scoped.

What

createTestLock / createTestLockWithPR lose the parameter; all call sites across six test files updated in one mechanical sweep. No behavior change.

Before / after

Before: createTestLock(t, store, dbName, dbType, env)   // env discarded in the body
After:  createTestLock(t, store, dbName, dbType)

[Copilot]

Pull request overview

This PR simplifies the internal sqlstore test fixtures by removing the unused env parameter from createTestLock / createTestLockWithPR, and updates all in-package call sites accordingly. This reduces noise in tests and avoids implying that locks are environment-scoped when they are not.

Changes:

• Removed the dead env parameter from shared lock fixtures in pkg/storage/internal/sqlstore/applies_test.go.
• Mechanically updated all fixture call sites across the sqlstore test suite to match the new signatures.
• Preserved canonical lock row shape by continuing to delegate fixture creation to pkg/storage/storagetest.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
pkg/storage/internal/sqlstore/tasks_test.go	Updates createTestLock calls to drop the unused env argument.
pkg/storage/internal/sqlstore/plan_comments_test.go	Updates lock fixture call to the new signature.
pkg/storage/internal/sqlstore/control_requests_test.go	Updates lock fixture call to the new signature.
pkg/storage/internal/sqlstore/apply_operations_test.go	Updates lock fixture calls across apply operation tests to the new signature.
pkg/storage/internal/sqlstore/apply_comments_test.go	Updates lock fixture calls across apply comment tests to the new signature.
pkg/storage/internal/sqlstore/applies_test.go	Removes env from the fixture helper signatures and updates local uses (including PR-scoped lock helper calls).

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[aparajon]

🤖 Adversarial correctness review, requested by @aparajon and performed by his agent. Reviewed at head 9973284f.

Verdict: a genuinely mechanical sweep, and the parameter really was dead — safe to land. The thing worth attacking on a ~250-call-site sweep is whether anything was quietly deleted or weakened along the way, and nothing was: zero test functions removed, zero assertions added or removed, and the only removed lines that are not a createTestLock call are the two _ = env compatibility lines the PR body describes. One optional follow-up.

Findings

1. (optional) The wrappers are now pure re-exports. With the parameter gone, createTestLock and createTestLockWithPR in applies_test.go are one-line delegations — return storagetest.CreateLock(t, store, dbName, dbType) — with no transformation or special-case handling left. That is the shape AGENTS.md's Imports rule asks callers to skip in favour of importing the source package directly, and the parameter drop is what removed the last thing the wrappers were doing (adapting a five-argument call to a four-argument one). Since this sweep already touches every call site, folding the rename into it costs almost nothing and leaves one less indirection between a test and the fixture it uses. The counter-argument is real though — createTestLock is shorter than storagetest.CreateLock at 250 sites — so this is your call, not a blocker.

2. (nit) The "environment-scoped" wording undersells the finding. storage.Lock has no environment field at all — it is keyed by database name and type, with repository, PR, owner and pending plan ID alongside. So the parameter was not merely discarded by the fixture bodies; there was never anywhere for it to go. Worth a clause in the commit message, because "the fixture ignored it" and "locks have no such concept" are different claims and the second is the more useful one for a future reader wondering whether env-scoped locking regressed.

Action items

1. (Finding 1) (optional) Delete both wrappers and call storagetest.CreateLock / CreateLockWithPR directly, in this same sweep — or keep them deliberately and say why.
2. (Finding 2) (optional) Note in the commit message that storage.Lock carries no environment field, so nothing env-scoped could have been lost.

Verified (tried to break, couldn't)

grep -E '^-\s*func Test' and ^-.*(require|assert)\. over the diff both return zero, so no test function or assertion was removed, and the added-assertion count is zero too, meaning nothing was swapped for a weaker check; the only non-createTestLock deletions in the whole 2197-line diff are the two _ = env lines; storage.Lock has no environment field, so the discarded parameter could not have been carrying meaning that is now lost; both fixtures resolve to storagetest.CreateLockWithPR, which sets database name, type, repository, PR and owner and never reads an environment; go build ./... and go vet -tags=integration ./pkg/storage/... both pass at head; all 34 CI checks are green; no internal names, hosts, or links appear in the diff.

This review was generated by Claude Code (claude-opus-5).

[aparajon]

🤖 Approving on @aparajon's behalf after the adversarial correctness review above (no blocking findings). Verified mechanically that no test or assertion was removed or weakened across the sweep.

This stamp was left by Claude Code (claude-opus-5).

[Kiran01bm]

Review response from Kiran's (@Kiran01bm) AI code review assessment agent (Amp / Claude Opus 4.5)

Both optional findings assessed: the wording fix is applied to the PR body (which becomes the squash-commit message); the wrapper deletion is deliberately deferred until #1041 lands.

#	Finding	Status	Explanation
2	"environment-scoped" wording undersells the finding — storage.Lock has no environment field	fixed	The PR body's Why section now makes the stronger claim: locks are keyed by database name/type (with repository, PR, owner alongside), so there was never anywhere for env to go and nothing environment-scoped could have been lost.
1	Wrappers are now pure one-line re-exports of storagetest.CreateLock / CreateLockWithPR	deferred	Agreed on the AGENTS.md shape, but the inline sweep re-touches ~250 call sites and #1041 (still open) adds more createTestLock calls in applies_test.go — folding it in here would enlarge that PR's conflict surface. Tracked as an internal follow-up to sweep once #1041 merges.