⚠️ GLM review did not complete

This run did not finish, so there is no verdict for this commit.
That is not the same as a clean bill of health — a review that ran
and found nothing says so explicitly ("✅ GLM review: no defects
found.").

Reason: the run was cancelled before the agent finished — usually a newer push superseding it, since this workflow cancels in-progress runs for the same pull request. Nothing is wrong with the review itself: the newest commit is reviewed by its own run, which posts its verdict as a separate comment. This notice is not replaced by that run and can be disregarded once it appears.

View job run — the agent transcript is attached there as a
glm-review-transcript-* artifact when the agent got far enough to
write one.

What this comment held when the run stopped

Precision review in progress

Bug-hunting review of PR #1303 (logic errors, security, data loss, races only — style/coverage handled elsewhere).

• Gather PR diff and existing review comments
• Read core/spend_limit.py + callers (runtime, conductor, tasks_v2, batches_v2)
• Read credential/scope changes (ui/dependencies.py, settings_v2, github_integrations_v2)
• Read auth user-create, schema change, cost_tracker/react_agent/agent clamp math
• Verify each suspicious hunk against surrounding code and callers
• Post confirmed findings + summary

View job run

Cross-family review — codex (post-PR pass at 284b982)

The new daily limit can be bypassed through delegated engines, and reservations do not respect the promised UTC-day reset. Both issues undermine the limit's execution behavior.

Full review comments:

• [P2] Refuse unmetered engines when enforcing a daily ceiling — /home/frankbria/projects/codeframe/codeframe/core/runtime.py:912-918
  When a principal selects an external engine such as claude-code or codex, the daily budget is reserved but this clamp is only passed to built-in adapters. The external-engine branch neither enforces the ceiling nor records its spending in token_usage, so repeated delegated runs can spend indefinitely without exhausting the configured daily limit. Reject unmetered execution when this operator limit applies, unless the adapter can enforce and account for it.
• [P2] Track the UTC day associated with budget reservations — /home/frankbria/projects/codeframe/codeframe/core/spend_limit.py:97-99
  If a run spans midnight UTC, its previous-day reservation remains in _held and is subtracted from the new day's budget. A single run reserves the entire remainder by default, so a run started before midnight can prevent all subsequent starts after midnight even when today's recorded spending is zero, contradicting the advertised daily reset. Make reservations day-aware and reconcile active runs' budgets at rollover rather than retaining an undated hold until completion.

Earlier pre-PR rounds (codex r1: 2 findings, r2: 2 findings) and the internal review are triaged in the PR body; all were fixed. These two are being fixed now.

Triage of the codex post-PR pass:

Both are covered by tests that fail under mutation.

⚠️ GLM review did not complete

This run did not finish, so there is no verdict for this commit.
That is not the same as a clean bill of health — a review that ran
and found nothing says so explicitly ("✅ GLM review: no defects
found.").

Reason: the run was cancelled before the agent finished — usually a newer push superseding it, since this workflow cancels in-progress runs for the same pull request. Nothing is wrong with the review itself: the newest commit is reviewed by its own run, which posts its verdict as a separate comment. This notice is not replaced by that run and can be disregarded once it appears.

View job run — the agent transcript is attached there as a
glm-review-transcript-* artifact when the agent got far enough to
write one.

What this comment held when the run stopped

Precision review in progress

Bug-hunting review of PR #1343 (logic errors, security, data loss, races only — style/coverage handled elsewhere).

• Gather PR diff and existing review comments
• Read core/spend_limit.py + callers (runtime, conductor, tasks_v2, batches_v2)
• Read credential/scope changes (ui/dependencies.py, settings_v2, github_integrations_v2)
• Read auth user-create, schema change, cost_tracker/react_agent/agent clamp math
• Verify each suspicious hunk against surrounding code and callers
• Post confirmed findings + summary

View job run

Claude finished @frankbria's task in 12m 57s —— View job

GLM precision review — PR #1343 @ eaa4913

Bug-hunting pass over the full diff (logic errors, security, data loss, races — style/coverage excluded). Every suspicious hunk was verified against the surrounding code and its callers in the checked-out repo; earlier codex/internal findings and their fixes were re-checked and not repeated.

• Gather PR diff and existing review comments
• Read core/spend_limit.py + callers (runtime, conductor, tasks_v2, batches_v2)
• Read credential/scope changes (ui/dependencies.py, settings_v2, github_integrations_v2)
• Read auth user-create, schema change, cost_tracker/react_agent/agent clamp math
• Verify each suspicious hunk against surrounding code and callers
• Post confirmed findings + summary

2 findings, both minor (posted as inline review comments):

Verified sound (checked, no defect): the 429 passes through every route's broad except (except HTTPException: raise / check placed outside try); the clamp math min(workspace cap, prior + ceiling) correctly offsets prior task spend so resumes aren't double-counted, in both engines and both resolve sites; holds are day-tagged so midnight UTC resets; release on the 404 path and in the worker's finally; the per-task scope re-read and the unreadable-ledger fail-closed (429, sqlite3.Error-scoped); all six conductor _execute_task_subprocess call sites (incl. retries and supervisor re-runs) funnel through the reserving wrapper, and unmetered engines are refused pre-reserve; workspace_spend_users matches the #898 backfill predicate exactly (deactivated/placeholder admins counted the same on both sides); get_costs_summary(1) anchors to UTC midnight, matching the documented reset; store_key/delete_key write only the caller's per-user store with the GitHub PAT still admin-gated in both routers (migration is admin-conditional inside the shared dependency, so dependency-vs-scope ordering no longer matters); user-create INSERT matches the users schema, normalizes case, and the login test runs through the real router.

Demo (eaa4913): real codeframe serve, auth ON, self-hosted

Script: tasks/demo_1303.py and tasks/demo_1303_clamp.py. Fake keys only; the LLM is the mock provider.

Full transcript: tasks/demo-evidence-1303.txt (local; tasks/ is gitignored).

Review: #1343 (hosted-launch prerequisites)

Went through the full diff (spend_limit.py, cost_tracker/agent/react_agent/runtime/conductor wiring, the shared credential-manager dependency, the settings/github scope changes, cf auth user-create, and the new tests). This is a large, carefully-built change — the fail-closed posture on unreadable spend ledgers, the hold/release lifecycle to stop concurrent runs from each claiming the full remainder, the one-shared-dependency fix for the GitHub-router credential-migration bug, and the write-once ownership semantics are all sound, and the test suite (test_spend_limit_1303.py, test_spend_limit_routes_1303.py, test_tenant_owned_keys_1303.py, test_user_create_1303.py) is unusually thorough — it exercises exactly the races and edge cases (resume not double-refused, workspace-switch not resetting the meter, failed start releasing its hold, unreadable ledger failing closed) that a review would otherwise have to point out.

A few things worth a look:

1. Parallel/auto batch share is sized by configured max_parallel, not actual concurrency (Suggestion)

conductor.execute_batch sets share = batch.max_parallel if batch.strategy != "serial" else 1, and each task's hold is remaining / share (_execute_task_subprocess → reserve_today_usd(..., share=spend[1])).

This is sized off the configured max_parallel, not the number of tasks actually running concurrently. Two concrete cases where that under-allocates:

• A "parallel" batch with only 1-2 tasks but max_parallel=4 (the default) — each task only gets 1/4 of the day's remaining budget even though at most 1-2 tasks ever run at once.
• An "auto" batch where the inferred dependency graph is a mostly-linear chain — _execute_parallel still applies, so share stays pinned to max_parallel even though real concurrency is 1.

Net effect: a task can be artificially clamped to a tiny cost_ceiling_usd (and hit CostCapExceeded mid-run) despite the user having ample daily budget left. It's tested as current behavior (test_parallel_batch_child_gets_its_slots_share asserts exactly this for a 1-task batch), so it reads as a deliberate simplification rather than an oversight — but unlike the other approximations in this PR (workspace-not-caller attribution, per-process holds), it isn't called out in the "Known Limitations" list. Worth either documenting it alongside those, or sizing share by min(max_parallel, len(task_ids)) (or the actual in-flight count) so small/serialized batches aren't needlessly throttled.

2. Global lock serializes spend I/O across every user (Performance, minor)

reserve_today_usd/remaining_today_usd hold the single process-wide _held_lock for the full duration of _free_today, which includes spend_today_usd's synchronous SQLite reads across every workspace in scope. Since this isn't keyed per-user, every task start / batch task dispatch across every tenant now serializes on one lock plus disk I/O — in the hosted multi-tenant scenario this PR exists to enable, that's the one place where concurrent tenants starting work will contend with each other. Given "holds are per-process" is already a called-out limitation for a different reason, this is likely acceptable for the single-worker target, but moving the DB read outside the lock (compute spent first, then take the lock just to update _held) would remove the unnecessary cross-tenant serialization at effectively no cost.

3. _require_admin_unless_llm_key gates by exclusion, not inclusion (nit)

_require_admin_unless_llm_key (settings_v2.py) treats "anything that isn't GIT_GITHUB" as a tenant-owned LLM key. That's safe today only because _PROVIDER_MAP/KEY_PROVIDERS happen to restrict the reachable providers to LLM_ANTHROPIC / LLM_OPENAI / GIT_GITHUB — the CredentialProvider enum also has GIT_GITLAB, CICD_GENERIC, and DATABASE, which would silently fall through to write-scope-only if ever added to _PROVIDER_MAP without someone remembering to update this check too. Not exploitable as-is, but an inclusion check (cp in {LLM_ANTHROPIC, LLM_OPENAI}) would make the invariant self-evident instead of depending on a second list staying in sync.

Everything else checked out fine

• resolve_cost_cap's prior-spend offset math (budget = prior_usd + ceiling_usd, then min(cap, budget)) is correct for the "cap only", "ceiling only", and "both" cases, and both engines (ReactAgent, Agent) reach it the same way.
• The write-before-check ordering is right everywhere traced: approve_tasks_endpoint, start_execution, and resume_batch all call check_spend_limit before any batch/approval state is persisted; start_single_task/resume_task release their hold if start_task_run/resume_run itself fails.
• The GitHub-router credential-manager bug this PR fixes is actually fixed: get_credential_manager's migrate flag is now derived from has_scope(auth, SCOPE_ADMIN) directly, so it's correct regardless of FastAPI's dependency-resolution order relative to the route's own require_scope(SCOPE_ADMIN) check.
• token_usage is created unconditionally at workspace-init time, so the "unreadable ledger fails closed" path in spend_today_usd won't false-positive on a brand-new, never-run workspace (checked this because it looked like a plausible trap, but the table exists before any run).

Nothing here blocks merging — #1 and #2 are usability/scale considerations for the hosted-launch use case this PR is explicitly targeting, and #3 is a maintainability note.

Review (feat/issue-1303)

Scope is large but well-factored. I focused on the new spend_limit module, its wiring through conductor/runtime/tasks_v2/batches_v2, and the credential-scope changes in settings_v2/ui/dependencies.py, since those are the security/correctness-sensitive parts. The codex cross-family review already caught and fixed the two most dangerous issues (delegated-engine bypass, holds surviving UTC midnight) — I verified both fixes land as described (cost_tracker.covers_engine gate in _execute_task_subprocess, and the day-tagged _held entries in spend_limit.py), so I won't re-litigate those.

Finding: inverted allowlist in _require_admin_unless_llm_key (latent, not currently exploitable)

codeframe/ui/routers/settings_v2.py:233-240:

def _require_admin_unless_llm_key(auth: dict, cp: CredentialProvider) -> None:
    """Only LLM keys are tenant-owned; anything else (GitHub PAT) needs admin (#717, #1303)."""
    if cp is not CredentialProvider.GIT_GITHUB or has_scope(auth, SCOPE_ADMIN):
        return
    raise HTTPException(...)

The docstring states the intended rule as an allowlist: "only LLM keys are tenant-owned, everything else needs admin." The code instead implements a denylist: it only gates GIT_GITHUB specifically, falling through for every other non-LLM CredentialProvider (GIT_GITLAB, CICD_GENERIC, DATABASE).

This is currently harmless because KEY_PROVIDERS in ui/models.py only exposes ("LLM_ANTHROPIC", "LLM_OPENAI", "GIT_GITHUB") through _resolve_provider, so those other enum members can't reach this function via the /settings/keys/{provider} route today. But the check is written backwards relative to its own stated intent — the moment someone adds a fourth provider to KEY_PROVIDERS (e.g. a GitLab PAT, following the same pattern as GitHub), it would silently become writable by any write-scoped tenant instead of requiring admin, because the new provider isn't GIT_GITHUB. That's a fail-open trap of exactly the shape this codebase has repeatedly flagged elsewhere (CLAUDE.md's running theme: a check that doesn't match its own stated invariant re-creates the bug it meant to prevent.

Suggest inverting it to match the docstring, e.g.:

_TENANT_OWNED_PROVIDERS = {CredentialProvider.LLM_ANTHROPIC, CredentialProvider.LLM_OPENAI}

def _require_admin_unless_llm_key(auth: dict, cp: CredentialProvider) -> None:
    if cp in _TENANT_OWNED_PROVIDERS or has_scope(auth, SCOPE_ADMIN):
        return
    raise HTTPException(...)

or check cp.name.startswith("LLM_"). Either way, the next provider added should default to admin-only without a code change here.

Everything else I checked looked solid

• The reserve/release lifecycle in tasks_v2.py (start_single_task/resume_task) correctly releases the hold on a failed runtime.start_task_run/resume_run and in the worker's finally, matching the new tests (test_a_failed_start_releases_its_hold, test_the_worker_releases_its_hold_when_the_run_ends).
• get_credential_manager's migrate=has_scope(auth, SCOPE_ADMIN) correctly makes the migration decision independent of FastAPI dependency evaluation order, which is what actually fixes the "GitHub router copied operator keys before its admin check" bug described in the PR body — not just the dependency consolidation itself.
• resolve_cost_cap's prior-spend offset (budget = prior_usd + ceiling_usd) is consistent across both agent.py (plan engine) and react_agent.py, and the cap computed at Agent.__init__/ReactAgent.__init__ (prior=0) vs. recomputed in _load_prior_cost/run() is coherent — the cap_usd is not None init-time check correctly gates whether the later recompute happens.
• workspace_spend_users UNION with owner_user_id in spend_paths() correctly handles the "moved to an unowned-but-used workspace" case called out in the PR body.
• Schema addition is a lazy CREATE TABLE IF NOT EXISTS, consistent with repo convention.

Nothing else rose to the level of a bug in the parts I covered. Nice test coverage on the concurrency/midnight-rollover edge cases in test_spend_limit_1303.py.

Review — PR #1343 (hosted-launch prerequisites)

I read through spend_limit.py, its callers in runtime.py/conductor.py/tasks_v2.py/batches_v2.py, the credential-dependency consolidation in ui/dependencies.py, settings_v2.py/github_integrations_v2.py, the schema/registry change, and cf auth user-create. The codex rounds already fixed real issues (midnight-UTC holds, delegated-engine bypass, the admin/migration-order leak); this pass is looking for anything that survived those.

Confirmed finding: parallel-batch share division compounds instead of splitting evenly

conductor.execute_batch fixes share = batch.max_parallel once per batch for non-serial strategies and stores it in _batch_spend_scope. Every task in the batch then reserves via the same share:

```python

codeframe/core/spend_limit.py

def reserve_today_usd(user_id, repo_paths, share=1):
with _held_lock:
free = _free_today(user_id, repo_paths) # already subtracts existing holds
amount = free / max(share, 1) # divides AGAIN by the fixed share
...
```

_free_today recomputes limit - spent - held fresh on every call, so each successive reservation in the same parallel wave divides an already-shrunk remainder by share again, instead of each slot getting original_remainder / share. For a 4-way parallel batch with a $10 remainder (share=4 for every call):

• slot 1: free=10 -> holds 2.50
• slot 2: free=7.50 -> holds 1.875
• slot 3: free=5.625 -> holds 1.40625
• slot 4: free=4.21875 -> holds 1.0546875

Total held is about $6.84, not $10, and the 4th sibling gets about 10.5% of the daily remainder instead of the intended 25% — more than 2x smaller than slot 1, purely as a function of thread-scheduling order inside _execute_group_parallel's thread pool. This isn't an overspend/security issue (the system ends up more conservative, not less), but it breaks the documented guarantee ("a parallel batch splits the remainder across its slots") and makes a parallel sibling's effective budget nondeterministic and spawn-order-dependent — it can under-fund and spuriously cap later-starting siblings in the same wave for no real budget reason.

This looks like it slipped through: test_parallel_batch_child_gets_its_slots_share (tests/core/test_spend_limit_1303.py) only ever creates one task in the batch (_run_batch calls tasks.create(...) once), so it only exercises the first reservation in a wave and can't observe the compounding across multiple concurrent slots. test_second_concurrent_run_gets_only_what_is_left does two reservations but with different share values (4, then the default 1), which doesn't model what execute_batch actually does (same fixed share for every slot in the wave).

Suggested fix direction: compute the wave's per-slot allocation once (e.g. snapshot free before the wave starts, divide by share, and have each slot reserve a fixed amount rather than re-deriving free/share from a shrinking free), or have execute_batch pre-reserve all effective_workers slots atomically under one lock acquisition.

Everything else checked out

• Credential-migration leak (critical, internal review): the shared get_credential_manager in ui/dependencies.py ties migrate=has_scope(auth, SCOPE_ADMIN) to the resolved auth dict itself rather than relying on FastAPI dependency ordering relative to require_scope(SCOPE_ADMIN) — correctly closes the hole regardless of parameter order.
• settings_v2.store_key/delete_key: provider resolved before the admin check (matches the updated scope tests targeting GIT_GITHUB), write-scope gate only exempts LLM providers, GitHub PAT still admin-gated via _require_admin_unless_llm_key.
• check_spend_limit/reserve_today_usd/release hold bookkeeping: day-tagged holds correctly stop counting at UTC midnight; release matches by held amount, which is safe since holds are fungible once reserved.
• resolve_cost_cap's prior_usd offset is correctly plumbed through both react_agent.py and the legacy agent.py (_load_prior_cost), with a small inefficiency where ReactAgent.run calls _resolve_cost_cap() once with prior_usd=0 before recomputing with the real prior cost whenever a workspace cap exists, even with no daily ceiling — harmless (same result, one extra DB read), not worth a review comment on its own.
• cf auth user-create: offline/direct-DB trust boundary is consistent with set-password, duplicate-email check is case-insensitive, and the "no admin exists yet, require --admin" guard correctly pre-empts the [P0.4] Enforce real scopes and tenancy: JWT sessions get admin, read-only keys can revoke keys, workspace ownership is reassignable #898 backfill from silently promoting a non-admin first account.

Minor / low-severity, not blocking

In tasks_v2.start_single_task/resume_task, if an exception occurs after runtime.start_task_run/resume_run succeeds but before _spawn_agent_worker is actually invoked (e.g. threading.Thread(...).start() failing under resource exhaustion), the reserved ceiling hold is never released — there's no finally covering that narrow window, only the inner except BaseException around the state-transition call itself. Very unlikely in practice (dict construction and Thread.start() essentially never raise), so I'm not flagging it as a real bug, just a gap worth knowing about if it ever surfaces as a "budget appears to shrink for no recorded spend" report.

Nice test discipline overall (mutation-tested, explicit triage tables in the PR body). The parallel-share issue above is the one thing I'd want addressed or consciously deferred before relying on max_parallel > 1 batches under a tight daily limit.

Triage of the bot reviews at eaa4913 (GLM precision review + claude-review), fixed in b8a1f39:

CI's 3 failures (test_concurrent_write_guards_1085) were a patch target the shared dependency moved away from: it patched settings_v2.CredentialManager, which is no longer constructed there. It now patches the source module. Mutation check: 21/21 caught.

Claude finished @frankbria's task in 11m 38s —— View job

GLM precision review — PR #1343 @ b8a1f39

Bug-hunting pass over the full diff (logic errors, security, data loss, races — style/coverage excluded). The newest commit (b8a1f39) reworks the parallel-share division, slot sizing, auth-off exemption, provider gate and lock scope, so those got the closest look; earlier-commit findings already posted by GLM/claude-review are re-checked only where the fix touched them, and not repeated.

• Gather PR diff and existing review comments
• Read core/spend_limit.py + callers (runtime, conductor, tasks_v2, batches_v2)
• Read credential/scope changes (ui/dependencies.py, settings_v2, github_integrations_v2)
• Read auth user-create, schema change, cost_tracker/react_agent/agent clamp math
• Verify each suspicious hunk against surrounding code and callers
• Post confirmed findings + summary

1 finding, minor (posted as an inline review comment):

Verified sound (checked, no defect) — with the four b8a1f39 fixes each confirmed against real code:

• Even-share division: amount = free/(share − siblings) under the reservation lock is self-balancing — after k same-day reservations of a group, each has committed exactly free₀/share, total ≤ free₀; a mid-wave release returns budget symmetrically. The committed-today invariant (spent + held ≤ limit after every append) holds in every interleaving I could construct, including mixed groups and interleaved group=None single-task runs.
• Lock scope: moving spend_today_usd outside _held_lock cannot over-commit — unrecorded spend only exists while its run is in flight, and that run's hold (≥ its run-spend ceiling, since the clamp hands the run exactly its hold) is counted inside the lock; the REST path releases only after execute_agent returns (usage persisted per call) and the conductor path releases only after the child process exits (the child persists before exiting).
• Auth-off exemption: type == "disabled" is set only on the synthetic principal (auth/dependencies.py:771); JWT and API-key principals get "jwt"/"api_key" even on an auth-off server, so no tenant-shaped principal can reach the exemption.
• Slot sizing min(max_parallel, len(task_ids)): 2-task/4-slot batch now splits evenly (tested); share ≤ 0 degrades to 1 via max(..., 1); resume_batch really is serial (_execute_serial_resume), so its hardcoded share of 1 is right.
• PAT allowlist: _TENANT_OWNED_PROVIDERS inclusion check, applied after _resolve_provider validation, in both store_key and delete_key; migration is admin-conditional inside the shared _build_credential_manager, so dependency ordering is irrelevant; both routers use the one shared dependency (no other CredentialManager( construction outside core).
• user-create: INSERT matches the users schema (all omitted columns have defaults), duplicate check is case-normalized both sides, and the "first account must be admin" guard's predicate (is_superuser=1 AND hashed_password != '!DISABLED!') is exactly the _ensure_bootstrap_superuser backfill's NOT EXISTS predicate — no is_active mismatch to reopen the silent-promotion hole.
• Clamp math: min(workspace_cap, prior + ceiling) compared against prior + run_spend yields run_spend ≤ ceiling in both engines and both resolve sites (ReactAgent.run's double-resolve is coherent; plan engine's _load_prior_cost follows the same chain); worktree batch children resolve the main workspace by the walk-up (.codeframe is untracked, so the worktree has none), so their token_usage lands in the DB the meter reads; get_costs_summary(1) anchors to the same UTC midnight as the day tags; the unmetered-engine refusal covers retries and supervisor re-runs and fires before the reserve (no hold leaked).

Not repeated: the auto-strategy/max_parallel under-allocation, the Thread.start() hold-leak window, and the per-process-holds / tenant-writable-ledger caveats — all already posted or documented in the PR body.

Review (post-b8a1f39, after the parallel-share / auth-off / PAT-allowlist / lock-scope fixes)

This PR has already been through an unusually thorough gauntlet (codex x3, GLM, claude-review x3), and I re-verified the fixes from the latest round land correctly in the checked-out code rather than just re-reviewing the diff:

• _require_admin_unless_llm_key is now a real allowlist (_TENANT_OWNED_PROVIDERS = {LLM_ANTHROPIC, LLM_OPENAI}, settings_v2.py:235-242) — matches its own docstring now, no longer fails open on a future provider addition.
• check_spend_limit's auth-off exemption checks auth.get("type") == "disabled" in addition to user_id is None (ui/dependencies.py:330), matching the real post-[P2.13] Fix auth configuration timing and auth-disabled degradations #963 principal shape.
• reserve_today_usd's even-split math checks out: amount = _free_today(...) / max(share - siblings, 1), computed with siblings counted under the same lock — I traced it by hand for a 4-slot/$10 case and got 2.5/2.5/2.5/2.5, not the earlier geometric 2.5/1.875/1.40625/1.05 decay.
• execute_batch's share = min(batch.max_parallel, len(batch.task_ids)) correctly fixes the "1-2 tasks but max_parallel=4" case.
• spend_today_usd disk I/O happens outside _held_lock (remaining_today_usd/reserve_today_usd both read spent before acquiring the lock) — the global-lock-serializes-everyone finding is addressed.
• covers_engine() gate in _execute_task_subprocess, day-tagged holds in _held, resolve_cost_cap's prior_usd offset, the shared _build_credential_manager dependency, and the reserve/release lifecycle in tasks_v2.py (including the finally release around _spawn_task_child) — all consistent with what's described in the triage tables.

Two small residual items, both conservative (fail-closed) rather than exploitable, so not blocking:

1. execute_batch's share still doesn't track real per-group concurrency for "auto"-strategy batches. share = min(batch.max_parallel, len(batch.task_ids)) uses the whole batch's task count, but _execute_parallel computes real concurrency per dependency group as effective_workers = min(group_size, batch.max_parallel) (conductor.py:2100). For an "auto" batch with, say, 10 tasks in a mostly-linear dependency chain (groups of 1-2), share is still min(max_parallel, 10) even though at most 1-2 tasks ever run concurrently — so those tasks can still be clamped to a fraction of the budget they're entitled to. This is the second sub-case from the earlier claude-review comment ("an auto batch where the inferred graph is a mostly-linear chain"); the fix that landed only addresses the first sub-case (small task count under a larger max_parallel). Worth a follow-up if auto + daily limits turns out to be a common combination, but it only under-allocates, never over-allocates.

2. spend_limit.release() matches a hold by amount equality alone, with no group check (spend_limit.py:167-178). If two different reservation groups for the same user ever produce numerically identical amounts (plausible since the even-split formula can yield the same value across unrelated batches), release can remove the wrong group's entry. This can't break the overall budget invariant — _held_today sums regardless of group, so total held/spent stays correct — but it can leave one group's siblings count stale and another's undercounted, skewing that group's subsequent per-slot shares in the conservative direction. Narrow and low-impact; flagging in case it's worth a comment noting holds are "fungible across groups, not just within one."

Everything else I looked at (auth user-create's backfill-guard and case-insensitive duplicate check, workspace_spend_users/spend_paths UNION semantics, the KeySlot.tsx frontend gating matching the backend allowlist) matches the PR description and prior reviews' sign-off.

Claude finished @frankbria's task in 11m 49s —— View job

GLM precision review — PR #1343 @ e5694ad

Bug-hunting pass over the full diff (logic errors, security, data loss, races — style/coverage excluded). The three newest commits (e85b6b8, 35a7e97, e5694ad) rework the per-group share sizing, the day-aware sibling count and the group-matched release, so those got the closest look; findings already posted by GLM/claude-review/codex for earlier commits were re-checked only where these commits touched them, and not repeated.

• Gather PR diff and existing review comments
• Read core/spend_limit.py + callers (runtime, conductor, tasks_v2, batches_v2) at HEAD
• Read credential/scope changes (ui/dependencies.py, settings_v2, github_integrations_v2)
• Read auth user-create, schema change, cost_tracker/react_agent/agent clamp math
• Verify each suspicious hunk against surrounding code and callers
• Post confirmed findings + summary

✅ GLM review: no defects found.

Verified sound (checked against the real code, no defect) — the three newest fixes first:

• Day-aware sibling count (spend_limit.py:162-164): the GLM@b8a1f39 midnight over-commit is fixed — a D1 hold no longer consumes a D2 slot; I re-derived the wave math (4-slot group straddling midnight now holds 4 × free_D2/4, not 3 × free_D2/3). The residual double _today() (count at :161, append at :166) spans a microsecond window and can only mis-tag in the conservative direction (divisor from a stale sibling set → smaller hold), so it is not reportable.
• Per-group shares + dropped batch-level sizing: _set_spend_share runs at the top of every group iteration (:2073) and again with effective_workers (:2102); single-task groups run synchronously in the group loop and _execute_group_parallel joins its pool before returning, so the share mutation can never race a still-running group; max(share, 1) guards ≤ 0; serial and serial-resume correctly keep share 1.
• Group-matched release: both conductor sides pass group=batch_id (:2619/:2633) and the REST single-task holds use group=None on reserve and on all three release sites (check_spend_limit:353, tasks_v2:990/1038/1172). I chased the ambiguous-match case (same amount, same group, different day): entries append in time order with non-decreasing day tags and release deletes the first match, so a cross-day ambiguity removes the older entry — leaving a live today-hold counted (under-commit), never dropping a live hold (over-commit).
• Reserve/release lifecycle: all seven conductor spawn sites (serial, serial-resume retries, single-task group, parallel pool, supervisor re-run at :2407 and :1861) funnel through _execute_task_subprocess, whose finally releases exactly once; the reconciliation thread spawns nothing; a refused (429/SpendLimitExceeded) or unmetered-engine task returns FAILED before any hold is taken.
• Invariant spent + held_today ≤ limit holds in every interleaving I constructed (mixed groups, group_size > workers, mid-wave releases, interleaved group=None single starts, two concurrent batches — the last only under-commits, the documented "only what is left"). _held_today/_free_today take no lock themselves, so there is no nested-acquire deadlock on the non-reentrant _held_lock.
• 429 propagation: check_spend_limit sits outside each route's try (start/resume task, resume batch) or behind except HTTPException: raise (start_execution, approve) — no route re-wraps it as 500; the reservation precedes every state write.
• Credential consolidation: one shared _build_credential_manager with migrate=has_scope(auth, SCOPE_ADMIN) (order-independent vs require_scope), _TENANT_OWNED_PROVIDERS inclusion check applied after provider validation in both store_key and delete_key, unreadable-store → formatted 500 preserved, no import cycle.
• user-create: INSERT matches the users schema (omitted columns all have defaults), duplicate check case-normalized on both sides, the "first account must be admin" predicate matches _ensure_bootstrap_superuser's NOT EXISTS exactly, print_error/escape() per [P3.8] Replace the ad-hoc Rich-markup scanner test with a real lint rule #1054, and the command is classified in the hostile-data guard.
• Schema/plumbing: workspace_spend_users (PK (user_id, repo_path) + INSERT OR IGNORE) has no constraint-failure path; spend_paths UNION with ownership is parameterized; the platform DB is cross-thread-safe (check_same_thread=False + RLock) for the conductor-thread spend_scope() calls; repr(float) round-trips the env ceiling; get_costs_summary(1) anchors to the same UTC midnight as the day tags; the clamp offset (min(cap, prior + ceiling) against lifetime spend) cancels correctly in both engines and both resolve sites.

Not repeated (already posted or documented in the PR body): the Thread.start() hold-leak window (claude-review@eaa4913), tenant-writable spend ledgers, per-process holds, workspace-not-caller attribution, and the unmetered PRD/discovery/chat routes — all known and deferred.

Triage of the reviews at b8a1f39, fixed in e85b6b8, 7e… (HEAD e5694ad):

Mutation check: 20/20 caught on the current code. The earlier batch share from strategy and share capped by task count mutations were retired with the code they targeted.

Review (HEAD: e5694ad)

This is the fifth pass I've made on this PR across its review rounds (codex x3, GLM x2, claude x4 including this one), and at this point I'm reviewing primarily to confirm the latest fixes land correctly rather than hunting for new ground.

Both outstanding findings from the last round are now fixed and tested:

1. GLM's midnight-UTC over-commit (spend_limit.py:161 in the prior revision): sibling counting in reserve_today_usd ignored the day tag, so a stale hold from a previous UTC day consumed a slot without contributing to _held_today, letting a batch wave straddling midnight over-commit. Now fixed — siblings filters on day == today in addition to group == g (spend_limit.py). Covered by test_yesterdays_sibling_does_not_take_a_slot_today, which pins the exact scenario (day-1 hold with share=4 must not shrink today's divisor to 3).

2. My own finding that release() matched only by amount, not group: a numerically-identical hold from a different group could be removed instead of the intended one. Now fixed — release matches on held == amount and held_group == group. Covered by test_release_matches_the_group_too, which constructs two groups with intentionally identical amounts and asserts the right one survives.

Also resolved since my last pass: the "share sized by configured max_parallel instead of real per-group concurrency" issue I raised for auto-strategy/mostly-linear batches. execute_batch no longer fixes share once for the whole batch; _set_spend_share is now called per dependency group with min(group_size, batch.max_parallel) (or effective_workers when per-status concurrency applies), so a linear chain inside an auto batch no longer gets throttled by a max_parallel it never actually uses. test_each_dependency_group_sets_its_own_share exercises exactly this: a 2-task parallel group splits the remainder, and the 1-task group that depends on it afterward gets the full remainder, not the earlier group's half. I traced the release/reserve sequencing by hand to confirm groups run strictly sequentially (_execute_single_task/_execute_group_parallel block before the next group starts), so a prior group's holds are always released before the next group's reserve_today_usd call counts siblings — no leftover cross-group contention.

Nothing new found. I re-checked the surrounding invariants this round touched (even-split math, lock scope, day-tagging) and they're unchanged and still sound per the prior rounds' sign-off.

The two previously-noted, consciously-deferred items remain in that state and don't need re-litigating:

• Thread.start() hold-leak window in tasks_v2.py (vanishingly unlikely, not a real bug)
• The "tenants can write their own spend data" / "holds are per-process" limitations, already called out in the PR body as follow-up work

This PR is in good shape to merge. The review loop here (codex → internal → GLM → claude, repeated) has converged: each round's findings are narrower and lower-severity than the last, and this round's fixes come with tests that would fail under mutation if the fix regressed.

Post-merge disposition: filed #1344 ([P2.71] server-side spend ledger: tenant-writable data, workspace attribution, per-process holds, delegated engines), #1345 ([P2.72] spend limit on PRD/discovery/task-generation/session chat), #1346 ([P3.39] key provenance in LLM 401 messages). Load-timing flake noted on #1326. Lessons: 4 entries appended to tasks/lessons.md. Dropped as trivia: the Thread.start() hold-leak window (claude-review, vanishingly unlikely) and a USER_CREATED audit row for user-create (matches set-password/activate, which have no offline actor).