Land high-value improvements tranche 1 (M0–M3): architecture gate, turn integrity, security profiles, credential vault, retry identity, occurrence identity#237
Open
hwuiwon wants to merge 6 commits into
Conversation
…rity, security profiles, credential vault, retry identity, occurrence identity Implements seven P0 tasks (0.1, 0.2, 1.1, 1.2, 2.1, 2.3, 3.1) from docs/engineering-discipline/plans/2026-07-22-moa-high-value-improvements.md, each with full verification and a fail-then-restore mutation ledger: - resurrect the architecture checker with pre-scan path validation, honest ratchets (ADR 0003), repository-extracted SQL, and an injected event appender replacing raw OrchestratorCtx::current() - fix both standing baseline failures: the stale journal-role fixture and a contact-group ingestion race (23505/40P01 vs a non-arbiter unique index), now guarded by a per-group advisory lock with DB errors preserving SQLSTATE/constraint/DETAIL - one canonical TurnFailed fact per actor+turn with FIFO disposition for failed/cancelled outcomes, worker-neutral classification, and a closed allowlist preserving typed policy-rejection codes - explicit Local/Cloud security profiles with fail-closed cloud construction and hard-deleted local-provider escape hatches - durable encrypted tenant credential vault (V000346) with append-only audit, bounded-batch tenant purge, reserve/claim/finalize link flow (V000358), Nango/Merge trigger idempotency, and non-serializable secret handoff - required ClientMessageId with a canonical-request-hash admission fence, deterministic create-only attachment slots, an exact reply matrix, and fence-checked SSE reconnect; Session/post_message deleted - chunk_uid becomes the graph occurrence identity (V000347 backfill with a replay-vanishing predicate); hydration DISTINCT ON removed; deletion covers every active version Also: bounded TestDb drop cleanup (fixes an intermittent 240s db-memory hang), pinned transformers 5.14.1 for the PII sidecar, repaired two stale planner-contract assertions, and un-ignored 13 vault DB tests the milestone gate was silently skipping. Verification: fast-pr 2,924 passed / 0 failed (baseline 2,872 with 2 failures); clean-e2e --live passed end-to-end twice (runs 20260724193230 and 20260724214424); workspace clippy -D warnings, build, fmt clean; 40+ mutations each failed a named assertion and were restored green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KoeipyuEzdLi5yw5QiHd9e
|
Important Review skippedToo many files! This PR contains 222 files, which is 122 over the limit of 100. To get a review, narrow the scope: Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (222)
You can disable this status message by setting the ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…indings - db-tests: per-test-schema auth migrations reference moa_app but exclude V000001, which creates it; on a pristine CI cluster the bootstrap raced whichever full-template build ran first, and the new vault tests reordered the schedule so the auth harness now runs first and loses. Create the three shared roles idempotently in install_shared_extensions_locked, under the same advisory lock that already serializes extension creation. - restate-fixture-smoke: rename knowledge_caller to authorize_knowledge_caller so the handler-authz scan sees the authorization it performs, and repoint the turn_events trace-manifest sender from append_session_event to append_with_identity, where the replay-safe request now lives. check-architecture-boundaries exits 0; auth_providers_db 32/32; orchestrator lib 409/409; manifest tests 4/4; fmt, clippy -D warnings, git diff clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KoeipyuEzdLi5yw5QiHd9e
… race Main's db-tests has been red on the auth-harness role race, so fail-fast never reached these; the previous fix unblocked the pipeline and surfaced them: - execution_db: the shared budget helper built nanosecond deadlines that only equalled their Postgres round-trip when the clock landed on a whole microsecond; truncate up front so equality is exact, not clock-lucky - messaging rate-limiting: the paused-clock wait helper budgeted 100ms of virtual time, less than a legitimate limiter-refill or jittered backoff on a loaded runner; widen to 5 virtual seconds (zero wall-time cost), full suite stress-verified 10x - rustfs: pin rustfs/rc and rustfs/rustfs by locally-verified digest — CI's fresh `:latest` pull crashed init with exit 7 while local caches kept a working revision, the same unpinned-image drift the PII sidecar hit Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KoeipyuEzdLi5yw5QiHd9e
…smoke workflow that could never pass - moa-ocsf: concurrent first-event signers ran full rotate_key, so a signer arriving after the winner's commit deactivated the winner and minted a second key generation — signers could observe different key ids (caught by CI's sign_cached concurrency test). First use is now create-if-absent against the partial unique active index and never deactivates anything; rotate_key remains for explicit rotation only. - messaging rate-limiting tests: the paused-clock wait helper busy-spun through its iterations in microseconds of wall time, so on loaded runners the loop exhausted before the OS ever delivered the localhost round-trip; which test in the family died was scheduling luck. Each iteration now awaits a 1ms blocking-thread sleep so the runtime gets real windows to drive I/O, alongside the virtual advance that funds limiter and backoff schedules. - integration-tests workflow: pointed at ports nothing publishes (18080/9070/25432 vs compose's 10010/10011/10040) and forced the external orchestrator env onto the scripted fixture that refuses one by design — the smoke could never have passed as written. Ports corrected and the two tests split into external-orchestrator and dedicated-fixture steps, mirroring the clean-e2e ladder's profile split; timeout raised for the fixture build the always-bailing invocation never paid for. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KoeipyuEzdLi5yw5QiHd9e
The previous fix repaired one site of a platform-dependent class: Utc::now()-derived deadlines carry nanoseconds, Postgres round-trips microseconds, and equality assertions against read-back budgets therefore pass on microsecond-granular macOS clocks while failing almost always on nanosecond-granular Linux CI clocks. Local runs were green by platform, not by correctness. Every DB-reachable budget deadline construction now truncates to microseconds: a shared pg_deadline helper in execution_db plus the replan_budget and execution_service_db fixture sites. Remaining Utc::now() fixtures were audited as offline/mock-only. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KoeipyuEzdLi5yw5QiHd9e
…ture The dedicated scripted fixture boots its own testcontainers stack and orchestrator; alongside the still-running compose stack the two-core runner starved badly enough (218s for one scripted turn) that the route classifier's bounded call timed out and the router took its deliberate conservative Inline fallback, so the expected Accepted outcome came back Completed. The compose-backed smoke passed with the corrected ports, so tear the compose stack down between the two steps. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KoeipyuEzdLi5yw5QiHd9e
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.
Implements the first tranche of the high-value improvements program
(
docs/engineering-discipline/plans/2026-07-22-moa-high-value-improvements.md):seven P0 tasks across M0–M3, each with full verification and a
fail-then-restore mutation ledger.
Completed tasks
OrchestratorCtx::current()eliminated via injectedTurnEventAppender<response_schema>contract; contact-group ingestion race (concurrent identical upserts vs a non-arbiter unique index → 23505/40P01) reproduced deterministically and fixed with a per-grouppg_advisory_xact_lock; DB errors now preserve SQLSTATE/constraint/DETAILTurnFailedfact with actor+turn dedupe key; coordinator-Terminal / worker-Neutral classification; legacy pending projection deleted; deterministic root+worker catch-all failure e2e; typed policy-rejection codes survive sanitization via a closed allowlistSecurityProfile::{Local,Cloud}; cloud construction fail-closed (Deny default, real rule store, non-local sandbox, credentials present) before serving;allow_local_provider/MOA_CLOUD_HANDS_ALLOW_LOCALhard-deleted; production k8s renders Cloud+Deny+E2B with the E2B secret moved out of baseV000346encrypted versioned credential store + append-only audit (forced RLS, DB-owned one-active-version); typed trait (create/resolve/rotate/revoke/delete_connection/purge_tenant); env vaults deleted; single runtime owner injected everywhere;V000358link-claim table driving a reserve→written→compensating→compensated→finalized CAS flow; Nango/Merge trigger idempotency (no credit-consuming force-resync); plaintext handoff via non-serializableRedactedSecretClientMessageId+ typedreply_to; domain-versioned canonical request hash; Session admission fence consulted before every side effect (24h/256-entry terminal retention, eviction metrics); deterministic UUIDv5 attachment slots with create-only object storage (S3ConditionalPut::ETagMatch); exact reply matrix; SSE reconnect passes the fence;Session/post_messagedeletedchunk_uidIS the graph identity (domain field deleted; DB column constrainedNOT NULL+CHECK(= chunk_uid));V000347backfills occurrence nodes/edges/embeddings with a replay-vanishing predicate and vector-outbox sync; hydrationDISTINCT ONremoved; deletion covers all active versions; content-hash uniqueness index dropped (occurrence identity owns uniqueness)Also in this change
TestDb::Dropcleanup (fixes a client-side hang that intermittentlykilled a db-memory test at 240s under lane adjacency; blackhole-URL
regression pins the bound between 15s and the ~35s OS SYN floor)
transformerspinned to5.14.1(the git@mainpin drifted to a revision incompatible with torch 2.7.1)
response_formatvsin-prompt schema marker) — only visible inside the clean-e2e lanes
#[ignore]d (the plan's own verificationcommand silently skipped all of them)
Verification
cargo nextest run --locked --profile fast-pr: 2,924 passed / 0 failed(program baseline: 2,872 with 2 failures)
MOA_RUN_LIVE_E2E=1 ./scripts/run-clean-e2e.sh --live: passed end-to-end(22 phases; runs
20260724193230and20260724214424, the latter including Task 1.2's fence tests in the shared-orchestrator lane)--all-targets --all-features -D warnings, workspace build,fmt,
git diff --check: clean2.3 (15), 1.2 (6), 3.1 (7) — every mutation failed a named assertion and was
restored green
Recorded deviations from the plan text (all argued in-doc)
AdminReviewis a liftable cautious default (an exactmatched persisted Allow rule wins); only intrinsic
Denyandoperator-configured overrides are unliftable floors. The literal clause
would have made
bashand every MCP tool permanently ungrantable.V000358(plan pre-allocates V000347–357to later tasks); tenant purge is a bounded-batch
purge_tenant(limit, ctx)sweep on the vault trait.
reply_to+attachments is a typed 400); text-only ambiguity still rejects.
Remaining program work (1.3, 2.2, 2.4, 3.2, 3.3, M4–M7) continues on top of
this tranche.
🤖 Generated with Claude Code
https://claude.ai/code/session_01KoeipyuEzdLi5yw5QiHd9e