[OSIG] Replace Templates With Canvas Renderer#35
Conversation
|
@greptile review |
Greptile SummaryThis PR replaces the fixed-template image pipeline with a typed canvas renderer built on composable
Confidence Score: 5/5Safe to merge; the new canvas renderer is well-validated, SSRF mitigations are in place, the gradient serialization bug from the prior review is fixed, and the migrations are correct. The previous blocking issues (SSRF, gradient background crash, single-line height estimate) are all addressed in this revision. The two remaining findings are both cosmetic: a misleading return-type annotation with a dead None-guard, and align/valign fields on TextLayer being silently ignored when the corresponding bounding dimension is absent. Neither causes incorrect renders or data loss. No files require special attention. The _layer_bounds annotation and the silent align/valign behaviour are in agent_images/services.py but are non-blocking. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Caller
participant MCP as mcp.py
participant Services as services.py
participant Styles as image_styles.py
participant Safety as image_url_safety.py
Caller->>MCP: render_image_preview(spec: ImageSpec)
MCP->>Services: render_image(spec)
Services->>Services: normalize_image_spec(spec)
Services->>Services: _dimensions_for_spec(spec)
Services->>Services: _fill_dump(background)
Services->>Services: _layer_dump(layers[])
Services->>Services: _canvas_warnings(spec, w, h)
Services->>Styles: render_canvas_image(render_params)
Styles->>Styles: _fill_image(background)
loop each layer
alt "kind == rect"
Styles->>Styles: _draw_shadow / _draw_rect
else "kind == text"
Styles->>Styles: _draw_text
else "kind == image"
Styles->>Safety: "validate_remote_image_url(url, resolve=True)"
Styles->>Styles: _load_source_image
Styles->>Styles: _draw_image
end
end
Styles->>Styles: add_watermark (if no pro sub)
Styles-->>Services: image buffer
Services->>Services: _record_render_attempt_safely
Services-->>MCP: render result dict
MCP-->>Caller: "{spec, warnings, image_base64, ...}"
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Caller
participant MCP as mcp.py
participant Services as services.py
participant Styles as image_styles.py
participant Safety as image_url_safety.py
Caller->>MCP: render_image_preview(spec: ImageSpec)
MCP->>Services: render_image(spec)
Services->>Services: normalize_image_spec(spec)
Services->>Services: _dimensions_for_spec(spec)
Services->>Services: _fill_dump(background)
Services->>Services: _layer_dump(layers[])
Services->>Services: _canvas_warnings(spec, w, h)
Services->>Styles: render_canvas_image(render_params)
Styles->>Styles: _fill_image(background)
loop each layer
alt "kind == rect"
Styles->>Styles: _draw_shadow / _draw_rect
else "kind == text"
Styles->>Styles: _draw_text
else "kind == image"
Styles->>Safety: "validate_remote_image_url(url, resolve=True)"
Styles->>Styles: _load_source_image
Styles->>Styles: _draw_image
end
end
Styles->>Styles: add_watermark (if no pro sub)
Styles-->>Services: image buffer
Services->>Services: _record_render_attempt_safely
Services-->>MCP: render result dict
MCP-->>Caller: "{spec, warnings, image_base64, ...}"
Reviews (8): Last reviewed commit: "fix(agent-images): bound canvas layer re..." | Re-trigger Greptile |
Greptile SummaryThis PR replaces the fixed template contract (
Confidence Score: 4/5Safe to merge with two non-blocking concerns: an incomplete canvas-overflow warning for multi-line text, and an unvalidated URL field on ImageLayer that could be used to probe internal services from the unauthenticated trial endpoint. The canvas renderer, discriminated-union layer models, and retry/observability pipeline are all well-structured and covered by tests. The migration is minimal and correct. The two flagged issues are non-blocking: the clipping warning gap won't break rendering, and the SSRF risk is a hardening item that also existed under the old image_url field. No data-loss, crash, or auth paths are affected by this change. agent_images/services.py — the _layer_bounds height approximation and ImageLayer.url field both warrant a second look before this endpoint is gated behind real auth.
|
| Filename | Overview |
|---|---|
| agent_images/services.py | Core rewrite — replaces the template-based ImageSpec with typed canvas layer models (RectLayer, TextLayer, ImageLayer), adds dimension/bounds validation, and re-implements image_contract(). Two issues: TextLayer height underestimation in _layer_bounds leads to silent canvas-overflow, and ImageLayer.url has no http/https-only enforcement (SSRF). |
| core/image_styles.py | New canvas renderer — adds _draw_rect, _draw_text, _draw_image, and render_canvas_image using Pillow. Opacity, clipping, image resizing (cover/contain/stretch), and text wrapping look correct. Old template generator code removed. |
| agent_images/mcp.py | Removes list_image_templates tool, updates docstrings to canvas terminology, and imports are cleaned up. Logic is unchanged from the pre-PR version. |
| core/migrations/0012_rename_renderattempt_style_renderer.py | Simple RenameField migration; dependencies chain correctly to 0011_renderattempt. |
| core/tasks.py | Single-line change: swaps generate_image_router for render_canvas_image. Pre-existing setattr loop in regenerate_and_update_image will silently skip canvas spec keys that don't map to Image model fields, but this is unchanged behavior. |
| core/tests/test_generate_image_features.py | Replaced template-style test fixtures with canvas spec helpers; covers custom dimensions, Google Font provider, color validation, remote image layers, and JPEG determinism. |
| core/tests/test_render_reliability.py | Tests updated to use canvas spec; retry logic, non-transient error paths, and metrics dashboard tests all preserved and passing. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Agent
participant MCP as MCP Tool
participant Services as agent_images/services.py
participant Styles as core/image_styles.py
participant Observability as core/render_observability.py
Agent->>MCP: render_image_preview(spec)
MCP->>Services: render_image(spec, profile)
Services->>Services: normalize_image_spec(spec)
note over Services: validate layers, check canvas bounds, emit warnings
Services->>Services: track_profile_usage(profile)
loop Up to OSIG_RENDER_MAX_ATTEMPTS
Services->>Styles: render_canvas_image(render_params)
Styles->>Styles: draw rect / text / image layers
alt render fails (transient)
Styles-->>Services: Exception
Services->>Observability: "record_render_attempt(success=False)"
Services->>Services: retry if transient
else render succeeds
Styles-->>Services: BytesIO buffer
Services->>Observability: "record_render_attempt(success=True)"
Services-->>MCP: "{spec, image_base64, metadata}"
end
end
MCP-->>Agent: result dict
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Agent
participant MCP as MCP Tool
participant Services as agent_images/services.py
participant Styles as core/image_styles.py
participant Observability as core/render_observability.py
Agent->>MCP: render_image_preview(spec)
MCP->>Services: render_image(spec, profile)
Services->>Services: normalize_image_spec(spec)
note over Services: validate layers, check canvas bounds, emit warnings
Services->>Services: track_profile_usage(profile)
loop Up to OSIG_RENDER_MAX_ATTEMPTS
Services->>Styles: render_canvas_image(render_params)
Styles->>Styles: draw rect / text / image layers
alt render fails (transient)
Styles-->>Services: Exception
Services->>Observability: "record_render_attempt(success=False)"
Services->>Services: retry if transient
else render succeeds
Styles-->>Services: BytesIO buffer
Services->>Observability: "record_render_attempt(success=True)"
Services-->>MCP: "{spec, image_base64, metadata}"
end
end
MCP-->>Agent: result dict
Reviews (2): Last reviewed commit: "feat: replace templates with canvas rend..." | Re-trigger Greptile
|
@greptile review |
|
@greptile review\n\nPlease review the latest head commit 3d285ca after the SSRF and text-warning fixes. |
|
@greptile review |
|
@greptile review |
|
@greptile review\n\nPlease review the latest head commit 52c43d9. All currently visible Greptile review threads are resolved. |
|
@greptile-apps review |
Summary
Tests
Notes