SER-6025: Add agents commands for the public Agents API - #30
Open
Priyanshu (priyanshu-plivo) wants to merge 14 commits into
Open
SER-6025: Add agents commands for the public Agents API#30Priyanshu (priyanshu-plivo) wants to merge 14 commits into
Priyanshu (priyanshu-plivo) wants to merge 14 commits into
Conversation
Replaces the coming-soon stub with a real command group covering the public Agents API: list/get/create/update/delete, runs list/get, and the node catalogue plus per-node JSON Schema. A flow graph is supplied with --file so a generated JSON document can be piped straight in; flags override file values. delete refuses without --yes, per the destructive-verb contract. Adds --api-url (and PLIVO_API_URL) resolved in getClient(), so the CLI can be pointed at a non-production gateway. Precedence is flag > env > built-in default; the value must parse as an absolute http(s) URL rather than silently falling back, and a non-default base prints a notice to stderr so piped output is never mistaken for production. Verified against a non-production gateway end to end: node catalogue (52 types), per-node schema, create with a graph, read back with nodes and connections intact, rename, runs list, destructive-refusal without --yes, delete, and 404 afterwards. Server-side validation errors surface verbatim rather than being flattened.
Priyanshu (priyanshu-plivo)
changed the title
The published CLI must always target production. A runtime override is a credential-exfil vector: anyone who can get a user to set the env var (a copy-pasted export line, a poisoned shell profile) receives that user's auth_id and auth_token on the next command, with no visible failure. Drops --api-url/PLIVO_API_URL, its resolution in getClient(), the README section, and the tests that existed only to cover it. HTTP-shape coverage for the agents commands moves to internal/api, where a Client's BaseURL is settable directly without exposing a redirect to end users. That is also the package's existing convention for HTTP tests. The command-level tests keep the argument handling that runs before any request. Golden help snapshots and docs/COMMANDS.md regenerated.
Priyanshu (priyanshu-plivo)
changed the title
Agent.ID was tagged `json:"id"` while the API publishes `agent_id`, so `plivo agents list` and `agents get` printed an empty ID column against a correct server. No test decoded a real list payload, so nothing caught it. Also surfaces flow_type, which the API already publishes and which determines which console route can open an agent -- it was being silently dropped. Adds two tests: one decoding a real list payload, one asserting a bare `id` key does NOT populate Agent.ID, so a server-side regression cannot hide behind a CLI that still works.
Building an agent flow through the API has a few rules that are not guessable
and that a 201 will not warn you about. This packages them as a second skill,
distributed the same two ways as the CLI skill (embedded in the binary via
`plivo skill install agents`, or `skills.sh -s agents` with no binary).
The skill is built around discovery rather than a static reference: it tells the
model to read GET /AgentNode/{type}/ for schema + a working example instead of
inventing config, so it cannot drift as nodes change.
It leads with the six rules that actually break flows -- orphan nodes being
excluded from the version, connections addressing nodes by id while template
variables address them by name, endpoints needing <node_id>.<handle>, and node
ids being permanent -- and ships a dependency-free preflight script that checks
all of them offline. The id-vs-name rule is the one worth a script: it saves
cleanly and then resolves to nothing at runtime.
skill install keeps its previous no-argument behaviour exactly, and each skill
installs into its own directory so two files named SKILL.md cannot clobber each
other. --dir and --print are rejected with "all" rather than silently writing
one over the other.
Verified: the worked example creates, publishes and deletes against dev, and
the preflight script accepts valid flows while catching orphans, bare-id
endpoints, id-in-variable, duplicate ids and multiple starts.
A blind test of the skill (fresh agent, skill as its only documentation) built a
working 4-node branching agent first try, but surfaced a real gap: a source
handle that does not exist is accepted with 201, echoed back by GET unchanged,
and publishes to ACTIVE. The branch is permanently dead and nothing reports it.
Confirmed on dev with a handle of 'TOTALLY_MADE_UP'.
Naive validation would be worse than none: 'branch' mints a handle per
config.model.conditions[].alias -- the API's own example wires
'branch-1.vip_caller' -- and those aliases are absent from output_states. So
the preflight now checks handles against the live catalogue UNION the aliases
the node itself defines, and degrades with an explicit note when credentials
are absent rather than pretending to have checked.
That dynamic-handle rule was also missing from the skill entirely, which is a
notable hole in a document that teaches branching.
Other corrections from the same test:
- config.model was described as flat. True on write; http_request returns it
nested under its own type key, and the server injects a 'name' it never sent.
- Publish/Pause/Resume had no status codes while every other row had one.
- The opening 'Contract' line told the reader to compare schema_version against
a value it never gave. Removed.
- Trigger payload fields are not published anywhere, so the skill now says to
read a real run or ask, and names the three paths the examples actually prove
instead of implying any {{Start.*}} path works.
- Added the {{secrets.NAME}} namespace, the four terminal node types, branch
layout coordinates, and a pointer to x-plivo-coverage.
Preflight matrix: 10/10, including the blind test's bogus handle and a valid
condition alias that must NOT be flagged.
Tracks the API changes made after review: agent_id -> agent_uuid, run_id -> run_uuid, resource_uri on every object, Plivo's space-separated timestamp shape, limit clamping rather than rejection, ?types= for fetching several node schemas in one call, and "agent created" as the create message. The substantive change is that node ids and canvas positions are now generated server-side, so the skill no longer tells a caller to invent either. It explains the id rule instead -- <type>_<n>, numbered per type in payload order -- because connections still have to reference those ids, so a caller either predicts them or reads them back. Two of the old rules collapse into that one; the permanence rule survives but now applies to ids the server minted. The preflight had to change with it. It now mints ids using the server's exact rule before running any check, otherwise it would compare connections against ids that do not exist yet and report every node as an orphan -- the same bug this ordering just fixed in core, and the reason the script is worth keeping in step with the backend rather than describing it loosely. Preflight matrix: 13/13, covering the three new id-less cases plus all ten earlier ones (orphans, bare-id endpoints, id-in-variable, duplicate ids, multiple starts, bogus handles, and a valid branch condition alias that must NOT be flagged).
The API moved agent_id -> agent_uuid and run_id -> run_uuid to match Plivo's convention (the suffix follows the value's shape: uuid-valued ids get _uuid, as in message_uuid and profile_uuid; numeric ones get _id, as in app_id). The structs still said agent_id, so `plivo agents list` printed an empty ID column against a correct server. That is the second time this has broken the same way. First the struct said `id` while the API said `agent_id`; now the reverse. Both times the tests passed, because both times I had updated the mocked fixture to assert whatever the struct happened to say -- the test verified the code against itself and confirmed nothing about the wire. So the fixture is now a REAL captured response from the live API, embedded from testdata, and there is a test asserting that NEITHER `id` nor the old `agent_id` can populate Agent.ID -- otherwise a server-side regression could hide behind a CLI that still appeared to work. Also decodes resource_uri, which Plivo publishes on every object. Verified by pointing the real Client at dev and decoding a live list response, not just the fixture: id, name, flow_type and resource_uri all populate.
The public API's resource segments are AgentFlow and AgentFlowNode, so AccountURL now builds those paths. Renames the AgentNode types to match, and updates the bundled agents skill so generated calls target the right paths. No behaviour change beyond the paths: request shapes, response decoding and the agent_uuid / run_uuid field names are unchanged.
…rift left/top were float64 with omitempty, so Go could not distinguish "user wrote 0" from "unset" and stripped a deliberate 0 before the request was built. The server checks `is None` specifically to honour 0, so the struct tags were undoing that. Now *float64: nil omits, &0 sends 0. The captured fixture still said /Agent/ after the rename, and its test only asserted the fields were non-empty -- any string satisfies that, so the exact drift the fixture exists to catch sailed past it. It now asserts the path shape and the trailing slash, and fails if the resource segment moves again. SKILL.md contradicted itself and the code: it correctly said limit>20 is silently clamped, then two hundred lines later listed 400-on-limit>20 as an error case. An LLM following the table would write a retry path that never fires. mergeAgentFlowFile extracts the --file merge block that create and update had verbatim; create still tracks the requested name, update discards it.
|
First proper pass on this one, at 363cc92. The binary itself is in good shape — I checked the things that matter and they hold: the base-URL override is genuinely gone from every reachable path in the public build ( Two things need fixing before this merges though. The bundled skill reinstates the exact vector the binary removed. BASE = os.environ.get("PLIVO_API_BASE", "https://api.plivo.com")and then sends Basic Auth to whatever that resolves to. The PR body's own argument for deleting
One gap that undercuts the feature: there's no Smaller stuff, take or leave:
The |
The bundled skill reinstated the exact vector the binary removed. Its preflight read the base URL from PLIVO_API_BASE and then sent Basic Auth to whatever that resolved to. That argument applies more strongly to the skill than to the CLI, not less: the script is designed to run autonomously under an LLM agent with no human watching the request leave, so a copy-pasted export line, a poisoned shell profile or prompt injection silently redirects live credentials. Now a literal, with a comment saying why it must stay one. docs/COMMANDS.md was stale for the skill-install section, so the repo's own drift gate (make docs plus git diff --exit-code) would have failed CI. Regenerated.
…gents
publish, pause and resume. hodor exposed all three and the bundled skill's own
workflow pivots on Publish, so without them create left you in DRAFT and forced a
drop to raw curl -- the thing this CLI exists to replace. One shared runner: the
three differ only by a path segment and their wording, so three copies would be
duplication. They send no body, which is why core's handler opts into
ALLOW_EMPTY_BODY.
--all was a declared-but-unconsumed root persistent flag, so every command
advertised auto-pagination and none delivered it. Wired for agents list, where the
server clamps limit to 20 and a silently truncated list is the worst outcome. Guards
against spinning if a page comes back empty before total_count is reached.
--explain was advertised on every agents subcommand but only honoured in create;
list and the lifecycle verbs now print their intended request too.
Tests for two branches that had none. An explicit left/top of 0 must reach the wire:
the server checks `is None` so a deliberate 0 is meaningful, and these are *float64
for exactly that reason. Reverting them to plain float64 makes the node marshal to
{"type":"send_message"} with both positions dropped, and nothing else would have
caught it. Also readAgentFlowFile's invalid-JSON branch, which is the likelier
mistake than a missing file and must name the path.
Golden help files and COMMANDS.md regenerated; make docs is idempotent, so the
repo's drift gate passes.
Ashish Rai Plivo (ashishrai-plivo)
approved these changes
Aug 3, 2026
Ashish Rai Plivo (ashishrai-plivo)
left a comment
Ashish Rai Plivo (ashishrai-plivo)
left a comment
There was a problem hiding this comment.
Both must-fixes verified at 8788946. The skill hardcodes https://api.plivo.com with the "hardcoded on purpose" comment, and I swept the whole tree — no env-sourced value feeds a URL that receives credentials anywhere. make docs at head produces zero diff, so the CI gate passes. The lifecycle verbs construct exactly the routes hodor registers (checked against its PR directly, trailing-slash form included), send the documented empty body, and --explain runs before any network call.
One thing I'd close before merge: --all got wired for agents list but not agents runs list. Same flags, same ListMeta/total_count shape, and the flag still shows in that command's help — so a busy agent with hundreds of runs silently truncates to page one under --all, which is the exact bug your commit message describes fixing. The list-side loop is good (advancing by actual objects returned handles the server clamp correctly); it just needs the same treatment one command over. (agents nodes list correctly has no pagination, that omission is fine.)
Smaller, take or leave:
- Nothing tests the lifecycle verbs' execution path — the constructed URL, the empty POST, the response decode. The help-snapshot test proves they're registered, but a route typo would only be caught in production.
registration_test.go's agents list also still reads{"create","list","get","update","delete"}. - The empty-response fallback in
runAgentLifecyclebuilds the message asverb + "d"— fine for pause/resume, but publish becomes "publishd". Pass the past tense in explicitly. agents pausehas no--yesgate. Every other stop-a-live-thing verb in this CLI (voice calls hangup,conferences end,streams stop, maskingsessions end) requires one. Pause is reversible so I can see it either way, but it's a deliberate break from the codebase's own convention — your call, just deciding it on purpose would be good.- The captured-real-response fixture still only covers
Agent/AgentList;AgentRunandAgentFlowNoderemain hand-authored JSON, which is the gap that letagent_uuidbreak twice before. - The
--allloop has no max-iteration cap — a server that ignoresoffsetand returns the same non-empty page loops forever. Unlikely against hodor, cheap to bound.
Also noticed the CI smoke workflow (dry-run URL assertions, destructive-refusal checks) was never extended to any agents subcommand, and its per-group help step still carries the "agent ships as a coming-soon stub" comment. Not this PR's regression, but the largest new command surface in the CLI currently has no binary-level smoke coverage — fine as a follow-up.
…l see it The limitation was only in a core docstring, which is not where a customer-facing person looks before a customer hits it. Two shapes cannot be restored on write and are rejected with 400 rather than guessed: a list of bare secret strings (the api_tokens shape, nothing distinguishes the elements so pairing would be positional and a reorder would swap credentials), and two entries in one list sharing an identity. Spells out the consequence too -- a node holding either shape cannot be saved through a full-graph update at all until the real values are included, even if nothing else changed.
Masking now covers the whole value under a secret-named key, including a nested object's inner fields, so the round-trip note has to say so: an object list with no identity field is as unrestorable as a bare string list.
Rule 8 promised that posting back exactly what you read always works. Verified on dev that it does not: an http_request node's url and headers come back null after an unmodified GET-POST round trip, under a 202. send_message, which has no AS_DICTIONARY rendering group, round-trips losslessly -- so the promise held only for the node types that happen not to nest. Says what actually happens, names the affected types, and gives the workaround until it is fixed server-side.
|
Checked both doc commits against the server's final behaviour at contacto-core#1302's head — the limitation table is accurate on everything load-bearing (unique-identity pairing, the atomic 400, the exact error path, the remediation). Two one-line accuracy nits worth fixing since agents will follow this file literally:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Replaces the coming-soon
agentstub with a real command group for the public Agents API.agents list--limit --offset --name --stateagents get <id>agents create--name --description --file <flow.json>agents update <id>--name --description --file <flow.json>agents delete <id>--yesagents runs list <id>--limit --offsetagents runs get <id> <run_id>agents nodes listagents nodes get <node_type>--filetakes a JSON document holding the flow graph (nodes,connections, optionally name/description) so a generated flow can be piped straight in. Flags override file values.The CLI always targets production
An earlier revision of this branch added a
--api-url/PLIVO_API_URLoverride. It has been removed and will not ship.A runtime base-URL override on a published CLI is a credential-exfil vector: anyone who can get a user to set that env var — a copy-pasted export line, a poisoned shell profile, a "try this" snippet — receives that user's
auth_idandauth_tokenon their next command, with no visible failure. The convenience is not worth it; pointing at a non-production gateway is a local-build concern.Consequence for testing: the agents commands cannot be redirected at runtime, so HTTP-shape coverage lives in
internal/api, where aClient'sBaseURLis settable directly without exposing a redirect to end users. That is also the package's existing convention for HTTP tests. The command-level tests cover the argument handling that runs before any request.API behaviours surfaced deliberately
" 1"/" 2"on a name collision. The command shows the returned name so the user sees what was actually created."<node_id>.<handle>"; a bare node id is rejected by the API. Subaccount credentials get 403.limitis capped at 20.Verification
go build ./...,go vet ./...,gofmtandgo test ./... -raceall clean. Golden--helpsnapshots anddocs/COMMANDS.mdregenerated.The full surface was exercised end to end against a non-production gateway with a locally-built binary before the override was removed: node catalogue (52 types), per-node JSON Schema, create with a graph, read back with nodes and connections intact, rename, runs list, destructive-refusal without
--yes, delete, and 404 afterwards.Note
internal/api/types_internal.go'sAgentis renamed toPhloAgentto free the name for the public type. It sits behind//go:build internaland has no callers.Two
gofmtfindings incmd/voice_streams_*.goare pre-existing and untouched here.