Project config write safety and daemon defaults (#298 P1) by polymath-orchestrator · Pull Request #302 · polymath-ventures/agent-orchestrator

polymath-orchestrator · 2026-07-12T17:06:48Z

Summary

Adds daemon-side standard project defaults and a role-scoped spawnable completeness gate so new projects are born runnable across UI, CLI, and raw API creation paths.
Adds project-config ETag/If-Match handling for stale whole-object config writes, including Settings and CLI read-modify-write flows.
Prevents SCM origin backfill from clobbering project config, fixes the ao project get worker-agent read model, and stops Settings saves from killing drifted orchestrators on unrelated edits.

Refs #298

Test plan

npm run ci:backend
npm run format:check
npm --prefix frontend test -- --run CreateProjectAgentSheet ProjectSettingsForm Sidebar
npm --prefix frontend run typecheck

Notes

This is P1 from the accepted Project config plane is unsafe by construction: defaults in the wrong layer, validation that validates nothing, clobbering writes, a fake agent catalog, self-reviewing reviewers, and unsafe worktree defaults (consolidated) #298 design. It intentionally parks for human merge because it touches sensitive backend config/session-adjacent paths.

^{Need help on this PR? Tag /codesmith with what you need. Autofix is disabled.}

polymath-orchestrator · 2026-07-12T17:40:08Z

SDLC marker: independent review requested

Reviewer/model family: Claude (foreground local reviewer)
PR head reviewed: b236163
Scope: re-review after fixes for the first final-review findings on Project config plane is unsafe by construction: defaults in the wrong layer, validation that validates nothing, clobbering writes, a fake agent catalog, self-reviewing reviewers, and unsafe worktree defaults (consolidated) #298.

polymath-orchestrator · 2026-07-12T18:00:43Z

SDLC marker: independent review requested

Reviewer/model family: Claude (foreground local reviewer)
PR head reviewed: b3035b4
Scope: second re-review after fixing the prior NOT CLEAN findings: default worker baseline, scalar/modelByHarness preservation, exact SetConfig replacement semantics, and quoted ETag handling.

polymath-orchestrator · 2026-07-12T18:16:16Z

SDLC marker: independent review requested

Reviewer/model family: Claude (foreground local reviewer)
PR head reviewed: cdafaba
Scope: third re-review after fixing stale harness model copying, workspace pre-validation before filesystem mutation, and stale CLI helper comment.

polymath-orchestrator · 2026-07-12T18:39:28Z

SDLC marker: independent review requested

Reviewer/model family: Claude (foreground local reviewer)
PR head reviewed: c3db26c
Scope: fourth re-review after fixing workspace spawnability pre-validation, RoleOverride WakeBackoff IsZero, strict add --config-json unknown-key handling, and Settings PUT-response cache seeding.

polymath-orchestrator · 2026-07-12T18:57:34Z

SDLC marker: independent review requested

Reviewer/model family: Claude (foreground local reviewer)
PR head reviewed: c1abc3e
Scope: fifth re-review after fixing default model pins shadowing scalar model overrides, permission-mode UI guard/backfill, --clear docs, CLI mirror parity coverage, and spawnability empty-mix hardening.

polymath-orchestrator · 2026-07-12T19:16:32Z

SDLC marker: independent review requested

Reviewer/model family: Claude (foreground local reviewer)
PR head reviewed: 0def3ac
Scope: sixth re-review after fixing duplicate add validation, --clear repo-derived defaults, legacy CLI flag-merge baseline, role-aware Settings permission handling, cross-project notice reset, project-add docs, If-Match list/weak parsing, and default helper map cloning.

polymath-orchestrator · 2026-07-12T19:33:50Z

SDLC marker: independent review requested

Reviewer/model family: Claude (foreground local reviewer)
PR head reviewed: 131ae57
Scope: seventh re-review after narrowing CLI legacy baseline to spawnability-only fields, making ProjectConfig.IsZero content-based, and adding SetConfig/Get ETag invariant coverage.

polymath-orchestrator · 2026-07-12T19:42:54Z

SDLC marker: independent review requested

reviewer_family: claude
head_sha: 775c366
scope: final review re-check after raw Add config validation fix

polymath-orchestrator · 2026-07-12T19:53:15Z

SDLC marker: independent review requested

reviewer_family: claude
head_sha: 1653ca2
scope: final review re-check after workspace parent origin hardening

polymath-orchestrator · 2026-07-12T19:57:16Z

SDLC marker: independent review verdict

reviewer_family: claude
head_sha: 1653ca2
verdict: clean
findings: critical=0 high=0 medium=0 low=0
fixes_required: no
notes: residual non-blocking notes only; local gates passed separately

polymath-orchestrator · 2026-07-12T19:57:22Z

SDLC marker: CI run

head_sha: 1653ca2
commands:

npm run ci:backend
npm --prefix frontend test -- --run CreateProjectAgentSheet ProjectSettingsForm Sidebar
npm --prefix frontend run typecheck
npm run format:check
conclusion: passed
notes: focused go test ./internal/service/project also passed before the full gate

polymath-orchestrator · 2026-07-12T20:03:35Z

SDLC marker: UI visual verification

head_sha: 1653ca2
method: manual Playwright browser run against npm --prefix frontend run dev:web, with API and bridge stubs
flow: New project -> Project -> manual Project path -> Project agents sheet
assertions: agents sheet visible; Worker agent, Orchestrator agent, Permission mode, and Model controls visible
screenshot: frontend/test-results/manual-create-project-agents.png
conclusion: passed
notes: the existing create-project-defaults e2e is stale because it expects the removed client-side bypass/opus prefill and skips the current import-kind chooser; the current PR intentionally leaves these as daemon defaults instead of UI-seeded values.

polymath-orchestrator · 2026-07-12T20:03:49Z

SDLC marker: CI run

head_sha: 1653ca2
remote: GitHub Actions statusCheckRollup
conclusion: passed
checks: CLI E2E changes/container/native ubuntu/native macos/native windows; Frontend test; Go migration-version-guard/build-test/boot-daemon-smoke/lint/api-drift; Prettier format; gitleaks scan
logs: linked from PR #302 checks tab

polymath-orchestrator · 2026-07-12T20:04:10Z

SDLC marker: final-review verdict

head_sha: 1653ca2
verdict: clean
reviewer_family: claude
review_status: final-review=success and review-passed=success will be set on this SHA
human_merge_required: yes
reason: sensitive path touched: backend/internal/daemon/daemon.go
merge_action: parked for human authorization; do not merge autonomously

Records the decisions #298 left open, the evidence behind them, and the four-PR sequence the work lands in. Four investigations re-verified the ticket against code at 7de5373 and the live daemon. Three premises did not survive: - The worktree teardown failure is inverted on a nickified host. .git/info/exclude lives in the git common dir and propagates into linked worktrees, so an agent-created inner worktree is invisible to `git status`. AO's dirty probe reports clean, the un-forced remove succeeds, and the agent's uncommitted work is silently deleted. The ticket describes the leak that happens only on a host without the exclude file. Both branches reproduced. - agent-vault is no longer broken; the bug classes are real but there is no live fire. - ports.AgentModelCatalog is already wired and preferred; `codex app-server` exposes a real `model/list`. AC5's hedge is retired. And a strict Spawnable() gate would have failed all four live projects on prime.agent, locking the config editor fleet-wide. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hc971Csas2GdM7EHdjMhC5

Validate() is a vocabulary check: it verifies the values you DID set are known values and explicitly permits empty, so a config that provably cannot launch a session persisted with a 201. Spawnable() is the completeness gate it is not. The gate is exactly two requirements, both derived from the spawn path: - The role must resolve to a harness (else ErrMissingHarness). A non-empty WorkerMix satisfies this for the worker role. - The role needs a permission mode only when a harness it can resolve to is a Claude-provider harness. The codex adapter maps an empty mode onto an explicit bypass flag; the claude-code adapter emits no --permission-mode flag and defers to ~/.claude/settings.json, where defaultMode is unset. That asymmetry is why the original failure presented as a hang rather than an error. ValidateSpawnable() is role-scoped: worker and orchestrator always, prime only when the operator has actually configured it. Every live project carries an empty prime role, so a blanket gate would reject all four configs on write and lock the config editor fleet-wide. Also closes three validation gaps that persisted cleanly and failed far away: - defaultBranch is checked against git-check-ref-format, so "mian" is caught at save rather than failing every later spawn at workspace creation. - env keys must be legal POSIX names; an illegal key was forwarded into session runtimes and silently dropped. - projectPrefix is capped at 12 runes. Session display names are capped at 20, so an unbounded prefix truncated the issue number out of every worker session name. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hc971Csas2GdM7EHdjMhC5

…able gate (#298) Defaults lived in the React app (NEW_PROJECT_DEFAULTS), so a project created through the UI got a permission mode and a model pin and ran, while one created by `ao project add` or the raw API was born with an empty config and deadlocked at spawn. Add() is the one chokepoint every creation path funnels through, so the standard defaults are applied there and every path now produces the same runnable project. Models are pinned per harness, never as a scalar: each role's model default keys off the harness that role actually resolves to, so a role pointed at a different harness never acquires a foreign model that would be silently dropped at spawn. A worker mix already resolves the harness, so the default does not stamp a Worker.Harness on top of one. SetConfig now runs ValidateSpawnable(). A partial config is refused rather than silently repaired, because the PUT replaces the config wholesale — "partial" really does mean "drop the rest". A non-empty config is never topped up with defaults either, or `ops/project-config.mjs apply` could not converge live config to the committed spec and the drift check would go permanently red. Clearing a config resets it to the standard defaults rather than to nothing: an empty config cannot spawn, so "cleared" must not mean "deadlocked". Also derives env.POLYPOWERS_REPO from the git remote at registration, so a project is born knowing which repo its SDLC skills file and PR against. And fixes the read-model lie: `ao project get` reported Agent from the daemon's config.DefaultAgent constant without ever consulting the config, so every project read back as claude-code — including the codex-worker ones, and including projects whose config the spawn path would refuse outright. It now reports the project's own worker harness, empty when it has none. Two tests changed because they pinned the bugs: one asserted the hardcoded claude-code agent and an empty config on a new project; the other asserted that clearing a config leaves it empty. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hc971Csas2GdM7EHdjMhC5

…298) PUT /projects/{id}/config is a whole-object replace, and the Settings form snapshots the config once at mount and PUTs that snapshot back. So an operator who opens Settings, edits one field, and saves silently reverts every field another writer changed in between — including autonomousMerge, the merge-authority bit. A true PATCH is not expressible against this struct: every field is a value type, so with omitempty "unset" and "explicitly cleared" are byte-identical on the wire for ~20 fields. The existing trackerIntake.enabled guard is the proof — recovering absent-vs-false for ONE field needed a hand-rolled shadow parse of the raw body, and that does not scale. PATCH would also invert the contract ops/project-config.mjs apply depends on: converging live config to the committed spec requires that omitting a field REMOVES it. So: optimistic concurrency. The stored config is a single deterministic JSON blob, so hashing it yields an ETag with no schema change and no new column — which matters, because a new column needs a migration and a migration needs a version number, the one thing concurrent PRs collide on. A read returns configETag; a write echoes it in If-Match; a mismatch is refused with PROJECT_CONFIG_STALE. "*" opts out deliberately, for whole-object writers like the config-as-code restore path whose entire job is overwriting drift. A tokenless write is still accepted, so existing clients are exactly as safe as before. SetConfig's load-compare-write is serialised, or two savers interleave and the later one clobbers anyway. Also fixes a clobber found while building (rule 8): the SCM observer's origin backfill read the whole project row, set RepoOriginURL, and UpsertProject'd it back — rewriting every column including config, so a config save landing in that window was silently reverted. It now writes only the origin URL, through a narrow setter that mirrors the reasoning already documented for `paused`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hc971Csas2GdM7EHdjMhC5

…strators (#298) Three fixes on the Settings/create surface. 1. The clobber (R3). The form snapshotted config at mount and PUT that snapshot back as the whole object, so an operator editing one field silently reverted everything another writer changed meanwhile. It now sends If-Match with the token from the read it was built on, and a refused save reloads the newer config and says so, instead of overwriting it. The form is keyed by that token, so it re-seeds from the reloaded config — otherwise a retry would replay the same stale values and clobber on the second attempt. The notice lives in the parent, which does not remount, or it would be destroyed by the very reload that triggers it. 2. The surprise orchestrator kill (AC10). Replacement fired when the live orchestrator's provider differed from config — a condition independent of what the operator edited. So a project whose orchestrator had drifted for any reason got it torn down mid-work by an unrelated env-var edit, with no confirmation. Replacement now follows what actually changed. 3. NEW_PROJECT_DEFAULTS is deleted (AC1). The baseline belongs to the daemon, which now applies it on every creation path. This also retires the scalar `model: "opus"` the sheet sent unconditionally: on a codex worker that model is cross-provider, so it saved clean, read back as set, and was silently dropped at spawn — a setting that appears set and does nothing. Three tests changed because they pinned the bugs, including one named "restarts when the saved orchestrator agent already differs from the running orchestrator" — that restart IS the defect. It now asserts the orchestrator survives. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hc971Csas2GdM7EHdjMhC5

polymath-orchestrator · 2026-07-13T20:09:46Z

Final review verdict for head 935adafa3a27c4eaf1107e5f23542a35df58523a: clean. Independent reviewer family: claude. Local gates passed: npm run ci:backend (with isolated GolangCI cache), npm run format:check, npm test --prefix frontend, npm run typecheck --prefix frontend, npm run build:web --prefix frontend. Remote checks are green. Visual verification captured via Playwright at /home/orchestrator/agent-orchestrator/issue-298-settings-actual-desktop.png. This PR touches sensitive path backend/internal/daemon/daemon.go, so it is parked for human merge; merge-park=human-required is set on this head.

…ack, #313) Per the operator-approved #313 plan: remove the automatic worker respawn/retry subsystem; keep a clear terminal failure notification and require an explicit operator restart. Removed: - observe/trackerintake: the respawn engine — retryDecision and its retry-cap logic, orphaned-PR branch adoption (canAdoptOpenPR, spawnCfg.Branch, postSpawnIntent/AdoptsOpenPR plumbing), workerRetryExhaustedIntent, workerRetryBlockedIntent. The duplicate-PR guard and intake dedup (seenIssueIDs, live-driver semantics from #181/#230) stay intact; a live non-worker session still cannot silence a dead worker's escalation. - domain/tracker.go: DefaultWorkerRespawnMaxRetries and the WithDefaults/IsEnabled/EffectiveMaxRetries/EffectiveRespawnPolicy machinery plus respawn validation. The respawn config FIELD is retained as a tolerated-deprecated no-op (accepted, round-tripped, ignored) because #302's strict decode on project add/set-config would otherwise 400 every stored config that still carries a respawn stanza (including the committed ops/project-config spec). Test pins the tolerance. - ports/notifications.go: RetryCount/RetryLimit/AdoptsOpenPR intent fields (TerminalFailureReason stays and now rides worker_died_unfinished). - domain/notification.go: the worker_retry_exhausted type (const, Valid, WithInferredSubject) is gone. Reworked — worker_died_unfinished is now the terminal death escalation: - It carries the dead session's terminal failure reason (#318 provenance) and names an orphaned open PR as a fact; the body demands an explicit operator restart, with no respawn/retry language. - It takes worker_retry_exhausted's operator-attention slot: projection type list, attention item metadata, DTO kind enum, notifier MENTION_KINDS, icon and tone maps. It leaves the notifier's INFORMATIONAL stream set so it is never double-posted. - Store dedupe (subject+type+body, survives read) keeps the per-tick re-emission from spamming; a new real-store test pins one notification across polls and no resurrection after acknowledgement. Legacy data degrade (no migration needed): existing unread worker_retry_exhausted rows simply drop out of the type-filtered projection query and become invisible; the DB CHECK constraints in applied migrations keep tolerating the value; the Slack notifier keeps only the reconcile-away migration mapping for legacy needs-response records. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K1BedAdnAVFqVQTBoVyyp5

…nd-trip test; PR-guard comment Review findings on PR #320 (head d5aa978): 1. [MEDIUM] The terminal-death suppression treated ANY historical merged PR as issue completion before considering the latest dead worker, so a reopened issue — or a later worker dying after an earlier worker's PR merged — had its death silenced. dispatchDecision now computes the latest terminated worker first, and inspectHandledPRs counts a merged PR as completion only when it belongs to that worker or postdates its chronology (mergedPRIsCurrent). Tests pin all three shapes: latest worker's own merged PR suppresses; an earlier worker's stale merged PR no longer silences a later death; a posthumous merge of an earlier PR still suppresses. 2. [LOW] The respawn compat test only strict-decoded into the struct. Added TestManager_SetConfigRoundTripsLegacyRespawnStanza: the exact legacy stanza strict-decodes (the #302 API write path), saves through Service.SetConfig into a real sqlite store, reads back, re-encodes byte-identically, and has zero behavioral effect on the effective intake config. 3. (Copilot thread) The Store.ListOpenPRs comment claimed intake skips issues with an open PR "even when the owning session row is gone", overstating seenIssueIDs, which pins only live-driven PRs. Comment now states the real contract: live driver pins; terminated/missing owner routes to the terminal death escalation (#230). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K1BedAdnAVFqVQTBoVyyp5

…y everywhere Operator ruling (2026-07-15): the codex-fugu baseline is fugu; premium tiers (fugu-ultra, fable) are high-cost, special-use, manual-only — never a default, workerMix pin, modelByHarness baseline, or captured spec value. - live ao config fixed first (workerMix fugu-ultra→fugu; deprecated respawn stanza dropped), then all four project specs re-baselined from live via ops/project-config.mjs capture (adds agent-vault spec, absorbs #302's per-harness pin + canonical-name normalization) - knownModelsForHarness: baseline models listed first — fugu for codex-fugu (was fugu-ultra ONLY), gpt-5.5 for codex (was only the legacy *-codex names #302's DefaultCodexModel comment documents as bogus on this account) - routing-label descriptions: agent:fugu says (fugu), agent:codex says (gpt-5.5) - model-tier policy recorded in ops/project-config/README.md - every casual fugu-ultra test fixture neutralized (fugu or an explicitly-fake premium name); catalog test asserts the baseline is listed and truthfully unknown when unprobed Closes #324 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K1BedAdnAVFqVQTBoVyyp5

polymath-orchestrator mentioned this pull request Jul 12, 2026

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

Closed

polymath-orchestrator force-pushed the issue-298/p1-write-safety branch from 0204ace to b236163 Compare July 12, 2026 17:39

polymath-orchestrator force-pushed the issue-298/p1-write-safety branch from b236163 to b3035b4 Compare July 12, 2026 18:00

polymath-orchestrator mentioned this pull request Jul 13, 2026

Project config plane is unsafe by construction: defaults in the wrong layer, validation that validates nothing, clobbering writes, a fake agent catalog, self-reviewing reviewers, and unsafe worktree defaults (consolidated) #298

Closed

nhod mentioned this pull request Jul 13, 2026

Audit: July 8–13 autonomous change review (merged PRs, open PRs, queue) #313

Closed

polymath-orchestrator and others added 11 commits July 13, 2026 19:25

fix(project): finish config write-safety wiring (#298)

55d3530

fix(cli): send config If-Match tokens (#298)

8022bf3

feat(cli): expose project config defaults overrides (#298)

f411b32

fix(cli): remove stale putJSON helper (#298)

801c6af

fix(project): address final review config safety findings (#298)

0ae66dc

fix(project): resolve config review follow-ups (#298)

f0e32ec

polymath-orchestrator added 7 commits July 13, 2026 19:27

fix(project): handle review edge cases (#298)

b97d473

fix(project): close final review edge cases (#298)

61ad2ed

fix(project): preserve explicit config choices (#298)

1f6dbb5

fix(project): settle final review regressions (#298)

184cc6d

fix(project): narrow legacy config baselines (#298)

aadc984

fix(project): validate add config before defaults (#298)

16e185d

fix(project): avoid inherited workspace parent origins (#298)

1932319

polymath-orchestrator force-pushed the issue-298/p1-write-safety branch from 1653ca2 to 1932319 Compare July 13, 2026 19:36

fix(project): address final review config edge cases (#298)

935adaf

polymath-orchestrator force-pushed the issue-298/p1-write-safety branch from 2103d46 to 935adaf Compare July 13, 2026 20:01

polymath-orchestrator added this pull request to the merge queue Jul 14, 2026

Merged via the queue into main with commit 0fb88e1 Jul 14, 2026
16 checks passed

polymath-orchestrator deleted the issue-298/p1-write-safety branch July 14, 2026 06:11

polymath-orchestrator mentioned this pull request Jul 14, 2026

refactor(#313): remove PR-mutation executor, review-state veto, and worker respawn subsystem #320

Merged

This was referenced Jul 15, 2026

Re-baseline project-config spec: post-#302 normalization drift + drop respawn stanza #324

Closed

fix(#324): config spec re-baseline; fugu-ultra manual-only everywhere #326

Merged

nhod-claude mentioned this pull request Jul 18, 2026

Upstream sync: merge AgentWrapper main @41eed0819 (50 commits) #361

Open

Uh oh!

Conversation

polymath-orchestrator commented Jul 12, 2026 • edited by blacksmith-sh Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Notes

Uh oh!

polymath-orchestrator commented Jul 12, 2026

Uh oh!

polymath-orchestrator commented Jul 12, 2026

Uh oh!

polymath-orchestrator commented Jul 12, 2026

Uh oh!

polymath-orchestrator commented Jul 12, 2026

Uh oh!

polymath-orchestrator commented Jul 12, 2026

Uh oh!

polymath-orchestrator commented Jul 12, 2026

Uh oh!

polymath-orchestrator commented Jul 12, 2026

Uh oh!

polymath-orchestrator commented Jul 12, 2026

Uh oh!

polymath-orchestrator commented Jul 12, 2026

Uh oh!

polymath-orchestrator commented Jul 12, 2026

Uh oh!

polymath-orchestrator commented Jul 12, 2026

Uh oh!

polymath-orchestrator commented Jul 12, 2026

Uh oh!

polymath-orchestrator commented Jul 12, 2026

Uh oh!

polymath-orchestrator commented Jul 12, 2026

Uh oh!

polymath-orchestrator commented Jul 13, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

polymath-orchestrator commented Jul 12, 2026 •

edited by blacksmith-sh Bot

Loading