Repository navigation
feat(containment): structure-preserving render + deploy-time preflight (Feature 6) - #146
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8d07ceb10a
ℹ️ 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".
| if max_in and len(tree.encode("utf-8", "surrogatepass")) > max_in: | ||
| raise RenderBudgetExceeded("input_too_large", max_in) | ||
| return _child_render("file", tree, context) | ||
| return _child_render(which, tree, context) |
There was a problem hiding this comment.
Rendering each leaf through _child_render resets GREFFER_RENDER_MAX_OUTPUT for every value, so a custom compose with many environment or label values can produce nearly 4 MiB per leaf while the complete result grows far beyond the documented 4 MiB body limit. Because the rendered tree is retained before serialization, this reopens the memory-exhaustion path that the render budget is meant to contain; track cumulative output across the entire tree job rather than independently per leaf.
Useful? React with 👍 / 👎.
| image = svc.get("image") | ||
| if not isinstance(image, str) or not image: | ||
| # Services without an image: refused at start (the allowlist | ||
| # requires image); no digest to resolve. | ||
| continue |
There was a problem hiding this comment.
When a service omits image or supplies a non-string value, this path silently skips it and the endpoint still returns 200. normalize_compose only allowlists the key and does not require it, while build is forbidden by the strict allowlist, so such a service has neither an image nor a build context and will be rejected by Compose at deployment time despite a green preflight.
Useful? React with 👍 / 👎.
| if max_in and len(tree.encode("utf-8", "surrogatepass")) > max_in: | ||
| raise RenderBudgetExceeded("input_too_large", max_in) | ||
| return _child_render("file", tree, context) | ||
| return _child_render(which, tree, context) |
There was a problem hiding this comment.
For any YAML scalar ending in a newline, passing that scalar alone through the body Jinja environment removes its terminal newline because _COMPOSE_RENDER_ENV uses Jinja's default keep_trailing_newline=False. Previously only the final newline of the serialized YAML document was subject to that behavior, so embedded scalar newlines survived; the regenerated Visio command and WordPress WORDPRESS_CONFIG_EXTRA snapshots already demonstrate these value changes, and newline-sensitive commands or configuration content can break.
Useful? React with 👍 / 👎.
| repo_digests = [ | ||
| d for d in (info.attrs.get("RepoDigests") or []) | ||
| if "@" in d | ||
| ] | ||
| if not repo_digests: | ||
| raise RuntimeError("image has no repo digest") | ||
| digests[name] = repo_digests[0] |
There was a problem hiding this comment.
When the inspected image has multiple RepoDigests because the same local image is tagged or pulled from multiple repositories, taking index zero can return a digest-qualified reference for a repository other than the requested image. The resulting pin may be inaccessible or fail on a later clean deployment even though the requested repository resolved successfully; filter the entries to the repository portion of the requested reference before selecting the digest.
Useful? React with 👍 / 👎.
8d07ceb to
02fe5e1
Compare
compose-containment Feature 6 — the last feature of the epic. Two
halves:
ROUND-TRIP GUARD. The compose body no longer renders through
Template(yaml.dump(compose)) — whose single-quote escaping (an embedded
' doubled to '') corrupted any Jinja expression containing a
single-quoted literal into a TemplateSyntaxError, with the catalog
working around it by double-quote convention. Every string LEAF now
renders in ONE budgeted child job (the same tree walker the json
destinations use, generalized to select the body env) and the tree is
yaml.dump'd exactly once afterwards — the fragility class is killed
structurally: values render as Python strings, then yaml serializes
with correct quoting applied once. Keys never render (same intentional
choice as the json walker). Regression test pins the exact
formerly-broken shape (a single-quoted Jinja literal).
THE ORACLE GATE, honored: all 29 catalog snapshots regenerated with
CATALOG_RENDER_UPDATE=1 and every moved byte reviewed — an executable
semantic-equality check (yaml.safe_load old == new) proves all 29
diffs are quoting/whitespace/EOF-newline only, zero value changes. The
first regeneration was produced by a buggy submit that hardwired the
StrictUndefined FILE env for the body tree (the consolidation left
pool.submit(_child_render_json...) in _run_tree_job) — caught by the
oracle itself via 4 entry diffs, fixed by submitting _child_render_tree
with the which parameter, then re-generated and re-verified.
PREFLIGHT (custom-app LLD 4.1 wire; validator internals per the
containment HLD): POST /api/controller/preflight/ {compose_yaml} ->
200 {services, ports, config_candidates, image_digests, warnings} or
422 {errors:[{service,key,message}]} surfacing the shape pass verbatim.
The SAME parse + strict-allowlist path as a real start (a green
preflight provably implies a start that parses), the operator key
grant threaded through; image references pinned by pull+inspect (the
one deliberate node-side write; unresolvable = error naming the image,
never a deploys-unpinned fallback); config candidates from environment
keys plus {{ config.X }} references (referenced ones pre-ticked); the
unset-integration STRIP WARNING distinguishes genuine {{
smtp.host }} references (the strip is correct behaviour) from literal
text like an innocent smtp.gmail.com inside a templated MAIL_URL (the
false-positive class) by matching against the value with all {{
... }} expressions removed — covering every KNOWN_INTEGRATION_TYPES
entry (smtp and oidc), per the containment HLD's requirement.
10 preflight tests (payload, verbatim shape errors, digest failures,
grant threading, the warning discrimination, route 200/422/token).
Suite at the pinned catalog: 1072 passed, 0 failed — the full oracle
green through the new render path.
Adversarial review round (DO NOT SHIP, 2 P1s) — folded:
P1-1 the body env lacked keep_trailing_newline, so per-leaf rendering
silently truncated every multi-line value by exactly one trailing
newline (the old whole-document pipeline only ever lost the document-
final one). Fixed; the 4 affected snapshots re-regenerated and verified
to differ from origin/main's by EXACTLY the restored newline (docs/
plausible/visio/wordpress: command/entrypoint/WORDPRESS_CONFIG_EXTRA
leaves) — i.e. main's committed snapshots carried the truncation, and
the new bytes restore the catalog's ground truth. Regression test pins
the leaf-level semantics.
P1-2 preflight CPU DoS: compose_yaml had no max_length (the only
unbounded string in the models — everything else caps 64-512) and the
expression-strip regex backtracked quadratically on brace-padded values
(256KB body -> 210s, pinning a threadpool token; tenant-triggerable
through the manager proxy). Bounded at 1 MiB (GREFFER_RENDER_MAX_INPUT
order) and replaced the regex with a linear single-pass span remover
(same semantics for well-formed templates; unterminated {{ conservatively
stays literal).
Also: keys now COUNT toward the tree input cap (a 3 MiB key previously
passed a 1 MiB cap as one counted byte — keys never render, but their
bulk must be bounded; regression test); bracket-form {{ config['X'] }}
references detected (dot-only let them read as unreferenced — the
silent-empty class; regression test); container_port str()'d for a
stable wire type; the stale 'yaml.dump stays OUTSIDE the try' comment
reconciled with the code.
Suite at the pinned catalog: 1075 passed, 0 failed.
Codex CLI review round (2 P1s + 6 P2s) — folded:
P1 job-wide output cap: per-leaf counters let N leaves each render
just under GREFFER_RENDER_MAX_OUTPUT, so the assembled document
exceeded it unbounded (the serialized output also grew in the
controller, outside the child's memory guard). The tree job now
carries ONE budget accumulator across all leaves.
P1 alias-bomb: YAML aliases let a sub-KiB compose expand to
exponentially many visits in the preflight leaf walk (outside every
render guard; recursive aliases recurse forever). The walk is now
alias-safe (shared containers visited once — which alone neuters the
doubling chain, asserted by a timing test) and budget-bounded (20k
visits; a huge non-aliased structure trips a named refusal). Both
regression tests.
P2s: an imageless service is now a structured refusal (the allowlist
permits the key but never requires it, and strict mode refuses build —
it could never deploy; it previously returned success with an empty
digest map); config references detected ANYWHERE inside expressions
({{ 'p' ~ config.SECRET }} missed by the opening-token-only match) and
the bracket-form quote DELIMITER no longer leaks a spurious quote
candidate (groups filtered); ReaderError (no .problem attribute) is a
named 422 instead of an AttributeError 500; the candidate name-set is
built once (was quadratic per referenced key); the body wiring test
re-pinned to the tree-job aggregate semantics (output cap). CR/LF
preservation in scalar leaves NOT folded: yaml.safe_load itself
normalizes line endings before any greffer code runs, so the old
pipeline lost them identically — not a regression of this change (left
as a note for a future newlines-are-load-bearing audit).
Suite at the pinned catalog: 1081 passed, 0 failed.
02fe5e1 to
e985062
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e985062e10
ℹ️ 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".
| src_len = sum( | ||
| len(leaf.encode("utf-8", "surrogatepass")) | ||
| for leaf in _iter_string_leaves(tree)) | ||
| for leaf in _iter_string_leaves(tree, keys=True)) |
There was a problem hiding this comment.
When a compose contains a short chain of YAML aliases where each container references the preceding container twice, yaml.safe_load preserves that shared graph but this generator revisits every reference. Consequently sum() can perform roughly 2^N work in the parent before _submit_budgeted starts, so neither the render timeout nor the child memory limit applies; the alias-safe preflight walker can approve the same input while deployment ties up a request worker. Track visited container identities and impose a visit budget during this accounting pass.
Useful? React with 👍 / 👎.
| elif isinstance(env, list): | ||
| for item in env: | ||
| if isinstance(item, str) and "=" in item: | ||
| name, value = item.split("=", 1) | ||
| yield name.strip(), value |
There was a problem hiding this comment.
For a valid Compose environment list such as environment: [DEBUG, FOO=bar], this branch silently drops DEBUG because it has no =. Mapping-form DEBUG: null is included even though it has the same host-import semantics, and the existing deployment code explicitly supports bare list entries, so the preflight response inconsistently omits an environment key that the manager needs to present as a configuration candidate.
Useful? React with 👍 / 👎.
| _CONFIG_REF = re.compile( | ||
| r"config(?:\.([A-Za-z0-9_]+)|\[(['\"])([A-Za-z0-9_]+)\2)") |
There was a problem hiding this comment.
Because this regex is applied directly to every string leaf without first restricting the search to Jinja expressions, ordinary literal content such as https://config.example/path or myconfig.SECRET creates a referenced example or SECRET candidate. Those phantom candidates are returned as pre-ticked references and can make a valid custom compose appear to require configuration it never renders; extract matches only from actual template blocks and require an identifier boundary before config.
Useful? React with 👍 / 👎.
1 participant
compose-containment Feature 6 — the last feature of the epic. Two halves:
Round-trip guard. The compose body no longer renders through
Template(yaml.dump(compose))— whose single-quote escaping (an embedded'doubled to'') corrupted any Jinja expression containing a single-quoted literal into aTemplateSyntaxError, with the catalog working around it by double-quote convention. Every string leaf now renders in one budgeted child job (the json-destination tree walker, generalized to select the body env) and the tree isyaml.dump'd exactly once — the fragility class is killed structurally. Keys never render. Regression test pins the exact formerly-broken shape.The oracle gate, honored: all 29 catalog snapshots regenerated per protocol and every moved byte reviewed — an executable semantic-equality check (
yaml.safe_load(old) == yaml.safe_load(new)) proves all diffs are quoting/whitespace/EOF only, zero value changes. Notably, the oracle caught a bug in my first pass: the consolidation left the tree submit hardwired to the StrictUndefined file env, changing lenient-body semantics — 4 entries diffed, the submit was fixed, snapshots regenerated and re-verified.Preflight (custom-app LLD §4.1 wire; validator internals per the containment HLD):
POST /api/controller/preflight/→200 {services, ports, config_candidates, image_digests, warnings}/422 {errors:[{service,key,message}]}surfacing the shape pass verbatim. Same parse + strict-allowlist path as a real start; operator key grant threaded; image references pinned by pull+inspect (the one node-side write; unresolvable = error naming the image, never an unpinned fallback); config candidates from env keys +{{ config.X }}references (referenced pre-ticked); the unset-integration strip warning distinguishes genuine{{ smtp.host }}references (strip = correct behavior) from literalsmtp.gmail.comtext in a templated MAIL_URL (the false-positive class) by matching with all{{ ... }}expressions removed — covering everyKNOWN_INTEGRATION_TYPESentry (smtp and oidc).10 preflight tests + the guard regression. Suite at the pinned catalog: 1072 passed, 0 failed — the full 29-entry oracle green through the new render path.