lisa/feat/workflow-seed-cmd#410
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a CLI ChangesWorkflow Seed Import
Sequence Diagram(s)sequenceDiagram
participant CLI as workflows CLI
participant Importer as importWorkflowsFromFile
participant Service as workflow service layer
participant DB as Database
CLI->>Importer: provide --file path, invoke command
Importer->>Importer: decode JSON, iterate definitions
Importer->>Service: validate & upsert WorkflowDefinition
Importer->>Service: upsert Steps, Dependencies, ControlRelationships, Instances
Service->>DB: create/update rows (upsert)
DB-->>Service: persisted results
Service-->>Importer: operation counts / errors
Importer-->>CLI: aggregated summary (created/updated/failed)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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.
Inline comments:
In `@cmd/seed/workflows_test.go`:
- Around line 112-125: The negative assertion in
assertWorkflowSeedControlRelationships is too weak: replace the current
conditional that checks relationship.ControlSource == "Technology control scope
is defined" with a strict expectation (e.g., assert that
relationship.ControlSource == "" or the exact expected stripped value). Update
the test in assertWorkflowSeedControlRelationships to fail unless
relationship.ControlSource equals the expected empty/stripped value, referencing
the workflows.ControlRelationship struct and its ControlSource field to locate
the check.
- Around line 17-19: The in-memory SQLite DB is volatile across pooled
connections; after opening with gorm.Open (sqlite.Open(":memory:")) get the
underlying sql.DB via db.DB() and constrain the pool to one connection (call
sqlDB.SetMaxOpenConns(1) and sqlDB.SetMaxIdleConns(1), and optionally
sqlDB.SetConnMaxLifetime(0)) so GORM uses a single persistent connection for the
in-memory database; update the code around the gorm.Open call and the local db
variable to retrieve and configure sqlDB accordingly.
In `@cmd/seed/workflows.go`:
- Around line 167-190: Before calling importWorkflowSeedDefinition, detect
duplicate seedDef.Key values in the incoming definitions slice and reject
duplicates instead of letting later upserts silently override earlier imports:
add a local seen map[string]struct{} (or bool) at the start of the function and,
inside the for _, definitions loop (before processing/trim check or immediately
after validating non-empty key), check if seedDef.Key is already in seen; if it
is, increment summary.Failed, log an error (include "key" seedDef.Key and "name"
seedDef.Name) and continue without calling importWorkflowSeedDefinition or
mergeWorkflowSeedSummary; otherwise add the key to seen and proceed as before.
- Around line 156-159: In importWorkflowsFromFile when decoding into
[]workflowSeedDefinition, enable strict JSON decoding by creating a
json.Decoder, calling decoder.DisallowUnknownFields() before
decoder.Decode(&definitions) to cause unknown/misspelled fields to error, and
propagate that error (including the original error) so importWorkflowSeeds fails
fast on unexpected attributes; locate the json.NewDecoder(f).Decode call and
replace it with the decoder + DisallowUnknownFields flow.
- Around line 394-397: preserveWorkflowInstanceSchedule currently treats any
non-nil DB error like gorm.ErrRecordNotFound and lets execution continue,
risking zeroing schedule fields during the upsert; change
preserveWorkflowInstanceSchedule(tx, instanceSvc, instance) to return an error,
and inside it inspect the error from the First(...) call: if err ==
gorm.ErrRecordNotFound then proceed without modifying LastExecutedAt, but for
any other err return that error immediately; then update the caller to check the
returned error (e.g., if err := preserveWorkflowInstanceSchedule(...); err !=
nil { return summary, fmt.Errorf("preserve workflow schedule: %w", err) })
before calling instanceSvc.ValidateInstance to ensure DB failures are bubbled
up.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6428dc9e-604c-4fae-95a0-993bb13a8e22
📒 Files selected for processing (4)
cmd/seed/seed.gocmd/seed/testdata/soc2_workflows.sample.jsoncmd/seed/workflows.gocmd/seed/workflows_test.go
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new seed workflows command to import workflow definitions (including steps, dependencies, control relationships, instances, and role assignments) from a JSON file, along with sample SOC 2 workflow templates and an end-to-end test validating idempotent imports.
Changes:
- Introduces
seed workflowsCobra command that reads JSON, validates entities, and upserts workflow-related records transactionally. - Adds sample SOC 2 workflow seed JSON for immediate import/use.
- Adds SQLite-based end-to-end test verifying counts, dependency wiring, relationship persistence, cron cadence persistence, and idempotent re-import behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| cmd/seed/workflows.go | Implements JSON import + validation + deterministic IDs + upsert logic for workflow seed data. |
| cmd/seed/workflows_test.go | Adds end-to-end test coverage for import correctness and idempotency. |
| cmd/seed/testdata/soc2_workflows.sample.json | Provides sample SOC 2 workflow templates used by tests and for seeding. |
| cmd/seed/seed.go | Registers the new workflows seed subcommand. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@cmd/seed/workflows_test.go`:
- Around line 73-100: The review comment about adding a context parameter is
incorrect—leave the test call to importWorkflowSeedDefinition(tx *gorm.DB,
seedDef workflowSeedDefinition) as-is because importWorkflowSeedDefinition is
defined without a context parameter (see importWorkflowSeedDefinition signature
in workflows.go); optionally tighten the two strings.Contains assertions into a
single targeted check (or replace with a typed error match) that ensures the
returned error contains both "duplicate step name" and the offending step name
"Collect evidence".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d6963da0-7914-44a3-9f61-5467d9c12cd5
📒 Files selected for processing (2)
cmd/seed/workflows.gocmd/seed/workflows_test.go
Contributor
Author
|
No new automated feedback in the past 64 minutes. Waiting for a human review before continuing. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@cmd/seed/workflows_test.go`:
- Around line 147-213: The test must assert the upsert return value for the
soft-deleted path (or add a complementary non-soft-deleted case) because
upsertWorkflowSeed currently computes the "created" flag via
tx.Model(value).Where("id = ?", id).Count(&count) which ignores rows with
DeletedAt.Valid = true; update
TestUpsertWorkflowSeedPreservesWorkflowDefinitionAuditAndSoftDeleteFields to
capture the (bool, error) returned from upsertWorkflowSeed and assert it is
false (or add a separate seed case where DeletedAt is nil and assert true) so
the test validates the created/updated flag behavior for upsertWorkflowSeed and
its OnConflict update path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: dc738fd2-9535-43d3-8630-12fbea10d10e
📒 Files selected for processing (2)
cmd/seed/workflows.gocmd/seed/workflows_test.go
gusfcarvalho
approved these changes
Jun 1, 2026
Contributor
Author
|
PR approved. Marking as ready for e2e. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
automated implementation by lisa.
Summary by CodeRabbit
New Features
Bug Fixes / Validation
Tests