From e9f0f3294a235081509c319a71e52c1d7ee805df Mon Sep 17 00:00:00 2001 From: ablancorobayna Date: Wed, 22 Apr 2026 12:40:57 +0100 Subject: [PATCH 1/3] feat: add check_project_health tool for governance health checks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Introduces project-level health checks that surface soft recommendations about missing setup — sprints, Jira config, source processing, and AEM phase readiness. Complements the existing run_doctor tool which focuses on document-level structural issues. Six checks implemented: - empty-project: detects new projects, suggests onboarding steps - unprocessed-sources: flags pending/errored source files - no-sprints: work exists but no sprints defined - unassigned-actions: open actions without sprint assignment - no-jira-project: Jira project key not configured - phase-readiness: AEM phase gate prerequisites incomplete Also adds run_doctor to the agent session allowed tools list (was previously missing). --- src/agent/mcp-server.ts | 9 +- src/agent/session.ts | 4 + src/agent/tools/doctor.ts | 58 +++++- src/doctor/health/checks/empty-project.ts | 52 +++++ src/doctor/health/checks/index.ts | 17 ++ src/doctor/health/checks/no-jira-project.ts | 30 +++ src/doctor/health/checks/no-sprints.ts | 38 ++++ src/doctor/health/checks/phase-readiness.ts | 99 ++++++++++ .../health/checks/unassigned-actions.ts | 37 ++++ .../health/checks/unprocessed-sources.ts | 49 +++++ src/doctor/health/engine.ts | 24 +++ src/doctor/health/types.ts | 38 ++++ src/mcp/stdio-server.ts | 2 + test/doctor/health/engine.test.ts | 180 ++++++++++++++++++ test/doctor/health/phase-readiness.test.ts | 119 ++++++++++++ 15 files changed, 753 insertions(+), 3 deletions(-) create mode 100644 src/doctor/health/checks/empty-project.ts create mode 100644 src/doctor/health/checks/index.ts create mode 100644 src/doctor/health/checks/no-jira-project.ts create mode 100644 src/doctor/health/checks/no-sprints.ts create mode 100644 src/doctor/health/checks/phase-readiness.ts create mode 100644 src/doctor/health/checks/unassigned-actions.ts create mode 100644 src/doctor/health/checks/unprocessed-sources.ts create mode 100644 src/doctor/health/engine.ts create mode 100644 src/doctor/health/types.ts create mode 100644 test/doctor/health/engine.test.ts create mode 100644 test/doctor/health/phase-readiness.test.ts diff --git a/src/agent/mcp-server.ts b/src/agent/mcp-server.ts index e956b34..621fcad 100644 --- a/src/agent/mcp-server.ts +++ b/src/agent/mcp-server.ts @@ -6,6 +6,7 @@ import { import type { DocumentStore } from "../storage/store.js"; import type { SessionStore } from "../storage/session-store.js"; import type { SourceManifestManager } from "../sources/manifest.js"; +import type { MarvinProjectConfig } from "../core/config.js"; import { createDecisionTools } from "./tools/decisions.js"; import { createActionTools } from "./tools/actions.js"; import { createQuestionTools } from "./tools/questions.js"; @@ -24,6 +25,8 @@ export interface McpServerOptions { skillTools?: SdkMcpToolDefinition[]; projectName?: string; navGroups?: NavGroup[]; + config?: MarvinProjectConfig; + marvinDir?: string; } export function createMarvinMcpServer( @@ -42,7 +45,11 @@ export function createMarvinMcpServer( ...(options?.projectName && options?.navGroups ? createWebTools(store, options.projectName, options.navGroups) : []), - ...createDoctorTools(store), + ...createDoctorTools(store, { + config: options?.config, + manifest: options?.manifest, + marvinDir: options?.marvinDir, + }), ]; return createSdkMcpServer({ diff --git a/src/agent/session.ts b/src/agent/session.ts index 7b47bf9..8c32a95 100644 --- a/src/agent/session.ts +++ b/src/agent/session.ts @@ -74,6 +74,8 @@ export async function startSession(options: SessionOptions): Promise { skillTools: codeSkillTools, projectName: config.project.name, navGroups, + config: config.project, + marvinDir, }); const systemPrompt = buildSystemPrompt( persona, @@ -188,6 +190,8 @@ export async function startSession(options: SessionOptions): Promise { "mcp__marvin-governance__get_dashboard_board", "mcp__marvin-governance__get_dashboard_upcoming", "mcp__marvin-governance__get_dashboard_sprint_summary", + "mcp__marvin-governance__run_doctor", + "mcp__marvin-governance__check_project_health", ...pluginTools.map((t) => `mcp__marvin-governance__${t.name}`), ...codeSkillTools.map((t) => `mcp__marvin-governance__${t.name}`), ], diff --git a/src/agent/tools/doctor.ts b/src/agent/tools/doctor.ts index 2f8493d..c29ebbb 100644 --- a/src/agent/tools/doctor.ts +++ b/src/agent/tools/doctor.ts @@ -1,10 +1,22 @@ import { z } from "zod/v4"; import { tool, type SdkMcpToolDefinition } from "@anthropic-ai/claude-agent-sdk"; import type { DocumentStore } from "../../storage/store.js"; +import type { SourceManifestManager } from "../../sources/manifest.js"; +import type { MarvinProjectConfig } from "../../core/config.js"; import { runDoctorScan, runDoctorFix } from "../../doctor/engine.js"; +import { runHealthCheck } from "../../doctor/health/engine.js"; -export function createDoctorTools(store: DocumentStore): SdkMcpToolDefinition[] { - return [ +export interface DoctorToolOptions { + config?: MarvinProjectConfig; + manifest?: SourceManifestManager; + marvinDir?: string; +} + +export function createDoctorTools( + store: DocumentStore, + options?: DoctorToolOptions, +): SdkMcpToolDefinition[] { + const tools: SdkMcpToolDefinition[] = [ tool( "run_doctor", "Scan project documents for structural issues and optionally auto-repair them. Returns a JSON report with all issues found and fixes applied.", @@ -49,4 +61,46 @@ export function createDoctorTools(store: DocumentStore): SdkMcpToolDefinition { + try { + const report = runHealthCheck({ + store, + config: options.config!, + manifest: options.manifest, + marvinDir: options.marvinDir!, + }); + + return { + content: [ + { + type: "text" as const, + text: JSON.stringify(report, null, 2), + }, + ], + }; + } catch (err) { + return { + content: [ + { + type: "text" as const, + text: `Health check error: ${err instanceof Error ? err.message : String(err)}`, + }, + ], + isError: true, + }; + } + }, + { annotations: { readOnlyHint: true } }, + ), + ); + } + + return tools; } diff --git a/src/doctor/health/checks/empty-project.ts b/src/doctor/health/checks/empty-project.ts new file mode 100644 index 0000000..3438764 --- /dev/null +++ b/src/doctor/health/checks/empty-project.ts @@ -0,0 +1,52 @@ +import type { HealthCheck, HealthContext, HealthFinding } from "../types.js"; + +const CHECK_ID = "empty-project"; +const CHECK_NAME = "Empty Project Detection"; + +/** Flags new projects with zero artifacts but source files ready for processing. */ +export const emptyProjectCheck: HealthCheck = { + id: CHECK_ID, + name: CHECK_NAME, + description: "Detects new projects with no artifacts and suggests onboarding steps", + + run(ctx: HealthContext): HealthFinding[] { + const counts = ctx.store.counts(); + const totalArtifacts = Object.values(counts).reduce((sum, n) => sum + n, 0); + + if (totalArtifacts > 0) return []; + + const findings: HealthFinding[] = []; + + // Check for unprocessed sources + if (ctx.manifest) { + try { + ctx.manifest.scan(); + } catch { + // Ignore scan errors — other checks handle that + } + const pending = ctx.manifest.list("pending"); + if (pending.length > 0) { + findings.push({ + checkId: CHECK_ID, + checkName: CHECK_NAME, + severity: "recommendation", + message: `Project has no artifacts yet, but ${pending.length} source file(s) are ready for processing.`, + suggestion: + "Set a persona with set_persona (e.g. 'po'), then ingest the source files to generate initial artifacts.", + }); + return findings; + } + } + + findings.push({ + checkId: CHECK_ID, + checkName: CHECK_NAME, + severity: "observation", + message: "Project is empty — no artifacts or source files found.", + suggestion: + "Start by placing source documents in .marvin/sources/ or create your first artifact with set_persona.", + }); + + return findings; + }, +}; diff --git a/src/doctor/health/checks/index.ts b/src/doctor/health/checks/index.ts new file mode 100644 index 0000000..46ca602 --- /dev/null +++ b/src/doctor/health/checks/index.ts @@ -0,0 +1,17 @@ +import type { HealthCheck } from "../types.js"; +import { emptyProjectCheck } from "./empty-project.js"; +import { unprocessedSourcesCheck } from "./unprocessed-sources.js"; +import { noSprintsCheck } from "./no-sprints.js"; +import { unassignedActionsCheck } from "./unassigned-actions.js"; +import { noJiraProjectCheck } from "./no-jira-project.js"; +import { phaseReadinessCheck } from "./phase-readiness.js"; + +/** Health checks in priority order — most actionable first. */ +export const allHealthChecks: HealthCheck[] = [ + emptyProjectCheck, + unprocessedSourcesCheck, + noSprintsCheck, + unassignedActionsCheck, + noJiraProjectCheck, + phaseReadinessCheck, +]; diff --git a/src/doctor/health/checks/no-jira-project.ts b/src/doctor/health/checks/no-jira-project.ts new file mode 100644 index 0000000..e6e5f5b --- /dev/null +++ b/src/doctor/health/checks/no-jira-project.ts @@ -0,0 +1,30 @@ +import type { HealthCheck, HealthContext, HealthFinding } from "../types.js"; + +const CHECK_ID = "no-jira-project"; +const CHECK_NAME = "Jira Project Not Configured"; + +/** Flags projects with artifacts but no Jira project key configured. */ +export const noJiraProjectCheck: HealthCheck = { + id: CHECK_ID, + name: CHECK_NAME, + description: "Detects projects with artifacts but no Jira integration configured", + + run(ctx: HealthContext): HealthFinding[] { + const counts = ctx.store.counts(); + const totalArtifacts = Object.values(counts).reduce((sum, n) => sum + n, 0); + if (totalArtifacts === 0) return []; + + if (ctx.config.jira?.projectKey) return []; + + return [ + { + checkId: CHECK_ID, + checkName: CHECK_NAME, + severity: "observation", + message: "No Jira project key configured for this project.", + suggestion: + "Add a jira.projectKey to .marvin/config.yaml to enable Jira sync and artifact push.", + }, + ]; + }, +}; diff --git a/src/doctor/health/checks/no-sprints.ts b/src/doctor/health/checks/no-sprints.ts new file mode 100644 index 0000000..6198c42 --- /dev/null +++ b/src/doctor/health/checks/no-sprints.ts @@ -0,0 +1,38 @@ +import type { HealthCheck, HealthContext, HealthFinding } from "../types.js"; + +const CHECK_ID = "no-sprints"; +const CHECK_NAME = "Missing Sprint Setup"; + +/** Flags projects that have actionable work but no sprints defined. */ +export const noSprintsCheck: HealthCheck = { + id: CHECK_ID, + name: CHECK_NAME, + description: "Detects projects with actions or epics but no sprints defined", + + run(ctx: HealthContext): HealthFinding[] { + const counts = ctx.store.counts(); + + const hasWork = (counts["action"] ?? 0) > 0 || (counts["epic"] ?? 0) > 0; + if (!hasWork) return []; + + const hasSprints = (counts["sprint"] ?? 0) > 0; + if (hasSprints) return []; + + const actionCount = counts["action"] ?? 0; + const epicCount = counts["epic"] ?? 0; + const parts: string[] = []; + if (actionCount > 0) parts.push(`${actionCount} action(s)`); + if (epicCount > 0) parts.push(`${epicCount} epic(s)`); + + return [ + { + checkId: CHECK_ID, + checkName: CHECK_NAME, + severity: "recommendation", + message: `Project has ${parts.join(" and ")} but no sprints defined.`, + suggestion: + "Consider setting up a Sprint 0 to organize bootstrapping work, then plan Sprint 1 for delivery.", + }, + ]; + }, +}; diff --git a/src/doctor/health/checks/phase-readiness.ts b/src/doctor/health/checks/phase-readiness.ts new file mode 100644 index 0000000..c522b82 --- /dev/null +++ b/src/doctor/health/checks/phase-readiness.ts @@ -0,0 +1,99 @@ +import * as fs from "node:fs"; +import * as path from "node:path"; +import * as YAML from "yaml"; +import type { HealthCheck, HealthContext, HealthFinding } from "../types.js"; + +const CHECK_ID = "phase-readiness"; +const CHECK_NAME = "AEM Phase Readiness"; + +/** Checks AEM phase gate prerequisites for the current phase. */ +export const phaseReadinessCheck: HealthCheck = { + id: CHECK_ID, + name: CHECK_NAME, + description: "Checks whether AEM phase gate prerequisites are met for the current phase", + + run(ctx: HealthContext): HealthFinding[] { + if (ctx.config.methodology !== "sap-aem") return []; + + const phase = readPhase(ctx.marvinDir); + if (!phase) return []; + + const findings: HealthFinding[] = []; + + if (phase === "assess-use-case") { + const useCases = ctx.store.list({ type: "use-case" }); + if (useCases.length === 0) { + findings.push({ + checkId: CHECK_ID, + checkName: CHECK_NAME, + severity: "recommendation", + message: "Phase 1 (Assess Use Case) is active but no use cases have been defined.", + suggestion: "Use the PO persona to create use cases before advancing to Phase 2.", + }); + } else { + const drafts = useCases.filter((uc) => uc.frontmatter.status === "draft"); + if (drafts.length === useCases.length) { + findings.push({ + checkId: CHECK_ID, + checkName: CHECK_NAME, + severity: "observation", + message: `All ${drafts.length} use case(s) are still in draft status.`, + suggestion: + "Assess and approve use cases before advancing to Phase 2 (Assess Technology).", + }); + } + } + } + + if (phase === "assess-technology") { + const tas = ctx.store.list({ type: "tech-assessment" }); + const approvedUCs = ctx.store + .list({ type: "use-case" }) + .filter( + (uc) => uc.frontmatter.status === "assessed" || uc.frontmatter.status === "approved", + ); + + if (approvedUCs.length > 0 && tas.length === 0) { + findings.push({ + checkId: CHECK_ID, + checkName: CHECK_NAME, + severity: "recommendation", + message: `Phase 2 is active with ${approvedUCs.length} approved use case(s) but no tech assessments created.`, + suggestion: "Use the TL persona to create tech assessments linked to approved use cases.", + }); + } + } + + if (phase === "define-solution") { + const designs = ctx.store.list({ type: "extension-design" }); + const recommendedTAs = ctx.store + .list({ type: "tech-assessment" }) + .filter((ta) => ta.frontmatter.status === "recommended"); + + if (recommendedTAs.length > 0 && designs.length === 0) { + findings.push({ + checkId: CHECK_ID, + checkName: CHECK_NAME, + severity: "recommendation", + message: `Phase 3 is active with ${recommendedTAs.length} recommended tech assessment(s) but no extension designs created.`, + suggestion: + "Use the TL persona to create extension designs linked to recommended assessments.", + }); + } + } + + return findings; + }, +}; + +function readPhase(marvinDir: string): string | undefined { + try { + const configPath = path.join(marvinDir, "config.yaml"); + const raw = fs.readFileSync(configPath, "utf-8"); + const config = YAML.parse(raw) as Record; + const aem = config.aem as Record | undefined; + return aem?.currentPhase as string | undefined; + } catch { + return undefined; + } +} diff --git a/src/doctor/health/checks/unassigned-actions.ts b/src/doctor/health/checks/unassigned-actions.ts new file mode 100644 index 0000000..42ae5ed --- /dev/null +++ b/src/doctor/health/checks/unassigned-actions.ts @@ -0,0 +1,37 @@ +import type { HealthCheck, HealthContext, HealthFinding } from "../types.js"; + +const CHECK_ID = "unassigned-actions"; +const CHECK_NAME = "Actions Without Sprint"; + +/** Flags open actions that are not assigned to any sprint. */ +export const unassignedActionsCheck: HealthCheck = { + id: CHECK_ID, + name: CHECK_NAME, + description: "Detects open actions not assigned to a sprint", + + run(ctx: HealthContext): HealthFinding[] { + const counts = ctx.store.counts(); + if ((counts["sprint"] ?? 0) === 0) return []; // no-sprints check covers this + + const actions = ctx.store.list({ type: "action" }); + const openActions = actions.filter( + (a) => a.frontmatter.status === "open" || a.frontmatter.status === "in-progress", + ); + if (openActions.length === 0) return []; + + const unassigned = openActions.filter( + (a) => !a.frontmatter.sprint && !a.frontmatter.tags?.some((t) => t.startsWith("sprint:")), + ); + if (unassigned.length === 0) return []; + + return [ + { + checkId: CHECK_ID, + checkName: CHECK_NAME, + severity: "recommendation", + message: `${unassigned.length} open action(s) are not assigned to any sprint.`, + suggestion: "Assign these actions to a sprint so they appear in delivery tracking.", + }, + ]; + }, +}; diff --git a/src/doctor/health/checks/unprocessed-sources.ts b/src/doctor/health/checks/unprocessed-sources.ts new file mode 100644 index 0000000..1418b1c --- /dev/null +++ b/src/doctor/health/checks/unprocessed-sources.ts @@ -0,0 +1,49 @@ +import type { HealthCheck, HealthContext, HealthFinding } from "../types.js"; + +const CHECK_ID = "unprocessed-sources"; +const CHECK_NAME = "Unprocessed Source Files"; + +/** Flags source files that are pending or errored. */ +export const unprocessedSourcesCheck: HealthCheck = { + id: CHECK_ID, + name: CHECK_NAME, + description: "Detects source files that have not been processed into artifacts", + + run(ctx: HealthContext): HealthFinding[] { + if (!ctx.manifest) return []; + + try { + ctx.manifest.scan(); + } catch { + return []; + } + + const findings: HealthFinding[] = []; + + const pending = ctx.manifest.list("pending"); + if (pending.length > 0) { + const names = pending.map((f) => f.name).join(", "); + findings.push({ + checkId: CHECK_ID, + checkName: CHECK_NAME, + severity: "recommendation", + message: `${pending.length} source file(s) pending processing: ${names}`, + suggestion: "Ingest these source files to extract artifacts from them.", + }); + } + + const errored = ctx.manifest.list("error"); + if (errored.length > 0) { + const names = errored.map((f) => f.name).join(", "); + findings.push({ + checkId: CHECK_ID, + checkName: CHECK_NAME, + severity: "recommendation", + message: `${errored.length} source file(s) failed processing: ${names}`, + suggestion: "Review the errors and retry processing these files.", + }); + } + + return findings; + }, +}; diff --git a/src/doctor/health/engine.ts b/src/doctor/health/engine.ts new file mode 100644 index 0000000..398fb54 --- /dev/null +++ b/src/doctor/health/engine.ts @@ -0,0 +1,24 @@ +import type { HealthContext, HealthReport } from "./types.js"; +import { allHealthChecks } from "./checks/index.js"; + +/** Run all health checks and produce a report with findings. */ +export function runHealthCheck(ctx: HealthContext): HealthReport { + const findings = allHealthChecks.flatMap((check) => check.run(ctx)); + + const byCheck: Record = {}; + let recommendations = 0; + let observations = 0; + + for (const f of findings) { + byCheck[f.checkId] = (byCheck[f.checkId] ?? 0) + 1; + if (f.severity === "recommendation") recommendations++; + else observations++; + } + + return { + checkedAt: new Date().toISOString(), + totalFindings: findings.length, + findings, + summary: { recommendations, observations, byCheck }, + }; +} diff --git a/src/doctor/health/types.ts b/src/doctor/health/types.ts new file mode 100644 index 0000000..a6bd6e7 --- /dev/null +++ b/src/doctor/health/types.ts @@ -0,0 +1,38 @@ +import type { DocumentStore } from "../../storage/store.js"; +import type { SourceManifestManager } from "../../sources/manifest.js"; +import type { MarvinProjectConfig } from "../../core/config.js"; + +export type FindingSeverity = "recommendation" | "observation"; + +export interface HealthFinding { + checkId: string; + checkName: string; + severity: FindingSeverity; + message: string; + suggestion: string; +} + +export interface HealthContext { + store: DocumentStore; + config: MarvinProjectConfig; + manifest?: SourceManifestManager; + marvinDir: string; +} + +export interface HealthCheck { + id: string; + name: string; + description: string; + run(ctx: HealthContext): HealthFinding[]; +} + +export interface HealthReport { + checkedAt: string; + totalFindings: number; + findings: HealthFinding[]; + summary: { + recommendations: number; + observations: number; + byCheck: Record; + }; +} diff --git a/src/mcp/stdio-server.ts b/src/mcp/stdio-server.ts index 42fac82..ad00ade 100644 --- a/src/mcp/stdio-server.ts +++ b/src/mcp/stdio-server.ts @@ -17,6 +17,7 @@ import { resolvePlugin, getPluginTools } from "../plugins/registry.js"; import { loadAllSkills, getSkillTools, collectSkillRegistrations } from "../skills/registry.js"; import { createSkillActionTools } from "../skills/action-tools.js"; import { createWebTools } from "../agent/tools/web.js"; +import { createDoctorTools } from "../agent/tools/doctor.js"; import { buildNavGroups } from "../web/server.js"; import { PersonaContextManager } from "./persona-context.js"; import { createPersonaTools } from "./persona-tools.js"; @@ -68,6 +69,7 @@ export function collectTools(marvinDir: string): SdkMcpToolDefinition[] { ...(manifest ? createSourceTools(manifest) : []), ...createSessionTools(sessionStore), ...createWebTools(store, config.name, navGroups), + ...createDoctorTools(store, { config, manifest, marvinDir }), ...pluginTools, ...codeSkillTools, ...actionTools, diff --git a/test/doctor/health/engine.test.ts b/test/doctor/health/engine.test.ts new file mode 100644 index 0000000..49eb86c --- /dev/null +++ b/test/doctor/health/engine.test.ts @@ -0,0 +1,180 @@ +import { describe, it, expect, afterEach } from "vitest"; +import * as fs from "node:fs"; +import * as path from "node:path"; +import * as os from "node:os"; +import * as YAML from "yaml"; +import { DocumentStore } from "../../../src/storage/store.js"; +import { SourceManifestManager } from "../../../src/sources/manifest.js"; +import { runHealthCheck } from "../../../src/doctor/health/engine.js"; +import type { HealthContext } from "../../../src/doctor/health/types.js"; +import type { MarvinProjectConfig } from "../../../src/core/config.js"; + +function setup(options?: { methodology?: string; jiraProjectKey?: string }) { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "marvin-health-test-")); + const marvinDir = path.join(tmpDir, ".marvin"); + const docsDir = path.join(marvinDir, "docs"); + const sourcesDir = path.join(marvinDir, "sources"); + fs.mkdirSync(docsDir, { recursive: true }); + fs.mkdirSync(sourcesDir, { recursive: true }); + + const config: MarvinProjectConfig = { + name: "test-project", + methodology: options?.methodology, + jira: options?.jiraProjectKey ? { projectKey: options.jiraProjectKey } : undefined, + }; + + fs.writeFileSync(path.join(marvinDir, "config.yaml"), YAML.stringify(config), "utf-8"); + + const registrations = [ + { type: "feature", dirName: "features", idPrefix: "F" }, + { type: "epic", dirName: "epics", idPrefix: "E" }, + { type: "sprint", dirName: "sprints", idPrefix: "S" }, + { type: "use-case", dirName: "use-cases", idPrefix: "UC" }, + { type: "tech-assessment", dirName: "tech-assessments", idPrefix: "TA" }, + { type: "extension-design", dirName: "extension-designs", idPrefix: "XD" }, + ]; + + const store = new DocumentStore(marvinDir, registrations); + const manifest = new SourceManifestManager(marvinDir); + + const ctx: HealthContext = { store, config, manifest, marvinDir }; + return { tmpDir, marvinDir, store, manifest, ctx }; +} + +describe("Health Check Engine", () => { + let tmpDir: string; + + afterEach(() => { + if (tmpDir) fs.rmSync(tmpDir, { recursive: true, force: true }); + }); + + it("should return no findings for an empty project with no sources", () => { + const env = setup(); + tmpDir = env.tmpDir; + + // Remove sources dir so empty-project doesn't suggest ingesting + fs.rmSync(path.join(env.marvinDir, "sources"), { recursive: true, force: true }); + const ctx = { ...env.ctx, manifest: undefined }; + + const report = runHealthCheck(ctx); + + // Only the empty project observation + expect(report.totalFindings).toBe(1); + expect(report.findings[0].checkId).toBe("empty-project"); + expect(report.findings[0].severity).toBe("observation"); + }); + + it("should flag empty project with pending source files", () => { + const env = setup(); + tmpDir = env.tmpDir; + + fs.writeFileSync(path.join(env.marvinDir, "sources", "requirements.pdf"), "fake pdf content"); + + const report = runHealthCheck(env.ctx); + + const emptyProject = report.findings.find((f) => f.checkId === "empty-project"); + expect(emptyProject).toBeDefined(); + expect(emptyProject!.severity).toBe("recommendation"); + expect(emptyProject!.message).toContain("source file(s)"); + + // unprocessed-sources also fires + const unprocessed = report.findings.find((f) => f.checkId === "unprocessed-sources"); + expect(unprocessed).toBeDefined(); + }); + + it("should flag missing sprints when actions exist", () => { + const env = setup(); + tmpDir = env.tmpDir; + + env.store.create("action", { title: "Set up CI/CD" }); + env.store.create("action", { title: "Provision cloud" }); + + const report = runHealthCheck(env.ctx); + + const noSprints = report.findings.find((f) => f.checkId === "no-sprints"); + expect(noSprints).toBeDefined(); + expect(noSprints!.severity).toBe("recommendation"); + expect(noSprints!.message).toContain("2 action(s)"); + expect(noSprints!.suggestion).toContain("Sprint 0"); + }); + + it("should not flag missing sprints when sprints exist", () => { + const env = setup(); + tmpDir = env.tmpDir; + + env.store.create("action", { title: "Set up CI/CD" }); + env.store.create("sprint", { title: "Sprint 0", status: "active" }); + + const report = runHealthCheck(env.ctx); + + const noSprints = report.findings.find((f) => f.checkId === "no-sprints"); + expect(noSprints).toBeUndefined(); + }); + + it("should flag unassigned actions when sprints exist", () => { + const env = setup(); + tmpDir = env.tmpDir; + + env.store.create("action", { title: "Unassigned task", status: "open" }); + env.store.create("action", { title: "Assigned task", status: "open", sprint: "S-001" }); + env.store.create("sprint", { title: "Sprint 1", status: "active" }); + + const report = runHealthCheck(env.ctx); + + const unassigned = report.findings.find((f) => f.checkId === "unassigned-actions"); + expect(unassigned).toBeDefined(); + expect(unassigned!.message).toContain("1 open action(s)"); + }); + + it("should not flag unassigned actions when no sprints exist", () => { + const env = setup(); + tmpDir = env.tmpDir; + + env.store.create("action", { title: "Some action", status: "open" }); + + const report = runHealthCheck(env.ctx); + + const unassigned = report.findings.find((f) => f.checkId === "unassigned-actions"); + expect(unassigned).toBeUndefined(); + }); + + it("should flag missing Jira project when artifacts exist", () => { + const env = setup(); + tmpDir = env.tmpDir; + + env.store.create("decision", { title: "Use React" }); + + const report = runHealthCheck(env.ctx); + + const noJira = report.findings.find((f) => f.checkId === "no-jira-project"); + expect(noJira).toBeDefined(); + expect(noJira!.severity).toBe("observation"); + }); + + it("should not flag Jira when project key is configured", () => { + const env = setup({ jiraProjectKey: "TEST" }); + tmpDir = env.tmpDir; + + env.store.create("decision", { title: "Use React" }); + + const report = runHealthCheck(env.ctx); + + const noJira = report.findings.find((f) => f.checkId === "no-jira-project"); + expect(noJira).toBeUndefined(); + }); + + it("should produce correct summary counts", () => { + const env = setup(); + tmpDir = env.tmpDir; + + env.store.create("action", { title: "Some action" }); + fs.writeFileSync(path.join(env.marvinDir, "sources", "spec.md"), "# Spec"); + + const report = runHealthCheck(env.ctx); + + expect(report.summary.recommendations + report.summary.observations).toBe(report.totalFindings); + expect(Object.values(report.summary.byCheck).reduce((a, b) => a + b, 0)).toBe( + report.totalFindings, + ); + }); +}); diff --git a/test/doctor/health/phase-readiness.test.ts b/test/doctor/health/phase-readiness.test.ts new file mode 100644 index 0000000..1324bd9 --- /dev/null +++ b/test/doctor/health/phase-readiness.test.ts @@ -0,0 +1,119 @@ +import { describe, it, expect, afterEach } from "vitest"; +import * as fs from "node:fs"; +import * as path from "node:path"; +import * as os from "node:os"; +import * as YAML from "yaml"; +import { DocumentStore } from "../../../src/storage/store.js"; +import { phaseReadinessCheck } from "../../../src/doctor/health/checks/phase-readiness.js"; +import type { HealthContext } from "../../../src/doctor/health/types.js"; +import type { MarvinProjectConfig } from "../../../src/core/config.js"; + +function setup(phase?: string) { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "marvin-phase-test-")); + const marvinDir = path.join(tmpDir, ".marvin"); + const docsDir = path.join(marvinDir, "docs"); + fs.mkdirSync(docsDir, { recursive: true }); + + const projectConfig: Record = { + name: "test-project", + methodology: "sap-aem", + }; + if (phase) { + projectConfig.aem = { currentPhase: phase }; + } + fs.writeFileSync(path.join(marvinDir, "config.yaml"), YAML.stringify(projectConfig), "utf-8"); + + const config: MarvinProjectConfig = { + name: "test-project", + methodology: "sap-aem", + }; + + const registrations = [ + { type: "use-case", dirName: "use-cases", idPrefix: "UC" }, + { type: "tech-assessment", dirName: "tech-assessments", idPrefix: "TA" }, + { type: "extension-design", dirName: "extension-designs", idPrefix: "XD" }, + ]; + + const store = new DocumentStore(marvinDir, registrations); + const ctx: HealthContext = { store, config, marvinDir }; + return { tmpDir, store, ctx }; +} + +describe("Phase Readiness Check", () => { + let tmpDir: string; + + afterEach(() => { + if (tmpDir) fs.rmSync(tmpDir, { recursive: true, force: true }); + }); + + it("should skip for non-AEM projects", () => { + const env = setup("assess-use-case"); + tmpDir = env.tmpDir; + + const ctx = { ...env.ctx, config: { name: "test", methodology: "scrum" } }; + const findings = phaseReadinessCheck.run(ctx); + expect(findings).toHaveLength(0); + }); + + it("should flag no use cases in Phase 1", () => { + const env = setup("assess-use-case"); + tmpDir = env.tmpDir; + + const findings = phaseReadinessCheck.run(env.ctx); + expect(findings).toHaveLength(1); + expect(findings[0].message).toContain("no use cases"); + }); + + it("should flag all-draft use cases in Phase 1", () => { + const env = setup("assess-use-case"); + tmpDir = env.tmpDir; + + env.store.create("use-case", { title: "UC1", status: "draft" }); + env.store.create("use-case", { title: "UC2", status: "draft" }); + + const findings = phaseReadinessCheck.run(env.ctx); + expect(findings).toHaveLength(1); + expect(findings[0].message).toContain("2 use case(s) are still in draft"); + }); + + it("should not flag when use cases are assessed", () => { + const env = setup("assess-use-case"); + tmpDir = env.tmpDir; + + env.store.create("use-case", { title: "UC1", status: "assessed" }); + env.store.create("use-case", { title: "UC2", status: "draft" }); + + const findings = phaseReadinessCheck.run(env.ctx); + expect(findings).toHaveLength(0); + }); + + it("should flag no tech assessments in Phase 2 with approved use cases", () => { + const env = setup("assess-technology"); + tmpDir = env.tmpDir; + + env.store.create("use-case", { title: "UC1", status: "approved" }); + + const findings = phaseReadinessCheck.run(env.ctx); + expect(findings).toHaveLength(1); + expect(findings[0].message).toContain("no tech assessments"); + }); + + it("should flag no extension designs in Phase 3 with recommended assessments", () => { + const env = setup("define-solution"); + tmpDir = env.tmpDir; + + env.store.create("tech-assessment", { title: "TA1", status: "recommended" }); + + const findings = phaseReadinessCheck.run(env.ctx); + expect(findings).toHaveLength(1); + expect(findings[0].message).toContain("no extension designs"); + }); + + it("should return no findings when phase is not set", () => { + const env = setup(); + tmpDir = env.tmpDir; + + const findings = phaseReadinessCheck.run(env.ctx); + expect(findings).toHaveLength(0); + }); +}); From fc425877bccff8ed23c0de6a74a18c3ae9d1e801 Mon Sep 17 00:00:00 2001 From: ablancorobayna Date: Wed, 22 Apr 2026 13:56:46 +0100 Subject: [PATCH 2/3] fix: trim whitespace when checking Jira project key presence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A whitespace-only projectKey string is truthy but invalid — trim before the check so it correctly flags as unconfigured. --- src/doctor/health/checks/no-jira-project.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/doctor/health/checks/no-jira-project.ts b/src/doctor/health/checks/no-jira-project.ts index e6e5f5b..2315dbc 100644 --- a/src/doctor/health/checks/no-jira-project.ts +++ b/src/doctor/health/checks/no-jira-project.ts @@ -14,7 +14,7 @@ export const noJiraProjectCheck: HealthCheck = { const totalArtifacts = Object.values(counts).reduce((sum, n) => sum + n, 0); if (totalArtifacts === 0) return []; - if (ctx.config.jira?.projectKey) return []; + if (ctx.config.jira?.projectKey?.trim()) return []; return [ { From 9cd9b3be88b03ff3300f4ebc7557cb56ab623437 Mon Sep 17 00:00:00 2001 From: ablancorobayna Date: Wed, 22 Apr 2026 15:03:11 +0100 Subject: [PATCH 3/3] fix: address PR review findings for health check - Always register check_project_health tool; return structured error when config/marvinDir unavailable instead of conditionally omitting - Destructure options to remove non-null assertions in closure - Scan manifest once in engine before running checks, not per-check - Read AEM phase from config.aem.currentPhase instead of re-parsing YAML from disk; add AemConfig interface to MarvinProjectConfig - Filter only "approved" use cases in Phase 2 check (was including "assessed" which is a pre-approval status) - Truncate file name lists to 10 entries in unprocessed-sources --- src/agent/tools/doctor.ts | 85 ++++++++++--------- src/core/config.ts | 5 ++ src/doctor/health/checks/empty-project.ts | 7 +- src/doctor/health/checks/phase-readiness.ts | 21 +---- .../health/checks/unprocessed-sources.ts | 20 ++--- src/doctor/health/engine.ts | 9 ++ test/doctor/health/phase-readiness.test.ts | 21 ++--- 7 files changed, 82 insertions(+), 86 deletions(-) diff --git a/src/agent/tools/doctor.ts b/src/agent/tools/doctor.ts index c29ebbb..a7a2b62 100644 --- a/src/agent/tools/doctor.ts +++ b/src/agent/tools/doctor.ts @@ -16,7 +16,7 @@ export function createDoctorTools( store: DocumentStore, options?: DoctorToolOptions, ): SdkMcpToolDefinition[] { - const tools: SdkMcpToolDefinition[] = [ + return [ tool( "run_doctor", "Scan project documents for structural issues and optionally auto-repair them. Returns a JSON report with all issues found and fixes applied.", @@ -60,47 +60,50 @@ export function createDoctorTools( } }, ), - ]; - if (options?.config && options?.marvinDir) { - tools.push( - tool( - "check_project_health", - "Run governance health checks on the project. Returns soft recommendations about missing setup (sprints, Jira, source processing) and phase readiness — not document-level issues (use run_doctor for those).", - {}, - async () => { - try { - const report = runHealthCheck({ - store, - config: options.config!, - manifest: options.manifest, - marvinDir: options.marvinDir!, - }); + tool( + "check_project_health", + "Run governance health checks on the project. Returns soft recommendations about missing setup (sprints, Jira, source processing) and phase readiness — not document-level issues (use run_doctor for those).", + {}, + async () => { + if (!options?.config || !options?.marvinDir) { + return { + content: [ + { + type: "text" as const, + text: "Health check unavailable: project config or marvinDir not initialized.", + }, + ], + isError: true, + }; + } - return { - content: [ - { - type: "text" as const, - text: JSON.stringify(report, null, 2), - }, - ], - }; - } catch (err) { - return { - content: [ - { - type: "text" as const, - text: `Health check error: ${err instanceof Error ? err.message : String(err)}`, - }, - ], - isError: true, - }; - } - }, - { annotations: { readOnlyHint: true } }, - ), - ); - } + const { config, marvinDir, manifest } = options; + + try { + const report = runHealthCheck({ store, config, manifest, marvinDir }); - return tools; + return { + content: [ + { + type: "text" as const, + text: JSON.stringify(report, null, 2), + }, + ], + }; + } catch (err) { + return { + content: [ + { + type: "text" as const, + text: `Health check error: ${err instanceof Error ? err.message : String(err)}`, + }, + ], + isError: true, + }; + } + }, + { annotations: { readOnlyHint: true } }, + ), + ]; } diff --git a/src/core/config.ts b/src/core/config.ts index 3a28a77..94b28eb 100644 --- a/src/core/config.ts +++ b/src/core/config.ts @@ -62,6 +62,10 @@ export interface JiraProjectConfig { }; } +export interface AemConfig { + currentPhase?: string; +} + export interface MarvinProjectConfig { name: string; methodology?: string; @@ -70,6 +74,7 @@ export interface MarvinProjectConfig { git?: GitConfig; skills?: Record; jira?: JiraProjectConfig; + aem?: AemConfig; } export interface PersonaConfigOverride { diff --git a/src/doctor/health/checks/empty-project.ts b/src/doctor/health/checks/empty-project.ts index 3438764..2c459ae 100644 --- a/src/doctor/health/checks/empty-project.ts +++ b/src/doctor/health/checks/empty-project.ts @@ -17,13 +17,8 @@ export const emptyProjectCheck: HealthCheck = { const findings: HealthFinding[] = []; - // Check for unprocessed sources + // Check for unprocessed sources (manifest already scanned by engine) if (ctx.manifest) { - try { - ctx.manifest.scan(); - } catch { - // Ignore scan errors — other checks handle that - } const pending = ctx.manifest.list("pending"); if (pending.length > 0) { findings.push({ diff --git a/src/doctor/health/checks/phase-readiness.ts b/src/doctor/health/checks/phase-readiness.ts index c522b82..b8913d1 100644 --- a/src/doctor/health/checks/phase-readiness.ts +++ b/src/doctor/health/checks/phase-readiness.ts @@ -1,6 +1,3 @@ -import * as fs from "node:fs"; -import * as path from "node:path"; -import * as YAML from "yaml"; import type { HealthCheck, HealthContext, HealthFinding } from "../types.js"; const CHECK_ID = "phase-readiness"; @@ -15,7 +12,7 @@ export const phaseReadinessCheck: HealthCheck = { run(ctx: HealthContext): HealthFinding[] { if (ctx.config.methodology !== "sap-aem") return []; - const phase = readPhase(ctx.marvinDir); + const phase = ctx.config.aem?.currentPhase; if (!phase) return []; const findings: HealthFinding[] = []; @@ -49,9 +46,7 @@ export const phaseReadinessCheck: HealthCheck = { const tas = ctx.store.list({ type: "tech-assessment" }); const approvedUCs = ctx.store .list({ type: "use-case" }) - .filter( - (uc) => uc.frontmatter.status === "assessed" || uc.frontmatter.status === "approved", - ); + .filter((uc) => uc.frontmatter.status === "approved"); if (approvedUCs.length > 0 && tas.length === 0) { findings.push({ @@ -85,15 +80,3 @@ export const phaseReadinessCheck: HealthCheck = { return findings; }, }; - -function readPhase(marvinDir: string): string | undefined { - try { - const configPath = path.join(marvinDir, "config.yaml"); - const raw = fs.readFileSync(configPath, "utf-8"); - const config = YAML.parse(raw) as Record; - const aem = config.aem as Record | undefined; - return aem?.currentPhase as string | undefined; - } catch { - return undefined; - } -} diff --git a/src/doctor/health/checks/unprocessed-sources.ts b/src/doctor/health/checks/unprocessed-sources.ts index 1418b1c..ffe2f5d 100644 --- a/src/doctor/health/checks/unprocessed-sources.ts +++ b/src/doctor/health/checks/unprocessed-sources.ts @@ -2,6 +2,7 @@ import type { HealthCheck, HealthContext, HealthFinding } from "../types.js"; const CHECK_ID = "unprocessed-sources"; const CHECK_NAME = "Unprocessed Source Files"; +const MAX_LISTED_FILES = 10; /** Flags source files that are pending or errored. */ export const unprocessedSourcesCheck: HealthCheck = { @@ -12,34 +13,27 @@ export const unprocessedSourcesCheck: HealthCheck = { run(ctx: HealthContext): HealthFinding[] { if (!ctx.manifest) return []; - try { - ctx.manifest.scan(); - } catch { - return []; - } - + // Manifest already scanned by engine const findings: HealthFinding[] = []; const pending = ctx.manifest.list("pending"); if (pending.length > 0) { - const names = pending.map((f) => f.name).join(", "); findings.push({ checkId: CHECK_ID, checkName: CHECK_NAME, severity: "recommendation", - message: `${pending.length} source file(s) pending processing: ${names}`, + message: `${pending.length} source file(s) pending processing: ${truncateNames(pending.map((f) => f.name))}`, suggestion: "Ingest these source files to extract artifacts from them.", }); } const errored = ctx.manifest.list("error"); if (errored.length > 0) { - const names = errored.map((f) => f.name).join(", "); findings.push({ checkId: CHECK_ID, checkName: CHECK_NAME, severity: "recommendation", - message: `${errored.length} source file(s) failed processing: ${names}`, + message: `${errored.length} source file(s) failed processing: ${truncateNames(errored.map((f) => f.name))}`, suggestion: "Review the errors and retry processing these files.", }); } @@ -47,3 +41,9 @@ export const unprocessedSourcesCheck: HealthCheck = { return findings; }, }; + +function truncateNames(names: string[]): string { + if (names.length <= MAX_LISTED_FILES) return names.join(", "); + const shown = names.slice(0, MAX_LISTED_FILES).join(", "); + return `${shown} ...and ${names.length - MAX_LISTED_FILES} more`; +} diff --git a/src/doctor/health/engine.ts b/src/doctor/health/engine.ts index 398fb54..eb324c3 100644 --- a/src/doctor/health/engine.ts +++ b/src/doctor/health/engine.ts @@ -3,6 +3,15 @@ import { allHealthChecks } from "./checks/index.js"; /** Run all health checks and produce a report with findings. */ export function runHealthCheck(ctx: HealthContext): HealthReport { + // Pre-scan manifest once so individual checks don't need to + if (ctx.manifest) { + try { + ctx.manifest.scan(); + } catch { + // Scan failure is non-fatal — checks will see stale/empty manifest data + } + } + const findings = allHealthChecks.flatMap((check) => check.run(ctx)); const byCheck: Record = {}; diff --git a/test/doctor/health/phase-readiness.test.ts b/test/doctor/health/phase-readiness.test.ts index 1324bd9..a600ead 100644 --- a/test/doctor/health/phase-readiness.test.ts +++ b/test/doctor/health/phase-readiness.test.ts @@ -2,7 +2,6 @@ import { describe, it, expect, afterEach } from "vitest"; import * as fs from "node:fs"; import * as path from "node:path"; import * as os from "node:os"; -import * as YAML from "yaml"; import { DocumentStore } from "../../../src/storage/store.js"; import { phaseReadinessCheck } from "../../../src/doctor/health/checks/phase-readiness.js"; import type { HealthContext } from "../../../src/doctor/health/types.js"; @@ -14,18 +13,10 @@ function setup(phase?: string) { const docsDir = path.join(marvinDir, "docs"); fs.mkdirSync(docsDir, { recursive: true }); - const projectConfig: Record = { - name: "test-project", - methodology: "sap-aem", - }; - if (phase) { - projectConfig.aem = { currentPhase: phase }; - } - fs.writeFileSync(path.join(marvinDir, "config.yaml"), YAML.stringify(projectConfig), "utf-8"); - const config: MarvinProjectConfig = { name: "test-project", methodology: "sap-aem", + aem: phase ? { currentPhase: phase } : undefined, }; const registrations = [ @@ -98,6 +89,16 @@ describe("Phase Readiness Check", () => { expect(findings[0].message).toContain("no tech assessments"); }); + it("should not flag Phase 2 when use cases are only assessed (not approved)", () => { + const env = setup("assess-technology"); + tmpDir = env.tmpDir; + + env.store.create("use-case", { title: "UC1", status: "assessed" }); + + const findings = phaseReadinessCheck.run(env.ctx); + expect(findings).toHaveLength(0); + }); + it("should flag no extension designs in Phase 3 with recommended assessments", () => { const env = setup("define-solution"); tmpDir = env.tmpDir;