Skip to content

Commit ef827c3

Browse files
authored
Fix/agent repl ux (#151)
* docs: add Quick Install section near top of README - Add `## Quick Install` block (gh auth login + npm install -g + run) immediately after the warning callouts, so new users see the install path within the first screen - Link forward to the existing `## Install and Run` section for Docker, source builds, and full prerequisites - Prettier also realigned the capability table dividers in `How It Works` (pre-existing alignment drift from when the `execute_bash` row was added) — pure formatting, no content change Signed-off-by: Simon Davies <simongdavies@users.noreply.github.com> * fix(agent): three skill-system bugs from kql-expert hallucination Bug 1 — sandbox verbose traces leak to user terminal src/sandbox/tool.js gained a debugLog callback option that ~25 verbose-gated console.error sites now route through, defaulting to console.error when no callback is wired. src/agent/index.ts passes its existing debugLog (which writes ~/.hyperagent/logs/agent-debug- *.log when --debug is on) into both sandbox factories, so the noisy '[sandbox] setPlugins / invalidateSandboxWithSave / autoSaveState' chatter stops leaking into the REPL. Bug 2 — generate_skill silently shadowed bundled built-in skills skill-writer now exports systemSkillExists(name, dir) alongside the existing userSkillExists. generate_skill refuses (without an explicit overwrite=true) when the requested name matches a curated built-in under <CONTENT_ROOT>/skills/, with an error pointing at a safer '-custom' suffix. When overwrite=true *is* set on a system collision, the approval banner shouts ⚠️ SHADOW built-in skill and the y/n prompt reads 'Shadow built-in skill? [y/n]' so the user can't sleep through a destructive override. Added 4 tests for systemSkillExists covering present/missing/invalid-name cases. Bug 3 — '/skills <name>' didn't actually invoke the skill The Copilot SDK speaks /<skillname>, not /skills <skillname>. The REPL was forwarding the raw '/skills kql-expert' string, which the SDK couldn't parse — the LLM saw it as a natural-language request and sometimes mis-fired generate_skill to 'save kql-expert', triggering Bug 2. The intake layer in agent/index.ts now rewrites '/skills <name>' → '/<name>' before dispatch (subcommands info|edit| delete|list are left alone). slash-commands.ts bridges the same way for synthetic callers and the default case now recognises user skills in addition to system skills. Listing footer updated to show 'Invoke: /<name>'. Quality gate: just lint clean, 2452 TS tests pass, 124 Rust tests pass. 🎬 Number Five says verbose-log is alive! Signed-off-by: Simon Davies <simongdavies@users.noreply.github.com> * fix(agent): markdown UX, MCP shortcut surfacing, skill hot-reload, profile preview table Four intertwined fixes for the interactive REPL experience: • Markdown rendering: patched marked-terminal v7's broken `text` renderer (it used token.text raw instead of recursing via parseInline, so **bold** inside tight list items leaked asterisks to the terminal); flipped showSectionPrefix to false so headings render cleanly without ### prefix; made /markdown queryable (status/on/off/toggle, no toggle-trap); removed the looksLikeMarkdown() gate that produced inconsistent output for opted-in users; fixed two callsites printing literal **Configuration:** via console.log. • MCP shortcut surfacing: expanded MCP_SETUP_COMMANDS map to all 5 supported shortcuts (was 1); restructured formatGuidance() so missing-MCP prerequisites land at the TOP under MISSING PREREQUISITES (was buried mid-document, model ignored it); distinguished supported (→specific shortcut) vs unsupported (→config.json) servers; removed the synthesized --mcp-setup-${name} fallback that would emit non-existent flags for unsupported servers. • Skill hot-reload: exposed the SDK's built-in skill tool in ALLOWED_TOOLS + availableTools; added /skills reload subcommand calling session.rpc.skills.reload(); added 'reload' to RESERVED_SKILL_NAMES; auto-reload after generate_skill writes so freshly authored skills are invocable mid-session. • Profile-apply preview table: applyProfileImpl now emits a proper markdown table (Limit/Before/After columns) via renderMarkdown when /markdown is on; marked-terminal converts it to a unicode box-drawing table with bold headers, much easier to scan than the previous flat 'cpu: 1000ms → 2000ms' list. Plain-text fallback preserved verbatim for markdown-off sessions. Regression tests added throughout (markdown-renderer asserts absence of ** markers, absence of ### heading prefix, and GFM table cells survive rendering; pattern-integrity asserts MCP shortcuts list stays in sync with CLI; approach-resolver asserts ordering and no-fake-flag policy). Signed-off-by: Simon Davies <simongdavies@users.noreply.github.com> * fix(agent): address PR #151 review — /skills rewrite + path traversal Two issues caught by the Copilot reviewer on PR #151: • /skills <name> → /<name> rewrite was breaking /skills reload. The local KNOWN_SKILLS_SUBS set in src/agent/index.ts omitted "reload", so the new hot-reload subcommand got rewritten to /reload and never reached the handler. Fix: gate the rewrite on validateSkillName(token) === null instead — that consults RESERVED_SKILL_NAMES in skill-writer.ts, which already lists every /skills subcommand (single source of truth, no parallel hardcoded set that can drift). • Slash-command default-case skill detection used existsSync(join(skillsDir, cmd.slice(1), "SKILL.md")) with unvalidated user input. A literal /../etc would resolve outside skillsDir, turning the "is this a skill?" check into an arbitrary filesystem probe. Fix: route through systemSkillExists(skillName, skillsDir) which calls validateSkillName() first, rejecting empty / oversized / path-traversal / reserved names before any join touches disk. Regression tests in tests/pattern-integrity.test.ts assert both the new gating mechanism and the absence of the old unsafe patterns. Signed-off-by: Simon Davies <simongdavies@users.noreply.github.com> --------- Signed-off-by: Simon Davies <simongdavies@users.noreply.github.com>
1 parent 8e36e53 commit ef827c3

12 files changed

Lines changed: 1102 additions & 143 deletions

src/agent/approach-resolver.ts

Lines changed: 57 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -13,10 +13,23 @@ import { loadPatterns } from "./pattern-loader.js";
1313
import { loadModule, type ModuleHints } from "./module-store.js";
1414

1515
// ── MCP server name → CLI setup command mapping ──────────────────────
16-
// Maps an MCP server name (as declared in skills) to the CLI flag that
17-
// configures it. Used by formatGuidance() to show actionable hints.
16+
// Maps an MCP server name (the key written into ~/.hyperagent/config.json
17+
// under `mcpServers`) to the CLI flag that configures it. When a skill
18+
// requires a server that is NOT yet configured, formatGuidance() uses
19+
// this table to recommend the specific shortcut to the LLM (and through
20+
// it, to the user). Servers absent from this table fall back to the
21+
// generic "edit ~/.hyperagent/config.json" guidance — we deliberately
22+
// don't suggest a `--mcp-setup-${name}` flag for unsupported names
23+
// because that flag does NOT exist and would set the user up for failure.
24+
//
25+
// Keep in sync with the setup functions in src/agent/mcp/setup-commands.ts
26+
// and the CLI cases in src/agent/cli-parser.ts.
1827
const MCP_SETUP_COMMANDS: Record<string, string> = {
1928
"fabric-rti-mcp": "--mcp-setup-fabric-rti",
29+
everything: "--mcp-setup-everything",
30+
github: "--mcp-setup-github",
31+
filesystem: "--mcp-setup-filesystem",
32+
workiq: "--mcp-setup-workiq",
2033
};
2134

2235
// ── Types ────────────────────────────────────────────────────────────
@@ -253,7 +266,42 @@ const GENERIC_GUIDANCE: MaterialisedGuidance = {
253266
export function formatGuidance(guidance: MaterialisedGuidance): string {
254267
const parts: string[] = ["--- TASK GUIDANCE ---"];
255268

256-
// Anti-patterns and rules go FIRST — the LLM is most likely to follow
269+
// ── Unconfigured MCP servers go FIRST as a blocker ──────────────
270+
// A missing prerequisite is the single most important thing the LLM
271+
// needs to surface to the user. Buried under "Rules:" or "Modules:"
272+
// it gets ignored — the model has been observed to skip the hint and
273+
// hallucinate config.json snippets or non-existent CLI flags instead.
274+
// Promoting this block to the top, with strong "TELL THE USER" wording,
275+
// measurably improves the user-visible recommendation quality.
276+
const missingMcp = guidance.mcpStatus.filter((s) => !s.configured);
277+
if (missingMcp.length > 0) {
278+
parts.push("🛑 MISSING PREREQUISITES — tell the user how to fix this:");
279+
for (const s of missingMcp) {
280+
const setupFlag = MCP_SETUP_COMMANDS[s.name];
281+
if (setupFlag) {
282+
// Explicitly supported — recommend the specific shortcut.
283+
parts.push(
284+
` ❌ MCP server "${s.name}" is required but not configured.`,
285+
` SHORTCUT: have the user run \`hyperagent ${setupFlag}\` ` +
286+
`(exits after writing config, then restart hyperagent).`,
287+
);
288+
} else {
289+
// Not in the supported-shortcuts table — give honest generic
290+
// guidance. DO NOT suggest a `--mcp-setup-${name}` flag here;
291+
// that flag does not exist and the user will hit "unknown
292+
// option" if you say it does.
293+
parts.push(
294+
` ❌ MCP server "${s.name}" is required but not configured.`,
295+
` This server has no built-in setup shortcut. Tell the ` +
296+
`user to add it to \`~/.hyperagent/config.json\` under ` +
297+
`\`mcpServers\` (see https://modelcontextprotocol.io for ` +
298+
`the entry shape), then restart hyperagent.`,
299+
);
300+
}
301+
}
302+
}
303+
304+
// Anti-patterns and rules — the LLM is most likely to follow
257305
// instructions at the top of the context injection.
258306
if (guidance.antiPatterns.length > 0) {
259307
parts.push("⚠️ DO NOT:");
@@ -278,15 +326,13 @@ export function formatGuidance(guidance: MaterialisedGuidance): string {
278326
`Plugins: ${guidance.plugins.join(", ")} — enable via manage_plugin or apply_profile`,
279327
);
280328
}
281-
if (guidance.mcpStatus.length > 0) {
329+
// MCP server status block — only shows CONFIGURED servers here.
330+
// Unconfigured ones are surfaced above as missing prerequisites.
331+
const configuredMcp = guidance.mcpStatus.filter((s) => s.configured);
332+
if (configuredMcp.length > 0) {
282333
parts.push("MCP Servers:");
283-
for (const s of guidance.mcpStatus) {
284-
if (!s.configured) {
285-
const setupFlag = MCP_SETUP_COMMANDS[s.name] ?? `--mcp-setup-${s.name}`;
286-
parts.push(
287-
` ❌ ${s.name} — not configured. Run: hyperagent ${setupFlag}`,
288-
);
289-
} else if (s.state === "connected") {
334+
for (const s of configuredMcp) {
335+
if (s.state === "connected") {
290336
parts.push(
291337
` ✅ ${s.name} — connected (${s.toolCount ?? 0} tools). Import from host:mcp-${s.name}`,
292338
);

0 commit comments

Comments
 (0)