Add system status handler by chrisqm-dev · Pull Request #507 · aws-cloudformation/cloudformation-languageserver

chrisqm-dev · 2026-03-31T14:56:56Z

Description of changes:

Currently, the only way to tell if the schemas, lint, guard, and settings are successfully loaded is by checking the logs (for schemas, the logs only appear if the schemas needed to be downloaded) or by directly using an lsp request that uses the schemas
Added an LSP request to return numerous system components settings including:
- regions where schemas are loaded
- if lint is initialized
- if guard is initialized
- what settings are applied (since settings are propagated async)
- This allows for more granular testing by decoupling the async initialization logic from the lsp request that is being tested

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

tst/unit/handlers/SystemHandler.test.ts

src/handlers/SystemHandler.ts

tst/unit/handlers/SystemHandler.test.ts

src/handlers/SystemHandler.ts

tst/unit/handlers/SystemHandler.test.ts

satyakigh · 2026-04-01T16:32:00Z

src/system/SystemTypes.ts

+
+export type ReadinessStatus = {
+    ready: boolean;
+    reason?: string;


do we need reason?

Removed reason field

satyakigh · 2026-04-01T16:33:33Z

src/system/SystemTypes.ts

+    currentSettings: Settings;
+};
+
+export const GetSystemStatusRequestType = new RequestType<void, GetSystemStatusResponse, void>('aws/cfn/system/status');


aws/system/status

satyakigh · 2026-04-01T16:34:04Z

src/settings/SettingsManager.ts

        return this.settingsState.toSettings();
    }

+    /**


Is this comment required?

satyakigh · 2026-04-01T16:35:01Z

src/settings/SettingsManager.ts

    @Telemetry() private readonly telemetry!: ScopedTelemetry;
    private readonly settingsState = new SettingsState();
    private readonly subscriptionManager = new SubscriptionManager<Settings>();
+    private isUpdatingSettings = false;


I think we need to know "has settings initialized or updated atleast once" - any workspace change or settings change will make this true, but that doesn't exactly say if the system is ready

Modified isUpdatingSettings to instead be a settingsReady, it is initially set to false. If the syncConfiguration is completed successfully as part of the initializedHandler, the settings will be marked as ready. Upon making a settings update, settings readiness is set to false until the update successfully completes.

satyakigh · 2026-04-01T16:35:42Z

src/settings/SettingsManager.ts

-            this.settingsState.update(newSettings);
-            logger.info(`Settings updated: ${toString(difference)}`);
-            this.subscriptionManager.notify(newSettings, oldSettings);
+            this.isUpdatingSettings = true;


Even if nothing has changed if settings have been synced atleast once, then settings are ready

Modified isUpdatingSettings to instead be a settingsReady, it is initially set to false. If the syncConfiguration is completed successfully as part of the initializedHandler, the settings will be marked as ready. Upon making a settings update, settings readiness is set to false until the update successfully completes.

satyakigh · 2026-04-01T16:36:01Z

src/services/guard/GuardService.ts

+    /**
+     * Get the readiness status of the Guard service
+     */
+    public getReadinessStatus(): { ready: boolean; reason?: string } {


Use the types defined

satyakigh · 2026-04-01T16:37:07Z

For settings and lint initializations, can the initialize handler be used?

chrisqm-dev · 2026-04-01T21:50:07Z

For settings and lint initializations, can the initialize handler be used?

Yes, made updates so that:

inside initializeHandler we call syncConfiguration(), which will update the settings readiness status
Additionally, the InitializeHandler calls cfnLintService.initialize(), which will update the lint readiness status

tst/unit/handlers/SystemHandler.test.ts

Zee2413 · 2026-04-03T16:26:56Z

src/system/SystemTypes.ts

+import { RequestType } from 'vscode-languageserver-protocol';
+import { Settings } from '../settings/Settings';
+
+export type ReadinessStatus = {


We're creating a new folder src/system just for these two types. Can we move this to src/protocol or inside src/protocol/LspSystemHandlers.ts. We already have tons of directories inside src. Would be good to avoid adding another one with 1 file, 2 types.

Deleted folder and moved types to LspSystemHandler.ts

Zee2413 · 2026-04-03T16:34:48Z

src/protocol/LspSystemHandlers.ts

+export class LspSystemHandlers {
+    constructor(private readonly connection: Connection) {}
+
+    onGetSystemStatus(handler: ServerRequestHandler<void, GetSystemStatusResponse, never, void>) {


Why not just RequestHandler? We're not using progress reporter

Changed method to use RequestHandler

Zee2413 · 2026-04-03T16:37:48Z

src/handlers/SystemHandler.ts

+                const reason = settingsStatus.reason ?? 'Server initializing';
+                return {
+                    settingsReady: settingsStatus,
+                    schemasReady: { ready: false, reason, availableRegions: [] },


Adding extra key to ReadinessStatus type just for schemas. Is this needed?

Removed extra key, availableRegions is no longer supplied

Zee2413 · 2026-04-03T16:46:22Z

src/handlers/SystemHandler.ts

+                schemasReady: {
+                    availableRegions: [...availableRegions],
+                    ready: availableRegions.length > 0,
+                    ...(availableRegions.length === 0 ? { reason: 'No schemas loaded' } : {}),


I am not finding reason to be a helpful field. It might be simpler to keep the status response as just a ready or not flag.

Removed reason from status response

Zee2413 · 2026-04-03T17:07:27Z

Design Feedback: Interface-based Readiness

The current implementation has each component (CfnLintService, GuardService, SettingsManager) independently adding getReadinessStatus() with no shared contract, and schemas have no readiness method at all — the handler derives it inline. This creates three different patterns for the same concept:

CfnLintService: externally set via setReadinessStatus() from Initialize.ts
GuardService: internally derived from isLoadingRules + enabledRules.length
SettingsManager: internally managed via readinessStatus field
Schemas: no readiness method — handler checks getPublicSchemaRegions().length > 0 inline

The handler also contains aggregation logic (settings not ready → short-circuit all others as not ready), which is business logic that doesn't belong in a request handler.

Proposed: `ReadinessContributor` interface

Following the same pattern as Closeable and Configurable in this codebase, define a shared interface:

// src/utils/ReadinessContributor.ts

export type ReadinessStatus = {
    readonly ready: boolean;
};

export interface ReadinessContributor {
    isReady(): ReadinessStatus;
}

Each component implements it:

// CfnLintService
export class CfnLintService implements SettingsConfigurable, Closeable, ReadinessContributor {
    isReady(): ReadinessStatus {
        if (!this.settings.enabled) return { ready: true };
        return { ready: this.status === STATUS.Ready };
    }
}

// GuardService
export class GuardService implements SettingsConfigurable, Closeable, ReadinessContributor {
    isReady(): ReadinessStatus {
        if (!this.settings.enabled) return { ready: true };
        return { ready: !this.isLoadingRules && this.enabledRules.length > 0 };
    }
}

// SettingsManager
export class SettingsManager implements ISettingsSubscriber, ReadinessContributor {
    private initialized = false;

    isReady(): ReadinessStatus {
        return { ready: this.initialized };
    }
    // set this.initialized = true after first successful syncConfiguration()
}

For schemas, wrap SchemaStore since we shouldn't modify it for readiness concerns:

// src/schema/SchemaReadiness.ts
export class SchemaReadiness implements ReadinessContributor {
    constructor(private readonly schemaStore: SchemaStore) {}

    isReady(): ReadinessStatus {
        return { ready: this.schemaStore.getPublicSchemaRegions().length > 0 };
    }
}

Aggregator as single source of truth

// src/system/SystemReadiness.ts
export class SystemReadiness {
    constructor(
        private readonly settings: ReadinessContributor,
        private readonly schemas: ReadinessContributor,
        private readonly cfnLint: ReadinessContributor,
        private readonly cfnGuard: ReadinessContributor,
    ) {}

    getStatus(): GetSystemStatusResponse {
        return {
            settingsReady: this.settings.isReady(),
            schemasReady: this.schemas.isReady(),
            cfnLintReady: this.cfnLint.isReady(),
            cfnGuardReady: this.cfnGuard.isReady(),
        };
    }
}

Handler becomes trivial

export function getSystemStatusHandler(
    systemReadiness: SystemReadiness,
): RequestHandler<void, GetSystemStatusResponse, void> {
    return (): GetSystemStatusResponse => systemReadiness.getStatus();
}

Why this is better

Uniform contract — no ad-hoc setReadinessStatus() on some components but not others
Each component owns its own readiness — no external setting from Initialize.ts, no inline derivation in the handler
Handler has zero logic — just delegates to the aggregator
Extensible — adding a new component = implement interface + register in SystemReadiness
Follows existing codebase patterns — Closeable, Configurable are the same idea

Also agreeing with the other review comments:

src/system/ directory for 2 types is unnecessary
ServerRequestHandler → RequestHandler since progress reporting isn't used
availableRegions extending ReadinessStatus inline for schemas is messy — if region info is needed, it belongs on a separate endpoint or as a field on the response, not mixed into the readiness type

tst/unit/handlers/SystemHandler.test.ts

+
+            const handler = getSystemStatusHandler(mockComponents);
+
+            const result = handler(undefined, CancellationToken.None);


tst/unit/handlers/SystemHandler.test.ts

+
+            const handler = getSystemStatusHandler(mockComponents);
+
+            const result = handler(undefined, CancellationToken.None);


chrisqm-dev · 2026-04-03T20:06:53Z

Design Feedback: Interface-based Readiness

The current implementation has each component (CfnLintService, GuardService, SettingsManager) independently adding getReadinessStatus() with no shared contract, and schemas have no readiness method at all — the handler derives it inline. This creates three different patterns for the same concept:

* **CfnLintService**: externally set via `setReadinessStatus()` from `Initialize.ts`

* **GuardService**: internally derived from `isLoadingRules` + `enabledRules.length`

* **SettingsManager**: internally managed via `readinessStatus` field

* **Schemas**: no readiness method — handler checks `getPublicSchemaRegions().length > 0` inline

The handler also contains aggregation logic (settings not ready → short-circuit all others as not ready), which is business logic that doesn't belong in a request handler.

Proposed: `ReadinessContributor` interface

Following the same pattern as Closeable and Configurable in this codebase, define a shared interface:

// src/utils/ReadinessContributor.ts

export type ReadinessStatus = {
    readonly ready: boolean;
};

export interface ReadinessContributor {
    isReady(): ReadinessStatus;
}

Each component implements it:

// CfnLintService
export class CfnLintService implements SettingsConfigurable, Closeable, ReadinessContributor {
    isReady(): ReadinessStatus {
        if (!this.settings.enabled) return { ready: true };
        return { ready: this.status === STATUS.Ready };
    }
}

// GuardService
export class GuardService implements SettingsConfigurable, Closeable, ReadinessContributor {
    isReady(): ReadinessStatus {
        if (!this.settings.enabled) return { ready: true };
        return { ready: !this.isLoadingRules && this.enabledRules.length > 0 };
    }
}

// SettingsManager
export class SettingsManager implements ISettingsSubscriber, ReadinessContributor {
    private initialized = false;

    isReady(): ReadinessStatus {
        return { ready: this.initialized };
    }
    // set this.initialized = true after first successful syncConfiguration()
}

For schemas, wrap SchemaStore since we shouldn't modify it for readiness concerns:

// src/schema/SchemaReadiness.ts
export class SchemaReadiness implements ReadinessContributor {
    constructor(private readonly schemaStore: SchemaStore) {}

    isReady(): ReadinessStatus {
        return { ready: this.schemaStore.getPublicSchemaRegions().length > 0 };
    }
}

Aggregator as single source of truth

// src/system/SystemReadiness.ts
export class SystemReadiness {
    constructor(
        private readonly settings: ReadinessContributor,
        private readonly schemas: ReadinessContributor,
        private readonly cfnLint: ReadinessContributor,
        private readonly cfnGuard: ReadinessContributor,
    ) {}

    getStatus(): GetSystemStatusResponse {
        return {
            settingsReady: this.settings.isReady(),
            schemasReady: this.schemas.isReady(),
            cfnLintReady: this.cfnLint.isReady(),
            cfnGuardReady: this.cfnGuard.isReady(),
        };
    }
}

Handler becomes trivial

export function getSystemStatusHandler(
    systemReadiness: SystemReadiness,
): RequestHandler<void, GetSystemStatusResponse, void> {
    return (): GetSystemStatusResponse => systemReadiness.getStatus();
}

Why this is better

1. **Uniform contract** — no ad-hoc `setReadinessStatus()` on some components but not others

2. **Each component owns its own readiness** — no external setting from `Initialize.ts`, no inline derivation in the handler

3. **Handler has zero logic** — just delegates to the aggregator

4. **Extensible** — adding a new component = implement interface + register in `SystemReadiness`

5. **Follows existing codebase patterns** — `Closeable`, `Configurable` are the same idea

Also agreeing with the other review comments:

* `src/system/` directory for 2 types is unnecessary

* `ServerRequestHandler` → `RequestHandler` since progress reporting isn't used

* `availableRegions` extending `ReadinessStatus` inline for schemas is messy — if region info is needed, it belongs on a separate endpoint or as a field on the response, not mixed into the readiness type

Added the ReadinessContributor interface and modified the services to implement it. I didn't add SystemReadiness because the handler was concise enough

chrisqm-dev requested a review from a team as a code owner March 31, 2026 14:56

chrisqm-dev mentioned this pull request Mar 31, 2026

Add schema readiness handler #506

Closed

github-code-quality bot found potential problems Mar 31, 2026

View reviewed changes

tst/unit/handlers/SystemHandler.test.ts Fixed Show fixed Hide fixed

tst/unit/handlers/SystemHandler.test.ts Fixed Show fixed Hide fixed

chrisqm-dev commented Mar 31, 2026

View reviewed changes

src/handlers/SystemHandler.ts Outdated Show resolved Hide resolved

github-code-quality bot found potential problems Mar 31, 2026

View reviewed changes

tst/unit/handlers/SystemHandler.test.ts Fixed Show fixed Hide fixed

tst/unit/handlers/SystemHandler.test.ts Fixed Show fixed Hide fixed

tst/unit/handlers/SystemHandler.test.ts Fixed Show fixed Hide fixed

chrisqm-dev commented Mar 31, 2026

View reviewed changes

src/handlers/SystemHandler.ts Outdated Show resolved Hide resolved

github-code-quality bot found potential problems Mar 31, 2026

View reviewed changes

tst/unit/handlers/SystemHandler.test.ts Fixed Show fixed Hide fixed

tst/unit/handlers/SystemHandler.test.ts Fixed Show fixed Hide fixed

chrisqm-dev mentioned this pull request Mar 31, 2026

Add LSP Client and long running tests in tools #511

Open

satyakigh reviewed Apr 1, 2026

View reviewed changes

github-code-quality bot found potential problems Apr 1, 2026

View reviewed changes

tst/unit/handlers/SystemHandler.test.ts Fixed Show fixed Hide fixed

tst/unit/handlers/SystemHandler.test.ts Fixed Show fixed Hide fixed

chrisqm-dev force-pushed the system-readiness branch 2 times, most recently from d77c62f to 39e76a4 Compare April 1, 2026 21:57

Zee2413 reviewed Apr 3, 2026

View reviewed changes

chrisqm-dev added 9 commits April 3, 2026 13:56

Add system status handler

5da2ec8

Add unit tests, fix settings retrieval

4333687

Remove unused file

a5151df

Remove totalRegions attribute and cleaned up types

7ee341b

Working long running tests

5137d8a

Renaming files

a1af3b1

Linting

f5ec2d1

Handle settings and lint readiness in initialization handler

aa0408a

Add readiness contributor interface and removed available regions field

ac8cc03

chrisqm-dev force-pushed the system-readiness branch from ba53669 to ac8cc03 Compare April 3, 2026 20:04

github-code-quality bot found potential problems Apr 3, 2026

View reviewed changes

Zee2413 approved these changes Apr 3, 2026

View reviewed changes


		const handler = getSystemStatusHandler(mockComponents);

		const result = handler(undefined, CancellationToken.None);

Conversation

chrisqm-dev commented Mar 31, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisqm-dev Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisqm-dev Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisqm-dev Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

satyakigh commented Apr 1, 2026

Uh oh!

chrisqm-dev commented Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Zee2413 commented Apr 3, 2026

Design Feedback: Interface-based Readiness

Proposed: ReadinessContributor interface

Aggregator as single source of truth

Handler becomes trivial

Why this is better

Uh oh!

chrisqm-dev commented Apr 3, 2026

Design Feedback: Interface-based Readiness

Proposed: ReadinessContributor interface

Aggregator as single source of truth

Handler becomes trivial

Why this is better

Uh oh!

Reviewers

chrisqm-dev Apr 1, 2026 •

edited

Loading

chrisqm-dev Apr 1, 2026 •

edited

Loading

chrisqm-dev Apr 1, 2026 •

edited

Loading

chrisqm-dev commented Apr 1, 2026 •

edited

Loading

Proposed: `ReadinessContributor` interface

Proposed: `ReadinessContributor` interface