Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 23 additions & 1 deletion CubeOps/internal/cubemaster/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,26 @@ func (e *CMError) Error() string {
return fmt.Sprintf("cubemaster error %d: %s", e.RetCode, e.RetMsg)
}

// HTTPError carries a CubeMaster failure that arrived as an HTTP status rather
// than as a ret_code in a 200 body. Both shapes occur, and a caller that knows
// only about CMError mis-handles this one silently: errors.As(&CMError) fails,
// so the response arrives as an opaque error and collapses into a generic 502
// regardless of what it said. Keeping it typed lets a caller ask the same
// question of either shape.
type HTTPError struct {
Status int
Body string
}

func (e *HTTPError) Error() string {
return fmt.Sprintf("cubemaster returned %d: %s", e.Status, e.Body)
}

// IsNotFound reports an HTTP-level not-found.
func (e *HTTPError) IsNotFound() bool {
return e.Status == http.StatusNotFound
}

// IsNotFound returns true for CubeMaster "not found" ret codes.
func (e *CMError) IsNotFound() bool {
return e.RetCode == 130404 || e.RetCode == 404
Expand Down Expand Up @@ -420,7 +440,9 @@ func readResponse(resp *http.Response) (json.RawMessage, error) {
return nil, fmt.Errorf("read response: %w", err)
}
if resp.StatusCode >= 400 {
return nil, fmt.Errorf("cubemaster returned %d: %s", resp.StatusCode, string(data))
// Typed; the message is byte-identical to the previous fmt.Errorf so
// logs and any operator greps keep working.
return nil, &HTTPError{Status: resp.StatusCode, Body: string(data)}
}
// Check CubeMaster business error code. CubeMaster uses ret_code=200 for
// success (and sometimes 0). Any other value is a failure, even when HTTP
Expand Down
85 changes: 84 additions & 1 deletion CubeOps/internal/service/agenthub.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,19 @@ type CubeMasterClient interface {
// SDKInstanceType is the CubeMaster instance_type used for AgentHub sandboxes.
const SDKInstanceType = "cubebox"

// defaultAgentTemplateID is the last-resort template identifier CreateInstance
// uses when the caller supplies neither a snapshot nor a templateId and no
// AgentHub template is registered to default to.
//
// Nothing in this repository provisions a template under this identifier: it is
// not a tpl-/snap- id, so CubeMaster resolves it through GetTemplateByAlias,
// and it only resolves on installs where an operator happened to claim it as a
// template alias. Reaching it therefore means "no template is registered",
// which CreateInstance reports as such instead of surfacing CubeMaster's
// not-found. Kept as a fallback rather than dropped so that installs which do
// carry the alias keep working.
const defaultAgentTemplateID = "wecom-ds-openclaw"

// ── Error ───────────────────────────────────────────────────────────────────

// Error is the service-layer error type. It carries an HTTP status code so
Expand Down Expand Up @@ -105,6 +118,26 @@ func wrapCMError(err error) *Error {
}
}

// isCMNotFound reports whether err is a CubeMaster not-found, in either shape
// CubeMaster uses it: the business code 130404 carried in a 200 body, and an
// HTTP-level 404. The repro in #1327 is the first shape, but keying only on it
// would make this silently stop working the day CubeMaster answers with an HTTP
// status instead — and the caller's whole purpose is to avoid leaking an
// identifier the requester never supplied.
//
// Callers that need the full status mapping should use wrapCMError instead. Note
// that wrapCMError deliberately still maps HTTPError to 502: widening the
// service-wide status mapping is a larger behavioural change than this fix and
// belongs in its own patch.
func isCMNotFound(err error) bool {
Comment thread
ENCHIGO marked this conversation as resolved.
var cmErr *cubemaster.CMError
if errors.As(err, &cmErr) && cmErr.IsNotFound() {
return true
}
var httpErr *cubemaster.HTTPError
return errors.As(err, &httpErr) && httpErr.IsNotFound()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CMError branch keys on ret codes (130404/404), but the HTTPError branch keys only on the HTTP status being exactly 404. A not-found that arrives as a non-404 HTTP status carrying ret_code: 130404 in the body (e.g. HTTP 400 + the 130404 envelope) would slip through here and the actionable 400 in CreateInstance would silently stop firing — the exact "silently stop working" regression this helper exists to prevent. Worth parsing the body's ret_code in HTTPError.IsNotFound() (or in isCMNotFound for the HTTPError case) so both shapes are recognised by content, not by status. Low severity since the #1327 repro (200 body, 130404) and a plain HTTP 404 are both covered.

}

// ── AgentStore interface ────────────────────────────────────────────────────

// AgentStore is the subset of *store.Store that AgentHubService depends on.
Expand All @@ -123,6 +156,8 @@ type AgentStore interface {
GetAgentSnapshot(ctx context.Context, agentID, snapshotID string) (*store.AgentSnapshot, error)
DeleteAgentSnapshot(ctx context.Context, agentID, snapshotID string) error
GetAgentTemplate(ctx context.Context, templateID string) (*store.AgentTemplate, error)
GetRecommendedAgentTemplate(ctx context.Context) (*store.AgentTemplate, error)
ListAgentTemplates(ctx context.Context, limit, offset int) ([]store.AgentTemplate, error)
RecordOperation(ctx context.Context, agentID, sandboxID, operationType, status, errMsg string) error
LatestHealthySnapshot(ctx context.Context, agentID string) (string, error)
SetBaseSnapshotID(ctx context.Context, agentID, snapshotID string) error
Expand Down Expand Up @@ -391,6 +426,41 @@ type CreateInstanceResult struct {
Instance *store.AgentInstance
}

// defaultTemplateID picks the template CreateInstance uses when the caller
// omits templateId: the template an operator marked recommended if there is
Comment thread
ENCHIGO marked this conversation as resolved.
// one, otherwise the most recently registered one.
//
// Both are single bounded queries — the recommended preference is exact
// without reading the registry, which matters because nothing sets the flag
// automatically (see store.GetRecommendedAgentTemplate), so an install can
// accumulate any number of newer non-recommended templates after the marked
// one.
//
// none is true only when the registry was read successfully and holds nothing.
// A failed read returns none=false — the caller must not report "nothing is
// registered" on the strength of a query that never completed. Either way the
// returned id is then defaultAgentTemplateID, which the caller still passes to
// CubeMaster so installs carrying that alias keep working.
func (s *AgentHubService) defaultTemplateID(ctx context.Context) (id string, none bool) {
recommended, err := s.Store.GetRecommendedAgentTemplate(ctx)
if err != nil {
logging.G(ctx).Warnf("failed to read the recommended agent template: %v", err)
return defaultAgentTemplateID, false
}
if recommended != nil {
return recommended.TemplateID, false
}
newest, err := s.Store.ListAgentTemplates(ctx, 1, 0)
if err != nil {
logging.G(ctx).Warnf("failed to list agent templates while choosing a default: %v", err)
return defaultAgentTemplateID, false
}
if len(newest) > 0 {
return newest[0].TemplateID, false
}
return defaultAgentTemplateID, true
}

// CreateInstance orchestrates the full agent creation flow:
//
// 1. Resolve LLM config + domain + egress network config from settings.
Expand Down Expand Up @@ -431,14 +501,19 @@ func (s *AgentHubService) CreateInstance(ctx context.Context, req CreateInstance
snapshotID := strings.TrimSpace(req.SnapshotID)
rootfsSourceType := "template"
rootfsSourceID := ""
// Set when the request named no template and the registry was read and
// found empty, so the unprovisioned defaultAgentTemplateID was used; lets
// the CreateSandbox error below name the real problem. Deliberately not
// set when the registry could not be read — see defaultTemplateID.
noTemplateRegistered := false
if snapshotID != "" {
rootfsSourceType = "snapshot"
rootfsSourceID = snapshotID
} else {
if req.TemplateID != "" {
rootfsSourceID = req.TemplateID
} else {
rootfsSourceID = "wecom-ds-openclaw"
rootfsSourceID, noTemplateRegistered = s.defaultTemplateID(ctx)
}
}
templateID := rootfsSourceID
Expand Down Expand Up @@ -535,6 +610,14 @@ func (s *AgentHubService) CreateInstance(ctx context.Context, req CreateInstance
// --- Create sandbox ---
sandboxResp, err := s.CM.CreateSandbox(ctx, cmReq)
if err != nil {
// The caller named no template, none is registered, and CubeMaster
// could not resolve the fallback either. Report the missing
// registration rather than a not-found for an identifier the caller
// never supplied.
if noTemplateRegistered && isCMNotFound(err) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rewrite fires on any CubeMaster not-found once the registry was read empty — not specifically a template-resolution failure. In the empty-registry request the only template-ish input is the fallback id, but CreateSandbox also carries instance_type, network_type: "tap", and distribution_scope; a 130404 raised for one of those (e.g. a misconfigured network) would be reported as "no agent template is registered", masking the real cause. Since the error message in the repro already names the fallback identifier, a tighter predicate such as strings.Contains(err.Error(), defaultAgentTemplateID) would scope the rewrite to the case this PR is about while still being strictly better than the old 502. Not blocking — the current broad check is a reasonable default if you'd rather always err toward the actionable hint.

return nil, NewBadRequest("no agent template is registered: register one from the template market " +
"(POST /agenthub/templates/market), or pass templateId explicitly")
}
return nil, NewBadGateway("failed to create sandbox: " + err.Error())
}
var sbResult struct {
Expand Down
Loading
Loading