feat: concept-explanation tools for queryable methodology knowledge#78
Merged
Conversation
Add a concept registry and three MCP tools (list_concepts, explain_concept, explain_phase) that expose Marvin's methodology definitions as structured, queryable data. Concepts cover AEM phases, rituals (Sprint 0, phase gates, iterative loops), artifact types, and persona roles. The registry is the single source of truth for methodology knowledge, replacing the need to extract definitions from persona guidance text. Filtering by category and methodology is supported, and AEM-specific checklist items are automatically included/excluded based on project config.
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a methodology concepts domain: core types, concept collections (phases, rituals, artifact types, roles), ALL_CONCEPTS, a ConceptRegistry with filtering and methodology-aware checklist behavior, three MCP tools (list_concepts, explain_concept, explain_phase), MCP server wiring, and tests for registry and tools. ChangesMethodology Concepts System
Sequence DiagramsequenceDiagram
participant Client
participant MCP_Tools as MCP Tools
participant Registry as ConceptRegistry
Client->>MCP_Tools: list_concepts(category?, methodology?)
MCP_Tools->>Registry: list(filter)
Registry-->>MCP_Tools: ConceptSummary[]
MCP_Tools-->>Client: JSON response
Client->>MCP_Tools: explain_concept(id, methodology?)
MCP_Tools->>Registry: explain(id, methodology)
Registry-->>MCP_Tools: ConceptDefinition (with conditional checklist)
MCP_Tools-->>Client: JSON response
Client->>MCP_Tools: explain_phase(name_or_id)
MCP_Tools->>Registry: get(id) or list() + name match
Registry-->>MCP_Tools: ConceptDefinition (validated as phase)
MCP_Tools-->>Client: JSON response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/methodology/registry.ts (1)
36-51: ⚡ Quick winReturn a copy consistently for safety and predictability.
The
explain()method returns the original internal concept reference on line 42 when no filtering is needed, but returns a shallow copy on line 50 when filtering is applied. This inconsistency could allow callers to accidentally mutate the registry's internal state in the unfiltered case.🔒 Proposed fix to always return a shallow copy
if (!concept.checklist || methodology === "aem") { - return concept; + return { ...concept }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/methodology/registry.ts` around lines 36 - 51, The explain() method currently returns the internal concept object (variable concept) when no filtering is needed but returns a shallow copy when filtering occurs; always return a shallow copy to avoid exposing internal state. Modify explain() so both branches return a new object (e.g., { ...concept, checklist: concept.checklist } or when filtered use filteredChecklist) instead of returning concept directly; ensure the checklist property is preserved (possibly undefined) and that the filteredChecklist is used when methodology !== "aem".test/methodology/tools.test.ts (1)
41-41: ⚡ Quick winAvoid hard-coding the phase count in this test.
expect(data.concepts.length).toBe(3)will break when new phase concepts are added, even if behavior is correct.Suggested patch
- expect(data.concepts.length).toBe(3); + expect(data.concepts.length).toBeGreaterThan(0);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/methodology/tools.test.ts` at line 41, The test currently hard-codes the phase count with expect(data.concepts.length).toBe(3), which will break when new phase concepts are added; update the assertion to derive the expected count dynamically (for example by comparing data.concepts.length to the source of truth such as the Phase enum/array or methodologyPhases list used by the implementation) or assert presence of specific expected concept names instead of an exact numeric literal; refer to data.concepts in the test and the Phase enum/array (or methodologyPhases) in the production code to compute the expected value and use that computed value in the expect() call.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/methodology/tools.ts`:
- Line 8: The code blindly casts config?.methodology to Methodology when setting
the methodology constant, which skips the config-ID mapping path; instead call
the existing normalization helper (e.g., normalizeMethodology or whichever
helper maps config IDs to Methodology) on config?.methodology and then fallback
to "generic-agile" if the normalized result is falsy. Replace the current
expression that uses (config?.methodology as Methodology) with a call like
normalizeMethodology(config?.methodology) ?? "generic-agile", keeping the
variable name methodology.
- Around line 110-120: The phase lookup and "available phases" message are
currently using registry.list({ category: "phase" }) which returns phases across
all methodologies; change both registry.list calls in explain_phase to scope to
the active methodology (e.g., pass the methodology identifier/filter used
elsewhere in code such as registry.list({ category: "phase", methodology }) or
registry.list({ category: "phase", methodologyId: methodology.id }) so the match
variable and available list only include phases for the current methodology;
ensure the name comparison (match = phases.find(...)) and the available string
(available = phases.map(...)) both use this filtered list.
- Line 7: The exported function createConceptTools currently returns
SdkMcpToolDefinition<any>[] which forces unsafe downstream casts; change the
exported signature to avoid any (e.g., use SdkMcpToolDefinition<unknown>[] or
make the function generic so each tool’s arg type is explicit) and then
narrow/validate args inside the tool handlers instead of casting (remove the
explicit `as ConceptCategory | undefined` and `as Methodology` casts in the tool
implementations and replace them with runtime checks or type guards). Apply the
same pattern to other tool factory functions (or introduce a small
wrapper/generic alias for SdkMcpToolDefinition<T>) so the codebase no longer
relies on `any` for tool args.
In `@test/methodology/registry.test.ts`:
- Around line 135-142: The test title is misleading: it says "includes AEM
addendum when no methodology specified" but the body (using ConceptRegistry,
registry.explain("sprint-0"), sprint0 and the aem-addendum check) asserts the
AEM section is excluded; update the it(...) description to reflect that behavior
(e.g., "excludes AEM addendum when no methodology specified (no filtering)") so
the test name matches the expectations and inline comment.
In `@test/methodology/tools.test.ts`:
- Around line 1-15: The test suite "Concept tools" (in
test/methodology/tools.test.ts) is missing per-test temp directory lifecycle;
add creation of a temp dir using fs.mkdtempSync in a beforeEach (or at top of
the describe) and remove it in afterEach (fs.rmSync with recursive and force) so
each test gets an isolated directory; update any helpers or tests that rely on a
temp path to use this per-test directory (refer to the describe("Concept tools")
block and helper functions extractHandler and parseResult to locate where to
wire the temp dir into test setup/teardown).
---
Nitpick comments:
In `@src/methodology/registry.ts`:
- Around line 36-51: The explain() method currently returns the internal concept
object (variable concept) when no filtering is needed but returns a shallow copy
when filtering occurs; always return a shallow copy to avoid exposing internal
state. Modify explain() so both branches return a new object (e.g., {
...concept, checklist: concept.checklist } or when filtered use
filteredChecklist) instead of returning concept directly; ensure the checklist
property is preserved (possibly undefined) and that the filteredChecklist is
used when methodology !== "aem".
In `@test/methodology/tools.test.ts`:
- Line 41: The test currently hard-codes the phase count with
expect(data.concepts.length).toBe(3), which will break when new phase concepts
are added; update the assertion to derive the expected count dynamically (for
example by comparing data.concepts.length to the source of truth such as the
Phase enum/array or methodologyPhases list used by the implementation) or assert
presence of specific expected concept names instead of an exact numeric literal;
refer to data.concepts in the test and the Phase enum/array (or
methodologyPhases) in the production code to compute the expected value and use
that computed value in the expect() call.
🪄 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: 298d4df8-41c5-4c47-8b42-8151de3a2217
📒 Files selected for processing (11)
src/agent/mcp-server.tssrc/methodology/concepts/artifact-types.tssrc/methodology/concepts/index.tssrc/methodology/concepts/phases.tssrc/methodology/concepts/rituals.tssrc/methodology/concepts/roles.tssrc/methodology/registry.tssrc/methodology/tools.tssrc/methodology/types.tstest/methodology/registry.test.tstest/methodology/tools.test.ts
| import type { Methodology, ConceptCategory } from "./types.js"; | ||
| import { getConceptRegistry } from "./registry.js"; | ||
|
|
||
| export function createConceptTools(config?: MarvinProjectConfig): SdkMcpToolDefinition<any>[] { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify `any` usage and downstream casts in changed methodology tool wiring.
rg -n --type=ts 'SdkMcpToolDefinition<any>|as ConceptCategory|as Methodology' src/methodology/tools.ts src/agent/mcp-server.tsRepository: ablanrob/marvin-cli
Length of output: 614
🏁 Script executed:
# Check the full function implementation to understand the context
sed -n '1,50p' src/methodology/tools.tsRepository: ablanrob/marvin-cli
Length of output: 2088
🏁 Script executed:
# Find the definition of SdkMcpToolDefinition
rg -n 'type SdkMcpToolDefinition|interface SdkMcpToolDefinition' --type=tsRepository: ablanrob/marvin-cli
Length of output: 3317
🏁 Script executed:
# Check what args type is used in the function and look for Zod schemas
rg -n 'args\.|ZodSchema\|z\.object' src/methodology/tools.ts -A 2 -B 2Repository: ablanrob/marvin-cli
Length of output: 1716
🏁 Script executed:
# Check if other tool functions in the codebase use <any> or have found a better pattern
rg -n 'export function.*Tool.*SdkMcpToolDefinition' src/ --type=ts -A 1 | head -40Repository: ablanrob/marvin-cli
Length of output: 2504
🏁 Script executed:
# Look for any type parameter examples in tool definitions
rg -n 'SdkMcpToolDefinition<(?!any)' src/ --type=tsRepository: ablanrob/marvin-cli
Length of output: 316
🏁 Script executed:
# Search with proper regex for any type parameter usage
rg -n 'SdkMcpToolDefinition<[^>]*>' src/ --type=ts | cut -d: -f3 | sort | uniq -cRepository: ablanrob/marvin-cli
Length of output: 1442
🏁 Script executed:
# Check the tool() function return type to understand if it enforces <any>
rg -n 'const.*=.*tool\(' src/methodology/tools.ts -A 5 | head -25Repository: ablanrob/marvin-cli
Length of output: 45
🏁 Script executed:
# Check if tool() function has generic parameters
rg -n 'function tool|const tool|export.*tool' node_modules/@anthropic-ai/claude-agent-sdk -A 3 2>/dev/null | head -20 || echo "SDK not in node_modules, checking imports"Repository: ablanrob/marvin-cli
Length of output: 45
🏁 Script executed:
# Look at how the tool() results are being used to understand the pattern better
sed -n '7,70p' src/methodology/tools.tsRepository: ablanrob/marvin-cli
Length of output: 2471
Remove any from the exported tool signature to preserve type safety.
SdkMcpToolDefinition<any>[] causes the downstream casts at lines 28–29. The any type on args forces explicit as ConceptCategory | undefined and as Methodology casts, violating the guideline: "Never use @ts-ignore or any unless absolutely unavoidable — prefer unknown with type narrowing."
Note: This pattern appears across all tool factory functions in the codebase. Consider a systematic approach (e.g., wrapper type, SDK upgrade, or documented exception) rather than addressing this file in isolation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/methodology/tools.ts` at line 7, The exported function createConceptTools
currently returns SdkMcpToolDefinition<any>[] which forces unsafe downstream
casts; change the exported signature to avoid any (e.g., use
SdkMcpToolDefinition<unknown>[] or make the function generic so each tool’s arg
type is explicit) and then narrow/validate args inside the tool handlers instead
of casting (remove the explicit `as ConceptCategory | undefined` and `as
Methodology` casts in the tool implementations and replace them with runtime
checks or type guards). Apply the same pattern to other tool factory functions
(or introduce a small wrapper/generic alias for SdkMcpToolDefinition<T>) so the
codebase no longer relies on `any` for tool args.
Comment on lines
+1
to
+15
| import { describe, it, expect } from "vitest"; | ||
| import { createConceptTools } from "../../src/methodology/tools.js"; | ||
| import type { MarvinProjectConfig } from "../../src/core/config.js"; | ||
|
|
||
| function extractHandler(tools: any[], name: string): (args: any) => Promise<any> { | ||
| const t = tools.find((tool) => tool.name === name); | ||
| if (!t) throw new Error(`Tool "${name}" not found`); | ||
| return (t as any).handler; | ||
| } | ||
|
|
||
| function parseResult(result: any): any { | ||
| return JSON.parse(result.content[0].text); | ||
| } | ||
|
|
||
| describe("Concept tools", () => { |
There was a problem hiding this comment.
Add per-test temp directory setup/cleanup for this suite.
Current tests skip the required temp-dir lifecycle (mkdtempSync + cleanup in afterEach).
As per coding guidelines, test/**/*.test.ts: "Each test should create a temp directory (fs.mkdtempSync) and clean up in afterEach."
Also applies to: 15-207
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/methodology/tools.test.ts` around lines 1 - 15, The test suite "Concept
tools" (in test/methodology/tools.test.ts) is missing per-test temp directory
lifecycle; add creation of a temp dir using fs.mkdtempSync in a beforeEach (or
at top of the describe) and remove it in afterEach (fs.rmSync with recursive and
force) so each test gets an isolated directory; update any helpers or tests that
rely on a temp path to use this per-test directory (refer to the
describe("Concept tools") block and helper functions extractHandler and
parseResult to locate where to wire the temp dir into test setup/teardown).
- Add normalizeMethodology() to map config IDs (sap-aem → aem) instead
of raw-casting config.methodology to Methodology type
- Scope explain_phase phase lookups to active methodology so only
relevant phases appear in suggestions
- Always return shallow copy from registry.explain() to avoid exposing
internal state
- Fix misleading test title ("includes" → "excludes" AEM addendum)
- Derive expected phase count from source data instead of hard-coding
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/methodology/types.ts (1)
47-50: ⚡ Quick winAvoid silently downgrading invalid methodology IDs
Unknown non-empty values currently fall back to
"generic-agile", which can mask config mistakes and return the wrong concept/checklist set. Keep fallback for missing values, but raise a typed config error for invalid IDs.Suggested refactor
+import { ConfigError } from "../core/errors.js"; + export function normalizeMethodology(configValue?: string): Methodology { - if (!configValue) return "generic-agile"; - return METHODOLOGY_ALIASES[configValue] ?? "generic-agile"; + if (!configValue?.trim()) return "generic-agile"; + const methodology = METHODOLOGY_ALIASES[configValue.trim().toLowerCase()]; + if (!methodology) { + throw new ConfigError(`Unsupported methodology: ${configValue}`); + } + return methodology; }As per coding guidelines "Use the project's error hierarchy in
src/core/errors.ts(MarvinError,ProjectNotFoundError,ConfigError,ApiKeyMissingError). Throw typed errors, never plain strings."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/methodology/types.ts` around lines 47 - 50, The current normalizeMethodology silently maps unknown non-empty IDs to "generic-agile"; instead, keep the existing fallback when configValue is missing but throw a typed ConfigError for invalid non-empty IDs. Update the normalizeMethodology function to: if configValue is falsy return "generic-agile"; else if METHODOLOGY_ALIASES has the key return that value; otherwise throw a ConfigError (from src/core/errors.ts) describing the invalid methodology id so callers get a typed error rather than a silent downgrade. Ensure the function signature and Methodology return type remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/methodology/types.ts`:
- Around line 47-50: The current normalizeMethodology silently maps unknown
non-empty IDs to "generic-agile"; instead, keep the existing fallback when
configValue is missing but throw a typed ConfigError for invalid non-empty IDs.
Update the normalizeMethodology function to: if configValue is falsy return
"generic-agile"; else if METHODOLOGY_ALIASES has the key return that value;
otherwise throw a ConfigError (from src/core/errors.ts) describing the invalid
methodology id so callers get a typed error rather than a silent downgrade.
Ensure the function signature and Methodology return type remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b0e765ee-8ca2-47e6-9ac1-703269a21514
📒 Files selected for processing (5)
src/methodology/registry.tssrc/methodology/tools.tssrc/methodology/types.tstest/methodology/registry.test.tstest/methodology/tools.test.ts
✅ Files skipped from review due to trivial changes (1)
- test/methodology/tools.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- test/methodology/registry.test.ts
- src/methodology/tools.ts
- src/methodology/registry.ts
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
src/methodology/) with 23 methodology concepts covering AEM phases, rituals (Sprint 0, phase gates, iterative loops), artifact types, and persona roleslist_concepts,explain_concept,explain_phasethat return structured, queryable methodology definitionsnormalizeMethodology()to map config IDs (sap-aem) to concept values (aem)Test plan
list_concepts({})returns all expected concepts (AC1.1)explain_concept("sprint-0")returns Sprint 0 checklist matching DM persona guidance (AC1.2)explain_concept("nonexistent-id")returns clear error suggestinglist_concepts(AC1.3)