From 6b3dda8e5a7b06cec189d0e89ebe1f0db4d1aaa3 Mon Sep 17 00:00:00 2001 From: Suriyakumarvijayanayagam Date: Fri, 28 Aug 2026 19:24:56 +0530 Subject: [PATCH] Record versions when importagents publishes, and refuse a silent rewrite MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01PJvibeSc1JYXjatankqM1g --- go-api/cmd/importagents/main.go | 68 +++++++++++++++++++++++++++++---- 1 file changed, 60 insertions(+), 8 deletions(-) diff --git a/go-api/cmd/importagents/main.go b/go-api/cmd/importagents/main.go index b26c871..f1e308b 100644 --- a/go-api/cmd/importagents/main.go +++ b/go-api/cmd/importagents/main.go @@ -7,11 +7,16 @@ // // What it does NOT do, deliberately: // -// - It does not create versions. §3 says specs are immutable once published -// and editing publishes a new version; this re-publishes in place, which is -// right for a curated set shipped with the deployment and wrong for -// authored ones. Version immutability is Phase 3's, and this command is the -// thing that makes Phase 3 worth doing rather than a substitute for it. +// - It does not validate every spec against a running model. Parsing and +// dependency checks happen here; behaviour is what the eval suites are for. +// +// It DOES record versions, in the same transaction as the definitions. §3 says +// a published version is immutable and editing publishes a new one, and a +// command that re-published in place was the one path that ignored that: the +// live row took the new text and nothing recorded what the old one said, so +// every deploy quietly rewrote v1. A spec whose content has changed without +// its `version:` being raised is now refused, and refused for the whole set — +// see the note above the import loop. // - It does not validate tool names against the registry. §3 wants an unknown // tool to fail at publish; today the runtime records and drops one. The // check is cheap to add and belongs here — see the note in run(). @@ -31,9 +36,12 @@ import ( "github.com/jackc/pgx/v5" "github.com/jackc/pgx/v5/pgxpool" + "github.com/krow/krow-backend/go-api/internal/authctx" "github.com/krow/krow-backend/go-api/internal/config" "github.com/krow/krow-backend/go-api/internal/db" "github.com/krow/krow-backend/go-api/internal/definition" + "github.com/krow/krow-backend/go-api/internal/domain" + "github.com/krow/krow-backend/go-api/internal/repo" ) func main() { @@ -164,7 +172,22 @@ func run(dir, skillDir, orgSlug string, dryRun bool, timeout time.Duration) erro skillsWritten++ } - inserted, updated := 0, 0 + // Versions are recorded through the same transaction, so the history and + // the definition it describes cannot disagree: either both land or neither + // does. + // + // Snapshot is what refuses a spec that changed without raising its + // `version:`. Every such spec is collected rather than the first one + // returned, for the same reason the parse errors above are — an operator + // who forgot to bump three files should see three. Collecting is safe here + // because that refusal comes from comparing a row this code read, not from + // a failed statement: the INSERT is ON CONFLICT DO NOTHING, so the + // transaction is still healthy and the remaining specs can be checked. + versions := repo.NewVersionsRepo(tx) + ident := authctx.Identity{OrgID: orgID, UserID: author} + + inserted, updated, versioned := 0, 0, 0 + var rewrites []string for _, s := range specs { wasNew, err := upsert(ctx, tx, orgID, author, s) if err != nil { @@ -175,13 +198,42 @@ func run(dir, skillDir, orgSlug string, dryRun bool, timeout time.Duration) erro } else { updated++ } + + err = versions.Snapshot(ctx, ident, repo.SnapshotInput{ + Kind: repo.KindAgent, + DefinitionID: s.parsed.ID, + Version: s.parsed.Version, + Markdown: s.raw, + Name: s.parsed.Name, + Description: s.parsed.Description, + Pages: s.parsed.Pages, + }) + var apiErr *domain.Error + switch { + case err == nil: + versioned++ + case errors.As(err, &apiErr) && apiErr.Code == "conflict": + rewrites = append(rewrites, fmt.Sprintf(" %s: %s", s.name, apiErr.Message)) + default: + return fmt.Errorf("%s: record version: %w", s.name, err) + } } + + if len(rewrites) > 0 { + return fmt.Errorf( + "%d spec(s) would rewrite a version that is already published:\n%s\n\n"+ + "Nothing was written. Raise `version:` in the frontmatter of each, or "+ + "restore the published text.", + len(rewrites), strings.Join(rewrites, "\n")) + } + if err := tx.Commit(ctx); err != nil { return fmt.Errorf("commit: %w", err) } - fmt.Printf("\n%d agent(s) published, %d updated, %d skill(s) written, into %s\n", - inserted, updated, skillsWritten, orgSlug) + fmt.Printf("\n%d agent(s) published, %d updated, %d version(s) recorded, "+ + "%d skill(s) written, into %s\n", + inserted, updated, versioned, skillsWritten, orgSlug) return nil }