Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #219 +/- ##
==========================================
+ Coverage 93.30% 94.05% +0.75%
==========================================
Files 54 78 +24
Lines 9871 11656 +1785
Branches 252 668 +416
==========================================
+ Hits 9210 10963 +1753
- Misses 614 622 +8
- Partials 47 71 +24
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
lan17
left a comment
lan17
left a comment
There was a problem hiding this comment.
Review of 3414d5f against the #218 design. One item to fix before merge, three non-blocking notes, and a list of what was verified. Inline comments mark the exact spots.
Fix before merge
Python delete has an undocumented Key-instance branch that silently ignores its own arguments. When key is a Key, the required key_type and use_case keyword arguments and track_for_invalidation are discarded without checking that they agree with the Key. Nothing in the tests, docs, interop harness, or examples exercises this branch. It can target a different entry than the caller named, and because a missing entry succeeds the mismatch is invisible. Delete the branch, or make the three kwargs optional when a Key is passed, reject disagreement, and test it.
Non-blocking
deletionsjoined the shared observation record. That one change inconformance-observations.qntis why twenty-plus smoke traces and witness fixtures churned and whydifferential.mjsneededreferenceDescriptorto project a zero baseline into corpora generated fromorigin/main. The bridge is correct and its test is thorough, including the negative controls, and it is self-described as temporary. Track its removal oncemaindeclares the field, and consider a profile-local channel for theputslice so the next maintenance counter does not repeat the churn.- README bullet got long. "Off by default: readers cache only inside
enable(). Maintenance calls (invalidateRemote,delete) always act." reads better than the semicolon one-liner. - Signature nit.
LocalCache.deletetakes a URN string whileputandgettake aDialCacheKey.
Verified
- Ordering and atomicity in every port. Validate, then capability check, then count, then remote, then local and memo with no await between them. Remote failure leaves both memory stores intact. Unsupported adapters fail before any mutation and before the counter. The TypeScript gate test, the Go remote-callback assertion, the Rust
BrokenRemovalstore, and the Python remote assertion each pin this. - Rust locking.
deletetakes the cache state lock, then the owner lock. Every other acquisition site inengine.rs,execution.rs, andscope.rstakes one or the other, never both, so there is no reverse order. Displaced entries drop outside both locks per the existing convention. - Go race safety.
owner.liveis read underc.mu, the mutex the scope'sdonewrites under. - Adapters. One keyed
DEL, slot-primary routing in node-redis, GLIDE, go-redis, and the Rust connection, reply validation accepting only integers 0 and 1, no retry. Cluster integration confirms the same-slot watermark survives. - Formal. Four kernel transitions, eight invariants (seven new), fourteen named regressions, and eight fault probes covering all seven new invariants. Seven native mutants each for TypeScript and Go, matched by seven Rust mutants, spanning C61 to C63 including "unsupported reports success" and "detach flight". The witness classifier reads only public inputs and observations.
- Interop. Writer, deleter, and reader permutations across all languages for tracked and untracked identities, with the watermark asserted unchanged.
- Docs match the implementation everywhere checked, including
DELin the required-command list and the Rust breaking-change notes forLocalStore::remove,Event, andMetricKind.
Merge gate
Draft by design until the full formal workflow and the pending differential shards finish. Every proposed decision in #218 was resolved the way the issue recommended; tick that checklist on merge.
🤖 Generated with Claude Code
| """ | ||
| if use_case == "watermark": | ||
| raise UseCaseNameIsReservedError(use_case) | ||
| if isinstance(key, Key): |
There was a problem hiding this comment.
Fix before merge. This branch discards the required key_type and use_case kwargs and track_for_invalidation without checking they agree with the Key. Nothing in tests, docs, interop, or examples calls delete with a Key, so it is untested dead surface. A mismatched identity would delete a different entry and report success. Remove the branch, or make the three kwargs optional when a Key is passed, reject disagreement, and add a test.
There was a problem hiding this comment.
Fixed in 1a9d212. Removed the undocumented prebuilt-Key branch; delete now accepts a scalar ID or an {id, args} mapping and derives identity from its required keyword arguments. The docstring and Python guide state that boundary explicitly.
Added four regression cases covering matching identity and mismatched key_type, use_case, and tracking mode. Each requires TypeError before adapter dispatch or metrics and checks that local and request-memo values remain intact. All four fail against the old implementation and pass with the fix. Existing structured-key reader/aget behavior is unchanged.
Validation passed: make check-python (2,153 tests), make check-ts (3,553 tests plus typecheck/build/package checks), make docs, and make audit. Current-head CI and full formal verification are queued/running; the PR remains draft.
— Levicus 🤖
| // pinned reference source declares the older record; candidate traces and | ||
| // reference sources that already declare the counter keep the strict parser. | ||
| // Remove it once supported reference revisions all declare `deletions`. | ||
| export function referenceDescriptor(tree, profileId, descriptor) { |
There was a problem hiding this comment.
Correct and well tested, including the negative controls. Once this merges, origin/main declares deletions and this becomes dead code, so please track the removal the comment above promises. For the put slice, a profile-local observation field would avoid regenerating every smoke trace and witness fixture and needing another bridge.
There was a problem hiding this comment.
Kept the bridge for this PR because its differential reference is the merge-base, d470b2f, which still lacks deletions. Advancing origin/main alone does not change an older branch's merge-base.
Recorded removal in the PR's follow-up notes for when supported reference revisions include the field, along with the profile-local observation suggestion for the later put slice. No deletion-model redesign is included here.
— Levicus 🤖
| behavioral contracts and documentation. | ||
|
|
||
| - **Off by default:** caching runs only inside an `enable()` scope. | ||
| - **Off by default:** automatic reads cache only inside `enable()`; explicit maintenance (`invalidateRemote`, `delete`) acts regardless of scope. |
There was a problem hiding this comment.
Nit: the semicolon one-liner is dense. Suggest two sentences:
Off by default: readers cache only inside
enable(). Maintenance calls (invalidateRemote,delete) always act.
There was a problem hiding this comment.
Applied the suggested two-sentence wording in 1a9d212. Documentation and source-audit checks pass.
— Levicus 🤖
| }); | ||
| } | ||
|
|
||
| delete(key: string): void { |
There was a problem hiding this comment.
Nit: put and getWithResolvedConfig take a DialCacheKey and read key.urn themselves; this takes the URN string. Either shape works, but the class now has two key conventions.
There was a problem hiding this comment.
Aligned LocalCache.delete with the existing get/put convention in 1a9d212: it accepts DialCacheKey and reads key.urn internally; its sole caller passes the key object.
Updated the four affected mutation anchors and verified that M65/M67/M68/M69 retain every required generated/portable detection. TypeScript checks pass. This targeted mutation run is partial evidence; the full campaign remains a separate CI gate.
— Levicus 🤖
lan17
left a comment
lan17
left a comment
There was a problem hiding this comment.
Re-review of 1a9d212 (the two fix commits since 3414d5f). Both address the earlier review correctly; nothing new to fix.
Blocking item resolved. The Python Key branch is gone rather than validated. A Key instance now takes the scalar path, where scalar_string raises a deterministic TypeError before the capability check, the counter, or any store change. This is not accidental: str(Key) would have produced a nested URN, but the normalizer never falls back to str for unknown objects. The new test covers the matching case and three mismatched kwargs and asserts no adapter call, no event, and intact local and memo entries. The Python language guide documents the rejection.
Go change is a real fix beyond the review. go-redis's typed Del helper coerces numeric strings into integers, so a proxy answering +1 or bulk "1" would have passed validation. Switching to the raw Do call preserves the reply type, matching how the adapter already dispatches SET, so cluster routing and ACL behavior are unchanged in kind. The new test drives go-redis's real RESP decoder over a pipe and rejects status strings, bulk strings, big integers, doubles, booleans, nulls, and out-of-range integers. The Go package passes locally under the race detector.
Nits taken. README bullet is the two-sentence form. LocalCache.delete takes a DialCacheKey, and the four mutation entries whose anchor text changed were updated with it, so the fault campaign still compiles. Evidence catalogs were refreshed along the usual hash chain.
Unchanged, as expected. The differential bridge stays; its removal after merge remains the open follow-up.
CI at this head. TypeScript, Python 3.11 and 3.14, Go, Rust, wire, and docs are green. Quint and the four differential shards were still pending when I checked, and the full formal workflow gates the draft flip. The three review threads the fixes addressed can be resolved.
🤖 Generated with Claude Code
This branch has not been deployed
2 participants
Closes #218.
Adds exact-key
delete/Deletein TypeScript, Go, Rust, and Python. Deletion validates the identity and optional remote capability, removes the remote value first, then removes this instance's local entry and the live request memo—even insidedisable(). Missing entries succeed; unsupported adapters and remote errors preserve local state. Identity includes arguments and tracking mode. Python deletion accepts a scalar ID or an{id, args}mapping; prebuiltKeyobjects reject before dispatch, metrics, or cache changes so they cannot override the required identity arguments.Bundled adapters issue one primary-routed
DELand accept only integer 0/1 replies. Go tests exercise the real Redis response decoder so numeric strings and big integers cannot be mistaken for successful deletion. Deletion has its own metrics and errors, executable documentation examples, and a shared Quint profile with generated histories, independent properties, and fault probes. Watermarks, sibling keys, other instances, and in-flight work remain unchanged. An earlier load can still publish after deletion.Validation:
1a9d212: TypeScript, Go, Rust, Python 3.11 and 3.14, and cross-language Redis integration. Documentation, CodeQL, and formal smoke also pass on this head.make check-go,make integration-go, andmake audit. The decoder regression fails against the original implementation and passes with the fix.make check-python(2,153 tests),make check-ts(3,553 tests, typecheck, build and packed-package checks),make docs, andmake audit. Four new Python input-boundary cases fail before the fix and pass afterward. The affected TypeScript mutations M65/M67/M68/M69 retain every required detection against the generated corpus; this targeted measurement is partial evidence, not the complete mutation gate.3414d5fagainst the same generated corpus: 7,902 required cases per port, including 6,164 histories, 244 scenarios, 1,477 protocol cases, and 17 witness checks. Go ran with race detection. The deletion profile's 128 sampled histories and 14 named regressions passed in every port. Full corpus generation, committed fixture recomputation, and required shared witnesses also passed. The follow-up fixes leave the shared models and corpus unchanged.Follow-up after merge: remove the temporary
referenceDescriptorprojection once supported differential reference revisions include thedeletionsfield. The current merge-base still requires it. Keep profile-local observation channels in mind for the laterputslice.Rust compatibility: custom
LocalStoreimplementations must addremove, and exhaustive observer/error matches must handle the new variants. Existing remote adapters retain their previous required methods; deletion is an optional capability in every port.— Levicus 🤖