refactor: extract the agent-spec crate - #54
Conversation
Move the declaration model (`spec`), the KDL parser (`kdl_format`), and the
catalog walk (`discovery`) into `crates/agent-spec`, and have st2 consume it.
No behavior change: `spec.rs` and `kdl_format.rs` move byte-for-byte.
st2 re-exports the crate under the original paths (`st2::spec::…`,
`st2::discovery::…`), so `src/main.rs` and the whole test suite are untouched.
The ~9 in-tree consumers change only `crate::spec::` → `agent_spec::spec::`.
`kdl_format` stays private inside the crate, so the published surface is
exactly `spec` + `discovery` — a catalog reader parses through `discover` and
resolves identity/host the one way the runner does.
`validate` needed the pre-lowering `type` and `identity`, which it read via the
`pub(crate)` `parse_raw_file`. Rather than export the permissive on-disk
`RawSpec`, the crate exposes a narrow `Declared { identity, job_type }` and
`parse_declared`. `RawSpec` stays private and free to change.
The root stays a real package — `flake.nix` reads `package.version` from it, and
a virtual manifest has no `[package]`. `cargoTestFlags` gains `--workspace`
because cargo would otherwise select only `st2` and silently stop gating the
crate's 5 unit tests (149 lib tests before = 144 + 5 after).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
agent-session-id: 8464a883-6285-4cd0-a9ec-3eba09830300
agent-tool: Claude Code
agent-tool-version: 2.1.220
agent-model: claude-opus-5
agent-runtime-profile: /nix/store/i8y8b542cyqi385ywcjw5fvsq24f75v4-coding-agent-runtime-profile/share/coding-agents/profile.json
agent-skills-manifest: /nix/store/i81qxhzlrzcxrrdwpp6i8hagka2gby8y-agent-skills-corpus/share/agent-skills/manifest.json
tooling-profile: dotfiles@unknown-dirty
`#[cfg(doc)]`-importing a type just to satisfy an intra-doc link is a construct this tree uses nowhere else; plain backticks read the same and match the surrounding doc comments. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> agent-session-id: 8464a883-6285-4cd0-a9ec-3eba09830300 agent-tool: Claude Code agent-tool-version: 2.1.220 agent-model: claude-opus-5 agent-runtime-profile: /nix/store/i8y8b542cyqi385ywcjw5fvsq24f75v4-coding-agent-runtime-profile/share/coding-agents/profile.json agent-skills-manifest: /nix/store/i81qxhzlrzcxrrdwpp6i8hagka2gby8y-agent-skills-corpus/share/agent-skills/manifest.json tooling-profile: dotfiles@unknown-dirty
The doc said callers could pair the result positionally with `discover`'s specs for the same file. They cannot: `resolve_spec` returns `Ok(None)` for any node failing `looks_like_spec`, so specs are a subset of parsed nodes. `validate` never pairs positionally, so this is a doc fix only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> agent-session-id: 8464a883-6285-4cd0-a9ec-3eba09830300 agent-tool: Claude Code agent-tool-version: 2.1.220 agent-model: claude-opus-5 agent-runtime-profile: /nix/store/i8y8b542cyqi385ywcjw5fvsq24f75v4-coding-agent-runtime-profile/share/coding-agents/profile.json agent-skills-manifest: /nix/store/i81qxhzlrzcxrrdwpp6i8hagka2gby8y-agent-skills-corpus/share/agent-skills/manifest.json tooling-profile: dotfiles@unknown-dirty
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: baab666aaf
ℹ️ 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".
myobie
left a comment
There was a problem hiding this comment.
Blocking exact head baab666aafce5d7d00564358ff780ec07c93de5e on test-gate coverage.
Cargo.toml:4-5 creates a non-virtual workspace but does not set default-members. Because the root remains a package, cargo metadata reports only st2 in workspace_default_members. The documented complete gate at README.md:301-306 therefore skips the extracted crate entirely:
cargo test --all-targets --all-features -- --list => 293 tests
cargo test --workspace --all-targets --all-features -- --list => 315 tests
parses_full_kdl_service_job present in the first command => 0
parses_full_kdl_service_job present with --workspace => 1
That is 5 crate unit tests plus the 17 tests moved to crates/agent-spec/tests/discovery.rs. flake.nix:97-101 adds --workspace, but --lib --bins still deliberately excludes the 17 moved integration tests, so exact-head hosted Nix CI being green does not cover the parser/lowering regression net.
Please make the documented root gate include both workspace packages (for example with default-members or documented --workspace) and add a hosted/hermetic gate for the extracted discovery integration test target. Also format the changed assertion in crates/agent-spec/tests/discovery.rs:216; cargo fmt --all --check reports it (alongside inherited baseline diffs).
Independent behavior evidence is otherwise green: 149 workspace unit tests; 17 discovery; 29 validate including malformed type = "srvice"; invariants; reconcile/materialize/catalog selection/compile-agent/run; full Hetz native suite including PTY survival and both systemd isolation tests; strict Clippy for workspace lib/bin; agent-spec rustdoc. Hosted Nix check is green for this exact SHA. Full all-target Clippy remains red on the pre-existing unused import at tests/exec_backend.rs:5, not this PR.
API audit: Declared preserves the content identity (including None for path-derived identity) and raw type before lowering; st2::spec / st2::discovery remain re-exported; the crate matches the runner-normative subset of evals/AGENT-SPEC.md. PR #50 will need a small rebase resolution in two src/run.rs import hunks, but no semantic incompatibility was found.
myobie
left a comment
There was a problem hiding this comment.
Superseding CHANGES_REQUESTED review 4806561853: exact head 3cd5d9a8b84d8135233ab1d0d959a92897619cb6 resolves every blocker. The documented root default gate now selects both workspace packages and lists/runs all 315 tests; hosted Nix now gates 149 workspace unit tests plus the 17 moved discovery integration tests; the affected discovery file passes rustfmt. Local full native coverage is green, including validation, invariants, survival, PTY, and both Linux systemd isolation tests; strict applicable Clippy is green. Hosted Nix run 30441991215 / job 90543228907 is green for this exact SHA. Approved.
…2026-07-29-agent-spec-crate # Conflicts: # flake.nix
myobie
left a comment
There was a problem hiding this comment.
Fresh exact-head approval for 8c803e018dfa2e3db58df0babe54eff1eb575ca7 after incorporating current main 85d5cc2cce496e82f889d28bceece7627bae9b49 (PR #56). The sole flake.nix conflict was resolved by retaining PR #56 lifecycle-hook gates and adding PR #54 workspace/discovery coverage. Current-base root coverage is 319/319 (the prior 315 plus four PR #56 units); the exact hermetic selection is green for 153 workspace units, 17 discovery tests, and 14 hook integration tests; full native survival/systemd coverage and strict applicable Clippy are green. Hosted Nix run 30442506571 / job 90544894207 is green for this exact SHA. Approved.
Why
A second reader of the same catalog — in our case a read-only TUI over the agent
tree — has to answer "what agents are declared, on which host, under which
supervisor?". Today its only options are to shell out to
st2 agents --jsonorto re-implement the declaration format.
Neither works:
{identity, status, name, retired, lastActivity, inbox}—supervisor,roleandworkspaceare absent, andhostsurvives only fused into the<host>.<identity>bus id. That is the right shape for a roster, andINVARIANTS.md pins it as a stable wire contract, so "just add fields" is the
wrong fix.
it is
discovery's identity/host precedence (content wins, the path suppliesdefaults, a mismatch warns) and the compact-form lowering that derives the
agentpty and thedingsidecar. A second hand-rolled copy of that driftssilently, and the drift shows up as an agent the TUI names differently than
the runner does.
Extracting the parser makes st2 the reference implementation of the format
rather than one of two copies of it.
What
spec(the model),kdl_format(the KDL parser) anddiscovery(the catalogwalk) move to
crates/agent-spec. st2 depends on the crate and re-exports itunder the original paths, so it is the same code running.
How
src/spec.rsandsrc/kdl_format.rsmove byte-for-byte (gitrecordsboth as pure renames, 0 changed lines).
src/lib.rsre-exportsagent_spec::{spec, discovery}plus the same itemlist it exported before, so
st2::spec::…/st2::discovery::…stillresolve.
src/main.rsis untouched, as is everytests/*.rsexcept the onethat moves (below) — nothing in the suite had to learn a new path.
crate::spec::→agent_spec::spec::.tests/discovery.rsmoves into the crate; its 4st2::references becomeagent_spec::.Two things that are not pure moves, both called out because they are the only
places a reviewer needs to think:
validateneeded the raw shape. It readparse_raw_file(pub(crate)) tosee the
typestring before normalization — that is how a typo'dtype = "srvice"gets caught, since lowering turns anytypeintoJobType::Service. Exporting that would have draggedRawSpec— thedeliberately-permissive on-disk shape — into the public API, which is a semver
liability for a struct whose whole job is to absorb unknown fields.
Instead the crate exposes a narrow view of what a declaration says before
lowering:
RawSpecstays private. Noteidentityhere is the content-declared one(
Nonewhen it came from the path) — that distinction is load-bearing, becausevalidatereports it as the issue's agent and the resolvedAgentSpec.identitywould have silently substituted the path-derived value.
flake.nixgains--workspace(1 line). With a root[package]plus[workspace] members,cargo test --lib --binsselects onlyst2. Verifiedempirically: the crate's 5 unit tests (
spec's duration parser,discovery's 4path_tests) would have silently stopped being gated. 149 lib tests before =144 + 5 after.
The root deliberately stays a real package rather than becoming a virtual
manifest, because
flake.nixreads(fromTOML (readFile ./Cargo.toml)).package.versionand a virtual root has no[package]to read.build.rsandtests/stay at the root too.Explicit non-goals
Kept out on purpose, so this stays reviewable as a move:
status.rs,message.rs,resource.rs,context.rsare not included.These are st2's own conventions, not agent-spec surface — DQ3 in
docs/vrs/spec.mddefers exactly this shape as not yet inAGENT-SPEC.md.Shipping them in a crate called
agent-specwould assert a contract upstreamhas not made.
anyhow→thiserror, norender_onlyon themodel, no splitting delete-on-read. Those change behavior or the error
contract and belong in their own PR.
validate.rs,materialize.rs,run.rs,hooks.rsstay.materializepulls
run+hooks, so the cut stops before it.Verified
nix flake checkgreen — 149 lib/bin unit tests (144st2+ 5agent-spec), the same 149 that run onmain.cargo build --lockedgreen;Cargo.lockregenerated for the new member andcommitted here.
cargo test --test validategreen (29 tests), run in isolation. This isthe one place the PR is not a pure move, and
--lib --binsdoes not reach it,so I ran it deliberately:
tests/validate.rscovers theunknown-typecase(
type = "srvice") that theparse_declaredswap has to keep working, bothas a human issue and in the JSON output.
--test reconcile(14),--test catalog_selection(5),--test message(5),--test message_cli(2),--test compile_agent(4) green — the rest of thesuite that leans on
spec/discovery.cargo test --test invariantsgreen, also run deliberately since--lib --binsdoes not reach it. INVARIANTS.md needs no edit: none of its 15referenced proof files are moved by this PR, and
tests/invariants.rsstaysin the root package, so
CARGO_MANIFEST_DIRstill resolves every row and thechecked >= 20floor still holds.What I did not verify: the remaining
tests/*.rs— the survival, transport,PTY and hook gates. As the
cargoTestFlagscomment already notes, they needjq,/bin/bash, live PTY backends and a systemd--usermanager; my machinedoes not fully provide them and they fail on
maintoo. They are not gated byCI either, so I am not offering them as a signal in any direction. No file they
touch is modified by this PR.
One question for you
Should the crate declare a spec revision — and in which direction? I left the
constant out, and having now read
AGENT-SPEC.mdI think that is right, but forthe opposite reason to the one I first assumed.
That document already pins, in the other direction: it declares itself pinned to
st2
9887b2842222def0838c2cd82e6c24c218f7efa6(0.1.0, source9887b28) anddocuments "the hand-authored KDL accepted at that commit". So the spec names an
st2 revision, not the reverse. A crate-side constant naming a spec rev would
either duplicate that relationship or contradict it, and this repo deliberately
avoids duplicating ledgers.
If you do want the crate to declare conformance, the question is which direction
you intend: the crate naming the st2 commit the spec is pinned to, or the spec
growing its own identifier. For reference,
9887b28is an ancestor ofmain,20 commits back as of this PR — so the two are already drifting, which is the
argument for deciding this deliberately rather than in a refactor.
Happy to drop or reshape either the
DeclaredAPI or the--workspaceline ifyou would rather solve those differently.
Corrections, applied after this description was first posted. Two things in
this section were wrong when originally published, and I would rather flag them
than quietly edit them:
compoundingtech/agent-specexists as a separate repository with adraft README, and built a second question about crate naming on top of that.
No such repository exists. I had seen a local checkout with that name and a
remote configured for it, and took that as proof the remote existed — it is an
unpushed local artifact, and
git remote -vonly reports configured intent.Both the claim and the question it supported are removed.
AGENT-SPEC.md"carries no version,rev or date to point at". That was also wrong — it opens by pinning itself to
an explicit st2 commit. The question above is rewritten against what the
document actually says.
Nothing in the code, the diff, or the "Verified" section depends on either claim;
both were confined to this question. Apologies for the noise.
Posted on behalf of @schickling
agent_nameagent_session_idagent_toolagent_tool_versionagent_runtimeagent_modelruntime_profileskills_manifestworktreemachinetooling_profile