Migrate branding spec to deployConfig + transformRemoteToLocal#6910
Migrate branding spec to deployConfig + transformRemoteToLocal#6910ryancbahan wants to merge 1 commit intorcb/contract-point-of-salefrom
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
We detected some changes at Caution DO NOT create changesets for features which you do not wish to be included in the public changelog of the next CLI release. |
ryancbahan
force-pushed
the
rcb/contract-point-of-sale
branch
from
036d663 to
e0af67f
Compare
ryancbahan
force-pushed
the
rcb/contract-branding
branch
from
1f345db to
805f23f
Compare
Coverage report
Test suite run success3782 tests passing in 1450 suites. Report generated by 🧪jest coverage report action from 0053593 |
![binks-code-reviewer[bot]](https://avatars.githubusercontent.com/in/2788732?s=60&v=4){kind=small}
| identifier: BrandingSpecIdentifier, | ||
| schema: BrandingSchema, | ||
| transformConfig: BrandingTransformConfig, | ||
| deployConfig: async (config) => configWithoutFirstClassFields(config), |
There was a problem hiding this comment.
deployConfig signature mismatch (ignored params) may cause behavior drift
ExtensionSpecification.deployConfig is typed as (config, directory, apiKey, moduleId?) => Promise<...> (per specification.ts). In app_config_branding.ts, deployConfig is now async (config) => ..., ignoring directory/apiKey/moduleId. While extra args are tolerated at runtime, this can silently diverge from wrappers/future logic that rely on arity/params (logging, auditing, feature flags, module scoping). It’s also unclear whether configWithoutFirstClassFields alone preserves the required remote payload key shape previously handled by transformConfig.
| identifier: BrandingSpecIdentifier, | ||
| schema: BrandingSchema, | ||
| transformConfig: BrandingTransformConfig, | ||
| deployConfig: async (config) => configWithoutFirstClassFields(config), |
There was a problem hiding this comment.
Removing transformLocalToRemote may change remote payload keys (handle vs app_handle)
Previously, transformConfig provided a forward transform (handle -> app_handle). With transformConfig removed and transformLocalToRemote now undefined (and explicitly tested), branding configs may be deployed without translating handle to app_handle unless another layer does it.
Evidence: createConfigExtensionSpecification only auto-creates transformLocalToRemote if transformConfig is provided; otherwise it remains undefined.
Impact: deploy/update may fail validation or silently ignore fields if the API expects app_handle, causing confusing drift between local and remote state.
|
🤖 Code Review · #projects-dev-ai for questions ✅ Complete - 2 findings 📋 History✅ 2 findings → ✅ No issues → ✅ 2 findings |
ryancbahan
force-pushed
the
rcb/contract-branding
branch
from
805f23f to
722da12
Compare
Remove transformConfig, set deployConfig (configWithoutFirstClassFields pass-through) and transformRemoteToLocal directly (app_handle → handle). transformLocalToRemote is now undefined. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ryancbahan
force-pushed
the
rcb/contract-branding
branch
from
722da12 to
0053593
Compare
| transformRemoteToLocal: (content: object) => ({ | ||
| name: (content as {name?: string}).name, | ||
| handle: (content as {app_handle?: string}).app_handle, | ||
| }), |
There was a problem hiding this comment.
Reverse transform only reads app_handle; could drop handle if backend migrated
transformRemoteToLocal maps handle exclusively from content.app_handle. If the backend has migrated to return handle (matching the new deploy behavior), pulls/syncs will start producing handle: undefined and lose the value in local config.
Evidence:
handle: (content as {app_handle?: string}).app_handle,Impact:
- Local config may lose branding handle on pull/sync.
- Causes local/remote drift and repeated diffs.
| transformRemoteToLocal: (content: object) => ({ | ||
| name: (content as {name?: string}).name, | ||
| handle: (content as {app_handle?: string}).app_handle, | ||
| }), |
There was a problem hiding this comment.
Reverse transform returns explicit undefined keys, which may clobber existing config
The new transformRemoteToLocal always returns { name: ..., handle: ... } even when fields are missing, which can produce {name: undefined, handle: undefined}. Some merge strategies treat explicit undefined as an overwrite, potentially clearing existing local values.
Previously, reverse-transform logic only copied keys when present (e.g., if (content[key] !== undefined) result[key] = content[key]), avoiding clobbering.
Impact:
- Pulling/syncing from partial remote responses could wipe locally configured
name/handle. - Leads to configuration loss and hard-to-debug drift.
1 participant
Summary
transformConfigfrom branding spec, setdeployConfigandtransformRemoteToLocaldirectlyapp_handle→handle,name→nametransformLocalToRemoteis nowundefinedTest plan
transformLocalToRemoteisundefined🤖 Generated with Claude Code