Skip to content

Commit cd2b2ca

Browse files
committed
Merge branch 'fix/remaining-issues'
Clears the remaining known issues from the agent investigation: - POST /users/admin/reset 500'd on the mcp_tokens FK once setup-agent.sh had minted an admin token - no way to forget a stale CLI profile short of editing auth.json - install.sh never mentioned the DeepSQL CLI, so it silently went stale - smoke-test.sh sat mute for up to 20 minutes during brain init - CLAUDE.md now records the SDK pin and the verification anti-patterns
2 parents 8ccb970 + e301980 commit cd2b2ca

14 files changed

Lines changed: 495 additions & 16 deletions

File tree

CLAUDE.md

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -178,6 +178,65 @@ returns a number).
178178
- Do NOT commit automatically — wait for explicit user instruction.
179179
- Conventional commits: `feat:`, `fix:`, `refactor:`, `docs:`, `test:`, `chore:`, `perf:`, `ci:`
180180

181+
### Agent Runtime Rules (learned the hard way, 2026-08-07)
182+
183+
1. **Pin the Python MCP SDK below 2.0.** `scripts/self-host/setup-agent.sh` installs
184+
it as `mcp>=1.0,<2`. SDK **2.0.0 renamed `CallToolResult.isError` to `is_error`**
185+
and split the models into a separate `mcp-types` package, while `hermes-agent`
186+
0.20.0 still reads `result.isError` (`tools/mcp_tool.py:5222`). With the old
187+
unbounded `mcp>=1.0`, every DeepSQL tool call raised
188+
`AttributeError: 'CallToolResult' object has no attribute 'isError'` — a time bomb
189+
that detonated the day 2.0.0 shipped, with no code change on our side. Raise the
190+
ceiling only once hermes reads `is_error`.
191+
2. **Three tool failures trip hermes's circuit breaker**, after which the rest are
192+
refused as `MCP server 'deepsql' is unreachable` — blaming a healthy server for a
193+
client-side parse error. Do not trust that message; find the *first* failure in
194+
`~/.hermes/logs/errors.log`.
195+
3. **A running webui holds its SDK in memory.** After changing the SDK it must be
196+
restarted; `setup-agent.sh` now does that itself. It previously printed
197+
`✓ Hermes webui already running` and left the broken SDK loaded, so re-running the
198+
repair script gave a full column of ticks and no change.
199+
4. **The agent version was never the problem.** `agent/distribution.yaml` once pinned
200+
`hermes_requires <0.20.0` on a "verified" 401 that came from a hand-rolled
201+
`hermes serve` run rather than `hermes webui`. 0.20.0 works. Verify against the
202+
real start path before writing a version constraint.
203+
204+
### Verification Anti-Patterns (do not repeat)
205+
206+
These all reported success over broken systems — which is how the agent shipped
207+
broken. Assert the *outcome*, never the attempt:
208+
209+
- **`e2e-agent-check.py`** passed on `any("execute_sql" in t for t in tools)` — a tool
210+
being *attempted*. It printed `✓ All agent UI paths OK` and exited 0 while the
211+
agent's own reply said "I'm blocked". It now requires the answer itself.
212+
- **A dashboard that is "HTML and long"** proves nothing: with every tool failing, the
213+
agent emitted a plausible artifact full of invented numbers. A real one calls
214+
`deepsql.query()`; absence of that call means the data never came from the database.
215+
- **Presence ≠ compatibility.** The SDK check tested only that `mcp` imports, so it
216+
printed `✓ Python MCP SDK available` on an SDK whose every call failed. It now
217+
asserts `CallToolResult` still carries `isError`.
218+
- **Mocks hide SDK breaks.** `tests/tools/test_mcp_structured_content.py` uses a
219+
`_FakeCallToolResult` with a hardcoded `.isError`, so it kept passing precisely when
220+
the real SDK stopped matching. Pin the dependency; a fake cannot catch this.
221+
- **Never claim a check you did not run.** `install.sh` reported "up to date" when it
222+
could not reach npm; it now says it could not check.
223+
- **`set -e` + `read` at EOF aborts silently.** Prompts in `install.sh` use
224+
`read … || true` so the explicit emptiness checks report the problem. Without it the
225+
installer exited 1 with no message, after writing generated secrets to `.env`.
226+
- **Silent-failure rule, concretely:** the CLI rendered an unreachable server as
227+
`No databases connected yet` because one `catch` covered both the connection fetch
228+
and decorative extras. An unreachable host must never look like an empty account.
229+
230+
### Data Model Rules
231+
232+
- **`mcp_tokens.user_id` is a non-null FK with no cascade.** Deleting a user who holds
233+
a token throws `ConstraintViolationException`. `UserController` clears the user's
234+
tokens first via `McpTokenRepository.deleteByUserId`, which carries its own
235+
`@Transactional` — a derived delete needs one, and annotating a self-invoked caller
236+
does nothing (Spring proxies are bypassed by `this::`). This broke
237+
`POST /users/admin/reset` on every install that had run `setup-agent.sh`, since that
238+
mints an admin MCP token on each run.
239+
181240
### MCP & CLI Release Rules
182241

183242
**Whenever you add, rename, or remove an MCP tool or a CLI subcommand, you MUST update all of these in the same commit — they are agent-facing surfaces and drift silently breaks discoverability:**

backend/src/main/java/com/dbaagent/controller/UserController.java

Lines changed: 34 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
import com.dbaagent.model.User;
44
import com.dbaagent.model.UserAccountStatus;
5+
import com.dbaagent.repository.McpTokenRepository;
56
import com.dbaagent.repository.UserRepository;
67
import com.dbaagent.service.SystemConfigService;
78
import lombok.RequiredArgsConstructor;
@@ -30,6 +31,7 @@
3031
public class UserController {
3132

3233
private final UserRepository userRepository;
34+
private final McpTokenRepository mcpTokenRepository;
3335
private final PasswordEncoder passwordEncoder;
3436
private final SystemConfigService systemConfigService;
3537

@@ -94,7 +96,7 @@ public ResponseEntity<?> resetAdmin(@RequestBody Map<String, String> request, Ht
9496
if (password == null || password.isBlank()) {
9597
return ResponseEntity.badRequest().body(Map.of("message", "Password is required"));
9698
}
97-
userRepository.findByUsername("admin").ifPresent(userRepository::delete);
99+
userRepository.findByUsername("admin").ifPresent(this::deleteUserAndOwnedCredentials);
98100
User user = new User();
99101
user.setUsername("admin");
100102
user.setPassword(passwordEncoder.encode(password));
@@ -117,10 +119,40 @@ public ResponseEntity<?> deleteUser(@PathVariable String username) {
117119
if (userOpt.isEmpty()) {
118120
return ResponseEntity.status(404).body(Map.of("message", "User not found"));
119121
}
120-
userRepository.delete(userOpt.get());
122+
deleteUserAndOwnedCredentials(userOpt.get());
121123
return ResponseEntity.ok(Map.of("message", "User deleted successfully"));
122124
}
123125

126+
/**
127+
* Delete a user together with the credentials that belong to them.
128+
*
129+
* <p>{@code mcp_tokens.user_id} is a non-null FK with no cascade, so a plain
130+
* {@code userRepository.delete(user)} throws
131+
* {@code ConstraintViolationException: update or delete on table "users"
132+
* violates foreign key constraint "fkhm5walli9xtthjoek1ia4paqg" on table
133+
* "mcp_tokens"} — surfacing to the caller as a bare 500.
134+
*
135+
* <p>It is reachable on any self-host install: {@code setup-agent.sh} mints an
136+
* MCP token for the admin every run, so setting up the agent was enough to
137+
* make {@code POST /users/admin/reset} fail permanently — exactly when an
138+
* operator locked out of the UI needs it most.
139+
*
140+
* <p>Deleting the tokens is the correct behaviour and not merely a way to
141+
* satisfy the constraint: they are bearer credentials scoped to this user, and
142+
* leaving them behind would orphan working credentials whose owner is gone.
143+
*
144+
* <p>The transaction lives on {@code McpTokenRepository.deleteByUserId}. Putting
145+
* {@code @Transactional} here would be inert: both callers reach this method by
146+
* self-invocation, which never passes through the Spring proxy.
147+
*/
148+
private void deleteUserAndOwnedCredentials(User user) {
149+
long revoked = mcpTokenRepository.deleteByUserId(user.getId());
150+
if (revoked > 0) {
151+
log.info("Revoked {} MCP token(s) belonging to user {} before deletion", revoked, user.getUsername());
152+
}
153+
userRepository.delete(user);
154+
}
155+
124156
private Map<String, Object> toUserSummary(User user) {
125157
Map<String, Object> summary = new java.util.LinkedHashMap<>();
126158
summary.put("id", user.getId());

backend/src/main/java/com/dbaagent/repository/McpTokenRepository.java

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
import com.dbaagent.model.McpToken;
44
import org.springframework.data.jpa.repository.JpaRepository;
5+
import org.springframework.transaction.annotation.Transactional;
56

67
import java.util.List;
78
import java.util.Optional;
@@ -11,4 +12,27 @@ public interface McpTokenRepository extends JpaRepository<McpToken, Long> {
1112
List<McpToken> findByUserIdOrderByCreatedAtDesc(Long userId);
1213
Optional<McpToken> findByIdAndUserId(Long id, Long userId);
1314
boolean existsByPublicId(String publicId);
15+
16+
/**
17+
* Remove every token belonging to a user. {@code mcp_tokens.user_id} is a
18+
* non-null FK with no cascade, so a user holding tokens cannot be deleted —
19+
* the delete fails with
20+
* {@code ConstraintViolationException: update or delete on table "users"
21+
* violates foreign key constraint on table "mcp_tokens"}.
22+
*
23+
* <p>This bit self-host in practice: {@code setup-agent.sh} mints a token for
24+
* the admin on every run, so merely setting up the agent made
25+
* {@code POST /users/admin/reset} return 500 from then on.
26+
*
27+
* <p>Tokens are credentials owned by the user, so removing them with the user
28+
* is the correct semantics rather than a workaround — and leaving them behind
29+
* would orphan live credentials.
30+
*
31+
* <p>{@code @Transactional} is required here, not decorative: a derived delete
32+
* query needs an active transaction, and Spring Data's repository proxy is the
33+
* only place that reliably supplies one. Annotating the caller instead does
34+
* nothing when the caller reaches this through self-invocation.
35+
*/
36+
@Transactional
37+
long deleteByUserId(Long userId);
1438
}
Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,106 @@
1+
package com.dbaagent.repository;
2+
3+
import com.dbaagent.model.McpToken;
4+
import com.dbaagent.model.User;
5+
import org.junit.jupiter.api.Test;
6+
import org.springframework.beans.factory.annotation.Autowired;
7+
import org.springframework.boot.test.context.SpringBootTest;
8+
import org.springframework.test.context.ActiveProfiles;
9+
import org.springframework.transaction.annotation.Transactional;
10+
11+
import java.time.LocalDateTime;
12+
13+
import static org.junit.jupiter.api.Assertions.assertEquals;
14+
import static org.junit.jupiter.api.Assertions.assertTrue;
15+
16+
/**
17+
* Regression cover for the FK that made {@code POST /users/admin/reset} return 500.
18+
*
19+
* <p>{@code mcp_tokens.user_id} is a non-null FK with no cascade, so deleting a user
20+
* who holds a token failed with
21+
* {@code ConstraintViolationException: update or delete on table "users" violates
22+
* foreign key constraint "fkhm5walli9xtthjoek1ia4paqg" on table "mcp_tokens"}.
23+
*
24+
* <p>Reachable on every self-host install: {@code setup-agent.sh} mints an MCP token
25+
* for the admin on each run, so setting up the agent was enough to permanently break
26+
* the admin-reset escape hatch — exactly when an operator locked out of the UI needs
27+
* it most. {@code UserController} now clears a user's tokens before deleting them.
28+
*/
29+
@SpringBootTest
30+
@ActiveProfiles("test")
31+
@Transactional
32+
class McpTokenUserDeletionTest {
33+
34+
@Autowired
35+
private UserRepository userRepository;
36+
37+
@Autowired
38+
private McpTokenRepository mcpTokenRepository;
39+
40+
private User persistUser(String username) {
41+
User user = new User();
42+
user.setUsername(username);
43+
user.setPassword("not-a-real-hash");
44+
user.setEmail(username + "@example.invalid");
45+
user.setRole("ADMIN");
46+
return userRepository.saveAndFlush(user);
47+
}
48+
49+
private void persistToken(User user, String publicId) {
50+
McpToken token = new McpToken();
51+
token.setUser(user);
52+
token.setName("self-host-agent");
53+
token.setPublicId(publicId);
54+
token.setTokenPrefix("dsql_test");
55+
token.setTokenHash(new byte[] { 1, 2, 3, 4 });
56+
token.setStatus(McpToken.Status.ACTIVE);
57+
token.setCreatedAt(LocalDateTime.now());
58+
mcpTokenRepository.saveAndFlush(token);
59+
}
60+
61+
@Test
62+
void deleteByUserIdClearsTheWayForUserDeletion() {
63+
User user = persistUser("admin-reset-ok");
64+
persistToken(user, "pub-reset-1");
65+
persistToken(user, "pub-reset-2");
66+
Long userId = user.getId();
67+
68+
assertEquals(2, mcpTokenRepository.findByUserIdOrderByCreatedAtDesc(userId).size());
69+
70+
long revoked = mcpTokenRepository.deleteByUserId(userId);
71+
assertEquals(2, revoked, "both of the user's tokens must be removed");
72+
73+
// The delete that used to throw a FK ConstraintViolationException.
74+
userRepository.delete(user);
75+
userRepository.flush();
76+
77+
assertTrue(userRepository.findById(userId).isEmpty(), "user must be gone");
78+
assertTrue(mcpTokenRepository.findByUserIdOrderByCreatedAtDesc(userId).isEmpty(),
79+
"the user's tokens must not outlive them");
80+
}
81+
82+
@Test
83+
void deleteByUserIdLeavesOtherUsersTokensAlone() {
84+
User keep = persistUser("keeper");
85+
User drop = persistUser("dropped");
86+
persistToken(keep, "pub-keep");
87+
persistToken(drop, "pub-drop");
88+
89+
long revoked = mcpTokenRepository.deleteByUserId(drop.getId());
90+
assertEquals(1, revoked);
91+
92+
assertEquals(1, mcpTokenRepository.findByUserIdOrderByCreatedAtDesc(keep.getId()).size(),
93+
"deleting one user's tokens must not touch another's");
94+
assertTrue(mcpTokenRepository.findByUserIdOrderByCreatedAtDesc(drop.getId()).isEmpty());
95+
}
96+
97+
@Test
98+
void deleteByUserIdIsSafeForAUserWithNoTokens() {
99+
User user = persistUser("tokenless");
100+
assertEquals(0, mcpTokenRepository.deleteByUserId(user.getId()));
101+
102+
userRepository.delete(user);
103+
userRepository.flush();
104+
assertTrue(userRepository.findById(user.getId()).isEmpty());
105+
}
106+
}

mcp/README.md

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,15 @@ Both share one auth file (`~/.config/deepsql/auth.json`, mode 0600). Log
1717
in once with `deepsql login`; the MCP server uses the same token
1818
automatically — no token needs to be embedded in your editor's config.
1919

20+
One profile is saved per DeepSQL URL, and logging in to a second host does
21+
**not** make it active. Check which host bare `deepsql` will use:
22+
23+
```bash
24+
deepsql config show # list profiles, * marks the default
25+
deepsql config set-default <url> # choose which host bare `deepsql` uses
26+
deepsql config remove <url> # forget a host you no longer run
27+
```
28+
2029
## Install
2130

2231
```bash

mcp/package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
{
22
"name": "@deepsql/mcp",
3-
"version": "0.26.1",
3+
"version": "0.27.0",
44
"description": "DeepSQL CLI, DBA Agent (thin client), and stdio MCP server for self-hosted deployments",
55
"bin": {
66
"deepsql": "bin/deepsql.js",

mcp/skills/SKILL_BODY.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -149,7 +149,7 @@ deepsql query "SELECT 1" --connection prod-pg --caller-agent claude-code --json
149149
| `deepsql login` | Authorize CLI against a DeepSQL host (browser PKCE / device code / password) | none — interactive only |
150150
| `deepsql logout` | Revoke the saved token | none |
151151
| `deepsql whoami` | Show the logged-in user, role, URL, pinned connection | none |
152-
| `deepsql config show\|set-default <url>\|path` | Manage saved profiles | none |
152+
| `deepsql config show\|set-default <url>\|remove <url>\|path` | Manage saved profiles (one per DeepSQL URL) | none |
153153
| `deepsql mcp` | Run the stdio MCP server | this skill spawns it |
154154
| `deepsql mcp config --install --for <editor>` | Install MCP entry + this skill into editor config | none — interactive |
155155
| `deepsql connections list\|use\|current\|unset\|schema\|add\|update\|remove\|test\|show\|init` | Full connection CRUD | partial: `list_connections`, `get_schema` |

mcp/src/auth/store.js

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,12 @@ function removeProfile(baseUrl) {
121121
delete state.profiles[key];
122122
if (state.default === key) {
123123
const remaining = Object.keys(state.profiles);
124-
state.default = remaining[0] || null;
124+
// One survivor is unambiguous, so adopt it. Two or more is a choice only the
125+
// user can make: `remaining[0]` is insertion order — "whichever host you
126+
// logged into first" — the same implicit guess `deepsql login` refuses to
127+
// make. Silently repointing the default there would aim the CLI at a
128+
// different database without saying so.
129+
state.default = remaining.length === 1 ? remaining[0] : null;
125130
}
126131
save(state);
127132
}

mcp/src/cli.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -155,6 +155,7 @@ const COMMAND_HELP = {
155155
subcommands: [
156156
["show", "List saved profiles (default)"],
157157
["set-default <url>", "Set the default profile"],
158+
["remove <url>", "Forget a saved profile (e.g. a host you no longer run)"],
158159
["path", "Print the auth file path"],
159160
],
160161
},

mcp/src/cli.test.js

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -138,6 +138,10 @@ test("--no-color suppresses ANSI escapes in help output", async () => {
138138
const HELP_DRIFT_TARGETS = [
139139
{ command: "slow-queries", modulePath: "./commands/slow-queries" },
140140
{ command: "brain", modulePath: "./commands/brain" },
141+
// `config` dispatched from a bare switch with no SUBCOMMANDS export, so it sat
142+
// outside this guard entirely and its help could drift unnoticed. It now
143+
// exports the map like its siblings.
144+
{ command: "config", modulePath: "./commands/config" },
141145
];
142146

143147
for (const { command, modulePath } of HELP_DRIFT_TARGETS) {

0 commit comments

Comments
 (0)