Skip to content

Project config plane: design pass + phased fix (#298)#301

Closed
polymath-orchestrator wants to merge 1 commit into
mainfrom
agent-orchestrator-208/issue-298-project-config
Closed

Project config plane: design pass + phased fix (#298)#301
polymath-orchestrator wants to merge 1 commit into
mainfrom
agent-orchestrator-208/issue-298-project-config

Conversation

@polymath-orchestrator

@polymath-orchestrator polymath-orchestrator commented Jul 12, 2026

Copy link
Copy Markdown

Design pass for #298 — the consolidated project-config plane ticket.

This PR currently contains the design doc only. Implementation phases land on top of it (see the delivery table in the doc). Opening it early and in draft so the design is reviewable and the work has a durable home — the two prior sessions on this ticket died before producing any artifact.

What the design closes

The ticket left four acceptance criteria carrying unresolved decisions. All four are now settled with evidence from four parallel read-only investigations against the code and the live daemon:

  • AC3 (write semantics) — full-replace guarded by an ETag/If-Match. Merge/PATCH is rejected: ProjectConfig has zero pointer fields, so a typed merge could never clear a scalar (autonomousMerge on but never off); merge would break ops/project-config.mjs apply, our documented wipe-recovery path; and merge doesn't even fix the reported reviewer-truncation symptom, which is frontend-only.
  • AC5 (catalog) — a hybrid, because one harness genuinely cannot enumerate. See the correction below.
  • AC2 (Spawnable()) — the gate as scoped passes all four live projects, so no migration and nothing bricked. Three plausible-looking gates would brick 4/4 (gating reviewers, prime.agent, or workspace=worktree), so the operator's standard reviewer is a default to inject, never a gate.
  • AC8 (worktrees)defer the default flip. See the critical finding below.

Corrections to the ticket

Four of the ticket's claims are wrong in ways that change the work:

  1. codex debug models exists. The ticket asserts "there is no non-interactive codex models list — which is precisely why AO hardcoded a Go switch", and hedges toward a codex app-server JSON-RPC design. In fact codex debug models renders the catalog as JSON, exit 0, in 0.27s. The app-server design is unnecessary.
  2. Empty config is a hard spawn error (ErrMissingHarness), not the "deadlock" the ticket describes. The real deadlock surface is permissions, and it is harness-dependent — codex's empty-permissions default already bypasses; only claude-family harnesses hang.
  3. The nested-worktree teardown leak has not actually occurred in production. It is mechanically certain and lab-proven, but a correct prediction of what flipping the default would cause. The 8 orphaned AO worktrees leaked for ordinary dirty-checkout reasons.
  4. Code refs are under backend/internal/adapters/workspace/gitworktree/…, not backend/internal/gitworktree/….

🔴 The finding that changes the plan

AC8 step 1, applied alone, is actively dangerous. Committing .claude/worktrees/ to the tracked .gitignore blinds AO's dirty-guard: git worktree remove's clean check does not consider ignored files, so isDirty() reports clean, ErrWorkspaceDirty never fires, Destroy returns success, and the agent's nested worktree — including uncommitted work — is silently deleted. Lab-proven.

It is harmless today only because in-place mode makes Destroy a no-op. So the ticket's "obvious safe first step" arms a landmine that step 3 would detonate. Hence: defer the flip.

Relatedly, the teardown fix is not --force. The no-force Destroy is a deliberate, documented safety feature that converts a dirty worktree into ErrWorkspaceDirty so agent work is preserved.

Bug found while investigating (ships in the same PR, per rule 8)

A second lost-update path the ticket doesn't have: observe/scm/observer.go:643-646 refreshes RepoOriginURL via a full-row UpsertProject, rewriting the config column from whatever it read. A config PUT landing in that window is silently reverted, and an HTTP ETag does not protect against it.

Good news that shrinks the work

The operator's mandated "model per-harness, harness per-role" shape is already fully implemented in the backendAgentConfig.ModelByHarness exists, is validated for cross-provider mismatches server-side, and already wins over the scalar Model at spawn. ConfiguredModelPins already enumerates every (harness, model) pair. The operator's table is that model made editable. AC12 is reached by deleting the scalar, not by building a new concept.

Similarly, the reviewer path is manual-UI-only (no lifecycle hook, scheduler, or CLI triggers it), and the review tables have no CHECK constraint on harness — so widening the reviewer vocabulary needs no migration and carries no production traffic.

Refs #298

🤖 Generated with Claude Code

https://claude.ai/code/session_01TZTkC2D2ZgxAEhdzY8Ds57


View with Codesmith Autofix with Codesmith
Need help on this PR? Tag /codesmith with what you need. Autofix is disabled.

Closes the four decisions issue #298 left open (AC2, AC3, AC5, AC8) with
evidence from four parallel read-only investigations against the code and
the live daemon.

Key outcomes:
- AC3: full-replace + ETag/If-Match. Merge/PATCH is rejected — ProjectConfig
  has zero pointer fields, so a typed merge cannot clear a scalar, and merge
  would break ops/project-config.mjs apply, our wipe-recovery path.
- AC5: `codex debug models` exists, so the adapter catalog is achievable for
  codex and codex-fugu; claude-code has no enumeration path and falls back to
  a config-driven list. The Go switch disappears either way.
- AC2: the Spawnable() gate as scoped passes all four live projects — no
  migration. Gating reviewers/prime/worktree-mode would brick 4/4.
- AC8: defer the workspace-default flip. Committing the ignore rule blinds
  AO's dirty-guard, converting a loud leak into silent deletion of agent work.

Also records four corrections to the ticket's own claims, including that
`codex debug models` exists (the ticket assumed it did not) and that the
teardown leak is a correct prediction rather than an observed failure.

Refs #298

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TZTkC2D2ZgxAEhdzY8Ds57
@polymath-orchestrator

Copy link
Copy Markdown
Author

Closing as an orphan. This PR is docs-only — a single commit adding docs/design/issue-298-project-config-plane.md (459 lines, zero code). It was pushed by one of four claude-code workers that crash-looped on #298 and died; the session that opened it no longer exists.

PR #302 supersedes it. #302 is the live implementation for #298 — 31 files, +1911/-143, 8 test files — and it carries its own design doc (docs/design/298-project-config-plane.md, commit c50c11d10). There is zero code-file overlap between the two branches, so nothing here is lost and nothing was clobbered: #301 touched only its doc, #302 touches the code.

If anything in this branch's design doc is better than #302's, lift it into #302 rather than reviving this PR. Do not merge both — that would land two competing design docs for the same ticket.

Branch agent-orchestrator-208/issue-298-project-config is orphaned and can be deleted. (Note the branch name is derived from the orchestrator's session id, not a worker's — that is itself a bug worth tracking.)

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