Skip to content

Centralize plugin state and filesystem paths in the SDK - #23

Open
Var1377 wants to merge 107 commits into
mainfrom
codex/consumer-driven-lab
Open

Var1377 wants to merge 107 commits into
mainfrom
codex/consumer-driven-lab

Conversation

@Var1377

@Var1377 Var1377 commented Sep 19, 2026 •

Copy link
Copy Markdown

Summary

  • collapse active plugin code into the single acyclic crate and use the local SDK filesystem API
  • centralize capture, merge, restore, and state mechanics in public SDK surfaces
  • batch subtree lookup, directory traversal, reads, writes, and removals
  • defer repository baseline work until a content operation needs it; keep status and session hooks scan free

Validation

  • Windows: strict clippy, 96 library tests, 7 merge integration tests
  • WSL/Linux: strict clippy, 93 library tests, 7 merge integration tests
  • macOS pending host authentication

This remains a work in progress while the consumer-driven API and cross-platform qualification loop continues.


Summary by cubic

Centralizes plugin state, diffs, and filesystem paths in the local SDK so capture, merge, restore, and rewind share one implementation. The engine and protocol crates are folded into acyclic, qualification runs through the SDK's qualify harness, and the unfinished Safe Mode feature is removed.

Behavior changes

  • Shared capture, merge, diff, and restore operations use the SDK's batched, generation-safe filesystem APIs; merge planning is proportional to changed paths; rewind swaps use the SDK's native exchange journal with a durable rename, a shared operation identity, and isolated recovery for partial staging.
  • Repository baseline and native watcher startup are deferred until first filesystem demand, keeping status and idle startup constant time and session hooks scan free; daemon readiness waits for that first baseline instead of a fixed delay.
  • Forks require a native mount provider and fail closed instead of falling back to copies; Windows uses ProjFS when verified.
  • Stores inside the repository tree and existing dry_run configuration are rejected; explicit forks replace dry_run.
  • On Windows, daemon calls use overlapped named-pipe I/O with a bounded wait instead of blocking indefinitely.

Migration

  • acyclic-fs resolves from the local relative path ../../../sdk/rust/crates/filesystem, and CI and acceptance tests run the SDK's qualify gate from that checkout; check out the SDK repo at ../../../sdk.
  • Stop daemons with active shadow mounts using the old binary before upgrading; a protocol-v2 CLI cannot stop a protocol-v1 daemon.
  • If the old daemon already crashed, unmount the shadow with fusermount3 -u <repo> on Linux or umount -f <repo> on macOS before starting the new binary.

Written for commit b6d25e9. Summary will update on new commits.

Review in cubic

Signed-off-by: Var1377 <vl331@cam.ac.uk>
Signed-off-by: Var1377 <vl331@cam.ac.uk>
Signed-off-by: Var1377 <vl331@cam.ac.uk>
Signed-off-by: Var1377 <vl331@cam.ac.uk>
Signed-off-by: Var1377 <vl331@cam.ac.uk>
Signed-off-by: Var1377 <vl331@cam.ac.uk>
Signed-off-by: Fixture <fixture@example.invalid>
Signed-off-by: Fixture <fixture@example.invalid>
Signed-off-by: Var1377 <vl331@cam.ac.uk>
Signed-off-by: Var1377 <vl331@cam.ac.uk>
Signed-off-by: Var1377 <vl331@cam.ac.uk>
Signed-off-by: Var1377 <vl331@cam.ac.uk>
Signed-off-by: Var1377 <vl331@cam.ac.uk>
Signed-off-by: Var1377 <vl331@cam.ac.uk>
Signed-off-by: Var1377 <vl331@cam.ac.uk>
Signed-off-by: Var1377 <vl331@cam.ac.uk>
Signed-off-by: Var1377 <vl331@cam.ac.uk>
Signed-off-by: Var1377 <vl331@cam.ac.uk>
Signed-off-by: Var1377 <vl331@cam.ac.uk>
Signed-off-by: Var1377 <vl331@cam.ac.uk>
Signed-off-by: Var1377 <vl331@cam.ac.uk>
@Var1377
Var1377 marked this pull request as ready for review September 19, 2026 18:20
Signed-off-by: Var1377 <vl331@cam.ac.uk>
Signed-off-by: Var1377 <vl331@cam.ac.uk>
@greptile-apps

greptile-apps Bot commented Sep 19, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 1/5

This PR is not safe to merge because releases cannot build, timer-only checkpointing is disabled, crashed speculative processes can leak, and the full acceptance runner is broken.

Findings

  1. P1 Release Builds Lose Dependency ▶
  2. P1 Automatic Checkpointing Never Starts ▶
  3. P1 Crashed Model Processes Survive ▶
  4. P2 CI Uses Mutable SDK ▶
Fix with agent prompt
### Issue 1
Cargo.toml:15
`acyclic-fs` now resolves only from `../../../sdk/rust/crates/filesystem`, but the release matrix checks out only this repository before running `cargo build`. Tagged releases will therefore fail dependency resolution before producing binaries or npm artifacts. Please either restore an immutable fetchable dependency or check out the SDK at the required relative path in the release workflow.

### Issue 2
crates/acyclic/src/pipeline.rs:1590-1595
A daemon now starts in `NeedsBaseline`, but the idle checkpoint timer immediately returns in that state. A plain AGENTS.md or shell-only agent can edit files after `acyclic init` without invoking another Acyclic content operation; in that case no watcher or baseline starts, so the edits create no checkpoint or recoverable history. This breaks the documented timer-based safety net for hosts without lifecycle hooks.

### Issue 3
crates/acyclic/src/spec_runner.rs:359-365
The Unix cleanup for speculative model processes is now compiled only in tests and is no longer called during daemon startup. Because these children run in their own process groups, force-killing or crashing the daemon leaves them running-and potentially billing-along with their PID and scratch files. Restarting the daemon cleans database rows but does not terminate these stale processes.

### Issue 4
.github/workflows/ci.yml:29-34
The sole CI gate executes qualification code from the mutable `codex/generation-reader` branch. Updates to that external branch can change which checks this repository runs without a reviewed change here, making CI results non-reproducible. Pin the SDK checkout to an immutable commit and update it deliberately.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Summary

This PR consolidates the engine and protocol into the acyclic crate, migrates filesystem operations to the local SDK, adds deferred baseline initialization and SDK-backed recovery, removes Safe Mode and copy-mode forks, and replaces repository CI with the SDK qualification harness.

Key review findings:

  • The release workflow cannot resolve the new local SDK dependency.
  • Deferred initialization prevents timer-only checkpointing from ever starting.
  • Unix speculative children are no longer reaped after daemon crashes.
  • The acceptance runner still references the deleted Safe Mode suite.
  • CI qualification depends on a mutable external branch.
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Daemon starts] --> B[Pipeline: NeedsBaseline]
    B --> C{Incoming operation}
    C -->|Checkpoint, diff, rewind, fork| D[ensure_ready]
    D --> E[Open watcher and capture baseline]
    E --> F[Ready]
    C -->|Status, session hooks, turn metadata| B
    B --> G[Idle auto-checkpoint tick]
    G -->|Returns immediately| B
    H[Shell-only agent edits repository] --> G
    G --> I[No watcher, baseline, or checkpoint]
Loading

Reviews (1) · Last reviewed commit: "Merge current plugin main"

Comment thread Cargo.toml

[workspace.dependencies]
acyclic-fs = { git = "https://github.com/acyclic-labs/sdk.git", rev = "22b4e752f46d8f6db6e024b3137fe520ef64bcc1", features = ["local", "native-watch", "native-mount"] }
acyclic-fs = { path = "../../../sdk/rust/crates/filesystem", features = ["local", "native-watch", "native-mount"] }

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Release Builds Lose Dependency

acyclic-fs now resolves only from ../../../sdk/rust/crates/filesystem, but the release matrix checks out only this repository before running cargo build. Tagged releases will therefore fail dependency resolution before producing binaries or npm artifacts. Please either restore an immutable fetchable dependency or check out the SDK at the required relative path in the release workflow.

Prompt To Fix With AI
This is a comment left during a code review.
Path: Cargo.toml
Line: 15

Comment:
**Release Builds Lose Dependency**

`acyclic-fs` now resolves only from `../../../sdk/rust/crates/filesystem`, but the release matrix checks out only this repository before running `cargo build`. Tagged releases will therefore fail dependency resolution before producing binaries or npm artifacts. Please either restore an immutable fetchable dependency or check out the SDK at the required relative path in the release workflow.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Comment on lines +1590 to +1595
// An idle daemon must remain O(1) in repository size. The first
// consumer operation that needs authenticated contents performs the
// baseline; a timer alone is not such an operation.
if self.state == State::NeedsBaseline {
return;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Automatic Checkpointing Never Starts

A daemon now starts in NeedsBaseline, but the idle checkpoint timer immediately returns in that state. A plain AGENTS.md or shell-only agent can edit files after acyclic init without invoking another Acyclic content operation; in that case no watcher or baseline starts, so the edits create no checkpoint or recoverable history. This breaks the documented timer-based safety net for hosts without lifecycle hooks.

Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/acyclic/src/pipeline.rs
Line: 1590-1595

Comment:
**Automatic Checkpointing Never Starts**

A daemon now starts in `NeedsBaseline`, but the idle checkpoint timer immediately returns in that state. A plain AGENTS.md or shell-only agent can edit files after `acyclic init` without invoking another Acyclic content operation; in that case no watcher or baseline starts, so the edits create no checkpoint or recoverable history. This breaks the documented timer-based safety net for hosts without lifecycle hooks.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Comment on lines +359 to +365
#[cfg(all(unix, test))]
#[allow(
unsafe_code,
reason = "kill(pid, 0) only tests for the process's existence; the \
killpg that follows is guarded by a command-name match"
)]
pub fn sweep_stale_runs(spec_runs: &Path) {
fn sweep_stale_runs(spec_runs: &Path) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Crashed Model Processes Survive

The Unix cleanup for speculative model processes is now compiled only in tests and is no longer called during daemon startup. Because these children run in their own process groups, force-killing or crashing the daemon leaves them running—and potentially billing—along with their PID and scratch files. Restarting the daemon cleans database rows but does not terminate these stale processes.

Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/acyclic/src/spec_runner.rs
Line: 359-365

Comment:
**Crashed Model Processes Survive**

The Unix cleanup for speculative model processes is now compiled only in tests and is no longer called during daemon startup. Because these children run in their own process groups, force-killing or crashing the daemon leaves them running—and potentially billing—along with their PID and scratch files. Restarting the daemon cleans database rows but does not terminate these stale processes.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Comment thread .github/workflows/ci.yml
Comment on lines +29 to +34
- name: Checkout SDK
uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
with:
repository: acyclic-labs/sdk
ref: codex/generation-reader
path: sdk

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 CI Uses Mutable SDK

The sole CI gate executes qualification code from the mutable codex/generation-reader branch. Updates to that external branch can change which checks this repository runs without a reviewed change here, making CI results non-reproducible. Pin the SDK checkout to an immutable commit and update it deliberately.

Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/workflows/ci.yml
Line: 29-34

Comment:
**CI Uses Mutable SDK**

The sole CI gate executes qualification code from the mutable `codex/generation-reader` branch. Updates to that external branch can change which checks this repository runs without a reviewed change here, making CI results non-reproducible. Pin the SDK checkout to an immutable commit and update it deliberately.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

@greptile-apps

greptile-apps Bot commented Sep 19, 2026

Copy link
Copy Markdown

Comments Outside Diff

These findings sit on lines the diff does not cover, so they could not be posted inline. Each one leaves this list once its file changes.

  • P1 Acceptance Runner Calls Deleted Script tests/acceptance/safe-mode.sh:1 ▶

    This PR deletes safe-mode.sh, but tests/acceptance/run-all.sh still lists it in the unconditional suite. Running the documented full acceptance command will try to execute the missing file, count that as a failure, and exit nonzero on every platform. Remove the deleted suite from the runner as part of removing Safe Mode.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant