feat(agent): Harper best-practices grounding + agent.systemPromptAppend (#626)#1560
Open
kriszyp wants to merge 10 commits into
Open
feat(agent): Harper best-practices grounding + agent.systemPromptAppend (#626)#1560kriszyp wants to merge 10 commits into
kriszyp wants to merge 10 commits into
Conversation
…tered (#626) Fold the unified MCP tool registry's Operations profile (#617) into the built-in agent's toolset, fulfilling #626's tool-composition design: the agent consumes the same registry the MCP server exposes, RBAC-filtered for its configured user. Re-does the intent of the abandoned #893 against current main (that branch was ~465 commits stale and used the pre-merge registry API). - agent/registryTools.ts (new): ensureOperationsToolsRegistered() populates the Operations profile on the main thread (idempotent); composeRegistryTools() snapshots the profile, filters by visibleTo(agentUser), and adapts each ToolDef -> AgentTool (inputSchema->parameters, destructiveHint->approval gate, ToolResult->return value with isError->throw so the loop records a recoverable failure rather than aborting). - agent/agent.ts: resolve the configured agent user via server.getUser (falls back to a super_user identity with a warning when unresolved) and thread the registry tools through both composeToolset calls. - agent/toolset.ts: merge operator-only + registry tools; operator-only tools win on a name collision. - unitTests/agent/registryTools.test.js: 10 tests over RBAC filtering, shape adaptation, destructive gating, result unwrapping, and the collision rule. No server boot / no LLM credits. Enforcement note: visibleTo controls listing only; real RBAC runs per-call in the operation handler via hdb_user set to the agent's identity. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Cross-model review (Codex + Gemini) flagged two authorization defects in the
registry integration; both fixed at root:
- Fail-open super_user: resolveAgentUser previously fabricated a
{ super_user: true } identity whenever agent.user couldn't be resolved,
silently escalating a misconfigured or transient restricted service account to
admin. Now resolveAgentIdentity only falls back to super_user for the *default*
hdb_agent bootstrap user (whose provisioning #626 defers); an explicitly
configured user that won't resolve throws (fail closed), and the agent runs with
only its operator-only tools until the operator fixes agent.user.
- Stale cached identity: the agent user was resolved once at startup and closed
over in every tool handler, so a role revocation/change wasn't honored until
restart. The enforcement identity is now resolved *per call* (composeRegistryTools
takes a resolveIdentity thunk), mirroring how the MCP HTTP path re-auths per
request. The startup snapshot is used only for visibleTo listing (not a boundary).
Adjudicated out: Gemini's "setConfig ignores agent.user changes" — set_agent_config
does not accept `user` (not in its patch keys), so it can't change at runtime.
Adds 2 unit tests: per-call re-resolution honors a live role change; a fail-closed
rejection propagates and the operation never dispatches. 51 agent unit tests green.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…#626) Adds the operator-only inspector tool surface from the #626 design — the last of the operator tools not yet built. The agent (main thread) attaches over the Chrome DevTools Protocol to *worker* thread inspector ports to evaluate, set breakpoints/logpoints, and CPU-profile a worker without stalling the thread it runs on. - agent/tools/inspectorTool.ts (new): buildInspectorTools(deps) → five tools: inspector_attach, inspector_evaluate, inspector_set_breakpoint, inspector_set_logpoint, inspector_profile_cpu. A minimal CDP client over `ws` (id-correlated request/response + event listeners), one reused connection per worker debug port. Deps (debug config + live worker count) are injected so the tool is unit-testable without a server boot. - Safety envelope (resolvePort): requires threads_debug + threads_debug_startingPort, range-checks workerIndex against the live worker pool, and REJECTS workerIndex < 0 (the main thread — a self-attach/breakpoint would deadlock the agent). - Worker debug port = threads_debug_startingPort + workerIndex (mirrors server/threads/threadServer.js). Logpoints are non-pausing (breakpoint whose condition console.logs and returns false). CPU profiles are summarized to the hottest functions by self time so the observation stays small. - evaluate + set_breakpoint are marked destructive (arbitrary in-worker code / can pause a live worker) → gated by the loop's approval flow. - agent/toolset.ts + agent/agent.ts: compose inspector tools into the operator-only set; deps read from env (threads_debug*) and the live `workers` pool. - unitTests/agent/inspectorTool.test.js: safety-envelope guards, summarizeProfile, and a LIVE CDP round-trip (opens a real inspector on an ephemeral port; drives attach + evaluate + profile). 60 agent unit tests green. Stacked on #1545 (registry tools). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Cross-model review (Gemini) flagged real lifecycle/concurrency hazards in the CDP tooling; all addressed: - openCdp never settled if the socket closed/aborted before 'open' → sessionFor hung the agent. Now a pre-open close/error/abort rejects the connect promise. - Session-cache races: cache the connection *promise* (dedupes concurrent opens); evict by identity so a stale close from a replaced connection can't drop a live session; drop a failed connect so the next call retries. - Breakpoints could wedge a worker (paused, no resume path). Every connection now registers a Debugger.paused handler that logs a stack snapshot and auto-resumes, so a hit is observable via the Harper log but never leaves the worker paused. - Logpoint injection: logExpression is now JSON-encoded and eval'd (not spliced as raw code), so it can't break out of the wrapper to force a pause. set_logpoint is also marked destructive (it runs an expression in the worker on every hit), alongside evaluate and set_breakpoint. - CDP calls now carry a per-call abort signal + 30s timeout, so an unresponsive worker can't hang the agent loop. The connect signal guards only the handshake (aborting one caller no longer tears down the shared connection). - Strict workerIndex parsing: reject ""/null/false/[] instead of Number()-coercing them to worker 0. - Profiler.disable on cleanup; resolvePort rejects a port past 65535. - _closeInspectorSessions handles in-flight opens. Also fixes a test-teardown deadlock: inspector.close() blocks until CDP clients drop, so the live-round-trip after() hook only closes our client and lets --exit tear down the inspector. Full agent unit suite: 61 passing, build clean. Codex leg returned no structured findings. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…nd (#626) Gives the built-in agent Harper conventions so it builds idiomatic apps, and lets operators tune its persona/policy without a rebuild. - Depends on @harperfast/skills and sources the `harper-best-practices` skill from it (versioned, no drift). Progressive disclosure, mirroring the skill's own design: * agent/bestPractices.ts (new): loadBestPracticesOverview() injects the SKILL.md overview (rule index + when-to-use, ~1.2k tokens) into the system prompt; buildBestPracticeTool() exposes a `harper_best_practice` tool that lists rules (no arg) or returns rules/<name>.md on demand — so the agent only spends context on the guidance relevant to the task. Both degrade to nothing if the package isn't resolvable (agent still runs). Rule arg is guarded against path traversal. - agent.systemPromptAppend: operator text appended after the built-in grounding and the best-practices overview. Read from liveConfig each run, and accepted by set_agent_config, so it can be tuned on a running instance. Added to the config schema and AgentConfig. - System prompt is now assembled: built-in grounding → best-practices overview → operator append. Stacked on the inspector PR (kris/agent-inspector). Agent unit suite green (incl. new bestPractices tests exercising the real skill package). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request integrates Harper best-practices knowledge from the @harperfast/skills package into the built-in agent, injecting an overview into the system prompt and registering a tool to retrieve detailed rules on demand. It also adds a systemPromptAppend configuration option for custom operator instructions. The review feedback highlights critical improvements: replacing synchronous file system operations with asynchronous ones to avoid blocking the main thread's event loop, ensuring ESM compatibility by falling back to createRequire when require is undefined, and validating the append configuration property to prevent downstream TypeErrors.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
|
One blocker: |
dawsontoth
reviewed
Jul 2, 2026
Base automatically changed from
kris/agent-inspector
to
kris/agent-registry-tools
July 2, 2026 16:11
… test (#626) node:inspector is a single process-wide agent. In CI, threads_debug defaults to true (installer.ts DEV_MODE_CONFIG), and server/threads/threadServer.js opens the main-thread inspector as a top-level side effect of merely being imported (e.g. via DurableSubscriptionsSession.ts pulling in whenComponentsLoaded) whenever that config is on. That leaves this process's one inspector slot already occupied by the time the "live CDP round-trip" suite's before() hook runs, so inspector.open() throws ERR_INSPECTOR_ALREADY_ACTIVATED. Check inspector.url() first and reuse whatever is already listening instead of assuming the suite is the sole owner of the process's debug port. Verified by reproducing the exact CI error locally (mocha --require a script that opens the inspector before test files load), confirming the fix resolves it, and running the suite in isolation, within unitTests/agent/**, and across 3 full unitTests/**/*test.*js runs (2985 passing, 0 failing each time).
Lets any MCP client drive the built-in agent (agent_prompt, get_agent_session, list_agent_sessions, approve_agent_action, cancel_agent_run). Why not just `mcp.operations.allow`: the generic MCP operations profile walks OPERATION_FUNCTION_MAP once, BEFORE this component registers its operations, so the agent ops are added to the map too late for the walk — allow-listing them has no effect (confirmed live: profile builds at 14 tools, then the 6 agent ops register; tools/list never shows them). So we register a curated agent tool set directly into the registry AFTER the ops exist. - agent/mcpTools.ts (new): registerAgentMcpTools(operations) adds curated tools (proper schemas/descriptions, destructive/read-only annotations) on the operations profile. visibleTo is super_user-only for listing; each handler dispatches to the operation's execute with the MCP caller as hdb_user, so the op's own super_user check + downstream RBAC still apply. set_agent_config is intentionally NOT exposed (operator/config action). - agent/agent.ts: call it after registering the ops. Tools sit inertly in the registry unless the MCP HTTP surface is enabled. Verified live over the MCP Streamable HTTP transport: an MCP client completed initialize → tools/list (agent tools present) → tools/call agent_prompt → get_agent_session, and the agent ran a tool and answered. Stacked on the best-practices PR (kris/agent-bestpractices). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ponses Fall back to String(err) before the generic message so string/plain-object thrown values aren't reduced to a useless generic message (#1561).
dawsontoth
approved these changes
Jul 10, 2026
…the filesystem (#626) Per review, @harperfast/skills exposes its skill content directly as module exports — skillSummary (SKILL.md), ruleNames, and a rules name→markdown map — so there's no need to resolve the package on disk and read files by hand. - bestPractices.ts: import { ruleNames, rules, skillSummary } and serve from them. This removes the sync readdirSync/readFileSync in the tool handler (no event-loop blocking on the main thread), the require.resolve path walk (ESM require-undefined hazard), and the path-traversal regex — rule lookup is now a plain map access, so malformed/traversal names simply miss. - agent.ts: tighten the systemPromptAppend guard to an explicit string check before trim(). - dependencies.md: rewrite the @harperfast/skills entry to reflect module-export consumption (no fs access, rule bodies resident in heap, hard runtime dep). - test: traversal/malformed names now surface as "No such best-practice rule". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
dawsontoth
approved these changes
Jul 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two related enhancements to the built-in agent's grounding (#626):
harper-best-practicesskill in@harperfast/skills(a new dependency, so it's versioned with the release, no drift).agent.systemPromptAppend— operator text appended to the system prompt, tunable on a running instance.Stacked on #1547 (inspector).
Best practices — progressive disclosure
Mirrors how the skill is designed to be used, so we don't burn ~136K of rules on every LLM call:
loadBestPracticesOverview()injectsSKILL.md(the rule index + when-to-use, ~1.2k tokens) so the agent always knows which practices exist.harper_best_practicetool lists the rules (no arg) or returnsrules/<name>.md(schema design, relationships, auth, caching, vector indexing, TypeStrip, deployment, …) only when the agent needs them. Therulearg is guarded against path traversal.@harperfast/skillsisn't resolvable, the overview is omitted and the tool isn't registered — the agent still runs.Sourced by resolving the package's main entry and reading the
harper-best-practices/assets alongsidedist/(the packageexportsonly expose., so files are read directly, notrequired).agent.systemPromptAppendliveConfigeach run and accepted byset_agent_config, so it can be tuned on a live instance.config-root.schema.jsonandAgentConfig.Assembled system prompt = built-in grounding → best-practices overview → operator append.
Verification
Agent unit suite green, including new
bestPracticestests that exercise the real@harperfast/skillspackage: overview loads, tool lists rules, returns a rule body, and rejects traversal / unknown names.🤖 Generated with Claude Code