Skip to content

fix(compose): sandbox the compose body render, and add a catalog render oracle - #136

Open
omarsy wants to merge 11 commits into
mainfrom
feat/compose-render-sandbox
Open

omarsy wants to merge 11 commits into
mainfrom
feat/compose-render-sandbox

Conversation

@omarsy

@omarsy omarsy commented Sep 2, 2026 •

Copy link
Copy Markdown
Member

The compose body was rendered through the stock jinja2.Template, so any {{ ... }} in any string value reached the interpreter. Verified: {{ cycler.__init__.__globals__.os.popen("id").read() }} returns real uid= output through the old path. The greffer process holds the manager token, the Docker socket and every instance's TLS private key, so that is host root.

Reachability. Two sources feed this render, and only one is reviewed. The catalog template is PR-reviewed (community-contributed, so a missed malicious PR is a real supply-chain path), but the config VALUES injected into it are user input and render in the same pass: verified through the real pipeline, a config env value of {{ 7*7 }} comes out 49. So the injection surface is reachable from authenticated instance configuration today, not only from the catalog or the future custom-app-deploy path. On a shared greffer the process holds other tenants' secrets, so the ceiling is cross-tenant; confirming whether the manager constrains config values is worth doing as defence in depth, but the greffer must not depend on it.

Against the compose-containment epic

This is Feature 1 of the compose-containment epic (merged in greffon/greffon#267), specifically the half the epic says stands alone near-term as catalog supply-chain defense: the sandbox. The catalog render oracle below is the epic's Feature 2, and round 2's 422 mapping is the render-failure slice of Feature 3 ("fail with a diagnosis, never a 500").

Not here, on purpose: Feature 1's other half, an input/output/time budget around the render. The sandbox stops code execution, not resource use: {{ "A" * (10 ** 8) }} is sandbox-legal and renders a 100 MB compose. The epic scopes that budget to the custom (untrusted) path.

The fix

Render through an ImmutableSandboxedEnvironment with Jinja's mutable-type globals removed. Both render envs (compose body and baked files) are hardened identically.

Deliberately unchanged: the undefined policy. The body keeps its lenient Undefined, so a missing {{ config.X }} still renders empty. Tightening that would start failing catalog entries that rely on an empty render, which is a separate decision with its own blast radius. Baked files keep their own StrictUndefined, because a silently-empty secret is a security failure whereas a silently-empty compose value is the status quo this preserves.

Two holes that only showed up under review

Codex found both, after I had already described the problem as fixed:

  1. A sandbox is not enough. SandboxedEnvironment blocks the attribute walk but still permits mutating calls, and render() receives the live greffon_info dicts. {{ volumes.update({"evil": {"value": "/"}}) }} renders as empty output while adding a volume, and create_volumes_then_copy_files then runs docker container create -v /:/root (volume.py:61) and copies attacker content onto the host.
  2. Immutable is not enough either. It decides mutability from the object a method is bound to, so the unbound form slips past: {{ dict.update(volumes.data, {"value": "/"}) }} inspects the dict class, not the live mapping. Verified rendering clean and setting the value to /. Fixed by removing dict/cycler/joiner/namespace/range/lipsum from both envs; no pinned catalog entry uses any of them.

Round 2: four findings from an adversarial review

A fresh review confirmed the sandbox is sound (59 escape payloads, nothing reached os, subprocess, builtins or the environment; no catalog entry changes), and found four defects around it. Each fix has a mutant that was watched to fail.

  1. A refused compose was a bare 500. The sandbox added SecurityError as a new way for create_compose to fail, and it surfaced as {"message": "internal_error"}. The manager forwards the greffer's status and detail as-is, so the operator saw no cause. create_compose now raises ConfigRenderError, the class the baked-file path already uses, and the start router answers 422 with the reason. This also turns the TemplateSyntaxError / UndefinedError / TypeError a broken template already raised from 500 into 422; the manager handles both through the same branch, so nothing changes there except that the cause reaches the operator.
  2. The baked-file env's immutability was untested. Swapping it for a plain SandboxedEnvironment passed the whole suite while letting {{ volumes.data.update({"value": "/"}) }} set a volume to /, mounted as -v /:/root. Now tested, and the tests assert the live context comes out untouched.
  3. The oracle blessed a greffon that cannot render. A failed render's marker became its snapshot. Reproduced: an SSTI entry plus CATALOG_RENDER_UPDATE=1 wrote <<RENDER FAILED ...>>, then 31 passed. Now a failure unless _KNOWN_UNRENDERABLE names the entry with a reason, checked before any snapshot is read or written.
  4. The oracle went red off the pinned catalog, one "render changed" per drifted entry, each pointing at the regenerate command that then breaks CI. CATALOG_SHA now records the commit; on a mismatch the coverage test fails once, naming both commits and how to get the right one. A tripwire requires both workflows to pin that commit.

Smaller: the unbound-mutator tests asserted only that something raised, which passed {% set _ = <mutate> %}{{ 1 / 0 }} with the mutation landed (they now check the context), and _harden's docstring claimed a memory-blowup mitigation it does not provide.

Round 3: a fresh review of the round-2 fix

An independent pass confirmed the sandbox holds and the oracle gate is sound, and found the round-2 422 mapping was still under-broad and leaked its detail. Both fixed in f6ed739:

  • The catch named TemplateError/TypeError/ValueError, so an evaluation error on valid Jinja escaped as a 500 again: {{ 1 // 0 }} (ZeroDivisionError), {{ 10.0 ** 400 }} (OverflowError), {{ "x".encode("no-such-codec") }} (LookupError), a recursive macro (RecursionError). Now any exception from render() maps, with yaml.dump kept outside so a non-representable dict stays an honest 500.
  • The forwarded detail interpolated str(exc), which the manager logs and forwards. A render-failure message can carry rendered bytes (a value quoted back, an unbounded length), so the detail now names the exception TYPE only; the full reason stays in the greffer log.

A create_compose-level test now pins the exception set across both hierarchies the old catch missed, and a second pins that the detail carries the type and not the message. Before these, narrowing or broadening the catch and reverting the detail all passed the full suite. 940 passed.

The catalog render oracle

The merge gate for any future change to this path: every catalog entry is rendered and diffed against a committed snapshot. It mirrors controller.py exactly (get_greffon_info → build_render_context → get_compose_template → apply_configuration → create_compose), with only host-port allocation and the daemon-backed L4 reservation stubbed, because those answers are not stable and a snapshot that moves on its own proves nothing.

Fidelity is measured rather than asserted: 0/29 render failures, 29/29 include the injected greffon_nginx sidecar, 15/29 interpolate the SMTP username (matching exactly the 15 composes that reference it), and the two L4 entries render as L4 (0.0.0.0:20000:20000/udp published directly with same_port pinning) rather than being downgraded to HTTP.

CI now fetches a pinned catalog. Pinned rather than @main because the snapshots are artifacts of one catalog state, so tracking main would turn this repo's CI red whenever the catalog changes. Applied to docker-publish.yml too, since that job gates the signed release and must not test less than the PR path.

Evidence

  • 936 passed with the pinned catalog, on this branch merged with current main (922 before round 2), zero failures.
  • Every change mutation-checked. Reverting the render fails 5 tests; restoring the globals fails 5; altering render output fails all 29 snapshots.
  • The mutation checks found three defects in my own tests: single-quoted payloads were blocked by the yaml.dump round-trip rather than the sandbox (passing vacuously, only 2 of 5 biting), a leaked GREFFON_PATH, and a leaked L4 pending reservation that broke a later existing test.

Pre-existing issue found, not fixed here

tests/test_workers_register.py hangs indefinitely in this environment, on unmodified origin/main. Named on baseline: test_register_worker_does_not_poll_cert_after_rejected_register, with a sampled stack blocked in socket_gethostbyname. It is excluded from the runs above and is out of scope for this PR, but it is worth its own fix: a hang in the suite that gates the signed release.

Merge order: before #137

#137 defines its own _COMPOSE_RENDER_ENV = SandboxedEnvironment(), under the same name, and that is the mutable, unhardened kind finding 2 above shows is bypassable into -v /:/root. Merge this first; #137's rebase must keep this PR's definition and drop its own.

The compose body was rendered through the stock jinja2.Template, so any
{{ ... }} in any string value reached the interpreter. A value of
{{ cycler.__init__.__globals__.os.popen("id").read() }} executes in the
greffer process, which holds the manager token, the Docker socket and every
instance's TLS private key -- host root, not a sandbox escape. Verified: the
payload returns real uid= output through the old path.

Latent rather than live today, because the catalog is the only input and is
PR-reviewed. But the catalog is community-contributed, so a missed malicious
PR is a real supply-chain path, and custom app deploy would hand this render
unreviewed input by design.

The change is three lines: render through a SandboxedEnvironment instead.

Deliberately NOT changed: the undefined policy. The body keeps its lenient
Undefined, so a missing {{ config.X }} still renders empty. Tightening that
would start failing catalog entries that rely on an empty render, which is a
separate decision with its own blast radius. Baked files keep their own
STRICT env -- a silently-empty secret in a config file is a security failure,
whereas a silently-empty compose value is the status quo this preserves.

Evidence:
- New catalog render oracle: all 31 catalog entries render byte-identically
  before and after (snapshots committed). This is the merge gate for any
  future change to this path.
- New sandbox suite: 5 escape chains now raise SecurityError.
- Mutation-checked, and the check found a flaw in my own tests: single-quoted
  payloads are killed by the yaml.dump round-trip (inner ' becomes '' ->
  TemplateSyntaxError) BEFORE the sandbox is consulted, so they passed
  vacuously -- only 2 of 5 bit on revert. Switched to double-quoted payloads
  and asserted SecurityError specifically; now all 5 bite. Reverting the
  render fails 5 tests; altering render output fails all 31 snapshots.
…e's own holes

Codex review of the previous commit found two P1s. Both verified and real.

P1 -- the sandbox was not enough. SandboxedEnvironment blocks the attribute
walk to the interpreter but still permits MUTATING calls, and render() is
handed the LIVE greffon_info dicts. Verified: dict.update and list.append
both succeed under SandboxedEnvironment and both raise SecurityError under
ImmutableSandboxedEnvironment. The sink is real and short:
  {{ volumes.update({"evil": {"value": "/"}}) }}
renders as empty output while adding a volume, then
create_volumes_then_copy_files (compose.py:613) feeds volume['value'] into
  docker container create -v <value>:/root      (volume.py:61)
and copies attacker content into the host filesystem. Blocking dunder
traversal alone left that write primitive wide open. Both envs are now
ImmutableSandboxedEnvironment -- the baked-file env too, since it renders
with the same context and had the same hole.

P1 -- the tests leaked GREFFON_PATH. A bare os.environ assignment survives
the test (pytest is one process), and test_settings.py asserts the unset
default of /data, so the full suite would fail from my own fixtures. Now
monkeypatch.setenv in the sandbox suite and a finally-restore in the harness;
verified by running both files in the same process as test_settings.

P2 -- the oracle silently covered NOTHING outside my own invocation. The
ancestor search checked fixed depths 4-6, but this worktree's monorepo root
is parents[3] and a plain checkout is closer still, so without the env
override _ENTRIES was empty -- and the coverage guard SKIPPED on empty, which
reads exactly like green. Now it walks every ancestor (verified: finds the
catalog with no override) and the guard FAILS rather than skips, with
GREFFON_CATALOG_OPTIONAL=1 as the deliberate opt-out. CI must fetch a catalog
checkout or it will now say so out loud.

Two new mutation regression tests cover the update/append primitive.
Mutation-checked: downgrading to a plain sandbox fails exactly those two.
All 31 catalog snapshots remain byte-identical, so immutability costs the
catalog nothing.
Codex on the previous commit: the coverage guard I added would have turned
every PR run AND the signed publish red, because actions/checkout fetches
only this repo and nothing sets GREFFON_CATALOG_DIR. That is a real break,
and 'make the test skip' would have been the wrong fix -- a skip is what let
the oracle cover nothing while reading green in the first place.

So CI now fetches the catalog. greffon-catalog is public, so no token.

PINNED to a SHA rather than tracking @main: the snapshots are artifacts of
one catalog state, so following main would turn THIS repo's CI red whenever
the catalog adds or edits a greffon -- a failure caused by another repo's
change. Bumping is deliberate: regenerate with CATALOG_RENDER_UPDATE=1 and
review the diff.

Snapshots regenerated against the pinned SHA (5652a755). They had been
generated from a local catalog checkout that was 13 commits behind origin
AND carried 3 untracked WIP greffons (kokoro, openfamily, tcptest), so
pinning without regenerating would have failed CI on day one: 29 entries now,
matching the pin exactly.

Applied to docker-publish.yml as well as ci.yml -- that job gates the signed
release and its comment requires it to test no less than the PR path.

Verified all three states: catalog present -> 30 pass; catalog absent -> the
guard FAILS with an actionable message (not a skip); GREFFON_CATALOG_OPTIONAL=1
-> skips deliberately.
…P contract

Codex round 3 on the oracle itself (it called the sandbox change sound both
times). Two real defects in my regression gate:

1. The oracle skipped production preprocessing. It called create_compose
   directly, so snapshots carried RAW catalog ports and volumes, no
   greffon_nginx sidecar, and no config destinations applied -- not the bytes
   a deployment renders. A regression in the real deployment input could have
   passed the gate. Now it mirrors app/routers/controller.py exactly:
   get_greffon_info -> build_render_context -> get_compose_template ->
   apply_configuration -> create_compose, with ONLY host-port allocation
   stubbed (it probes real sockets, and a snapshot that moves on its own
   proves nothing). Fixing this exposed that my seed was still the DERIVED
   greffon_info shape rather than the start-request shape, which is why the
   first attempt failed all 29 renders; the manager also assigns each port
   its public URL, so that is now supplied deterministically instead of
   leaving url=None and deriving instance_url from a fallback.

2. The SMTP fixture used 'user' where the manager's contract
   (SMTPConfigSerializer: host/port/username/password/from_address/tls_mode)
   says 'username'. 15 catalog composes reference {{ smtp.username }}, so
   those rendered EMPTY -- the snapshots blessed an impossible
   configured-SMTP state and never exercised the interpolation at all.

Fidelity is now measurable rather than asserted: 0/29 render failures,
29/29 include the injected greffon_nginx sidecar, 15/29 interpolate the SMTP
username (matching the 15 composes that reference it).

Mutation-checked again: altering the render still fails 29 snapshots.
…o HTTP

Codex round 4, same class as rounds 2 and 3: the oracle's request shape was
not the manager's, so it silently under-covered.

Exposure is manager-authoritative -- create_greffon_info reads
greffon['ports'][port_name] and DEFAULTS to http/tcp when absent
(repository.py:239). My seed omitted that map entirely, so the two catalog
entries that declare L4/UDP (visio's livekit_7882 and wireguard's
wg-easy_51820, both same_port with udp_reviewed) rendered as ordinary HTTP:
their UDP ports were placed behind nginx, instance_l4_* stayed empty, and the
production L4 render path was never exercised once. That is a real gap given
how central L4 and tunnel mode are here.

The seed now builds the port map from each entry's metadata.json, and the
snapshots show the L4 path actually running: 0.0.0.0:20000:20000/udp
published directly on the owning service with same_port pinning, instead of
an nginx upstream.

Two stubs, both for determinism and neither hiding logic: get_free_ports
probes real sockets, and l4_ports.published_l4_ports/pending_and_prune ask
the docker daemon which host ports are occupied. Neither answer is stable
across runs. The allocation LOGIC -- sticky reuse, same_port pinning,
per-protocol namespacing -- still runs for real against an empty host.

Also: only Tier-A ports now receive an https URL, since an L4 port is not a
web entry and the manager does not give it one.

(The suite's docker-daemon-at-import requirement is pre-existing: test_compose.py
fails identically with a bogus DOCKER_HOST, and CI's ubuntu-latest ships a
daemon, which ci.yml documents.)

Mutation-checked: altering the render still fails all 29 snapshots.
…passable

Codex round 5 found a genuine bypass of the previous commit's fix, and it is
the same host-write primitive I thought I had closed.

ImmutableSandboxedEnvironment decides mutability from the object a method is
BOUND to. An UNBOUND call therefore slips past: 'dict' is one of Jinja's
default globals, so

  {{ dict.update(volumes.data, {"value": "/"}) }}

inspects the dict CLASS rather than the live mapping. Verified directly: the
bound form raises SecurityError while this form RENDERED CLEAN and set the
volume's value to "/" -- after which create_volumes_then_copy_files runs
'docker container create -v /:/root' (volume.py:61) and copies attacker
content onto the host. Same sink as before, reached a different way.

Fix: remove the mutable-type and escape-chain globals from BOTH render envs.
dict is the mutator vector; cycler/joiner/namespace are the documented first
hops of the classic attribute-walk chains; lipsum/range are useless in a
compose file and range invites a cheap memory blowup. Verified across every
pinned catalog entry that none of the six is used, and the 29 snapshots are
byte-identical afterwards, so this costs the catalog nothing.

Tests: four unbound-mutator payloads (update/setdefault/pop/clear), plus a
test asserting the globals are absent -- pinning the MECHANISM, because a
payload test can pass for the wrong reason (a typo also raises) while the
surface is quietly re-opened.

Mutation-checked: restoring the globals fails 5 tests.
Codex round 6, and the same class as the GREFFON_PATH leak in round 2: my
test mutated global state that outlives it.

get_greffon_info calls l4_ports.mark_pending, which records the reservation
in a MODULE-GLOBAL set. I stubbed published_l4_ports and pending_and_prune
for determinism but not mark_pending, so rendering visio and wireguard left
udp/20000 reserved for the rest of the pytest process. test_l4_network_
exposure.py then allocated 20001 while asserting 20000.

Reproduced before fixing: that file passes alone (25 passed) and fails when
run after the oracle. Since the previous commit wires the pinned catalog into
BOTH workflows, this would have turned the merge gate and the signed release
gate red -- the second time this branch's CI wiring would have broken CI.

Fix: stub mark_pending too. It records cross-call reservations, and each
snapshot render is an independent instance rendered against an empty host, so
stubbing it is the correct model rather than a convenience. Intra-call
dedup (the 'batch' dict) is untouched, so the allocation logic still runs.

Verified: oracle + L4 tests in one process now 55 passed, and the 29
snapshots are byte-identical, so the stub changes no rendered output.
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 2, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-04T10:10:32.586301Z f6ed739 Manual request
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 517d5bc77b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread apps/utils/docker/compose.py Outdated
Comment on lines 630 to 631
t = _COMPOSE_RENDER_ENV.from_string(yaml.dump(compose))
compose_file = t.render(**greffon_info)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Render only trusted template text

Render the catalog template before inserting configuration values, or otherwise prevent those values from being interpreted as Jinja. In the real start flow, apply_configuration first copies every env-destination value into compose (compose.py:668-674), and this line then compiles that user-controlled value together with the catalog YAML. The immutable sandbox prevents attribute escapes but provides no output or arithmetic limit, so a configuration such as {{ "x" * 1000000000 }} can allocate an enormous string and OOM the greffer process, affecting every instance on the node; it can also evaluate ordinary references against the entire greffon_info context rather than remaining literal configuration data.

Useful? React with 👍 / 👎.

Comment thread apps/utils/docker/compose.py Outdated
Comment on lines 630 to 631
t = _COMPOSE_RENDER_ENV.from_string(yaml.dump(compose))
compose_file = t.render(**greffon_info)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Translate sandbox rejections into a client error

Catch TemplateError/SecurityError from this new render boundary and convert it to the same structured render error used for baked files. When a catalog or future custom-app compose contains a forbidden expression, render() now raises SecurityError; however, start_greffon only translates ConfigRenderError around apply_configuration (controller.py:295-304), while the later create_compose call is covered by a generic handler that re-raises (controller.py:305-314). Thus the intended rejection is exposed as an opaque HTTP 500 rather than an actionable 4xx render failure.

Useful? React with 👍 / 👎.

omarsy added 3 commits October 4, 2026 11:02
Brings in #139 (bare env key) and #138
(oidc as a known type). Both touch apps/utils/docker/compose.py, in the
integration strip rather than the render path this branch changes, and
the merge is clean.
The sandbox made SecurityError a new way for create_compose to fail, and
it surfaced as a bare 500 whose body said only "internal_error". The
manager forwards whatever status and detail the greffer returns, so the
operator saw no cause and could not tell an unsafe or broken compose
from a transient greffer fault. The module docstring already promised
"a SecurityError surfaces as a clean ConfigRenderError (-> 422)", which
was true only of the baked-file path.

create_compose now reports a render failure as ConfigRenderError, the
same class the baked-file path uses, and the start router answers it
with 422 and the reason. That covers the sandbox's SecurityError and
the TemplateSyntaxError, UndefinedError and TypeError a broken template
raised before the sandbox existed. yaml.dump stays outside the mapping:
a dict that cannot be represented is greffer-side, and a 500 is the
honest answer for that. Nothing reaches a volume copy or `compose up`,
so a refused compose leaves no half-started instance.

Tests that exercised the old shape are tightened, not loosened. The
sandbox tests still require the CAUSE to be SecurityError, since a
payload that merely fails to parse also becomes a ConfigRenderError.

Three gaps a review measured are closed:

- Making _FILE_RENDER_ENV a plain SandboxedEnvironment passed the whole
  suite while letting `volumes.data.update(...)` set a volume to "/",
  which create_volumes_then_copy_files mounts as `-v /:/root`. Only the
  compose env's immutability was tested; the baked-file env now is too.
- The unbound-mutator tests asserted only that something raised, which
  cannot tell "refused" from "mutated, then crashed":
  `{% set _ = dict.update(volumes.data, {"value": "/"}) %}{{ 1 / 0 }}`
  passed against an unhardened env with the volume already rewritten.
  They now assert the live context comes out untouched.
- _harden's docstring said removing `range` limited memory blowups.
  It does not: `*` and `**` are not intercepted, and
  `{{ "A" * (10 ** 8) }}` renders a 100 MB compose. It now says the
  sandbox stops code execution, not resource use, and points at the
  render budget the compose-containment epic scopes to custom composes.
…the commit

Two ways the render oracle, the merge gate for this change, could report
green or red for the wrong reason.

A render failure became the snapshot. `_render` returns a marker such as
`<<RENDER FAILED SecurityError>>`, and the test compared or wrote it
like any other output, so a greffon that cannot deploy at all reported
green, and CATALOG_RENDER_UPDATE=1 blessed it silently. Reproduced with
an SSTI entry added to a catalog copy: regenerate, then 31 passed. A
failed render is now a failure unless _KNOWN_UNRENDERABLE names the
entry with a reason, and the check runs before a snapshot is read or
written. An allowlisted entry that starts rendering fails too, so the
list cannot go stale. Same reproduction now: 1 failed, nothing written.

The snapshots belong to one catalog commit, but nothing said which. CI
checks out 5652a755; a developer whose catalog is anywhere else got one
"render changed" failure per drifted entry, each pointing at the
regenerate command, the one fix that then breaks CI. CATALOG_SHA now
records the commit. When the catalog under test is elsewhere the entry
tests skip and the coverage test fails ONCE, naming both commits and how
to check out the right one. It fails rather than skips, so this cannot
turn into a fake green. A tripwire requires both workflows to pin the
commit CATALOG_SHA names.

The refusals are functions, each with a test that makes it fire. In CI
every entry renders and the commit matches, so otherwise nothing would
ever exercise the branches that exist to say no; the entry floor, for
one, had never been seen to fail.
@omarsy

omarsy commented Oct 4, 2026

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4c3d40f8cb

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread apps/utils/docker/compose.py Outdated
dumped = yaml.dump(compose)
try:
compose_file = _COMPOSE_RENDER_ENV.from_string(dumped).render(**greffon_info)
except (TemplateError, TypeError, ValueError) as exc:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Map every template evaluation failure to ConfigRenderError

The new catch still misses exceptions raised directly while evaluating valid Jinja expressions. For example, a catalog compose containing {{ 1 / 0 }} raises ZeroDivisionError, not TemplateError, TypeError, or ValueError; it therefore reaches the generic handler in start_greffon and remains an opaque HTTP 500 instead of the intended actionable 422. Catch other evaluation-time exceptions from render() while preserving the deliberate 500 behavior for yaml.dump outside this block.

Useful? React with 👍 / 👎.

…he detail

Round 2's 422 mapping named three exception classes
(TemplateError/TypeError/ValueError), and template evaluation raises more.
`{{ 1 // 0 }}` is ZeroDivisionError, `{{ 10.0 ** 400 }}` OverflowError,
`{{ "x".encode("no-such-codec") }}` LookupError, a recursive macro
RecursionError. None are in that set, so each escaped create_compose as a
bare 500 again, and a user-entered `{{ config.X }}` reaches them today. The
catch is now any exception from render(): nothing but template evaluation
runs inside the try, and the env has no custom filters or globals that could
raise a greffer bug, so it cannot mislabel a greffer fault. yaml.dump stays
outside, so a non-representable dict is still an honest 500.

The forwarded detail now names the exception TYPE only, not str(exc). The
manager logs and forwards whatever the greffer returns, and a render failure's
message can carry rendered bytes (a value an expression read, quoted back; an
unbounded length). The type tells the operator whether the compose is unsafe,
malformed, or references something unset; the full reason goes to the greffer
log, which is not caller-reachable.

Two tests close the gap that let this through: narrowing or broadening the
catch, and reverting the detail to str(exc), both passed the whole suite
before. One pins the exception SET at the create_compose level across two
hierarchies the old catch missed; the other pins that the detail carries the
type and not the message text, on a failure whose message is known and
distinct from its type name.
@omarsy

omarsy commented Oct 4, 2026

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Chef's kiss.

Reviewed commit: f6ed739b5d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

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.

1 participant