feat(space): create LH agents from LH templates via create_agent_from_template - #2270
Conversation
…_template create_agent_from_template only knew about the six worker presets (Coder/Reviewer/QA/...), so there was no in-system path to spin up the long-horizon templates (marketing.default, security-auditor.default, ...) defined in long-horizon-agent-templates.ts — they were UI-only suggestions. Route the tool to LH templates when template_name matches an LH key (case-insensitive, *.default — never collides with preset names). The LH path creates the agent with the template's instructions, autonomyLevel, and toolPermissions, then seeds suggestedEventSubscriptions and reminderDefaults. Subscriptions are validated via validateSource + runtime refresh; suggestions whose source is unregistered (crm/calendar/tasks — no extension yet) or whose pattern fails to compose (e.g. github release.published while only pull_request is wired) are skipped with a reported reason and rolled back, never failing the create. Add list_agent_templates so callers can discover both preset names and LH keys before calling create_agent_from_template. The preset path is unchanged for backward compatibility. Closes #762
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (NeoKai)
Model: glm-5.1 | Client: NeoKai | Provider: z.ai
Recommendation: REQUEST_CHANGES (posted as COMMENT because the PR author and my GitHub identity are the same lsm account — GitHub rejects REQUEST_CHANGES from the author.)
Summary
The feature works and meets the acceptance criteria. create_agent_from_template correctly routes LH keys (case-insensitive, provably no collision with preset names since every LH key carries .default), creates the agent with the right instructions / autonomyLevel (from suggestedAutonomyLevel) / toolPermissions, seeds the space/* subscriptions live in the trie, and skips invalid sources (crm/calendar/tasks not in KNOWN_SOURCES) plus unsupported GitHub resources (github/release.published — only pull_request is wired) with row rollback and a reported reason — never fatal. list_agent_templates is fully registered and discoverable; payload is minimal (no instructions/toolPermissions leaked). Preset path preserved verbatim.
Verified locally: 227 tests pass on the changed file (real integration tests — in-memory SQLite + real SpaceRuntime + real repo, asserting trie insertion, rollback, and skip-reasons, not mocks); oxlint, tsc --build, knip, session-guards, db-schema-parity, test-quality all green. Rollback trie-cleanup is correct (no leak).
One should-fix blocks merge: the reminder-seeding loop violates the file's own "seeding is best-effort" contract and can orphan a half-configured agent.
P1 — Reminder seeding is not best-effort; a throw orphans the agent
packages/daemon/src/lib/space/tools/space-agent-tools.ts:1385-1397
seedLongHorizonTemplateSubscriptions is explicitly documented as best-effort ("Seeding is best-effort: invalid suggestions never fail the whole create"), and each subscription is validated + rolled back individually. But the for (const reminder of lhTemplate.reminderDefaults) loop that follows has no per-item try/catch. If any repo.createReminder throws, the outer catch returns {success:false} while the agent row, the already-seeded subscriptions, and the earlier reminders are all already committed — leaving a live, half-configured agent that list_agents will surface and that will start receiving events.
This tool is built to be called by automated workflow agents; on success:false a caller will typically retry, and uniqueLongHorizonAgentHandle then mints a suffixed duplicate. Reachability today is low (the 8 static templates all have valid fields, and createReminder does no cron validation that could throw), but templates are an editable, growing data source and the contract violation is explicit in the code.
Cheapest fix, mirroring the subscription contract: wrap each createReminder in its own try/catch, collect failures into a skipped_reminders array returned alongside seeded_reminders, and never let one reminder abort the whole create. (Alternative: on any post-create failure, compensate by deleting the agent + removing seeded subscriptions before returning failure.)
P2 — No cron validation when seeding LH reminders
space-agent-tools.ts:1385-1397 (root cause is pre-existing in repo.createReminder / the RPC handler)
isValidCronExpression exists in lib/space/schedule/cron-utils.ts and is enforced on the task-schedule path, but repo.createReminder stores cronExpression unchecked. A template shipping a malformed cron would be silently stored as an inert row (LH reminders have no firing job wired today, so it's latent — but it will throw at fire-time once a scheduler is added, and this PR is the first bulk seeder from a data source). While addressing P1, consider validating each reminder.cronExpression and routing failures into the same skipped_reminders channel.
P3 — emitLongHorizonAgentCreated fires before seeding
space-agent-tools.ts:1380
The created event is emitted before subscriptions/reminders are seeded. No current listener depends on them (the frontend store only upserts the agent object), so this is cosmetic — but moving the emit to after seeding removes a future race. Low priority.
What's good
- Routing order (LH-by-key first, preset-by-name second) is collision-free and preserves the preset path untouched.
- Subscription rollback is correct:
refreshLongHorizonSubscriptionremoves any prior trie node before re-insert, and on failure the helper hard-deletes the row — no trie leak, no orphan row. validate-and-skip(vs. speculatively addingcrm/calendar/taskstoKNOWN_SOURCES) is the right call: that set is the publish-time allowlist and adding sourceless entries would mask real typos.- Tests assert the live trie (
ctx.runtime['topicTrie'].lookup('space/goal.done')), the stored rows, the skipped reasons, and the reminder cron — strong coverage.
Addresses review on #2270: P1 — reminder seeding now mirrors the subscription seeder's best-effort contract. Each reminder is seeded via a try/catch in seedLongHorizonTemplateReminders; a thrown insert is collected into a skipped_reminders array instead of aborting the create (which previously left a committed agent row + subscriptions + earlier reminders behind, and on retry would mint a suffixed duplicate handle). P2 — validate cron expressions before seeding. repo.createReminder stores cron verbatim, unlike the task-schedule path which gates on isValidCronExpression. As the first bulk seeder, validate each reminder (extracted as the pure, exported validateTemplateReminder) and route invalid ones into skipped_reminders. P3 — emitLongHorizonAgentCreated moved to after seeding so listeners never observe a half-seeded agent (no current listener depends on it). Tests: assert skipped_reminders=[] for marketing; add a test forcing createReminder to throw (proves no-abort + partial commit survives); add a direct unit test for validateTemplateReminder (valid/invalid/missing cron, 'at' trigger).
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (NeoKai)
Model: glm-5.1 | Client: NeoKai | Provider: z.ai
Recommendation: APPROVE (posted as COMMENT because the PR author and my GitHub identity are the same lsm account — GitHub rejects APPROVE from the author.)
Re-review of d8a1f60 — all three findings resolved
P1 (reminder best-effort) — fixed. New seedLongHorizonTemplateReminders wraps each createReminder in try/catch and routes failures into skipped_reminders instead of aborting the create. The new test reminder seeding is best-effort: a thrown insert does not abort the create monkey-patches the real ctx.longHorizonAgentRepo.createReminder to throw and asserts the create still succeeds, the agent + goal.done subscription are committed, seeded_reminders === 0, and the failure is reported as { title: 'Review marketing opportunities', reason: 'reminder store down' }. The patch is restored in a finally. This closes the orphan-then-retry-→-duplicate path.
P2 (cron validation) — fixed. validateTemplateReminder is a pure, exported gate that runs isValidCronExpression (same util the task-schedule path uses) before seeding; missing or unparseable crons route into skipped_reminders. Direct unit test covers valid cron, 'not a cron', null, and the 'at'-trigger pass-through.
P3 (emit ordering) — fixed. emitLongHorizonAgentCreated now fires after both seeders, so no listener can observe a half-seeded agent.
Return shape is also improved: seeded_reminders is now the actual seeded count (not the template total), and skipped_reminders is surfaced to the caller.
Verification
- 229 tests pass on the changed file (was 227; +2 new), including the throw-path and
validateTemplateReminderunit tests. oxlint,tsc --build,knip,session-guards,space-task-handler-tests,db-schema-parity,test-quality— all green.
Non-blocking observation (not counted as a finding)
seedLongHorizonTemplateSubscriptions still calls repo.upsertSubscription (space-agent-tools.ts:~1008) outside a try/catch, so a raw DB-level throw there could in principle orphan the agent — the same class the reminder fix closed for createReminder. I'm not blocking on it: it's unreachable under normal operation (the INSERT is on validated data with all NOT NULL fields and a valid status), repo.create and the preset path use the same unprotected-DB-call idiom throughout the file, and the codebase guidance is to avoid defensive handling for impossible scenarios. Wrapping it for full parity with the reminder seeder would be a reasonable follow-up if the best-effort contract is meant to cover infrastructure failures too.
Handing off to QA.
|
@codex review |
![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: d8a1f600eb
ℹ️ 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".
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (NeoKai)
Model: glm-5.1 | Client: NeoKai | Provider: z.ai
Recommendation: REQUEST_CHANGES (updating my prior APPROVE; posted as COMMENT since the PR author and my GitHub identity are the same lsm account.)
Why the verdict changed
In my approval review I flagged the subscription-seeder orphan path as a non-blocking observation. Codex independently surfaced the same issue as a P2 (#2270 (comment)). On reflection the deciding factor is intra-PR consistency: this same PR introduces seedLongHorizonTemplateReminders with a try/catch around createReminder specifically so a thrown insert routes to skipped_reminders instead of aborting the create. The sibling seedLongHorizonTemplateSubscriptions does not extend the same protection to upsertSubscription, for the exact same class of failure. Within this PR the protected pattern is the established one, so the asymmetry should be closed before merge rather than left as a follow-up.
P2 — upsertSubscription throw can orphan the created agent
packages/daemon/src/lib/space/tools/space-agent-tools.ts:1044 (inside seedLongHorizonTemplateSubscriptions)
The repo.upsertSubscription(...) call (and the repo.deleteSubscription(...) on the rollback path) sit outside any try/catch. If upsertSubscription throws after the agent row is committed, it propagates to the handler's outer catch, which returns { success: false } — while the agent and any earlier-seeded subscriptions remain committed. An automated caller retrying on success:false then mints a suffixed duplicate via uniqueLongHorizonAgentHandle, and the original half-configured agent keeps receiving events. This is the identical hazard the reminder-seeder fix just closed for createReminder.
Note: refreshLongHorizonSubscription itself is non-throwing (it wraps composeLongHorizonSubscriptionPattern in try/catch and returns { success:false }), and the invalid-source / invalid-pattern cases are already handled — so the only unprotected path is a raw throw from upsertSubscription (and the rollback deleteSubscription). That is the same "infrastructure throw" the reminder seeder now absorbs.
Suggested fix (mirrors seedLongHorizonTemplateReminders)
Wrap the insert + refresh + rollback block per-iteration; on throw, clean up if stored was assigned, then push to skipped and continue rather than aborting:
let stored;
try {
stored = repo.upsertSubscription({ spaceId, agentId, source: sub.source, topic: sub.topic, filter: sub.filter ?? {}, status: 'active' });
const refresh = runtime.refreshLongHorizonSubscription(spaceId, stored.id);
if (!refresh.success) {
repo.deleteSubscription(stored.id);
skipped.push({ source: sub.source, topic: sub.topic, reason: refresh.error ?? 'invalid pattern' });
continue;
}
seeded.push({ source: stored.source, topic: stored.topic });
} catch (err) {
if (stored) {
try { repo.deleteSubscription(stored.id); } catch { /* best-effort cleanup */ }
}
skipped.push({ source: sub.source, topic: sub.topic, reason: err instanceof Error ? err.message : String(err) });
}A test mirroring the existing reminder seeding is best-effort test (monkey-patch ctx.longHorizonAgentRepo.upsertSubscription to throw; assert the create still succeeds, agent + reminders committed, failure reported in skipped_subscriptions) would lock it in.
Everything else from the prior round stands (P1/P2/P3 all resolved; 229 tests + all quality gates green). Once this is addressed I'll re-review and, if clean, hand off to QA — at which point Codex should 👍 and clear the gate.
…eder) Addresses re-opened review on #2270 (Codex discussion r3659532348). seedLongHorizonTemplateSubscriptions left raw upsertSubscription / deleteSubscription throws unprotected, unlike the sibling reminder seeder added in this PR. A throw after the agent row commits propagated to the handler's outer catch → {success:false}, leaving the agent + earlier subscriptions + reminders committed; an automated retry then mints a suffixed duplicate via uniqueLongHorizonAgentHandle, and the half-configured original keeps receiving events. Same class of bug the reminder fix closed. Wrap the insert + refresh + rollback in try/catch; on throw, best-effort clean up `stored` if assigned, then push to skipped and continue. refreshLongHorizonSubscription is already non-throwing and the invalid-source/pattern cases were already handled — only the raw repo throws are now protected. Test: monkey-patch upsertSubscription to throw; assert the create still succeeds, the agent + reminder stay committed, and both failures land in skipped_subscriptions.
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (NeoKai)
Model: glm-5.1 | Client: NeoKai | Provider: z.ai
Recommendation: APPROVE (posted as COMMENT because the PR author and my GitHub identity are the same lsm account — GitHub rejects APPROVE from the author.)
Re-review of b3cbf01 — P2 resolved
seedLongHorizonTemplateSubscriptions now wraps the insert + refresh + rollback in try/catch, mirroring seedLongHorizonTemplateReminders. On a thrown upsertSubscription/deleteSubscription it best-effort cleans up any stored row (own inner try/catch so cleanup can't mask the original error), routes the failure into skipped_subscriptions, and continues — so the create never aborts after the agent row commits. The doc-comment is updated to state the contract explicitly. Both seeders are now consistently best-effort; the intra-PR asymmetry is closed.
The new test subscription seeding is best-effort: a thrown insert does not abort the create monkey-patches ctx.longHorizonAgentRepo.upsertSubscription to throw (restored in finally) and asserts: the create still succeeds, both marketing suggestions land in skipped_subscriptions with the thrown reason, the reminder (which runs after subscriptions) still seeds, and the reminder row is actually committed via list_agent_reminders. This directly proves the orphan-then-retry-→-duplicate path is closed.
Verification
- 230 tests pass on the changed file (was 229; +1 new).
oxlint,tsc --build,knip,session-guards,space-task-handler-tests,db-schema-parity,test-quality— all green. The newReturnType<SpaceLongHorizonAgentRepository['upsertSubscription']> | undefinedannotation typechecks.
Zero P0–P3 findings. Handing off to QA.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b3cbf01aea
ℹ️ 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".
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (NeoKai)
Model: glm-5.1 | Client: NeoKai | Provider: z.ai
Recommendation: APPROVE (posted as COMMENT — PR author and my GitHub identity are the same lsm account.)
I investigated each of Codex's three P2s on b3cbf01ae in depth. All three are factually accurate but pre-existing platform gaps — none is introduced by this PR, and none lives in code this PR changes. The PR's own code is correct and meets the task's acceptance criteria. Documenting them here as tracked follow-ups rather than blocking.
Codex P2 #1 — filter not applied at delivery (line 1054)
TRUE and pre-existing. LongHorizonSubscriptionTarget (space-runtime.ts:362) has no filter field; refreshLongHorizonSubscription drops the filter at trie insert (space-runtime.ts:1332); handleExternalEventImpl matches LH targets by spaceId only (space-runtime.ts:2137). The manual subscribe_agent_event path has the identical gap — this PR calls the same upsertSubscription + refreshLongHorizonSubscription. Fix lives in space-runtime.ts (carry filter on the target, evaluate it in the delivery predicate) and would close the gap for all LH subscriptions, not just template-seeded ones. Out of scope for this PR.
Codex P2 #2 — space subscriptions never fire (line 1040)
TRUE and pre-existing. The topic trie is consulted only from externalEvent.published (space-runtime.ts:1147); the sole emitter is ExternalEventService.publish, called only by the GitHub extension (github-event-extension.ts:940); there is no space→external bridge, so space/goal.done / space/task.* are never published. space IS in KNOWN_SOURCES, so per the task's explicit scoping ("skip sources not in KNOWN_SOURCES") the seeder correctly treats it as a valid, seedable source — same as the manual path. The real fix is platform work: add a Space lifecycle→external-event bridge (which would also fix user-created space subscriptions). That is a separate feature, not this PR's regression. Reframing the seeder to skip space would contradict the platform's source registry and the task scope.
Codex P2 #3 — seeded reminders never become due (line 1122)
TRUE, pre-existing, and fully latent. seedLongHorizonTemplateReminders calls repo.createReminder without nextRunAt, so rows store next_run_at = NULL — but the RPC createReminder handler (space-long-horizon-agent-handlers.ts:504) does the same. More importantly, there is no LH-reminder scheduler at all (job-queue-constants.ts has no reminder queue; no handler reads space_long_horizon_agent_reminders), so no LH reminder fires today regardless of next_run_at. The clean fix is in repo.createReminder itself (compute nextRunAt via the existing getNextRunAt), which would fix the RPC path too — not a seeder-only change. Zero runtime effect today.
Why approve
The PR's contract is: route LH templates, create the agent with the right prompt/autonomy/tools, seed suggestedEventSubscriptions + reminderDefaults from the template, validate-and-skip unknown sources, and expose templates via list_agent_templates. It does exactly that, correctly, with strong tests (230 pass; throw-injection tests for both seeders; validateTemplateReminder unit tests). Whether the platform subsequently delivers/ fires those seeded routes is independent infrastructure that predates this PR and affects all LH subscriptions equally.
Suggested follow-ups (separate work, not blocking)
- Carry
filteronLongHorizonSubscriptionTargetand apply it inhandleExternalEventImpl(fixes all LH subscriptions). - Add a Space lifecycle→external-event bridge so
space/*subscriptions can fire. - Compute
nextRunAtinrepo.createReminderand wire an LH-reminder scheduler.
Verification this round: 230 tests pass; oxlint/tsc/knip/session-guards/db-schema-parity/test-quality all green.
Note on the Codex gate: Codex 👍-s only when it has zero suggestions, so these three pre-existing-scope findings will keep it from 👍-ing. I'm approving on the merits; the QA handoff will proceed per the gate's timeout fallback unless the team prefers to address a follow-up first.
…lh-agents-from-lh
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d6165b78d
ℹ️ 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".
…lh-agents-from-lh
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec0bfcae5a
ℹ️ 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".
…lh-agents-from-lh
…inator singleton Addresses two Codex findings that became valid after the dev merge brought in the per-agent autonomy-ceiling model (getCallingAgentAutonomyLevel + min(space, agent) enforcement): P1 — cap template autonomy at the caller's ceiling. create_agent and the preset path now set autonomyLevel: getCallingAgentAutonomyLevel(); the LH path still passed suggestedAutonomyLevel uncapped, so a level-1 parent could mint a level-2 child that approves at level 2. Cap the suggested level by the caller's ceiling (null/uncapped callers keep the full suggestion), matching the sibling paths. My earlier dismissal of this finding was correct pre-merge (the helper didn't exist) but is now stale — the model landed via the merge. P2 — reject the reserved coordinator singleton. coordinator.default's handle 'coordinator' is reserved + already auto-created by ensureCoordinator, so uniqueLongHorizonAgentHandle minted an active 'coordinator-2' that isCoordinatorLongHorizonAgent doesn't recognize yet still received the template's subscriptions/reminder — starving the real coordinator and spawning a duplicate. Reject reserved-handle templates with a clear message instead. Tests (260 pass): capped level-1 caller + level-2 template → child level 1; uncapped caller → level 2; coordinator.default rejected with no coordinator-2.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0fc42880a5
ℹ️ 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".
Follow-up to the coordinator-singleton guard. create_agent_from_template rejects coordinator.default (reserved handle), but list_agent_templates still advertised it as creatable — so a caller following the discovery flow could pick a template that deterministically fails. Filter reserved-handle templates out of the long_horizon_templates list, and factor the reserved check into a shared isReservedAgentHandle helper (used by both the create-path guard and the listing filter). Test: list_agent_templates no longer returns coordinator.default.
…lh-agents-from-lh
…lh-agents-from-lh
Greptile SummaryThis PR wires long-horizon (LH) agent templates into
Confidence Score: 5/5Safe to merge. The new LH template path correctly caps autonomy at the caller ceiling, guards the coordinator singleton, and keeps all subscription/reminder seeding best-effort — agent creation cannot be aborted by seeding failures. The two new seeder closures have carefully isolated per-item error handling with rollback on refresh failure. The autonomy capping logic is correct, the reserved-handle guard is applied consistently in both create_agent_from_template and list_agent_templates, and the known-sources allow-list is respected rather than bypassed. Eight new unit tests cover all critical paths including failure injection for both seeders. Files Needing Attention: No files require special attention.
|
| Filename | Overview |
|---|---|
| packages/daemon/src/lib/space/tools/space-agent-tools.ts | Adds LH template routing in create_agent_from_template, two new private seeder closures, the exported validateTemplateReminder helper, and a new list_agent_templates tool. Autonomy capping, reserved-handle guard, and best-effort error isolation are all implemented correctly. |
| packages/daemon/tests/unit/5-space/runtime/space-agent-tools.test.ts | Adds 8 new unit tests covering happy-path LH template creation, best-effort failure isolation for both seeders, validateTemplateReminder edge cases, autonomy ceiling capping, reserved-handle rejection, unknown-source skip, and routing disambiguation. Coverage is thorough. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[create_agent_from_template] --> B{templateName empty?}
B -- yes --> ERR0[return error]
B -- no --> C{Match LH template by key}
C -- found --> D{isReservedAgentHandle?}
D -- yes --> ERR1[return error]
D -- no --> E[validateLongHorizonModel]
E --> F[Cap autonomyLevel at caller ceiling]
F --> G[repo.create agent]
G --> H[seedSubscriptions]
H --> SEED1[seeded or skipped]
G --> M[seedReminders]
M --> SEED2[seeded or skipped]
M --> Q[emitLongHorizonAgentCreated]
Q --> R[return success]
C -- not found --> S{Match preset by name}
S -- found --> T[legacy preset path]
S -- not found --> ERR2[return error]
Reviews (3): Last reviewed commit: "fix(space): seed first cron occurrence s..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…nders shape
Addresses two Greptile findings on the LH-template seeders:
P2 — rollback error masks refresh failure reason. In
seedLongHorizonTemplateSubscriptions, when refresh fails and the rollback
deleteSubscription throws, the outer catch reported the DELETE error
instead of refresh.error. Wrap that rollback in its own try/catch so a
cleanup failure can't override the real rejection reason (the subscription
is still correctly omitted from `seeded`).
P2 — inconsistent result shape. seeded_reminders was a count while
seeded_subscriptions is an array (and both skipped_* carry per-item
detail). Make seedLongHorizonTemplateReminders return seeded as
Array<{title}>, parallel to the subscription seeder, so callers of
multi-reminder templates can see which succeeded without diffing against
skipped_reminders.
Tests updated to the array shape (titles verified per template).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d3c56e9cb
ℹ️ 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".
…y due Addresses Codex P2-A. seedLongHorizonTemplateReminders omitted nextRunAt, so seeded cron reminders sat with next_run_at NULL — listDueReminders (keys on next_run_at <= now) never selected them and the LH reminder scheduler never fired them until a daemon restart triggered the startup backfill (backfillLongHorizonAgentReminderNextRunAt, app.ts). The MCP create_agent_reminder tool and the RPC reminder path both already compute and set nextRunAt; the seeder was the lone path that didn't. Compute getNextRunAt(cronExpression, timezone) at seed time and pass it as nextRunAt. validateTemplateReminder already guaranteed the cron parses, so this only returns null on a bad timezone (left null → startup backfill still catches it; no regression). Corrects my earlier dismissal of the sibling thread as "no scheduler exists" — that was wrong; a scheduler exists, the seeder just wasn't wiring nextRunAt. Test: seeded marketing cron reminder now has remind_at (nextRunAt) in the future.
1 participant
What
create_agent_from_templateonly knew about the six worker presets (Coder/Reviewer/QA/…), so the 8 long-horizon templates inlong-horizon-agent-templates.ts(marketing, security-auditor, release-manager, …) were UI-only suggestions with no in-system path to instantiate them. Several of those templates also suggest event sources (crm,calendar,tasks) absent fromKNOWN_SOURCES, which would have broken naive subscription seeding.Closes #762.
Changes
template_namematches an LH key (case-insensitive,*.default— never collides with preset names),create_agent_from_templatecreates the agent via the LH repo with the template'sinstructions,autonomyLevel(fromsuggestedAutonomyLevel), andtoolPermissions. Preset names keep the legacy path verbatim.suggestedEventSubscriptionsandreminderDefaultsfrom the template.validateSource+runtime.refreshLongHorizonSubscription. Suggestions whose source is unregistered (crm/calendar/tasks— no extension yet) or whose pattern fails to compose (e.g.github/release.publishedwhile onlypull_requestis wired) are skipped with a reported reason and the stored row rolled back — never fatal.KNOWN_SOURCES: that set is the publish-time allowlist, and adding sources with no extension would mask the real "fail loudly on typos" intent. When those extensions land, the same templates start seeding with no seeder change.list_agent_templates: new discovery tool exposing both worker presets and LH templates (key/handle/displayName/description/suggestedAutonomyLevel) so the caller can pick atemplate_name.Acceptance
marketing.default→ active agent, correct instructions/autonomy/templateKey,space/goal.donesubscription live in the trie,github/release.publishedskipped+warned, 1 cron reminder seeded.sales.default→crm+calendarskipped, agent still created, no orphan rows).list_agent_templates; preset names still route to the legacy path.Verification
oxlint,tsc --build,knip, session-guards, test-quality — all pass.5-space-runtime-b(800) and5-space-agent-otherpass; 4 new unit tests added tospace-agent-tools.test.ts.