polish: batch policy_type deprecation UPDATE (PR #606 follow-up)

[jaidhyani]

Autonomous overnight work from nightly sitrep — needs human review.

Trello: https://trello.com/c/gw6MJIlG (low-priority follow-ups from PR #606 review).

Scope

The card has three items. Items 1 (acronym-then-word regex split in derive_builtin_name) and 3 (intentional-coupling comment on the _apply_sqlite_migrations import in the integration test) were already landed in commit 85872f5b on main, so this PR only does item 2.

Item 2 — batch the deprecation UPDATE

sync_policy_types marked stale built-in rows deprecated with one UPDATE per row inside a Python loop (after a SELECT + set-difference). Replaced with a single batched UPDATE ... WHERE definition_type = 'built-in' AND class_ref NOT IN ($1, ..., $N), placeholders built dynamically from the seen class_refs. The db_sqlite $N->? translator handles the dynamic placeholder list, so it stays portable across asyncpg and SQLite.

Empty-seen edge case (NOT IN () is invalid SQL): when no class_refs imported successfully, falls back to deprecating all built-in rows — matching the old loop's behavior. Locked by a new test, test_sync_deprecates_all_when_no_classes_seen.

Incidental fix (required for validation)

The sqlite_e2e sync tests are deselected by default in addopts, and a stale hardcoded == 18 assertion had drifted from the actual REGISTERED_BUILTINS length (19, after the DeAI preset was added in 33dc4280). The suite was red before any change here. Rewrote the three count assertions to derive from len(REGISTERED_BUILTINS) so they can't go stale again.

Validation

• pytest tests/luthien_proxy/integration_tests/test_policy_type_sync.py -m sqlite_e2e — all pass
• pytest tests/luthien_proxy/unit_tests/test_policy_types.py — all pass
• dev_checks on changed files

Not done

Items 1 and 3 — already on main. No code change needed; verified present and correct.

[github-actions]

CHANGELOG reminder — This PR has no changelog fragment.

Add a file to changelog.d/ (see changelog.d/README.md for format).
If this is a chore/infra-only change, add the skip-changelog or chore label to suppress this reminder.

[claude]

Review

Tight, well-scoped change. The N+1 → 1 round-trip is a clear win, the empty-seen edge case is correctly handled and locked by a new test, and the stale == 18 fix is a good incidental improvement.

Issues

1. dev/OBJECTIVE.md is being committed at the wrong path (blocking)

CLAUDE.md is explicit:

dev/scratch/: Gitignored. Per-worktree planning space — OBJECTIVE.md, NOTES.md, in-flight plans, design iterations all live here. Scoped to the worktree's life; never merged to main.

And .gitignore confirms dev/scratch/ is ignored. This PR ships a 60-line planning doc to dev/OBJECTIVE.md (tracked). Please move it to dev/scratch/OBJECTIVE.md (or just delete it — git history already captures the why via the PR body). Otherwise it sets a precedent for shipping per-worktree planning into main.

Nits

2. Placeholder generation could be a touch clearer (src/luthien_proxy/policy_types.py:175)

placeholders = ", ".join(f"${i + 2}" for i in range(len(seen_class_refs)))

Slightly clearer with enumerate(..., start=2):

placeholders = ", ".join(f"${i}" for i, _ in enumerate(seen_class_refs, start=2))

The i + 2 arithmetic is correct ($1 is the True literal, class_refs start at $2), but the version above makes the start offset explicit and removes the off-by-one risk on future edits. Take it or leave it.

Correctness check — passes

• $1 = True, $2..$N+1 = *seen_class_refs — argument positions match placeholders. ✓
• _translate_params in db_sqlite.py:50 does generic $N → ? substitution, so the dynamic placeholder list portably round-trips through SQLite. ✓
• Empty-seen guard preserves old loop behavior (the old set-difference would have included every existing built-in row when seen_class_refs is empty). ✓
• The deprecation UPDATE only sets deprecated = TRUE and doesn't bump updated_at for already-deprecated rows — same as the old loop (it also iterated over existing - seen and would re-update already-deprecated rows without touching updated_at). Behavior preserved. ✓
• No SQL injection risk: placeholder strings are built from range(...) integers, never from class_ref values. ✓

Test coverage — good

• New test_sync_deprecates_all_when_no_classes_seen directly exercises the NOT IN () fallback.
• Existing test_sync_marks_missing_classes_as_deprecated and test_sync_resurrects_class_when_returned_to_list cover the happy path and resurrection.
• One coverage gap that probably isn't worth filling: no explicit single-element test (NOT IN ($1) with one placeholder), but both asyncpg and SQLite handle single-element NOT IN natively, and the existing test_sync_marks_missing_classes_as_deprecated exercises len(seen) = len(REGISTERED_BUILTINS) - 1 = 18 which is functionally equivalent.

Summary

LGTM once dev/OBJECTIVE.md is moved out of tracked space. The core change is correct, well-tested, and an unambiguous improvement over the loop.

[claude]

Review

Overall: clean, targeted refactor. N+1 → 1 round trip is a real win, the empty-seen fallback is the right call, and the SQL is safe. Two workflow nits I'd want addressed before merge, and a couple of small code-level comments.

🟠 Workflow / merge hygiene

1. dev/OBJECTIVE.md should not be in the PR. Per CLAUDE.md, the objective lives at dev/scratch/OBJECTIVE.md (gitignored, per-worktree). The .gitignore already has dev/scratch/. The file currently committed is at dev/OBJECTIVE.md (no scratch/) and will land on main. Please git rm it or move it to dev/scratch/.
2. Missing changelog fragment. Workflow step 7 expects a changelog.d/<short-handle>.md. This is small enough that a one-liner is fine, but it's still required by the convention.

🟢 Correctness

The batched UPDATE is correct:

• $1 binds True; class_refs start at $2 (f"${i + 2}"), and arguments are passed True, *seen_class_refs — placeholder numbering and arg order line up.
• Empty-seen fallback (UPDATE ... WHERE definition_type = 'built-in') matches the old loop's behavior — the prior existing_refs - set() would yield all existing built-in rows. New test test_sync_deprecates_all_when_no_classes_seen locks this in.
• Partial-failure case (some imports succeed, some fail) is preserved: only successful upserts hit seen_class_refs, so failing rows get deprecated correctly (as before).
• Duplicate entries in class_refs would harmlessly produce NOT IN ('x','x',...) — fine.
• db_sqlite._translate_params handles dynamic $N lists generically (regex over the whole query), so the dynamic placeholder string is portable across asyncpg and aiosqlite. Verified in src/luthien_proxy/utils/db_sqlite.py:50.

🟡 Minor code suggestions (non-blocking)

• Fragile placeholder numbering. f"${i + 2}" couples the loop offset to the fact that $1 is deprecated. If someone later adds a parameter before the IN-list, the +2 becomes wrong and the failure mode is silent arg shifting. Cheap mitigations:
  • Put the boolean last and number the IN-list $1..$N: placeholders = ", ".join(f"${i+1}" for i in range(len(seen_class_refs))), then UPDATE ... SET deprecated = ${len+1} WHERE ... NOT IN (...) with args *seen_class_refs, True.
  • Or just hardcode True in the SQL: SET deprecated = TRUE (both asyncpg/Postgres and SQLite accept this), drop the $1 entirely, and number the IN-list from $1. Simplest and removes the offset entirely.
• Comment ordering. The leading comment talks about NOT IN () before the reader sees the SQL. Reads slightly cleaner if the comment is above the if seen_class_refs: guard with one short line, and the empty-case rationale sits inline on the else branch where it's load-bearing. Taste call — current form is fine.

🟢 Performance & security

• Performance: clear improvement (N+1 → 1). With ~19 builtins, far from any SQLite SQLITE_MAX_VARIABLE_NUMBER ceiling.
• Security: no injection surface — placeholder string is built from range(len(...)), all class_ref values are bound as parameters.

🟢 Tests

• Good new test for the empty-seen guard.
• Replacing == 18 with len(REGISTERED_BUILTINS) is a correct fix for the stale assertion (per PR body, the suite was already red because REGISTERED_BUILTINS had grown to 19 after the DeAI preset).
• One small gap: no explicit assertion that the non-empty path is a single round trip. Not worth adding for this PR, but worth noting that the perf claim is currently only validated by inspection.

Note on PR scope

The stale == 18 fix is technically a separate bug fix bundled with the refactor (against "One PR = One Concern" guidance). It's a fair call here — the assertion fix was a precondition to running the test that locks the new behavior, and splitting would have added churn. Calling it out so the COE-skip implication is conscious.

Summary

LGTM after (1) moving dev/OBJECTIVE.md out of the tree (it should be in dev/scratch/, gitignored) and (2) adding the changelog fragment. The placeholder-numbering nit is nice-to-have, not blocking.

[claude]

Code review — PR #793

Overall the core change is solid: N+1 deprecation UPDATEs collapsed into one batched statement, behavior preserved, dynamic placeholder pattern matches what history/service.py and retention/purger.py already do, and the SQLite shim handles $N → ? generically (verified in db_sqlite._translate_params). The test refactor away from a hard-coded == 18 toward len(REGISTERED_BUILTINS) is exactly the right fix for the assertion-drift bug. Nice catch.

A few things to address before merge:

🚩 Blocking

dev/OBJECTIVE.md is committed at the wrong path. Per CLAUDE.md (Objective Workflow), the objective doc must live at dev/scratch/OBJECTIVE.md, which is gitignored and per-worktree. dev/OBJECTIVE.md (root of dev/) is not gitignored — only dev/scratch/ is. This file shouldn't be merged to main. Please move it to dev/scratch/OBJECTIVE.md (or just delete it from this branch — it's only useful within the worktree).

No changelog fragment for the actual change in this PR. The existing changelog.d/policy-types-nits.md on main describes item 1 (acronym regex split). This PR's delivered change — the batched UPDATE — isn't reflected anywhere in changelog.d/. Per CLAUDE.md workflow step 7, please add a fragment (category likely Refactors or Fixes, e.g. changelog.d/polish-batch-policy-type-deprecation.md).

🟡 Nits (non-blocking)

Placeholder offset deserves a one-line comment. The other dynamic-IN sites in the repo (history/service.py:640, retention/purger.py:115, retention/archiver.py:293) all use f"${i + 1}" because they bind nothing else. Here it's f"${i + 2}" because $1 is the deprecated = True parameter. A + 2 with no context is the kind of thing a future reader will stare at for 30 seconds. Either move True to the end of the args list and use i + 1 (matches the convention), or add a short comment:

# $1 is the `True` bind; the IN list starts at $2.
placeholders = ", ".join(f"${i + 2}" for i in range(len(seen_class_refs)))

Adjacent f-strings. Minor: f"..." f"..." reads slightly worse than one f-string. Just f"UPDATE policy_type SET deprecated = $1 WHERE definition_type = 'built-in' AND class_ref NOT IN ({placeholders})" is fine on one line, or use implicit concat without f on the second piece.

✅ Looking good

• Empty-seen guard is the right call and matches the prior loop's behavior. New test_sync_deprecates_all_when_no_classes_seen locks it in correctly (verifies both total == len(REGISTERED_BUILTINS) and still_active == 0).
• seen_class_refs is appended only after a successful upsert — duplicates in class_refs are harmless because NOT IN handles repeated values fine, and the upsert is idempotent via ON CONFLICT (class_ref).
• Both upsert and batched UPDATE share the same conn.transaction(), so the deprecation step sees the freshly-upserted rows and the whole sync is atomic.
• Test assertion refactor (len(REGISTERED_BUILTINS) vs hardcoded 18) is a genuine improvement — it directly prevents the drift that bit you here. Worth a one-line callout in the changelog fragment.

Verification I did

• Traced _translate_params in utils/db_sqlite.py — generic regex over $N, count-agnostic, so the dynamic placeholder list is portable as claimed.
• Checked existing idx_policy_type_builtin_class_ref partial unique index on (class_ref) WHERE definition_type = 'built-in' — matches the upsert's conflict target and the new UPDATE's predicate.
• Walked test_sync_per_class_failure_does_not_break_loop against the new code path: seen_class_refs = [REGISTERED_BUILTINS[0]], batched UPDATE excludes that one row, nothing else exists → 0 rows deprecated, matches the existing assertion.

[jaidhyani]

Autonomous overnight follow-up from the nightly sitrep agent — addressed review feedback in 3179f09. Needs human review; PR stays draft.

Addressed:

• dev/OBJECTIVE.md removed from the diff (blocking item): moved to dev/scratch/OBJECTIVE.md on disk (gitignored, untracked), deleted from the branch.
• Changelog fragment added: changelog.d/polish-batch-policy-type-deprecation.md (category: Refactors, pr: 793). Named distinctly from the existing policy-types-nits.md already on main.
• Placeholder-numbering nit: took the simplest suggested option — hardcoded deprecated = TRUE in the SQL (valid in both Postgres and SQLite) and numbered the NOT IN list from $1 via f"${i + 1}", matching the convention in history/service.py, retention/purger.py, and retention/archiver.py. This removes the i + 2 offset entirely. Also collapses the adjacent-f-string nit's concern (string layout unchanged otherwise).

Deliberately skipped (judgment/taste calls per reviews):

• Comment-ordering suggestion ("taste call — current form is fine" per the review).
• Single-element NOT IN test and explicit single-round-trip assertion (both reviews said not worth adding).
• One PR = One Concern note about the == 18 fix being bundled — scope decision for a human, leaving as-is.

Validation: uv run pytest tests/luthien_proxy/integration_tests/test_policy_type_sync.py -m sqlite_e2e — 10 passed. ruff check and ruff format --check clean on the edited file.

[claude]

Review

Small, well-scoped change. The N+1 → 1 optimization is correct, the empty-seen edge case is properly handled with a dedicated test, and the changelog is in order. A few observations below.

Correctness — minor behavioral improvement (worth noting)

The rewrite isn't a strict 1:1 of the old logic — it's slightly more correct in one subtle way. The old loop ran:

UPDATE policy_type SET deprecated = $1 WHERE class_ref = $2

with no definition_type filter. The partial unique index on class_ref is scoped to definition_type = 'built-in', so a non-built-in row with the same class_ref is allowed to coexist. The old code could have silently marked such a row deprecated. The new statement scopes the UPDATE to definition_type = 'built-in', so this can't happen. Worth a one-liner in the PR body or commit message — it's a quiet correctness win, not just a perf change.

Code quality

• src/luthien_proxy/policy_types.py:177 — first piece of the implicitly-concatenated SQL is an f-string with no interpolation:

  f"UPDATE policy_type SET deprecated = TRUE "
  f"WHERE definition_type = 'built-in' AND class_ref NOT IN ({placeholders})",

  The leading f on line 177 is unnecessary (only line 178 interpolates). Stylistic — drop it for clarity, or leave it for consistency. Not a blocker.

• The comment block at policy_types.py:171-173 explains the why of the if seen_class_refs branch (empty NOT IN () is invalid SQL) — exactly the kind of non-obvious WHY-comment CLAUDE.md asks for.

Security

• No injection risk: placeholders is built from range(len(seen_class_refs)) (integers only). seen_class_refs itself is bound through parameters, not interpolated.
• Class refs are sourced from the hardcoded REGISTERED_BUILTINS tuple or test-controlled input; no user data path.

Performance

• Trivially better. With ~19 builtins it's not load-bearing today, but the prior pattern would have grown linearly. SQLite's default SQLITE_LIMIT_VARIABLE_NUMBER is 999 (older) / 32766 (newer), so headroom is enormous.
• Whole sync still runs under a single transaction (conn.transaction() at policy_types.py:132), so atomicity is preserved.

Test coverage

• test_sync_deprecates_all_when_no_classes_seen is the right test for this change: it seeds with the full list, then forces every class_ref to fail import so seen_class_refs is empty, then asserts every built-in row is deprecated. This is exactly the edge case the if seen_class_refs: branch exists for.
• Removing the hardcoded == 18 in favor of len(REGISTERED_BUILTINS) is a good incidental cleanup — the previous drift (registry grew to 19 after the DeAI preset, assertion stayed at 18) is the exact failure mode this prevents.
• Pre-existing test test_sync_per_class_failure_does_not_break_loop already covers the partial-failure case; combined with the new test, both empty and partial-empty paths are locked in.

Nit on the changelog

changelog.d/polish-batch-policy-type-deprecation.md:7 is a sub-bullet under the main entry. Render-wise it's fine, but the test-assertion fix is arguably a separate concern from the deprecation batching (per CLAUDE.md's "One PR = One Concern" guidance). Not worth splitting at this point — the PR body already calls it out as an "incidental fix required for validation" — but worth keeping in mind for similar future polish PRs.

Summary

LGTM. The behavioral improvement around definition_type scoping is worth mentioning in the PR description for reviewers; everything else is solid.

[scottwofford]

Claude-generated merge-queue triage of all open Luthien PRs, requested by Scott (Jul 7, 2026). Advisory only; Scott has not yet acted on these recommendations.

Recommendation: mark ready and merge (the code diff is roughly 20 lines).

Still doubly relevant on main: the per-row deprecation UPDATE loop is still there, and the test suite still asserts 18 built-ins while REGISTERED_BUILTINS has 19, so the (default-deselected) sqlite_e2e sync suite is currently red on main; this PR fixes both. All blocking review feedback was addressed on June 10 and the final automated review is a clean pass. The SQL is parameterized and the empty-list edge case is tested.