feat: add provider-neutral remote agent command tree#1790
Open
liuxinyanglxy wants to merge 24 commits into
Open
feat: add provider-neutral remote agent command tree#1790liuxinyanglxy wants to merge 24 commits into
liuxinyanglxy wants to merge 24 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces a complete ChangesAgent CLI feature
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as lark-cli agent send
participant Common as resolveSpec/preflight
participant Runtime as cmdRuntime
participant Provider as AgentSpec.Send
User->>CLI: agent send <ref> --text ...
CLI->>Common: resolveSpec(ref, identity)
Common-->>CLI: Provider, AgentSpec
CLI->>Common: parseAndValidateParams(card)
CLI->>Common: preflightScopesForScheme()
Common-->>CLI: nil or missing_scope
CLI->>Runtime: runtimeFor(identity)
CLI->>Provider: Send(ctx, rt, SendInput)
Provider-->>CLI: AgentTask
CLI->>CLI: normalizeTask + nextForTask
CLI-->>User: Envelope{task, meta.next}
sequenceDiagram
participant User
participant CLI as lark-cli agent task get --watch
participant Provider as AgentSpec.GetTask
participant Poll as pollToStop
User->>CLI: agent task get <ref> <task_id> --watch
CLI->>Provider: GetTask(ctx, rt, taskID)
Provider-->>CLI: AgentTask (working)
CLI->>Poll: pollToStop(ctx, GetTask)
loop until stop condition
Poll->>Provider: GetTask(ctx, rt, taskID)
Provider-->>Poll: AgentTask
Poll->>Poll: ShouldStopPolling()?
end
Poll-->>CLI: final AgentTask
CLI->>CLI: semanticExitError(task)
CLI-->>User: Envelope + exit code
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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@63f78c18f710d210705eb685ee4c995709337c1e🧩 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.58% 75.19% +0.61%
==========================================
Files 861 924 +63
Lines 89971 98438 +8467
==========================================
+ Hits 67106 74024 +6918
- Misses 17669 18744 +1075
- Partials 5196 5670 +474 ☔ 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
+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
+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.
liuxinyanglxy
force-pushed
the
feat/remote-agent
branch
from
July 8, 2026 03:49
53d7f7c to
8f54f9e
Compare
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
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/agent/task_test.go (1)
81-88: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the full typed error payload in these tests.
cmd/agent/task_test.go:81-88, 739-743, 871-872still stop atIsValidation, message text, or subtype-only checks. Useerrs.ProblemOffor category/subtype,errors.Asfor*errs.ValidationError.Paramwhere applicable, anderrs.SubtypeUnsupportedCapabilityinstead of the string literal; keep a cause/unwrap check when the error is wrapped.🤖 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_test.go` around lines 81 - 88, Update the task tests to assert the full typed error payload instead of only checking validation, message text, or subtype strings. In the relevant assertions around the task error handling, use errs.ProblemOf for category/subtype, switch to errs.SubtypeUnsupportedCapability instead of the hardcoded unsupported_capability literal, and add errors.As checks for *errs.ValidationError.Param where needed. Also preserve a cause/unwrap assertion for wrapped errors so the tests verify the underlying typed error chain.Sources: Coding guidelines, Learnings
🧹 Nitpick comments (1)
cmd/agent/context.go (1)
154-157: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winLexicographic sort on
UpdatedAtcan mis-order timestamps.Comparing
UpdatedAtas raw strings only produces correct chronological order if every provider emits a fixed-width, same-offset RFC3339 string. A timestamp with fractional seconds (e.g....:00.5Z) sorts before one without (...:00Z) because.<Zlexicographically, and mixed UTC/+08:00offsets aren't comparable as strings at all. SinceUpdatedAtcomes from third-party/remote providers, this is a real (if edge-case) correctness risk for "newest-first" ordering.Proposed fix
+import "time" + sort.SliceStable(contexts, func(i, j int) bool { - return contexts[i].UpdatedAt > contexts[j].UpdatedAt + ti, ei := time.Parse(time.RFC3339, contexts[i].UpdatedAt) + tj, ej := time.Parse(time.RFC3339, contexts[j].UpdatedAt) + if ei != nil || ej != nil { + return contexts[i].UpdatedAt > contexts[j].UpdatedAt // fallback + } + return ti.After(tj) })🤖 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 154 - 157, The newest-first ordering in the context sorting logic uses raw string comparison on UpdatedAt, which can mis-order RFC3339 timestamps from different providers. Update the sort in the context sorting code to parse UpdatedAt into real time values before comparing, and keep the stable sort behavior so equal timestamps preserve provider order and missing/invalid timestamps still sort last.
🤖 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/list_test.go`:
- Around line 379-411: `TestAgentListScheme_OnlineChecksIdentity` only checks
the validation category, so tighten it to assert the exact typed problem
metadata too. After `agentListRun(opts)` fails, use `errs.ProblemOf` on the
returned error and verify the expected `Subtype` (and `Param` if applicable),
matching the style used by the sibling preflight test. Keep the existing checks
that the call is blocked and that the error is a validation error, but add the
typed assertion so the test proves the specific identity-whitelist failure path.
In `@cmd/agent/list.go`:
- Around line 120-126: The unknown agent provider validation in list should name
the failing input. Update the validation error built in
`iagent.Info(opts.Scheme)`’s `!ok` branch to chain `.WithParam(...)` with the
corresponding user-facing argument/flag for `opts.Scheme`, keeping the existing
`errs.NewValidationError(errs.SubtypeInvalidArgument, ...)` and hint intact.
In `@cmd/agent/runtime.go`:
- Around line 58-62: The `runtime.go` file-open failure in `vfs.Open` is being
reported as a validation issue, but this path should be treated as an internal
file I/O problem after `validate.SafeInputPath` succeeds. Update the error
handling in the `Open`/`resolved` flow to return
`errs.NewInternalError(errs.SubtypeFileIO, ...)` instead of
`errs.NewValidationError(...)`, and keep attaching the underlying cause so
callers can distinguish I/O failures from bad input.
In `@cmd/agent/task_test.go`:
- Around line 72-74: The direct cmdutil.TestFactory-based tests are using shared
CLI config state, which can leak into developer or CI environments. In
TestTaskCancelUnsupportedGated and the other affected *_test.go cases, set
LARKSUITE_CLI_CONFIG_DIR to a temp directory with t.Setenv before calling
cmdutil.TestFactory so any config reads/writes stay isolated. Use the test names
and the cmdutil.TestFactory setup as the place to apply the env override
consistently across the mentioned tests.
- Around line 727-735: The http-rejection test currently only checks the
returned error, so it can still pass even if fetchArtifactURL consults an HTTP
client before rejecting the URL. In TestFetchArtifactURL_HttpRejected, install a
fail-fast HttpClient seam on the factory returned by cmdutil.TestFactory so any
transport attempt panics or fails immediately, then call fetchArtifactURL and
assert the http:// URL is rejected without invoking the client. Use the
fetchArtifactURL and HttpClient hooks to make the no-request guarantee
executable.
---
Outside diff comments:
In `@cmd/agent/task_test.go`:
- Around line 81-88: Update the task tests to assert the full typed error
payload instead of only checking validation, message text, or subtype strings.
In the relevant assertions around the task error handling, use errs.ProblemOf
for category/subtype, switch to errs.SubtypeUnsupportedCapability instead of the
hardcoded unsupported_capability literal, and add errors.As checks for
*errs.ValidationError.Param where needed. Also preserve a cause/unwrap assertion
for wrapped errors so the tests verify the underlying typed error chain.
---
Nitpick comments:
In `@cmd/agent/context.go`:
- Around line 154-157: The newest-first ordering in the context sorting logic
uses raw string comparison on UpdatedAt, which can mis-order RFC3339 timestamps
from different providers. Update the sort in the context sorting code to parse
UpdatedAt into real time values before comparing, and keep the stable sort
behavior so equal timestamps preserve provider order and missing/invalid
timestamps still sort last.
🪄 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: 92fe59ae-99de-4198-8d36-64ca1d460de8
📒 Files selected for processing (39)
agent/example/example.goagent/example/example_test.goagent/example/state.goagent/register.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/preflight.gocmd/agent/preflight_test.gocmd/agent/register_test.gocmd/agent/runtime.gocmd/agent/runtime_test.gocmd/agent/scripted_provider_test.gocmd/agent/send.gocmd/agent/task.gocmd/agent/task_test.gocmd/agent/unsupported_test.goerrs/subtypes.gointernal/agent/agenttest/agenttest.gointernal/agent/card.gointernal/agent/card_test.gointernal/agent/contract.gointernal/agent/contract_test.gointernal/agent/provider.gointernal/agent/registry.gointernal/agent/registry_test.gointernal/agent/runtime.goskills/lark-agent/SKILL.mdskills/lark-agent/references/lark-agent-card.mdskills/lark-agent/references/lark-agent-context.mdskills/lark-agent/references/lark-agent-task.mdskills/lark-agent/references/providers/lark-agent-example.md
✅ Files skipped from review due to trivial changes (2)
- skills/lark-agent/references/providers/lark-agent-example.md
- skills/lark-agent/references/lark-agent-task.md
🚧 Files skipped from review as they are similar to previous changes (13)
- cmd/agent/register_test.go
- errs/subtypes.go
- skills/lark-agent/references/lark-agent-card.md
- skills/lark-agent/SKILL.md
- internal/agent/contract.go
- cmd/agent/card_test.go
- cmd/agent/format.go
- cmd/agent/preflight.go
- cmd/agent/send.go
- cmd/agent/card.go
- cmd/agent/format_test.go
- cmd/agent/common.go
- cmd/agent/task.go
Comment on lines
+379
to
+411
| // TestAgentListScheme_OnlineChecksIdentity pins #8: the online enumeration path | ||
| // enforces the user|bot identity whitelist. An explicitly unsupported --as is | ||
| // rejected as a validation error before the online ListAgents call. | ||
| func TestAgentListScheme_OnlineChecksIdentity(t *testing.T) { | ||
| called := false | ||
| spec := catSpec("", "", "") | ||
| iagent.Register(iagent.Provider{ | ||
| Scheme: "fakelivewl", | ||
| Label: "test fake (identity-whitelist live-enum)", | ||
| AgentIDSource: "test only", | ||
| Identities: []iagent.IdentitySpec{{Type: iagent.IdentityUser}, {Type: iagent.IdentityBot}}, | ||
| Instance: &spec, | ||
| ListAgents: func(context.Context, iagent.Runtime) ([]iagent.AgentSummary, error) { | ||
| called = true | ||
| return nil, nil | ||
| }, | ||
| }) | ||
|
|
||
| cfg := &core.CliConfig{AppID: "cli_x", AppSecret: "fake-secret", Brand: core.BrandFeishu} | ||
| f, _, _, _ := cmdutil.TestFactory(t, cfg) | ||
| opts := &listOptions{Factory: f, Cmd: resolveCmd(t, true, "admin"), Format: "json", Scheme: "fakelivewl", As: "admin"} | ||
|
|
||
| err := agentListRun(opts) | ||
| if err == nil { | ||
| t.Fatal("an unsupported identity should be rejected before the online call") | ||
| } | ||
| if !errs.IsValidation(err) { | ||
| t.Fatalf("unsupported identity should be a validation error, got %T (%v)", err, err) | ||
| } | ||
| if called { | ||
| t.Error("ListAgents must NOT be called when the identity whitelist fails") | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert typed subtype, not just the error category.
TestAgentListScheme_OnlineChecksIdentity only checks errs.IsValidation(err); it never verifies the specific Subtype (or param) via errs.ProblemOf, unlike the sibling scope-preflight test just above it which does assert p.Subtype.
Proposed fix
if !errs.IsValidation(err) {
t.Fatalf("unsupported identity should be a validation error, got %T (%v)", err, err)
}
+ if p, ok := errs.ProblemOf(err); !ok || p.Subtype == "" {
+ t.Fatalf("expected a typed problem with a subtype, got %+v", p)
+ }Based on coding guidelines, `**/*_test.go`: Error-path tests must assert typed metadata via `errs.ProblemOf` (`category` / `subtype` / `param`) and cause preservation, not message substrings alone.
📝 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
| // TestAgentListScheme_OnlineChecksIdentity pins #8: the online enumeration path | |
| // enforces the user|bot identity whitelist. An explicitly unsupported --as is | |
| // rejected as a validation error before the online ListAgents call. | |
| func TestAgentListScheme_OnlineChecksIdentity(t *testing.T) { | |
| called := false | |
| spec := catSpec("", "", "") | |
| iagent.Register(iagent.Provider{ | |
| Scheme: "fakelivewl", | |
| Label: "test fake (identity-whitelist live-enum)", | |
| AgentIDSource: "test only", | |
| Identities: []iagent.IdentitySpec{{Type: iagent.IdentityUser}, {Type: iagent.IdentityBot}}, | |
| Instance: &spec, | |
| ListAgents: func(context.Context, iagent.Runtime) ([]iagent.AgentSummary, error) { | |
| called = true | |
| return nil, nil | |
| }, | |
| }) | |
| cfg := &core.CliConfig{AppID: "cli_x", AppSecret: "fake-secret", Brand: core.BrandFeishu} | |
| f, _, _, _ := cmdutil.TestFactory(t, cfg) | |
| opts := &listOptions{Factory: f, Cmd: resolveCmd(t, true, "admin"), Format: "json", Scheme: "fakelivewl", As: "admin"} | |
| err := agentListRun(opts) | |
| if err == nil { | |
| t.Fatal("an unsupported identity should be rejected before the online call") | |
| } | |
| if !errs.IsValidation(err) { | |
| t.Fatalf("unsupported identity should be a validation error, got %T (%v)", err, err) | |
| } | |
| if called { | |
| t.Error("ListAgents must NOT be called when the identity whitelist fails") | |
| } | |
| } | |
| // TestAgentListScheme_OnlineChecksIdentity pins `#8`: the online enumeration path | |
| // enforces the user|bot identity whitelist. An explicitly unsupported --as is | |
| // rejected as a validation error before the online ListAgents call. | |
| func TestAgentListScheme_OnlineChecksIdentity(t *testing.T) { | |
| called := false | |
| spec := catSpec("", "", "") | |
| iagent.Register(iagent.Provider{ | |
| Scheme: "fakelivewl", | |
| Label: "test fake (identity-whitelist live-enum)", | |
| AgentIDSource: "test only", | |
| Identities: []iagent.IdentitySpec{{Type: iagent.IdentityUser}, {Type: iagent.IdentityBot}}, | |
| Instance: &spec, | |
| ListAgents: func(context.Context, iagent.Runtime) ([]iagent.AgentSummary, error) { | |
| called = true | |
| return nil, nil | |
| }, | |
| }) | |
| cfg := &core.CliConfig{AppID: "cli_x", AppSecret: "fake-secret", Brand: core.BrandFeishu} | |
| f, _, _, _ := cmdutil.TestFactory(t, cfg) | |
| opts := &listOptions{Factory: f, Cmd: resolveCmd(t, true, "admin"), Format: "json", Scheme: "fakelivewl", As: "admin"} | |
| err := agentListRun(opts) | |
| if err == nil { | |
| t.Fatal("an unsupported identity should be rejected before the online call") | |
| } | |
| if !errs.IsValidation(err) { | |
| t.Fatalf("unsupported identity should be a validation error, got %T (%v)", err, err) | |
| } | |
| if p, ok := errs.ProblemOf(err); !ok || p.Subtype == "" { | |
| t.Fatalf("expected a typed problem with a subtype, got %+v", p) | |
| } | |
| if called { | |
| t.Error("ListAgents must NOT be called when the identity whitelist fails") | |
| } | |
| } |
🤖 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 379 - 411,
`TestAgentListScheme_OnlineChecksIdentity` only checks the validation category,
so tighten it to assert the exact typed problem metadata too. After
`agentListRun(opts)` fails, use `errs.ProblemOf` on the returned error and
verify the expected `Subtype` (and `Param` if applicable), matching the style
used by the sibling preflight test. Keep the existing checks that the call is
blocked and that the error is a validation error, but add the typed assertion so
the test proves the specific identity-whitelist failure path.
Source: Coding guidelines
Comment on lines
+120
to
+126
| prov, ok := iagent.Info(opts.Scheme) | ||
| if !ok { | ||
| return errs.NewValidationError(errs.SubtypeInvalidArgument, | ||
| "未知的 agent provider '%s'，当前支持: %s", | ||
| opts.Scheme, iagent.KnownSchemes()). | ||
| WithHint("用 lark-cli agent list 查看可用 provider") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Missing .WithParam on the unknown-scheme validation error.
This is a user positional-argument validation failure (invalid_argument) but doesn't set .WithParam(...) to name the failing input.
Proposed fix
if !ok {
return errs.NewValidationError(errs.SubtypeInvalidArgument,
"未知的 agent provider '%s'，当前支持: %s",
opts.Scheme, iagent.KnownSchemes()).
- WithHint("用 lark-cli agent list 查看可用 provider")
+ WithHint("用 lark-cli agent list 查看可用 provider").
+ WithParam("scheme")
}Based on coding guidelines, `cmd/**/*.go`: Use `errs.NewValidationError(errs.SubtypeInvalidArgument, ...).WithParam("--flag")` for user flag/argument validation failures.
📝 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
| prov, ok := iagent.Info(opts.Scheme) | |
| if !ok { | |
| return errs.NewValidationError(errs.SubtypeInvalidArgument, | |
| "未知的 agent provider '%s'，当前支持: %s", | |
| opts.Scheme, iagent.KnownSchemes()). | |
| WithHint("用 lark-cli agent list 查看可用 provider") | |
| } | |
| prov, ok := iagent.Info(opts.Scheme) | |
| if !ok { | |
| return errs.NewValidationError(errs.SubtypeInvalidArgument, | |
| "未知的 agent provider '%s'，当前支持: %s", | |
| opts.Scheme, iagent.KnownSchemes()). | |
| WithHint("用 lark-cli agent list 查看可用 provider"). | |
| WithParam("scheme") | |
| } |
🤖 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 120 - 126, The unknown agent provider
validation in list should name the failing input. Update the validation error
built in `iagent.Info(opts.Scheme)`’s `!ok` branch to chain `.WithParam(...)`
with the corresponding user-facing argument/flag for `opts.Scheme`, keeping the
existing `errs.NewValidationError(errs.SubtypeInvalidArgument, ...)` and hint
intact.
Source: Coding guidelines
Comment on lines
+72
to
+74
| func TestTaskCancelUnsupportedGated(t *testing.T) { | ||
| cfg := &core.CliConfig{AppID: "cli_x", AppSecret: "fake-secret", Brand: core.BrandFeishu} | ||
| f, _, _, _ := cmdutil.TestFactory(t, cfg) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Isolate CLI config state in direct factory tests.
These direct cmdutil.TestFactory tests should set LARKSUITE_CLI_CONFIG_DIR to a temp dir before constructing the factory, so future config reads/writes cannot touch developer or CI state.
Proposed fix
func TestTaskCancelUnsupportedGated(t *testing.T) {
+ t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())
cfg := &core.CliConfig{AppID: "cli_x", AppSecret: "fake-secret", Brand: core.BrandFeishu}
f, _, _, _ := cmdutil.TestFactory(t, cfg) func TestFetchArtifactURL_HttpRejected(t *testing.T) {
+ t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())
cfg := &core.CliConfig{AppID: "cli_x", AppSecret: "fake-secret", Brand: core.BrandFeishu}
f, _, _, _ := cmdutil.TestFactory(t, cfg) func TestFetchArtifactURL_LimitEnforced(t *testing.T) {
+ t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())
restore := swapHardenDownloadClient(passthroughClient)As per coding guidelines, **/*_test.go: isolate config state with t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()).
Also applies to: 731-733, 855-860
🤖 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_test.go` around lines 72 - 74, The direct
cmdutil.TestFactory-based tests are using shared CLI config state, which can
leak into developer or CI environments. In TestTaskCancelUnsupportedGated and
the other affected *_test.go cases, set LARKSUITE_CLI_CONFIG_DIR to a temp
directory with t.Setenv before calling cmdutil.TestFactory so any config
reads/writes stay isolated. Use the test names and the cmdutil.TestFactory setup
as the place to apply the env override consistently across the mentioned tests.
Source: Coding guidelines
Comment on lines
+727
to
+735
| // TestFetchArtifactURL_HttpRejected pins the https-only enforcement, distinct | ||
| // from the SSRF guard: a PUBLIC http:// URL passes the SSRF check (routable, | ||
| // http-family) but is still rejected because artifact bytes must travel over | ||
| // https (no plaintext download). No request is made. | ||
| func TestFetchArtifactURL_HttpRejected(t *testing.T) { | ||
| cfg := &core.CliConfig{AppID: "cli_x", AppSecret: "fake-secret", Brand: core.BrandFeishu} | ||
| f, _, _, _ := cmdutil.TestFactory(t, cfg) | ||
|
|
||
| _, err := fetchArtifactURL(context.Background(), f, "http://203.0.113.7/artifacts/report.txt") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Make the no-transport guarantee executable.
The test says no request is made, but it does not fail if fetchArtifactURL asks the factory for an HTTP client before rejecting http://. Install a fail-fast HttpClient seam before the call.
Proposed fix
cfg := &core.CliConfig{AppID: "cli_x", AppSecret: "fake-secret", Brand: core.BrandFeishu}
f, _, _, _ := cmdutil.TestFactory(t, cfg)
+ f.HttpClient = func() (*http.Client, error) {
+ t.Fatal("plain-http artifact URL should be rejected before constructing an HTTP client")
+ return nil, nil
+ }
_, err := fetchArtifactURL(context.Background(), f, "http://203.0.113.7/artifacts/report.txt")📝 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
| // TestFetchArtifactURL_HttpRejected pins the https-only enforcement, distinct | |
| // from the SSRF guard: a PUBLIC http:// URL passes the SSRF check (routable, | |
| // http-family) but is still rejected because artifact bytes must travel over | |
| // https (no plaintext download). No request is made. | |
| func TestFetchArtifactURL_HttpRejected(t *testing.T) { | |
| cfg := &core.CliConfig{AppID: "cli_x", AppSecret: "fake-secret", Brand: core.BrandFeishu} | |
| f, _, _, _ := cmdutil.TestFactory(t, cfg) | |
| _, err := fetchArtifactURL(context.Background(), f, "http://203.0.113.7/artifacts/report.txt") | |
| // TestFetchArtifactURL_HttpRejected pins the https-only enforcement, distinct | |
| // from the SSRF guard: a PUBLIC http:// URL passes the SSRF check (routable, | |
| // http-family) but is still rejected because artifact bytes must travel over | |
| // https (no plaintext download). No request is made. | |
| func TestFetchArtifactURL_HttpRejected(t *testing.T) { | |
| cfg := &core.CliConfig{AppID: "cli_x", AppSecret: "fake-secret", Brand: core.BrandFeishu} | |
| f, _, _, _ := cmdutil.TestFactory(t, cfg) | |
| f.HttpClient = func() (*http.Client, error) { | |
| t.Fatal("plain-http artifact URL should be rejected before constructing an HTTP client") | |
| return nil, nil | |
| } | |
| _, err := fetchArtifactURL(context.Background(), f, "http://203.0.113.7/artifacts/report.txt") |
🤖 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_test.go` around lines 727 - 735, The http-rejection test
currently only checks the returned error, so it can still pass even if
fetchArtifactURL consults an HTTP client before rejecting the URL. In
TestFetchArtifactURL_HttpRejected, install a fail-fast HttpClient seam on the
factory returned by cmdutil.TestFactory so any transport attempt panics or fails
immediately, then call fetchArtifactURL and assert the http:// URL is rejected
without invoking the client. Use the fetchArtifactURL and HttpClient hooks to
make the no-request guarantee executable.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/runtime_test.go`:
- Around line 198-203: The CallUpload error-path test currently only checks
errs.IsValidation(err); update it to assert the typed validation metadata too.
In runtime_test.go, use errs.ProblemOf on the returned error to verify the
category/subtype and preserve the cause, then use errors.As with
*errs.ValidationError to assert the Param is --file. Keep the existing unsafe
--file path setup, but make the test fail if the subtype or param does not match
the expected validation error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
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.
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.
…AgentSpec
Supersede the struct-of-func-fields Provider with a runtime-injection model that
stops leaking framework plumbing to integrators (the Deps{Client, As} an
onboarding author previously had to receive and destructure — the mock didn't
even use it).
Framework (internal/agent):
- Provider is now one declarative value per business domain (scheme): metadata +
a Catalog []AgentSpec (offline-enumerable) XOR an Instance *AgentSpec template,
plus an optional online ListAgents hook. AgentSpec carries per-agent card
metadata, the FileInput/InputRequired flags, and the verb hooks.
- Every hook receives an identity-opaque agent.Runtime (AgentID/IsBot/CallAPI/
CallMultipart) instead of a raw client; the concrete cmdRuntime lives in
cmd/agent (like event's consumeRuntime), so internal/agent no longer depends on
internal/client and the Deps struct is gone. CallMultipart is the centralized,
SafeInputPath-validated file-upload seam that makes file_input deliverable.
- Register takes a Provider (pure-struct validation, fail-fast). LookupSpec
resolves ref→spec fully offline. Capability = wired-hook presence
(DeriveCapabilities), card synthesized by BuildCard (rt=nil ⇒ offline caps +
static metadata; rt!=nil ⇒ best-effort Describe enrichment). Deleted catalog.go,
Deps, Factory, Resolve, NewCard, the zero-Deps probe, and the Discoverer interface.
Command layer (cmd/agent):
- resolveSpec (offline: identity + LookupSpec) then capability nil-gate BEFORE
runtimeFor, so an unsupported verb returns unsupported_capability (exit 2)
before any client is built — uniformly across list/context/artifact (previously
only cancel gated offline). Then runtimeFor + scope preflight + spec.<hook>(rt).
- agent list: catalog enumerates offline (ListCatalog); instance enumerates via
the online ListAgents hook, else reports not-enumerable.
Providers: example is now a declarative Provider() value + plain hooks reading
rt.AgentID() (echo minimal / reporter full differ only by wired fields). Explicit
aggregation in agent/register.go.
Adds cmd/agent runtime tests (CallAPI unwrap/error/transport, IsBot, CallMultipart
SafeInputPath), negative capability-gate tests (send --file / task list / context
get / artifact download), the https-only artifact check, and BuildCard's dynamic
Describe path.
…active_task)
`task list` / `context get` carried only {task_id, context_id, state,
is_terminal} — too thin for a caller (especially an AI) to tell which task
to resume without a `task get` per item. Enrich the summary surface so the
list is self-sufficient for triage, aligning with A2A's Task
(status.timestamp + last message).
- TaskSummary: add updated_at + summary (last agent message, or the pending
prompt for input_required; rune-truncated)
- AgentTask: add created_at + updated_at
- ContextSummary: add updated_at + task_count + awaiting_input
- ContextDetail: drop the embedded tasks[]; add updated_at, task_count,
awaiting_input, and active_task (the latest-updated task). Full task
enumeration stays in `agent task list --context-id`.
- task list / context list sort by updated_at desc
- route task list / context list / context get through the content-safety
scan (they now carry untrusted agent text); ANSI-strip + flatten summary
in pretty/TSV
- example provider fills the new fields; tests + lark-agent skill docs updated
The agent scope preflight now mirrors cmd/event's scopeRemediationHint for both identities, replacing the bespoke replacement-era hint: - user: the re-auth hint lists ONLY the missing scopes (the open platform authorizes incrementally, so re-login with just the missing keeps existing grants — no merge needed). Uses the canonical repo-wide `auth login --scope` phrasing instead of a one-off Chinese string. - bot: previously skipped entirely; now checks the app's published TenantScopes (fetched best-effort via appmeta.FetchCurrentPublished behind a swappable seam; a fetch failure downgrades to a no-op). A missing scope reports the developer-console re-publish remediation (the event-style scan-to-enable deep link lives in cmd/event and is not duplicated here). preflightScopesForRef keeps its signature (no call-site changes); bot fetch uses a bounded background context so no ctx param is threaded. Tests cover the incremental user hint, the bot missing/present/no-scopes branches, and the bot seam wiring.
Runtime.CallAPI/CallMultipart now return the response "data" object as json.RawMessage instead of map[string]any, and provider hooks decode it through the generic Call[T] / CallUpload[T] helpers: a hook declares the response struct it expects and the framework unmarshals + classifies errors, instead of poking at a map. Call[map[string]any] remains available for genuinely dynamic shapes; a response with no "data" (e.g. a pure write) yields T's zero value and a nil error. decodeData centralizes the unmarshal + invalid_response classification. cmdRuntime re-encodes the unwrapped "data" sub-object to raw JSON after CheckResponse. Test doubles (cmd/agent + example fakeRuntime, card_test fakeRT) updated to the raw signature; the runtime tests keep the raw-data assertion and add a Call[T] decode assertion.
Review follow-up on the agent command tree. A batch of low-risk hardening fixes; no behavior change for the shipped example provider. - Terminal-injection: pretty/TSV renderers now sanitize the agent-controlled State / UpdatedAt / CreatedAt fields (kvValue on pretty rows, stripANSI on TSV), matching the id/summary/title fields — a malicious provider can no longer inject CSI/OSC escapes via a forged state or timestamp. - Nil-safety: `task get --watch` and artifact download return a typed invalid_response error when a provider hook yields (nil, nil) (a legitimate Call[*T] result on an empty "data") instead of panicking; an artifact with neither inline bytes nor a URL no longer writes a 0-byte file. - Array convention: task/context/agent list normalize a nil slice to [] so an empty list serializes as [] not null, matching Card.Parameters. - Error hint: unknown-agent errors keep LookupSpec's scheme-scoped `agent list <scheme>` hint instead of being flattened to the generic one. - agent list <scheme> (online path) sets the resolved identity on its envelope, consistent with the other leaves. - Comment/doc drift: drop references to the removed Deps probe / Discoverer / ProviderInfo / resolveProvider symbols; rename Supports(cap) -> capKey. - Tests: cross-agent isolation in the example store, empty-list [] contract, State/timestamp sanitization regression, and a real stdout assertion for context list --jq.
Two review follow-ups on the agent command tree. Artifact download (data integrity): fetchArtifactURL now reads one byte past maxArtifactBytes and refuses a body over the cap with a typed error, instead of letting io.LimitReader silently truncate a >256 MiB artifact to a corrupt, partial file that reported success (exit 0). The LimitEnforced test is inverted to assert the rejection. Capabilities: the single multi_turn card bit is replaced by three independent capabilities — context_list / context_get / context_delete — each derived from its own wired hook (ListContexts / GetContext / DeleteContext). One bit could not honestly represent three separately-deliverable verbs: a provider wiring ListContexts but not DeleteContext advertised multi_turn=true while `context delete` failed claiming multi_turn=false. Each context verb now gates on and reports its own capability. Card schema, capability matrix, the three context gates, tests, and the lark-agent skill docs are updated in lockstep.
The instance/ListAgents branch of `agent list <scheme>` makes a real online call but skipped the two gates every other online verb runs via resolveSpec + preflightScopesForRef: the user|bot identity whitelist and the all-or-nothing scope preflight. A future network-backed provider would get a worse contract on list than on send/task/context — a scope-lacking user hit a raw platform error instead of a clean missing_scope (exit 3), and an out-of-whitelist identity was never rejected. - preflightScopesForRef is split into a scheme-keyed core (preflightScopesForScheme) plus a thin ref wrapper; ref-addressed callers are unchanged. - The online list branch now calls f.CheckIdentity and preflightScopesForScheme (the full RequiredScopes, same all-or-nothing rule as the other verbs) before ListAgents. Enumeration is treated as a real API verb, not a special case. - `agent list` registers --as so the enumeration identity can be chosen (the offline no-scheme provider listing ignores it). Catalog providers (offline enumeration) are unaffected. Tests cover the scope preflight (missing_scope, ListAgents not called), the identity whitelist, and the --as flag registration.
The typed upload helper CallUpload[T] had no caller anywhere — only its JSON counterpart Call[T] was exercised (by TestCmdRuntime_CallAPI_UnwrapsData) — so the incremental dead-code gate flagged it as newly unreachable. Add TestCmdRuntime_CallUpload_PropagatesError, mirroring the Call[T] coverage: it drives CallUpload through the unsafe --file reject path and asserts the validation error propagates and T stays zero. This keeps the provider-facing typed pair (Call[T] / CallUpload[T]) symmetric — both entry points are now tested rather than only the JSON one — and makes the helper reachable so the gate passes.
…itted)
Restores the structured HITL decision that the initial cut flattened to a prompt
+ free text, so multi-endpoint arbitration is expressible. Grounded in A2A: the
decision rides a DataPart and the answer reuses `agent send` (A2A's single
SendMessage), not new top-level protocol fields or a new verb.
Read side:
- InputRequired gains DecisionID, InputType (single_select|multi_select|text),
Options[]{OptionID,Label}, Submitted (+SubmittedOptionID); Options was []string.
- `task get` pretty view renders the decision (prompt, decision_id, options,
submitted); every agent-controlled field is ANSI-stripped.
Write side:
- SendInput gains DecisionID + OptionIDs. `agent send` gains --decision-id and
--option (repeatable); --text becomes optional when answering by option.
Guards: --option needs --decision-id; --decision-id needs --context-id/--task-id.
- meta.next for an input_required task carrying a decision points at the
structured answer command (decision_id whitelisted via safeNextID; falls back
to --text otherwise).
- Arbitration is server-side: a provider's Send returns conflict on an
already-answered decision; the CLI only reads/echoes submitted.
Reference provider: a new example:planner agent demonstrates the loop end to end
— the first send opens the decision, --decision-id/--option completes it and
records the winning option, and a re-answer returns conflict.
Skill docs (SKILL, send, card, example) updated; card capability count corrected
7 -> 9.
Implements the v5 params design: business parameters become first-class,
per-operation declarations that every agent verb can carry, validate, and
chain — replacing the send-only, agent-global parameter table.
SPI (internal/agent):
- AgentSpec's eight hook funcs become eight Op units (Op[H]{Params, Handler}):
a parameter physically cannot be declared on an unimplemented operation, and
capability derivation enumerates one table (Ops()) shared by every consumer.
Handler signatures are unchanged; migration is wrapping `Send: f` into
`Send: SendOp{Handler: f}`.
- CardParam gains real validation semantics: Type (string|integer|number|
boolean, empty normalizes to string), Enum (string+integer), Default
(backfilled), Min/Max (numeric ranges). Register fail-fasts on twelve
declaration mistakes (charset, duplicates, enum/default/range conflicts,
params on unwired ops, ListParams without ListAgents).
- Runtime gains Params() with a hard contract: required keys present and
non-empty, defaults backfilled, every value validated — before any handler
runs. Typed consumption via BindParams[T] (param struct tags) plus
ParamInt/ParamBool; agenttest.CheckParamsBinding locks declaration↔struct
drift in CI. SendInput.Params and CardInfo.Parameters are removed.
Command layer (cmd/agent):
- --param key=value on all eight verb leaves plus `agent list <scheme>`
(Provider.ListParams; discovered via providers[].list_parameters since the
caller holds no agent_ref at list time). `task get --artifact` validates
strictly against the artifact_download declaration.
- Collect-all validation: every violation reported in one typed error
(errs.InvalidParam gains an optional Spec field embedding the full
declaration), so a caller can fix all mistakes in one round without a
discovery trip. Empty values count as "not provided" (defaults still
backfill; required still rejects). send gains an explicit mode discriminator
(start/continue/answer) formalizing the existing guards.
- meta.next carries params by the three-way rule (whitelisted values ride
literally; whitelist failures degrade required params to placeholders;
target-verb requireds the caller never gave are added as placeholders), and
terminal tasks emit a ready-made download command per artifact.
- Lean card: parameter details move behind `agent card <ref> --operation
<verb|all>` (returns supported/command-template/parameters); the default
card carries a has_parameters cue. Instance providers are labeled
parameters_source:"template".
The example provider migrates to Op units and reporter declares two demo
params (enum+default, integer+range+default) consumed via BindParams — the
copy-start template now exercises the whole declaration surface offline.
Skill docs (SKILL.md + six references) updated to the new discovery and error
contracts.
An adversarial review pass (16 findings, all verified) is folded in — notably:
empty-valued optional params no longer bypass validation and default backfill
(was an internal-error/exit-5 path violating the rt.Params() contract);
invalid values no longer double-report as missing; BindParams returns a typed
error on unexported tagged fields; ValidateValue rejects non-finite numbers.
Object parameters land on the declaration model: CardParam gains Fields (one
nesting level, scalar leaves only — an object itself declares nothing but its
members; requiredness, enums, defaults and ranges all live on the leaves) and
Register recursively fail-fasts six new object rules.
Transport is dual-channel with one canonical form: dotted paths are the
primary channel (--param filter.region=east — no shell quoting, AI error rate
stays at scalar level, and meta.next can carry leaves literally) and a JSON
value is the fallback (--param filter='{"region":"east"}', numbers decoded via
json.Number so literals survive). Both channels validate leaf-by-leaf with the
same teaching errors (dotted-path violation names, enum/range sets, unknown
members listing the field set) and normalize into flat dotted keys in
rt.Params() — a provider never sees which channel the caller used. Mixing
channels for one object is rejected; leaf defaults backfill on both.
Consumption: ParamObject[T] assembles an object's leaves into a typed struct;
BindParams supports nested tagged structs; agenttest.CheckParamsBinding
recurses so declaration/struct drift on leaves still dies in CI.
NoCarry opts a parameter out of the meta.next carry: values never ride the
chain literally (a required NoCarry param degrades to a placeholder so the
caller supplies a fresh value). It is the declared escape hatch for per-call
parameters (trace tags) where the carry rule's same-resource continuity
assumption does not hold. Object leaves otherwise carry as ordinary scalars
under the unchanged three-way rule.
The example reporter declares a render object (enum + boolean + defaults)
consumed through a nested binding struct — with defaults the historical reply
stays byte-identical. Skill docs updated (card fields/no_carry semantics, send
dual-channel rules, example walkthrough).
Code (all findings verified by re-run before fixing):
- send --file: local gate before any capability/confirmation gate —
relative-within-CWD, must exist, not a directory; violations collected
in one pass (dry-run included)
- --param scalars: canonicalize accepted variants (TRUE/1/+5/04 ->
true/5/4) so both channels produce one wire form; integer overflow now
reports a range error, not a type error; non-object JSON no longer
leaks Go type text
- unknown-param suggestions: nearest-first by edit distance (<=2),
cross-verb hits no longer mix verb names into suggestions
- meta.next: no self-loop on terminal task get (caller-aware); --as is
carried only when the caller passed it explicitly, matching the
shortcut-family convention of never pinning identity
- pretty task view: request/reply lines (result was invisible before)
- context delete gate: self-contained Chinese irreversibility message
- teaching hints: --timeout/--task-id now carry actionable fixes;
example conflict message drops internal jargon
- list-class envelopes: empty lists omit meta entirely (no '{}' shape)
- example: artifacts carry name/mime before download (csv/xlsx);
planner now covered by conformance
Docs: skill family synced to verified binary behavior — bot preflight
truth, incremental-auth hint semantics, 8-verb
--operation vocabulary, real-output sample backfills (3 agents,
created_at, submitted omitempty), redundancy converged to single
authority sections, provider file slimmed to runtime-AI-only content.
liuxinyanglxy
force-pushed
the
feat/remote-agent
branch
from
July 13, 2026 06:09
d2bec7e to
b740c0b
Compare
Renames the provider-neutral agent surface from singular to plural across every layer, for internal/external consistency: - CLI command `lark-cli agent` → `lark-cli agents` (cobra Use, the NewCmdAgents constructor, root registration, and the Agent-tooling help group key so it still groups correctly). - Go packages / dirs: agent/→agents/, cmd/agent/→cmd/agents/, internal/agent/→internal/agents/; every package declaration, import path, and the iagent→iagents import alias. - Skill: skills/lark-agent/→skills/lark-agents/, reference files lark-agent-*.md→lark-agents-*.md, SKILL.md frontmatter name/cliHelp, and every documented command reference. Domain nouns deliberately stay singular — they model one agent, not a collection: the AgentSpec/AgentCard/AgentTask/AgentSummary types, the agent_id / agent_ref / agent_ref_format / agent_id_source wire keys, and the A2A message Role "agent". Three adversarial review passes confirmed no wire-key or type corruption and no missed references.
Adds Feishu-OpenAPI-style page_token/page_size pagination to the three list
operations (`agents task list`, `agents context list`, instance
`agents list <scheme>`), consistent with the shortcuts/contact & calendar
convention.
SPI (internal/agents):
- New PageParams{Token,Size} / PageInfo{NextToken,HasMore}.
- ListTasks/ListContexts hooks and the Provider.ListAgents field gain a
PageParams arg and a PageInfo return; the provider owns cross-page ordering
(contract: most-recent-first) and maps the opaque cursor to its backend.
Command layer (cmd/agents):
- --page-size (default 20, range 1-100, validated client-side in RunE) and
--page-token on the three list leaves. output.Meta gains has_more +
page_token (both omitempty); listMetaPage emits them and, when a next page
exists, a ready-made "下一页" meta.next command so an AI pages by running the
suggested command instead of threading a cursor. The next-page cursor is
safeNextID-whitelisted before it is interpolated into that command (it still
rides meta.page_token as data if it fails), and ref/scheme/context-id are
each whitelisted too. The catalog list path stays offline/unpaged.
- Per-page CLI re-sort removed: ordering is now purely the provider's
responsibility, so a within-page re-sort could only make the concatenation
across pages inconsistent.
example provider paginates its in-memory store via an opaque offset cursor,
most-recent-first (Seq desc). Skill docs (task/context/list) document the flags,
has_more/page_token, and the meta.next paging idiom.
liuxinyanglxy
force-pushed
the
feat/remote-agent
branch
from
July 17, 2026 06:01
c1f25e2 to
04dad76
Compare
The set of agents and the capabilities each exposes can be scoped to the resolved account environment, so discovery and gating reflect only what the current account can actually use. SPI (internal/agents): - AgentSpec and Op gain an optional scope field (empty = unrestricted); Register fail-fasts on an unknown scope value and on a scope declared on an unwired operation. - DeriveCapabilities and BuildCard take the resolved environment: an operation-backed capability is exposed only when wired AND in scope; the card echoes the environment it was rendered for. - Provider.ListCatalog filters the catalog to the in-scope agents. Command layer (cmd/agents): - The resolved environment defaults offline (nil-safe) so the gates hold before config init. Every verb path gates offline before the client is built: the whole-agent gate fires before the per-verb capability nil-gate, so an out-of-scope agent uniformly returns a single dedicated validation error (exit 2) for every verb instead of a misleading "unsupported" on an incidentally-unwired one; the per-operation gate follows the nil-gate. List filtering mirrors the same rule. The example provider demonstrates the per-capability scope end to end; skill docs updated accordingly.
liuxinyanglxy
force-pushed
the
feat/remote-agent
branch
from
July 17, 2026 06:29
04dad76 to
e2b7ef0
Compare
Replace the decision_id/--option HITL contract with the question-group
answer scheme:
- contract: InputRequired becomes {label, description, questions[]}
(a single question is a length-1 group); Option gains description;
decision_id/input_type/submitted fields are removed
- answering: --answer <qid>=<option_id> | <qid>.text=<text> is the only
answer channel (--decision-id/--option removed); --text is always the
message-level remark; offline collect-all key-grammar guards plus an
input_required capability gate
- keys: minted at group-creation time (MintQuestionIDs /
DeriveGroupSuffix), per-group-unique for stale-retry protection;
central normalization with size caps and whole-group degradation on
non-conforming keys (JSON _notice.provider_defect); KeyCharsetRE is
the single charset source and safeNextID now requires an alphanumeric
first character (rejects flag-lookalike ids)
- meta.next: per-question --answer templates with a relay-first label
- example/planner: three-question reference group; strict collect-all
validation (Reason enum + question spec), atomic acceptance with a
resolved_answers echo, context binding, no sibling-task fork on bare
--text; Register enforces InputRequired => brand-covering CancelTask
- errs: ValidationError gains the resolved_answers extension field
- skills/lark-agents 1.3.0: relay-first rules, answer grammar, recovery
playbook, untrusted question-text discipline
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 family for provider discovery,cardviewing,send, andtask/contextworkflows.--format prettyand--jqfiltering, plusmeta.nextfollow-up action suggestions.--dry-runpreviews foragent sendand clearer--watch/--timeoutpolling behavior for task watching.Bug Fixes