feat(lab): CL-07 task effectiveness evidence - #1438
Merged
Merged
Conversation
Add a bounded Lab-owned fabric-core synthetic-patch producer with exact-tree-diff verification, scratch sandbox, TaskSubjectV1 identity, and observation ingestion without a general Agent Fabric platform.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCL-07 adds a bounded Fabric task-effectiveness producer for one synthetic patch scenario. It defines task outcomes and manifests, enforces scratch and execution limits, verifies exact tree changes, persists idempotent observations, exposes catalog metadata, and projects verification verdicts. ChangesFabric task-effectiveness evidence
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant SyntheticPatchProducer
participant FabricExecutor
participant ScratchWorkspace
participant TreeVerifier
participant ObservationStore
participant VerdictProjection
SyntheticPatchProducer->>FabricExecutor: Produce bounded synthetic patch
FabricExecutor->>ScratchWorkspace: Create fixture and apply patch
FabricExecutor->>TreeVerifier: Verify exact tree diff
TreeVerifier-->>FabricExecutor: Return structured result
FabricExecutor->>ObservationStore: Persist outcome and artifacts
ObservationStore->>VerdictProjection: Provide task-effectiveness observation
VerdictProjection-->>ObservationStore: Project verification verdict
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
✅ Deterministic PR hygiene checks passed. |
Record the implementation head and draft PR on the programme status documents.
Wire task_effectiveness + fabric through all-applicable-required-pass-v1 so successful synthetic-patch observations can leave PROBED.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Harden scratch IO, timeouts, outcome validation, ledger idempotency, and projection/catalog paths so the review findings stay closed on this branch.
Keep the CL-06 accepted head from dev and preserve the in-progress CL-07 row.
Wibias
marked this pull request as ready for review
August 10, 2026 21:41
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lab/fabric/observe.ts (1)
189-198: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not persist raw producer verifier data.
Line 194 stores
outcome.verifierwithout an allowlist or diagnostic sanitization. A producer can place a token inverifier.reasonor an extra nested property. The assertion at Line 236 sanitizes the diagnostic, but theverifier_summaryartifact keeps the raw value. Artifact and read surfaces can then expose credentials.Build this artifact from validated, allowlisted fields. Sanitize and truncate every diagnostic field before persistence. Reject unknown nested verifier fields. Add a regression test that persists a credential-like verifier diagnostic and confirms that neither the ledger nor artifact payload contains it.
🤖 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/fabric/observe.ts` around lines 189 - 198, The verifier_summary artifact construction in observe should not persist raw outcome.verifier data. Build verifier from an explicit allowlist of validated fields, reject unknown nested properties, and sanitize and truncate every diagnostic field before passing the payload to store.put. Add a regression test covering a credential-like verifier diagnostic and assert the secret is absent from both the ledger and persisted artifact payload.Source: Path instructions
🤖 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 `@src/lab/fabric/executor.ts`:
- Around line 109-116: Move options.producePatch execution out of the caller’s
event loop into a terminable worker or equivalent isolated boundary, and update
the flow around createTimeoutController and controller.race to terminate it on
total or inactivity timeout. Avoid exposing scratch.root directly to an
unbounded producer; mediate filesystem access through the protected boundary.
Add a regression test using a non-terminating worker producer that verifies task
completion and scratch cleanup within the configured timeout.
In `@src/lab/fabric/observe.ts`:
- Around line 126-134: In src/lab/fabric/observe.ts lines 126-134, update
assertFabricOutcomeV1 to validate the complete nested TaskSubjectV1,
RouteSubjectV1, verifier, usage, limits, failure, artifact-digest, and
source-reference contracts before observation creation, and reject contradictory
top-level versus nested subjects with FabricTaskError. In
tests/lab-fabric-task.test.ts lines 323-329, add valid-baseline outcome cases
with malformed verifier, usage, and subject fields, asserting each call throws
FabricTaskError.
In `@src/lab/fabric/scratch.ts`:
- Around line 19-20: Update the scratch-file write flow around the path
validation and open logic to traverse each directory via file descriptors using
O_NOFOLLOW rather than reopening absolute pathnames after lstatSync validation.
Open the final file from the validated parent descriptor, then fstat that
descriptor and retain the existing scratch-root and file-mode checks.
In `@src/lab/ledger/store.ts`:
- Around line 35-58: Update withLedgerLock to record lock ownership and creation
metadata, and when acquisition encounters an existing lock, recover it only when
its metadata proves it is safely stale; otherwise continue retrying until the
existing deadline and error behavior. Ensure cleanup removes only the lock owned
by the current acquisition, and add a regression test that pre-creates an
expired lock and verifies appendLabEventIfAbsent succeeds.
In `@src/lab/observe/from-live.ts`:
- Around line 23-25: Update the environment-attribution classifications used by
failureFromLiveResult to include "inactivity_timeout", ensuring execution
timeouts persist with attribution "environment" rather than "route". Add a
regression test covering a persisted inactivity-timeout failure and asserting
its environment attribution.
In `@src/lab/paths.ts`:
- Around line 5-10: Update ensureRestrictedDir to reject symbolic links and
non-directory entries immediately after lstatSync, before any chmodSync call;
preserve the existing mode enforcement only for real directories. Add a
regression test covering a symlinked labScratchDir, asserting
createSyntheticScratch fails and no scratch data is written outside the Lab
root.
In `@src/lab/projection/verification.ts`:
- Around line 86-92: The requirements applicability logic in
src/lab/projection/verification.ts:86-92 must preserve the wildcard default by
treating omitted requirements.platforms as ["*"] rather than []. Update the
platform default used by the relevant verification function while preserving
explicit platform handling. Add a test case in
tests/lab-fabric-task.test.ts:351-378 with platforms omitted and a valid Fabric
capability, asserting that the scenario is applicable.
---
Outside diff comments:
In `@src/lab/fabric/observe.ts`:
- Around line 189-198: The verifier_summary artifact construction in observe
should not persist raw outcome.verifier data. Build verifier from an explicit
allowlist of validated fields, reject unknown nested properties, and sanitize
and truncate every diagnostic field before passing the payload to store.put. Add
a regression test covering a credential-like verifier diagnostic and assert the
secret is absent from both the ledger and persisted artifact payload.
🪄 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: 6b9c3041-916f-4423-94cb-91c1577c2ea1
📒 Files selected for processing (21)
devlog/_plan/260807_compatibility_lab/001_pr_stack_status.mdsrc/lab/conformance/types.tssrc/lab/events/types.tssrc/lab/events/validate.tssrc/lab/fabric/constants.tssrc/lab/fabric/executor.tssrc/lab/fabric/index.tssrc/lab/fabric/manifest.tssrc/lab/fabric/observe.tssrc/lab/fabric/patch.tssrc/lab/fabric/scratch.tssrc/lab/fabric/subject.tssrc/lab/fabric/types.tssrc/lab/ledger/store.tssrc/lab/observe/from-conformance.tssrc/lab/observe/from-live.tssrc/lab/paths.tssrc/lab/projection/verdicts.tssrc/lab/projection/verification.tssrc/lab/query/catalog.tstests/lab-fabric-task.test.ts
Harden fabric observation validation and scratch IO, recover stale ledger locks, attribute inactivity_timeout as environment, and add regression tests.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lab/fabric/executor.ts (1)
119-147: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
usage.inactiveMsrecords total production time, not inactive time, and the success path never sets it at all.Line 122 computes
usage.inactiveMs = Math.max(0, completedAt - produceStarted).produceStartedis captured at Line 106, immediately before the producer starts. The expression therefore measures the entire wall-clock duration of patch production. It does not measure idle time.Two consequences follow.
- Wrong value on every producer failure. This assignment runs for every
FabricTaskErrorfrom the producer, includingtimeout,budget_exhausted, andmalformed_producer_outcome. A producer that streams output continuously for four seconds and then fails a budget check recordsinactiveMs: 4000while it was never inactive.- Always zero on success. The success path at Line 161 sets only
usage.elapsedMs.inactiveMskeeps its initial value of0from Line 95, even when the producer had real idle gaps thatreportActivityobserved.
src/lab/fabric/observe.tsvalidateFabricUsageonly checks that the value is a non-negative integer, so neither case is rejected. The wrong value is sealed intoFabricTaskOutcomeV1, persisted through the artifact store, and written to the ledger. Any later analysis of Fabric inactivity reads fabricated numbers.
createTimeoutControlleralready tracks activity throughreportActivity. Expose the measured idle time and use it on both paths.🐛 Proposed fix — report measured inactivity
Add an accessor to the controller:
function createTimeoutController(totalMs: number, inactivityMs: number, options: RunFabricTaskOptions) { + // Track the longest observed gap between reportActivity() calls so the + // outcome can report real inactivity instead of total elapsed time. + let maxInactiveMs = 0;Then use it in both branches:
const completedAt = options.now?.() ?? Date.now(); usage.elapsedMs = completedAt - startedAt; - usage.inactiveMs = Math.max(0, completedAt - produceStarted); + usage.inactiveMs = controller.inactiveMs();const verifier = verifyExactTreeDiffV1(scratch.root); const completedAt = options.now?.() ?? Date.now(); usage.elapsedMs = completedAt - startedAt; + usage.inactiveMs = controller.inactiveMs();The
controllerbinding must move out of the innertryso both paths can read it.Add a regression test with a producer that calls
reportActivityon a known cadence, then assert that the sealedusage.inactiveMsmatches the largest injected gap rather than the total duration.As per path instructions: "A behavior change in src/ should come with a focused regression test near the existing tests for that subsystem."
🤖 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/fabric/executor.ts` around lines 119 - 147, Expose the measured idle duration from createTimeoutController, then update the executor’s controller binding so it is available on both success and FabricTaskError paths. Replace the elapsed-duration assignment to usage.inactiveMs with the controller’s measured inactivity accessor, and set it before sealing either outcome. Add a focused regression test using periodic reportActivity calls to verify inactiveMs reflects the largest injected gap rather than total production time.Source: Path instructions
🤖 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 `@src/lab/fabric/observe.ts`:
- Around line 110-115: Change wrapValidationError to declare a never return
type, since every execution path throws. Remove the definite-assignment
assertions from taskSubject and routeSubject so TypeScript enforces their
initialization while preserving the existing catch-and-throw behavior.
In `@src/lab/fabric/scratch.ts`:
- Around line 77-89: Update revalidateScratchDir so its behavior no longer
implies that comparing fstatSync(dir.fd) with dir.identity detects directory
replacement: either document that the check only validates the descriptor and
detects invalid descriptors, or replace it with a descriptor-versus-path
comparison that can detect pathname entry replacement. Preserve the existing
descriptor-relative and O_NOFOLLOW protections, and anchor the change in
revalidateScratchDir and openTrustedScratchRoot.
- Around line 173-210: Update openScratchRelativePath to close all
intermediateFds on the successful final openAtScratch path as well as on errors,
while preserving the returned final descriptor; add a focused regression test
near the existing lab fabric tests that performs many writeScratchFileUtf8 calls
against one scratch tree and verifies the process descriptor count does not grow
monotonically.
- Around line 379-396: Remove the unreachable stats.isSymbolicLink() checks and
their symlink-rejection branches from both readScratchFileUtf8 and
writeScratchFileUtf8. Keep the existing assertRegularFile checks and all
surrounding descriptor cleanup, size validation, and read/write behavior
unchanged.
- Around line 150-171: Change getTrustedScratch to reject scratch roots absent
from trustedScratchRoots instead of opening and caching them lazily; only
createSyntheticScratch registration should establish trusted descriptors, while
releaseTrustedScratch remains responsible for cleanup. Add a focused regression
test covering createSyntheticScratch cleanup followed by readScratchFileUtf8 on
the same root, asserting the read fails rather than reopening it.
- Around line 398-415: Update writeScratchFileUtf8 to perform
resolveInsideScratch containment validation before any directory creation, and
replace the pathname-based ensureRestrictedDir call with trusted-descriptor,
mkdirat-style creation for each missing parent component. Preserve the guarded
openScratchRelativePath flow. Add a focused regression test covering an
intermediate scratch-directory symlink to an external target and assert that no
nested directory is created outside the scratch tree.
- Around line 297-308: Wrap the construction logic in createSyntheticScratch,
from openTrustedScratchRoot through file creation and writing, in failure
cleanup that calls releaseTrustedScratch and removes the created scratch tree
before rethrowing. Ensure cleanup runs for budget validation,
openScratchRelativePath, writeSync, and any later construction errors while
preserving the existing successful ScratchTree cleanup behavior.
- Around line 28-33: Remove the OpenSyncWithDir type, the "dirfd" mode, and
detectScratchIoMode from scratch I/O handling. Update the affected
scratch-directory operations to always use the existing validated pathname-based
branch, including nested scratch directories, without relying on unsupported
fs.openSync options.
In `@src/lab/ledger/store.ts`:
- Around line 59-75: Update tryAcquireLedgerLock to bound stale-lock retries,
propagate unlinkSync failures, and close the descriptor when writeSync fails;
replace time-based lock replacement with ownership held through the critical
section so live holders cannot be replaced and cleanup cannot remove a
replacement lock. Add coverage for deletion failure, concurrent writers, and
duplicate-event corruption. Update capability verification to treat "*" in
capability.platforms as matching every required platform, and strengthen
lab-fabric-task tests to assert FabricTaskError and expected error codes.
In `@src/lab/paths.ts`:
- Around line 6-18: Update ensureRestrictedDir to create and validate each path
component incrementally rather than calling recursive mkdirSync on the full
path, rejecting any existing symbolic-link component before proceeding; import
resolve, sep, and join from node:path, and tolerate EEXIST from per-component
creation. Add a focused regression test near the existing lab tests that makes a
scratch-directory ancestor link to an external directory, calls ensureLabDirs,
and verifies the link target remains untouched.
---
Outside diff comments:
In `@src/lab/fabric/executor.ts`:
- Around line 119-147: Expose the measured idle duration from
createTimeoutController, then update the executor’s controller binding so it is
available on both success and FabricTaskError paths. Replace the
elapsed-duration assignment to usage.inactiveMs with the controller’s measured
inactivity accessor, and set it before sealing either outcome. Add a focused
regression test using periodic reportActivity calls to verify inactiveMs
reflects the largest injected gap rather than total production time.
🪄 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: 6f3c4a6b-ec8c-4f7d-a889-14acdb1b1a43
📒 Files selected for processing (10)
src/lab/fabric/executor.tssrc/lab/fabric/observe.tssrc/lab/fabric/scratch.tssrc/lab/ledger/store.tssrc/lab/live/transport.tssrc/lab/observe/from-live.tssrc/lab/paths.tssrc/lab/projection/verification.tstests/lab-fabric-task.test.tstests/lab-live-probe.test.ts
Document exported and internal fabric helpers, scratch IO, observation validation, and ledger lock recovery.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/lab/fabric/observe.ts`:
- Around line 208-211: Update routeSubjectsMatch to compare canonical
representations instead of JSON.stringify output, using jcsStringify for both
RouteSubjectV1 values or the existing canonical subject ID mechanism. Preserve
the boolean equality result while making equivalent subjects with different key
insertion orders match.
🪄 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: 2e4e0fc0-dad5-41b5-88fd-9fbbf3a4b823
📒 Files selected for processing (11)
src/lab/fabric/executor.tssrc/lab/fabric/index.tssrc/lab/fabric/manifest.tssrc/lab/fabric/observe.tssrc/lab/fabric/patch.tssrc/lab/fabric/scratch.tssrc/lab/fabric/subject.tssrc/lab/fabric/types.tssrc/lab/fabric/verifier.tssrc/lab/ledger/store.tssrc/lab/projection/verification.ts
Harden scratch IO and restricted dirs, improve ledger lock ownership, treat capability platform wildcard as universal, and strengthen regression tests.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lab/projection/verification.ts (1)
188-196: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the capability object shape into a named type.
Lines 192-196 repeat the exact structural literal that
taskSubjectApplicableToRequirementsdeclares at Lines 81-85. The two must stay identical, because the evaluator forwards this option straight into that predicate. A named type keeps them synchronized and documents the option on the public surface.♻️ Proposed shared capability type
+export interface FabricCapability { + harnessFeatures: readonly string[]; + platforms: readonly string[]; + routePreconditions: readonly string[]; +} + export function taskSubjectApplicableToRequirements( requirements: ScenarioRequirements, - capability: { - harnessFeatures: readonly string[]; - platforms: readonly string[]; - routePreconditions: readonly string[]; - }, + capability: FabricCapability, ): boolean {- fabricCapability?: { - harnessFeatures: readonly string[]; - platforms: readonly string[]; - routePreconditions: readonly string[]; - }; + fabricCapability?: FabricCapability;🤖 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/projection/verification.ts` around lines 188 - 196, Extract the inline fabricCapability object shape into a shared named type, then reuse that type both in the public projection options around subject/routeSupportedClaims and in taskSubjectApplicableToRequirements. Preserve the existing readonly harnessFeatures, platforms, and routePreconditions properties so the evaluator can forward the option unchanged.
🤖 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 `@src/lab/fabric/scratch.ts`:
- Around line 247-267: Update ensureScratchRelativeDir to remove the
existsSync(next) pre-check and attempt mkdirSync(next, { mode: 0o700 }) directly
for each component. Match the sibling paths implementation by catching and
ignoring only EEXIST, while propagating other errors; retain the subsequent
lstatSync, symlink rejection, and assertRealDirectory checks.
- Around line 153-157: Consolidate intermediate descriptor cleanup around the
open/return flow in the scratch descriptor method, using one guarded helper or
finally path for intermediateFds. Ensure cleanup runs exactly once on both
success and failure, closes finalFd if cleanup fails after it is opened, and
preserves FabricTaskError wrapping for cleanup errors; remove the duplicated
unguarded and catch-path loops.
In `@src/lab/ledger/store.ts`:
- Around line 64-72: Update isLockHolderAlive to distinguish process.kill(pid,
0) errors: return false only for ESRCH, return true for EPERM, and preserve the
existing true result when no error occurs. Keep the documented predicate
behavior for other unexpected errors according to the surrounding error-handling
conventions.
- Around line 74-80: Update isLedgerLockStale to require the metadata-free lock
file to be at least LEDGER_LOCK_STALE_MS old before treating it as stale; use
the lock file’s modification time for this grace-period check and preserve
existing metadata-based holder and age handling. Add a focused regression test
near the expired-lock test in tests/lab-fabric-task.test.ts that creates a
current, metadata-free lock and verifies appendLabEventIfAbsent does not steal
it.
- Around line 82-109: Update tryAcquireLedgerLock so a failed unlinkSync waits
with sleepSyncMs(10) before retrying, while preserving immediate retry after
successful removal and rethrowing once the deadline is reached. Add a focused
regression test near the expired-lock test in tests/lab-fabric-task.test.ts that
forces lock removal to fail and verifies appendLabEventIfAbsent rejects within
the configured wait budget without spinning.
In `@src/lab/paths.ts`:
- Around line 11-41: Limit symlink rejection in the directory-creation helper to
the Lab root and its descendants, while treating ancestors above that boundary
as infrastructure. Add an explicit Lab-root boundary parameter to the helper and
pass it from ensureLabDirs and Fabric scratch creation; compare normalized path
components rather than string lengths. Preserve lstatSync validation for the Lab
root and child components, and add a regression test covering a symlinked
ancestor.
In `@tests/lab-fabric-task.test.ts`:
- Around line 498-512: Update the ensureLabDirs assertion in the “symlinked lab
root rejects directory escape via ensureLabDirs” test to require the specific
“restricted directory component is a symbolic link” error message, while
preserving the existing side-effect assertions that scratch and artifacts are
not created.
- Line 472: Replace the hard-coded PID in the stale-lock fixture of the relevant
test with a dynamically selected PID that is proven not to belong to a live
process, using a focused dead-PID helper near the existing ledger-lock tests.
Keep the stale timestamp and token unchanged, and ensure the fixture remains
compatible with isLedgerLockStale and tryAcquireLedgerLock recovery behavior.
- Around line 408-428: Replace the manual try/catch blocks around
observationFromFabricOutcome with direct object-form toThrow assertions, using
expect.objectContaining to assert name "FabricTaskError" and the expected
malformed_producer_outcome or layer_subject_mismatch code. Keep the existing
malformed usage and providerId mismatch inputs unchanged.
---
Outside diff comments:
In `@src/lab/projection/verification.ts`:
- Around line 188-196: Extract the inline fabricCapability object shape into a
shared named type, then reuse that type both in the public projection options
around subject/routeSupportedClaims and in taskSubjectApplicableToRequirements.
Preserve the existing readonly harnessFeatures, platforms, and
routePreconditions properties so the evaluator can forward the option unchanged.
🪄 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: 9827a712-64c0-4d7a-adce-e4ba2476f946
📒 Files selected for processing (6)
src/lab/fabric/observe.tssrc/lab/fabric/scratch.tssrc/lab/ledger/store.tssrc/lab/paths.tssrc/lab/projection/verification.tstests/lab-fabric-task.test.ts
Limit symlink checks to Lab-owned paths, harden ledger lock recovery, consolidate scratch fd cleanup, and compare route subjects via JCS.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@src/lab/fabric/scratch.ts`:
- Around line 284-317: Update createSyntheticScratch so its try block begins
before the root and parent-directory setup, ensuring failures during any
construction phase reach cleanup. In the catch, close trusted when present, then
best-effort remove root with rmSync before rethrowing; preserve cleanup of
partial files such as SYNTHETIC_VALUE_PATH. Add a regression test that forces
failure after root creation and verifies labScratchDir contains no fabric-*
directory.
In `@src/lab/ledger/store.ts`:
- Around line 77-83: Update isLedgerLockStale to recover metadata-free lock
files only when their filesystem modification time exceeds LEDGER_LOCK_STALE_MS,
while treating recent empty locks as active; also remove the lock file when
metadata writing fails in the lock-creation flow. Add focused regression cases
near the expired-lock test in appendLabEventIfAbsent coverage for both stale
empty-lock recovery and protection of current empty locks.
In `@src/lab/paths.ts`:
- Around line 19-25: Update ensureRestrictedDir so labRootBoundary is a required
parameter, then remove the labRootBoundary ?? abs fallback and resolve the
supplied boundary directly. Preserve the existing Windows behavior and
downstream boundary checks.
- Around line 20-23: Update the path creation logic in src/lab/paths.ts to
remove the Windows early return and validate every path component on all
platforms, rejecting non-directories and Windows reparse points rather than
relying only on Stats.isSymbolicLink(). Keep chmodSync restricted to POSIX
systems, and add Windows coverage for the cases represented by the tests near
lines 487, 503, and 519 in lab-fabric-task.test.ts.
In `@src/lab/projection/verification.ts`:
- Around line 87-95: Update the platform applicability logic in the
requirements-checking function by replacing the `platforms.every(...)` condition
with allow-list overlap semantics: return true when the capability supports at
least one required platform, while preserving wildcard handling and the existing
feature/precondition checks. Add a focused regression test beside the existing
applicability test covering requirements `["linux", "darwin"]` with capability
`["linux"]` returning true and `["win32"]` returning false, with no wildcards.
🪄 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: 3010842b-07e8-4d1f-b5b8-90c47f7a6055
📒 Files selected for processing (6)
src/lab/fabric/observe.tssrc/lab/fabric/scratch.tssrc/lab/ledger/store.tssrc/lab/paths.tssrc/lab/projection/verification.tstests/lab-fabric-task.test.ts
- Clean up partial scratch dirs on createSyntheticScratch failure - Age out metadata-free ledger locks via mtime and unlink on write failure - Require lab root boundary for ensureRestrictedDir; validate reparse points on Windows - Use platforms.some for capability allow-list semantics - Add junction/symlink and scratch cleanup regression tests
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lab/ledger/store.ts (1)
97-120: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPropagate lock metadata write failures immediately.
If
writeSyncfails at Line 98, Lines 99-106 remove the lock and rethrow. The outer catch at Line 109 then treats that error as a lock-acquisition collision. It retries until the deadline and finally throws"ledger lock acquisition timed out".Handle
openSynccollision errors separately from failures after exclusive lock creation. After cleanup, rethrow the metadata-write error directly. Add a regression test that forces the metadata write to fail and asserts thatappendLabEventIfAbsentreturns that error without waiting for the lock deadline.🤖 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/store.ts` around lines 97 - 120, Update the lock-acquisition flow around openSync and writeSync so metadata-write failures are distinguished from exclusive-open collisions: after the writeSync cleanup in the inner catch, propagate that original error directly instead of letting the outer catch retry until the deadline. Preserve stale-lock retry behavior only for openSync acquisition collisions, and add a regression test proving appendLabEventIfAbsent returns the forced metadata-write error without waiting for the lock timeout.Source: Path instructions
🤖 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.
Outside diff comments:
In `@src/lab/ledger/store.ts`:
- Around line 97-120: Update the lock-acquisition flow around openSync and
writeSync so metadata-write failures are distinguished from exclusive-open
collisions: after the writeSync cleanup in the inner catch, propagate that
original error directly instead of letting the outer catch retry until the
deadline. Preserve stale-lock retry behavior only for openSync acquisition
collisions, and add a regression test proving appendLabEventIfAbsent returns the
forced metadata-write error without waiting for the lock timeout.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e5c59657-6408-40a3-96ff-b518de2d00d7
📒 Files selected for processing (5)
src/lab/fabric/scratch.tssrc/lab/ledger/store.tssrc/lab/paths.tssrc/lab/projection/verification.tstests/lab-fabric-task.test.ts
Separate openSync collision retries from writeSync metadata failures so appendLabEventIfAbsent surfaces the original error without waiting for the lock timeout. Add regression test.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/lab/ledger/store.ts`:
- Line 113: Update the lock-metadata write in the lock-acquisition flow around
writeSync to verify that the returned byte count equals the metadata buffer
length; throw on a short or zero write so the existing cleanup removes the lock.
Add a focused test in the lab evidence ledger tests that simulates an incomplete
metadata write and verifies appendLabEventIfAbsent fails without leaving the
lock file.
🪄 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: 614ac4ec-7202-4a4f-a76f-62fb2bbc7818
📒 Files selected for processing (2)
src/lab/ledger/store.tstests/lab-evidence-ledger.test.ts
Verify writeSync byte count for lock metadata and throw short_write so cleanup unlinks the lock. Add regression test for zero-byte metadata write.
Bind production task evidence to trusted route execution via buildRouteSubjectV1 and host-issued patch executors. Run patch producers in an isolated Bun child with hard kill on timeout. Separate harness vs trusted persistence, fix verifier infrastructure failure taxonomy, inactiveMs accounting, and privacy canary pattern.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 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 `@src/lab/artifacts/sanitize.ts`:
- Around line 11-12: Derive SECRETISH_GLOBAL from the single-match SECRETISH
pattern instead of duplicating the regular-expression source, while preserving
the global flag and existing non-global SECRETISH behavior used by
assertNoSecretMaterial.
In `@src/lab/fabric/executor.ts`:
- Around line 128-140: Update the deprecated runFabricSyntheticPatchTask
wrapper’s parameter type to use Omit<RunFabricTaskHarnessOptions, "harnessKind">
while retaining producePatch?: never, so harnessKind can be omitted and the
existing "deterministic_correct" fallback remains effective.
In `@src/lab/fabric/observe.ts`:
- Around line 441-454: Remove the exported authority-free persistence path by
making persistFabricOutcome private or requiring a host-branded trusted result
at its boundary. Update persistFabricRunResult to use that trusted path, and
route any required harness persistence through a separate non-production API so
runFabricSyntheticPatchTaskHarness output cannot write production
compatibility.jsonl. Add a regression test covering direct harness output
rejection.
In `@src/lab/fabric/producer-child.ts`:
- Around line 77-80: Replace the interval callback throw in
src/lab/fabric/producer-child.ts:77-80 with a rejecting timeout promise, race it
against mod.execute(input), clear the interval in finally, and return the
structured inactivity_timeout response. Update
tests/lab-fabric-task.test.ts:462-473 to assert result.outcome.failure?.code
equals "inactivity_timeout".
- Around line 51-57: Scope the declarations inside the "mutate_after_delay"
branch by wrapping that case’s statements in a block, including the dynamically
imported filesystem and path symbols. Preserve the existing delay, directory
creation, file write, and correctPatch() return behavior.
In `@src/lab/fabric/producer-isolate.ts`:
- Around line 104-118: The producer isolate cannot transmit reportActivity or
AbortSignal through JSON.stringify. In src/lab/fabric/producer-isolate.ts lines
104-118, either implement newline-delimited activity frames on child stdout and
consume them before the final result, or remove reportActivity and signal from
FabricPatchExecutorInput; in src/lab/fabric/executor.ts lines 204-225, align
lastActivityAt and controller.abort() with that chosen contract, using a real
timeout path or removing the controller; update the inactivity accounting
description in
devlog/_plan/260807_compatibility_lab/007_cl07_task_effectiveness.md lines
260-262 to match the implemented behavior.
- Around line 55-57: Bound stdout accumulation in the child.stdout data handler
using FABRIC_LIMITS.maxAggregateIoBytes, tracking byte length rather than string
characters; once the limit is exceeded, stop accepting further output and
terminate the child process early while preserving the existing result/error
handling path.
- Around line 119-120: Attach an error handler to child.stdin before writing
payload in the producer flow: ignore EPIPE errors and pass all other stream
errors to finish. Keep the existing child.on("error") handling and stdin
write/end sequence unchanged.
- Around line 30-53: Update runIsolatedFabricProducer to enforce
inactivity_timeout in the parent: add an explicit child-to-parent activity
heartbeat, handle heartbeat messages separately from the final result, and reset
a parent-side inactivity timer on each heartbeat. Do not use stdout chunks to
reset inactivity, and ensure the timer rejects with FabricTaskError using the
inactivity_timeout classification when heartbeats stop.
In `@src/lab/fabric/verifier.ts`:
- Around line 99-104: Remove the no-op try/catch wrapper surrounding the
verifier logic in the function containing the FabricTaskError check; delete the
redundant instanceof branch and matching try/catch, then de-indent the body so
errors propagate directly. Leave error classification to the
verifyExactTreeDiffV1 call site in the executor.
In `@tests/helpers/fabric-task-test.ts`:
- Around line 65-91: The fabricRouteBoundPatchExecutor helper currently writes
generated modules into the repository and reuses provider-specific filenames.
Add a temporary-home argument, create the generated executor module beneath that
directory using a unique or isolated path, and update its callers to provide the
test home. Align cleanup with fabricWrongPatchExecutor and
fabricOversizedPatchExecutor so the generated directory is removed during test
teardown.
🪄 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: ba3a6c8a-2143-4ac8-aa4b-7c3b7e5c7fe5
📒 Files selected for processing (14)
devlog/_plan/260807_compatibility_lab/007_cl07_task_effectiveness.mdsrc/lab/artifacts/sanitize.tssrc/lab/fabric/executor.tssrc/lab/fabric/index.tssrc/lab/fabric/observe.tssrc/lab/fabric/producer-child.tssrc/lab/fabric/producer-isolate.tssrc/lab/fabric/types.tssrc/lab/fabric/verifier.tssrc/lib/fabric-task-execution-authority.tssrc/lib/fabric-task-host.tstests/fixtures/fabric-executors/correct-patch.tstests/helpers/fabric-task-test.tstests/lab-fabric-task.test.ts
Seal production task-effectiveness ingestion behind persistFabricRunResult, add parent-owned NDJSON child IPC with bounded protocol output, and move timeout authority to the parent with deterministic classification tests.
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
fabric-core@1.0.0/fabric-core.task.synthetic-patch@1.0.0undersrc/lab/fabric/.b66e33ce7207d91014644d99317e456c992a3418(CL-06 merge feat(lab): CL-06 routing compatibility policy #1394).Task producer contract
Closed
FabricTaskOutcomeV1(schemaVersion 1) with task class, exact nestedRouteSubjectV1,TaskSubjectV1/ subjectId, fixture/verifier/fabric/sandbox digests, timing, limits, usage, normalized outcome, verifier result, typed failure, sanitized artifact digests, optional safe sourceRefs.Forbidden in outcomes/artifacts: user repos, prompts, credentials, host paths, unrestricted logs, raw model transcripts.
TaskSubjectV1 identity
Uses existing Lab
TaskSubjectV1. Material changes to route, fixture, verifier, fabric compatibility version, or sandbox profile produce a distinct subject id.Synthetic-patch V1 scenario
Lab-owned scratch tree with
src/value.txt=before\n; requested finalafter\n. Limits: one file, 64 KiB aggregate I/O, one patch op, 30s total, 5s inactivity, 1 MiB artifacts. No network / user MCP / shell / user repository inside scratch.Deterministic verifier
exact-tree-diff-v1: no-follow walk, UTF-8 path sort, reject traversal/symlinks/special/unexpected files; pass only for sole changebefore\n → after\n.Sandbox design
Ephemeral scratch under Lab paths; direct file write patch apply (no shell); deny-by-default capability probe; cleanup on all exits; frozen
sandboxProfileDigest.Lab ingestion
observationFromFabricOutcome/ idempotentpersistFabricOutcome→ existing JSONL + artifact store + rebuildable SQLite.evidenceLayer: task_effectiveness,executionMode: fabric. Catalog discovers fabric-core via CL-04queryLabCatalog.CL-06 boundary
Routing
requiredSuitesremains protocol/live only.TaskSubjectV1cannot be resolved pre-dispatch; CL-06 semantics unchanged.Validation
bun x tsc --noEmitbun test tests/lab-fabric-task.test.ts(20)bun test tests/lab-read-surfaces.test.ts tests/routing-compatibility.test.tsbun test tests/repo-hygiene.test.tsbun run privacy:scanExplicit non-goals
CL-08 (automation / background execution) is not started.
Test plan
Summary by CodeRabbit
New Features
Bug Fixes
Tests