Skip to content

fix: address multi-workflow mode review feedback#138

Merged
intel352 merged 2 commits intofeat/issue-113-multi-workflowfrom
copilot/sub-pr-124
Feb 23, 2026
Merged

fix: address multi-workflow mode review feedback#138
intel352 merged 2 commits intofeat/issue-113-multi-workflowfrom
copilot/sub-pr-124

Conversation

Copy link
Contributor

Copilot AI commented Feb 23, 2026

Nine issues identified in the PR review for the multi-workflow mode implementation, ranging from DB constraint violations to silent errors and insecure defaults.

Changes

Correctness

  • WorkflowRecord was missing NOT NULL FK fields (ProjectID, CreatedBy, UpdatedBy) — added ensureSystemProject() to find/create a "system" company + "default" project and populate all required fields
  • bootstrapAdmin silently swallowed non-ErrNotFound DB errors; now uses errors.Is(err, evstore.ErrNotFound) and returns (uuid.UUID, error) so callers get the admin ID
  • pg.Workflows().List() error was discarded (existing, _ :=) — now propagated

Reliability

  • API server ListenAndServe failures (e.g. port already in use) were only logged; added srvErrCh so server errors trigger the same graceful shutdown path as SIGINT/SIGTERM

Maintainability

  • Extracted defaultEnginePlugins() []plugin.EnginePlugin — eliminates the duplicated 17-plugin list between buildEngine and the runMultiWorkflow engine builder closure

Minor

  • slugify() replaces the naive strings.ReplaceAll(name, " ", "-") — handles special chars, consecutive/leading/trailing hyphens, non-ASCII
  • URL display uses net.SplitHostPort so 0.0.0.0:8080 renders as http://localhost:8080/api/v1/ instead of http://localhost0.0.0.0:8080/api/v1/
  • JWT insecure-default log level upgraded from WarnError
  • Clarified misleading "single-config admin infrastructure" comment

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

- Extract defaultEnginePlugins() to eliminate plugin list duplication
- Fix bootstrapAdmin to check ErrNotFound explicitly; return admin UUID
- Fix seedWorkflow: handle List error, improve slug via slugify(), populate
  required WorkflowRecord fields (ProjectID, CreatedBy, UpdatedBy) using new
  ensureSystemProject() helper
- Change JWT secret logger.Warn -> logger.Error
- Use srvErrCh to propagate API server failures to initiate graceful shutdown
- Fix URL display using net.SplitHostPort for non-localhost bind addresses
- Fix misleading comment about admin infrastructure setup

Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>
Copilot AI changed the title [WIP] Wire multi-workflow mode into main server fix: address multi-workflow mode review feedback Feb 23, 2026
Copilot AI requested a review from intel352 February 23, 2026 14:20
@intel352 intel352 marked this pull request as ready for review February 23, 2026 14:31
@intel352 intel352 merged commit 1c75913 into feat/issue-113-multi-workflow Feb 23, 2026
@intel352 intel352 deleted the copilot/sub-pr-124 branch February 23, 2026 14:31
intel352 added a commit that referenced this pull request Feb 23, 2026
* feat: wire multi-workflow mode into main server (#113)

When -database-dsn is provided, the server starts in multi-workflow mode:
- Connects to PostgreSQL and runs migrations
- Creates WorkflowEngineManager for managing concurrent engines
- Mounts the REST API at /api/v1/ on the same HTTP server
- Bootstraps admin user when -admin-email and -admin-password are set
- -config flag becomes a seed for the initial workflow
- Without -database-dsn, single-config mode is preserved unchanged

Closes #113

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* fix: address multi-workflow mode review feedback (#138)

* Initial plan

* fix: address all review comments for multi-workflow mode

- Extract defaultEnginePlugins() to eliminate plugin list duplication
- Fix bootstrapAdmin to check ErrNotFound explicitly; return admin UUID
- Fix seedWorkflow: handle List error, improve slug via slugify(), populate
  required WorkflowRecord fields (ProjectID, CreatedBy, UpdatedBy) using new
  ensureSystemProject() helper
- Change JWT secret logger.Warn -> logger.Error
- Use srvErrCh to propagate API server failures to initiate graceful shutdown
- Fix URL display using net.SplitHostPort for non-localhost bind addresses
- Fix misleading comment about admin infrastructure setup

Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com>
Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants