Repository navigation
Conversation
aviggiano
left a comment
aviggiano
left a comment
There was a problem hiding this comment.
Technical verdict: REQUEST_CHANGES.
Reviewed detached head 16f8ce392b117260b0d020a8be29c797b58ecafa against current unstable (81aec3bdff8fa943118d9979a2a68eaa4c678466). Three reproducible findings are attached: the generation token does not bind the child reset closure, a preview can create SQLite WAL/shared-memory sidecars, and an existing compatibility declaration gate fails.
Validation passed: CLI dependency build; runtime, CLI, and EVMBench TypeScript checks; runtime/CLI test compilation; repository lint and changed-file strict lint; changed-file formatting; patch consistency, documentation checks, and git diff --check. The eight new reset integrations, three runtime audit contracts, three existing targeted-reset regressions, three CLI lifecycle/preview cases, native Bun detached reset continuation, and compatibility patch rewrite/anchor tests passed. Independent probes also verified persisted attempt ties/latest-attempt/later-iteration behavior, ambiguous-iteration rejection, receipt reuse without a database/controller, generation mismatch rejection on receipt reuse, legacy receipts, malformed identities, task-content projection, and receipt symlink refusal.
The child-status probes reproduced both acceptance of an old token for a larger closure (3 nodes previewed, 4 reset) and acceptance of a closure change injected after effect processing. A checkpointed WAL-mode database reproduced sidecar creation with unchanged database bytes. The compatibility declaration test fails independently from packages/runtime; its import is valid JavaScript, but the existing gate does not recognize that binding.
Exact-head CI is still running at review time, so it is not green. The draft state and author branch were left unchanged. This is posted as a COMMENT review because the authenticated account owns the PR; the technical verdict remains REQUEST_CHANGES.
| const addGeneration = (id, workflow, allNodes, attempts) => { | ||
| generations.push([id, workflow?.workflowHash ?? null, | ||
| allNodes.map((node) => [node.nodeId, node.iteration ?? 0, node.state, node.lastAttempt ?? null, node.updatedAtMs]) | ||
| .sort((left, right) => compare(JSON.stringify(left), JSON.stringify(right))), | ||
| attempts.map((attempt) => [attempt.nodeId, attempt.iteration ?? 0, attempt.attempt, attempt.state, | ||
| attempt.startedAtMs, attempt.finishedAtMs ?? null]), | ||
| ]); |
There was a problem hiding this comment.
[P1] Bind the generation to the child reset closure
The digest records a child's workflow hash, nodes, and attempts, but omits the run status that resolveExistingChildResetPlans uses to choose between all child nodes and only unfinished child nodes. The canonical reset identities are also absent from the digest. This lets the same generation authorize a different destructive reset.
Reproduced with a recorded parent (target, after) and an owned child containing child-done in finished and child-work in pending: with the child paused, preview returns 3 identities. Change only _smithers_runs.status to continued; preview now returns 4 identities with exactly the same SHA. Calling timeTravel(adapter, { ...opts, expectedGeneration: oldPlan.generation }) succeeds and changes the previously finished child to pending. Injecting the same status change through hooks.afterEffectReverts also passes the transaction guard and commits the cached pre-change plan.
Hash the canonical selected closure and the state/lineage used to choose it, or compare the canonical plan under the transaction as well. Both a status change before execution and one after initial resolution must reject the old generation before resetting durable state.
|
|
||
| export async function ultrafuzzPreviewReset(databasePath, opts) { | ||
| const { Database } = await import("bun:sqlite"); | ||
| const db = new Database(databasePath, { readonly: true, strict: true }); |
There was a problem hiding this comment.
[P2] Prevent preview from creating SQLite sidecars
readonly: true prevents writes to the main database, but SQLite may still create WAL/shared-memory files. On Bun 1.3.14, checkpoint a valid WAL-mode Smithers database, copy only smithers.db into an empty directory, and call ultrafuzzPreviewReset(copyPath, opts). The call succeeds and leaves a new 0-byte smithers.db-wal and a new 32,768-byte smithers.db-shm; the main database hash is unchanged. This is a normal offline/checkpointed database state, and the documented dry run has now changed the project file set.
Use a read path that preserves the source database/WAL state, including absent sidecars, and add this case to the read-only regression test. The existing fixture already has sidecars and explicitly skips SHM, so it does not exercise this failure. Any snapshot approach must still include committed WAL data and preserve one coherent generation.
| const run = await adapter.getRun(c.options.runId); | ||
| if (!c.options.stealOwnership) {`, | ||
| patched: ` if (c.options.dryRun) { | ||
| const { ultrafuzzPreviewReset } = await import("@smthrs/time-travel/timetravel"); |
There was a problem hiding this comment.
[P2] Restore the compatibility helper-declaration test
This destructured dynamic import is valid, but the existing compatibility patches declare every ultrafuzz helper inside the module they patch test recognizes only directly named declarations. It now reports @smthrs/cli/src/index.js uses ultrafuzz helpers its patched text never declares with ['ultrafuzzPreviewReset'] and fails the runtime suite.
Reproduction after building and compiling runtime tests:
cd packages/runtime
node --test --test-name-pattern='compatibility patches declare every ultrafuzz helper' dist-test/test/runtime.test.jsUpdate the declaration check to recognize this binding without weakening its cross-module safety check, or express the import using a form the gate already understands. Re-run the existing gate as part of the PR validation.
|
Addressed the independent review in
Validation: all 11 reset integrations, 3 compatibility gates, 3 audit contracts, 3 existing targeted-reset regressions, 3 CLI lifecycle/preview cases, and the native Bun detached reset continuation passed. CLI dependency build; runtime/CLI/EVMBench typechecks; runtime/CLI test compilation; repository lint and strict changed-line lint; formatting; generated patch consistency; docs checks; and Both reviewer probe scripts were replayed against this worktree. The independent probe required harness-only normalization of expected generation-rejection exceptions and restoration of the child fixture's running status before its separate parent-ordering assertions (the original sequence relied on the buggy successful reset doing that). All probe findings are now clear. Keeping this PR draft for independent re-review. Exact-head CI is pending. |
|
Fixed the remaining reset-command assertion drift in Reproduced the reported lifecycle assertion on the exact CI Node version (24.21.0), and reproduced the same stale argument order in the canonical failed-node reset test. This follow-up changes only those two regex assertions in All 19 focused tests passed on Node 24.21.0: both corrected lifecycle/reset cases, three compatibility gates, eleven reset-preview integrations, and three audit contracts. Runtime test compilation/typecheck, repository lint, strict changed-line lint, formatting, patch consistency, and New-head CI run is pending. The PR remains draft. |
There was a problem hiding this comment.
Technical verdict: REQUEST_CHANGES.
Independently reviewed exact detached head b2d2a4f348e29b180e560a6fac7b4a247c4c82aa against unstable at 81aec3bdff8fa943118d9979a2a68eaa4c678466, including the complete 18-file diff and the earlier review.
The three prior findings are resolved: the generation binds the selected closure and relevant child status/lineage; preview preserves source DB/WAL/SHM state and absent sidecars; and the unchanged helper-declaration compatibility gate passes. The latest commit changes only the two lifecycle argv assertions, and both pass on Node 24.21.0.
One new P2 finding is attached: the snapshot adapter and the native adapter enumerate child runs differently. With a stable parent containing 1,001 explicitly named subflows, preview reports 1,002 identities while native execution resolves 1,000, so the preview generation is rejected indefinitely. I also reproduced the rejection through ordinary targeted resume without an explicit resetGeneration. This prevents recovery for the affected lineage; it is not a concurrent database race.
Validation passed:
- Node 24.21.0: eight focused runtime tests (both corrected lifecycle assertions, three compatibility gates, and three existing targeted-reset regressions), all eleven reset-preview integrations, three runtime audit contracts, and two CLI lifecycle/preview tests: 24 passed.
- Bun 1.3.14: native detached reset continuation passed.
- Replayed independent probes for stale child-closure tokens, transaction races with and without an explicit token, attempt ordering and iteration selection, receipt reuse/legacy compatibility, malformed identities and content projection, absent/symlinked sidecars, rollback journals, committed/uncommitted WAL frames, and copy-race cleanup.
- CLI dependency build; runtime/CLI/EVMBench typechecks; runtime/CLI test compilation; repository lint and strict changed-code lint; changed-file formatting; generated patch consistency; docs checks; and
git diff --check.
At submission, exact-head CI has passing build gates, external static analysis, Semgrep, and Socket checks; release-validation lanes are still running or queued. Overall CI is not yet green.
Posted as COMMENT because the authenticated account owns the PR; the technical verdict is REQUEST_CHANGES. The author branch and draft state were left unchanged.
| getRun: async (id) => rows("SELECT * FROM _smithers_runs WHERE run_id = ?", [id])[0], | ||
| listNodes: async (id) => rows("SELECT * FROM _smithers_nodes WHERE run_id = ? ORDER BY rowid ASC", [id]), | ||
| listAttemptsForRun: async (id) => rows("SELECT * FROM _smithers_attempts WHERE run_id = ? ORDER BY started_at_ms ASC, node_id ASC, iteration ASC, attempt ASC", [id]), | ||
| listRuns: async (_limit, _status, _workflow, options) => rows("SELECT * FROM _smithers_runs WHERE parent_run_id = ?", [options.parentRunId]), |
There was a problem hiding this comment.
[P2] Match native child enumeration when resolving the preview
This adapter exposes only listRuns, so listOwnedChildRuns takes a different path than it does with the real SmithersDb. This query returns every child, whereas native resolution calls listRunDescendants(runId), whose default limit is 1,000 rows including the parent. Native discovery also has branch exclusions that this query does not apply.
Reproduced on this exact head with an unchanged, completed parent containing one target node and 1,001 finished explicitly named subflows (parent_run_id = parent, config_json.subflowWorkspaceParentRunId = parent, one recorded node/attempt each). ultrafuzzPreviewReset returns 1,002 identities, but ultrafuzzResolveResetPlan(new SmithersDb(...), opts) returns 1,000. Repeated previews are identical, yet timeTravel(..., { expectedGeneration: preview.generation }) always rejects with “generation changed”. runSmithersLifecycleCommand({ action: "resume", resetNode: "target", ... }) also rejects without any explicit generation option, because targeted resume now always passes its preview token.
Use the same child-discovery semantics on the snapshot and native paths, and add a regression that compares their plans on a lineage exceeding the native descendant limit. Otherwise the advertised preview cannot be executed, and an ordinary targeted reset that previously proceeded is blocked permanently for that stable run.
|
Addressed the child-enumeration parity review in Preview now uses the native Added production regressions for ordinary targeted resume with 1,001 owned children (without an explicit generation option), and for fork/continue-as-new parity. The large regression checks the identical preview/execution plan and verifies every child node and attempt was reset. Replaying the review fixture with success assertions now reports 1,002 identities on both paths, a successful ordinary resume, and all 1,001 children reset. Validation passed: 26 Node 24.21.0 tests (13 reset integrations, 8 focused runtime/compatibility cases, 3 audit contracts, 2 CLI cases), native Bun 1.3.14 detached reset continuation, and the prior independent/receipt/snapshot reviewer probes. CLI dependency build; runtime/CLI/EVMBench typechecks; runtime/CLI test compilation; repository lint and strict changed-line lint; formatting; patch consistency; docs checks; and The PR remains draft for independent re-review. New-head CI is pending. |
aviggiano
left a comment
aviggiano
left a comment
There was a problem hiding this comment.
Technical verdict: APPROVE.
Independently re-reviewed exact detached head 8dd41a6233b0cdfc746cb8149112148e8e9657e9 against unstable at 81aec3bdff8fa943118d9979a2a68eaa4c678466, including the complete 18-file diff, four-commit history, and earlier review findings. No remaining actionable finding.
The prior child-enumeration P2 is resolved. An independent synthetic fixture with one parent target and 1,001 explicitly named owned subflows now returns identical preview/native plans and generations containing 1,002 identities. Ordinary targeted resume without an explicit resetGeneration succeeds, clears its receipt, and leaves all 1,001 child nodes pending and child attempts cancelled. A separate native execution confirms that explicit and unlabelled forks, including a newer fork sibling, remain untouched while the owned child and its continue-as-new successor reset.
The earlier fixes remain effective: the generation binds the selected closure and relevant child status/lineage; stale tokens and transaction races are rejected with and without an explicit token; snapshot reads preserve source database/WAL/SHM state and absent sidecars. Independent probes also passed for active rollback journals, committed versus uncommitted WAL frames, symlinked sidecars, bounded copy-race cleanup, persisted attempt ordering/iteration selection, committed and legacy receipts, malformed identities, and task-content projection.
Validation on this head:
- Node 24.21.0: 13 reset-preview integrations, three audit-contract tests, eight focused runtime tests (both corrected lifecycle argv assertions, three compatibility gates, and three existing reset regressions), and two CLI lifecycle/preview tests: 26 passed, zero failures or skips.
- Bun 1.3.14: native detached reset continuation passed. Independent child-parity, generation, snapshot, and receipt probes passed.
- CLI dependency build; runtime/CLI/EVMBench typechecks; runtime/CLI test compilation; repository lint and strict changed-code lint; changed-file formatting; generated patch consistency; docs checks; and
git diff --checkpassed.
At submission, exact-head CI has passing build gates, external static analysis, package/dependency/policy gates, runtime support/Bun adapter contracts, the controller-kill/resume end-to-end lane, workspace typecheck/history checks, Semgrep, and Socket checks. The four runtime integration shards and CLI lane are still running, so overall CI is not yet green.
Posted as COMMENT because the authenticated account owns the PR; the technical verdict is APPROVE. The author branch and draft state were left unchanged.
|
Pushed bff9ae0 to fix the stale doctor assertion from the CLI CI gate. This changes only The old regex required Validation:
Fresh exact-head CI: https://github.com/monad-developers/ultrafuzz/actions/runs/38777950148 (queued). PR remains draft. |
aviggiano
left a comment
aviggiano
left a comment
There was a problem hiding this comment.
Technical verdict: APPROVE.
Narrow re-review of exact head bff9ae00899b4cb89304b7fbc65926c2b100f3b1 against previously approved 8dd41a6233b0cdfc746cb8149112148e8e9657e9, with unstable still at 81aec3bdff8fa943118d9979a2a68eaa4c678466. The sole incremental change is the doctor assertion in packages/cli/test/lifecycle-commands.test.ts (22 additions, 7 deletions). All previously approved production files, compatibility patches, schemas, and dependency configuration are identical. The prior production approval stands.
The previous exact-head CLI job failed only the doctor test: its regex required local delegation first, although the output correctly listed the reset-plan entries before it. The replacement parses the compatibility-patches line and requires exact name applied entries. It preserves all five prior patch names and adds all nine reset-plan names. Independent execution of the assertion block extracted from this commit accepted 16 reordered/extra-entry cases and rejected 70 per-entry deletion, status, and near-match mutations, plus missing-line and off-line-name controls.
Independently reran the focused doctor test on Node 24.21.0 with Bun 1.3.14, the CI environment flags, eatmydata, and --test-concurrency=1: 1 passed, zero failures or skips. Inspected the local exact CLI release-gate evidence and validated its report against the production schema: overall pass; required cli gate; pnpm --filter @ultrafuzz/cli test; exit code 0; duration 1,717,397 ms. The workflow's CLI release lane invokes this same gate. git diff --check also passed.
At submission, exact-head CI has passing build gates, external static analysis, Semgrep, and Socket checks. Release-validation lanes remain running or queued, so overall CI is not yet green.
Posted as a COMMENT review with technical APPROVE. The PR remains open and draft; no branch changes were made.
1 participant
resume --reset-nodepreviously changed workflow state without showing which nodes would reopen. Add--dry-runwith a versioned, identity-onlyreset_plan, and report that same plan after execution. Human output lists every node and the closure size; JSON includes the selected attempt/iteration and an opaque generation hash.Preview reads one read-only SQLite snapshot and returns before lifecycle locking, workflow loading, controller/prompt preparation, or archive work. Execution recomputes the plan; optional
--reset-generationrequires the previewed generation, which is checked again inside the reset transaction. A committed reset receipt retains the plan when controller admission fails, so retry and preview can report the already-applied reset.The pinned runner resolves resets by attempt order, including later attempted work and later iterations. The preview shares that resolver instead of estimating the closure from topology dependencies. The compatibility patches preserve this policy and expose a read-only planning path.
--dry-runand--reset-generationrequire--reset-nodeand reject combination with--retry-failed.Fixes #1307.
Validation:
pnpm --filter @ultrafuzz/cli... build; runtime, CLI, and EVMBench TypeScript checks; compiled runtime/CLI tests.pnpm -w lint; strict lint on changed TypeScript; Prettier andgit diff --check.node scripts/smithers-patches.mjs --checkandnode scripts/docs-check.mjs.Draft for independent review.
The PR appears safe to merge; no actionable issue was established.Summary
Adds targeted reset previews and reports the same versioned plan after execution.
Diagram
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Resume with reset node] --> B{Saved receipt?} B -->|Yes| C[Use saved plan] B -->|No| D[Read private database snapshot] D --> E[Resolve reset plan] C --> F[Check requested generation] E --> F F --> G{Dry run?} G -->|Yes| H[Return plan without launching controller] G -->|No| I[Prepare continuation] I --> J{Reset already applied?} J -->|No| K[Check generation inside reset transaction] K --> L[Apply reset and save receipt] J -->|Yes| M[Launch controller] L --> M M -->|Accepted| N[Clear receipt and return plan] M -->|Failed| O[Keep receipt for retry]Reviews (1) · Last reviewed commit: "test(cli): check doctor patches independ..." · Reviewed by Greptile