[paynejd]

OpenConceptLab/ocl_online#620 and OpenConceptLab/ocl_online#532, with the public metering parts of OpenConceptLab/ocl_online#352.

What this does

CAPABILITY_POLICY names the class that decides and records the use of capabilities, as EMAIL_BACKEND does for mail. CapabilityPolicy is its abstract base, and get_capability_policy() beside it returns one. The default, GroupLimitPolicy, is the per-user override and group limit logic that lived on UserProfile, moved unchanged. UserProfile.get_capability_limit, get_capability_usage and check_and_consume_capability now delegate to it, so their callers don't change.

• System check. core/capabilities/checks.py fails migrate and manage.py check on a CAPABILITY_POLICY that doesn't import (capabilities.E001), isn't a subclass (E002), leaves a method unimplemented (E003), or has a method that can't take the arguments it's called with (E004). It reads signatures and doesn't make the policy. A process that starts without running either command doesn't run the check.
• One policy per operation. The consume and refund routes, $match and its reversal, and a list of capabilities each use one policy instance, so a policy that keeps a snapshot per instance stays coherent within a response.
• Usage windows. UsageCounter and UsageEvent get window_start. null is the one window that never ends; every existing row and the default policy use it. A subclass of GroupLimitPolicy can count in windows by overriding get_window_start(). A counter is unique per (user, capability, window), with a second, partial constraint because PostgreSQL 14 treats nulls as distinct. Idempotency keys stay unique per (user, capability) across windows.
• One lock per account. consume() locks the user's row before the counter, so a user's requests serialize even when they count in different windows.
• Reversal. A failed $match reverses its own usage event (UsageEvent.reverse), which lowers the counter that event counted in and records reversed_units. It never takes back more than the event recorded. The staff refund (POST /capabilities/refund/) stays a counter correction, made by the policy's refund(), in its current window.
• Refusals. The policy raises UsageRefused, whose detail is the 403 body. $match and POST /capabilities/consume/ send it as built, adding only their own sentence as detail when the policy gave none.
• Owner. get_limit, get_usage, consume and describe, and UserProfile's three methods, take an optional owner: the user or organization whose content a use is for. The default policy ignores it. No call site passes one yet, and no request can set one. Windows, counters, events and reversals stay per user. A policy that decides by owner first checks that user may act for it.
• Usage reports. GET /user/?includeCapabilities=true and the consume and refund responses send every field the policy's get_usage() returns.
• Loader. OCL_EXTRA_APPS and OCL_EXTRA_URLCONFS, comma-separated dotted paths from the environment, empty by default, add Django apps and URLconfs to a build. Extra URLconfs are included after core's own routes, so those keep precedence.
• used, units and reversed_units are BigIntegerField.

Behaviour under the default

No API response changes: the same limits, the same 403 bodies (detail, error_code, limit, used, with the same sentences and codes), and the same used. The migration gives existing rows window_start = null and keeps every used. The differences are the extra row lock in consume(), reversed_units recorded when a $match search fails, and the Swagger schema typing the capability limit and used as integers.

Tests

• The full existing suite passes. The two $match integration tests that patched the moved methods now patch GroupLimitPolicy, with every response assertion kept, and the users test that patched get_capability_usage patches GroupLimitPolicy.get_usage.
• CapabilityPolicyContractTest runs a fake policy, set through CAPABILITY_POLICY, through every call site: the UserProfile delegates, the user's capabilities, consume (success and refusal), $match (refusal, a failed search that's reversed, a search that isn't), and the staff refund, which leaves the counter to the policy. It also checks that each operation or response makes one policy.
• CapabilityPolicyInterfaceTest, CapabilityPolicyCheckTest and CallSitesFitTheCheckTest: a policy missing a method can't be made, each of the check's errors, signatures that do and don't fit, and a policy the check passes can be called.
• GroupLimitPolicyWindowTest: each window counts from 0, a key replays in a later window, a reversal lowers the window its units counted in, and a reversal never takes back more than the event or the counter holds.
• GroupLimitPolicyDefaultWindowTest: the default counts in the window that never ends; consume() locks the user row FOR NO KEY UPDATE; an event inserted without the new columns, as the previous release does while a deploy migrates, gets their defaults.
• ExtraURLConfsTest: an extra URLconf is included after core's routes.
• GroupLimitPolicyOwnerTest: under the default, an owner changes no limit, usage or charge. ReplacedPolicyOwnerTest: a policy that replaces get_limit gets the owner, and an inherited consume() enforces that limit. The contract test checks the owner reaches the policy, and that a consume request can't name one.

Run locally in an isolated compose stack on f4fa8c5, as CI runs them (TEST_ES=off, --exclude-tag es --parallel=4 --keepdb, PYTHONHASHSEED=2): 3,048 tests OK. pylint -j0 core/ (pylint 4.0.9): no messages. PR CI runs the ES-tagged tests as well.

Review

Reviewed before opening, with Codex (codex-cli 0.164.0, xhigh) and two fresh Claude reviews. Each Codex pass from before the owner argument is posted below.

• Codex pass 1 (03e82de): one P2. The user-row FOR UPDATE could deadlock a first consume against a staff refund creating the same counter. Fixed in 3765046 (FOR NO KEY UPDATE).
• Codex pass 2 (3765046): clean.
• Simplification review: one cut, CapabilityExceeded's unread attributes, applied in 777c70a. Codex check of that commit: clean.
• Adversarial review: one P1. reversed_units lost its database default after the migration, so the previous release would fail every usage event insert during a rolling deploy. Fixed in d7cff42 (db_default=0). Codex check of that commit: clean. One P2 was accepted: the int-to-bigint rewrite locks two small tables for milliseconds. Six P3s were recorded, not fixed: a rare deadlock between a consume and deleting that same user (on master the same race ends in a 500), and contract edges for other policies and extra URLconfs.

After master moved on and the owner argument went in (each pass posted below as a summary, in full on OpenConceptLab/ocl_online#620):

• Codex pass 1 (e9a93d7): clean.
• Simplification review: one cut applied (two describe() assertions the contract test already covers), one declined.
• Adversarial review: no P0 or P1. Two P2s fixed in 529573e: what the owner docstring claimed, and a test policy that dropped blocked and unlimited values. Two P2s accepted: an inherited consume() counts on the acting user's counter, and a rollback past this PR has to delete windowed counters first. Eight P3s recorded.
• Codex check of e9a93d7..529573e: clean.

After the first review (in full on OpenConceptLab/ocl_online#620):

• Changes requested: the abstract base with its check, the staff correction through the policy, and the loader beside the policy are all in. Moving the default policy's logic out of this repo is OpenConceptLab/ocl_online#630, in the order agreed on OpenConceptLab/ocl_online#620.
• Codex check of 529573e..3673494: clean.
• Adversarial review: no P0. Three P1s: one fixed in c6ae07b (the check read only method names), and two on code outside this repo, handled there. Four P2s: one fixed in c6ae07b (abstract methods raise again), one fixed outside this repo, one accepted (processes that skip migrate don't run the check), and one recorded (deploy jobs don't wait for service stability, which predates this PR). Four P3s recorded.
• Codex check of 3673494..c6ae07b: one P2, fixed in f3e75aa (a call site passed units by position). Codex check of that commit: clean.
• Second look: one policy instance for each operation or response, in f4fa8c5. Codex check of f3e75aa..f4fa8c5: clean.
• Approved at f4fa8c5.

The branch brought in master by merging, not rebasing. It merged as squash 42bd393, whose tree is f4fa8c5 merged with master at 4423361.

🤖 Generated with Claude Code

https://claude.ai/code/session_01AaZ4qTgak472qGyADUGEGq

[paynejd]

Codex review, pass 1 (codex-cli 0.164.0, commit 03e82de)

Verdict: 1 finding(s).

• F1.1 P2 core/capabilities/policy.py:119: The new user-row lock can deadlock a first consume against a staff refund.

findings.json

{
  "reviewer": "codex-cli 0.164.0",
  "perspective": "defects",
  "mode": "full",
  "commit": "03e82de8",
  "verdict": "findings",
  "prior": [],
  "findings": [
    {
      "id": "F1.1",
      "severity": "P2",
      "tag": null,
      "file": "core/capabilities/policy.py",
      "line": 119,
      "claim": "The new user-row lock can deadlock a first consume against a staff refund.",
      "scenario": "When the current window has no counter, a concurrent staff refund can insert it before consume locks the user. Consume then waits on the refund's uncommitted unique entry; the refund's deferred user foreign-key check waits on consume's FOR UPDATE lock. PostgreSQL aborts one transaction, producing an uncaught 500. This follows from static lock-order analysis using PostgreSQL's [uniqueness checks](https://www.postgresql.org/docs/14/index-unique-checks.html) and [foreign-key locking](https://github.com/postgres/postgres/blob/REL_14_STABLE/src/backend/utils/adt/ri_triggers.c).",
      "action": "Use select_for_update(no_key=True) for the user lock, which still serializes consumes while permitting foreign-key checks under PostgreSQL's [lock compatibility rules](https://www.postgresql.org/docs/14/explicit-locking.html#LOCKING-ROWS), or make refund acquire the user lock first. Add a PostgreSQL concurrency regression test for an absent counter.",
      "lines_delta": null,
      "confidence": "high"
    }
  ]
}

Response: F1.1 fixed in 3765046. consume() locks the user row FOR NO KEY UPDATE, which still serializes a user's consumes and lets the foreign-key check through. Pinned by test_consume_locks_the_account_without_blocking_foreign_key_checks.

[paynejd]

Codex review, pass 2 (codex-cli 0.164.0, commit 3765046)

Verdict: clean. Earlier findings: F1.1 resolved.

findings.json

{
  "reviewer": "codex-cli 0.164.0",
  "perspective": "defects",
  "mode": "verify",
  "commit": "3765046e",
  "verdict": "clean",
  "prior": [
    {
      "id": "F1.1",
      "status": "resolved"
    }
  ],
  "findings": []
}

[paynejd]

Codex check of commits 3765046..777c70a (the simplification cut)

Verdict: clean.

findings.json

{
  "reviewer": "codex-cli 0.164.0",
  "perspective": "defects",
  "mode": "cuts",
  "commit": "777c70a2",
  "verdict": "clean",
  "prior": [],
  "findings": []
}

[paynejd]

Codex check of commits 777c70a..d7cff42 (the adversarial review's P1 fix)

Verdict: clean.

findings.json

{
  "reviewer": "codex-cli 0.164.0",
  "perspective": "defects",
  "mode": "cuts",
  "commit": "d7cff42d",
  "verdict": "clean",
  "prior": [],
  "findings": []
}

[paynejd]

Codex review, pass 1 (codex-cli 0.166.0, commit e9a93d7): clean. The full pass is on OpenConceptLab/ocl_online#620.

[paynejd]

Simplification and adversarial reviews (commit e9a93d7). The full record is on OpenConceptLab/ocl_online#620.

• Simplification: one cut applied (two describe() assertions the contract test already covers), one declined.
• Adversarial: no P0 or P1. Of four P2s, two are fixed in 529573e: what the owner docstring claimed, and a test policy that dropped blocked and unlimited values. Two are accepted: an inherited consume() counts on the acting user's counter, and a rollback past this PR has to delete windowed counters first. Eight P3s are recorded.
• Also in 529573e: UsageRefused's flag is now no_access. Error codes and response bodies don't change.

The branch brings in master with a merge, not a rebase. A squash merge gives the same result.

[paynejd]

Codex check of commits e9a93d7..529573e (codex-cli 0.166.0): clean. The full pass is on OpenConceptLab/ocl_online#620.

[snyaggarwal]

Verdict: Request changes

Reviewed together with OpenConceptLab/ocl_online_api#41, focusing on object ownership, existing patterns, readability, module placement, and single sources of truth.

• P1: the new public default still computes and enforces deployment-specific access/limit decisions. The public side should expose neutral metering/checkpoints and remain fully usable by itself; the replacement layer should supply finished decisions.
• P2: the policy is presented as the owner of usage recording, but the staff correction path still reaches through it into UsageCounter.
• P2: CapabilityPolicy is only a convention, so the paired PR has to duplicate its required-method list to validate implementations.
• P3: the policy loader is placed in the catch-all common utilities module instead of beside the policy abstraction.

No additional nits beyond the inline comments.

[snyaggarwal]

[P2] Make the policy contract enforceable where it is defined. Using ABC plus abstractmethod (and a public Django system check/loader beside this class) would let Python validate implementations. As written, missing overrides fail only at request time unless the private extension happens to be installed, and the paired PR has to maintain a second REQUIRED_METHODS list that can drift from this class.

[snyaggarwal]

[P2] Route corrections through the policy abstraction. CapabilityPolicy says it decides and records usage, but this view asks it for a window and then mutates UsageCounter directly. Any replacement that stores usage differently, applies correction rules, or does not use UsageCounter cannot implement the advertised interface. Add a policy method for this staff correction and let GroupLimitPolicy own the UsageCounter call.

[snyaggarwal]

[P3] Keep the loader with the abstraction it constructs. get_capability_policy is capability-domain wiring, not a general utility; putting it in core.capabilities.policy (or a small core.capabilities.loader module) makes the dependency discoverable and avoids growing common.utils as a service locator.

[snyaggarwal]

Second-pass verdict: Request changes

I read all new PR replies and the full updated discussion on OpenConceptLab/ocl_online#620 and #630. The prior ABC/check, refund ownership, and loader-placement findings are resolved. Per the explicit decision on #620, removal of GroupLimitPolicy is deferred to #630 and is not re-raised here.

One P2 remains: a single consume response uses two independently constructed policy objects, and capability-list serialization constructs another object per item. That defeats the instance lifecycle provided by the abstraction and can mix snapshots in one response for a stateful replacement policy. Reuse one policy instance for each logical operation/response.

Current CI is green: Pylint, Tests, and Snyk pass.

[snyaggarwal]

[P2] Reuse the policy instance that performed the consume when describing its result. request.user.check_and_consume_capability() constructs a policy internally, then this line constructs a second one; a stateful replacement that caches a consistent snapshot per instance can therefore record against one snapshot and report another in the same response. Instantiate once in this view and call policy.consume(...) followed by policy.describe(...). Apply the same lifecycle to CapabilitySerializer, which currently constructs one policy per capability, so a list response is one coherent snapshot and avoids repeated setup/queries.

[snyaggarwal]

Third-pass verdict: Approve — recommend merge.

The remaining policy-instance P2 is resolved across consume responses, $match/reversal, refunds, and capability-list serialization, with focused regression tests. The prior ABC/check, policy-owned refund, and loader-placement findings remain resolved. The temporary public default is intentionally deferred to OpenConceptLab/ocl_online#630 per the agreed deployment order.

Current Pylint, Tests, and Snyk checks pass.

[snyaggarwal]

Third-pass verdict: Approve — recommend merge.

The remaining policy-instance P2 is resolved across consume responses, $match/reversal, refunds, and capability-list serialization, with focused regression tests. The prior ABC/check, policy-owned refund, and loader-placement findings remain resolved. The temporary public default is intentionally deferred to OpenConceptLab/ocl_online#630 per the agreed deployment order.

Current Pylint, Tests, and Snyk checks pass.

[paynejd]

The changes made in response to the reviews, approved at f4fa8c5 and merged as squash 42bd393. Each pass is in full on OpenConceptLab/ocl_online#620.

Changed

• CapabilityPolicy is an ABC. core/capabilities/checks.py fails migrate and manage.py check on a CAPABILITY_POLICY that doesn't import (capabilities.E001), isn't a subclass (E002), leaves a method unimplemented (E003), or has a method that can't take the arguments it's called with (E004). A process that starts without running either command doesn't run the check.
• POST /capabilities/refund/ goes through a new CapabilityPolicy.refund(), and GroupLimitPolicy makes the UsageCounter call. The route's response and effect don't change.
• get_capability_policy moved from core/common/utils.py to core/capabilities/policy.py.
• One policy instance for each operation or response: the consume route, $match and its reversal, and a list of capabilities each use one, so a policy that keeps a snapshot per instance stays coherent. No response changes under the default.
• get_window_start moved to GroupLimitPolicy, its only reader. The refusal hook nothing overrides is gone, and UserProfile passes units to the policy by name.

Not changed here: moving the default policy's logic out of this repo is OpenConceptLab/ocl_online#630, in the order agreed on OpenConceptLab/ocl_online#620.

Tests: pylint -j0 core/: no messages. Non-ES suite locally at f4fa8c5: 3,048 OK. PR CI on f4fa8c5: passes (Pylint, Tests including the ES-tagged run, Snyk).

Review

• Codex check of 529573e..3673494: clean.
• Adversarial review: no P0. Three P1s: one fixed here (the check read only method names), and two on code outside this repo, handled there. Four P2s: one fixed here (abstract methods raise again), one fixed outside this repo, one accepted (processes that skip migrate don't run the check) and one recorded (deploy jobs don't wait for service stability, which predates this PR). Four P3s recorded.
• Codex check of 3673494..c6ae07b: one P2, fixed in f3e75aa.
• Codex check of c6ae07b..f3e75aa: clean.
• Codex check of f3e75aa..f4fa8c5: clean.