Repository navigation
[feat] Add codeExecutor.enabled to decouple code-executor from workflows - #419
Merged
Merged
Conversation
Contributor
 [Medium impact] The PR appears safe to merge, with no new actionable issues found. Reviews (3) · Last reviewed commit: "Address review comments: trim comments a..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
jmhwang7
commented
Nov 5, 2026
jmhwang7
left a comment
jmhwang7
left a comment
Contributor
Author
There was a problem hiding this comment.
Re: Greptile's "Missing executor still passes" finding on ci/test-code-executor-enabled-option.yaml —
Good catch, addressed in 9c4db99. The kubeconform fixture is schema-only, so I added .github/test_code_executor.py (a unittest suite wired into the lint-chart CI job, matching the existing test_mcp_*.py tests) with behavioral assertions:
test_standalone_renders_with_workflows_off: with workflows off +codeExecutor.enabled=true, asserts the code-executor Deployment and ConfigMap render,CODE_EXECUTOR_INGRESS_DOMAINis present in the backend env, and the workflow workloads (deployment_workflows.yaml) +WORKFLOW_BACKEND_HOSTare absent — so restoring the old workflows-only gate would fail this test.- Plus coverage for
enabled: nullinheritingworkflows.enabledboth ways, explicit-false disabling, the workflows-on + false loud failure, non-bool rejection, and the AppArmor installer still rendering for a standalone executor.
jmhwang7
marked this pull request as ready for review
November 5, 2026 19:25
jmhwang7
marked this pull request as draft
November 5, 2026 19:25
jmhwang7
marked this pull request as ready for review
November 5, 2026 20:16
jmhwang7
commented
Nov 5, 2026
The code executor is a standalone sandbox service, but until now its rendering was welded to workflows.enabled, so it could only run as part of the workflows stack. Add a top-level codeExecutor.enabled flag, co-located with the rest of the codeExecutor config, so the service can be deployed on its own (e.g. for custom auth and firebase raw mode) or turned off independently. Semantics mirror the rr component switches: - null (default) -> inherit workflows.enabled (back-compat: existing installs that only toggle workflows.enabled are unaffected) - true/false -> override - non-bool -> fail loudly Also: - Split the backend env block so CODE_EXECUTOR_INGRESS_DOMAIN follows the new flag (the backend learns the executor's address even with workflows off), while WORKFLOW_BACKEND_HOST stays on workflows.enabled. - Gate the shared nsjail AppArmor installer's code-executor branch on codeExecutor.enabled so it still renders for a standalone code executor. - Add retool.codeExecutor.validateEnabled: fail when workflows are enabled but codeExecutor.enabled is false, which would ship a broken workflows stack. - Add ci/test-code-executor-enabled-option.yaml exercising the standalone path. - Bump chart version to 6.13.6. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Addresses Greptile review: the ci/ option fixture is only schema-validated by kubeconform, so a regression that re-couples code-executor rendering to workflows.enabled would drop the executor while the fixture still passes. Add .github/test_code_executor.py (run in the lint-chart CI job, like the existing MCP tests) asserting the actual behavior: - standalone executor (workflows off + codeExecutor.enabled=true) renders the Deployment + ConfigMap, wires CODE_EXECUTOR_INGRESS_DOMAIN into the backend, and leaves the workflow workloads / WORKFLOW_BACKEND_HOST absent - enabled: null inherits workflows.enabled both ways (back-compat) - explicit false disables; workflows-on + false fails loudly; non-bool fails - the AppArmor installer still renders for a standalone executor Bump chart version to 6.13.7 (6.13.6 was released). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Drop the unnecessary "(e.g. ...)" parenthetical from the retool.codeExecutor.enabled helper doc comment. - Restore the "legacy" descriptor on code-executor in the AppArmor installer helper comment. - Trim ci/test-code-executor-enabled-option.yaml to the minimal case (workflows off + codeExecutor enabled + seccomp + AppArmor installer); behavioral assertions live in .github/test_code_executor.py. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
jmhwang7
force-pushed
the
claude/executor-services-setup-04e85a
branch
from
November 5, 2026 20:19
aa44d5e to
6658c98
Compare
The values-yaml-synced CI check requires values.yaml and charts/retool/values.yaml to be byte-identical. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
slaminad
approved these changes
Nov 5, 2026
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.
What
Adds a top-level
codeExecutor.enabledflag so the code-executor service can be deployed (or disabled) independently of workflows.Until now, code-executor rendering was gated solely on
workflows.enabled, so the service could only run as part of the workflows stack. The code executor is actually a standalone sandbox service — workflows need it, but it also serves other use cases (e.g. custom auth and firebase raw mode).Semantics
codeExecutor.enabledmirrors the existingrr.*component switches:null(default)workflows.enabled— back-compat: existing installs that only toggleworkflows.enabledare unaffectedtruefalseChanges
retool.codeExecutor.enabledhelper (_helpers.tpl) with null-inherit semantics; gatesdeployment_code_executor.yamlandconfigmap_code_executor.yaml.deployment_backend.yaml):CODE_EXECUTOR_INGRESS_DOMAINnow followscodeExecutor.enabledso the backend knows the executor's address even with workflows off;WORKFLOW_BACKEND_HOSTstays onworkflows.enabled.retool.appArmorNsjailInstaller.enabled): the code-executor branch now gates oncodeExecutor.enabled, so the shared nsjail DaemonSet still renders for a standalone code executor.retool.codeExecutor.validateEnabledguard: fails the render whenworkflows.enabled: true+codeExecutor.enabled: false, which would ship a workflows stack with no executor.values.yaml:codeExecutor.enabled: nulldocumented at the top of thecodeExecutorblock.ci/test-code-executor-enabled-option.yaml: exercises the standalone path (workflows off + code-executor on, with seccomp/AppArmor/PDB branches).6.13.6.Reviewer notes
enabled: null, the helper returns exactly whatretool.workflows.enabledreturns, at every call site.helm templateacross: workflows-on/null, workflows-off/null (not rendered), workflows-off/true (rendered,CODE_EXECUTOR_INGRESS_DOMAINonly), workflows-on/false (loud fail), and non-bool (loud fail).helm lintis clean aside from the pre-existing PDB document-separator false-positive that the existing js-executor test also produces.🤖 Generated with Claude Code