Fix multiWorkflowAddr undeclared variable, path traversal in bundle import, and test robustness

[Copilot]

Several issues across the server binary and tests: missing flag declaration causing a compile error, a path traversal vulnerability in bundle workflow loading, silently swallowed cleanup errors, and fragile tests accessing private fields or ignoring errors.

cmd/server/main.go

• Add multiWorkflowAddr flag — was referenced at 3 call sites but never declared, causing a build failure
• Fix flag alignment — databaseDSN/jwtSecret had inconsistent spacing vs. the rest of the block
• Path traversal guard on bundle import — workflowPath from bundle.Import was read directly without verifying it stayed within destDir; now validated via filepath.Abs + filepath.Rel:

  rel, relErr := filepath.Rel(absDestDir, absWorkflowPath)
  if relErr != nil || strings.HasPrefix(rel, "..") || filepath.IsAbs(rel) {
      logger.Error("Workflow path is outside destination directory; possible path traversal", ...)
      continue
  }

• Log cleanup errors — os.Remove/os.RemoveAll failures are now logged (suppressing IsNotExist) instead of silently ignored

engine_structural_test.go

• Replace magic index allPlugins()[0] with findPluginByName("workflow-plugin-http") helper
• Remove private field access in TestEngineFactoryMapPopulatedByPlugins — uses schema.KnownModuleTypes() instead of engine.moduleFactories
• Add getEngineModuleTypes helper for TestSchemaKnowsPluginModuleTypes to encapsulate the remaining necessary factory map access

module/api_v1_test.go

• Add mustCreateCompany/Org/Project helpers — replace scattered _, _ ignoring patterns with fail-fast setup helpers
• Fix silently-ignored errors on ListWorkflows after delete, three UpdateWorkflow calls in versioning test, and setup in CreateAndListProjects
• Assert idempotency of EnsureSystemHierarchy — replaced _ = c2; _ = o2; _ = p2 with equality checks against the first-call IDs

plugins/admin/plugin_test.go

• TestPluginImplementsEnginePlugin — add runtime EngineManifest() call and Validate() check alongside the compile-time assertion
• TestModuleFactories — add nil guard with continue and assert mod.Name() matches the requested name
• TestWiringHookMergesAdminConfig — verify len(cfg.Modules) grows after the hook runs, not just that admin-server is present

Original prompt

Please apply the following diffs and create a pull request.
Once the PR is ready, give it a title based on the messages of the fixes being applied.

[{"message":"The variable `multiWorkflowAddr` is referenced but never declared or defined in this file. This will cause a compilation error. You should either declare this flag variable (e.g., `multiWorkflowAddr = flag.String(...)`) or use an existing flag like `addr`.","fixFiles":[{"filePath":"cmd/server/main.go","diff":"diff --git a/cmd/server/main.go b/cmd/server/main.go\n--- a/cmd/server/main.go\n+++ b/cmd/server/main.go\n@@ -85,8 +85,9 @@\n \t// Multi-workflow mode flags\n \tdatabaseDSN       = flag.String(\"database-dsn\", \"\", \"PostgreSQL connection string for multi-workflow mode\")\n \tjwtSecret         = flag.String(\"jwt-secret\", \"\", \"JWT signing secret for API authentication\")\n-\tadminEmail    = flag.String(\"admin-email\", \"\", \"Initial admin user email (first-run bootstrap)\")\n-\tadminPassword = flag.String(\"admin-password\", \"\", \"Initial admin user password (first-run bootstrap)\")\n+\tadminEmail        = flag.String(\"admin-email\", \"\", \"Initial admin user email (first-run bootstrap)\")\n+\tadminPassword     = flag.String(\"admin-password\", \"\", \"Initial admin user password (first-run bootstrap)\")\n+\tmultiWorkflowAddr = flag.String(\"multi-workflow-addr\", \":8081\", \"HTTP listen address (multi-workflow API)\")\n \n \t// License flags\n \tlicenseKey = flag.String(\"license-key\", \"\", \"License key for the workflow engine (or set WORKFLOW_LICENSE_KEY env var)\")\n"}]},{"message":"The variable name `adminEmail` is inconsistent with the naming pattern used for other flags. It should be `adminEmail` (no extra spaces) to maintain alignment consistency with other flag declarations, or adjust formatting across all flag declarations for uniformity.","fixFiles":[{"filePath":"cmd/server/main.go","diff":"diff --git a/cmd/server/main.go b/cmd/server/main.go\n--- a/cmd/server/main.go\n+++ b/cmd/server/main.go\n@@ -83,8 +83,8 @@\n \tanthropicModel = flag.String(\"anthropic-model\", \"\", \"Anthropic model name\")\n \n \t// Multi-workflow mode flags\n-\tdatabaseDSN       = flag.String(\"database-dsn\", \"\", \"PostgreSQL connection string for multi-workflow mode\")\n-\tjwtSecret         = flag.String(\"jwt-secret\", \"\", \"JWT signing secret for API authentication\")\n+\tdatabaseDSN   = flag.String(\"database-dsn\", \"\", \"PostgreSQL connection string for multi-workflow mode\")\n+\tjwtSecret     = flag.String(\"jwt-secret\", \"\", \"JWT signing secret for API authentication\")\n \tadminEmail    = flag.String(\"admin-email\", \"\", \"Initial admin user email (first-run bootstrap)\")\n \tadminPassword = flag.String(\"admin-password\", \"\", \"Initial admin user password (first-run bootstrap)\")\n \n"}]},{"message":"The comment claims the path is from 'trusted bundle extraction', but `workflowPath` comes from `bundle.Import(f, destDir)` where `destDir` is constructed from user-controlled `bundlePath` via `--import-bundle` flag. If the bundle contains path traversal sequences, this could read arbitrary files. Consider validating that `workflowPath` is within `destDir` before reading.","fixFiles":[{"filePath":"cmd/server/main.go","diff":"diff --git a/cmd/server/main.go b/cmd/server/main.go\n--- a/cmd/server/main.go\n+++ b/cmd/server/main.go\n@@ -1196,10 +1196,29 @@\n \t\t\tcontinue\n \t\t}\n \n+\t\t// Ensure the extracted workflow.yaml path is within the expected destination directory\n+\t\tabsDestDir, absDestErr := filepath.Abs(destDir)\n+\t\tif absDestErr != nil {\n+\t\t\tlogger.Error(\"Failed to resolve destination directory\", \"destDir\", destDir, \"error\", absDestErr)\n+\t\t\tcontinue\n+\t\t}\n+\n+\t\tabsWorkflowPath, absWorkflowErr := filepath.Abs(workflowPath)\n+\t\tif absWorkflowErr != nil {\n+\t\t\tlogger.Error(\"Failed to resolve workflow path\", \"path\", workflowPath, \"error\", absWorkflowErr)\n+\t\t\tcontinue\n+\t\t}\n+\n+\t\trel, relErr := filepath.Rel(absDestDir, absWorkflowPath)\n+\t\tif relErr != nil || strings.HasPrefix(rel, \"..\") || filepath.IsAbs(rel) {\n+\t\t\tlogger.Error(\"Workflow path is outside destination directory; possible path traversal\", \"path\", absWorkflowPath, \"destDir\", absDestDir, \"error\", relErr)\n+\t\t\tcontinue\n+\t\t}\n+\n \t\t// Read the extracted workflow.yaml\n-\t\tyamlData, err := os.ReadFile(workflowPath) //nolint:gosec // G703: path from trusted bundle extraction\n+\t\tyamlData, err := os.ReadFile(absWorkflowPath) //nolint:gosec // G703: path validated to be within destDir\n \t\tif err != nil {\n-\t\t\tlogger.Error(\"Failed to read workflow.yaml\", \"path\", workflowPath, \"error\", err)\n+\t\t\tlogger.Error(\"Failed to read workflow.yaml\", \"path\", absWorkflowPath, \"error\", err)\n \t\t\tcontinue\n \t\t}\n \t\tyamlContent := string(yamlData)\n"}]},{"message":"The cleanup operations ignore errors from `os.Remove` and `os.RemoveAll`. While the nolint comment addres...

---

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

[Copilot]

Pull request overview

This PR fixes multiple issues across the server binary and test suite: a compilation error from an undeclared flag variable, a path traversal security concern in bundle import handling, and several test robustness improvements.

Changes:

• Fixed undeclared multiWorkflowAddr flag causing compilation failure
• Added defense-in-depth path traversal validation for bundle workflow imports
• Improved test robustness by replacing error-ignoring patterns with fail-fast helpers and explicit assertions

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File	Description
cmd/server/main.go	Added missing multiWorkflowAddr flag declaration, aligned flag spacing, added path traversal validation for bundle imports, and improved cleanup error logging
engine_structural_test.go	Replaced fragile index-based plugin lookup with findPluginByName() helper, removed private field access using public schema.KnownModuleTypes() API, added getEngineModuleTypes() helper for necessary factory map access
module/api_v1_test.go	Added mustCreate* test helpers with proper error handling, fixed silently-ignored errors in multiple test functions, added explicit idempotency assertions
plugins/admin/plugin_test.go	Enhanced runtime interface validation, added nil guards and name assertions in factory tests, improved wiring hook test to verify module count increases