Skip to content

docs: add Adding Engines guide and update backends README#1030

Merged
aaight merged 2 commits intodevfrom
feature/adding-engines-guide
Mar 24, 2026
Merged

docs: add Adding Engines guide and update backends README#1030
aaight merged 2 commits intodevfrom
feature/adding-engines-guide

Conversation

@aaight
Copy link
Copy Markdown
Collaborator

@aaight aaight commented Mar 24, 2026

Summary

  • Creates docs/adding-engines.md — comprehensive step-by-step guide for adding a new agent engine to CASCADE
  • Updates src/backends/README.md to reference the new guide and reflect the NativeToolEngine base class and archetype system

What's in the guide

The guide covers every step needed to add a new subprocess-based engine (e.g. Gemini CLI, Kilo Code):

  1. Archetype selection — when to use native-tool (subprocess CLI) vs sdk (in-process like LLMist), with clear decision criteria
  2. Directory layout — standard structure for src/backends/<engine-name>/
  3. Engine definitionAgentEngineDefinition in catalog.ts with archetype field
  4. Env filtering — allowlist pattern using SHARED_ALLOWED_ENV_EXACT, blocking server-side secrets
  5. Model definitionsmodels.ts pattern, including free-text option for open-ended models
  6. Settings schema — optional Zod schema + resolver for configurable engine behaviour
  7. Engine class — minimal concrete NativeToolEngine subclass with all abstract methods, lifecycle hooks, and key helpers annotated
  8. Bootstrap registrationregisterEngineWithSettings() in bootstrap.ts
  9. Dockerfile.worker — where to add the CLI install step
  10. Testing requirements — engine-contract tests, env-filter tests, settings tests
  11. CLI/Dashboard — explains why no additional changes are needed (dynamic catalog)

Includes a reference table of all four existing engines (Claude Code, Codex, OpenCode, LLMist) with their archetypes and notable implementation patterns.

Test plan

  • TypeScript type check passes (npm run typecheck)
  • Linter passes (npm run lint) — no issues in markdown or TS files
  • All unit tests pass (npm test)
  • Verified all file paths, type names, and function signatures in the guide against the actual codebase

Trello card

https://trello.com/c/EggwMeUU/542-as-a-developer-i-want-an-adding-engines-guide-so-that-adding-a-new-harness-takes-hours-instead-of-days

🤖 Generated with Claude Code

🕵️ claude-code · claude-sonnet-4-6 · run details

zbigniewsobiecki added a commit that referenced this pull request Mar 24, 2026
…rray

GitHub fires check_suite webhooks with pull_requests: [] when CI runs on
the refs/pull/{N}/head virtual ref rather than the named feature branch.
The CheckSuiteFailureTrigger's matches() guard silently rejected these,
so respond-to-ci never fired even when CI failed on implementer PRs.

Root cause confirmed via webhook log f0f88951 for PR #1030: conclusion
was "failure", head_branch was "refs/pull/1030/head", pull_requests was [].

Changes:
- Extract parsePrNumberFromRef() to utils.ts (removes duplication between
  the two trigger files, adds JSDoc explaining the merge-ref exclusion)
- Restrict the regex to refs/pull/{N}/head only — the /merge variant uses
  a synthetic merge-commit SHA that is not part of the PR branch history
- Update matches() in both CheckSuiteFailureTrigger and CheckSuiteSuccessTrigger
  to accept events where pull_requests is empty but head_branch parses as a PR ref
- Simplify prBranch resolution to always use prDetails.headRef (already
  fetched) instead of a conditional that picked the same value either way
- Fix head_branch type to string | null (GitHub sends null, not undefined,
  when the check suite is not associated with a named branch)
- Add respond-to-ci trigger example to docs/getting-started.md

Tests:
- New unit tests for the empty pull_requests + refs/pull/{N}/head path in
  both trigger test files (matches and handle coverage)
- New describe('parsePrNumberFromRef') block in github-utils.test.ts
  covering null/undefined/empty, plain branches, /merge rejection,
  valid /head refs, and partial-match rejection

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
zbigniewsobiecki added a commit that referenced this pull request Mar 24, 2026
…rray (#1031)

GitHub fires check_suite webhooks with pull_requests: [] when CI runs on
the refs/pull/{N}/head virtual ref rather than the named feature branch.
The CheckSuiteFailureTrigger's matches() guard silently rejected these,
so respond-to-ci never fired even when CI failed on implementer PRs.

Root cause confirmed via webhook log f0f88951 for PR #1030: conclusion
was "failure", head_branch was "refs/pull/1030/head", pull_requests was [].

Changes:
- Extract parsePrNumberFromRef() to utils.ts (removes duplication between
  the two trigger files, adds JSDoc explaining the merge-ref exclusion)
- Restrict the regex to refs/pull/{N}/head only — the /merge variant uses
  a synthetic merge-commit SHA that is not part of the PR branch history
- Update matches() in both CheckSuiteFailureTrigger and CheckSuiteSuccessTrigger
  to accept events where pull_requests is empty but head_branch parses as a PR ref
- Simplify prBranch resolution to always use prDetails.headRef (already
  fetched) instead of a conditional that picked the same value either way
- Fix head_branch type to string | null (GitHub sends null, not undefined,
  when the check suite is not associated with a named branch)
- Add respond-to-ci trigger example to docs/getting-started.md

Tests:
- New unit tests for the empty pull_requests + refs/pull/{N}/head path in
  both trigger test files (matches and handle coverage)
- New describe('parsePrNumberFromRef') block in github-utils.test.ts
  covering null/undefined/empty, plain branches, /merge rejection,
  valid /head refs, and partial-match rejection

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
@zbigniewsobiecki zbigniewsobiecki requested review from nhopeatall and removed request for nhopeatall March 24, 2026 16:28
Copy link
Copy Markdown
Collaborator

@nhopeatall nhopeatall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Documentation-only PR that adds a comprehensive engine authoring guide and updates the backends README. The guide is well-structured, accurately reflects the codebase's current architecture, and the core CI checks (lint, test, typecheck, Docker build, integration) all pass. The two failing checks (Analyze (javascript-typescript) and Analyze (actions)) are CodeQL scans unrelated to markdown/documentation changes.

I verified the guide's code templates, file paths, type names, function signatures, and architectural descriptions against the actual codebase. Everything checks out with one minor inconsistency worth noting.

Should Fix

  • docs/adding-engines.md (Sections 4 vs 7) — The env.ts template in Section 4 exports ALLOWED_ENV_EXACT, and the test template in Section 10 imports from env.ts — but the engine class template in Section 7 inlines the same set directly in getAllowedEnvExact() rather than importing from env.ts. This creates a duplication hazard: a developer following the guide would define the same set in two places (env.ts and the class), which could drift. The Section 7 template should import from ./env.js instead:

    import { ALLOWED_ENV_EXACT } from './env.js';
    
    getAllowedEnvExact(): Set<string> {
      return ALLOWED_ENV_EXACT;
    }

    This matches the pattern used by Claude Code's engine class (which imports ALLOWED_ENV_EXACT from ./env.js and returns it directly). The Codex and OpenCode engines do inline the set, but they also have legacy buildEnv() functions in their env.ts that predate the NativeToolEngine refactor — new engines shouldn't follow the legacy pattern.

Notes

  • The NativeToolEngine base class has a resolveSettings(input, schema) helper (line 121 of NativeToolEngine.ts) that the guide's template doesn't mention. All existing engines use standalone resolveXxxSettings() functions instead, so the template is consistent with current practice — but a brief mention of this base-class alternative could be useful for implementors who prefer it.
  • All file paths, export names, function signatures, and type references in the guide are accurate against the current codebase.
  • The relative link in src/backends/README.md (../../docs/adding-engines.md) resolves correctly.

🕵️ claude-code · claude-opus-4-6 · run details

…s template

Import ALLOWED_ENV_EXACT from ./env.js and return it directly in
getAllowedEnvExact(), eliminating the duplication hazard of defining
the same set in both env.ts and the engine class. This matches the
pattern used by the Claude Code engine class.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 24, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Copy link
Copy Markdown
Collaborator

@nhopeatall nhopeatall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

LGTM — Well-structured, comprehensive documentation that accurately reflects the codebase. The guide covers all necessary steps for adding a new engine, and the code examples are consistent with actual patterns in the repository.

Verification

I verified the guide's claims against the actual source files:

  • NativeToolEngine abstract methodsgetAllowedEnvExact(), getExtraEnvVars(), resolveEngineModel(), execute() all match the abstract class contract in src/backends/shared/NativeToolEngine.ts
  • buildSystemPrompt / buildTaskPrompt signatures — match src/backends/shared/nativeToolPrompts.ts
  • buildEngineResult / extractAndBuildPrEvidence — match src/backends/shared/engineResult.ts
  • registerEngineWithSettings pattern — matches src/backends/bootstrap.ts (checks getSettingsSchema presence, registers both engine and schema)
  • AgentEngineDefinition type shape — capabilities array, modelSelection union type, settings field, archetype field all match src/backends/types.ts
  • SHARED_ALLOWED_ENV_EXACT / SHARED_BLOCKED_ENV_EXACT — match src/backends/shared/envFilter.ts
  • Engine directory layouts — verified claude-code/, codex/, opencode/ all follow the pattern described
  • Test directory — confirmed test files exist at tests/unit/backends/ as referenced
  • Architecture quick-reference — file tree matches actual src/backends/shared/ contents (core files present)
  • Real-world examples table — Claude Code is correctly noted as "SDK-based (not subprocess)", Codex correctly noted as "subprocess + JSONL"
  • Relative link ../../docs/adding-engines.md from src/backends/README.md resolves correctly to project root

All CI checks pass. The backends README update is a clean improvement — replacing the terse 3-line "how to add an engine" section with a proper archetype explanation and a link to the full guide.

🕵️ claude-code · claude-opus-4-6 · run details

@aaight aaight merged commit 8a74326 into dev Mar 24, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants