feat(lab): CL-02 immutable evidence ledger and projection - #1333
Merged
Conversation
Persist Compatibility Lab evidence as append-only JSONL with content-addressed artifacts and a disposable rebuildable SQLite projection, including CL-01 observation persistence without starting CL-03 probe work.
Contributor
📝 WalkthroughWalkthroughThis PR adds the Compatibility Lab evidence pipeline. It defines event contracts, validation, secure artifact storage, durable JSONL ledger operations, conformance observation persistence, invalidation and purge handling, deterministic SQLite projection, verdict evaluation, and regression tests. ChangesCompatibility Lab evidence and projection
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Conformance
participant ObservationBuilder
participant ArtifactStore
participant Ledger
participant Projection
participant SQLite
Conformance->>ObservationBuilder: provide scenario result and execution context
ObservationBuilder->>ArtifactStore: sanitize and store manifests and evidence
ObservationBuilder->>Ledger: append validated observation event
Projection->>Ledger: replay events
Projection->>ArtifactStore: verify required artifacts
Projection->>SQLite: persist verdicts and corruption records
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
Contributor
|
✅ Deterministic PR hygiene checks passed. |
Pin artifact directory handles for descriptor-bound I/O, fail closed on sensitive purge with shared-artifact retention and atomic ledger rewrite, enforce CL-00 event admission ceilings, exclude unusable evidence from projection, evaluate all-applicable-required-pass-v1 with subject-aware applicability, and correct protocol behavior fingerprinting.
Expand evidence-ledger tests for verification semantics, shared-artifact purge retention, unusable evidence exclusion, and privacy-safe secret canaries. Update CL-02 stack status to reflect implementation complete without independent acceptance.
Wibias
marked this pull request as ready for review
August 9, 2026 05:17
Wibias
marked this pull request as draft
August 9, 2026 05:21
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 64
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@devlog/_plan/260807_compatibility_lab/001_pr_stack_status.md`:
- Around line 102-104: Update the plan’s ledger contract and related scope
wording to explicitly document sensitive purge as an exception to the
immutable/append-only JSONL behavior, including the audit record and rebuild
semantics of the physical rewrite performed by purge.ts through
atomicRewriteLedger. Ensure the documented behavior aligns with purgeActions
when it includes "ledger", rather than leaving the implementation and contract
contradictory.
In `@src/lab/artifacts/sanitize.ts`:
- Line 11: Update scrubString to use a separate global secret-matching regex for
its replacement, ensuring every secret occurrence is redacted. Keep SECRETISH
non-global for assertNoSecretMaterial, since that method relies on repeated
RegExp.test calls.
- Around line 30-31: Update assertNoSecretMaterial so exceeding its depth limit
rejects the payload instead of returning successfully; preserve recursive
scanning for values within the limit. Ensure redactForArtifact’s assert-only
manifest path propagates this rejection, preventing deeply nested secret-bearing
payloads from reaching ArtifactStore.put.
In `@src/lab/artifacts/secure-fs.ts`:
- Around line 324-330: Update the catch logic in putArtifactBytes to mirror
putNamedDigestBytes: only suppress ArtifactFsError instances whose message
indicates a missing artifact, and rethrow every other error, including
non-ArtifactFsError I/O failures and mismatch, ceiling, regular-file, or
hard-link errors.
- Around line 110-130: Update detectOpenAt to verify that the probe created by
the descriptor-relative open is located inside dir.path before setting support
to true, using statSync and the probe’s expected path; ensure cleanup removes
the actual created probe in both relative and fallback cases. Add O_NOFOLLOW to
the probe flags, and move openAtSupported from module scope onto
TrustedArtifactDir so detection is cached per directory rather than globally.
In `@src/lab/artifacts/store.ts`:
- Around line 82-85: Remove the unreachable mismatch check after the digest
assignment, or change it to compare input.expectedDigest against an
independently computed contract digest. Keep the existing hashFn(bytes)
validation unchanged; update the digest flow around computeContractDigest and
the "contract artifact digest mismatch" error so the comparison is genuinely
independent.
- Around line 112-153: Update getVerified to accept an optional artifactClass
and select the matching digest function directly when provided, retaining the
existing candidate loop only for bare-digest callers. In
validateRequiredArtifacts, pass ref.artifactClass to artifactStore.get; remove
the redundant got.digest comparison. Refactor get and getVerified to close over
a named getVerified function instead of relying on this, so destructured get
calls remain valid.
- Around line 225-245: Change loadClaimSourceManifest to return a discriminated
union with an ok discriminator: successful validation returns ok: true with the
manifest, while invalid digest and exception paths return ok: false with
corruption and no manifest; preserve mismatch results as the appropriate outcome
with their real manifest. Update callers in verdicts and rebuild to branch on ok
before accessing manifest.
- Around line 239-244: Sanitize the corruption value returned by the catch block
in the manifest-loading function before it reaches the SQLite projection. Reuse
redactSecretString or scrubString from sanitize.ts when converting err to the
corruption message, while preserving the existing fallback for non-Error values
and avoiding unsanitized field paths, duplicate kind values, or filesystem
paths.
In `@src/lab/conformance/suite-manifest.ts`:
- Around line 66-75: Remove the unused exported helpers
suiteManifestObjectForCase and suiteManifestDigestForCase from suite-manifest.ts
and their exports from src/lab/index.ts. Preserve the existing inline usage in
from-conformance.ts unless these helpers are intentionally retained as public
API; if retained, replace that inline object construction with
suiteManifestObjectForCase and keep both object and digest paths consistent.
- Around line 31-46: Update expandSuiteManifest after collecting cases to
validate that every case’s capability matches cases[0]!.capability; throw an
error for any mismatch before constructing or returning the manifest. Preserve
the existing unknown-suite check and use the validated shared capability for
SuiteManifestV1.capability.
In `@src/lab/events/limits.ts`:
- Around line 78-80: Update the raw-path validation in
enforceEventStructureLimits to use the same path-prefix coverage as scrubString,
including POSIX home, temporary, variable, and configuration directories
alongside the existing Windows checks. Preserve the LabValidationError behavior
and ensure paths such as /home/<username>/..., /tmp/..., /var/..., and /etc/...
are rejected before ledger admission.
- Around line 86-98: Move the depth check in enforceEventStructureLimits before
the Array.isArray(value) branch so arrays are validated against
MAX_EVENT_NESTING_DEPTH before recursive descent. Preserve the existing
array-size validation, recursion, and LabValidationError behavior for all
container types.
In `@src/lab/events/validate.ts`:
- Around line 1-7: Break the circular dependency between validate.ts and
limits.ts by moving the LabValidationError class into a new errors.ts module,
then update both validate.ts and limits.ts to import it from that shared module
while preserving existing exports and behavior.
- Around line 368-372: Update sourceEventIds validation in the surrounding event
validator to use the same strict validateSortedUniqueHexIds logic as supersedes,
rejecting empty strings, duplicates, and unsorted IDs. Preserve the existing
array-type validation and ensure both fields enforce identical valid SHA-256 ID
semantics.
- Around line 276-309: Add element-level validation for assertions and limits in
the event validation flow. Import and use AssertionRecordV1 to validate each
raw.assertions entry, requiring the expected assertion fields such as boolean
passed values, and reject malformed entries with LabValidationError instead of
casting the array. Validate every raw.limits value as either a number or null
before constructing the event, preserving the existing invalid_limits error
behavior, then assign only the validated results to assertions and limits.
In `@src/lab/ledger/artifact-refs.ts`:
- Around line 54-70: Update the artifact-selection flow around
artifactsStillRequired and the returned candidate list so explicitly targeted
digests are not silently filtered when still required by surviving events.
Return the retained explicit digests alongside the deletable artifacts, then
update purgeSensitiveEvidence to throw PurgeError("sensitive_bytes_retained",
...) for any retained explicit digest before writing the tombstone, ensuring the
purge fails closed.
- Around line 13-24: Update the event filtering in artifactsStillRequired to
skip only explicitly purged events, not merely invalidated ones, so retained
invalidated observations continue contributing artifact references. Remove the
now-unused isEventExcluded import and preserve the existing exclude-set,
observation, and claim_snapshot handling.
In `@src/lab/ledger/invalidation.ts`:
- Line 80: Remove the unused purgedEventIds parameter from the affected function
signature and delete the corresponding argument at its call site. Remove the
void purgedEventIds statement while preserving the existing explanatory comment
and all other behavior.
- Around line 86-97: Update buildInvalidationIndex to maintain a first-pass map
from every event ID to its event kind and position. Replace the all.slice(0,
index).find lookup for unresolved targets with an O(1) lookup in that map,
preserving the existing unknown_target and bad_target_kind validation outcomes
while ensuring only events preceding the current event are considered valid.
In `@src/lab/ledger/purge.ts`:
- Around line 145-149: Remove the exported readLedgerText helper from the
production ledger module src/lab/ledger/purge.ts. Move its raw-file reading
logic into the test file that requires it, or expose it only through a clearly
non-production test module, while leaving production callers to use
validated/sanitized ledger APIs such as replayLabLedger.
- Line 98: Define and export a shared producer-version constant in
lab/constants.ts alongside LAB_PRODUCER, then import and use it in the purge
flow’s producerVersion fallback and from-conformance’s PACKAGE_VERSION usage.
Remove both duplicated "2.10.2" literals while preserving the existing
producerVersion and event-ID behavior.
- Around line 66-71: Add a structured code discriminator to ArtifactFsError in
secure-fs.ts, following PurgeError’s code-field pattern, and assign distinct
not-found and digest-mismatch codes at the corresponding throw sites while
preserving the existing constructor contract. In src/lab/ledger/purge.ts lines
66-71, match the not-found code instead of parsing err.message; in
src/lab/projection/rebuild.ts lines 380-391, match the digest-mismatch code
instead of parsing message text. Use code names and constructor arguments
consistent with ArtifactFsError’s actual definition.
- Around line 41-58: Update the temporary-file rewrite sequence around tmpPath
to unlink the temporary file on every failure before rethrowing, while
preserving successful rename behavior and avoiding masking the original error.
Replace the post-rename file fsync in the ledger rewrite flow with an fsync of
the containing directory to provide rename durability; if directory fsync is
unsupported on relevant platforms, document that limitation in a comment rather
than implying file fsync provides it.
- Around line 106-127: Update purgeSensitiveEvidence to record completed purge
steps as each operation succeeds, pushing "ledger" after the ledger rewrite or
append block and "sqlite" after the SQLite cleanup block. Ensure the existing
error path attaches completed to the thrown PurgeError so callers can
distinguish partial progress when a later operation fails.
In `@src/lab/ledger/store.ts`:
- Around line 29-38: Update the append logic around openSync, writeSync, and
fsyncSync to write bytes in a loop until the entire buffer is consumed,
advancing the offset by each successful byte count. Treat a zero-byte write as
the failure condition and preserve cleanup through the existing finally
closeSync path; do not throw merely because an individual write is short.
- Around line 145-149: Update the trailing carry handling around processLine so
the final remainder is always passed with false for the trailing-newline
argument, ensuring it is classified as partial_line. Remove the now-unused
hasTrailingNewline state and its assignments throughout the ledger parsing flow,
while preserving complete-line processing.
- Around line 116-143: The replayLabLedger read loop decodes each chunk
independently, corrupting UTF-8 sequences split across chunk boundaries. Import
and use a UTF-8 StringDecoder so decoder.write() feeds decoded text into carry
for each chunk, then append decoder.end() output after reading completes before
processing the final carry with processLine.
In `@src/lab/observe/from-conformance.ts`:
- Around line 110-111: Update observationFromConformanceResult and
persistConformanceResult to track whether each function created its
ArtifactStore, then close only that owned store in a finally block after
persistence or observation processing completes. Preserve caller-provided stores
without closing them, and avoid the redundant ensureLabDirs call when the store
is already being created for the same configDir.
- Around line 112-114: Update the observation construction around
ScenarioRunResult to use the real execution start and completion timestamps
carried by ScenarioRunResult instead of deriving startedAt from recordedAt.
Thread those timestamps through the CL-01 harness if they are not currently
exposed; otherwise make any unavoidable synthetic origin explicit rather than
storing recordedAt - 1 as measured evidence.
- Line 118: The double cast in the suite expansion flow discards the
producer-consumer type contract. Update the value returned by
expandSuiteManifest in the observe flow to use SuiteManifestV1, then access it
structurally for suiteManifestDigest and artifact storage; alternatively, add
the required index signature to SuiteManifestV1 so the result can be indexed
without casting through unknown.
- Around line 42-44: Update behaviorFingerprintForCase and the related
requirement handling around effectiveAdapter and subjectId to stop substituting
default protocol values when required requirement arrays are empty. Import
LabValidationError from ../events/validate and throw it for each missing
protocol requirement so cases fail loudly before behaviorFingerprint or
subjectId evidence is published.
- Around line 89-98: Update outcomeFromResult to replace the unconditional
fallback return with the exhaustiveness-check pattern used for obs.outcome in
verdict projection. Explicitly handle any remaining FailureClassification values
that should map to "fail", then use the guard in the default branch so newly
added classifications fail type-checking instead of silently becoming a negative
verdict.
In `@src/lab/paths.ts`:
- Around line 29-32: Update the directory initialization flow around labRoot,
labArtifactsDir, and the two mkdirSync calls to verify and tighten existing
directory permissions to 0o700 after creation on POSIX platforms. Skip
permission checks and chmod operations on Windows, while preserving recursive
creation behavior for both directories.
In `@src/lab/projection/rebuild.ts`:
- Around line 358-359: Update validateRequiredArtifacts to validate each claim’s
source manifest during its existing artifact-validation loop and add invalid
claim event IDs directly to unusableClaimEventIds. Keep the returned set
complete at function exit, and remove the caller-side mutation that currently
populates it afterward.
- Around line 314-321: Remove the redundant all-purged check in the verdict loop
and retain a single defensive check that skips verdicts when no contributing
event IDs remain after purged and invalidated IDs are excluded. Update the
retained check’s naming or comment to clarify it is defensive assertion logic,
not an expected filtering path.
- Around line 30-41: Update the retry loop in wipeSqlite so that exhausting all
eight attempts for a candidate rethrows the last EBUSY or EPERM error instead
of silently continuing. Preserve immediate propagation for other error codes and
only proceed to the next candidate after successful deletion or confirmed
absence, ensuring rebuildLabProjection cannot open a stale database.
- Around line 286-288: Replace the null-bearing insertArtifact call in the purge
loop with a status-only update for each digest, preserving existing
artifact_class, media_type, and byte_count values while setting status to
"purged_unavailable". Add and use a dedicated prepared statement alongside the
existing artifact statements, such as the logic around
event.targetArtifactDigests and insertArtifact.
- Around line 161-166: Update the claim event preparation near
resolveClaimStates to reuse the exported usableClaims helper from
invalidation.ts, preserving the existing exclusion behavior and obtaining a
properly typed ClaimSnapshotEvent[]; remove the redundant eventKind filter and
the as never cast from the resolveClaimStates call.
- Around line 108-127: Wrap the complete projection write phase in the rebuild
function—starting after resetProjectionSchema(db) and including schema metadata,
corruption, verdict, and related INSERT operations—inside a single
db.transaction callback. Keep resetProjectionSchema(db) outside the transaction,
and preserve normal error propagation so failed writes roll back before the
existing cleanup and snapshot reads proceed.
- Around line 71-73: Add focused regression tests covering the projection
failure branches: exercise the `expectedFailure` path producing `UNSUPPORTED`,
verify cross-key supersession reports `"cross-key supersession"`, validate the
`freshness.maxAgeMs` behavior in the projection flow, and test `wipeSqlite`
retry exhaustion after repeated `EBUSY`/`EPERM` failures. If any branch is
intentionally unsupported, remove its unused handling instead of adding
coverage.
In `@src/lab/projection/schema.ts`:
- Around line 87-113: Add CHECK constraints to the DDL definitions for
artifacts.status, verdicts.verdict, observations.outcome, and claims.polarity,
using the exact allowed member lists from the artifact status union and
CompatibilityVerdict in src/lab/constants.ts. Update the corresponding CREATE
TABLE statements in the schema so invalid enum strings cause rebuild failure,
without changing the existing column types or other constraints.
In `@src/lab/projection/verdicts.ts`:
- Around line 94-105: Update resolveClaimStates and both callers in
projectVerdicts and rebuild so the resolver receives the invalidation index’s
purgedEventIds. In the supersedes predecessor check, treat an absent predecessor
whose id is in purgedEventIds as an accepted supersession; only record
claim_corruption and clear current for predecessors absent from both the claims
map and purgedEventIds.
- Around line 276-283: Update the failure mapping in the conformance observer’s
result conversion, specifically the path in from-conformance that handles
!result.passed, to populate failure.expectedFailure from
result.expectedFailureMatched while preserving the existing failure fields. This
ensures the projection logic in verdicts.ts can classify matching capability
failures as UNSUPPORTED.
- Around line 284-291: Update the verdict selection in the function containing
the `sawPass` branch to avoid using the oldest `ordered[0]` observation’s
`executionMode`: either add `executionMode` to `ProjectionKey` and propagate it
through `projectionKeyString`, the `verdicts` schema table, and the rebuild
insert, or use `ordered[ordered.length - 1]` and add a note when modes are
mixed. Ensure fixture/live observations are never evaluated under the wrong
verification rules.
- Around line 28-37: Replace delimiter-ambiguous key construction in
projectionKeyString with a shared, reversible component encoding that prevents |
from appearing within encoded values. Apply the same encoding to the claim-state
key created by resolveClaimStates and to its counterpart in the rebuild lookup,
ensuring both producers and consumers use the identical format so claim states
continue resolving correctly.
- Around line 106-114: Update the predecessor lookup in the verdict processing
flow around `byId` so it uses a map containing all claims, such as `allById`,
rather than the key-scoped map. Preserve the existing missing-predecessor
handling, then compare the resolved predecessor’s subject and capability in the
cross-key branch so genuine cross-key supersession is reported as `cross-key
supersession`.
- Around line 204-229: Update the claim snapshot loop in verdicts.ts to process
usable claims regardless of polarity, while retaining corruption and
unusable-claim filtering. Map “supported” claims to “CLAIMED”, “not_supported”
claims to “UNSUPPORTED”, and represent “withdrawn” claims through the existing
explicit negative or withdrawal verdict type/path. Preserve deduplication by
projection key and continue emitting the claim’s source and event metadata.
In `@src/lab/projection/verification.ts`:
- Line 31: Update the required-scenario evaluation around the manifest-loading
logic near lines 85-95 so an unloaded required manifest is recorded as an
evaluation note and causes verification to fail closed. Do not treat a null
result from loadScenarioManifest as inapplicable; preserve the existing handling
for successfully loaded manifests while ensuring canVerify remains false
whenever any required scenario manifest is unavailable.
- Around line 37-42: Update the applicability check in the projection
verification return expression to use the intended single surface rule; if
scenarios apply to any declared surface, remove the surfaces[0] ===
subject.surface condition and retain membership via
surfaces.includes(subject.surface), so manifest ordering does not affect
applicability. Ensure multi-surface scenarios remain applicable for each
declared surface.
- Around line 16-25: Update isScenarioApplicable to use the scenario’s declared
manifest metadata rather than matching ".live." in scenarioId. Add and propagate
an applicability field on SuiteManifestV1["scenarios"][number] alongside role,
then exclude fixture-mode protocol-conformance scenarios based on that field so
leading, trailing, and embedded live-reserved IDs behave consistently.
- Around line 107-134: Update evaluateAllApplicableRequiredPassV1 to accept the
projection’s asOf value, passed from projectObservationGroup, and enforce each
scenario’s freshness.maxAgeMs using the observation completedAt timestamp. When
asOf is defined and the observation exceeds maxAgeMs (defaulting to 0), classify
it as missing and add a stale_observation:<scenarioId> note before it can count
as passing; preserve existing digest and outcome checks.
- Around line 150-173: Update the manifest validation in the parser around the
scenario mapping and required metadata fields to reject missing or invalid
values by returning null instead of coercing them to empty strings. Validate
evidenceLayer, capability, assertionDslVersion, and evidenceSchemaVersion
against their declared closed sets, and validate each scenario role against
SCENARIO_ROLES before constructing the typed scenario; align SCENARIO_ROLES with
the actual SuiteManifestV1 role union. Preserve the existing null result for
malformed manifests so callers emit the conservative unavailable verdict.
In `@tests/lab-evidence-ledger.test.ts`:
- Around line 422-435: Ensure artifact stores are closed even when test
assertions throw: in tests/lab-evidence-ledger.test.ts lines 422-435, wrap the
assertions using the store from createArtifactStore in try/finally and call
store.close() in finally; in lines 674-689, move the existing store.close() into
a finally block around the assertions.
- Around line 674-689: Wrap the artifact-store usage in the test around
createArtifactStore, put, decoding, and assertions in a try/finally block, and
move store.close() into the finally clause. Preserve the existing assertions
while ensuring the store created in this test is closed even when any assertion
fails.
- Around line 630-636: Strengthen the negative snapshot assertions: in
tests/lab-evidence-ledger.test.ts lines 630-636, capture the pre-purge snapshot
and assert it contains event.eventId in contributing_event_ids_json before
retaining the post-purge every check; at lines 845-848, explicitly assert
snap.length is 0 alongside the existing verdict !== "CLAIMED" assertion.
- Around line 554-557: The three scenario applicability filters disagree on
whether multi-valued protocols and surfaces apply. Add one helper next to
persistAllSuiteScenarios that mirrors the applicability rule in verification.ts,
then replace the inline predicates at tests/lab-evidence-ledger.test.ts:554-557,
:717-720, and :747-750 with that helper; all three sites require the same
change.
- Around line 707-711: Remove the misleading comment and four unreachable void
statements at tests/lab-evidence-ledger.test.ts:707-711, then either add the
intended assertions or remove the unused imports chmodSync, existsSync,
claimSourceManifestDigest, and LabValidationError; use LabValidationError in the
assertions around lines 803-806 as requested. At
tests/lab-evidence-ledger.test.ts:86-98, remove persistAllSuiteScenarios or
invoke it from both duplicated persist loops at lines 558-567 and 751-761.
- Around line 69-84: Update syntheticPassResult to classify passing synthetic
results as "inconclusive", matching sourceConformanceResult and normal passing
runs; do not use "protocol_failure" for a result with passed set to true.
- Around line 220-237: Remove the dead first artifact loop and its unused
createArtifactStore setup from the test. Keep the second loop using
openTrustedArtifactDir and putNamedDigestBytes to create the event-referenced
digest files, and update the surrounding comment so it accurately describes that
setup.
- Around line 207-208: Update the domain-separation test around
eventIdForPayload and subjectIdForSubject to hash the same canonical object with
both helpers, then assert those resulting IDs differ. Replace the current
comparison between id1 and subjectId, while preserving the existing
invalidation-payload assertions.
- Around line 800-807: Update the event-admission test around
enforceEventStructureLimits to assert LabValidationError and the specific codes
nesting_depth, forbidden_field, and secret_pattern for each independent
rejection, using MAX_EVENT_NESTING_DEPTH + 1 nesting levels. In
src/lab/events/limits.ts, change the thrown nesting-depth error code to
MAX_EVENT_NESTING_DEPTH, and remove the obsolete void LabValidationError
statement near the test setup.
- Around line 294-320: Extend the test “invalidation targets must be earlier
observation/claim only” to cover both promised constraints: add a case where an
invalidation’s recordedAt precedes its target observation/claim and assert the
corruption kind returned by buildInvalidationIndex, and add cases targeting an
invalidation and a purge_tombstone and assert their expected corruption kinds.
Confirm the implementation’s canonical corruption kind values before using
literal assertions, while preserving the existing valid-target and
unknown-reference checks.
- Around line 48-50: Remove the empty beforeEach hook and its misleading
OPENCODEX_HOME isolation comment from the test setup. Also remove beforeEach
from the bun:test import, leaving isolation to withHome and afterEach.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6600341b-4e9f-4231-b01f-6082497dc596
📒 Files selected for processing (22)
devlog/_plan/260807_compatibility_lab/001_pr_stack_status.mdsrc/lab/artifacts/sanitize.tssrc/lab/artifacts/secure-fs.tssrc/lab/artifacts/store.tssrc/lab/conformance/suite-manifest.tssrc/lab/constants.tssrc/lab/digest.tssrc/lab/events/limits.tssrc/lab/events/types.tssrc/lab/events/validate.tssrc/lab/index.tssrc/lab/ledger/artifact-refs.tssrc/lab/ledger/invalidation.tssrc/lab/ledger/purge.tssrc/lab/ledger/store.tssrc/lab/observe/from-conformance.tssrc/lab/paths.tssrc/lab/projection/rebuild.tssrc/lab/projection/schema.tssrc/lab/projection/verdicts.tssrc/lab/projection/verification.tstests/lab-evidence-ledger.test.ts
Harden artifact I/O, purge, replay, projection, admission, and persistence seams per frozen CL-00 contracts without regressing prior review fixes.
putArtifactBytes and putNamedDigestBytes now probe the digest path with O_NOFOLLOW before writing so rename cannot replace a pre-existing symlink on macOS dirfd artifact I/O.
macOS dirfd opens can report ENOENT for symlink digest paths; probe with lstatSync before create so putArtifactBytes fails closed instead of replacing the link.
Wibias
marked this pull request as ready for review
August 9, 2026 08:03
Collaborator
Author
|
Final gate check:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lab/conformance/types.ts (1)
144-159: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the required
executionContextpath instead of the unneeded fallback.
ScenarioRunResult.executionContextis required, soprotocolSubject()doesresult.executionContext ?? resolveProtocolExecutionContext(caseRecord), and the optionalexecutionContextparameter inbehaviorFingerprintForCase()also falls back there. Remove the??fallback and pass the result’sexecutionContextdirectly; keepbehaviorFingerprintForCaseconsistent (for example, make the parameter required, or require it where persisted observations are built).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lab/conformance/types.ts` around lines 144 - 159, The required ScenarioRunResult.executionContext should be used directly instead of falling back to resolveProtocolExecutionContext(caseRecord). Remove the nullish fallback in protocolSubject(), and make behaviorFingerprintForCase() consistently require and use an execution context, updating persisted-observation callers as needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@devlog/_plan/260807_compatibility_lab/001_pr_stack_status.md`:
- Line 24: Update the CL-02 status entry to keep the Accepted head value as —
until independent acceptance. Add a Current candidate head field containing the
remediation commit SHA, and use that same SHA in the validation status recorded
in the CL-02 history section around the existing prior accepted SHA.
In `@src/lab/artifacts/secure-fs.ts`:
- Around line 274-292: Update assertArtifactTargetCreatable to report an
existing clean regular file as reusable instead of throwing, while preserving
symlink and non-regular-file rejection. Adjust putArtifactBytes and
putNamedDigestBytes to skip publishing when the target is reusable, then retain
their final readArtifactBytes verification so the on-disk content is still
validated.
In `@src/lab/artifacts/store.ts`:
- Around line 65-87: Contain parsing and manifest-validation failures within
digestForArtifactClass by converting them to ArtifactFsError with code
artifact_mismatch and concise messages that do not include artifact content.
Apply this to jsonDigest and the claim_source_manifest validation path so
secure-fs readArtifactBytes propagates the mismatch unchanged, allowing
getVerified to continue candidates and rebuild classification to remain
artifact_mismatch.
In `@src/lab/events/limits.ts`:
- Around line 78-84: Update the POSIX-path detection in
enforceEventStructureLimits to recognize embedded paths after non-path
delimiters such as “=”, “:”, “,”, “>”, and “@”, matching scrubString’s broader
detection behavior. Prefer a boundary that rejects path-safe preceding
characters while still avoiding matches embedded within longer path segments,
and preserve the existing Windows-path and raw_path validation behavior.
In `@src/lab/ledger/purge.ts`:
- Line 139: The default purgeActions in the purge request flow should include
every action defined by the shared PURGE_ACTIONS constant. Replace the narrower
hardcoded fallback near purgeActions with a copy of PURGE_ACTIONS, preserving
sorting and explicit req.purgeActions behavior so default sensitive purges also
include "export".
- Around line 81-95: Update atomicRewriteLedger so the successful rename is
recorded as the commit before parent-directory openSync/fsyncSync durability
operations. Preserve the committed state and ensure fsync failures are surfaced
as a distinct non-rollback error, allowing purgeSensitiveEvidence to report
"ledger" in completed while still indicating durability failure.
In `@src/lab/ledger/store.ts`:
- Around line 137-147: Update the oversized-line detection branch in
replayLabLedger to return an empty carry instead of retaining tail when setting
skippingOversizedLine, so oversized bytes are discarded immediately. Preserve
the existing malformed-line accounting and ensure EOF handling does not count
the same skipped line again.
- Around line 212-221: Update the read loop around splitIncompleteUtf8Tail and
processBufferedLines so carry is always an owned copy detached from the reusable
chunk buffer, and concatenate result.carry before the UTF-8 remainder to
preserve stream order. Add a focused replayLabLedger regression test near the
existing ledger replay tests that places a non-ASCII character across the 64 KiB
read boundary and verifies the event is returned intact.
In `@src/lab/observe/from-conformance.ts`:
- Around line 155-158: Update the default branch of the classification switch in
observationFromConformanceResult to retain the _never exhaustiveness check but
throw an error instead of returning _never. Ensure unmapped classifications fail
closed before the result reaches eventWithoutId.outcome.
- Around line 162-176: Update the ScenarioRunResult contract and runScenario
return paths to include and populate startedAt and completedAt, then have
requireExecutionTimes read those typed fields without casting. Preserve the
existing options override behavior while allowing real runScenario output to
supply both timestamps.
In `@src/lab/projection/rebuild.ts`:
- Around line 255-257: Index corruptions once before the event-processing loop
in rebuildLabProjection, using lookup structures keyed by eventId and by the
specified `${eventId ?? ""}\u0000${detail}` composite key. Replace the per-event
corruptions.find at the claimCorruption lookup and corruptions.some checks for
invalidations and verdicts with indexed lookups, preserving the existing
corruption matching behavior.
In `@src/lab/projection/verdicts.ts`:
- Around line 371-372: Update the PROBED fallback in the verdict computation
around currentPasses so it records a note whenever passing evidence coexists
with blocked or inconclusive observations. Preserve the PROBED verdict,
distinguish the applicable suppressed-evidence condition in the note, and keep
the existing notes behavior for cases without such evidence.
- Around line 281-289: Add a V1 fixture and manifest entry using
expectedFailure.controlKind "capability_absence_control", expectedClass
"capability_failure", the appropriate expectedCode, and onMatch "unsupported";
then add a regression test with a matched occurrence that drives
projectObservationGroup through isMatchedCapabilityAbsenceControl and asserts
verdict "UNSUPPORTED".
In `@src/lab/projection/verification.ts`:
- Around line 71-85: Align freshness handling in scenario manifest expansion
with suite defaults by applying the existing `{ maxAgeMs: null }` fallback when
authority defaults are missing, or consistently reject such manifests during
loading. Ensure scenarioContractFromManifest does not mark otherwise valid
manifests unavailable solely because freshness is omitted.
In `@tests/lab-evidence-ledger.test.ts`:
- Around line 952-973: Add focused regression tests beside the existing
verification tests: one configuring suite/scenario maxAgeMs and asserting an
older passing observation becomes missing with a stale_observation:<id> note,
including coverage that the stricter limit is selected; and one exercising the
verdicts.ts UNSUPPORTED branch with the expected controlKind and onMatch keys,
asserting the UNSUPPORTED verdict.
- Around line 1081-1084: Replace the vacuous snap.every assertion in the rebuild
projection test with an exact assertion that the snapshot contains one row for
C2, with verdict UNKNOWN, note current_claim_unusable, and contributingEventIds
identifying C2; preserve the existing rebuilt snapshot setup.
- Around line 975-981: Strengthen the test around projectVerdicts in “newer pass
supersedes older fail without DEGRADED” by asserting the projected verdict
exists and its verdict is exactly “PROBED”, matching the
suite_manifest_unavailable behavior when no loadSuiteManifest is supplied. Keep
the contradictingEventIds assertion for failEvent unchanged.
- Around line 1150-1160: Update the test “observationFromConformanceResult
closes internally created artifact store” to assert the observable cleanup:
after observationFromConformanceResult returns, remove the artifacts directory
with the appropriate filesystem operation and assert it succeeds. Replace the
ineffective open-and-close trusted directory calls while preserving the
eventKind assertion.
- Around line 931-950: Update the test named “replay handles UTF-8 split across
chunk boundaries” to construct ledger content where the bytes of a multi-byte
character straddle offset 65536, forcing replayLabLedger’s chunked reader and
splitIncompleteUtf8Tail path to run. Respect MAX_SERIALIZED_EVENT_BYTES and
MAX_SANITIZED_STRING_FIELD; if one padded event exceeds limits, use two smaller
events whose combined serialized length positions the café bytes across the
boundary while preserving the existing replay and corruption assertions.
---
Outside diff comments:
In `@src/lab/conformance/types.ts`:
- Around line 144-159: The required ScenarioRunResult.executionContext should be
used directly instead of falling back to
resolveProtocolExecutionContext(caseRecord). Remove the nullish fallback in
protocolSubject(), and make behaviorFingerprintForCase() consistently require
and use an execution context, updating persisted-observation callers as needed.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d50858d1-47ce-4174-9b4a-739cf4a87ecd
📒 Files selected for processing (22)
devlog/_plan/260807_compatibility_lab/001_pr_stack_status.mdsrc/lab/artifacts/sanitize.tssrc/lab/artifacts/secure-fs.tssrc/lab/artifacts/store.tssrc/lab/conformance/executor.tssrc/lab/conformance/suite-manifest.tssrc/lab/conformance/types.tssrc/lab/constants.tssrc/lab/events/errors.tssrc/lab/events/limits.tssrc/lab/events/validate.tssrc/lab/ledger/artifact-refs.tssrc/lab/ledger/invalidation.tssrc/lab/ledger/purge.tssrc/lab/ledger/store.tssrc/lab/observe/from-conformance.tssrc/lab/paths.tssrc/lab/projection/rebuild.tssrc/lab/projection/schema.tssrc/lab/projection/verdicts.tssrc/lab/projection/verification.tstests/lab-evidence-ledger.test.ts
| | CL-00 | `feat/cl-00-compatibility-contracts` | `3ad5bb6bd3f76f6879d84b78ea39edd3e01ec296` | `c014464237fd3c95bda08bc18bfab8ba8f532308` | [#1286](https://github.com/lidge-jun/opencodex/pull/1286) | ACCEPTED AFTER CODERABBIT REMEDIATION (merged to `dev` at `243c3f4905797aa11c62ba933bb03d6d721266fd`) | | ||
| | CL-01 | `feat/cl-01-conformance-harness` | `c2113ca47b8a05c5a5f90679e4eaa640ca2c6a66` | `22d608c82d82e2746c0cef9cd761db19a8e465ee` | [draft Wibias #10](https://github.com/Wibias/opencodex/pull/10) | ACCEPTED (contract-corrected revalidation) | | ||
| | CL-01 | `feat/cl-01-conformance-harness` | `c2113ca47b8a05c5a5f90679e4eaa640ca2c6a66` | `22d608c82d82e2746c0cef9cd761db19a8e465ee` | [#1320](https://github.com/lidge-jun/opencodex/pull/1320) | MERGED TO `dev` at `4bb249b756abd468c675d2d92fffe4da95ad3e2a` | | ||
| | CL-02 | `feat/cl-02-evidence-ledger` | `4bb249b756abd468c675d2d92fffe4da95ad3e2a` | (phase-2 review fixes in progress) | [draft #1333](https://github.com/lidge-jun/opencodex/pull/1333) | IMPLEMENTATION COMPLETE — PHASE-2 REVIEW FIXES — NOT INDEPENDENTLY ACCEPTED | |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Pin the current unaccepted CL-02 revision.
Line 24 uses the Accepted head column for a status string. Lines 125-138 record the prior accepted SHA but not the SHA for the current CodeRabbit remediation. A draft PR can move, so this document cannot identify the exact revision covered by the stated validation and pending acceptance.
Keep Accepted head as — until independent acceptance. Add a Current candidate head entry with the remediation commit SHA. Reference the same SHA in the current validation status.
Proposed wording
-| CL-02 | `feat/cl-02-evidence-ledger` | `4bb249b756abd468c675d2d92fffe4da95ad3e2a` | (phase-2 review fixes in progress) | [draft `#1333`](https://github.com/lidge-jun/opencodex/pull/1333) | IMPLEMENTATION COMPLETE — PHASE-2 REVIEW FIXES — NOT INDEPENDENTLY ACCEPTED |
+| CL-02 | `feat/cl-02-evidence-ledger` | `4bb249b756abd468c675d2d92fffe4da95ad3e2a` | — | [draft `#1333`](https://github.com/lidge-jun/opencodex/pull/1333) | IMPLEMENTATION COMPLETE — PHASE-2 REVIEW FIXES — NOT INDEPENDENTLY ACCEPTED |
...
+- **Current candidate head:** `<current CL-02 remediation SHA>`
- **Current CodeRabbit remediation:** committed on draft PR `#1333`; current CI and review reconciliation are required before this head may be recorded as accepted.
+- **Current CodeRabbit remediation:** committed at `<current CL-02 remediation SHA>` on draft PR `#1333`; current CI and review reconciliation are required before this head may be recorded as accepted.Also applies to: 123-140
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260807_compatibility_lab/001_pr_stack_status.md` at line 24,
Update the CL-02 status entry to keep the Accepted head value as — until
independent acceptance. Add a Current candidate head field containing the
remediation commit SHA, and use that same SHA in the validation status recorded
in the CL-02 history section around the existing prior accepted SHA.
Comment on lines
+274
to
+292
| function assertArtifactTargetCreatable(dir: TrustedArtifactDir, name: string): void { | ||
| revalidateDir(dir); | ||
| assertRelativeName(name); | ||
| try { | ||
| const stats = lstatSync(childPath(dir, name)); | ||
| if (stats.isSymbolicLink()) { | ||
| harnessFailure("artifact target is a symbolic link", "artifact_unsafe_target"); | ||
| } | ||
| assertRegularFileStats(stats, "artifact create target"); | ||
| harnessFailure("artifact target exists but is not reusable", "artifact_unsafe_target"); | ||
| } catch (err) { | ||
| if (isRawMissingError(err)) return; | ||
| if (err instanceof ArtifactFsError) throw err; | ||
| harnessFailure( | ||
| `artifact create target check failed: ${err instanceof Error ? err.message : String(err)}`, | ||
| "artifact_unsafe_target", | ||
| ); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
assertArtifactTargetCreatable turns a benign content-addressed race into a hard failure.
Line 283 throws "artifact target exists but is not reusable" on the success path of the checks above it. Trace the control flow:
- Line 278 lstats the target. If the file is absent, line 285 returns and the write proceeds. That is the normal case.
- If the file is present and is a symlink, line 280 throws. Correct.
- If the file is present and is a clean regular file with
nlink === 1, line 282 passes, and line 283 then throws unconditionally.
Branch 3 is the problem. The only callers are putArtifactBytes line 405 and putNamedDigestBytes line 432, and both reach this call only after the readback at line 400 or 427 already failed with a missing-artifact error. So the file was absent a moment ago. A TOCTOU window separates that read from this lstat.
Failure mode: two processes persist the same fixture concurrently. Process A completes writeTempArtifact and publishes <digest>.bin while process B sits between its failed readback and this check. Process B then throws ArtifactFsError("artifact_unsafe_target"). Because the store is content-addressed, the file process B found holds exactly the bytes process B was about to write — the digest is the identity. writeTempArtifact already publishes through renameAtDir, which replaces atomically, so the underlying write is idempotent by construction. This check removes that idempotence.
The reachable path is a parallel conformance runner: CL01 suites share fixtures, so two scenarios can produce the same fixture digest at the same time. The current tests are sequential, so CI will not surface this.
Make branch 3 fall through and let the caller's final readback verify the published bytes.
🐛 Proposed fix
Report reusability to the caller instead of throwing on it:
-function assertArtifactTargetCreatable(dir: TrustedArtifactDir, name: string): void {
+/** Returns true when the digest was published concurrently and the write can be skipped. */
+function artifactTargetAlreadyPublished(dir: TrustedArtifactDir, name: string): boolean {
revalidateDir(dir);
assertRelativeName(name);
try {
const stats = lstatSync(childPath(dir, name));
if (stats.isSymbolicLink()) {
harnessFailure("artifact target is a symbolic link", "artifact_unsafe_target");
}
assertRegularFileStats(stats, "artifact create target");
- harnessFailure("artifact target exists but is not reusable", "artifact_unsafe_target");
+ // Content-addressed: an existing clean regular file under this digest name
+ // holds the same bytes. A concurrent writer won the race.
+ return true;
} catch (err) {
- if (isRawMissingError(err)) return;
+ if (isRawMissingError(err)) return false;
if (err instanceof ArtifactFsError) throw err;
harnessFailure(
`artifact create target check failed: ${err instanceof Error ? err.message : String(err)}`,
"artifact_unsafe_target",
);
}
+ return false;
}Then skip the write in both callers. putArtifactBytes:
- assertArtifactTargetCreatable(dir, digestFileName(digest));
- const tmpName = `.tmp-${digest}-${process.pid}-${Date.now()}.partial`;
- writeTempArtifact(dir, tmpName, bytes, digest, artifactBytesDigest);
+ if (!artifactTargetAlreadyPublished(dir, digestFileName(digest))) {
+ const tmpName = `.tmp-${digest}-${process.pid}-${Date.now()}.partial`;
+ writeTempArtifact(dir, tmpName, bytes, digest, artifactBytesDigest);
+ }
return readArtifactBytes(dir, digest, bytes.byteLength);putNamedDigestBytes:
- assertArtifactTargetCreatable(dir, digestFileName(digest));
- const tmpName = `.tmp-${digest}-${process.pid}-${Date.now()}.partial`;
- writeTempArtifact(dir, tmpName, bytes, digest, contentDigest);
+ if (!artifactTargetAlreadyPublished(dir, digestFileName(digest))) {
+ const tmpName = `.tmp-${digest}-${process.pid}-${Date.now()}.partial`;
+ writeTempArtifact(dir, tmpName, bytes, digest, contentDigest);
+ }
return readArtifactBytes(dir, digest, { expectedByteCount: bytes.byteLength, contentDigest });The final readArtifactBytes in both callers re-verifies the digest against the on-disk bytes, so the skip stays fail-closed. The hard-link and symlink rejections are preserved.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function assertArtifactTargetCreatable(dir: TrustedArtifactDir, name: string): void { | |
| revalidateDir(dir); | |
| assertRelativeName(name); | |
| try { | |
| const stats = lstatSync(childPath(dir, name)); | |
| if (stats.isSymbolicLink()) { | |
| harnessFailure("artifact target is a symbolic link", "artifact_unsafe_target"); | |
| } | |
| assertRegularFileStats(stats, "artifact create target"); | |
| harnessFailure("artifact target exists but is not reusable", "artifact_unsafe_target"); | |
| } catch (err) { | |
| if (isRawMissingError(err)) return; | |
| if (err instanceof ArtifactFsError) throw err; | |
| harnessFailure( | |
| `artifact create target check failed: ${err instanceof Error ? err.message : String(err)}`, | |
| "artifact_unsafe_target", | |
| ); | |
| } | |
| } | |
| /** Returns true when the digest was published concurrently and the write can be skipped. */ | |
| function artifactTargetAlreadyPublished(dir: TrustedArtifactDir, name: string): boolean { | |
| revalidateDir(dir); | |
| assertRelativeName(name); | |
| try { | |
| const stats = lstatSync(childPath(dir, name)); | |
| if (stats.isSymbolicLink()) { | |
| harnessFailure("artifact target is a symbolic link", "artifact_unsafe_target"); | |
| } | |
| assertRegularFileStats(stats, "artifact create target"); | |
| // Content-addressed: an existing clean regular file under this digest name | |
| // holds the same bytes. A concurrent writer won the race. | |
| return true; | |
| } catch (err) { | |
| if (isRawMissingError(err)) return false; | |
| if (err instanceof ArtifactFsError) throw err; | |
| harnessFailure( | |
| `artifact create target check failed: ${err instanceof Error ? err.message : String(err)}`, | |
| "artifact_unsafe_target", | |
| ); | |
| } | |
| return false; | |
| } |
Suggested change
| function assertArtifactTargetCreatable(dir: TrustedArtifactDir, name: string): void { | |
| revalidateDir(dir); | |
| assertRelativeName(name); | |
| try { | |
| const stats = lstatSync(childPath(dir, name)); | |
| if (stats.isSymbolicLink()) { | |
| harnessFailure("artifact target is a symbolic link", "artifact_unsafe_target"); | |
| } | |
| assertRegularFileStats(stats, "artifact create target"); | |
| harnessFailure("artifact target exists but is not reusable", "artifact_unsafe_target"); | |
| } catch (err) { | |
| if (isRawMissingError(err)) return; | |
| if (err instanceof ArtifactFsError) throw err; | |
| harnessFailure( | |
| `artifact create target check failed: ${err instanceof Error ? err.message : String(err)}`, | |
| "artifact_unsafe_target", | |
| ); | |
| } | |
| } | |
| if (!artifactTargetAlreadyPublished(dir, digestFileName(digest))) { | |
| const tmpName = `.tmp-${digest}-${process.pid}-${Date.now()}.partial`; | |
| writeTempArtifact(dir, tmpName, bytes, digest, artifactBytesDigest); | |
| } | |
| return readArtifactBytes(dir, digest, bytes.byteLength); |
Suggested change
| function assertArtifactTargetCreatable(dir: TrustedArtifactDir, name: string): void { | |
| revalidateDir(dir); | |
| assertRelativeName(name); | |
| try { | |
| const stats = lstatSync(childPath(dir, name)); | |
| if (stats.isSymbolicLink()) { | |
| harnessFailure("artifact target is a symbolic link", "artifact_unsafe_target"); | |
| } | |
| assertRegularFileStats(stats, "artifact create target"); | |
| harnessFailure("artifact target exists but is not reusable", "artifact_unsafe_target"); | |
| } catch (err) { | |
| if (isRawMissingError(err)) return; | |
| if (err instanceof ArtifactFsError) throw err; | |
| harnessFailure( | |
| `artifact create target check failed: ${err instanceof Error ? err.message : String(err)}`, | |
| "artifact_unsafe_target", | |
| ); | |
| } | |
| } | |
| if (!artifactTargetAlreadyPublished(dir, digestFileName(digest))) { | |
| const tmpName = `.tmp-${digest}-${process.pid}-${Date.now()}.partial`; | |
| writeTempArtifact(dir, tmpName, bytes, digest, contentDigest); | |
| } | |
| return readArtifactBytes(dir, digest, { expectedByteCount: bytes.byteLength, contentDigest }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lab/artifacts/secure-fs.ts` around lines 274 - 292, Update
assertArtifactTargetCreatable to report an existing clean regular file as
reusable instead of throwing, while preserving symlink and non-regular-file
rejection. Adjust putArtifactBytes and putNamedDigestBytes to skip publishing
when the target is reusable, then retain their final readArtifactBytes
verification so the on-disk content is still validated.
Comment on lines
+65
to
+87
| function jsonDigest( | ||
| digest: (value: Record<string, unknown>) => string, | ||
| ): (bytes: Uint8Array) => string { | ||
| return (bytes) => digest(JSON.parse(new TextDecoder().decode(bytes)) as Record<string, unknown>); | ||
| } | ||
|
|
||
| function digestForArtifactClass(artifactClass: ArtifactClass): (bytes: Uint8Array) => string { | ||
| switch (artifactClass) { | ||
| case "fixture": | ||
| return fixtureDigest; | ||
| case "scenario_manifest": | ||
| return jsonDigest(scenarioManifestDigest); | ||
| case "suite_manifest": | ||
| return jsonDigest(suiteManifestDigest); | ||
| case "claim_source_manifest": | ||
| return (bytes) => { | ||
| const parsed = JSON.parse(new TextDecoder().decode(bytes)); | ||
| return claimSourceManifestDigest(validateClaimSourceManifest(parsed).manifest); | ||
| }; | ||
| default: | ||
| return artifactBytesDigest; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
A parse failure inside a candidate digest function escapes as harness_failure and aborts the candidate loop.
jsonDigest at line 68 calls JSON.parse before it can compute a digest, and the claim_source_manifest closure at lines 80-83 additionally calls validateClaimSourceManifest, which throws LabValidationError. Neither error is an ArtifactFsError.
Trace what happens when one of these throws. The call site is contentDigest(buf) at src/lab/artifacts/secure-fs.ts line 367, inside the try. The catch at line 370 finds no raw ENOENT and no ArtifactFsError, so line 375 wraps it with the default code: ArtifactFsError("harness_failure", "artifact read failed: <parse message>"). Back in getVerified at lines 119-125, that error has code !== "artifact_mismatch" and a message that does not contain "mismatch", so the loop rethrows immediately instead of trying the remaining candidates.
Reachable case: a corrupt suite_manifest read through the fallback path with no artifactClass. Candidate 1 and candidate 2 hash raw bytes and report a mismatch, so the loop continues. Candidate 3 calls JSON.parse on the truncated bytes, throws a SyntaxError, and the loop aborts. validateRequiredArtifacts in src/lab/projection/rebuild.ts then classifies the result by err.code === "artifact_mismatch" || err.message.includes("mismatch"), so the corruption is recorded as missing_artifact instead of artifact_mismatch. The artifact is present on disk; only its content is bad.
Scope note: rebuild.ts passes artifactClass on every call, so the single-candidate path is used there and the loop abort itself does not fire. The misclassification still does. Contain the parse failure where it occurs.
🐛 Proposed fix
Translate a content-parse failure into a mismatch, which is what it is:
function jsonDigest(
digest: (value: Record<string, unknown>) => string,
): (bytes: Uint8Array) => string {
- return (bytes) => digest(JSON.parse(new TextDecoder().decode(bytes)) as Record<string, unknown>);
+ return (bytes) => {
+ try {
+ return digest(JSON.parse(new TextDecoder().decode(bytes)) as Record<string, unknown>);
+ } catch {
+ throw new ArtifactFsError("artifact_mismatch", "artifact content is not valid JSON");
+ }
+ };
}
function digestForArtifactClass(artifactClass: ArtifactClass): (bytes: Uint8Array) => string {
switch (artifactClass) {
case "fixture":
return fixtureDigest;
case "scenario_manifest":
return jsonDigest(scenarioManifestDigest);
case "suite_manifest":
return jsonDigest(suiteManifestDigest);
case "claim_source_manifest":
return (bytes) => {
- const parsed = JSON.parse(new TextDecoder().decode(bytes));
- return claimSourceManifestDigest(validateClaimSourceManifest(parsed).manifest);
+ try {
+ const parsed = JSON.parse(new TextDecoder().decode(bytes));
+ return claimSourceManifestDigest(validateClaimSourceManifest(parsed).manifest);
+ } catch (err) {
+ if (err instanceof ArtifactFsError) throw err;
+ throw new ArtifactFsError("artifact_mismatch", "claim-source artifact failed validation");
+ }
+ };
default:
return artifactBytesDigest;
}
}readArtifactBytes already rethrows an ArtifactFsError unchanged at line 374, so the artifact_mismatch code reaches getVerified, the candidate loop continues as intended, and rebuild.ts records artifact_mismatch. The replacement messages carry no payload content, which keeps the corruption strings free of artifact bytes.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function jsonDigest( | |
| digest: (value: Record<string, unknown>) => string, | |
| ): (bytes: Uint8Array) => string { | |
| return (bytes) => digest(JSON.parse(new TextDecoder().decode(bytes)) as Record<string, unknown>); | |
| } | |
| function digestForArtifactClass(artifactClass: ArtifactClass): (bytes: Uint8Array) => string { | |
| switch (artifactClass) { | |
| case "fixture": | |
| return fixtureDigest; | |
| case "scenario_manifest": | |
| return jsonDigest(scenarioManifestDigest); | |
| case "suite_manifest": | |
| return jsonDigest(suiteManifestDigest); | |
| case "claim_source_manifest": | |
| return (bytes) => { | |
| const parsed = JSON.parse(new TextDecoder().decode(bytes)); | |
| return claimSourceManifestDigest(validateClaimSourceManifest(parsed).manifest); | |
| }; | |
| default: | |
| return artifactBytesDigest; | |
| } | |
| } | |
| function jsonDigest( | |
| digest: (value: Record<string, unknown>) => string, | |
| ): (bytes: Uint8Array) => string { | |
| return (bytes) => { | |
| try { | |
| return digest(JSON.parse(new TextDecoder().decode(bytes)) as Record<string, unknown>); | |
| } catch { | |
| throw new ArtifactFsError("artifact_mismatch", "artifact content is not valid JSON"); | |
| } | |
| }; | |
| } | |
| function digestForArtifactClass(artifactClass: ArtifactClass): (bytes: Uint8Array) => string { | |
| switch (artifactClass) { | |
| case "fixture": | |
| return fixtureDigest; | |
| case "scenario_manifest": | |
| return jsonDigest(scenarioManifestDigest); | |
| case "suite_manifest": | |
| return jsonDigest(suiteManifestDigest); | |
| case "claim_source_manifest": | |
| return (bytes) => { | |
| try { | |
| const parsed = JSON.parse(new TextDecoder().decode(bytes)); | |
| return claimSourceManifestDigest(validateClaimSourceManifest(parsed).manifest); | |
| } catch (err) { | |
| if (err instanceof ArtifactFsError) throw err; | |
| throw new ArtifactFsError("artifact_mismatch", "claim-source artifact failed validation"); | |
| } | |
| }; | |
| default: | |
| return artifactBytesDigest; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lab/artifacts/store.ts` around lines 65 - 87, Contain parsing and
manifest-validation failures within digestForArtifactClass by converting them to
ArtifactFsError with code artifact_mismatch and concise messages that do not
include artifact content. Apply this to jsonDigest and the claim_source_manifest
validation path so secure-fs readArtifactBytes propagates the mismatch
unchanged, allowing getVerified to continue candidates and rebuild
classification to remain artifact_mismatch.
Comment on lines
+78
to
+84
| if ( | ||
| /^[A-Za-z]:\\/.test(value) || | ||
| /(?:^|[\s"'([])\/(?:home|Users|tmp|var|etc|root|mnt)\//.test(value) || | ||
| value.includes("\\Users\\") | ||
| ) { | ||
| throw new LabValidationError("raw_path", `${path} contains raw filesystem path`); | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
The boundary class [\s"'([] lets embedded POSIX paths through. Widen it or drop it.
Line 80 requires the path to start the string or to follow a space, a quote, an opening parenthesis, or an opening bracket. Common separators are missing: =, :, ,, >, and @.
Concrete failure: the value config=/home/alice/work/repo does not match. /home/alice/... is preceded by =, which is not in the class. The value therefore passes enforceEventStructureLimits and the operator username is written into the append-only JSONL ledger. observedSummary is populated from harness output in src/lab/observe/from-conformance.ts, and free-form harness text of the form key=/home/<user>/... is realistic. Removal afterwards requires a purge tombstone.
Note the asymmetry with the artifact sanitizer. scrubString in src/lab/artifacts/sanitize.ts line 86 applies the same directory set with no leading boundary at all, so the artifact path is strictly stricter than the ledger path for embedded occurrences. Align the two.
Keep a boundary if you want to avoid tripping on https://host/var/x, but widen it to cover non-space delimiters.
🔒️ Proposed fix
if (
/^[A-Za-z]:\\/.test(value) ||
- /(?:^|[\s"'([])\/(?:home|Users|tmp|var|etc|root|mnt)\//.test(value) ||
+ /(?:^|[^A-Za-z0-9._~-])\/(?:home|Users|tmp|var|etc|root|mnt)\//.test(value) ||
value.includes("\\Users\\")
) {The negated class rejects any delimiter that is not a path-safe character, so config=/home/alice/x, dir:/tmp/y, and a,/var/z are all caught, while .../var/... inside a longer path segment name is not newly matched.
As per path instructions: "Watch for credential handling: tokens and OAuth material must never be logged or serialized into responses." The same reasoning applies to operator usernames embedded in filesystem paths.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lab/events/limits.ts` around lines 78 - 84, Update the POSIX-path
detection in enforceEventStructureLimits to recognize embedded paths after
non-path delimiters such as “=”, “:”, “,”, “>”, and “@”, matching scrubString’s
broader detection behavior. Prefer a boundary that rejects path-safe preceding
characters while still avoiding matches embedded within longer path segments,
and preserve the existing Windows-path and raw_path validation behavior.
Source: Path instructions
Comment on lines
+81
to
+95
| if (process.platform !== "win32") { | ||
| const dirFd = openSync(parent, "r"); | ||
| try { | ||
| fsyncSync(dirFd); | ||
| } finally { | ||
| closeSync(dirFd); | ||
| } | ||
| } | ||
| } catch (err) { | ||
| try { | ||
| unlinkSync(tmpPath); | ||
| } catch { | ||
| // Preserve the original failure. The temp file may already have been renamed. | ||
| } | ||
| throw err; |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
A directory-fsync failure after a successful rename is reported as a failed ledger step.
Line 80 completes renameSync, so the new ledger is committed and visible. Lines 81-88 then open the parent directory and fsyncSync it for rename durability. If either openSync or fsyncSync throws, control reaches the catch on line 89 and the error propagates out of atomicRewriteLedger.
purgeSensitiveEvidence pushes "ledger" onto completed on line 206, which is after atomicRewriteLedger returns. So the thrown PurgeError reports completedActions without "ledger", while the ledger was in fact replaced. The operator is told the ledger step did not run. Artifact deletion already happened on line 190. The reported residual state is wrong in the one direction that matters for a sensitive purge audit.
Separate the durability step from the commit step. Record the commit, then surface an fsync failure as a distinct, non-rollback condition.
🐛 Proposed fix to distinguish commit from durability
renameSync(tmpPath, ledgerPath);
- if (process.platform !== "win32") {
- const dirFd = openSync(parent, "r");
- try {
- fsyncSync(dirFd);
- } finally {
- closeSync(dirFd);
- }
- }
} catch (err) {
try {
unlinkSync(tmpPath);
} catch {
// Preserve the original failure. The temp file may already have been renamed.
}
throw err;
}
+ // The rename is committed. A directory fsync failure affects durability only,
+ // so it must not be reported as a failed ledger rewrite.
+ if (process.platform !== "win32") {
+ try {
+ const dirFd = openSync(parent, "r");
+ try {
+ fsyncSync(dirFd);
+ } finally {
+ closeSync(dirFd);
+ }
+ } catch {
+ // Best effort: the ledger content is already committed.
+ }
+ }
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lab/ledger/purge.ts` around lines 81 - 95, Update atomicRewriteLedger so
the successful rename is recorded as the commit before parent-directory
openSync/fsyncSync durability operations. Preserve the committed state and
ensure fsync failures are surfaced as a distinct non-rollback error, allowing
purgeSensitiveEvidence to report "ledger" in completed while still indicating
durability failure.
Comment on lines
+931
to
+950
| test("replay handles UTF-8 split across chunk boundaries", () => { | ||
| withHome((home) => { | ||
| const ledger = join(home, "lab", "compatibility.jsonl"); | ||
| mkdirSync(join(home, "lab"), { recursive: true }); | ||
| const obs = baseObservation({ | ||
| assertions: [{ | ||
| id: "utf8", | ||
| operator: "equals", | ||
| required: true, | ||
| passed: true, | ||
| expectedSummary: "café", | ||
| observedSummary: "café", | ||
| }], | ||
| }); | ||
| writeFileSync(ledger, `${jcsStringify(obs)}\n`, "utf8"); | ||
| const replay = replayLabLedger(ledger); | ||
| expect(replay.events).toHaveLength(1); | ||
| expect(replay.corruptions).toHaveLength(0); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
This test never crosses a chunk boundary, so it does not test what its name says.
replayLabLedger in src/lab/ledger/store.ts line 187 reads in 64 KiB chunks and calls splitIncompleteUtf8Tail on each chunk. The ledger written on line 945 holds one small observation, well under 64 KiB. The read loop therefore performs a single iteration, the carry buffer stays empty, and the multi-byte handling under test never runs. The café payload is decoded inside one chunk, exactly as any ASCII payload would be.
The test passes today and would keep passing if splitIncompleteUtf8Tail were deleted.
Pad the ledger so a multi-byte character straddles offset 65536.
💚 Proposed fix to force a split multi-byte sequence
writeFileSync(ledger, `${jcsStringify(obs)}\n`, "utf8");
+ // Pad the first line past 64 KiB so the second line's multi-byte
+ // characters land on the chunk boundary of the 64 KiB read loop.
+ const padded = baseObservation({
+ assertions: [{
+ id: "pad",
+ operator: "equals",
+ required: true,
+ passed: true,
+ expectedSummary: "p".repeat(64 * 1024),
+ observedSummary: "ok",
+ }],
+ });
+ writeFileSync(ledger, `${jcsStringify(padded)}\n${jcsStringify(obs)}\n`, "utf8");
const replay = replayLabLedger(ledger);
- expect(replay.events).toHaveLength(1);
+ expect(replay.events).toHaveLength(2);
expect(replay.corruptions).toHaveLength(0);Check the padded line against MAX_SERIALIZED_EVENT_BYTES and MAX_SANITIZED_STRING_FIELD in src/lab/events/limits.ts first. If the limit blocks a single oversized event, write two smaller events whose combined length places the café bytes across offset 65536.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/lab-evidence-ledger.test.ts` around lines 931 - 950, Update the test
named “replay handles UTF-8 split across chunk boundaries” to construct ledger
content where the bytes of a multi-byte character straddle offset 65536, forcing
replayLabLedger’s chunked reader and splitIncompleteUtf8Tail path to run.
Respect MAX_SERIALIZED_EVENT_BYTES and MAX_SANITIZED_STRING_FIELD; if one padded
event exceeds limits, use two smaller events whose combined serialized length
positions the café bytes across the boundary while preserving the existing
replay and corruption assertions.
Comment on lines
+952
to
+973
| test("zero applicable required scenarios yields UNKNOWN not PROBED", () => { | ||
| const authority = loadCaseAuthority(); | ||
| const suiteManifest = expandSuiteManifest("responses-core", authority); | ||
| const subject = protocolSubject("zero-applicable"); | ||
| const loadScenarioManifest = (digest: string) => { | ||
| for (const caseRecord of authority.cases) { | ||
| const expanded = expandScenario(caseRecord, authority); | ||
| if (scenarioManifestDigest(expanded) === digest) return expanded; | ||
| } | ||
| return null; | ||
| }; | ||
| const evaluation = evaluateAllApplicableRequiredPassV1( | ||
| suiteManifest, | ||
| [], | ||
| "fixture", | ||
| { subject: { ...subject, surface: "responses-ws" }, loadScenarioManifest }, | ||
| ); | ||
| expect(evaluation.applicableRequiredScenarioIds).toEqual([]); | ||
| expect(evaluation.notes).toContain("empty_applicable_required_set"); | ||
| const projected = projectVerdicts([]); | ||
| expect(projected.verdicts).toEqual([]); | ||
| }); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Two new behaviors in src/lab/projection have no regression test.
This suite covers the empty-applicable-set path well, and the surrounding tests cover purge, rebuild determinism, and claim corruption. Two behaviors added in this PR are not exercised anywhere in tests/:
- Freshness enforcement.
src/lab/projection/verification.tslines 208-216 downgrade a stale pass tomissingand push astale_observation:<id>note.effectiveMaxAgeMson lines 87-94 also selects the stricter of the suite and scenario limits. No test setsmaxAgeMsand no test assertsstale_observation. A regression that drops theasOf - obs.completedAt > maxAgeMscomparison would silently restore VERIFIED for arbitrarily old evidence. - The UNSUPPORTED verdict.
src/lab/projection/verdicts.tslines 332-334 are the only producer of UNSUPPORTED, and no test reaches them. See the separate comment on that branch about thecontrolKindandonMatchkey names.
Add one test per behavior next to the existing verification tests.
As per path instructions, "A behavior change in src/ should come with a focused regression test near the existing tests for that subsystem."
#!/bin/bash
# Confirm that no test exercises freshness staleness or the UNSUPPORTED verdict.
set -uo pipefail
echo "=== staleness and freshness in tests ==="
rg -n -C3 'stale_observation|maxAgeMs|freshness' tests
echo
echo "=== UNSUPPORTED and capability-absence controls in tests ==="
rg -n -C3 'UNSUPPORTED|capability_absence_control' tests🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/lab-evidence-ledger.test.ts` around lines 952 - 973, Add focused
regression tests beside the existing verification tests: one configuring
suite/scenario maxAgeMs and asserting an older passing observation becomes
missing with a stale_observation:<id> note, including coverage that the stricter
limit is selected; and one exercising the verdicts.ts UNSUPPORTED branch with
the expected controlKind and onMatch keys, asserting the UNSUPPORTED verdict.
Source: Path instructions
Comment on lines
+975
to
+981
| test("newer pass supersedes older fail without DEGRADED", () => { | ||
| const failEvent = assignEventId({ ...baseObservation({ completedAt: 100 }), outcome: "fail" }); | ||
| const passEvent = assignEventId({ ...baseObservation({ completedAt: 200 }), outcome: "pass" }); | ||
| const projected = projectVerdicts([failEvent, passEvent]); | ||
| expect(projected.verdicts[0]?.verdict).not.toBe("DEGRADED"); | ||
| expect(projected.verdicts[0]?.contradictingEventIds).toContain(failEvent.eventId); | ||
| }); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
not.toBe("DEGRADED") also passes when the projection collapses.
Line 979 asserts only what the verdict is not. UNKNOWN, BLOCKED, and an absent row at index 0 all satisfy it. If a regression drops the group entirely, projected.verdicts[0] is undefined, ?.verdict is undefined, and the assertion still passes.
The expected value is deterministic here. The test passes no loadSuiteManifest, so src/lab/projection/verdicts.ts line 342 takes the suite_manifest_unavailable branch and returns PROBED.
💚 Proposed stronger assertions
const projected = projectVerdicts([failEvent, passEvent]);
- expect(projected.verdicts[0]?.verdict).not.toBe("DEGRADED");
+ expect(projected.verdicts).toHaveLength(1);
+ expect(projected.verdicts[0]!.verdict).toBe("PROBED");
+ expect(projected.verdicts[0]!.notes).toContain("suite_manifest_unavailable");
expect(projected.verdicts[0]?.contradictingEventIds).toContain(failEvent.eventId);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/lab-evidence-ledger.test.ts` around lines 975 - 981, Strengthen the
test around projectVerdicts in “newer pass supersedes older fail without
DEGRADED” by asserting the projected verdict exists and its verdict is exactly
“PROBED”, matching the suite_manifest_unavailable behavior when no
loadSuiteManifest is supplied. Keep the contradictingEventIds assertion for
failEvent unchanged.
Comment on lines
+1081
to
+1084
| const rebuilt = rebuildLabProjection(home); | ||
| const snap = readVerdictSnapshot(rebuilt.sqlitePath); | ||
| expect(snap.every((row) => (row as { verdict: string }).verdict !== "CLAIMED")).toBe(true); | ||
| }); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Array.prototype.every returns true for an empty snapshot, and the expected row here is precisely known.
Line 1083 asserts that no row carries the CLAIMED verdict. every is vacuously true, so the assertion passes whether the corrupt-supersession rule works or the projection produced nothing at all. The test title also promises that C1 is not resurrected, and no assertion checks that.
The expected state is determinate. C2 supersedes C1, so resolveClaimStates in src/lab/projection/verdicts.ts line 149 leaves C2 as the only current claim. C2's sourceManifestDigest on line 1076 points at bytes that were never stored, so validateRequiredArtifacts in src/lab/projection/rebuild.ts line 438 adds C2 to unusableClaimEventIds. state.unusable is then true, and src/lab/projection/verdicts.ts lines 253-264 emit exactly one row: verdict UNKNOWN with the note current_claim_unusable and contributingEventIds naming C2.
Assert that row. It proves both halves of the title.
💚 Proposed fix to pin the expected projection state
const rebuilt = rebuildLabProjection(home);
const snap = readVerdictSnapshot(rebuilt.sqlitePath);
- expect(snap.every((row) => (row as { verdict: string }).verdict !== "CLAIMED")).toBe(true);
+ expect(snap).toHaveLength(1);
+ const row = snap[0] as {
+ verdict: string;
+ notes_json: string;
+ contributing_event_ids_json: string;
+ };
+ expect(row.verdict).toBe("UNKNOWN");
+ expect(JSON.parse(row.notes_json)).toContain("current_claim_unusable");
+ // C1 must not be resurrected as the current claim.
+ expect(JSON.parse(row.contributing_event_ids_json)).toEqual([c2.eventId]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/lab-evidence-ledger.test.ts` around lines 1081 - 1084, Replace the
vacuous snap.every assertion in the rebuild projection test with an exact
assertion that the snapshot contains one row for C2, with verdict UNKNOWN, note
current_claim_unusable, and contributingEventIds identifying C2; preserve the
existing rebuilt snapshot setup.
Comment on lines
+1150
to
+1160
| test("observationFromConformanceResult closes internally created artifact store", () => { | ||
| withHome((home) => { | ||
| const authority = loadCaseAuthority(); | ||
| const caseRecord = discoverScenarios(authority, ["responses-core"])[0]!; | ||
| const result = syntheticPassResult(caseRecord); | ||
| const { event } = observationFromConformanceResult(result, caseRecord, authority, { configDir: home }); | ||
| expect(event.eventKind).toBe("observation"); | ||
| const dir = openTrustedArtifactDir(join(home, "lab", "artifacts")); | ||
| closeTrustedArtifactDir(dir); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
This test cannot fail for the reason its title states.
The title says the internally created artifact store is closed. Lines 1157-1158 open a second trusted directory handle and close it. That call succeeds on POSIX whether or not the first descriptor leaked, because nothing limits concurrent directory handles at this scale. On Windows the directory is opened for reading, which also does not conflict with another read handle. So the assertion surface is expect(event.eventKind).toBe("observation") plus two calls that never throw.
observationFromConformanceResult in src/lab/observe/from-conformance.ts closes the store in a finally block when it owns it. A regression that deletes that finally leaves this test green.
Test the observable consequence instead: after the call, removing the artifacts directory must succeed. An open descriptor blocks rmSync on Windows, which is the platform this PR targets.
💚 Proposed fix to assert the observable effect
const { event } = observationFromConformanceResult(result, caseRecord, authority, { configDir: home });
expect(event.eventKind).toBe("observation");
- const dir = openTrustedArtifactDir(join(home, "lab", "artifacts"));
- closeTrustedArtifactDir(dir);
+ // A leaked directory descriptor blocks removal on Windows, the target
+ // platform for this PR.
+ expect(() => rmSync(join(home, "lab", "artifacts"), { recursive: true })).not.toThrow();
+ expect(existsSync(join(home, "lab", "artifacts"))).toBe(false);If rmSync proves too permissive on the CI platform, assert instead that a caller-supplied store is left open while an internally created one is not, by passing artifactStore explicitly in a second case and checking that the caller can still call store.get.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| test("observationFromConformanceResult closes internally created artifact store", () => { | |
| withHome((home) => { | |
| const authority = loadCaseAuthority(); | |
| const caseRecord = discoverScenarios(authority, ["responses-core"])[0]!; | |
| const result = syntheticPassResult(caseRecord); | |
| const { event } = observationFromConformanceResult(result, caseRecord, authority, { configDir: home }); | |
| expect(event.eventKind).toBe("observation"); | |
| const dir = openTrustedArtifactDir(join(home, "lab", "artifacts")); | |
| closeTrustedArtifactDir(dir); | |
| }); | |
| }); | |
| test("observationFromConformanceResult closes internally created artifact store", () => { | |
| withHome((home) => { | |
| const authority = loadCaseAuthority(); | |
| const caseRecord = discoverScenarios(authority, ["responses-core"])[0]!; | |
| const result = syntheticPassResult(caseRecord); | |
| const { event } = observationFromConformanceResult(result, caseRecord, authority, { configDir: home }); | |
| expect(event.eventKind).toBe("observation"); | |
| // A leaked directory descriptor blocks removal on Windows, the target | |
| // platform for this PR. | |
| expect(() => rmSync(join(home, "lab", "artifacts"), { recursive: true })).not.toThrow(); | |
| expect(existsSync(join(home, "lab", "artifacts"))).toBe(false); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/lab-evidence-ledger.test.ts` around lines 1150 - 1160, Update the test
“observationFromConformanceResult closes internally created artifact store” to
assert the observable cleanup: after observationFromConformanceResult returns,
remove the artifacts directory with the appropriate filesystem operation and
assert it succeeds. Replace the ineffective open-and-close trusted directory
calls while preserving the eventKind assertion.
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.
Summary
compatibility.jsonlevidence ledger, content-addressedartifacts/store with descriptor/handle-bound no-follow I/O, and disposable rebuildablecompatibility.sqliteprojection undergetConfigDir()/lab/.observation,claim_snapshot,invalidation,purge_tombstone), JCS/domain-separated IDs, ClaimSourceManifestV1, invalidation/purge semantics, and a CL-01 → observation persistence seam (no CL-03 probes).4bb249b756abd468c675d2d92fffe4da95ad3e2a(feat(lab): CL-01 deterministic protocol conformance harness #1320); updates programme stack status accordingly. CL-03 remains NOT STARTED.Verification
bun x tsc --noEmit: passedbun run privacy:scan: passedbun test tests/repo-hygiene.test.ts: 11 passedbun test tests/lab-conformance-harness.test.ts tests/lab-evidence-ledger.test.ts: 38 passed (CL-01 + CL-02)git diff --check: passedbun run testnot claimed green on this Windows/Bun 1.3.14 host (known baseline; see CL-00/CL-01 notes)Checklist
devEOF
Summary by CodeRabbit