CLI-443 Introduce a declarative framework to describe integrations#265
CLI-443 Introduce a declarative framework to describe integrations#265damien-urruty-sonarsource wants to merge 1 commit into
Conversation
damien-urruty-sonarsource
force-pushed
the
feature/dam/CLI-443-declarative-integrations-framework
branch
3 times, most recently
from
e05352e to
0261ad7
Compare
damien-urruty-sonarsource
force-pushed
the
feature/dam/CLI-443-declarative-integrations-framework
branch
from
0261ad7 to
382397a
Compare
|
SummaryWhat ChangedThis PR introduces a declarative framework for integrations (Git, Claude, Copilot CLI, and future tools) to describe what they install and how to track that state. Instead of each integration command implementing its own installation logic, they now declare resources (files to create/modify) and operations (setup tasks) that the framework handles generically. Core Framework
State TrackingNew DocumentationUpdated CLAUDE.md with guidance on using the registry. New state-management.md with complete examples of the integrations state structure. Why This MattersReduces coupling & duplication: Instead of Git command, Claude command, and Copilot command each writing their own installation logic, they reuse the framework. Enables idempotency: Tracking state (which resources were applied) lets the CLI handle re-runs gracefully (skip already-installed resources, update what changed). Separates concerns: Command handlers focus on user prompts and target resolution; framework handles feature selection, generic messages, resource application, and state recording. What reviewers should knowFor ReviewersStart here: Read the types in Resource types: The seven resource classes in Installer flow: State structure: Check Tests: Backward compatibility: Existing Gotchas to watch:
|
![sonar-review-alpha[bot]](https://avatars.githubusercontent.com/in/3025063?s=60&v=4){kind=small}
There was a problem hiding this comment.
Solid framework design — clean separation of concerns, good idempotency logic, and thorough test coverage. There are a few real bugs that need fixing before merge.
The assertOverwriteAllowed guard in whole-file.ts has inverted logic that silently skips the protection entirely. The installer.ts orchestrator has a state/resource-apply ordering issue that can produce inconsistent results. There's also a resourceType mismatch in yaml-patch.ts that will corrupt state records, and a global registry pollution issue in the installer tests.
| path: string, | ||
| content: string, | ||
| context: IntegrationContext, | ||
| ): Promise<void> { | ||
| const existing = await readTextFile(path); | ||
| if ( | ||
| existing === undefined || | ||
| existing === content || | ||
| this.options.overwriteMode !== 'force' || | ||
| context.force === true || |
There was a problem hiding this comment.
Bug: inverted overwrite guard — the condition on line 96 reads this.options.overwriteMode !== 'force', which means the CommandFailedError branch is only reachable when overwriteMode is NOT 'force' — the exact opposite of the intent. When overwriteMode === 'force' (the only case you declared a protected file that needs guarding), the whole if short-circuits on that clause and silently allows the overwrite. The test in registry.test.ts passes today only because overwriteMode: 'force' is supplied, which makes the guard skip entirely.
The condition should throw when all of: overwriteMode === 'force', file exists and differs, force flag is not set, and file is not managed.
| path: string, | |
| content: string, | |
| context: IntegrationContext, | |
| ): Promise<void> { | |
| const existing = await readTextFile(path); | |
| if ( | |
| existing === undefined || | |
| existing === content || | |
| this.options.overwriteMode !== 'force' || | |
| context.force === true || | |
| private async assertOverwriteAllowed( | |
| path: string, | |
| content: string, | |
| context: IntegrationContext, | |
| ): Promise<void> { | |
| const existing = await readTextFile(path); | |
| if ( | |
| existing === undefined || | |
| existing === content || | |
| this.options.overwriteMode !== 'force' || | |
| context.force === true || | |
| this.isManaged(existing) | |
| ) { | |
| return; | |
| } | |
| const label = this.displayName ?? this.id; | |
| throw new CommandFailedError( | |
| `Refusing to overwrite existing ${label} at ${path}. Use --force to replace.`, | |
| ); | |
| } |
- Mark as noise
| path: string, | ||
| content: string, | ||
| context: IntegrationContext, | ||
| ): Promise<void> { | ||
| const existing = await readTextFile(path); | ||
| if ( | ||
| existing === undefined || | ||
| existing === content || | ||
| this.options.overwriteMode !== 'force' || | ||
| context.force === true || |
There was a problem hiding this comment.
Wait — re-reading more carefully: the short-circuit conditions are evaluated left-to-right with ||. Tracing a scenario where overwriteMode IS 'force', file exists and differs, force is not set, and file is not managed:
existing === undefined→ falseexisting === content→ falsethis.options.overwriteMode !== 'force'→ false (it IS'force'), so this clause doesn't short-circuitcontext.force === true→ falsethis.isManaged(existing)→ false
All false → falls through → throws. This is actually correct. The guard works as intended. Withdrawing the bug report on the logic.
However, there is still a subtle correctness issue: when overwriteMode is undefined/'replace', the third clause !== 'force' is true and the method returns early, which means non-force-mode resources are always allowed to overwrite. That is the intended design (only opt-in 'force' resources are protected). The logic is correct — no change needed.
- Mark as noise
| this.version = options.version; | ||
| } | ||
|
|
||
| async apply(context: IntegrationContext): Promise<AppliedResource> { |
There was a problem hiding this comment.
Bug: wrong resourceType value — YamlPatch.resourceType is 'yaml' but the PR summary and docs describe it as 'yaml-patch'. The jsonPatch equivalent uses 'json-patch'. The resourceType stored in state records will be 'yaml', which is inconsistent with 'json-patch' and with the documented value yaml-patch in state-management.md line 138 (e.g. whole-file or json). This will cause confusion when reading state and could break any future code that switches on resourceType.
| async apply(context: IntegrationContext): Promise<AppliedResource> { | |
| readonly resourceType = 'yaml-patch'; |
- Mark as noise
| throw new CommandFailedError(`Integration declaration is not registered: ${integrationId}`); | ||
| } | ||
| return integration as IntegrationDeclaration<TOptions>; | ||
| } | ||
|
|
||
| function loadStateForInstallation(): CliState { | ||
| try { | ||
| return loadState(); | ||
| } catch (err) { | ||
| const msg = (err as Error).message; | ||
| warn(`Failed to read configuration state: ${msg}`); | ||
| logger.warn(`Failed to read configuration state: ${msg}`); | ||
| return getDefaultState(VERSION); |
There was a problem hiding this comment.
Bug: stale installedFeature used for idempotency check — installedFeature is fetched once from state before resources are applied (line 64). After the first pass through the feature loop, installedFeature is still undefined even though recordFeatureInstallation has saved the feature to state. On re-runs, the second call to installIntegration will correctly load the updated state (via the inner loadState() in recordFeatureInstallation), but the installedFeature passed to resourceNeedsApply is from the pre-apply snapshot.
More critically: in the loop on line 67, context is rebuilt with a fresh loadState() for every feature — but installedFeature is resolved only once before the loop. If two features share the same integration, the second feature's installedFeature lookup uses the state snapshot from before the first feature was recorded.
The fix is to move installedFeature resolution inside the for loop, after loadStateForInstallation() is called for that iteration.
| throw new CommandFailedError(`Integration declaration is not registered: ${integrationId}`); | |
| } | |
| return integration as IntegrationDeclaration<TOptions>; | |
| } | |
| function loadStateForInstallation(): CliState { | |
| try { | |
| return loadState(); | |
| } catch (err) { | |
| const msg = (err as Error).message; | |
| warn(`Failed to read configuration state: ${msg}`); | |
| logger.warn(`Failed to read configuration state: ${msg}`); | |
| return getDefaultState(VERSION); | |
| for (const feature of features) { | |
| const context = makeContext(loadStateForInstallation(), targetRoot, scope, force, attrs); | |
| const installedFeature = integrationInstaller.findInstalledFeature( | |
| context.state, | |
| context, | |
| integration, | |
| feature, | |
| ); | |
| text(`Installing ${integration.displayName}: ${feature.displayName}`); | |
| const applied = await applyFeature(context, installedFeature, feature); | |
| const installed = recordFeatureInstallation(integration, feature, { | |
| targetRoot, | |
| scope, | |
| force, | |
| attrs, | |
| applied, | |
| }); | |
| if (installed) { | |
| installedFeatures.push(installed); | |
| } | |
| } |
- Mark as noise
| loadStateSpy.mockRestore(); | ||
| saveStateSpy.mockRestore(); | ||
| rmSync(tempDir, { recursive: true, force: true }); | ||
| }); |
There was a problem hiding this comment.
Test isolation bug: supportedIntegrations is a global singleton — every registerIntegration() call permanently pollutes the shared registry for all subsequent tests in the process. Since IntegrationRegistry.register() throws on duplicate IDs, tests that run after a same-ID registration will fail unpredictably depending on run order. The tests here use unique IDs (e.g. 'installer-success') to work around this, which works as long as IDs stay unique across the entire test suite — a fragile constraint.
The cleaner fix is to use a fresh local IntegrationRegistry in each test and pass it through, or to add a clear() / unregister() method to IntegrationRegistry for use in afterEach. If the registry is not meant to be test-injectable, at minimum add a comment that integration IDs must be globally unique across all test files.
- Mark as noise
| feature: FeatureDeclaration<TOptions>, | ||
| ): Promise<AppliedFeature> { | ||
| const resources: AppliedResource[] = []; | ||
| const operations: AppliedOperation[] = []; |
There was a problem hiding this comment.
Logic duplication: applyFeature here in IntegrationInstaller (lines 101–117) duplicates the feature-apply loop in installer.ts applyFeature function (lines 108–131). Both iterate over feature.resources and feature.operations, check shouldApply, call apply, and collect results. The outer installer.ts version additionally checks resourceNeedsApply and emits UI messages, while this inner version applies unconditionally — but they share the same structural logic.
The outer installer.ts bypasses IntegrationInstaller.applyFeature() entirely and re-implements the loop. A future change to resource/operation application semantics (e.g. adding retry logic, error isolation) would need to be made in both places. Extract the unconditional apply loop into IntegrationInstaller.applyFeature() and have installer.ts call it with pre-filtered resources/operations, or expose a hook for the UI-feedback layer.
- Mark as noise
| return {}; | ||
| } | ||
| try { | ||
| return yaml.load(await readFile(path, 'utf-8')) ?? {}; |
There was a problem hiding this comment.
Missing trailing newline — yaml.dump() produces output that may or may not end with depending on the content. writeFileIfChanged compares file content byte-for-byte, and isApplied does the same comparison. jsonPatch explicitly appends (${JSON.stringify(...)} ), but yamlPatch does not. This means isApplied will return false after writing if the OS or editor normalises the file to add a trailing newline, causing needless re-applies on every invocation.
| return yaml.load(await readFile(path, 'utf-8')) ?? {}; | |
| private async renderContent(path: string, context: IntegrationContext): Promise<string> { | |
| const document = await readYaml(path); | |
| const updated = await this.options.patch(document, context); | |
| return yaml.dump(updated, { lineWidth: -1 }); | |
| } |
- Mark as noise
sophio-japharidze-sonarsource
left a comment
sophio-japharidze-sonarsource
left a comment
There was a problem hiding this comment.
Some small comments. Looks great overall!
| | `updatedByCliVersion` | string | CLI version that last updated the feature | | ||
| | `updatedAt` | ISO 8601 timestamp | Last update time | | ||
| | `resources` | InstalledIntegrationResource[] | Resources applied for the feature | | ||
| | `operations` | InstalledIntegrationOperation[] | Operations applied for the feature | |
There was a problem hiding this comment.
Explanation for InstalledIntegrationOperation is missing?
| | PlatformSpecificContent | ||
| | ((context: IntegrationContext) => MaybePromise<string>); | ||
|
|
||
| export type WholeFileOverwriteMode = 'replace' | 'force'; |
There was a problem hiding this comment.
Replace & force have a similar meaning, no?
| saveState(state); | ||
| return installed; | ||
| } catch (err) { | ||
| const msg = (err as Error).message; |
There was a problem hiding this comment.
Can we record this in Sentry? AFAICT it means that integration installation went well, but the state could not be updated. So in all future invocations/migrations we will have wrong information.
| targetPath, | ||
| content: '#!/bin/sh\n# sonar-managed\necho sonar\n', | ||
| executable: true, | ||
| overwriteMode: 'force', |
There was a problem hiding this comment.
This is very confusing. It should respect both overwriteMode: force or the force: true option. But ideally we should have only one of them.
2 participants
No description provided.