Conversation
One-time m228 schema migration: for every workflow node agent slot bound by agentId without a templateKey, synthesize a template from the referenced agent's current config (orphaned refs get generated templates), persist it in space_agent_templates, and write templateKey onto the slot while keeping the agentId fallback data. Executions of migrated slots drop their recorded agent_id so in-flight runs revalidate against the template binding. Resolution follows the data: resolveSlotSpawnConfig resolves templateKey against built-ins plus stored templates and falls back to the kept agentId when the key resolves nowhere; resolveNodeAgentConfig passes modelPool through the template branch so pool scheduling survives the migration. The workflow save validator and import preview accept stored template keys. Refs #3614
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f115bcec4b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
All five Codex findings plus the three CI breaks: - Export carries the kept agentRef alongside templateKey for migrated slots; import preview accepts an unknown template key when the agentRef resolves, and import execute binds the agent fallback (dropping an unresolvable-on-target template key, keeping a known one plus the fallback) so cross-database export/import round-trips again. - The migration test no longer inserts an execution row with a dangling agent_id (FK-enforced impossible state); orphan slots' executions are seeded with NULL. - m228 skips slots bound to non-runnable agents (paused/disabled/ archived, except migrated-worker mirrors), preserving the pre-upgrade spawn rejection instead of silently making them runnable. - deleteTemplate refuses to delete a template referenced by workflow node slots. - Slot template resolution is stored-first, matching SpaceAgentTemplateManager.getByKey/list precedence so a stored record shadowing a built-in key spawns with the settings the UI shows. - validateNodeAgentRef keeps a resolvable agentId fallback on template slots instead of clearing it on every save. - Fix the synthesis test import path (one ../ too deep) and the missing exportedAt in the preview test bundle. Refs #3614
|
@codex review |
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.3-flash (z.ai)
Model: glm-5.3-flash | Client: HyperNeo | Provider: z.ai
Task: #1831 — chore(daemon): migrate workflow agentId refs to templateKey [AT-M1] (#3614)
Diff: +1734/−80 across 17 files — code 489 | tests 1245 | comments 0 | other 0
Review policy: source auto — the external gate is Codex (round-2 review running on this head; Devin dead: trial expired, no credits) — at depth deep (one-time data migration), with an independent second pass on migration correctness. Round 1 was reviewed at f115bcec4b; the head moved mid-review, so every conclusion below was re-derived against 618efa6264 (full delta read; changed seams re-read end to end).
Ask: one-time m228 migration — synthesize a template per workflow-slot agentId (orphans get generated templates), rewrite slot bindings to templateKey, keep agentId as fallback data until the DELETE slice; resolution seams follow the data; characterization pins prove resolution. No UI.
Verified resolved (round-1 findings, re-derived at this head)
- CI red + migration-test FK violation: the fixture no longer inserts a dangling
agent_id(an FK-enforced impossible state), sorunMigration228is actually exercised. - Non-runnable agents becoming runnable: m228 now skips paused/disabled/archived agents (
isRunnableAgentRow, m228:91,128-131), so those slots keep their pre-upgrade spawn rejection; pinned by a new test. - Template deletion hazard:
deleteTemplatenow refuses keys referenced by workflow node slots (space-agent-handlers.ts), and the save validator keeps a resolvableagentIdfallback instead of stripping it (space-workflow-manager.ts:657-661) — the two halves close each other. - Export/import round-trip: migrated slots export
agentRefalongsidetemplateKey(export-format.ts:341-344); preview accepts an unknown key when the agentRef resolves; execute binds the agent fallback (dropping the unknown key) or keeps a known key plus the fallback — three new round-trip tests cover all branches. - Precedence divergence: slot resolution is now stored-first (task-agent-manager.ts:3717-3721), matching
SpaceAgentTemplateManager.getByKey/list.
The in-flight-run rescue itself remains sound at this head: unpinned runs validate against the migrated live head and NULLing node_executions.agent_id for migrated slots correctly skips the permanent mismatch rejection at workflow-node-execution-validation.ts:173; pinned runs validate the pre-migration snapshot and are untouched. m223 plus the runtime worker→mirror mirroring guarantee every live slot agentId exists in space_long_horizon_agents when m228 runs; all agent_id readers are NULL-tolerant and template:<key> ids are never persisted. The equivalence pins cover every spawn-consumed field.
Remaining findings
P2-1 · m228 is not transactional, unlike its sibling rewrites. m223 (m223-unify-space-agents-copy.ts:128-160) and m224 (m224-retarget-node-executions-agent-fk.ts:8-66) run their multi-statement rewrites inside BEGIN/COMMIT with rollback; m228 autocommits per statement and the migration_228 marker is written only after the whole body (migrations.ts:557-568). A mid-run throw leaves the marker unwritten and the next-boot retry re-allocates fresh suffixed keys for slots whose templates were already created — permanently orphaning those templates in the user's template list. Wrap the rewrite like its siblings.
P2-2 · Merge-contract accounting. The task budget is ~200 prod lines; the actual prod diff at this head is +489/−74 across 8 prod files (synthesis 93 + m228 181 alone exceed it), and the PR description still states "Prod ~200 lines", which is now off by ~2.4×. Per ADR 0004 budgets are contracts and overruns are flagged: please correct the description to state the real size. No split demanded — the content is cohesive and in-contract (migration + repo/synthesize + resolution seams, no UI).
External gate (source auto)
- Codex round-1 threads: all five resolved with fixes verified above.
- Codex round-2 on
618efa6264: running (triggered 2026-09-05T01:47:55Z) — no verdict yet; a COMMENTED-with-findings state is not a pass. Devin: dead gate (trial expired, no credits). - CI at review time: on this head everything finished is green (lint/knip, Deno boot, all online suites, 5-space-a, shared); the four decisive unit shards (1-core, 5-space-b, handlers-migrations, storage-migrations) were still pending.
Passing observations
- Orphan slots: generated templates implement the task's "orphans get generated templates", but note the flip — a dangling
agentIdused to fail spawn loudly; post-migration it silently spawns a blank agent (empty instructions/tools). Worth an explicit note in the epic's DELETE slice. spaceExport.bundle's reference check still counts the keptagentIds of template-bound slots; observable behavior matches pre-PR in every scenario, but the DELETE slice must revisit it when the fallback goes away.- The web node picker is built-ins-only (out of this slice's contract): migrated keys render as a disabled raw-key option; saves are not blocked. Flag for the epic's web slices.
- Frozen-snapshot semantics (agent edits no longer propagate to migrated slots) is intended template behavior — release-note it.
- The
migrated.key namespace is unreserved; user templates are indistinguishable from synthesized ones. - The orphan-execution test now seeds
agent_idNULL directly (FK-safe), so it no longer exercises the orphan clearing path itself; the live-agent clearing case is still proven.
Recommendation: REQUEST_CHANGES
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 618efa6264
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- m228 rewrite now runs inside BEGIN/COMMIT/ROLLBACK (matching m223/ m224), so a mid-run failure leaves no orphaned templates and a retry re-allocates cleanly; pinned by a rollback test. - Import binds the exported agent in preference to a stored template key when the slot carries a resolvable agentRef, so a colliding migrated.* key from an unrelated database can no longer shadow the bundled agent's configuration. - The template deletion guard also scans definition versions pinned by executable runs, covering workflows edited after their runs started. - Export prechecks (workflows + bundle) no longer require agents referenced only as the kept fallback of template-bound slots, so orphaned or autonomy-bearing fallbacks cannot abort the export; the exported agentRef companion is only written when the fallback agent is actually in the export set. - Test repairs: distinct node ids in the manager test, version-5 bundles in the import/export handler tests. Refs #3614
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3ea78f2da5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
SpaceAgentRepository.isAgentReferenced now parses candidate node configs and counts only slots that bind the agent outright — a slot whose agentId is merely the kept fallback under a usable templateKey no longer blocks deleting the snapshotted agent. Owning agentId-only slots still block as before. Refs #3614
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c46cf500da
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Synthesis preserves an explicitly empty settingSources list ([] disables all setting sources; collapsing it to null silently re-enabled the space defaults after migration). tools/modelPool keep the empty-to-null normalization — [] and undefined resolve identically there. - Import keeps a known built-in templateKey alongside the resolved agent fallback (built-ins are portable, so the collision hazard does not apply); stored keys remain agent-preferred per round 2. - isAgentReferenced also scans definition versions pinned by executable runs, mirroring the template deletion guard: a run pinned to a pre-migration agent-bound definition keeps its agent undeletable until the run's tasks archive. Refs #3614
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6cad72193
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- The template reference guard moved into the manager's delete pipeline (new deleteCheckReferences stage fed by an injected reference counter in space-agent-template-references.ts); the RPC handler is a thin caller again and every SpaceAgentTemplateManager.delete path gets the guard. - Import preserves a built-in templateKey alongside the resolved agent fallback only when no stored template shadows that key on the target (stored-first spawn precedence would otherwise silently run the target's unrelated override). - spaceExport.bundle judges fallback availability against the full exportable set rather than the agentIds-filtered set, so an exportable migrated fallback excluded by the filter fails the missing-refs check instead of producing a non-portable templateKey-only bundle. - m228 normalizes blank slot names to the agentId (mirroring rowToNode) when clearing execution agent ids and seeding orphan display names. Refs #3614
|
@codex review |
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.3-flash (z.ai)
Model: glm-5.3-flash | Client: HyperNeo | Provider: z.ai
Task: #1831 — chore(daemon): migrate workflow agentId refs to templateKey [AT-M1] (#3614)
Diff: +243/−129 across 9 files — code 130 | tests 113 | comments 0 | other 0 (delta b6cad72 → d82b742; whole PR at d82b742 is ≈ +1900/−200 across ~20 files, majority tests)
Review policy: source auto → external gate is Codex (Devin is dead — its check reports "trial expired and no credits remaining"); depth deep (one-time data migration). This review is a fresh whole-PR re-derivation (rounds 1–4 state at b6cad72) plus a full delta review of the round-5 fixes at d82b742, with an independent second pass on migration correctness. The head moved twice during the session (b6cad72 → d82b742); every conclusion below was re-derived against d82b742.
Recommendation: APPROVE
Round-5 delta (b6cad72 → d82b742) — all four fixes verified
-
m228 blank-slot-name normalization (Codex P1) — fixed and load-bearing.
slotName = slot.name?.trim() ? slot.name : agentIdmirrorsrowToNodeat space-workflow-repository.ts:92 (name: a.name?.trim() ? a.name : a.agentId) exactly, including keeping trim-nonblank names verbatim, soclearExecutionAgentIdnow matches the names the runtime actually recorded. The clearing itself is necessary, not cosmetic: post-migration a slot resolves astemplate:<key>, andvalidateExecutionAgainstWorkflow(workflow-node-execution-validation.ts:173) turns a stale non-nullexecution.agentIdinto a permanent spawn error; the runtime already writes NULLagent_idfor templateKey slots, so the migration makes old rows consistent. A test pins the blank-name case. -
Stored shadows vs preserved built-in bindings (Codex P1) — fixed.
buildWorkflowCreateParamsnow keeps a built-intemplateKeybeside the resolved agent fallback only when the target db has no stored template shadowing that key (builtInTemplate && !storedTemplate) — stored-first spawn precedence inlookupSlotTemplateSourcewould otherwise silently run the target's override (wrong instructions/tools/model) instead of the imported agent. Preview (merged built-in+stored known-set) and execute (stored-only set) stay consistent; template-only slots keep the target db's shadow semantics by design. A test covers the shadow case end to end. -
Filtered bundle exports (Codex P2) — fixed. The bundle precheck now judges fallback availability against the full exportable set computed before the
agentIdsfilter (exportableIds), so an exportable fallback excluded by the filter fails the export with the missing-refs error instead of emitting a bundle whosemigrated.*key no clean db can resolve. Unexportable (e.g. paused) fallbacks still export template-only — the round-2/3 contract, unchanged and still pinned by test. -
Reference guard moved into the delete pipeline (Codex P1, ADR 0004) — fixed. Counting logic moved verbatim to
space-agent-template-references.ts;deleteCheckReferencesnow runs insiderunDeleteTemplatewith the counter injected at the single production wiring point (rpc-handlers/index.ts) and in the handler tests, so everySpaceAgentTemplateManager.delete()caller enforces the invariant, not just the RPC path.
Round-3 dispute ruling (thread on space-workflow-manager.ts:651 — "Serialize stored templates with exported workflows")
Dispute upheld; the finding is dismissed as out of slice for AT-M1. I verified the dispute's factual basis in code: m228 never creates a fallback-less stored-template slot (runnable agents keep a live agentId fallback, orphaned refs keep the orphan fallback — both portable through export + agentRef-resolving import); nothing exportable before this slice lost portability (pre-slice, validateWorkflow only accepted built-in keys, so stored-key slots could not exist); and cross-db import of the residual state fails safe at preview with an explicit "references unknown template" error. The one in-slice sharp edge — a fallback excluded by an agentIds filter — was exactly the round-5 P2 and is fixed above. The complete fix is a bundle-format feature (serializing template definitions into SpaceExportBundle with import-side create/collision handling) and belongs with the template-library slices of epic #3591. Action for the implementer (non-blocking): file the follow-up task ("bundle carries agent template definitions") under that epic so it is not lost.
Whole-PR verification (fresh this session)
- Migration: transactional BEGIN/COMMIT/ROLLBACK with a mid-run failure test; deterministic key allocation rebuilt from the claimed set (idempotent rerun pinned);
isRunnableAgentRowexactly mirrorsisRunnableUnifiedAgent(migrated-worker mirror or active); cross-space handle collisions disambiguated; template-only/agentless slots untouched. settingSourcesempty-list semantics: preserved end to end — synthesis keeps[](truthy spread), the repository round-trips the JSON empty array,storedTemplateToNodeAgentSourcemaps null→undefined but keeps[], andQueryOptionsBuilderline ~438 doesconfig.settingSources ?? globalSettings.settingSources, so[]genuinely disables all setting sources while undefined falls back to globals. The empty→null collapse fortools/modelPoolis forced by the storage encoding (encodeJsonArray) and is behavior-preserving (getAllowedToolsand the pool scheduler treat empty and absent identically).isAgentReferenced: counts only slots binding the agent outright (templateKey present ⇒ fallback, not owner) and scans definition versions pinned by executable runs with the exacthasExecutableRunspredicate; the m228 fallback-ignoring deletion test and pinned-run blocking test cover both directions.- Import/export: agent-preferred binding with built-in retention, preview/execute set consistency, export writes the kept fallback
agentRefbesidetemplateKey, both export prechecks skip only unexportable template-slot fallbacks. - Tests are substantive: equivalence pins (migrated-template spawn ≡ legacy agent spawn across all spawn fields), spawn-config resolution through stored/shadow/fallback branches, rollback, idempotency, deletion guards, and the four round-5 regression tests. CI on d82b742 is green across shards (1-core was still running at review time; lint/knip/type-check pass).
External gate status
Codex round-5 findings (3×P1, 1×P2) are all fixed and every review thread is resolved. Codex round-6 review is in flight on this head (eyes reaction at 02:56:07Z, trigger comment 5548885566) — its clean verdict on d82b742 is required by the gate before merge. Per the dispatch, I am holding terminal actions until the implementer confirms.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d82b74238a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- m228 leaves orphaned migrated-worker mirrors on the agentId binding: a legacy mirror whose authoritative space_agents twin is missing is not runnable at spawn time (resolveUnifiedSlotAgent requires the worker), so migrating it would resurrect the deleted agent's config; mirrors with a live same-space twin still migrate. - isAgentReferenced treats a template-bound agentId as a non-owning fallback only when the template key actually resolves (built-in or stored); with an unresolved key the fallback is the spawn source and the agent stays protected from deletion. Refs #3614
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5ffd1e75d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- m228 resolves each node's workflow space and only synthesizes from an agent whose space matches it: a stale slot referencing another space's agent keeps its pre-upgrade unresolved binding instead of exposing and running that agent's configuration (resolveUnifiedSlotAgent enforces the same space scoping at spawn time). - Export prechecks exempt portable built-in template keys from the fallback-agent requirement: a built-in key round-trips on its own, so an agentIds filter omitting that fallback no longer fails the bundle; stored keys still require an exportable fallback when one exists. Refs #3614
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac45c8fe36
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| const slots2 = nodeConfig(db, 'node-2').agents as Array<Record<string, unknown>>; | ||
| expect(slots2[0].templateKey).toBe('migrated.coder.agentb'); | ||
| expect(slots2[1].templateKey).toBe('migrated.coder'); |
There was a problem hiding this comment.
This test creates agent-a in space-1 but references it from the workflow in space-2; the migration now deliberately skips that cross-space reference, so slots2[1].templateKey remains undefined. Running this test fails with Expected: "migrated.coder", Received: undefined, causing the daemon unit shard to fail. Move the reused reference into the agent's space or expect it to remain agent-bound.
Useful? React with 👍 / 👎.
| a.templateKey?.trim() && | ||
| (getLongHorizonAgentTemplate(a.templateKey.trim()) !== undefined || | ||
| !liveById.has(agentId)) |
There was a problem hiding this comment.
When spaceExport.workflows handles a built-in-template slot with an exportable retained fallback and another exportable agent has the same normalized display name, this early exit omits the fallback from referencedAgentIds, so the ambiguity check never runs. exportWorkflow still emits its agentRef, and the later name-based filtering includes both same-named agents; spaceImport.execute then rejects the exported bundle through findDuplicateBundleAgentNames. Either omit the unnecessary fallback from this export or continue validating its name.
Useful? React with 👍 / 👎.
|
@hyperneo-ai-test review |
1 participant
Closes #3614 (slice MIGRATE of #3591).
Adds the one-time m228 schema migration: every workflow node agent slot bound by
agentIdwithout atemplateKeygets a synthesized template inspace_agent_templates(copied from the referenced agent's current config; orphaned refs get generated templates), and the slot binding is rewritten to carrytemplateKeywhile keeping theagentIdfallback data — removal waits for a later DELETE slice once the pins prove resolution. Executions of migrated slots drop their recordedagent_idso in-flight runs revalidate against the template binding instead of dying on the agent-id mismatch.Making the migration safe required the resolution seams to follow the data:
resolveSlotSpawnConfignow resolvestemplateKeyagainst built-ins ∪ stored templates and falls back to the keptagentIdwhen the key resolves nowhere (previously it only knew built-in keys and returned null → permanent spawn error).resolveNodeAgentConfigpassesmodelPoolthrough the template branch so pool scheduling survives agents that had pools.Pins first: the equivalence pin in
spawn-slot-resolution.test.tsproves a synthesized template resolves the same spawn fields (prompt, model, provider, thinkingLevel, settingSources, tools, modelPool) as the legacy agent binding, and the migration test re-proves it end-to-end through the DB round-trip.Size note: the issue budgeted ~200 prod lines for the migration + repo/synthesize logic; the actual prod diff is ~530 added lines (synthesis module, m228, spawn resolution, workflow-manager and import validation, and the review-driven hardening across two Codex rounds — export/import round-trip, deletion guards for live and run-pinned references, non-runnable-agent preservation, transactional migration). The overrun is reported here rather than silently absorbed; no split requested.