This command had no tests at all, while carrying the rules that decide whether
a deploy may change a published agent. Everything interesting was inside run(),
which loads configuration, opens its own pool and resolves a tenant from a
slug — none of which a test can supply. So it was untestable by construction
rather than by neglect, and the fix is a seam, not a test-only helper.
Three functions come out of run(), each doing one thing:
validateSpecs the parse and status checks. Pure.
validateGraph §3's DAG check over the whole set. Pure.
importInto the write phase, taking a transaction the caller owns and
returning what it did.
run() is now the wiring around them. importInto does not commit — the caller
does — so a refused rewrite leaves the caller's deferred rollback to undo the
writes that already happened, which is the behaviour that was there before and
is now visible in the signature rather than implied by where the code sat.
Six tests, four of them against a real database:
- every problem is reported, not the first: two bad specs produce two
messages and a good one produces none;
- a chain is not a cycle, and a cycle names the edge to cut;
- a first import records versions, and a second over unchanged specs
records none — the counter that used to say nine every deploy;
- a changed spec at the same version is refused AND nothing is committed,
checked by reading the row back;
- a lowered version is refused and the live row is still at the higher one;
- a raised version is accepted and leaves two rows in the history.
The author is resolved through resolveAuthor rather than passed as a literal,
so the tests exercise that path too and fail loudly on an organization with no
active admin — a real deployment condition. The first draft passed "" and got
`invalid input syntax for type uuid`, which is what a literal buys you.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PJvibeSc1JYXjatankqM1g
Two rules §3 states and nothing checked.
MONOTONIC. The rewrite guard added earlier compares content at ONE version
number, so republishing an OLDER number with the text originally published
under it looked like a no-op: nothing conflicted, nothing was refused, and the
live row silently reverted. The agent then reads v1 in the UI while the newest
thing anybody approved was v2. The test publishes v1, publishes v2, republishes
v1 byte-for-byte, and asserts both the refusal and that the live row is still
v2. Without the guard it answers 200 and the row goes back to version 1.
DAG. §3 says cycle detection runs at publish; only the runtime depth cap
existed. That cap means a cycle was never a safety problem — it was a budget
one. Every run entering the loop spends its whole allowance delegating in a
circle before terminating, and the author learns about it from a bill rather
than from the publish that created it.
definition.FindSubagentCycle is a pure function over id -> subagent ids, so it
is tested directly: chains, diamonds, self-reference, loops not involving the
first agent walked, and a 5000-long chain that would matter if this were
written to recurse carelessly. It REPORTS the cycle ("a -> b -> c -> a")
rather than merely detecting one, because an operator otherwise has to find it
by hand across a set of specs. The report is deterministic — a test runs it
fifty times over a graph with two cycles and requires the same answer, since Go
randomises map iteration and an error message that changes between identical
runs is one nobody trusts.
Wired into both publish paths. importagents has every spec in hand, which is
the only place that is cheaply true. The API builds the graph from
organization-visible agents plus the incoming definition standing in for its
stored self — otherwise an edit that CREATES a cycle is checked against the
version that did not have one and passes. Personal agents are excluded: they
are invisible to everyone else so cannot complete anyone else's loop, and
reading them would mean reading other people's drafts to validate your own.
An edge to an agent that is not in the set is ignored rather than reported.
That is a different failure with a different message
(runtime.unknown_subagent), and conflating them prints "cycle detected" for
what is actually a typo.
The cycle test bumps the version on the loop-closing edit. Without that the
rewrite guard refuses it for changing published text, the test passes for the
wrong reason, and it would keep passing with cycle detection deleted — which
is how it was first written, and what running it without the guard showed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PJvibeSc1JYXjatankqM1g
Two problems found by deploying the previous commits to production.
FIRST: the rewrite guard compared raw Markdown, so it refused a publish over
formatting. The authoring UI re-serialises a definition when somebody saves it
— writing `webSearch: false` where the hand-authored file omitted the key, and
ordering the frontmatter its own way — and the parser reads absent and false
identically (agent.go: `data["webSearch"] == true`). A definition nobody
meaningfully changed stopped a deploy. Refusing a change that is not a change
is still a bug, even though it fails safe.
definition.SameAgent and SameSkill compare the parsed definition instead, and
are deliberately conservative, because the two ways of being wrong are not
equally bad. A false difference blocks a deploy: visible, recoverable. A false
SAMENESS lets a changed agent overwrite an approved version silently, which is
the thing versioning exists to prevent. So:
- The body is compared verbatim. Agent.Body carries `json:"-"`, so a
comparison that only marshalled the struct would call a completely
rewritten system prompt "unchanged". There is a test that fails loudly on
exactly that, because it is the mistake this design invites.
- List ORDER stays significant. loader.go resolves Skills in order and that
order reaches prompt assembly, so two definitions listing the same skills
differently are still different. A deploy that only reorders still has to
raise its version. That is a limit, recorded in a test rather than left to
be discovered: loosening it needs somebody to decide skill order cannot
matter, which is not a decision to bury in a comparison function.
What it absorbs is exactly what the round trip produces: frontmatter key order,
whitespace, and a defaulted value written out in full.
SECOND: importagents reported "9 agent version(s) recorded" on a run that
recorded nothing. The counter incremented on every successful Snapshot call,
and Snapshot returns nil for the idempotent no-op as well as for a real insert.
The skill counter was already honest; the agent one was not. snapshotAgent now
distinguishes recorded / conflict / already-present, and only the first counts.
Verified locally: 1 on the run that added activity-agent v2, 0 on the re-run,
where it previously said 9. A number that says nine every time is one nobody
checks on the day it matters.
Full suite green against PostgreSQL, only TestLive* skipped.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PJvibeSc1JYXjatankqM1g
repo.KindSkill existed with nothing writing it. Migration 000010 says "agents
and skills version identically", the table has always accepted kind='skill',
and no path on either side ever recorded one — so an edit to a skill left no
record of what it used to say. Agents name their skills and the runtime refuses
to load one whose skill is missing, so a skill changing under a pinned agent is
the same class of problem the last two commits fixed, one layer down.
Skills are numbered differently, and not by preference. An agent's frontmatter
carries `version:`, so its author decides when a change is a new version and can
be refused for rewriting an old one. definition.Skill has no such field, the
skill_definitions table has no such column, and the vocabulary is active |
inactive rather than draft | published. Giving skills an authored version would
mean a migration, a parser change on BOTH sides of the conformance test in
internal/definition — which replays a capture of the real frontend module graph
— and an edit to all 23 shipped skills. That is a feature, not this fix.
So the server assigns it: one after whatever was last published. This is not an
invention. repo.VersionsRepo.LatestVersion was written for exactly this and
says so — "the next published version has to follow what was actually published
rather than what somebody wrote in the frontmatter" — and had no callers
outside its own test.
Because the author never names a version, there is nothing to refuse: an edit
is always a new version. What needs care instead is the opposite — a save that
changed nothing must NOT be one, or every deploy would add a version to all 23
skills and the number would stop meaning anything. Each publish is compared
against the last recorded copy first. Inactive skills are not recorded at all;
inactive is this vocabulary's draft.
Verified against a live stack:
- first import over 23 unversioned skills: 23 skill version(s) recorded
- second import, files unchanged: 0 recorded, total still 23
- one skill edited: 1 recorded, that skill at v1, v2; v1 still holds the
original text and v2 the edit
The test fails without the change — "after create: 0 version(s), want 1" — and
covers the three behaviours that matter: an edit versions, an identical save
does not, and an inactive skill is not recorded.
Both races noted on the agent path apply here as well: two simultaneous edits
can compute the same next number, and the loser's snapshot is dropped rather
than failing the author's save.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PJvibeSc1JYXjatankqM1g
The previous commit closed this hole on the authoring path. This is the other
half, and the larger one: every organization agent is published by this
command, so until now none of them were versioned at all. definition_versions
was empty on a fully deployed system, and each deploy rewrote v1 in place with
whatever the files happened to say.
Versions are now recorded through the same transaction as the definitions, so
the history and the row it describes cannot disagree — either both land or
neither does. repo.VersionsRepo.Snapshot is what refuses a spec whose content
changed without its `version:` being raised, and that refusal now stops the
import rather than being absent.
Every offending spec is collected instead of the first being returned, matching
how the parse errors above it already behave: an operator who forgot to bump
three files should see three. That is safe here because the refusal comes from
comparing a row this code read, not from a failed statement — the INSERT is ON
CONFLICT DO NOTHING, so the transaction stays healthy and the remaining specs
can still be checked.
The header comment claimed "it does not create versions" as a deliberate
omission, deferring immutability to Phase 3. Phase 3 shipped; the comment is
updated rather than left to describe a decision that has been reversed.
Verified against a live stack:
- first run over nine unversioned agents: 9 version(s) recorded
- second run, files unchanged: still 9, not 18 — republishing is a no-op
- a spec edited without a bump: refused by name, exit 1, and the live row
did NOT contain the edit; the whole transaction rolled back
- the same spec with version: 2: exit 0, v1 and v2 both in history, live
row at v2
Not addressed, and visible while testing this: the command does not enforce
monotonicity. A file whose version is LOWERED still overwrites the live row,
because the upsert writes whatever the frontmatter says. History is unharmed —
the older version is already recorded and matches — but the deployed
definition silently goes backwards. That wants its own change.
cmd/importagents still has no test files, which predates this. The refusal
itself is covered by repo/versions_test.go; what is untested here is the
collecting and rollback around it, and run() opens its own pool from config,
so making it testable is a refactor rather than an addition.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PJvibeSc1JYXjatankqM1g
