Fix Workforce persona authoring for master workflows#66
Conversation
📝 WalkthroughWalkthroughThis PR removes an early-return guard in ChangesMaster Execution Workflow Persona Generation
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/product/generation/workforce-persona-writer.test.ts (1)
315-316: ⚡ Quick winStrengthen master plan assertion to verify preservation, not just existence.
toBeDefined()can miss metadata drift. Assert equality (or at least a stable subset) against the pre-persona baseline plan.Suggested assertion upgrade
expect(result.success).toBe(true); expect(result.masterExecutionPlan).toBeDefined(); + expect(result.masterExecutionPlan).toMatchObject(base.masterExecutionPlan!);🤖 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/product/generation/workforce-persona-writer.test.ts` around lines 315 - 316, Replace the loose existence check on result.masterExecutionPlan with a deterministic equality (or stable-subset) assertion against the pre-persona baseline plan to ensure metadata is preserved; specifically, change the assertion involving result.masterExecutionPlan to compare it to the baseline (e.g., use toEqual(prePersonaMasterExecutionPlan) or toMatchObject(prePersonaMasterExecutionPlanSubset)) and keep the calls length assertion as-is to still verify only one invocation.
🤖 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/product/generation/workforce-persona-writer.test.ts`:
- Around line 315-316: Replace the loose existence check on
result.masterExecutionPlan with a deterministic equality (or stable-subset)
assertion against the pre-persona baseline plan to ensure metadata is preserved;
specifically, change the assertion involving result.masterExecutionPlan to
compare it to the baseline (e.g., use toEqual(prePersonaMasterExecutionPlan) or
toMatchObject(prePersonaMasterExecutionPlanSubset)) and keep the calls length
assertion as-is to still verify only one invocation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ac89059c-9438-4d90-b9bf-fc1ef27de579
📒 Files selected for processing (2)
src/product/generation/pipeline.tssrc/product/generation/workforce-persona-writer.test.ts
💤 Files with no reviewable changes (1)
- src/product/generation/pipeline.ts
b7f09cb to
3f95ba9
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f95ba9b9f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (input.workforcePersonaWriter === false || !baseResult.artifact || !baseResult.success) { | ||
| return baseResult; |
There was a problem hiding this comment.
Preserve the master executor contract
When generateWithWorkforcePersona is called for a spec that produced baseResult.masterExecutionPlan, this path now sends the already-decomposed master workflow to the persona writer without passing the child plan or enforcing that the returned artifact still contains/materializes/runs those children. A persona can return any otherwise-valid workflow for the broad spec, pass validateGeneratedArtifact, and the return value still spreads the stale masterExecutionPlan, so callers believe they have a master executor while the artifact may no longer execute the planned child workflows. Please either keep master workflows out of this path or pass and validate the master plan/RICKY_MASTER_EXECUTOR_WORKFLOW contract before accepting the persona artifact.
Useful? React with 👍 / 👎.
Summary
Verification
Notes