feat: add first-class workflow profiles#130
feat: add first-class workflow profiles#130RogerNavelsaker wants to merge 5 commits intojayminwest:mainfrom
Conversation
jayminwest
left a comment
There was a problem hiding this comment.
Review
Overall direction is strong — workflow profiles are a natural extension of the canopy profile system. A few issues to address before merge:
Must fix
-
Missing
--workflowvalidation onov slingandov coordinator start—ov workflow startcorrectly validates vianormalizeWorkflowNameand throwsValidationErrorfor unknown values. Butov sling --workflow bogusandov coordinator start --workflow bogussilently pass through —resolveProfileName("bogus")returns"bogus"(the passthrough branch), which then gets used as a canopy profile name that probably doesn't exist. Either validate on all three entry points, or at minimum emit a warning. -
repoRootFromCommandDiris fragile for installed packages (src/workflow.ts:87-89) —join(dirname(commandDir), "..")assumes the file layout is<root>/src/commands/. Other code in the repo usesjoin(import.meta.dir, "..", "..")directly (seeupdate.ts:89,init.ts:753). Consider inlining the two-level..join at the call site as other commands do, or renaming to something clearer likeresolveOverstoryPackageRootwith a doc comment explaining the layout assumption. -
specWriteCommandnow loads full config (src/commands/spec.ts) — Changed fromresolveProjectRoot(lightweight) toloadConfig(heavyweight, requires valid config.yaml). This could surface errors in edge cases like partially initialized.overstory/. Consider whetherresolveProjectRoot+ a targeted config read fordefaultProfilewould be less risky.
Non-blocking suggestions
discover.tschange addingworkflow: undefinedis redundant since the property is optional — consider removing.- Consider adding
ov-delivery.mdto the agent manifest if profiles should be discoverable viaov agents. - Shell completions for
ov workflow startinclude--monitorbut the help text doesn't describe what monitor does in this context.
cfcf84a to
4464165
Compare
|
I finished a Trellis-native replacement branch for this work and verified it locally. Replacement branch:
What it contains:
Local verification on the replacement branch passed:
Downstream dogfooding result is that the Trellis boundary is now stable:
I have not opened the replacement PR yet because there are two reasonable paths and I wanted to leave the choice with you:
I think option 2 is cleaner now that the downstream shape is proven, but either path works. Separately: Trellis itself is now in a presentable standalone state and is already being dogfooded downstream with Overstory. If you want it under the |
- Add 'workflow' to bash completions hardcoded commands string - Replace internal 'openspec' jargon with user-visible path in spec write description and --openspec option help text Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary
deliveryandco-creationacross coordinator startup,ov workflow start,ov sling, andov spec writeopenspec/changes/<task-id>/tasks.mdwhile keeping delivery on.overstory/specs/Closes #106.
Testing