Repository navigation
feat(custom-app): inline compose transport on start (Feature 4 — LLD §4.2) - #147
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: ab7eac889e
ℹ️ 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".
| inline = greffon.get('compose_yaml') | ||
| if inline: | ||
| try: | ||
| return yaml.safe_load(inline) |
There was a problem hiding this comment.
For a newly supported inline compose where two services reference the same YAML-anchored service mapping, yaml.safe_load() returns the same dictionary object for both services. The later normalization and create_compose_template_from_greffon() pipeline mutates service definitions in place; when it clears and rebuilds ports, volumes, and networks, both services therefore share the rebuilt lists and receive duplicate or cross-service entries. Such an input passes preflight but /start/ can emit a compose different from the validated source and let the asynchronous compose child fail after a successful response. Reject shared service mappings or clone each service definition independently before passing the parsed inline compose into the mutating pipeline.
Useful? React with 👍 / 👎.
…LD §4.2) GreffonStartRequest grows compose_yaml (str | None, bounded to the same 1 MiB order as the preflight request): when present, the pinned compose bytes travel in the start request and the URL fetch is skipped — a custom version has no URL to fetch. repository_url becomes conditionally required (exactly one compose source per start: neither is the same 400 the old required field produced; both is refused rather than silently preferring one). The containment Feature 3 trust discriminator (_compose_strict) is live on this path: inline bytes are unreviewed input and get the strict allowlist set. Version 0.13.0 — the release the manager's min_greffer_version floor must name (0.9.x-0.12.x pass a 0.9.0 floor but extra=ignore the bytes). Adversarial-review round folded: - P0 alias-DAG: a sub-KiB anchor DAG under an allowed key (e.g. environment) passes every shape check AND returned a green preflight (its own walk is id-deduped), but /start/'s render accounting walks the tree in the parent process with no dedup — 2^25 visits measured at ~71s, ~2^35 possible, pinning the single worker. Fix at the boundary: compose_shape.parse_compose_yaml (SafeLoader subclass refusing AliasEvent — anchor definitions without references stay legal) used by BOTH tenant parse sites (start inline branch and preflight). The allowlist permits no x- extension fields, so the anchor-and-merge pattern was never deployable; refusing the reference names it at preflight instead of a green light that pins a node. The preflight's dedup walker keeps its direct unit test as defense in depth. - P2 RecursionError (deeply nested inline YAML) is not a YAMLError — escaped both handlers and 500'd; now mapped to the same clean 422 at both sites. - P3 InlineComposeError bound at import time in the controller (the file's own convention — an except on a mocked module attribute TypeErrors mid-handler); the both/neither comment says 400 (the request-validation handler), not 422. Suite: 1112 passed; the only failure is the pre-existing-on-main oracle/catalog-pin environment artifact.
ab7eac8 to
93dd48b
Compare
Custom-app-deploy foundation Feature 4, transport slice (LLD §4.2; epic row: "Greffer consumes inline compose" — preflight + handshake halves already merged as #146).
What
GreffonStartRequest.compose_yaml: str | None— additive; bounded to the same 1 MiB order as the preflight request (an unbounded body is tenant-triggerable memory before any render budget runs).get_compose_file_from_repositoryskips the URL fetch and parses the inline bytes.repository_urlbecomes conditionally required: exactly one compose source per start (_require_compose_sourcemodel validator — neither → the same 422 the old required field produced; both → refused rather than silently preferring one). Themin_length=1floor stays for the catalog path._compose_strict) — wired in feat(containment): allowlist compose keys; namespace all mounts; stage baked files #142 with "activates with the inline transport" — is now live: inline bytes get the strict allowlist set.InlineComposeError→ clean 422 before any port allocation or volume write. Those bytes passed the identical parse in/preflight/, so divergence means the manager sent something else — name the transport, never a 500.min_greffer_versionon custom versions and the gate must name a release that actually understands the field (0.9.x–0.12.x would pass a 0.9.0 floor butextra=ignorethe bytes).Wire compatibility
compose_yaml→None→ the URL path, byte-for-byte today's behavior.compose_yaml, 422s on the missingrepository_url— prevented by the version gate, which is the HLD's stated answer ("the old-node problem is handled bymin_greffer_version").Tests
Model (conditional source, empty/oversized inline, empty URL floor), repository (inline skips the fetch, invalid YAML raises, absent inline still fetches), routes (inline start 200 with
strict=Trueasserted through toget_greffon_info, invalid inline 422 with nothing started).Suite: 1106 passed; the only failure is the pre-existing-on-main oracle/catalog-pin environment artifact (local catalog checkout differs from the pinned 5652a75). The load-fragile workers_register test deselected locally per usual.
Follow-up slices under the same Feature 4 row: all-tier disk admission (HLD §9) and the one-shot-status mechanism (Story 5 — the HLD names it an open design gap).