|
| 1 | +# MCP Server — Parameter Mismatch Test Plan |
| 2 | + |
| 3 | +Hunt for silent parameter mismatches between MCP tool request bodies and backend input structs. Same class of bug as the `channel_id` → `channel_ids` issue fixed in PR #43. |
| 4 | + |
| 5 | +## Scope |
| 6 | + |
| 7 | +20 tools across 6 toolsets in `pkg/flashduty/`: |
| 8 | + |
| 9 | +| Toolset | Tools | |
| 10 | +|---|---| |
| 11 | +| incidents | QueryIncidents, QueryIncidentTimeline, QueryIncidentAlerts, ListSimilarIncidents, CreateIncident, UpdateIncident, AckIncident, CloseIncident | |
| 12 | +| changes | QueryChanges | |
| 13 | +| status_page | QueryStatusPages, ListStatusChanges, CreateStatusIncident, CreateChangeTimeline | |
| 14 | +| users | QueryMembers, QueryTeams | |
| 15 | +| channels | QueryChannels, QueryEscalationRules | |
| 16 | +| fields | QueryFields | |
| 17 | + |
| 18 | +## Confirmed bugs (same class as PR #43) |
| 19 | + |
| 20 | +### 1. `query_members` — `member_name` dropped |
| 21 | +- **File:** `pkg/flashduty/users.go:73` |
| 22 | +- **Sends:** `{"p": 1, "limit": 20, "member_name": "<name>"}` |
| 23 | +- **Backend expects** (`fc-pgy/cmd/server/controller/member/member.go:38-45`): |
| 24 | + ```go |
| 25 | + type memberListInput struct { |
| 26 | + RoleID uint64 `json:"role_id"` |
| 27 | + Page int `json:"p"` |
| 28 | + Limit int `json:"limit"` |
| 29 | + Orderby string `json:"orderby"` |
| 30 | + Asc bool `json:"asc"` |
| 31 | + Query string `json:"query"` |
| 32 | + } |
| 33 | + ``` |
| 34 | +- **Impact:** Search-by-name returns all members; `member_name` is silently ignored. |
| 35 | +- **Fix:** Send `query: <name>` instead of `member_name: <name>`. |
| 36 | + |
| 37 | +### 2. `query_teams` — `team_name` dropped |
| 38 | +- **File:** `pkg/flashduty/users.go:170` |
| 39 | +- **Sends:** `{"p": 1, "limit": 20, "team_name": "<name>"}` |
| 40 | +- **Backend expects** (`fc-pgy/cmd/server/controller/team/team.go:311-318`): |
| 41 | + ```go |
| 42 | + type listTeamInput struct { |
| 43 | + Page int `json:"p"` |
| 44 | + Limit int `json:"limit"` |
| 45 | + Orderby string `json:"orderby"` |
| 46 | + Asc bool `json:"asc"` |
| 47 | + PersonID uint64 `json:"person_id"` |
| 48 | + Query string `json:"query"` |
| 49 | + } |
| 50 | + ``` |
| 51 | +- **Impact:** Search-by-name returns all teams; `team_name` is silently ignored. |
| 52 | +- **Fix:** Send `query: <name>` instead of `team_name: <name>`. |
| 53 | +- **Bonus:** backend also supports `person_id` filter the MCP tool doesn't currently expose. |
| 54 | + |
| 55 | +## Suspected / needs runtime verification |
| 56 | + |
| 57 | +### 3. `create_incident` — `assigned_to` shape |
| 58 | +- **File:** `pkg/flashduty/incidents.go:413-416` |
| 59 | +- **Sends:** `{"assigned_to": {"type": "assign", "person_ids": [123, 456]}}` |
| 60 | +- Verify the backend accepts the nested `{type, person_ids}` wrapper vs expecting flat `person_ids` at the top level. |
| 61 | + |
| 62 | +## Runtime test procedure |
| 63 | + |
| 64 | +For each tool, log the outbound HTTP body (look for `msg=duty request` lines in server logs) and confirm the JSON keys match the backend struct tags. |
| 65 | + |
| 66 | +### Confirmed-bug tests |
| 67 | +| # | MCP call | Expected outbound body | Red flag if body contains | |
| 68 | +|---|---|---|---| |
| 69 | +| 1 | `query_members {name: "alice"}` | `{"p":1, "limit":20, "query":"alice"}` | `"member_name"` | |
| 70 | +| 2 | `query_teams {name: "backend"}` | `{"p":1, "limit":20, "query":"backend"}` | `"team_name"` | |
| 71 | + |
| 72 | +### Regression tests (post-PR #43) |
| 73 | +| # | MCP call | Expected outbound body | |
| 74 | +|---|---|---| |
| 75 | +| 3 | `query_incidents {channel_ids: "100", start_time: T0, end_time: T1}` | `{"channel_ids":[100], ...}` | |
| 76 | +| 4 | `query_incidents {channel_ids: "100,200", ...}` | `{"channel_ids":[100,200], ...}` | |
| 77 | +| 5 | `query_incidents {...no channel_ids...}` | body MUST NOT contain `channel_ids` | |
| 78 | +| 6 | `query_changes {channel_ids: "100"}` | `{"channel_ids":[100], ...}` | |
| 79 | + |
| 80 | +### Smoke tests for the other 17 tools |
| 81 | +Auditor flagged these as clean; verify once by calling each with minimal args and confirming no `400 invalid parameter` from backend: |
| 82 | + |
| 83 | +Incidents: `query_incident_timeline`, `query_incident_alerts`, `list_similar_incidents`, `create_incident`, `update_incident`, `ack_incident`, `close_incident`. |
| 84 | +Status page: `query_status_pages`, `list_status_changes`, `create_status_incident`, `create_change_timeline`. |
| 85 | +Channels: `query_channels`, `query_escalation_rules`. |
| 86 | +Fields: `query_fields`. |
| 87 | + |
| 88 | +### Special case — `create_incident assigned_to` |
| 89 | +Call `create_incident` with `assigned_to: "100,101"`. Fetch the created incident and confirm persons 100 and 101 are actually listed as responders. If they're not, the wrapper shape is wrong. |
| 90 | + |
| 91 | +## Exit criteria |
| 92 | + |
| 93 | +- Tests 1–2: fail → open follow-up PR renaming `member_name`/`team_name` → `query`. |
| 94 | +- Tests 3–6: pass → PR #43 fix is correct end-to-end. |
| 95 | +- Smoke tests: 17/17 return 200 or a documented business error (not `400 invalid parameter`). |
| 96 | +- Test for `assigned_to`: responders actually assigned. |
| 97 | +- Glossary pass (section below): tool schemas use canonical terms only (no "collaboration space"). |
| 98 | + |
| 99 | +## Audit method reproducibility |
| 100 | + |
| 101 | +To re-audit after new tools are added: |
| 102 | +1. For each `mcp.NewTool("<name>", ...)` call in `pkg/flashduty/*.go`, capture the `mcp.With*` schema. |
| 103 | +2. Locate the corresponding `makeRequest(ctx, "POST", "/<endpoint>", requestBody)`. |
| 104 | +3. Grep the backend repos (`fc-event`, `fc-pgy`, `monit-webapi`) for the handler input struct tagged with matching `json:` fields. |
| 105 | +4. Compare field names, types (singular vs plural), and required fields. |
| 106 | +5. Flag any MCP-side key that doesn't appear as a backend `json:` tag — that key is silently dropped. |
| 107 | + |
| 108 | +## Glossary pass — canonical terminology |
| 109 | + |
| 110 | +Goal: keep user-visible strings (tool descriptions, parameter descriptions, READMEs) aligned with the canonical |
| 111 | +Flashduty glossary at `flashduty-docs/glossary.md` so agents and humans see consistent wording. |
| 112 | + |
| 113 | +### Terms checked (from `flashduty-docs/glossary.md`) |
| 114 | + |
| 115 | +| Chinese | Canonical English | Misnomers to hunt | |
| 116 | +|---|---|---| |
| 117 | +| 协作空间 | channel (lowercase in prose; "Channel" as class/section title) | collaboration space, collab space | |
| 118 | +| 故障 | incident | outage, alarm | |
| 119 | +| 告警 / 报警 | alert | alarm | |
| 120 | +| 集成来源 / 数据源 | integration | data source | |
| 121 | +| 分派策略 | escalation rule | — | |
| 122 | +| 成员 | member | — (wire uses `person_id`; see "Skipped" below) | |
| 123 | +| 处理人员 / 响应人员 | responder | acker, assignee | |
| 124 | + |
| 125 | +zh README (`README_zh.md`) is source of truth and already uses 协作空间; no zh-side edits were required. |
| 126 | + |
| 127 | +### Files edited in this pass |
| 128 | + |
| 129 | +- `pkg/flashduty/channels.go` — `queryChannelsDescription`, `channel_id` param description |
| 130 | +- `pkg/flashduty/incidents.go` — `channel_ids` and `channel_id` param descriptions |
| 131 | +- `pkg/flashduty/changes.go` — `channel_ids` param description |
| 132 | +- `pkg/flashduty/tools.go` — `channels` toolset description |
| 133 | +- `README.md` — toolset table + `channels` section heading + `query_channels` bullet |
| 134 | +- `e2e/README.md` — TestQueryChannels comment + limitations bullet |
| 135 | + |
| 136 | +### Verification |
| 137 | + |
| 138 | +1. `go build ./...` compiles cleanly. |
| 139 | +2. `rg -i "collaboration"` in the repo returns no hits. |
| 140 | +3. Start the server and call `list_tools` via MCP; confirm the `query_channels`, `query_escalation_rules`, |
| 141 | + `query_incidents`, `query_changes`, and `create_incident` schemas describe channels using "channel" (not "collaboration space"). |
| 142 | +4. Run existing snapshot tests (`go test ./pkg/flashduty/...`) — snapshots for these tools do not embed tool |
| 143 | + descriptions, so they should still pass without updates. |
| 144 | + |
| 145 | +### Skipped / uncertain |
| 146 | + |
| 147 | +- `person_ids` parameter and its description in `pkg/flashduty/users.go:26` and `pkg/flashduty/incidents.go:379`. |
| 148 | + Glossary says 成员 → "member", but `person_ids` is the wire-level backend field name. The description text |
| 149 | + explains what the parameter accepts ("Comma-separated person IDs"), so renaming the description would misalign |
| 150 | + with the literal parameter name. Leaving as-is; a future full rename would need to touch wire payload too. |
| 151 | +- All `Person*` Go identifiers, struct fields, and JSON tags (wire-level, not user-facing). |
| 152 | +- `partial_outage` / `full_outage` enum values in status-page tools — these are wire-level component status |
| 153 | + enums, not references to Flashduty 故障. |
0 commit comments