feat: project-less scratch sessions#2808
Conversation
AgentWrapper
left a comment
There was a problem hiding this comment.
Review: changes requested
The feature shape is good — reusing the existing gitworktree machinery by threading cfg.RepoPath through is the right call, and the spawn-path test coverage is solid. But there is one blocking lifecycle bug: the per-session repo path is never persisted, so every workspace operation after spawn (kill, shutdown save, restore, even in-spawn rollback) fails to resolve the repo for scratch sessions. Details in the inline comments; summary below.
Blocker
-
Scratch sessions cannot be killed, saved on shutdown, or restored. Spawn passes the bare-repo path to
Workspace.Createviacfg.RepoPath, but nothing persists it.workspaceInfo(rec)(manager.go:2569) buildsports.WorkspaceInfowithoutRepoPath, so the adapter falls back torepoPath(info.ProjectID)and the daemon-wiredprojectRepoResolvererrors withno project registered with id "scratch". Consequences:Killdestroys the runtime, thenworkspace.Destroyerrors, so Kill returns beforeMarkTerminated— a zombie session (dead runtime, non-terminated, un-removable from the dashboard).SaveAndTeardownAll/boot reconcile:StashUncommittederrors, so scratch sessions are never shutdown-saved.RestoreAllbuilds itsWorkspaceConfigwithoutRepoPath, so restore fails.- In-spawn rollback (
destroySpawnWorkspace) fails too, sinceWorkspace.Createreturns info withoutRepoPath(workspace.go:145).
The new manager test only exercises Spawn, which is why this went unnoticed. Persist the repo path (e.g. in
SessionMetadataor the session_worktrees row) and thread it throughworkspaceInfo/restore config — or derive<dataDir>/scratch/<sessionID>for scratch sessions in those paths.
Should fix
- Reserved id not enforced:
validateProjectIDstill acceptsscratch, soao project add --project-id scratchsucceeds; the list then has twoscratchentries and the registered project is unreachable (spawns silently land in the throwaway repo). - List/Get inconsistency:
GET /api/v1/projectsincludesscratchbutGET /api/v1/projects/scratchreturns 404. The CLI already special-cases around this; other consumers (frontend picker) will hit the 404. - Disk leak: nothing ever removes
<dataDir>/scratch/<sessionID>; every throwaway session permanently leaks a bare repo under~/.ao/scratch.
Minor
- The CLI now silently falls back to Scratch where it previously errored with guidance — a user spawning from inside an unregistered repo may expect the agent to work on that repo. A one-line notice would help.
- The hard-coded SHA-1 empty-tree oid breaks under
init.defaultObjectFormat = sha256.
Verified locally: backend builds and the new tests in cli, service/session, service/project, and session_manager all pass.
Add a built-in Scratch pseudo-project so users can spawn agent sessions without registering a repo first. Backend: - Add ProjectKindScratch and ScratchProjectID domain constants. - Session service defaults an empty projectId to scratch and falls back to the daemon's default harness for scratch spawns. - Session manager creates a disposable bare repo under ~/.ao/scratch/<session> per scratch session and passes RepoPath to the gitworktree adapter. - Project service pins a Scratch summary first in List results. - Spawn controller converts empty projectId to scratch. - gitworktree Create now honors cfg.RepoPath for scratch sessions. CLI: - ao spawn falls back to the Scratch pseudo-project when no project context is available. - Scratch spawns with no explicit agent skip local agent preflight, deferring harness resolution to the daemon. - CWD project resolution skips the Scratch pseudo-project. Tests: - Add coverage for scratch spawn in service, session manager, CLI, and project service. - Update existing project list/delete assertions to account for the pinned Scratch entry. - Isolate spawn tests from inherited AO_PROJECT_ID/AO_SESSION_ID. Closes #2803
Address review feedback on #2808 and a foreign-key failure found during end-to-end verification: - Seed the built-in scratch project row via migration 0025 so scratch sessions satisfy the sessions.project_id FK (and every other project-scoped FK). The stored kind stays 'single_repo' (the projects.kind CHECK predates scratch); the service layer presents it as kind 'scratch'. - Derive the throwaway repo path (<dataDir>/scratch/<session>) for scratch sessions on every post-spawn workspace path: Kill, SaveAndTeardownAll, Cleanup, RestoreAll/restoreSessionWorkspace, and spawn rollback. Previously these fell back to the project repo resolver and failed, leaving scratch sessions un-killable, unsavable, and unrestorable. - Remove the throwaway bare repo after a scratch worktree is destroyed (kill, cleanup, spawn rollback) so sessions do not leak disk. - gitworktree Create now returns RepoPath so rollback can resolve the repo. - Reject registering a project with the reserved id 'scratch' and reject removing the built-in Scratch project. - Present GET /api/v1/projects/scratch consistently with the list entry. - Exclude the scratch seed from first-project onboarding telemetry counts. - Derive the empty-tree oid via git mktree instead of hard-coding the sha1 oid, so sha256 repos work. - Print a one-line CLI notice when spawn falls back to the Scratch project. Tests: scratch Kill integration (worktree removed, repo deleted, session terminated), shutdown-save repo-path derivation, restore config derivation, store-level FK regression, Add/Remove reserved-id guards.
5663d66 to
39f15c8
Compare
|
Follow-up to review 4729425711 — all six points addressed in 39f15c8 (rebased onto main; the rebase also resolved the merge conflicts with #2850's --name change):
Additionally, end-to-end verification against the real SQLite store surfaced a bug my fake-store tests had masked: the sessions.project_id FK rejected scratch session inserts. Fixed by seeding the built-in scratch project row via migration 0025 (kind stored as 'single_repo' because the projects.kind CHECK predates scratch; the service layer always presents it as 'scratch'). Store-level regression test included. Backend: go build ./... and go test ./... green after rebase. Live verification on a dev daemon: zero-setup spawn (no --project/--agent) works, GET projects lists Scratch pinned first, GET projects/scratch → 200, kill → freed:true + terminated + repo dir removed. |
CI runners have no global git identity, so the repo fixture's git commit failed. Match the other commit-creating tests by calling configureCommitter.
Implements #2803.
Adds a built-in Scratch pseudo-project so users can spawn agent sessions without registering a repo first.
Backend
CLI
Tests
Verification
Closes #2803