fix: add strict TemplateLogic compile validation by Rishabh060105 · Pull Request #165 · accordproject/template-engine

Rishabh060105 · 2026-07-12T17:11:01Z

Summary

Fixes #163 by adding opt-in strict validation to TemplateArchiveProcessor.compileLogic().

Today compileLogic() returns emitted code without checking whether it contains a class extending TemplateLogic, so downstream sandbox consumers only discover malformed output later at execution time. This change adds a strict mode for callers that need that guarantee, while keeping the current default behavior unchanged for existing plain-class logic.

Changes

add CompileLogicOptions to TemplateArchiveProcessor.compileLogic()
keep backward compatibility with the existing boolean cache argument
support requireTemplateLogic: true to enforce that logic/logic.ts defines a class extending TemplateLogic
validate the source using the TypeScript AST before returning compiled output
keep default compilation behavior unchanged so existing plain default-exported classes still compile unless strict mode is enabled
export CompileLogicOptions from the package root
add regression coverage for:
- default compile behavior
- strict compile success when the logic extends TemplateLogic
- strict compile failure for plain default-exported classes

Test

Implemented and pushed on branch Rishabh060105/strict-template-logic-validation.

Validation notes:

npm test -- --runTestsByPath test/TemplateArchiveProcessor.test.ts --runInBand
- currently blocked by existing upstream build/type issues unrelated to this change
verified the new compileLogic() behavior directly with a focused runtime stub check covering:
- strict mode success for TemplateLogic subclasses
- default mode success for plain classes
- strict mode rejection for plain classes
- cache interaction so strict validation is not bypassed by a previously cached non-strict compile

Signed-off-by: Rishabh Jain <rishabhj2005@email.com>

23harshitkumar · 2026-07-13T20:33:08Z

@Rishabh060105 This is great work. Thanks for the contribution! I left some comments!

Signed-off-by: Rishabh Jain <rishabhj2005@email.com>

Rishabh060105 · 2026-07-14T06:46:31Z

Hi @23harshitkumar have addressed the issues raised above, please go through it. Thanks!

23harshitkumar · 2026-07-14T07:01:40Z

Thanks @Rishabh060105! You have my approval (as the "issue creator").

@sanketshevkar @mttrbrts Could we consider merging it?

mttrbrts · 2026-07-14T19:56:46Z

Thanks @Rishabh060105! You have my approval (as the "issue creator").

@sanketshevkar @mttrbrts Could we consider merging it?

I'm tempted to say that this is a bug, and that the opt-in flag is unnecessary. Let's discuss on the WG call tomorrow?

mttrbrts · 2026-07-15T16:07:53Z

Thanks @Rishabh060105! You have my approval (as the "issue creator").
@sanketshevkar @mttrbrts Could we consider merging it?

I'm tempted to say that this is a bug, and that the opt-in flag is unnecessary. Let's discuss on the WG call tomorrow?

If these templates will never work without this TemplateLogic extension, then this is a pure bug fix and we don't need the flag to opt-in.

Rishabh060105 · 2026-07-15T18:15:30Z

Thanks @Rishabh060105! You have my approval (as the "issue creator").
@sanketshevkar @mttrbrts Could we consider merging it?

I'm tempted to say that this is a bug, and that the opt-in flag is unnecessary. Let's discuss on the WG call tomorrow?

If these templates will never work without this TemplateLogic extension, then this is a pure bug fix and we don't need the flag to opt-in.

Hi @mttrbrts , I checked this against both template-engine and template-playground.

For the playground/sandbox flow, I agree with the concern: that path expects the compiled output to contain a class extending TemplateLogic, otherwise it cannot safely derive the class constructor.

For template-engine itself though, this is not currently enforced at runtime. JavaScriptEvaluator imports the default export, constructs it, and calls the requested method, without checking extends TemplateLogic. So a plain default-exported class with the expected methods can still work today.

That is why I kept this as an opt-in flag: it lets sandbox consumers enforce the stricter contract without changing existing engine behavior by default. If we agree with the fact that plain logic classes are unsupported by contract, I’m happy to simplify the PR and make the validation unconditional.

Signed-off-by: Rishabh Jain <rishabhj2005@email.com>

23harshitkumar · 2026-07-15T21:34:52Z

If these templates will never work without this TemplateLogic extension, then this is a pure bug fix and we don't need the flag to opt-in.

The TemplateLogic extension was enforced in the playground as per the US:02 acceptance criteria. This extension helps us in autocomplete and finding out (via regex) which class to instantiate for the sandbox. Therefore, we have an enforcement for this extension in the store.ts. So Yes! we do need this extension validation for the playground.

As for the engine, @Rishabh060105 is right that it currently allows plain classes without failing (could be an accident). To decide if it needs the validation, we'd have to ask ourselves: "do we really want to enforce the TemplateLogic in the engine?". If "yes", then this is a pure bug and we should definately drop the opt-in flag and make the validation unconditional!

mttrbrts · 2026-07-18T19:50:42Z

The evidence suggests that it's accidental that the engine works for classes that don't extend TemplateLogic class. Let's drop the flag. That greatly simplifies the code. Thanks @Rishabh060105!

Signed-off-by: Rishabh Jain <rishabhj2005@email.com>

Rishabh060105 · 2026-07-19T07:16:17Z

Hi @mttrbrts, @23harshitkumar I have pushed the changes:

Removed the requireTemplateLogic opt-in flag.
compileLogic() now always checks that the logic class extends TemplateLogic.
Kept the existing compile cache behavior.

mttrbrts · 2026-07-19T13:02:00Z

        const result = fun(...args);
        process.send({ result }, () => {
-            process.exit();
+            process.disconnect();


What is the impact of this change?

I initially changed this to process.disconnect() because I thought process.exit() was interrupting coverage cleanup.I later realised that process.exit() is viable here since each worker handles one evaluation this means termination must be deterministic after sending the result.

Copilot

Pull request overview

This PR aims to add stricter (opt-in) validation during TemplateArchiveProcessor.compileLogic() so callers can require that logic/logic.ts defines a class extending TemplateLogic, helping downstream sandbox consumers fail fast during compilation rather than at execution time.

Changes:

Adds a TypeScript AST-based check for a TemplateLogic subclass during logic compilation.
Refactors the TemplateArchiveProcessor test suite to load templates via a helper and adds new validation-related tests.
Adjusts the child-process worker shutdown behavior after sending results/errors.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File	Description
test/TemplateArchiveProcessor.test.ts	Adds helpers/mocks for loading templates and introduces new compile-time validation/caching tests.
src/worker.js	Changes worker termination after `process.send()` from `process.exit()` to `process.disconnect()` (with exit code set on error).
src/TemplateArchiveProcessor.ts	Adds a new AST-based `TemplateLogic` subclass assertion and calls it during TypeScript logic compilation.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Rishabh060105 · 2026-07-19T20:01:50Z

        if (logicManager.getLanguage() === 'typescript') {
            const compiledCode: Record<string, TwoSlashReturn> = {};
            const tsFiles: Array<Script> = logicManager.getScriptManager().getScriptsForTarget('typescript');
+            const logicScript = tsFiles.find((tsFile) => tsFile.getIdentifier() === 'logic/logic.ts');
+            await this.assertTemplateLogicSubclass(logicScript);


Validation has been kept unconditional as discussed above.The boolean argument now only controls compilation caching.

Rishabh060105 · 2026-07-19T20:04:23Z

+        expect(validationSpy).toHaveBeenCalledTimes(1);
+    });
+
+    test('should reject plain default-exported classes', async () => {


This test is based on the agreed behavior.compileLogic() should reject classes that don't extend TemplateLogic without the requirement of an opt-in flag.

Rishabh060105 · 2026-07-19T20:07:54Z

        process.send({ result }, () => {
-            process.exit();
+            process.disconnect();
        });


changed back process.disconnect() to process.exit() and added a test to confirm the same.

Rishabh060105 · 2026-07-19T20:14:44Z

        process.send({ message: err.toString() }, () => {
-            process.exit(1);
+            process.exitCode = 1;
+            process.disconnect();
        });


The error path now uses process.exit(1),and added a test confirming the original error being returned instead of timeout

Signed-off-by: Rishabh Jain <rishabhj2005@email.com>

Rishabh060105 · 2026-07-20T10:23:18Z

Hi @mttrbrts, I have addressed the latest review feedback:

Removed the opt-in flag, compileLogic() now always requires a class extending TemplateLogic.
The boolean argument now only controls compilation caching.
Changed back process.disconnect() to process.exit()
Added tests covering workers with open timers for both success and error paths.

fix: add strict TemplateLogic compile validation

905dd0d

Signed-off-by: Rishabh Jain <rishabhj2005@email.com>

Rishabh060105 requested a review from a team July 12, 2026 17:11

23harshitkumar reviewed Jul 13, 2026

View reviewed changes

Comment thread src/TemplateArchiveProcessor.ts Outdated

23harshitkumar reviewed Jul 13, 2026

View reviewed changes

Comment thread src/TemplateArchiveProcessor.ts Outdated

fix: lazy load strict validation

3853668

Signed-off-by: Rishabh Jain <rishabhj2005@email.com>

github-actions Bot added the maintainer-engaged A maintainer has commented or reviewed this item label Jul 15, 2026

mttrbrts closed this Jul 15, 2026

mttrbrts reopened this Jul 15, 2026

Stabilize worker process shutdown

0455ed4

Signed-off-by: Rishabh Jain <rishabhj2005@email.com>

fix: enforce TemplateLogic validation by default

ab1e662

Signed-off-by: Rishabh Jain <rishabhj2005@email.com>

Rishabh060105 force-pushed the Rishabh060105/strict-template-logic-validation branch from b6331fa to ab1e662 Compare July 19, 2026 07:11

mttrbrts requested a review from Copilot July 19, 2026 13:01

Copilot started reviewing on behalf of mttrbrts July 19, 2026 13:01 View session

mttrbrts reviewed Jul 19, 2026

View reviewed changes

Copilot AI reviewed Jul 19, 2026

View reviewed changes

fix: terminate evaluator workers deterministically

a7db557

Signed-off-by: Rishabh Jain <rishabhj2005@email.com>

Uh oh!

Uh oh!

Conversation

Rishabh060105 commented Jul 12, 2026

Summary

Changes

Test

Uh oh!

Uh oh!

Uh oh!

23harshitkumar commented Jul 13, 2026

Uh oh!

Rishabh060105 commented Jul 14, 2026

Uh oh!

23harshitkumar commented Jul 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mttrbrts commented Jul 14, 2026

Uh oh!

mttrbrts commented Jul 15, 2026

Uh oh!

Rishabh060105 commented Jul 15, 2026

Uh oh!

23harshitkumar commented Jul 15, 2026

Uh oh!

mttrbrts commented Jul 18, 2026

Uh oh!

Rishabh060105 commented Jul 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mttrbrts Jul 19, 2026

Choose a reason for hiding this comment

Uh oh!

Rishabh060105 Jul 19, 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!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Rishabh060105 Jul 19, 2026

Choose a reason for hiding this comment

Uh oh!

Rishabh060105 Jul 19, 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!

Rishabh060105 Jul 19, 2026

Choose a reason for hiding this comment

Uh oh!

Rishabh060105 Jul 19, 2026

Choose a reason for hiding this comment

Uh oh!

Rishabh060105 commented Jul 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

23harshitkumar commented Jul 14, 2026 •

edited

Loading

Rishabh060105 commented Jul 19, 2026 •

edited

Loading

Rishabh060105 Jul 19, 2026 •

edited

Loading

Rishabh060105 Jul 19, 2026 •

edited

Loading