-
Notifications
You must be signed in to change notification settings - Fork 6
Magi PRD Generation Flow #82
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
packages/mobile-native-mcp-server/src/tools/magi/prd/magi-prd-orchestrator/tool.ts
Outdated
Show resolved
Hide resolved
packages/mobile-native-mcp-server/src/tools/magi/prd/magi-prd-orchestrator/tool.ts
Outdated
Show resolved
Hide resolved
packages/mobile-native-mcp-server/src/tools/magi/prd/magi-prd-orchestrator/tool.ts
Outdated
Show resolved
Hide resolved
packages/mobile-native-mcp-server/src/workflow/magi/prd/metadata.ts
Outdated
Show resolved
Hide resolved
|
|
||
| try { | ||
| // Ensure the magi-sdd directory exists in the project path | ||
| const prdWorkspacePath = ensureMagiSddDirectory(projectPath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No action for this PR, but we should sync up on specifics for abstracting the "working directory" functionality in the extraction. We'll probably need some tweaking on one end or the other (or both).
packages/mobile-native-mcp-server/src/workflow/magi/prd/nodes/prdFeatureBriefGeneration.ts
Outdated
Show resolved
Hide resolved
packages/mobile-native-mcp-server/src/workflow/magi/prd/nodes/prdMagiInitialization.ts
Show resolved
Hide resolved
packages/mobile-native-mcp-server/src/workflow/magi/prd/nodes/prdFeatureBriefReview.ts
Outdated
Show resolved
Hide resolved
|
@khawkins this one is ready for another look! |
| const userUtterance = state.userInput?.userUtterance as string; | ||
|
|
||
| // Validate that projectPath is provided | ||
| if (!projectPath || !userUtterance) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is intentionally a bit hand wavey here, but I'm specifically curious about how userUtterance relates to userInput. Are we envisioning that, as part of extraction, we would keep the user utterance wholesale, as part of the extraction?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed the naming is complicated - userUtterance is meant to capture the original user input so we never lose track of their original intent. userInput is the god-object that comes in for every orchestrator call
packages/mobile-native-mcp-server/src/utils/wellKnownDirectory.ts
Outdated
Show resolved
Hide resolved
packages/mobile-native-mcp-server/src/tools/magi/prd/magi-prd-feature-brief/tool.ts
Outdated
Show resolved
Hide resolved
packages/mobile-native-mcp-server/src/utils/wellKnownDirectory.ts
Outdated
Show resolved
Hide resolved
| // Create a new directory with the next available number | ||
| const nextNumber = getNextFeatureNumber(magiSddPath); | ||
| const paddedNumber = nextNumber.toString().padStart(3, '0'); | ||
| const featureDirName = `${paddedNumber}-${featureId}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a nit of consideration, but you may want to either validate that the LLM didn't create its own incremented numeric ID as part of creating the featureId, or explicitly tell the LLM not to do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved validation (including enforcing non numerical starts) to the schema, good suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the downfall of that approach however is if these methods were to used outside of this direct use case and the other consumers did not do upstream validation - we should probably put some stronger stakes in the ground from a code guidelines perspective of where validation SHOULD occur in magen in general
packages/mobile-native-mcp-server/src/tools/magi/prd/magi-prd-initial-requirements/tool.ts
Outdated
Show resolved
Hide resolved
| private generateGapRequirementsGuidance(input: GapRequirementsInput) { | ||
| const gapsList = input.identifiedGaps | ||
| .map( | ||
| (gap, index) => `${index + 1}. **${gap.title}** (${gap.severity} severity) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't have to be now, but generally speaking when data manipulation logic gets sufficiently tangled inline with the prompt template it's populating, I prefer to pull out the data manipulation logic into its own method, and encapsulate that work away from the prompt template.
Honestly at some point we probably need to move to a strict prompt template model, where the prompt template text is wholly static, and we replace inline keys with passed-in string values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you think I'm better of giving the LLM instructions on formatting?
i.e.
{gapsJson}
### Format for Reference
When reviewing the gaps above, format each gap as follows:
**Example Gap Format:**
\`\`\`
1. **Gap Title** (severity level)
- ID: gap-id
- Category: Category Name
- Description: Detailed description of what is missing
- Impact: What happens if this gap is not addressed
- Suggested Requirements: Requirement Title 1, Requirement Title 2, ...
\`\`\`
**Example:**
\`\`\`
1. **Missing Authentication Flow** (high severity)
- ID: GAP-001
- Category: Security
- Description: The requirements do not specify how users authenticate to the app
- Impact: Users cannot securely access the application, leading to security vulnerabilities
- Suggested Requirements: User Login Requirement, Biometric Authentication Support
\`\`\`
| 3. **Completeness**: Generate requirements that fill the identified gaps comprehensively | ||
| 4. **Integration**: Ensure new requirements integrate well with existing requirements | ||
| **Important**: When analyzing existing requirements, focus on **approved requirements** and **modified requirements**. Ignore **rejected requirements** and **out-of-scope requirements** as they have been explicitly excluded from the feature scope. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: for readability in development, we probably need to establish a max column width for our string lines. I think my thumb in the air has traditionally been to line break around 90 chars, but we could just formally enforce it by configuring max-len in our ESLint rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed - I suggest we do that as a fast follow in case it touches a bunch across the project and creates a lot of noise
|
|
||
| private generateRequirementsReviewGuidance(input: RequirementsReviewInput) { | ||
| return ` | ||
| You are facilitating a requirements review session with the user. Your role is to review the requirements.md file with the user and facilitate their decisions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just an observation: this prompt is pretty heavy on instructions / responsibilities. Have you had any problems with deviations from the script?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's also a lot of use of "CRITICAL, IMPORTANT, MANDATORY, REMEMBER". This is often a sign that a set of tasks needs to be decomposed into smaller sets of responsibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah this is one of the more complicated nodes - so far it's behaved well for me but I think it's one to put on the watch list as we start testing. The complexity comes because there's a binary decision to be made: "do you want to review requirements or finalize with pending changes?" - technically we can split that decision out into a branching node but I think it would negatively impact user experience where a simple yes/no would kick of a chain of tool calls - but we'll see maybe that will be necessary... I always waffle between how small of tasks each node should do at the expense of user experience
What does this PR do?
This implements the first workflow for magi, our SDD process within Magen.
It implements the design here (some modifications to the design doc in this PR)
Refer to documentation for how this is structured.
Review should primarily focus on the graph, state and input/output schemas.
There is a lot of duplication with our main magen workflow which much will disappear as we work on abstracting out the common bits (actively being worked on)
What issues does this PR fix or reference?
W-20034988