Rectify: format_ingredients_table GFM Width Cap#490
Conversation
…m_table Adds _render_gfm_table to core/_terminal_table.py (L0 primitive) sharing the same TerminalColumn spec and width-cap contract as _render_terminal_table. Refactors format_ingredients_table to delegate to _render_gfm_table via _GFM_INGREDIENT_COLUMNS (max_width=30/60/20), eliminating the ad-hoc uncapped max(len(...)) width computation that allowed 220-char run_mode descriptions to produce 220-wide GFM columns (Issue #489). Adds 5 behavioural tests mirroring the terminal-path tests in test_ansi.py and 3 architectural guard tests that fail immediately if delegation is reverted.
Trecek
left a comment
There was a problem hiding this comment.
AutoSkillit PR Review — Verdict: changes_requested
| data_width = min(data_width, col.max_width) | ||
| col_widths.append(data_width) | ||
|
|
||
| def _cell(value: str, width: int, align: str) -> str: |
There was a problem hiding this comment.
[warning] arch: _render_gfm_table defines a local _cell() helper that duplicates the logic of the module-level _cell() defined for _render_terminal_table. Consider extracting a shared private helper to keep truncation/padding logic in one place.
There was a problem hiding this comment.
Valid observation — flagged for design decision. Extracting a shared module-level _cell() helper would eliminate duplication between _render_terminal_table and _render_gfm_table. The current design keeps each function self-contained, allowing them to diverge if GFM ever needs different truncation behaviour (e.g., different ellipsis character or byte-aware truncation). This is a DRY vs independence trade-off that warrants a human decision.
|
|
||
| def _cell(value: str, width: int, align: str) -> str: | ||
| if len(value) > width: | ||
| value = value[: width - 1] + "…" |
There was a problem hiding this comment.
[warning] bugs: After truncation, value[:width-1] + '…' produces a string of the correct Python character count (width), but '…' (U+2026) is 3 bytes in UTF-8. If any downstream consumer measures byte length rather than character count, cells will appear wider than intended. Accepted trade-off in a GFM/HTML context, but worth confirming this is intentional.
There was a problem hiding this comment.
Investigated — this is intentional. The '…' (U+2026) truncation produces width Python characters (not bytes), which is the correct contract for GFM/HTML display contexts where renderers measure Unicode code points. Python's format() alignment is also character-count based, making the column alignment self-consistent. No downstream consumer in this codebase measures byte length. The reviewer acknowledged this as 'accepted trade-off in a GFM/HTML context'.
| """ | ||
| from autoskillit.recipe._api import format_ingredients_table | ||
|
|
||
| src = inspect.getsource(format_ingredients_table) |
There was a problem hiding this comment.
[warning] tests: inspect.getsource(format_ingredients_table) returns raw source including comments and docstrings. The assertion assert "max(len(" not in src would false-positive if a future docstring or comment inside the function mentions 'max(len(' as an example of prohibited code (e.g., to explain what not to do). Consider a more targeted structural check.
There was a problem hiding this comment.
Investigated — this is intentional. The false-positive scenario requires a developer to add a docstring or comment inside format_ingredients_table mentioning max(len( as an example of prohibited code — an extremely unlikely authoring pattern. More critically, the first assertion ("_render_gfm_table" in src) already provides the primary structural guard; the second is supplementary defence. The inspect.getsource() pattern is consistent with other structural regression guards in tests/arch/. AST rewriting would add disproportionate complexity for a purely theoretical risk.
Trecek
left a comment
There was a problem hiding this comment.
AutoSkillit review found 9 blocking issues (1 critical, 8 warning). See inline comments.
Summary
format_ingredients_table(the GFM/MCP rendering path for recipe ingredients) computescolumn widths via raw
max(len(...))with floors but no ceilings. The result: a 220-charrun_modedescription inimplementation.yamlforces the GFM table to 220+ column-widerows, bloating every MCP response that loads that recipe.
The immunity solution: extend
core/_terminal_table.pywith_render_gfm_table, acceptingthe same
TerminalColumnspecs already used by the terminal path. Both rendering paths nowshare the same L0 primitive and the same column-spec source of truth. Width capping becomes
structurally implicit — any new GFM renderer must declare
TerminalColumnspecs andautomatically inherits the cap.
Architecture Impact
Module Dependency Diagram
%%{init: {'flowchart': {'nodeSpacing': 50, 'rankSpacing': 70, 'curve': 'basis'}}}%% graph TB %% CLASS DEFINITIONS %% classDef cli fill:#1a237e,stroke:#7986cb,stroke-width:2px,color:#fff; classDef stateNode fill:#004d40,stroke:#4db6ac,stroke-width:2px,color:#fff; classDef handler fill:#e65100,stroke:#ffb74d,stroke-width:2px,color:#fff; classDef phase fill:#6a1b9a,stroke:#ba68c8,stroke-width:2px,color:#fff; classDef newComponent fill:#2e7d32,stroke:#81c784,stroke-width:2px,color:#fff; classDef output fill:#00695c,stroke:#4db6ac,stroke-width:2px,color:#fff; classDef detector fill:#b71c1c,stroke:#ef5350,stroke-width:2px,color:#fff; classDef gap fill:#ff6f00,stroke:#ffa726,stroke-width:2px,color:#000; classDef integration fill:#c62828,stroke:#ef9a9a,stroke-width:2px,color:#fff; classDef terminal fill:#1a237e,stroke:#7986cb,stroke-width:2px,color:#fff; subgraph L0 ["L0 — core/ (stdlib only, zero autoskillit imports)"] direction LR TT["● core/_terminal_table.py<br/>━━━━━━━━━━<br/>TerminalColumn (NamedTuple)<br/>_render_terminal_table()<br/>★ _render_gfm_table() NEW"] INIT["● core/__init__.py<br/>━━━━━━━━━━<br/>Re-exports TerminalColumn<br/>Re-exports _render_terminal_table<br/>★ Re-exports _render_gfm_table NEW"] end subgraph L1 ["L1 — pipeline/"] direction LR TFM["pipeline/telemetry_fmt.py<br/>━━━━━━━━━━<br/>TelemetryFormatter<br/>imports: TerminalColumn<br/>imports: _render_terminal_table"] end subgraph L2 ["L2 — recipe/"] direction LR API["● recipe/_api.py<br/>━━━━━━━━━━<br/>format_ingredients_table()<br/>_GFM_INGREDIENT_COLUMNS<br/>imports: TerminalColumn<br/>★ imports: _render_gfm_table NEW"] end subgraph L3 ["L3 — cli/"] direction LR ANSI["cli/_ansi.py<br/>━━━━━━━━━━<br/>ingredients_to_terminal()<br/>imports: TerminalColumn only<br/>(inline _render_terminal_table)"] SHIM["cli/_terminal_table.py<br/>━━━━━━━━━━<br/>Re-export shim<br/>TerminalColumn<br/>_render_terminal_table"] end subgraph TESTS ["Tests"] direction LR GUARD["★ tests/arch/test_gfm_rendering_guard.py<br/>━━━━━━━━━━<br/>Arch guard: asserts delegation<br/>and bounded column specs"] TAPI["● tests/recipe/test_api.py<br/>━━━━━━━━━━<br/>GFM width cap behavioral tests<br/>Integration test vs real recipe"] end TT -->|"defines"| INIT TFM -->|"imports TerminalColumn<br/>_render_terminal_table"| INIT API -->|"imports TerminalColumn<br/>★ + _render_gfm_table"| INIT ANSI -->|"imports TerminalColumn"| INIT SHIM -->|"imports TerminalColumn<br/>_render_terminal_table"| INIT GUARD -->|"asserts delegation<br/>+ bounded max_width"| API GUARD -->|"verifies export surface"| INIT TAPI -->|"behavioral tests<br/>width cap + truncation"| API class TT,INIT stateNode; class TFM handler; class API phase; class ANSI,SHIM cli; class GUARD newComponent; class TAPI output;Color Legend:
core/_terminal_table.pyandcore/__init__.py— high fan-in primitivespipeline/telemetry_fmt.py— unchanged consumerrecipe/_api.py— key change: now imports_render_gfm_tablecli/_ansi.pyand re-export shim — unchanged consumerstest_gfm_rendering_guard.py— new arch guardtest_api.py— new behavioral tests addedProcess Flow Diagram
%%{init: {'flowchart': {'nodeSpacing': 40, 'rankSpacing': 55, 'curve': 'basis'}}}%% flowchart TB %% CLASS DEFINITIONS %% classDef terminal fill:#1a237e,stroke:#7986cb,stroke-width:2px,color:#fff; classDef stateNode fill:#004d40,stroke:#4db6ac,stroke-width:2px,color:#fff; classDef handler fill:#e65100,stroke:#ffb74d,stroke-width:2px,color:#fff; classDef phase fill:#6a1b9a,stroke:#ba68c8,stroke-width:2px,color:#fff; classDef newComponent fill:#2e7d32,stroke:#81c784,stroke-width:2px,color:#fff; classDef detector fill:#b71c1c,stroke:#ef5350,stroke-width:2px,color:#fff; classDef gap fill:#ff6f00,stroke:#ffa726,stroke-width:2px,color:#000; START([Caller: load_and_validate<br/>or MCP tools]) subgraph FIT ["● recipe/_api.py — format_ingredients_table()"] direction TB G1{"ingredients<br/>non-empty?"} BIR["build_ingredient_rows()<br/>━━━━━━━━━━<br/>Full-length tuples<br/>(name, desc, default)<br/>No truncation at data layer"] G2{"rows<br/>non-empty?"} DELEGATE["● delegate to _render_gfm_table()<br/>━━━━━━━━━━<br/>_GFM_INGREDIENT_COLUMNS spec:<br/>Name: max_width=30<br/>Description: max_width=60<br/>Default: max_width=20"] end subgraph RGT ["● core/_terminal_table.py — _render_gfm_table()"] direction TB WIDTHS["● Compute column widths<br/>━━━━━━━━━━<br/>col_w = min(<br/> max(cell_lengths, label_width),<br/> max_width ← BOUNDED"] HEADER["Render header row<br/>━━━━━━━━━━<br/>| Name | Description | Default |"] SEP["Render separator row<br/>━━━━━━━━━━<br/>| ---: | :--- | ---: |<br/>(alignment from TerminalColumn.align)"] ROWLOOP{"For each<br/>data row"} TRUNC{"cell length<br/>> col_w?"} PAD["Pad cell to col_w<br/>━━━━━━━━━━<br/>f'{cell:<{col_w}}'<br/>or right-aligned"] ELLIPSIS["Truncate + append '…'<br/>━━━━━━━━━━<br/>cell[:col_w-1] + '…'"] EMIT["Emit row:<br/>| cell | cell | cell |"] JOIN["Join all rows with newline<br/>━━━━━━━━━━<br/>Return GFM table string"] end ELIMINATED["✗ ELIMINATED: inline ad-hoc width math<br/>━━━━━━━━━━<br/>dw = max(len(r[1])...) — no ceiling<br/>f'| {desc:<{dw}} |' — uncapped padding<br/>220-char description → 220-wide column"] NONE([Return None]) RESULT([Return GFM table string<br/>All rows ≤ 120 chars wide]) START --> G1 G1 -->|"empty"| NONE G1 -->|"non-empty"| BIR BIR --> G2 G2 -->|"empty"| NONE G2 -->|"non-empty"| DELEGATE DELEGATE --> WIDTHS WIDTHS --> HEADER HEADER --> SEP SEP --> ROWLOOP ROWLOOP -->|"next row"| TRUNC TRUNC -->|"yes"| ELLIPSIS TRUNC -->|"no"| PAD ELLIPSIS --> EMIT PAD --> EMIT EMIT -->|"more rows"| ROWLOOP EMIT -->|"done"| JOIN JOIN --> RESULT ELIMINATED -.->|"replaced by DELEGATE"| DELEGATE class START terminal; class NONE terminal; class RESULT terminal; class G1,G2 stateNode; class BIR handler; class DELEGATE phase; class WIDTHS,HEADER,SEP newComponent; class ROWLOOP stateNode; class TRUNC stateNode; class PAD newComponent; class ELLIPSIS newComponent; class EMIT newComponent; class JOIN newComponent; class ELIMINATED gap;Color Legend:
build_ingredient_rows— unchanged data producer_render_gfm_tablevia column specImplementation Plan
Plan file:
/home/talon/projects/autoskillit-runs/remediation-20260322-173128-741993/.autoskillit/temp/rectify/rectify_format_ingredients_table_gfm_width_cap_2026-03-22_000000.md🤖 Generated with Claude Code via AutoSkillit