SiteAgent 2.13.0 — two-phase site unbind (#434) - #76
Merged
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…enced write/delete, reported by /status (#434) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fix Critical #1: Aura_Worker_Unbind::read() no longer collapses a genuine database read error into "marker absent" — it now mirrors Aura_Worker_Rules::stored_uncached()'s tri-state (array|null|WP_Error, documented not type-hinted, PHP 7.4). is_set() keeps its boolean contract but fails OPEN on a WP_Error (documented); a new is_set_strict() surfaces the WP_Error so a later mutation-boundary task can fail CLOSED. status_fragment() returns null on a WP_Error, since /status is a witness, not a gate. read()'s validity check now also requires `at`. Fix Important #2: removed write_under_claim()'s redundant, non-claim-fenced add_option() pre-step — write_option_if_claimed() already inserts the row (with the given autoload) when absent. Fix Important #3: tests/bootstrap.php now tracks each option's autoload flag in $GLOBALS['_rows_autoload'] (add_option, update_option, and the claim-fenced INSERT/DELETE branches used by write_option_if_claimed() / delete_option_if_claimed()), so autoload='no' has real regression coverage. Minors: the write's read-back now compares a fresh raw read (not self::read()), so a DB blip at verification time reads as "unverified" rather than inheriting read()'s WP_Error handling; documented the site+seq-only comparison; split get_site_url()/get_home_url() onto separate bootstrap globals. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
sa_sign_ruleset / sa_install_gateway_key / sa_token_hash move out of RulesetStoreTest's private methods so every Unbind test file can share them, ahead of the accept()-under-claim feature commit. RulesetStoreTest now delegates to the lifted helpers; behavior is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…answers 503 aura_site_busy (#434) accept() now takes Aura_Worker_Magic_Link::claim_site() before deciding anything and releases it in a finally, so an unbind (Task 3) and an ordinary ruleset push can never interleave. A caller that cannot take the claim gets 503 aura_site_busy, which receive_rules() already transports unchanged. The former accept() body moves verbatim into a private accept_under_claim( $envelope, $attempt, $fence ); the CAS retry branch recurses into accept_under_claim() (not accept()) so the claim is taken exactly once per request and never re-entered. Three RulesetStoreTest contention tests modeled a racing write as a nested Aura_Worker_Rules::accept() call injected mid-write by the $wpdb stub — now blocked by the same claim (correctly: two real concurrent pushes now serialize on it rather than racing at the option layer). Updated to model the race as a direct row mutation via the existing after-store-read/before-swap seams instead, which still exercises the CAS/retry path for the cases where the stored row can change out from under an in-flight decision by some other means. Two blanket _db_query_error tests and a "no write was attempted" query-log assertion needed the same before-swap deferral / narrowing, since the site claim now issues its own INSERT ahead of the ruleset's. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Root cause of C1-C3: Aura_Worker_Magic_Link::claim_magic_link() issues the same SQL shape as insert_if_absent(), so the bootstrap stub's test seams fired on the site claim's own insert instead of the ruleset's, hollowing out seven RulesetStoreTest assertions. Scoped every seam to the ruleset option; converted every racing-write test off nested accept() calls (now structurally blocked by the claim itself) onto direct row mutations via the existing before-swap/after-store-read seams. I1: accept_under_claim() re-verifies holds_site_claim($fence) immediately before every ruleset write (and on each CAS retry, for free, since the retry recurses back into accept_under_claim()) - an evicted handler's write is now refused (503 aura_site_busy), not silently installed. I2: SITE_CLAIM_TAKEOVER_AFTER (120s) lets claim_site() and connect's own claim seize a stale claim via the same conditional compare-and-swap the ruleset store uses - never a blind overwrite. Safe because I1 (and the claim-conditional writes connect/regenerate already had) means a seized original's resumed writes are all refused, not racing the replacement. Per-magic-link claims keep their original no-timed-takeover behaviour. I3: backfill_from_stored_envelope() now also takes the site claim (a cheap, low-risk close of a genuine traced race); clear()'s unclaimed default is left alone since no production caller reaches it. M2/M4/M5 addressed; M1/M3 explicitly deferred with reasoning in the report (not cheap enough for this round). composer test: 965 tests, 3433 assertions, 0 failures. composer lint: 0 errors. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…nd is never seized (Task 2 re-review N1) The docblock and its test promised that a value without a recorded timestamp is never seizable, but only a value with no pipe at all took that path: `abc|xyz` went through `(int)` as age = now and was seized on sight. Validate the suffix with ctype_digit() and pin the guarantee. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…he claim after the existing checks; the marker fast path answers a retry before any signature work (#434) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…hook, the dangerous fail-closed case, the verified append, and peek_payload (#434) I1: the hook registration (name/priority/arity) and capture_app_password() are now driven for real, end to end into the marker — the reviewer's mutant (hook deleted + callback gutted) left all 981 tests green. I3: the case that matters — marker SET, marker read fails, an ORDINARY push — now reddens when step 0's fail-closed branch goes, proving the mutant installs rules on an unbound site. M1: a new bootstrap seam models a claimed write that reports success while the row diverges, so the append's verification re-read is genuinely pinned. M3: the marker write documents why its read-back may be weaker than the append's, and warns Tasks 4/8 off copying it. M4: direct coverage for Grant::peek_payload() — what the fast path trusts to read `final`, which gates Task 4's irreversible token deletion. M2: b64url_decode()'s docblock moved back onto b64url_decode(). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…'s arity regression is visible (Task 3 re-review M7) do_action already sliced arguments to the declared arity; filters did not, and the plugin's arity-sensitive registrations are mostly filters — the core-REST seam Tasks 5/6 extend registers rest_request_before_callbacks at 5, 3. Half-applied fidelity would have left exactly that seam's arity regressions invisible. Also states plainly that the divert seam's INSERT half is unexercised today (M8). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…s last and only on final:true; maybe_finish_unbind() on init, throttled, under the claim, never the token (#434) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…nce of absence (C1); cleanup continues past a failed step (I1); step (3) accounts on the row (M3); the step trace is internal (M4); write_under_claim's guard is observed (M1) (#434) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rd, never inferred from a guessed owner (C2/I2); a malformed marker fails closed instead of reading as absent (I3) (#434) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…cit unknown; Phase B does one authoritative lookup and never searches (C3); /status reports a malformed marker as unbound (I4) (#434) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…k named, never the request's current one (C4); "gone" is never answered from a read that failed (I5); the leftovers travel on the wire (M9); /status's empty fragment is an object (M10) (#434)
C4 (Critical). capture_app_password() discarded the WP_User WordPress hands
it beside the uuid, and the marker re-derived an owner from
get_current_user_id() — a fact about the request, which any
determine_current_user filter or wp_set_current_user() (user-switching, SSO,
impersonation) desynchronises from the password. Probed end to end: the hook
names 5, the global says 9, the marker records 9, Phase B's one lookup finds
nothing, cleanup_complete: true, the token deleted and user 5's
manage_options credential still live. The pairing is now captured at the hook
and passed as $claimed; only a WP_User with a positive ID counts, anything
else is an explicit unknown. Never re-derive what you were told.
I5 (Important). managed_password_gone() answered "gone" from a user-meta read
it could not complete: core maps a failed read to array(), indistinguishable
from "this user has none", and round 3 made that the sole evidence gating the
irreversible step. It becomes a tri-state — password_state() answers present /
gone / unknown — confirmed by an error-surfacing raw read modelled on
read_option_uncached(). password_gone() now means PROVEN gone; every caller
fails closed on unknown except credential_state(), a screen render, which
keeps its previous answer deliberately and is documented as such. Phase A's
candidate confirmation reads STATE_PRESENT, so an unreadable list never
promotes a guess to knowledge.
M9. leftovers() now has a consumer: the unbind response and the /rules body
carry `leftovers` (string[]). cleanup_complete: false had two opposite causes
— something owed, or the deliberate ! final token retention — that Aura could
not separate from a bool.
M10. /status's `unbound` fragment goes on the wire as an object, so the
"no field readable" answer is {} and not [].
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…434) Found by running the mutations rather than reasoning about them: - the confirming raw read's "no database handle" branch read as absence with the whole suite green — nothing exercised a $wpdb without a usermeta table; - the fast path's `leftovers` had only an EMPTY assertion behind it, so a retry that actually owed something was never checked on that path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…nnot be read (#434 I5) The riskiest half of round 4's shared-code change had no test of its own: managed_password_gone() is read by the rotation, the mint's cleanup and reconcile_mint_intent(), and only the unbind's callers were pinned. Mutating managed_password_gone() back to "not present" now reddens this instead of passing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…M12/M13/M11) wpdb::get_var() ignores query()'s return value and extracts its answer from $last_result, and wpdb::query() has two early returns BEFORE its flush() — an unready handle, and a `query` filter that blanks the SQL. On either path last_result, last_error and last_query stay exactly as the PREVIOUS statement left them, so app_password_row_state() could read another query's rows, find null, and call it a proof that an administrator-level credential is gone — while our own `last_error = ''` guaranteed it looked clean. The reset was never a guard; it removes the only accidental signal. So the probe now checks readiness, builds the prepared SQL into a variable, and requires $wpdb->last_query to equal the SQL it issued. A filter that rewrote the query, or a statement that never ran, answers UNKNOWN. (M12) The bootstrap's wpdb stub was more forgiving than WordPress, which is how this hid: it modelled the two early returns not at all. get_var() now serves the previous statement's answer, untouched last_error and last_query included, when the handle is not ready or a `query` filter blanked the SQL — and an _application_passwords query in any shape it does not recognise THROWS, so reformatting the production SQL can never silently unhook the tests that prove this. credential_state() keeps its deliberate 'none' for an unreadable list — the one row of the caller table that does not fail closed — and now has the test that goes red when someone "tidies" it into painting the green check over a read that failed. (M13) leftovers() can never name the site token, so a final:true request whose step (5) delete does not land answers cleanup_complete:false with an empty list. Recorded beside step (5) with why it is safe; no behavioural change. (M11) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… (#434 N1) wpdb bookkeeping is not evidence a statement ran. `$wpdb->ready` is `false` by declaration on every db.php drop-in that extends wpdb and never calls parent::__construct() — HyperDB and LudicrousDB both do — so the readiness check fired on every probe there, `leftovers()` owed app_passwords forever and Phase B could never complete. And `last_query` is assigned by those same drop-ins BEFORE they acquire a connection, so the comparison could pass on a statement that never reached the server. Replace the whole scheme with a per-call nonce selected as a second column: a row that carries THIS call's nonce can only have come from THIS call's statement. Absence is read only from such a row; anything else is UNKNOWN, and says so through a new `aura_worker_app_password_probe_unproven` action so an eternally pending tombstone is diagnosable. The readiness check, the `last_query` comparison and the `last_error` check are deleted, not kept alongside — the nonce subsumes all three, and it also converges on a `query` filter that REWRITES the SQL, which the `last_query` comparison never could. The stub follows the probe to get_row(), modelling core faithfully: an unready handle and a blanked query hand back the previous statement's row, a falsy query answers null outright, and an unrecognised `_application_passwords` shape still throws rather than passing silently. Tests: the readiness test is rewritten around the nonce (its old rationale was false and it was pinned by a side effect — N2); the no-row path and the nonce-mismatch path are pinned separately; the prepare() refusal is pinned on the breadcrumb, which is the only thing that tells "never asked" from "asked and disbelieved" (N3). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
wp_generate_password() is pluggable AND filtered (`random_password`), so a third party could pin it to a fixed string — and a fixed nonce is no nonce, because the stale row a probe meets is another probe's, which would then carry the same value and be believed. Mutating the nonce to a literal turns two tests red, so this is the difference between a proof and a coincidence. wp_generate_uuid4() is neither pluggable nor filtered and never throws. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ree (N4), the breadcrumb carries only the owner (N5 payload), and the docblocks stop describing a mechanism that was deleted (N6) (#434) The nonce's job is freshness, not secrecy: it is echoed back in the same statement and is never confidential. The randomiser alone cannot promise freshness, because since WP 7.0 wp_generate_uuid4() draws from wp_rand(), which is pluggable and loads after plugins — so a third party can pin it, and a constant nonce puts us back where M12 began, always matching and proving only that some row came back. A function-local counter is the part nothing outside the method can make repeat. The breadcrumb's payload is now pinned to the owner alone, so no credential identifier can start riding a diagnostic that travels wherever the site sends its logs. And two docblocks still told the reader that $wpdb->last_error is what separates "no such row" from "the database is broken" — the mechanism round 6 deleted. That is the same defect N2 was raised for: a comment that outlives its code teaches the next reader the wrong invariant. Refs #434
…use aura_site_unbound at the boundary; reads and /rules stay reachable (#434) The marker makes the site refuse; this makes the refusal real at the two places a caller holding valid credentials can still reach a mutation. - Aura_Worker_Security::refuse_if_unbound() runs in the four mutating permission callbacks, AFTER validate_request() succeeds, so a caller who cannot prove it holds the token gets the token layer's answer and learns nothing about this site's binding. It uses is_set(), which answers TRUE on an unreadable marker: at a refusal boundary an unreadable marker is not a clean site. - Aura_Worker_Grant::verify() refuses FIRST, before the signature, the window and the nonce. This is the WordPress Abilities path, which never touches a REST permission callback, and a grant minted before the unbind still verifies perfectly. The nonce is not reserved on this path — refusing a grant is not spending it. - Aura_Worker_MCP::execute_tool() renders that WP_Error as its own code and status; the previous code concatenated the verifier's answer into a message string, which a WP_Error would have fataled on. The refusal is asserted for EVERY registered non-safe-method route, enumerated from the LIVE route table at test time (sa_registered_routes()), never from a list — a hard-coded list lets a mutating route ship next year with an unguarded permission callback and a green suite. Two brief-sketch corrections, both found by that enumeration and both named with a reason in the test: /aura/mcp/tools/preview is a second POST read (non-executing), and /aura/v1/connect is a non-safe route on __return_true that must stay reachable or an unbound site could never be rebound.
…SET of registrars is not a list either (#434 round-1 I1, M3, M4, M6) IMPORTANT-1. sa_registered_routes() read each registrar's table live but named the registrars by hand, which made the set of registrars the maintained list the brief forbade, one level up: a third rest_api_init registrar exposing an unauthenticated mutating POST left the whole suite green at 1084/3860 (reproduced). It now runs ( new Aura_Worker() )->init() and fires rest_api_init — the mechanism WordPress itself uses — with $GLOBALS['_filters'] saved and restored WHOLE around the build, so none of the bootstrap's other registrations can leak into whichever test triggered it. The four init() methods it reaches do nothing but register hooks, so that restore is complete. The last named thing is the entry point, and a new test keeps it honest: every add_action( 'rest_api_init', … ) in the plugin must live in the file that entry point lives in, computed over the source tree. A registrar hooked up anywhere else never runs during the build and its routes would be invisible to the whole class. MINOR-3: the exemption regex is anchored at both ends. Right-anchored alone it also exempted '/aura/v1/anything/aura/v2/rules' — unreachable today because the only registered capture excludes slashes, reachable the day one does not. MINOR-4: nothing asserted that a preview mutates nothing, which is the entire reason /aura/mcp/tools/preview is excluded from the refusal sweep. Every preview-capable tool is now driven and its side effects compared, and cleanup_orphaned_assets is driven with the adversarial dry_run=false its dry_run() must override. A preview-capable tool with no fixture fails the sweep rather than being skipped. MINOR-6: is_set()'s docblock said "FAILS OPEN … a mutation boundary MUST NOT use this method" while the mutation boundary uses it, correctly. Both readings were true from different framings, so neither word survives: the docblocks now say what the method ANSWERS and leave open/closed to the caller that decides it. is_set_strict() no longer advertises callers it does not have.
…out of the way; the scan sees both quote styles and counts what it cannot reach (#434 round-1 NEW-1..NEW-4) NEW-3. "I could not pin it" was a claim like any other and it did not survive checking. The ordering half was right — PHPUnit resolves the data provider before any setUp() — but the premise was wrong ($GLOBALS['_filters'] holds aura_worker_unbind_step at build time, so the restore restores something real) and the conclusion did not follow: the obstacle was MEMOISATION, which belongs to the wrapper, not to the guard. sa_build_route_table() is now the uncached build and sa_registered_routes() the memo over it, so both properties are directly testable: a sentinel filter must survive the build, and an ambient rest_api_init listener must not contribute a route to the table. NEW-1: the scan matched only a single-quoted hook name, so the same file-scope registrar written add_action( "rest_api_init", … ) was a green survivor. Both quote styles now match; a hook name held in a constant or variable is still invisible and the failure message says so rather than implying otherwise. NEW-2: being in the right FILE is not the same as being REACHED. A registrar inside init()'s existing `if ( is_admin() )` branch passes a file-level scan and never executes. Closed two ways: the number of rest_api_init registrations in the entry-point file is pinned, so a third one reddens whatever guards it; and the table is built with is_admin() both false and true and the two compared, so the one conditional the plugin actually has cannot hide a route. The residual — any other defined()/class_exists() guard — is stated in the docblock and in the assertion message instead of being left for a reader to discover. NEW-4: my own round-1 doc fix named the wrong caller. is_set_strict()'s one production caller is Aura_Worker_Rules::accept_under_claim(), at step 0 of a push, where the three states genuinely produce three different answers — not fast_path_or_refusal(), which does not call it. The line number is gone with it; the other two claims in that docblock were re-checked against the code and are true.
…binding — by the marker's Application Password UUIDs or the token run-as path, never by live options (#434) Task 5 closed SiteAgent's own routes and Aura_Worker_Grant::verify(). This closes the other door: WordPress core's REST API (/wp/v2/*, /wc/v3/*, anything a plugin registers), which the departed binding reaches with the same credentials and which passes through none of those seams. Identity comes from the MARKER, never from a live option: by the time this seam matters, Phase B has deleted aura_worker_connect_user_id and the managed Application Password record, and deletes the site token last. The token path is proved by the run-as PATH, not by the user id it resolved to — after Phase B, resolve_connect_user() falls back to the first administrator, so the id routinely differs from the marker's connect_user_id on exactly the requests this seam exists to catch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ebind does not clear the marker, so the path alone over-fires (#434 round-1 MAJOR-1/MINOR-1) The comment claimed "a rebind clears the marker before issuing another". Nothing does: Aura_Worker_Unbind::delete_under_claim() has no production caller and handle_connect() never touches the option. A site re-connected while still marked had its NEW binding's token-only requests refused at every core REST route — fail-closed, so an availability defect rather than a hole, but a claim of safety resting on a fact the code does not provide. The run-as branch now compares the stored token hash against the marker's `site`, exactly as fast_path_or_refusal() does. It cannot weaken the departed case: Layer 2.5 runs only for a token Layer 2 accepted, and Phase B deletes that token last, so whenever ran_as is non-null there is a token to compare. An unreadable token, or either side empty, still answers TRUE. The comment now states what is enforced, and a test pins the premise: if anything ever does clear the marker under claim, it reddens rather than silently making the clause dead weight. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…B under the claim and clear the marker only after the replacement binding is established (#434) The way back. Phase A's marker refuses every mutation and, until now, nothing ever took the refusal away — a re-connected site was bound and permanently refusing. Two calls bracket the token install: Aura_Worker_Unbind::finish_before_rebind( $fence ) settles the DEPARTED binding's Phase-B debt (steps 1-4; never the token, which the rebind is about to replace) and refuses the whole rebind 409 aura_unbind_incomplete when anything is still owed. It must come FIRST: writing the replacement token permanently disarms maybe_finish(), which bails on the hash mismatch, so an Application Password recorded only in the marker would be stranded live with nothing left on the site that would ever revoke it. Aura_Worker_Unbind::release_marker_after_rebind( $fence ) lifts the refusal, and only as the LAST fallible step of a rebind that succeeded end to end. The ordering is the safety property: the marker outlives the old token, so every earlier exit leaves the site refusing the old token AND the half-installed replacement. departed_binding_request()'s token-hash clause is REMOVED with it (round-1 MAJOR-1 added it only because nothing cleared the marker). With the premise supplied, "marker present, token differs" now means a rebind that installed the token and then failed — a hash comparison would have waved exactly that through core REST, re-opening the hole the ordering closes. It also puts this boundary back in step with refuse_if_unbound(), which has always gated on is_set() alone. The Task 6 tripwire is not deleted but re-aimed at the new truth: one gateway for the delete, exactly two flows that reach it, and both must settle Phase B first. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…onnect, and the connect proves its connect user the way the rotation does (#434 Task 7 round-1) MINOR-1: finish_before_rebind() reads is_set_strict() and answers 409 aura_unbind_unreadable — no `leftover` list — when the marker row could not be read or does not parse. Every such case shares exactly one true statement (the record could not be READ), and at a site that was never unbound the old aura_unbind_incomplete wording described an event that never happened. Both paths still refuse. MINOR-2: handle_connect() now proves its own aura_worker_connect_user_id write with an uncached read before releasing the marker, scoped to a marked site, exactly as ajax_regenerate_token() does — Phase B deleted that row, so on a rebind the write is the only thing naming the install's administrator, not the refresh it is on an ordinary connect. Both flows now mean the same thing by "a proven rebind", and the two twin tests are what stop the asymmetry returning. NIT: the mint's app_password_unavailable bookkeeping — the install's last fallible write — moves ahead of the release, so nothing that can fail sits after the refusal is lifted; pinned by write order. NIT: the tripwire keys its sources by path, not basename, so identically named files in different directories cannot silently collide. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ask 5's registrar sweep could silently lose one (#434 Task 7 round-2)
Round 1 fixed the basename collision in Task 7's tripwire and stopped there.
The same pattern was live in Task 5's guard: rest_api_init_registrations()
(UnbindRefusalTest) keyed `filename => count`, and RecursiveDirectoryIterator
reaches includes/aaa_legacy/ before includes/, so a second file sharing a
basename with a real one had its slot OVERWRITTEN by the real file. Reproduced
before the fix: a third add_action('rest_api_init', ...) sitting on disk left
the whole class green, with its routes invisible to every assertion in it. An
enumeration whose guarantee is completeness must not have a key space in which
two findings can collapse into one.
Keys are now paths relative to SA_PLUGIN_DIR. Both scans additionally PIN the
key space with a test that puts the colliding file on disk and requires both
findings to be reported — the property, not the current spelling of it. The
fixture goes through a new sa_with_plugin_file() bootstrap helper that removes
the file in a finally, so a red test cannot leave the tree dirty.
Swept for the pattern across tests/ and the plugin: every other file walk is an
rrmdir teardown over one directory at a time, and every other basename() is in
value position. This was the only remaining instance.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…when the body throws (#434 Task 7 round-2) sa_with_plugin_file() is what both collision pins stand on, and a mutant that removed its unlink left every test green while leaving a file inside the plugin tree. A red assertion is an exception, so that is exactly the case that would have leaked — poisoning every later scan in the process and dirtying the working tree. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c1174fe501
ℹ️ 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".
Comment on lines
+945
to
+946
| if ( null === $owner ) { | ||
| // Nothing can be deleted from a user the site cannot name, |
There was a problem hiding this comment.
When a marker contains an unknown owner, this branch intentionally leaves the Application Password live until an operator resolves it. The new refusal paths do not contain that credential outside REST: guard_core_any() and guard_core_delete() are gated by is_agent_rest_request(), while WordPress Application Passwords also authenticate XML-RPC. Consequently, the departed administrator credential can continue creating, editing, or deleting content through XML-RPC for as long as this debt remains; reject UUIDs named by the marker during Application Password authentication or add equivalent non-REST mutation guards.
Useful? React with 👍 / 👎.
Comment on lines
+76
to
+79
| if ( ! is_array( $m ) ) { | ||
| // Not a marker at all — the row holds something else. Genuinely | ||
| // absent: nobody ever wrote a marker here. | ||
| return null; |
There was a problem hiding this comment.
If the option row exists but its serialized value is truncated or otherwise decodes to a scalar, this returns null, which every caller interprets as no marker. The mutation permission callbacks then reopen the site and accept_under_claim() can accept an ordinary ruleset even though Aura already recorded the disconnect. Since this option name is exclusively the marker, only an actually absent raw row should return null; a present but undecodable value needs the malformed fail-closed result.
Useful? React with 👍 / 👎.
| || ! self::field_is( $m, 'seq', 'int' ) ) { | ||
| return self::malformed(); | ||
| } | ||
| $uuids = isset( $m['app_password_uuids'] ) && is_array( $m['app_password_uuids'] ) ? $m['app_password_uuids'] : array(); |
There was a problem hiding this comment.
When a marker's app_password_uuids field is missing or corrupted to a non-array, validation silently normalizes it to an empty list. Cleanup can then report the credential step complete and delete the token, while departed_binding_request() no longer recognizes the still-live Application Password on core REST routes. Because every marker writer supplies this security-critical array, an invalid shape must make the marker malformed rather than assert that no credentials exist.
Useful? React with 👍 / 👎.
…nferring absence (#434, Codex round-4 P1 x3) Three findings, one shape again: a boundary that answers "nothing here" from evidence that cannot say so. 1. THE REFUSAL WAS REST-SHAPED. Every guard was gated behind is_agent_rest_request(), but WordPress authenticates Application Passwords on every API surface it recognises — XML-RPC included — so a credential Phase B could not prove revoked went on creating, editing and deleting content through xmlrpc.php for as long as the debt stood. Fixed as the rule rather than as one more surface: the credential is refused on `wp_authenticate_application_password_errors`, where WordPress decides whether it authenticates at all, so a surface core adds tomorrow is covered on the day it lands. REST keeps its own seam — it answers 403 aura_site_unbound and says why, which a 401 here would lose — resolved through the SAME $rest_request_override the REST guards use, so the two boundaries cannot disagree about what REST is. An unreadable marker refuses every Application Password on those surfaces: the ruling departed_binding_request() already makes, stricter here only because authentication cannot see whether an XML-RPC call means to read or to write. Cookie-authenticated admin access is untouched. 2. A ROW THAT DOES NOT DECODE READ AS NO ROW. read() answered null — "no marker" — for a value that failed to unserialize, which reopened every mutation boundary and let accept_under_claim() take an ordinary ruleset for a binding Aura had already disconnected. This option name is the marker's alone, so a present row is evidence something wrote here. ABSENT now means the absence of a ROW and nothing else; undecodable is malformed, the same unknown a failed read is. Same contract change as Task 4's round-2 I3, one level further out. 3. A DAMAGED CREDENTIAL LIST WAS NORMALISED TO AN EMPTY ONE. That list's emptiness is a security claim: cleanup() reads it to decide step (1) is complete and then deletes the token, and departed_binding_request() reads it to recognise a live Application Password. Every writer supplies the array, so a missing or non-array field is a row this build cannot read — malformed, not "this binding held no credentials". Stub work the fixes needed: WP_Error now accumulates codes and answers has_errors() the way core's does (a fake holding one code makes "already refused by somebody else" untestable), and has_filter()/has_action() exist so a registration nothing asserts cannot be dropped in silence. Tests: 1306 (+12), 5063 assertions. Lint 0 errors / 47 warnings. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Member
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 59ca12f0da
ℹ️ 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".
Comment on lines
+1101
to
+1105
| $ours = self::site_token_uncached(); | ||
| if ( is_wp_error( $ours ) ) { | ||
| return $ours; | ||
| } | ||
| $ours = (string) $ours; |
There was a problem hiding this comment.
If a bare unbind authenticates with the old token, then pauses while an administrator regenerates the token, this code takes the claim afterward and treats the newly read token as the one that authenticated the request. On an unkeyed site the old request can consequently create a marker for the replacement binding and, with final: true, delete its fresh token. Pass the authenticated token hash into the claimed section and compare it with this uncached value before writing the marker.
Useful? React with 👍 / 👎.
Comment on lines
+722
to
+723
| if ( null === $raw || ! isset( $raw['app_password_uuids'] ) || ! is_array( $raw['app_password_uuids'] ) ) { | ||
| return $found; // nothing legible to add |
There was a problem hiding this comment.
Although normal marker validation now rejects a missing or non-array UUID list, this fresh repair path still replaces that unknown list with only the name/record sweep. If the damaged marker had captured a manually supplied Application Password—which is not necessarily named Aura SiteAgent—the operator teardown can then omit the live credential, report success, and delete its token and sole tracking marker. Return an unrepairable result when the stored UUID list cannot be read instead of treating the sweep as exhaustive.
Useful? React with 👍 / 👎.
| // (2) The options that name the departed dashboard and its connector. | ||
| do_action( 'aura_worker_unbind_step', 'options' ); | ||
| Aura_Worker_Rules::delete_option_if_claimed( 'aura_worker_dashboard_url', $claim, $fence ); | ||
| Aura_Worker_Rules::delete_option_if_claimed( 'aura_worker_connect_user_id', $claim, $fence ); |
There was a problem hiding this comment.
On a site whose connecting user has a custom role granting manage_options but no user has the literal administrator role, a successful non-final unbind deletes this option and revokes the Application Password while retaining the token for a later tombstone. The next token-only /rules retry then fails in resolve_connect_user() before reaching the marker fast path because its stored user is gone and the administrator-role fallback is empty, so the final cleanup can never be delivered remotely. Keep this identity until the token is deleted, or resolve marked-site retries from the marker's captured connect_user_id.
Useful? React with 👍 / 👎.
…egible list is not an empty one, and a marked site still resolves (#434, Codex round-5 P1/P1/P2) 1. THE TOKEN CAN MOVE UNDER A PAUSED REQUEST. Authentication happens before the site claim is taken, so a bare unbind that authenticated with the old token and then paused while an administrator pressed Regenerate Token read the REPLACEMENT token under the claim, marked the new binding with it, and — with `final: true` — deleted that fresh token. An unbind for a binding that had already ended, executed against the one that replaced it. check_aura_token() now records the hash it accepted, and the ONE marker builder both Phase A paths share refuses to name any other. Absence is refused as firmly as a mismatch: both paths run behind that check, so "no token authenticated this request" is not a state a real Phase A can be in, and treating it as permission would be the fail-open the check exists to close. Answers 409 `aura_site_token_changed`; nothing is written. 2. AN ILLEGIBLE CREDENTIAL LIST IS NOT AN EMPTY ONE. Round 4 made a missing or non-array `app_password_uuids` malformed — which sends exactly those rows to the repair path, where the damaged row's list was treated as "nothing legible to add" and the NAME SWEEP as exhaustive. It is not: the sweep finds what SiteAgent minted, and the credential this rule protects is the one it cannot see — a password an operator connected by hand, or one Aura's PATCH installed, carrying a name of somebody else's choosing. The row was its only record. Repairing from the sweep alone let the teardown report success and delete the token beside a live administrator credential. The repair now refuses; the marker stays malformed and the site keeps refusing, which is the honest state of a site whose credential list is gone. (Enumerating every Application Password instead would be worse — it would sweep the owner's unrelated credentials in for revocation.) 3. A MARKED SITE COULD STOP RESOLVING ITS OWN RETRY. Phase B step (2) deletes `aura_worker_connect_user_id` while the token deliberately lives on, after which resolve_connect_user() falls back to the literal `administrator` ROLE — while every other check here asks for the CAPABILITY. A site whose connecting user holds manage_options through a custom role, with nobody holding the administrator role, resolved nobody: its token-only /rules retry failed before reaching the marker's fast path, so the final cleanup could never be delivered. The marker captured that identity before the delete, and it is consulted between the two, still gated on the capability. The fake was why nothing caught it: user_can() and get_users( role ) both read $_admins, so role and capability were the same set in tests and could not disagree. They are two sets now. Tests: 1312 (+6), 5075 assertions. Lint 0 errors / 47 warnings. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Member
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 561341e387
ℹ️ 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".
Comment on lines
+110
to
+111
| if ( Aura_Worker_Unbind::is_set() ) { | ||
| return Aura_Worker_Unbind::refusal(); |
There was a problem hiding this comment.
When an unbind lands after a mutating request has passed its permission callback, Phase B can delete the grant key before the handler runs. Both Aura_Worker_Grant::require_for() and Aura_Worker_MCP::execute_tool() skip verify() when is_enforced() is false, so this new marker check never executes and the already-authorized update/tool can mutate the disconnected site. Check the marker before the no-key bypass (and propagate the WP_Error; while the key remains, require_for() currently attempts to concatenate it and fatals).
Useful? React with 👍 / 👎.
Comment on lines
+146
to
+150
| $m['app_password_uuids'] = array(); | ||
| foreach ( $uuids as $uuid ) { | ||
| if ( is_string( $uuid ) || is_int( $uuid ) ) { | ||
| $m['app_password_uuids'][] = (string) $uuid; | ||
| } |
There was a problem hiding this comment.
If a stored marker is damaged so that one app_password_uuids entry becomes an array or object while the corresponding Application Password remains live, this loop silently removes that entry and returns an otherwise valid marker. leftovers() can then report no credential debt and a final cleanup can delete the token, leaving the administrator credential active with its tracking lost. Treat any non-string/non-integer entry as a malformed marker rather than normalizing it away.
Useful? React with 👍 / 👎.
Comment on lines
+124
to
+126
| $aura_marker_uuids = ( is_array( $aura_marker ) && isset( $aura_marker['app_password_uuids'] ) && is_array( $aura_marker['app_password_uuids'] ) ) ? $aura_marker['app_password_uuids'] : array(); | ||
| $aura_marker_users = ( is_array( $aura_marker ) && isset( $aura_marker['app_password_users'] ) && is_array( $aura_marker['app_password_users'] ) ) ? $aura_marker['app_password_users'] : array(); | ||
| $aura_marker_settled = true; |
There was a problem hiding this comment.
When aura_worker_unbound exists but its app_password_uuids field is missing or non-array, this maps the unreadable list to an empty array and initializes the marker as settled. The namespace sweep therefore deletes the marker even though it may be the only record of a still-live manually supplied Application Password. A present marker with an unreadable credential list must be retained as unresolved rather than treated as having no credentials.
Useful? React with 👍 / 👎.
… a guard keeps it that way (#434, Codex round-6 P1 x3) Six review rounds found the same inference — "a list I could not read is a list that holds nothing" — in five different readers. Each one decided for itself what an unreadable list meant, so fixing the instance Codex named produced the next instance next round: a truncated aggregate read as a whole list (round 2), a row that would not unserialize read as no row (round 4), a non-array field normalised to array() (round 4), an unreadable field treated as "nothing to add" (round 5), and now a corrupt ENTRY dropped from an otherwise valid list, plus uninstall.php's own copy of the same mapping (round 6). Patching pointwise was the mechanism. THE RULE NOW HAS ONE DEFINITION. includes/unbind-credential-list.php holds a single pure function — no class, no hooks, no bootstrap — answering what a stored `app_password_uuids` PROVES: the uuids, or null when it proves nothing. Null for a missing or non-array field, for ANY entry that is not a string or int, and for ANY empty entry. An empty array is a real answer and a strong one. Four readers ask it now instead of restating it: read()/validated(), merged_with_damaged_row(), append_authenticating_uuid() (defensive there today — "already validated upstream" is exactly the assumption that becomes next round's finding), and uninstall.php, which loads that one file precisely because it must not load the plugin. That also retires the duplication that produced round 3. AND A GUARD KEEPS IT THERE. tests/unit/CredentialListRuleTest.php scans production sources, computed rather than listed, and fails when any line outside the rule's own file decides the shape of that field for itself — `is_array()`, `isset()` or `array_map()` applied to it. The rule's file must also be loaded by both the plugin and uninstall.php. Written before the sweep and it found three more call sites; sharpened so `is_array( $marker )` beside a read of an already-proven list is not a false positive. Round-6 P1 #1, a different class, fixed with it: the marker refusal lived INSIDE Aura_Worker_Grant::verify(), and both callers skip verify() when no gateway key is configured. Phase B step (4) deletes that key while the site is still marked, so from that moment a mutating request that had already passed its permission callback found no key, skipped the grant path, and never met the marker. The refusal is not a verdict about a grant, so it no longer lives behind the question "is a grant required": refusal_if_unbound() is asked first by require_for() and by the MCP tool path. require_for() also returns a WP_Error verdict as itself — it used to concatenate it into a message, which is a fatal, not a 403. Two tests that asserted the old per-entry drop are updated; their original intent (refuse, never fatal on strval of an object) is unchanged and still asserted. Tests: 1335 (+23), 5106 assertions. Lint 0 errors / 47 warnings. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Member
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f56fa7e823
ℹ️ 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".
| $aura_marker_settled = ! $aura_marker_present || null !== $aura_marker_uuids; | ||
| foreach ( (array) $aura_marker_uuids as $aura_marker_uuid ) { | ||
| $aura_marker_uuid = (string) $aura_marker_uuid; | ||
| $aura_marker_owner = (int) ( $aura_marker_users[ $aura_marker_uuid ] ?? 0 ); |
There was a problem hiding this comment.
When a damaged marker contains an owner such as "42junk", this cast accepts it as user 42 instead of treating the attribution as unknown, unlike Aura_Worker_Unbind::validated(). If the UUID actually belongs to another user, deletion checks only user 42, finds nothing, leaves $aura_marker_settled true, and the namespace sweep deletes the sole record of the still-usable credential; validate the stored owner as a positive integer or all-digit string, otherwise retain the marker.
Useful? React with 👍 / 👎.
…same rule (#434, Codex round-7 P1) The mechanism landed last commit and covered half of it. `uninstall.php` read a stored owner with a bare `(int)` cast, and PHP reads "42junk" as 42: a damaged attribution became a confident one, the revocation asked user 42's list, was told "not there", and reported a credential belonging to somebody else as settled — after which the sweep deleted the marker that was its only record. validated() had the correct test all along, in its own copy, which is exactly the arrangement that lets one copy be wrong. aura_worker_credential_owner() joins the list rule in the same pure function file: a positive int, or null meaning "this site does not know whose password this is". Three readers ask it now — validated(), password_owner() (which carried its own `(int)` cast) and uninstall.php. The guard grew the second claim, and stayed narrow enough to be true: - the shape check still fires only for the uuid LIST. Deciding the shape of the owners MAP is deliberately not flagged — an unreadable map means every owner is unknown, which fails CLOSED, where an unreadable list read as empty fails OPEN. Only one of them is a security claim, and a guard that flags both would have to be suppressed to stay green, which is how a computed guard becomes a maintained one. - a new line check fires on any `(int)` cast of an owner. - and a file check, because a line scan cannot see a read through a local alias — which is precisely how this one hid: a file that holds the owners map AND revokes a credential must ask the rule. A file that only carries the map forward into a marker it is about to write interprets nothing and is not asked to. Tests: 1350 (+15), 5124 assertions. Lint 0 errors / 47 warnings. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Member
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19ae15a647
ℹ️ 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".
| * duration of the operation they guard, and nothing in this task changes | ||
| * that reasoning for them. | ||
| */ | ||
| const SITE_CLAIM_TAKEOVER_AFTER = 120; // seconds; well above any push. |
There was a problem hiding this comment.
When a connect or token regeneration remains active for more than 120 seconds, any subsequent rules/lifecycle request can seize this claim because its timestamp is never refreshed. The original connect only rechecks ownership at line 677 and performs no final check before returning at line 810, so a replacement connect can revoke or replace the just-minted password/token after that check while the original still returns 200 with those now-invalid credentials. Refresh the lease while work is active, use a separate recoverable lock for short rules pushes, or revalidate ownership immediately before every success response.
Useful? React with 👍 / 👎.
… is proven before it is reported (#434, Codex round-8 P1) seize_stale_claim() bounds a claim stranded by a fatal to SITE_CLAIM_TAKEOVER_AFTER seconds, and its own docblock says a claim "a live request refreshing it" cannot be seized — but nothing ever refreshed one. A connect that legitimately ran past the window became seizable while it was still working, and this branch made that more likely, not less: since #434 a rebind runs a full Phase B cleanup before installing the new binding. Two changes, and the second is the one that makes reporting the failure impossible rather than merely unlikely: - touch_site_claim() refreshes the lease once the mint — the step where a slow host spends its seconds — is behind us, so the tail runs on a lease measured from there. The same conditional compare-and-swap the seizure uses, in the other direction: the UPDATE names the exact bytes just read, so a fence that has already lost the site cannot refresh it back, and a loser's refresh cannot disturb the winner. - ownership is proven once more immediately before the success response. Every step above verifies the claim when it acts, but the LAST check still had a tail after it, and a handler evicted there answered 200 carrying a token and an Application Password the replacement had already revoked — leaving Aura holding credentials that authenticate nothing and a site it believes is connected. It now answers 409 `aura_site_taken` and KEEPS the magic link, so the retry that follows can still use it. The repo's own raw-options-write guard reddened on the new statement, as designed; its ledger records the third compare-and-swap on `aura_worker_connect_lock`. Tests: 1356 (+6), 5139 assertions. Lint 0 errors / 47 warnings. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…to the 47-warning baseline (#434) Both carry the same rationale the neighbouring compare-and-swaps do: the bytes a swap names must come from the row, never the option cache, and a CAS has no storage-function equivalent. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…een (#434) Removing the refresh from the connect left the whole suite green: it changes nothing observable in a fast request, and a timestamp two statements apart lands in the same second, so no assertion about the row could tell the difference. The refresh now announces itself (`aura_worker_connect_lease_refreshed`) and the connect test asserts it fired — the call site is pinned, not just the helper. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! 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". |
…ther include (#434) Plugin Check failed the branch: the new pure-function file had no ABSPATH guard. It is loaded by the plugin bootstrap and by uninstall.php, which runs with WP_UNINSTALL_PLUGIN defined and ABSPATH available — never directly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Member
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b489b477b
ℹ️ 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".
Comment on lines
+159
to
+160
| WP_Application_Passwords::delete_application_password( $aura_marker_owner, $aura_marker_uuid ); | ||
| foreach ( WP_Application_Passwords::get_user_application_passwords( $aura_marker_owner ) as $aura_pw_item ) { |
There was a problem hiding this comment.
When the Application Password usermeta read fails during uninstall, WordPress's get_user_application_passwords() collapses that failure to an empty list. This loop therefore treats a failed deletion/read as proof that the UUID is gone, leaves $aura_marker_settled true, and the namespace sweep deletes the marker even though the administrator credential may still be live. Use the same raw, self-proving usermeta read used by runtime cleanup, or otherwise retain the marker whenever absence cannot be proven.
Useful? React with 👍 / 👎.
… that proves it is shared (#434, Codex round-10 P1)
`WP_Application_Passwords::get_user_application_passwords()` is a
`get_user_meta( …, true )` followed by `if ( ! is_array( … ) ) return
array();`, so a meta read that could not be completed is
indistinguishable at that layer from "this user holds no Application
Passwords". The plugin learned that in Task 4 and answers absence only
through a raw read that PROVES IT RAN — a per-call nonce echoed back in
the same statement. uninstall.php never learned it: all four of its
proof loops read core's list, so a failed read said "revoked", and the
namespace sweep then deleted the only record of a live administrator
credential.
Third finding in this file, and the same cause each time: it re-derives,
from memory, reasoning the plugin already has. So the probe joins the
credential rules rather than being copied into it. The file is renamed
`includes/credential-rules.php` — it no longer holds only the list rule —
and gains aura_worker_app_password_list() (the nonce probe, verbatim) and
aura_worker_app_password_state() ('present' / 'gone' / 'unknown').
The state function keeps BOTH properties the plugin's version had, which
is why it is shared rather than reimplemented: core's own list is asked
first and only for the positive, so a filter or an alternative meta store
keeps its say over "this password exists", and the raw probe runs only on
the path about to conclude absence — it can turn that into 'present' or
'unknown', never the other way. A probe-only implementation in
uninstall.php would have been a new fail-open on an alt meta store.
Magic_Link::managed_password_state() and app_password_list() now delegate
there; app_password_row_state() had nothing left to do and is gone.
Tests: 1359 (+2), 5145 assertions. Lint 0 errors / 47 warnings.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Member
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 862f5f4ba3
ℹ️ 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".
Comment on lines
+1063
to
+1064
| if ( ! self::is_set() ) { | ||
| // Deliberately BEFORE set_transient(): a site with no marker must not |
There was a problem hiding this comment.
On every normally bound site, the finish transient is absent, so this init hook reaches is_set() on every frontend, admin, and REST request; is_set() deliberately performs an uncached options-table query, and the absent-marker branch never sets a transient, so the query repeats on every page load indefinitely. This adds site-wide database traffic even though cleanup is only relevant after an unbind; cache/throttle the negative result and invalidate that cache when Phase A writes the marker so a newly interrupted unbind can still heal promptly.
Useful? React with 👍 / 👎.
… on every request (#434, Codex round-11 P2) maybe_finish() runs on `init` and is_set() is a deliberately UNCACHED raw read. FINISH_TRANSIENT throttles a MARKED site — but a bound site, which is nearly all of them and nearly all requests, matched no throttle at all and paid that query on every frontend, admin and REST hit, forever, to be told again that there is nothing to finish. The negative is throttled too, and by a SECOND transient rather than the first: arming FINISH_TRANSIENT on the absent path would make a Phase A landing a moment later wait out FINISH_THROTTLE before its first self-heal, which an earlier round put that ordering there to prevent. write_under_claim() clears the new one when a write is confirmed landed, so a marker written now heals on the request that follows it. The TTL is what keeps the invalidation from being load-bearing: the worst a stale negative can cost is that much delay before the site sweeps ITSELF. Nothing that refuses a request reads it — every boundary asks is_set() directly — so a stale negative can never un-refuse a marked site. Two repo guards fired, as designed: the existing test that a bound site does not arm the MARKED site's throttle (now asserting both halves), and the fixed-name-transient sweep, since a persistent object cache keeps transients out of the options table entirely. sa_set_marker() clears the negative too — a fixture that seeds the row straight into the database has to model what the real writer does. Tests: 1361 (+2), 5151 assertions. Lint 0 errors / 47 warnings. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… does (#434) The "failed Phase A leaves the negative throttle alone" test passed a foreign fence, so write_under_claim() refused several guards before the read-back and the assertion proved nothing about the branch it names — a mutant that cleared the transient unconditionally stayed green. It now uses _sa_option_write_divert, the one seam where a claim-fenced statement reports success while the row diverges. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 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". |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Aura can now actually disconnect a site. Until this release, "disconnect" cleared state on Aura's side while the site kept a valid token and a live Application Password — an administrator-level credential nobody could revoke through the dashboard.
Spec:
docs/superpowers/specs/2026-08-29-site-unbind-design.mdin the Aura repo (merged as5386d5fc). Plan:docs/superpowers/plans/2026-08-29-site-unbind-plan-b-siteagent-2-13.md.What it does
A clear from Aura carrying
unbind: truewrites anaura_worker_unboundmarker under the site claim. From that moment every mutation answers403 aura_site_unbound— SiteAgent's own token-only routes,Aura_Worker_Grant::verify(), and WordPress core REST writes made as the departed binding — while reads keep working.Phase B then cleans up in fixed order: Application Passwords → options → ruleset store → gateway key → the site token last, and only when Aura says
final: true. That ordering is the safety property: the token is what lets Aura's retry be recognised, so it outlives everything it might still need to identify.The core-REST seam matches on the marker's own record, never on live options — by the time it matters, Phase B has already deleted them. That is why Phase A records the credential UUIDs and their owners before removing anything.
Response contract
Both the enveloped and the bare form answer:
leftoversis the field Aura branches on. Empty means only the shared token is outstanding; non-empty names something the site could not prove revoked; absent means "something may be owed" — the transport defaults it fail-closed, so an answer that carries no list is never read as "owes nothing".Error codes
aura_site_unbound(403) ·aura_site_busy(503, retryable) ·aura_unbind_incomplete(409, withleftover[]) ·aura_unbind_unreadable(409) ·aura_unbind_unrepairable(409) ·aura_unbind_marker_stuck(500) ·aura_ruleset_client_mismatch(409, bare form) ·aura_unbind_store_failed(500, retryable).Other surfaces
/statusreportsunbound: { at, site_ref }while the marker is set, plusapp_password_probe_unprovenwhen a credential could not be proven gone — so a site that cannot converge is diagnosable instead of silently pending.{ "unbind": true, … }body authenticated by the token alone. A keyed site refuses that form.Rollout
Aura's Plan C starts sending
unbindonly after its migration B. A 2.13 site that never receives one behaves exactly as 2.12. The fleet upgrades through Aura self-update.Review
Nine implementation tasks, each with its own review, fix rounds and mutation testing, then a whole-branch review that returned SHIP. 1277 tests / 4601 assertions;
composer lint0 errors and 47 warnings across 7 files — identical to the branch point, verified against a worktree there.Roughly ten defects were found and fixed along the way, and nearly all were one shape: safety or absence inferred from evidence that could not support it. Several were caught only because a reviewer tested a justification instead of accepting it.
🤖 Generated with Claude Code