refactor(workflows): rename execution plans to workflows#228
Merged
SantiagoDePolonia merged 3 commits intomainfrom Apr 13, 2026
Merged
refactor(workflows): rename execution plans to workflows#228SantiagoDePolonia merged 3 commits intomainfrom
SantiagoDePolonia merged 3 commits intomainfrom
Conversation
Contributor
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughSystematically replaces the "execution plan" concept with "workflow" across the codebase: types, config, middleware, server handlers, persistence (tables/columns), admin UI/assets, audit logging, cache/usage, guardrails, and docs—retaining existing behavior while renaming and repointing integrations. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HTTP_Server as Server
participant WorkflowSvc as WorkflowService
participant Guardrails
participant Provider
participant AuditDB as Audit/Storage
Client->>HTTP_Server: Request (path, headers, body)
HTTP_Server->>WorkflowSvc: WorkflowResolution(selector, request)
WorkflowSvc-->>HTTP_Server: *core.Workflow* (policy, features, resolved model)
HTTP_Server->>Guardrails: Patch request with Workflow pipeline
Guardrails-->>HTTP_Server: Patched request
HTTP_Server->>Provider: Dispatch request (resolved model, provider info)
Provider-->>HTTP_Server: Response
HTTP_Server->>AuditDB: Enrich & persist audit entry (workflow_version_id, workflow_features)
HTTP_Server->>Client: Response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
internal/auditlog/reader_postgresql.go (1)
94-121:⚠️ Potential issue | 🔴 CriticalError handling must use
core.GatewayErrorinstead offmt.Errorf.The code returns plain error values at lines ~99 and ~111 (
fmt.Errorf("failed to query audit logs: %w", err)andfmt.Errorf("failed to scan audit log row: %w", err)), which violates the guideline requiring all errors returned to clients to be instances ofcore.GatewayError. These should be replaced with appropriately typedcore.GatewayErrorvalues (likelyprovider_errorfor database failures).The column rename from
execution_plan_version_idtoworkflow_version_idis consistently applied across the SELECT list, scan variables, and field assignments, and the scan order correctly aligns with column positions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/auditlog/reader_postgresql.go` around lines 94 - 121, Replace the plain fmt.Errorf returns after the database operations with core.GatewayError instances: when r.pool.Query(ctx, dataQuery, dataArgs...) fails, return a core.GatewayError of type provider_error that includes the original err and a clear message like "failed to query audit logs"; likewise when rows.Scan(...) fails, return a core.GatewayError of type provider_error that wraps the scan error and a message like "failed to scan audit log row". Update the error construction at the r.pool.Query failure and the rows.Scan failure sites so they use the project's core.GatewayError constructor/API (preserving the original error as cause) instead of fmt.Errorf.internal/auditlog/store_sqlite.go (1)
70-80:⚠️ Potential issue | 🟠 MajorMigrate the existing
execution_plan_version_idcolumn before addingworkflow_version_id.Line 79 adds a fresh
workflow_version_idcolumn, but existing SQLite databases already have populatedexecution_plan_version_idvalues. After this upgrade,reader_sqlite.goonly readsworkflow_version_id, so historical audit rows will silently lose their version linkage until they are renamed/backfilled.🛠 Suggested migration update
if err := renameSQLiteAuditColumn(db, sqliteAuditLogTable, "model", "requested_model"); err != nil { return nil, fmt.Errorf("failed to rename audit_logs.model to requested_model: %w", err) } +if err := renameSQLiteAuditColumn(db, sqliteAuditLogTable, "execution_plan_version_id", "workflow_version_id"); err != nil { + return nil, fmt.Errorf("failed to rename audit_logs.execution_plan_version_id to workflow_version_id: %w", err) +} migrations := []string{ "ALTER TABLE audit_logs ADD COLUMN requested_model TEXT", "ALTER TABLE audit_logs ADD COLUMN resolved_model TEXT", "ALTER TABLE audit_logs ADD COLUMN provider_name TEXT", "ALTER TABLE audit_logs ADD COLUMN alias_used INTEGER DEFAULT 0", "ALTER TABLE audit_logs ADD COLUMN workflow_version_id TEXT", "ALTER TABLE audit_logs ADD COLUMN cache_type TEXT", "ALTER TABLE audit_logs ADD COLUMN auth_key_id TEXT", "ALTER TABLE audit_logs ADD COLUMN auth_method TEXT", "ALTER TABLE audit_logs ADD COLUMN user_path TEXT", }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/auditlog/store_sqlite.go` around lines 70 - 80, Existing rows with execution_plan_version_id will lose their version info because the migration adds workflow_version_id but doesn't migrate/rename existing execution_plan_version_id; update the migration in internal/auditlog/store_sqlite.go to preserve values by either (a) using the existing rename helper (renameSQLiteAuditColumn) to rename "execution_plan_version_id" to "workflow_version_id" before/when adding the new column, or (b) if renaming is not possible, add the new "workflow_version_id" column and immediately backfill it from "execution_plan_version_id" via an UPDATE so reader_sqlite.go continues to see versions; reference renameSQLiteAuditColumn and sqliteAuditLogTable to locate where to apply the change and ensure the migration runs before reader_sqlite.go starts reading the new column.internal/auditlog/store_postgresql.go (1)
87-93:⚠️ Potential issue | 🟠 MajorMigrate the legacy PostgreSQL column before creating the renamed one.
Line 92 adds
workflow_version_id, but nothing here migrates existingexecution_plan_version_iddata. On upgraded databases, historical audit rows will keep their version only in the old column, while new writes go to the new one. Please rename/copy the legacy column as part of startup migration and clean up the legacy index at the same time; otherwise old rows stop surfacing a workflow version, and a future column rename will leave duplicate indexes behind.Also applies to: 105-113
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/auditlog/store_postgresql.go` around lines 87 - 93, The migrations currently add workflow_version_id without migrating existing execution_plan_version_id values; update the migrations in the migrations slice in internal/auditlog/store_postgresql.go to first detect and migrate the legacy column (either by ALTER TABLE RENAME COLUMN execution_plan_version_id TO workflow_version_id when safe, or by adding workflow_version_id then running UPDATE audit_logs SET workflow_version_id = execution_plan_version_id WHERE workflow_version_id IS NULL), and then remove/drop the old execution_plan_version_id column and any legacy index (e.g., DROP INDEX IF EXISTS <execution_plan_version_id_index_name>) so historical rows surface the workflow version and no duplicate indexes remain; apply the same change to the later migration block referenced around lines 105-113.internal/auditlog/auditlog_test.go (1)
468-483:⚠️ Potential issue | 🟡 MinorThis test never exercises the legacy-vs-workflow precedence it names.
The setup only injects
core.WithWorkflow(...). If the middleware accidentally kept preferring the legacy resolution path, this test would still pass. Add a conflicting legacy resolution/request-metadata value here, or rename the test to match what it actually covers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/auditlog/auditlog_test.go` around lines 468 - 483, TestMiddleware_PrefersWorkflowOverLegacyResolution currently only injects core.WithWorkflow(...) so it never verifies precedence; update the test to also inject a conflicting legacy resolution/request-metadata value into req.Context() (e.g., set the older legacy resolution key with a different Requested/Resolved model) so the middleware must choose the Workflow's core.RequestModelResolution over the legacy value, or if you don't want to test precedence, rename TestMiddleware_PrefersWorkflowOverLegacyResolution to reflect that it only covers Workflow-based resolution.internal/workflows/store_postgresql.go (1)
31-62:⚠️ Potential issue | 🔴 CriticalDatabase schema migration for existing deployments is missing.
This refactor renamed the table from
execution_plan_versionstoworkflow_versionsand columns fromplan_payload/plan_hashtoworkflow_payload/workflow_hash. While the initialization code uses idempotent statements (CREATE TABLE IF NOT EXISTSandALTER TABLE ADD COLUMN IF NOT EXISTS), it does not migrate data from the old table to the new one.Existing deployments with an
execution_plan_versionstable will end up with:
- An empty
workflow_versionstable created by the initialization code- The old data remaining in
execution_plan_versionsuntouched- Runtime failures when the application tries to read from the new table
A data migration step must be added to copy data from
execution_plan_versionstoworkflow_versionsbefore dropping or archiving the old table.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/workflows/store_postgresql.go` around lines 31 - 62, The init SQL must include a data migration that copies rows from the old execution_plan_versions table into the new workflow_versions table mapping plan_payload->workflow_payload and plan_hash->workflow_hash and providing values for renamed/added columns (scope_user_path NULL, managed_default default FALSE, column mappings for scope_provider/scope_model/scope_key/version/name/description/created_at), and it must be idempotent (only insert rows that do not already exist in workflow_versions). Add an SQL statement to the statements slice (before any DROP/ALTER that would remove the old table) such as an INSERT INTO workflow_versions (...) SELECT ... FROM execution_plan_versions epv WHERE NOT EXISTS (SELECT 1 FROM workflow_versions wv WHERE wv.id = epv.id) so existing deployments are migrated; ensure you reference the same symbols used here (statements slice, ManagedDefaultGlobalName, ManagedDefaultGlobalDescription) and include this migration statement alongside the CREATE/ALTER statements.internal/app/app.go (1)
608-678:⚠️ Potential issue | 🟠 MajorStop workflow and guardrail refreshers before closing providers.
providers.Close()runs beforea.workflows.Close()anda.guardrails.Close(), even though both subsystems are initialized with refresh intervals and can call back into provider-backed executors. That leaves shutdown racing background refresh work against a torn-down provider stack. Close workflows/guardrails first, then tear down providers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/app/app.go` around lines 608 - 678, The shutdown currently calls a.providers.Close() before stopping refresh-driven subsystems, which can race with provider teardown; modify the shutdown ordering so a.workflows.Close() and a.guardrails.Close() are called (and their errors handled) before a.providers.Close(), relocating the existing workflows and guardrails close blocks to precede the providers close block and keeping the same slog.Error and errs append behavior for each Close() call.internal/admin/handler.go (1)
1499-1618:⚠️ Potential issue | 🟠 MajorMap workflow service failures to typed
core.GatewayErrors.These handlers forward non-validation
h.workflowserrors intohandleError, which downgrades them to the generic fallback response. Please wrap backend/storage failures with an explicit client-facing category (provider_erroris the likely default here) before returning, the same way validation and not-found are handled.As per coding guidelines "All errors returned to clients must be instances of
core.GatewayError" and "Use the typed client-facing error categoriesprovider_error,rate_limit_error,invalid_request_error,authentication_error, andnot_found_error".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/gomodel/docs/swagger.json`:
- Around line 2186-2190: The change renaming response fields is a breaking v1
admin API change; restore backward compatibility by keeping the old keys as
aliases or shipping a new API version: update the audit response schema to
include both "workflow_features" and the previous "execution_features" (both
referencing auditlog.WorkflowFeaturesSnapshot) and include both
"workflow_version_id" and the previous "execution_plan_version_id" (same type),
or alternatively implement a separate v2 response/schema and route this new
field naming behind that version; ensure the definitions referenced in the
swagger.json include both property names so existing clients keep working.
In `@config/config.go`:
- Around line 44-45: The rename removed support for the legacy keys
execution_plans.refresh_interval and ENV EXECUTION_PLAN_REFRESH_INTERVAL; update
the config loading so legacy keys are treated as deprecated aliases (or cause a
fast failure) by detecting them and mapping their value into the new field.
Concretely, inside the ExecutionPlansConfig unmarshalling path (or the central
config loader that constructs WorkflowsConfig / ResilienceConfig), check the raw
YAML/ENV input for the keys "execution_plans.refresh_interval" and
"EXECUTION_PLAN_REFRESH_INTERVAL"; if found, set the new RefreshInterval field,
emit a deprecation warning, and continue (or return an explicit error to
fail-fast if you prefer). Ensure any code paths that currently read
WorkflowsConfig/ResilienceConfig respect this mapping so existing deployments
keep their configured refresh interval for one release.
In `@docs/adr/0003-policy-resolved-workflow.md`:
- Around line 58-61: The ADR still mixes the old terms "plan" / "plan version"
with the new "workflow" terminology; update all occurrences in this ADR
(including the examples `(provider=NULL, model=NULL)`, `(provider=openai,
model=NULL)`, `(provider=openai, model=gpt-5)` and the sections around them) to
consistently use "workflow" and "workflow version" (or other chosen
workflow-native terms) everywhere the concepts refer to the same thing,
replacing any leftover "plan"/"plan version" phrasing and adjusting surrounding
sentences for grammatical consistency so the document uniformly reflects the new
terminology.
In `@docs/adr/0004-capability-model-and-provider-attempts.md`:
- Line 85: Typo: change the article in the sentence referencing ADR-0003 so "an
workflow" becomes "a workflow"; update the text in
docs/adr/0004-capability-model-and-provider-attempts.md where the phrase
"ADR-0003 defines how policies are resolved into an workflow" appears to read
"ADR-0003 defines how policies are resolved into a workflow" and commit the
correction.
In `@docs/superpowers/specs/2026-03-30-user-path-design.md`:
- Around line 9-10: The spec text uses "plans" in the phrase "path-scoped plans"
which is inconsistent with the rest of the document that uses "workflows";
update that occurrence to "path-scoped workflows" so the terminology is uniform
(search for the literal "path-scoped plans" and replace it with "path-scoped
workflows" in the spec document).
In `@internal/admin/dashboard/static/js/dashboard.js`:
- Around line 121-123: The branch checking and reassigning the page variable is
a no-op; either remove the entire if (page === 'workflows') { page =
'workflows'; } block, or restore the intended legacy rewrite by changing it to
if (page === 'execution-plans') { page = 'workflows'; } so old /execution-plans
links route to workflows; locate the code that sets/uses the page variable in
dashboard.js (the legacy-route rewrite) and apply one of these two fixes.
In `@internal/admin/dashboard/templates/index.html`:
- Around line 1305-1317: Add visible labels for the guardrail inputs inside the
workflow-guardrail-list-editor block: create unique ids for the ref and step
inputs (e.g., based on index like 'guardrail-ref-{{index}}' and
'guardrail-step-{{index}}'), add <label> elements that reference those ids and
include readable text like "Guardrail reference" and "Step", and ensure labels
are tied to the inputs bound by x-model (step.ref and step.step) so the visible
labels complement the existing aria-label attributes and follow the same pattern
used elsewhere in the template.
In `@internal/auditlog/reader_mongodb.go`:
- Around line 21-31: The mongoLogRow struct currently only declares
WorkflowVersionID (`bson:"workflow_version_id"`) so legacy documents with
`execution_plan_version_id` are lost; add a new field ExecutionPlanVersionID
with `bson:"execution_plan_version_id"` to mongoLogRow and update the mapping
into LogEntry (where WorkflowVersionID is assigned) to fall back to
ExecutionPlanVersionID when WorkflowVersionID is empty (i.e., set
targetLogEntry.WorkflowVersionID = mongo.WorkflowVersionID; if that is empty,
use mongo.ExecutionPlanVersionID).
In `@internal/auditlog/stream_wrapper.go`:
- Around line 84-93: The streaming audit entry construction omitted copying
ProviderName from the source entry, so when building the new record (the block
that copies fields from baseEntry into the streaming audit entry in
stream_wrapper.go) add ProviderName: baseEntry.ProviderName to the struct
literal; locate the code that currently sets ID, Timestamp, DurationNs,
RequestedModel, ResolvedModel, Provider, AliasUsed, WorkflowVersionID,
CacheType, StatusCode and insert ProviderName alongside them so the streaming
entry preserves baseEntry.ProviderName.
In `@internal/httpclient/client.go`:
- Line 71: The change removed spacing around the multiplication operator in
duration literals (e.g., "600*time.Second") causing inconsistent formatting with
other literals like "90 * time.Second"; revert those literals to use a space
around the "*" to match existing style (e.g., change the Timeout value produced
by getEnvDuration to use "600 * time.Second" and similarly restore spacing in
the nearby duration literals referenced in the same block), locating occurrences
via the Timeout field assignment and getEnvDuration usage in the HTTP client
configuration.
In `@internal/responsecache/semantic_test.go`:
- Line 35: Remove the trailing whitespace from the line defining the helper
function intPtr (func intPtr(v int) *int { return &v }) in semantic_test.go so
the file has no trailing spaces; locate the intPtr function and trim the extra
space at end of the line to satisfy formatting.
In `@internal/server/http.go`:
- Around line 328-332: Add deprecated compatibility aliases mapping the old
/execution-plans* routes to the new /workflows* handlers so existing clients
don't 404: register routes such as adminAPI.GET("/execution-plans",
cfg.AdminHandler.ListWorkflows), adminAPI.GET("/execution-plans/guardrails",
cfg.AdminHandler.ListWorkflowGuardrails), adminAPI.GET("/execution-plans/:id",
cfg.AdminHandler.GetWorkflow), adminAPI.POST("/execution-plans",
cfg.AdminHandler.CreateWorkflow), and
adminAPI.POST("/execution-plans/:id/deactivate",
cfg.AdminHandler.DeactivateWorkflow); mark these aliases as deprecated in
comments and, if needed, emit a warning log in the handler wrappers to encourage
migration and remove them in the next release.
In `@internal/server/passthrough_semantic_enrichment.go`:
- Line 12: The doc comment in internal/server/passthrough_semantic_enrichment.go
contains a duplicated word "workflow workflow resolution"; update the comment to
remove the duplicate so it reads "workflow resolution" (or rephrase as
appropriate) near the top of the file/comment block that references workflow
resolution to keep the documentation accurate (look for the comment containing
"workflow workflow resolution").
In `@internal/workflows/view.go`:
- Around line 10-17: The View struct embeds the persistence/domain model Version
which leaks storage internals into the admin API; replace the embedded Version
with explicit response fields (e.g., ID, CreatedAt, UpdatedAt, Name, State, etc.
that are needed) or stop embedding Version and map its fields into View in a
constructor/mapper function so the public JSON contract is intentional and
stable; update the View definition (remove "Version" embedding) and add a
NewViewFromVersion(or MapVersionToView) helper that copies only the intended
Version fields into View while leaving out internal/persistent-only fields and
tags.
---
Outside diff comments:
In `@internal/app/app.go`:
- Around line 608-678: The shutdown currently calls a.providers.Close() before
stopping refresh-driven subsystems, which can race with provider teardown;
modify the shutdown ordering so a.workflows.Close() and a.guardrails.Close() are
called (and their errors handled) before a.providers.Close(), relocating the
existing workflows and guardrails close blocks to precede the providers close
block and keeping the same slog.Error and errs append behavior for each Close()
call.
In `@internal/auditlog/auditlog_test.go`:
- Around line 468-483: TestMiddleware_PrefersWorkflowOverLegacyResolution
currently only injects core.WithWorkflow(...) so it never verifies precedence;
update the test to also inject a conflicting legacy resolution/request-metadata
value into req.Context() (e.g., set the older legacy resolution key with a
different Requested/Resolved model) so the middleware must choose the Workflow's
core.RequestModelResolution over the legacy value, or if you don't want to test
precedence, rename TestMiddleware_PrefersWorkflowOverLegacyResolution to reflect
that it only covers Workflow-based resolution.
In `@internal/auditlog/reader_postgresql.go`:
- Around line 94-121: Replace the plain fmt.Errorf returns after the database
operations with core.GatewayError instances: when r.pool.Query(ctx, dataQuery,
dataArgs...) fails, return a core.GatewayError of type provider_error that
includes the original err and a clear message like "failed to query audit logs";
likewise when rows.Scan(...) fails, return a core.GatewayError of type
provider_error that wraps the scan error and a message like "failed to scan
audit log row". Update the error construction at the r.pool.Query failure and
the rows.Scan failure sites so they use the project's core.GatewayError
constructor/API (preserving the original error as cause) instead of fmt.Errorf.
In `@internal/auditlog/store_postgresql.go`:
- Around line 87-93: The migrations currently add workflow_version_id without
migrating existing execution_plan_version_id values; update the migrations in
the migrations slice in internal/auditlog/store_postgresql.go to first detect
and migrate the legacy column (either by ALTER TABLE RENAME COLUMN
execution_plan_version_id TO workflow_version_id when safe, or by adding
workflow_version_id then running UPDATE audit_logs SET workflow_version_id =
execution_plan_version_id WHERE workflow_version_id IS NULL), and then
remove/drop the old execution_plan_version_id column and any legacy index (e.g.,
DROP INDEX IF EXISTS <execution_plan_version_id_index_name>) so historical rows
surface the workflow version and no duplicate indexes remain; apply the same
change to the later migration block referenced around lines 105-113.
In `@internal/auditlog/store_sqlite.go`:
- Around line 70-80: Existing rows with execution_plan_version_id will lose
their version info because the migration adds workflow_version_id but doesn't
migrate/rename existing execution_plan_version_id; update the migration in
internal/auditlog/store_sqlite.go to preserve values by either (a) using the
existing rename helper (renameSQLiteAuditColumn) to rename
"execution_plan_version_id" to "workflow_version_id" before/when adding the new
column, or (b) if renaming is not possible, add the new "workflow_version_id"
column and immediately backfill it from "execution_plan_version_id" via an
UPDATE so reader_sqlite.go continues to see versions; reference
renameSQLiteAuditColumn and sqliteAuditLogTable to locate where to apply the
change and ensure the migration runs before reader_sqlite.go starts reading the
new column.
In `@internal/workflows/store_postgresql.go`:
- Around line 31-62: The init SQL must include a data migration that copies rows
from the old execution_plan_versions table into the new workflow_versions table
mapping plan_payload->workflow_payload and plan_hash->workflow_hash and
providing values for renamed/added columns (scope_user_path NULL,
managed_default default FALSE, column mappings for
scope_provider/scope_model/scope_key/version/name/description/created_at), and
it must be idempotent (only insert rows that do not already exist in
workflow_versions). Add an SQL statement to the statements slice (before any
DROP/ALTER that would remove the old table) such as an INSERT INTO
workflow_versions (...) SELECT ... FROM execution_plan_versions epv WHERE NOT
EXISTS (SELECT 1 FROM workflow_versions wv WHERE wv.id = epv.id) so existing
deployments are migrated; ensure you reference the same symbols used here
(statements slice, ManagedDefaultGlobalName, ManagedDefaultGlobalDescription)
and include this migration statement alongside the CREATE/ALTER statements.
🪄 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
Run ID: 746cf01c-7684-4e19-b559-484cf4baf238
📒 Files selected for processing (123)
.env.template2026-03-16_ARCHITECTURE_SNAPSHOT.mdPossibleRefactoring.mdREADME.mdcmd/gomodel/docs/docs.gocmd/gomodel/docs/swagger.jsonconfig/cache_validation_test.goconfig/config.example.yamlconfig/config.goconfig/config_test.godocs/adr/0002-ingress-frame-and-semantic-envelope.mddocs/adr/0003-policy-resolved-workflow.mddocs/adr/0004-capability-model-and-provider-attempts.mddocs/adr/0006-semantic-response-cache.mddocs/advanced/workflows.mdxdocs/features/cache.mdxdocs/superpowers/plans/2026-03-30-user-path-tracking.mddocs/superpowers/specs/2026-03-30-user-path-design.mdinternal/admin/dashboard/dashboard_test.gointernal/admin/dashboard/static/css/dashboard.cssinternal/admin/dashboard/static/js/dashboard.jsinternal/admin/dashboard/static/js/modules/audit-list.jsinternal/admin/dashboard/static/js/modules/audit-list.test.jsinternal/admin/dashboard/static/js/modules/execution-plans-layout.test.jsinternal/admin/dashboard/static/js/modules/guardrails.jsinternal/admin/dashboard/static/js/modules/usage.jsinternal/admin/dashboard/static/js/modules/workflows-layout.test.jsinternal/admin/dashboard/static/js/modules/workflows.jsinternal/admin/dashboard/static/js/modules/workflows.test.jsinternal/admin/dashboard/templates/execution-plan-chart.htmlinternal/admin/dashboard/templates/index.htmlinternal/admin/dashboard/templates/layout.htmlinternal/admin/dashboard/templates/workflow-chart.htmlinternal/admin/handler.gointernal/admin/handler_guardrails_test.gointernal/admin/handler_workflows_test.gointernal/aliases/provider.gointernal/aliases/service.gointernal/app/app.gointernal/app/app_test.gointernal/app/runtime_refresh.gointernal/auditlog/auditlog.gointernal/auditlog/auditlog_test.gointernal/auditlog/middleware.gointernal/auditlog/middleware_test.gointernal/auditlog/reader_mongodb.gointernal/auditlog/reader_postgresql.gointernal/auditlog/reader_sqlite.gointernal/auditlog/store_mongodb.gointernal/auditlog/store_postgresql.gointernal/auditlog/store_postgresql_test.gointernal/auditlog/store_sqlite.gointernal/auditlog/store_sqlite_test.gointernal/auditlog/stream_wrapper.gointernal/batch/store.gointernal/cache/modelcache/modelcache.gointernal/cache/modelcache/redis_test.gointernal/core/context.gointernal/core/execution_plan_test.gointernal/core/passthrough.gointernal/core/requested_model_selector.gointernal/core/semantic.gointernal/core/workflow.gointernal/core/workflow_test.gointernal/executionplans/view.gointernal/guardrails/planned_executor.gointernal/guardrails/registry.gointernal/guardrails/system_prompt_test.gointernal/guardrails/workflow_executor.gointernal/httpclient/client.gointernal/modeloverrides/factory.gointernal/responsecache/handle_request_test.gointernal/responsecache/middleware_test.gointernal/responsecache/semantic.gointernal/responsecache/semantic_test.gointernal/responsecache/simple.gointernal/responsecache/usage_hit.gointernal/responsecache/vecstore_pgvector.gointernal/server/execution_policy.gointernal/server/fallback_test.gointernal/server/handlers.gointernal/server/handlers_test.gointernal/server/http.gointernal/server/http_test.gointernal/server/internal_chat_completion_executor.gointernal/server/internal_chat_completion_executor_test.gointernal/server/model_validation.gointernal/server/model_validation_test.gointernal/server/native_batch_service.gointernal/server/native_batch_support.gointernal/server/passthrough_execution_helpers.gointernal/server/passthrough_execution_helpers_test.gointernal/server/passthrough_semantic_enrichment.gointernal/server/passthrough_semantic_enrichment_test.gointernal/server/passthrough_service.gointernal/server/passthrough_support.gointernal/server/request_model_resolution.gointernal/server/request_snapshot_test.gointernal/server/translated_inference_service.gointernal/server/translated_inference_service_test.gointernal/server/translated_request_patcher.gointernal/server/workflow_helpers.gointernal/server/workflow_helpers_test.gointernal/server/workflow_policy.gointernal/server/workflow_policy_test.gointernal/workflows/compiler.gointernal/workflows/compiler_test.gointernal/workflows/factory.gointernal/workflows/service.gointernal/workflows/service_test.gointernal/workflows/store.gointernal/workflows/store_helpers.gointernal/workflows/store_helpers_test.gointernal/workflows/store_mongodb.gointernal/workflows/store_postgresql.gointernal/workflows/store_sqlite.gointernal/workflows/store_sqlite_test.gointernal/workflows/types.gointernal/workflows/types_test.gointernal/workflows/view.gotests/e2e/release-e2e-scenarios.mdtests/integration/setup_test.gotests/stress/stress_test.go
💤 Files with no reviewable changes (6)
- internal/core/execution_plan_test.go
- internal/admin/dashboard/templates/execution-plan-chart.html
- internal/executionplans/view.go
- internal/admin/dashboard/static/js/modules/execution-plans-layout.test.js
- internal/guardrails/planned_executor.go
- internal/server/execution_policy.go
Comment on lines
+2186
to
+2190
| "workflow_features": { | ||
| "description": "WorkflowFeatures captures the request-time effective workflow features\nafter runtime caps were applied. This keeps audit views historically accurate\neven if the active process config changes later.", | ||
| "allOf": [ | ||
| { | ||
| "$ref": "#/definitions/auditlog.ExecutionFeaturesSnapshot" | ||
| "$ref": "#/definitions/auditlog.WorkflowFeaturesSnapshot" |
Contributor
There was a problem hiding this comment.
These response-field renames are a breaking v1 admin API change.
Changing execution_features → workflow_features and execution_plan_version_id → workflow_version_id will break any existing consumer of /admin/api/v1/audit/... that still deserializes the old contract. Please either keep backward-compatible aliases for a deprecation window or ship this behind a new API version / explicit migration note.
Also applies to: 2264-2266
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/gomodel/docs/swagger.json` around lines 2186 - 2190, The change renaming
response fields is a breaking v1 admin API change; restore backward
compatibility by keeping the old keys as aliases or shipping a new API version:
update the audit response schema to include both "workflow_features" and the
previous "execution_features" (both referencing
auditlog.WorkflowFeaturesSnapshot) and include both "workflow_version_id" and
the previous "execution_plan_version_id" (same type), or alternatively implement
a separate v2 response/schema and route this new field naming behind that
version; ensure the definitions referenced in the swagger.json include both
property names so existing clients keep working.
Comment on lines
+44
to
+45
| Workflows WorkflowsConfig `yaml:"workflows"` | ||
| Resilience ResilienceConfig `yaml:"resilience"` |
Contributor
There was a problem hiding this comment.
Keep the legacy refresh config keys as deprecated aliases.
After this rename, execution_plans.refresh_interval and EXECUTION_PLAN_REFRESH_INTERVAL stop applying and the process silently falls back to the 1m default. That makes an in-place upgrade change runtime behavior without any explicit error. Please keep the old keys working for at least one release, or fail fast when legacy keys are present so operators don’t unknowingly lose their configured refresh interval.
Also applies to: 272-276, 938-940
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/config.go` around lines 44 - 45, The rename removed support for the
legacy keys execution_plans.refresh_interval and ENV
EXECUTION_PLAN_REFRESH_INTERVAL; update the config loading so legacy keys are
treated as deprecated aliases (or cause a fast failure) by detecting them and
mapping their value into the new field. Concretely, inside the
ExecutionPlansConfig unmarshalling path (or the central config loader that
constructs WorkflowsConfig / ResilienceConfig), check the raw YAML/ENV input for
the keys "execution_plans.refresh_interval" and
"EXECUTION_PLAN_REFRESH_INTERVAL"; if found, set the new RefreshInterval field,
emit a deprecation warning, and continue (or return an explicit error to
fail-fast if you prefer). Ensure any code paths that currently read
WorkflowsConfig/ResilienceConfig respect this mapping so existing deployments
keep their configured refresh interval for one release.
| func (m *mockEmbedder) Identity() string { return "test\x00mock-model" } | ||
|
|
||
| func intPtr(v int) *int { return &v } | ||
| func intPtr(v int) *int { return &v } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor observation: trailing whitespace.
Line 35 appears to have trailing whitespace added. This is a trivial formatting detail that doesn't affect functionality.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/responsecache/semantic_test.go` at line 35, Remove the trailing
whitespace from the line defining the helper function intPtr (func intPtr(v int)
*int { return &v }) in semantic_test.go so the file has no trailing spaces;
locate the intPtr function and trim the extra space at end of the line to
satisfy formatting.
Comment on lines
+328
to
+332
| adminAPI.GET("/workflows", cfg.AdminHandler.ListWorkflows) | ||
| adminAPI.GET("/workflows/guardrails", cfg.AdminHandler.ListWorkflowGuardrails) | ||
| adminAPI.GET("/workflows/:id", cfg.AdminHandler.GetWorkflow) | ||
| adminAPI.POST("/workflows", cfg.AdminHandler.CreateWorkflow) | ||
| adminAPI.POST("/workflows/:id/deactivate", cfg.AdminHandler.DeactivateWorkflow) |
Contributor
There was a problem hiding this comment.
Keep a compatibility alias for the old admin routes.
These v1 endpoints drop the previous /execution-plans* surface outright, so any existing admin scripts or clients will start returning 404s on rollout even though the API version did not change. Please either keep deprecated aliases for one release window or move this rename behind a version bump.
Possible compatibility shim
adminAPI.GET("/workflows", cfg.AdminHandler.ListWorkflows)
adminAPI.GET("/workflows/guardrails", cfg.AdminHandler.ListWorkflowGuardrails)
adminAPI.GET("/workflows/:id", cfg.AdminHandler.GetWorkflow)
adminAPI.POST("/workflows", cfg.AdminHandler.CreateWorkflow)
adminAPI.POST("/workflows/:id/deactivate", cfg.AdminHandler.DeactivateWorkflow)
+ // Temporary aliases for existing admin clients; remove after migration.
+ adminAPI.GET("/execution-plans", cfg.AdminHandler.ListWorkflows)
+ adminAPI.GET("/execution-plans/guardrails", cfg.AdminHandler.ListWorkflowGuardrails)
+ adminAPI.GET("/execution-plans/:id", cfg.AdminHandler.GetWorkflow)
+ adminAPI.POST("/execution-plans", cfg.AdminHandler.CreateWorkflow)
+ adminAPI.POST("/execution-plans/:id/deactivate", cfg.AdminHandler.DeactivateWorkflow)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| adminAPI.GET("/workflows", cfg.AdminHandler.ListWorkflows) | |
| adminAPI.GET("/workflows/guardrails", cfg.AdminHandler.ListWorkflowGuardrails) | |
| adminAPI.GET("/workflows/:id", cfg.AdminHandler.GetWorkflow) | |
| adminAPI.POST("/workflows", cfg.AdminHandler.CreateWorkflow) | |
| adminAPI.POST("/workflows/:id/deactivate", cfg.AdminHandler.DeactivateWorkflow) | |
| adminAPI.GET("/workflows", cfg.AdminHandler.ListWorkflows) | |
| adminAPI.GET("/workflows/guardrails", cfg.AdminHandler.ListWorkflowGuardrails) | |
| adminAPI.GET("/workflows/:id", cfg.AdminHandler.GetWorkflow) | |
| adminAPI.POST("/workflows", cfg.AdminHandler.CreateWorkflow) | |
| adminAPI.POST("/workflows/:id/deactivate", cfg.AdminHandler.DeactivateWorkflow) | |
| // Temporary aliases for existing admin clients; remove after migration. | |
| adminAPI.GET("/execution-plans", cfg.AdminHandler.ListWorkflows) | |
| adminAPI.GET("/execution-plans/guardrails", cfg.AdminHandler.ListWorkflowGuardrails) | |
| adminAPI.GET("/execution-plans/:id", cfg.AdminHandler.GetWorkflow) | |
| adminAPI.POST("/execution-plans", cfg.AdminHandler.CreateWorkflow) | |
| adminAPI.POST("/execution-plans/:id/deactivate", cfg.AdminHandler.DeactivateWorkflow) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/server/http.go` around lines 328 - 332, Add deprecated compatibility
aliases mapping the old /execution-plans* routes to the new /workflows* handlers
so existing clients don't 404: register routes such as
adminAPI.GET("/execution-plans", cfg.AdminHandler.ListWorkflows),
adminAPI.GET("/execution-plans/guardrails",
cfg.AdminHandler.ListWorkflowGuardrails), adminAPI.GET("/execution-plans/:id",
cfg.AdminHandler.GetWorkflow), adminAPI.POST("/execution-plans",
cfg.AdminHandler.CreateWorkflow), and
adminAPI.POST("/execution-plans/:id/deactivate",
cfg.AdminHandler.DeactivateWorkflow); mark these aliases as deprecated in
comments and, if needed, emit a warning log in the handler wrappers to encourage
migration and remove them in the next release.
This was referenced Apr 17, 2026
This was referenced Apr 25, 2026
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
1 participant
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.
Summary
Tests
Notes
Summary by CodeRabbit