feat: add provider-neutral remote agent command tree#1790
Open
liuxinyanglxy wants to merge 5 commits into
Open
feat: add provider-neutral remote agent command tree#1790liuxinyanglxy wants to merge 5 commits into
liuxinyanglxy wants to merge 5 commits into
Conversation
📝 WalkthroughWalkthroughThis PR introduces a new ChangesAgent CLI Feature
Estimated code review effort: 5 (Critical) | ~150 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@3900974469225baa93d8daec9567ee5e9fc7cd15🧩 Skill updatenpx skills add larksuite/cli#feat/remote-agent -y -g |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1790 +/- ##
==========================================
- Coverage 74.51% 74.50% -0.02%
==========================================
Files 850 877 +27
Lines 86895 90967 +4072
==========================================
+ Hits 64753 67777 +3024
- Misses 17188 17947 +759
- Partials 4954 5243 +289 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (5)
cmd/agent/task.go (1)
482-486: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winSilent truncation writes a corrupt artifact.
io.ReadAll(io.LimitReader(resp.Body, maxArtifactBytes))caps the read but cannot distinguish "body was exactly the cap" from "body exceeded the cap and was truncated". A hostile/oversized artifact larger than 256 MiB is silently truncated and written to disk with a success envelope (exit 0), so the caller believes it downloaded a complete file. ReadingmaxArtifactBytes+1and rejecting when the extra byte is present would surface the truncation instead of persisting a corrupt file.Note this changes the behavior pinned by
TestFetchArtifactURL_LimitEnforced(currently asserts truncate-not-fail), which would need updating.♻️ Detect truncation instead of silently capping
- data, err := io.ReadAll(io.LimitReader(resp.Body, maxArtifactBytes)) - if err != nil { - return nil, errs.NewNetworkError(errs.SubtypeNetworkTransport, "读取产物响应失败: %v", err).WithCause(err) - } - return data, nil + data, err := io.ReadAll(io.LimitReader(resp.Body, maxArtifactBytes+1)) + if err != nil { + return nil, errs.NewNetworkError(errs.SubtypeNetworkTransport, "读取产物响应失败: %v", err).WithCause(err) + } + if int64(len(data)) > maxArtifactBytes { + return nil, errs.NewValidationError(errs.SubtypeInvalidArgument, "产物超过大小上限 %d 字节，拒绝写入截断文件", maxArtifactBytes) + } + return data, nil🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/agent/task.go` around lines 482 - 486, The artifact download logic in fetchArtifactData currently uses io.ReadAll(io.LimitReader(...)) in task.go, which can silently truncate oversized responses and still return success. Update this path to read maxArtifactBytes+1 and detect when the extra byte is present, then return a network error instead of writing a partial artifact; keep the change localized around fetchArtifactData and the callers that persist the downloaded bytes. Also adjust TestFetchArtifactURL_LimitEnforced to expect a failure on over-limit content rather than truncate-not-fail.cmd/agent/list.go (1)
142-153: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winWrap untyped errors from
ListAgentsper cmd-layer error contract.Per coding guidelines, for unclassified lower-layer errors as final output in
cmd/**/*.go, useerrs.NewInternalError(errs.SubtypeUnknown, ...).WithCause(err). Line 152 returns the raw error fromListAgentsdirectly. IfListAgentsreturns an untyped error, it bypasses the typed error contract.Proposed fix
agents, err := p.(iagent.Discoverer).ListAgents(opts.Cmd.Context()) if err != nil { if errors.Is(err, iagent.ErrUnsupported) { return errs.NewValidationError(errs.SubtypeUnsupportedCapability, "provider '%s' 暂不支持列举 agent", opts.Scheme). WithCause(err). WithHint("%s", info.AgentIDSource) } - return err + if errs.IsTyped(err) { + return err + } + return errs.NewInternalError(errs.SubtypeUnknown, "%s", err.Error()).WithCause(err) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/agent/list.go` around lines 142 - 153, The ListAgents error path in cmd/agent/list.go currently returns raw untyped errors from the ListAgents call, which bypasses the cmd-layer typed error contract. Update the error handling in the ListAgents wrapper so that any non-ErrUnsupported failure is converted with errs.NewInternalError(errs.SubtypeUnknown, ...).WithCause(err) before returning, while preserving the existing unsupported-capability mapping for errors.Is(err, iagent.ErrUnsupported). Use the ListAgents branch and the existing errs/iagent symbols to locate the fix.Source: Coding guidelines
cmd/agent/list_test.go (3)
296-308: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard inline
iagent.Registercalls withsync.Once.The
iagent.Register("fakedeps", ...)call at line 298 andiagent.Register("fakedirty", ...)at line 341 are unguarded. If tests are re-run or reordered, duplicate registration panics. Apply the samesync.Oncepattern used byregisterScriptedandregisterFakeUnsup.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/agent/list_test.go` around lines 296 - 308, The inline iagent.Register test helpers are unguarded, so repeated or reordered test execution can trigger duplicate-registration panics. Update the registration paths in TestAgentListScheme_PropagatesIdentity and the similar fake provider setup to use a sync.Once guard, following the pattern used by registerScripted and registerFakeUnsup, and keep the registration logic behind a small helper so each provider is only registered once.
196-199: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
errs.SubtypeUnsupportedCapabilityconstant instead of string literal.Line 197 uses
errs.Subtype("unsupported_capability")whileunsupported_test.goline 83 correctly useserrs.SubtypeUnsupportedCapability. The string literal is brittle if the constant value ever changes.Proposed fix
- if !ok || p.Subtype != errs.Subtype("unsupported_capability") { + if !ok || p.Subtype != errs.SubtypeUnsupportedCapability {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/agent/list_test.go` around lines 196 - 199, The test in list_test.go is hardcoding the unsupported capability subtype as a string literal instead of using the shared constant. Update the assertion around ProblemOf and p.Subtype to compare against errs.SubtypeUnsupportedCapability so the test stays aligned with the canonical value used elsewhere, matching the pattern already used in unsupported_test.go.
249-258: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
registerFakeDisclackssync.Onceprotection unlike other test registrations.
registerScripted(line 142) andregisterFakeUnsup(line 51) both usesync.Onceto prevent duplicate registration panics.registerFakeDisccallsiagent.Registerdirectly. If any test ordering causesregisterFakeDisc()to be called twice,Registerpanics. The same applies to the inlineiagent.Registercalls at lines 298 and 341.Proposed fix
+var registerFakeDiscOnce sync.Once + func registerFakeDisc() { + registerFakeDiscOnce.Do(func() { iagent.Register("fakedisc", iagent.ProviderInfo{ Factory: func(deps iagent.Deps, agentID string) (iagent.Provider, error) { return &fakeDiscProvider{}, nil }, Label: "test fake (discoverer)", AgentRefFormat: "fakedisc:<agent_id>", AgentIDSource: "test only", Kind: iagent.KindCatalog, Identities: []iagent.IdentitySpec{{Type: iagent.IdentityUser}}, }) + }) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/agent/list_test.go` around lines 249 - 258, `registerFakeDisc` and the inline `iagent.Register` test registrations can panic if invoked more than once because they lack `sync.Once` protection. Update `registerFakeDisc` to follow the same pattern as `registerScripted` and `registerFakeUnsup`, and apply the same duplicate-registration guard to the other `iagent.Register` calls in this test file so repeated test execution cannot re-register the same provider.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/agent/card.go`:
- Around line 152-165: The parameter rendering in the card output still writes
`pr.Name` and `pr.Type` directly, so sanitize those fields before printing just
like `pr.Desc`. Update the `CardParam` display logic in `cmd/agent/card.go`
inside the parameter loop to apply `stripANSI` to both the parameter name and
type when building the `fmt.Fprintf` output, keeping the existing required flag
and description handling intact.
In `@cmd/agent/context_test.go`:
- Around line 236-245: `TestContextListWithJq` only verifies that
`agentContextListRun` succeeds, so it can miss regressions in the jq-filtered
output. Update the test to capture stdout from the run and assert the filtered
result is the expected `1`, following the same pattern used by
`TestContextGetWithJq`; use the existing `opts`, `agentContextListRun`, and jq
flag setup to locate the test.
In `@cmd/agent/context.go`:
- Around line 201-234: Add a test for the new pretty-format branch in
agentContextDeleteRun so the `context delete` command’s `--format pretty` output
is covered like the list/get paths. Locate the logic in `agentContextDeleteRun`
and add a case that exercises `opts.Format == "pretty"` with no jq expression,
asserting the human-readable deleted output is printed after a successful
delete.
In `@cmd/agent/list.go`:
- Around line 137-141: Update the factory construction error handling in list.go
so the `info.Factory(...)` failure returns `errs.SubtypeFailedPrecondition`
instead of `SubtypeInvalidArgument`, since the request is already validated and
this is a system-state problem. Also guard the `iagent.Discoverer` cast by
changing the `p.(iagent.Discoverer)` assertion in `ListAgents` to a comma-ok
check and return a handled error if the provider does not implement
`Discoverer`, since `probeDiscoverer` and the real construction path may yield
different concrete types.
In `@cmd/agent/preflight.go`:
- Around line 22-25: Update the header comment in preflight.go so it matches the
actual behavior of preflightScopes: missing scope should be described as a
permission error with exit 3, not a validation error with exit 2. Keep the
wording aligned with the later comment and the preflightScopes behavior/tests,
and reference the permission-error path in preflightScopes so the contract is
accurate.
In `@internal/agent/card.go`:
- Around line 56-69: In NewCard, Identity is assigned directly from
info.Identities and may remain nil, causing JSON to emit null instead of an
empty array like Parameters. Update NewCard in AgentCard construction to
normalize info.Identities to an empty slice when it is nil, so AgentCard always
marshals identity as [] for consistency.
In `@internal/agent/example/state.go`:
- Around line 95-131: The snapshot I/O in memoryStore is using os.ReadFile and
os.WriteFile directly, which bypasses the mockable filesystem abstraction.
Update loadLocked and saveLocked in memoryStore to use the internal/vfs
read/write helpers instead of os access, keeping the existing error handling and
state-loading logic intact.
---
Nitpick comments:
In `@cmd/agent/list_test.go`:
- Around line 296-308: The inline iagent.Register test helpers are unguarded, so
repeated or reordered test execution can trigger duplicate-registration panics.
Update the registration paths in TestAgentListScheme_PropagatesIdentity and the
similar fake provider setup to use a sync.Once guard, following the pattern used
by registerScripted and registerFakeUnsup, and keep the registration logic
behind a small helper so each provider is only registered once.
- Around line 196-199: The test in list_test.go is hardcoding the unsupported
capability subtype as a string literal instead of using the shared constant.
Update the assertion around ProblemOf and p.Subtype to compare against
errs.SubtypeUnsupportedCapability so the test stays aligned with the canonical
value used elsewhere, matching the pattern already used in unsupported_test.go.
- Around line 249-258: `registerFakeDisc` and the inline `iagent.Register` test
registrations can panic if invoked more than once because they lack `sync.Once`
protection. Update `registerFakeDisc` to follow the same pattern as
`registerScripted` and `registerFakeUnsup`, and apply the same
duplicate-registration guard to the other `iagent.Register` calls in this test
file so repeated test execution cannot re-register the same provider.
In `@cmd/agent/list.go`:
- Around line 142-153: The ListAgents error path in cmd/agent/list.go currently
returns raw untyped errors from the ListAgents call, which bypasses the
cmd-layer typed error contract. Update the error handling in the ListAgents
wrapper so that any non-ErrUnsupported failure is converted with
errs.NewInternalError(errs.SubtypeUnknown, ...).WithCause(err) before returning,
while preserving the existing unsupported-capability mapping for errors.Is(err,
iagent.ErrUnsupported). Use the ListAgents branch and the existing errs/iagent
symbols to locate the fix.
In `@cmd/agent/task.go`:
- Around line 482-486: The artifact download logic in fetchArtifactData
currently uses io.ReadAll(io.LimitReader(...)) in task.go, which can silently
truncate oversized responses and still return success. Update this path to read
maxArtifactBytes+1 and detect when the extra byte is present, then return a
network error instead of writing a partial artifact; keep the change localized
around fetchArtifactData and the callers that persist the downloaded bytes. Also
adjust TestFetchArtifactURL_LimitEnforced to expect a failure on over-limit
content rather than truncate-not-fail.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9699694b-cbc4-4e32-a097-3debd13b408b
📒 Files selected for processing (52)
.gitignorecmd/agent/agent.gocmd/agent/agent_test.gocmd/agent/card.gocmd/agent/card_test.gocmd/agent/common.gocmd/agent/common_test.gocmd/agent/context.gocmd/agent/context_test.gocmd/agent/format.gocmd/agent/format_test.gocmd/agent/list.gocmd/agent/list_test.gocmd/agent/next_contract_test.gocmd/agent/preflight.gocmd/agent/preflight_test.gocmd/agent/scripted_provider_test.gocmd/agent/send.gocmd/agent/send_test.gocmd/agent/task.gocmd/agent/task_test.gocmd/agent/unsupported_test.gocmd/build.gocmd/root.goerrs/subtypes.gointernal/agent/agenttest/agenttest.gointernal/agent/card.gointernal/agent/card_test.gointernal/agent/catalog.gointernal/agent/catalog_test.gointernal/agent/contract.gointernal/agent/contract_test.gointernal/agent/example/example.gointernal/agent/example/example_test.gointernal/agent/example/state.gointernal/agent/provider.gointernal/agent/ref.gointernal/agent/ref_test.gointernal/agent/registry.gointernal/agent/registry_test.gointernal/agent/spi.gointernal/agent/state.gointernal/agent/state_test.gointernal/output/envelope.gointernal/output/envelope_test.goskills/lark-agent/SKILL.mdskills/lark-agent/references/lark-agent-card.mdskills/lark-agent/references/lark-agent-context.mdskills/lark-agent/references/lark-agent-list.mdskills/lark-agent/references/lark-agent-send.mdskills/lark-agent/references/lark-agent-task.mdskills/lark-agent/references/providers/lark-agent-example.md
Comment on lines
+152
to
+165
| if len(card.Parameters) > 0 { | ||
| fmt.Fprintln(w, " parameters:") | ||
| for _, pr := range card.Parameters { | ||
| req := "" | ||
| if pr.Required { | ||
| req = " (required)" | ||
| } | ||
| fmt.Fprintf(w, " %s: %s%s", pr.Name, pr.Type, req) | ||
| if pr.Desc != "" { | ||
| fmt.Fprintf(w, " — %s", stripANSI(pr.Desc)) | ||
| } | ||
| fmt.Fprintln(w) | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate CardParam definition and any dynamic/remote card synthesis that fills Name/Type.
fd -e go | xargs rg -nP -C3 '\bCardParam\b'
rg -nP -C3 'CardParam\{' --type=go
# Find providers that build a dynamic AgentCard (non-static synthesis)
rg -nP -C3 'AgentCard\{' --type=goRepository: larksuite/cli
Length of output: 4669
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the card model and the pretty-printer around the reported lines.
sed -n '1,220p' internal/agent/card.go
printf '\n--- cmd/agent/card.go ---\n'
sed -n '120,210p' cmd/agent/card.goRepository: larksuite/cli
Length of output: 5519
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find all assignments/builders for AgentCard.Parameters and CardParam fields.
rg -n --hidden --glob '!**/vendor/**' '\.Parameters\s*=|Parameters:\s*\[\]i?agent\.CardParam|CardParam\{' .Repository: larksuite/cli
Length of output: 1052
Strip ANSI from parameter names and types.
pr.Desc is sanitized, but pr.Name and pr.Type are still written raw. CardParam comes from the card payload, so escape sequences in those fields can reach the terminal.
Suggested fix
- fmt.Fprintf(w, " %s: %s%s", pr.Name, pr.Type, req)
+ fmt.Fprintf(w, " %s: %s%s", stripANSI(pr.Name), stripANSI(pr.Type), req)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if len(card.Parameters) > 0 { | |
| fmt.Fprintln(w, " parameters:") | |
| for _, pr := range card.Parameters { | |
| req := "" | |
| if pr.Required { | |
| req = " (required)" | |
| } | |
| fmt.Fprintf(w, " %s: %s%s", pr.Name, pr.Type, req) | |
| if pr.Desc != "" { | |
| fmt.Fprintf(w, " — %s", stripANSI(pr.Desc)) | |
| } | |
| fmt.Fprintln(w) | |
| } | |
| } | |
| if len(card.Parameters) > 0 { | |
| fmt.Fprintln(w, " parameters:") | |
| for _, pr := range card.Parameters { | |
| req := "" | |
| if pr.Required { | |
| req = " (required)" | |
| } | |
| fmt.Fprintf(w, " %s: %s%s", stripANSI(pr.Name), stripANSI(pr.Type), req) | |
| if pr.Desc != "" { | |
| fmt.Fprintf(w, " — %s", stripANSI(pr.Desc)) | |
| } | |
| fmt.Fprintln(w) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/agent/card.go` around lines 152 - 165, The parameter rendering in the
card output still writes `pr.Name` and `pr.Type` directly, so sanitize those
fields before printing just like `pr.Desc`. Update the `CardParam` display logic
in `cmd/agent/card.go` inside the parameter loop to apply `stripANSI` to both
the parameter name and type when building the `fmt.Fprintf` output, keeping the
existing required flag and description handling intact.
Comment on lines
+236
to
+245
| func TestContextListWithJq(t *testing.T) { | ||
| opts, _ := contextTestOpts(t, "list") | ||
| opts.Cmd.Flags().String("jq", ".data.contexts | length", "") | ||
| setScripted(t, scriptedHooks{listContexts: func() ([]iagent.ContextSummary, error) { | ||
| return []iagent.ContextSummary{{ContextID: "sess_1"}}, nil | ||
| }}) | ||
| if err := agentContextListRun(opts); err != nil { | ||
| t.Fatalf("context list --jq should not error: %v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
# Locate relevant files and inspect the surrounding code.
printf '\n## cmd/agent/context_test.go (relevant sections)\n'
nl -ba cmd/agent/context_test.go | sed -n '220,310p'
printf '\n## Search for jq handling and list run implementation\n'
rg -n "agentContextListRun|jqExpr|Flags\\(\\)\\.Changed|\\.Set\\(\"jq\"|String\\(\"jq\"" cmd/agent -S
printf '\n## Implementation files containing agentContextListRun / jqExpr\n'
fd -a ".*\\.go$" cmd internal | rg "agent|jq"
# If found, print the relevant implementation sections with line numbers.
for f in $(rg -l "func agentContextListRun|jqExpr|Flags\\(\\)\\.Changed|\\.Set\\(\"jq\"|String\\(\"jq\"" cmd internal -S); do
printf '\n## %s\n' "$f"
nl -ba "$f" | sed -n '1,260p'
doneRepository: larksuite/cli
Length of output: 241
🏁 Script executed:
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
paths = [
Path("cmd/agent/context_test.go"),
]
for path in paths:
print(f"\n## {path}")
lines = path.read_text().splitlines()
for start, end in [(220, 310)]:
for i in range(start-1, min(end, len(lines))):
print(f"{i+1:4d}: {lines[i]}")
PY
printf '\n## jq-related symbols\n'
rg -n "agentContextListRun|jqExpr|Changed\\(|String\\(\"jq\"\\)|Set\\(\"jq\"\\)" cmd internal -S
printf '\n## candidate implementation files\n'
fd -a "context.*\\.go$|.*jq.*\\.go$" cmd internalRepository: larksuite/cli
Length of output: 10836
🏁 Script executed:
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
targets = [
Path("cmd/agent/common.go"),
Path("cmd/agent/context.go"),
Path("cmd/agent/context_test.go"),
]
for path in targets:
print(f"\n## {path}")
lines = path.read_text().splitlines()
# Print focused ranges based on known line numbers / symbols.
ranges = {
"cmd/agent/common.go": [(210, 245)],
"cmd/agent/context.go": [(123, 205)],
"cmd/agent/context_test.go": [(235, 245), (277, 297)],
}[str(path)]
for start, end in ranges:
for i in range(start-1, min(end, len(lines))):
print(f"{i+1:4d}: {lines[i]}")
PYRepository: larksuite/cli
Length of output: 6585
Assert the jq output in TestContextListWithJq The test only checks that --jq doesn’t error, so a regression in the filtered result could still pass. Capture stdout and assert the expected 1 (as TestContextGetWithJq does).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/agent/context_test.go` around lines 236 - 245, `TestContextListWithJq`
only verifies that `agentContextListRun` succeeds, so it can miss regressions in
the jq-filtered output. Update the test to capture stdout from the run and
assert the filtered result is the expected `1`, following the same pattern used
by `TestContextGetWithJq`; use the existing `opts`, `agentContextListRun`, and
jq flag setup to locate the test.
Comment on lines
+201
to
+234
| func agentContextDeleteRun(opts *contextOptions) error { | ||
| if !opts.Yes { | ||
| return cmdutil.RequireConfirmation("agent context delete") | ||
| } | ||
|
|
||
| f := opts.Factory | ||
| p, id, err := resolveProvider(f, opts.Cmd, opts.Ref, opts.As) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| // Local scope preflight: after resolveProvider, before the API call. | ||
| if err := preflightScopesForRef(f, id, opts.Ref); err != nil { | ||
| return err | ||
| } | ||
| if err := p.DeleteContext(opts.Cmd.Context(), opts.CtxID); err != nil { | ||
| return convertUnsupported(opts.Ref, "context delete", err) | ||
| } | ||
| // pretty is a human view only; a --jq expression implies structured JSON. | ||
| if opts.Format == "pretty" && jqExpr(opts.Cmd) == "" { | ||
| fmt.Fprintf(f.IOStreams.Out, "context_id: %s\ndeleted: true\n", kvValue(opts.CtxID)) | ||
| return nil | ||
| } | ||
| env := output.Envelope{ | ||
| OK: true, | ||
| Identity: string(id), | ||
| Data: map[string]interface{}{"context_id": opts.CtxID, "deleted": true}, | ||
| Notice: output.GetNotice(), | ||
| } | ||
| if jq := jqExpr(opts.Cmd); jq != "" { | ||
| return output.JqFilter(f.IOStreams.Out, env, jq) | ||
| } | ||
| output.PrintJson(f.IOStreams.Out, env) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Missing test for --format pretty branch on context delete.
Lines 219–221 implement a pretty-format output path for delete, but there is no corresponding test (unlike list and get which both have pretty-format tests). Per coding guidelines, every behavior change needs a test alongside the change.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/agent/context.go` around lines 201 - 234, Add a test for the new
pretty-format branch in agentContextDeleteRun so the `context delete` command’s
`--format pretty` output is covered like the list/get paths. Locate the logic in
`agentContextDeleteRun` and add a case that exercises `opts.Format == "pretty"`
with no jq expression, asserting the human-readable deleted output is printed
after a successful delete.
Source: Coding guidelines
Comment on lines
+137
to
+141
| p, err := info.Factory(iagent.Deps{Client: apiClient, As: id}, "") | ||
| if err != nil { | ||
| return errs.NewValidationError(errs.SubtypeInvalidArgument, "%s", err.Error()).WithCause(err) | ||
| } | ||
| agents, err := p.(iagent.Discoverer).ListAgents(opts.Cmd.Context()) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Consider SubtypeFailedPrecondition for factory construction errors and guard the Discoverer type assertion.
Two concerns:
-
Line 139: The scheme already passed
iagent.Info()validation, so the argument is valid. A factory failing to construct (e.g., missing config) is a system-state issue, not an invalid argument. Per coding guidelines,SubtypeFailedPreconditionis for "valid request has wrong system state." -
Line 141:
p.(iagent.Discoverer)uses a bare type assertion without comma-ok.probeDiscovererconstructs the provider with emptyDeps, while here it's constructed with realDeps. If a factory returns different concrete types based onDeps, this panics at runtime.
Proposed fix
p, err := info.Factory(iagent.Deps{Client: apiClient, As: id}, "")
if err != nil {
- return errs.NewValidationError(errs.SubtypeInvalidArgument, "%s", err.Error()).WithCause(err)
+ return errs.NewValidationError(errs.SubtypeFailedPrecondition, "%s", err.Error()).WithCause(err)
}
- agents, err := p.(iagent.Discoverer).ListAgents(opts.Cmd.Context())
+ disc, ok := p.(iagent.Discoverer)
+ if !ok {
+ return errs.NewValidationError(errs.SubtypeUnsupportedCapability,
+ "provider '%s' 暂不支持列举 agent", opts.Scheme).
+ WithHint("%s", info.AgentIDSource)
+ }
+ agents, err := disc.ListAgents(opts.Cmd.Context())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| p, err := info.Factory(iagent.Deps{Client: apiClient, As: id}, "") | |
| if err != nil { | |
| return errs.NewValidationError(errs.SubtypeInvalidArgument, "%s", err.Error()).WithCause(err) | |
| } | |
| agents, err := p.(iagent.Discoverer).ListAgents(opts.Cmd.Context()) | |
| p, err := info.Factory(iagent.Deps{Client: apiClient, As: id}, "") | |
| if err != nil { | |
| return errs.NewValidationError(errs.SubtypeFailedPrecondition, "%s", err.Error()).WithCause(err) | |
| } | |
| disc, ok := p.(iagent.Discoverer) | |
| if !ok { | |
| return errs.NewValidationError(errs.SubtypeUnsupportedCapability, | |
| "provider '%s' 暂不支持列举 agent", opts.Scheme). | |
| WithHint("%s", info.AgentIDSource) | |
| } | |
| agents, err := disc.ListAgents(opts.Cmd.Context()) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/agent/list.go` around lines 137 - 141, Update the factory construction
error handling in list.go so the `info.Factory(...)` failure returns
`errs.SubtypeFailedPrecondition` instead of `SubtypeInvalidArgument`, since the
request is already validated and this is a system-state problem. Also guard the
`iagent.Discoverer` cast by changing the `p.(iagent.Discoverer)` assertion in
`ListAgents` to a comma-ok check and return a handled error if the provider does
not implement `Discoverer`, since `probeDiscoverer` and the real construction
path may yield different concrete types.
Source: Coding guidelines
Comment on lines
+22
to
+25
| // from the credential cache (keychain), never from the network — so a missing | ||
| // scope surfaces as an actionable validation error (exit 2) instead of a | ||
| // round-trip API 99991679. `--dry-run` never reaches it (dry-run returns before | ||
| // resolveProvider), preserving its always-available contract. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Header comment misstates the exit code / error class.
This comment says the missing scope "surfaces as an actionable validation error (exit 2)", but preflightScopes returns errs.NewPermissionError(errs.SubtypeMissingScope, ...) (exit 3) — consistent with the later comment on lines 60-61 and asserted by preflightScopes tests (preflight_test.go line 51-52). Align the header to avoid misleading the exit-code contract.
📝 Suggested wording fix
-// scope surfaces as an actionable validation error (exit 2) instead of a
+// scope surfaces as an actionable missing_scope permission error (exit 3) instead of a
// round-trip API 99991679. `--dry-run` never reaches it (dry-run returns before📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // from the credential cache (keychain), never from the network — so a missing | |
| // scope surfaces as an actionable validation error (exit 2) instead of a | |
| // round-trip API 99991679. `--dry-run` never reaches it (dry-run returns before | |
| // resolveProvider), preserving its always-available contract. | |
| // from the credential cache (keychain), never from the network — so a missing | |
| // scope surfaces as an actionable missing_scope permission error (exit 3) instead of a | |
| // round-trip API 99991679. `--dry-run` never reaches it (dry-run returns before | |
| // resolveProvider), preserving its always-available contract. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/agent/preflight.go` around lines 22 - 25, Update the header comment in
preflight.go so it matches the actual behavior of preflightScopes: missing scope
should be described as a permission error with exit 3, not a validation error
with exit 2. Keep the wording aligned with the later comment and the
preflightScopes behavior/tests, and reference the permission-error path in
preflightScopes so the contract is accurate.
Comment on lines
+56
to
+69
| func NewCard(scheme, agentID string) *AgentCard { | ||
| info, ok := Info(scheme) | ||
| if !ok { | ||
| panic("agent: NewCard for unregistered scheme: " + scheme) | ||
| } | ||
| return &AgentCard{ | ||
| Provider: scheme, | ||
| ProviderLabel: info.Label, | ||
| AgentID: agentID, | ||
| Identity: info.Identities, | ||
| Parameters: []CardParam{}, | ||
| AgentIDSource: info.AgentIDSource, | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Initialize Identity to an empty slice when nil for JSON consistency with Parameters.
NewCard explicitly initializes Parameters to []CardParam{} so it always marshals as "parameters": [], but Identity is set directly from info.Identities which may be nil. A nil slice marshals as "identity": null, creating an inconsistency: consumers that type-assert identity as an array will break when a provider declares no identities.
🔧 Proposed fix
func NewCard(scheme, agentID string) *AgentCard {
info, ok := Info(scheme)
if !ok {
panic("agent: NewCard for unregistered scheme: " + scheme)
}
+ identity := info.Identities
+ if identity == nil {
+ identity = []IdentitySpec{}
+ }
return &AgentCard{
Provider: scheme,
ProviderLabel: info.Label,
AgentID: agentID,
- Identity: info.Identities,
+ Identity: identity,
Parameters: []CardParam{},
AgentIDSource: info.AgentIDSource,
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func NewCard(scheme, agentID string) *AgentCard { | |
| info, ok := Info(scheme) | |
| if !ok { | |
| panic("agent: NewCard for unregistered scheme: " + scheme) | |
| } | |
| return &AgentCard{ | |
| Provider: scheme, | |
| ProviderLabel: info.Label, | |
| AgentID: agentID, | |
| Identity: info.Identities, | |
| Parameters: []CardParam{}, | |
| AgentIDSource: info.AgentIDSource, | |
| } | |
| } | |
| func NewCard(scheme, agentID string) *AgentCard { | |
| info, ok := Info(scheme) | |
| if !ok { | |
| panic("agent: NewCard for unregistered scheme: " + scheme) | |
| } | |
| identity := info.Identities | |
| if identity == nil { | |
| identity = []IdentitySpec{} | |
| } | |
| return &AgentCard{ | |
| Provider: scheme, | |
| ProviderLabel: info.Label, | |
| AgentID: agentID, | |
| Identity: identity, | |
| Parameters: []CardParam{}, | |
| AgentIDSource: info.AgentIDSource, | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/agent/card.go` around lines 56 - 69, In NewCard, Identity is
assigned directly from info.Identities and may remain nil, causing JSON to emit
null instead of an empty array like Parameters. Update NewCard in AgentCard
construction to normalize info.Identities to an empty slice when it is nil, so
AgentCard always marshals identity as [] for consistency.
Add the `lark-cli agent` command tree: a provider-neutral surface over remote A2A agents. One constant verb set (list / card / send / task / context) routes by agent_ref (<scheme>:<agent_id>) to registered providers; remote agents never grow new top-level commands and their capabilities are declared in a machine-readable card. - SPI (internal/agent): Provider interface, registry with fail-fast registration checks, typed ProviderKind / IdentityType, a closed Capabilities struct, NewCard single-source card synthesis, and the 9-state task machine aligned with A2A. - Command surface (cmd/agent): list / card / send / task / context with default JSON envelopes, meta.next suggested commands, fire + bounded `--watch --timeout` polling, local all-or-nothing scope preflight, and capability gating. Two CLI-enforced high-risk-write confirmations: `send --file` (off-machine upload) needs --yes, and artifact download refuses to clobber an existing -o target without --force; both return confirmation_required (exit 10) before any network/write. Artifact download is SSRF-guarded, https-only and size-capped. - Typed error contract with stable exit codes and codemeta classification. - Ignore local-only proof artifacts (tests_e2e/, tests_skill_eval/, coverage.html).
… harness - StaticCatalog (internal/agent/catalog.go): a framework helper carrying the catalog-provider boilerplate — enumeration, per-agent card lookup and typed unknown-id errors — so catalog providers do not reinvent it. - agenttest.RunConformance: a one-call conformance suite pinning the SPI's implicit contracts (registration metadata, zero-Deps factory, card single source, enumeration stability). - example provider (internal/agent/example): an offline in-memory reference provider (echo / reporter, deliberately different capability matrices) that doubles as the provider-onboarding template and the command tree's zero-network demo backend.
lark-agent skill: a framework-layer SKILL.md (verb contract, task state machine, polling, exit codes) written with provider placeholders, plus per-provider files under references/providers/. Adding a provider means adding one provider file; the framework docs and verb references stay put.
liuxinyanglxy
force-pushed
the
feat/remote-agent
branch
from
53d7f7c to
8f54f9e
Compare
Mirror the events layering: the framework/SPI stays in internal/agent, the concrete business providers move to a top-level agent/ package (agent/example/), and agent/register.go blank-imports each so their init() self-registration runs. cmd/build.go blank-imports the top-level agent package (alongside events); the command layer (cmd/agent) no longer wires providers directly. A test-only blank import keeps the example scheme registered for cmd/agent tests, which exercise example:echo / example:reporter offline.
…ork-gated) Replace the fat 9-method Provider interface with a struct of function fields, mirroring the events KeyDefinition / shortcuts Shortcut convention: a provider wires only the capabilities it supports and leaves the rest nil. This removes the two things every integrator previously had to keep in sync by hand — the Capabilities bool matrix and the per-method ErrUnsupported returns. - Provider is now a struct: core Send/GetTask (mandatory, asserted at Register) plus optional func fields (ListTasks/CancelTask/context trio/DownloadArtifact/ ListAgents) whose presence == support, plus FileInput/InputRequired flags and an optional Describe for per-agent card metadata. - The card capability matrix is DERIVED from which fields are wired (DeriveCapabilities / BuildCard), so declaration and behavior are single- sourced and cannot drift. CatalogEntry drops its Capabilities field. - The command layer gates every optional verb on the nil field and returns a unified unsupported_capability (exit 2) before any network access; the ErrUnsupported sentinel and convertUnsupported are deleted. --file is now capability-gated too (file_input=false ⇒ unsupported before the upload prompt). - The Discoverer interface becomes the ListAgents field; catalog providers must wire it (asserted at Register). example expresses echo's minimal set vs reporter's full set purely by which fields its Factory wires per agent — no bool matrix, no refusal code. Conformance + tests updated to the new shape.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
agent/example/example_test.go (1)
187-191: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssert the error subtype for the unknown-artifact-id case.
All other error-path tests in this file check
prob.Subtype(e.g.,TestReporterCancelTerminalline 230,TestSendGuardsline 262), but this one discards the*errs.Problemwith_and only verifies the error is typed. The comment says "typed validation error" yet the subtype is not asserted. As per coding guidelines, error-path tests must assert typed metadata viaerrs.ProblemOf(category/subtype/param).🔧 Proposed fix
// Unknown artifact id -> typed validation error. if _, err := p.DownloadArtifact(ctx, task.TaskID, "art_nope"); err == nil { t.Fatal("unknown artifact id should return an error") - } else if _, ok := errs.ProblemOf(err); !ok { - t.Fatalf("unknown artifact id should be a typed error, got %T: %v", err, err) + } else if prob, ok := errs.ProblemOf(err); !ok || prob.Subtype != errs.SubtypeInvalidArgument { + t.Fatalf("unknown artifact id should be an invalid_argument typed error, got %T: %v", err, err) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agent/example/example_test.go` around lines 187 - 191, The unknown-artifact-id test in example_test.go only checks that DownloadArtifact returns a typed error via errs.ProblemOf, but it does not verify the expected subtype metadata. Update the assertion in the DownloadArtifact test to inspect the returned errs.Problem and assert its Subtype matches the validation/error subtype used by the other error-path tests in this file, following the pattern in TestReporterCancelTerminal and TestSendGuards. Use the DownloadArtifact and errs.ProblemOf symbols to locate the check and keep the typed metadata assertion aligned with the existing error-path conventions.Source: Coding guidelines
cmd/agent/common.go (1)
94-108: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winWrap
f.NewAPIClient()failures here
f.NewAPIClient()can still surface plain config/credential errors from the factory path, so returning it raw may leak an untyped final error. Wrap it witherrs.NewInternalError(errs.SubtypeUnknown, "failed to create API client: %v", err).WithCause(err).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/agent/common.go` around lines 94 - 108, The resolveProvider flow returns raw factory errors from f.NewAPIClient(), which can leak untyped config or credential failures. Update resolveProvider to wrap the API client creation error with errs.NewInternalError(errs.SubtypeUnknown, "failed to create API client: %v", err).WithCause(err) before returning, while keeping the existing resolveProviderNoClient and iagent.Resolve handling unchanged.
♻️ Duplicate comments (1)
cmd/agent/list.go (1)
138-138: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFactory construction error should use
SubtypeFailedPrecondition, notSubtypeInvalidArgument.The scheme already passed
iagent.Info()validation (line 115), so the argument is valid. A factory failing to construct with a valid scheme is a system-state issue. Per coding guidelines,SubtypeFailedPreconditionis for "valid request has wrong system state." This was previously flagged and the type-assertion concern from the same comment has been addressed, but this subtype classification remains.Proposed fix
p, err := info.Factory(iagent.Deps{Client: apiClient, As: id}, "") if err != nil { - return errs.NewValidationError(errs.SubtypeInvalidArgument, "%s", err.Error()).WithCause(err) + return errs.NewValidationError(errs.SubtypeFailedPrecondition, "%s", err.Error()).WithCause(err) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/agent/list.go` at line 138, The factory construction error in the list flow is misclassified as `SubtypeInvalidArgument` even though `iagent.Info()` already validated the scheme, so this is a system-state failure. Update the error returned from the factory construction path in `list.go` to use `errs.NewValidationError` with `SubtypeFailedPrecondition` instead, keeping the existing error message and `.WithCause(err)` so the failure is categorized correctly.Source: Coding guidelines
🧹 Nitpick comments (1)
internal/agent/provider.go (1)
57-63: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCard capability matrix doesn't distinguish
GetContext/DeleteContextfromListContexts.
MultiTurnis derived solely fromListContexts != nil(perDeriveCapabilitiesininternal/agent/card.go), butGetContext/DeleteContextare independently nil-gated (per their own doc comments here and confirmed byTestContextDeleteUnsupportedGated/TestContextListUnsupportedGated). A provider that wiresListContextsbut notGetContext/DeleteContextwould advertisemulti_turn: trueon its card while still returningunsupported_capabilityforcontext get/context delete— undermining the stated goal that "declaration and behavior are single-sourced and cannot drift."Consider adding explicit
ContextGet/ContextDeletecapability fields (or asserting atRegistertime thatGetContext/DeleteContextare wired wheneverListContextsis) so the machine-readable card accurately reflects what each verb supports.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/agent/provider.go` around lines 57 - 63, The capability mapping in provider registration only uses ListContexts to derive MultiTurn, so the card can claim context support even when GetContext or DeleteContext are missing. Update the capability model and derivation around Provider, DeriveCapabilities, and Register so context get/delete are represented explicitly (or enforce that they must be wired whenever ListContexts is set). Make sure the machine-readable card distinguishes ListContexts, GetContext, and DeleteContext instead of inferring all three from one field.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@agent/example/example_test.go`:
- Around line 187-191: The unknown-artifact-id test in example_test.go only
checks that DownloadArtifact returns a typed error via errs.ProblemOf, but it
does not verify the expected subtype metadata. Update the assertion in the
DownloadArtifact test to inspect the returned errs.Problem and assert its
Subtype matches the validation/error subtype used by the other error-path tests
in this file, following the pattern in TestReporterCancelTerminal and
TestSendGuards. Use the DownloadArtifact and errs.ProblemOf symbols to locate
the check and keep the typed metadata assertion aligned with the existing
error-path conventions.
In `@cmd/agent/common.go`:
- Around line 94-108: The resolveProvider flow returns raw factory errors from
f.NewAPIClient(), which can leak untyped config or credential failures. Update
resolveProvider to wrap the API client creation error with
errs.NewInternalError(errs.SubtypeUnknown, "failed to create API client: %v",
err).WithCause(err) before returning, while keeping the existing
resolveProviderNoClient and iagent.Resolve handling unchanged.
---
Duplicate comments:
In `@cmd/agent/list.go`:
- Line 138: The factory construction error in the list flow is misclassified as
`SubtypeInvalidArgument` even though `iagent.Info()` already validated the
scheme, so this is a system-state failure. Update the error returned from the
factory construction path in `list.go` to use `errs.NewValidationError` with
`SubtypeFailedPrecondition` instead, keeping the existing error message and
`.WithCause(err)` so the failure is categorized correctly.
---
Nitpick comments:
In `@internal/agent/provider.go`:
- Around line 57-63: The capability mapping in provider registration only uses
ListContexts to derive MultiTurn, so the card can claim context support even
when GetContext or DeleteContext are missing. Update the capability model and
derivation around Provider, DeriveCapabilities, and Register so context
get/delete are represented explicitly (or enforce that they must be wired
whenever ListContexts is set). Make sure the machine-readable card distinguishes
ListContexts, GetContext, and DeleteContext instead of inferring all three from
one field.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c3aeccb1-a337-48c9-94cf-f4a75dff09fd
📒 Files selected for processing (26)
agent/example/example.goagent/example/example_test.goagent/example/state.goagent/register.gocmd/agent/agent.gocmd/agent/card.gocmd/agent/common.gocmd/agent/common_test.gocmd/agent/context.gocmd/agent/list.gocmd/agent/list_test.gocmd/agent/register_test.gocmd/agent/scripted_provider_test.gocmd/agent/send.gocmd/agent/task.gocmd/agent/unsupported_test.gocmd/build.gointernal/agent/agenttest/agenttest.gointernal/agent/card.gointernal/agent/card_test.gointernal/agent/catalog.gointernal/agent/catalog_test.gointernal/agent/provider.gointernal/agent/registry.gointernal/agent/registry_test.gointernal/agent/spi.go
💤 Files with no reviewable changes (3)
- cmd/agent/agent.go
- internal/agent/spi.go
- agent/example/state.go
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/agent/card_test.go
- cmd/build.go
- internal/agent/agenttest/agenttest.go
- cmd/agent/card.go
- cmd/agent/send.go
- cmd/agent/list_test.go
- cmd/agent/task.go
- cmd/agent/common_test.go
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
1 participant
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.
Summary
Add the
lark-cli agentcommand tree: a provider-neutral CLI surface over remote A2A agents. One constant verb set (list/card/send/task/context) routes byagent_ref(<scheme>:<agent_id>) to registered providers. Remote agents never grow new top-level commands — their capabilities are declared in a machine-readable card, and the command surface never specializes for any single provider.The branch ships the framework plus an offline in-memory
exampleprovider that doubles as the provider-onboarding reference and a zero-network demo backend.Changes
internal/agent,cmd/agent):Providerinterface, a registry with fail-fast registration checks, typedProviderKind/IdentityType, a closedCapabilitiesstruct, single-sourceNewCard, and a 9-state task machine aligned with A2A. Commands emit default JSON envelopes withmeta.nextsuggestions, fire + bounded--watch --timeoutpolling, local all-or-nothing scope preflight, and capability gating.send --file(off-machine upload) requires--yes; artifact download refuses to clobber an existing-otarget without--force. Both returnconfirmation_required(exit 10) before any network or write. Artifact download is SSRF-guarded, https-only and size-capped.internal/agent/example,catalog.go,agenttest):StaticCataloghelper for catalog providers,agenttest.RunConformanceone-call conformance suite, and theecho/reporterreference agents with deliberately different capability matrices.skills/lark-agent): a framework-layer SKILL.md written with provider placeholders plus per-provider files underreferences/providers/.Test Plan
go build ./...and unit tests acrossinternal/agent/...,cmd/agent,cmd,errs,internal/errclass,internal/outputall pass.agenttest.RunConformancepasses for the example provider (bothechoandreporter).--filerequires--yes;-ooverwrite requires--force; dry-run exempt).agent list/list example/card/send→task get→ multi-turn / artifact download (kind + suggested_name) / cancel gating / error exit codes.Related Issues
N/A
Summary by CodeRabbit
New Features
agentcommand group withlist,card,send,task, andcontextactions.Bug Fixes
Documentation