fix: preserve vault CEL expressions during model type upgrades

[keeb]

Summary

• Fix upgrade persistence in executeWorkflow to re-read the original definition from the repository before persisting, preventing vault sentinel tokens from leaking into model YAML
• When a model type version bump triggers an upgrade, the in-memory definition has already had vault expressions resolved to per-process sentinel tokens — persisting those sentinels corrupts the YAML and breaks all subsequent runs
• The fix re-reads the on-disk definition (which has vault CEL expressions intact), applies the upgrade to that copy, and persists it

Closes swamp-club#91

Test Plan

• Reproduced the bug in /tmp/swamp-repro-issue-91 — confirmed sentinel __SWAMP_VSEC_* was written to YAML after version bump
• Verified the fix: vault expression survives upgrade and resolves correctly on subsequent runs
• Added unit test: executeWorkflow: upgrade persists original vault expressions, not sentinel tokens
• Added unit test: DefinitionUpgradeService: vault expression strings pass through unchanged
• All 4304 tests pass (1 pre-existing flaky failure in whoami_test.ts)
• Type checking, linting, and formatting pass

🤖 Generated with Claude Code

[github-actions]

Code Review

Clean, well-motivated bug fix. The problem is clear: when a model type version bump triggers a definition upgrade, the in-memory definition already has vault CEL expressions replaced with per-process sentinel tokens. Persisting those sentinels corrupts the YAML and breaks subsequent runs.

The fix correctly re-reads the on-disk definition (via findById), applies the upgrade to that pristine copy, and persists it — keeping vault expressions intact.

Blocking Issues

None.

Suggestions

1. Test narrowing cast (method_execution_service_test.ts:2616): The as unknown as Definition double-cast could be replaced with assertExists(savedDefinition) from @std/assert, which narrows the type and makes the assertion more idiomatic. Not blocking since the current approach works correctly in test code.

DDD Assessment

• Domain Service placement: The fix lives in DefaultMethodExecutionService, which is the correct domain service for orchestrating definition upgrades and persistence — no misplaced responsibilities.
• Repository pattern: Re-reading the pristine definition via DefinitionRepository.findById is exactly the right approach — the repository is the authority on persisted state, and the domain service correctly delegates to it rather than trying to track "original vs. resolved" state internally.
• Separation of concerns: DefinitionUpgradeService remains a pure, stateless domain service that takes a Definition and returns an upgraded copy. The persistence concern stays in the orchestrating service. Good separation.

Other Notes

• The if (originalDefinition) null guard at line 350 correctly handles the edge case where the definition was deleted between workflow start and upgrade persistence.
• No libswamp import boundary violations.
• License headers present on all modified files.
• Both unit tests are well-structured and follow the project's naming conventions.
• This is also a security improvement — it prevents vault sentinel tokens (representing secrets) from being written to YAML files on disk.

[github-actions]

Adversarial Review

Critical / High

None.

Medium

None.

Low

1. Silent no-op when findById returns null (src/domain/models/method_execution_service.ts:350): If the on-disk definition has been deleted between the in-memory load and the re-read (findById returns null), the upgrade is silently not persisted. The next run will re-trigger the upgrade and succeed (self-healing), so this is not a correctness problem. A logger.warn(...) here would aid debugging of unusual repository states, but it's not required.

2. Redundant save if concurrent process already upgraded (src/domain/models/method_execution_service.ts:351-358): The result of diskUpgrade.upgraded is not checked before save(). If another process already upgraded the on-disk definition to the target version, diskUpgrade.upgraded would be false and the save is a no-op write of the same data. Harmless — the upgrade is idempotent — but a if (diskUpgrade.upgraded) guard would save one unnecessary I/O.

Verdict

PASS — The fix is correct and well-targeted. The core logic is sound: re-reading from disk avoids persisting per-process vault sentinel tokens, the DefinitionUpgradeService.upgrade() is pure (returns new Definition via withUpgradedGlobalArguments, doesn't mutate), globalArguments returns structuredClone so upgrades on the disk copy can't corrupt the in-memory execution copy, and the null path from findById is handled. Both new tests cover the right things: the unit test verifies vault expressions survive the upgrade service, and the integration-style test verifies the full executeWorkflow path persists vault expressions instead of sentinels.