feat(request): add idempotent service principals - #134
Conversation
|
Nathan and CoS like this direction. Declared service principals, typed request/reply, and caller-supplied idempotency fit the product direction. When you consider this ready for review and the final checks are green, we would be comfortable merging it. |
agent-session-id: dev3.dotfiles-cos-misc-issue-1329-upstream agent-tool: Codex agent-tool-version: 0.145.0 agent-model: gpt-5.6-sol agent-runtime-profile: /nix/store/qlk5xbdfmj5nn1q145j0fx73f2pmdjj7-coding-agent-runtime-profile/share/coding-agents/profile.json agent-skills-manifest: /nix/store/2km8dbiyv3wc484l1hd8n2lh65121qvq-agent-skills-corpus/share/agent-skills/manifest.json tooling-profile: dotfiles@unknown-dirty
89bae87 to
d8d8f21
Compare
The request examples and the principal test fixture named a specific deployment's host and service. Replace them with neutral placeholders (`host-a`, `example-ci`) matching the convention the surrounding docs already use for illustrative values, such as `github-issue://example/project/123`. Example strings and one test fixture name only: no behavioural, API, or design change. `cargo test --test request_cli` passes unchanged, 7/7. agent-tool: Claude Code agent-tool-version: 2.1.220 agent-runtime: Claude Code 2.1.220 agent-session-lookup: unavailable tooling-profile: dotfiles@unknown-dirty
|
Rebased onto current
The request examples in Example strings and one test fixture identifier only: no behavioural, API, or design change. Rebase notes Three conflicts, all reconciled toward
The rebased branch's diff against Posted on behalf of @schickling
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a78045f8d9
ℹ️ 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".
| let spec = discovered | ||
| .specs | ||
| .into_iter() | ||
| .find(|spec| spec.bus_id(this_host) == identity || spec.identity == identity) |
There was a problem hiding this comment.
Reject nonlocal or ambiguous bare agent identities
When a catalog contains the same identity on multiple hosts—or only a remote agent with that identity—a bare recipient such as worker selects the first discovered spec regardless of this_host. This contradicts the CLI's local-bare-identity contract and can publish a request to the wrong host's canonical inbox; bare matches should be restricted to the local resolved host and rejected when ambiguous.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed at a78045f8d9583cbc, and the decisive evidence is inside this file rather
than in the CLI contract.
resolve_agent matches with:
.find(|spec| spec.bus_id(this_host) == identity || spec.identity == identity)
The second disjunct accepts a bare identity from ANY host and find takes the first in
discovery order, so a bare recipient can resolve to a remote host's declaration.
What makes this a defect rather than a deliberate looseness: resolve_principal, forty
lines above in the same file, does the same job correctly —
principal.bus_id() == identity
|| (principal.host == this_host && principal.identity == identity)
Same shape, same file, and it gates the bare form on this_host. Two resolvers
disagreeing about whether a bare identity is host-local is an inconsistency one of them
has to lose, and the one WITHOUT the host gate is the one that can publish to another
host's inbox.
Ambiguity is the second half and is not covered either way: find returns the first
match and never reports that a second existed, so a genuinely ambiguous bare identity
resolves silently instead of erroring.
What would have shown me wrong, and does not hold: bus_id(this_host) already being
host-qualified for every spec, which would make the second disjunct unreachable — it is
not, it is a distinct field comparison and it is reachable exactly when the identity is
bare.
One bound I will state rather than let stand as implied evidence: I checked a live
catalog for the same identity declared on two hosts and found none, but that catalog
declares a single host, so it could not have exhibited the condition regardless. That
measurement is not support for the finding and I am not offering it as such. The source
asymmetry is the argument.
Not resolving: no fixing commit exists yet, and I do not resolve a thread whose fix I
have not verified myself.
| for directory in [principal.inbox(), principal.archive()] { | ||
| for candidate in message::list_dir(&directory)? { |
There was a problem hiding this comment.
Propagate reply-directory read failures from status
When either the principal inbox or archive is unreadable (for example because of permissions or a transient filesystem error), message::list_dir converts every read_dir error into an empty list, so this status path reports pending even if a reply exists. A durable workflow cannot distinguish an unobserved reply from a genuinely absent one; status should treat only NotFound as empty and propagate other observation failures.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed at a78045f8d9583cbc. The swallow is at message.rs:291-294:
let entries = match fs::read_dir(dir) {
Ok(e) => e,
Err(_) => return Ok(msgs), // no dir yet → empty
};
The comment says "no dir yet", which is the NotFound case and is the right thing to
treat as empty. The match arm is Err(_), so it also absorbs PermissionDenied, EIO,
ENOTDIR and anything transient. The intent in the comment is narrower than the code.
The consequence at the call site is what makes it worth fixing rather than noting: the
status path iterates the inbox and archive to find a reply, so an unreadable directory
yields zero candidates, no reply matches, and the caller is told pending. That is the
one answer a durable workflow must be able to trust, and it is indistinguishable here
from a genuine absence — the failure is silent and it reports the optimistic side.
Narrowing Err(_) to e if e.kind() == ErrorKind::NotFound and propagating the rest
preserves the documented intent exactly and changes behaviour only in the cases the
comment never claimed to cover.
What would have shown me wrong: a check upstream of list_dir that establishes the
directory is readable, which would make the arm unreachable in this path. There is
none — status calls it directly on both directories.
Not resolving: no fixing commit exists yet, and I do not resolve a thread whose fix I
have not verified myself.
|
Please hold this one before merging. Two review-thread findings on this head are confirmed at source, and one of them is a correctness bug with cross-host reach. Raising it at top level because a thread comment is easy to miss at merge time — the detail is on the two threads. The one that matters:
|
| field | value |
|---|---|
agent_name |
unknown |
agent_tool |
Claude Code |
agent_tool_version |
2.1.220 |
agent_runtime |
Claude Code 2.1.220 |
agent_session_lookup |
unavailable |
tooling_profile |
dotfiles@unknown-dirty |
agent-tool: Codex agent-tool-version: 0.145.0 agent-runtime: Codex 0.145.0 agent-session-lookup: unavailable tooling-profile: dotfiles@unknown-dirty
agent-tool: Codex agent-tool-version: 0.145.0 agent-runtime: Codex 0.145.0 agent-session-lookup: unavailable tooling-profile: dotfiles@unknown-dirty
|
The two fixes described in issuecomment-5228502774 are now on the PR head ( |
Why
Long-running services need durable agent request/reply without impersonating an Agent Spec identity or relying on orphan inbox layout. Retries must not duplicate work.
What
Adds declared non-agent service principals and an idempotent typed request/reply command family with canonical routing and JSON status.
How
Caller-supplied idempotency keys are published atomically and reused across retries. Principal/Agent Spec collisions, envelope forgery, conflicting key reuse, and non-canonical routing fail closed.
Validation
Whole-repo Clippy retains two pre-existing unrelated lints in eval_run paths.