docs: record #579 as won't-fix — suspension is immediate for use, not issuance - #582
Conversation
The check-then-mint window in login stays open by decision (#532, re-confirmed on #579): a FOR SHARE on the account row in login's issuance transaction would put a lock + round trip on every login to prevent a credential that is inert by construction. The decision record names the four premises its inertness rests on, so a refactor breaking any one reopens #579 instead of silently widening the window. Closes #579.
|
You have reached your Codex usage limits for security reviews. Please try again later. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38dbc161c2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ing claim P1: state in the record that this is an accepted-risk decision, not an incident-earned rule, so the AGENTS.md link shape reads correctly against the repo's own meta-rule. P2: the 'every locking path takes the account row first' claim was false — the currency-lock protocol (UpdateInventoryItemHandler, RecordPurchaseHandler) is item-first/account-second. Narrow the no-deadlock argument to the suspension-side paths, which is all the conclusion needs, and name the protocol as the counterexample with its own cycle-freedom basis.
|
@codex please review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd4a420bfb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex round 2 (P2) was right that the same-transaction premise had no
dedicated guard: the existing SuspendAsync_BumpsEveryEpoch test only observes
the successful final state. The guard is two halves, and the split is load
bearing — it came out of running the mutation:
* runtime half: faults the audit write (the last in-transaction write
before the final SaveChanges) and asserts the rollback reverted the
account row, every epoch/stamp, the token and the audit row.
* static half: no SaveChanges call before the final flush in
AccountSuspensionService.cs.
The mutation (an extra SaveChanges after the account mutation) stayed GREEN
on the runtime half — a split-FlushChanges still commits in one Postgres
transaction, so no fault injection can catch it. The static check is the only
thing that reddens on it; verified red, then restored.
Docs: the decision record's premise list now claims each premise is pinned by
a guard that fails when THAT premise alone breaks, with the per-premise guard
table, and says plainly that the window itself is unguarded by design.
9818495 to
d36fdcf
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d36fdcf9cb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ransaction-boundary guard Three findings, all addressed: 1. P1 (meta-rule): AGENTS.md's 'linked rule = earned by a defect' binary is false the moment an accepted-risk record exists. The meta-rule now names both kinds — earned (defect narrative) and accepted-risk (declination, load-bearing) — and says the record says which (No incident + load-bearing vs. the defect narrative). 2. P2 (premise-3 scope): 'nothing minted before or during the suspension survives it' is false for the window artifact — a refresh row minted AFTER the sweep survives the suspension transaction. That is the central artifact of the record, and the record was claiming premise 3 destroys it. Premise 3 now says 'for every credential that exists when the sweep executes' and names the other three premises + reactivation as what makes the post-sweep row inert-then-destroyed. AGENTS.md parity updated. 3. P2 (guard scope): the flush-count check is gameable — commit the account in one transaction, run the sweep in a second, and the file still carries exactly one SaveChangesAsync. The static half now checks the TRANSACTION BOUNDARY instead: the sweep (ExecuteUpdateAsync) and the flush (SaveChangesAsync) must both sit positionally between AmbientTransaction.RunAsync and transaction.CommitAsync. Verified by mutation: the two-transaction variant (sweep moved after the commit) reddens the sweep-position assertion; the flush-count predecessor let it pass. Needles are explicit and the failure messages say how to update them when the code shape legitimately changes.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b44580f18a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // a different transaction than the account row. | ||
| var begin = serviceSource.IndexOf("AmbientTransaction.RunAsync", StringComparison.Ordinal); | ||
| var commit = serviceSource.IndexOf("transaction.CommitAsync", StringComparison.Ordinal); | ||
| var sweep = serviceSource.IndexOf(sweepNeedle, StringComparison.Ordinal); |
There was a problem hiding this comment.
Fresh evidence beyond the earlier transaction-boundary review is that AccountSuspensionService.cs contains two ExecuteUpdateAsync calls—the user epoch/stamp sweep at line 175 and the refresh-token sweep at line 187—while this generic IndexOf identifies only whichever appears first. If the user sweep moves after CommitAsync into a second transaction while the refresh sweep remains inside the first, this assertion still finds an in-boundary call; the injected audit fault prevents the moved code from running, and successful-path tests observe the eventual final state, so the entire guard stays green even though premise 3 is broken. Match the specific db.Users update rather than any ExecuteUpdateAsync.
AGENTS.md reference: AGENTS.md:L139-L143
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed at b57e506. The needle was the generic "ExecuteUpdateAsync", which IndexOf matches at whichever of the file's two ExecuteUpdateAsync calls appears first. A refactor moving only the USER epoch/stamp sweep after the commit — leaving the refresh sweep inside — passed the boundary check on the refresh sweep's position while premise 3 was broken.
The user sweep needle is now "db.Users" with a 400-char forward window confirming an ExecuteUpdateAsync in the same statement, verified by mutation (user sweep moved after commit reddens the boundary assertion; the generic predecessor let it pass). Round 5 then correctly extended the same treatment to the db.RefreshTokens sweep (00cca23).
The transaction-boundary guard's sweep needle was the generic "ExecuteUpdateAsync", which IndexOf matches at whichever of the file's TWO sweep calls appears first (the user epoch/stamp sweep and the refresh-token sweep). A refactor moving only the USER sweep after the commit — leaving the refresh sweep inside — passes the boundary check on the refresh sweep's position while premise 3 is broken. The needle is now "db.Users" with a 400-char forward window confirming an ExecuteUpdateAsync in the same statement. A bare db.Users elsewhere (a query, a CountAsync) would not carry one. Verified by mutation: moving the user sweep after the commit (refresh sweep stays inside) reddens the boundary assertion; the generic-needle predecessor let it pass.
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b57e506c78
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // a different transaction than the account row. | ||
| var begin = serviceSource.IndexOf("AmbientTransaction.RunAsync", StringComparison.Ordinal); | ||
| var commit = serviceSource.IndexOf("transaction.CommitAsync", StringComparison.Ordinal); | ||
| var sweep = serviceSource.IndexOf(sweepNeedle, StringComparison.Ordinal); |
There was a problem hiding this comment.
After the prior fix anchors the db.Users sweep, this guard still ignores the separate db.RefreshTokens revocation. Moving only that update after transaction.CommitAsync leaves every assertion green: the positional checks still see the user sweep and flush inside the transaction, the injected audit fault prevents the moved code from running and therefore still observes a live token, and the successful-path test eventually observes the token revoked. This permits the documented same-transaction revocation premise to break undetected, so also locate and bound the refresh-token sweep.
AGENTS.md reference: AGENTS.md:L79-L79
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed at 00cca23. The round-4 fix anchored the db.Users epoch/stamp sweep but the second ExecuteUpdateAsync — the db.RefreshTokens revocation — was ignored. Moving only the token sweep after the commit left the user sweep + flush in-boundary: green on every prior assertion, with premise 3's token revocation broken.
The guard now anchors and bounds BOTH sweeps between AmbientTransaction.RunAsync and transaction.CommitAsync, each with its own receiver needle and 400-char forward window confirming the ExecuteUpdateAsync in the statement. Verified by mutation: moving only the refresh sweep after the commit reddens the token-sweep boundary assertion; the user-sweep-only predecessor let it pass.
…ction The round-4 fix anchored the db.Users epoch/stamp sweep but ignored the second ExecuteUpdateAsync — the db.RefreshTokens revocation. Moving only the token sweep after the commit leaves the user sweep + flush in-boundary: green on every prior assertion, with the token revocation broken (the documented premise-3 same-transaction revocation). The guard now anchors and bounds BOTH sweeps between AmbientTransaction.RunAsync and transaction.CommitAsync, each with its own receiver needle and 400-char forward window confirming the ExecuteUpdateAsync in the statement. Verified by mutation: moving only the refresh sweep after the commit reddens the token-sweep boundary assertion; the user-sweep-only predecessor let it pass.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 00cca23aad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| var sweepIsUpdate = sweep >= 0 && | ||
| serviceSource.Substring(sweep, Math.Min(sweepWindow, serviceSource.Length - sweep)).Contains(sweepMethod, StringComparison.Ordinal); | ||
| var tokenSweep = serviceSource.IndexOf(tokenSweepNeedle, StringComparison.Ordinal); | ||
| var tokenSweepIsUpdate = tokenSweep >= 0 && | ||
| serviceSource.Substring(tokenSweep, Math.Min(sweepWindow, serviceSource.Length - tokenSweep)).Contains(sweepMethod, StringComparison.Ordinal); |
There was a problem hiding this comment.
Fresh evidence in the final guard is that it bounds only the db.Users/db.RefreshTokens receiver offsets, while merely checking that an ExecuteUpdateAsync occurs somewhere in the following 400 characters. A refactor can capture those query roots before CommitAsync but defer the awaited updates until immediately afterward; both substring checks and position assertions remain green, and the fault-injection half still exits before the split, although the revocations now run outside the suspension transaction. Locate and bound each ExecuteUpdateAsync call itself rather than its receiver.
AGENTS.md reference: AGENTS.md:L139-L143
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed at 1949410. The guard bounded the db.Users/db.RefreshTokens receiver offsets and only checked an ExecuteUpdateAsync occurred within 400 chars. A deferred-await refactor — capture the query root (db.Users … where …) inside the transaction, await the ExecuteUpdateAsync only after the commit — kept the receiver offset in-boundary while the revocation ran outside it.
The guard now locates each receiver's own ExecuteUpdateAsync( call (the first call after the receiver; the user's further constrained to sit before the db.RefreshTokens receiver, which defeats a first-match-on-the-wrong-sweep) and bounds THAT offset to the transaction span (between AmbientTransaction.RunAsync and transaction.CommitAsync).
Verified by a three-mutation matrix:
- user sweep moved after the commit (refresh stays inside) → red
- refresh sweep moved after the commit (user stays inside) → red
- deferred user await (root captured in the transaction, ExecuteUpdateAsync awaited after the commit — exactly the threat described) → red
- base → green
| | 1 — live middleware read | `SuspendedAccount_MiddlewareRefusesEveryAuthenticatedRequest` (plus the #364 no-cache tests) | middleware stops reading `IsActive` (or caches it) while login still checks | | ||
| | 2 — `RefreshAsync`'s suspended check | `SuspendedAccount_RefreshAsyncRefusesTheWindowMintedToken` | that block is deleted while the middleware stays live | |
There was a problem hiding this comment.
A repo-wide search finds both names in this table only; the implemented guards are ABearerWhoseEpochStillMatches_IsRejected_WhenTheFarmIsInactive and ARefreshTokenWhoseEpochStillMatches_IsRejected_WhenTheFarmIsInactive at lines 507 and 609 of AccountSuspensionTests.cs. Because this accepted-risk record relies on readers finding the named tripwires, update these entries to the real test names.
AGENTS.md reference: AGENTS.md:L79-L79
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed at 1949410. The record's premise table named two tests that do not exist (SuspendedAccount_MiddlewareRefusesEveryAuthenticatedRequest, SuspendedAccount_RefreshAsyncRefusesTheWindowMintedToken); a repo-wide search found both names in the table only. Corrected to the real guards:
- Premise 1 (live middleware read) → ABearerWhoseEpochStillMatches_IsRejected_WhenTheFarmIsInactive — the bearer's epoch still matches, so the epoch clause cannot reject it; only the middleware's AccountIsActive clause can. Delete that clause and it 200s.
- Premise 2 (RefreshAsync's suspended check) → ARefreshTokenWhoseEpochStillMatches_IsRejected_WhenTheFarmIsInactive — the IssuedEpoch still equals the user's CredentialEpoch, so the epoch check in RefreshAsync passes; only the suspended-farm check refuses the rotation.
Premises 3 and 4 already referenced tests that exist (SuspendAsync_SuspendedPremiseIsAtomic_RollbacksWithTheEpochBump, ReactivationRevokesTheSessionsMintedBetweenSuspendAndReactivate) and are unchanged. The table's premise-1/2 "fails when" cells now name the load-bearing clause in each, and premise 3's cell + body text now describe the actual transaction-boundary + both-sweeps + call-anchoring check rather than the superseded flush-count guard.
Round 6, two findings, both addressed: 1. P2 (bound the calls, not the receivers): the guard bounded the db.Users/db.RefreshTokens receiver offsets and only checked an ExecuteUpdateAsync occurred within 400 chars. A deferred-await refactor could capture the query root inside the transaction and await the ExecuteUpdateAsync only after the commit; the receiver offset stayed in-boundary while the revocation ran outside it. The guard now locates each receiver's own ExecuteUpdateAsync( call (the first call after the receiver, the user's further constrained to sit before the refresh receiver) and bounds THAT offset to the transaction span. Verified by a three-mutation matrix — user sweep after commit, refresh sweep after commit, and the deferred-await variant — all redden; base stays green. 2. P2 (guard names): the record's premise table named two tests that do not exist (SuspendedAccount_MiddlewareRefusesEveryAuthenticatedRequest, SuspendedAccount_RefreshAsyncRefusesTheWindowMintedToken). A repo-wide search found both names in the table only. Corrected to the real guards: ABearerWhoseEpochStillMatches_IsRejected_WhenTheFarmIsInactive (premise 1, the middleware AccountIsActive gate — the epoch still matches so only that clause can reject) and ARefreshTokenWhoseEpochStillMatches_IsRejected_When TheFarmIsInactive (premise 2, RefreshAsync's suspended-farm check). Premise 3's 'fails when' row and body text now describe the actual transaction-boundary + both-sweeps + call-anchoring check rather than the superseded flush-count guard.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 194941059b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ? serviceSource.IndexOf(sweepMethod, tokenReceiver, StringComparison.Ordinal) | ||
| : -1; | ||
| var tokenSweepIsUpdate = tokenSweep >= 0 && tokenSweep - tokenReceiver < sweepWindow; | ||
| var flush = serviceSource.IndexOf(flushNeedle, StringComparison.Ordinal); |
There was a problem hiding this comment.
Fresh evidence in the final guard is that flush identifies only the first SaveChangesAsync and never ties it to the tracked account.Suspend() mutation. Moving that mutation after the existing in-transaction save/commit and adding a second save there would commit the revocation sweeps first and IsActive separately, while every positional assertion still passes; the injected audit fault exits before the moved mutation, and successful-path tests observe only the eventual final state. The guard therefore still misses the exact split-transaction regression it claims to prevent; verify structurally or at runtime that the account update itself is flushed before this commit.
AGENTS.md reference: AGENTS.md:L139-L143
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed at ecf0549. The flush needle found only the FIRST SaveChangesAsync and never tied it to the tracked account.Suspend()/Reactivate() mutation. A split-transaction refactor could move that mutation to after the in-transaction save + commit and add a SECOND save there — committing the revocation sweeps first and IsActive separately — while every positional assertion on the first save still passes; the runtime fault exits before the moved mutation and the successful-path tests observe only the eventual final state.
Two structural checks close this:
- the account mutation (account.Suspend()/Reactivate()) must sit BEFORE the in-transaction flush, so the tracked account row is dirty when the suspension transaction's save runs;
- NO SaveChangesAsync may appear after the commit (the account row must not be re-flushed in a second transaction).
Verified by mutation: moving the account mutation after the commit with a second save there reddens the new checks. The sweep-deferral matrix (user sweep after commit, refresh sweep after commit, deferred user await) stays green on base and red on each variant. The record's premise-3 table cell and body text now name the account-mutation-deferral variant alongside the sweep variants.
Round 7: the guard's flush needle found only the FIRST SaveChangesAsync and never tied it to the tracked account.Suspend()/Reactivate() mutation. A split-transaction refactor could move that mutation to after the in-transaction save + commit and add a SECOND save there — committing the revocation sweeps first and IsActive separately — while every positional assertion on the first save still passes; the runtime fault exits before the moved mutation and the successful-path tests observe only the eventual final state. Two structural checks close this: - the account mutation (account.Suspend()/Reactivate()) must sit BEFORE the in-transaction flush, so the tracked account row is dirty when the suspension transaction's save runs; - NO SaveChangesAsync may appear after the commit (the account row must not be re-flushed in a second transaction). Verified by mutation: moving the account mutation after the commit with a second save there reddens the new checks; the sweep-deferral matrix (user sweep after commit, refresh sweep after commit, deferred user await) stays green-on base and red-on-each-variant. The record's premise-3 table cell and body text now name the account-mutation-deferral variant alongside the sweep variants.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ecf0549fe4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| | [Production logs: compact JSON on stdout (#404)](404-production-logs.md) | AGENTS · Conventions | | ||
| | [Generated PostgreSQL schema documentation (#417)](417-schema-docs.md) | AGENTS · Conventions | | ||
| | [`AuditEvents` is not time-partitioned (#505)](505-audit-events-no-time-partition.md) | AGENTS · Conventions | | ||
| | [Suspension is immediate for use, not issuance (#579)](579-suspension-issuance-window.md) | AGENTS · Conventions | |
There was a problem hiding this comment.
Fresh evidence after the earlier AGENTS.md fix is that this new #579 entry is now indexed while this file's own introduction still describes every record as rationale that was “earned” by something that shipped (lines 3–14), and its footer still says link presence distinguishes incident-backed rules from conventions without incidents (lines 52–57). That leaves contributors with two conflicting explanations of what this index contains; update those passages to distinguish earned records from linked accepted-risk records such as #579.
AGENTS.md reference: AGENTS.md:L11-L20
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed at 2475829. The introduction described every record as rationale "earned" by something that shipped, and the footer said link presence distinguishes incident-backed rules from conventions without incidents — two conflicting explanations once the #579 (accepted-risk) entry was indexed.
Both passages now say a linked bullet is one of two kinds: an earned record (a defect that shipped — the narrative names the incident and the review round) or an accepted-risk record (no incident — the record says "No incident" and the rule is load-bearing, e.g. #579). The plain conventions carry no link at all. The record's own "Incident" / "No incident" line is what distinguishes the two linked kinds.
Round 8: the decisions/README introduction described every record as rationale that was 'earned' by something that shipped, and the footer said link presence distinguishes incident-backed rules from conventions without incidents. The #579 entry is now indexed, but it is an accepted-risk record (no incident, a deliberate declination) — the two passages left contributors with two conflicting explanations of what the index contains. Both passages now say a linked bullet is one of two kinds: an earned record (a defect that shipped — the narrative names the incident and the review round) or an accepted-risk record (no incident — the record says 'No incident' and the rule is load-bearing, e.g. #579). The plain conventions carry no link at all.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24758298f0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Assert.True(sweep > begin && sweep < commit && (tokenReceiver < 0 || sweep < tokenReceiver), | ||
| "#579 premise 3: the epoch/stamp sweep is outside the suspension transaction (between BeginAsync and CommitAsync). " + | ||
| "Premise 3 of docs/decisions/579-suspension-issuance-window.md is one Postgres transaction around IsActive and the sweep; " + | ||
| "if the sweep must leave the transaction, the premise is broken and #579 reopens — do not move the needle to make this green."); |
There was a problem hiding this comment.
Fresh evidence in the final guard is that these assertions still equate the textual position of ExecuteUpdateAsync with when it executes. A refactor can define each sweep as a local lambda before CommitAsync and invoke it afterward; the receiver and call offsets remain inside the accepted span, the fault-injection path exits before the deferred calls, and successful-path tests observe the eventual final state, so every assertion stays green despite a crash after the account commit leaving credentials unrevoked. After repeated misses of this shape, use a structural or runtime guard that proves the operations execute before the commit rather than extending the substring parser.
AGENTS.md reference: AGENTS.md:L139-L145
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Accepted at 52f670b. You are right, and the meta-rule (two misses of the same shape mean the method is wrong) applies: after four rounds of extending the substring parser, the guard still equates the textual position of the sweep calls with when they execute. A refactor that wraps each sweep in a local lambda defined before the commit and invoked after it evades every offset check.
Rather than extend the parser a fifth time pretending it is sound, I've done two things:
- The guard's comment now carries an HONEST LIMITATION block naming the lambda-wrap evasion and stating the guard is best-effort against the realistic refactor shapes (moving a statement, each verified by mutation on this slice — the mutation matrix is in the review thread).
- The record's premise-3 body states the same limitation: closing the evasion needs a production test seam (a hook so a runtime guard can observe execution order), which is out of scope for a won't-fix decision record. A lambda-wrap refactor leaves the premise on the runtime half + code review, and the guard's comment must be revisited.
I deliberately did NOT add the production test seam here — that is a non-trivial production change to AccountSuspensionService for a won't-fix decision, and the right response to "the method is wrong" is to name the limit honestly, not to silently extend a parser that has been evaded four times. If the service is later refactored to wrap the sweeps, the limitation block is the tripwire to revisit the guard.
| (#579)). `AGENTS.md` says a bullet is linked by whether it carries one; the | ||
| record's own `Incident` / `No incident` line says which of the two linked kinds | ||
| it is. |
There was a problem hiding this comment.
This says each linked record has an Incident / No incident line, but a repo-wide search finds those markers only in this README: the new #579 record has a status line and prose saying there was no defect, not the promised No incident line. Consequently readers cannot classify the first accepted-risk record using the mechanism this index and the canonical rule now specify; add the marker to the record or stop describing it as a formal line.
AGENTS.md reference: AGENTS.md:L14-L18
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed at 52f670b. You are right that the marker existed only in the README — a repo-wide search finds it in no record. Two-part fix:
- The Suspension: login can still mint an inert credential in the check-then-mint window #579 record now carries an explicit "No incident — accepted-risk record" marker line, so the first accepted-risk record is classifiable using the mechanism the index specifies.
- The README footer no longer promises every earned record carries an "Incident:" line (they don't). It now says an accepted-risk record carries the "No incident" marker, and an earned record's narrative names the shipped defect and the review round that found it — which is how the existing earned records actually distinguish themselves.
…t limit Round 9, two P2s: 1. The README/AGENTS now say a linked record is classified by its 'Incident'/'No incident' marker, but a repo-wide search found that marker in no record — the #579 record had a status line and prose, not the promised line. Add a 'No incident — accepted-risk record' marker to the #579 record (so the first accepted-risk record is classifiable), and stop promising every earned record carries an 'Incident:' line (they don't); the README footer now says an accepted-risk record carries the marker and an earned record's narrative names the defect and review round. 2. Meta-rule (two misses of the same shape mean the METHOD is wrong): after four rounds of extending the substring parser, the guard still equates the TEXTUAL position of the sweep calls with when they EXECUTE. A refactor that wraps each sweep in a local lambda defined before the commit and invoked after it evades every offset check. Closing that evasion needs a production test seam (a hook so a runtime guard can observe execution order), which is out of scope for a won't-fix decision record. Rather than extend the parser a fifth time pretending it is sound, the guard's comment and the record's premise-3 body now state the honest limitation: best-effort against the realistic refactor shapes (moving a statement, each mutation-verified); a lambda-wrap refactor leaves the premise on the runtime half + code review, and the comment must be revisited.
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
2 participants
Closes #579
Slice: #579 (epic #530, T6a) — the suspension check-then-mint window.
Decision: Horn B — document, don't fix. No product code changes; no user-visible behaviour changes.
What this changes
docs/decisions/579-suspension-issuance-window.md(new) records the declination: a login that passes theIsActivecheck can still mint a credential that a concurrent suspension revokes in the same transaction. The credential is inert by design, and the record pins why:FOR SHARElock on hot-path cost; this record says so plainly and states what the narrower guarantee is.RefreshAsync's suspended-farm check (refresh tokens refuse), suspension's same-transaction epoch/stamp revocation (old credentials fail the epoch check), and reactivation's revoke (no lingering valid token after the window closes). The record names each premise's guard test, and each premise's claim is that its guard fails when that premise alone breaks — the per-premise guard table, including premise 3's two-half atomicity guard (fault-injection + a static no-early-SaveChangescheck). The window itself is unguarded by design — that is the declination.AGENTS.mdgains the one-paragraph rule (accepted-risk, not an earned-rule link, with the meta-rule's own escape hatch cited); the index row is added.tests/Cluckwork.Api.IntegrationTests/AccountSuspensionTests.csgainsSuspendAsync_SuspendedPremiseIsAtomic_RollbacksWithTheEpochBump— the runtime half (fault the audit write, the last in-transaction write before the finalSaveChanges; assert the rollback reverted the account row, every epoch/stamp, the token, and the audit row) plus the static half (noSaveChangesbefore the final flush inAccountSuspensionService.cs). The split-FlushChanges mutation stayed green on the runtime half — a split still commits in one Postgres transaction — so the static check is the thing that reddens on it; both halves verified by mutation.Why not fix it
Horn A (the
FOR SHARElock, or anAccountIsActiveclaim checked in the middleware) would make the decision record's entire argument wrong: the record is not "the window is harmless because it is tiny", it is "the window produces nothing that works, and making it produce nothing is what the four premises guarantee". A lock that closes the window is a lock that buys a 200 instead of a 403 in a race — on every login, forever — for a window whose worst-case outcome is a user who logs in, gets 403'd on their first real call, and logs in again. #532 declined that trade once with full information; re-litigating it inside a docs PR is out of scope, and the record says where to go if the farm actually sees it (Horn C, a cheap audit counter, is the honest escalation).Verification
main:AuthEndpoints.cs:208,IdentityProvider.cs:227(the Auth: login by farm code, per-account email identity, immediate suspension #532 suspended-farm refresh check),CredentialEpochMiddleware.cs:31-80,AccountSuspensionService.cs(single transaction, four statements),AccountSuspensionTests.cs:464.