fix(sync): enforce agent preset version immutability on replay - #3130
Open
daryllimyt wants to merge 9 commits into
Open
fix(sync): enforce agent preset version immutability on replay#3130daryllimyt wants to merge 9 commits into
daryllimyt wants to merge 9 commits into
Conversation
- Compute dry-run pull diffs from the correlated snapshot so an already-applied cross-environment preset no longer previews as a spurious modification. - Clear the deployment-local base_url when remapping a catalog UUID so the target's provider credentials are never routed to the source deployment's endpoint (openai/anthropic honor preset base_url at the gateway; custom providers re-resolve it from local provider config).
Add set-based AgentCatalogService.enabled_catalog_ids and resolve_catalog_ids_by_models with identical visibility, enablement, org-over-platform precedence, and id tie-break semantics as the single-item resolvers, and rewire agent preset correlation to resolve all incoming catalog UUIDs and model tuples in at most three round trips (override scope check, enabled-id set, model resolution) instead of up to four queries per distinct key.
Replace the bare tuples in agent preset catalog correlation with named types, matching the PreparedSnapshot pattern: - CorrelatedAgentPresets for the (presets, diagnostics) return of AgentPresetAdapter.correlate_catalog_ids - ModelKey for the (model_provider, model_name) pair used to resolve deployment-local catalog ids Both are NamedTuples, so existing tuple unpacking and dict lookups with plain tuples are unaffected.
Quality-only cleanup of the agent preset catalog correlation path; no behavior change. - Drop the unused resolve_catalog_ids_by_models batch resolver, whose query duplicated catalog_candidates_by_models. - Collapse CatalogMatchCandidate into CatalogMappingCandidate so the catalog service and sync vocabulary share one type. - Add _failed_pull_result to replace three near-identical failure results, and drop the always-empty diagnostics splat in the dry run. - Reduce the preset rewrite loop to two branches and skip rebuilding the spec when no catalog id is remapped. - Precompute reference paths and drop the redundant catalog id field. - Memoize pull tab selection state and make candidate label disambiguation a single pass. - Extract catalog-enabling and YAML patch helpers in the sync tests.
|
✅ No security or compliance issues detected. Reviewed everything up to dfc6b53. Security Overview
Detected Code Changes
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dfc6b53f43
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| changed_fields = [ | ||
| key | ||
| for key, desired_value in desired_attrs.items() | ||
| if getattr(version, key) != desired_value |
There was a problem hiding this comment.
When a preset version was initially imported with noncanonical ordering (for example actions: ["z", "a"], similarly namespaces, mcp_integrations, or subagents), this direct list equality reports different content after a normal export/import round trip: _version_specs_for_preset and _subagent_refs sort these collections during projection, while _version_attrs_from_spec and _resolved_subagents_config preserve the incoming order when storing them. Pulling the projected commit therefore raises immutable_agent_preset_version_conflict and rolls back even though the version is semantically unchanged; normalize these unordered collections before insertion/comparison.
Useful? React with 👍 / 👎.
jordan-umusu
force-pushed
the
daryl/ws-sync-custom-catalog
branch
2 times, most recently
from
July 27, 2026 22:44
80dbcc7 to
89321fa
Compare
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.
Stacked on #3117 (
daryl/ws-sync-custom-catalog).Problem
_upsert_agent_preset_versionunconditionally overwrote an existing exact-numberAgentPresetVersionrow's attributes — includingcatalog_idandbase_url— and its caller then unconditionally replaced the version's skill bindings. Preset versions are pinned by consumers, and #3117 introduces per-pull catalog mapping choices, so replaying the same Git commit with a different mapping choice silently rewrote a historical version's provider routing for everything pinned to it.Fix
_version_attrs_from_spec, resolved subagent config, and the skill-binding set) is compared against the stored state via a shared_version_changed_fieldshelper (_version_matches_presetnow delegates to it, same contract).TracecatValidationErrorcarryingdetail.code = "immutable_agent_preset_version_conflict"plus the preset slug, version number, and the list of changed field names. The transaction rolls back, so the stored version keeps its original content.TracecatException.detailinto the failure diagnostic'sdetails, so the conflict code reaches the pull result machine-readably instead of only as stringified text.To intentionally change a version's provider routing, publish a new preset version; rebinding an existing pinned version would be a separate explicit feature.
Review guide
The invariant to check is in
_upsert_agent_preset_version(tracecat/workspace_sync/adapters/agent_preset.py): the existing-row branch performs no attribute assignment, no flush, and no binding replacement. The comparison-normalization asymmetries (actions or None, default-filled model fields, resolvedagents) are the false-positive risk; the regression tests exercise the real pull path twice specifically to catch those.Tests
Four new regressions in
tests/unit/test_workspace_sync_acceptance_contract.py, all through the real sync pull path:test_agent_preset_exact_version_replay_is_no_optest_agent_preset_conflicting_exact_version_replay_rolls_backtest_agent_preset_higher_version_replay_preserves_existing_versiontest_agent_preset_defaulted_exact_version_replay_is_no_op(normalization/defaults round-trip)Suite: 109 passed. Ruff, ruff format, and basedpyright (
--warnings, including the test file) are clean.Summary by cubic
Prevents accidental mutation of existing exact-number
AgentPresetVersionon sync replay by enforcing immutability and surfacing structured conflicts. Exact replays are no-ops; conflicting replays fail and roll back safely.TracecatValidationErrorwith codeimmutable_agent_preset_version_conflictincluding preset slug, version, and changed fields; transaction rolls back.TracecatException.detailfor machine-readable error details.Written for commit dfc6b53. Summary will update on new commits.