test(api): regression for remove_granular_acl on multi-version scripts (WIN-2004)

[rubenfiszel]

Summary

remove_granular_acl used a CTE whose result was read as a scalar subquery in
RETURNING (SELECT old_write FROM old)::bool. The script table is keyed on
(workspace_id, hash), so several rows can share the same
(workspace_id, path) across versions. add_granular_acl writes extra_perms
to every row matching the path, so once a script has ≥2 versions carrying
the permission key, that CTE returned more than one row and PostgreSQL rejected
the request with:

more than one row returned by a subquery used as an expression

The production fix — LIMIT 1 on the RETURNING scalar subquery — already
shipped in #9388, but without a regression test. This PR adds that missing
test so the bug can't silently come back.

What this PR contains

• backend/tests/granular_acl_multi_version.rs — integration test that:
  1. creates a script and a second version at the same path (two rows sharing (workspace_id, path)),
  2. grants a granular ACL (lands the perm key on both versions),
  3. revokes it and asserts the request succeeds (HTTP 200),
  4. asserts the permission key is gone from every version.

No production code changes — granular_acls.rs is unchanged from main.

Validation

• With the fix: test passes.
• Without the fix (temporarily reverted LIMIT 1 locally): test fails with
  exactly more than one row returned by a subquery used as an expression
  (HTTP 400), confirming it reproduces the reported bug.
• The test uses the runtime sqlx::query_scalar() function, so no .sqlx
  offline-cache update is required.

Fixes WIN-2004

🤖 Generated with Claude Code

---

Summary by cubic

Adds a regression test for remove_granular_acl on scripts with multiple versions to prevent the multi-row scalar subquery error from returning. The test creates two versions at the same path, grants then revokes a granular ACL, and asserts 200 plus removal from all versions, covering WIN-2004.

^{Written for commit 03c3f08. Summary will update on new commits.}

[claude]

Claude finished @rubenfiszel's task in 3m 48s —— View job

---

Code review

Good to merge.

This is a clean, correct regression test for remove_granular_acl on multi-version scripts (WIN-2004). I verified the test against the codebase rather than just reading it:

Correctness checks (all pass):

• ✅ Hash is clean text, not quoted JSON — create_script returns the hash via Ok((StatusCode::CREATED, format!("{}", hash))) (scripts.rs:643), so let v1_hash: String = resp.text().await? yields a bare hash and v2["parent_hash"] = json!(v1_hash) resolves the parent correctly. This matches the existing scripts.rs integration test, which also feeds a String hash into parent_hash.
• ✅ The double resp.text() (assert message on line 63, then binding on line 64) compiles — resp.text() consumes resp, but it only runs inside the assert_eq! panic branch, which diverges, so the continuing path still owns resp.
• ✅ #[sqlx::test(fixtures("base"))] is the right form for backend/tests/ — backend/migrations exists, so the default migration path resolves; python_jobs.rs and others use the same attribute. SECRET_TOKEN / test-workspace / test-user all come from fixtures/base.sql.
• ✅ The scenario actually reproduces the bug — the test asserts two rows share (workspace_id, path) and that both carry the perm key before removal, which is precisely the condition that made the pre-fix scalar subquery return >1 row. The production fix (RETURNING (SELECT old_write FROM old LIMIT 1)::bool) is present at granular_acls.rs:395, so the test passes on this branch.
• ✅ No production code changes, as stated.

Findings: none (no P0/P1/P2).

Optional note (non-blocking): there is already an ACL-focused integration test at backend/windmill-api-integration-tests/tests/granular_acls.rs (test_granular_acls_endpoints). That crate would be a slightly more discoverable home and offers reusable helpers (authed, assert_2xx). That said, backend/tests/ is an established location for endpoint tests (scripts.rs, python_jobs.rs, etc.), so the current placement is perfectly valid — purely a discoverability preference, not a defect.

Test coverage

This PR is test coverage — a backend integration test for a previously-fixed-but-untested DB bug. It exercises the full HTTP path (create → add ACL → remove ACL → assert key gone) and asserts both the success status and the post-state. No further automated tests needed. No manual verification required beyond CI running the new test.

[cubic-dev-ai]

No issues found across 1 file

_{Re-trigger cubic}

[github-actions]

I've examined the diff, the referenced production code (granular_acls.rs), the existing regression-test patterns, and the broader resp.text() usage in the test suite. Here's my review:

Pi Review

Good to merge.

This is a clean regression test that adds the missing coverage mentioned in the PR description. The test properly sets up the multi-version script scenario, verifies the two-version invariant, grants/revokes a granular ACL, and asserts the permission key is gone from every version.

What I checked

• resp.text().await? inside assert_eq! format args — initially looked like a double-consumption bug, but this is the established pattern across the test suite (e.g. protection_rules.rs, app_anonymous_execution_mode.rs, folder_default_permissioned_as.rs). Rust's assert_eq! only evaluates format arguments when the assertion fails, so the body is still available on the next line when the test passes normally. The test is correct.

• Test follows existing conventions — #[sqlx::test(fixtures("base"))], ApiServer::start, initialize_tracing(), authed() helper, and the new_script()-with-content signature are all consistent with sibling regression tests like script_auto_parent_archived.rs.

• No production code changes — the diff is test-only. The LIMIT 1 fix referenced by the test already shipped in the prior PR mentioned in the description.

Test coverage

Backend: this is a new integration test covering a previously untested edge case (multi-version script + granular ACL removal). The PR description confirms the test was validated both with and without the fix — it passes with LIMIT 1 and fails with the exact expected error message without it. No additional tests are needed.

No manual verification required — this is a backend-only integration test with no UI surface.

[cloudflare-workers-and-pages]

Deploying windmill with    Cloudflare Pages

Latest commit:	03c3f08
Status:	✅  Deploy successful!
Preview URL:	https://0ff78ddc.windmill.pages.dev
Branch Preview URL:	https://ruben-win-2004-fix-remove-gr.windmill.pages.dev

View logs

[github-actions]

Codex Review

Good to merge.

No confirmed issues found. Checked the backend test-only diff for bugs, security concerns, and AGENTS.md compliance.

Test coverage

This PR adds a targeted backend integration regression test for removing granular ACLs from multi-version scripts, which matches the changed surface. No in-app manual verification is needed because the diff only adds test coverage.

Manual verification still useful before merge: run the targeted backend test for backend/tests/granular_acl_multi_version.rs in CI or a backend dev environment with the expected SQLx/database setup.