Split the agent route from its durable key: mutable address, optional immutable id - #468
Conversation
|
@codex review This continues #456 (which cannot be reopened after the trim force-push). The branch was rewritten since your last pass: the reserved version-2 reader arms (harness-state, harness-context, sent ledger) and the dead Prior comments about the record version arms are moot. The description explains why they went; the short version is that Worth checking: whether Gates: Posted on behalf of @schickling
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…dress Agent Spec gains the two fields decision 0015 separates from the positional `identity`: an explicit catalog-global immutable `id` and an optional mutable `address`. Both are optional during the DELTA-003 migration window; `id` is required in the target grammar once every live and archived subject is migrated. - `id` is opaque. Its only two producers are UUIDv7 for a new subject and the frozen `<host>.<identity>` bus identity of a migrated legacy subject, and R26 reuses it verbatim as the canonical task ID and therefore as a session socket path component, so validation refuses exactly what would stop being a usable task ID rather than imposing the address grammar. - `address` carries R24's grammar: at most 255 ASCII characters, dotted 1..=63-character segments of lowercase letters, digits, and hyphens, each beginning and ending with a letter or digit. - `effective_address()` falls back to the positional identity, `bus_address()` host-qualifies it, and `effective_id()` answers with the value catalog ID migration freezes, so a partially migrated catalog stays coherent. Declaring either field twice is the shape refusal `identity` and `host` already carry. No writer emits either field yet.
Catalog admission gains the two uniqueness rules R24 states, and the read projections gain the fields R24 and decision 0015 require them to publish. - `dup-id` now keys on the effective agent ID rather than the bus identity, so one check covers a legacy duplicate identity, two explicit ids colliding across hosts (an ID is catalog-global, never per-host), and an explicit id claiming another subject's still-unmigrated frozen bus identity. - `dup-address` is new: effective addresses are unique per resolved logical host among running and suspended subjects. A retired subject releases its address, so it neither claims nor collides. Explicit-vs-explicit and explicit-vs-identity-fallback are the same collision. - Roster and graph JSON append `id`, `address`, and nullable `busAddress`, preserving existing field order and meanings. `identity` keeps its meaning as the positional declaration key and legacy address fallback. - The graph re-keys `id`, `parentId`, `rootId`, and `ancestorIds` onto the effective agent ID so one namespace spans a partially migrated catalog; for an unmigrated subject that value is its bus identity, so output is unchanged. Archived-subject ID uniqueness joins these checks with the migration verb.
`id` and `address` are two of the most common keys in arbitrary JSON/TOML, and this predicate exists to keep such files out of the spec plane. Admitting them turned a stray file beside a real declaration into a phantom agent — in the roster and in `catalog graph` — and, because such a file carries no launch, also stopped the catalog from admitting at all. Nothing needs the two arms: every real declaration carries an identity, a driver block, a launch, or `type = "service"`, and migration adds `id` to declarations that are candidates for other reasons. agent-identity: dev3.direct.omp.v6c4mkm2 agent-persona: generalist agent-supervisor: unavailable agent-tool: OMP agent-tool-version: 18.1.2 agent-runtime: OMP 18.1.2 tooling-profile: dotfiles@7534055
The previous grammar admitted every printable ASCII byte except `/`, `\\`, and `:` — so backtick, `$`, `;`, `&`, `|`, `*`, `?`, and quotes were all legal in a value R26 reuses verbatim as the canonical task ID, as a session socket path component, and in shell-adjacent bus text, where a backtick has already executed a command on a live host (schickling/dotfiles#1614). The grammar is now `[A-Za-z0-9._-]`. It stays wider than the address grammar on purpose: a frozen legacy ID keeps the case and underscores its identity carried, so freezing an admissible identity can never be refused here. No writer emits `id` yet, so this narrows an unproduced namespace. agent-identity: dev3.direct.omp.v6c4mkm2 agent-persona: generalist agent-supervisor: unavailable agent-tool: OMP agent-tool-version: 18.1.2 agent-runtime: OMP 18.1.2 tooling-profile: dotfiles@7534055
Two asymmetries between the `dup-id` and `dup-address` rules: - `dup-address` keyed on the resolved logical host while `dup-id` keyed on an empty placeholder, so under `--host h` a host-less declaration and an explicit `host "h"` one — one physical subject — were reported only under the address code. Both rules now read the same host key. - Suppressing the address check for a declaration already refused for a duplicate ID also left that declaration's address unregistered, so a third subject could claim it undetected — including through `st2 agent address`, whose gate re-runs exactly this rule. Both keys now register in one pass before either is reported, and one physical conflict is still one diagnostic. agent-identity: dev3.direct.omp.v6c4mkm2 agent-persona: generalist agent-supervisor: unavailable agent-tool: OMP agent-tool-version: 18.1.2 agent-runtime: OMP 18.1.2 tooling-profile: dotfiles@7534055
9d313ab to
b99b063
Compare
Reopens the address-only lane. #456 carried this branch and cannot be reopened through the API (its
recorded head no longer exists after the trim force-push), so this PR continues it with the same head
branch and the same base.
Read #456 for the review history. What changed since its last review: the version-2 reader arms
(harness-state, harness-context, sent ledger) and the dead
DeclaredAgent::id()/::address()accessors are gone — 234 lines. What remains is the identity/address model only.
Coordination. #456 and #457 were closed at 15:18 UTC in favour of #464, which implements the whole
ID/address program as one 12.5k-line change. That is option B of decision
Q23— the option theanswer of record rejected: *"Land st2#456; cherry-pick set_address/refuse_address_collision/address_cmd
[deferred] Activate the immutable-id identity model behind a catalog migration gate #463 as deferred"* (answered by @schickling;
Q24then chose trim-then-two-PRs,Q25amend-0015-stage-id). This PR and feat(agent): mutable address, immutable positional identity #467 execute that answer. Implement immutable agent IDs and mutable addresses coherently #464 is untouched and needs a human call
before either lane merges; the two overlap the same contract completely.
Problem
Decision 0015 and root
requirements R19, R24-R26 were accepted as contract in #453, with
DELTA-003 left open because nothing
implements them.
The load-bearing half of that problem statement is one string doing two jobs: positional
identityis simultaneously the human route and the durable key (
ST_AGENT, task IDs, session socket paths,declaration-parent state, supervisor edges). So a provisional semantic route —
help-key.verifier—becomes permanent, and refining it either strands routing or creates a new subject.
Splitting the two needs one new field. This PR adds the grammar, the resolution accessors, the
uniqueness rules, and the read projections for it. No writer emits either field yet.
Change
AgentSpecgains an optionaladdress(mutable route) and an optionalid(immutable catalog-global key), with KDL/TOML/JSON lowering parity and a once-only shape refusal
matching
identityandhost(crates/agent-spec/src/spec.rs,kdl_format.rs,declared.rs).validate_agent_addressis R24's grammar verbatim (≤255 ASCII bytes, dotted1..=63-byte lowercase alphanumeric/hyphen segments);
validate_agent_idis the closed safe set[A-Za-z0-9._-]with no leading or trailing dot — wider than the address grammar so a frozenlegacy ID keeps its identity's case and underscores, and narrow enough that no shell
metacharacter, separator, or non-ASCII byte can enter a value R26 reuses as a task ID, a socket
path component, and shell-adjacent bus text (
crates/agent-spec/src/spec.rs).effective_address()falls back to the positional identity,bus_address()host-qualifies it,effective_id()answers with the value a later ID migrationwould freeze — so a partially migrated catalog stays coherent and an unmigrated one is unchanged.
dup-addressis new: effective addresses are unique per resolved logical host amongrunning and suspended subjects; retirement releases the address.
dup-idkeys on the effectiveagent ID. Both rules read one host key and register in one pass, so they judge the same physical
subject and a declaration refused for a duplicate ID still claims its address.
Explicit-vs-explicit and explicit-vs-identity-fallback are the same collision (
src/validate.rs).id,address, and nullablebusAddress, preservingexisting field order and meanings (
src/agents.rs,src/catalog_graph.rs).Why the version-2 reader arms are gone
This PR previously also reserved version-2 readers for harness-state, harness-context, and the sent
ledger. They were dropped after an audit of the whole three-PR stack, and after the decision to ship
the mutable address first and stage the immutable-id half behind observed triggers (recorded as
0015 Amendment 1in the follow-up PR). Reasons, in order of weight:agent/from/tothat arrives with the ID writers. With those deferred, the arms guard recordversions nothing in the program produces.
these records.
crates/st2-wire/src/lib.rs:18-23documents the opposite as policy ("No type hereuses
deny_unknown_fields"), and the new sent-row fields wereskip_serializing_if. The singlegenuine cross-build hazard is routing, and it is caused by
address, not by a record version:an old binary routes
verifierand refuseskeymap.verifierwhere a new one does the inverse. Sothe remaining reader-first obligation is "deploy an
address-reading build everywhere beforeauthoring any address" — a single-field requirement this PR alone satisfies.
commit's own test asserted exactly that), and
record.agentis written but never read for anydecision:
read_raw_atdoes not look at it.SentRecordisdeny_unknown_fields,src/message.rs:38), which is precisely why it belongs in the PR that addsthat writer, not here.
Also dropped:
DeclaredAgent::id()/::address(), which had no production caller in this PR or inthe writers branch —
src/agent_author.rswalks KDL through its ownexact_agent_node.Net effect of the trim: −222 lines of reader arms and −12 lines of dead ring-1 surface; what remains
is the identity/address model itself (~360 production lines) plus its proof.
Review round 2 — the independent review's findings
Three commits on top of the reviewed head;
git diffagainst it is 3 files of production code and3 of tests.
Blocking: a lone
idoraddressno longer makes a file a spec candidate(
crates/agent-spec/src/spec.rs). They are two of the most common keys in arbitrary JSON/TOML, andlooks_like_specexists to keep such files out of the spec plane. Admitting them turned a stray filebeside a real declaration — a dropped GitHub payload, a task cache — into a phantom agent in the
roster and in
catalog graph, and, because such a file carries no launch, stopped the catalog fromadmitting at all. Both arms are deleted; nothing needed them, since every real declaration carries an
identity, a driver block, a launch, or
type = "service", and migration addsidto declarationsthat are already candidates. The reviewer's control case is now a test: a valid
agents/h/root/agent.kdlplus a
notes.tomlholding onlyid = "gh-123"discovers exactly one agent, with no errors.The
idgrammar is the closed safe set. It admitted every printable ASCII byte except/,\,and
:— so backtick,$,;,&,|,*,?, and quotes were legal in a value R26 reusesverbatim as the canonical task ID and as a session socket path component, and which travels through
shell-adjacent bus text (the class of schickling/dotfiles#1614). Now
[A-Za-z0-9._-]. It stays widerthan the address grammar deliberately: a frozen legacy ID keeps whatever case and underscores its
identity carried, so freezing an admissible identity can never be refused here. No writer emits
id,so this narrows an unproduced namespace.
One registration pass over both duplicate keys (
src/validate.rs). Two asymmetries the reviewfound:
dup-addresskeyed on the resolved logical host whiledup-idkeyed on an empty placeholder, sounder
--host ha host-less declaration and an explicithost "h"one — one physical subject —were reported only under the address code. Both rules now read the same host key.
declaration's address unregistered, so a third subject could claim it undetected — including
through
st2 agent address, whose gate re-runs exactly this rule. Both keys now register beforeeither is reported, and one physical conflict is still one diagnostic.
Each fix ships with a test that fails on the reviewed head and passes here (verified by reverting the
source hunks and re-running:
a_lone_id_or_address_is_not_a_spec_candidate,the_id_grammar_admits_uuidv7_and_frozen_bus_identities_only,a_host_less_and_an_explicit_host_declaration_collide_as_one_duplicate_id,a_declaration_refused_for_a_duplicate_id_still_claims_its_address— 4 failed before, all pass now).Gates and evidence
cargo fmt --checkdisagrees with committed code in 40+ untouched files, so formatting is leftalone; the lines this PR adds are rustfmt-shaped (verified per-hunk against the pre-change drift
set).
cargo clippy --workspace --all-targetsreports no finding in the changed surface; its 2errors are pre-existing in
src/resync.rs(from_mode(0)in two tests, byte-identical onorigin/main).Behaviour on an existing catalog is unchanged: with no declaration carrying either field,
effective_idanswers the bus identity andeffective_addressanswers the positional identity, soroster, graph, and admission output are byte-identical to
main.Intentionally left
ST_AGENT, default task IDs, PTY presentation tags, message provenance,supervisor edge keys, and runtime ownership are untouched by this PR.
st2 agent addressauthoring and address resolution. The stacked follow-up PR addsagent_author::set_address,st2 agent address,AgentSelector, the fail-closed bare-or-qualifiedidentity::resolve_address, and the recipient-resolution cutover toeffective_address().st2 catalog migrate-ids, UUIDv7 creation, ID-keyed durable records,and the activation gate — is deferred behind named triggers (a completed cross-host seat move, a
live/archive identity collision, or a UUIDv7 creation call site). [deferred] Freeze every legacy subject's immutable agent id in one catalog transaction #457 and the remainder of [deferred] Activate the immutable-id identity model behind a catalog migration gate #463
are closed as deferred with their branches kept;
0015 Amendment 1records the staging and the twodefects that must be fixed before that half lands (
supervisor_chain::resolve_specmust accepteffective_id;migrate-idsmust exemptagent-id-missingfrom its own pre-admission gate).endpoint keep their
// DELTA-003:notes: both need the deferred migration's combined index anddurable collision metadata.
GraphArchivedaddress fields. The archived row is projected verbatim from a tombstone that hasno address field, and an archived subject is non-routable by definition.
Posted on behalf of @schickling
agent_identitysessionagent_personaagent_supervisoragent_toolagent_tool_versionagent_runtimetooling_profile