Skip to content
Merged
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
6 changes: 6 additions & 0 deletions docs/api/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,12 @@ Same logical operation, opposite id semantics. Also, MCP `list` omits
`userVerification`, `agentForward`, and `isDemo` that REST returns. Two clients
build different mental models of "an endpoint."

Related, now resolved in both backends: Node's MCP tool schema used to
silently strip `userVerification`/`agentForward` from create/update `data`, so
endpoints were not fully editable via MCP. That was a bug, not the contract —
create/update accept and validate both fields (see
[`mcp-tools.md`](./mcp-tools.md)).

### D. Near-identical WS message names differ by one character / tense

`terminal:close` (client→server, _do close_) vs. `terminal:closed`
Expand Down
12 changes: 11 additions & 1 deletion docs/api/mcp-tools.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,19 @@ Manage SSH endpoints. Input:
```ts
{ action: "list" | "read" | "create" | "update" | "delete";
id?: string; // required for read/update/delete
data?: { label?; host?; port?; username?; description?: string|null } }
data?: { label?; host?; port?; username?;
userVerification?: "required" | "preferred" | "discouraged";
agentForward?: boolean;
description?: string|null /* ≤1000 chars; null clears */ } }
```

Every REST-editable field is editable here too. `create` defaults:
`userVerification: "required"`, `agentForward: true`, `port: 22`. Invalid
values (unknown `userVerification`, wrong types, over-long `description`)
are rejected without writing. _(Node's tool schema used to silently strip
`userVerification`/`agentForward` — that was a bug, not the contract; both
backends now accept them.)_

Per-action success payload (JSON in the text block):

- `list` → `{ endpoints: [{ id, label, host, port, username, description }] }`
Expand Down
54 changes: 33 additions & 21 deletions internal/agent/session.go
Original file line number Diff line number Diff line change
Expand Up @@ -104,38 +104,50 @@ func (s *Session) CreateEndpoint(ctx context.Context, ep store.Endpoint) error {
return s.deps.Endpoints.Create(ctx, ep)
}

// UpdateEndpoint read-merges a patch into an account-scoped endpoint and writes
// it back. Returns false when no endpoint matched. Unknown keys are ignored.
func (s *Session) UpdateEndpoint(ctx context.Context, id string, patch map[string]any) (bool, error) {
// EndpointPatch is a typed partial endpoint update; nil fields keep the
// stored value. Description carries an explicit set flag so a JSON null can
// clear it.
type EndpointPatch struct {
Label *string
Host *string
Port *int64
Username *string
UserVerification *string
AgentForward *bool
Description *string // applied only when DescriptionSet; nil clears
DescriptionSet bool
}

// UpdateEndpoint read-merges a typed patch into an account-scoped endpoint and
// writes it back. Returns false when no endpoint matched. Wire-shape decoding
// and field validation (userVerification enum, description cap) happen at the
// caller (internal/mcp).
func (s *Session) UpdateEndpoint(ctx context.Context, id string, patch EndpointPatch) (bool, error) {
existing, err := s.deps.Endpoints.GetForAccount(ctx, id, s.accountID)
if err != nil || existing == nil {
return false, err
}
merged := *existing
if v, ok := patch["label"].(string); ok {
merged.Label = v
if patch.Label != nil {
merged.Label = *patch.Label
}
if v, ok := patch["host"].(string); ok {
merged.Host = v
if patch.Host != nil {
merged.Host = *patch.Host
}
if v, ok := patch["username"].(string); ok {
merged.Username = v
if patch.Port != nil {
merged.Port = *patch.Port
}
if v, ok := patch["userVerification"].(string); ok {
merged.UserVerification = v
if patch.Username != nil {
merged.Username = *patch.Username
}
if v, ok := patch["port"].(float64); ok { // JSON numbers decode as float64
merged.Port = int64(v)
if patch.UserVerification != nil {
merged.UserVerification = *patch.UserVerification
}
if v, ok := patch["agentForward"].(bool); ok {
merged.AgentForward = v
if patch.AgentForward != nil {
merged.AgentForward = *patch.AgentForward
}
if v, ok := patch["description"]; ok {
if s2, isStr := v.(string); isStr {
merged.Description = &s2
} else if v == nil {
merged.Description = nil
}
if patch.DescriptionSet {
merged.Description = patch.Description
}
return s.deps.Endpoints.Update(ctx, merged)
}
Expand Down
22 changes: 20 additions & 2 deletions internal/agent/session_endpoint_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ func TestSessionEndpointMutations(t *testing.T) {
}

// Update (partial patch merges).
ok, err := sess.UpdateEndpoint(ctx, "ep1", map[string]any{"label": "Renamed", "port": float64(2222), "agentForward": true})
label, port, fwd := "Renamed", int64(2222), true
ok, err := sess.UpdateEndpoint(ctx, "ep1", EndpointPatch{Label: &label, Port: &port, AgentForward: &fwd})
if err != nil || !ok {
t.Fatalf("update: ok=%v err=%v", ok, err)
}
Expand All @@ -42,8 +43,25 @@ func TestSessionEndpointMutations(t *testing.T) {
t.Fatalf("update merge wrong: %+v", ep)
}

// Full editability: userVerification and description are patchable too.
uv, desc := "discouraged", "jump host"
if ok, err := sess.UpdateEndpoint(ctx, "ep1", EndpointPatch{UserVerification: &uv, Description: &desc, DescriptionSet: true}); err != nil || !ok {
t.Fatalf("uv/desc update: ok=%v err=%v", ok, err)
}
ep, _ = sess.GetEndpoint(ctx, "ep1")
if ep.UserVerification != "discouraged" || ep.Description == nil || *ep.Description != "jump host" {
t.Fatalf("uv/desc merge wrong: %+v", ep)
}
// DescriptionSet with nil clears; unset leaves it alone.
if ok, err := sess.UpdateEndpoint(ctx, "ep1", EndpointPatch{DescriptionSet: true}); err != nil || !ok {
t.Fatalf("desc clear: ok=%v err=%v", ok, err)
}
if ep, _ = sess.GetEndpoint(ctx, "ep1"); ep.Description != nil {
t.Fatalf("description should be cleared: %+v", ep)
}

// Update a missing endpoint -> false.
if ok, _ := sess.UpdateEndpoint(ctx, "nope", map[string]any{"label": "x"}); ok {
if ok, _ := sess.UpdateEndpoint(ctx, "nope", EndpointPatch{Label: &label}); ok {
t.Error("update of missing endpoint should return false")
}
// Cross-account isolation: another account can't touch ep1.
Expand Down
76 changes: 76 additions & 0 deletions internal/httpserver/mcp_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -175,3 +175,79 @@ func TestMCPToolGoldens(t *testing.T) {
map[string]any{"sessionId": sid, "keys": []string{"text:echo hi", "enter"}})
assertToolGolden(t, "mcp-close-session", "shellwatch_close_session", sess, map[string]any{"sessionId": sid})
}

// Full endpoint editability via MCP: create + update accept every
// REST-editable field, with the same validation as REST. Not golden-pinned —
// these flows postdate the golden freeze (both backends changed together).
func TestMCPEndpointFullEdit(t *testing.T) {
ts := mcpServer(t)
sess := mcpConnect(t, ts)

text, isErr := callTool(t, sess, "shellwatch_manage_endpoints", map[string]any{
"action": "create", "id": "edit-me",
"data": map[string]any{
"label": "Edit Me", "host": "10.0.0.1", "username": "ops",
"userVerification": "preferred", "agentForward": false, "description": "staging box",
},
})
if isErr {
t.Fatalf("create: %s", text)
}

text, isErr = callTool(t, sess, "shellwatch_manage_endpoints", map[string]any{
"action": "update", "id": "edit-me",
"data": map[string]any{"userVerification": "discouraged", "agentForward": true, "description": nil},
})
if isErr {
t.Fatalf("update: %s", text)
}

text, isErr = callTool(t, sess, "shellwatch_manage_endpoints", map[string]any{"action": "read", "id": "edit-me"})
if isErr {
t.Fatalf("read: %s", text)
}
var ep struct {
UserVerification string `json:"userVerification"`
AgentForward bool `json:"agentForward"`
Description *string `json:"description"`
}
if err := json.Unmarshal([]byte(text), &ep); err != nil {
t.Fatal(err)
}
if ep.UserVerification != "discouraged" || !ep.AgentForward || ep.Description != nil {
t.Fatalf("update not applied: %+v", ep)
}

// Invalid enum is rejected — whether by the tool handler (isError) or by
// SDK-side schema validation (protocol error), the write must not land.
res, err := sess.CallTool(context.Background(), &mcpsdk.CallToolParams{
Name: "shellwatch_manage_endpoints",
Arguments: map[string]any{
"action": "update", "id": "edit-me",
"data": map[string]any{"userVerification": "none"},
},
})
if err == nil && !res.IsError {
t.Error("invalid userVerification was accepted")
}
text, _ = callTool(t, sess, "shellwatch_manage_endpoints", map[string]any{"action": "read", "id": "edit-me"})
_ = json.Unmarshal([]byte(text), &ep)
if ep.UserVerification != "discouraged" {
t.Errorf("invalid update mutated the row: %+v", ep)
}

// Create defaults match REST/Node: userVerification required, agentForward true.
if text, isErr := callTool(t, sess, "shellwatch_manage_endpoints", map[string]any{
"action": "create", "id": "defaults",
"data": map[string]any{"label": "D", "host": "10.0.0.2", "username": "ops"},
}); isErr {
t.Fatalf("create defaults: %s", text)
}
text, _ = callTool(t, sess, "shellwatch_manage_endpoints", map[string]any{"action": "read", "id": "defaults"})
if err := json.Unmarshal([]byte(text), &ep); err != nil {
t.Fatal(err)
}
if ep.UserVerification != "required" || !ep.AgentForward {
t.Errorf("create defaults wrong: %+v", ep)
}
}
Loading