Conversation
Refs #161. Partial implementation with retained local deterministic evidence; physical multi-host and production-identity acceptance remain open.
There was a problem hiding this comment.
Reviewed commit 00a13ff1cf41bf90fc6efd5a5f1bda17ece92f6b against ECorp's product, architecture, security, and eval contract.
Hold merge: readiness and integration sequencing.
Validation: Reviewed domain/protocol/migration/store/server/CLI authority implementation, UI additions and acceptance evidence. Migration checker passed all 42 migrations. Independent owned PostgreSQL fixture: 6 claim-authority store tests and 3 real-handler server tests passed, with no ignored cases left in these targeted runs. Five Node UI/QA-plan tests passed. Authored browser/server/runner plus two controlled runner acceptance inspected, not independently replayed. Scope explicitly remains partial/nonclosing #161, using one-host development auth and deterministic providers.
-
Both #255 and #283 allocate migration 0042. Resolve shared migration lineage without altering checksums of already-applied migrations before combining them.
-
The ledger UUID is an explicit immutable authority pin, not distributed locking for copied databases. Source drift is rejected at dispatch after the blocked claim/mission, not preclaim.
Readiness follow-up after the merge pass: GitHub currently labels this PR non-draft, but its description still explicitly asks for review as a draft partial contribution and excludes merge. No later readiness comment is present. Reconcile that stated readiness before landing this contribution; the partial, nonclosing #161 scope is otherwise preserved.
The migration-number collision alone would not prevent #255 from landing first. #283 independently needs its reproduced ledger/destination deadlock fixed, and its eventual migration sequence must preserve the migrations already committed and applied. This hold does not assert a new defect in #255's independently tested authority implementation.
afateen
left a comment
There was a problem hiding this comment.
Reviewed against the repository product, architecture, security, evaluation, and CI requirements. The current head is mergeable, all reported checks pass, and I found no unresolved correctness blocker in this bounded contribution. This approval does not claim completion of the explicitly documented follow-up scopes.
|
Addressed the readiness/integration follow-up in
Source-bound follow-up evidence. |
shyamsridhar123
left a comment
There was a problem hiding this comment.
Reassessed 690ee80e692d848c4c3a371ddb31388a78b9ddf3 against current main and the product, architecture, security and evaluation contracts. The old draft-only wording has been superseded: the current description and docs/evidence/issue161/review-integration-followup.md explicitly describe a partial, non-closing implementation ready for fresh review only, with no merge requested.
Source inspection confirms that the integration retains both authority-pin validation and main's planning-cost admission. The added regression covers a valid pin with invalid cost leaving the ledger and operation key untouched, followed by valid reuse. The original migration 0042 and checksum remain unchanged. I found no new demonstrated defect in this follow-up. All 11 observed current-head GitHub checks were successful. The six contributor gates and 15 targeted database/handler cases in the new report are author-provided evidence; I have not represented them as independently executed here.
Keep the PR on hold until the contributor supplies a superseding completed, merge-ready handoff for its bounded scope and current-revision acceptance for the user-visible authority path. The old-head approval and historical one-host rehearsal do not validate this new combined revision. The broader production multi-host and independent identity work can remain tracked separately; this PR need not close #161, and it currently has no closing issue association.
The documented authority-first migration order is useful, but #283's competing 0042 still needs reconciliation against the actual landed lineage before that later PR can merge. Preserve applied migration checksums and the other contributor's branch. This review records the new evidence and current readiness; it does not reinstate the obsolete draft-only wording or assert a new correctness failure.
Description
Refs #161. Ready for review: partial, non-closing shared-authority implementation.
This is not qualification of production multi-host execution, copied writable
databases or independent human/provider identities. No merge is requested.
inspection, CLI comparison, trusted controller pins and scoped UI diagnostics.
preserve existing claim fencing, idempotency, source/actor/Corp boundaries and
legacy omission without rebinding history.
and native planning-cost admission. Add a valid-pin/invalid-cost regression
proving rejected claims leave ledgers and the operation key untouched.
Base: main
aa2ef457d86d727232ae353f53a13c8c9f149bf6.Final head:
690ee80e692d848c4c3a371ddb31388a78b9ddf3.Branch:
codex/issue-161-shared-authority->main; published history preserved.Original reviewed head:
00a13ff1cf41bf90fc6efd5a5f1bda17ece92f6b.Net scope: 39 files in the existing authority domain/protocol/store/server/CLI/UI,
original migration0042, focused tests, QA helpers and source-bound documentation.
The follow-up changes two conflicted store files, extends one existing authority
test and adds one report; other main changes are incorporated, not reimplemented.
Three untracked local draft/patch files remain excluded.
Type of change
How Has This Been Tested?
All six local gates rerun on final integrated content:
cargo fmt --check: passed.cargo clippy --workspace --all-targets --offline --locked -- -D warnings: passed.cargo test --workspace --offline --locked -- --quiet: 561 passed,352 ignored, zero failures. Ignored:1 runner,8 server,343 store; not executed by that generic gate.pnpm build:webandpnpm lint:web: passed.patch retains its previously documented single-space context line; it was not
rewritten to suppress a diagnostic.
Real database/handler execution: a newly owned, disjoint loopback PostgreSQL
fixture applied actual migrations. Six authority-store tests, three authority
real-handler tests and six cost-admission/history tests passed: 15 passed,
zero failed/ignored in those targeted runs. This includes concurrent handlers,
scope denial, immutable identity, legacy replay and authority-plus-cost admission.
Only that PostgreSQL process was started; it was stopped with ownership verified,
port checked closed, and its data/evidence retained. No retained DB or runner was used.
Fresh integration report
records exact commands, the two integration stages and limits. Main's later
multiplayer UI merge changed no Rust/migration bytes; final frontend coverage
includes both the authority notice and the landed collaboration UI.
Historical runtime report
remains one-host/development-identity/deterministic-provider evidence at its old
source, not a newly repeated browser/server/runner rehearsal. Old hosted CI and
Afateen's old-head approval are not verification of this new head. Assess fresh checks.
Test Configuration: Windows ARM64, native PostgreSQL17 on dedicated port55462,
Node24.19.0, pnpm11.19.0, Cargo1.98.1, locked/offline dependencies. No provider SDK,
real model or external GitHub effect was exercised by the database validation.
AI agent integration handoff and migration sequence
and verifier policy. A ledger UUID is an authority pin, not a distributed lock
for copied or independently writable databases.
claim admission. Existing/reclaimed policy history is not rewritten by new validation.
sequence is authority first; the audit contributor must reconcile later migration
allocation against the actual landed lineage without changing applied checksums.
This PR does not modify Implement State Audit Protocol V1 #283, adopt its schema or authorize either merge.
preclaim rejection; Distinguish Factory plan validity from dispatch readiness for deterministic source-pinned work #256 remains the separate plan-validity/readiness follow-up.
current review corrections remain a separate contribution; do not rewrite the
historical source/evidence pin or claim it is newly replayed here.
owners, runner credentials and source/worktree lineage. Multiplayer U9: package private deployment and operator recovery #248/Multiplayer U10: prove the complete three-device journey and release readiness #249 still own
deployment, physical-device, real-provider and independent-identity qualification.
Checklist
This supersedes the old draft-only request: the operator approved these review
corrections and Ready handoff. Keep #161 open. Required reviewer approval remains
independent; no merge, auto-merge, deployment, Factory dispatch or issue closure.