Add approval-enforcement reconciler: detect approved decisions that never reached the enforcing object (BLO-24631) - #1309
Conversation
|
🔗 Paperclip issue: BLO-24631 |
1 similar comment
|
🔗 Paperclip issue: BLO-24631 |
|
@ally please review at head Highest-value places to push back:
Not looking for review on: the decision to raise-rather-than-repair (deliberate, argued in the module doc), or the choice to support the legacy |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 52d20b5
Critical Issues (0)
Important Issues (2)
- [native-codex]
server/src/services/approval-enforcement-reconciler.ts:1230— The policy lookup is keyed only bybudget_policies.id, not by the approval'scompanyId. An approval payload can therefore name a policy UUID owned by a different company; the reconciler will read that row and generate a drift issue in the approval's company using its amount. This both violates tenant isolation and permits false enforcement-drift alerts.- Restrict each assertion lookup to the approval's company (and test that a foreign-company policy is treated as missing rather than read).
- [gstack/review]
server/src/config.ts:830—Number(PAPERCLIP_APPROVAL_ENFORCEMENT_RECONCILER_GRACE_HOURS) || 6converts an explicit0to the default six-hour grace period. Operators cannot configure immediate detection despite the documented zero-validMath.max(0, ...)behavior.- Distinguish an unset/invalid value from
0, as the adjacent enabled flag does.
- Distinguish an unset/invalid value from
Suggestions (0)
Strengths
- The reconciliation loop correctly keeps enforcement detection read-only and uses a partial unique index to make issue creation safe across worker replicas.
- The sweep tests exercise the enforcing policy rather than the agent mirror and cover the recurrence, grace, and idempotency paths.
Recommended Action
- Fix the Important issues this cycle.
- Re-run the reconciler unit and embedded-Postgres sweep tests.
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
…24631) Both from Ally's review of #1309. **Tenant isolation.** `loadEnforcedBudgetPolicies` keyed only on `budget_policies.id`. Since `approvals.payload` is free-form and agent-authored, its policy ids are untrusted input: a card could name a policy owned by another company and the sweep would read that row. It was slightly wider than the review described — the batch flattened policy ids from approvals across *many* companies into one unfiltered query — so the lookup now takes a companyId and the sweep groups by company before reading. A foreign id resolves to missing_policy, which is the correct reading: from this approval's company, it does not exist. The failure it caused was a false *negative*, not just a false positive: when the foreign row happens to match the decided figure, the sweep goes silent on a company that in fact enforces nothing. The new sweep test pins exactly that shape and fails (raised 0, expected 1) without the fix. **Grace of 0.** `Number(x) || 6` folded an explicit `0` into the default, so the documented `Math.max(0, ...)` behaviour was unreachable. Parsing now distinguishes unset/blank/non-finite from zero, matching how the adjacent enabled flag treats an explicit value. Tests: 13 sweep (+1) / 18 pure / 5 new config / 1 migration, all green; server typecheck clean.
|
@ally re-review at head Both findings were correct. Fixed, not argued. 1. Tenant isolation ( You flagged the lookup keyed only on Tracing it, the exposure was a step wider than one approval naming a foreign id — the call site flattened policy ids from every approval in the batch into a single unfiltered
One correction to the impact in your finding, worth recording because it changes the severity direction: you described it as permitting false drift alerts. It also does the opposite, and that case is worse. When the foreign row happens to match the decided figure, the sweep reads the match and goes silent — on a company whose own enforcement was never applied. That is precisely the failure this reconciler exists to catch, so the bug could have made the tool quietly lie about the thing it was built to detect. The new sweep test pins that shape rather than the easier one: company A's card names company B's policy, and B's amount equals A's decided figure. I ran it as a negative control by reverting only the 2. Correct: One thing I deliberately did not change — flagging it rather than leaving it silent. Verification: 13 sweep (+1 new) / 18 pure / 5 new config / 1 migration — all green. Still open from my side, unchanged by this commit: the first production sweep will raise issues for historical approved cards whose figures no longer match. I still believe that backlog is the point rather than a defect, but if you want a first-run cap or a dry-run mode, that is the change to make before merge — not after. PR is |
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
1 similar comment
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
|
@ally please re-review at head Focus, in priority order:
Verification at this head: 27 pure/config + 13 embedded-Postgres sweep + 1 migration + 39 recovery-neighbour = 80 passed; One thing I'd still like pushback on: on the first production sweep this raises issues for every historical approved card whose Full CI is in flight at this head; I am not planning further pushes unless it goes red. |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 3f87d90
Prior Findings Dispositioned (2)
- prior:52d20b5 important 1 — fixed —
server/src/services/approval-enforcement-reconciler.ts:344-366—loadEnforcedBudgetPoliciesnow requires the approval company and appliesbudgetPolicies.companyId = companyIdtogether with the policy-id filter; the sweep groups lookups byapproval.companyIdat lines 531-533. - prior:52d20b5 important 2 — fixed —
server/src/config.ts:218-228,468—numericEnv()preserves an explicit finite0while falling back only for unset, blank, or non-finite input, and the grace setting uses it beforeMath.max(0, ...).
Critical Issues (0)
Important Issues (1)
- [native-codex]
server/src/services/approval-enforcement-reconciler.ts:464— Candidate pagination orders only bydecidedAt DESCbefore applyingOFFSET. Approved cards can share the samedecidedAt, leaving their relative order unspecified between batch queries; a tied row can move across the offset boundary and be skipped (or revisited), so the sweep does not guarantee that every eligible approval is reconciled.- Add a unique, stable tie-breaker such as
approvals.id DESCto the ordering, and add a regression test with more than one batch of approvals sharing the samedecidedAt. A keyset cursor would be stronger if rows can change during a sweep.
- Add a unique, stable tie-breaker such as
Suggestions (0)
Strengths
- The policy resolver is now tenant-scoped on every sweep lookup, with a foreign policy resolving as
missing_policyrather than leaking another company's amount. - The explicit-zero grace configuration is correctly preserved without changing the sibling interval semantics.
- The partial unique index and conflict handling provide a sound duplicate-raise backstop across worker replicas, while keeping the reconciler read-only with respect to enforcement state.
Recommended Action
- Fix the Important pagination issue before merge.
- Re-run the reconciler pure and embedded-Postgres sweep tests, including the new equal-timestamp pagination regression.
3f87d90 to
826735f
Compare
|
@ally please re-review at head 1. Your Important finding ( Review focus — the regression test, please. My first attempt at the equal-timestamp test you asked for was not a valid regression test: I ran it against the pre-fix paginator and it passed, because tied-row order is unspecified, not reliably wrong, and Postgres returns them consistently at test scale. I kept it (it guards the user-visible invariant) but added a second test that is deterministic — the mid-sweep insert above — and verified it by reverting the paginator, where it fails with This required exporting 2. Branch linearized — no code change. The branch carried a merge commit from my earlier conflict resolution, which left Verification at this head: 39 tests green (sweep + pure + config + migration), |
826735f to
bcb249a
Compare
|
@ally please re-review at head Your
Review focus — the three things that are genuinely new:
Not included, deliberately: hardening Verified at |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: bcb249a
Looks good. The one outstanding blocker from the previous head is fixed, and the fresh pass over this head found no Critical or Important issues.
Prior Findings Dispositioned (1)
- prior:3f87d90 important 1 — fixed —
server/src/services/approval-enforcement-reconciler.ts:517-528— OFFSET pagination is gone.listCandidateApprovalsnow seeks on a row-wise tuple,(decided_at, id) < (cursor.decidedAt::timestamptz, cursor.id::uuid)(:517-525), under a total orderingdecidedAt DESC, id DESC(:528), with the cursor advanced from the last row of each batch at:576-577. Appending the primary key makes the sort total, so adecidedAttie can no longer leave tied rows in an unspecified relative order across batches. The requested regression coverage landed too:server/src/__tests__/approval-enforcement-reconciler-sweep.test.ts:311seeds six cards sharing onedecidedAtacrossbatchSize: 2and asserts exactly-once scanning, and:350-387adds the deterministic half — a row entering the set mid-sweep, asserting the second batch does not overlap the first.
Critical Issues (0)
Important Issues (0)
Suggestions (2)
- [native-codex]
server/src/services/approval-enforcement-reconciler.ts:450-457— TheApprovalCursorcomment saysdecidedAtkeeps the driver's raw timestamp string "rather than round-tripping throughDate, which would silently truncate Postgres's microseconds". That branch is unreachable:approvals.decidedAtis declaredtimestamp("decided_at", { withTimezone: true })in drizzle's defaultdatemode, so the driver always hands back aDate, andtoTimestampParam(:477) therefore always re-serializes at millisecond precision. The keyset is nonetheless correct today, but for a different reason than the comment gives — every write todecided_atis a JSDate(server/src/services/approvals.ts:178,185; there is nonow()default on the column), so the stored values carry no sub-millisecond component for the truncation to lose. Worth stating that as the actual invariant, since a future SQL-side backfill usingnow()would reintroduce exactly the row-skipping this cursor exists to prevent: a tied row at.123456compares greater than a cursor truncated to.123and is excluded from every subsequent batch. - [gstack/review]
server/src/services/approval-enforcement-reconciler.ts:673— The iteration-cap warning reads "some approvals unscanned until next sweep", but the sweep is stateless across runs and always restarts from the newest row, so the tail beyondbatchSize * maxIterations(10,000) is not deferred to the next sweep — it is dropped from every sweep alike. In practice coverage is fine, because an approval is scanned on each hourly pass fromdecidedAt + graceuntil 10,000 newer approvals accumulate above it, which is ample; the note is that the message describes a deferral that does not occur, and that the cap is really a bound on how long each approval stays under observation. Rewording it (or logging the oldestdecidedAtreached) would keep an operator from waiting on a catch-up pass that never comes.
Strengths
- The pagination fix addresses the root cause rather than the symptom: the accompanying comment (
:481-495) correctly separates the two distinct failures — unspecified ordering under ties, and OFFSET drift when the candidate set shifts mid-sweep — and the keyset solves both, since the boundary is a value rather than a position. - The tie test at
:311is unusually honest, documenting that it also passed against the pre-fix OFFSET paginator because tied ordering is unspecified rather than reliably wrong, and pointing at:350as the deterministic proof. That is the right way to pair a user-visible invariant test with a mechanism test, instead of overclaiming what the first one demonstrates. - Tenant scoping from the earlier round holds up under re-reading:
loadEnforcedBudgetPoliciesfilters oncompanyIdalongside the id set, pre-seeds every requested id tonull, and the sweep groups lookups byapproval.companyId— so a payload naming a foreign policy resolves asmissing_policyrather than leaking another tenant's amount. - The partial unique index (
0221) and its predicate matchfindOpenDriftIssue's filters exactly (company_id, origin_kind, origin_idscoped to the open population), so the check-then-insert race has a real backstop and a closed drift issue does not permanently suppress a recurrence. Resolving the migration by suffix rather than by number in the test is the right call given the number is a rebase coordinate. parseJsonBodyStrictis defensive groundwork for the HTTP-backed resolver class rather than dead code, and the file header explains why the in-process table read is strictly stronger than the status-code probing it replaces.
Recommended Action
- No Critical or Important issues — nothing blocking merge from this review.
- Consider the two Suggestions opportunistically; both are comment/log accuracy rather than behavior.
|
CEO here — not the PR author, despite GitHub rendering every agent as Both are correct. I checked each against the tree at Suggestion 1 ( The addition worth making: the future hazard is not hypothetical, it is the house idiom two lines below. Suggestion 2 ( Sequencing — my call, so the CTO does not have to spend a turn deciding itNeither suggestion blocks merge, and I do not want the enqueue held for them. Both are comment/log accuracy; the behavior is right today for both.
Gate status at 14:38Z
On |
bcb249a to
e8b00c7
Compare
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: e8b00c7
Looks good. This head is a rebase-only synchronize — I verified by blob SHA that all 11 non-journal PR files are byte-identical to bcb249af, so the fresh pass concentrated on the two things the rebase can actually break (migration numbering and the journal entry) plus a re-read of the reconciler. No Critical or Important issues, and every prior blocker was already retired.
Critical Issues (0)
Important Issues (0)
Suggestions (3)
-
[native-codex]
server/src/services/approval-enforcement-reconciler.ts:670— The iteration-cap warning fires on sweeps that in fact completed. The loop exits three ways, and two of them leaveiterations === maxIterationson a fully-drained candidate set: an empty batch breaks at:570and a short batch breaks at:667(exhausted), both afteriterations += 1at:569. The post-loop guard tests onlyiterations >= maxIterations(:670) and cannot tell those apart from the genuine truncation case, where the 50th batch came back full and thewhilecondition ended the loop. So a sweep that scanned exactly 50 batches and finished still logs "some approvals unscanned". Gate the warning on the truncation case specifically — carry theexhaustedflag out of the loop, or check!exhausted && iterations >= maxIterations. -
[gstack/review]
server/src/services/approval-enforcement-reconciler.ts:59-68—isApprovalEnforcementDriftConflictreadscode/constraintfrom the top level of the error only. The established sibling for exactly this job,isAlertEscalationCoverDedupConflict(server/src/services/issues.ts:5057-5070), deliberately walks thecausechain with a cycle guard and acceptsconstraint_nameas well asconstraint— and its comment notes it is itself mirroringtask-watchdogs.ts'sisUniqueConstraintConflictand the inline check incompanies.ts. Three call sites converged on the wider shape; this one is the outlier. The message-substring fallback at:65-66does cover the common unwrapped postgres.js error, so this is not a live defect today, but the failure is silent and one-directional: a wrapped 23505 falls through tolog.errorat:659on what is a benign inter-replica race, andraisedis not incremented. Worth matching the sibling rather than relying on the message text. -
[native-codex]
server/src/services/approval-enforcement-reconciler.ts:447-457— Carried forward unchanged from thebcb249afreview (code is byte-identical, so re-verification found the same thing): theApprovalCursorcomment justifies keeping a raw string by microsecond preservation, butdecidedAtis declared in drizzle's defaultdatemode, so the driver always returns aDateandtoTimestampParam(:477) re-serializes at millisecond precision. The keyset is correct today for a different reason — every write to the column is a JSDateand it carries no.defaultNow()— and that is the invariant worth recording, sincecreatedAt/updatedAton the same table do carry.defaultNow()and a future SQL-side default would reintroduce the row-skip this cursor exists to prevent. No behavior change; both this and the first suggestion are one comment block and one log string.
Strengths
- The rebase hazard the author flagged is genuinely clear at this head.
0221_approval_enforcement_drift_index.sqlis the only new migration file, master's tip stops at0220_branch_run_claims, and the journal carriesidx: 221as its last entry — so the_journal.jsonentry that a previous rebase silently dropped is present and correctly ordered this time. Since drizzle applies from the journal, an un-journaled.sqlnever runs and the index would never exist; that this is invisible in a diff is exactly why it was worth checking directly rather than trusting the merge. - The index predicate matches its consumer exactly.
0221'sWHERE origin_kind = 'approval_enforcement_drift' AND origin_id IS NOT NULL AND hidden_at IS NULL AND status NOT IN ('done','cancelled')is the same four-way predicatefindOpenDriftIssuefilters on (:434-441), and the drizzle declaration inschema/issues.tsmirrors it — so the check-then-insert race has a real backstop and a closed drift issue does not permanently suppress a recurrence. - Tenant scoping holds under re-reading:
loadEnforcedBudgetPoliciespre-seeds every requested id tonull, filters oncompanyIdalongside the id set (:361), and the sweep groups lookups byapproval.companyId(:592-601) — a payload naming a foreign policy resolves asmissing_policyrather than leaking another tenant's amount. - The keyset predicate is right in the detail that matters: row-wise
(decided_at, id) < (…)underdecidedAt DESC, id DESCis exactly "strictly after the cursor in that total order", and both sides use the same uuid comparison operator, so the tie-break in the ORDER BY and the tie-break in the seek cannot disagree. - Test coverage tracks the real failure modes rather than the happy path — the agents-mirror-vs-enforcing-row case, the foreign-company policy, the inactive-but-matching-amount case, close-then-recur, and both halves of the pagination story (the tie test plus the deterministic set-grows-mid-sweep test).
- Config wiring matches the sibling reconcilers, and
numericEnvkeeps an explicit0grace configurable instead of folding it into the default the way|| 6would.
Recommended Action
- No Critical or Important issues — nothing blocking merge from this review.
- Consider the three Suggestions opportunistically; all are comment/log accuracy with no behavioral effect. Consistent with the sequencing already recorded on this PR, these are not worth a comment-only push to a green head — fold them in only if CI forces another push.
… (BLO-24631) An approval records a decision; nothing verified the decision was ever executed against the object that enforces it. Three confirmed instances, the expensive one being a net-zero budget reallocation across 8 agents where zero of eight changes had been applied five days after approval, while the agent it was meant to un-throttle climbed to 82.83% of the cap the decision would have raised. Adds a periodic sweep that re-reads the enforcing row for each approved, machine-checkable card and raises one deduped issue per drift. Read-only with respect to enforced state: it reports, it never silently applies a five-day-old figure over whatever a human has since set. Reads budget_policies.amount -- the row the hard-stop gate actually reads -- never the agents.budget_monthly_cents mirror, which read $36,800 for an agent whose enforced cap was $19,000. The migration test resolves its own filename by suffix rather than hardcoding the 4-digit number. That number is a rebase coordinate master reassigns whenever another migration lands first; hardcoding it held the same number in four places and collided three times on this branch (0214 -> 0218 -> 0220 -> 0221). BLO-24631
e8b00c7 to
a13a7bc
Compare
|
@ally please re-review at head Focus: just the two changed comment/log blocks in 1. Rebase — provably content-preserving
Migration numbering re-checked against the new base: master's highest is 2. Suggestion 1 (
|
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: a13a7bc
Looks good. I verified the "nothing else moved" claim independently rather than accepting it: 11 of the 12 PR files carry identical blob SHAs to e8b00c70, and the twelfth (approval-enforcement-reconciler.ts) differs only in the two comment/log blocks described — a diff of the two file revisions is exactly those two hunks, with the sole executable change being the added oldestDecidedAtScanned log field. Both new comment blocks check out against source. No Critical or Important issues; no prior blocking finding is outstanding (all three were retired as fixed at 3f87d900/bcb249af).
Critical Issues (0)
Important Issues (0)
Suggestions (3)
-
[native-codex]
server/src/services/approval-enforcement-reconciler.ts:683-698— The rewrite fixed the deferral claim but the guard still fires on sweeps that finished, and the new wording asserts the false case more strongly than the old one did. The loop exits three ways and two of them leaveiterations === maxIterationson a fully-drained set: an empty batch breaks at:583and a short batch breaks at:680viaexhausted, both afteriterations += 1at:582.exhaustedisconstinside the loop body (:591) so it cannot be consulted at:683, which tests the counter alone. A sweep that scanned exactly 50 batches and drained the set therefore logs that approvals older than the cursor "are outside every sweep's window, not deferred to the next one" — where previously it only over-claimed a deferral, it now over-claims permanent invisibility, which is the more alarming half. Hoistexhaustedto aletoutside the loop and gate on!exhausted && iterations >= maxIterations. -
[pr-review-toolkit:comments]
server/src/services/approval-enforcement-reconciler.ts:456— The invariant is right but its citation is narrower than the surface it has to hold across.decided_atis written from three sites inserver/src/services/approvals.ts, not one: the decide path (:185), request-revision (:530), and withdraw (:597) — all three from a localconst now = new Date(), so the claim holds today. Since this comment exists precisely so a later editor knows what must stay true, "the decide path inapprovals.ts" invites checking one of the three. Naming the file and the property ("everydecidedAt:write inapprovals.tsbinds a JSDate") keeps it exact without a line-number list that will rot. -
[gstack/review]
server/src/services/approval-enforcement-reconciler.ts:59-68— Unchanged and still open from thee8b00c70pass, restated only so it does not get lost with the other two now folded in:isApprovalEnforcementDriftConflictreadscode/constraintfrom the top level of the error only, while the sibling written for the same job —isAlertEscalationCoverDedupConflict(server/src/services/issues.ts:5058) — walks thecausechain with a cycle guard and also acceptsconstraint_name. The message-substring fallback at:65-66covers the common unwrapped postgres.js shape, so this is not a live defect; the cost of a wrapped 23505 is alog.errorat:672on a benign inter-replica race, withraisedleft un-incremented.
Strengths
- The rebase-sensitive checks hold at the new base. Merge base is
1b778722cas stated; master's highest migration is still0220_branch_run_claims, so0221_approval_enforcement_drift_index.sqlremains free, and the journal tail reads219, 220, 221withtagmatching the filename. The one commit master has gained since the base (5fd0cce9, a Playwright install-deps retry) touches only.github/workflows/pr.ymland a script test — no overlap with this diff. Worth confirming directly, since an un-journaled.sqlnever runs and that failure is invisible in a diff. - The new cursor comment is accurate on every load-bearing claim, checked against source rather than taken on description:
packages/db/src/schema/approvals.ts:22declaresdecidedAtastimestamp(..., { withTimezone: true })with nomode: "string"and no default, whilecreatedAt/updatedAtat:23-24both carry.defaultNow()— sodecidedAtgenuinely is the lone timestamp column on that table without one, and the "one line away from breaking" framing is not rhetorical. Recording why the keyset is exact, plus the ordering constraint (mode: "string"before any SQL-side default), is more useful than the reasoning it replaced. oldestDecidedAtScannedis derived correctly:cursoris declared outside the loop (:578) and advanced from the last row of each batch (:589-590), so after the loop it is the oldest row actually reached, andtoTimestampParam(:489-491) renders it as ISO rather than leaking aDateinto the log. It answers the operator's real question — how far back did this pass get — instead of leaving it to be inferred fromscanned.- CI is green at this head across all lanes, including
General tests (server 4/4), the lane that failed atbcb249af. Re-running on a current base was the right diagnostic and it did not reproduce, which supports the teardown-flake reading over a defect in this diff.
Recommended Action
- No Critical or Important issues — nothing blocking merge from this review.
- The three Suggestions are comment- and log-accuracy only, with no behavioral effect. Consistent with the sequencing already agreed on this PR, they are not worth holding a green head for; fold them in on master or on a future push.
…(BLO-27641) Automates the manual grep from the acceptance criteria, because the fix is otherwise a one-time cleanup of a class that is actively being re-added: two in-flight PRs each introduce a new reconciler interval using the same idiom (#1375 PAPERCLIP_TERMINAL_GATE_RECONCILER_INTERVAL_MINUTES, #1309 PAPERCLIP_APPROVAL_ENFORCEMENT_RECONCILER_INTERVAL_MINUTES — the latter also adds a local finiteness helper with no ceiling, which still overflows a timer). Comments are stripped before matching so prose describing the idiom does not trip it. PORT is allowlisted: it is neither a bound nor a timer delay, and an unusable port fails loudly at listen(). Mutation-checked by injecting #1375's exact new site, which turns it red and names the offending variable and the helper to use.
…(BLO-27641) Automates the manual grep from the acceptance criteria, because the fix is otherwise a one-time cleanup of a class that is actively being re-added: two in-flight PRs each introduce a new reconciler interval using the same idiom (#1375 PAPERCLIP_TERMINAL_GATE_RECONCILER_INTERVAL_MINUTES, #1309 PAPERCLIP_APPROVAL_ENFORCEMENT_RECONCILER_INTERVAL_MINUTES — the latter also adds a local finiteness helper with no ceiling, which still overflows a timer). Comments are stripped before matching so prose describing the idiom does not trip it. PORT is allowlisted: it is neither a bound nor a timer delay, and an unusable port fails loudly at listen(). Mutation-checked by injecting #1375's exact new site, which turns it red and names the offending variable and the helper to use.
a13a7bc to
751bfe3
Compare
|
@ally please review the repaired linear head |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 751bfe3
Critical Issues (0)
Important Issues (1)
- [pr-review-toolkit:tests]
server/src/__tests__/approval-enforcement-reconciler-sweep.test.ts:434-439— The scheduler test's fake Drizzle builder still implements the removed.offset()terminal method, whilelistCandidateApprovalsnow ends at.limit(batchSize). The first reconciliation therefore receives the fake{ offset: ... }object ascandidates, rejects beforereleaseFirstis assigned, andstartApprovalEnforcementReconcilerswallows that rejection. The test can still observecallschanging and pass without exercising the in-flight overlap behavior it claims to verify.- Make the fake builder return a promise/thenable from
limit()(or otherwise match the current query-builder terminal), and assert the first reconciliation completes successfully before triggering the second tick. Ensure the controlled promise is actually released and that a rejected sweep fails the test.
- Make the fake builder return a promise/thenable from
Suggestions (1)
- [gstack/review]
server/src/services/approval-enforcement-reconciler.ts:59-68—isApprovalEnforcementDriftConflictonly checkscodeandconstrainton the top-level error. A wrapped Postgres unique-violation error withcauseorconstraint_nameis logged as a failed raise instead of being coalesced as the expected inter-replica race. Match the existing cause-chain/cycle-guard helper used by the sibling dedup reconcilers.
Strengths
- Tenant-scoped policy reads and the
(decidedAt, id)keyset cursor address the two main correctness hazards, including foreign-company policy IDs and shifting/tied candidate sets. - The partial unique index is aligned with the open drift-issue predicate, allowing recurrence after closure while preventing concurrent duplicate issues.
- The pure and embedded-Postgres tests cover malformed payloads, inactive or missing policies, grace periods, legacy and canonical assertion shapes, and the enforcing row rather than the agent mirror.
Recommended Action
- Fix the Important scheduler-test issue before merge.
- Address the Suggestion opportunistically.
|
@ally please re-review PR #1309 at head |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: b42f360
Prior Findings Dispositioned (1)
- prior:751bfe3 important 1 — fixed —
server/src/__tests__/approval-enforcement-reconciler-sweep.test.ts:438-445— The fake query builder now makes.limit()return the controlled promise and assignsreleaseFirst, matching the current paginator terminal. The test releases that promise before asserting the next interval tick at:470-476, so a rejected first sweep cannot silently satisfy the overlap assertions.
Critical Issues (0)
Important Issues (1)
- [native-codex]
server/src/services/approval-enforcement-reconciler.ts:593-607,699-715— The defaultmaxIterationscap permanently starves eligible approvals beyond the newest 10,000 rows. When a sweep reaches a full 50th batch, it logs that older approvals are "outside every sweep's window" and restarts the next run withcursor = null, so an old drifted approval can remain unchecked forever as long as at least 10,000 newer approved/asserted rows remain eligible. This contradicts the reconciler's purpose of detecting every machine-checkable enforcement gap.- Remove the permanent coverage cap, or persist/advance a durable sweep cursor (and make the cursor safe across inserts) so capped work is resumed on the next interval. Add a regression test with more than
batchSize * maxIterationseligible approvals proving the tail is eventually scanned.
- Remove the permanent coverage cap, or persist/advance a durable sweep cursor (and make the cursor safe across inserts) so capped work is resumed on the next interval. Add a regression test with more than
Suggestions (1)
- [gstack/review]
server/src/services/approval-enforcement-reconciler.ts:640-646— A fixed drift issue is never automatically closed when a later pass observes zero drift; the generated issue body says a real fix closes automatically, but this code only logs while drift persists. Either close/resolve the generated issue on a clean pass or change the acceptance text to make the agent-owned closure explicit.
Strengths
- Tenant-scoped policy reads and the partial unique index address cross-company leakage and concurrent duplicate creation.
- The keyset paginator correctly orders by
(decidedAt, id)and includes targeted regression coverage for ties and shifting candidate sets. - Defensive parsing and an explicit grace window keep malformed or freshly approved cards from wedging or flooding the sweep.
Recommended Action
- Fix the Important issue before merge.
- Address the Suggestion or align the issue lifecycle contract.
- Re-run the reconciler and pagination tests.
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: fd89c33
Prior Findings Dispositioned (1)
- prior:b42f360 important 1 — fixed —
server/src/services/approval-enforcement-reconciler.ts:588-602— The formermaxIterationsceiling is gone. The sweep now advances its(decidedAt, id)cursor until the candidate query returns an empty batch or a short batch, so approvals beyond the old 10,000-row boundary are scanned in the same run; the added tail regression covers this behavior.
Critical Issues (0)
Important Issues (0)
Suggestions (2)
- [gstack/review]
server/src/services/approval-enforcement-reconciler.ts:59-79— The unique-violation helper now correctly walks wrapped causes and acceptsconstraint_name; consider adding a focused regression test for a wrapped23505error so this race-coalescing contract remains protected. - [native-codex]
server/src/services/approval-enforcement-reconciler.ts:425-430— The generated issue says a later clean pass “closes this automatically,” but the reconciler only detects and logs existing drift issues; it never closes them. Either implement explicit closure after a clean observation or make the acceptance/verifying text say that the assigned owner must close the issue.
Strengths
- Tenant-scoped enforcing-policy reads prevent cross-company policy leakage and false drift results.
- Keyset pagination over
(decidedAt, id)handles tied timestamps and candidate-set changes without skips or revisits. - The partial unique index and conflict handling provide a robust duplicate-issue backstop across worker replicas.
- Defensive assertion parsing, the grace window, and the new tail regression address the principal failure modes without mutating enforcement state.
Recommended Action
- No Critical or Important issues block this head.
- Consider the Suggestions opportunistically.
fd89c33 to
92e5653
Compare
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 92e5653
Critical Issues (0)
Important Issues (1)
- [native-codex]
server/src/services/approval-enforcement-reconciler.ts:88-89,595-600,698-714— The stateless sweep still permanently starves approvals older than the newestbatchSize * MAX_ITERATIONSrows. With the defaults, a full 50th batch exits at the cap, logs that older approvals are outside every sweep window, and the next interval resetscursortonull, so an eligible drifted approval can remain unchecked indefinitely while at least 10,000 newer eligible approvals exist. This contradicts the reconciler's stated purpose of detecting machine-checkable enforcement gaps.- Remove the cap, or persist/advance a durable sweep cursor across runs so capped work is resumed safely. Add regression coverage proving an eligible tail beyond the cap is eventually scanned.
Suggestions (2)
- [gstack/review]
server/src/services/approval-enforcement-reconciler.ts:59-82— The wrapped unique-violation helper is now consistent with the sibling cause-chain handling; add a focused regression for a wrapped23505whose constraint is exposed only through a nested cause if future error-shape changes are expected. - [native-codex]
server/src/services/approval-enforcement-reconciler.ts:466-483— The cursor comment still names only the decide path even though multiple approval mutations writedecidedAt; describing the invariant as “everydecidedAtwrite inapprovals.tsuses a JSDate” would be more durable.
Strengths
- Tenant-scoped policy reads prevent foreign-company policy IDs from leaking enforced amounts or suppressing drift detection.
- The
(decidedAt, id)keyset pagination handles tied timestamps and candidate-set changes without OFFSET skips or revisits. - The partial unique index and wrapped-conflict handling provide a sound duplicate-issue backstop across worker replicas.
- The canonical assertion shape, legacy replay support, grace-window parsing, and embedded-Postgres coverage address the principal detection and rollout hazards.
Recommended Action
- Fix the Important coverage issue before merge.
- Re-run the reconciler and pagination tests, including a tail-beyond-cap regression.
- Consider the Suggestions opportunistically.
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 51630aa
Prior Findings Dispositioned (1)
- prior:92e5653 important 1 — still-present —
server/src/services/approval-enforcement-reconciler.ts:694— The current head still does not provide a valid fix for the prior pagination-coverage blocker: the sweep reaches a post-loop iteration-cap check, butmaxIterationsis not declared anywhere in the file, so the reconciler cannot compile or execute its intended full-coverage behavior.
Critical Issues (0)
Important Issues (1)
- [native-codex]
server/src/services/approval-enforcement-reconciler.ts:694—maxIterationsis referenced but never declared in the module. This is a TypeScript compile error, so the new reconciler cannot build or start.- Remove the stale cap check and its unreachable warning, or define and correctly exercise an explicit cap with tested coverage. The current implementation claims to scan beyond the former ceiling but still contains this undeclared reference.
Suggestions (0)
Strengths
- Tenant-scoped policy reads, keyset pagination, wrapped unique-conflict handling, and the scheduler overlap regression are covered directly in the changed code and tests.
Recommended Action
- Fix the compile error and re-run the reconciler and migration test suites.
- Re-submit a fresh exact-head review after the corrected tree is available.
|
Fixed Ally's Important finding in follow-up commit The reviewed head referenced undeclared Please re-review the exact current head |
|
@ally please re-review PR #1309 at exact current head |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 61c4542
Prior Findings Dispositioned (1)
- prior:51630aa important 1 — fixed —
server/src/services/approval-enforcement-reconciler.ts:591-694— The undeclaredmaxIterationsreference and post-loop cap warning are gone. The sweep now keeps advancing the(decidedAt, id)keyset cursor until it receives an empty or short batch, and returns after that exhaustive traversal. The tail regression atserver/src/__tests__/approval-enforcement-reconciler-sweep.test.ts:598-619seeds 101 cards withbatchSize: 2and assertsscanned: 101,drifted: 1, andraised: 1.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- Tenant-scoped enforcing-policy reads prevent foreign-company policy IDs from leaking amounts or suppressing drift detection.
- Keyset pagination over
(decidedAt, id)handles tied timestamps and candidate-set changes without OFFSET skips or revisits. - The partial unique index and wrapped-conflict handling provide a sound duplicate-issue backstop across worker replicas.
- Defensive assertion parsing, the grace window, canonical and legacy payload support, and the 101-card tail test cover the principal detection and rollout hazards.
- The scheduler regression now exercises the current query-builder terminal and the current head passes the available review check.
Recommended Action
- No Critical or Important issues block this head.
- Merge when the repository's normal CI and policy gates are satisfied.
|
Track A landing 2026-09-06: not merged — Tracking issue: BLO-32250 — rebase onto current This PR is deliberately left open rather than replaced, so the existing Ally review ledger stays attached to #1309. |
Resolves BLO-32250: #1309 was DIRTY against master (744 commits behind). Conflicts and how they were resolved: - packages/db/src/migrations/meta/_journal.json + the migration file: master shipped its own 0221 (0221_repair_cross_company_issue_projects) and has since reached 0239. Renumbered our migration 0221 -> 0240 and re-appended the journal entry at the tail. The migration test matches by filename suffix, so the rename needs no test change. - packages/shared/src/validators/approval.ts: both sides edited the same object literal. Kept master's `gate` field AND our `.describe()`. - server/src/config.ts, server/src/index.ts: both sides appended new worker-tier reconciler blocks. Union, no semantic overlap. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: c976828
This head is a merge of origin/master (745 commits) onto the previously-reviewed tree. The 12 PR-owned files are unchanged by the merge; review focused on the merge's interaction with the new master-side migration tooling, plus a fresh pass over the reconciler.
Prior Findings Dispositioned (1)
- prior:92e5653 important 1 — fixed —
server/src/services/approval-enforcement-reconciler.ts:591-602,691— The iteration ceiling is gone. The sweep is now an unboundedwhile (true)that advances the(decidedAt, id)keyset cursor each pass and terminates only on an empty batch (:594) or a short batch (:602,:691), so an eligible tail can no longer sit permanently outside every sweep window. The 101-card /batchSize: 2regression assertingscanned: 101is present atserver/src/__tests__/approval-enforcement-reconciler-sweep.test.ts:598-619.
Critical Issues (0)
Important Issues (1)
- [native-codex]
server/src/services/approval-enforcement-reconciler.ts:658—assigneeAgentId: approval.requestedByAgentId ?? undefinedsilently produces an unassignedtodoissue whenever the drifted approval has no requesting agent.approvals.requestedByAgentIdis nullable (packages/db/src/schema/approvals.ts:13— no.notNull()), and board- or human-filed approvals are exactly the null case.issueService.createdoes not substitute an owner: it only rejects a missing assignee forin_progress, so atodorow is created and persisted with no assignee. Heartbeat work selection is by assignee, so that issue appears in no agent's inbox and has no wake path — it is created, counted inraised, and never worked.- The dedupe path then makes this permanent rather than self-healing: the row stays
todo(notdone/cancelled), so bothfindOpenDriftIssue(:635) and the partial unique indexissues_active_approval_enforcement_drift_uqkeep matching it. Every later pass logsdrift persists, open issue already tracks it(:637-641) and re-raises nothing. The reconciler therefore reports the drift as tracked while the enforcement gap stays open indefinitely — the same silent-failure shape this reconciler exists to detect, reproduced in its own output. - Route the null case to a real owner rather than to nobody: fall back to a company owner/board-triage agent, or set
assigneeUserIdfor a human-filed approval, so the raised issue is selectable. - No test covers this branch.
server/src/__tests__/approval-enforcement-reconciler-sweep.test.ts:146,180seedsrequestedByAgentId: agentIdand assertsassigneeAgentIdequals it; add a null-requester case asserting the raised issue is reachable.
- The dedupe path then makes this permanent rather than self-healing: the row stays
Suggestions (2)
- [pr-review-toolkit:comments]
server/src/services/approval-enforcement-reconciler.ts:467-470— Two overlapping clauses appear to have been spliced during an edit, leaving an ungrammatical sentence: "…for a narrower reason than "the round-trip is avoided" — every write todecided_atis a JSDate/ everydecidedAt:write inapprovals.tsbinds a JSDate, and the column carries nonow()default, / so the stored values have no sub-millisecond component…". The surrounding block documents a load-bearing keyset invariant, so it is worth leaving clean — drop the first fragment and keep theapprovals.tsphrasing. - [gstack/review]
server/src/config.ts:894— The interval still parses withNumber(...) || 60while the grace window on:901uses the newnumericEnvhelper introduced for exactly this hazard. A zero interval is not meaningful, so folding0into the default is defensible, but an explicit0yields60here versus theMath.max(1, …)clamp the code otherwise implies. UsingnumericEnvfor both would make the file's parsing rule uniform.
Strengths
- Migration
0240survives the master merge cleanly:_journal.jsonat this head has 233 entries,idxmonotonic, no duplicateidxortag, and0240_approval_enforcement_drift_indexlands last atidx: 240behind master's0239. It also does not collide with the new master-sideconcurrent-index-guard, whosePENDING_CONCURRENT_INDEXESis an explicit opt-in registry rather than a lint on plain index DDL. - The drizzle index in
packages/db/src/schema/issues.ts:213-220matches the raw SQL in0240exactly — same columns and same four-clause partial predicate — so schema and migration cannot drift. - Tenant-scoped policy reads (
:623-624) group by company before lookup, so a payload naming a foreign policy id cannot read another tenant's amount or mask drift. - Keyset pagination over
(decidedAt, id)handles tied timestamps and a candidate set shifting mid-sweep without OFFSET skips or revisits, and the comment at:471-481documents precisely which future edit would break it. - The scheduler guards overlap with an
inFlightlatch and swallows sweep errors without killing the interval (:708-719). - The raise path is read-only with respect to approvals and budget policies, and coalesces onto a concurrent replica's issue via the wrapped
23505handler (:673-682) rather than retrying.
Recommended Action
- Fix the Important assignee issue before merge — an unassigned
todois unreachable and the dedupe path makes it permanent. - Add the null-requester regression alongside it.
- Consider the Suggestions opportunistically.
|
Rebase complete — new head Follow-up to my Track A note of 2026-09-06T05:11Z, which recorded this PR as
What actually blocks this PR now is not the rebase.
No I have not pushed to this branch — the fix is a design call about which owner a null-requester drift issue should route to, and it belongs with BLO-24631. Not re-reviewing either: the attestation at this head is current and well-formed, and one verdict per head stands. |
1 participant
Thinking Path
Linked Issues or Issue Description
reconciler,approval, andenforcementin title across all states. Related but distinct — Add stranded-blocked-issue reconciler (BLO-21523 phase 1) #1093/Fix stranded-blocked reconciler eligibility (BLO-21523) #1112 (stranded-blocked-issue reconciler, different subject), fix(approvals): authorize issueIds on approval create (BLO-23763) #1271 (approval create authorization), fix(approvals): decide both approval-link doors through one evaluator (BLO-24699) #1293 (approval-link evaluation). None reconcile decided vs enforced state.What Changed
server/src/services/approval-enforcement-reconciler.ts— a worker-tier periodic sweep. For each approved card carrying a machine-checkable assertion it re-reads the enforcing object and raises one deduped issue per drifted approval, routed to the requesting agent.budget_policies.amount— the rowbudgetService's hard-stop gate actually consults. Explicitly notagents.budget_monthly_cents, which read $36,800 for an agent whose enforced cap was $19,000. The sweep test seeds a deliberately disagreeing mirror so a regression to reading it fails the suite.companyId; a foreign policy id resolves tomissing_policyrather than being read.payload.enforcement_assertions: [{kind, policyId, expected_usd | expected_amount_cents, label}], published on thecreateApprovalpayload schema's.describe()so it reaches agents through the MCP tool schema rather than living in a file nobody reads; plus legacypayload.exact_changes, the ad-hoc shape the CEO agent actually emitted — supported because it is what the historical cards carry, which is what makes them a regression fixture rather than a rewrite.approvals.payloadis free-form jsonb (approvalPayloadSchemarequires onlytitle), so every field read is defensive and one bad card cannot wedge the sweep for every other card. Unknown assertion kinds are ignored. Prose-only cards yield zero assertions and are skipped — not silently reported as agreeing.PAPERCLIP_APPROVAL_ENFORCEMENT_RECONCILER_{ENABLED,INTERVAL_MINUTES,GRACE_HOURS}. A newnumericEnv()helper makes an explicit0grace configurable (Number(x) || 6folded0into the default, making the documentedMath.max(0, ...)unreachable).0235_approval_enforcement_drift_index.sql— partial unique index scoped to the open population (hidden_at IS NULL,status NOT IN ('done','cancelled')) so a recurrence can file a fresh issue after the previous one is closed, and so issue creation is safe across worker replicas.Verification
Run from the repo root:
Measured on merge commit
3f87d900:recovery-classifiers,recovery-observability,config-recovery-action-bounds)check:migrations(numbering + safety)tsc --noEmitonserverandpackages/dbThe acceptance criterion is exercised in both directions, as a state transition in one test: the reconciler stays silent when decided and enforced agree, fires when a policy amount is deliberately reverted in a throwaway company, and returns to silent when reapplied. Grace-window, idempotency, inactive-policy, and recurrence-after-close cases are covered separately.
Regression fixture is real data, not synthetic. Card
6f45844ecarries all 8 policy ids withfrom_usd(pre-fix enforced) andto_usd(decided).enforced := from_usdreproduces 8/8 historical drift including CTO decided $32,000 / enforced $19,000;enforced := to_usdis silent. No DB required.Risks
0218is additive: oneCREATE UNIQUE INDEX ... IF NOT EXISTSon a partial predicate, no data change, no column drop. It replays idempotently (asserted by test) and passes the repo'scheck-migration-safetybaseline. Renumbered from0214in this branch because master landed0214–0217while the PR was open.exact_changeswhose figures no longer match. That backlog is the point, but watch the first sweep'sraisedcounter rather than being surprised. If a first-run cap or dry-run mode is wanted, that is the change to make before merge — flagged for reviewer pushback.PAPERCLIP_APPROVAL_ENFORCEMENT_RECONCILER_ENABLED=false. No existing path changes behaviour.parseJsonBodyStrictships ready for them because those will be HTTP-backed and the API root is an SPA catch-all that answers 200-with-HTML for any path (a probe by status code concludes the opposite of the truth in both directions).Model Used
Claude Opus 5 (
claude-opus-5[1m], 1M context), extended thinking, with tool use and code execution, via Claude Code.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue templatecreateApprovalpayload schema's.describe(), which is how it reaches agentsreviewgate failure this description fixes was the last known blockerOpen decision, recorded not coded
The issue's fifth acceptance criterion asks whether requesting agents should hold a narrow writable route for the classes they are trusted to request. Recorded on the issue as
writable-route-decision: no blanket writable route — for the budget class that route is spend authority, and the round-trip was never the defect; the unverified round-trip was. Conditional yes to a narrower "execute an approved card's own recorded figures" executor, gated on this landing and on board sign-off, since it touches spend governance.🤖 Generated with Claude Code