Repository navigation
Normalize delegation identifiers - #291
Conversation
samgbafa
left a comment
There was a problem hiding this comment.
Publication limitation: GitHub rejected the formal REQUEST_CHANGES event because the configured authenticated account is also the PR author. This is a REQUEST_CHANGES verdict, not an approval; the COMMENT transport does not clear the release hold.
REQUEST_CHANGES — independent Sol correctness review
Reviewed exact head 63b66ce. Three P1 blockers remain. Evidence uses only this reviewer's checkout, newly generated local keys, file-backed/in-memory SQLite, and native production authorization APIs. No production endpoints, stored secrets, Docker, or PR source edits.
P1: Preserve revocation across the pre-fix stored-CID transition
tinycloud-auth/src/authorization.rs:53-58 changes the identity assigned on import, but registration and revocation authorization still operate on only the requested persisted hash (tinycloud-core/src/db.rs:1648-1683; models/revocation.rs:71-79). I actually registered a v=0/1 grant and processed a real owner-signed revocation on base f9d700b, then reopened the same DB on this final head. The old CID remains addressable, Revoked, and denied; its grant/revocation IDs and stored bytes remain unchanged. Re-importing the identical old header nevertheless creates an Active canonical-CID row, and a real holder-signed invocation citing that returned CID authorizes successfully. Preserving historical IDs alone is insufficient.
Required: retain historical signed references/addressability while sharing revocation state across canonical-equivalent historical and new IDs. Backfill equivalence from stored serializations, not just one v-byte hash: pre-fix null, reordered-map, and nonminimal-integer encodings were also independently accepted. Regression: seed an actual old grant+revocation, reopen after upgrade, re-import both equivalent v forms, and require every old/new proof to remain denied, including ancestor authorization. Also preserve the advertised CID/raw-header relationship in legacy list/chain responses; decoding an old row then re-encoding currently changes its bytes/CID.
P1: Canonical DAG-CBOR is not sufficient signed-message identity
tinycloud-auth/src/authorization.rs:54-55 re-encodes the parsed CACAO without canonicalizing its unsigned payload representation against the verifier's SIWE message view. Independently generated conventional and issuer-case variants reconstruct identical SIWE message bytes and EIP-191 hashes, have the identical signature, and both verify, but receive different node CIDs. On this final head, a real revocation of the conventional CID denies that signed invocation while the equivalent variant remains Active and authorizes.
Required: normalize or reject such payload aliases before CID assignment, consistently with the SIWE reconstruction used for verification; include them in old-state equivalence handling. Regression must demonstrate shared CID or rejection and actual post-revoke denial, not merely successful verification.
P1: The native SDK still computes the pre-normalization CID
dependencies/cacao/src/siwe_cacao.rs:184-187 retains unchecked array construction. tinycloud-sdk-rs/src/serde_siwe.rs:47-48 uses it for wallet hex signatures, and tinycloud-sdk-wasm/src/session.rs:699-709 hashes that CACAO before the new node decoder normalizes it. I exercised real prepare_session → SignedSession deserialization → complete_session_setup → node registration → Session::invoke_any. A valid low-s v=0 signature gives different SDK/node CIDs; the SDK's actual minted proof fails MissingParents. The conventional v=27 session's proof succeeds.
Required: canonical validated signature construction before every client-side CACAO serialization/CID computation. Regression must register both supported wallet v forms and authorize the actual SDK-minted invocations against the returned node CID.
Verified correct / CI
Both recovery parities normalize low-s v aliases to stable conventional bytes/CIDs. High-s rejects before CID assignment; malformed lengths, zero/out-of-range r/s, and invalid recovery bytes reject. Optional null, map ordering, and nonminimal version heads normalize. The existing signature-CID regression passed locally on this exact final head.
The original m1-realdata failure was a PR-related stale nested lock, not a runtime test failure: job 115952524845 exited 101 before tests. The final delta contains only three lock repairs plus changelog; m1-realdata now reports SUCCESS at job 115961763778. This fixes that CI failure, not these correctness blockers. No approval or release authorization is given.
|
Lock-only CI repair is pushed at 63b66ce; this is not release approval. Cargo normal tooling updated |
TC-1071 integrated source/CI repair — release remains heldExact pushed head: Integrated the issuer-identity, historical-CID/revocation, and SDK session-CID fixes, then corrected the introduced cross-backend bootstrap blockers:
All 34 changed paths relative to the initial head. The three standalone Cargo locks remain the only lockfile changes; root Failing-before / passing-after
Exact-head CI: successful, unfiltered
Local supported checks used a fresh corrected target: workspace 1,042 passed (five explicit infrastructure exclusions), DuckDB 473 passed (one PG16-only exclusion), strict all-target workspace Clippy, Wasm SDK/verifier builds and formatting passed. Exact-head CI ran those locally unavailable cases unfiltered; no red CI was waived. Gates still held
Release handoff remains with Miller/Amos after the independent gates. No production access, stored-secret reads, local Docker changes or Sam-board writes. Only integration-owned temporary runners were removed. |
samgbafa
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES — P1 remaining revocation consumer
Reviewed head: 3af478809e97ae542aab06b10eafd94434b767cf. This is a fresh independent review, not a carry-forward of the earlier head. The original core upgrade, issuer-normalization, and SDK CID regressions now pass. The migration is still incomplete at Policy/v3 account-owner admission.
P1: Policy/v3 ignores effective revocation of equivalent historical CACAO IDs
authenticate_account_owner (tinycloud-node-server/src/policy_v3.rs:5460–5465) and validate_locked_account_owner (:5498–5503) still query only revocation.revoked == row.id. The new alias-aware core correctly reports both equivalent rows revoked, but these admission checks accept the sibling whose exact ID is not the signed revocation target. load_signed_delegation validates stored bytes/projections; it does not enforce effective revocation.
I exercised the actual production authentication and locked-recheck functions from an exact source copy with only an appended access wrapper. Local SQLite, throwaway signing keys, real signed delegations and real SpaceDatabase::revoke; no production access or tracked source edits:
| Signed revocation target | Direct target admission | Equivalent sibling admission | Core status of both |
|---|---|---|---|
| historical low-v CID | denied, account-authorization-inactive |
authentication succeeds; locked recheck Ok(()) |
Revoked |
| canonical CID | denied, account-authorization-inactive |
authentication succeeds; locked recheck Ok(()) |
Revoked |
Both denial-asserting smoke commands exit 1. This is an actual authorization-function regression, not a claim that I exercised a complete policy-mint HTTP request. This focused credentials fixture uses real signatures and the public generic decoder to preserve historical wire bytes; the genuine pre-fix database proof below is separate.
Minimal requirement: route both checks through effective alias-aware revocation, including the in-transaction recheck, and coordinate its lock with the canonical identity used by revocation. Add both-direction account-bound credential admission tests and a revocation/admission ordering regression. Audit the remaining exact-ID ancestry check in legacy share_v2 while completing the consumer cutover; that additional path is a source-review concern, not a second independently reproduced finding here.
Migration answer and independent passing evidence
An alias backfill plus ongoing equivalent-CID revocation lookup is necessary; rewriting historical signed IDs/bytes is not. The implemented core mechanism now passes my retained genuine f9d700bf database regression: old-CID revocation remains effective after identical-header canonical reimport, including inverse and both-preexisting-row cases. Earlier behavior at 9998fef did not.
On this exact head, using my own target-astra-final-3af4788:
- Retained old-DB probe, inverse probe, issuer-alias probe, and separately retained revoked histories with no canonical successor row: all exit 0; real signed
kv/listremains denied before and after canonical reimport. - Independently executed historical API runner over private copies of genuine old fixtures: SQLite and native loopback PostgreSQL, six signature/CBOR/issuer encodings, root and child invocations, both revoked directions, canonical-absent history, reimports and new-write single-row deduplication: 0 failures.
- Native PostgreSQL concurrent constructors: 2/2 ready, 12/12 aliases, original signed rows unchanged. Pending and completed encrypted-history startup rejects missing/wrong keys; correct key permits all original root/child signed reads and preserves ciphertext/IDs.
- Real issuer HTTP smoke: active
200→ signed revoke → equivalent reimport same CID and denied401; changed signed fields and malformed/high-s signatures rejected; independently signed distinct grant accepted. - Real loopback TCP SDK smoke: v0/27 and v1/28 each agree with node CID; all eight signed put/get requests return
200with expected values. JSON and direct Rust SDK integration tests also pass both parities. - Historical list/chain raw-header CID regression passes. Additional signed two-parent UCAN smoke preserves the exact proof order and codecs
[0x71, 0x55], retains the original JWT bytes, and successfully invokes the child. - 22 focused tests passed: historical upgrade 8; alias bootstrap/key/concurrency 7; signature-CID 1; SDK 2; issuer 3; raw list/chain response 1.
Commands (common environment: owned Cargo target, four build jobs, dev debug info disabled; native PostgreSQL only at 127.0.0.1:34941, unique disposable schemas):
cargo run --locked -p tinycloud-core --example tc1071_review -- probe <owned-db-copy>
cargo run --locked -p tinycloud-core --example tc1071_review -- probe-both <owned-both-rows-copy>
cargo run --locked -p tinycloud-core --example tc1071_review -- variants <owned-db-copy>
cargo run --locked -p tinycloud-core --example tc1071_review -- probe-absent <owned-canonical-absent-copy>
cargo run --locked -p tinycloud-core --example tc1071_historical_smoke
cargo run --locked -p tinycloud-core --example tc1071_boot_race_smoke
cargo run --locked -p tinycloud-core --example tc1071_postgres_keys_smoke
cargo run --locked -p tinycloud-core --example tc1071_proof_order_smoke
cargo test --locked -p tinycloud-core --test historical_cacao_revocation -- --nocapture
cargo test --locked -p tinycloud-core --lib models::delegation_cid_alias::tests -- --nocapture
cargo test --locked -p tinycloud-core --lib siwe_signature_encodings_have_one_delegation_cid -- --nocapture
cargo run --locked -p tinycloud-node --example siwe_issuer_smoke
cargo run --locked -p tinycloud-node --example sdk_session_cid_smoke
cargo test --locked -p tinycloud-node --test sdk_session_cid --test siwe_issuer_identity -- --nocapture
cargo test --locked -p tinycloud-node --lib historical_list_and_chain_json_preserve_raw_header_cids -- --nocapture
cargo run --locked -p tinycloud-node --example tc1071_account_alias_smoke
cargo run --locked -p tinycloud-node --example tc1071_account_alias_smoke -- reverse
The last two commands are the failing P1 checks. Review runners are throwaway harnesses, not proposed PR source changes. Local evidence and precise fixture/source-copy provenance were sent privately to the integration owner and Miller.
CI, API and release gates
Live GitHub checks were independently inspected for this exact head: Rust, image, and Release succeeded; conditional release/deployment jobs are skipped. The original stale nested-lock failure is resolved, not waived.
MySQL full-constructor support remains unapproved: the reported pre-existing initial-schema error 1074 is a separate held platform prerequisite; neither PostgreSQL CI nor alias-subsystem tests establish full MySQL support. Stop old-version writers before cutover. The documented Rust API cuts (new_with_encryption, serialized list/chain wrappers, and InvalidSIWESignature) remain downstream integration considerations.
The configured GitHub identity is the PR author (samgbafa), so this explicit REQUEST_CHANGES is published as a comment review rather than a formal self-review. COMMENTED state is not approval and does not satisfy either required final-head approval. No merge, deploy, release, or board-write authorization is implied.
samgbafa
left a comment
There was a problem hiding this comment.
APPROVE — independent Sol code-correctness verdict
Exact reviewed head: 3af478809e97ae542aab06b10eafd94434b767cf. Live head and completed CI were rechecked immediately before publication. No remaining introduced correctness blocker observed in the reviewed TC-1071 paths. This supersedes my three P1 findings on the older head for this exact revision only.
Publication restriction: the configured GitHub identity is the PR author. This explicit APPROVE verdict is published through COMMENTED transport, not a native APPROVED review. It does not waive a formal-review requirement or authorize release, merge, or deployment.
Independently observed fixes
- Historical compatibility: restored my original failing harness and generated fresh genuine f9d700b SQLite state in an owned source archive with independent throwaway keys. Historical grant/revocation CIDs and signed bytes remain addressable and unchanged. Identical-header canonical reimport now remains Revoked and its signed invocation denies, rather than creating usable authority.
- Expanded old-data transition: five additional genuine own f9 databases covered both revoked directions, six accepted v/CBOR/issuer representations, real signed descendants, active encrypted history, and an already-revoked legacy root with no canonical row. Exact-final-head upgrade/reimports/restart preserve original IDs, ciphertext, revocation targets and signed proof edges. All equivalent roots and descendants deny after owner revoke; new redelegations do not resurrect them. Active history works before revoke.
- Issuer identity: same SIWE message/signature issuer-case forms now share the node CID. My independent native flow denies both after real revoke. Actual Rocket HTTP regression was active 200 → signed owner revoke → same-CID reimport → revoked 401. Invalid signatures and changed signed fields reject; independently signed distinct grants remain distinct.
- SDK: the formerly failing native SDK proof now computes the node CID and authorizes. Actual SDK JSON and direct-Rust imports for v0/27 and v1/28 register and execute signed put/get through the node with 200 responses and expected bodies. Malformed/high-s signature imports reject before CID computation.
- Response/proof integrity: own active/encrypted capability lists and chains retain the exact historical raw header under its advertised CID. Full signed parent CID codecs/order survive. A real multi-proof child using ordered 0x71/0x55 references executes before alias revocation and denies for ancestor revocation afterward.
Bootstrap and local verification
Reviewed all 34 changed paths and API cutovers. Persistent aliases are backfilled transactionally; PG/MySQL lock the existing migration-history row before the first historical consistent read; SQLite retains BEGIN IMMEDIATE. No signed-history rewrite, conflict-ignore, or mutable alias cache. New grant and alias writes commit together.
Own native PostgreSQL 14 schemas populated from genuine old revoked/encrypted records produced 2/2 ready full constructors under forced overlapping lock waits, with original rows/ciphertext/edges/targets unchanged. Correct keys preserve actual old root/child KV list behavior; missing/wrong keys reject pending and completed history. Own schemas removed. Actual MySQL 8.0.46 alias migration/backfill subsystem likewise produced 2/2 ready transactions; correct-key acceptance, pending/completed wrong/missing-key denial, full unpadded hash preservation and unchanged ciphertext were exercised in my own temporary database, then removed.
Locally exercised: historical consumer regression suite 8 passed; SDK+issuer HTTP suites 5 passed; alias/bootstrap target 7 passed, including native PostgreSQL concurrency; independent baseline/upgrade, signature-boundary, SDK, issuer, multi-proof, PG full-constructor and MySQL subsystem smoke. Unique review-owned targets only; no production, stored-secret, Docker/iptables, board, other-worktree, tracked PR source edit or push.
Exact CI and remaining gates
Exact Rust CI, image CI, and Release CI succeeded; conditional deployment skipped. Verified synthetic merge 84d9786796b421873afbd3aa7de7548461cf5448 has parents f9d700b and this head and the identical Git tree. Parsed CI log totals: workspace 1,047 passed / 0 failed / 8 existing ignored / 0 filtered; DuckDB 474 passed / 0 failed / 0 filtered. Explicit PG16.15 ICU, new concurrency and all four privileged blackhole cases passed in CI; these are not local PG14/privileged-network claims. Root Cargo.lock checksum unchanged.
MySQL full-schema/full-constructor gate remains HELD and unwaived. Alias-only checks do not establish end-to-end MySQL support. The separately reported genuine unmodified-f9 error 1074 on default-strict utf8mb4 precedes this alias migration; it was not rerun just to confirm the owner's report. Stop old-version writers before cutover. Documented Rust constructor/outcome/error API cuts require downstream caller attention. A fresh second independent verdict, any formal-review requirement, and Miller/Amos release authority remain separate. No release/merge/deploy approval is given.
samgbafa
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES — P1 account-auth consumer; prior Sol APPROVE withdrawn
Exact reviewed SHA: 3af478809e97ae542aab06b10eafd94434b767cf. I explicitly withdraw and override my earlier code-correctness APPROVE for this head: prior Sol review. The revised verdict is REQUEST_CHANGES, not merely an operational release hold.
I independently inspected and exercised the discrepancy reported in Astra's same-head REQUEST_CHANGES review. Astra's P1 is confirmed; my passing core migration/issuer/SDK checks did not cover this separate account-admission consumer.
P1: Policy/v3 account admission and its locked recheck ignore equivalent-CID revocation
authenticate_account_owner still checks revocation.revoked == row.id at tinycloud-node-server/src/policy_v3.rs:5460–5465. validate_locked_account_owner repeats an exact-ID check at :5498–5503. Account-bound claim-session admission calls these at :2158 and :2459. The new core alias-effective predicate is not used at either boundary. load_signed_delegation validates stored bytes/projections, not revocation liveness.
Own exercised observations: newly generated throwaway wallet/holder, local SQLite and local file blocks; real signed credential-space host grants and real SpaceDatabase::revoke. Both account proofs passed actual authentication and locked recheck before revocation. After revocation:
| Signed revocation target | Direct target | Equivalent sibling | Core status of both |
|---|---|---|---|
| historical low-v CID | authentication denied, 403 account-authorization-inactive |
canonical proof authenticated; locked recheck Ok(()) |
Revoked |
| canonical CID | authentication denied, 403 account-authorization-inactive |
historical proof authenticated; locked recheck Ok(()) |
Revoked |
The denial-asserting own smoke exits 1 with 2 effectively revoked sibling account proofs admitted by actual private auth and locked recheck. This is a confirmed failure of the actual private account-auth functions, not a claim that a full policy-mint HTTP request was exercised.
The module was freshly copied from my pinned production source, SHA256 69895b97b78ec2ef6d41ce4daab3e0c5e7e96558f1b783e182586bf306bdebde, with its entire prefix unchanged and only an access wrapper appended. The wrapper calls the two original functions without replacing their checks. A public generic HeaderEncode adapter preserves legitimate historical raw wire bytes for fixture construction; normal decoding registers the canonical row. This account fixture is not represented as a genuine f9-generated database; my separate genuine f9 old-state regression evidence remains valid. No PR source edits or another worktree were used.
Minimal correction and acceptance
Route both account-auth checks through the shared alias-effective revocation logic. Keep the locked recheck on its existing transaction and coordinate admission with the canonical identity/lock used by revocation; locking only a sibling's exact row is not an equivalent-revocation ordering guarantee. Preserve historical signed IDs, bytes and targets. Add permanent account-bound admission tests in the production module for both revocation directions, direct-target controls, locked rechecks, and revocation/admission ordering. Audit the remaining exact-ID consumers while completing the cutover; the legacy share ancestry path is a source concern, not an independently exercised second finding here.
Core upgrade/reimport/root/descendant revocation, issuer identity, SDK CID, and bootstrap checks still pass. Green CI does not cover or excuse this P1. Await a new corrective SHA and fresh independent reviews; neither this withdrawn approval nor any old-head approval counts for it.
Publication uses COMMENTED transport under PR-author credentials, not a native review state. It is explicitly REQUEST_CHANGES, provides no approval, and waives no formal-review, MySQL full-constructor, release, merge, or deploy gate. Miller and the sole push owner are being sent the exact verdict and public link. No production, stored-secret, Docker/iptables, board, merge or deploy actions.
samgbafa
left a comment
There was a problem hiding this comment.
APPROVE — code-correctness, source/CI-only
Exact reviewed head: bed0cf9ca53d748846d6d411fbfa280516b36b7f; base: f12ad15fc847683a8feb542d154271b023016d3f. Fresh review of the complete 42-path PR diff and affected consumers in my isolated checkout. No remaining evidence-backed TC-1071 correctness blocker found. This supersedes my 3af4788 REQUEST_CHANGES for this head only; it does not inherit any earlier approval.
This is not release authorization. Node 1.20.7 remains OFF production under the separate TC-1176 incident hold.
Corrective source review
- Both Policy/v3 account boundaries are fixed.
authenticate_account_owneracquires canonical shared chain guards and usesrevocation::is_revoked;validate_locked_account_ownerrepeats that effective lookup in the admission transaction.AccountOwnerProofowns the guards, remains alive across the transaction commit, and is acquired before space-sequence/writer/SQL locks. The same canonical-key registry is used exclusively by revocation. New regression source covers both alias directions, unrelated active grants, a previously authenticated proof after a separately committed sibling revocation, and same-registry writer contention through commit/proof release. - Share v2 is included: both
cid_revokedandrevocation_in_ancestryuse the shared effective predicate. Traced owner verification and registered-ancestor consumers through registration, resolution, invocation and delivery. New tests exercise both historical/canonical revocation directions with valid signed grants and an unrelated positive control. - Historical identity remains intact: canonical identities are an additional persisted index, not rewrites of delegation IDs, signed bytes, revocation targets or proof edges. Reviewed symmetric revocation queries, canonical-absent history, canonical lock keys, transactionally inserted aliases, bounded startup backfill, encrypted-history key validation, and PostgreSQL/MySQL backfill serialization before history reads. Raw list/chain output retains stored headers and full signed proof CIDs/order.
- Signature/SDK cutover: recovery normalization and low-s/scalar checks precede node CID assignment; issuer serialization follows the verifier's parsed identity. JSON and direct-Rust session setup use the checked decoder before SDK CID calculation. Exact UCAN bytes remain unchanged. Reviewed the verifier and response consumers, regression source and documented public Rust API changes.
- Main/W5 integration: the restart fixture now registers genuine signed grants through
/delegateand uses returned CIDs for subsequent reads; it no longer plants invalid synthetic serialization/ID pairs. Restart, returned-content and write-fence assertions remain. The webhook merge retains encryption supplied before startup and main's test-only egress policy. Lockfile changes are limited to the required CACAO dependency edges and W5's local node version alignment.
Evidence and limits
I ran no builds, tests, database connections, local services, Docker, or heavy workloads for this review. The TC-1176 worker's exclusive host reservation was respected. My commands were source/provenance inspection: git status --short, detached checkout, git diff against actual main and corrective predecessors, scoped source searches/reads, git ls-remote, gh pr view ... --json headRefOid,baseRefOid,state,statusCheckRollup,url, and small byte/hash comparisons.
I independently observed live check statuses for this exact head: 10 SUCCESS, 8 conditional SKIPPED, 0 failures across Rust, Docker, and Release. Deployment is skipped. Workflow source configures PostgreSQL 16, enables the privileged blackhole tests and runs unfiltered workspace/DuckDB suites; M1 uses its locked standalone command.
Raw per-test CI logs were not retrieved. I did not retry the reported REST-log 403 or switch credentials. I do not claim exact new CI test totals, individually observed ICU/four-blackhole case output, or an independently verified synthetic-merge checkout tree. The live job results plus committed workflow, complete source review and provenance-checked owner evidence suffice for this bounded code verdict—not for a liveness/release verdict.
Owner evidence is not my execution. Their packet reports workspace 1080, DuckDB 473, M1 13 and final W5 11 passing, with explicit local exclusions and native historical/account/constructor smokes. The broad local checks name the integrated merge tree; I verified that the subsequent final commit changes only the W5 fixture. The preserved W5 source and workflow equal this checkout byte-for-byte; the owner account-smoke module has this head's exact production-source prefix. The packet's astra-* failure logs explicitly name old 3af4788 and are before-fix evidence, not final-head failures.
Verified all 40 sealed content hashes against SHA256SUMS.json; manifest SHA-256: de8db6a8d06e16645b68d17ecad8361e918460236e1b188d05e10d5bc8069168. Packet: /tmp/tc1071-main-integration-handoff-b6c4151-qh1wx2bm.
Unwaived holds and residual risks
- W5 shutdown diagnostic remains unresolved. The preserved isolated restart run emits eleven Tokio-worker “context ... being shutdown” panics before reporting the test passed; the full suite reports 11 passing. Exit 0 does not prove those worker failures benign. Attribution and runtime safety remain unestablished. I found no source evidence assigning them to this PR, and make no claim connecting them to—or separating them causally from—the production incident. Preserve this as an open diagnostic/release-risk item.
- TC-1176 / node 1.20.7 production read-outage hold remains. Short tests, green CI and this code review do not establish prolonged mixed-workload liveness. Amos/Miller retain release authority; no merge/deploy instruction is issued.
- MySQL full constructor remains unapproved. Alias-subsystem evidence does not waive the older schema error 1074 or establish full-platform support.
- Canonical graph guards coordinate a database instance and its clones; they are not distributed locks. The SQL recheck rejects a sibling revocation committed before that check; no cross-process linearizability claim is made. Stop old-version writers before the security cutover, and account for the documented Rust API changes.
- This is an explicit APPROVE in a COMMENTED review under the shared PR-author identity. It is not formal GitHub approval and does not waive formal-review or second-independent-review gates.
No PR source edits, pushes, merge, deploy, production probes, secret access, other-worker checkout access, or board writes.
samgbafa
left a comment
There was a problem hiding this comment.
Sol independent final-head review: APPROVE — source/CI-only, not release authorization
Exact head: bed0cf9ca53d748846d6d411fbfa280516b36b7f. Actual main/base reviewed: f12ad15fc847683a8feb542d154271b023016d3f. I independently fetched/pinned this head in my isolated checkout, inspected the complete 42-path diff and affected consumer contracts, read TC-1071 through the configured issue CLI, and reconfirmed the live head/base before this verdict. No old-head approval transfers. I found no additional blocking defect in the reviewed TC-1071 changes within this expressly bounded scope.
Source conclusions
- The previous account-owner P1 is corrected, not merely hidden by passing core status tests.
policy_v3.rs:5460–5481obtains shared canonical chain guards before its effective-revocation query and retains them inAccountOwnerProof;5495–5509usesrevocation::is_revokedon the actual admission transaction. The real mint caller retains the proof through its2459locked recheck and2657commit.SpaceDatabase::guard_delegation_chainsandrevokeresolve identities through the same canonical closure and shared registry, with stable sorted lock ordering. The three permanent production-module account tests cover both revoked directions, a committed sibling revoke from an independent registry, and same-registry writer contention before admission SQL/through commit, with active/unrelated controls. - Share v2 cutover is present at the actual consumers.
revocation_in_ancestryandcid_revokednow use that same effective predicate;verify_owner_delegationstill binds the caller's exact bytes/CID to persisted signed bytes before signature/authority checks. Both signed sibling directions have consumer tests. The remaining server exact-target query found in the audit is a persistence assertion in tests, not account/share admission. - Canonical ingestion and SDK proof IDs agree for the supported alias classes. The checked fixed-array signature decoder validates scalar ranges/low-s and accepts only 0/1 or 27/28, before delegation CID derivation. Header decoding reserializes canonical DAG-CBOR; issuer serialization shares the verifier's strict eip155/u64/address parsing and does not alter the reconstructed signed SIWE message. Signature/time/authority verification precedes persistence. JSON wallet imports and direct Rust
complete_session_setupboth use the checked decoder before deriving the raw-codec proof CID. Malformed/high-s/changed-signed-field and distinct-valid-grant controls are present. - Historical compatibility is an index, not a signed-history rewrite. Boot derives aliases from exact decrypted history, rejects decoding/hash/key failures, and new rows plus aliases commit atomically. Effective single/batched/list revocation predicates relate persisted siblings even when the canonical identity initially has no stored row. Stored delegation IDs/bytes, revocation targets and signed child references remain intact;
load_signed_delegation, list/chain wrappers and HTTPrawoutput retain the exact advertised bytes. Parent fields are parsed from signed source, preserving full CID codecs and order rather than rebuilding them from hash-only edges. Permanent genuine-f9 transition tests cover real root/child invocation, both revoked directions, reimports, encrypted history, canonical-absent history, new-write deduplication, and raw-header/CID correspondence. - Boot serialization/key boundaries are maintained. PostgreSQL/MySQL backfill locks the existing alias-migration history row before its first history snapshot/read; SQLite reserves its existing immediate-equivalent write lock first. No alias conflict-ignore or mutable authority cache was introduced. Pending/completed encrypted history requires the column key; current constructors provide it before backfill. MySQL alias hash columns are unpadded VARBINARY(255), and the encrypted sample query uses JOIN plus outer LIMIT.
- W5/main integration is not a production relaxation. The final W5 fixture registers real owner-signed UCAN grants through
/delegateand invokes/restarts using returned CIDs; the quarantine assertion remains a semantic denial. Against actual main, TC-1132 egress/config/quota/hooks/tunnel code is unchanged. The dispatcher delta is test-only encryption-constructor cutover, preserving the test egress-policy argument; the production app delta supplies the existing derived encryption key before boot. Public Rust constructor/outcome/error cutovers are documented and callers migrated; the deliberately f9-only fixture generator is historical source, not a compatibility shim.
Evidence and limits
My new verification was source/CI metadata only. I ran no local Cargo/build/test/database/Docker/service/heavy job and did not access production, stored secrets, other worktrees, or mutate PR source/the owner packet. I verified all 40 sealed packet content checksums; SHA256SUMS.json matches de8db6a8d06e16645b68d17ecad8361e918460236e1b188d05e10d5bc8069168. The packet's policy copy has the exact current 410,970-byte source prefix (SHA256 ff6f7ea3bc4ee19d760903ca9b56dccca5ad24bdf0cc0b3e96eb9a4354f5b355), its W5 source matches this checkout, and its five historical SQLite inputs match the committed fixtures byte-for-byte.
Owner-exercised evidence, not my local rerun or new CI totals: retained logs support workspace 1080 passed/8 ignored/5 explicit filters, DuckDB 473 passed/1 filter, M1 13, W5 11, genuine-f9 SQLite/PG upgrade failures=0, actual PG constructors 2/2 and missing/wrong-key controls, issuer reimport revoked401, SDK loopback put/get200, and actual account private functions in both directions (post-revoke authentication403, cached transaction409, canonical guard contention). Account smoke is not full mint HTTP; its signed raw-wire account fixture is not a genuine-f9 account DB.
Independently observed live exact-head check metadata: 10 applicable SUCCESS, 8 conditional SKIPPED, no failure/pending check. Rust, Docker, Release. The committed workflow configures PG16, unfiltered workspace/DuckDB tests and privileged blackhole tests. Raw new-CI PG16 ICU/four-blackhole individual output, exact new test totals and checkout merge-tree evidence remain unavailable after the reported403; I did not retry raw logs or switch credentials. Those unobserved details are not asserted or waived. W5 is outside the default workspace/CI and its standalone result is owner-local evidence.
Unwaived holds
- TC-1176 remains a distinct unresolved P1. This head includes main1.20.7; 1.20.7 remains OFF production. Source review/green CI do not establish 60+minute mixed-workload parity with1.20.6 and do not diagnose, cause, fix, or clear that incident. Host reservation remains respected.
- W5 shutdown-worker panics remain unattributed. I observed the actual
A Tokio 1.x context was found, but it is being shutdownmessages despite successful test exit/body. Source candidates are the blocking SQL/DuckDB actors' retained runtime Handle plus timeout; both candidate files are byte-identical to f9 and current main. [INFERENCE] actor/runtime teardown is a candidate only: no stack/baseline runtime attribution, benign classification, production-safety claim, or connection to TC-1176 is established. - MySQL full-constructor coverage remains HELD. Native alias-subsystem results do not waive the separately established older full-schema1074/default-strict utf8mb4 prerequisite or prove end-to-end MySQL support.
- Stop old-version writers before the shared-DB security cutover. The canonical guard registry coordinates one database instance and clones, not distributed in-flight writers; transactional rechecks must remain.
- Shared author credentials prevent native formal approval. This is deliberately COMMENTED APPROVE, not GitHub APPROVED, and does not waive the formal-review gate, the other fresh independent final-head review, or Amos/Miller release authority. No merge/deploy/release authorization.
This new bounded verdict applies only to bed0. It does not reinstate my withdrawn3af approval; my3af REQUEST_CHANGES and Astra's3af finding describe the earlier defect corrected here. Any corrective/source-changing SHA requires a fresh review.
Local checks passed. TC-1071.
Locked M1 tests, strict lint and formatting passed. TC-1071.
samgbafa
left a comment
There was a problem hiding this comment.
APPROVE — TC-1071
Verified workspace, DuckDB, WASM, M1/W5, signer and local runtime checks. TC-1071; TC-1302.
Verified workspace, DuckDB node binaries, WASM, M1/W5 and local runtime checks. TC-1071; TC-1304.
|
APPROVE: see TC-1302 |
1 similar comment
|
APPROVE: see TC-1302 |
Review F1: on the first boot after 1.20.6, #291's CREATE TABLE delegation_cid_alias ... REFERENCES delegation runs before #296 and takes SHARE ROW EXCLUSIVE on delegation, so it still waited forever behind an open write transaction. Set SET LOCAL lock_timeout = '30s' once on the outer migrate() transaction, next to the TC-1054 statement_timeout exemption, so it covers every boot migration. Remove the per-migration SET LOCAL / TO DEFAULT from #296, which would have reset the outer setting for later migrations. Map 55P03 from anywhere in the migrations to a DbErr::Migration naming the lock wait; sea-orm-migration does not expose the failing migration's name in the error (its 'Applying migration' log line names it). Tests: from the 1.20.6 schema, an open write on delegation makes the boot fail at 30s with no migration applied or recorded and no lock_timeout left on the pooled session; after release the boot applies everything.
* fix: bound lock wait in delegation cursor index migration (TC-1176) On PostgreSQL, m20261015_000000_delegation_cursor_index (#296) dropped and recreated idx_delegation_delegator with no lock_timeout, so a boot blocked indefinitely if any session held a lock on delegation. It now sets SET LOCAL lock_timeout = '30s' for its index rebuild and resets it after. sea-orm-migration runs all PostgreSQL migrations in one transaction, so a lock-wait failure rolls back the index change and the migration row; boot fails with an error naming the lock wait. SQLite and MySQL are unchanged. * fix: bound every boot migration's lock wait on PostgreSQL (TC-1176) Review F1: on the first boot after 1.20.6, #291's CREATE TABLE delegation_cid_alias ... REFERENCES delegation runs before #296 and takes SHARE ROW EXCLUSIVE on delegation, so it still waited forever behind an open write transaction. Set SET LOCAL lock_timeout = '30s' once on the outer migrate() transaction, next to the TC-1054 statement_timeout exemption, so it covers every boot migration. Remove the per-migration SET LOCAL / TO DEFAULT from #296, which would have reset the outer setting for later migrations. Map 55P03 from anywhere in the migrations to a DbErr::Migration naming the lock wait; sea-orm-migration does not expose the failing migration's name in the error (its 'Applying migration' log line names it). Tests: from the 1.20.6 schema, an open write on delegation makes the boot fail at 30s with no migration applied or recorded and no lock_timeout left on the pooled session; after release the boot applies everything.
Normalize delegation identifiers and align SDK handling.
Verification: local checks and exact-head CI passed; fresh review pending.
TC-1071