Repository navigation
feat: flow lint, structured findings handoff, and mid-step resume - #53
Merged
Merged
Conversation
One invalid flow no longer breaks every flow. Discovery loads each candidate in isolation, resolves one winner per id in source priority order (invalid overrides still shadow), and loadFlow throws structured invalid_flow / flow_load_failed errors with details. Adds a flow_id_mismatch warning when raw.id differs from the directory. Closes nax-cusl.1, nax-cusl.2
@davidwells/box-logger sniffs process.argv at require time and disables colors on --json; its disabled chalk returns strings from hex(), so the root help box threw 'borderFn is not a function' for every --json command (e.g. nax list --json). Load the CLI modules with --json hidden from argv, matching the existing --verbose workaround.
listFlowCatalog discovers once and returns runnable flows plus invalid summaries. nax list prints a stderr skip line per broken flow, the picker shows invalid flows (selecting one prints diagnostics), and dashboard listWorkflows / MCP workflow_list include invalid entries. Closes nax-cusl.3
Flow-loading mutation routes are served by the Hono API, which already forwards error.details; loadFlow now throws invalid_flow with 422 and diagnostics. The legacy server catch is unreachable for these routes, so it is left unchanged. Closes nax-cusl.4
invalid_flow and flow_load_failed errors now tell agents to fix the listed diagnostics (or syntax) and run nax lint <flow> --json, with diagnostics passed through structured details. Closes nax-cusl.6
New checks: followup_without_input, followup_source_not_agent_step, invalid_default_transport, transport_lineup_conflict (lineups validated with the declared transport), plus warnings empty_prompt_file and unused_prompt_file. Lineup warnings (catalog_passthrough, effort clamping) are carried into flow warnings instead of dropped, and printFlowPlan no longer prints them twice. TRANSPORT_ALIASES moves to core constants as the single source for accepted transports. Closes nax-cusl.7
nax lint [flows...] [--json] [--strict] reports every error and warning per flow with step index, code, message and fix hint, lists shadowed candidates, and exits 1 on errors (or warnings with --strict). JSON goes to stdout only. Closes nax-cusl.12
flowManifest/flowDigest hash normalized steps, defaults and exact prompt bytes. Workflow plans store flowDigest and include it in requestHash. startStoredPlan calls executionBackend.validatePlan after the replay, starting and ambiguous branches and right before claimStart, so a flow edited after planning fails with recoverable flow_changed_since_plan (409) and the plan stays prepared; replays never revalidate. MCP guides agents to re-plan. Closes nax-cusl.9
createRunState stores the digest of the loaded flow as runState.flowDigest. Plan-started runs pass the plan digest through dashboard execution and the engine refuses with flow_changed_since_plan before creating any run state when the loaded flow no longer matches. Closes nax-cusl.10
The dashboard plan service now passes promptBytesByStep (prompt file bytes plus explicit context) so prompt_near_inline_limit and prompt_offload_required warnings fire during workflow_plan. Documented as a lower bound: auto-context and prior-round results are unknown at plan time. Closes nax-cusl.11
The dry-run/run output alert now preserves line breaks, so an invalid_flow error renders one diagnostic and fix hint per line. Adds a Playwright spec that breaks a flow while the dashboard is open and runs it in dashboard:smoke. Closes nax-cusl.5
Synthesize outputs with 3/7/10 consensus findings, round-1 review outputs per provider, a codex output with no structured heading, a hand-broken JSON sample, and a security-audit synthesize sample, copied from this repo's .nax runs with paths, URLs and tokens redacted. Closes nax-fw4s.1
findStructuredBlocks locates '## Structured <Kind>' headings (numbered or not), parses the fenced JSON payload (or first balanced JSON span) and reports parse errors. round-results keeps its numbered-heading prompt-shrinking behavior by calling it with numberedOnly. Closes nax-fw4s.2
normalizeFlow keeps findings: { step, adapter } (null when absent) so it
survives loading and enters the flow manifest digest. The validator
reports invalid_findings_source for unregistered adapters, unknown
steps, and human-review steps. Adapter ids live in the dependency-free
FINDINGS_ADAPTER_IDS constant.
Closes nax-fw4s.3
Maps consensus/contested/merge-dependent findings to a stable shape: 1-based rank for consensus items, titles from agent title or first claim sentence (100 chars, word boundary), severity normalization (words and P-levels, unknown -> info + severityRaw), line ranges, camelCase fields, and validated agents attribution. Deviations become diagnostics, never exceptions. Closes nax-fw4s.4
buildFindings derives schema v1 (stable <runId>/<stepId>/<localId> keys, source, target incl. pullRequest, findings, diagnostics) purely from run state and current attempts only. Fan-out source steps merge per run with encoded instance-prefixed localIds. readFindings never writes; writeFindings persists atomically. TS mirror in contracts/workflow.ts. Closes nax-fw4s.5
completeRun marks a run completed and writes findings.json; every engine/CLI completion path now uses it, and the failure path writes findings for partial results. saveRunState never writes findings, and a findings write error only warns, never changing run status or exit code. Closes nax-fw4s.6
resolvePullRequestTarget requests number and url and returns one pullRequest object; normalizeTarget carries it, and all three resolveTarget paths (GitHub advisory, generic, Netlify verified) plus legacy re-normalization preserve it. Non-PR targets omit it. Closes nax-fw4s.7
The bundled review flow declares findings: { step: synthesize, adapter:
review-consensus }, and the consensus schema gains an agents list naming
the agents whose findings each item merges (same field as the
security-audit synthesis prompt).
Closes nax-fw4s.8
resolveFindingsRun returns an explicit run id, or the newest run that is completed, completed_with_failures, or failed, declares findings, and has a completed source-step run. Independent of handoff-sources, which only offers fully completed runs. Closes nax-fw4s.9
nax handoff [run-id] --findings shows a ranked table (rank, severity, title, location, bucket/status/agents, diagnostics) for the given or latest eligible run; --json prints the findings.json artifact. Read-only. Closes nax-fw4s.10
nax handoff --to github-issues selects findings (--select, --limit, or an interactive picker preselecting top-ranked consensus items), filters by severity/bucket/status, plans issues, and applies them sequentially. Each issue carries a nax-finding marker; a label-scoped issue scan skips findings already filed, so reruns create nothing new. --dry previews, non-interactive apply requires --force, missing labels degrade to a warning, and a mid-list failure stops and reports partial progress. Tests run the real CLI with a fake gh executable on PATH. Closes nax-fw4s.11, nax-fw4s.12
The interactive handoff menu offers 'Create GitHub issues from findings' when the latest source is a workflow with consensus findings, and routes into the github-issues target picker. Closes nax-fw4s.13
The dashboard serves GET /api/runs/:id/findings (computed in memory,
never written). MCP run details now include a findings summary (count,
severity histogram, top ten ranked findings), run_get accepts
view: findings for the full artifact, and a new
nax://scopes/{scope}/runs/{id}/findings resource template exposes it.
Read-only; no mutation tools.
Closes nax-fw4s.14
nax handoff --to beads creates one bead per selected finding via argv-only br create calls: bug for defects else task, priority mapped from severity, nax-finding/severity labels, and external ref nax:<finding key>. Existing refs are skipped, so reruns create nothing. Fails with guidance when br or the .beads workspace is missing. Targets now declare whether they need a repo and their own confirmation text; issue and bead bodies share one finding markdown builder. Closes nax-fw4s.17
nax handoff --to pr-review resolves the PR (run target, --pr, then the open PR for the run branch), inlines findings only on right-side diff lines parsed from the PR file patches, and lists everything else under 'Outside this diff'. Reviews are always event COMMENT. When the PR head moved, commit_id pins the run SHA if it is in the PR history, otherwise the review is summary-only. A nax-review marker refuses reposting the same run unless --force. Closes nax-fw4s.16
Run details show a collapsible Findings table (rank, severity, title, location, agents, copy button, diagnostics) fed by GET /api/runs/:id/findings, which the legacy server now delegates to the Hono API. Renders nothing for flows without a findings declaration. Playwright spec added to dashboard:smoke. Closes nax-fw4s.18
New Hand Off Findings guide (findings declaration, findings.json schema, --findings, github-issues/pr-review/beads targets and idempotency, the dashboard panel and MCP views). Commands, workflow-files and MCP references cover the new flags, findings key, run_get view and resource. Unreleased changelog entry for the findings and lint work. Closes nax-fw4s.15
…-running it Forward execution proceeds past completed_with_failures steps, but resume treated them as unfinished and re-ran (and re-billed) the whole step. A shared stepAllowsContinuation predicate in core/status now drives forward execution, the resume completed-step map and start index, and unfinished-run detection; a partial final step is still resumed because a workflow cannot finish on one. resumeLocalFlow accepts the Agent Runner boundary for tests. Closes nax-2hq7.1, nax-2hq7.2
…ting it executeLocalFlow pushed a second stepState with the same id when a step was re-executed, so first-match readers saw the stale record and the step's artifacts moved to a new NN- directory. startStepState resets an existing record in place (keeping blobRefs for cleanup), so each step id appears once with a stable position. Closes nax-2hq7.3
resubmissionRun creates a fresh attempt (new attemptId, lineage) from a saved run's prompt text (compact when asked), keeps its instance and model settings, and drops every remote handle and delivery ref so the Agent Runner SDK re-chooses inline or blob delivery and uploads under a new key. Expired or cleaned blob refs are never reused. Closes nax-2hq7.12
…ly failures Keeps completed results, polls in-flight runs, resubmits failed/never-sent instances with their saved prompt. Refuses on flow change, ambiguous send, or moved branch head (--force). Failed runs record failureCode; runs that stopped on failed instances are explicitly resumable.
--dry prints the keep/poll/resubmit/skip table; non-TTY needs --force; --force also allows a moved branch or ambiguous resubmission; --include-cancelled resubmits cancelled agents. The TTY resume offer shows the same preview. GitHub runs get an explicit step-level error.
run.lock/owner.json records pid, host, nonce and command. Stale only when the owner ran on this host and its pid is gone; release is owner-checked. Held while a run is tracked (fresh run, resume, retry), released on clear or exit. Contended -> run_locked; --force-unlock takes over. A failed resume records failureCode so it can be resumed again.
… failure Submission-retry test proves the SDK retries only pre-send network failures (backoff, attempt budget) and never retries 401, 5xx or resets after send. Those ambiguous creates were recorded as plain failures, so resume would resubmit blindly. Record the SDK error code and have reconcile stop on create-ambiguous unless --force.
…tting into the step Dashboard and MCP run workflows in-process, so a second execution of the same run in one process now fails with run_locked. A moved branch only blocks resume when new agents would join kept results in the resumed step, so review-gate approvals are not blocked by pushes during review.
GET /api/runs/:id/resume-preview returns the per-instance plan (or why a run cannot resume, including a live run lock). POST /api/runs/:id/resume refuses with 409 before starting anything, then resumes in-process. Eligibility, flow digest and preview move into a shared prepareResume used by the CLI, the TTY offer and the dashboard.
Failed and interrupted runs show the keep/poll/resubmit/skip preview in run details and can be resumed (optionally including cancelled agents). Blocked runs explain why and keep the button disabled.
Resumes a failed or interrupted run through the dashboard resume API and returns the per-agent preview. The dashboard stores the outcome by request_id, so a replay never starts a second resume. Resume refusals (run_locked, flow_changed_since_run, ...) are recoverable MCP errors with guidance.
The GitHub transport comments with a follow-up step's own agents, so the declaration is only ignored on flows pinned to netlify-api. Bundled flows now lint clean under --strict.
Covers nax lint usage and exit codes, discovery shadowing, 422 refusals, and plan digests. A test keeps the table in sync with the codes the validator and lineup resolver can emit.
…econcile Also syncs bead state for the lint, findings and resume epics.
SDK: optional onSubmitCheckpoint hands the exact effective input and sentAt to the caller before every runner/session create (incl. capacity retries); a throwing checkpoint sends nothing. nax saves the checkpoint before sending and keeps the SDK request window on ambiguous creates. Resume reconciles those runs by request marker: one match is adopted and polled, no match stops for sends that may have landed and resubmits otherwise, several matches stop and are listed. SDK version bump and npm publish are a human handoff.
Codex review: capacity and prompt-shrink retries created runners without a saved request, so a crash after the retry create was resubmitted on resume. The poller now saves raw.retrySubmitCheckpoint before each retry create; resume adopts a matching runner and otherwise stops. A reconcile error on a checkpointed submit failure fell through to a blind resubmit; it now stops (auth errors as resume_auth_failure).
Codex review: two processes seeing the same stale lock could each remove it, deleting the other's fresh lock. Removal now requires run.lock.takeover (atomic mkdir) and a re-check that the lock still has the stale owner; mkdir stays the only way to acquire. An 8-process race test asserts one winner.
Codex review: handleRetry, resumeGithubFlow and the start of handleRunEngine could throw after taking the lock and keep the run locked in long-lived processes. They now release it in finally, keeping the run tracked so a process exit still marks it interrupted.
Codex review round 2: a crash between creating run.lock.takeover and writing its owner file left a mutex nothing could clear, not even --force-unlock. A takeover lasts milliseconds, so an ownerless one older than 60s is reclaimed.
… is active Codex review round 2 (also on master): when the merge keeps a newer disk attempt that is still active, the stale writer's finished step status and workflow status no longer overwrite the current ones.
|
NAX review completed with status: success
Final resultsReview · 2026-09-26T05-09-00-077Z-review
Contents
Review
Claude Results
Repository State
Structured Findings[
{
"id": "R1",
"category": "defect",
"severity": "high",
"status": "confirmed",
"file": "src/integrations/netlify/local-runner.js",
"line": 1449,
"claim": "A single transient error while finalizing or auto-retrying one runner stops polling for every other runner in the step.",
"evidence": "Inside the poll loop, only handleFor/getSnapshot are wrapped in try/catch (1404-1432). terminalRun() (getRunner + listSessions, which throws when the current session is missing) and retryFailedRun() (retryClient.retry/followUp + getSession) run with no catch at 1449 and 1462. Any rejection escapes waitForLocalAgentRuns. The other pending runners are left unfinished and are never stopped or marked timeout (the cleanup loop at 1481 is skipped), and the step fails even though its siblings may still succeed remotely.",
"suggested_fix": "Catch errors per run around terminalRun/retryFailedRun. Treat transient errors like a poll failure (report progress and `continue`) with a bounded retry count, and finalize that one run as failed after repeated errors. The retry checkpoint saved through onSubmitCheckpoint already covers a retry request that was sent before the error.",
"confidence": "high"
},
{
"id": "R2",
"category": "defect",
"severity": "medium",
"status": "confirmed",
"file": "src/workflows/engine/local-executor.js",
"line": 1386,
"claim": "resumeLocalFlow reconciles submissions, plans the resume and may call completeRun before it takes the run lock.",
"evidence": "recoverSubmissions (1386), planResume, the `reconciled.stop` throw (1402) and assertBranchUnmoved all run before trackRunState (1405). The plan.complete branch calls completeRun (1396), which writes state with no lock. While another process is live and mid automatic retry (retrySubmitCheckpoint saved), a second `nax run --resume` makes Netlify API reconcile calls and can fail with resume_ambiguous_submission ('rerun with --force') instead of run_locked. That points the user at a duplicate-risk flag while the real owner is still running. The lock still prevents a duplicate submission, because --force then hits run_locked.",
"suggested_fix": "Call trackRunState(runState, { forceUnlock }) first, wrap everything after it in the existing try/finally, and move recovery, planning and the completeRun early return inside the lock.",
"confidence": "high"
},
{
"id": "R3",
"category": "defect",
"severity": "low",
"status": "likely",
"file": "src/storage/local/run-lock.js",
"line": 111,
"claim": "Two processes that both clear a stale or orphaned takeover mutex can each end up holding it, and one can then delete the other's newly acquired run lock.",
"evidence": "acquireTakeover decides a takeover is stale (dead pid or orphaned >60s), then does rmSync + createLockDir with no guard. Process A can create the new takeover between B's staleness read and B's rmSync, so B deletes A's mutex and both proceed. In acquireRunLock, B's `existsSync(lockDir)` check (159) and its rmSync (160) are not atomic. If A's createLockDir (164) lands between them, B removes A's live lock and then creates its own, so both processes believe they are the executor. The window is narrow and needs a crashed takeover, but this is exactly the case the mutex exists to handle.",
"suggested_fix": "Clear a stale takeover by renaming it to a unique tombstone (fs.renameSync to `${takeover}.${nonce}.stale`) and then mkdir, so only one remover succeeds. Or re-read the takeover owner after mkdir and back off if it is not ours. Likewise, rename the run lock aside instead of rmSync, and only when its owner still matches the owner judged stale.",
"confidence": "medium"
},
{
"id": "P1",
"category": "polish",
"severity": "medium",
"status": "confirmed",
"file": "src/storage/local/run-lock.js",
"line": 145,
"claim": "The run_locked message always suggests --force-unlock, but only `nax run --resume` accepts that flag.",
"evidence": "--force-unlock is registered only in addResumeOptions (src/cli/commands/nax.js:253) and passed only by src/cli/commands/resume.js:89. These call sites cannot force-unlock at all: nax retry (src/cli/main.js:2564), the interactive resume prompt (main.js:2848-2850), dashboard/MCP resumeRunById (main.js:2870-2872) and resumeGithubFlow (github-executor.js:509). A lock left behind with no owner.json (a crash between mkdir and the write in createLockDir) is never treated as stale, so on those paths it is a dead end.",
"suggested_fix": "Word the hint per caller (for example, pass a `recoveryHint` option into acquireRunLock), or thread forceUnlock through retry, interactive resume and GitHub resume. Consider applying the ORPHANED_TAKEOVER_MS age rule to a run lock with no owner file.",
"confidence": "high"
},
{
"id": "P2",
"category": "polish",
"severity": "low",
"status": "confirmed",
"file": "src/cli/main.js",
"line": 2862,
"claim": "Dashboard/MCP resume approves the human-review gate and saves state before acquiring the run lock.",
"evidence": "approveHumanReviewGate runs at 2862, before resumeGithubFlow/resumeLocalFlow calls trackRunState. If another process already owns the run, the approval is still persisted and the resume then fails with run_locked.",
"suggested_fix": "Acquire the lock first (or check runLockHolder and refuse early), then approve and resume under it.",
"confidence": "medium"
},
{
"id": "P3",
"category": "polish",
"severity": "low",
"status": "confirmed",
"file": "src/storage/local/run-lock.js",
"line": 173,
"claim": "release() re-implements the owner comparison that sameOwner() already provides.",
"evidence": "Line 173 compares nonce, pid and hostname inline, while sameOwner (116-119) performs the same comparison for takeover.",
"suggested_fix": "Use `if (!current || !sameOwner(current, owner))` so the two identity checks cannot drift apart.",
"confidence": "high"
},
{
"id": "P4",
"category": "polish",
"severity": "low",
"status": "confirmed",
"file": "src/integrations/netlify/local-runner.js",
"line": 1461,
"claim": "A run that is about to be auto-retried is first reported and persisted as a terminal failure.",
"evidence": "onTerminalRun(normalized) persists the failed artifact, emits agentStatus terminal 'failed' and reports a terminal failure before retryFailedRun decides to retry, so the dashboard or CLI briefly shows a failure that then turns into 'retrying'. The new checkpoint logic depends on this ordering, because the saved record is 'failed' plus retrySubmitCheckpoint.",
"suggested_fix": "Decide whether to retry before emitting terminal UI events, or mark that event as non-final (for example `retryPending: true`) so the UI does not flash a failure.",
"confidence": "medium"
}
]Explore / Review SummaryThe project is The most recent six commits harden mid-step resume. They take over stale run locks under a takeover mutex, reclaim orphaned takeovers, release locks on failure (for long-lived dashboard and MCP processes), checkpoint automatic capacity and argument-limit retries so resume can adopt a replacement runner instead of duplicating it, and add a merge guard in Flows traced
Defects (by severity)
Positive observations
Improve (ranked)Quick wins: R2 (reorder so the lock is taken first, a small change), P3 (reuse Robustness: R1 is the most important fix. Also add a test where UX polish: P1 (a Test gaps: no test covers a failing terminalRun or retryFailedRun inside a multi-run step. No test covers two resumes racing against a live owner (the error code the user gets). No test covers concurrent takeovers of an orphaned mutex. Verification note: I could not run the targeted unit tests because dependencies are not installed in this environment ( Items Considered And Rejected
Gemini Results
Repository State
Structured Findings[
{
"id": "R1",
"category": "defect",
"severity": "high",
"status": "confirmed",
"file": "src/storage/local/run-state.js",
"line": 356,
"claim": "Stale writer merge reverts workflow status to running but leaves stale terminal completedAt and failure fields.",
"evidence": "In mergeExistingStateForWrite, when keptActiveNewerAttempt is true and incomingState is settled while existingState is not, incomingState.status is reverted to existingState.status ('running'). However, incomingState.completedAt, failureCode, and failureReason (set by stale completion/failure routines) are not cleared or synced from existingState, leaving an active running workflow record on disk with a terminal completedAt timestamp.",
"suggested_fix": "When reverting incomingState.status because a newer active attempt was kept, also reset or delete incomingState.completedAt, incomingState.failureCode, and incomingState.failureReason (and incomingStep.completedAt for affected steps) if existingState does not have them.",
"confidence": "high"
},
{
"id": "R2",
"category": "defect",
"severity": "medium",
"status": "confirmed",
"file": "src/storage/local/graceful-run-state.js",
"line": 12,
"claim": "In-process runs clobber singleton activeRunState, preventing graceful interruption on process shutdown for concurrently or interleaved runs.",
"evidence": "heldRunLocks tracks locks for multiple in-process runs in a Map, but activeRunState and activeInterruptHandler remain single global references. When multiple runs execute in-process via Dashboard or MCP, a newer run overwrites activeRunState; furthermore, when that newer run completes, clearTrackedRunState resets activeRunState to null. Consequently, on shutdown or exit, earlier in-flight runs in heldRunLocks have their locks released without being marked interrupted on disk.",
"suggested_fix": "Store active tracked runs in a collection or iterate over heldRunLocks values in persistInterruptedState / onAnyExit so every currently held in-flight run is marked interrupted upon process termination.",
"confidence": "high"
},
{
"id": "R3",
"category": "defect",
"severity": "low",
"status": "confirmed",
"file": "src/storage/local/run-lock.js",
"line": 83,
"claim": "describeRunLockOwner formats empty object owners as 'pid undefined on undefined' instead of reporting unreadable ownership.",
"evidence": "runLockHolder returns {} when a lock directory exists but readRunLockOwner yields null or empty data. Because {} is truthy, describeRunLockOwner skips the '!owner' check and interpolates owner.pid and owner.hostname, generating 'pid undefined on undefined' which surfaces in dashboard 409 error messages and UI alerts.",
"suggested_fix": "Update describeRunLockOwner to check if (!owner || (!owner.pid && !owner.hostname)) return 'an unknown owner (owner.json unreadable)'.",
"confidence": "high"
},
{
"id": "R4",
"category": "polish",
"severity": "low",
"status": "confirmed",
"file": "site/content/reference/commands.mdx",
"line": 14,
"claim": "Top-level CLI command 'nax lint' is documented in guides but missing from reference/commands.mdx.",
"evidence": "The 'nax lint' command was added in the CLI suite to validate workflow flows and outputs with diagnostic flags (--json, --strict), and is referenced in custom workflow guides, but has no corresponding section in site/content/reference/commands.mdx.",
"suggested_fix": "Add a dedicated '## nax lint' section to site/content/reference/commands.mdx documenting arguments, flags (--json, --strict), and return codes.",
"confidence": "high"
},
{
"id": "R5",
"category": "polish",
"severity": "low",
"status": "confirmed",
"file": "src/workflows/engine/resume.js",
"line": 442,
"claim": "Ambiguous retry submissions without run.sentAt render as 'sent undefined' in CLI ambiguous resume messages.",
"evidence": "When maybeCreatedSubmission matches an automatic retry due to raw.retrySubmitCheckpoint, run.sentAt may not be populated on the top-level run object, causing ambiguous.push(`${instanceId} (sent ${run.sentAt}${candidates})`) to output '(sent undefined)'.",
"suggested_fix": "Fallback to raw.retrySubmitCheckpoint.sentAt formatted as an ISO timestamp when run.sentAt is falsy.",
"confidence": "high"
}
]ExploreProject Overview
Architecture
Key Execution Flows Traced
Test Coverage Gaps
Conventions
ReviewRecent Changes SummaryCommits in
Defects & Root Causes
Positive Observations
ImproveQuick Wins
Polish Items
Refactoring Opportunities
Items Considered And Rejected
Codex Results
Repository State
Structured Findings[
{"id":"R1","category":"defect","severity":"warning","status":"confirmed","file":"src/storage/local/run-state.js","line":337,"claim":"A stale snapshot can regress the step and workflow status after a newer replacement attempt has already finished.","evidence":"The merge marks a kept replacement only when it is non-terminal; when the newer disk attempt is terminal, lines 353-357 do not preserve the newer step/workflow statuses, so the older writer's failed status is saved beside the newer completed run.","suggested_fix":"Treat any kept different attempt as newer for status reconciliation, and add a regression test where a stale failed a1 snapshot arrives after completed a2.","confidence":"high"},
{"id":"R2","category":"defect","severity":"warning","status":"confirmed","file":"src/storage/local/run-lock.js","line":39,"claim":"A lock whose owner metadata write fails is acquired successfully but cannot later be released or reclaimed automatically.","evidence":"createLockDir intentionally swallows owner.json write failures and returns true, while release at line 173 refuses deletion when readLockOwner returns null; staleness checks also cannot identify an ownerless run lock.","suggested_fix":"Fail acquisition and remove the just-created directory if owner metadata cannot be written, or retain sufficient in-process ownership evidence to release it safely; cover the write-failure path.","confidence":"high"}
]ExploreProject overview and architectureNAX is a CommonJS Node.js CLI for multi-step agent workflows across Netlify Agent Runner and GitHub Actions. Workflow execution is split between transport-specific executors, durable JSON state under Execution flows traced
Findings and coverage gapsThe two defects above sit at concurrency/failure boundaries. Existing tests cover an active replacement resisting a stale terminal writer, but not a completed replacement resisting that writer ( ConventionsThe code consistently uses JSDoc shapes, dependency injection for network-facing tests, durable checkpoints before ambiguous requests, atomic directory locks, and focused ReviewRecent changes summaryThe last five commits hardened resume behavior: checkpointing automatic retries, releasing locks on retry/resume failures, serializing stale-lock takeover, reclaiming crashed takeover mutexes, and preventing an active replacement attempt from being settled by stale state. Defects and root causes
Positive observationsThe takeover mutex rechecks ownership before removal ( ImproveQuick wins
Polish and refactoringCentralize aggregate status reconciliation around attempt lineage rather than separate active/settled predicates ( Performance and designNo material performance bottleneck was found in the reviewed paths. Synchronous filesystem operations are appropriate for short state transitions, but correctness depends on keeping lock acquisition and metadata creation one logical transaction. ValidationThe focused dependency-free lineage and basic lock tests passed. Items Considered And Rejected
Opencode Results
Context reviewed. This is the
What would you like to work on? Cross Review
Claude Results |
|
NAX review completed with status: success
Final resultsReview · 2026-09-26T05-09-15-216Z-review
Contents
Review
Claude Results
Repository State
Structured Findings[
{
"id": "R1",
"category": "defect",
"severity": "high",
"status": "likely",
"file": "src/storage/local/graceful-run-state.js",
"line": 11,
"claim": "SETTLED_RUN_STATUSES leaves out 'completed_with_failures' and 'cancelled', so the exit handler can overwrite those settled statuses with 'interrupted'.",
"evidence": "persistInterruptedState (line 14) only skips completed/failed/awaiting_review. handleRetry (src/cli/main.js:2564) tracks the run and never changes runState.status up front. When a retry of a partial step fails, it throws at main.js:2683. The finally block at main.js:2711 releases the lock but keeps the run tracked on purpose, so onAnyExit -> persistActiveRunState('process-exit') rewrites status 'completed_with_failures' to 'interrupted'. A run that server.js:1037 set to 'cancelled' gets the same treatment.",
"suggested_fix": "Base the check on isTerminalRunStatus(status) || status === 'completed_with_failures' || status === 'awaiting_review' (the same idea as isSettledStatus in run-state.js), and add a test where a retry of a partial step fails and the process then exits.",
"confidence": "high"
},
{
"id": "R2",
"category": "defect",
"severity": "medium",
"status": "likely",
"file": "src/workflows/engine/github-executor.js",
"line": 585,
"claim": "Releasing the lock while keeping the run tracked lets a long-lived process (dashboard/MCP) later write 'interrupted' over a run that another process has since locked and is running.",
"evidence": "releaseTrackedRunLock (graceful-run-state.js:131) drops the lock, but activeRunState still points at the stale snapshot. If another process acquires the lock and resumes, the dashboard's later shutdown/exit calls saveRunState with status 'interrupted'. The merge guard in run-state.js only restores the existing status when the incoming status is *settled*, and 'interrupted' is not settled, so the stale write wins. The same applies to handleRetry at main.js:2711.",
"suggested_fix": "After releasing the lock in the failure path, also clear activeRunState. Alternatively, have persistInterruptedState skip runs whose lock this process no longer holds (heldRunLocks has no entry for runState.dir).",
"confidence": "medium"
},
{
"id": "R3",
"category": "defect",
"severity": "medium",
"status": "confirmed",
"file": "src/workflows/engine/resume.js",
"line": 379,
"claim": "When reconcile finds no replacement for an automatic retry, resume stops with an error saying nax 'never saved a runner id', and --force then resubmits from scratch instead of polling the runner it already has.",
"evidence": "maybeCreatedSubmission returns true whenever raw.retrySubmitCheckpoint exists, even when run.runnerId is set. submission-recovery.js:92 records kind 'none' but never clears the checkpoint, so every resume ends in resume_ambiguous_submission (resume.js:465). That message says no runner id was saved, which is false here. The test at tests/unit/resume-submission-recovery.test.js:276 locks in the stop but not the message or what --force does.",
"suggested_fix": "Give retry checkpoints their own message that names the existing runnerId, and on --force consider polling the original runner (or clearing the checkpoint) instead of a full resubmit.",
"confidence": "medium"
},
{
"id": "R4",
"category": "polish",
"severity": "low",
"status": "confirmed",
"file": "src/storage/local/run-lock.js",
"line": 173,
"claim": "release() repeats the owner-identity comparison inline instead of calling sameOwner().",
"evidence": "sameOwner (line 116) checks nonce/pid/hostname. release checks the same three fields by hand, so the two could drift apart.",
"suggested_fix": "Replace the check with `if (!current || !sameOwner(current, owner))`.",
"confidence": "high"
},
{
"id": "R5",
"category": "polish",
"severity": "low",
"status": "confirmed",
"file": "src/storage/local/run-state.js",
"line": 324,
"claim": "Core merge helpers (mergeExistingStateForWrite, mergeRunDurableFields, matchingExistingRun, hasValue) have no JSDoc, which goes against the AGENTS.md typing rule; check:jsdoc-types does not catch it.",
"evidence": "scripts/check-jsdoc-types.js only requires type info somewhere in each file (typeInfoPattern), not on each function, so a file with one annotated helper passes. graceful-run-state.js (persistInterruptedState, persistActiveRunState, installGracefulRunStateHandlers) has the same gap.",
"suggested_fix": "Add @param/@returns to these helpers. Optionally tighten the checker to flag exported or top-level functions that have no JSDoc block.",
"confidence": "high"
},
{
"id": "R6",
"category": "polish",
"severity": "low",
"status": "confirmed",
"file": "src/storage/local/run-state.js",
"line": 356,
"claim": "When the step/workflow status is restored because a newer attempt is still active, related settle fields (for example completedAt or failureCode) stay as the stale writer set them.",
"evidence": "Only `status` is restored from existingState/existingStep. The other fields the stale writer set pass through, so the record can end up 'running' with a completedAt value.",
"suggested_fix": "Restore or drop the paired settle fields together with status, and add an assertion to run-state-lineage.test.js.",
"confidence": "medium"
}
]Explore (condensed)
Flows traced:
ReviewRecent changes: The last ~10 commits harden the run lock (stale takeover under a mutex, orphaned-mutex reclaim, release on failure), checkpoint automatic retries for resume, and stop a stale state snapshot from settling a step whose newer attempt is still active. Each commit was a response to a Codex review round, and the fixes are narrow and come with tests. Root cause behind R1/R2: Lock ownership and "active run" tracking live in two separate module-level registries ( Positive:
Tests run: Improve (ranked)Defects (fix first)
Polish
Design concern: Move the "settled status" predicate into Items Considered And Rejected
Gemini Results
Repository State
Structured Findings[
{
"id": "R1",
"category": "defect",
"severity": "critical",
"status": "confirmed",
"file": "src/storage/local/run-state.js",
"line": 337,
"claim": "Stale snapshot writer overwrites settled step and workflow status when a newer disk attempt has completed.",
"evidence": "In mergeExistingStateForWrite, line 337 gates setting stepKeptActiveNewerAttempt on !isTerminalRunStatus(merged.status). If the newer attempt on disk has already completed (terminal), this evaluates to false. Lines 353 and 356 then fail to preserve existingStep.status and existingState.status, allowing a delayed snapshot from a superseded attempt to overwrite completed workflow and step statuses with failed.",
"suggested_fix": "Set stepKeptActiveNewerAttempt = true whenever merged.attemptId !== incomingRun.attemptId regardless of whether merged.status is terminal, and in line 357 preserve existingState.status whenever keptActiveNewerAttempt is true.",
"confidence": "high"
},
{
"id": "R2",
"category": "defect",
"severity": "warning",
"status": "confirmed",
"file": "src/cli/main.js",
"line": 2860,
"claim": "Human review gate is approved and persisted to disk before acquiring the run lock in resumeRunById.",
"evidence": "In resumeRunById, approveHumanReviewGate modifies and saves workflow.json to disk before resumeLocalFlow or resumeGithubFlow calls trackRunState to acquire the run lock. If another process already holds run.lock, the disk state is mutated to running/approved before trackRunState throws run_locked.",
"suggested_fix": "Acquire the run lock with trackRunState(runState) at the orchestration boundary before calling approveHumanReviewGate, and ensure releaseTrackedRunLock is invoked in a finally block.",
"confidence": "high"
},
{
"id": "R3",
"category": "defect",
"severity": "warning",
"status": "confirmed",
"file": "src/dashboard/server.js",
"line": 1802,
"claim": "Dashboard and workflow runner record an invalid 'nax resume' CLI command instead of 'nax run --resume'.",
"evidence": "startResumeRun in src/dashboard/server.js:1802 and resumeWorkflow in src/workflows/engine/runner.js:313 record command: ['nax', 'resume', runId, ...]. Because nax has no top-level resume command (only nax run --resume), running the recorded command directly in a terminal fails with commander error unknown command 'resume'.",
"suggested_fix": "Change the command array in src/dashboard/server.js:1802 and src/workflows/engine/runner.js:313 to ['nax', 'run', '--resume', runId, ...].",
"confidence": "high"
},
{
"id": "R4",
"category": "polish",
"severity": "suggestion",
"status": "confirmed",
"file": "site/content/reference/mcp.mdx",
"line": 10,
"claim": "MCP reference documentation states 13 entity-first tools instead of 14 after the addition of run_resume.",
"evidence": "site/content/reference/mcp.mdx:10 states 'NAX exposes 13 entity-first tools', but lines 19-33 list 14 tools including run_resume, and line 177 documents ### run_resume.",
"suggested_fix": "Update the tool count from 13 to 14 in site/content/reference/mcp.mdx:10.",
"confidence": "high"
},
{
"id": "R5",
"category": "polish",
"severity": "suggestion",
"status": "confirmed",
"file": "src/workflows/handoff-targets/pr-review.js",
"line": 125,
"claim": "PR review handoff fails to match diff lines when finding file paths include a leading './'.",
"evidence": "pr-review.js:125 looks up diff lines with diffLines.get(finding.file). GitHub PR file paths never have a leading './', so findings produced with paths like ./src/index.js fail the map lookup and are placed in 'Outside this diff'.",
"suggested_fix": "Normalize finding.file by stripping any leading ./ before diff line lookup, e.g. finding.file.replace(/^\\.\\//, '').",
"confidence": "high"
},
{
"id": "R6",
"category": "polish",
"severity": "suggestion",
"status": "confirmed",
"file": "src/workflows/engine/resume.js",
"line": 445,
"claim": "Ambiguous submission stop message renders 'sent undefined' when reconciling automatic retries.",
"evidence": "src/workflows/engine/resume.js:445 constructs ambiguous.push(`${instanceId} (sent ${run.sentAt}${candidates})`). When maybeCreatedSubmission is true due to raw.retrySubmitCheckpoint, run.sentAt is often undefined on the run record.",
"suggested_fix": "Fall back to raw.retrySubmitCheckpoint?.sentAt || raw.submitCheckpoint?.sentAt or omit the sent clause if absent.",
"confidence": "high"
}
]ExploreProject OverviewNAX (
Architecture
Execution Flows Traced
Test Coverage & Conventions
ReviewRecent Changes SummaryBranch
Defects[Critical]
|
…iled runs PR review: resume approved a review gate, reconciled and planned before taking the run lock, and failed retry/GitHub resume paths released the lock but stayed tracked, so a later exit could write a stale snapshot over state another process owned. Resume now locks first (refusals leave the run untouched) and failed paths save the run as interrupted under the lock, then stop tracking.
…t and nax lint PR review: automatic-retry ambiguity showed 'sent undefined', the run_locked hint suggested --force-unlock on commands without it, the MCP reference said 13 tools (14), and the command reference lacked nax lint.
|
NAX review completed with status: cancelled
Final resultsReview · 2026-09-26T15-59-04-667Z-review
ContentsReview
Claude Results
Repository StateStructured Findings[
{
"id": "R1",
"category": "defect",
"severity": "warning",
"status": "likely",
"file": "src/cli/commands/resume.js",
"line": 55,
"claim": "Every resume entry point loads the run state before it takes the run lock and never reloads it afterwards, so 'take the run lock before resume reads' (05c9ec5) still leaves a stale-snapshot window.",
"evidence": "handleResumeCommand, resumeRunById (src/cli/main.js:2857-2862) and the dashboard all call listRunStates()/durableRunStateForId() first. resumeLocalFlow (src/workflows/engine/local-executor.js:1388) then locks that same in-memory object and runs recoverSubmissions/planResume on it without reading the file again. If another executor saves progress or settles between the load and the lock, resume plans from the old snapshot and can resubmit instances that already ran. The write-merge in run-state.js only fixes this up after the submissions have gone out.",
"suggested_fix": "Right after trackRunState() succeeds in resumeLocalFlow/resumeGithubFlow/resumeRunById, reload state from runState.dir, Object.assign it onto the tracked object, and check the run is still resumable before recovery and planning.",
"confidence": "medium"
},
{
"id": "R2",
"category": "defect",
"severity": "warning",
"status": "confirmed",
"file": "src/storage/local/graceful-run-state.js",
"line": 140,
"claim": "settleTrackedRun marks a failed run 'interrupted' only if it is the most recently tracked run, so the dashboard and MCP, which run several runs in the same process, leave earlier failed runs stuck as 'running'.",
"evidence": "activeRunState is a single global (line 5) but heldRunLocks is a per-directory Map (line 9). If run A starts, then run B starts (activeRunState is now B), and A then throws, settleTrackedRun(A) skips persistInterruptedState because activeRunState !== A. It only releases A's lock, so A's state file still says 'running' with no lock or live process. Shutdown persistence (persistActiveRunState) has the same limit and saves only the last run.",
"suggested_fix": "Store runState/onInterrupt per directory in heldRunLocks, then settle and persist on shutdown by looping over that map instead of the activeRunState singleton.",
"confidence": "high"
},
{
"id": "R3",
"category": "defect",
"severity": "suggestion",
"status": "confirmed",
"file": "src/storage/local/run-lock.js",
"line": 129,
"claim": "When owner.json is missing, runLockHolder returns {}, and the dashboard shows it as 'already being executed by pid undefined on undefined'.",
"evidence": "runLockHolder returns `owner || {}`. Because {} is truthy, describeRunLockOwner skips its `!owner` branch. Running `node -e` with describeRunLockOwner({}) prints 'pid undefined on undefined'. The callers are src/dashboard/server.js:1388 and src/dashboard/storage/local-runs.js:322.",
"suggested_fix": "Have describeRunLockOwner treat an owner without a pid or hostname as unknown, or return null from runLockHolder with a separate 'held' flag.",
"confidence": "high"
},
{
"id": "R4",
"category": "defect",
"severity": "suggestion",
"status": "confirmed",
"file": "src/workflows/engine/resume-preparation.js",
"line": 83,
"claim": "prepareResume now calls the Agent Runner API on every preview. That includes the dashboard's GET /resume-preview, which re-runs whenever the run query is invalidated and runs even while another process holds the run lock.",
"evidence": "recoverSubmissions → reconcileLocalSubmission → client.reconcileCreate/reconcileSession (lists runners/sessions). getResumePreview (src/dashboard/storage/local-runs.js:319) calls prepareResume before it checks runLockHolder. The resumePreview query key sits under run(runId), so each invalidateQueries(run(runId)) refetches it (dashboard-cache.ts:85). The CLI also reconciles twice per resume, once in the preview and once in resumeLocalFlow.",
"suggested_fix": "Check runLockHolder before prepareResume in getResumePreview and short-circuit when the run is locked. Cache reconcile results per checkpoint requestId for a short TTL, or add a `reconcile: false` option for the dashboard's passive previews.",
"confidence": "high"
},
{
"id": "R5",
"category": "defect",
"severity": "suggestion",
"status": "confirmed",
"file": "src/integrations/netlify/local-runner.js",
"line": 947,
"claim": "A session checkpoint with no source handle is reported as reconcile result 'none', even though nothing was checked.",
"evidence": "`if (!sourceHandle) return { kind: 'none' }`. submission-recovery.js:92 then stores submitReconcile {kind:'none'}. Resume still stops as ambiguous, so this is safe, but the stored reconcile table claims nax looked and found no runner. For a retry session with no run.sdkHandle, the sourceRun fallback (submission-recovery.js:67) searches completed steps for the run's own runnerId and can never match.",
"suggested_fix": "Return or record a distinct outcome (for example {kind:'unchecked', reason:'no source handle'}) and handle it like 'error' without the auth check.",
"confidence": "high"
},
{
"id": "R6",
"category": "polish",
"severity": "suggestion",
"status": "confirmed",
"file": "src/workflows/engine/resume.js",
"line": 468,
"claim": "The resume_ambiguous_submission message tells the user to check the Netlify agent runs page even when nax already reconciled and found no matching runner.",
"evidence": "Only reconciled.candidates is added to the message (line 443). A reconcile result of 'none' (which still stops for retry and pending runs, as the test at tests/unit/resume-submission-recovery.test.js:276 expects) is not mentioned. Users can't tell whether the automatic check ran.",
"suggested_fix": "Add per-instance reconcile context, for example '; nax checked: no runner found' or '; check failed: <message>', so users can decide on --force with confidence.",
"confidence": "high"
},
{
"id": "R7",
"category": "polish",
"severity": "suggestion",
"status": "confirmed",
"file": "src/workflows/engine/local-executor.js",
"line": 1410,
"claim": "If resume is refused after recovery has already adopted a runner or recorded none/ambiguous, those recovery results are thrown away.",
"evidence": "recoverSubmissions changes runState in place, but the preflight catch calls clearTrackedRunState without saveRunState. Examples are a moved branch or an ambiguous stop after a match on another instance. The next resume, the dashboard reconcile table and `nax handoff` don't see the adopted runner or the candidate list.",
"suggested_fix": "Save run state inside the preflight catch while the lock is still held, or at least when recovery changed any run.",
"confidence": "medium"
},
{
"id": "R8",
"category": "polish",
"severity": "suggestion",
"status": "confirmed",
"file": "src/storage/local/run-lock.js",
"line": 173,
"claim": "release() repeats the sameOwner() comparison inline, so the two ownership checks can drift apart.",
"evidence": "Line 173 re-implements the nonce/pid/hostname comparison already defined at line 116.",
"suggested_fix": "Use `if (!current || !sameOwner(current, owner))`.",
"confidence": "high"
},
{
"id": "R9",
"category": "polish",
"severity": "suggestion",
"status": "confirmed",
"file": "src/workflows/engine/github-executor.js",
"line": 519,
"claim": "resumeGithubFlow calls clearTrackedRunState on every success path and then settleTrackedRun again in finally, and repeats the 'Resuming/Flow/State/Repair' banner in two branches.",
"evidence": "Lines 526/548/570/582 clear tracking, then the finally block (585) settles again. That second call is a harmless no-op but hides the real contract. The four console.log lines are duplicated at 533-536 and 555-558.",
"suggested_fix": "Let finally be the only settle/clear point and factor the banner into a helper.",
"confidence": "high"
},
{
"id": "X1",
"category": "rejected",
"severity": "suggestion",
"status": "rejected",
"file": "src/storage/local/run-lock.js",
"line": 111,
"claim": "The rmSync+mkdir reclaim of an orphaned takeover mutex is a TOCTOU race.",
"evidence": "This is real but already tracked as open bead nax-v9p9.",
"suggested_fix": "n/a",
"confidence": "high"
}
]Explore (condensed)
ReviewRecent changes: The last ~15 commits add submit checkpoints and lost-response recovery ( Critical: none found. Warnings:
Suggestions: R3 (bad lock-holder text), R4 (network calls inside a GET preview), R5 ( Positive: The lock is still acquired only by mkdir, even during takeover (with a recheck under the mutex). Checkpoints are written before sending. Adopted runners strip stale error fields. Auth failures during reconcile produce a specific Improve (ranked)Quick wins
Robustness Performance Refactoring / design Items Considered And Rejected
Gemini ResultsCodex Results
Repository State
Structured Findings[
{
"id": "R1",
"category": "defect",
"severity": "warning",
"status": "confirmed",
"file": "src/storage/local/run-state.js",
"line": 337,
"claim": "A stale writer can mark a step and workflow failed after a newer replacement attempt has completed.",
"evidence": "Status protection only activates when the retained newer attempt is non-terminal. A reproduction retained completed attempt a2 but persisted both workflowStatus and stepStatus as failed from stale attempt a1.",
"suggested_fix": "Treat any retained different attempt as authoritative and derive or preserve enclosing statuses from the retained current runs; add a completed-replacement regression test.",
"confidence": "high"
},
{
"id": "R2",
"category": "polish",
"severity": "suggestion",
"status": "confirmed",
"file": "README.md",
"line": 225,
"claim": "The installation prerequisites promise Node 18 support although the package requires Node 20 or newer.",
"evidence": "README.md says Node 18+ while package.json:22 declares node >=20 and canonical site documentation also requires Node 20+.",
"suggested_fix": "Update the root README prerequisite to Node 20+ and keep runtime support claims generated or checked against package.json.",
"confidence": "high"
}
]ExploreProject overview
No Architecture and execution flowsThree principal flows were traced:
The separation among core decisions, transports, storage, dashboard services, and presentation is generally strong. Conventions
ReviewRecent changesThe last five commits strengthened:
Defect and root causeR1 originates in the distinction between “newer active attempt” and “newer attempt.” At run-state.js, the enclosing-status guard is enabled only when the retained attempt is non-terminal. Once that newer attempt completes, a stale snapshot can retain its obsolete failed step/workflow statuses while the merge correctly retains the newer completed run. This creates internally contradictory durable state and may make a successful replacement look failed or resumable. Positive observationsThe locking changes acquire ownership before review-gate or resume mutations, use owner nonces when releasing, and clear process-global tracking on failure. Regression coverage for active-attempt races and lock release is focused and readable. ImproveQuick wins
Test coverage gaps
Performance and designNo material performance regression was found. Atomic state writes and bounded dashboard pagination are sensible. The central remaining design concern is that enclosing status is merged independently from the current run records instead of being recalculated from authoritative attempt state. Verification
Items Considered And Rejected
Opencode Results |
|
NAX review completed with status: cancelled
Final resultsReview · 2026-09-26T15-59-04-646Z-review
ContentsReview
Claude Results
Repository State
Structured Findings[
{
"id": "R1",
"category": "defect",
"severity": "warning",
"status": "confirmed",
"file": "src/storage/local/run-lock.js",
"line": 145,
"claim": "The run_locked error always tells the user to run `nax run --resume <id> --force-unlock`, but that command refuses GitHub-transport runs, and `nax retry` and the interactive resume offer have no force-unlock option.",
"evidence": "resume-preparation.js:71-72 throws resume_unsupported_transport for any run that is not netlify-api. resumeGithubFlow (github-executor.js:509), handleRetry (main.js:2564) and resumeRunById (main.js:2862) call trackRunState without forceUnlock. A lock held by another host, or one with an unreadable owner.json, is never treated as stale (run-lock.js:80-82), so for GitHub runs the suggested fix leads nowhere.",
"suggested_fix": "Only print the --force-unlock hint for netlify-api runs, or add a transport-neutral takeover (for example `nax unlock <run-id>`) and point the message at it.",
"confidence": "high"
},
{
"id": "R2",
"category": "defect",
"severity": "warning",
"status": "confirmed",
"file": "src/storage/local/run-lock.js",
"line": 40,
"claim": "If owner.json fails to write, the run lock is still acquired, but release() then refuses to remove it, so the lock is stuck until someone forces it open.",
"evidence": "createLockDir ignores the write error (lines 38-42). release() reads the owner back and returns false when it is null (lines 172-175). Later acquirers read null, isStaleOwner(null) is false, and they get 'unknown owner'. A missing owner file also leaves a crash between mkdir and write unrecoverable. The takeover mutex has an age-based orphan check for this case (line 109); the run lock does not.",
"suggested_fix": "For the run lock, treat a failed owner write as a failed acquire: remove the directory and throw. Otherwise, apply the same age-based orphan rule used for the takeover mutex.",
"confidence": "high"
},
{
"id": "R3",
"category": "defect",
"severity": "warning",
"status": "likely",
"file": "src/storage/local/graceful-run-state.js",
"line": 114,
"claim": "The process tracks many run locks but only one active run state, so when runs execute at the same time in one process, only the last one tracked is saved as interrupted.",
"evidence": "heldRunLocks is a Map (line 9) but activeRunState is a single value that each trackRunState call overwrites (line 114). settleTrackedRun(A) only saves when activeRunState === A (line 140), and the exit handler saves only activeRunState but releases every lock (lines 63-66). The dashboard only blocks duplicates per flow (server.js:1784-1795), so two flows can run at once. The older run's lock is released while its state still says 'running'.",
"suggested_fix": "Keep each run's state and interrupt handler in the heldRunLocks entry, and save or settle each held run on exit and in settleTrackedRun.",
"confidence": "medium"
},
{
"id": "R4",
"category": "polish",
"severity": "suggestion",
"status": "confirmed",
"file": "src/storage/local/run-lock.js",
"line": 156,
"claim": "When two processes race to take over a stale lock, the loser gets an error naming the dead pid as the current owner.",
"evidence": "When acquireTakeover returns null, the code throws locked(current), where current is the stale owner read before the race, not whoever holds the lock now.",
"suggested_fix": "Read the owner again before throwing, or word the message as 'another process is taking over this run'.",
"confidence": "high"
},
{
"id": "R5",
"category": "polish",
"severity": "suggestion",
"status": "confirmed",
"file": "src/dashboard/server.js",
"line": 1387,
"claim": "The dashboard's run-lock check before resuming is advisory, so if a lock is taken between the check and the start, the resume fails after the API has already returned 202.",
"evidence": "runLockHolder is checked, then startResumeRun sets up a live run, and trackRunState in resumeRunById (main.js:2862) acquires the lock later. If another process gets the lock in between, the run fails inside the live run instead of returning 409.",
"suggested_fix": "Acquire the lock (trackRunState) inside the request before returning 202, then hand the held state to the background run.",
"confidence": "medium"
},
{
"id": "R6",
"category": "polish",
"severity": "suggestion",
"status": "confirmed",
"file": "src/storage/local/run-lock.js",
"line": 81,
"claim": "The stale check is by pid only, so if the OS reuses a dead owner's pid, the lock looks live forever.",
"evidence": "processIsAlive(pid) only checks that some process has that pid. The startedAt field is recorded but never compared with the process start time.",
"suggested_fix": "Where the platform allows (for example /proc/<pid>/stat on Linux), compare the process start time with owner.startedAt.",
"confidence": "medium"
},
{
"id": "R7",
"category": "polish",
"severity": "suggestion",
"status": "confirmed",
"file": "src/storage/local/run-lock.js",
"line": 173,
"claim": "release() repeats the owner-identity check inline instead of reusing sameOwner().",
"evidence": "Line 173 re-implements the nonce, pid and hostname comparison that sameOwner (line 116) already does.",
"suggested_fix": "Use `if (!current || !sameOwner(current, owner))`.",
"confidence": "high"
}
]Explore / Review summaryThis branch ( What works well: mkdir as the only way to acquire the lock, and checking the owner again under the takeover mutex, is a sound design. Defects (ranked):
Polish: R4–R7 cover a misleading message when two takeovers race, the dashboard's advisory lock check before 202, pid reuse, and duplicated owner-comparison code. Improve
Verification note: The full test suite couldn't run because dependencies ( Items Considered And Rejected
Gemini ResultsCodex Results
Repository State
Structured Findings[
{
"id": "R1",
"category": "defect",
"severity": "warning",
"status": "confirmed",
"file": "src/cli/main.js",
"line": 3088,
"claim": "Fresh-run failure handling releases the run lock before persisting the failed state.",
"evidence": "The catch block calls clearTrackedRunState at line 3088, then performs cleanup and saveRunState at line 3099; another process can acquire the same run lock and begin recovery during that unlocked interval.",
"suggested_fix": "Persist failure state and terminal artifacts while holding the lock, then release it in a finally block; add a concurrent failure/resume regression test.",
"confidence": "high"
},
{
"id": "R2",
"category": "defect",
"severity": "warning",
"status": "confirmed",
"file": "src/storage/local/run-lock.js",
"line": 152,
"claim": "forceUnlock can remove a lock belonging to a process that is still alive.",
"evidence": "When forceUnlock is true, the live-owner and stale-owner check is bypassed, after which the existing lock is removed; this permits two executors to mutate the same run despite documentation describing takeover only after the owner is gone.",
"suggested_fix": "Refuse takeover of a verifiably live local PID, or introduce a separately named hazardous override with an explicit warning and confirmation.",
"confidence": "high"
},
{
"id": "R3",
"category": "defect",
"severity": "warning",
"status": "confirmed",
"file": "src/storage/local/run-lock.js",
"line": 39,
"claim": "A failed owner.json write still reports successful lock acquisition and can leave an unreleasable lock.",
"evidence": "createLockDir suppresses every metadata-write error and returns true, but release requires matching owner metadata; an absent or unreadable owner therefore makes release return false and leaves run.lock behind.",
"suggested_fix": "Treat owner metadata creation as part of acquisition: remove the newly created directory and rethrow if writing owner.json fails.",
"confidence": "high"
},
{
"id": "R4",
"category": "polish",
"severity": "suggestion",
"status": "confirmed",
"file": "src/cli/main.js",
"line": 2855,
"claim": "Resume, retry, fresh-run orchestration, UI prompting, and presentation remain concentrated in one oversized CLI module.",
"evidence": "The module owns transport execution, lock lifecycle, resume selection, dashboard entry points, formatting, and command handlers, making lifecycle ordering bugs such as R1 harder to detect and test independently.",
"suggested_fix": "Move the shared locked-run lifecycle into a focused orchestration service and keep CLI functions responsible for argument and presentation concerns.",
"confidence": "high"
}
]ExploreProject overviewNAX is a Node.js CLI and local dashboard for orchestrating multi-agent workflows through Netlify Agent Runner or GitHub Actions. It uses CommonJS/JSDoc for the CLI and services, TypeScript/React for dashboard UI and contracts, and Next.js/Nextra for documentation. Key areas:
Execution flows traced
ConventionsThe code generally uses dependency injection at network boundaries, atomic filesystem writes, structured error codes, precise JSDoc shapes, and extensive unit coverage. Recent attempt-lineage safeguards are particularly careful. ReviewRecent changesThe last five commits strengthened:
The changes are focused and accompanied by regression tests. The remaining defects are adjacent lifecycle gaps rather than failures in the principal resume algorithm. Root causes
Positive observationsAttempt lineage prevents stale writers from regressing newer active attempts. Resume takes the run lock before recovery reads and writes. Submission ambiguity is conservatively handled to avoid duplicate paid work. ImproveQuick wins
Refactoring opportunityCentralize “acquire → execute → settle/persist → release” so fresh runs, retries, local resumes, and GitHub resumes share one lifecycle abstraction: src/cli/main.js. Test coverage gapsAdd regression tests for:
Targeted lock and lineage tests produced 15 passes. The test importing the full CLI could not start because checkout dependencies, including Items Considered And Rejected
Opencode Results |
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.
Implements three epics: flow lint + fail-fast (nax-cusl), structured findings + handoff targets (nax-fw4s), and mid-step resume (nax-2hq7). Plans:
docs/ai/plans/flow-lint-and-fail-fast.md,structured-findings-and-handoff-targets.md,mid-step-resume.md,sdk-presend-checkpoint.md.Flow lint + fail-fast
nax lint [flows...] [--json] [--strict]; new checks (follow-up source, transport/lineup conflict, prompt files, findings source); every diagnostic code documented under "Validate your flow".invalid_flow/flow_load_failedreturn 422 with diagnostics in the dashboard and MCP.flow_changed_since_plan.check:flows(nax lint --strict) runs inrelease:verify.Structured findings + handoff
artifacts/findings.json(schema v1) from the review consensus.nax handoff --findings, andnax handoff --to github-issues | pr-review | beads: plan first,--dry, idempotent via markers / external refs.run_getviewfindings, findings resource).Mid-step resume
nax run --resume <run-id> [--dry] [--force] [--include-cancelled] [--force-unlock]: keeps completed agents, polls running ones, resubmits only failed or never-sent agents with their saved prompt. A per-agent preview shows the plan first.flow_changed_since_run,resume_ambiguous_submission,resume_plan_unavailable,resume_auth_failure,branch_moved_since_run,run_locked.step.attempts; costs count every attempt once; lineage-aware state merge.run.lock): one executor per run across processes and in-process (dashboard/MCP).GET /api/runs/:id/resume-preview/POST /api/runs/:id/resume; MCPrun_resume(idempotent byrequest_id).onSubmitCheckpoint(additive): the exact request is saved before each create, so resume adopts a runner whose response was lost instead of duplicating it.Review
Two Codex review rounds via
nax run agent codex; all valid findings fixed (retry checkpoints, reconcile-error stop, atomic lock takeover, lock release on errors, orphaned takeover mutex, stale-snapshot status regression).Verification (clean worktree)
npm run check,npm test(1224 pass),check:cli-help,check:flows,sdk:ci,dashboard:build+ dashboard e2e (23/23),npm pack --dry-run.Follow-ups (human)
nax-agent-runner-sdk0.4.0 (npm run release:minor). Revenue Engine needs no change (hook is optional).