fix: region bug and minor UI changes#11
Merged
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses a region-mismatch bug in pack detonations, improves deletion cleanup to reclaim Terraform working directories, and makes a few UI/UX tweaks (toast notifications, links, toaster placement) alongside updated OpenSpec documentation.
Changes:
- Pin detonation AWS region (
AWS_REGION/AWS_DEFAULT_REGION) to the resolvedaws_regionparameter to prevent cross-region NotFound issues. - Extend run deletion to best-effort remove Terraform working directories keyed by
scenario_results.execution_id, with tests and spec updates. - UI: replace inline delete errors with toast notifications, adjust toaster position, and link assessment run details directly to the scenario.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| web/frontend/src/routes/scenarios/+page.svelte | Uses toast notifications for scenario deletion feedback. |
| web/frontend/src/routes/assessments/+page.svelte | Uses toast notifications for assessment deletion feedback. |
| web/frontend/src/routes/assessments/[runId]/+page.svelte | Links run details to the specific scenario route. |
| web/frontend/src/routes/+layout.svelte | Moves toaster position to top-center. |
| web/frontend/src/lib/components/RetentionDialog.svelte | Avoids PUT-ing invalid days for disabled retention sections. |
| web/frontend/src/lib/components/PackParametersDialog.svelte | Validates string-map parameters to prevent blank keys/values. |
| web/frontend/src/lib/components/PackCard.svelte | Adds toast success/error feedback on pack deletion. |
| pack/builtins.go | Exposes DefaultAWSRegion constant for shared defaulting. |
| internal/detonators/simrun_detonator.go | Ensures execution_id is returned on failure paths; pins AWS region env for detonation. |
| internal/detonators/simrun_detonator_test.go | Adds tests for detonation AWS region env precedence/defaulting. |
| internal/runner/runner.go | Preserves execution_id even when scenario execution fails. |
| internal/runner/runner_test.go | Updates tests to assert failed scenarios retain execution_id. |
| internal/web/run_deletion.go | Adds best-effort Terraform-dir cleanup on run deletion. |
| internal/web/retention_test.go | Adds sweep tests ensuring Terraform dirs are removed/skipped safely. |
| openspec/specs/runs/spec.md | Updates delete-run requirement to include Terraform-dir cleanup constraints. |
| openspec/changes/archive/2026-06-22-purge-terraform-dirs/tasks.md | Archived change task checklist for Terraform-dir purge work. |
| openspec/changes/archive/2026-06-22-purge-terraform-dirs/specs/runs/spec.md | Archived snapshot of modified run-delete requirements. |
| openspec/changes/archive/2026-06-22-purge-terraform-dirs/proposal.md | Archived proposal describing motivation and impact. |
| openspec/changes/archive/2026-06-22-purge-terraform-dirs/design.md | Archived design covering safety decisions and risks. |
| openspec/changes/archive/2026-06-22-purge-terraform-dirs/.openspec.yaml | Archived OpenSpec metadata for the change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+73
to
+83
| for _, id := range executionIDs { | ||
| if strings.TrimSpace(id) == "" || | ||
| strings.ContainsRune(id, '/') || strings.ContainsRune(id, filepath.Separator) { | ||
| continue | ||
| } | ||
| dir := filepath.Join(dataDir, "terraform", id) | ||
| if err := os.RemoveAll(dir); err != nil && !os.IsNotExist(err) { | ||
| logrus.WithError(err).WithField("path", dir).WithField("run_id", runID). | ||
| Warn("failed to remove Terraform working directory") | ||
| } | ||
| } |
Comment on lines
+182
to
+183
| for _, execID := range []string{"", " ", "../escape", "nested/id"} { | ||
| t.Run("id="+execID, func(t *testing.T) { |
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.
Summary
Fixes a region-mismatch bug that caused detonations to act on the wrong AWS region (producing
NotFounderrors), preservesexecution_idon detonation failures so partial runs stay correlatable, reclaims Terraform working directories on run deletion, and ships several minor UI fixes (toast notifications, retention/pack validation, scenario links).Bug Fixes
var.aws_region, but the detonation resolved its region from the container's ambientAWS_REGIONvia the SDK default chain. When they diverged, Terraform created resources in one region while the detonation acted on another — surfacing asNotFound. A newdetonationEnvVarsnow pinsAWS_REGION/AWS_DEFAULT_REGIONto the resolvedaws_region, mirroringterraformEnvVarsprecedence (per-sim > pack-level > built-in default).DefaultAWSRegionis extracted as a shared constant inpack/builtins.go.execution_idon failure paths.SimrunDetonator.DetonateandRunner.runScenarionow carry theexecution_idout on every failure past ID generation, so a partially-completed detonation (e.g. Terraform applied, then a timeout) stays correlatable in the persisted result instead of returning a nil/empty map./scenarios/{scenarioId}instead of the generic/scenarioslist.Terraform Directory Cleanup
deleteRunWithArtifacts(shared by manual delete and the retention sweeper) now best-effort removes<DataDir>/terraform/<execution_id>/for each scenario result viaos.RemoveAll. Blank/whitespace or path-bearing execution IDs are skipped so cleanup can never escape or wipe theterraform/base. Accompanied by the archived OpenSpec change (purge-terraform-dirs) and updatedrunsspec.UI Changes
svelte-sonnertoasts instead of failing silently or rendering inline errors. Toaster repositioned totop-center.PackParametersDialognow validates string-map params (e.g.default_tags) up front, since empty tag keys/values can't be applied by Terraform/cloud providers and would silently break every sim in the pack.Tests
TestDetonationEnvVars_AWSRegion), failure-pathexecution_idpreservation, and Terraform-dir cleanup (removal, unsafe-ID skip, missing-dir best-effort).