feat: add check_project_health tool for governance health checks - #74
Merged
Conversation
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).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a health-check subsystem and MCP tool that inspects Marvin projects (artifacts, sources, sprints, actions, Jira, and AEM phase readiness), aggregates findings into a HealthReport, and exposes it via a new Changes
Sequence DiagramsequenceDiagram
actor User
participant MCP as "MCP Tool"
participant Engine as "Health Engine"
participant Checks as "Health Checks"
participant Store as "Document Store"
participant Manifest as "Source Manifest"
User->>MCP: call check_project_health
MCP->>Engine: runHealthCheck(ctx with store, config, manifest, marvinDir)
Engine->>Manifest: optional scan()
loop for each check in allHealthChecks
Engine->>Checks: check.run(ctx)
Checks->>Store: query counts / list entities
Checks->>Manifest: list pending/error (if present)
Store-->>Checks: entity data
Manifest-->>Checks: manifest entries
Checks-->>Engine: HealthFinding[]
end
Engine->>Engine: aggregate findings by severity & checkId
Engine-->>MCP: HealthReport (JSON)
MCP-->>User: report
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/agent/tools/doctor.ts (2)
65-78: Minor: narrow options before non-null assertions.
options.config!/options.marvinDir!work given the outer guard, but destructuring once into localconst { config, marvinDir, manifest } = options;after a narrowing check avoids the!assertions and keeps the closure typing clean.Proposed tweak
- if (options?.config && options?.marvinDir) { + if (options?.config && options?.marvinDir) { + const { config, marvinDir, manifest } = options; tools.push( tool( "check_project_health", "...", {}, async () => { try { - const report = runHealthCheck({ - store, - config: options.config!, - manifest: options.manifest, - marvinDir: options.marvinDir!, - }); + const report = runHealthCheck({ store, config, manifest, marvinDir });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/tools/doctor.ts` around lines 65 - 78, After checking options is truthy in the if (options?.config && options?.marvinDir) guard, destructure const { config, marvinDir, manifest } = options and use those local variables when calling runHealthCheck inside the check_project_health tool closure (and when passing marvinDir/config to runHealthCheck) instead of using options.config! and options.marvinDir!; this removes the non-null assertions and keeps closure typing clean while leaving the surrounding guard and tool(...) call intact.
65-103: Conditional tool registration creates brittle mismatch with hardcoded allowedTools.The
check_project_healthtool is registered conditionally when bothconfigandmarvinDirare provided, butallowedToolsinsrc/agent/session.ts(line 194) unconditionally includes it. While current code paths always provide both parameters, this creates a fragile pattern: ifcreateDoctorToolsis called without these options, the tool won't exist but the allowedTools list still references it, leaving callers with no signal about unavailability.Either always register the tool and return a structured error response ("project not initialized") when config/marvinDir are absent, or conditionally build
allowedToolsto match actual tool registration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/tools/doctor.ts` around lines 65 - 103, The conditional registration of the "check_project_health" tool creates a mismatch with allowedTools; change createDoctorTools so the tool("check_project_health", ...) is always pushed regardless of options, and inside its async handler check options.config and options.marvinDir; if either is missing return a structured error response (e.g., content with type "text" and message like "Project not initialized: missing config or marvinDir", plus isError: true) instead of attempting runHealthCheck, otherwise call runHealthCheck as before; this ensures allowedTools (referenced in src/agent/session.ts) always points to a registered tool and callers get a clear, consistent error when the project is not initialized.src/doctor/health/checks/unprocessed-sources.ts (1)
25-25: Consider truncating file name lists in messages.
pending.map((f) => f.name).join(", ")can produce very long messages if a project has many pending/errored sources, making the health report hard to read in tool output. Consider capping at ~10 names with an "…and N more" suffix.Also applies to: 37-37
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/doctor/health/checks/unprocessed-sources.ts` at line 25, The generated names string (const names = pending.map((f) => f.name).join(", ")) can become very long; change the logic that builds the names string (and the similar mapping at the other occurrence) to limit output to a fixed max (e.g., 10) by taking the first N pending.map(f => f.name), joining those with ", ", and if pending.length > N append a suffix like "…and X more" where X = pending.length - N; update the variable(s) (e.g., names) and any usage sites to use this truncated string.src/doctor/health/checks/empty-project.ts (1)
21-27: Duplicatemanifest.scan()across checks.Both
emptyProjectCheckandunprocessedSourcesCheckcallctx.manifest.scan(), causing redundant filesystem I/O per health run. Consider scanning once inrunHealthCheck(engine) before invoking checks, or memoizing on theHealthContext, so individual checks only consumemanifest.list(...).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/doctor/health/checks/empty-project.ts` around lines 21 - 27, Both emptyProjectCheck and unprocessedSourcesCheck redundantly call ctx.manifest.scan(); remove those scan calls from the checks and ensure the manifest is scanned only once prior to running checks by either invoking ctx.manifest.scan() inside runHealthCheck (engine) or adding a memoized scan method/property on HealthContext (e.g., HealthContext.ensureManifestScanned or a cached ctx.manifest.scanned true flag). Update emptyProjectCheck and unprocessedSourcesCheck to only call ctx.manifest.list(...) and rely on the single pre-scan, and add defensive no-op protection if scan has already run.src/doctor/health/checks/phase-readiness.ts (1)
1-3: Addaem?: { currentPhase?: string }toMarvinProjectConfigand usectx.config.aem?.currentPhaseinstead of re-reading YAML.
readPhaseduplicates I/O and YAML parsing on every health run despitectx.configalready containing the parsed configuration. This creates two sources of truth—methodology comes fromctx.configwhile the phase is read from disk, allowing them to diverge if the config is mutated. Additionally, all errors (including malformed YAML) are silently swallowed with no observation emitted. Reading the phase fromctx.configdirectly also eliminates the unusednode:fsandyamlimports from this check.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/doctor/health/checks/phase-readiness.ts` around lines 1 - 3, Add an optional aem field to the MarvinProjectConfig type (aem?: { currentPhase?: string }) and update the readPhase logic to use ctx.config.aem?.currentPhase instead of re-reading and parsing the YAML on disk (replace any calls to readPhase with direct access to ctx.config.aem?.currentPhase); remove the unused node:fs and yaml/YAML imports and any try/catch that previously swallowed file/YAML errors, and ensure you handle a missing/undefined phase value explicitly (emit an observation or fallback behavior where the code previously relied on the disk-read result).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/doctor/health/checks/no-jira-project.ts`:
- Line 17: The check treating ctx.config.jira?.projectKey as configured should
trim whitespace first because a whitespace-only string is truthy but invalid;
replace the direct truthiness test of ctx.config.jira?.projectKey with a
trimmed-value check (e.g., compute trimmed = ctx.config.jira?.projectKey?.trim()
and use that) so only non-empty, non-whitespace project keys bypass the finding
and maintain the rest of the logic in the same function where the current if
(ctx.config.jira?.projectKey) return [];.
In `@src/doctor/health/checks/phase-readiness.ts`:
- Around line 50-54: The variable approvedUCs currently includes both "assessed"
and "approved" statuses but its name and surrounding message claim "approved";
change the filter to only include uc.frontmatter.status === "approved" (keep the
approvedUCs name) so the selection matches the AEM lifecycle in
src/plugins/builtin/sap-aem.ts, and verify any message text that references
approved use case(s) remains accurate after this change; locate the array
creation via ctx.store.list({ type: "use-case" }) and the approvedUCs variable
in phase-readiness.ts to make the edit.
---
Nitpick comments:
In `@src/agent/tools/doctor.ts`:
- Around line 65-78: After checking options is truthy in the if (options?.config
&& options?.marvinDir) guard, destructure const { config, marvinDir, manifest }
= options and use those local variables when calling runHealthCheck inside the
check_project_health tool closure (and when passing marvinDir/config to
runHealthCheck) instead of using options.config! and options.marvinDir!; this
removes the non-null assertions and keeps closure typing clean while leaving the
surrounding guard and tool(...) call intact.
- Around line 65-103: The conditional registration of the "check_project_health"
tool creates a mismatch with allowedTools; change createDoctorTools so the
tool("check_project_health", ...) is always pushed regardless of options, and
inside its async handler check options.config and options.marvinDir; if either
is missing return a structured error response (e.g., content with type "text"
and message like "Project not initialized: missing config or marvinDir", plus
isError: true) instead of attempting runHealthCheck, otherwise call
runHealthCheck as before; this ensures allowedTools (referenced in
src/agent/session.ts) always points to a registered tool and callers get a
clear, consistent error when the project is not initialized.
In `@src/doctor/health/checks/empty-project.ts`:
- Around line 21-27: Both emptyProjectCheck and unprocessedSourcesCheck
redundantly call ctx.manifest.scan(); remove those scan calls from the checks
and ensure the manifest is scanned only once prior to running checks by either
invoking ctx.manifest.scan() inside runHealthCheck (engine) or adding a memoized
scan method/property on HealthContext (e.g., HealthContext.ensureManifestScanned
or a cached ctx.manifest.scanned true flag). Update emptyProjectCheck and
unprocessedSourcesCheck to only call ctx.manifest.list(...) and rely on the
single pre-scan, and add defensive no-op protection if scan has already run.
In `@src/doctor/health/checks/phase-readiness.ts`:
- Around line 1-3: Add an optional aem field to the MarvinProjectConfig type
(aem?: { currentPhase?: string }) and update the readPhase logic to use
ctx.config.aem?.currentPhase instead of re-reading and parsing the YAML on disk
(replace any calls to readPhase with direct access to
ctx.config.aem?.currentPhase); remove the unused node:fs and yaml/YAML imports
and any try/catch that previously swallowed file/YAML errors, and ensure you
handle a missing/undefined phase value explicitly (emit an observation or
fallback behavior where the code previously relied on the disk-read result).
In `@src/doctor/health/checks/unprocessed-sources.ts`:
- Line 25: The generated names string (const names = pending.map((f) =>
f.name).join(", ")) can become very long; change the logic that builds the names
string (and the similar mapping at the other occurrence) to limit output to a
fixed max (e.g., 10) by taking the first N pending.map(f => f.name), joining
those with ", ", and if pending.length > N append a suffix like "…and X more"
where X = pending.length - N; update the variable(s) (e.g., names) and any usage
sites to use this truncated string.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f0ba93c7-8c43-43ea-bfea-1a6e308af842
📒 Files selected for processing (15)
src/agent/mcp-server.tssrc/agent/session.tssrc/agent/tools/doctor.tssrc/doctor/health/checks/empty-project.tssrc/doctor/health/checks/index.tssrc/doctor/health/checks/no-jira-project.tssrc/doctor/health/checks/no-sprints.tssrc/doctor/health/checks/phase-readiness.tssrc/doctor/health/checks/unassigned-actions.tssrc/doctor/health/checks/unprocessed-sources.tssrc/doctor/health/engine.tssrc/doctor/health/types.tssrc/mcp/stdio-server.tstest/doctor/health/engine.test.tstest/doctor/health/phase-readiness.test.ts
A whitespace-only projectKey string is truthy but invalid — trim before the check so it correctly flags as unconfigured.
- 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
This was referenced Apr 22, 2026
6 tasks
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
1 participant
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
Implements feedback item #3 (Governance Health Check / Active Guardrails) from the eam-rpt-forecast PoC report.
Adds a new
check_project_healthMCP tool that inspects project state and returns soft recommendations about missing governance setup. This complements the existingrun_doctortool which focuses on document-level structural issues.Six health checks:
.marvin/sources/Also fixes:
run_doctorwas missing from the agent session'sallowedToolslist — now included alongsidecheck_project_health.Architecture: follows the existing doctor pattern — rule-based checks under
src/doctor/health/checks/, an engine that runs them all, and a tool that exposes the results via MCP. UsesHealthFindingwithrecommendation/observationseverities instead ofDoctorIssuewitherror/warning.Test plan
test/doctor/health/engine.test.ts— 8 tests covering all 6 checkstest/doctor/health/phase-readiness.test.ts— 7 tests for AEM phase logiccheck_project_healthon an empty project with source filescheck_project_healthon a project with actions but no sprintscheck_project_healthon a project with SAP AEM methodology