feat: add escapeTemplatePlaceholders option#21
Merged
Conversation
…conflicts
Escape `{{` patterns in message content to prevent SAP orchestration API
from misinterpreting them as template placeholders. Fixes issues with AI
coding agents (OpenCode, Cursor, Cline) that use `{{?variable}}` syntax.
- Add escapeOrchestrationPlaceholders/unescapeOrchestrationPlaceholders utils
- Add escapeTemplatePlaceholders option to SAPAISettings and providerOptions
- Export utility functions from index.ts
- Add troubleshooting documentation
Refs: SAP/ai-sdk-js#1453, SAP/ai-sdk-js#1479
jerome-benoit
force-pushed
the
feat/escape-template-placeholders
branch
from
0db85cc to
3759bea
Compare
Extends the escapeTemplatePlaceholders option to escape all three Jinja2
opening delimiters, not just {{ patterns:
- {{ - Variable expressions (e.g., {{variable}}, {{?optional}})
- {% - Block statements (e.g., {% for %}, {% if %})
- {# - Comments (e.g., {# comment #})
This provides complete protection against SAP AI Core orchestration API
errors when AI coding agents return content containing any Jinja2 syntax.
Refs SAP/ai-sdk-js#1453, SAP/ai-sdk-js#1479
jerome-benoit
force-pushed
the
feat/escape-template-placeholders
branch
from
0c0e02e to
4220853
Compare
BREAKING CHANGE: escapeTemplatePlaceholders now defaults to true for safer
handling of template syntax in AI agent tool results.
Additional changes:
- Use 'template delimiters' terminology in public APIs instead of 'Jinja2'
- Cache regex pattern in unescapeOrchestrationPlaceholders for performance
- Update export comment in src/index.ts to mention all three delimiters
- Add comprehensive tests for {% and {# escaping
- Update API_REFERENCE.md and TROUBLESHOOTING.md documentation
Refs: SAP/ai-sdk-js#1453, SAP/ai-sdk-js#1479
…workflow - Make documentation generic (remove specific error examples) - Make test data generic (variable1/variable2 instead of specific names) - Remove publish-check job from PR checks (was causing CI failures) - Add publish simulation step to release workflow before actual publish
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds an escapeTemplatePlaceholders option to prevent conflicts with SAP AI Core's orchestration template syntax ({{, {%, {#). This is essential for AI coding agents (OpenCode, Cursor, Cline) that use similar placeholder syntax in their tool definitions and content.
Changes:
- Adds
escapeTemplatePlaceholdersoption (default:true) at provider, model, and per-call levels - Implements
escapeOrchestrationPlaceholders()andunescapeOrchestrationPlaceholders()utility functions using zero-width space (U+200B) insertion - Applies escaping to all message content types: system, user, assistant, tool calls/results, and reasoning parts
- Adds comprehensive test coverage for edge cases including overlapping delimiters, JSON preservation, and Jinja2 syntax
- Documents troubleshooting guidance and API reference for the new option
- Refactors CI workflow to move npm publish dry-run from PR checks to publish workflow
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/sap-ai-settings.ts |
Adds escapeTemplatePlaceholders property to SAPAISettings interface with JSDoc |
src/sap-ai-provider-options.ts |
Adds escapeTemplatePlaceholders to provider options schema with Zod validation |
src/sap-ai-language-model.ts |
Integrates option with proper fallback chain: call option → model setting → default true |
src/convert-to-sap-messages.ts |
Implements escaping logic with regex patterns and loop for overlapping delimiters; exports utility functions |
src/index.ts |
Exports utility functions as public API |
src/convert-to-sap-messages.test.ts |
Adds 330+ lines of comprehensive tests covering all edge cases and integration scenarios |
TROUBLESHOOTING.md |
Adds detailed troubleshooting section with symptoms, causes, solutions, and examples |
API_REFERENCE.md |
Documents the new option in SAPAISettings table with explanation note |
.github/workflows/npm-publish-npm-packages.yml |
Adds npm publish dry-run step before actual publish |
.github/workflows/check-pr.yaml |
Removes standalone publish-check job to reduce PR CI time |
Comment on lines
+1083
to
+1084
| > **Note:** The `escapeTemplatePlaceholders` option is enabled by default to prevent SAP AI Core orchestration API errors when content contains template syntax (`{{variable}}`, `{% if %}`, `{# comment #}`). Set to `false` only if you intentionally use SAP orchestration templating features. See [Troubleshooting - Problem: Template Placeholder Conflicts](./TROUBLESHOOTING.md#problem-template-placeholder-conflicts) for details. | ||
|
|
There was a problem hiding this comment.
While the escapeTemplatePlaceholders property is documented in this table, the new exported utility functions escapeOrchestrationPlaceholders and unescapeOrchestrationPlaceholders (exported from src/index.ts) should also be documented in the API Reference. Consider adding them to the Utility Functions section with their signatures, parameters, return values, and usage examples to maintain consistency with the existing documentation pattern.
3 tasks
jerome-benoit
added a commit
that referenced
this pull request
May 5, 2026
- types.ts: parseFindingsSafe with partial recovery (finding #18) - concurrency-pool.ts: guard NaN/float/Infinity in constructor (#1,#4), JSDoc re-entrant warning (#2) - task-source.ts: NFKC normalize + extended blocklist in sanitizeForPrompt (#17,#20), Map lookup in validatePlan (#7) - refinement-loop.ts: readFileSync replaces execSync/sed (shell injection #5), radius used (#12), fallback hash hardened (#13), line clamping (#14), realpathSync symlinks (#15), file cache Map (#11), parseFindingsSafe (#18), nonce validation (#16), trivial match skip (#19), implementer try/catch (#7), ratchet guard round<=2 (#6), no duplicate nonLowFindings (#9) - finalizer.ts: pushBranch returns boolean (#22), crypto rescue suffix (#21), rollback error level (#8) - main.ts: TASK_TIMEOUT_MS + Promise.race timeout (#3), flat budget JSDoc (#10)
jerome-benoit
added a commit
that referenced
this pull request
May 5, 2026
* feat: implement sandcastle refinement loop with critic-based convergence Replace single-pass implement→review→merge with iterative implement↔critic loop per task. Key changes: - Orchestrator fetches and sanitizes issues (prevents prompt injection) - Implement↔Critic loop with deterministic dedup convergence - Critic produces structured findings (nonce-tagged JSON, zod-validated) - Decreasing iteration budget per round [100, 50, 25, 10, 10] - Host-side validation and rebase (no agent needed) - One PR per task (no merger agent) - Draft PR on non-convergence with outstanding findings listed Implements #110 * fix: address review findings (shell injection, false convergence, nesting, zod validation) - Replace execSync with execFileSync for gh pr create (prevents shell injection) - Guard parseFindings against empty matches (prevents false convergence) - Add try/catch on gh issue list startup call - Guard git push in rebase catch block - Extract finalizeIssue function (reduces nesting from 6+ to 3 levels) - Add zod schema for rawIssues (replaces unsafe 'as' cast) - Implement validation retry round per spec (one more implement→critic if budget remains) * fix: guard retry calls, split rebase logic, remove dead critic retry * fix: handle nullable issue body and guard JSON parse * fix: distinguish stalled from converged (re-reported findings → draft PR) * fix: LOW-only findings should not prevent convergence * fix: log validation errors, conditional checklist, derive PR title from labels * refactor: extract sandcastle into modular architecture Split main.ts (525 lines) into 6 self-contained modules: - types.ts: shared domain types (TaskSpec, Finding, LoopResult, FinalizeResult) - refinement-loop.ts: reusable implement↔critic loop engine - finalizer.ts: validation, rebase, PR creation - concurrency-pool.ts: semaphore utility - task-source.ts: TaskSource interface + GithubIssueSource - main.ts: 74-line thin orchestrator wiring all modules The refinement loop is now reusable by any task source (GitHub issues, CI failures, manual triggers) without coupling to the planner. * fix: centralize constants, fix PR type-of-change, sanitize titles * fix: full validation post-rebase, execFileSync for gh issue list * perf: skip critic when implementer produces 0 commits on round 2+ * fix: remove unsound type guard, filter unknown plan IDs, guard ConcurrencyPool, warn on planner exhaustion * feat: state-of-the-art algorithmic improvements - Budget: flat constant 50/round (ARCS/SWE-Agent pattern, replaces suboptimal decreasing schedule) - Dedup: context-hash fingerprint of ±3 lines (CodeQL/Qodana pattern, drift-safe) - Quality ratchet: rollback round on regression (findings increase → git reset --hard) - Log: post-rebase validation failure now logs stderr - Safety: rescue branch on push failure (preserves commits before sandbox disposal) * fix(sandcastle): resolve all algorithmic audit findings - Replace execSync+sed shell injection with readFileSync+join in hashContextLines (RCE, P0) - Fix quality ratchet to compare nonLowFindings.length not findings.length (logic bug, P1) - Replace title truncation with SHA-256 hash in findingKey (collision fix, P2) - Hash single target line + file salt instead of positional context window (line-drift fix, P2) - Replace overlapping regex quantifiers in sanitizeForPrompt (ReDoS O(n²), P2) - Replace Array FIFO queue with O(1) singly-linked list in ConcurrencyPool (P3) - Move totalCommits accumulation after ratchet check to fix phantom counting (P3) - Replace O(T×I) .find() with O(1) Map lookup in validatePlan (P3) * fix(sandcastle): harden subprocess calls and reduce cyclomatic complexity - Replace all execSync template literals with execFileSync (array args) to eliminate shell injection vectors from LLM-controlled data - Strengthen path traversal guard: resolve() + sep instead of join() - Validate SHA format (/^[0-9a-f]{40}$/) before git reset --hard - Decompose finalizeTask (CC 15→5) into runValidation, attemptRebase, pushBranch, buildPrArgs, extractStderr - Decompose runRefinementLoop (CC 18→8) into executeRound, deduplicateFindings, checkQualityRatchet - Replace process.exit(0) with natural script termination - Add title validation (length ≤200, no control chars) in validatePlan - Add console.warn in previously silent catch blocks * fix(sandcastle): address review findings — convergence, planner failure, rebase note - Treat newFindings=0 as 'converged' regardless of repeated non-LOW findings (previously marked 'exhausted', forcing draft PRs) - Set process.exitCode=1 when planner exhausts retries (prevents masking a broken planner as 'no work to do') - Pass rebaseSucceeded to buildPrArgs, append rebase failure note to PR body when branch is not rebased onto main - Reword sanitizeForPrompt JSDoc to accurately describe scope * fix(.sandcastle): address all algorithmic audit findings - types.ts: parseFindingsSafe with partial recovery (finding #18) - concurrency-pool.ts: guard NaN/float/Infinity in constructor (#1,#4), JSDoc re-entrant warning (#2) - task-source.ts: NFKC normalize + extended blocklist in sanitizeForPrompt (#17,#20), Map lookup in validatePlan (#7) - refinement-loop.ts: readFileSync replaces execSync/sed (shell injection #5), radius used (#12), fallback hash hardened (#13), line clamping (#14), realpathSync symlinks (#15), file cache Map (#11), parseFindingsSafe (#18), nonce validation (#16), trivial match skip (#19), implementer try/catch (#7), ratchet guard round<=2 (#6), no duplicate nonLowFindings (#9) - finalizer.ts: pushBranch returns boolean (#22), crypto rescue suffix (#21), rollback error level (#8) - main.ts: TASK_TIMEOUT_MS + Promise.race timeout (#3), flat budget JSDoc (#10) * fix: unref timeout timer to prevent process hang, catch critic throws * fix: count commits before break on critic failure (prevents work loss) * fix: suppress unhandled rejection from timeout promise on task success * fix: force process exit after completion (prevents hang from timed-out sandboxes) * fix: check findings null before commits zero (correct status on implementer crash) * fix: report known findings in PR body even when converged (prevents silent false convergence) * feat: state-of-the-art convergence improvements (ARCS, SWE-Agent, OpenHands) - Validation in-loop (ARCS): deterministic convergence when tests pass mid-loop - Best-state checkpoint (SWE-Agent): reset to best SHA on non-convergence - Severity-weighted convergence (OpenHands): refuse convergence if CRITICAL/HIGH persist * fix: move bestSha after ratchet, add validation timeout, fix severity/bestSha mismatch * fix: recount totalCommits from git after best-state reset (semantic correctness) * refactor(.sandcastle): address all 45 quality audit findings - New constants.ts: shared constants (VALIDATION_COMMAND, timeouts, model names) + utilities (getHeadSha, toErrorMessage) - refinement-loop.ts: decompose runRefinementLoop (CC 17→≤10), RoundContext/HashInput param objects, computeFindingKey rename - finalizer.ts: add timeouts to all execFileSync, use runValidation helper consistently - task-source.ts: add timeout, replace char loop with regex, fix terse names - main.ts: extract withTimeout helper, use model constants - types.ts: unexport FindingsSchema (internal only) * fix: use constants from constants.ts + add planner timeout (multi-agent audit findings) * refactor(.sandcastle): harden prompts — cap findings, add known decisions, scope preference * perf(.sandcastle): convert execFileSync to async execFileAsync (unblock event loop) Replace all blocking execFileSync calls with util.promisify(execFile) to enable true parallelism between tasks during subprocess execution. - constants.ts: add execFileAsync export, convert getHeadSha to async - refinement-loop.ts: captureHeadSha, checkQualityRatchet, checkConvergence, runMidLoopValidation, resetToBestState all async - finalizer.ts: runValidation, attemptRebase, pushBranch all async - task-source.ts: fetchAndSanitizeIssues async readFileSync/realpathSync stay sync (<1ms local I/O, no benefit from async). maxBuffer: 8MB added to validation and gh issue list calls. * fix: catch planner timeout rejection (retry instead of crash) + add catch type annotations
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
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.
Summary
escapeTemplatePlaceholdersoption to escape{{patterns in message content{{?variable}}syntaxChanges
escapeOrchestrationPlaceholders()/unescapeOrchestrationPlaceholders()utilitiesRelated Issues