feat(tool-capability): persist external-extension lifecycle evidence - #574
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (21)
📝 WalkthroughWalkthrough외부 확장 라이프사이클의 증거 검증기와 Durable Object 기반 append-only 저장소를 추가했습니다. CAS, 멱등 replay, 감사 체인, current projection, 손상 감지와 복구 계약을 구현하고 관련 테스트와 아키텍처·운영 문서를 갱신했습니다. Changes외부 확장 라이프사이클
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant LifecycleRepository
participant EvidenceVerifier
participant ForeignAuthority
participant DurableObjectStorage
Caller->>LifecycleRepository: active 전이 append
LifecycleRepository->>EvidenceVerifier: 현재 증거 검증
EvidenceVerifier->>ForeignAuthority: Policy/Approval 및 receipt 조회
ForeignAuthority-->>EvidenceVerifier: 참조와 digest 반환
EvidenceVerifier-->>LifecycleRepository: 검증 결과
LifecycleRepository->>DurableObjectStorage: CAS로 event, index, head 기록
DurableObjectStorage-->>LifecycleRepository: 커밋 결과
LifecycleRepository-->>Caller: accepted 또는 replay 반환
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Exact-head review finding on 2d7ea9a118a4768cbbc11df680f7b6a46a5f644a: the new preflight replay integrity checks do not protect the transaction-time loser path. If preflight sees no transition index, then another writer commits (or durable evidence is corrupted) before this transaction acquires the serialization point, the existingIndex !== undefined branch returns existingEvent after structural field checks only. It does not recompute requestHashMaterial(existingEvent) / eventHashMaterial(existingEvent) or verify the current audit tail, so this race can return evidence that the normal readExistingReplay() path would reject. Keep Web Crypto outside the short storage transaction as #561 requires: have the transaction return a detached replay candidate/reference, then cryptographically validate that candidate and head/tail after the transaction before returning kind: replay (or equivalently re-enter the existing replay verifier without re-reading mutable Policy/Approval evidence). Add a deterministic hostile race where preflight misses the index, a competing commit/tamper lands, the transaction sees the index, and a request/event digest or tail mismatch fails closed. Do not weaken the 100% coverage gate; current application CI 34348143585 is already RED in release tests, so the repair should make both this authority gap and the exact-head coverage failure GREEN.
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Follow-up exact-head finding on c3731898ab1af7c9cdd2acff51b9c054328794f2: the post-transaction replay verifier closes the digest/tail race from my prior review, but NEW active requests still have a narrower idempotency race between the initial readExistingReplay() miss and assertCurrentActivationEvidence(). If writer A commits this exact transition after writer B's preflight miss, and Policy/Approval or owner evidence is revoked before B's live verifier returns, B throws ExternalExtensionLifecycleEvidenceError even though the exact transition ID + request digest is already durably committed. That contradicts #561 invariants that exact duplicates are idempotent replay and historical committed events remain evidence rather than live authority. Preserve fail-closed activation for genuinely new transitions: on activation-evidence failure, re-run the immutable replay check before propagating the evidence error; return replay only if the exact committed request now exists and passes full digest/head/tail verification, otherwise throw the original evidence error. Add a deterministic barrier regression for preflight miss → competing exact commit → evidence revocation/failure → immutable replay. Do not move foreign-owner verification into the storage transaction or weaken fresh-evidence requirements for the winner.
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Blocking finding on exact 74b55559efa85ec44245ed25442d0db184945889: the NEW active evidence verifier still does not bind the activation event to the approval's product/role scope or to an exact approval identity. TrustedExtensionPolicyApproval carries allowed_product_repositories and allowed_execution_roles, but assertApprovalCurrent() never checks them; the hostile test that claims to vary every authority-bearing approval field omits both. A live approval may therefore narrow repository/role scope immediately before append and the activation can still be accepted. Separately, policy_approval_reference and effective_scope_reference are persisted request URNs but are not derived from or checked against the approval object returned by resolvePolicyApproval(extensionId), so the append-only audit event can name an arbitrary syntactically valid approval/scope reference while revalidating a different current approval.
This violates #561 invariants 4/5 and its buyer requirement to prove which exact Noema Policy/Approval plus effective product/role scope caused each activation. Keep foreign-owner truth as references/digests; the repair belongs in Noema's approval port: expose/bind a canonical approval identity and canonical effective-scope identity (or exact scope digest derived from the approved repo/role sets), add REDs for repo-scope drift, role-scope drift, forged approval reference and forged scope reference, then make NEW activation fail closed on any mismatch. Exact committed replay should continue using immutable stored evidence without reconsulting mutable authority.
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Second blocking recovery finding on the same exact: readAudit() verifies every event's stream/hash chain and then checks only head.version/state/head_event_sha256 against the last event. It does not verify head.schema_version or head.stream against the requested stream/tail. A persisted snapshot whose stream identity is corrupted/substituted can therefore pass the full audit/recovery path even though readCurrent() and replay verification would reject it. #561 explicitly requires malformed persisted snapshots and forged/truncated snapshot-event-prefix continuity to fail closed. Add a hostile RED that tampers only the persisted head stream (and one for head schema version), then make readAudit() enforce the same exact head schema/stream binding before treating the chain as recovery evidence. This should remain O(n) only on audit/recovery; do not move full-prefix work back into readCurrent().
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Fresh exact-head finding on 7cae6b6c5a62a614cf35e40fa0645623651ae28f: AuthorityBackedExternalExtensionLifecycleEvidenceVerifier.assertApprovalCurrent() trusts Date.parse() + finiteness for valid_from / valid_to, while the canonical external-extension admission boundary already requires an exact UTC instant by round-tripping new Date(parsed).toISOString() === input. ECMAScript normalizes impossible calendar dates such as 2026-09-31T11:00:00.000Z to October 1, so a Noema Policy/Approval can carry impossible/non-canonical authority time bytes, derive a matching approval SHA-256 reference from those bytes, and still be treated as live. That weakens #561's requirement that the exact Noema Policy/Approval and validity window be reproducible authority. Add a hostile RED using a request reference computed from an impossible-but-parseable approval timestamp, then make the lifecycle evidence port reject any approval time that is not a real canonical UTC instant. Keep exact committed replay semantics unchanged and do not weaken the existing approval-reference binding.
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Fresh exact-head review found one Noema-owned validation gap in the lifecycle ledger. The approval window now rejects impossible-but-parseable UTC instants, but the event occurred_at path still accepts them. Keep Draft and repair with a hostile regression plus the same canonical UTC round-trip invariant; do not weaken lifecycle semantics or foreign-owner evidence checks.
1 participant
Noema Tool Capability / State / Checkpoint lifecycle evidence
This Draft owns only Noema external-extension lifecycle authority: transition/version/head state and immutable Noema Policy / Approval plus AppGuardrail, quarantine/isolation, and Egress references/digests. It does not copy foreign scanner/runtime/outbound truth or contextual-orchestrator provider/model routing into Noema.
Reality RED → causal repairs
The branch preserves fail-closed lifecycle/state invariants through hostile findings and minimal repairs:
readCurrent()verifies compact head + exact tail in O(1) storage cardinality while complete prefix verification remains audit/recovery work;occurred_atvalues must round-trip to exact canonical UTC instants before they can become immutable authority.Historical exact-head review findings remain preserved in the review timeline. The latest causal source repair before reconvergence was
0e7b24ddad4f16e378f22d6b16ff7cbe842a8aa1, which rejects impossible-but-parseable lifecycle occurrence instants without weakening replay or foreign-owner evidence checks.Current exact authority
Protected main advanced through the independent hourly-lane and agent-handoff changes without touching this PR's 21 owned paths. This lane therefore ordinary/non-force reconverged by a two-parent merge onto protected
main@f83d42817ac90aad4159e7b8649eed5b9a6f8740. The post-reconvergence compare is ahead-only and contains exactly the same 21 lifecycle files, with no protected-main path lost.Current exact is
f3c4343a3d764fe380c0f9ad5b26fb108e4960b5. All predecessor-head GREEN is invalidated by the new merge commit. Keep Draft until application CI, reviewer-ci, Security Scan, and patch-validator-image are all GREEN on this unchanged exact and fresh review/base remain clean.Canonical documentation / operability
The candidate keeps lifecycle treatment aligned through
docs/CONTEXT_MAP.md, Proposed ADR 0015,docs/TEST_STRATEGY.md,docs/OPERABILITY.md,docs/TRACEABILITY.md,docs/external-extension-lifecycle-recovery.md,docs/PRD.mdFR-023,docs/TRD.md, rootARCHITECTURE.md, anddocs/UML.md.docs/product-technical-gap-baseline.mdis not silently reconstructed from partial bytes; issue #5 remains its dedicated authority.Issue #561 still requires realistic actual Durable Object current-projection and contended-append p95 <= 20 ms where synchronous, partition/lock/storage-growth evidence, snapshot/audit rebuild and recovery rehearsal. Unit-test timing and the O(1) algorithmic bound do not satisfy that operational acceptance. Source integration remains separate from immutable release, deployment, pilot and production KPI authority.
Summary by CodeRabbit
새 기능
문서
테스트